Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update to latest vc-jwt spec; differentiate between id and key id #341

Merged
merged 4 commits into from
Apr 7, 2023

Conversation

decentralgabe
Copy link
Member

fixes #340

// normalizePresentationClaims takes a set of Presentation Claims and turns them into map[string]any as
// go-JSON representations. The claim format and signature algorithm type are noted as well.
// This method is greedy, meaning it returns the set of claims it was able to normalize.
func normalizePresentationClaims(claims []exchange.PresentationClaim) []exchange.NormalizedClaim {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed some dead code along the way

@codecov-commenter
Copy link

codecov-commenter commented Apr 7, 2023

Codecov Report

Merging #341 (1cf728a) into main (025a20e) will decrease coverage by 0.19%.
The diff coverage is 45.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #341      +/-   ##
==========================================
- Coverage   57.51%   57.32%   -0.19%     
==========================================
  Files          51       51              
  Lines        6201     6232      +31     
==========================================
+ Hits         3566     3572       +6     
- Misses       1967     1983      +16     
- Partials      668      677       +9     
Impacted Files Coverage Δ
credential/signing/jwt.go 30.43% <28.41%> (-2.48%) ⬇️
credential/exchange/submission.go 74.37% <57.14%> (-0.71%) ⬇️
credential/signing/jws.go 52.63% <62.50%> (+1.28%) ⬆️
crypto/jwt.go 46.41% <72.41%> (+0.26%) ⬆️
credential/exchange/request.go 42.31% <100.00%> (ø)
credential/exchange/verification.go 52.38% <100.00%> (ø)
credential/util/util.go 29.55% <100.00%> (ø)
cryptosuite/jsonwebkey2020.go 47.48% <100.00%> (ø)

Copy link
Contributor

@nitro-neal nitro-neal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of changes, I hope our test vectors are up to par. Good stuff!

@@ -18,7 +18,7 @@ import (

func TestBuildPresentationSubmission(t *testing.T) {
t.Run("Unsupported embed target", func(tt *testing.T) {
_, err := BuildPresentationSubmission(crypto.JWTSigner{}, PresentationDefinition{}, nil, "badEmbedTarget")
_, err := BuildPresentationSubmission(crypto.JWTSigner{}, "requester", PresentationDefinition{}, nil, "badEmbedTarget")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if "requester" and "submitter" are things we will use mostly in production may be good to make a const

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not used in production as these strings. I would imagine they're each the DIDs of the requester/submitter

return nil, errors.Wrap(err, "could not set nbf value")
}
// remove the issuance date from the credential
cred.IssuanceDate = ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to spec?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, strange to me too - see the spec text below and examples https://w3c.github.io/vc-jwt/#jwt-decoding

In the example above, vc does not contain the id property because the JWT encoding uses the jti attribute to represent a unique identifier. The sub attribute encodes the information represented by the id property of credentialSubject


idVal := cred.ID
if idVal != "" {
if err := t.Set(jwt.JwtIDKey, idVal); err != nil {
return nil, errors.Wrap(err, "could not set jti value")
}
// remove the id from the credential
cred.ID = ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too

}

subVal := cred.CredentialSubject.GetID()
if subVal != "" {
if err := t.Set(jwt.SubjectKey, subVal); err != nil {
return nil, errors.Wrap(err, "setting subject value")
}
// remove the id from the credential subject
delete(cred.CredentialSubject, "id")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too

// JWTVVPParameters represents additional parameters needed when constructing a JWT VP as opposed to a VP
type JWTVVPParameters struct {
// Audience is a required intended audience of the JWT.
Audience string `validate:"required"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an awesome field btw

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah turns out for JWTs you must have an intended audience. cc: @amika-sq

// BuildPresentationSubmission builds a submission using...
// https://github.com/TBD54566975/ssi-sdk/blob/d279ca2779361091a70b8aa3c685a388067409a9/credential/exchange/submission.go#L126
func BuildPresentationSubmission(presentationRequest []byte, signer crypto.JWTSigner, verifier crypto.JWTVerifier, vc credential.VerifiableCredential) ([]byte, error) {
func BuildPresentationSubmission(presentationRequestJWT string, signer crypto.JWTSigner, vc credential.VerifiableCredential) ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] JWT Verifiable Presentations should utilize the iss property
3 participants