openid / OpenID4VCI

68 stars 20 forks source link

Is c_nonce required in proof or not? #331

Closed awoie closed 3 months ago

awoie commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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.

Sakurann commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

WG discussion/proposal on the table:

peppelinux commented 5 months ago

(potentially, also no c_nonce from the token endpoint (https://github.com/openid/OpenID4VCI/issues/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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

Sakurann commented 5 months ago

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 https://github.com/openid/OpenID4VCI/issues/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 commented 5 months ago
  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 commented 5 months ago

We should keep discussion about removing c_nonce from token endpoint under the existing issue for that: https://github.com/openid/OpenID4VCI/issues/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 commented 5 months ago

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

Sakurann commented 5 months ago

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

awoie commented 5 months ago

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 commented 5 months ago

We have an existing issue on removing c_nonce from token endpoint (which has a few new comments on it): https://github.com/openid/OpenID4VCI/issues/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 commented 5 months ago

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 https://github.com/openid/OpenID4VCI/issues/39.

bc-pi commented 5 months ago

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 commented 5 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.

peppelinux commented 4 months ago

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:

nemqe commented 4 months ago

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 commented 4 months ago

@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 https://github.com/openid/OpenID4VCI/issues/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 commented 4 months ago

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 commented 4 months ago

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: https://github.com/openid/OpenID4VCI/issues/357