fix(security): rotation JWT compliance (#25, #26)#39
Merged
Conversation
Resolves two Low-severity audit findings on the from_prior rotation path. Both passed a PoC-backed adversarial red-team pass (AGENTS.md §2). Full suite 625 green; warnings-as-errors clean. - #25 FromPriorBuilder now emits exp/nbf (FR-ROT-05). It previously serialized only {iat,iss,sub}, discarding FromPriorClaims.Exp/Nbf, so every self-issued rotation JWT was non-expiring and the validator-side freshness control could never fire. Claims are built via an insertion-ordered JsonObject (exp,iat,iss,nbf,sub), emitting exp/nbf only when set (no-expiry payload is byte-identical). New BuildAsync(claims, signerJwk, TimeSpan lifetime) overload sets exp = iat + lifetime (rejects a sub-second lifetime — red-team: avoids a silent exp==iat already-expired token). PRD rotation cookbook/API-matrix realigned to the real API (BuildAsync + WithFromPrior); the never-built WithDidRotation helper is noted as future DX. - #26 from_prior validator now rejects a crit protected header (RFC 7515 §4.1.11) with ProtocolException, before signature verification — parity with JwsParser/JweParser, which previously the hand-rolled from_prior parse omitted. Closes #25, closes #26 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The plan promised an end-to-end test for a future nbf being rejected on
unpack (FR-ROT-05); only the expired-Exp case was present. Adds
DidCommClient_RejectsNotYetValidFromPrior_FrRot05: pins the clock to iat so
nbf = iat + 1 day is deterministically not-yet-valid (the literal "Nbf = iat +
86400" with the 2023 iat would otherwise be in the past under the real clock),
packs authcrypted, and asserts UnpackAsync throws ConsistencyException
("not yet valid"). Full suite 626 green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Good catch — added in 765b91f: One subtlety worth flagging on the suggested var pinnedNow = DateTimeOffset.FromUnixTimeSeconds(iat);
var claims = new FromPriorClaims(Sub: "did:example:alice", Iss: PriorDid, Iat: iat, Nbf: iat + 86400);
// … pack authcrypted …
var client = new DidCommClient(secrets, keyService, new DidCommOptions { Clock = () => pinnedNow });
await act.Should().ThrowAsync<ConsistencyException>().Where(e => e.Message.Contains("not yet valid"));Both freshness directions are now covered end-to-end (expired |
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
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.
Summary
Resolves two Low-severity audit findings on the
from_priorrotation path (#25, #26). Both passed a PoC-backed adversarial red-team pass (AGENTS.md §2). Full suite: 625 tests green; warnings-as-errors clean.Fixes
FromPriorBuilder.BuildAsyncpreviously serialized only{iat, iss, sub}, silently discardingFromPriorClaims.Exp/Nbf— so every self-issued rotation JWT was non-expiring and the validator-side FR-ROT-05 freshness control could never fire. Claims are now built via an insertion-orderedJsonObject(exp, iat, iss, nbf, sub), emittingexp/nbfonly when set (no-expiry payload byte-identical). NewBuildAsync(claims, signerJwk, TimeSpan lifetime)overload setsexp = iat + lifetimeso issuing a freshness-bounded token is one call.FromPriorValidatorhand-parses the JWS header and previously read onlyalg/kid, silently ignoringcrit— unlikeJwsParser/JweParser, which fail closed (RFC 7515 §4.1.11). It now rejects anycritmember withProtocolException, before signature verification.The validator/enforcement side already worked (
ValidateFromPriorFreshnessenforcesexp/nbfwith clock skew) — so these are narrow, sender-side and header-parse gaps.Decision applied
The PRD's rotation cookbook (§N) and API matrix referenced a
.WithDidRotation(...)helper that was never built (the real API isFromPriorBuilder.BuildAsync+MessageBuilder.WithFromPrior). Per the maintainer's call, the PRD example is realigned to the actual API;WithDidRotationis noted as possible future DX rather than built.Adversarial red-team pass (AGENTS.md §2) — both hold
TimeSpan.MaxValue.TotalSeconds≈ 9.2e11 ≪longoverflow), and a forced wrap fails closed at the receiver (negativeexp→ always expired). No JSON injection (JsonObjectescapes); round-trip type-correct; freshness boundary math clean (negative skew tightens, never loosens). Remediated the one footgun found: a sub-secondlifetimefloored toexp == iat(already-expired) — the overload now rejects< 1s.\u-partial / duplicate(→malformed) / empty / null / wrong-type variants; the only "accepted" inputs are non-normative member names (Crit,crit, nested) no JOSE peer honors as crit. Runs before signature verify;ProtocolExceptionpropagates uncaught.Test plan
exp,iat,iss,nbf,suborder and the byte-identical no-exp path; the lifetime overload setsexpcorrectly and rejects≤ 0/ sub-second; end-to-end a self-issued token withexpin the past is rejected onUnpackAsync(FR-ROT-05).crit-bearingfrom_prior(spliced without re-signing) →ProtocolException.Closes #25, closes #26
🤖 Generated with Claude Code