chore: drop direct rand dep, add BDD corruption & tempfile tests#19
Conversation
`uselesskey-core` only used `rand` for `rand::rngs::OsRng`. Since `rand_core 0.6` provides `OsRng` directly (gated on its `std` feature), we can remove the `rand` crate entirely from workspace dependencies. This eliminates a brittle coupling that caused build failures during the `rand 0.10` upgrade attempt — downstream crates pulling `rand 0.10` would conflict with our pinned `rand 0.8`.
…9 edge cases - Deterministic corruption stability tests for RSA, ECDSA, Ed25519 - X.509 wrong-key-usage scenarios (sign-with-encipherment, etc.) - DER tempfile output verification across key types - Chain and CRL tempfile scenarios
Summary by CodeRabbitRelease Notes
WalkthroughThis PR removes the direct Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 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 @EffortlessSteven, 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 refines the project's dependency structure by removing an unnecessary direct 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
This pull request is a nice improvement. It correctly removes the direct rand dependency in favor of rand_core, which simplifies the dependency tree and avoids potential version conflicts. The addition of over 600 lines of BDD scenarios significantly enhances test coverage for deterministic corruption, tempfile outputs, and various X.509 edge cases. The new tests are thorough and well-structured. My review includes a couple of suggestions to reduce boilerplate and improve the maintainability of the newly added test code.
| #[when(regex = r#"^I deterministically corrupt the RSA PKCS8 PEM with variant "([^"]+)"$"#)] | ||
| fn det_corrupt_rsa_pem(world: &mut UselessWorld, variant: String) { | ||
| let rsa = world.rsa.as_ref().expect("rsa not set"); | ||
| world.deterministic_text_1 = Some(rsa.private_key_pkcs8_pem_corrupt_deterministic(&variant)); | ||
| } | ||
|
|
||
| #[when(regex = r#"^I deterministically corrupt the RSA PKCS8 PEM with variant "([^"]+)" again$"#)] | ||
| fn det_corrupt_rsa_pem_again(world: &mut UselessWorld, variant: String) { | ||
| let rsa = world.rsa.as_ref().expect("rsa not set"); | ||
| world.deterministic_text_2 = Some(rsa.private_key_pkcs8_pem_corrupt_deterministic(&variant)); | ||
| } | ||
|
|
||
| #[when(regex = r#"^I deterministically corrupt the RSA PKCS8 DER with variant "([^"]+)"$"#)] | ||
| fn det_corrupt_rsa_der(world: &mut UselessWorld, variant: String) { | ||
| let rsa = world.rsa.as_ref().expect("rsa not set"); | ||
| world.deterministic_bytes_1 = Some(rsa.private_key_pkcs8_der_corrupt_deterministic(&variant)); | ||
| } | ||
|
|
||
| #[when(regex = r#"^I deterministically corrupt the RSA PKCS8 DER with variant "([^"]+)" again$"#)] | ||
| fn det_corrupt_rsa_der_again(world: &mut UselessWorld, variant: String) { | ||
| let rsa = world.rsa.as_ref().expect("rsa not set"); | ||
| world.deterministic_bytes_2 = Some(rsa.private_key_pkcs8_der_corrupt_deterministic(&variant)); | ||
| } |
There was a problem hiding this comment.
There's significant boilerplate in the when steps for deterministic corruption. This pattern of getting a key, calling a corruption method, and storing the result is repeated for RSA, Ed25519, ECDSA, and X.509, for both PEM and DER formats, and for both the initial and ... again steps. This results in many nearly identical functions.
To reduce this duplication and improve maintainability, you could use a macro to generate these step definitions. For example:
macro_rules! define_det_corrupt_steps {
(
$fn_name:ident, $fn_name_again:ident,
$regex_key_name:literal, $key_field:ident,
$dest_field1:ident, $dest_field2:ident,
$method:ident
) => {
#[when(regex = concat!(r#"^I deterministically corrupt the "#, $regex_key_name, r#" with variant "([^"]+)"$"#))]
fn $fn_name(world: &mut UselessWorld, variant: String) {
let key = world.$key_field.as_ref().expect(concat!(stringify!($key_field), " not set"));
world.$dest_field1 = Some(key.$method(&variant));
}
#[when(regex = concat!(r#"^I deterministically corrupt the "#, $regex_key_name, r#" with variant "([^"]+)" again$"#))]
fn $fn_name_again(world: &mut UselessWorld, variant: String) {
let key = world.$key_field.as_ref().expect(concat!(stringify!($key_field), " not set"));
world.$dest_field2 = Some(key.$method(&variant));
}
};
}You could then replace these functions with macro calls, like so:
define_det_corrupt_steps!(
det_corrupt_rsa_pem,
det_corrupt_rsa_pem_again,
"RSA PKCS8 PEM",
rsa,
deterministic_text_1,
deterministic_text_2,
private_key_pkcs8_pem_corrupt_deterministic
);
define_det_corrupt_steps!(
det_corrupt_rsa_der,
det_corrupt_rsa_der_again,
"RSA PKCS8 DER",
rsa,
deterministic_bytes_1,
deterministic_bytes_2,
private_key_pkcs8_der_corrupt_deterministic
);| #[then(regex = r#"^the X\.509 DER tempfile path should end with "([^"]+)"$"#)] | ||
| fn x509_cert_der_tempfile_path_ends_with(world: &mut UselessWorld, suffix: String) { | ||
| let tf = world | ||
| .x509_cert_der_tempfile | ||
| .as_ref() | ||
| .expect("x509_cert_der_tempfile not set"); | ||
| let path = tf.path().to_string_lossy().to_string(); | ||
| assert!( | ||
| path.ends_with(&suffix), | ||
| "expected path to end with '{suffix}', got '{path}'" | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new then steps for verifying tempfile paths and contents introduce a lot of repetitive code. For example, x509_cert_der_tempfile_path_ends_with, chain_tempfile_path_ends_with, and their content-checking counterparts like x509_tempfile_matches_cert_der all share the same structure.
To make this more maintainable, you could extract this common logic into helper functions. For example:
// Helper for checking path suffixes
fn assert_tempfile_path_ends_with(
temp_artifact: Option<&uselesskey_core::sink::TempArtifact>,
name: &str,
suffix: &str,
) {
let tf = temp_artifact.unwrap_or_else(|| panic!("{} not set", name));
let path = tf.path().to_string_lossy();
assert!(
path.ends_with(suffix),
"expected path to end with '{suffix}', got '{path}'"
);
}
// Using the helper
#[then(regex = r#"^the X\.509 DER tempfile path should end with "([^\"]+)"$"#)]
fn x509_cert_der_tempfile_path_ends_with(world: &mut UselessWorld, suffix: String) {
assert_tempfile_path_ends_with(
world.x509_cert_der_tempfile.as_ref(),
"x509_cert_der_tempfile",
&suffix,
);
}A similar helper could be created for verifying file contents. This would significantly reduce the boilerplate across the test suite.
There was a problem hiding this comment.
Pull request overview
This pull request removes the direct rand dependency from the workspace in favor of using rand_core::OsRng directly, which has been available since rand_core 0.6 with the std feature. The PR also adds comprehensive BDD test scenarios (604 lines) covering deterministic corruption stability for RSA, ECDSA, Ed25519, and X.509 artifacts, as well as tempfile output verification for X.509 certificates, chains, and CRLs.
Changes:
- Removed direct
randworkspace dependency and replacedrand::rngs::OsRngimport withrand_core::OsRnginuselesskey-core - Added BDD test scenarios for deterministic corruption methods across RSA, ECDSA, Ed25519, and X.509 certificate types
- Extended BDD test coverage for X.509 wrong-key-usage variant and additional tempfile outputs (DER format, chain variants, CRL files)
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
Cargo.toml |
Removed rand = "0.8.5" from workspace dependencies and updated comment to reflect rand_core 0.6 |
Cargo.lock |
Removed rand 0.8.5 from uselesskey-core dependencies |
crates/uselesskey-core/Cargo.toml |
Removed optional rand dependency and added rand_core/std to the std feature |
crates/uselesskey-core/src/factory.rs |
Changed import from rand::rngs::OsRng to rand_core::OsRng |
crates/uselesskey-bdd/tests/bdd.rs |
Added 8 new fields to UselessWorld struct and 45+ new test step functions for deterministic corruption and tempfile scenarios |
crates/uselesskey-bdd/features/rsa.feature |
Added 2 scenarios for deterministic RSA corruption stability testing |
crates/uselesskey-bdd/features/ed25519.feature |
Added 2 scenarios for deterministic Ed25519 corruption stability testing |
crates/uselesskey-bdd/features/ecdsa.feature |
Added 2 scenarios for deterministic ECDSA corruption stability testing |
crates/uselesskey-bdd/features/x509.feature |
Added 3 scenarios for wrong-key-usage variant, deterministic X.509 corruption, and DER tempfile output |
crates/uselesskey-bdd/features/chain.feature |
Added 2 scenarios for additional chain tempfile outputs and CRL tempfile verification |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-bdd/features/x509.feature`:
- Around line 135-140: The scenario "deterministic X.509 PEM corruption with
variant is stable" is missing the parse-failure assertion; add a final step
mirroring other key tests such as RSA/ECDSA/Ed25519: append "And the
deterministic X.509 PEM artifact should fail to parse" to the scenario so the
corrupted PEM is asserted unparsable (match the existing DER test pattern and
naming).
In `@crates/uselesskey-bdd/tests/bdd.rs`:
- Around line 61-64: The four fields deterministic_text_1, deterministic_text_2,
deterministic_bytes_1 and deterministic_bytes_2 on UselessWorld are shared by
RSA/Ed25519/ECDSA/X509 deterministic corruption steps and can be silently
overwritten if a scenario mixes key types; refactor by introducing a nested
struct (e.g., DeterministicArtifacts) keyed or namespaced per key type and
replace the flat fields on UselessWorld with a map or per-key-type fields (or
alternately add a clear doc comment on UselessWorld stating the
single-key-type-per-scenario invariant) so tests either isolate artifacts by key
type or clearly document the constraint.
- Around line 343-365: Multiple near-identical step functions
(det_corrupt_rsa_pem, det_corrupt_rsa_pem_again, det_corrupt_rsa_der,
det_corrupt_rsa_der_again and their counterparts for other key types) repeat
boilerplate; replace them with a single macro (e.g., det_corrupt_steps!) that
takes the world field name (like rsa, ed25519, ecdsa, x509), the PEM/DER
corruption method names (private_key_pkcs8_pem_corrupt_deterministic,
private_key_pkcs8_der_corrupt_deterministic or equivalents), the four regex
literals, and four function identifiers, and expands to the four #[when(...)]
functions that assert world.<key_field>.as_ref().expect("<key_field> not set")
and assign to world.deterministic_text_1 / deterministic_text_2 or
world.deterministic_bytes_1 / deterministic_bytes_2 as appropriate; then remove
the duplicated explicit functions and invoke det_corrupt_steps! for each key
type (refer to the existing function names and world fields to wire arguments).
| deterministic_text_1: Option<String>, | ||
| deterministic_text_2: Option<String>, | ||
| deterministic_bytes_1: Option<Vec<u8>>, | ||
| deterministic_bytes_2: Option<Vec<u8>>, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Shared deterministic_text/bytes slots across key types — safe but fragile.
These four fields are written by RSA, Ed25519, ECDSA, and X.509 deterministic corruption steps alike. Since cucumber resets UselessWorld per scenario this works, but a future scenario that mixes key types in a single flow would silently overwrite artifacts. Consider namespacing (e.g., a nested DeterministicArtifacts struct) or documenting the single-key-type-per-scenario invariant so future contributors don't hit a subtle test bug.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/uselesskey-bdd/tests/bdd.rs` around lines 61 - 64, The four fields
deterministic_text_1, deterministic_text_2, deterministic_bytes_1 and
deterministic_bytes_2 on UselessWorld are shared by RSA/Ed25519/ECDSA/X509
deterministic corruption steps and can be silently overwritten if a scenario
mixes key types; refactor by introducing a nested struct (e.g.,
DeterministicArtifacts) keyed or namespaced per key type and replace the flat
fields on UselessWorld with a map or per-key-type fields (or alternately add a
clear doc comment on UselessWorld stating the single-key-type-per-scenario
invariant) so tests either isolate artifacts by key type or clearly document the
constraint.
| #[when(regex = r#"^I deterministically corrupt the RSA PKCS8 PEM with variant "([^"]+)"$"#)] | ||
| fn det_corrupt_rsa_pem(world: &mut UselessWorld, variant: String) { | ||
| let rsa = world.rsa.as_ref().expect("rsa not set"); | ||
| world.deterministic_text_1 = Some(rsa.private_key_pkcs8_pem_corrupt_deterministic(&variant)); | ||
| } | ||
|
|
||
| #[when(regex = r#"^I deterministically corrupt the RSA PKCS8 PEM with variant "([^"]+)" again$"#)] | ||
| fn det_corrupt_rsa_pem_again(world: &mut UselessWorld, variant: String) { | ||
| let rsa = world.rsa.as_ref().expect("rsa not set"); | ||
| world.deterministic_text_2 = Some(rsa.private_key_pkcs8_pem_corrupt_deterministic(&variant)); | ||
| } | ||
|
|
||
| #[when(regex = r#"^I deterministically corrupt the RSA PKCS8 DER with variant "([^"]+)"$"#)] | ||
| fn det_corrupt_rsa_der(world: &mut UselessWorld, variant: String) { | ||
| let rsa = world.rsa.as_ref().expect("rsa not set"); | ||
| world.deterministic_bytes_1 = Some(rsa.private_key_pkcs8_der_corrupt_deterministic(&variant)); | ||
| } | ||
|
|
||
| #[when(regex = r#"^I deterministically corrupt the RSA PKCS8 DER with variant "([^"]+)" again$"#)] | ||
| fn det_corrupt_rsa_der_again(world: &mut UselessWorld, variant: String) { | ||
| let rsa = world.rsa.as_ref().expect("rsa not set"); | ||
| world.deterministic_bytes_2 = Some(rsa.private_key_pkcs8_der_corrupt_deterministic(&variant)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Repeated boilerplate across four key types for deterministic corruption steps.
Each key type (RSA, Ed25519, ECDSA, X.509) repeats the same four-function pattern (PEM, PEM-again, DER, DER-again) differing only in which world field is read and which corruption method is called. That's 16 functions with near-identical bodies. A helper macro or generic step could cut this to a single implementation.
Example sketch:
macro_rules! det_corrupt_steps {
($key_field:ident, $pem_method:ident, $der_method:ident,
$pem_re:literal, $pem_again_re:literal,
$der_re:literal, $der_again_re:literal,
$fn_pem:ident, $fn_pem_again:ident,
$fn_der:ident, $fn_der_again:ident) => {
#[when(regex = $pem_re)]
fn $fn_pem(world: &mut UselessWorld, variant: String) {
let key = world.$key_field.as_ref().expect(concat!(stringify!($key_field), " not set"));
world.deterministic_text_1 = Some(key.$pem_method(&variant));
}
// ... analogous for the other three
};
}Also applies to: 817-847, 1185-1207, 1571-1601
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/uselesskey-bdd/tests/bdd.rs` around lines 343 - 365, Multiple
near-identical step functions (det_corrupt_rsa_pem, det_corrupt_rsa_pem_again,
det_corrupt_rsa_der, det_corrupt_rsa_der_again and their counterparts for other
key types) repeat boilerplate; replace them with a single macro (e.g.,
det_corrupt_steps!) that takes the world field name (like rsa, ed25519, ecdsa,
x509), the PEM/DER corruption method names
(private_key_pkcs8_pem_corrupt_deterministic,
private_key_pkcs8_der_corrupt_deterministic or equivalents), the four regex
literals, and four function identifiers, and expands to the four #[when(...)]
functions that assert world.<key_field>.as_ref().expect("<key_field> not set")
and assign to world.deterministic_text_1 / deterministic_text_2 or
world.deterministic_bytes_1 / deterministic_bytes_2 as appropriate; then remove
the duplicated explicit functions and invoke det_corrupt_steps! for each key
type (refer to the existing function names and world fields to wire arguments).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-x509/src/cert.rs`:
- Around line 463-464: The assertion assert_ne!(pem_a, "xyzzy") is a no-op
because corrupt_cert_pem_deterministic will never produce that literal; replace
it with a meaningful check that the corrupted PEM (pem_a) fails to parse: after
generating pem_a with corrupt_cert_pem_deterministic, attempt to parse it using
the project's PEM/X.509 parse routine (e.g., the function/method used elsewhere
to parse certs like X509::from_pem or parse_cert_pem) and assert that parsing
returns an error (or Err variant) rather than Ok; keep the existing
assert!(!pem_a.is_empty()) and remove the trivial assert_ne! line.
---
Duplicate comments:
In `@crates/uselesskey-bdd/features/x509.feature`:
- Around line 135-149: The deterministic X.509 corruption feature already
includes parse-failure assertions for both PEM and DER cases, so no code changes
are needed; confirm the two scenarios "deterministic X.509 PEM corruption with
variant is stable" and "deterministic X.509 DER corruption with variant is
stable" keep the lines asserting the deterministic artifacts are identical and
the lines asserting the PEM/DER artifact should fail to parse, and then
approve/merge the change as-is.
In `@crates/uselesskey-bdd/tests/bdd.rs`:
- Around line 61-64: The four shared fields deterministic_text_1,
deterministic_text_2, deterministic_bytes_1, deterministic_bytes_2 in
UselessWorld can be clobbered if a scenario mixes key types; change the storage
to be keyed by key type (e.g., replace those four Option fields with a
HashMap<KeyType, (Option<String>, Option<String>, Option<Vec<u8>>,
Option<Vec<u8>>) or separate per-key-type fields like
rsa_deterministic_text_1/2, ed25519_deterministic_text_1/2, etc.), and update
all uses in the deterministic corruption steps to read/write via the new keyed
names or map entries (search for uses of deterministic_text_1,
deterministic_text_2, deterministic_bytes_1, deterministic_bytes_2 to update).
- Around line 343-365: The four nearly identical step functions
(det_corrupt_rsa_pem, det_corrupt_rsa_pem_again, det_corrupt_rsa_der,
det_corrupt_rsa_der_again) should be collapsed into a macro or helper to remove
boilerplate; create a macro that accepts the world key accessor (e.g.,
world.rsa), the two target world fields (deterministic_text_1/2 or
deterministic_bytes_1/2), and the method names to call on the key
(private_key_pkcs8_pem_corrupt_deterministic and
private_key_pkcs8_der_corrupt_deterministic), and generate the paired step
handlers (normal and "again") for both PEM and DER variants; update the RSA,
Ed25519, ECDSA and X.509 blocks to use that macro so the four duplicated
patterns are defined once and reference the correct world fields and key
methods.
| assert!(!pem_a.is_empty()); | ||
| assert_ne!(pem_a, "xyzzy"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
assert_ne!(pem_a, "xyzzy") is a trivially-true no-op assertion.
corrupt_cert_pem_deterministic will never return the literal string "xyzzy", so this line never exercises any code path. Consider replacing it with a meaningful assertion such as verifying the corrupted PEM fails to parse.
🔧 Proposed replacement
- assert_ne!(pem_a, "xyzzy");
+ // Corrupted PEM should fail to parse
+ use x509_parser::prelude::*;
+ let pem_bytes = pem_a.as_bytes();
+ let result = x509_parser::pem::parse_x509_pem(pem_bytes);
+ assert!(result.is_err() || result.map(|(_, p)| X509Certificate::from_der(p.contents.as_slice()).is_err()).unwrap_or(true),
+ "deterministically corrupted PEM should fail to parse as a valid certificate");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert!(!pem_a.is_empty()); | |
| assert_ne!(pem_a, "xyzzy"); | |
| assert!(!pem_a.is_empty()); | |
| // Corrupted PEM should fail to parse | |
| use x509_parser::prelude::*; | |
| let pem_bytes = pem_a.as_bytes(); | |
| let result = x509_parser::pem::parse_x509_pem(pem_bytes); | |
| assert!(result.is_err() || result.map(|(_, p)| X509Certificate::from_der(p.contents.as_slice()).is_err()).unwrap_or(true), | |
| "deterministically corrupted PEM should fail to parse as a valid certificate"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/uselesskey-x509/src/cert.rs` around lines 463 - 464, The assertion
assert_ne!(pem_a, "xyzzy") is a no-op because corrupt_cert_pem_deterministic
will never produce that literal; replace it with a meaningful check that the
corrupted PEM (pem_a) fails to parse: after generating pem_a with
corrupt_cert_pem_deterministic, attempt to parse it using the project's
PEM/X.509 parse routine (e.g., the function/method used elsewhere to parse certs
like X509::from_pem or parse_cert_pem) and assert that parsing returns an error
(or Err variant) rather than Ok; keep the existing assert!(!pem_a.is_empty())
and remove the trivial assert_ne! line.
Summary
randfrom workspace dependencies.uselesskey-coreonly usedrandforrand::rngs::OsRng; sincerand_core 0.6providesOsRngdirectly (gated on itsstdfeature), therandcrate is no longer needed as a direct dependency. This eliminates a brittle coupling that caused build failures during therand 0.10upgrade attempt.Test plan
cargo check --workspace --all-features— compilescargo check -p uselesskey-core --no-default-features— no-std path compilescargo xtask feature-matrix— all 18 feature combos passcargo xtask test— all unit/integration tests passcargo xtask bdd— all 230 scenarios (1113 steps) passcargo xtask clippy— no warningscargo xtask dep-guard— no duplicaterand_core