-
Notifications
You must be signed in to change notification settings - Fork 16
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
IAM: Handle s2s token request #2518
Conversation
ca72d15
to
96c8c85
Compare
We need to do opaque tokens since we need access to the entire VC/VP, they are too big to put in a header. Introspection will need to get them from session storage. |
a2473cd
to
6c6f0a3
Compare
Rebased, changes:
|
Further VC/VP verification optimizations are part of #2528 (since it's existing code). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The presentation submission is incomplete without path_nested
, but I'm not sure if that is blocking now.
OpenID4VP §6.1 has the following example of a presentation submission for a VP with nested credentials that matches our use case
{
"id": "Presentation example 1",
"definition_id": "Example with selective disclosure",
"descriptor_map": [
{
"id": "ID card with constraints",
"format": "ldp_vp",
"path": "$",
"path_nested": {
"format": "ldp_vc",
"path": "$.verifiableCredential[0]"
}
}
]
}
but I think JSONPath/PEX also allows this to be expressed as
{
"id": "Presentation example 1",
"definition_id": "Example with selective disclosure",
"descriptor_map": [
{
"id": "ID card with constraints",
"format": "ldp_vc",
"path": "$.verifiableCredential[0]"
}
]
}
IMO the former is nicer since you get the format of the VP, but not strictly necessary. Maybe we can get away with skipping path_nested
for now?
ffb6e48
to
1981064
Compare
@woutslakhorst @gerardsn rewritten most of PR based on introduced |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from Gerards comments, the handle looks quite good. The rest of the PR is a bit cloudy since there have been some changes in master that should be merged first.
vcr/holder/wallet.go
Outdated
@@ -118,12 +119,16 @@ func (h wallet) buildJWTPresentation(ctx context.Context, subjectDID did.DID, cr | |||
jwt.IssuerKey: subjectDID.String(), | |||
jwt.SubjectKey: subjectDID.String(), | |||
jwt.JwtIDKey: id.String(), | |||
"nonce": crypto.GenerateNonce(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be challenge from options.proofOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
challenge is not the same as nonce? I'd say nonce is a technical random value included in the signature to prevent replay attacks, while challenge is a functional value (like the IRMA contract message)? In JSON-LD Proofs they're 2 different fields anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openid4vp requires it to be passed
vcr/credential/util.go
Outdated
// ResolveSubjectDID resolves the subject DID from the given credentials. | ||
// It returns an error if: | ||
// - the credentials do not have the same subject DID. | ||
// - the credentials do not have a subject DID. | ||
func ResolveSubjectDID(credentials ...vc.VerifiableCredential) (*did.DID, error) { | ||
var subjectID did.DID | ||
for _, credential := range credentials { | ||
sid, err := credential.SubjectDID() | ||
if err != nil { | ||
return nil, err | ||
} | ||
if !subjectID.Empty() && !subjectID.Equals(*sid) { | ||
return nil, errors.New("not all VCs have the same credentialSubject.id") | ||
} | ||
subjectID = *sid | ||
} | ||
return &subjectID, nil | ||
} | ||
|
||
// PresenterIsCredentialSubject checks if the presenter of the VP is the same as the subject of the VCs being presented. | ||
// If the presentation signer or credential subject can't be resolved, it returns an error. | ||
// If parsing succeeds and the signer DID is the same as the credential subject DID, it returns true. | ||
func PresenterIsCredentialSubject(vp vc.VerifiablePresentation) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a disclaimer for these methods that this is what we expect for now. This is not required by any international spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure that will add anything, when you feed the Nuts node a Verifiable Credential that it was never meant to understand (e.g. issuer is not a DID), things will also horribly fail. They're util functions that exactly state what they do, so either call 'm or don't?
If it's important to have a clear disctinction between what is standardized and what not (could be useful), we could consider organization code in such way (e.g. defining things like "profile")
747f302
to
3a37275
Compare
Fixes #2495
Noteworthy things:
path_nested
in Presentation Exchanges (not sure, maybe only if we support VCs and VPs in JWT format?) -> support pending (generate presentation submission with path_nested #2563)[]interface{}
checking inPresentationSubmission.Credentials()
, but theoretically there could be presentation definitions that map to compacted arrays in other Verifiable Credential types. So we might need to convert JSON-LD VPs/VCs to plain JSON first (uncompacting arrays). But maybe leave that out of scope for now, and fix it when implementing JWT support?Requires #2566