fix: Attested signature verification gets options#45
Closed
Peeja wants to merge 1 commit into
Closed
Conversation
We need to pass the current resolvers and verifier factories down recursively into the attested signature verification.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression in attested signature verification by ensuring the same validator configuration (notably DID resolvers and verifier factories) used by top-level token validation is also applied when validating the nested proof-invocation inside an attested signature. This restores correct recursive verification behavior that broke in PR #37.
Changes:
- Thread validator options into
attestation.Verifierand pass them tovalidator.ValidateInvocation. - Update
attestation.NewVerifierFactoryto pass the active DID resolver and verifier-factory registry down intoAttestedVerifier. - Add a regression test covering chained
did:mailtoattestation (root → service → alice).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| attestation/verifier.go | Stores validator options on the attested verifier and uses them during invocation validation. |
| attestation/verificationmethod.go | Passes resolver + verifier-factory options into AttestedVerifier to enable recursive verification. |
| attestation/signer_test.go | Adds a test ensuring chained attestations validate with the same resolver/factory configuration as ValidateToken. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
38
to
+45
| v, err := newMultiVerifier(ctx, verifierFactories, doc.CapabilityInvocation.All()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to derive multi-verifier: %w", err) | ||
| } | ||
| return AttestedVerifier(ctx, authorityDid, v), nil | ||
| return AttestedVerifier(ctx, authorityDid, v, | ||
| validator.WithDIDResolver(resolver), | ||
| validator.WithVerifierFactories(verifierFactories), | ||
| ), nil |
Comment on lines
54
to
60
| if inv.Subject() != v.authorityID { | ||
| return false | ||
| } | ||
|
|
||
| if validator.ValidateInvocation(v.ctx, inv) != nil { | ||
| if validator.ValidateInvocation(v.ctx, inv, v.validationOptions...) != nil { | ||
| return false | ||
| } |
Member
Contributor
Author
|
@frrist Whoops, I thought I closed this already. This was an alternative, and then as I was writing out the comparison I realized that yours was just more correct. |
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
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.
We need to pass the current resolvers and verifier factories down recursively into the attested signature verification. This broke in #37, which just missed plugging tab A into slot B.