Skip to content

feat(picky-crypto): add RSA CRT parameter completion - #553

Draft
Benoît Cortier (CBenoit) wants to merge 13 commits into
masterfrom
cbenoit-picky-crypto-rsa-crt
Draft

Benoît Cortier (CBenoit) wants to merge 13 commits into
masterfrom
cbenoit-picky-crypto-rsa-crt

Conversation

@CBenoit

@CBenoit Benoît Cortier (CBenoit) commented Oct 8, 2026 •

Copy link
Copy Markdown
Member

Format decoders such as SSH and PuTTY hold RSA private keys as n, e, d, p and q, while PKCS#8 also needs dP, dQ and qInv.

Add the picky-crypto-rsa-crt crate, which computes these parameters with constant-time crypto-bigint arithmetic and checks that the components are consistent before returning them: p and q are odd and at least 3, p * q = n, q is invertible modulo p, and e * dP and e * dQ are 1 modulo p - 1 and q - 1. Secret intermediates and outputs are zeroized. Known-answer and error tests use published Wycheproof keys.

@CBenoit

Copy link
Copy Markdown
Member Author

Validation and notes for review.

  • Net lines: +491 / -2 across 9 files (src/lib.rs 136, tests 99, vendored key constants 145, README 37, INTENT 24, CI 18).
  • Local checks: cargo fmt --check, cargo clippy --locked --workspace --all-features --all-targets -D warnings, cargo test -p picky-crypto-rsa-crt (23 tests, debug and release), cargo hack check --feature-powerset, cargo +1.85 check -p picky-crypto-rsa-crt.
  • Cargo.lock only gains the new package entry; no version changes.
  • Vectors: testGroups[i].privateKey from C2SP Wycheproof at 12fd3aaf33eb5fa1f52e026912ee00c054f9d984 (1024 to 8192-bit keys, including 2688, 3104 and 4032 bits), copied verbatim and cited per constant. Error cases only substitute whole fields between cited keys.
  • Callers in picky (SSH, PuTTY, generate_rsa) only pass two-prime keys.
  • Not tested: empty n and n above MAX_MODULUS_LEN, since that would require constructed inputs.
  • Limitation: crypto-bigint does not zeroize some internal temporaries (safegcd state, multiplication and division scratch); documented in the README.

Note

LLM-assisted content (no human feedback).

Copilot AI 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.

🟡 Changes recommended

It violates the intent-file policy and does not enforce its advertised modulus-size limit.

3 open findings
What changed in this PR

Adds a no_std crate for deriving and validating RSA CRT parameters using constant-time big-integer arithmetic.

Changes:

  • Implements CRT completion with consistency checks and zeroized outputs.
  • Adds Wycheproof-based tests and documentation.
  • Integrates the crate into the workspace and CI.
File Description
picky-crypto-rsa-crt/​src/​lib.rs Implements CRT completion.
picky-crypto-rsa-crt/​tests/​crt.rs Tests valid and invalid keys.
picky-crypto-rsa-crt/​tests/​support/​vectors.rs Provides Wycheproof vectors.
picky-crypto-rsa-crt/​README.md Documents behavior and limitations.
picky-crypto-rsa-crt/​INTENT.md Adds proposed crate intent.
picky-crypto-rsa-crt/​Cargo.toml Defines crate metadata and dependencies.
Cargo.toml Registers the workspace member.
Cargo.lock Locks the new crate dependencies.
.github/​workflows/​ci.yml Adds MSRV and feature checks.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread picky-crypto-rsa-crt/src/lib.rs Resolved
Comment thread picky-crypto-rsa-crt/INTENT.md Resolved
Comment thread picky-crypto-rsa-crt/tests/crt.rs Resolved

Copilot AI 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.

🟡 Changes recommended

The 16,384-bit boundary logic needs regression coverage for its special 2,049-byte cases.

1 open finding
3 resolved since last review

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread picky-crypto-rsa-crt/src/lib.rs Resolved

Copilot AI 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.

🔵 Needs a closer look

The constant-time and zeroization guarantees are security-critical and warrant final human cryptographic review.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Copilot AI 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.

🔵 Needs a closer look

The constant-time arithmetic and secret-zeroization guarantees warrant final human cryptographic review.

0 open findings

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Copilot AI 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.

🔵 Needs a closer look

Cryptographic arithmetic, constant-time behavior, and zeroization guarantees warrant final human security review.

0 open findings

🧠 Review effort: Balanced

@CBenoit
Benoît Cortier (CBenoit) force-pushed the cbenoit-picky-crypto-rsa-crt branch 2 times, most recently from b7b877c to 8af8e36 Compare October 10, 2026 12:08
@CBenoit
Benoît Cortier (CBenoit) changed the base branch from master to cbenoit-picky-crypto-testsuite October 10, 2026 12:08
@CBenoit
Benoît Cortier (CBenoit) requested a balanced review from Copilot October 10, 2026 12:46

Copilot AI 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.

🔵 Needs a closer look

Constant-time RSA arithmetic and secret-zeroization guarantees warrant final human cryptographic review.

0 open findings

🧠 Review effort: Balanced

Copilot AI 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.

🔵 Needs a closer look

New constant-time cryptographic arithmetic and secret-handling code warrants final human security review despite no confirmed defects.

0 open findings

🧠 Review effort: Balanced

Copilot AI 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.

🔵 Needs a closer look

Cryptographic constant-time and zeroization guarantees warrant final human review despite no concrete defect identified.

0 open findings

🧠 Review effort: Balanced

Copilot AI 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.

🔵 Needs a closer look

The swapped-prime test does not verify the computed coefficient’s defining modular-inverse relation.

0 open findings

Previously missed (1)

In code that hasn't changed since last review

Low severity Test q > p inverse correctness, not just coefficient length

picky-crypto-rsa-crt/​tests/​crt.rs:61

This is the only case exercising q > p, but it checks only the coefficient's length. Since complete_crt_params validates that an inverse exists rather than validating the returned coefficient, an incorrect reduction or inversion result would still pass this test. Verify the defining relation q × qInv ≡ 1 (mod p) for the swapped key.

🧠 Review effort: Balanced

Copilot AI 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.

🔵 Needs a closer look

New constant-time private-key arithmetic and zeroization guarantees warrant final human cryptographic review.

0 open findings

🧠 Review effort: Balanced

@CBenoit
Benoît Cortier (CBenoit) force-pushed the cbenoit-picky-crypto-testsuite branch from 60b60e5 to db6a01f Compare October 10, 2026 15:42
@CBenoit
Benoît Cortier (CBenoit) force-pushed the cbenoit-picky-crypto-rsa-crt branch 2 times, most recently from 2b7c04a to 1cf09e6 Compare October 13, 2026 06:53
Base automatically changed from cbenoit-picky-crypto-testsuite to master October 13, 2026 08:43
Format decoders such as SSH and PuTTY hold RSA private keys as n, e, d,
p and q, while PKCS#8 also needs dP, dQ and qInv.

Add the picky-crypto-rsa-crt crate, which computes these parameters
with constant-time crypto-bigint arithmetic and checks that the
components are consistent before returning them: p and q are odd and
at least 3, p * q = n, q is invertible modulo p, and e * dP and e * dQ
are 1 modulo p - 1 and q - 1. Secret intermediates and outputs are
zeroized. Known-answer and error tests use published Wycheproof keys.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reject 2049-byte moduli whose first byte is not a zero sign byte, and
cover even and small primes with substituted published fields.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Add empty, oversized, zero and one inputs with their specified errors,
align the zeroization limitation with the crate intent, and pin the
cargo-hack version installed in CI.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Load the cited private keys through the conformance suite's Wycheproof
accessor instead of embedding their hexadecimal encodings.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep one known-answer key per modulus and prime alignment class and one
case per check and error variant.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A substituted published prime also breaks p * q = n, which hides the
other checks. Add small structural keys that each fail one check, and
drop the published cases they supersede.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Assert with independent crypto-bigint arithmetic that each hand-written
key fails only its targeted check, and check that an extra leading zero
on p pads dP and qInv byte for byte.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI 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.

🔵 Needs a closer look

The security-sensitive constant-time big-integer implementation warrants final human cryptographic review despite no concrete defect found.

0 open findings

🧠 Review effort: Balanced

…eature check

The crate has no Cargo features, so the MSRV matrix already covers its
only build configuration.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit
Benoît Cortier (CBenoit) marked this pull request as draft October 13, 2026 14:26

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants