chore(rng): clean support crates off direct legacy rand deps#245
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (10)
Summary by CodeRabbit
WalkthroughA refactoring that consolidates RNG/seed functionality by removing workspace dependencies on rand_chacha and rand_core, updating import paths to use uselesskey_core::Seed, and standardizing closure parameters in fuzz targets from rng to seed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 focuses on refining the Random Number Generation (RNG) dependency landscape within support crates. It systematically removes direct dependencies on older Highlights
Changelog
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request updates various dependencies, including rand and libc crates, and upgrades several uselesskey-* crates from version 0.1.0 to 0.3.0. A significant refactoring was done to centralize random number generation (RNG) and seeding logic by introducing a Seed type from uselesskey-core and removing direct rand_core and rand_chacha dependencies from many Cargo.toml files and fuzz targets. The review comments highlight an improvement opportunity in the fuzz tests, suggesting that multiple token generations within a single fuzz target should derive unique seeds from the initial input seed to ensure independent randomness and improve fuzzing effectiveness, rather than reusing the same seed for each generation.
| // Exercise individual generators with fuzz-derived label. | ||
| let mut rng_api = ChaCha20Rng::from_seed(input.seed); | ||
| let api = generate_api_key(&mut rng_api); | ||
| let api = generate_api_key(seed); | ||
| assert!(api.starts_with("uk_test_")); | ||
|
|
||
| let mut rng_bearer = ChaCha20Rng::from_seed(input.seed); | ||
| let bearer = generate_bearer_token(&mut rng_bearer); | ||
| let bearer = generate_bearer_token(seed); | ||
| assert_eq!(bearer.len(), 43); | ||
|
|
||
| let mut rng_oauth = ChaCha20Rng::from_seed(input.seed); | ||
| let oauth = generate_oauth_access_token(&input.label, &mut rng_oauth); | ||
| let oauth = generate_oauth_access_token(&input.label, seed); | ||
| assert_eq!(oauth.matches('.').count(), 2); | ||
|
|
||
| // Exercise random_base62 with fuzz-derived length. | ||
| let len = input.base62_len as usize; | ||
| let mut rng_b62 = ChaCha20Rng::from_seed(input.seed); | ||
| let b62 = random_base62(&mut rng_b62, len); | ||
| let b62 = random_base62(seed, len); | ||
| assert_eq!(b62.len(), len); |
There was a problem hiding this comment.
The fuzz test generates several token types (api, bearer, oauth, b62) using the same seed. This means the randomness used for each token is not independent, as each generation function will re-seed an RNG with the same value and produce correlated random streams. This reduces the effectiveness of the fuzzing. To improve this, you should derive a unique seed for each token generation.
// Exercise individual generators with fuzz-derived label.
let api_seed = uselesskey_core_id::derive_seed(&seed, &uselesskey_core_id::ArtifactId::new("fuzz", "api", &[], "v1", uselesskey_core_id::DerivationVersion::V1));
let api = generate_api_key(api_seed);
assert!(api.starts_with("uk_test_"));
let bearer_seed = uselesskey_core_id::derive_seed(&seed, &uselesskey_core_id::ArtifactId::new("fuzz", "bearer", &[], "v1", uselesskey_core_id::DerivationVersion::V1));
let bearer = generate_bearer_token(bearer_seed);
assert_eq!(bearer.len(), 43);
let oauth_seed = uselesskey_core_id::derive_seed(&seed, &uselesskey_core_id::ArtifactId::new("fuzz", "oauth", &[], "v1", uselesskey_core_id::DerivationVersion::V1));
let oauth = generate_oauth_access_token(&input.label, oauth_seed);
assert_eq!(oauth.matches('.').count(), 2);
// Exercise random_base62 with fuzz-derived length.
let len = input.base62_len as usize;
let b62_seed = uselesskey_core_id::derive_seed(&seed, &uselesskey_core_id::ArtifactId::new("fuzz", "b62", &[], "v1", uselesskey_core_id::DerivationVersion::V1));
let b62 = random_base62(b62_seed, len);
assert_eq!(b62.len(), len);| // Fuzz random_base62 with arbitrary lengths (capped to avoid OOM). | ||
| let len = input.base62_len as usize; | ||
| let mut rng = ChaCha20Rng::from_seed(input.seed); | ||
| let token = random_base62(&mut rng, len); | ||
| let seed = Seed::new(input.seed); | ||
| let token = random_base62(seed, len); | ||
| assert_eq!(token.len(), len); | ||
| assert!(token.chars().all(|c| c.is_ascii_alphanumeric())); | ||
|
|
||
| // Determinism: same seed + same length = same output. | ||
| let mut rng2 = ChaCha20Rng::from_seed(input.seed); | ||
| let token2 = random_base62(&mut rng2, len); | ||
| let token2 = random_base62(seed, len); | ||
| assert_eq!(token, token2); | ||
|
|
||
| // Fuzz individual generators. | ||
| let mut rng_api = ChaCha20Rng::from_seed(input.seed); | ||
| let api = generate_api_key(&mut rng_api); | ||
| let api = generate_api_key(seed); | ||
| assert!(api.starts_with("uk_test_")); | ||
|
|
||
| let mut rng_bearer = ChaCha20Rng::from_seed(input.seed); | ||
| let bearer = generate_bearer_token(&mut rng_bearer); | ||
| let bearer = generate_bearer_token(seed); | ||
| assert_eq!(bearer.len(), 43); | ||
|
|
||
| let label = String::from_utf8_lossy(&input.label_bytes); | ||
| let mut rng_oauth = ChaCha20Rng::from_seed(input.seed); | ||
| let oauth = generate_oauth_access_token(&label, &mut rng_oauth); | ||
| let oauth = generate_oauth_access_token(&label, seed); | ||
| assert_eq!(oauth.matches('.').count(), 2); |
There was a problem hiding this comment.
The fuzz test generates several token types (random_base62, api, bearer, oauth) using the same seed. This means the randomness used for each token is not independent, as each generation function will re-seed an RNG with the same value and produce correlated random streams. This reduces the effectiveness of the fuzzing. To improve this, you should derive a unique seed for each token generation.
// Fuzz random_base62 with arbitrary lengths (capped to avoid OOM).
let len = input.base62_len as usize;
let seed = Seed::new(input.seed);
let b62_seed = uselesskey_core_id::derive_seed(&seed, &uselesskey_core_id::ArtifactId::new("fuzz", "b62", &[], "v1", uselesskey_core_id::DerivationVersion::V1));
let token = random_base62(b62_seed, len);
assert_eq!(token.len(), len);
assert!(token.chars().all(|c| c.is_ascii_alphanumeric()));
// Determinism: same seed + same length = same output.
let token2 = random_base62(b62_seed, len);
assert_eq!(token, token2);
// Fuzz individual generators.
let api_seed = uselesskey_core_id::derive_seed(&seed, &uselesskey_core_id::ArtifactId::new("fuzz", "api", &[], "v1", uselesskey_core_id::DerivationVersion::V1));
let api = generate_api_key(api_seed);
assert!(api.starts_with("uk_test_"));
let bearer_seed = uselesskey_core_id::derive_seed(&seed, &uselesskey_core_id::ArtifactId::new("fuzz", "bearer", &[], "v1", uselesskey_core_id::DerivationVersion::V1));
let bearer = generate_bearer_token(bearer_seed);
assert_eq!(bearer.len(), 43);
let label = String::from_utf8_lossy(&input.label_bytes);
let oauth_seed = uselesskey_core_id::derive_seed(&seed, &uselesskey_core_id::ArtifactId::new("fuzz", "oauth", &[], "v1", uselesskey_core_id::DerivationVersion::V1));
let oauth = generate_oauth_access_token(&label, oauth_seed);
assert_eq!(oauth.matches('.').count(), 2);c99b664 to
bc58f69
Compare
4508483 to
4b13092
Compare
9f87ad0 to
88cae74
Compare
4b13092 to
d40008b
Compare
d40008b to
7347a2e
Compare
Summary
This is PR 3 in the RNG upgrade lane, stacked on top of #244.
It cleans up support crates that were still depending directly on the legacy rand line even though they no longer need to.
What changed
rand_core/rand_chachadependencies from the fuzz craterand 0.10by refactor(rng): move seed and helper crates to rand 0.10 #244uselesskey-bdd-stepscore_token_shape_stepsto importSeedfromuselesskey-coreinstead of an optional crate gate that is not enabled byuk-core-token-shaperand_coredev-dependency fromuselesskey-rustcryptorsa's own re-exportedrand_core::OsRnginstead of a workspace-level direct dependencyWhy this PR exists separately
The helper/core migration in #244 is the real dual-stack topology shift.
This PR is follow-up cleanup around local/test/support code:
bdd-stepsstill carried stale direct rand depsuselesskey-rustcryptoonly needed legacy rand for a test-localOsRnghelperKeeping this separate makes the review boundary cleaner and avoids mixing support cleanup into the dependency-topology PR.
Testing
cargo test -p uselesskey-rustcrypto --features all --test rustcrypto_cross_verifycargo test -p uselesskey-bdd-steps --features uk-core-token-shape --no-runcargo check --manifest-path fuzz/Cargo.toml --binscargo xtask gateNotes
This still does not converge the workspace on one RNG line.
The remaining legacy island is still the stable crypto edge and its transitive users:
uselesskey-rsauselesskey-ed25519uselesskey-ecdsauselesskey-pgpuselesskey-x509propteststill bringsrand 0.9