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

Remove c_nonce* from the token response #199

Closed
wants to merge 3 commits into from
Closed

Conversation

bc-pi
Copy link
Member

@bc-pi bc-pi commented Jan 3, 2024

Removing the optional c_nonce and c_nonce_expires_in from the token response in order to simplify the spec and implementations per #39. It’s a lot of non-obvious complexity and optionality for the potential avoidance of just one HTTP request/response direct from wallet/client to credential issuer.

@bc-pi bc-pi linked an issue Jan 3, 2024 that may be closed by this pull request
@Sakurann
Copy link
Collaborator

Sakurann commented Jan 4, 2024

There is no agreement in the issue to make this change. I am very confused to see this PR.

Copy link
Collaborator

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not enough implementation expeirence/discussions to do the PR yet, IMO

@bc-pi
Copy link
Member Author

bc-pi commented Jan 4, 2024

I am very confused to see this PR.

I was reminded again of this when reviewing the IANA update PR #144 (comment) and wanted to get something in before the Implementer’s Draft because after that, of course, there will be more reluctance for such a change. The only activity on the associated issue #39 in the last six months is a discussion that (to me anyway) underscores the kind of confusion or misexpectations that this can give rise to.

@jogu
Copy link
Contributor

jogu commented Jan 5, 2024

I'm probably tending towards Brian's summary that the issue discussion was moving in this direction, and agree on the grounds that it simplifies RP implementations which is always a good thing, also that it would be harder to remove this if it goes out in ID1.

Also if someone comes up with really compelling data that saving that single roundtrip is worth the enforced extra complexity in clients (and the extra conformance tests that it would make necessary ;-) ) we can always add it back in ID2.

Copy link
Member

@selfissued selfissued left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I support this PR for the reasons stated by @bc-pi and @jogu.

Copy link
Contributor

@paulbastian paulbastian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the issue I don't see lots of consensus, actually Taka was favouring to keep it. Also the non obvious complexity is not explained here, can you elaborate? To my knowledge this would be a breaking change for many.

@bc-pi
Copy link
Member Author

bc-pi commented Jan 6, 2024

The complexity comes from the authorization server and credential issuer being distinct entities and needing to share some state data to be able to have functional interoperable nonce values. Yes, in some cases they might be co-deployed such that they are like a single entity and there are absolutely ways to share data. But not in a standard or easy way.

@jogu
Copy link
Contributor

jogu commented Jan 6, 2024

The complexity comes from the authorization server and credential issuer being distinct entities and needing to share some state data to be able to have functional interoperable nonce values. Yes, in some cases they might be co-deployed such that they are like a single entity and there are absolutely ways to share data. But not in a standard or easy way.

I believe it is optional whether the AS returns c_nonce or not though, so that particular complexity at least isn't mandatory to implement.

But still it is complexity and it makes wallets more complicated (which in turn also makes conformance testing more complex, as it means extra tests are required to check the wallet correctly handles the value both being there and not being there - and having more conformance tests means it takes longer for each wallet to complete testing and there's more ways they can fail). The experience from OpenID Connect, FAPI, etc has always indicated that making the requirements on relying parties (wallets in this case) simple is good for adoption and interoperability (and sometimes good for security too, though I don't think that applies in this case).

@paulbastian
Copy link
Contributor

paulbastian commented Jan 6, 2024

Having less options is always good for simplicity and interoperability, I agree. I thought there was more than that AS and credential issuer might be separate entities, which seemed obvious to me.

However I fear that people might use this simplification in an argument against openid4vci calling it ineffecient or similar.

Additionally, calling the credential endpoint to get the nonce through an error still looks ugly to me.

@bc-pi
Copy link
Member Author

bc-pi commented Jan 6, 2024

I believe it is optional whether the AS returns c_nonce or not though, so that particular complexity at least isn't mandatory to implement.

That is true, of course, but having the option in the document adds to the cognitive overhead of the spec and leads to integrator/customer/deployment expectations that it'll just work between disparate components (and the need to repeatedly explain why that's not the case).

There have been a number of inconsistencies/defects in the spec related to the option too - although they have admittedly been mostly addressed now by generalizing to something like "server-provided nonce".

However I fear that people might use this simplification in an argument against openid4vci calling it ineffecient or similar.

Just as context, it's an optimization to avoid a single HTTP request/response. As one (imperfect) point of comparison - loading this gitbub page involved over one hundred HTTP request/response pairs.

Additionally, calling the credential endpoint to get the nonce through an error still looks ugly to me.

It's not pretty but neither is getting the nonce from the token endpoint. And the nonce from the credential endpoint has to be possible as the fallback regardless for expired/invalid/missing nonce in general.

I dunno, I've always thought this would be an easy win with respect to simplicity and interoperability. But there doesn't seem to be much agreement.

@TakahikoKawasaki
Copy link

Authlete has already implemented "c_nonce from the token endpoint", but if the final conclusion of the working group becomes "not to include c_nonce in the token response", then we will disable that functionality (while retaining the implementation for the possibility of future resurgence).

I don't intend to make a strong argument about whether to include c_nonce in the token response or not, but if c_nonce is issued from the token endpoint, demos for key proofs become more straightforward, like the one in the "4.1. Pre-Authorized Code Flow + Key Proof + SD-JWT VC" section in our online document.

Our online document explains c_nonce in the "2.9.2. c_nonce" section with diagrams like the following one.

c_nonce

@paulbastian
Copy link
Contributor

In general I believe this simplification could be beneficial overall. My concern is mostly how in the current spec the nonce is returned. I know, that its similar to DPoP, to my it looks ugly and if the Wallet knows that it requires a nonce and has to retrieve it in an error. I don't believe that a happy path looks like this:

HTTP/1.1 400 Bad Request
Content-Type: application/json
Cache-Control: no-store

{
  "error": "invalid_proof"
  "error_description":
       "Credential Issuer requires key proof to be bound to a Credential Issuer provided nonce.",
  "c_nonce": "8YE9hCnyV2",
  "c_nonce_expires_in": 86400
}

First, we need guidance in the Credential Request Chapter, that explains the normal path to get a nonce.

Second, I would like to discuss whether there is a possibility to get the nonce with HTTP 200 or 202?

Copy link
Contributor

@tplooker tplooker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in support of this proposal too and believe its important to get in before ID-1. Requiring the RS and AS to co-ordinate state which is what returning the c_nonce in the token response causes significant implementation complexity for issuers.

@tplooker
Copy link
Contributor

tplooker commented Jan 8, 2024

First, we need guidance in the Credential Request Chapter, that explains the normal path to get a nonce.

I agree with this too, the guidance that is provided in this PR as to how to properly get the nonce is probably insufficient and needs to be expanded.

@jogu
Copy link
Contributor

jogu commented Jan 9, 2024

@tplooker

Requiring the RS and AS to co-ordinate state which is what returning the c_nonce in the token response causes significant implementation complexity for issuers.

Whilst I agree with this PR, just to note that there was no requirement for the RS & AS to do this. It's completely optional for the issuer to return c_nonce in the token response.

I agree with this too, the guidance that is provided in this PR as to how to properly get the nonce is probably insufficient and needs to be expanded.

As that's a non-normative change it'd probably be best to handle it in a separate PR, so that we can get the normative part merged and start working group last call for ID1.

@jogu
Copy link
Contributor

jogu commented Jan 9, 2024

@paulbastian

Second, I would like to discuss whether there is a possibility to get the nonce with HTTP 200 or 202?

I don't think that's something we're going to get any agreement on in time for ID1. The choice we have to make this week is whether the normative change already in this PR goes in or not.

@paulbastian
Copy link
Contributor

paulbastian commented Jan 9, 2024

This may result in two cryptographic proofs, this may have a huge cost for cloud-based or other hardware-based keys and in double user consent.

Bundesdruckerei has implemented the c_nonce from Token Response and we had good experience with that.

@tlodderstedt
Copy link
Collaborator

First of all, I'm not happy with a PR that is not based on a consensus regarding the direction of a certain feature. In the end, we will be conducting the discussion whether we want the proposed change or not now on the PR.

@tlodderstedt
Copy link
Collaborator

tlodderstedt commented Jan 9, 2024

The reason for enabling provision of a c_nonce in the token response is optimization. The AS may reduce the number of calls the wallet needs to perform in order to get a credential. If implementations don't like the feature, no problem, it's optional.

So as basis for forming an opinion on whether we should remove this feature from the spec I would like to know, who has implemented the feature and why. Implementers, please speak up.

@tlodderstedt
Copy link
Collaborator

tlodderstedt commented Jan 9, 2024

On the coupling between AS and RS. If implementers don't want the coupling at all, they should not implement this feature.

If implementers want the optimization with loose coupling, they should use a signed JWT as nonce. So the credential issuer would need to check the signature of the nonce JWT in order to ensure it was issued by its AS. That's basically the same logic used to check the access token, right?

So where is the implementation complexity?

@tplooker
Copy link
Contributor

tplooker commented Jan 9, 2024

On the coupling between AS and RS. If implementers don't want the coupling at all, they should not implement this feature.

I don't think we should allow features in a protocol that couple two components together that would otherwise be regarded seperately, this becomes more than an optimisation but a conflation that is likely to create other issues.

@bc-pi
Copy link
Member Author

bc-pi commented Jan 9, 2024

It seems clear that there are some strong opinions on both sides of this argument. However, there does not seem to be sufficient consensus to make a change (and maybe/probably some implementations currently using the optimization). So I'm leaning towards withdrawing/closing this PR.

I do want note that any deficiencies or ugliness of getting the c_nonce from the credential endpoint should still be addressed because that is the default and MTI means of getting a c_nonce value that applies in general for cases with missing/expired/invalid nonce including the case where the optional (and controversial) c_nonce from the token endpoint isn't in play.

@jogu
Copy link
Contributor

jogu commented Jan 10, 2024

@paulbastian

This may result in two cryptographic proofs, this may have a huge cost for cloud-based or other hardware-based keys and in double user consent.

I think this is an important point, but it seems that also means that currently that the issuer can (by deciding not to implement c_nonce in the token response, which is 100% legal) impose costs and bad UX on the wallet, which is probably not a good outcome.

Regardless of whether this PR is merged or not, this sounds like an issue that needs further thought.

@babisRoutis
Copy link
Contributor

babisRoutis commented Jan 30, 2024

Authorization servers implementers may choose to ignore the c_nonce provided by the token endpoint, since in any case the Credential Issuer must provide it (if proof is needed).

Also a wallet (or a VCI library for wallets) can choose to ignore it, given that a conformant credential issuer has to provide a c_nonce in his response.

So, I see no added complexity should an issuer or a wallet choose to ignore it.

But then again, why have it in the first place?
I do get the optimization argument, but I think gains are minor, because it is applicable only when credential issuer is coupled to the authorization server.

@bc-pi
Copy link
Member Author

bc-pi commented Feb 7, 2024

It seems clear that there is not sufficient consensus to make this change so (somewhat reluctantly) closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove c_nonce from the token response
9 participants