Skip to content

fix(keys): accept conformant PKCS#8, SEC1, and PBES2 inputs - #85

Merged
kjanat merged 6 commits into
masterfrom
fix/key-parse-tolerance
Jul 24, 2026
Merged

fix(keys): accept conformant PKCS#8, SEC1, and PBES2 inputs#85
kjanat merged 6 commits into
masterfrom
fix/key-parse-tolerance

Conversation

@kjanat

@kjanat kjanat commented Jul 23, 2026

Copy link
Copy Markdown
Owner

What

Three key parsers rejected inputs that OpenSSL and Node produce or that the spec allows.

PKCS#8 with both optionals (RFC 5958 §2 / RFC 8410 §7)

OneAsymmetricKey has attributes [0] and publicKey [1], so a v2 key carrying both has five elements. parsePkcs8PrivateKey capped at four, so such a key returned malformed. It now validates the tail structurally: attributes [0] as a constructed SET OF at most once, an optional primitive publicKey [1] BIT STRING after it with octet-aligned content, the RFC 5958 version coupled to the public key's presence (v2 iff present), and well-formed unknown extension additions tolerated per X.680 extensibility.

A conformant five-element key still fails to import under Bun 1.3.14 — a Bun WebCrypto bug (oven-sh/bun#35432, fixed by oven-sh/bun#35433) that treated every trailing RFC 5958 field as seed bytes. micro509 forwards the conformant envelope unchanged rather than rewriting it; the gap is tracked by an unconditional test.failing canary that turns red once a Bun release carries the fix.

SEC1 ECPrivateKey version (RFC 5915 §3)

version SHALL be ... one (1). parseSec1PrivateKey checked only the tag, so version 0 or 2 passed and rejection was deferred to WebCrypto as Malformed PKCS#8 private key. It now compares the content octets.

PBES2 salt length (RFC 8018 §4.1)

The eight-octet minimum is a "should" for salt selection, and the salt "need not be checked for a particular format by the party receiving the salt". The decrypt path threw RangeError on a shorter salt, so openssl pkcs8 -topk8 -v2 aes-256-cbc -saltlen 4 could not be decrypted. The check is removed on decode; the encrypt path keeps the >= 8 minimum.

Tests

TDD: a version-2 SEC1 rejects as Malformed SEC 1; a four-byte PBES2 salt decodes. A real five-element Ed25519 OneAsymmetricKey imports (behind test.failing until the Bun fix reaches a release), and a six-case matrix asserts malformed tails reject: primitive attributes [0], constructed publicKey [1], duplicate and reordered fields, a version/public-key mismatch, and an empty public-key BIT STRING.

parsePkcs8PrivateKey accepts a five-element OneAsymmetricKey carrying
both attributes [0] and publicKey [1] (RFC 5958/8410), bounding the tail
by ASN.1 class instead of a count. parseSec1PrivateKey rejects a version
other than 1 (RFC 5915 §3) instead of deferring to WebCrypto. PBES2
decrypt drops the >=8-byte salt check (RFC 8018 §4.1 does not require
it); the encrypt path keeps the minimum.
@kjanat kjanat added bug Something isn't working rfc-conformance RFC adherence and conformance evidence labels Jul 23, 2026
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The change tightens PKCS#8 OneAsymmetricKey and SEC1 ECPrivateKey DER validation, including version, optional-field, ordering, duplication, and BIT STRING checks. PBES2 decoding now permits short PBKDF2 salts while rejecting invalid iteration counts. Regression tests cover malformed key encodings and short salts. RFC 2985, 5915, 5958, 8018, and 8410 texts, plus changelog entries, are added.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • kjanat/micro509#39: Also modifies PBES2 parsing helpers in parsePbes2AlgorithmIdentifier.

Suggested labels: tests, pkcs

Poem

Arrr, malformed DER now walks the plank,
With tidy versions lined in rank.
Short salts sail, bad counts sink,
RFCs fill the library’s link.
Clean keys gleam in the moonlit wake!

🚥 Pre-merge checks | ✅ 7 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Semver Version Bump Validation ⚠️ Warning Source files changed, but package.json stayed at 0.13.0 from base to head; no version bump was made. Bump the version field in package.json (PATCH for these bug fixes) and, if needed, mirror it in any other version file.
✅ Passed checks (7 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 30.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Changelog Update ✅ Passed Source changes are listed under Unreleased/Fixed in CHANGELOG.md, and no version bump is present vs origin/master.
Agents.Md Documentation Updated ✅ Passed AGENTS.md files exist, but the only actual code change is a test rename in test/keys.test.ts, not architecture/workflow/CLI/agent logic, so no AGENTS update is required.
Title check ✅ Passed The title uses a valid fix prefix and accurately reflects the PKCS#8, SEC1, and PBES2 parser fixes.
Description check ✅ Passed The description clearly matches the parser fixes and added tests in the changeset.

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 23, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
micro509 fe9819c Commit Preview URL

Branch Preview URL
Jul 24 2026, 02:42 PM

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.00000% with 2 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/keys/keys.ts 95.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@kjanat kjanat self-assigned this Jul 24, 2026
@kjanat kjanat added the cr:review Allow CodeRabbit review label Jul 24, 2026
Resolve CHANGELOG [Unreleased]: fold #85's key-parse-tolerance Fixed
entries in above master's Changed/Fixed blocks. keys.ts and pbes2.ts
auto-merged; #85's decrypt-path salt tolerance and version-tag checks
coexist with master's #83 PBKDF2 prf-DEFAULT logic, and its internal
bare-throw parse helpers still surface as malformed Results at the import
boundary.
@coderabbitai coderabbitai Bot added pkcs PKCS#7 / PKCS#12 workflows tests Test coverage and harnesses labels Jul 24, 2026
coderabbitai[bot]

This comment was marked as resolved.

kjanat added 2 commits July 24, 2026 15:51
Track Bun's rejection of v2 Ed25519 OneAsymmetricKey values carrying
attributes and publicKey. Vendor the related PKCS #9, HELD, and
EdDSA/XDH RFC references used for standards work.
The class-only tail gate accepted malformed five-plus-field PKCS#8: a
primitive attributes [0], a constructed publicKey [1], duplicates,
reordering, and any version/public-key pairing, leaving acceptance to the
WebCrypto backend. Parse the tail statefully instead: attributes [0] as a
constructed SET OF at most once, an optional primitive publicKey [1] BIT
STRING after it with octet-aligned content, the RFC 5958 version coupled
to the public key's presence, and well-formed unknown extension additions
tolerated per X.680 extensibility. A conformant v2 key still parses; the
Bun import gap is upstream (oven-sh/bun#35432) and stays behind the
test.failing canary.
@gitguardian

This comment was marked as low quality.

@pkg-pr-new

pkg-pr-new Bot commented Jul 24, 2026

Copy link
Copy Markdown
  • micro509-vite-demo

    npm i https://pkg.pr.new/micro509@85
    
    pnpm add https://pkg.pr.new/micro509@85
    
    bun add https://pkg.pr.new/micro509@85
    

commit: 02fd2a4

coderabbitai[bot]

This comment was marked as resolved.

kjanat added 2 commits July 24, 2026 16:38
rfc5985.txt held RFC 5985 (HTTP-Enabled Location Delivery), a 5958 digit
transposition. Replace it with RFC 5958 (Asymmetric Key Packages), and add
RFC 5915 (Elliptic Curve Private Key Structure) and RFC 8018 (PKCS#5 PBES2)
so the vendored set matches the OneAsymmetricKey, SEC1, and PBES2 citations.
The loop ran six malformed OneAsymmetricKey tails through one assertion;
a failure named none of them. Rethrow with the case name as the message
and the assertion error as the cause.
@kjanat
kjanat merged commit 8d20b0f into master Jul 24, 2026
23 of 24 checks passed
@kjanat
kjanat deleted the fix/key-parse-tolerance branch July 24, 2026 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working cr:review Allow CodeRabbit review pkcs PKCS#7 / PKCS#12 workflows rfc-conformance RFC adherence and conformance evidence tests Test coverage and harnesses

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant