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

add RFC021 vp_token-brearer grant type for oauth2 #265

Merged
merged 28 commits into from
Nov 29, 2023

Conversation

woutslakhorst
Copy link
Member

It specifies a new grant type that can be used to get an access_token.

  • RFC relations.

rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
Copy link
Member

@gerardsn gerardsn left a comment

Choose a reason for hiding this comment

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

first pass

rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
Copy link
Member

@gerardsn gerardsn left a comment

Choose a reason for hiding this comment

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

round 2

rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
@reinkrul
Copy link
Member

reinkrul commented Oct 6, 2023

I think it's missing error codes (default and/or new ones?), e.g. when not all VCs have a, or the same, credentialSubject.id

@woutslakhorst woutslakhorst force-pushed the feature/s2s_bearer_token branch from c0ad69f to 9c39be2 Compare October 9, 2023 07:08
@woutslakhorst
Copy link
Member Author

I think it's missing error codes (default and/or new ones?), e.g. when not all VCs have a, or the same, credentialSubject.id

Added error codes

rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
Copy link
Member

@gerardsn gerardsn left a comment

Choose a reason for hiding this comment

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

good enough for a draft version

Comment on lines 174 to 180
* `iss`: The issuer of the access token (DID).
* `sub`: The subject of the access token (DID), this is the resource owner. Usually the same as the `iss` field.
* `client_id`: The client (DID) that requested the access token.
* `exp`: The expiration time of the access token.
* `iat`: The time the access token was issued.
* `scope`: The granted scope.
* `vcs`: The Verifiable Credentials that were used to request the access token using the same encoding as used in the access token request.
Copy link
Member

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.

The OAuth Assertion Framework [RFC7521] defines generic HTTP parameters for transporting assertions (a.k.a. security tokens) during interactions with a token endpoint.
This section defines specific parameters and treatments of those parameters for use with VP Bearer Tokens as Authorization Grants.

To use a VP as an authorization grant, the client uses an access token request as defined in Section 4 of the OAuth Assertion Framework [RFC7521] with the following specific parameter values and encodings.
Copy link
Member

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?

Copy link
Member Author

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.

rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved

In order for a client to know which Presentation Definition [PE] to use, the Authorization Server MUST provide a Presentation Definition endpoint.
The Presentation Definition endpoint MUST be registered as a `presentation_definition_endpoint` in the Authorization Server metadata [RFC8414].
The Presentation Definition endpoint MUST return a single Presentation Definition that corresponds with the requested scope.
Copy link
Member

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.

Copy link
Member Author

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.

Comment on lines +160 to +161
The endpoint has a single query parameter `scope` that contains the requested scope. The parameter may contain multiple values.
Values are separated by a space and MUST be URL encoded.
Copy link
Member

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?

Copy link
Member Author

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?

rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved

The Authorization Server MAY support an access token introspection endpoint as defined in OAuth 2.0 [RFC7662].
The introspection endpoint SHOULD map the fields as follows:

* `iss`: The issuer of the access token (DID).
* `sub`: The subject of the access token (DID).
* `sub`: The subject of the access token (DID), this is the resource owner. Usually the same as the `iss` field.
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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)

7. The `created` field of the proof MUST be present and contain a valid timestamp. It MUST be before the current time.
8. The `expires` field of the proof MUST be present and contain a valid timestamp. It MUST be after the current time.
9. The difference between the `expires` and `created` fields MUST be equal or less than 5 seconds.
2. The proof of the VP MUST be a valid [JSONWebSignature2020] object. Additional requirements for the proof object:
Copy link
Member

Choose a reason for hiding this comment

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

do we need to support JSON-LD?

rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
* `iss`: The issuer of the access token (DID).
* `sub`: The subject of the access token (DID), this is the resource owner. Usually the same as the `iss` field.
* `client_id`: The client (DID) that requested the access token.
* `exp`: The expiration time of the access token.
* `iat`: The time the access token was issued.
* `scope`: The granted scope.
* `vcs`: The Verifiable Credentials that were used to request the access token using the same encoding as used in the access token request.
* `vps`: The Verifiable Presentations that were used to request the access token using the same encoding as used in the access token request.
Copy link
Member

Choose a reason for hiding this comment

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

I think we discussed adding fields specified by input descriptors?

Copy link
Member Author

Choose a reason for hiding this comment

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

both

Copy link
Member

@reinkrul reinkrul Nov 10, 2023

Choose a reason for hiding this comment

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

I'd prefer only to include credentials in the response, that were input for the access token, meaning only those matched by the presentation definition. Why do you think we need the whole VP?

Adding fields later on is a lot easier than removing them (which is breaking)

Copy link
Member Author

Choose a reason for hiding this comment

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

You shouldn't need any of them if all is done correctly. If we're including any data that was used as input then best to include the raw data which are the vps. The vps will also tell you which wallet signed which vcs. Only including vcs won't give you that info. But we can remove it from the spec and just add them to the implementation?

Copy link
Member

Choose a reason for hiding this comment

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

If the vps are always the vp_token (which is true here), then I think we also need to return the presentation_submission to interpret the contents of the vps.

Copy link
Member

Choose a reason for hiding this comment

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

That's also fine. Might want to choose a style short (acronyms), or long form, though

rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved

The Authorization Server MAY support an access token introspection endpoint as defined in OAuth 2.0 [RFC7662].
The introspection endpoint SHOULD map the fields as follows:

* `iss`: The issuer of the access token (DID).
* `sub`: The subject of the access token (DID).
* `sub`: The subject of the access token (DID), this is the resource owner. Usually the same as the `iss` field.
Copy link
Member

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)

rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
The Authorization Server provides this information in the form of a Presentation Definition [PE].
The complete flow consists of the following steps:

1. The client requests a Presentation Definition from the Authorization Server based on a scope.
Copy link
Member

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

Copy link
Member Author

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.

rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
If the Authorization Server determines that the VP is invalid, the Authorization Server MUST return an error response as defined in OAuth 2.0 [RFC6749].
In addition to the error response defined in OAuth 2.0 [RFC6749], the Authorization Server MUST return a HTTP 400 (Bad Request) and use the following error codes when the VP is invalid:

* `invalid_verifiable_presentation`: The VP is invalid. This error code is used when the signature is incorrect or when a required field is missing.
Copy link
Member

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)

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 all custom errors since it creates too much confusion amongst reviewers.

* `invalid_verifiable_presentation`: The VP is invalid. This error code is used when the signature is incorrect or when a required field is missing.
* `invalid_presentation_submission`: The Presentation Submission is invalid. This error code is used when the Presentation Submission is not an answer to the Presentation Definition that corresponds with the requested scope.

An `invalid_request` is returned for any submitted Verifiable Credentials that do not meet the requirements, when the Verifiable Credentials aren't corresponding to the Presentation Definition or when the Verifiable Credentials are expired, not trusted or invalid.
Copy link
Member

Choose a reason for hiding this comment

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

Kinda ambiguous with "invalid_verifiable_presentation"; if the contained credentials are invalid, it could also be seen as "invalid_verifiable_presentation"?

Also, I think there (ideally) should not be any other "trust" anchors than the one specified by the Presentation Definition. Otherwise, RP and AS still have to agree on a set of trust anchors outside of this specification.

Copy link
Member Author

Choose a reason for hiding this comment

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

We currently decided on putting trust anchors in the PDs, that does not mean that this is mandatory through this RFC. 'Trust' is therefore still required, but undefined how it's solved.

## 7. Security Considerations

The jti/challenge (further called nonce) is used to prevent replay attacks. The nonce is a random string that is generated by the client.
The nonce is included in the signed data. The Authorization Server MUST reject any request that uses a nonce that has been used before.
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

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

implementation opzitmization.

rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
Copy link
Member

@reinkrul reinkrul left a comment

Choose a reason for hiding this comment

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

Just @gerardsn 's comment on nbf/iat left, i.m.o.

Copy link
Member

@gerardsn gerardsn left a comment

Choose a reason for hiding this comment

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

last one

7. The `nbf` field MUST be present and contain a valid unix timestamp. It MUST be before the the current time (time at which the JWT is processed).
8. The `exp` field MUST be present and contain a valid unix timestamp. It MUST be after the current time (time at which the JWT is processed).
9. The difference between the `exp` and `nbf` fields MUST be equal or less than 5 seconds.
10. The `jti` field MUST be present and contain a string that is unique for each access token request.
Copy link
Member

Choose a reason for hiding this comment

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

the vc-data-model maps jti to the VC/VP id value. A value should not have 2 meanings, so we should use nonce here too. We're then using nonce for replay prevention in both data formats which is a nice bonus I'd say.

Copy link
Member

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

### 4.4 Preventing Token Replay

The Authorization Server MUST reject any request that uses the a value for `jti` (JWT proofs) or `nonce` (JSON Web Proofs) that has been used before.
This approach has been chosen over the `nonce` field because there's no initial request to get a nonce from the Authorization Server.
Copy link
Member

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 RP

Suggested change
This approach has been chosen over the `nonce` field because there's no initial request to get a nonce from the Authorization Server.

rfc/rfc021-vp_token-grant-type.md Outdated Show resolved Hide resolved
@woutslakhorst woutslakhorst merged commit 469d6ee into master Nov 29, 2023
@woutslakhorst woutslakhorst deleted the feature/s2s_bearer_token branch November 29, 2023 13:59
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.

3 participants