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.
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
add RFC021 vp_token-brearer grant type for oauth2 #265
add RFC021 vp_token-brearer grant type for oauth2 #265
Changes from 19 commits
3055386
ac52094
ae71774
ac552e2
41918d8
d6dbbf8
4995470
fdb9f2c
a4d5481
0d04aa2
cc3d2e4
9c39be2
60d54cc
4716cea
d5ed03d
e70e169
6e8d31b
04021d5
3797e92
b82d74f
3186b22
39ee82d
c0b8639
8af6a96
38a97c8
67c6ae1
062a5e7
221c386
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
client or relying party? Client is the software instance, relying party the entity using the software instance to communicate with the AS
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.
These point use client and AS as the actual systems doing the requests.
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.
Does the grant always contain a single VP (makes sense for s2s flow) or can it contain a VP array as in OpenID4VP?
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.
Theoretically a server could combine multiple wallets... (like vendor and tenant). Let's start with 1 and see how far we get.
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.
Arbitrary, I'd say the 5 and 10 seconds are up to the implementation to decide. But we can give an example.
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.
Both sides must use the same value, it's therefore part of the spec and not the implementation.
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.
I saw some DID URLs (I think verificationMethod IDs) that just contained a fragment. We might have to support that in (near) future
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.
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.
Better to make it totally unique, that way it's still unique for the issuer.
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
vc-data-model
mapsjti
to the VC/VPid
value. A value should not have 2 meanings, so we should usenonce
here too. We're then usingnonce
for replay prevention in both data formats which is a nice bonus I'd say.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 security considerations section needs some rewording if this is added
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.
After seeing more specs the
nonce
value (in JWTs) can be issued by both AS and RPThere 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 it be "invalid assertion"? (as that reflects the parameter name)
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.
removed all custom errors since it creates too much confusion amongst reviewers.
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.
OAuth generally allows the AS to return an answer for a smaller scope than requested. Probably doesn't make sense for our use cases, but might if we want publish outside our domain. In that case the endpoint also needs to return the scope the PD is for.
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.
Let's see if we get that feedback from the rest of the world, since it would change the return value of the API.
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.
point to standard OAuth definition for scope?
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.
There it's mostly used as form encoded value and not as query param?
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.
this section is not really add anything useful?
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.
Entire chapter 6 or specific fields?
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.
May be useful after all, since we don't have a separate introspection RFC, and it adds some claims specific for this RFC (regarding the input descriptors)
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.
add
active
: (required per rfc7662) boolean indicating if the token is valid (syntax, iat < now > exp, valid signature, not revoked, etc.).vcs
should be replaced by the PDP map and friends as discussed, see nuts-foundation/nuts-node#2567. I'm fine updating this at a later point though.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.
but it only needs to store them for as long as the presentation is valid
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.
implementation opzitmization.