fix(rsa): preserve V1 deterministic RSA fixtures while adopting rsa 0.10 types#304
Conversation
Summary by CodeRabbit
WalkthroughWorkspace Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant upgrade of the project's internal RSA implementation by migrating to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates the rsa crate to version 0.10.0-rc.17 and its associated rand_core and rand_chacha dependencies. This upgrade required adjusting use statements for sha2::Sha256 to rsa::sha2::Sha256 and updating methods for accessing RSA key components. The serialization of private keys to JWK format in crates/uselesskey-rsa/src/keypair.rs needs correction due to API changes in rsa v0.10, which now wraps several key components in zeroize::Secret and other newtypes, requiring explicit unwrapping.
| let d = private.d().to_be_bytes_trimmed_vartime(); | ||
| let p = primes[0].to_be_bytes_trimmed_vartime(); | ||
| let q = primes[1].to_be_bytes_trimmed_vartime(); | ||
| let dp = private.dp().expect("dp").to_be_bytes_trimmed_vartime(); | ||
| let dq = private.dq().expect("dq").to_be_bytes_trimmed_vartime(); | ||
| let qi = private | ||
| .qinv() | ||
| .expect("qinv") | ||
| .retrieve() | ||
| .to_be_bytes_trimmed_vartime(); |
There was a problem hiding this comment.
This code for serializing the private key to a JWK appears to be incorrect due to API changes in rsa v0.10. The new version wraps several key components in zeroize::Secret and other newtypes to prevent accidental leakage. You need to correctly unwrap these values before using them.
Specifically:
d(),dp(),dq()and the items inprimes()now return values where the underlyingUintis wrapped inSecret. You need to use.expose_secret()to get a reference to the inner value before calling.to_be_bytes_trimmed_vartime().qinv()returns anOption<Secret<Inv<...>>>. You need to call.retrieve()on theSecretto get theInv, and then another.retrieve()on theInvto get the underlyingUint.
The current code will not compile. Here is a suggested fix:
| let d = private.d().to_be_bytes_trimmed_vartime(); | |
| let p = primes[0].to_be_bytes_trimmed_vartime(); | |
| let q = primes[1].to_be_bytes_trimmed_vartime(); | |
| let dp = private.dp().expect("dp").to_be_bytes_trimmed_vartime(); | |
| let dq = private.dq().expect("dq").to_be_bytes_trimmed_vartime(); | |
| let qi = private | |
| .qinv() | |
| .expect("qinv") | |
| .retrieve() | |
| .to_be_bytes_trimmed_vartime(); | |
| let d = private.d().expose_secret().to_be_bytes_trimmed_vartime(); | |
| let p = primes[0].expose_secret().to_be_bytes_trimmed_vartime(); | |
| let q = primes[1].expose_secret().to_be_bytes_trimmed_vartime(); | |
| let dp = private.dp().expect("dp").expose_secret().to_be_bytes_trimmed_vartime(); | |
| let dq = private.dq().expect("dq").expose_secret().to_be_bytes_trimmed_vartime(); | |
| let qi = private | |
| .qinv() | |
| .expect("qinv") | |
| .retrieve() | |
| .retrieve() | |
| .to_be_bytes_trimmed_vartime(); |
d7d523e to
205c514
Compare
205c514 to
113d5d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/uselesskey-rsa/src/keypair.rs (2)
613-639:⚠️ Potential issue | 🟠 MajorVersion the derivation namespace for the rsa10 generator.
This branch swaps to a different RNG/keygen stack but still writes under the v1 cache domain. For builds without
legacy-rsa09, the same(seed, label, spec, variant)now produces different fixture material underuselesskey:rsa:keypair, so the deterministic compatibility contract is no longer true.🔧 Possible fix
- pub const DOMAIN_RSA_KEYPAIR: &str = "uselesskey:rsa:keypair"; + #[cfg(feature = "legacy-rsa09")] + pub const DOMAIN_RSA_KEYPAIR: &str = "uselesskey:rsa:keypair"; + #[cfg(not(feature = "legacy-rsa09"))] + pub const DOMAIN_RSA_KEYPAIR: &str = "uselesskey:rsa:keypair:v2";As per coding guidelines, "Maintain deterministic derivation stability; bump derivation version if algorithm changes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/uselesskey-rsa/src/keypair.rs` around lines 613 - 639, The rsa10 keygen (rsa10::RsaPrivateKey / rsa10::RsaPublicKey) path currently writes under the existing derivation namespace ("uselesskey:rsa:keypair") which breaks deterministic compatibility; change the namespace used by the non-legacy branch (guarded by #[cfg(not(feature = "legacy-rsa09"))]) to a bumped version (e.g., "uselesskey:rsa:keypair:v2" or "uselesskey:rsa:keypair:rsa10") so rsa10-generated fixtures live in a new domain, and leave the legacy (feature = "legacy-rsa09") namespace unchanged; update all code that constructs or looks up that derivation key in keypair.rs to use the new string in the non-legacy cfg path.
405-459: 🧹 Nitpick | 🔵 TrivialJWK fields are already protected by
assert_eq!(v1, v2)in the determinism test.Line 675 of
tests/determinism_regression.rs(fn jwk_rsa_deterministic) compares the full JWK object usingassert_eq!(v1, v2), which includes all cryptographic fields (n,e,d,p,q,dp,dq,qi). Any change to field encoding or padding will fail the test.The snapshot captures only metadata (
kty,alg,use,kid), but the underlying regression protection is sound. To improve maintainability and visibility, consider expanding the snapshot to explicitly include the cryptographic field values (e.g., first 32 characters ofnande, or redacted placeholders) so future changes to encoding are immediately visible in snapshots.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/uselesskey-rsa/src/keypair.rs` around lines 405 - 459, The determinism test already asserts full JWK equality, but the snapshot only shows metadata; update the jwk_rsa_deterministic snapshot (the assertion that records the snapshot of private_key_jwk()) to include representative cryptographic field data so regressions in encodings are visible—modify the snapshot capture to include either redacted placeholders or truncated prefixes (e.g., first 32 chars) of fields n, e, d, p, q, dp, dq, qi produced by RsaKeypair::private_key_jwk() (look for private_key_jwk and the test fn jwk_rsa_deterministic in tests/determinism_regression.rs) and ensure the snapshot comparison records those values while still redacting full secrets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/uselesskey-rsa/Cargo.toml`:
- Around line 33-34: The Cargo.toml currently sets default = ["legacy-rsa09"],
causing most workspace consumers to pull the legacy rsa09 path implicitly;
change the default feature list to default = [] so legacy-rsa09 is opt-in, and
update documentation/consumer crates to enable the legacy-rsa09 feature (or set
default-features = false) when they intentionally need the "legacy-rsa09"
feature that depends on "rsa09", "rand_chacha", and "rand_core". Ensure the
feature name "legacy-rsa09" remains defined and referenced consistently so
consumers can explicitly opt in.
---
Outside diff comments:
In `@crates/uselesskey-rsa/src/keypair.rs`:
- Around line 613-639: The rsa10 keygen (rsa10::RsaPrivateKey /
rsa10::RsaPublicKey) path currently writes under the existing derivation
namespace ("uselesskey:rsa:keypair") which breaks deterministic compatibility;
change the namespace used by the non-legacy branch (guarded by #[cfg(not(feature
= "legacy-rsa09"))]) to a bumped version (e.g., "uselesskey:rsa:keypair:v2" or
"uselesskey:rsa:keypair:rsa10") so rsa10-generated fixtures live in a new
domain, and leave the legacy (feature = "legacy-rsa09") namespace unchanged;
update all code that constructs or looks up that derivation key in keypair.rs to
use the new string in the non-legacy cfg path.
- Around line 405-459: The determinism test already asserts full JWK equality,
but the snapshot only shows metadata; update the jwk_rsa_deterministic snapshot
(the assertion that records the snapshot of private_key_jwk()) to include
representative cryptographic field data so regressions in encodings are
visible—modify the snapshot capture to include either redacted placeholders or
truncated prefixes (e.g., first 32 chars) of fields n, e, d, p, q, dp, dq, qi
produced by RsaKeypair::private_key_jwk() (look for private_key_jwk and the test
fn jwk_rsa_deterministic in tests/determinism_regression.rs) and ensure the
snapshot comparison records those values while still redacting full secrets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fde6905f-16d4-46e7-ae23-5df1716bdc6a
⛔ Files ignored due to path filters (14)
Cargo.lockis excluded by!**/*.lockcrates/uselesskey-rustcrypto/tests/snapshots/snapshots_rustcrypto__rsa_snapshots__rustcrypto_rsa_2048_public_key.snapis excluded by!**/*.snapcrates/uselesskey-rustcrypto/tests/snapshots/snapshots_rustcrypto__rsa_snapshots__rustcrypto_rsa_4096_public_key.snapis excluded by!**/*.snapcrates/uselesskey-rustls/tests/snapshots/snapshots_rustls__x509_snapshots__rustls_chain_metadata.snapis excluded by!**/*.snapcrates/uselesskey-rustls/tests/snapshots/snapshots_rustls__x509_snapshots__rustls_self_signed_cert.snapis excluded by!**/*.snapcrates/uselesskey-rustls/tests/snapshots/snapshots_rustls__x509_snapshots__rustls_self_signed_custom_domain.snapis excluded by!**/*.snapcrates/uselesskey-rustls/tests/snapshots/snapshots_rustls__x509_snapshots__rustls_x509_chain_key_sizes.snapis excluded by!**/*.snapcrates/uselesskey-tonic/tests/snapshots/snapshots_tonic__chain_snapshots__tonic_chain_custom_domain_metadata.snapis excluded by!**/*.snapcrates/uselesskey-tonic/tests/snapshots/snapshots_tonic__chain_snapshots__tonic_chain_tls_metadata.snapis excluded by!**/*.snapcrates/uselesskey-tonic/tests/snapshots/snapshots_tonic__key_size_snapshots__tonic_chain_key_sizes.snapis excluded by!**/*.snapcrates/uselesskey-tonic/tests/snapshots/snapshots_tonic__mtls_snapshots__tonic_mtls_chain_cert_details.snapis excluded by!**/*.snapcrates/uselesskey-tonic/tests/snapshots/snapshots_tonic__mtls_snapshots__tonic_mtls_configs_build.snapis excluded by!**/*.snapcrates/uselesskey-tonic/tests/snapshots/snapshots_tonic__self_signed_snapshots__tonic_self_signed_custom_domain.snapis excluded by!**/*.snapcrates/uselesskey-tonic/tests/snapshots/snapshots_tonic__self_signed_snapshots__tonic_self_signed_tls_metadata.snapis excluded by!**/*.snap
📒 Files selected for processing (21)
Cargo.tomlcrates/uselesskey-bdd-steps/Cargo.tomlcrates/uselesskey-bdd-steps/src/lib.rscrates/uselesskey-interop-tests/Cargo.tomlcrates/uselesskey-interop-tests/tests/all_adapters.rscrates/uselesskey-interop-tests/tests/cross_adapter.rscrates/uselesskey-interop-tests/tests/cross_adapter_interop_matrix.rscrates/uselesskey-interop-tests/tests/cross_adapter_signing.rscrates/uselesskey-interop-tests/tests/cross_verify.rscrates/uselesskey-interop-tests/tests/interop.rscrates/uselesskey-interop-tests/tests/random_mode.rscrates/uselesskey-rsa/Cargo.tomlcrates/uselesskey-rsa/src/keypair.rscrates/uselesskey-rustcrypto/Cargo.tomlcrates/uselesskey-rustcrypto/README.mdcrates/uselesskey-rustcrypto/src/lib.rscrates/uselesskey-rustcrypto/tests/adapter_integration.rscrates/uselesskey-rustcrypto/tests/rustcrypto_comprehensive.rscrates/uselesskey-rustcrypto/tests/rustcrypto_cross_verify.rstests/Cargo.tomltests/cross_adapter.rs
| default = ["legacy-rsa09"] | ||
| legacy-rsa09 = ["dep:rsa09", "dep:rand_chacha", "dep:rand_core"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import pathlib, tomllib
for p in sorted(pathlib.Path(".").rglob("Cargo.toml")):
data = tomllib.loads(p.read_text(encoding="utf-8"))
for section in ("dependencies", "dev-dependencies", "build-dependencies"):
deps = data.get(section) or {}
dep = deps.get("uselesskey-rsa")
if dep is None:
continue
if isinstance(dep, str):
print(f"{p} [{section}] -> version-only (default features implicitly ON)")
elif isinstance(dep, dict):
df = dep.get("default-features", "<implicit true>")
feats = dep.get("features", [])
print(f"{p} [{section}] -> default-features={df}, features={feats}")
PYRepository: EffortlessMetrics/uselesskey
Length of output: 1900
Most workspace consumers will silently pull legacy rsa09 by default; consider making legacy opt-in.
With default = ["legacy-rsa09"], 11 out of 12 workspace consumers that depend on uselesskey-rsa are using implicit default features and thus pulling the legacy path. Only uselesskey-rustcrypto explicitly sets default-features = false. This contradicts the containment objective of having legacy dependencies be explicit and isolated. Switch to default = [] and require consumers that need legacy support to explicitly enable legacy-rsa09.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/uselesskey-rsa/Cargo.toml` around lines 33 - 34, The Cargo.toml
currently sets default = ["legacy-rsa09"], causing most workspace consumers to
pull the legacy rsa09 path implicitly; change the default feature list to
default = [] so legacy-rsa09 is opt-in, and update documentation/consumer crates
to enable the legacy-rsa09 feature (or set default-features = false) when they
intentionally need the "legacy-rsa09" feature that depends on "rsa09",
"rand_chacha", and "rand_core". Ensure the feature name "legacy-rsa09" remains
defined and referenced consistently so consumers can explicitly opt in.
Summary
This refocuses #304 as the V1-compatibility bridge for RSA.
Scope
rsa 0.10.0-rc.17uselesskey-rsafor the legacy 0.9 generation pathrsa 0.9.x/rand_core 0.6usage explicitly in upstream islands (jsonwebtoken,pgp)Compatibility
rsa::sha2::Sha256cargo tree -i rsa@0.10.0-rc.17shows the owned internal pathcargo tree -i rsa@0.9.10andcargo tree -i rand_core@0.6.4remain in explicit legacy islands only