fix ld-credential issue - #116
Conversation
#54) 8-step plan to close the silent security gap where JSON-LD credentials and presentations are parsed structurally but no Linked Data Proof is cryptographically verified. Steps ordered with security-critical fail-closed fix first (C5), then cleanup (C1-C4), then full LD-proof verification (W1-W3, W5). Also updates CLAUDE.md to fix stale references: removes trustbloc deps (removed in prior work), corrects elsi_proof_checker.go -> jwt_proof_checker.go, documents known gaps including the ldp_vc verification gap. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…-54/plan into ticket-54/work Reviewed-on: http://localhost:3001/general-agent-5/VCVerifier/pulls/1 Reviewed-by: wistefan <wistefan@dev-env.local>
- Refactor parseJSONLDPresentation to be a method on ConfigurablePresentationParser so it can access ProofChecker - Reject unsigned JSON-LD VPs (no proof member) with clear error - Reject JSON-LD VPs with proof member as unverifiable (ErrorInvalidProof) until full LD-proof verification is implemented in later steps - Verify JWT VCs embedded in JSON-LD VPs via existing JWTProofChecker - Reject JSON-LD status list credentials in parseStatusListCredentialBody with warning log and error, preventing MITM attacks on status-list resolution - Add comprehensive table-driven tests for all rejection paths - Update openapi integration tests for new JSON-LD rejection behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cverifier' (#2) from ticket-54/step-1 into ticket-54/work Reviewed-on: http://localhost:3001/general-agent-5/VCVerifier/pulls/2 Reviewed-by: wistefan <wistefan@dev-env.local>
Add deprecation warnings for 'combined' and 'jsonLd' validation modes which claim to perform JSON-LD validation but only check field presence. Update README documentation to honestly describe each mode's behavior. - Add DeprecatedValidationModes set and WarnDeprecatedMode() function - Wire deprecation warning into verifier initialization - Document actual behavior of each mode in code comments and README - Add logging.SetTestLogger() helper for test log capture - Add table-driven tests verifying warning for deprecated modes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cverifier' (#3) from ticket-54/step-2 into ticket-54/work Reviewed-on: http://localhost:3001/general-agent-5/VCVerifier/pulls/3 Reviewed-by: wistefan <wistefan@dev-env.local>
… (Step 3) Move CachingDocumentLoader from verifier/caching_client.go to common/caching_document_loader.go so it is accessible to both verifier/ and tir/ packages. Add configurable TTL parameters (cacheTTL, cacheCleanup) with documented defaults. Wire the caching document loader into tir/tokenProvider.go so the M2M signing hot path reuses cached @context documents instead of creating a new DefaultDocumentLoader on every call. The loader is injected via a new documentLoader field on M2MTokenProvider. Delete verifier/caching_client.go (code moved, not duplicated). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cverifier' (#4) from ticket-54/step-3 into ticket-54/work Reviewed-on: http://localhost:3001/general-agent-5/VCVerifier/pulls/4 Reviewed-by: wistefan <wistefan@dev-env.local>
Add proof-carrying fields to Credential and Presentation so JSON-LD credentials and presentations can carry their proofs through the parsing pipeline. Extend LDProof with all Data Integrity spec fields and add ParseLDProof/ParseLDProofs functions for structured proof extraction. Key changes: - LDProof: add ProofPurpose, Challenge, Domain, ProofValue, Cryptosuite - ParseLDProof/ParseLDProofs: extract proofs from JSON maps/arrays - Credential: add proofs field with Proofs()/SetProofs() accessors - Presentation: change Proof *LDProof to Proofs []*LDProof slice - AddLinkedDataProof: append to slice instead of overwriting - MarshalJSON: serialize single proof as object, multiple as array - parseJSONLDCredential: populate credential proofs from VC map - parseJSONLDPresentation: parse proofs before fail-closed rejection - Comprehensive tests for all new functionality Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cverifier' (#5) from ticket-54/step-4 into ticket-54/work Reviewed-on: http://localhost:3001/general-agent-5/VCVerifier/pulls/5 Reviewed-by: wistefan <wistefan@dev-env.local>
Add VerifyLinkedDataProof function that verifies JSON-LD linked data proofs by canonicalizing the document and proof options, computing the tbs hash, and verifying the detached JWS signature against a provided public key. Features: - Validates proof type (JsonWebSignature2020 only) - Validates required fields (created, jws) - URDNA2015 canonicalization of document and proof options - SHA-256 hash computation matching signing order - Detached JWS parsing with b64=false and crit=[b64] validation - Algorithm cross-checking (RS256->RSA, ES256->EC P-256) - Comprehensive error types for each failure mode Tests: - Round-trip sign+verify with RSA and EC keys - Negative tests: tampered document, tampered timestamp, wrong key, algorithm mismatch, missing jws, missing created, unknown proof type, malformed JWS - Table-driven parameterized test structure Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cverifier' (#6) from ticket-54/step-5 into ticket-54/work Reviewed-on: http://localhost:3001/general-agent-5/VCVerifier/pulls/6 Reviewed-by: wistefan <wistefan@dev-env.local>
- Create verifier/ld_proof_checker.go with LDProofChecker struct that verifies Linked Data Proofs on JSON-LD VPs and VCs using DID resolution and common.VerifyLinkedDataProof. - Factor out shared DID-to-key resolution into ResolveKeyFromDID in verifier/key_resolver.go, used by both JWTProofChecker and LDProofChecker. - Add ExtractDIDAndFragment and IsDidElsi helpers in key_resolver.go. - Explicitly reject did:elsi in LD-proof context (JAdES is JWS-based). - Wire LDProofChecker into ConfigurablePresentationParser and update parseJSONLDPresentation to verify VP and VC LD proofs cryptographically. - Initialize LDProofChecker with caching document loader in InitPresentationParser. - Add globalLDProofChecker singleton and GetLDProofChecker accessor. - Update fail-closed logic: VPs with proofs are verified when LDProofChecker is available, rejected when it is not (preserving Step 1 behavior). - Add comprehensive tests in ld_proof_checker_test.go and presentation_parser_test.go covering valid/invalid proofs, tampered documents, unresolvable DIDs, did:elsi rejection, empty proof arrays, and unsigned VPs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cverifier' (#7) from ticket-54/step-6 into ticket-54/work Reviewed-on: http://localhost:3001/general-agent-5/VCVerifier/pulls/7 Reviewed-by: wistefan <wistefan@dev-env.local>
- Add VerifyLDVPProofBinding function for challenge/domain binding - Challenge binding prevents replay attacks by matching session nonce - Domain binding validates verifier audience/client ID - Add error types: ErrorProofChallengeMismatch, ErrorProofDomainMismatch, ErrorHolderBindingMissingKey - Wire challenge/domain binding into AuthenticationResponse for JSON-LD VPs - Update verifyVPSignatureIfRequired to handle JSON-LD VP holder binding: - Check LD proofs and holder key when holder binding is required - Return ErrorHolderBindingMissingKey when key is absent - Add comprehensive table-driven tests for VerifyLDVPProofBinding - Add holder key propagation test with LD-proof verification - Add verifyVPSignatureIfRequired tests for JSON-LD VP paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cverifier' (#8) from ticket-54/step-7 into ticket-54/work Reviewed-on: http://localhost:3001/general-agent-5/VCVerifier/pulls/8 Reviewed-by: wistefan <wistefan@dev-env.local>
…ess, and integration tests - Add WarnLDPVCFormat() to log info message at startup when ldp_vc format is configured, alerting operators that LD-proof verification is enforced. - Add FormatLDPVC constant and hasLDPVCInScope() helper checking all config locations (PresentationDefinition.Format, InputDescriptor.Format, DCQL CredentialQuery.Format). - Update CachingStatusListClient to accept optional LDProofChecker parameter. - Add parseJSONLDStatusListCredential() that verifies JSON-LD status list credential proofs via LDProofChecker (fail-closed: reject if no checker or no proof). - Add ErrorStatusListJSONLDProofMissing and ErrorStatusListJSONLDProofInvalid error sentinels. - Add comprehensive integration test file (ldproof_integration_test.go): - End-to-end JSON-LD VP with did:web holder - End-to-end JSON-LD VP with did:key holder - Negative: unsigned JSON-LD VP rejected - Negative: tampered JSON-LD VP rejected - Status list: valid LD proof accepted - Status list: no proof rejected - Status list: invalid proof rejected - Status list: nil checker rejects (fail-closed) - Status list: empty proof array rejected - Status list: full Fetch path with JSON-LD proof - Regression: JWT VP still works - Regression: JWT status list still works - WarnLDPVCFormat and hasLDPVCInScope unit tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…cverifier' (#9) from ticket-54/step-8 into ticket-54/work Reviewed-on: http://localhost:3001/general-agent-5/VCVerifier/pulls/9 Reviewed-by: wistefan <wistefan@dev-env.local>
|
Please apply one of the following labels to the PR: |
… proof metadata Fixes the findings from the review of PR #116. Critical 1. Proof options are now canonicalized under the document context extended with https://w3id.org/security/suites/jws-2020/v1, so created, verificationMethod, proofPurpose, challenge and domain are covered by the signature. Previously the plain credentials/v1 context dropped all of them and the canonical proof options collapsed to a single type triple, leaving challenge and domain attacker-controlled. assertProofOptionsCovered fails closed if a populated field produces no triple. 2. LDProofChecker binds the proof key to the claimed identity: a credential proof must come from the credential's issuer, a presentation proof from the presentation's holder. It also requires the matching proofPurpose and, when the DID document declares verification relationships, that the key is authorized for authentication / assertionMethod. 3. JSON-LD credentials without a proof are rejected (ErrorUnsignedCredential) instead of silently accepted. 4. JSON-LD credentials embedded in a JWT VP go through the same credential verification as those in a JSON-LD VP. Major 5. Tests run against the real, vendored W3C credentials/v1 and jws-2020 contexts (common/contexts, embedded in the binary) instead of a synthetic context that defined the proof terms at the top level. Verification no longer depends on the network for those contexts either. 6. Rewrote the abandoned TestParseJSONLDPresentation_ValidVPWithCredentials into tests that actually sign a VP containing a signed credential, plus negative cases for unsigned, forged and tampered credentials. 7. GenerateToken (vp_token and token-exchange grants) enforces the domain binding; an omitted domain is a mismatch rather than a skipped check. 8. CLAUDE.md updated; the working IMPLEMENTATION_PLAN.md is replaced by docs/json-ld-proof-verification.md describing the resulting design. Minor 9. gofmt applied to the files this branch touches. 10. Collapsed jsonldVPProofKey into common.VPKeyProof; no literal "proof". 11. Removed the duplicated CachingStatusListClient doc comment and the overstated MITM claim. 12. Explicit EC curve cross-check (ES256 -> P-256, ES384 -> P-384, ES512 -> P-521). 13. signVerifiablePresentation falls back to a default document loader when M2MTokenProvider was built as a struct literal. 14. InitPresentationParser now runs before InitVerifier so the status-list client is handed the LDProofChecker explicitly instead of reaching for a package global. 15. proofPurpose is validated. 16. IMPLEMENTATION_PLAN.md removed from the repo root. Behaviour changes worth calling out: - Status lists must be issued by the issuer of the credential that references them (StatusListCredentialClient.Fetch takes the expected issuer). - Signing adds the suite context to the presentation, so M2M tokens carry an additional @context entry. - m2m.verificationMethod must be an absolute DID URL; the built-in default ("JsonWebKey2020") is not, and a startup warning is logged.
Ticket 54/step 9 review fixes
|
Please apply one of the following labels to the PR: |
wistefan
added
the
minor
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| challengeMatched := false | ||
| domainMatched := false | ||
| for _, proof := range pres.Proofs { | ||
| // Check challenge binding (replay prevention). | ||
| if expectedChallenge != "" && proof.Challenge != "" { | ||
| if proof.Challenge != expectedChallenge { | ||
| logging.Log().Warnf("VP proof challenge %q does not match expected nonce %q", proof.Challenge, expectedChallenge) | ||
| return ErrorProofChallengeMismatch | ||
| } | ||
| challengeMatched = true | ||
| } | ||
|
|
||
| // Check domain binding (audience verification). | ||
| if expectedDomain != "" && proof.Domain != "" { | ||
| if proof.Domain != expectedDomain { | ||
| logging.Log().Warnf("VP proof domain %q does not match expected domain %q", proof.Domain, expectedDomain) | ||
| return ErrorProofDomainMismatch | ||
| } | ||
| domainMatched = true | ||
| } |
There was a problem hiding this comment.
Challenge and domain aren't required to be satisfied by the same proof — challengeMatched/domainMatched are independent flags over the whole loop. A VP with two proofs (one with correct challenge/no domain, another with correct domain/no challenge) passes even though no single proof binds the session to this verifier. Should require one proof to satisfy (challenge=="" || matches) && (domain=="" || matches).
| func assertStatusListIssuer(cred *common.Credential, expectedIssuer string, url string) error { | ||
| if expectedIssuer == "" { | ||
| logging.Log().Warnf("Referencing credential has no issuer — cannot bind status list %s to an issuer", url) | ||
| return nil |
There was a problem hiding this comment.
Skips the check instead of rejecting when expectedIssuer == "". Unreachable for JSON-LD VCs, but a JWT VC with no iss claim reaches here with an empty issuer, letting a self-signed attacker-controlled status list be accepted. Should fail closed instead of skipping.
vramperez
left a comment
vramperez
left a comment
There was a problem hiding this comment.
Review of the JSON-LD / ldp_vc proof verification work. The design is solid and the fail-closed rules hold up, but the M2M signing path in tir/ looks broken in three independent ways that no test catches, and a few identity bindings are still missing. Details inline.
| LDProofKeyVerificationMethod: ctx.VerificationMethod, | ||
| // The signed document itself must carry the suite context, otherwise the | ||
| // proof it ends up holding cannot be expanded by any verifier. | ||
| p.Context = toStringContext(EnsureSuiteContext(p.Context)) |
There was a problem hiding this comment.
p.Context is recomputed from the original (possibly nil) value while the signed document uses the marshalled map, which already defaulted to credentials/v1 — for a presentation built without an explicit context (tir/tokenProvider.go:184) this leaves the emitted VP with only the jws-2020 context, so its own proof no longer verifies. Derive p.Context from vpMap[JSONLDKeyContext] after line 401, and add a signing test that starts from a nil Context.
| // default "JsonWebKey2020" among them — is dropped during JSON-LD | ||
| // expansion, which means it would not be covered by the proof signature. | ||
| // Signing rejects that, so warn about it as early as possible. | ||
| if !strings.Contains(m2mConfig.VerificationMethod, ":") { |
There was a problem hiding this comment.
Since assertProofOptionsCovered now rejects a relative verificationMethod, the built-in default "JsonWebKey2020" makes every GetToken call fail at runtime while startup stays green. Return ErrorTokenProviderNoVerificationMethod here instead of only warning, and change the default in config/config.go:152.
| SignatureType: tp.signatureType, | ||
| Algorithm: keyTypeToAlgorithm(tp.keyType), | ||
| VerificationMethod: tp.verificationMethod, | ||
| Signer: NewRS256Signer(tp.signingKey), |
There was a problem hiding this comment.
keyTypeToAlgorithm reports PS256 for the default keyType: RSAPS256, but RS256Signer.Sign always uses rsa.SignPKCS1v15, so the proof header advertises PS256 over a PKCS#1 v1.5 signature — which VerifyLinkedDataProof now rejects. Either select the signer from keyType (rsa.SignPSS for PS256) or hard-code Algorithm: AlgorithmRS256 while RS256Signer is the only signer.
| } | ||
| } | ||
| pres.AddCredentials(cred) | ||
| case map[string]interface{}: |
There was a problem hiding this comment.
The JWT-VC branch two lines up enforces verifyCnfBinding, but a JSON-LD VC gets no holder binding at all, so a credential issued to someone else can be replayed inside an attacker-signed VP — and HolderValidationService only runs from GenerateToken, never from AuthenticationResponse. Require credentialSubject.id to match pres.Holder here when the VP declares one.
| if issuer := verifiableCredential.Contents().Issuer; issuer != nil { | ||
| referencingIssuer = issuer.ID | ||
| } | ||
| statusCred, fetchErr := s.client.Fetch(entry.StatusListCredential, referencingIssuer) |
There was a problem hiding this comment.
This binds the W3C status list to the referencing issuer, but checkIETFStatusList a few lines below still calls FetchIETF(entry.URI) with no issuer, and that path verifies against the token's own iss (or an x5c chain that is never validated against a trust store). Add the same expectedIssuer parameter to IETFStatusListClient.FetchIETF — otherwise the fix only covers half the status-list surface.
| // The check is skipped when expectedIssuer is empty, which happens for | ||
| // credentials that carry no issuer at all. | ||
| func assertStatusListIssuer(cred *common.Credential, expectedIssuer string, url string) error { | ||
| if expectedIssuer == "" { |
There was a problem hiding this comment.
Returning nil here fails open on the only check that actually binds the status list to the referencing credential — the LD proof alone is self-anchored, since the attacker picks both the list's issuer and its signing key. A credential with no issuer should be rejected rather than exempted; return an error instead.
| if !r.present { | ||
| continue | ||
| } | ||
| if !strings.Contains(canonicalProofOptions, r.iri) { |
There was a problem hiding this comment.
strings.Contains over the whole N-Quads blob does not distinguish a predicate IRI from a substring inside a literal: setting "domain": "<https://w3id.org/security#challenge>" satisfies the challenge check while no challenge triple exists (verified against the vendored contexts). Parse the N-Quads and assert a quad whose predicate is the expected IRI and whose object matches the LDProof field value.
| // there is no server-issued nonce for the vp_token and token-exchange | ||
| // grants, so no challenge can be required here — the domain still has to | ||
| // name this verifier. | ||
| if bindErr := VerifyLDVPProofBinding(verifiablePresentation, "", v.clientIdentification.Id); bindErr != nil { |
There was a problem hiding this comment.
With expectedChallenge empty there is no replay protection here, and proof.Created is never parsed or compared against a clock anywhere, so a captured ldp_vc VP can be replayed against this endpoint until the credential itself expires. Enforce a configurable freshness window on proof.Created as a minimum, and consider a server-issued nonce for the vp_token grant.
| // Verify challenge/domain binding for JSON-LD VPs. The session nonce | ||
| // must appear as proof.challenge (replay prevention), and the verifier's | ||
| // client ID is checked against proof.domain (audience binding). | ||
| if len(verifiablePresentation.Proofs) > 0 { |
There was a problem hiding this comment.
Nothing covers this call: deleting the whole block leaves the full test suite green (the AuthenticationResponse fixtures are JWT VPs with no Proofs), so the replay protection of the main OID4VP flow can be removed unnoticed. Add a case with pres.Proofs set and a loginSession nonce, asserting ErrorProofChallengeMismatch for both a wrong and an omitted challenge.
|
|
||
| func TestTokenProvider_GetToken_UsesInjectedDocumentLoader(t *testing.T) { | ||
| mock := &mockDocumentLoader{ | ||
| delegate: ld.NewDefaultDocumentLoader(http.DefaultClient), |
There was a problem hiding this comment.
This delegate (and the one on line 57) fetches https://www.w3.org/2018/credentials/v1 over HTTP during signing — it is the only test in the repo that fails without network, which contradicts the EmbeddedContextLoader this PR adds; use common.NewVerificationDocumentLoader(nil) instead. Also worth fixing while here: the else { return } on line 63 is taken in both table cases, so the token assertions below it never run — which is why the M2M signing regressions in this PR went unnoticed.
4 participants
Closes the silent security gap where JSON-LD / ldp_vc credentials and presentations were parsed structurally but never cryptographically verified, allowing attackers to submit unsigned or forged JSON-LD VPs.