Skip to content

feat: wiping keys on drop is opt-in, under a wipe feature - #461

Merged
kixelated merged 4 commits into
mainfrom
quest/m2/speed/wipe-feature
Nov 7, 2026
Merged

kixelated merged 4 commits into
mainfrom
quest/m2/speed/wipe-feature

Conversation

@kixelated

@kixelated kixelated commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator

Finishes quest/m2/speed/wipe-feature.md and deletes it.

Summary

rind now wipes keys, keyed state, and heap scratch derived from a private key only under a new additive wipe Cargo feature. The default build skips the wipe, as the maintainer decided on 2026-11-07 (see the quest's Plan).

  • rind/Cargo.toml gains wipe = [].
  • low::zeroise and zeroise_value wrap their zero_bytes call in if cfg!(feature = "wipe"). Every Drop that only wipes compiles away, and the Drop impls are unchanged. cfg! rather than #[cfg] keeps zero_bytes and every wipe site type-checked in the default build, so nothing in it goes dead.
  • The scope is today's: every wipe already routes through those two functions, so nothing else changes.
  • Tests: rind/tests/zeroing.rs is gated on feature = "wipe". The unit test that checked zeroise now calls zero_bytes directly, so it keeps running in every build. Without the feature, all 12 zeroing tests fail (checked locally by dropping the gate), so the feature does take the wipe out.
  • CI: the gate (sh/dispatch.sh check:rs) runs cargo nextest run -p rind --features wipe --test zeroing whenever rind is selected, as the zeroing tests ran in the gate before. Nightly build.yml adds the same test on every OS (macOS and the aarch64 runners cover aarch64's key layouts), and sh/armv7.sh runs it on both armv7 paths.
  • Docs: AGENTS.md's Required wipe rule (approved in the quest), REVIEW.md's checklist item, ARCHITECTURE.md, proofs/README.md, MIGRATING.md (a new "Across the API" row tells aws-lc-rs users to enable rind/wipe), and the README's new "Wiping keys" section, which says adapters don't forward the feature. Each "wiped on drop" doc comment in rind and rind-ring now says "under the wipe feature".

Impact

  • Public API: a new wipe feature. No signature changes.
  • Behavior: the default build no longer zeroes keys, keyed state, or RSA's private-key heap scratch on drop. A consumer who wants the wipe enables rind/wipe, and Cargo's feature unification turns it on beneath every adapter. Following moq-dev/rind#394, which also removed wipes, the title carries no !, since no code stops compiling.
  • Adapters: none forwards wipe. rind-rcgen's to_pkcs8 still returns a zeroize::Zeroizing buffer, as it did before; see Choices.

Benchmarks

A one-off A/B dispatch (run, from the scratch branch bench-probe/wipe-feature) built main (ff503970, always wipes) and this change (aff51fd5, default build, no wipe) on each runner and ran them alternately, main, head, main, head, so each row compares the same CPU. aws-lc-rs ran in the same runs. Each cell is the minimum of the two rounds.

benchmark EPYC 9V74 head / main head / aws-lc-rs Neoverse-N2 head / main head / aws-lc-rs
hmac-sha256/32B (one-shot) 0.984 0.36 0.971 0.46
hmac-sha256/64B (one-shot) 0.987 0.42 0.975 0.50
hmac-sha1/32B (one-shot) 0.983 0.47 0.973 0.48
hmac-sha384/32B (one-shot) 1.000 0.59 0.985 0.71
hmac-sha256/reused-key/32B (no drop in the loop) 1.000 0.33 1.000 0.44
hkdf-sha256/expand-aes-256-gcm-key 0.940 0.59 0.964 0.62
hkdf-sha384/expand-aes-256-gcm-key 0.965 0.43 0.977 0.57
hkdf-sha256/extract-expand 0.993 0.63 0.982 0.68
hkdf-sha384/extract-expand 0.998 0.67 0.980 0.85
x25519-ecdh 1.000 1.00 1.000 0.81
p256-ecdh 1.002 0.96 1.001 0.77
ed25519-sign 0.999 0.93 1.002 0.73
mlkem768-decaps 0.994 0.44 1.000 0.50
rustls-full-handshake-group/x25519 0.996 0.81 1.004 0.86
rustls-full-handshake-key/ecdsa-p256 0.998 0.81 1.007 0.86
rustls-resumed-handshake/x25519 0.988 0.61 1.000 0.70
rustls-resumed-handshake/x25519mlkem768 0.986 0.60 0.999 0.73

The saving is where a key is built and dropped around little work: one-shot HMAC on short messages is 1.3 to 2.9% faster, and HKDF expanding into an AES-256-GCM key, which builds and drops the AEAD key with its GHASH table, is 2.3 to 6% faster. Reused-key HMAC, which drops nothing in its loop, is unchanged, as expected. ECDH, signing, ML-KEM, and full rustls handshakes move by under 1%, inside the runners' noise (the arm runner's handshake rows drift +0.4 to 0.7% with no wipe left to cost anything). rind's standing against aws-lc-rs is unchanged on every row; the HMAC and HKDF rows widen a lead it already held. The full tables are in the run's step summaries.

Choices made without asking

  1. cfg! inside zeroise, not #[cfg] on zero_bytes. The default build still type-checks every wipe site and keeps zero_bytes's unit test, with no cfg sprinkled over the per-arch cpu.rs files. The optimizer drops the dead branch.
  2. Where CI turns wipe on. In the gate, only the zeroing test binary is built with --features wipe, a second rind build when rind changes. The zeroing tests were in the gate before, and a key type that misses its wipe is a REVIEW.md finding, so leaving them to nightly alone would weaken the gate. Nightly adds macOS, Windows (where the test is compiled out), aarch64, and armv7 coverage.
  3. No ! in the title, per moq-dev/rind#394's precedent for removing wipes. release-plz therefore bumps rind as a feature, not a breaking change.
  4. rind-rcgen's Zeroizing stays. to_pkcs8 returns Zeroizing<PrivatePkcs8KeyDer>; dropping it changes rind-rcgen's public return type, and key export is a cold path. The quest scopes the feature to rind.

Validation

  • just check on aff51fd5 plus the MIGRATING.md edit (b8045cbf, docs only): every module passes (rs, msrv, cross, deny, versions, license, inplace, compat, bench, fixtures, patch, quest, gh, hooks, shell, nix): 625 tests, then the 12 zeroing tests under --features wipe.
  • cargo clippy -p rind --all-targets with and without --features wipe: clean.
  • Negative check: with the zeroing tests' wipe gate removed, the default build fails all 12, so the default build really skips the wipe.
  • just check on 357895b7, the merge of main (165545ef): every module passes again, 625 tests, then the 12 zeroing tests under --features wipe.
  • Not run locally: just armv7 and the nightly build.yml matrix, which this PR extends.

Caused by

None: this is a new maintainer decision, not a fix.

Suggested follow-ups

  • rind-rcgen's always-on Zeroizing on to_pkcs8: drop. It costs nothing measurable on a cold export path, and changing it is a breaking change in rind-rcgen for consistency alone.

Models

Size: S
Implementer: Claude Code, claude-opus-5-5, high effort

Review

Rewritten by quest review after each round.

  • Reviewer: Codex, gpt-6.1-sol, medium effort
  • Session: 01a1b4de-0187-72f3-b34a-9c1a2ad13436, round 1
  • Head: b8045cb
  • Base: origin/main at 6dbdebd
  • Score: 10/10, The opt-in wipe behavior matches the recorded maintainer decision, consistently gates existing zeroisation helpers, and retains dedicated CI coverage; tests were not run in the read-only environment.
  • Findings: none

Merge of main at 357895b7: main added no .rs change since ff503970, so it brought no new wipe site to gate. The only overlap, quest/m2/speed/README.md, resolved without conflict (main appended a Required link after the line this PR removes), and git diff origin/main...357895b7 matches git diff ff503970...b8045cbf apart from that hunk's context. As a conflict-free merge from the base, it needs no new review round; the round above covers the PR's changes.

(Written by Claude Opus 5.5 with high)

kixelated and others added 3 commits November 6, 2026 23:03
`low::zeroise` and `zeroise_value` do nothing without `wipe`, so every
Drop that only wipes compiles away. `cfg!` keeps `zero_bytes` compiled
and its unit test running in every build. The zeroing tests run under
the feature in the gate, nightly on each OS, and on armv7's two paths.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title Wiping keys on drop is opt-in feat: wiping keys on drop is opt-in, under a wipe feature Nov 7, 2026
@kixelated
kixelated marked this pull request as ready for review November 7, 2026 08:03
@pullfrog

pullfrog Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

Your OpenCode Go account is out of credit. Top it up, then re-trigger Pullfrog.

OpenCode Go's response
provider error: Insufficient account funds

Top up OpenCode Go →

Pullfrog  | Rerun failed job ➔ | View workflow run | via Pullfrog | Using glm-5.3

@coderabbitai

coderabbitai Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 58 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a9c0bdae-3708-485d-a333-f02a30e8dbba
Commits

Reviewing files that changed from the base of the PR and between 165545e and 357895b.

Files selected for processing: 30

  • .github/workflows/build.yml
  • AGENTS.md
  • ARCHITECTURE.md
  • MIGRATING.md
  • README.md
  • REVIEW.md
  • proofs/README.md
  • quest/m2/speed/README.md
  • quest/m2/speed/wipe-feature.md
  • quest/m4/fips-scope.md
  • rind-ring/src/aead.rs
  • rind-ring/src/aead/quic.rs
  • rind-ring/src/agreement.rs
  • rind-ring/src/hmac.rs
  • rind-ring/src/pkcs8.rs
  • rind/Cargo.toml
  • rind/src/aead.rs
  • rind/src/aead/gcm.rs
  • rind/src/aead/quic.rs
  • rind/src/high/agreement.rs
  • rind/src/high/hkdf.rs
  • rind/src/high/hmac.rs
  • rind/src/high/kem.rs
  • rind/src/high/pbkdf2.rs
  • rind/src/high/signature.rs
  • rind/src/low/generic/zeroise.rs
  • rind/src/low/tests.rs
  • rind/tests/zeroing.rs
  • sh/armv7.sh
  • sh/dispatch.sh
  • 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.

Share

@kixelated
kixelated added this pull request to the merge queue Nov 7, 2026
kixelated added a commit that referenced this pull request Nov 7, 2026
The maintainer kept the panics behind snow's infallible `Cipher::encrypt`
and `Hash::result`; each method now says when it panics. The adapter wipes
nothing itself, so the scalar comment no longer promises an unconditional
wipe that #461 makes opt-in.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merged via the queue into main with commit be2cf06 Nov 7, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant