remove `c_nonce` from the token endpoint response #381

bc-pi · 2024-08-27T11:20:39Z

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

again

…ponse to fix #39

tlodderstedt

I suggest to add a dedicated nonce endpoint to the credential issuer as replacement for the c_nonce token response parameter. Just removing the c_nonce will force wallets to try the credential issuance request in order to get a nonce in order to be able to create a proper nonce-bound proof of possession. That's complicated.

jogu · 2024-08-27T21:00:13Z

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

#71

bc-pi · 2024-08-29T17:29:47Z

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

peppelinux · 2024-09-03T19:33:01Z

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

…ue without the overhead of a full Credential Request

bc-pi · 2024-09-04T21:09:35Z

as requested, 074065b adds a dedicated nonce endpoint to the credential issuer as replacement for the c_nonce token response parameter

paulbastian

I would like to mandate nonce endpoint if the issuer requires the use of c_nonce, i.e. the presence of the endpoint enforces its usage, therefore we could get rid of sending fake ugly Credential requests to fetch a c_nonce completely.

Having this endpoint optional and no other indication of the issuer whether he requires c_nonce for proofs seems inefficient.

bc-pi · 2024-09-06T22:57:51Z

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

48a3678 updates the proposed text to do that.

openid-4-verifiable-credential-issuance-1_0.md

Sakurann · 2024-09-10T13:50:45Z

openid-4-verifiable-credential-issuance-1_0.md

@@ -710,6 +706,45 @@ Cache-Control: no-store
 }
 ```

+# Nonce Endpoint {#nonce-endpoint}
+
+A Credential Issuer that requires `c_nonce` values to be incorporated into proofs in the Credential Request (see (#credential-request)) MUST offer a Nonce Endpoint. This endpoint allows a Client to acquire a fresh `c_nonce` value without the overhead of a full Credential Request.


improve readability

Suggested change

A Credential Issuer that requires `c_nonce` values to be incorporated into proofs in the Credential Request (see (#credential-request)) MUST offer a Nonce Endpoint. This endpoint allows a Client to acquire a fresh `c_nonce` value without the overhead of a full Credential Request.

This endpoint allows a Client to acquire a fresh `c_nonce` value without the overhead of a full Credential Request. A Credential Issuer that requires `c_nonce` values to be incorporated into proofs in the Credential Request (see (#credential-request)) MUST offer a Nonce Endpoint.

but also do 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?

was trying to keep the scope of the changes to something that had been agreed on while also accommodating Paul's request for changes

while also keeping my own frustration in check about the fact that these are all things that were only hidden by the nonce from token endpoint but have existed all along so why do they always and only come up in the context of trying to make an improvement

Also removing the ability to have send a c_nonce in the credential response would probably be a breaking change by most measures and it's unclear if the WG has the appetite for that.

openid-4-verifiable-credential-issuance-1_0.md

Sakurann

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?

Co-authored-by: Kristina <[email protected]>

bc-pi · 2024-09-10T16:33:20Z

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

openid-4-verifiable-credential-issuance-1_0.md

paulbastian

I like the current direction.

If we keep things as-is, we can completely delete the chapter 7.3.2 as the nonce endpoint is doing the job and all other details are already covered in the main section of Credential Response

openid-4-verifiable-credential-issuance-1_0.md

bc-pi · 2024-09-16T21:22:34Z

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.

openid-4-verifiable-credential-issuance-1_0.md

Sakurann · 2024-09-17T19:53:35Z

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).

peppelinux · 2024-09-19T16:00:17Z

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

openid-4-verifiable-credential-issuance-1_0.md

peppelinux

I have requested some small changes about naming, you can consider this PR approved by me

peppelinux · 2024-09-19T19:34:40Z

openid-4-verifiable-credential-issuance-1_0.md

+
+The Credential Issuer provides a nonce value in the HTTP response with a 2xx status code and the following parameters included as top-level members in the message body of the HTTP response using the application/json media type:
+
+* `c_nonce`: REQUIRED. String containing a nonce to be used when creating a proof of possession of the key proof (see (#credential-request)).


Okay, since this Nonce Endpoint is specifically tailored for credential issuance within the sole scope of the OpenID4VCI specs, it would make sense to rename the endpoint metadata parameter from nonce_endpoint to credential_nonce_endpoint.

In this way, other nonce endpoints created for other OpenID/IETF specs will not collide with this one.
The related section should therefore be renamed to "Credential Nonce Endpoint," adding "Credential" as a prefix to contextualize its scope.

c2bo · 2024-09-20T08:40:29Z

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 · 2024-09-20T12:12:20Z

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 #393 to discuss/track "why keep an option to return c_nonce in credential response?"

openid-4-verifiable-credential-issuance-1_0.md

Co-authored-by: Paul Bastian <[email protected]>

paulbastian · 2024-09-20T17:13:13Z

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 · 2024-09-20T20:26:00Z

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 · 2024-09-20T20:38:44Z

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 · 2024-09-20T20:54:46Z

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 #393 soon after this is merged. Which is hopefully soon.

openid-4-verifiable-credential-issuance-1_0.md

paulbastian · 2024-09-22T19:34:35Z

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 · 2024-09-23T14:42:05Z

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

Issuers that do provide nonce endpoint but don't provide c_nonce in the credential response
Issuers that do NOT provide nonce endpoint but provide c_nonce in the credential response
Issuers that support both nonce endpoint & c_nonce in the responses.

bc-pi · 2024-09-24T16:44:22Z

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 · 2024-09-24T16:50:06Z

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 · 2024-09-24T17:08:15Z

aadb2bb is what I was able to summon the appetite for

Co-authored-by: Giuseppe De Marco <[email protected]>

tlodderstedt · 2024-09-26T15:50:51Z

openid-4-verifiable-credential-issuance-1_0.md

+Below is a non-normative example of a Nonce Request:
+
+```
+GET /nonce HTTP/1.1


Wouldn't a POST request be more appropriate? I mean the endpoint is supposed to provide a new/different value with every response and the result should not be cached (in my opinion).

+1, I tried to push in this direction as well: #381 (comment)

I didn't realize this was GET. my suggestion has been POST. In german wallet project we have been prototyping with POST and it works well https://bmi.usercontent.opencode.de/eudi-wallet/eidas-2.0-architekturkonzept/flows/PID-IssuerSigned-cloud/#issuer-session-endpoint-at-the-pid-provider

Other than describing the media type of content that doesn't exist as JSON, which is semantically invalid and maybe syntactically wrong too, what value does the German wallet project get from using POST?

=== Screenshot of the previously cited german wallet project ===

Wouldn't a POST request be more appropriate?

No.

I mean the endpoint is supposed to provide a new/different value with every response and the result should not be cached (in my opinion).

That's why it was made not cacheable in the example https://github.com/openid/OpenID4VCI/pull/381/files#diff-1f424614b35a9899813079f1b1f6218631a2aedd993368ccb89bb81a9eda0289R741 and text https://github.com/openid/OpenID4VCI/pull/381/files#diff-1f424614b35a9899813079f1b1f6218631a2aedd993368ccb89bb81a9eda0289R734 using the Cache-Control construct that HTTP gives for control of caching.

+1, I tried to push in this direction as well: #381 (comment)

My naive feeling that using a GET to get some data is the most straightforward way to get some data remained unchanged by that discussion over the course of the last two weeks.

My 2 cents for what they are worth,

If we asked the question "is this call safe (i.e. is it read-only)" I think the answer would be "maybe". Safe calls shouldn't alter the state of the server in any way but without knowing how the server implements the nonce endpoint we would not know the answer to the question.

If we asked the question "is this call idempotent" I think we would also get a "maybe". Client could do this call just to "fetch a value" which might not assume any intention of a side-effect, and from the perspective of the server it would depend on the implementation.

I do see the point in making it a GET because it is technically just fetching a random value, so semantically if fits, but there might be an embedded assumption here that this random value is a prerequisite for some other steps, and that getting a nonce can be viewed as setting up a state that needs to be validated by the server.

One could argue that it might be safer to make this a POST because this would communicate that the call might not be safe or idempotent and that subsequent calls could cause load, burden, and side-effects for the server.

But this is just philosophizing, I personally do not have a clear preference, I see arguments for both GET and POST.

tlodderstedt · 2024-09-26T15:53:29Z

openid-4-verifiable-credential-issuance-1_0.md

@@ -1044,7 +1081,7 @@ Cache-Control: no-store

 The Credential Issuer MAY provide the Client with a `c_nonce` as defined in (#credential-response) in a Credential Error Response using `invalid_proof` error code defined in (#credential-error-response) if the Credential Issuer Metadata contains `proof_types_supported` indicating a key proof is required for the requested Credential. Depending on the Credential Issuer policy, this occurs if they receive a Credential Request without a `c_nonce` or with an invalid `c_nonce` value included in the proof(s) in the `proof` or `proofs` parameter.

-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 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 can 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.


When is this paragraph apply? I'm asking since the client can always obtain a c_nonce from the nonce endpoint.

+1, I was trying to push in this direction as well: #381 (comment)

As I've tried to convey in comments on this PR and in calls over the course of the last month, I attempted to keep the scope of this change narrow so as to hopefully make it acceptable to the wide range of opinions and oftentimes irregular participation of the WG members. I have clearly failed in my efforts. I believe the text in question here was written recently by @awoie and because it is still true that "Credential Issuer policy whether or not a c_nonce value is required in the key proofs" and a "Client can send a Credential Request that contains a proof or proofs parameter that is fully valid but does not include a c_nonce value" it seemed like it applied enough to not delete it.

Sakurann

i did not realize this was GET, approved assuming this was POST, sorry about that...

remove c_nonce and c_nonce_expires_in from the token endpoint res…

ce28a10

…ponse to fix #39

tlodderstedt requested changes Aug 27, 2024

View reviewed changes

added a Nonce Endpoint where a Client can acquire a fresh c_nonce val…

074065b

…ue without the overhead of a full Credential Request

bc-pi requested a review from tlodderstedt September 4, 2024 21:07

bc-pi requested review from tplooker, Sakurann and jogu September 4, 2024 21:10

paulbastian requested changes Sep 5, 2024

View reviewed changes

mandate nonce endpoint if the issuer requires the use of c_nonce

48a3678

bc-pi requested a review from paulbastian September 6, 2024 22:57

Sakurann reviewed Sep 10, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Show resolved Hide resolved

Sakurann reviewed Sep 10, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Show resolved Hide resolved

Sakurann reviewed Sep 10, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Show resolved Hide resolved

Sakurann requested changes Sep 10, 2024

View reviewed changes

improve readability via Sakurann

58bbf81

Co-authored-by: Kristina <[email protected]>

paulbastian reviewed Sep 10, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Show resolved Hide resolved

paulbastian reviewed Sep 10, 2024

View reviewed changes

pmhsfelix reviewed Sep 12, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Show resolved Hide resolved

openid-4-verifiable-credential-issuance-1_0.md Show resolved Hide resolved

make nonce response uncacheable

0e0d188

jogu added this to the Final 1.0 milestone Sep 16, 2024

c2bo reviewed Sep 17, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved

openid-4-verifiable-credential-issuance-1_0.md Show resolved Hide resolved

peppelinux reviewed Sep 19, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved

peppelinux requested changes Sep 19, 2024

View reviewed changes

c2bo approved these changes Sep 20, 2024

View reviewed changes

paulbastian reviewed Sep 20, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved

paul fix an a too far

aeb3839

Co-authored-by: Paul Bastian <[email protected]>

nemqe reviewed Sep 20, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Show resolved Hide resolved

nemqe reviewed Sep 20, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Show resolved Hide resolved

nemqe approved these changes Sep 20, 2024

View reviewed changes

paulbastian approved these changes Sep 21, 2024

View reviewed changes

big MUST to little can

aadb2bb

bc-pi requested a review from peppelinux September 24, 2024 16:59

tplooker approved these changes Sep 24, 2024

View reviewed changes

peppelinux approved these changes Sep 24, 2024

View reviewed changes

peppelinux mentioned this pull request Sep 24, 2024

remove c_nonce from token response, we have the nonce endpoint italia/eudi-wallet-it-docs#422

Open

big C

55c109e

Co-authored-by: Giuseppe De Marco <[email protected]>

paulbastian approved these changes Sep 26, 2024

View reviewed changes

tlodderstedt reviewed Sep 26, 2024

View reviewed changes

Sakurann requested changes Sep 26, 2024

View reviewed changes

	A Credential Issuer that requires `c_nonce` values to be incorporated into proofs in the Credential Request (see (#credential-request)) MUST offer a Nonce Endpoint. This endpoint allows a Client to acquire a fresh `c_nonce` value without the overhead of a full Credential Request.
	This endpoint allows a Client to acquire a fresh `c_nonce` value without the overhead of a full Credential Request. A Credential Issuer that requires `c_nonce` values to be incorporated into proofs in the Credential Request (see (#credential-request)) MUST offer a Nonce Endpoint.


		The Credential Issuer provides a nonce value in the HTTP response with a 2xx status code and the following parameters included as top-level members in the message body of the HTTP response using the application/json media type:

		* `c_nonce`: REQUIRED. String containing a nonce to be used when creating a proof of possession of the key proof (see (#credential-request)).

remove c_nonce from the token endpoint response #381

Are you sure you want to change the base?

remove c_nonce from the token endpoint response #381

Conversation

bc-pi commented Aug 27, 2024 • edited Loading

tlodderstedt left a comment

Choose a reason for hiding this comment

jogu commented Aug 27, 2024

bc-pi commented Aug 29, 2024 • edited Loading

peppelinux commented Sep 3, 2024 • edited Loading

bc-pi commented Sep 4, 2024

paulbastian left a comment

Choose a reason for hiding this comment

bc-pi commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sakurann left a comment

Choose a reason for hiding this comment

bc-pi commented Sep 10, 2024

paulbastian left a comment • edited Loading

Choose a reason for hiding this comment

bc-pi commented Sep 16, 2024

Sakurann commented Sep 17, 2024

peppelinux commented Sep 19, 2024 • edited Loading

peppelinux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c2bo commented Sep 20, 2024

bc-pi commented Sep 20, 2024

paulbastian commented Sep 20, 2024

bc-pi commented Sep 20, 2024

paulbastian commented Sep 20, 2024

bc-pi commented Sep 20, 2024

paulbastian commented Sep 22, 2024 • edited Loading

babisRoutis commented Sep 23, 2024

bc-pi commented Sep 24, 2024

paulbastian commented Sep 24, 2024

bc-pi commented Sep 24, 2024

tlodderstedt Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bc-pi Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

bc-pi Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

bc-pi Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

nemqe Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bc-pi Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Sakurann left a comment

Choose a reason for hiding this comment

remove `c_nonce` from the token endpoint response #381

remove `c_nonce` from the token endpoint response #381

bc-pi commented Aug 27, 2024 •

edited

Loading

bc-pi commented Aug 29, 2024 •

edited

Loading

peppelinux commented Sep 3, 2024 •

edited

Loading

paulbastian left a comment •

edited

Loading

peppelinux commented Sep 19, 2024 •

edited

Loading

paulbastian commented Sep 22, 2024 •

edited

Loading

tlodderstedt Sep 26, 2024 •

edited

Loading

bc-pi Sep 26, 2024 •

edited

Loading

bc-pi Sep 26, 2024 •

edited

Loading

bc-pi Sep 26, 2024 •

edited

Loading

nemqe Sep 27, 2024 •

edited

Loading

bc-pi Sep 26, 2024 •

edited

Loading