Repository navigation
fix(rcgen)!: KeyPair offers rcgen's KeyPair API - #515
Conversation
Quest-Claim: 74fe92618061898b
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Triage of every site memcheck flagged in pem-rfc7468 and base64ct found none that branches or indexes on key bits. base64ct's 6-bit transforms are branch-free; the rest test validity, line endings, padding, or that the encoded output is ASCII. The test now keeps the final base64 block public, since base64ct sizes its output from the public `=` padding and memcheck would otherwise taint every length after it. Debug builds turn off base64ct's overflow checks, which branch on bounded secret arithmetic as fiat-crypto's do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…wipe rind-rcgen now frames PEM itself, following pem 4.0's parser: text around the block, any label, RFC 1421 headers, and any wrapping or whitespace, as rcgen accepts. pem-rfc7468 refused all of these. base64ct decodes and encodes the base64. The framing reveals only where delimiters and whitespace sit, so admin/ctgrind.supp covers it narrowly, and the PEM ctgrind check moves into a unit test of the private codec. tests/zeroing.rs checks, as rind-aws-lc-rs's does, that a dropped KeyPair leaves no copy of its PKCS#8 on the heap under `wipe`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou'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 23 minutes. View limit details
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. |
# Conflicts: # MIGRATE.md # MIGRATING.md
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline.
Reviewed changes
KeyPairrebuilt to rcgen's API:generate()/generate_for(),from_pkcs8_der_and_sign_algo()/from_der_and_sign_algo(), the fourTryFromimpls,From<KeyPair>forPrivatePkcs8KeyDer/PrivateKeyDer,algorithm()/public_key_raw()/is_compatible()/compatible_algs(),serialize_der()/serialized_der();check()/MissingFeature/serial_number()/key_identifier()removed.- PEM support: new
rind-rcgen/src/pem.rsportingpem4.0's framing, whitespace handling, and error strings with base64ct doing the base64; the PEM methods sit behind a default-onpemfeature and are differentially tested against rcgen. wipefeature: the retained PKCS#8 copy lives in aWipedwrapper following rind-aws-lc-rs's precedent, with a scanning-allocator zeroing test wired intobuild.yml.- ctgrind:
pem::tests::constant_timemarks only the base64 lines secret;admin/ctgrind.suppgains individually justified entries;[profile.dev.package.base64ct] overflow-checks = falsewith a comment. - Caller and docs: rind-str0m inlines the serial-number and key-identifier helpers it used to import; README, MIGRATING, MIGRATE, mutants config, and Cargo.lock updated to match.
I verified the parity claims against the exact versions this workspace locks (rcgen 0.14.10, pem 4.0.0 sources): the method set and signatures, both From and all four TryFrom impls, the ring-path algorithm order (Ed25519 → P-256 → P-384 → RSA-SHA256), compatible_algs() = once(self.alg), RSA generation returning KeyGenerationUnavailable, and the README's panic-vs-UnsupportedSignatureAlgorithm divergence all match. The PEM framing search — including the partial-match restart-after-failing-byte quirk — whitespace stripping, header rules, and error strings match pem 4.0 byte for byte, and the encoder matches rcgen's ENCODE_CONFIG (64-char wrap, platform-chosen line endings); since from_pem takes &str and every delimiter is ASCII, pem's NotUtf8 rejects are unreachable through this API, so no divergence hides there. I also ran cargo test -p rind-rcgen (default features, --no-default-features, and --features wipe --test zeroing) plus cargo test -p rind-str0m: all pass.
glm-5.3 | 𝕏
There was a problem hiding this comment.
ℹ️ The merge itself is clean — its conflict resolutions are correct and everything still passes; the one minor suggestion from my previous review remains open.
Reviewed changes
- Merge of
origin/main(8e5039c) bringing in 12 commits unrelated to this PR's surface (the rind-aws-lc-rs 1.x API fix, dart mobile CI, PyPI publishing, big-endian s390x qemu testing, HKDFPrk::from(okm), and doc work). MIGRATE.md/MIGRATING.mdconflict resolutions: main had reworded the adapter-crates paragraphs to drop rind-aws-lc-rs (whose API fix now covers its own refusals), and the merge keeps that wording while correctly removing therind-rcgen::check()references this branch deletes — rind-rcgen users are pointed atrind::cpu::check()instead, which matches the post-PR API.
I checked both resolutions against both parents' text, and re-ran cargo test -p rind-rcgen -p rind-str0m on the merged head: all tests pass. The open thread from my previous review — a README bullet noting that rcgen's optional zeroize feature gives its KeyPair a Zeroize impl that rind-rcgen's wipe feature covers only behaviorally — still applies, unchanged by this merge.
glm-5.3 | 𝕏
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Completes
quest/m0/adapters/rcgen.md:rind_rcgen::KeyPairnow offersrcgen::KeyPair's API as rcgen's defaultringbackend builds it, so code written against rcgen'scryptofeature swaps the type in unchanged.Public API (breaking, rind-rcgen)
generate(alg)->generate_for(alg);generate()makes a P-256 key.from_pkcs8(alg, der)->from_pkcs8_der_and_sign_algo(der, alg); addedfrom_der_and_sign_algo(PKCS#8 only, as onring) andTryFromfor&[u8],Vec<u8>,&PrivatePkcs8KeyDer,&PrivateKeyDer(tries Ed25519, P-256, P-384, then RSA-SHA256, as rcgen'sringbackend does).to_pkcs8() -> Zeroizing<..>->serialize_der(),serialized_der(),From<KeyPair> for PrivatePkcs8KeyDerandPrivateKeyDer.algorithm(),public_key_raw(),is_compatible(),compatible_algs().from_pem,from_pkcs8_pem_and_sign_algo,from_pem_and_sign_algo,public_key_pem,serialize_pem) behind a newpemfeature, on by default as rcgen's is. Parsing accepts what rcgen's does, and output matches rcgen's byte for byte (both tested against rcgen).check(),MissingFeature,serial_number(),key_identifier().Migration: rename as above; set
params.serial_numberyourself, andkey_identifier_methodif you need key identifiers (MIGRATING.md shows rcgen's SHA-256 method). rind-str0m, the one in-tree caller, now inlines both.No wire impact.
Choices made without asking
wipefeature (rind/wipe) that wipes the stored PKCS#8 copy on drop, following rind-aws-lc-rs's precedent; without it nothing is wiped, per AGENTS.md. rcgen has no such feature.UnsupportedSignatureAlgorithmwhere rcgen panics (hostile-input rule); listed in the README.pemcrate, whose base64 indexes a table with secret bytes.rind-rcgen/src/pem.rsfollowspem4.0's parser and encoder (any label, RFC 1421 headers, text around the block, any wrapping or Unicode whitespace) and uses base64ct for the base64. The first review round found that the earlier pem-rfc7468 path refused PEM that rcgen accepts, a divergence AGENTS.md counts as a bug. pem-rfc7468 is dropped. Error messages followpem's wording, but base64 error details are base64ct's.Decisions
pemtest (2026-11-09): ✅ mark the PEM public, collect the base64 line spans, then mark only those undefined beforedecode_vec(rejected: drop the PEM methods and list them as omitted; drop the decode half of the ctgrind test). Implemented in ac0b541.admin/ctgrind.suppentry saying why, and a site that branches or indexes on key bits gets a small constant-time base64 in rind-rcgen (rejected: write our own constant-time base64 outright; suppress everything; drop the PEM methods). No site touched key bits, so no base64 of our own was needed. Implemented in 041f6a8. When 66918be replaced pem-rfc7468 with rind-rcgen's own framing, the same rule applied to that code.ctgrind triage
Run under valgrind 3.27.1 with
admin/ctgrind's runner onrind-rcgen --lib --test ctgrind, debug and--release. Both are clean. The PEM check is nowpem::tests::constant_time, a unit test of the private codec on RSA-2048, P-256, P-384, and Ed25519 PKCS#8.Against pem-rfc7468, as first triaged (041f6a8):
encode_3bytes,decode_3bytes(encode_6bits,decode_6bits): no secret branch. Release flags none of their arithmetic. It flagged their addresses only because the output length was tainted (next item). Debug flaggedseto; jneoverflow checks on 8-bit values held ini16, which can never fire. Fixed by[profile.dev.package.base64ct] overflow-checks = false, as is already done for fiat-crypto.decode_padding, and from it nearly every other flag (≈570 reports:memmove,memset, slice splits,chunks_exact,is_finished,validate_last_block's addresses): public padding length.decode_paddingsizes the output by whether the last two characters are=, and memcheck then treats every derived length as secret. Fixed in the test, which keeps the final 4-character block public (it holds the padding). Every other block goes through the same transform. No suppression.Encoding::decode(err == 0): validity only. Suppressed (base64ct-decode-validity, plus-inlined).validate_last_block's compare needed an entry under pem-rfc7468's line-by-line decoder. Underdecode_vecit only re-encodes the public final block, so it needs none.trim_end,strip_trailing_eol, its encoder's ASCII fold, andfrom_utf8on its output: line endings, and output being ASCII. Gone along with pem-rfc7468.rind-rcgen's own framing (66918be):
read_until,skip_whitespace: delimiter positions. Suppressed (rind-rcgen-pem-framing,rind-rcgen-pem-whitespace, plus-inlined), as moq-dev/rind#519 does for rustls-pki-types' PEM line scan.strip_whitespace: whitespace positions (char_indices,is_whitespace, slicing). Suppressed (rind-rcgen-pem-whitespace-strip). It joins the runs between whitespace because a filter over characters, which LLVM compiled branch-free, made memcheck treat the stripped length as secret, and that spread into base64ct and the allocator.str::from_utf8on the data and on encoded lines: the base64 is ASCII. Suppressed (rind-rcgen-pem-utf8,-utf8-index).Trade-off: debug builds have no line tables, so the
decodeentries cover whole functions there. That includes the inlineddecode_3bytes, which the release run still checks unsuppressed.Validation
just checkpasses.cargo test -p rind-rcgenpasses with default features and with--no-default-features.cargo test -p rind-rcgen --features wipe --test zeroingpasses, and it fails (left: 1, right: 0) withWiped's drop disabled.Models
Size: M
Implementer: Claude Code, Opus 5.5, unknown effort
Implementer: Claude Code, claude-opus-5-5, low effort (iterate, S)
Implementer: Claude Code, claude-opus-5-5, medium effort (iterate, M)
(Written by Opus 5.5 with medium)
Review
Rewritten by
quest reviewafter each round.01a1c2d7-4832-7901-99c4-423ef41d2e56, round 5The new head merges main and correctly adapts the rcgen integration test to the updated rustls provider API; code inspection found no new issues.Decisions (iterate)
zeroizeparity: ✅zeroizefeature withimpl Zeroize for KeyPairmirroring rcgen 0.14 (rejected: README bullet only). Public API: new opt-inzeroizefeature, additive.