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

Is c_nonce required in proof or not? #331

Closed
awoie opened this issue May 27, 2024 · 35 comments · Fixed by #365
Closed

Is c_nonce required in proof or not? #331

awoie opened this issue May 27, 2024 · 35 comments · Fixed by #365
Assignees

Comments

@awoie
Copy link
Contributor

awoie commented May 27, 2024

Nonces in proof types are OPTIONAL but the following sentence confused me:

The proof element MUST incorporate the Credential Issuer Identifier (audience), and a c_nonce value generated by the Authorization Server or the Credential Issuer to allow the Credential Issuer to detect replay.

It says MUST ..., and a c_nonce. Does that MUST refer to c_nonce as well? Isn't this conflicting with the optionality of nonces in proofs? If it is not conflicting then it is at least confusing.

@awoie
Copy link
Contributor Author

awoie commented May 27, 2024

Assuming it is NOT required, I'd propose changing the text to:

The proof element MUST incorporate the Credential Issuer Identifier (audience), and if provided a c_nonce value generated by the Authorization Server or the Credential Issuer to allow the Credential Issuer to detect replay.

@awoie
Copy link
Contributor Author

awoie commented May 27, 2024

This text makes it probably more clear:

The proof element MUST incorporate the Credential Issuer Identifier (audience), and a nonce value if a c_nonce value was provided by the Authorization Server or the Credential Issuer to allow the Credential Issuer to detect replay.

@awoie awoie changed the title Is c_nonce required or not? Is c_nonce required in proof or not? May 27, 2024
@Sakurann
Copy link
Collaborator

I think c_nonce is currently meant to be mandatory (either from a token endpoint or from credential endpoint in the error response)...
but I know an implementer for whom adding c_nonce was a hurdle to implement VCI, so they implemented at_hash in the proof instead to prevent replay and achieve the same result.

@awoie
Copy link
Contributor Author

awoie commented May 27, 2024

I think c_nonce is currently meant to be mandatory (either from a token endpoint or from credential endpoint in the error response)... but I know an implementer for whom adding c_nonce was a hurdle to implement VCI, so they implemented at_hash in the proof instead to prevent replay and achieve the same result.

Okay but why is nonce OPTIONAL in the proof in that case if it was meant to be mandatory? Does it mean that this particular implementer used at_hash for nonce in the jwt PoP?

@awoie
Copy link
Contributor Author

awoie commented May 27, 2024

For reference, from the spec:

nonce: OPTIONAL (string). The value type of this claim MUST be a string, where the value is a server-provided c_nonce. It MUST be present when the Wallet received a server-provided c_nonce.

@awoie
Copy link
Contributor Author

awoie commented May 27, 2024

For reference, from the spec:

nonce: OPTIONAL (string). The value type of this claim MUST be a string, where the value is a server-provided c_nonce. It MUST be present when the Wallet received a server-provided c_nonce.

This suggests that under some circumstances the intention was to allow JWT proofs without a nonce.

@Sakurann
Copy link
Collaborator

Sakurann commented May 27, 2024

because c_nonce can be returned either from the token endpoint or credential error response. and when it is received from credential error response, the first proof sent from the wallet cannot have a nonce parameter because the wallet is yet to receive it in the error..

@awoie
Copy link
Contributor Author

awoie commented May 27, 2024

because c_nonce can be returned either from the token endpoint or credential error response. and when it is received from credential error response, the first proof sent from the wallet cannot have a nonce parameter because the wallet is yet to receive it in the error..

Why would somebody create an "always" invalid proof (i.e. without a nonce) in that case? That sounds strange to me. Wouldn't it make more sense to send no proof (it is optional anyways) in this case?

@awoie
Copy link
Contributor Author

awoie commented May 27, 2024

because c_nonce can be returned either from the token endpoint or credential error response. and when it is received from credential error response, the first proof sent from the wallet cannot have a nonce parameter because the wallet is yet to receive it in the error..

Why would somebody create an "always" invalid proof (i.e. without a nonce) in that case? That sounds strange to me. Wouldn't it make more sense to send no proof (it is optional anyways) in this case?

And in that case, why make nonce OPTIONAL if it is required anyways to get a valid credential?

If nonce is really always required, then I would rather remove optionality by making nonce etc. mandatory and send no proof at all to get a fresh nonce from the issuer.

@bc-pi
Copy link
Member

bc-pi commented May 28, 2024

I hereby concur with @awoie that the current treatment of c_nonce into the nonce/challenge/Claim Key 10 (Nonce) of the proof(s) of the Credential Request is, if not conflicting, then at least confusing. I'd actually been meaning to raise an issue about it myself but hadn't yet managed to summon the time or energy to do so.

It has been stated elsewhere (slack, signal, email, etc., I can't keep track) that this the same mechanism as DPoP. But as currently written in VCI, it is definitely not the same.

@Sakurann
Copy link
Collaborator

Sakurann commented May 29, 2024

WG discussion/proposal on the table:

  • make c_nonce optional (credential ). credential endpoint has an option to respond with an c_nonce if it wants to require it (DPoP behavior)
  • ath (access token hash) mandatory (?) in the proof - does not prevent replay if access token is long-lived, might be an incentive for issuer to do c_nonce (issue VC Issuance is vulnerable to Unknown Key Share attacks #19)
  • (potentially, also no c_nonce from the token endpoint (Remove c_nonce from the token response #39), but decided to deal with it in the separate issue)
  • (proof does not have jti, so that cannot be used to detect proofs encountered before..)

@peppelinux
Copy link
Member

(potentially, also no c_nonce from the token endpoint (#39), but decided to deal with it in the separate issue)

I support this. The RS is technically able to evaluate if the AT was previously used. The Credential Endpoint can issue the c_nonce within the credential response. c_nonce is optional, because it is optional that the Credential Endpoint issues more than a credential using the same AT.

Removing c_nonce in the token endpoint response eliminates the need to modify legacy AS implementations while maintaining security against replay attacks and preventing the reuse of the AT at the RS (Credential Endpoint).

@awoie
Copy link
Contributor Author

awoie commented Jun 2, 2024

I'm still in favor of allowing the credential issuer to decide whether or not to accept proofs without a c_nonce value. But this is a separate discussion.

In any case, this language has to be adapted since it confuses wallet implementers:

The proof element MUST incorporate the Credential Issuer Identifier (audience), and a c_nonce value generated by the Authorization Server or the Credential Issuer to allow the Credential Issuer to detect replay.

Independent of whether or not c_nonce is required, the language above has to be adapted to something something like this:

The proof element MUST incorporate the Credential Issuer Identifier (audience) and if provided a c_nonce value generated by the Authorization Server or the Credential Issuer to allow the Credential Issuer to detect replay.

This would allow for:

  1. wallet hasn't received a c_nonce but has an access_token, then the wallet can create a valid in terms of spec-compliant credential request without a c_nonce.
  2. wallet has received a c_nonce from the token endpoint, then the wallet can create a valid in terms of spec-compliant credential request with a c_nonce value.

Otherwise, wallets cannot conform to the spec since they would not know where to get the c_nonce from if they haven't received one from the token endpoint and stop processing since the spec requires a c_nonce based on the original language. All language that relates to issuer sending invalid_proof plus c_nonce is issuer-related, not wallet-related. The wallet just reacts to invalid_proof error responses that may or may not include a c_nonce.

@paulbastian
Copy link
Contributor

I am not in favor of using ath as the lifetime of the access token is unspecified, so I would not rely on this. Instead I would mandate c_nonce, optionally from AS and mandatory from RS.

@tplooker
Copy link
Contributor

tplooker commented Jun 4, 2024

I broadly agree with the sentiment expressed in this issue.

Server generated nonces should be an optional feature for credential issuers to implement, not mandatory. Akin to server generated nonces in DPoP. That means the language as raised by @awoie around how a wallet creates a valid proof, needs to highlight they may or may not have a nonce depending on whether the credential issuer supports this feature.

I also agree with @peppelinux that we should consider removing the c_nonce from the token endpoint because of the difficulty some implementations have with modifying the token response.

@bc-pi
Copy link
Member

bc-pi commented Jun 4, 2024

I broadly agree with the sentiment expressed by @tplooker in his broad agreement with the sentiment expressed in this issue.

@Sakurann
Copy link
Collaborator

Sakurann commented Jun 11, 2024

WG discussion:

  1. no c_nonce from the token endpoint.
  2. c_nonce from the credential endpoint is optional. returned in the credential error response if needed.
  3. is attack in VC Issuance is vulnerable to Unknown Key Share attacks #19 worth protecting against? if yes, ath probably makes sense. if not no need. -> direction seems to be making ath mandatory.

@ve7jtb thinks key attestation greatly includes this model.

@peppelinux
Copy link
Member

  1. no c_nonce from the token endpoint.

hurrà!

  1. c_nonce from the credential endpoint is optional. returned in the credential error response if needed.

I would change to:

c_nonce from the credential endpoint is required when the VCI supports multiple issuance of multiple or the same credential type(s) using the same access token.

it doesn't make any sense returning a c_nonce in an error since an adversary that steals the access token may raise an error and get the credential with the bearer token. I really would stop using nonce issuance through errors :-) they represent a sort of information gathering by design. If the client misses the nonce: good bye, restart the flow.

  1. yes, ath represents a required enforcement.

@jogu
Copy link
Contributor

jogu commented Jun 12, 2024

We should keep discussion about removing c_nonce from token endpoint under the existing issue for that: #39

However just to clarify the reasoning given above for removing c_nonce:

Removing c_nonce in the token endpoint response eliminates the need to modify legacy AS implementations while maintaining security against replay attacks and preventing the reuse of the AT at the RS (Credential Endpoint).

This is not correct; the spec deliberately does not require legacy AS implementations to be modified, if such deployments want to use c_nonce they can choose to return it only at the credential endpoint.

@peppelinux
Copy link
Member

yes @jogu, you know that I am not a fan of security-through-optionality :-)

@Sakurann
Copy link
Collaborator

discussed on the WG call, does not feel like ready for PR, yet. few more people will review this issue.

@awoie
Copy link
Contributor Author

awoie commented Jun 21, 2024

discussed on the WG call, does not feel like ready for PR, yet. few more people will review this issue.

I couldn't attend the meeting. Which part is not ready, removing c_nonce from token endpoint, or fixing the broken language? The minutes are not clear enough. Would it help if we create a new issue on "removing c_nonce from token endpoint", so we can fix the broken language?

@jogu
Copy link
Contributor

jogu commented Jun 21, 2024

We have an existing issue on removing c_nonce from token endpoint (which has a few new comments on it): #39

I'm pretty sure the discussion on the WG call yesterday was only about that issue, I don't think we discussed the broken language.

@awoie
Copy link
Contributor Author

awoie commented Jun 21, 2024

We have an existing issue on removing c_nonce from token endpoint (which has a few new comments on it): #39

I'm pretty sure the discussion on the WG call yesterday was only about that issue, I don't think we discussed the broken language.

Thanks, got it. Perhaps we can talk about the broken language on the next call to clarify whether this is ready for PR and keep the discussion on removal of c_nonce from the token endpoint related to this issue #39.

@bc-pi
Copy link
Member

bc-pi commented Jun 21, 2024

We have an existing issue on removing c_nonce from token endpoint (which has a few new comments on it): #39
I'm pretty sure the discussion on the WG call yesterday was only about that issue, I don't think we discussed the broken language.

Thanks, got it. Perhaps we can talk about the broken language on the next call to clarify whether this is ready for PR and keep the discussion on removal of c_nonce from the token endpoint related to this issue #39.

No one could have possibly anticipated it[1] but the very existence of the c_nonce from the token endpoint has contributed to miscommunication and distraction around this issue.

[1] https://i.kym-cdn.com/photos/images/original/001/044/247/297.png

@jogu
Copy link
Contributor

jogu commented Jun 21, 2024

Thanks, got it. Perhaps we can talk about the broken language on the next call to clarify whether this is ready for PR and keep the discussion on removal of c_nonce from the token endpoint related to this issue #39.

Sounds good - I'll put it on the agenda for Tuesday.

@jogu
Copy link
Contributor

jogu commented Jun 25, 2024

As well as the sentence Oliver initially identified Nemanja spotted this sentence in 7.3.2 that potentially needs fixing too:

The Credential Issuer that requires the Client to send a key proof of possession of the key material for the Credential to be bound to (proof) MAY receive a Credential Request without such a key proof or with an invalid key proof. In such a case, the Credential Issuer MUST provide the Client with a c_nonce defined in Section 7.3

There appear to be two key alternatives:

  1. We make clear that c_nonce is optional, it is credential issuer policy whether or not a c_nonce is required, and that if clients don't have a c_nonce (and the credential issuer metadata contains the proof_types_supported indicating a proof is required for the request credential) then they must send a request that contains a proof that is fully valid but doesn't have a c_nonce. (The main downside here is that if the client doesn't have a c_nonce and one is required, it'll have to produce a proof, get an error, then produce another proof which may be a relatively expensive operation if a cloud HSM is being used.)

  2. We make clear that c_nonce is required, and if the client doesn't have a c_nonce (and the credential issuer metadata contains the proof_types_supported indicating a proof is required for the request credential) then they can send a request without a proof in order to obtain a c_nonce in the error response.

There was a third option discussed on the WG call today, Lukasz may add a comment later if he thinks the third option makes sense.

@nemqe
Copy link
Contributor

nemqe commented Jun 25, 2024

We might also need to consider changing the definition of the invalid_proof error when we reach an agreement, because with the current definition Option 1 will automatically allow for Option 2.

(7.3.1.2)

invalid_proof: The proof in the Credential Request is invalid. The proof field is not present or the provided key proof is invalid or not bound to a nonce provided by the Credential Issuer.

(7.3.2)

In such a case, the Credential Issuer MUST provide the Client with a c_nonce defined in Section 7.3 in a Credential Error Response using invalid_proof error code defined in [Section 7.3.1]

Additionally the error communicated c_nonce becomes the main driver of this flow as there is no clear way to say that you as an issuer need it or not. Slightly confusing thing about this might be a situation where you as an issuer don't want a c_nonce in the request, but are forced to provide it use it in case of a random proof error.

@nemqe
Copy link
Contributor

nemqe commented Jun 27, 2024

Just to nitpick a bit more, this one can also be a bit confusing because proof handling becomes a bit special (which might very well be the intention):

(7.2)

The proof object is REQUIRED if the proof_types_supported parameter is non-empty and present in the credential_configurations_supported

(7.3.1.2)

invalid_credential_request: The Credential Request is missing a required parameter, includes an unsupported parameter or parameter value, repeats the same parameter, or is otherwise malformed.

(7.3.1.2)

invalid_proof: The proof in the Credential Request is invalid. The proof field is not present or the provided key proof is invalid or not bound to a nonce provided by the Credential Issuer.

Additionally the way Token endpoint handles presence or absence of required filed, mostly talking about tx_code, is quite different but I am not sure if if makes sense to keep that a bit more consistent.

@jogu
Copy link
Contributor

jogu commented Jun 27, 2024

There appear to be two key alternatives:

  1. We make clear that c_nonce is optional, it is credential issuer policy whether or not a c_nonce is required, and that if clients don't have a c_nonce (and the credential issuer metadata contains the proof_types_supported indicating a proof is required for the request credential) then they must send a request that contains a proof that is fully valid but doesn't have a c_nonce. (The main downside here is that if the client doesn't have a c_nonce and one is required, it'll have to produce a proof, get an error, then produce another proof which may be a relatively expensive operation if a cloud HSM is being used.)

We discussed this again on today's WG call, there was a consensus that keeping the current situation where c_nonce is optional but making this consistent through the whole specification is at least a small step in the right direction, hence marking this ready for PR.

We also discussed this sentence that Nemanja drew our attention to:

The Credential Issuer that requires the Client to send a key proof of possession of the key material for the Credential to be bound to (proof) MAY receive a Credential Request without such a key proof or with an invalid key proof. In such a case, the Credential Issuer MUST provide the Client with a c_nonce defined in Section 7.3 in a Credential Error Response using invalid_proof error code defined in Section 7.3.1.

So this would also need to be modified to make it clear that returning a c_nonce is optional when an invalid/missing key proof is received.

@jogu jogu unassigned tplooker and bc-pi Jun 27, 2024
@peppelinux
Copy link
Member

To enhance clarity and maintain alignment with OAuth 2.0 principles, it's important to consider the specific roles and functionalities of different endpoints. The c_nonce parameter, designed specifically for the Resource Server (RS), the credential endpoint should not be handled in the token endpoint response, even if optional, which serves a broader purpose in token issuance.

OAuth 2.0 architecture is structured around the concept of specialized endpoints to ensure scalability and clear separation of features and scopes. Imposing properties on an endpoint to accept parameters intended for another can complicate the design and potentially conflict with the foundational specifications of OAuth 2.0.

therefore I support the removal of the c_nonce issuance in the token endpoint response.

I'm also not in favor of the issuance of nonce though endpoint errors, because from my perspective the issuance by error still represents to me a design error or, differently, a patchy way to provide something.

regarding the credential endpoint and the c_nonce:

  • each jwt proof can be identified as well against any reply attack.
  • facilitating the use of DPoP tokens and discouraging the use of bearer token should be recommended as well
  • any AT can be identified against reply attacks.
  • the RS that supports the issuance of multiple credentials using the same AT must issue the c_nonce to be used in the next requests
  • the first request to the RS not necessarly need a nonce, since it has a fresh AT never used to that endpoint and whatever jwt proof the client/wallet-instance wants to use (with or without the provisioned c_nonce)
  • any next request using the same AT requires therefore the c_nonce, otherwise they would be dropped
  • loosing the next c_nonce, if any, costs the wallet instance to obtain a new AT (since no other c_nonce by error could be provisioned)

@nemqe
Copy link
Contributor

nemqe commented Jun 29, 2024

loosing the next c_nonce, if any, costs the wallet instance to obtain a new AT (since no other c_nonce by error could be provisioned)

Is it ok for the c_nonce lifetime to limit the lifetime of the access_token? In some case it might be expensive to get a new one.

@jogu
Copy link
Contributor

jogu commented Jun 29, 2024

@peppelinux (and anyone else that has commented similarly) could you make your comment about removal of the c_nonce from the token endpoint response on #39 please?

I'm interpreting your response as in essence supporting my comment from 2 days ago, that a request without a c_nonce can be valid depending on RS policy. In the generic oid4vp spec it seems like it would be too much to require c_nonce, but we should look at that for HAIP if we haven't already.

@David-Chadwick
Copy link
Contributor

As I said at the last meeting, the wallet should send a jti as a nonce in the request to the issuer for the credential. The issuer can then either accept this or return a c_nonce if stronger replay protection is needed.
The token endpoint should not be sending a c_nonce.

@awoie
Copy link
Contributor Author

awoie commented Jul 1, 2024

As I said at the last meeting, the wallet should send a jti as a nonce in the request to the issuer for the credential. The issuer can then either accept this or return a c_nonce if stronger replay protection is needed. The token endpoint should not be sending a c_nonce.

@David-Chadwick I created an issue for this here: #357

Sakurann pushed a commit that referenced this issue Aug 20, 2024
4 approvals. open for more than a week.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants