chore(sdk): drop the internal DER round-trip on ECDSA signatures (DSPX-3634) - #996
Draft
dmihalcik-virtru wants to merge 1 commit into
Draft
chore(sdk): drop the internal DER round-trip on ECDSA signatures (DSPX-3634)#996dmihalcik-virtru wants to merge 1 commit into
dmihalcik-virtru wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
dmihalcik-virtru
changed the title
…DSPX-3634)
`sign()` converted WebCrypto's raw IEEE P1363 output to DER, and every
consumer immediately converted it straight back:
- `src/auth/dpop.ts` (DPoP proofs) — DER -> P1363
- `tdf3/src/crypto/jwt.ts` (KAS rewrap request token) — DER -> P1363
- `verify()` — DER -> P1363, with `jwt.ts` doing P1363 -> DER first
The DER form never reached the wire, a file, or a public API return
value; it was a detour between two functions in the same module. JWS
requires raw R||S anyway (RFC 7518 §3.4), which is exactly what WebCrypto
already produces.
So `sign()`/`verify()` now speak raw P1363 and the two converters —
`ieeeP1363ToDer` and `derToIeeeP1363` — are deleted along with their unit
tests, removing ~150 lines of hand-rolled ASN.1 parsing.
Two behavioral notes:
- `crypto-service.spec.ts` asserted DER well-formedness for ES512. It
now asserts what actually matters for interop: fixed-width 132-byte
output for P-521.
- A wrong-width signature used to hit the DER parser and throw
`Invalid IEEE P1363 signature: expected 64 bytes...`. WebCrypto just
returns false, so it now surfaces as the ordinary
`Invalid JWT: signature verification failed` — no detail about the
internal encoding.
How to test: `cd lib && npm test`.
Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
dmihalcik-virtru
force-pushed
the
dspx-3397-8-drop-der-roundtrip
branch
from
August 10, 2026 17:47
dd18da5 to
7afd5fe
Compare
|
X-Test Failure Report |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack level 8 of the DSPX-3397 split — the optional tail. Base: #994.
This one is droppable. It is not needed for DPoP nonce support; it's the cleanup DSPX-3634 tracks, made obvious by the audit that produced this stack.
What
sign()converted WebCrypto's raw IEEE P1363 output to DER, and every consumer immediately converted it straight back:src/auth/dpop.ts(DPoP proofs)tdf3/src/crypto/jwt.ts(KAS rewrap request token)verify()jwt.tsdoing P1363 → DER firstThe DER form never reached the wire, a file, or a public API return value — it was a detour between two functions in the same module. JWS requires raw R‖S anyway (RFC 7518 §3.4), which is exactly what WebCrypto already produces.
So
sign()/verify()now speak raw P1363, andieeeP1363ToDer/derToIeeeP1363are deleted along with their unit tests: ~150 lines of hand-rolled ASN.1 parsing gone.Interaction with #988 — please read before merging
This makes #988 moot. #988 hardens the DER parser's bounds checking; this PR deletes that parser and
der-signature.spec.tswith it.#988 is still correct to land first — it protects that code path for as long as the path exists, and it's the right fix if you'd rather not take this refactor. But you should pick one end state:
Behavioral changes
Two, both surfaced by existing tests rather than hidden:
crypto-service.spec.tsasserted DER well-formedness for ES512. It now asserts what actually matters for interop: fixed-width 132-byte output for P-521.Invalid IEEE P1363 signature: expected 64 bytes for ES256, got 63. WebCrypto simply returns false, so it now surfaces as the ordinaryInvalid JWT: signature verification failed. Marginally better — no leak of internal encoding detail.No change to
CryptoService's documented contract: thesign/verifydocs never specified an encoding.How to test
394 mocha passing, 394 karma SUCCESS, lint clean. CLI builds and passes its 32 tests against a freshly packed SDK.
Risk
Touches crypto. Mitigated by the fact that the removed encoding was purely internal — verified by auditing every
cryptoService.sign/verifycaller in the repo (there are three). The JWS conformance suite from #987, which verifies real tokens againstjose.jwtVerify, covers the output format end to end.