test(rng): pin crypto-edge deterministic fingerprints#259
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (2)
✨ 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 establishes critical guardrails for the crypto-edge Random Number Generator (RNG) convergence lane by enhancing the existing determinism regression suite. It introduces exact fixture-identity canaries for cryptographic outputs, particularly focusing on X.509 material, using BLAKE3 hashes instead of direct blob snapshots. This foundational work ensures that future changes to the RNG topology can be rigorously validated against known deterministic outputs, providing a robust mechanism for verifying consistency and preventing regressions. Highlights
Changelog
Activity
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 strengthens the determinism regression tests by adding fingerprint snapshots for various cryptographic artifacts, which is a great step towards ensuring stability. My review focuses on improving the efficiency and readability of the new test functions by avoiding redundant computations. The changes are otherwise solid and well-aligned with the project's goals.
| let snapshot = CryptoEdgeKeyFingerprints { | ||
| rsa: KeyMaterialFingerprint { | ||
| label: "fingerprint-rsa", | ||
| private_der_len: rsa.private_key_pkcs8_der().len(), | ||
| private_der_blake3: blake3_hex(rsa.private_key_pkcs8_der()), | ||
| public_der_len: rsa.public_key_spki_der().len(), | ||
| public_der_blake3: blake3_hex(rsa.public_key_spki_der()), | ||
| kid: rsa.kid(), | ||
| }, | ||
| ecdsa_p256: KeyMaterialFingerprint { | ||
| label: "fingerprint-ecdsa-p256", | ||
| private_der_len: ecdsa_p256.private_key_pkcs8_der().len(), | ||
| private_der_blake3: blake3_hex(ecdsa_p256.private_key_pkcs8_der()), | ||
| public_der_len: ecdsa_p256.public_key_spki_der().len(), | ||
| public_der_blake3: blake3_hex(ecdsa_p256.public_key_spki_der()), | ||
| kid: ecdsa_p256.kid(), | ||
| }, | ||
| ecdsa_p384: KeyMaterialFingerprint { | ||
| label: "fingerprint-ecdsa-p384", | ||
| private_der_len: ecdsa_p384.private_key_pkcs8_der().len(), | ||
| private_der_blake3: blake3_hex(ecdsa_p384.private_key_pkcs8_der()), | ||
| public_der_len: ecdsa_p384.public_key_spki_der().len(), | ||
| public_der_blake3: blake3_hex(ecdsa_p384.public_key_spki_der()), | ||
| kid: ecdsa_p384.kid(), | ||
| }, | ||
| ed25519: KeyMaterialFingerprint { | ||
| label: "fingerprint-ed25519", | ||
| private_der_len: ed25519.private_key_pkcs8_der().len(), | ||
| private_der_blake3: blake3_hex(ed25519.private_key_pkcs8_der()), | ||
| public_der_len: ed25519.public_key_spki_der().len(), | ||
| public_der_blake3: blake3_hex(ed25519.public_key_spki_der()), | ||
| kid: ed25519.kid(), | ||
| }, | ||
| }; |
There was a problem hiding this comment.
To improve efficiency and readability, it's better to call the key serialization functions (e.g., private_key_pkcs8_der()) only once per key and store their results in local variables. This avoids repeated, potentially expensive computations for each key type.
let rsa_private_der = rsa.private_key_pkcs8_der();
let rsa_public_der = rsa.public_key_spki_der();
let ecdsa_p256_private_der = ecdsa_p256.private_key_pkcs8_der();
let ecdsa_p256_public_der = ecdsa_p256.public_key_spki_der();
let ecdsa_p384_private_der = ecdsa_p384.private_key_pkcs8_der();
let ecdsa_p384_public_der = ecdsa_p384.public_key_spki_der();
let ed25519_private_der = ed25519.private_key_pkcs8_der();
let ed25519_public_der = ed25519.public_key_spki_der();
let snapshot = CryptoEdgeKeyFingerprints {
rsa: KeyMaterialFingerprint {
label: "fingerprint-rsa",
private_der_len: rsa_private_der.len(),
private_der_blake3: blake3_hex(rsa_private_der),
public_der_len: rsa_public_der.len(),
public_der_blake3: blake3_hex(rsa_public_der),
kid: rsa.kid(),
},
ecdsa_p256: KeyMaterialFingerprint {
label: "fingerprint-ecdsa-p256",
private_der_len: ecdsa_p256_private_der.len(),
private_der_blake3: blake3_hex(ecdsa_p256_private_der),
public_der_len: ecdsa_p256_public_der.len(),
public_der_blake3: blake3_hex(ecdsa_p256_public_der),
kid: ecdsa_p256.kid(),
},
ecdsa_p384: KeyMaterialFingerprint {
label: "fingerprint-ecdsa-p384",
private_der_len: ecdsa_p384_private_der.len(),
private_der_blake3: blake3_hex(ecdsa_p384_private_der),
public_der_len: ecdsa_p384_public_der.len(),
public_der_blake3: blake3_hex(ecdsa_p384_public_der),
kid: ecdsa_p384.kid(),
},
ed25519: KeyMaterialFingerprint {
label: "fingerprint-ed25519",
private_der_len: ed25519_private_der.len(),
private_der_blake3: blake3_hex(ed25519_private_der),
public_der_len: ed25519_public_der.len(),
public_der_blake3: blake3_hex(ed25519_public_der),
kid: ed25519.kid(),
},
};| let snapshot = X509MaterialFingerprint { | ||
| label: "x509-fingerprint", | ||
| cert_der_len: cert.cert_der().len(), | ||
| cert_der_blake3: blake3_hex(cert.cert_der()), | ||
| key_der_len: cert.private_key_pkcs8_der().len(), | ||
| key_der_blake3: blake3_hex(cert.private_key_pkcs8_der()), | ||
| cert_pem_header: pem_header(cert.cert_pem()).to_string(), | ||
| }; |
There was a problem hiding this comment.
To improve efficiency and readability, it's better to call cert_der() and private_key_pkcs8_der() only once and store their results in local variables.
| let snapshot = X509MaterialFingerprint { | |
| label: "x509-fingerprint", | |
| cert_der_len: cert.cert_der().len(), | |
| cert_der_blake3: blake3_hex(cert.cert_der()), | |
| key_der_len: cert.private_key_pkcs8_der().len(), | |
| key_der_blake3: blake3_hex(cert.private_key_pkcs8_der()), | |
| cert_pem_header: pem_header(cert.cert_pem()).to_string(), | |
| }; | |
| let cert_der = cert.cert_der(); | |
| let key_der = cert.private_key_pkcs8_der(); | |
| let snapshot = X509MaterialFingerprint { | |
| label: "x509-fingerprint", | |
| cert_der_len: cert_der.len(), | |
| cert_der_blake3: blake3_hex(cert_der), | |
| key_der_len: key_der.len(), | |
| key_der_blake3: blake3_hex(key_der), | |
| cert_pem_header: pem_header(cert.cert_pem()).to_string(), | |
| }; |
| let snapshot = X509ChainFingerprint { | ||
| label: "x509-chain-fingerprint", | ||
| chain_pem_cert_count: chain.chain_pem().matches("BEGIN CERTIFICATE").count(), | ||
| leaf_cert_der_len: chain.leaf_cert_der().len(), | ||
| leaf_cert_der_blake3: blake3_hex(chain.leaf_cert_der()), | ||
| intermediate_cert_der_len: chain.intermediate_cert_der().len(), | ||
| intermediate_cert_der_blake3: blake3_hex(chain.intermediate_cert_der()), | ||
| root_cert_der_len: chain.root_cert_der().len(), | ||
| root_cert_der_blake3: blake3_hex(chain.root_cert_der()), | ||
| leaf_key_der_blake3: blake3_hex(chain.leaf_private_key_pkcs8_der()), | ||
| intermediate_key_der_blake3: blake3_hex(chain.intermediate_private_key_pkcs8_der()), | ||
| root_key_der_blake3: blake3_hex(chain.root_private_key_pkcs8_der()), | ||
| }; |
There was a problem hiding this comment.
To improve efficiency and readability, it's better to call the various *_der() and *_pem() methods only once and store their results in local variables before creating the snapshot struct.
let chain_pem = chain.chain_pem();
let leaf_cert_der = chain.leaf_cert_der();
let intermediate_cert_der = chain.intermediate_cert_der();
let root_cert_der = chain.root_cert_der();
let leaf_key_der = chain.leaf_private_key_pkcs8_der();
let intermediate_key_der = chain.intermediate_private_key_pkcs8_der();
let root_key_der = chain.root_private_key_pkcs8_der();
let snapshot = X509ChainFingerprint {
label: "x509-chain-fingerprint",
chain_pem_cert_count: chain_pem.matches("BEGIN CERTIFICATE").count(),
leaf_cert_der_len: leaf_cert_der.len(),
leaf_cert_der_blake3: blake3_hex(leaf_cert_der),
intermediate_cert_der_len: intermediate_cert_der.len(),
intermediate_cert_der_blake3: blake3_hex(intermediate_cert_der),
root_cert_der_len: root_cert_der.len(),
root_cert_der_blake3: blake3_hex(root_cert_der),
leaf_key_der_blake3: blake3_hex(leaf_key_der),
intermediate_key_der_blake3: blake3_hex(intermediate_key_der),
root_key_der_blake3: blake3_hex(root_key_der),
};
Summary
Add the first execution guardrails for the crypto-edge RNG convergence lane.
This does not move any dependencies yet. It strengthens the existing determinism regression suite so later convergence work has exact fixture-identity canaries for the crypto-edge outputs.
What changed
blake3to the integration test crate so regression tests can fingerprint DER output without snapshotting secret-shaped blobstests/determinism_regression.rsso the determinism contract stays in one placeWhy
The workspace already had broad determinism coverage, but the remaining gap for the convergence lane was exact crypto-edge fixture identity pinning, especially around X.509 material. These snapshots give us before/after proof for later RNG-topology changes without committing PEM/DER blobs.
Verification
cargo test -p uselesskey-integration-tests --features determinism-regression --test determinism_regressioncargo test -p uselesskey-integration-tests --features determinism --test cross_crate_determinismcargo xtask gateRefs #257