Repository navigation
fix(crypto): let library consumers select their backend - #5046
varshaprasad96 wants to merge 3 commits into
Conversation
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
|
@cv would appreciate any reviews on this. Thanks! |
Ygnas
left a comment
There was a problem hiding this comment.
LGTM, the backend-selection split makes sense: libraries stay backend-neutral, and executables choose AWS-LC.
krishicks
left a comment
There was a problem hiding this comment.
Written by Claude Code, reviewed by @krishicks
Verdict: approve with changes. The fix is correct and the isolated consumer fixture is the right regression guard. Blocking issue: existing Rust SDK users get a runtime-only break that the docs and release notes don't mention.
Verified on 604c339 (test suites not run):
tests/crypto-consumergraphs forcore-minimal,core-default,core-all,sdk, andpolicycontain noaws-lc-*orring, including with--target all. In theruntimegraph, rcgen stays on Ring.- All 11 crates with binary targets resolve
openshell-cryptowithaws-lcboth with and without default features. That list is complete onmain. e2e/rust,tests/testkit,tests/suites/{conformance,features}, andexamples/governance-interceptorstill resolveaws-lc.- The
ensure_default_provider()placement matches tonic 0.14.6 provider selection intransport/channel/service/tls.rs: process default, then the compiled provider, thenClientConfig::builder()crate-feature auto-install. Without the call, a consumer's Ring would be picked up from Cargo features. openshell-{gateway-interceptors,isolation-interface,policy,prover,providers,supervisor-control,supervisor-middleware,supervisor-middleware-builtins,supervisor-process,tui}have no backend under standalonecargo test -p. None callopenshell_crypto::directly, so no change is needed.
Should fix before merge
1. Published SDK docs now lead to a runtime panic.
docs/sdk/rust.mdx tells users to run only cargo add openshell-sdk. After this change, build_channel over https://, OIDC, and the edge tunnel panic with install a crypto context before use when no default backend is built. Plaintext http:// is unaffected, so the docs example still runs. The build succeeds and the failure only appears at the first TLS connection. AGENTS.md requires a docs update for user-facing behavior, and the Release Notes section is empty. The backport mentioned in #4967 makes the release note more important, because a patch release would carry this break.
2. Restoring the previous behavior requires depending on an internal crate.
tests/testkit, examples/isolation-backend-v8, and examples/supervisor-middleware-content-guard all had to add a direct openshell-crypto dependency. Suggest an opt-in aws-lc feature on openshell-sdk and openshell-core that forwards to openshell-crypto/aws-lc. Libraries stay backend-neutral by default, and the docs fix becomes --features aws-lc. Making that feature on by default would let any crate in the graph that keeps defaults re-enable AWS-LC through feature unification. That is the failure mode #4967 describes, so opt-in is preferable.
3. More Result-returning public functions can now panic.
ensure_default_provider() is now reached from openshell_sdk::transport::build_channel, edge_tunnel::open_ws, openshell_extension_core::transport::connect_channel, openshell_core::grpc_client::configure_gateway_tls, and OtlpTarget::configure. With no backend selected, these panic inside a tokio task instead of returning an error. Suggest adding a fallible try_ensure_default_provider() -> Result<_, CryptoError> for at least the SDK and extension-core; it still refuses to run without a backend. Also suggest a panic message that names the fix: enable openshell-crypto/aws-lc or call install_default_context.
Worth addressing
4. verify-library-crypto.sh hardcodes the binary list.
The list is complete today. On the windows branch, openshell-mxc-boundary builds the openshell-windows-sandbox binary with no openshell-crypto dependency, so after a rebase it loses its backend and no check catches it. Any new binary crate has the same gap. Suggest deriving the list from cargo metadata (packages with bin targets) with an explicit exemption list.
5. The openshell-driver-vm manifest change exists only to satisfy the script.
openshell-crypto became non-optional with aws-lc so that the --no-default-features check passes. All crypto use in the crate is behind compute-driver. Without default features, the crate builds only the library and openshell-vm-init, the guest binary that tasks/scripts/vm/build-supervisor-bundle.sh builds with cargo zigbuild --no-default-features. Suggest keeping the dependency optional, adding "openshell-crypto/aws-lc" to compute-driver, and exempting driver-vm's minimal mode from the check. As a side benefit, openshell-vm-init would stop compiling aws-lc-sys for the guest target. On main, the workspace-level aws-lc forced that build; this PR could remove it.
6. Script robustness:
- Add
--target allto thecargo treecalls. Without it, cfg-gated dependencies for other platforms go unchecked. The graphs are clean with it today. - The regex
' aws-lc(,|$)'depends onaws-lcsorting first among enabled features.[ ,]aws-lc(,|$)does not.
7. CI cost.
The new step runs on all three rust-test matrix runners and builds a second, separate workspace (core-all, sdk, policy, ring, tokio) inside a 45-minute job. With --target all, the graph checks give the same result on any platform, so the step could run on one Linux runner.
8. Undocumented process-global side effect.
The new doc comments in openshell-core and openshell-sdk say an existing Rustls process provider is preserved. They don't say the reverse: on the first TLS use, the SDK installs the facade provider as the process default if the host has not installed one. A host that calls CryptoProvider::install_default() afterwards gets Err.
Nits
- Redundant dev-dependency in
crates/openshell-gateway/Cargo.toml: the normal dependency already enablesaws-lc. - Redundant
openshell-crypto = { workspace = true }dev-dependency incrates/openshell-driver-podman/Cargo.toml. - One line in
CRYPTO.mdunder "Selection and initialization" is not wrapped. - In
selected_backend.rs,let _ = openshell_sdk::oidc::http_client(false);asserts nothing. openshell-extension-corenow has a normal dependency onopenshell-crypto, which adds rcgen, jsonwebtoken, x509-parser, and pem to extension authors' graphs solely forensure_default_provider(). Acceptable, but worth knowing.
Process
- PR Issue Acceptance is failing: #4967 is
state:triage-neededand the author does not have write access. - Branch Checks have not run (waiting on
/ok to test), sorust:verify:library-cryptohas not run on Linux CI.
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
|
@krishicks Thanks for the detailed review. Addressed the feedback in 8ed99ad:
I also integrated the merged OpenSSL backend and added isolated consumer coverage for SDK TLS trust validation and core JWT operations with OpenSSL, alongside the existing host-provider tests. |
Summary
Core, SDK, and policy imports currently enable AWS-LC through workspace feature
unification, overriding a consumer's Ring-backed rcgen selection. Remove the
workspace-wide backend choice and OpenTelemetry's implicit AWS-LC activation.
Executable packages continue selecting AWS-LC explicitly, including with package
defaults disabled. TLS entrypoints initialize the host or facade provider before
constructing dependency-owned clients.
Related Issue
Closes #4967
Changes
This PR is based on
mainand independent of the OpenSSL addition in #4843.Release Notes
Testing
Passed locally on macOS after rebasing onto upstream
main:mise run rust:verify:library-crypto: isolated dependency graphs, backend-free compilation, and three integration tests.Linux/Nix CI and full product/E2E execution have not been run.
Checklist