-
Notifications
You must be signed in to change notification settings - Fork 385
feat: Modular crypto #1292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: Modular crypto #1292
Conversation
Added support for rustls, graviola and OpenSSL TLS backends This makes it easier to choose different TLS implementations depending on deployment needs.
|
|
||
| | Feature | Description | | ||
| | -------------------- | --------------------------------------------- | | ||
| | `tls-ring` (default) | rustls with ring crypto | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the default provider for crypto is a Pure-Rust implementation that excludes ring, tls defaults to ring. What about the idea of defaulting to a Pure-Rust implementation that supports the major cipher suites?
https://github.com/RustCrypto/rustls-rustcrypto
EDIT: Until the official support is released, we will create an experimental branch that supports the new rc crates.
https://github.com/nabetti1720/rustls-rustcrypto/tree/next
% make check
cargo clippy --all-targets --all-features -- -D warnings
Checking rustls-rustcrypto v0.0.2-alpha (/Users/shinya/Workspaces/rustls-rustcrypto)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.15s
% make build
cargo +nightly build -r --target aarch64-apple-darwin
Compiling rustls-rustcrypto v0.0.2-alpha (/Users/shinya/Workspaces/rustls-rustcrypto)
Finished `release` profile [optimized] target(s) in 3.88s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nabetti1720 I think the reason is that this is pretty much unmaintained:
https://github.com/RustCrypto/rustls-rustcrypto. Last commit was 2 years ago
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richarddavison Yes, so I am submitting a PR to update to the latest rustcrypto.
RustCrypto/rustls-rustcrypto#102
.github/workflows/build-modules.yml
Outdated
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 24 days ago
To fix the issue, explicitly define minimal permissions for the workflow so the GITHUB_TOKEN is limited to what the job actually needs. In this case, the steps only need to read repository contents (for actions/checkout) and then run local build/test commands, so contents: read is sufficient.
The best way to do this without changing existing functionality is to add a root-level permissions block near the top of .github/workflows/build-modules.yml, applying to all jobs in the workflow. Concretely, insert:
permissions:
contents: readbetween the name: line and the on: section. No other changes (jobs, steps, or additional permissions) are required, since none of the steps perform write operations to GitHub resources. This preserves current behavior while constraining the token.
-
Copy modified lines R2-R3
| @@ -1,4 +1,6 @@ | ||
| name: Setup, Build & Test modules | ||
| permissions: | ||
| contents: read | ||
| on: | ||
| workflow_call: | ||
| inputs: |
| | Feature | Description | | ||
| | ----------------------- | ----------------------------------------------------------------- | | ||
| | `crypto-rust` (default) | Pure Rust crypto using RustCrypto crates | | ||
| | `crypto-ring` | Ring-only crypto (limited algorithm support) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any point in providing partially supported features like crypto-ring or crypto-graviola?
By doing the following, it will be easier for users to choose a backend, and implementers will be able to optimize their internal implementations depending on the supported cipher suites of ring and glaviora without affecting users.
| Feature | Description |
|---|---|
crypto-rust (default) |
Pure Rust crypto using RustCrypto crates |
crypto-ring |
Ring for hashing/HMAC, RustCrypto for everything else |
crypto-graviola |
Graviola for hashing/HMAC/AES-GCM, RustCrypto for everything else |
crypto-openssl |
OpenSSL-based crypto |
EDIT: If we want to follow the tls concept, we may want to support crypto-aws-lc.
Sytten
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there
|
|
||
| # OpenSSL TLS backend | ||
| tls-openssl = ["dep:openssl"] | ||
| openssl-vendored = ["openssl?/vendored"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You dont need to add a vendored feature everywhere.
Usually libraries will not even provide that feature, the user of the library just has to enable it on their on in their Cargo.tom by adding openssl as a dep and enabling the feature.
For you this would mean removing all the openssl-vendored features in all module crates and just adding it to the llrt/Cargo.toml. As long as it is enabled once, it is enabled everywhere since features are cumulative in rust and -sys crates can only appear one time in the build (cant have two version of the crate)
| 256 => Self::Aes256(cbc::Encryptor::new_from_slices(key, iv)?), | ||
| _ => return Err(InvalidLength), | ||
| }; | ||
| // AES variant types - only available with full subtle crypto support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is unclear is you mean the _rustcrypto or the _subtlefull
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also move that code in a different file, it makes reading this top mod hard
| Err(Exception::throw_message(ctx, "Algorithm not supported")) | ||
| } | ||
|
|
||
| // Stub implementations for when full subtle crypto is not available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same idea here. For clarify I would do the following file structure:
subtle
|-> import_key
|-> mod.rs (export based on feature)
|-> full.rs
|-> stub.rs
Then later we can even create a more targeted feature only key import, key export, etc.
Also reading the code lightly I dont see dep in the import_key
|
|
||
| # OpenSSL TLS backend | ||
| tls-openssl = ["llrt_tls/tls-openssl", "dep:hyper-openssl", "dep:openssl"] | ||
| openssl-vendored = ["llrt_tls/openssl-vendored", "openssl?/vendored"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same idea here, you also dont need the ?
|
|
||
| # TLS crypto backend features (rustls-based) | ||
| tls-ring = ["llrt_tls/tls-ring", "dep:hyper-rustls", "hyper-rustls?/ring", "dep:rustls"] | ||
| tls-aws-lc = ["llrt_tls/tls-aws-lc", "dep:hyper-rustls", "hyper-rustls?/aws-lc-rs", "dep:rustls"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You dont need the ? since the dep is requested just before it
| - tls-ring | ||
| - tls-aws-lc | ||
| - tls-graviola | ||
| include: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if you really need the include now?
| tls_feature: | ||
| required: false | ||
| type: string | ||
| default: "tls-ring" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would replace that with
crypto:
required: false
type: string
default: "ring"| - name: Setup Rust | ||
| uses: actions-rust-lang/setup-rust-toolchain@v1 | ||
| with: | ||
| toolchain: ${{ inputs.toolchain }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then add here:
- name: Map crypto feature to TLS and crypto features
id: features
shell: bash
run: |
case "${{ inputs.crypto }}" in
graviola)
echo "tls_feature=tls-graviola" >> $GITHUB_OUTPUT
echo "crypto_feature=crypto-graviola" >> $GITHUB_OUTPUT
;;
ring)
echo "tls_feature=tls-ring" >> $GITHUB_OUTPUT
echo "crypto_feature=crypto-ring" >> $GITHUB_OUTPUT
;;
openssl)
echo "tls_feature=tls-openssl" >> $GITHUB_OUTPUT
echo "crypto_feature=crypto-openssl" >> $GITHUB_OUTPUT
;;
aws-lc)
echo "tls_feature=tls-aws-lc" >> $GITHUB_OUTPUT
echo "crypto_feature=crypto-ring" >> $GITHUB_OUTPUT
;;
*)
echo "Unknown crypto feature: ${{ inputs.crypto }}"
exit 1
;;
esacYou can add other cases if you want to mix and match
| uses: actions-rust-lang/setup-rust-toolchain@v1 | ||
| with: | ||
| toolchain: ${{ inputs.toolchain }} | ||
| - name: Run build crates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then replace the following with
- name: Run build crates
shell: bash
env:
RUSTFLAGS: ""
TLS_FEATURE: ${{ steps.features.outputs.tls_feature }}
CRYPTO_FEATURE: ${{ steps.features.outputs.crypto_feature }}
run: |
crates=$(cargo metadata --no-deps --format-version 1 --quiet | jq -r '.packages[] | select(.manifest_path | contains("modules/")) | .name')
for crate in $crates; do
echo "Compiling crate: $crate"
# Use TLS feature for crates that need TLS, with --no-default-features to avoid conflicts
if [ "$crate" = "llrt_fetch" ] || [ "$crate" = "llrt_http" ]; then
cargo build -p "$crate" --no-default-features --features "http1,http2,webpki-roots,compression-rust,$TLS_FEATURE"
elif [ "$crate" = "llrt_crypto" ]; then
cargo build -p "$crate" --no-default-features --features "$CRYPTO_FEATURE"
else
cargo build -p "$crate"
fi
done
- name: Run build all
env:
TLS_FEATURE: ${{ steps.features.outputs.tls_feature }}
CRYPTO_FEATURE: ${{ steps.features.outputs.crypto_feature }}
run: |
cargo build -p llrt_modules --no-default-features --features "base,$TLS_FEATURE,$CRYPTO_FEATURE"
- name: Run tests all
env:
TLS_FEATURE: ${{ steps.features.outputs.tls_feature }}
CRYPTO_FEATURE: ${{ steps.features.outputs.crypto_feature }}
run: |
cargo test -p llrt_modules --no-default-features --features "base,$TLS_FEATURE,$CRYPTO_FEATURE"
| #[cfg(feature = "tls-graviola")] | ||
| fn get_crypto_provider() -> Arc<rustls::crypto::CryptoProvider> { | ||
| Arc::new(rustls_graviola::default_provider()) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a tls-openssl, you can use the rustls-openssl crate for that
| let bytes = bytes.as_bytes(&ctx)?; | ||
| this.0.borrow_mut().context.update(bytes); | ||
|
|
||
| this.0.borrow_mut().data.extend_from_slice(bytes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not ideal since it will keep the whole data in memory until digest.
hmac and hash are streaming encoding so you only keep buffer the size of the output + one block.
I think you can use the trick <crate::provider::DefaultProvider as CryptoProvider>::Hashmac.
Otherwise I would box the trait like Box<dyn HmacProvider>, the indirection is worth the space save.
| fn hash_digest<'js>(&self, ctx: Ctx<'js>, encoding: Opt<String>) -> Result<Value<'js>> { | ||
| let digest = self.context.clone().finish(); | ||
| let bytes: &[u8] = digest.as_ref(); | ||
| let mut digest_hasher = CRYPTO_PROVIDER.digest(self.algorithm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
Issue # (if available)
Description of changes
Refactor crypto and HTTP modules to support multiple TLS backends
This makes it easier to choose different Crypto & TLS implementations depending on deployment needs.
Checklist
tests/unitand/or in Rust for my feature if neededmake fixto format JS and apply Clippy auto fixesmake checktypes/directoryBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.