Skip to content

refactor(crypto): introduce a generic interface for crypto libraries and refactor AWS-LC - #3351

Open
varshaprasad96 wants to merge 8 commits into
NVIDIA:mainfrom
varshaprasad96:feat/generic-crypto-backend
Open

varshaprasad96 wants to merge 8 commits into
NVIDIA:mainfrom
varshaprasad96:feat/generic-crypto-backend

Conversation

@varshaprasad96

@varshaprasad96 varshaprasad96 commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Centralize OpenShell's first-party cryptography behind a backend-neutral openshell-crypto crate 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

  • Add backend-neutral contracts for cryptographic randomness, incremental SHA-256, AES-256-GCM, capability reporting, backend context selection, and protocol adapters.
  • Add AWS-LC as the sole implementation and route Rustls, rcgen, jsonwebtoken, bootstrap PKI/JWT, credential encryption, CLI/SDK, gateway, supervisor, examples, and tests through the facade.
  • Centralize AWS-LC feature selection for Tonic, Hyper-Rustls, kube, and SQLx while documenting that these dependencies still select crypto internally.
  • Preserve TLS algorithms, Ed25519 gateway JWTs, P-256 certificate keys, native trust roots, credential envelope formats, and embedder-owned process defaults.
  • Reject strict posture requests in this stage and document unresolved provider ownership/version evidence plus the SSH, SigV4, SPIFFE, AWS SDK transport, and non-Rust runtime boundaries.
  • Add backend substitution, known-answer, tamper, JWT validation, provider initialization, and ciphertext compatibility coverage, plus a CI check that prevents new direct backend use outside openshell-crypto.

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable)

Additional validation:

  • cargo test -p openshell-server --features test-support passes on the rebased branch.
  • Focused crypto, bootstrap, credential-store, and supervisor-network suites were exercised. Three supervisor-network cases timed out while broader suites ran concurrently and passed when rerun serially.
  • mise run ci reached the VM suite and stopped on five tests requiring unavailable mke2fs/mkfs.ext4 tools and two VM provisioning timeouts.
  • Docker-backed E2E was attempted before the rebase, but the local Docker daemon health request timed out. The E2E workspace compiles and passes Clippy through pre-commit.

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

@copy-pr-bot

copy-pr-bot Bot commented Sep 15, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@varshaprasad96 varshaprasad96 changed the title refactor(crypto): centralize AWS-LC behind backend interface refactor(crypto): introduce a generic interface for crypto libraries and refactor AWS-LC Sep 15, 2026
@varshaprasad96
varshaprasad96 force-pushed the feat/generic-crypto-backend branch from 2db18e0 to 1ca93bb Compare September 15, 2026 19:10
@varshaprasad96
varshaprasad96 marked this pull request as ready for review September 15, 2026 20:08
@drew
drew requested a review from SDAChess September 15, 2026 21:35
@varshaprasad96
varshaprasad96 force-pushed the feat/generic-crypto-backend branch from 51dcf3e to 2d57d54 Compare September 15, 2026 22:42
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>
@varshaprasad96
varshaprasad96 force-pushed the feat/generic-crypto-backend branch from 2d57d54 to 3607f8e Compare September 16, 2026 19:45

@politerealism politerealism left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:809jsonwebtoken::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.rshash_executable() hashes a live executable via sha2::{Digest, Sha256} and feeds the result directly into BinaryIdentity.binary_digest (traced: resolve_linux_processhash_executableBinaryIdentity { 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>

@rhuss rhuss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. SecretJwt already uses Zeroizing, so the pattern exists in-tree.
  • lib.rs:286-289: the three JWT negative assertions use bare is_err(). A regression returning InvalidSignature where InvalidIssuer is expected would pass silently.
  • jwt_preserves_eddsa_and_claim_validation covers 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.toml declares rcgen and sha2 directly alongside openshell-crypto. Examples get copied, so this one teaches both the pattern and its bypass.
  • The Duration::from_secs to Duration::from_mins change 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.

@rhuss

rhuss commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

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.

CryptoContext does not control TLS end to end

ensure_default_provider() in crypto/src/tls.rs:20-25 only installs when CryptoProvider::get_default() is None. But rustls's ServerConfig::builder() and ClientConfig::builder() call get_default_or_install_from_crate_features(), which installs a process default from whatever backend feature is compiled in. With aws_lc_rs enabled on rustls in openshell-crypto/Cargo.toml, any builder() that runs before the facade's install wins, and the facade's install becomes a no-op.

The gateway does this on its own startup path: build_server_config() at openshell-server/src/tls.rs:418 calls ServerConfig::builder() as the first TLS operation, before any ensure_default_provider(). The crate rustdoc at lib.rs:166 already states the constraint ("select before initializing either protocol library, including indirect initialization by dependencies"), so the rule is documented; it is just not followed by the first binary that matters.

There are no builder_with_provider() calls anywhere in the tree. Every production TLS config goes through the process default: server tls.rs:418,427, CLI tls.rs:218,327, SDK transport.rs:198, supervisor l7/tls.rs:120,282, sandbox boundary_server.rs:3239.

The Fable pass confirmed the preemption with two throwaway tests in the crate, both deleted afterwards. The first shows that a single ServerConfig::builder() call with nothing installed leaves get_default() set, so the later ensure_default_provider() returns the auto-installed provider. The second installs a custom CryptoContext whose TLS provider has one cipher suite, calls builder(), and observes the process default holding the full AWS-LC suite list instead.

Why it matters for the follow-up: rustls has no OpenSSL feature, so an OpenSSL provider can only arrive through an explicit install. If the OpenSSL backend is selected via install_default_context and any code path reaches builder() first, TLS silently negotiates with AWS-LC while provider() and any_supported_signing_key() report OpenSSL. A key type OpenSSL supports and AWS-LC does not would pass facade validation and fail the handshake, and the process would be quietly non-FIPS with no error.

One related consequence today: InsecureServerCertVerifier::supported_verify_schemes() in sdk/transport.rs:186 and cli/tls.rs:287 reads from provider(), while the ClientConfig it attaches to is built with builder(). Harmless because the insecure verifier accepts everything, but the two can disagree in an embedder scenario.

tls_embedder.rs does not catch any of this, since it asserts what ensure_default_provider() and provider() return and never builds a config or opens a connection.

Suggested fix. Dropping the is_none() guard would break the embedder-preservation behaviour the test protects, so that is the wrong lever. The durable fix is builder_with_provider(Arc::new(openshell_crypto::tls::provider())) at each of the seven production config sites. That makes context selection genuinely true for TLS regardless of initialization order, which is the property the OpenSSL backend will need. A small facade helper that returns a pre-provisioned ConfigBuilder would keep call sites from drifting back to builder().

Together with the generate_keypair point above, the shape is the same: the facade holds the selection, but the underlying library has its own path around it. Closing both now is cheaper than discovering them from a failed FIPS handshake later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants