Ticket 64/work - #129
Merged
Merged
Ticket 64/work#129
Conversation
Plan covers 6 steps: extract multikey helper, decode publicKeyMultibase in DID documents, implement Data Integrity proof verification core (ecdsa-rdfc-2019, eddsa-rdfc-2022), integrate into proof dispatch chain, add end-to-end tests, and optionally support JCS-based suites. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix SHA-256/SHA-384 hashing: make intermediate hash curve-conditional (P-256/Ed25519 use SHA-256, P-384 uses SHA-384) per W3C VC-DI-ECDSA §3.3.3/§3.3.4, in algorithm steps, function spec, and JCS section - Remove misleading "applies its own hash" note from ECDSA verification - Specify IEEE P1363 byte layout for ECDSA proofValue (r||s, not DER) and note to use ecdsa.Verify, not ecdsa.VerifyASN1 - Rephrase "fail-open" to "deferred failure" in Step 2 publicKeyMultibase error handling description - Extend assertProofOptionsCovered description to check both the cryptosuite IRI and its canonicalized object value - Specify VCDM 2.0 context URI (https://www.w3.org/ns/credentials/v2) for Data Integrity proof options, with explicit note not to use data-integrity/v2 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t-64/plan into ticket-64/work Reviewed-on: http://localhost:3001/general-agent-3/VCVerifier/pulls/12
Factor multicodec-to-JWK conversion out of did/did_key.go into a reusable helper so both did:key resolution and publicKeyMultibase decoding in DID documents can share the same code path. - Create did/multikey.go with exported DecodeMultibaseKey and MulticodecToJWK - Export multicodec prefix constants (Ed25519, P-256, P-384, secp256k1) - Move decodeCompressedEC, type constants to multikey.go - Update did_key.go to call MulticodecToJWK instead of local function - Add comprehensive parameterized tests in did/multikey_test.go Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add DecodeMultibaseKeyWithType mid-level helper to eliminate duplicated multibase decode + varint read + length check between DecodeMultibaseKey and KeyVDR.Read - Refactor KeyVDR.Read to use DecodeMultibaseKeyWithType instead of reimplementing the multibase/varint/multicodec sequence - Rename "key data too short for multicodec" test to "key data too short for varint and key" for precision Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ifier' (#13) from ticket-64/step-1 into ticket-64/work Reviewed-on: http://localhost:3001/general-agent-3/VCVerifier/pulls/13 Reviewed-by: wistefan <wistefan@dev-env.local>
Make parseVerificationMethod in did/did_web.go call DecodeMultibaseKey when publicKeyMultibase is present and publicKeyJwk is absent, so that Multikey verification methods (Ed25519, P-256, P-384) produce usable JWKs via JSONWebKey(). This is the prerequisite for Data Integrity proof verification — without it, credentials signed with DI proofs fail at key resolution. Includes comprehensive tests covering all three key types, priority of publicKeyJwk over publicKeyMultibase, graceful handling of invalid multibase values, verification relationships, and end-to-end key resolution through the Registry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Clarify TestKeyResolverWithMultikeyVM docstring in did/ to explicitly state it simulates the resolution path due to import-cycle constraints and cross-references the real integration test - Add TestResolveKeyFromDID_MultikeyVM in verifier/ exercising the actual ResolveKeyFromDID with Ed25519, P-256, and P-384 Multikey VMs - Add TestResolveKeyForRelationship_MultikeyVM in verifier/ verifying verification-relationship enforcement for Multikey VMs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove redundant TestKeyResolverWithMultikeyVM test from did/ package: it only simulated key resolution (reimplementing verifier/key_resolver.go logic), which is already covered by TestParseDIDDocument_MultikeyVerificationMethods in did/ and by real integration tests TestResolveKeyFromDID_MultikeyVM and TestResolveKeyForRelationship_MultikeyVM in verifier/key_resolver_test.go - Remove associated multikeyMockVDR and resolveKeyFromRegistry helper Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ifier' (#14) from ticket-64/step-2 into ticket-64/work Reviewed-on: http://localhost:3001/general-agent-3/VCVerifier/pulls/14
…dsa-rdfc-2022) Add VerifyDataIntegrityProof for W3C Data Integrity proofs using proofValue (multibase-encoded raw signature) instead of detached JWS. Supported cryptosuites: - ecdsa-rdfc-2019: ECDSA with P-256 (SHA-256) or P-384 (SHA-384) - eddsa-rdfc-2022: EdDSA with Ed25519 (SHA-256) Implementation details: - New error sentinels for DI-specific failures (missing proofValue, malformed proofValue, unsupported cryptosuite, key mismatch) - Curve-conditional hash computation (SHA-256 for P-256/Ed25519, SHA-384 for P-384) - IEEE P1363 signature decoding (r||s) for ECDSA verification - buildProofOptions extended to include cryptosuite when present - assertProofOptionsCovered extended to verify cryptosuite IRI coverage - EnsureDataIntegrityContext adds VCDM 2.0 context for DI proof terms - ensureProofContext dispatches between JWS-2020 and VCDM 2.0 contexts Comprehensive tests cover all three key types, tampered documents, tampered proofValues, key-type mismatches, unsupported suites, missing fields, invalid multibase, wrong proof types, and signature length mismatches. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix doc comments on p256CoordSize and p384CoordSize to match identifier names and reference the production p1363CoordinateSize map - Add doc comment to copyMap noting it performs a shallow copy - Add P-384 tampered-document test case to cover the SHA-384 path - Replace local ed25519SignatureSize constant with ed25519.SignatureSize from the standard library Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…of verification core' (#15) from ticket-64/step-3 into ticket-64/work Reviewed-on: http://localhost:3001/general-agent-3/VCVerifier/pulls/15
Wire DataIntegrityProof verification into the existing LDProofChecker pipeline so both JsonWebSignature2020 and DataIntegrityProof proofs are verified end-to-end through the same VerifyCredential/VerifyPresentation API. Changes: - Add selectProofVerifier() dispatch function in ld_proof_checker.go that routes by proof type to the appropriate verification function - Modify verifyLDProofWithCandidateKeys() to use the dispatch function instead of directly calling VerifyLinkedDataProof - Unsupported proof types are rejected with ErrorLDProofUnsupportedType - Add comprehensive integration tests covering: - Valid VC/VP verification with ecdsa-rdfc-2019 (P-256) - Valid VC/VP verification with eddsa-rdfc-2022 (Ed25519) - Multikey verification method type integration - Tampered document rejection - Issuer/holder mismatch rejection - Wrong proof purpose rejection - Unsupported cryptosuite rejection - Unsupported proof type rejection - Verification relationship enforcement - JWS and DI coexistence in same checker instance Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Reuse testP256KeySize from ld_proof_checker_test.go instead of duplicating it as diP256CoordSize in the DI test file - Replace bare type assertion with require.IsType for clearer failure messages when extracting JWS proof in coexistence test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ifier' (#16) from ticket-64/step-4 into ticket-64/work Reviewed-on: http://localhost:3001/general-agent-3/VCVerifier/pulls/16
Add comprehensive e2e tests exercising the full VP parsing pipeline with DataIntegrityProof-signed credentials and presentations: - Full pipeline tests for ecdsa-rdfc-2019 and eddsa-rdfc-2022 - Mixed proof type scenarios (JWS VP + DI credential and vice versa) - Multikey verification method type support - Proof freshness check for DI proofs (parameterized: fresh, stale, disabled) - Tampered credential rejection through DI proofs - Unsigned credential rejection in DI-signed VPs - Holder binding verification with DI proofs Update docs/json-ld-proof-verification.md with a new section on Data Integrity proof verification covering supported cryptosuites, algorithm details, Multikey VM support, cross-checks, and integration with the proof pipeline. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix hash algorithm description in §1 to note SHA-384 for P-384 (was unconditionally SHA-256) - Document suite-specific proof options context: JWS-2020 for JsonWebSignature2020, VCDM 2.0 for DataIntegrityProof - Fix multicodec table to use decoded integer values matching the code constants (0x1200, 0x1201, 0xed, 0xe7) instead of wire-encoded varints - Remove unsupported codecs (X25519, RSA) from the table - Fix algorithm step 2 hash description to be curve-conditional Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tation' (#17) from ticket-64/step-5 into ticket-64/work Reviewed-on: http://localhost:3001/general-agent-3/VCVerifier/pulls/17
|
Please apply one of the following labels to the PR: |
The section named error identifiers and functions that do not exist, and described the proof-type dispatch as permissive where the code rejects: - ErrorLDProofCurveMismatch -> ErrorLDProofCryptosuiteKeyMismatch for a key that does not match the cryptosuite, and ErrorLDProofVerifyDataIntegrity -> ErrorLDProofMalformedProofValue for a proofValue that does not decode. Both named errors belong to other paths. - "dispatch is in LDProofChecker.verifyLDProof ... otherwise it falls through to VerifyLinkedDataProof" described neither the real function (selectProofVerifier) nor the real behaviour: an unrecognized proof type is rejected with ErrorLDProofUnsupportedType, not passed to the JWS verifier. - The multicodec table listed secp256k1 (0xe7) among the codecs that are decoded into JWKs, and as OKP. MulticodecToJWK rejects it, and it is an EC curve. Add a Limitations subsection recording what the implementation does not do yet: proof options are rebuilt from a fixed field set rather than copied from the proof (so expires/nonce/id/previousProof break verification), created is required although the spec makes it optional, the proof options @context is extended rather than copied, and the JCS, selective-disclosure, proof-set and VCDM 1.1 cases are unsupported.
The Known Gaps list still claimed that proofValue-based cryptosuites are "parsed but not verified", which stopped being true when VerifyDataIntegrityProof landed. Replace that entry with what is actually missing: proof options rebuilt from a fixed field set rather than copied from the proof, the created requirement the spec does not impose, the extended rather than copied proof-options context, and the suites that remain unimplemented (JCS, selective disclosure, proof sets/chains, DI on VCDM 1.1). Record in the JSON-LD section what a reader now needs to know first: both proof families are verified, dispatch is exhaustive, the hash is curve-conditional, cryptosuite is covered by the signature, and did/multikey.go is the reason key resolution reaches the signature check at all for a Multikey verification method.
Every other Data Integrity test signs its fixtures with a helper that mirrors the production hash-data construction, so the suite proves self-consistency rather than conformance: the two hashes concatenated in the wrong order, the second ECDSA hash omitted, SHA-256 used for P-384 or the proof options canonicalized under the wrong context would all pass it. Add the three published vectors - eddsa-rdfc-2022 (VC-DI-EDDSA B.1), ecdsa-rdfc-2019 with P-256 (VC-DI-ECDSA A.1) and with P-384 (A.3) - which the specification authors produced and this codebase did not. Each was checked to fail when the hash-data concatenation is reversed. The eddsa vector additionally asserts the two intermediate hashes it publishes, so a canonicalization regression names the input that drifted instead of only reporting a failed signature. The keys are derived through did.DecodeMultibaseKey, so the vectors cover the Multikey decoding path as well. The example context the vectors use is served from memory; it is not security-relevant and stays out of common/contexts.
P-384 was exercised only by the unit tests in common: the verifier-level tests hardcoded P-256, so the combination a P-384 issuer actually produces - Multikey verification methods in the DID document plus SHA-384 hashing through the whole pipeline - was never verified end to end. Generalize the ECDSA signing helper over the key's curve (coordinate size and hash both follow from it, per VC-DI-ECDSA) instead of assuming P-256, and make computeDITestHashData take the hash choice explicitly. Add a P-384 case to the credential dispatch table and a P-384 pipeline test with Multikey verification methods. The freshness helper stays on P-256: those tests are about `created`, not about the curve.
Follows the RELEASE_NOTES_*.md convention: what is now verified, the Multikey prerequisite that had to land first, the dispatch behaviour, the absence of configuration impact, and the suites and proof shapes that stay out of scope. States explicitly that this is an interoperability fix rather than a security fix - the previous behaviour rejected DataIntegrityProof credentials, it did not accept them unverified - since "parsed but not verified" invited the opposite reading.
The proof configuration that is canonicalized and hashed was rebuilt from the seven fields LDProof models. VC-DI-ECDSA 3.2.5 and the JsonWebSignature2020 equivalent use a copy of the whole proof minus its signature member, and the difference is not cosmetic: expires, nonce, id and previousProof are all defined for a proof and all expand to real triples, so the issuer signed over them. Reconstructing the options without them produced a different canonical form and rejected a conformant proof - fail-closed, but the interoperability failure this codebase exists to avoid. LDProof now keeps the proof as it was parsed, and buildVerificationProofOptions copies it minus proofValue/jws. A proof built in code rather than parsed carries no raw map and falls back to the struct, which for it holds everything there is, so signing is unchanged. The @context also follows the specification now: verbatim for DataIntegrityProof (VC-DI-ECDSA 3.2.5 step 4), where EnsureDataIntegrityContext used to append the VCDM 2.0 context. The two coincide for a VCDM 2.0 document, which is why the published test vectors passed either way; they no longer coincide for a document that does not carry it, and there the appended context was the wrong one. JsonWebSignature2020 keeps its suite context - that suite predates VCDM 2.0 and its terms are defined nowhere else. The tests that matter are the tampering ones: carrying a member into the proof options is only worth doing if changing it afterwards is detected.
VC-DATA-INTEGRITY 2.1 makes `created` OPTIONAL, and VC-DI-ECDSA 3.2.5 constrains it only "if proofConfig.created is set". VerifyDataIntegrityProof required it, so a conformant credential proof without one was rejected before its signature was ever checked. Drop the requirement and check the format instead: a value that is present but unparseable is now rejected with ErrorLDProofMalformedCreated rather than carried along as an opaque string that nothing downstream can use. Presentations keep their time bound. VerifyLDVPProofFreshness still rejects a VP proof without `created` on the grants that have no server-issued nonce, which is where replay protection depends on it; credential proofs have validFrom/validUntil for that. JsonWebSignature2020 is left alone: it predates VCDM 2.0, its proofs always carry `created`, and loosening it buys nothing.
`expires` became part of the canonicalized proof configuration when verification started building the options from the proof itself, so it is now covered by the signature - but nothing acted on it. A proof that declares when it stops being valid and is then honoured forever is worse than one that never declared it. LDProofChecker.assertProofNotExpired rejects an expired proof on both entry points, credentials and presentations, tolerating the same clock skew as the freshness check. A proof without `expires` never expires; a value that cannot be parsed is rejected rather than ignored, since a proof declaring an expiry nobody can read has not been shown to be valid. The checker takes its time from a common.Clock so tests can fix it, defaulting to the real one. TestLDProofChecker_ExpiresCannotBeExtended is the test that makes the rest worth having: rewriting `expires` in a captured document breaks the signature instead of prolonging the proof.
VC-DI-ECDSA 3.2.2 and VC-DI-EDDSA 3.1.2 both take "the Multibase decoded base58-btc value". multibase.Decode accepted any alphabet, so a base64url-encoded signature verified here and would be rejected by a conforming verifier - a non-conforming issuer looked interoperable against this implementation alone. Replace the unchecked canonDoc.(string) / canonProof.(string) assertions with a helper that returns an error. They are safe for the N-Quads format the code asks for, but a verification path should not be one assertion away from a panic. Raise the failed publicKeyMultibase decode in the DID document parser from Debug to Warn. The failure is deliberately not fatal - one unusable verification method must not cost the document its other keys - but it otherwise only surfaced much later as a missing verification key for a proof that named that method. Note on VerificationMethod.Value that it keeps the original encoding while jsonWebKey is what callers use, and on TypeMultikey that key resolution deliberately does not branch on the verification method type.
The notes were written against the implementation as it stood and listed three limitations that the following commits removed: proof options rebuilt from a fixed field set, a mandatory created, and an extended rather than copied proof options context. Replace them with what the release actually does - proof members beyond the modelled ones are covered and expires is enforced, created is optional, proofValue is pinned to base58-btc.
Add ecdsa-jcs-2019 and eddsa-jcs-2022. They are the same signatures over the same hash data as their -rdfc- counterparts; only the canonicalization differs, so a cryptosuite identifier now selects a canonicalization and an algorithm independently (dataIntegritySuites) rather than being switched on by name in three places. common/jcs.go implements RFC 8785: ECMAScript number serialization, the five predefined string escapes with lowercase \uhhhh for the remaining control characters, and property names sorted by UTF-16 code units. Sorting the UTF-8 bytes instead would agree for everything below U+FFFF and disagree above it, which is exactly what the RFC's sorting vector is built from. NaN, the infinities and invalid Unicode terminate canonicalization rather than being serialized as something. Two differences from the RDFC path are worth stating: A JCS proof configuration is the proof verbatim, including the @context the proof itself carries - a conforming issuer copies the document's context into the proof before signing (VC-DI-ECDSA 3.3.5). Nothing is substituted for it, and rewriting it breaks the signature. assertProofOptionsCovered does not apply. It exists because JSON-LD expansion silently drops terms the context does not define, which would leave challenge and domain outside the signature; JCS has no expansion step and drops nothing, so every member is covered by construction. Verified against the published W3C vectors for all three JCS suite/curve combinations, and against the RFC 8785 test data for the canonicalizer itself - including the appendix B number samples, where Go's formatting and ECMAScript's disagree on the exponential thresholds and on zero-padded exponents.
|
Please apply one of the following labels to the PR: |
wistefan
added
the
minor
wistefan
marked this pull request as ready for review
September 28, 2026 14:20
golangci-lint reported six issues; its max-same-issues default of 3 hid nine more of the same kind, so this fixes fifteen sites. QF1012: build the \uhhhh escape for a control character from its two hex digits instead of WriteString(fmt.Sprintf(...)). Every character that needs the escape is below U+0020, so the two high digits are constant. QF1008 (12 sites): drop the embedded PublicKey from privKey.PublicKey.X and .Y. SA1019 (2 sites): elliptic.Marshal and elliptic.Curve.IsOnCurve are deprecated. crypto/ecdh's PublicKey.Bytes() is the same uncompressed SEC 1 encoding, and ecdsa.PublicKey.ECDH() performs the same on-curve check - NewPublicKey rejects a point that is not on the curve - so the test still asserts what it asserted before. Verified with staticcheck -checks=all: no QF or SA findings remain. The ST (naming) findings it also reports are pre-existing across the repository and are not enabled in the CI configuration.
vramperez
reviewed
Sep 28, 2026
vramperez
left a comment
vramperez
left a comment
Collaborator
There was a problem hiding this comment.
A few comments on the Data Integrity support: one spec deviation on the JCS path, one hardening suggestion, and some small doc/cleanup items.
VC-DI-ECDSA 3.3.2 step 4, and identically VC-DI-EDDSA 3.3.2, make an @context carried by a JCS proof configuration govern the document as well: the presented document's context must start with every entry of the proof's, in the same order, and the document is canonicalized under the proof's context rather than its own. Neither half was implemented. The document was hashed under whatever context it presented, so a credential whose context was extended after issuance - which the specification explicitly permits - failed with a signature mismatch, and the context the proof carried bound nothing about the document it secures. The divergence was invisible against the published test vectors, where the two contexts are equal by construction. The prefix rule is what bounds the rebinding: entries may be appended, never reordered or replaced, so the base context entry that DetectVCDataModelVersion reads cannot be swapped out.
LDProofChecker.assertProofNotExpired rejects a proof past its expires timestamp, and both its comment and CLAUDE.md justified that by the timestamp being signed. assertProofOptionsCovered - the guard that turns exactly that assumption into a hard failure for created, challenge, domain and cryptosuite - did not list expires. Under a context that does not define the term the expiry expands to nothing, drops out of the canonical proof options and is enforced without ever having been signed. Add it to the list, matched verbatim the way created is: HasNQuad strips the xsd:dateTime datatype, so a proof cannot be covered for an expiry other than the one it presents.
Decoding a publicKeyMultibase in a DID document warned for every failure, including the one that is not a failure of the document: a verification method whose key type cannot verify signatures at all. X25519 key agreement keys are published routinely next to signing keys, so the warning fired on every resolution of a perfectly valid document and trained operators to ignore the line that matters. MulticodecToJWK now wraps ErrorUnsupportedMulticodec, which did_web.go logs at Debug. Warn stays for an actual decoding failure, where the document is malformed and the key silently missing later. did:key keeps failing hard on both: there the multibase value is the identifier, so an undecodable one leaves no key at all.
The branch wrote the code point and continued, in front of a default case that writes the code point: the distinction it documents - range yields RuneError for invalid encoding as well as for a literal U+FFFD - is real, but ValidString has already ruled out the first case, so the comment belongs on the default and the branch does not need to exist. ErrorJCSInvalidString read as though it guarded Data Integrity verification against lone surrogates. It does not: every string on that path comes through encoding/json, which substitutes U+FFFD first. The check is still right for the exported CanonicalizeJSON, which takes Go values directly - say so rather than leaving it to be read as a control it is not. Both behaviours are now pinned by tests, so the deleted branch cannot come back as a silent behaviour change.
The JCS suites landed without the prose catching up. LDProofChecker still named only the two rdfc suites, and three places narrowed the SHA-384 rule to ecdsa-rdfc-2019. The code was right on both counts: selectProofVerifier dispatches on the suite map, and shouldUseSHA384 branches on the suite's signature algorithm rather than its name, so a P-384 key gets SHA-384 under ecdsa-jcs-2019 too - which VC-DI-ECDSA 3.3.4 requires and the published P-384 JCS vector already pins.
vramperez
self-requested a review
September 29, 2026 06:40
vramperez
approved these changes
Sep 29, 2026
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
2 participants
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.
No description provided.