chore(rust): upgrade ed25519-dalek 2.x -> 3.x and rand 0.8 -> 0.10 (fixes #3355) - #3420
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
🤖 AI Agent: code-reviewer — View details
TL;DR: 0 blockers, 0 warnings. Safe and clean upgrade.
No issues found. Clean change. |
🤖 AI Agent: contributor-guide — View details
Welcome, and thank you for your contribution! Great job coordinating the Before we can merge, please address the following:
For more details, refer to our CONTRIBUTING.md. Let us know if you need any help! |
🤖 AI Agent: docs-sync-checker — Docs Sync
Docs Sync
Please ensure the |
🤖 AI Agent: security-scanner — View details
No security issues found. |
🤖 AI Agent: test-generator — View details
Test coverage looks good. No gaps identified. |
🤖 AI Agent: breaking-change-detector — API Compatibility
API Compatibility
|
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
|
🔴 Contributor Check: HIGH
Automated check by AGT Contributor Check. |
There was a problem hiding this comment.
Pull request overview
TL;DR: 0 blockers, 0 warnings. No issues found. Clean change.
Changes:
- Upgrade
ed25519-dalekfrom2.2.0to3.0.0andrandfrom0.8.6to0.10.2across theagent-governance-rustworkspace. - Update call sites for
rand0.10 API moves/renames (e.g.,distributions→distr,thread_rng()→rng(),Rng→RngExt,RngCore→Rng).
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
agent-governance-rust/Cargo.toml |
Pins ed25519-dalek to =3.0.0 and rand to =0.10.2 at the workspace level. |
agent-governance-rust/Cargo.lock |
Updates the resolved dependency graph for the coordinated ed25519-dalek/rand upgrade. |
agent-governance-rust/agentmesh/src/identity.rs |
Switches key generation RNG call from OsRng to rand::rng() for SigningKey::generate. |
agent-governance-rust/agentmesh/src/identity_support.rs |
Updates SigningKey::generate RNG call sites to rand::rng() for credential/key rotation paths. |
agent-governance-rust/agentmesh/src/credential_vault.rs |
Adapts RNG trait usage (RngCore → Rng) and updates thread_rng() → rng() for byte filling. |
agent-governance-rust/agentmesh-mcp/src/mcp/clock.rs |
Updates rand imports (distr, RngExt) and RNG creation (rng()) for nonce generation. |
|
MohammadHaroonAbuomar this is the closest Rust PR to merge: mergeable, CI clean, resolves #3355, and the duplicate #3418 is now closed. Could you give it a code-owner review when you have a moment? Thanks. |
5b3dfd9 to
9d3d088
Compare
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- Process: real CI has never run on this head (all green checks are pull_request_target bot jobs; build-rust/CodeQL/SBOM stuck action_required) and the PR is CONFLICTING on the exact Cargo.toml/lock block being upgraded. Rebase onto main, get workflow runs approved, require genuinely green CI. (Local compensating run at head: cargo test 514/514 pass; code content verified sound incl. dalek3 signing determinism and no seeded-RNG leaks.)
Minor:
- audit doc omission: OsRng->ThreadRng is not strictly equivalent (thread-local ChaCha12, reseeds per 64KiB, not fork-safe; no fork usage in workspace today). Add a line.
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- Process: real CI has never run on this head (all green checks are pull_request_target bot jobs; build-rust/CodeQL/SBOM stuck action_required) and the PR is CONFLICTING on the exact Cargo.toml/lock block being upgraded. Rebase onto main, get workflow runs approved, require genuinely green CI. (Local compensating run at head: cargo test 514/514 pass; code content verified sound incl. dalek3 signing determinism and no seeded-RNG leaks.)
Minor:
- audit doc omission: OsRng->ThreadRng is not strictly equivalent (thread-local ChaCha12, reseeds per 64KiB, not fork-safe; no fork usage in workspace today). Add a line.
762a580 to
a6b4604
Compare
|
Thanks for the review. Rebased onto the current upstream/main (8f7a4c9, 15 commits ahead of the previous base). The rebase applied cleanly with zero conflicts: none of the new main commits touch agent-governance-rust, and GitHub now reports the PR mergeable, so the earlier Cargo.toml/Cargo.lock conflict is resolved. Local validation on the rebased head: cargo test --workspace --locked passes with 0 failures on both a Linux gnu toolchain (516 passed) and a Windows gnu toolchain (515 passed; the one-test delta is a target-gated case, not a failure). This includes the forgery and replay rejection tests (trust::test_verify_peer_rejects_mismatched_claimed_peer, trust::test_verify_peer_rejects_signature_not_created_by_peer, mcp::signing::tests::rejects_replayed_messages), which I re-ran by name to confirm they execute rather than filter out. The --locked flag confirms the lockfile is fully consistent (ed25519-dalek 3.0.0, rand 0.10.2, single rand_core 0.10.1, no git dependencies). Both minor items from your review are in the audit doc: the OsRng to ThreadRng non-equivalence note (thread-local ChaCha12, per-64KiB reseed, not fork-safe, with the caveat that nothing in the workspace forks today) and the corrected getrandom table (0.2.17 + 0.3.4 + 0.4.3 after, matching the lockfile). The remaining gate is on the maintainer side: build-rust, CodeQL, and SBOM sit at action_required until a maintainer approves workflow runs for this fork head. Once approved, real CI will run against the rebased head. |
|
One further hardening since the last update: key generation no longer uses rand's thread RNG. All five key-material call sites (the four Ed25519 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docs/dependency-audits/2026-07-27-ed25519-dalek-3-rand-0.10.md:110
- The verification paragraph claims "514 tests passing" and "identical to the pre-upgrade baseline", but this PR adds new keygen tests (and the PR description reports a higher test count). To avoid leaving an incorrect audit record, update this section to avoid hard-coded counts (or update them to the actual current totals).
Verification: `cargo build`, `cargo test` and `cargo clippy` are green, with
514 tests passing, identical to the pre-upgrade baseline, with no tests
skipped or removed.
Coordinated bump (fixes microsoft#3355). ed25519-dalek 3 rides rand_core 0.9 which rand 0.10 provides, so the two must move together; dependabot's one-at-a-time bumps (microsoft#3269, microsoft#3271) cannot align them. The dalek 2->3 signing/verifying surface used here is source-compatible; the edits are the rand 0.9/0.10 reshuffle: distributions->distr, thread_rng->rng, the Rng extension trait ->RngExt, the RngCore core trait ->Rng, and OsRng (removed) -> rand::rng() (ThreadRng, an infallible CryptoRng, which SigningKey::generate requires and rand 0.10's SysRng -- only TryCryptoRng -- is not). cargo build/test/clippy green: 514 tests pass, unchanged from baseline, including the signature-reject tests (verification still refuses forged and replayed signatures). Signed-off-by: chopmob-cloud <250041792+chopmob-cloud@users.noreply.github.com> Signed-off-by: AlgoVoi <chopmob@gmail.com>
The vendored-patch-audit gate requires a dated audit doc whenever a lockfile changes, and this PR changes agent-governance-rust/Cargo.lock. Records why ed25519-dalek and rand must move together, the full transitive delta, the rand_core/getrandom duplicate collapse, the new digest/sha2 major duplication, and the OsRng -> rand::rng() migration that keeps an infallible CryptoRng for key generation. No CVE is being remediated; this is a compatibility-driven upgrade. Signed-off-by: chopmob-cloud <250041792+chopmob-cloud@users.noreply.github.com> Signed-off-by: AlgoVoi <chopmob@gmail.com>
Signed-off-by: AlgoVoi <chopmob@gmail.com>
Signed-off-by: AlgoVoi <chopmob@gmail.com>
…ence ThreadRng is a thread-local ChaCha12 CSPRNG that reseeds from the OS per 64 KiB of output. Unlike OsRng it is not fork-safe: a child that forks without exec inherits the parent's RNG state. No code in this workspace calls fork directly and neither Tokio nor the test harness uses a forking model, so this is not a current risk. Documenting it so the constraint is visible if a forking process model is introduced later. Signed-off-by: AlgoVoi <chopmob@gmail.com>
Copilot reviewer noted the audit table listed getrandom After as 0.4.2 but the actual lockfile has 0.4.3. On inspection the Before column also omitted the 0.3.4 version that was already present on main before this PR. Corrected changes: - Before: 0.2.17 + 0.3.4 + 0.4.2 (0.3.4 was already present on main) - After: 0.2.17 + 0.3.4 + 0.4.3 (minor patch bump driven by the rand 0.10 upgrade) Prose corrections: - Only rand_core collapses (2->1 version); getrandom stays at 3 versions. - Security bullet updated to match. Signed-off-by: AlgoVoi <chopmob@gmail.com>
…ed pattern Key generation previously used rand's thread RNG (ThreadRng: thread-local ChaCha12, reseeded from the OS per 64 KiB, not fork-safe). ed25519-dalek 3.0.0's SigningKey::generate documentation uses OS entropy directly via the UnwrapErr adapter: UnwrapErr(SysRng). This change adopts that pattern for every key-material path: - AgentIdentity::generate and AgentIdentity::delegate (identity.rs) - Credential::issue and KeyRotationManager::rotate (identity_support.rs) - CredentialVault::generate_key, the AES-256-GCM key (credential_vault.rs) rand::rngs::SysRng (re-export of getrandom 0.4 SysRng, TryCryptoRng with Error = Infallible under UnwrapErr) satisfies the infallible CryptoRng bound of SigningKey::generate via the rand_core blanket impl, so no new dependency is needed and Cargo.lock is unchanged. Thread rng remains only in non-key-material paths: the AES-GCM nonce, generated identifiers (credential, link, chain, incident, violation, report, grant, challenge, sandbox execution ids), the attestation challenge nonce, and the MCP clock nonce. The dependency-audit doc drops the ThreadRng-vs-OsRng caveat and now records the OS-entropy keygen pattern and the surviving thread-rng uses. Adds two tests: distinct usable Ed25519 keys with cross-verification rejection, and distinct non-zero vault keys. Validation: cargo test --workspace --locked green with the GNU host toolchain (378 lib + 101 integration + 36 mcp + 2 doc tests), clippy clean of new warnings. Signed-off-by: AlgoVoi <chopmob@gmail.com>
…cation note Signed-off-by: AlgoVoi <chopmob@gmail.com>
224a878 to
1cd8bfc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (4)
agent-governance-rust/agentmesh/src/identity.rs:83
- Same issue as above:
SigningKey::generate(&mut UnwrapErr(SysRng))borrows a temporaryUnwrapErr(SysRng)as&mut, which is not allowed in this call form. Use a localmutbinding for the RNG adapter.
let signing_key = SigningKey::generate(&mut UnwrapErr(SysRng));
let public_key = signing_key.verifying_key();
agent-governance-rust/agentmesh/src/identity_support.rs:1058
SigningKey::generate(&mut UnwrapErr(SysRng))borrows a temporary RNG adapter as&mutand is likely to fail to compile. Introduce a localmutcsprngbinding and pass&mut csprng.
});
let signing_key = SigningKey::generate(&mut UnwrapErr(SysRng));
let public_key = signing_key.verifying_key();
agent-governance-rust/agentmesh/src/identity.rs:34
SigningKey::generate(&mut UnwrapErr(SysRng))takes a mutable reference to a temporaryUnwrapErr(SysRng)value; Rust does not allow borrowing a temporary as&mutin a function call, so this is likely to fail to compile. Bind the RNG adapter to a localmutvariable first, then pass&mutto that.
This issue also appears on line 82 of the same file.
// OS entropy per the ed25519-dalek 3 documented pattern for key generation.
let signing_key = SigningKey::generate(&mut UnwrapErr(SysRng));
let public_key = signing_key.verifying_key();
agent-governance-rust/agentmesh/src/identity_support.rs:175
SigningKey::generate(&mut UnwrapErr(SysRng))attempts to take&mutto a temporaryUnwrapErr(SysRng)value; this pattern is likely to fail to compile. BindUnwrapErr(SysRng)to a localmutvariable first.
This issue also appears on line 1056 of the same file.
let expires_at_secs = issued_at_secs.saturating_add(ttl_seconds.max(1));
// OS entropy per the ed25519-dalek 3 documented pattern for key generation.
let signing_key = SigningKey::generate(&mut UnwrapErr(SysRng));
let token = URL_SAFE_NO_PAD.encode(signing_key.to_bytes());
These rand-crate identifiers appear in the dalek-3/rand-0.10 dependency audit docs and were missed in the initial spell commit (017d3c2). Signed-off-by: AlgoVoi <chopmob@gmail.com>
1cd8bfc to
aa4d171
Compare
Prayag (prayagupa)
left a comment
There was a problem hiding this comment.
Correct ed25519-dalek 2→3 / rand 0.8→0.10 migration — OsRng → UnwrapErr(SysRng), thread_rng() → rng(), distributions → distr/RngExt — with added tests asserting distinct, verifiable OS-entropy keys. Test Coverage, Security Scan, and Breaking Changes are all green. LGTM.
|
@MohammadHaroonAbuomar following up on imran-siddique's ping from a couple of weeks ago. The PR is rebased onto current main (as of 2026-08-04), all CI checks are green, and GitHub reports it mergeable. Happy to address any questions if something is holding up the review. |
Summary
Upgrades
ed25519-dalek2.2.0 -> 3.0.0andrand0.8.6 -> 0.10.2across theagent-governance-rustworkspace, as requested in #3355.Dependabot #3269 (dalek 3) and #3271 (rand 0.10) each failed
build-rustbecause they bump one crate at a time. The two must move together:ed25519-dalek3 ridesrand_core0.9, whichrand0.10 provides, so bumping either alone leaves arand_coreversion mismatch. This is a single coordinated PR.What actually changed
Bumping both together showed that the
ed25519-dalek2 -> 3 signing/verifying surface used in this crate is source-compatible (SigningKey::generate,Signature::from_bytes,VerifyingKey::from_bytes,Signer/Verifier). All the real edits are therand0.9/0.10 trait and module reshuffle:rand::distributions->rand::distr(clock.rs)rand::thread_rng()->rand::rng()in non-key-material paths (clock.rs, the AES-GCM nonce incredential_vault.rs)Rngextension trait (providingsample_iter) is nowRngExt(clock.rs)RngCorecore trait (providingfill_bytes) is now namedRng(credential_vault.rs)rand::rngs::OsRngwas removed; key generation now usesUnwrapErr(SysRng)(identity.rs,identity_support.rs,credential_vault.rs), see belowNote on key-generation RNG
SigningKey::generateined25519-dalek3 requires an infallibleCryptoRng(pub fn generate<R: CryptoRng + ?Sized>(csprng: &mut R)). The documented pattern in theed25519-dalek3.0.0SigningKey::generatedocs is OS entropy through theUnwrapErradapter:let mut csprng = UnwrapErr(SysRng); SigningKey::generate(&mut csprng).SysRng(thegetrandom0.4 system source re-exported byrand0.10 asrand::rngs::SysRng) implements the fallibleTryCryptoRng, andrand_core'sUnwrapErrwrapper turns it into an infallibleCryptoRngvia the blanket impl, satisfying the bound directly from OS entropy with no new dependency and no lockfile change.Every key-material path draws from
UnwrapErr(SysRng):AgentIdentity::generate/delegate,Credential::issue,KeyRotationManager::rotate, and the vault's AES-256-GCMCredentialVault::generate_key. Thread RNG (rand::rng()) remains only where key material is not involved (the AES-GCM nonce, generated identifiers, the attestation challenge nonce, and the MCP clock nonce). This draws long-lived key material straight from the OS CSPRNG and removes the thread-local reseed and fork-safety caveats from the keygen path. The dependency-audit doc records the split.Validation
agent-governance-rust, from a green baseline:cargo build --workspacecargo test --workspace --lockedcargo clippy --workspace --all-targetsThe PR adds two keygen tests (distinct usable keys with cross-verification rejection; distinct non-zero vault keys), which is the +4 delta over the 514 baseline together with the branch's earlier additions. The suite still includes the signature-rejection tests (
trust::test_verify_peer_rejects_mismatched_claimed_peer,trust::test_verify_peer_rejects_signature_not_created_by_peer,mcp::signing::tests::rejects_replayed_messages), so verification still refuses forged and replayed signatures, not merely accepts valid ones.Fixes #3355.