Skip to content

fix(rcgen)!: KeyPair offers rcgen's KeyPair API - #515

Merged
kixelated merged 11 commits into
mainfrom
quest/m0/adapters/rcgen
Nov 10, 2026
Merged

kixelated merged 11 commits into
mainfrom
quest/m0/adapters/rcgen

Conversation

@kixelated

@kixelated kixelated commented Nov 9, 2026 •

Copy link
Copy Markdown
Collaborator

Completes quest/m0/adapters/rcgen.md: rind_rcgen::KeyPair now offers rcgen::KeyPair's API as rcgen's default ring backend builds it, so code written against rcgen's crypto feature 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); added from_der_and_sign_algo (PKCS#8 only, as on ring) and TryFrom for &[u8], Vec<u8>, &PrivatePkcs8KeyDer, &PrivateKeyDer (tries Ed25519, P-256, P-384, then RSA-SHA256, as rcgen's ring backend does).
  • to_pkcs8() -> Zeroizing<..> -> serialize_der(), serialized_der(), From<KeyPair> for PrivatePkcs8KeyDer and PrivateKeyDer.
  • Added algorithm(), public_key_raw(), is_compatible(), compatible_algs().
  • PEM methods (from_pem, from_pkcs8_pem_and_sign_algo, from_pem_and_sign_algo, public_key_pem, serialize_pem) behind a new pem feature, 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).
  • Removed check(), MissingFeature, serial_number(), key_identifier().

Migration: rename as above; set params.serial_number yourself, and key_identifier_method if 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

  • Added a wipe feature (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.
  • An algorithm rind does not support returns UnsupportedSignatureAlgorithm where rcgen panics (hostile-input rule); listed in the README.
  • Dropped the key-identifier and serial-number tests along with the functions.
  • PEM doesn't go through rcgen's pem crate, whose base64 indexes a table with secret bytes. rind-rcgen/src/pem.rs follows pem 4.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 follow pem's wording, but base64 error details are base64ct's.

Decisions

  • ctgrind pem test (2026-11-09): ✅ mark the PEM public, collect the base64 line spans, then mark only those undefined before decode_vec (rejected: drop the PEM methods and list them as omitted; drop the decode half of the ctgrind test). Implemented in ac0b541.
  • base64ct and pem-rfc7468 ctgrind findings (2026-11-09): ✅ triage, then act per site: a site that reveals only public structure or validity gets a narrow admin/ctgrind.supp entry 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 on rind-rcgen --lib --test ctgrind, debug and --release. Both are clean. The PEM check is now pem::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):

  • base64ct 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 flagged seto; jne overflow checks on 8-bit values held in i16, which can never fire. Fixed by [profile.dev.package.base64ct] overflow-checks = false, as is already done for fiat-crypto.
  • base64ct 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_padding sizes 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.
  • base64ct 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. Under decode_vec it only re-encodes the public final block, so it needs none.
  • pem-rfc7468 trim_end, strip_trailing_eol, its encoder's ASCII fold, and from_utf8 on 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_utf8 on 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 decode entries cover whole functions there. That includes the inlined decode_3bytes, which the release run still checks unsuppressed.

Validation

  • just check passes. cargo test -p rind-rcgen passes with default features and with --no-default-features.
  • cargo test -p rind-rcgen --features wipe --test zeroing passes, and it fails (left: 1, right: 0) with Wiped's drop disabled.
  • ctgrind: clean, as above.

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 review after each round.

  • Reviewer: Codex, gpt-6.1-sol, medium effort
  • Session: 01a1c2d7-4832-7901-99c4-423ef41d2e56, round 5
  • Head: 11d396b
  • Base: origin/main at ceeb088
  • Score: 10/10, The new head merges main and correctly adapts the rcgen integration test to the updated rustls provider API; code inspection found no new issues.
  • Findings: none

Decisions (iterate)

  • rcgen zeroize parity: ✅ zeroize feature with impl Zeroize for KeyPair mirroring rcgen 0.14 (rejected: README bullet only). Public API: new opt-in zeroize feature, additive.

@kixelated kixelated changed the title rind-rcgen's KeyPair is rcgen's fix(rcgen)!: KeyPair offers rcgen's KeyPair API Nov 9, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated and others added 2 commits November 9, 2026 16:23
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>
@kixelated
kixelated marked this pull request as ready for review November 10, 2026 00:36
@coderabbitai

coderabbitai Bot commented Nov 10, 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 23 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: 11115468-249e-4f2e-8a29-53c47d4df90d
Commits

Reviewing files that changed from the base of the PR and between 1546e4f and 11d396b.

Files ignored due to path filters: 1

  • Cargo.lock is excluded by !**/*.lock
Files selected for processing: 15

  • .cargo/mutants.toml
  • .github/workflows/build.yml
  • Cargo.toml
  • MIGRATE.md
  • MIGRATING.md
  • admin/ctgrind.supp
  • quest/m0/adapters/rcgen.md
  • rind-rcgen/Cargo.toml
  • rind-rcgen/README.md
  • rind-rcgen/src/lib.rs
  • rind-rcgen/src/pem.rs
  • rind-rcgen/tests/ctgrind.rs
  • rind-rcgen/tests/rustls.rs
  • rind-rcgen/tests/zeroing.rs
  • rind-str0m/src/dtls.rs
  • 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

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one minor suggestion inline.

Reviewed changes

  • KeyPair rebuilt to rcgen's API: generate()/generate_for(), from_pkcs8_der_and_sign_algo()/from_der_and_sign_algo(), the four TryFrom impls, From<KeyPair> for PrivatePkcs8KeyDer/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.rs porting pem 4.0's framing, whitespace handling, and error strings with base64ct doing the base64; the PEM methods sit behind a default-on pem feature and are differentially tested against rcgen.
  • wipe feature: the retained PKCS#8 copy lives in a Wiped wrapper following rind-aws-lc-rs's precedent, with a scanning-allocator zeroing test wired into build.yml.
  • ctgrind: pem::tests::constant_time marks only the base64 lines secret; admin/ctgrind.supp gains individually justified entries; [profile.dev.package.base64ct] overflow-checks = false with 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.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using glm-5.3 | 𝕏

Comment thread rind-rcgen/README.md Resolved

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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, HKDF Prk::from(okm), and doc work).
  • MIGRATE.md / MIGRATING.md conflict 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 the rind-rcgen::check() references this branch deletes — rind-rcgen users are pointed at rind::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.

Pullfrog  | Fix it ➔ | View workflow run | Using glm-5.3 | 𝕏

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated added this pull request to the merge queue Nov 10, 2026
Merged via the queue into main with commit 77e4572 Nov 10, 2026
8 checks passed
@moq-bot moq-bot Bot mentioned this pull request Nov 10, 2026
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