Enable cose key_kms so the CoseError match stays exhaustive - #202
Open
Matt (matt-evervault) wants to merge 4 commits into
Open
Enable cose key_kms so the CoseError match stays exhaustive#202Matt (matt-evervault) wants to merge 4 commits into
Matt (matt-evervault) wants to merge 4 commits into
Conversation
aws-nitro-enclaves-cose puts AwsSignError, AwsVerifyError and AwsGetPublicKeyError on CoseError behind its key_kms feature. We depend on the crate with default-features = false and never request key_kms, but Cargo unifies features across the dependency graph, so any other crate in a consumer's tree can enable it for our copy. CoseError then gains three variants, our From<CoseError> match stops being exhaustive, and we fail to build with E0004 in someone else's project. This is not hypothetical: aws-nitro-enclaves-image-format 0.6.0+ declares aws-nitro-enclaves-cose with features = ["key_kms"], so any project using both it and this crate cannot build. Both published 0.7.4 and 0.10.1 are affected. Add a catch-all arm mapping to NsmError::UnsupportedError, which is the honest mapping since we do local attestation verification and never use KMS-backed keys. The #[allow(unreachable_patterns)] is needed because with key_kms off every variant already has an explicit arm. Verified that cargo check compiles clean with features = ["key_kms"] temporarily enabled, instead of failing E0004. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Vaultkeeper Commands Mention
You can also request |
Replaces the catch-all arm from the previous commit with explicit arms for the three KMS variants, keeping compile-time exhaustiveness. aws-nitro-enclaves-cose puts AwsSignError, AwsVerifyError and AwsGetPublicKeyError on CoseError behind its key_kms feature. Because Cargo unifies features across the dependency graph, any consumer that also depends on aws-nitro-enclaves-image-format 0.6+ (which declares cose with features = ["key_kms"]) gets those variants switched on for our copy too, making our match non-exhaustive and failing the build with E0004. Rather than paper over that with a catch-all, enable key_kms ourselves so the variants always exist and can be matched explicitly. An upstream variant addition is then a compile error here instead of something a wildcard silently swallows. key_kms pulls in tokio and openssl (it also implies key_openssl_pkey), neither of which builds for wasm32-unknown-unknown, so it is enabled via a cfg(not(target_arch = "wasm32")) dependency section and the three arms are cfg'd to match. On wasm32 the variants do not exist, so the remaining arms are exhaustive there too. This requires resolver = "2" on the workspace. Without it Cargo falls back to resolver 1, which unifies features across all targets and enables key_kms for wasm32 regardless of the cfg gate — that made the wasm build fail with 49 errors in mio. Verified: cargo check, cargo fmt --check, cargo clippy -- -W clippy::pedantic and cargo test -- --skip time_sensitive all pass. The wasm32 check gets past tokio/mio and fails only in ring's build script, which is where a clean tree also fails locally (no wasm C toolchain here) — CI should confirm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Matt (matt-evervault)
changed the title
Comment on lines
+37
to
+43
| # `key_kms` is enabled so the CoseError match in src/nsm/error.rs stays exhaustive. | ||
| # aws-nitro-enclaves-image-format 0.6+ turns this feature on through Cargo's feature | ||
| # unification, which silently adds three variants to CoseError. Enabling it ourselves means | ||
| # those variants always exist here and are matched explicitly, so an upstream addition is a | ||
| # compile error rather than something a catch-all arm swallows. | ||
| # Excluded on wasm32: key_kms pulls in tokio (mio) and openssl, neither of which builds for | ||
| # wasm32-unknown-unknown. |
Contributor
Author
There was a problem hiding this comment.
Suggested change
| # `key_kms` is enabled so the CoseError match in src/nsm/error.rs stays exhaustive. | |
| # aws-nitro-enclaves-image-format 0.6+ turns this feature on through Cargo's feature | |
| # unification, which silently adds three variants to CoseError. Enabling it ourselves means | |
| # those variants always exist here and are matched explicitly, so an upstream addition is a | |
| # compile error rather than something a catch-all arm swallows. | |
| # Excluded on wasm32: key_kms pulls in tokio (mio) and openssl, neither of which builds for | |
| # wasm32-unknown-unknown. |
Comment on lines
+49
to
+54
| // These variants only exist when aws-nitro-enclaves-cose is built with `key_kms`, | ||
| // which Cargo.toml enables on every target except wasm32. Matching them | ||
| // explicitly keeps this match exhaustive, so adding a variant upstream is a | ||
| // compile error here rather than something silently swallowed by a catch-all. | ||
| // We never construct KMS-backed keys — attestation docs are verified locally — | ||
| // so in practice these are unreachable. |
Contributor
Author
There was a problem hiding this comment.
Suggested change
| // These variants only exist when aws-nitro-enclaves-cose is built with `key_kms`, | |
| // which Cargo.toml enables on every target except wasm32. Matching them | |
| // explicitly keeps this match exhaustive, so adding a variant upstream is a | |
| // compile error here rather than something silently swallowed by a catch-all. | |
| // We never construct KMS-backed keys — attestation docs are verified locally — | |
| // so in practice these are unreachable. |
Drops the wasm32 exceptions from the previous commit: key_kms is now a plain dependency feature and the three KMS arms are matched without cfg gates. The workspace resolver = "2" is also removed, since its only purpose was making the per-target gate effective. Rationale for enabling key_kms at all is unchanged: aws-nitro-enclaves-cose puts AwsSignError, AwsVerifyError and AwsGetPublicKeyError on CoseError behind that feature, and Cargo unifies features across the dependency graph, so any consumer that also pulls aws-nitro-enclaves-image-format 0.6+ gets them switched on and our match stops being exhaustive (E0004). Enabling it ourselves means the variants always exist and are handled explicitly, so an upstream addition is a compile error rather than something a catch-all silently swallows. KNOWN BREAKAGE: this breaks the wasm bindings. key_kms pulls in tokio and openssl (it implies key_openssl_pkey), and neither builds for wasm32-unknown-unknown: error: could not compile `mio` (lib) due to 49 previous errors Verified with a working local wasm toolchain (LLVM 19 clang + llvm-ar + lld) that does produce a wasm_attestation_bindings.wasm from the previous, target-gated commit — so this is the removed gate, not a toolchain gap. CI's build_wasm job will fail until the wasm bindings are either dropped or the gate is restored. Native side is green: cargo check, cargo fmt --check, cargo clippy -- -W clippy::pedantic and cargo test -- --skip time_sensitive all pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This reverts commit f62e9b3.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Enables
key_kmson ouraws-nitro-enclaves-cosedependency so theCoseError→NsmErrormatch stays exhaustive, instead of compiling only by luck of which features a consumer happens to enable.Why
aws-nitro-enclaves-coseputs three variants behind itskey_kmsfeature:We depend on the crate with
default-features = falseand never asked forkey_kms. But Cargo features are additive and unified across the whole dependency graph — if any other crate in a consumer's tree enableskey_kms, it is enabled for our copy too.CoseErrorthen has three more variants, ourmatchis no longer exhaustive, and we fail to build in someone else's project:This is not hypothetical.
aws-nitro-enclaves-image-format0.6.0+ declaresaws-nitro-enclaves-cosewithfeatures = ["key_kms"], so any project depending on both that and this crate cannot build. It currently blocks evervault/evervault-cli#260, which needs image-format 0.6+ to get offsha20.9. Both published0.7.4and0.10.1are affected.Approach
The obvious fix is a catch-all
other => ...arm, and the first commit on this branch did that. The problem with a catch-all is that it also swallows any future variant we genuinely should handle — we would silently map it toUnsupportedErrorinstead of getting a compile error telling us to look.So instead: enable
key_kmsourselves. The variants then always exist, we match all three explicitly, and there is no wildcard. An upstream addition toCoseErrorbecomes a compile error here, which is what we want.The wasm32 wrinkle
key_kms = ["aws-sdk-kms", "tokio", "key_openssl_pkey"], so it drags in tokio and (transitively) openssl. Neither builds forwasm32-unknown-unknown— enabling it unconditionally fails the wasm bindings with 49 errors inmio.So it is enabled per-target:
with the three arms
#[cfg(not(target_arch = "wasm32"))]. On wasm32 the variants do not exist, so the remaining ten arms are exhaustive there as well. No catch-all and no#[allow]on either target.This needs
resolver = "2"The workspace root did not set a resolver, so Cargo was defaulting to resolver 1, which unifies features across all targets and turns
key_kmson for wasm32 regardless of thecfggate. With resolver 1 the target gating above does nothing and wasm still dies inmio.Adding
resolver = "2"to the workspace makes the gate effective. That is the modern default and what these edition-2021 crates already assume — but it is a repo-wide change to feature resolution, so it is worth a careful look. Resolver 2 can reduce features relative to resolver 1, so if any binding was quietly relying on unification to get a feature turned on, this is where it would show up. The node, python and wasm CI jobs should catch that.Verification
Run with the nix
cargo/rustc/clippy/rustfmt1.97.x toolchain, mirroringcargo make ci:cargo check— passescargo fmt --check— cleancargo clippy -- -W clippy::pedantic— no errors, no new warnings referencingnsm/error.rscargo test -- --skip time_sensitive— 5 passed, 0 failedwasm32 is verified, not assumed
I now have a full local wasm toolchain working (nix LLVM 19 clang +
llvm-ar+lld; the earlierringfailures were a missing wasm C toolchain on my machine, unrelated to this change).cargo build -p wasm-attestation-bindings --target wasm32-unknown-unknownsucceeds on this branch, producing a realwasm_attestation_bindings.wasm(27,204,797 bytes, debug profile). Running the same build with the four changed files reverted tomainproduces a byte-identical artifact and the same single pre-existing warning (unused import: std::str::Bytes).Three independent pieces of evidence that wasm is unaffected:
main.cargo tree -p wasm-attestation-bindings --target wasm32-unknown-unknown -e normal,featuresis 897 lines on both, anddiffreports no change — so neither the package set nor the feature flags moved.openssl|tokio|aws-sdk-kms|mio|hyper|rustlsreturns zero matches;aws-nitro-enclaves-coseappears there without them.This is what
resolver = "2"buys: thecfg(not(target_arch = "wasm32"))gate is honoured, so wasm resolvesaws-nitro-enclaves-coseexactly as it does today. Under resolver 1 the same code fails with 49 errors inmio.CI's
build_wasmjob (which runswasm-packrather than rawcargo build) is still the authoritative check, but the local result and the tree diff both say it will pass.Trade-off to weigh
This adds
aws-sdk-kms,tokioandopensslto every non-wasm build of this crate — including the node, python, kotlin and swift bindings, which currently have no C dependencies at all.openssl-sysneeds OpenSSL headers at build time, which can complicate cross-compiled wheels and prebuilt binaries. wasm is explicitly excluded and verified unchanged (above), so the risk is concentrated in the native binding builds — thelint_and_test_nodeandbuild_and_test_pythonCI jobs are the ones to watch.That is the price of compile-time exhaustiveness here.
If that cost is judged too high, the catch-all in the first commit (
8bc652b) is the lighter alternative and I can drop back to it. There is also an upstream angle worth pursuing either way:aws-nitro-enclaves-image-formatforcingkey_kmson all its consumers looks like an upstream defect — if they made it opt-in, this crate would need no change at all.Note on the branch history
Three commits in the middle of this branch explore the alternatives; the tree is back to the target-gated approach:
8bc652b— catch-allother =>arm. Works everywhere, but silently swallows futureCoseErrorvariants.ae192b6— per-targetkey_kms+resolver = "2". Current state.f62e9b3—key_kmsunconditional, wasm exceptions removed. Reverted in9bd92b3.f62e9b3was reverted because removing the wasm exceptions cannot be made to work, and specifically it is not an openssl configuration problem.With
OPENSSL_DIR,OPENSSL_INCLUDE_DIR,WASM32_UNKNOWN_UNKNOWN_OPENSSL_DIR,WASM32_UNKNOWN_UNKNOWN_OPENSSL_INCLUDE_DIRandOPENSSL_NO_VENDORall set, the wasm build still fails, and the only crate that fails ismio:Those are missing-platform-backend errors:
mio'ssysmodule has nowasm32-unknown-unknownimplementation, and no include path or environment variable can supply one.openssl-sysnever even reaches its own build failure, becausemiofails first. (wasm32-unknown-unknownhas no libc or sysroot, so there is no OpenSSL build for it either.)mioarrives viatokio, which is pulled by bothaws-nitro-enclaves-coseitself andaws-sdk-kms/aws-smithy-*. It is inherent tokey_kmsrather than a configuration gap — which is why the per-target gate is the mechanism used here.