Fix VP verification for holder-less and credential-less presentations (#11)

[moisesja]

Fixes #11 — and corrects its diagnosis.

What #11 actually was

The issue posited a "VP authentication-proof NoProof interop gap." Investigation showed that was wrong: the engine verifies a valid W3C-suite eddsa-rdfc-2022 holder-bound VP perfectly (decision=Accepted, holderBinding=Passed) once UseRdfcSuites() is registered. The dumped VP I'd first looked at happened to be a passing one.

Capturing the actual failing VPs revealed every one had holder: MISSING, and 3 of 4 also had no verifiableCredential. VCDM 2.0 makes both optional, but the verifier rejected them via two over-strict checks.

Fix

• BindHolder (engine): failed when holder was absent. Now a signed presentation with no holder passes the binding check on possession alone — the binding proof verified and the challenge/domain matched; there is simply no holder identity to bind. Not abusable: holder is inside the proof's signed scope, so stripping a victim's holder from a holder-bound VP invalidates the proof before this check runs. Locked by a new regression test (Stripping_the_holder_from_a_bound_presentation_breaks_the_proof).
• presentation_no_credentials: already gated on the existing RequireAtLeastOneCredential option — the conformance shim sets it false (a VP may legitimately carry no credentials). No engine default change.

Result

• W3C VCDM 2.0 conformance baseline 36 → 43 / 59 (the 4.13-verifiable-presentations group now passes); harness baseline + docs/conformance.md updated.
• New tests: holder-less signed VP verifies (binding Passed); the security guard (strip holder → proof fails, not the holder-less shortcut).
• Existing M6/M7 holder-binding forgery tests unchanged + green (the VM-base == holder check still runs when a holder IS present).
• Build 0-warning; 359 tests + the conformance harness green.

🤖 Generated with Claude Code

[moisesja]

Review — PR #12: Fix VP verification for holder-less and credential-less presentations

Bottom line: looks good. The fix is correct, the security invariant holds, and the regression test coverage is well-designed. One real concern below, rest is minor.

---

What I verified

The critical security claim — "stripping a victim's holder invalidates the proof before this check runs" — is enforced at the call site, not just in the comment. BindHolder is only reachable via CheckHolderBindingViaMechanismAsync when the securing status is exactly Verified (the switch arm at line 229); Invalid and NoProof statuses short-circuit before BindHolder is ever called. Challenge/domain replay checks also run before the call. So returning Passed unconditionally for a holder-less, proof-verified presentation is correct per VCDM 2.0.

The two regression tests are properly designed:

• Holder_less_signed_presentation_verifies_on_possession_alone — strips holder before signing, so the proof covers a genuinely holder-less document. ✓
• Stripping_the_holder_from_a_bound_presentation_breaks_the_proof — strips holder after signing a holder-bound VP, confirms proof breaks, and checks the diagnostic code is NOT holder_binding_missing (i.e., the holder-less shortcut is not triggered). ✓

---

One concern worth addressing

BindHolder carries no assertion that it is only called with a verified proof. The contract is real and enforced today inside CheckHolderBindingViaMechanismAsync, but it exists only by convention. If a future call path passes a non-Verified result, the early-return Passed for any holder-less VP becomes a silent bypass.

A single defensive line closes that gap permanently:

private static CheckResult BindHolder(VerifiablePresentation vp, SecuringVerificationResult result)
{
    Debug.Assert(result.Status == SecuringVerificationStatus.Verified,
        "BindHolder must only be called after the binding proof is Verified.");

    var holderId = vp.Holder;
    if (string.IsNullOrEmpty(holderId))
    {
        return CheckResult.Passed(CheckKinds.HolderBinding);
    }
    ...

This is not blocking — the current code is safe — but it's cheap insurance in a security-sensitive path.

---

Minor notes

• The comment block inside BindHolder runs 6 lines. The Debug.Assert above carries the same invariant more precisely; the comment could shrink to one line.
• Both new tests carry [FrTag("FR-041")]. The security regression test may merit its own FR tag for auditability purposes.
• RequireAtLeastOneCredential = false is correctly scoped to the conformance shim only; the engine default is unchanged.

---

Conformance baseline 36 → 43 / 59 is a meaningful jump, the removed tracking row in docs/conformance.md accurately reflects that the VP group now passes, and the existing M6/M7 forgery rejection tests being green is the right bar. Ship it once the assertion is in.

---

Generated by Claude Code

[moisesja]

Review (revised) — PR #12

This supersedes my earlier comment on this PR. I re-reviewed against the actual source (DefaultVerifier.cs, the options defaults, and the test helpers), not just the diff, and one point in the previous pass was wrong — flagged below.

Verdict: the fix is correct and I'd merge it. The core change is sound and the test design is genuinely good (it satisfies CLAUDE.md's own rule to test both post-sign tampering and self-consistent forgery). But the "not abusable" argument in the description is narrower than it reads, and there are two things worth stating plainly.

---

Verified by reading the code

• BindHolder is only reached on Verified. Confirmed at DefaultVerifier.cs:229 — Invalid/NoProof/Unresolvable never enter BindHolder. So the new holder-less Passed (line 279) only fires for a presentation whose binding proof actually verified.
• Replay defence survives. For DI, challenge/domain are enforced by the substrate via the VerifyRequest (lines 200–207); for JOSE, CheckPresentationFreshness still runs after BindHolder (line 196). The holder-less branch doesn't bypass either.
• The strip-holder attack genuinely fails. Removing holder from a holder-bound DI VP changes the canonicalized graph → proof verification returns Invalid → line 230, not the holder-less shortcut. Stripping_the_holder_from_a_bound_presentation_breaks_the_proof asserts exactly this, including that the diagnostic is not holder_binding_missing. Correct and valuable.

---

Correction to my previous review

I earlier suggested a Debug.Assert(result.Status == Verified, …) in BindHolder as "cheap insurance in a security-sensitive path." That was wrong: Debug.Assert is compiled out of Release builds, so it would provide zero protection in the shipped library — it's documentation, not a guard. If you want a real backstop for the call-site invariant it must be a runtime check (throw, or return Indeterminate). Honestly, though, for a private static method with a single, already-guarded call site, even that is optional — the invariant is structurally enforced today. I'd accept the code as-is here.

---

The one thing the "not abusable" note doesn't cover

The description's safety argument addresses holder-stripping only. It doesn't address the broader fact that the engine never binds the VP signer/holder to the contained credentials' credentialSubject.id (grep confirms: no subject↔holder check anywhere in the verifier or securing layer). So a party who obtains a victim's VC can wrap it in a VP signed with their own key and it composes to Accepted.

Important caveats so this isn't misread:

• This is pre-existing and not introduced by this PR. Before the change, the same attacker could already do it with a self-holder VP (holder = attacker's own DID, signed with attacker's key, which passes BindHolder). The new holder-less path is capability-equivalent — not a regression.
• It's a legitimate division of responsibility: holder binding proves possession + freshness; matching the subject to the presenter is the verifying application's job.

My ask is just documentation, not code: the holder-binding semantics ("possession alone; subject↔holder linkage is the verifier's responsibility") should be stated somewhere durable (XML doc on RequireHolderBinding/BindHolder, or the PRD), so a downstream integrator doesn't assume Accepted means "presented by the subject."

---

Minor

• Default-path impact is undersold. BindHolder is on the default verification path, so holder-less DI/JOSE VPs now pass binding for all callers under default options — not just the conformance shim. (Still replay-safe: RequireHolderBinding defaults true, which forces a challenge at line 178.) Only RequireAtLeastOneCredential=false is shim-scoped. The description reads as if both relaxations are conformance-only; the holder-less one is a real engine-default change (and the intended interop fix — just be explicit).
• Both new tests share [FrTag("FR-041")]; the security regression test arguably deserves its own tag for audit traceability. Style-level.
• The 6-line comment in BindHolder could be trimmed, but it's accurate, so no objection.

---

Hygiene

Scope is tight (+99/−6 over 6 files), the commit message is exemplary and honestly documents that the original #11 diagnosis was wrong, CHANGELOG + docs/conformance.md are updated, and the conformance baseline is raised with a regression gate (PassingBaseline = 43). Good. (I couldn't execute the suite — no .NET SDK in this review environment — so the "359 tests green" claim is taken on trust; the test logic I could read is correct.)

---

Generated by Claude Code

[moisesja]

Thanks for the thorough re-review (and the honest self-correction on Debug.Assert). Addressed in ed6cf6a:

The runtime backstop (your corrected suggestion)

Added a real runtime guard to BindHolder — if (result.Status != SecuringVerificationStatus.Verified) return Indeterminate("binding_not_verified") — not Debug.Assert (which, as you noted, compiles out of Release and would give zero protection). The holder-less Passed path is now self-enforcing and fails closed if a future call path ever passes a non-verified result.

The main ask — holder-binding semantics (subject↔holder)

You're right, and it's the most important point. Documented the scope of holder binding on the public PresentationVerificationOptions.RequireHolderBinding: a passing check proves possession + freshness, not that the presenter is the credentialSubject — the engine does no holder↔subject linkage, so a holder can present a credential in a VP signed with their own key and it Accepts; that linkage is the verifying application's responsibility. Added a test (Holder_binding_proves_possession_not_that_the_presenter_is_the_subject) that demonstrates exactly this, so it's documented and pinned. As you said, this is pre-existing and capability-equivalent to the self-holder case — not a regression.

Framing (default-path impact)

Sharpened the CHANGELOG to state explicitly that the holder-less acceptance is an engine-default change (all callers, still replay-safe via RequireHolderBinding default true), while RequireAtLeastOneCredential stays true by default and is only relaxed in the conformance shim.

Minor notes

• FrTag: kept FR-041 on both new tests — it's the accurate requirement (presentation verification) and there's no more-specific id; the coverage gate is satisfied.
• The 6-line BindHolder comment: kept (you had no objection); the runtime guard now carries the invariant precisely.

Extra: adversarial pass

Because this is a security-sensitive verifier change, I ran an adversarial pass — 11 exploit attempts against the holder-less path (replay with stale/missing challenge, strip-holder-to-evade, forged proof, unresolvable VM, no-proof, tampered contained credential, worse-than-self-holder, runtime-guard reachability). All blocked; no Accepted/Passed that should have been Rejected. The holder-less Passed is reachable only after the proof verifies, and the default RequireHolderBinding=true forces a challenge.

360 tests green, 0-warning build. (CI re-runs on the push.)