diff --git a/CHANGELOG.md b/CHANGELOG.md index d53a37c..ddd383d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,19 @@ All notable changes to didcomm-dotnet are documented here. Format follows ## [Unreleased] +### Added — fail-closed skid guard on authenticated decrypt (defense in depth, #52) + +Closes **#52**. The authcrypt unpack branch trusted the JOSE layer's definitional invariant +`IsAuthenticated ⟺ non-empty skid` without asserting it; had that invariant ever regressed +upstream, a null skid would have silently skipped the FR-CONSIST-01 `from`↔`skid` binding on an +envelope still reported authenticated (and an empty skid would have escaped as a raw +`ArgumentException`, off the FR-API-07 contract). `EnvelopeReader` now asserts a surfaced skid +whenever a decrypt is authenticated, rejecting with `ConsistencyException` — parity with the JWS +branch's empty-signer-kid guard. The check lives in `AddressingConsistency` as +`CheckAuthcryptSkidSurfaced` so it is directly unit-tested; it is unreachable through today's +parser, so no current input changes behavior. The JWS branch's `CryptoException` guard is +deliberately untouched. + ### Changed — lazy UTF-8 size on the inbound snapshot (perf, #53) Closes **#53**. Every successful `UnpackAsync` registers an `InboundMessageSnapshot`, but only the diff --git a/src/DidComm.Core/Composition/EnvelopeReader.cs b/src/DidComm.Core/Composition/EnvelopeReader.cs index b69beb4..fc169d4 100644 --- a/src/DidComm.Core/Composition/EnvelopeReader.cs +++ b/src/DidComm.Core/Composition/EnvelopeReader.cs @@ -318,6 +318,14 @@ public static async Task UnpackAsync( if (jweResult.IsAuthenticated) { + // Fail closed: an authenticated decrypt MUST surface the sender skid, + // else the FR-CONSIST-01 from↔skid check in the Plaintext branch would + // silently no-op while the message is still reported authenticated — + // letting 'from' assert an identity the envelope never bound. Guaranteed + // by DataProofs' IsAuthenticated ⟺ non-empty-skid contract today; assert + // it so the identity binding never depends on the delegated parser + // upholding it (issue #52; parity with the JWS signer-kid guard above). + AddressingConsistency.CheckAuthcryptSkidSurfaced(jweResult.SenderKid); authenticated = true; senderKid = jweResult.SenderKid; shape.Add(LayerShape.AuthEncrypt); diff --git a/src/DidComm.Core/Consistency/AddressingConsistency.cs b/src/DidComm.Core/Consistency/AddressingConsistency.cs index 3f83a15..d9189b0 100644 --- a/src/DidComm.Core/Consistency/AddressingConsistency.cs +++ b/src/DidComm.Core/Consistency/AddressingConsistency.cs @@ -39,6 +39,23 @@ public static void CheckAuthcryptFromMatchesSkid(string? from, string skid) $"Authcrypt 'skid' DID subject does not match plaintext 'from' (FR-CONSIST-01). from='{from}', skid='{skid}'."); } + /// + /// FR-CONSIST-01 precondition — an authenticated (authcrypt) decrypt MUST surface the sender + /// key identifier (skid). The JOSE layer defines IsAuthenticated as "skid + /// present", so a missing skid on an authenticated result means that upstream invariant + /// regressed; fail closed so cannot silently + /// no-op while the envelope is still reported authenticated. Parity with the JWS branch's + /// empty-signer-kid guard. (Issue #52.) + /// + /// The sender key identifier surfaced by the authenticated decrypt. + /// When is null or empty. + public static void CheckAuthcryptSkidSurfaced(string? skid) + { + if (string.IsNullOrEmpty(skid)) + throw new ConsistencyException( + "Authenticated (authcrypt) envelope did not surface a sender 'skid'; cannot bind the message 'from' to the sender (FR-CONSIST-01)."); + } + /// /// FR-CONSIST-02 — recipient to ↔ kid. When plaintext to is present, /// the DID subject of the recipient key actually used to decrypt MUST be a member of diff --git a/tasks/todo20260720-173121.md b/tasks/todo20260720-173121.md new file mode 100644 index 0000000..47c542e --- /dev/null +++ b/tasks/todo20260720-173121.md @@ -0,0 +1,56 @@ +# Issue #52 — Fail-closed skid guard on authenticated decrypt (authcrypt parity with JWS branch) + +Plan approval waived by user ("skip the approval for the plan for now. Implement"). + +## Context + +`EnvelopeReader`'s authcrypt branch trusts DataProofs' definitional invariant +`IsAuthenticated ⟺ non-empty skid` (`JweParser.cs:439/572`) without asserting it. If that +invariant ever regressed (IsAuthenticated=true, null/empty SenderKid): + +- **null skid** → the FR-CONSIST-01 `from`↔`skid` check at `EnvelopeReader.cs:127` is silently + skipped; `message.From` is attacker-set on an envelope reported authenticated. +- **empty skid** → `CheckAuthcryptFromMatchesSkid` throws a raw `ArgumentException`, escaping the + FR-API-07 unpack exception contract (fails closed, but wrong type). + +The JWS branch already has the equivalent guard (`EnvelopeReader.cs:232`, commit 6f393e9). The +guard is unreachable through today's parser — pure defense in depth, zero behavior change for any +current input. + +## Decisions (pre-made to avoid review churn) + +- **Exception type: `ConsistencyException`** — per the issue as written. The JWS branch's + `CryptoException` asymmetry is deliberate and left untouched (no scope creep). +- **Testability**: the guard lives in `AddressingConsistency` as a named helper so it is directly + unit-testable (the EnvelopeReader call site is unreachable via the real parser — no seam, by + design). Same pattern as every other FR-CONSIST rule. +- **Scope**: no changes to the JWS branch, line-127 semantics, DataProofs, or the PRD + (FR-CONSIST-01 already exists; this is an enforcement precondition, not a new rule). + +## Steps + +- [x] Branch `feat/authcrypt-skid-guard-52` off main +- [x] Add `AddressingConsistency.CheckAuthcryptSkidSurfaced(string? skid)` — throws + `ConsistencyException` on null/empty +- [x] Wire it in `EnvelopeReader` authcrypt branch (`if (jweResult.IsAuthenticated)`) before + setting `authenticated`/`senderKid` +- [x] Unit tests in `AddressingConsistencyTests`: null → throws, empty → throws, valid → passes +- [x] Full `dotnet test` — green, no existing test affected +- [x] CHANGELOG entry under [Unreleased] +- [x] Commit, push, PR referencing #52 + +## Review + +- Guard implemented exactly as scoped: one helper (`AddressingConsistency.CheckAuthcryptSkidSurfaced`, + ~5 lines), one call site (`EnvelopeReader` authcrypt branch, before any trust state is set), + three unit tests (null / empty / valid). +- Full suite green: 598 core + 159 interop = 757 passed, 0 failed, 0 skipped. No existing test + touched — consistent with the guard being unreachable through today's parser (DataProofs defines + `IsAuthenticated` as `!IsNullOrEmpty(senderKid)`, JweParser.cs:439/572). +- Pre-made decisions held: `ConsistencyException` per the issue text; JWS branch untouched; + no PRD/DataProofs changes. `docs/codebase-architecture.md` referenced by CLAUDE.md does not + exist in the repo, so nothing to sync there (only `didcomm-dotnet_PRD.md` is present). +- Verification note for reviewers: the EnvelopeReader call site cannot be exercised black-box by + construction (no parser seam — deliberate); the helper is unit-tested directly, same pattern as + every other FR-CONSIST rule. Precedent: the JWS signer-kid guard (6f393e9) shipped with no + message-level test at all. diff --git a/tests/DidComm.Core.Tests/Consistency/AddressingConsistencyTests.cs b/tests/DidComm.Core.Tests/Consistency/AddressingConsistencyTests.cs index cbec575..faa9a85 100644 --- a/tests/DidComm.Core.Tests/Consistency/AddressingConsistencyTests.cs +++ b/tests/DidComm.Core.Tests/Consistency/AddressingConsistencyTests.cs @@ -34,6 +34,23 @@ public void Authcrypt_with_null_from_short_circuits() AddressingConsistency.CheckAuthcryptFromMatchesSkid(from: null, "did:example:alice#k"); } + [Theory] + [InlineData(null)] + [InlineData("")] + public void Authenticated_decrypt_without_surfaced_skid_fails_closed(string? skid) + { + // Issue #52 — if the JOSE layer's IsAuthenticated ⟺ non-empty-skid invariant ever + // regressed, the guard must reject rather than let the from↔skid binding no-op. + Action act = () => AddressingConsistency.CheckAuthcryptSkidSurfaced(skid); + act.Should().Throw().WithMessage("*FR-CONSIST-01*"); + } + + [Fact] + public void Authenticated_decrypt_with_surfaced_skid_passes() + { + AddressingConsistency.CheckAuthcryptSkidSurfaced("did:example:alice#key-x25519-1"); + } + [Fact] public void Recipient_kid_membership_succeeds_when_subject_matches_any_to_entry() {