Security: Data Integrity tri-state key resolution (F7) + redirect SSRF hardening (PR-A)

[moisesja]

PR-A of 4 in the v1.0.0-readiness series (A security → B compliance → C cleanup → D release). Closes the two must-fix security findings from the readiness review. Both fixes are behaviour-only — no public API change (PublicAPI files untouched; 0-warning build; no new package refs).

S1 — Embedded Data Integrity key-resolution is now tri-state (F7)

The embedded Data Integrity path resolved a proof's verificationMethod through a 2-state (nullable) resolver, so a DID that resolved but did not publish the referenced method collapsed to the same Indeterminate outcome as a genuine resolution failure. An attacker could mangle a tampered/forged credential's verificationMethod fragment (over a still-resolvable base DID) to downgrade a definitive bad-signature Failed → Indeterminate, which a non-strict policy (TreatIndeterminateAsFailure = false) soft-accepts. The enveloping (JOSE/COSE/SD-JWT) path was hardened against exactly this in M4; the embedded path — the most common form — was not.

• New internal IVerificationMethodTriResolver + VerificationMethodResolution (Resolved | DidUnresolvable | MethodNotFound), symmetric with IEnvelopeKeyResolver.
• NetDidVerificationMethodResolver returns the tri-state; a method absent from a resolvable DID is MethodNotFound.
• DataIntegrityMechanism maps MethodNotFound → Invalid("verification_method_not_found") (→ Failed), DidUnresolvable → Unresolvable (→ Indeterminate), fail-closed across multi-proof credentials.
• Default deployments (TreatIndeterminateAsFailure = true) were not exploitable, but the diagnostic was dishonest and a non-strict policy was.

S2 — Opt-in HTTP fetchers no longer follow redirects (SSRF / HTTPS-downgrade)

UseHttpStatusListFetcher / UseHttpSchemaResolver validated only the initial URL's scheme, while the named "credentials-dotnet" HttpClient followed 3xx redirects by default — so a hostile HTTPS credentialStatus/credentialSchema URL could redirect to 169.254.169.254 / localhost / cleartext, silently breaking the "HTTPS by default" promise.

• The named client now sets AllowAutoRedirect = false, so the redirected request never fires (a status list / schema is a single canonical document).
• Docstrings made precise ("by default"; a caller that replaces the named client's primary handler owns the redirect posture — the existing caller-owns-egress contract).

Verification

• TDD red→green: proven RED on main (mangled fragment → Indeterminate; redirect → followed), GREEN after fix.
• New tests: resolver tri-state unit tests; end-to-end mangled-fragment regression (strict → Failed + verification_method_not_found; non-strict → Rejected) + genuinely-unresolvable companion (stays Indeterminate, no over-correction); loopback HttpListener redirect-not-followed test (asserts the internal target is never contacted).
• Full suite 0 failures; 0-warning build. Only the conformance SkippableFact skips (no Node/suite in this env).
• Adversarial re-verification (3 refute-by-default agents): S1 holds (multi-proof fail-closed; holder-binding VP path uses the same mechanism, covered); regression-hunt holds (internal-only interface, no public leak, HTTP defaults intact). S2's "caller-override" path was adjudicated not an attacker capability (attacker controls only the credential URL, not DI registration); its one valid point — a docstring over-claim — was addressed.

🤖 Generated with Claude Code

[moisesja]

Both fixes are correct and well-executed. The tri-state design is clean and symmetric with the enveloping path. A few things worth addressing before merge:

---

Must-address

Multi-proof test is missing despite the explicit claim.

The PR description and code comments repeatedly assert "fail-closed across multi-proof credentials." The logic is correct (any MethodNotFound in the loop short-circuits to Invalid; anyUnresolvable accumulates across all VMs), but there is no test exercising a credential with two proofs where one has a mangled fragment and the other has a valid one. The existing test DataIntegrity_mangled_verification_method_fragment_is_failed_not_indeterminate only issues and mangles a single-proof credential.

The ReadVerificationMethods deduplication (the !methods.Contains(value) guard in AddVerificationMethod) means two proofs sharing the same VM URL are resolved once — this is correct, but the test should cover at least: [vm_valid, vm_mangled_fragment] → Failed, not [vm_mangled] → Failed. The claim "fail-closed across multi-proof" is untested.

---

Minor / non-blocking

ConfigureFetchHttpClient() double-registration.

If a caller chains UseHttpStatusListFetcher().UseHttpSchemaResolver(), ConfigurePrimaryHttpMessageHandler is registered twice against the same named client. In practice the last write wins and both configs are identical, so it's functionally harmless — but the options setup-action list grows unnecessarily with each call. A simple guard is enough:

private bool _fetchHttpClientConfigured;

private void ConfigureFetchHttpClient()
{
    if (_fetchHttpClientConfigured) return;
    _fetchHttpClientConfigured = true;
    Services.AddHttpClient(HttpFetch.ClientName)
        .ConfigurePrimaryHttpMessageHandler(static () => new HttpClientHandler { AllowAutoRedirect = false });
}

The adversarial review correctly noted this doesn't prevent a caller override — that's a separate concern and already documented.

GetFreePort() TOCTOU race in HttpFetchRedirectTests.

private static int GetFreePort()
{
    var probe = new TcpListener(IPAddress.Loopback, 0);
    probe.Start();
    var port = ((IPEndPoint)probe.LocalEndpoint).Port;
    probe.Stop();   // <-- port is free here; another process can grab it before HttpListener binds
    return port;
}

In a busy CI runner with parallel test runs this can rarely race. A more robust pattern is to keep the TcpListener open until HttpListener has started (then stop it), or just catch HttpListenerException and retry with a new port. Unlikely to matter in practice, but it's the kind of thing that produces intermittent CI noise.

---

Positive notes

• The tri-state VerificationMethodResolution record struct with static factory members is exactly the right shape — immutable, unambiguous, no null-return path.
• The loop precedence (MethodNotFound early-return wins over DidUnresolvable accumulation) is the correct fail-closed ordering. Easy to verify by reading DataIntegrityMechanism.VerifyAsync.
• The companion test proving a genuinely-unresolvable base DID still lands on Indeterminate (not Failed) is good — guards against the obvious over-correction.
• static () => new HttpClientHandler { AllowAutoRedirect = false } — the static lambda avoids an unnecessary closure allocation. Small but appreciated.
• The HttpFetchRedirectTests fixture correctly asserts targetHits == 0 (the redirect target was never contacted), not just that the fetch returned null — the right assertion for a SSRF defence.

The multi-proof test is the only thing I'd call a gap rather than a nit. The code logic is correct; the coverage just needs to match the documented claim.

---

Generated by Claude Code

[moisesja]

Thanks — all three addressed in c191053:

Must-address — multi-proof coverage. Added DataIntegrity_multi_proof_with_one_method_not_found_is_failed_not_indeterminate: builds a two-proof document [valid, mangled-fragment] (distinct VM URLs, so not deduplicated; valid listed first to prove it can't rescue the bad one) and asserts Failed + verification_method_not_found under strict policy and Rejected under non-strict. The "fail-closed across multi-proof" claim is now tested, not just asserted.

Minor — ConfigureFetchHttpClient double-registration. Added the _fetchHttpClientConfigured guard you suggested, so chaining UseHttpStatusListFetcher().UseHttpSchemaResolver() registers the handler config once. (Agreed this is orthogonal to the documented caller-override behaviour.)

Minor — GetFreePort TOCTOU. Wrapped listener startup in StartLoopbackListener(), which retries HttpListener.Start() on HttpListenerException (port grabbed between probe and bind), and switched disposal to Close(). Should remove the intermittent-CI-noise risk.

Full DI suite 160 (+1), Core 145, 0 failures, 0-warning build. CI re-running.

[moisesja]

Thorough review (supersedes my earlier comment)

I traced both fixes end-to-end against the surrounding code paths (resolver → DataIntegrityMechanism → DefaultVerifier status mapping → DecisionComposer; and HttpFetch.TryGetAsync → named-client handler config). I could not run the build/tests in this environment (no dotnet SDK), so the verification below is by code reading, not execution. The PR's own RED→GREEN evidence covers that.

Verdict: both fixes are correct and the design is clean. One real coverage gap, plus a few minor/latent items. Nothing blocks merge except possibly the multi-proof test.

---

What I verified holds

S1 — the tri-state actually closes the downgrade. The only route to Indeterminate is now DidUnresolvable (genuine base-DID resolution failure). Every "method absent / key unusable" branch in NetDidVerificationMethodResolver returns MethodNotFound → Invalid("verification_method_not_found") → Failed. An attacker can't reach Indeterminate by mangling a fragment, because the base DID still resolves (→ MethodNotFound, not DidUnresolvable). To get DidUnresolvable they'd have to break the base DID, but a forgery needs a resolvable DID whose key they don't control — so there's no downgrade path. Confirmed.

The fail-closed precedence is order-independent. In the VerifyAsync loop, MethodNotFound returns immediately while DidUnresolvable only sets anyUnresolvable and continues — so Failed always wins over Indeterminate regardless of proof order ([Unresolvable, MethodNotFound] and [MethodNotFound, Unresolvable] both → Failed). That's the correct lattice (Failed ⊐ Indeterminate). Nice.

The fix covers the VP holder-binding seam, not just credential proofs. DataIntegrityMechanism is the same mechanism behind CheckKinds.HolderBinding, and DefaultVerifier maps Invalid → Failed / Unresolvable → Indeterminate for both Proof (DefaultVerifier.cs:466-467) and HolderBinding (DefaultVerifier.cs:230-231). So a mangled-fragment holder-binding proof on a VP is also caught. The PR claims this; it checks out.

The tri-state is faithfully symmetric with EnvelopeKeyResolution (same enum order, same factory/record-struct shape). The asymmetry that the lessons entry calls out — enveloping hardened, embedded not — is genuinely closed now.

S2 — disabling auto-redirect is the right primitive. With AllowAutoRedirect = false, a 3xx comes back as a non-2xx response, so HttpFetch.TryGetAsync returns null at the IsSuccessStatusCode gate (HttpFetch.cs:37) and the redirect target is never contacted. Validating the final URL post-hoc would be too late (a blind SSRF GET is itself the damage); not firing the request at all is the correct shape. The test asserts the hit-counter stays 0, not just that the return is null — the right assertion.

---

Must-address (or consciously wave off)

1. "Fail-closed across multi-proof credentials" is asserted everywhere but tested nowhere. The PR body, the class doc, and the inline comment all claim multi-proof fail-closed behavior, and it is a behavior change (old code short-circuited Unresolvable on the first null; new code collects across all VMs and lets any MethodNotFound win). But DataIntegrity_mangled_verification_method_fragment_is_failed_not_indeterminate issues and mangles a single-proof credential. There is no test for a two-proof credential where one VM resolves+verifies and the other is a mangled fragment → expect Failed. Given F7 is the whole point of this PR, the multi-proof path deserves the one test that exercises the new accumulation logic. (Note the ReadVerificationMethods dedup on the VM string — so the two proofs must carry distinct VM URLs for the test to actually traverse two iterations.)

---

Minor / latent (non-blocking)

2. Latent footgun in the tri-state struct: the enum zero-value is the success state. default(VerificationMethodResolution) yields Status == Resolved (enum 0) with Method == null — and DataIntegrityMechanism does resolved.Add(resolution.Method!) on Resolved, which would NPE/poison the list. It's not reachable today (the resolver only ever returns via the three factories), but FirstOrDefault over a VerificationMethodResolution sequence, an array slot, or any future new()/default would silently produce "Resolved + null." This is mirrored from EnvelopeKeyResolution, so it's a pre-existing pattern rather than a regression — but it's worth hardening both by making a non-success the zero value (e.g. reorder so DidUnresolvable = 0, or add an explicit Unknown = 0). Cheap insurance against a future NRE.

3. ConfigureFetchHttpClient() registers the handler twice if a caller chains UseHttpStatusListFetcher().UseHttpSchemaResolver(). Last-write-wins and both configs are identical, so it's functionally harmless — just a redundant HttpMessageHandlerBuilderAction and a wasted handler allocation per client build. A _fetchHttpClientConfigured guard tidies it. (This does not prevent a caller override, which is correctly documented as out of scope.)

4. GetFreePort() in HttpFetchRedirectTests has a classic TOCTOU race — the probe TcpListener is stopped before HttpListener binds, so a parallel test/process can grab the port in between → intermittent HttpListenerException. Keep the probe open until the real listener has started, or catch-and-retry. Unlikely, but it's exactly the shape that produces flaky-CI tickets.

---

Worth confirming the threat model, not a defect

5. The redirect fix is narrow by design — initial-URL SSRF remains open. A credential that directly names https://169.254.169.254/... or an internal host (no redirect) is still fetched; only the redirect escalation is closed. That's the documented "egress is the caller's responsibility — front the named client with an allowlist" contract, and the PR is honest about it. Flagging only so it's a conscious acceptance: the "HTTPS by default" promise still doesn't constrain the destination host, only the scheme and (now) redirects.

6. Pre-existing, low priority: a fragment-less verificationMethod resolves to methods[0] (NetDidVerificationMethodResolver.cs:79-80) regardless of which method the base DID intended — an asymmetry with the envelope path's exact-kid match. Not a downgrade vuln (it resolves → substrate → crypto check → Failed on mismatch), and unchanged by this PR. Separately, the FirstOrDefault(m => m.Id == verificationMethodUrl) match compares against the full URL including any ?query, while the base-DID parse strips the query — so a legitimate DID-URL with a query parameter would resolve to MethodNotFound. Both are pre-existing edges; mentioning for the backlog, not this PR.

---

Solid security work — the root-cause framing (asymmetric defense; validate-final-not-initial) is right and the lessons entries are good. Item 1 (multi-proof test) is the only thing I'd genuinely want before merge; 2-4 are cheap hardening; 5-6 are FYI.

---

Generated by Claude Code

[moisesja]

Addressed the second (fuller) review.

Item 2 — tri-state struct zero-value footgun (valid, fixed in 07bf59f). Reordered both VerificationMethodResolutionStatus and EnvelopeKeyResolutionStatus so the zero value is a non-success (DidUnresolvable). Now default(VerificationMethodResolution)/default(EnvelopeKeyResolution) fail closed instead of presenting as Resolved + null/empty key. No switch changes and no behaviour change for real values — the consuming switches match by name and reach the Resolved path only via the factory-set value. Hardened both structs (the symmetry this PR is about) and pinned the invariant with TriStateResolutionDefaultsTests. Full suite green (Core 147 +2, DI 160, 0-warning build).

Items 1, 3, 4 — already in the branch (c191053), before this review. The multi-proof fail-closed test (DataIntegrity_multi_proof_with_one_method_not_found_is_failed_not_indeterminate, distinct VM URLs so it traverses two iterations), the _fetchHttpClientConfigured guard, and the GetFreePort retry/HttpListener hardening all landed in c191053 — the review appears to have traced the first commit. No further action.

Items 5, 6 — agreed, out of scope for this PR. The initial-URL-host SSRF is the documented "caller owns egress" contract (5). The fragment-less methods[0] resolution and the query-string-in-VM exact match (6) are pre-existing edges unchanged by this PR; I've noted them for PR-C (cleanup) — stripping the DID-URL query before the id match is a small correctness fix that fits there.

Thanks for the thorough trace — the lattice/ordering and VP-holder-binding-seam confirmations are exactly right.