Skip to content

fix(keys): name the secp256r1 JWK curve P-256 - #234

Open
erkancamli wants to merge 1 commit into
agentcommercekit:mainfrom
erkancamli:fix/p256-jwk-crv
Open

erkancamli wants to merge 1 commit into
agentcommercekit:mainfrom
erkancamli:fix/p256-jwk-crv

Conversation

@erkancamli

@erkancamli erkancamli commented Sep 28, 2026 •

Copy link
Copy Markdown

What

publicKeyBytesToJwk writes secp256r1 keys as { kty: "EC", crv: "secp256r1" }. JOSE registers this curve as "P-256" (RFC 7518, Section 6.2.1.1, and the JWK Elliptic Curve registry in Section 7.6.2); "secp256r1" is not a registered JWK curve name.

Why it matters

createDidDocumentFromKeypair publishes keys as JWK by default, and did-jwt only matches an EC publicKeyJwk whose crv is "secp256k1" or "P-256". So an ES256 JWT or credential signed by a secp256r1 identity built with ACK fails verification:

Error: invalid_signature: no matching public key found

In the other direction, isJwk rejects standard P-256 JWKs such as the one in RFC 7515, Appendix A.3.1. The skyfire-kya demo already overrides crv: "P-256" by hand for this reason.

Change

  • JwkSecp256r1.crv is "P-256"; the guards, publicKeyBytesToJwk and publicKeyJwkToBytes use it.
  • jwkToKeypair maps "P-256" back to the secp256r1 curve.
  • Changeset: @agentcommercekit/keys minor, since the JWK type changes and JWKs stored with crv: "secp256r1" no longer pass the guards (the changeset says how to migrate them).

Tests

  • vc: a credential signed with ES256 by a secp256r1 did:web issuer (default JWK encoding) now parses; on main it fails with the error above.
  • keys: RFC 7515 A.3.1 key round trip and guard acceptance, rejection of crv: "secp256r1", and a secp256r1 keypairToJwk/jwkToKeypair round trip.
  • The existing public-key.test.ts expectation is updated to P-256 for secp256r1.

pnpm run build and pnpm run check pass locally (format, lint, all package tests).

AI disclosure: I used Claude to help with analysis, code and tests, and reviewed all changes myself.

Summary by CodeRabbit

  • Bug Fixes
    • P-256 keys now use the standard JOSE curve name in JWKs, improving key conversion and compatibility with DID and JWT verification.
    • Existing JWKs using secp256r1 as the curve name must be updated to P-256.

JOSE registers the NIST P-256 curve as "P-256" (RFC 7518, Section 6.2.1.1);
"secp256r1" is not a registered JWK curve name. publicKeyBytesToJwk emitted
crv: "secp256r1", so a DID document built from a secp256r1 keypair (JWK is the
default encoding) carried a key that did-jwt cannot match, and ES256 JWTs and
credentials from that identity failed with "no matching public key found".
The guards also rejected standard P-256 JWKs such as the one in RFC 7515,
Appendix A.3.

The JWK type, guards and conversions now use "P-256", and jwkToKeypair maps it
back to the secp256r1 curve.
@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: fb349a51-52e6-4310-b905-a95f19b93524

📥 Commits

Reviewing files that changed from the base of the PR and between 54e763c and 169c65f.

📒 Files selected for processing (7)
  • .changeset/p256-jwk-curve-name.md
  • packages/keys/src/encoding/jwk.test.ts
  • packages/keys/src/encoding/jwk.ts
  • packages/keys/src/keypair.test.ts
  • packages/keys/src/keypair.ts
  • packages/keys/src/public-key.test.ts
  • packages/vc/src/verification/parse-jwt-credential.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.


Walkthrough

The keys package now uses the JOSE curve name P-256 for secp256r1 JWKs. Conversion functions map between P-256 and the internal secp256r1 curve name. Tests cover JWK and keypair round trips, validation, and JWT credential parsing.

Changes

P-256 JWK conversion

Layer / File(s) Summary
JWK contract and byte conversion
packages/keys/src/encoding/jwk.ts, packages/keys/src/encoding/jwk.test.ts, packages/keys/src/public-key.test.ts, .changeset/p256-jwk-curve-name.md
The secp256r1 JWK type, validators, and byte conversions use P-256. Tests cover conversion round trips, accepted and rejected curve names, and expected JWK output. The changeset documents the curve-name migration.
Keypair and credential integration
packages/keys/src/keypair.ts, packages/keys/src/keypair.test.ts, packages/vc/src/verification/parse-jwt-credential.test.ts
Keypair conversion maps P-256 to the internal secp256r1 name. Tests cover keypair round trips and parsing a credential signed with a key published as a JWK.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: venables

Merge Risk: ⚪ Minimal · up to 169c6

The curve-name migration has no identified issue requiring a fix before merge. Stored JWKs using the old name still need the migration described in the changeset.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 169c6

The new P-256 representation fixes interoperability, but it is not compatible with keys saved under the old curve name. Deployments that retain those keys or roll back a package version may need a coordinated data change to keep signing available. No new signature-verification bypass was identified.

Retained concerns

  • Medium · reliability · inferred: Old and new secp256r1 JWK representations are not mutually readable. Where stored keys or mixed package versions exist, an upgrade or rollback can interrupt key loading and signing unless the curve label is migrated in coordination with readers.
Security review details

Security Blast Radius

  • inferred — Reachability changes at the public JWK input and output contract: consumers can accept standard P-256 JWKs and publish secp256r1 keys under that name. The inspected paths do not show a new key class, privilege, or cross-package authority being granted.

Trust Boundaries and Controls

  • observed — Incoming JWKs remain subject to exact curve and key-shape checks. Downstream credential parsing still calls signature verification and rejects an issuer identity that differs from the verified JWT issuer.

Resilience and Maintainability Implications

  • inferred — If an installation retains old-format private JWKs, strict curve-name replacement can prevent that installation from reconstructing signing identities after upgrade; a rollback can conversely encounter newly written P-256 keys. Actual storage and deployment exposure are unverified.

Hardening Proposals

  • proposed — For deployments with persisted JWKs, define an idempotent relabeling and coordinated reader-upgrade and rollback plan before replacing the stored representation.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: updating the secp256r1 JWK curve name to the JOSE-standard "P-256".
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 6 files. (1 skipped: 1 …
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant