openid / OpenID4VCI

68 stars 20 forks source link

remove `c_nonce` from the token endpoint response #381

Closed bc-pi closed 1 month ago

bc-pi commented 2 months ago

remove c_nonce and c_nonce_expires_in from the token endpoint response to fix #39

again

jogu commented 2 months ago

I think this is the previous issue about a nonce endpoint for wallet attestation that people were asking about on today's call:

https://github.com/openid/OpenID4VCI/issues/71

bc-pi commented 2 months ago

There has been a lot of discussion about this (and roughly mostly in favor of it) in recent months in https://github.com/openid/OpenID4VCI/issues/39 and also https://github.com/openid/OpenID4VCI/issues/331 and maybe elsewhere too. [edit: like https://github.com/openid/OpenID4VCI/issues/357 ]

peppelinux commented 2 months ago

I suggest to add a dedicated nonce endpoint to the credential issuer as replacement for the c_nonce token response parameter.

@tlodderstedt that's exactly what I had in mind when I started writing this draft: https://github.com/peppelinux/draft-demarco-oauth-nonce-endpoint?tab=readme-ov-file

@jogu many of the conversations and points raised within the issue #71 was filtered and mentioned in the nonce endpoint draft

bc-pi commented 2 months ago

as requested, https://github.com/openid/OpenID4VCI/pull/381/commits/074065b37c9c247ecc2c838ad0be201ad5f0169e adds a dedicated nonce endpoint to the credential issuer as replacement for the c_nonce token response parameter

bc-pi commented 2 months ago

I would like to mandate nonce endpoint if the issuer requires the use of c_nonce

48a367810ef59a559869be3bd74d191d6591b673 updates the proposed text to do that.

bc-pi commented 2 months ago

overall looks good. I almost approved, but I think we should discuss if we really want to mandate nonce endpoint for all issuers that require c_nonce? than why keep an option to return c_nonce in credential response?

fair questions that i guess should be discussed in the WG

bc-pi commented 2 months ago

overall looks good. I almost approved, but I think we should discuss if we really want to mandate nonce endpoint for all issuers that require c_nonce? than why keep an option to return c_nonce in credential response?

fair questions that i guess should be discussed in the WG

Noting that removing the ability to have send a c_nonce in the credential response would probably be a breaking change by most measures. Which, I think, would be okay by me. But should have some buy-in from the WG.

Sakurann commented 2 months ago

Discussed on the WG call, direction on removing section 7.3.2 and consolidating its content in Nonce Endpoint definition. https://openid.github.io/OpenID4VCI/openid-4-verifiable-credential-issuance-wg-draft.html#section-7.3.2

also in this PR, add a little more explanation that issuer can invalidate c_nonce any time. please open an issue on removing c_nonce_expires_in parameter (@tplooker).

also agreed to open a separate issue on removing an option to return c_nonce from the credential error response, so that we can merge this PR as-is (@bc-pi).

bc-pi commented 2 months ago

Discussed on the WG call, direction on removing section 7.3.2 and consolidating its content in Nonce Endpoint definition. https://openid.github.io/OpenID4VCI/openid-4-verifiable-credential-issuance-wg-draft.html#section-7.3.2

While I was part of that discussion, after looking again at that section and content of this PR, I don't see how that consolation could actually manifest in a meaningful way. I suggest this PR proceed without and consider that as part of the soon to be separate issue on removing an option to return c_nonce from the credential [error] response.

also in this PR, add a little more explanation that issuer can invalidate c_nonce any time.

https://github.com/openid/OpenID4VCI/pull/381#discussion_r1764090443 has a bit of text for that

also agreed to open a separate issue on removing an option to return c_nonce from the credential error response, so that we can merge this PR as-is (@bc-pi).

https://github.com/openid/OpenID4VCI/issues/393

Sakurann commented 2 months ago

on protecting the endpoint, discussed on the WG call, no strong reason to do it, agreed to proceed with unprotected endpoint, but open an issue to gather an implementation experience if anyone wants/needs a stateful c_nonce

peppelinux commented 2 months ago

on protecting the endpoint, discussed on the WG call, no strong reason to do it, agreed to proceed with unprotected endpoint, but open an issue to gather an implementation experience if anyone wants/needs a stateful c_nonce

interesting is the idea to use the AT released by the token endpoint, and for the purpose of the credential issuance, to the oauth nonce endpoint as a protected endpoint.

generally I assume that any endpoint might be protected with a token

c2bo commented 2 months ago

overall looks good. I almost approved, but I think we should discuss if we really want to mandate nonce endpoint for all issuers that require c_nonce? than why keep an option to return c_nonce in credential response?

I was wondering about this as well. One reason why I think it would make sense to keep the option to return c_nonce in the Credential Response would be to allow the Issuer to invalidate an existing nonce because of some kind security concern (some kind of anomaly detection etc. that got triggered). That way the issuer could create a stateful nonce for a specific client via the credential response if necessary.

bc-pi commented 2 months ago

overall looks good. I almost approved, but I think we should discuss if we really want to mandate nonce endpoint for all issuers that require c_nonce? than why keep an option to return c_nonce in credential response?

I was wondering about this as well. One reason why I think it would make sense to keep the option to return c_nonce in the Credential Response would be to allow the Issuer to invalidate an existing nonce because of some kind security concern (some kind of anomaly detection etc. that got triggered). That way the issuer could create a stateful nonce for a specific client via the credential response if necessary.

I've created https://github.com/openid/OpenID4VCI/issues/393 to discuss/track "why keep an option to return c_nonce in credential response?"

paulbastian commented 2 months ago

Just to be sure, this PR creates two possible ways for the Issuer to provide the c_nonce, as both are described with MAY, this means the Wallet must implement both. Is this analysis correct?

bc-pi commented 2 months ago

Just to be sure, this PR creates two possible ways for the Issuer to provide the c_nonce, as both are described with MAY, this means the Wallet must implement both. Is this analysis correct?

I don't think so, no.

paulbastian commented 2 months ago

Just to be sure, this PR creates two possible ways for the Issuer to provide the c_nonce, as both are described with MAY, this means the Wallet must implement both. Is this analysis correct?

I don't think so, no.

What do you think then?

bc-pi commented 2 months ago

Just to be sure, this PR creates two possible ways for the Issuer to provide the c_nonce, as both are described with MAY, this means the Wallet must implement both. Is this analysis correct?

I don't think so, no.

What do you think then?

@jogu's analysis from over in one of the 87 other communication channels seemed, as I said there, constructive:

It depends a little on the wallet's viewpoint. In some ways, it replaces a mechanism that may or may not happen (cnonce at token endpoint, cnonce error from credential endpoint) with a "if I list this endpoint in my metadata, you need to call it, and once you call it your credential endpoint call should succeed first time".

But also I think that this PR is a huge step in the right direction that is long overdue.

And further improvements should be considered seriously per https://github.com/openid/OpenID4VCI/issues/393 soon after this is merged. Which is hopefully soon.

paulbastian commented 2 months ago

Re-reading section 7.3.2 I think we should delete the following paragraph:

If the Client has not received a c_nonce and the Credential Issuer Metadata contains proof_types_supported indicating a key proof is required for the requested Credential, the Client MUST send a Credential Request that contains a proof or proofs parameter that is fully valid but does not include a c_nonce value. It is the Credential Issuer policy whether or not a c_nonce value is required in the key proofs.If the Client received a c_nonce, the c_nonce value MUST be incorporated in the respective parameter in the proof or proofs object.

I think leaving this paragraph in would lead to confusion, especially given that weird MUST that now contradicts the Nonce endpoint

babisRoutis commented 1 month ago

FWIW,

I think that the removal of c_nonce from Token Endpoint is a big improvement. Now, it is only the credential issuer's responsibility to provide such a c_nonce.

I was hoping, though, to have a single way (for the caller/wallet) to obtain such a c_nonce. Introducing the nonce endpoint and keeping c_nonce in both success and/or error response, I have the feeling makes solution more complex that is needed.

For instance, in draft 13, it was clear - although not explicitly stated - that the main way to obtain c_nonce was via credential response. Token response was an optimization and in worst case, wallet could just "ignore" the token response in the expense of an additional credential request

With this PR, unless I am missing something, a wallet should be prepared to handle

bc-pi commented 1 month ago

Re-reading section 7.3.2 I think we should delete the following paragraph:

If the Client has not received a c_nonce and the Credential Issuer Metadata contains proof_types_supported indicating a key proof is required for the requested Credential, the Client MUST send a Credential Request that contains a proof or proofs parameter that is fully valid but does not include a c_nonce value. It is the Credential Issuer policy whether or not a c_nonce value is required in the key proofs.If the Client received a c_nonce, the c_nonce value MUST be incorporated in the respective parameter in the proof or proofs object.

I think leaving this paragraph in would lead to confusion, especially given that weird MUST that now contradicts the Nonce endpoint

That that weird MUST needs to change but I'm reluctant to delete the whole paragraph.

paulbastian commented 1 month ago

As I would read this PR, it recommends the wallet to use the nonce endpoint if it's present and the parameter in credential response is kind of an alternative/fallback. Therefore this paragraph does not fit any longer.

bc-pi commented 1 month ago

aadb2bb8b0a245970dc7d3e9beafe552864eecf7 is what I was able to summon the appetite for

bc-pi commented 1 month ago

Happy to accept if Joseph's proposal is incorporated. https://github.com/openid/OpenID4VCI/pull/381/files#r1787382536

it's done with 9e0d0599fd81bfa8abe6a13be4e5b948f5f4aca5

tlodderstedt commented 1 month ago

Happy to accept if Joseph's proposal is incorporated. https://github.com/openid/OpenID4VCI/pull/381/files#r1787382536

it's done with 9e0d0599fd81bfa8abe6a13be4e5b948f5f4aca5

Great, thanks!

jogu commented 1 month ago

As discussed on today's WG call - we now have an astounding 12 approvals and no remaining objections/comments - merging! Thank you everyone!