openid / OpenID4VCI

68 stars 20 forks source link

remove c_nonce and c_nonce_expires_in from credential endpoint response #404

Closed mickrau closed 2 weeks ago

mickrau commented 1 month ago

resolves #393

Simplify spec by only provide one way to get a c_nonce

paulbastian commented 1 month ago

Using self-contained nonces, this allows the nonce endpoint to be better separated form the Credential Issuer and it makes Wallet implementaitons easier as there are less options.

mickrau commented 1 month ago

This PR provides a nice simplification but from my understanding has 2 potential downsides:

  • adds a round trip in the corner cases where a server rejects a proof because the nonce was deemed invalid
  • removes the possibility to provide stateful nonces - the nonce endpoint is not protected and there is no possibility (I think?) for this right now

Another downside could be additional calls to the nonce endpoint if several credentials have to be requested. The current spec is not entirely clear whether each credential request MUST us a fresh nonce in this case.

The name "nonce" indicates use only-once principle. However, I know that not everyone is so strict about this and that there are some implementations that allow the multiple use of a nonce within a certain period of time.

I would prefer it if it was either clearly defined as use only-once nonce or if the nonce was renamed to challenge, which can be used multiple times within a certain period of time.

nemqe commented 1 month ago

If I see it correctly this PR also addresses https://github.com/openid/OpenID4VCI/issues/394, right?

Also, not sure about the process, but do we need to add the notes for the next draft regarding what changed?

Sakurann commented 1 month ago

with my chair hat on, noting that there is not enough discussion in the issue to be called an agreement to remove this feature.

paulbastian commented 1 month ago

Neither was there enough discussion on this in the none endpoint PR, but Brian just said he doesn't see this. I think the PR helps to make people understand this and reflects our real developer experience from implementing nonce endpoint.

Sakurann commented 1 month ago

Brian first did a PR without discussion and that one got closed, we did discuss that in the following calls and there are multiple other issuer where the topic was discussed so when the PR came up the second time, the WG was more prepared.

and for clarify, I did not close this PR. I am just noting down that more discussion is needed for those looking at it

mickrau commented 1 month ago

@Sakurann Sorry if the pull request was too hasty. I had the feeling that the discussions about the nonces were very scattered and that a concrete proposal with links to the issues could help the discussion.

bc-pi commented 1 month ago

If I see it correctly this PR also addresses #394, right?

No, the ## Nonce Response {#nonce-response} section still has a c_nonce_expires_in so this one doesn't fully address #394.

https://github.com/openid/OpenID4VCI/pull/404/files#diff-1f424614b35a9899813079f1b1f6218631a2aedd993368ccb89bb81a9eda0289R755

bc-pi commented 1 month ago

Neither was there enough discussion on this in the none endpoint PR, but Brian just said he doesn't see this.

I'm not entirely sure I know what that means. But there were hundreds of comments across #381, #199, & #39 discussing removal of c_nonce* from the AS's token endpoint. The issue behind this PR #393 and an issue to get rid of c_nonce_expires_in in general #394 were broken out from #381 in an attempt to keep its scope tight and allow it to proceed. The nonce endpoint was added to #381, which was an expansion of scope, but as a condition of acceptance by a few key opponents. And even that had quite a bit of discussion.

bc-pi commented 1 month ago

This PR provides a nice simplification but from my understanding has 2 potential downsides:

* adds a round trip in the corner cases where a server rejects a proof because the nonce was deemed invalid

* removes the possibility to provide stateful nonces - the nonce endpoint is not protected and there is no possibility (I think?) for this right now

The only thing that worries me a bit is losing the possibility to provide stateful nonces, but I am not entirely sure if we need them - are people using that right now?

Stateful with respect to the access token or something contained therein. But good question.

If we really want/need to preserve the ability to have stateful nonces, then we could also add the option to have the nonce endpoint protected and signal that via metadata I guess?

That could be signaled by a 401 at the nonce endpoint too.

Sakurann commented 3 weeks ago

Discussed in pre-IIW hybrid meeting - agreed to proceed with this PR, merging once 1-2 more approvals

Sakurann commented 3 weeks ago

@mickrau can you please add a changelog entry that this option was removed? I will merge this PR after that

mickrau commented 3 weeks ago

@mickrau can you please add a changelog entry that this option was removed? I will merge this PR after that

@Sakurann done