Repository navigation
fix(pq): clear SHAKE state on drop - #361
Conversation
- Enable shake/zeroize from the experimental-pq feature, so a downstream crate gets the clearing without asking for it - Qualify the feature on an isolated consumer, because dev-dependencies enable it in every workspace build - Build experimental-pq without std, which the feature table promises and a missing import broke - Stop ML-KEM tests from assuming a randomness source a build without signing does not have, and pin the refusal such a build answers with Closes #292
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Enable and check SHAKE state clearing Cargo.toml, tests/shake_state_clearing.rs, docs/ml-dsa-patch.md |
The optional shake dependency enables zeroize. The experimental-PQ test checks ZeroizeOnDrop for Shake128 and Shake256, and checks that their reader types need drop. The documentation limits its claims to the dependency-owned state and cursor. |
Qualify the downstream feature graph tools/xml-sec-tools/src/check_features.rs, tools/xml-sec-tools/src/check_features/tests.rs, .github/workflows/ci.yml |
The experimental profile checks isolated consumer metadata for shake/zeroize and runs the state-clearing test. Tests cover missing packages, missing features, invalid metadata, and a valid feature graph. The alloc-only job adds a Clippy check for the experimental-PQ feature and roxmltree backend. |
Key-encapsulation test coverage
| Layer / File(s) | Summary |
|---|---|
Check encapsulation feature behavior src/provider/key_encapsulation/rustcrypto.rs, src/key_establishment.rs |
Tests check that encapsulation returns Unsupported without xmldsig-sign and gate tests that require that feature. FailedRandom now advertises random and encapsulation support, and the allowance-exhaustion retry uses it. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Possibly related PRs
- structured-world/xml-sec#269: Isolates Cargo feature profiles from workspace feature unification; this change applies downstream feature-graph checking to
shake/zeroize. - structured-world/xml-sec#320: Changes when ML-KEM encapsulation is supported; this change updates related key-encapsulation feature tests.
Merge Risk: 
b478d
-
SHAKE state clearing is enabled for experimental PQ. No merge-blocking risk was identified.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies the main change: enabling SHAKE state clearing on drop for experimental post-quantum support. Linked Issues check Issue #292is directly linked and active.Cargo.tomlenablesshake/zeroizeon the existing optional dependency withdefault-features = false. The newshake_state_clearingtarget asserts `Zeroi…Out of Scope Changes check The changed files support issue #292. The ML-KEMalloc::vec::Vecimport and test gating keep experimental-PQ and no_std checks valid. The CI lint adds coverage for the required bare-metal configurat…Full details: Linked Issues check
-
Explanation
Issue
#292is directly linked and active.Cargo.tomlenablesshake/zeroizeon the existing optional dependency withdefault-features = false. The newshake_state_clearingtarget assertsZeroizeOnDropforShake128andShake256and checks that both reader types require drop.check-features experimentalnow creates an out-of-workspace consumer without a direct SHAKE dependency and rejects metadata that lacksshake/zeroize. The summary reports that the required tests, feature checks, no_std lint, and supported configurations pass. The ML-DSA documentation keeps the assurance limited to dependency-owned state clearing.
Full details: Out of Scope Changes check
-
Explanation
The changed files support issue
#292. The ML-KEMalloc::vec::Vecimport and test gating keep experimental-PQ and no_std checks valid. The CI lint adds coverage for the required bare-metal configuration. The documentation states the required limits on secret-erasure claims. The downstream feature-graph code and its tests implement the issue regression. No unrelated product behavior, cryptographic implementation, or signature format change is shown.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 7 functions across 5 files. (3 skipped: 3 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Summary
experimental-pqenablesshake/zeroize, soShakeandShakeReaderclear their state and cursor on drop in a downstream build that does not ask for it.experimental-pqwith one XML backend builds withoutstd, as the feature table states.Changes
check-features experimentalresolves a throwaway consumer outside the workspace and requiresshake/zeroizein its graph. A workspace build cannot show this, because dev-dependencies enable the same feature.tests/shake_state_clearing.rsrequiresZeroizeOnDropfor both hashers.Vecfromalloc.no_stdconfiguration on a bare-metal target.Testing
Tests, lints, formatting, the feature profile and the dependency audit pass on macOS, including the
no_stdconfiguration; CI covers Linux and Windows.Closes #292
Summary by CodeRabbit