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

feat: initial proposal to use dpop #67

Closed
wants to merge 3 commits into from
Closed

feat: initial proposal to use dpop #67

wants to merge 3 commits into from

Conversation

tplooker
Copy link
Collaborator

@tplooker tplooker commented Jan 15, 2024

DRAFT PR FOR DISCUSSION

📑 Description

Following discussion across multiple issues and PR's including #59 and #64. This PR represents a draft proposal to use the DPoP HTTP Header as defined in RFC9449 instead of the client attestation pop.

Preview Link

click here for rendered preview of PR

@peppelinux
Copy link

peppelinux commented Jan 16, 2024

A similar vision was brought by me here
https://github.com/vcstuff/draft-ietf-oauth-attestation-based-client-auth/pull/67/files sorry, wrong url, see: #45

generally I agree with this, however we still have an issue with DPoP, as explained below.

There are discussions about the requirement to provide more than a single wallet attestation in eIDAS.
at the same time, for the purpose of a global standard, I see the requirement to provide a way to allow multiple client_assertion, where an ecosystem might require this, for future features.

at the current stage, we can provide multiple client_assertion in a single url (see this) providing their PoP. Using DPoP it is possible for HTTP to carry multiple headers with the same name. This is defined in the HTTP/1.1 specification (RFC 2616). According to the specification, multiple message-header fields with the same field-name may be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list. It must be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma.

however this approach doesn't give the binding of which PoP is related to which client attestation, requiring to the entity that evaluates assertions+pop to parse and validate them in a loop. This loop has computational costs that can be avoided by design, as the current specs allows.

another issue could be that HTTP Header maximum size configured in the httpd services could represent a limit where multiple DPoP are provided. Anyway I feel optimistic about each the DPoP payload size and the number of http headers parameters included in a single http request.

just to be clear:

I prefer DPoP and I love to reuse things without redefining new one.
At the same time the DPoP HTTP header doesn't give a proper mapping to the client assertion where multiple client assertion are provided. The real-world may require this and we should take the best decision in term of design

@bc-pi
Copy link
Contributor

bc-pi commented Jan 16, 2024

Thanks for doing the work on this @tplooker! I am very supportive of this direction. I do wonder if it'd be worthwhile to include some explanatory text with this change about the fact that use of a DPoP proof does not mandate bound access tokens? It was a point of confusion/contention in prior discussions (eg #64 (comment), #64 (comment), etc) which might benefit from a brief mention in this document.

Copy link
Contributor

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

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

I am pretty strongly against this mechanism. I think it is ok to have this as an optimization but mandating using the same PoP for client attestation and DPoP overloads both mechanisms and limits the use-cases.

@tlodderstedt
Copy link
Contributor

If I understand DPoP correctly, this proposals leads to a situation where authentication with an client attestation always leads to an access token bound to the public key in the 'cnf' claim of the attestation.

This means, clients must always bind their access tokens and cannot use different keys for client authentication and the key binding of their access tokens.

Is that correct?

@tplooker
Copy link
Collaborator Author

If I understand DPoP correctly, this proposals leads to a situation where authentication with an client attestation always leads to an access token bound to the public key in the 'cnf' claim of the attestation.

No that is not entirely correct, as @bc-pi pointed out in this comment just because you are using a DPoP proof in the token request to convey the client attestation PoP doesn't mean a DPoP enabled access token has to be issued as a response. You are correct though that in the event a DPoP access token is issued, the access token will be bound to the one in the cnf claim of the client attestation which I personally find a useful simplification. What are the usecases for wanting a seperate key for client auth vs token binding (in the event it is used)?

@tplooker
Copy link
Collaborator Author

I am pretty strongly against this mechanism. I think it is ok to have this as an optimization but mandating using the same PoP for client attestation and DPoP overloads both mechanisms and limits the use-cases.

Please see my comment above to ensure you understand what we are proposing here, I don't think this significantly limits use cases and on the flip side it significantly reduces what needs to be defined in this draft. If you still feel the same can you please elaborate on which usecases you think this would limit?

@peppelinux
Copy link

Is there any reaction about the potential requirement to provide multiple client assertions in a request and my previous comment, shown below:

however this approach doesn't give the binding of which PoP is related to which client attestation, requiring to the entity that evaluates assertions+(d)pop to parse and validate them in a loop. This loop has computational costs that can be avoided by design, as the current specs allows.

Co-authored-by: Brian Campbell <[email protected]>
@paulbastian
Copy link
Collaborator

A similar vision was brought by me here https://github.com/vcstuff/draft-ietf-oauth-attestation-based-client-auth/pull/67/files sorry, wrong url, see: #45

generally I agree with this, however we still have an issue with DPoP, as explained below.

There are discussions about the requirement to provide more than a single wallet attestation in eIDAS. at the same time, for the purpose of a global standard, I see the requirement to provide a way to allow multiple client_assertion, where an ecosystem might require this, for future features.

at the current stage, we can provide multiple client_assertion in a single url (see this) providing their PoP. Using DPoP it is possible for HTTP to carry multiple headers with the same name. This is defined in the HTTP/1.1 specification (RFC 2616). According to the specification, multiple message-header fields with the same field-name may be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list. It must be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma.

however this approach doesn't give the binding of which PoP is related to which client attestation, requiring to the entity that evaluates assertions+pop to parse and validate them in a loop. This loop has computational costs that can be avoided by design, as the current specs allows.

another issue could be that HTTP Header maximum size configured in the httpd services could represent a limit where multiple DPoP are provided. Anyway I feel optimistic about each the DPoP payload size and the number of http headers parameters included in a single http request.

just to be clear:

I prefer DPoP and I love to reuse things without redefining new one. At the same time the DPoP HTTP header doesn't give a proper mapping to the client assertion where multiple client assertion are provided. The real-world may require this and we should take the best decision in term of design

Hi Giuseppe,
the proposed idea in the eIDAS discussion EPIC-09 is the wrong way. Currently client attestation supports one attestation and that's the way it should stay. I have outlined my position here: openid/OpenID4VCI#215

Copy link
Collaborator

@paulbastian paulbastian left a comment

Choose a reason for hiding this comment

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

I have discussed this with my colleagues and we came to the following conclusions:

status quo:

preparation: potentially fetch 2x nonce
Token Request: client_attestation + client_attestation_pop + Dpop proof header
Token Response: Dpop-bound access token

proposed:

preparation: potentially fetch 1x nonce
Token Request: client_attestation + Dpop proof header (client_attestation.cnf == Dpop.header.jwk)
Token Response: Dpop-bound access token

con:

  • concepts are tightly coupled, key must be the same if both concepts want to be used, may have unforeseeable disadvantages
  • benefits in auth code flow are unclear if I would use client attestation at PAR endpoint
  • jwk present in the client attestation JWT is duplicated to jwk header in Dpop proof
  • refresh tokens are automatically dpop-bound?

pro:

For me, the advantages are bigger, so I'm supportive of this. But I would like to get more feedback and understand @Sakurann concerns. Also, some more editorial work may be needed

@tlodderstedt
Copy link
Contributor

tlodderstedt commented Jan 18, 2024

No that is not entirely correct, as @bc-pi pointed out in this comment just because you are using a DPoP proof in the token request to convey the client attestation PoP doesn't mean a DPoP enabled access token has to be issued as a response.

The text @bc-pi is referring to leaves it at the discretion of the AS to decide whether the access token is key bound. How would the client influence that decision?

@tplooker
Copy link
Collaborator Author

The text @bc-pi is referring to leaves it at the discretion of the AS to decide whether the access token is key bound. How would the client influence that decision?

I think that is a general question for DPoP, but one that is important to answer. If by influence you mean the client is able to communicate to the AS that it supports dpop bound access tokens then I would agree, but at the end of the day the decision as to whether to issue a DPop enabled access token is still always at the discretion of the AS. With regard to how this would be achieved practically I would assume client metadata is the most effective way for the client to communicate this and Section 5.2 has the dpop_bound_access_tokens element registered. Although the definition for this parameter might not intuitively appear to fit, I think the intended normative requirements on the AS do? Specifically this statement which applies to this metadata elements value:

If true, the authorization server MUST reject token requests from this client that do not contain the DPoP header.

Alternatively if we don't think this is sufficient perhaps we can consider registering a new client metadata element.

Perhaps @bc-pi you have some further thoughts on this?

@bc-pi
Copy link
Contributor

bc-pi commented Jan 19, 2024

Is there really a reason the client would need to influence that decision? I'd imagine the AS would issue the access token based on expectations/knowledge of the RS(s) it's intended for.

@tlodderstedt
Copy link
Contributor

Is there really a reason the client would need to influence that decision? I'd imagine the AS would issue the access token based on expectations/knowledge of the RS(s) it's intended for.

With the current revision of the spec (before this PR), a client can use an attestation to just authenticate. If it wants a DPoP bound access token, it would add a DPoP header to the request. With the proposal in this PR, this is no longer possible. This effectively means every client using client attestation must also implement DPoP and use it if the AS decides the access token must be key bound.

Also, the client does not have a choice to use different keys for attestation and DPoP. I could think of situations where the DPoP key is managed in a local security module to achieve better performance for subsequent API calls whereas the client attestation key lives in a remote HSM for higher security, which adds significant latency to every signing operation. With the new proposal, the application's performance would suffer simply because any API call might also require a remote signing operation for the DPoP proof.

Also, I would like to understand whether the proposed approach would work with PAR. We will most likely need attestation based client authentication at the PAR endpoint. I assume the DPoP header would be used to perform the authentication only. Is that correct?

@bc-pi
Copy link
Contributor

bc-pi commented Jan 21, 2024

With the current revision of the spec (before this PR), a client can use an attestation to just authenticate. If it wants a DPoP bound access token, it would add a DPoP header to the request. With the proposal in this PR, this is no longer possible. This effectively means every client using client attestation must also implement DPoP and use it if the AS decides the access token must be key bound.

True but ultimately the need to use key bound access token comes from the RS so the client would have to already be capable. I don't think it'd really be an issue in practice. But maybe I'm wrong. Some sort of signaling could be added to this - metadata as Tobias mentioned or a new claim in the DPoP proof or similar.

Also, the client does not have a choice to use different keys for attestation and DPoP. I could think of situations where the DPoP key is managed in a local security module to achieve better performance for subsequent API calls whereas the client attestation key lives in a remote HSM for higher security, which adds significant latency to every signing operation. With the new proposal, the application's performance would suffer simply because any API call might also require a remote signing operation for the DPoP proof.

It is true that this PR doesn't allow the client to use of different keys for attestation and DPoP. I don't know if the kind of scenario you describe is likely enough to warrant the extra complexity of requiring two proofs in the other more common and simpler cases. I tend to aim for simplicity as much as possible/appropriate.

Also, I would like to understand whether the proposed approach would work with PAR. We will most likely need attestation based client authentication at the PAR endpoint. I assume the DPoP header would be used to perform the authentication only. Is that correct?

It would work with PAR the same as at the token endpoint as far as I understand it. The authz code would be key bound as a result too but that'd be invisible to the client who anyway needs to present the same attestation auth and proof when subsequently exchange that auth code. So it'd just work while adding a bit of extra security to the code.

@tplooker
Copy link
Collaborator Author

tplooker commented Jan 21, 2024

With the current revision of the spec (before this PR), a client can use an attestation to just authenticate. If it wants a DPoP bound access token, it would add a DPoP header to the request. With the proposal in this PR, this is no longer possible. This effectively means every client using client attestation must also implement DPoP and use it if the AS decides the access token must be key bound.

I think the most important point here is highlighted by @bc-pi. In the situation where the AS requires DPoP bound access tokens as per an RS policy, whether we use the DPoP proof syntax in this spec or not, it doesn't change the fact that the client won't be able to get an access token it can actually use with the RS.

The only potential gap I see (which I think is highly unlikely) is in the event a client supports this client authentication method, but doesn't support DPoP access tokens AND the RS + AS supports both DPoP and bearer based access tokens, the situation could arise where the client might be sent back a DPoP access token (based on the DPoP header implying support in the token request) which it can't use, when it could have been sent a bearer token. Again as I said above, I find this situation highly unlikely, but I think it could be easily resolved through additional client metadata.

Also, the client does not have a choice to use different keys for attestation and DPoP. I could think of situations where the DPoP key is managed in a local security module to achieve better performance for subsequent API calls whereas the client attestation key lives in a remote HSM for higher security, which adds significant latency to every signing operation. With the new proposal, the application's performance would suffer simply because any API call might also require a remote signing operation for the DPoP proof.

I'm also wary as to how common this use case is and whether the complexity it creates is justifiable. To play the devils advocate, this proposal as it currently stands creates an efficient way (when DPoP access tokens are used), to authenticate that the key the DPoP access token will be bound to, is clearly authenticated back to the client (through the client attestation). If instead we want to continue to support different keys for client authentication and DPoP token binding, the question then becomes how do we get the same assurance e.g that the DPoP key is authenticated for the client? The client attestation PoP, to use the terminology pre this PR would have to somehow reference and authenticate the seperate DPoP key.

It would work with PAR the same as at the token endpoint as far as I understand it. The authz code would be key bound as a result too but that'd be invisible to the client who anyway needs to present the same attestation auth and proof when subsequently exchange that auth code. So it'd just work while adding a bit of extra security to the code.

This is my understanding too.

Copy link

@awoie awoie left a comment

Choose a reason for hiding this comment

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

I'm also supportive of this PR since it simplifies things since I'm unclear about the use cases for which the proposed approach won't work. It is actually a good higher assurance enforcing function.

@@ -117,31 +110,31 @@ To perform client authentication using this scheme, the client instance uses the

The value of the "client_assertion_type" parameter (as defined in {{RFC7521}}) set to "urn:ietf:params:oauth:client-assertion-type:jwt-client-attestation".

The value of the "client_assertion" parameter (as defined in {{RFC7521}}) set to a value containing two JWTs, separated by a '~' character. It MUST NOT contain more or less than precisely two JWTs separated by the '~' character. The first JWT MUST be the client attestation JWT defined in [](#client-attestation-jwt), the second JWT MUST be the client attestation PoP defined in [](#client-attestation-pop-jwt).
The value of the "client_assertion" parameter contains a single JWT known as the client attestation JWT as defined defined in [](#client-attestation-jwt). It MUST NOT contain more than one JWT.
Copy link

Choose a reason for hiding this comment

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

I don't think the second sentence is necessary since it is implied by the former sentence.

Suggested change
The value of the "client_assertion" parameter contains a single JWT known as the client attestation JWT as defined defined in [](#client-attestation-jwt). It MUST NOT contain more than one JWT.
The value of the "client_assertion" parameter contains a single JWT known as the client attestation JWT as defined defined in [](#client-attestation-jwt).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we would want this constraint wouldn't we? Why would you like to remove it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Didn't read your original comment just the suggestion, while I agree I think this makes the normative requirement a little more explicit. I took the language structure from RFC7523

The value of the "client_assertion" parameter contains a single JWT.
It MUST NOT contain more than one JWT.

I'm ok with removing this additional sentence though.

@tplooker
Copy link
Collaborator Author

tplooker commented Jan 26, 2024

As a consequence of discussing some of the advantages this PR brings, a new usecase/requirement surfaced which has been captured in PR #69

@tplooker
Copy link
Collaborator Author

Closing in favour of #74

@tplooker tplooker closed this Mar 28, 2024
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.

7 participants