Add signature checks for Verifiable Presentations #353

decentralgabe · 2023-04-14T18:10:10Z

Fixes #71

make all signer/verifier methods generic
shuffle packages to remove cycles
add signature checks for VPs
move in method to resolve a key given a DID and KID
disabled improperly implemented limited disclosure, opened Presentation Exchange: support limiting disclosure with ZKP and Selective Disclosure Credentials #354
had to refactor the test cases to accomodate real signed VCs / verifiers, updated the test wallet accordingly

decentralgabe · 2023-04-14T23:42:45Z

credential/signature.go

+
+// VerifyCredentialSignature verifies the signature of a credential of any type
+// TODO(gabe) support other types of credentials https://github.com/TBD54566975/ssi-sdk/issues/352
+func VerifyCredentialSignature(ctx context.Context, genericCred any, resolver did.Resolver) (bool, error) {


important stuff here

codecov-commenter · 2023-04-14T23:44:59Z

Codecov Report

Merging #353 (92684a2) into main (dd225d5) will increase coverage by 0.42%.
The diff coverage is 63.21%.

@@            Coverage Diff             @@
##             main     #353      +/-   ##
==========================================
+ Coverage   57.38%   57.80%   +0.42%     
==========================================
  Files          51       52       +1     
  Lines        6410     6566     +156     
==========================================
+ Hits         3678     3795     +117     
- Misses       2032     2063      +31     
- Partials      700      708       +8

Impacted Files	Coverage Δ
credential/exchange/request.go	`40.24% <0.00%> (-2.06%)`	⬇️
cryptosuite/bbsplussignaturesuite.go	`52.83% <ø> (ø)`
credential/verification/verifiers.go	`45.00% <20.00%> (-15.00%)`	⬇️
credential/exchange/verification.go	`50.81% <45.45%> (-2.19%)`	⬇️
credential/util.go	`46.81% <46.81%> (ø)`
crypto/jwt.go	`44.78% <66.67%> (ø)`
credential/jwt.go	`39.66% <68.42%> (ø)`
credential/signature.go	`80.00% <80.00%> (ø)`
credential/exchange/submission.go	`72.62% <84.62%> (-0.43%)`	⬇️
credential/jws.go	`52.63% <100.00%> (ø)`
... and 4 more

... and 3 files with indirect coverage changes

nitro-neal · 2023-04-17T21:26:49Z

credential/exchange/verification.go

+			return fmt.Errorf("verifier<%T> is not a JWT verifier", verifier)
+		}
+		// verify the VP, which in turn verifies all credentials in it
+		_, _, vp, err := credential.VerifyVerifiablePresentationJWT(context.Background(), jwtVerifier, resolver, string(submission))


eventually context.background should be top level

I agree, I will make it top level here

nitro-neal · 2023-04-17T21:32:04Z

credential/jwt.go

+// After decoding the signature of each credential in the presentation is verified. If there are any issues during
+// decoding or signature validation, an error is returned. As a result, a successfully decoded VerifiablePresentation
+// object is returned.
+func VerifyVerifiablePresentationJWT(ctx context.Context, verifier crypto.JWTVerifier, resolver did.Resolver, token string) (jws.Headers, jwt.Token, *VerifiablePresentation, error) {


nitro-neal · 2023-04-17T21:33:08Z

credential/signature.go

+	if genericCred == nil {
+		return false, errors.New("credential cannot be empty")
+	}
+	if resolver == nil {


if the resolver was empty could there be a default one we load?

I'm not sure if we should, because that would imply supporting DID methods that the caller may not want to resolve

nitro-neal · 2023-04-17T21:35:19Z

credential/signature.go

+	case *VerifiableCredential:
+		return VerifyCredentialSignature(ctx, *genericCred.(*VerifiableCredential), resolver)
+	case VerifiableCredential:
+		cred := genericCred.(VerifiableCredential)


no ok checking here?
cred, ok := genericCred.(VerifiableCredential)
I guess we know the type so not needed

yeah since the switch was hit we know the type

I'm not sure how to make it nicer without some crazy generic stuff

nitro-neal · 2023-04-17T21:37:47Z

credential/signature.go

+
+		// if that fails, try as a JWT
+		if _, _, _, err := VCJWTJSONToVC(credMapBytes); err == nil {
+			return false, errors.New("JWT credentials must include a signature to be verified")


Is this the right error message here?

It could just be an invalid map[string]any

removed this bit

nitro-neal · 2023-04-17T21:42:26Z

credential/util.go

+)
+
+// ToCredential turn a generic cred into its known object model
+func ToCredential(genericCred any) (jws.Headers, jwt.Token, *VerifiableCredential, error) {


A lot of code reuse, would it be possible to Call this funciton in the signature.go VerifyCredentialSignature?

good idea, I think it can be used for everything but the JWT case

nitro-neal · 2023-04-17T21:43:58Z

example/usecase/employer_university_flow/pkg/verifier.go

-	_, _, vp, err := signing.VerifyVerifiablePresentationJWT(verifier, string(credBytes))
+// 1. That the VP is valid
+// 2. All VCs in the VP are valid
+// 2. That the VC was issued by a trusted entity (implied by the presentation, according to the Presentation Definition)


nitro-neal

.

nitro-neal

Looks great! Just a few spots to look over and think about:
Biggest stuff is perhaps infinite loop / stack overflow problem if we are not careful with the return VerifyCredentialSignature

With the context, I wonder if we can extract it away and it will only be created once in the resolver or something

nitro-neal · 2023-04-17T21:50:12Z

credential/signature.go

+	}
+	switch genericCred.(type) {
+	case *VerifiableCredential:
+		return VerifyCredentialSignature(ctx, *genericCred.(*VerifiableCredential), resolver)


Are we sure we won't run into an infinite loop problem here or at the other
return VerifyCredentialSignature spots?

sure...maybe not. I have each case covered in a test that should be "good enough"

decentralgabe added 8 commits April 13, 2023 13:19

generic signers and verifiers

a76cb27

progress

e2ac1c9

more tests

5244d68

bad renames

14c5d7f

disabling limited disclosure

eaa11bc

give the university case a glow up

b913b69

lints

9462b3c

fix apt application

0e9a119

decentralgabe marked this pull request as ready for review April 14, 2023 23:38

decentralgabe requested review from nitro-neal and andresuribe87 as code owners April 14, 2023 23:38

remove schemas

c8d11b9

decentralgabe commented Apr 14, 2023

View reviewed changes

bad renames

eae2eca

decentralgabe added 2 commits April 15, 2023 12:04

test for verifying jwt cred

1ffc9cc

tests

9540ba7

decentralgabe added sign-verify credentials labels Apr 15, 2023

decentralgabe self-assigned this Apr 15, 2023

decentralgabe added this to the BTC Miami milestone Apr 15, 2023

nitro-neal reviewed Apr 17, 2023

View reviewed changes

nitro-neal approved these changes Apr 17, 2023

View reviewed changes

decentralgabe added 2 commits April 17, 2023 16:10

pr comments

036678a

fix lint

92684a2

decentralgabe merged commit 5d3dce1 into main Apr 18, 2023

decentralgabe deleted the pe-signature-checks-71 branch April 18, 2023 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add signature checks for Verifiable Presentations #353

Add signature checks for Verifiable Presentations #353

decentralgabe commented Apr 14, 2023 •

edited

Loading

decentralgabe Apr 14, 2023

codecov-commenter commented Apr 14, 2023 •

edited

Loading

nitro-neal Apr 17, 2023

decentralgabe Apr 17, 2023

nitro-neal Apr 17, 2023

nitro-neal Apr 17, 2023

decentralgabe Apr 17, 2023

nitro-neal Apr 17, 2023

decentralgabe Apr 17, 2023 •

edited

Loading

decentralgabe Apr 17, 2023

nitro-neal Apr 17, 2023

nitro-neal Apr 17, 2023

decentralgabe Apr 17, 2023

nitro-neal Apr 17, 2023

decentralgabe Apr 17, 2023

nitro-neal Apr 17, 2023

decentralgabe Apr 17, 2023

nitro-neal left a comment

nitro-neal left a comment

nitro-neal Apr 17, 2023

decentralgabe Apr 17, 2023

Add signature checks for Verifiable Presentations #353

Add signature checks for Verifiable Presentations #353

Conversation

decentralgabe commented Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Apr 14, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

decentralgabe Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitro-neal left a comment

Choose a reason for hiding this comment

nitro-neal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

decentralgabe commented Apr 14, 2023 •

edited

Loading

codecov-commenter commented Apr 14, 2023 •

edited

Loading

decentralgabe Apr 17, 2023 •

edited

Loading