refactor(crypto): introduce a generic interface for crypto libraries and refactor AWS-LC - #3351
varshaprasad96 wants to merge 8 commits into
Conversation
2db18e0 to
1ca93bb
Compare
51dcf3e to
2d57d54
Compare
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
…xt dispatch Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
2d57d54 to
3607f8e
Compare
politerealism
left a comment
There was a problem hiding this comment.
Crypto boundary already has drift — needs to be closed before merge, not after
This PR's stated goal is to make openshell-crypto the single seam for backend crypto so a future FIPS-validated backend only has to change one place. That boundary is already out of date against main: PR #2942 (RFC-0012's final merge, landed 2026-09-16) added new call sites that bypass the facade entirely, and none of them are caught by tasks/scripts/check_crypto_boundary.py. Rebasing onto current main needs to close these, or the boundary this PR builds is incomplete on day one.
1. crates/openshell-core/src/jwt.rs:809 — jsonwebtoken::crypto::aws_lc::DEFAULT_PROVIDER.install_default() called directly in install_crypto_provider(). This should route through openshell_crypto::install_jwt_provider() like every other JWT init site this PR migrated. (Note: this one line will actually fail crypto:check as currently written — its regex incidentally matches crypto::aws_lc:: as a substring — so this is a hard blocker on rebase, not just a style gap.)
2. crates/openshell-core/src/jwt.rs:897 and crates/openshell-core/src/driver_utils.rs:1278 — both call rcgen:: directly (KeyPair/PKCS_ED25519 in a test, and rcgen::generate_simple_self_signed(...) for a test CA fixture) instead of openshell_crypto::pki::*. These will not be caught by crypto:check — its regex only matches aws_lc_rs::/ring::/specific feature strings, not bare rcgen::. Recommend widening the regex to also flag rcgen:: outside openshell-crypto, otherwise this exact drift will keep landing silently.
3. New crate crates/openshell-binary-identity/src/lib.rs — hash_executable() hashes a live executable via sha2::{Digest, Sha256} and feeds the result directly into BinaryIdentity.binary_digest (traced: resolve_linux_process → hash_executable → BinaryIdentity { binary_digest: Some(...), .. }), which RFC-0012 backends bind to intercepted connections before trusting them. This is a security-relevant hash, not a cache key, and it's on a completely different implementation (sha2 crate) than everything else in the codebase (aws_lc_rs-backed via openshell_crypto::sha256). Recommend migrating this to openshell_crypto::sha256_digest() and adding sha2:: to the boundary check's banned patterns.
Checked: no existing review comment on this PR or on #2942 raised any of this — including the automated gator-agent review on #2942, which covered auth/session/TLS issues but not this. Flagging now so it's addressed as part of this PR's rebase rather than needing a follow-up.
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
There was a problem hiding this comment.
Back from a trip and catching up on this. I (via cc-review) reviewed the branch at 7bd236c06 with a focus on one question that I had in mind: can this seam carry a second, OpenSSL-backed implementation for FIPS 140-3? That is what makes a single-implementation abstraction worth having.
Short answer: yes, and I think the shape is right. A few things are worth fixing before merge, and one thing is worth correcting in the PR's favour.
The abstraction level is the right one
CryptoBackend and ProtocolBackend deal in protocol-library types (rustls, rcgen, jsonwebtoken) and never in AWS-LC types. Putting the seam between the protocol libraries and their crypto backend, rather than between OpenShell and the protocol libraries, is what makes an OpenSSL backend expressible at all. Seven of the eight trait methods are cleanly substitutable: TLS and JWT both route through pluggable CryptoProvider types, and the five primitives use raw byte interfaces.
I had expected the JWT path to be the blocker, since jsonwebtoken has no OpenSSL backend of its own. It turns out not to be a problem: its CryptoProvider exposes signer_factory and verifier_factory as function pointers, so an OpenSSL backend supplies its own Signer/Verifier. Non-trivial work, but no interface change needed.
The boundary is stronger than the removed lint suggests
Worth stating explicitly, because it is easy to miss and it argues in your favour. Stripping the backend features from workspace rustls, tokio-rustls, jsonwebtoken, sqlx and kube, and leaving aws-lc-rs consumed only by openshell-crypto, means a crate outside the facade cannot obtain a working crypto provider at all. That is structural enforcement through Cargo feature unification, and it is stronger than any grep-based lint.
Which reframes the removed check_crypto_boundary.py. The surfaces Cargo genuinely cannot gate are narrower than the general case: rcgen, declared with features = ["crypto"] workspace-wide so its key-material APIs work anywhere, and sha2, an ungated plain dependency. If the lint comes back, scoping it to KeyPair::generate / from_pem / from_der plus direct sha2 use would catch what matters without flagging CertificateParams, DnType or IsCa, which are legitimate parameter types. My guess is that noise is why it was dropped rather than widened, and that seems like the right instinct applied slightly too broadly. The PR description still advertises the check, so that line needs updating either way.
Worth addressing before merge
1. generate_keypair is the one surface that does not substitute. ProtocolBackend::generate_keypair returns rcgen::KeyPair, which owns the signing implementation compiled into rcgen. An OpenSSL backend cannot produce one that signs with OpenSSL. The README notes this in a parenthetical about rcgen remote keys, but that feature does not ship today, so the mitigation is hypothetical.
This matters more than it first appears, because pki::self_signed() and signed_by() are pure pass-throughs. Signing crypto is fixed by the key object at generation time, so key generation is the only real interception point in the PKI path, and it is precisely the one that leaks. I would promote this from a parenthetical to an explicit "known gap" section saying the signature is expected to change when the OpenSSL backend lands. Better to write that down now than to discover it during the backend work.
2. Nothing in-tree says why this abstraction exists. The README mentions OpenSSL only in the exclusions section. The crate rustdoc says "first stage" without naming what later stages target. architecture/build.md calls OpenSSL "separate follow-up work" without connecting it to the reason for the indirection. Issue #900 is not referenced anywhere in the crate.
An abstraction with one implementation and no stated second one reads as premature generalization. Someone will eventually propose inlining it, and the justification currently lives only in this PR description, which is not where they will look. A paragraph in the README opening and a line in the crate rustdoc naming the OpenSSL/FIPS backend and linking #900 would settle it permanently.
3. The backend substitution tests prove dispatch, not substitutability. In context_selection.rs, TestBackend delegates sha256_digest, tls_provider and jwt_provider to CryptoContext::default(), and stubs seal/open/generate_keypair to fail. NoEntropy is similar. The atomic counters do prove the selected context's factories get invoked, but the crypto executing underneath is still AWS-LC, so an interface that is subtly AWS-LC-shaped would pass every one of these.
One test with a stub that implements seal/open independently, even a trivial test-only cipher, asserting a facade-level roundtrip through it, would prove the Sealed layout, nonce handling and error contract are genuinely backend-neutral. That is the test that would have caught the generate_keypair issue above.
4. The ciphertext compatibility test lost its encryption-direction check. The previous test encrypted with a fixed nonce through the raw AWS-LC API and asserted byte-identical output. The new one calls encrypt_bytes(), which generates a nonce internally, so it can only round-trip. Decryption against the hardcoded vectors still works, which is the more important direction. But a change to ciphertext layout, say tag position or nonce ordering, would now pass. Since preserving the credential envelope format is one of the PR's stated guarantees, a KAT calling aead::open() directly with a known key, nonce and ciphertext would restore the missing half.
Smaller items
openshell-driver-db-credstore/src/lib.rs:112: the KEK is a bare[u8; 32]with no zeroize-on-drop, and the per-credential DEK local is the same.SecretJwtalready usesZeroizing, so the pattern exists in-tree.lib.rs:286-289: the three JWT negative assertions use bareis_err(). A regression returningInvalidSignaturewhereInvalidIssueris expected would pass silently.jwt_preserves_eddsa_and_claim_validationcovers encode-side algorithm mismatch but not the decode-side confusion case, which is the one that gets attacked in practice. Wrong-audience and tampered-signature cases are also absent.examples/governance-interceptor/Cargo.tomldeclaresrcgenandsha2directly alongsideopenshell-crypto. Examples get copied, so this one teaches both the pattern and its bypass.- The
Duration::from_secstoDuration::from_minschange across twelve e2e files is unrelated to the crypto work. It is harmless and does not break the declared MSRV, but it adds surface to an already large review.
Context worth having
The downstream driver here is FIPS 140-3 compliance for regulated deployments, where every cryptographic operation in a shipped image has to run through a validated module and images must dynamically link rather than statically embed their crypto. That work depends on this abstraction landing first. So the items above that look like documentation nits are not nits: they decide whether the second backend can be built against this interface without breaking it again.
Happy to help with the OpenSSL backend once this lands, and I can take the README and known-gap wording if that is useful.
|
One more item. The review above came back clean on correctness, so I ran a second, narrower pass with Claude Fable 5.1, scoped to initialization ordering and the sequences an embedder can hit, and let it write probe tests against the crate. It found one thing. It is benign today, but it is the thing most likely to bite the OpenSSL backend, so I would rather raise it while the interface is still open.
|
Summary
Centralize OpenShell's first-party cryptography behind a backend-neutral
openshell-cryptocrate while preserving AWS-LC as the only production backend and retaining current runtime behavior. This prepares a reviewable extension boundary for a later OpenSSL backend without introducing FIPS mode, changing algorithms, or migrating persisted data in this PR.Related Issue
Related to #900.
Changes
openshell-crypto.Testing
mise run pre-commitpassesAdditional validation:
cargo test -p openshell-server --features test-supportpasses on the rebased branch.mise run cireached the VM suite and stopped on five tests requiring unavailablemke2fs/mkfs.ext4tools and two VM provisioning timeouts.Checklist