Remove c_nonce from the token response

OIDF-automation commented 1 year ago

Imported from AB/Connect bitbucket: https://bitbucket.org/openid/connect/issues/1956

Original Reporter: b_d_c

The option to return c_nonce with token response should be dropped to simplify the spec and implementations. It’s a lot of complexity and optionality for the potential avoidance of just one HTTP request/response direct from wallet/client to credential issuer.

OIDF-automation commented 1 year ago

Imported from AB/Connect bitbucket - Original Commenter: josephheenan

I was tempted to make a comment onto https://bitbucket.org/openid/connect/pull-requests/535/adding-security-considerations-on-the but this issue is probably a more appropriate place.

It’s interesting to note that, if we made the assumption that dpop is in use, and that the (optional) dpop nonces are being used, then an optimal solution would be removing c_nonce from the token response and instead use the dpop nonce as the nonce for the VCI key proof. Doing that is probably also kind of a layering violation though.

On the original point though, Torsten mentioned on today’s call that returning c_nonce from the token response is a nice optimisation that avoids a round trip at the resource server. I wanted to observe that when you’re using nonces with DPoP the first nonce for the resource server has to be obtained from the resource server, similarly causing an extra round trip, and I don’t remember anyone complaining much about the additional round trip for that. (I’m sure Brian will correct me if they have.)

OIDF-automation commented 1 year ago

Imported from AB/Connect bitbucket - Original Commenter: authlete-taka

The current spec provides implementers with two choices.

	c_nonce from token endpoint	round trip at credential endpoint	server implementation	wallet implementation
1	issue	unnecessary	:worried: cumbersome	:grinning: unburdensome
2	not issue	necessary	:grinning: unburdensome	:worried: cumbersome

Removing the option to return c_nonce from the token endpoint will result in removing the first choice in the table above.

Even if we can agree not to issue c_nonce from the token endpoint among us at this moment, there will always be people in the future who will complain about the extra round trip. Therefore, it would be wise to provide two choices without modifying the current specification.

OIDF-automation commented 1 year ago

Imported from AB/Connect bitbucket - Original Commenter: b_d_c

The wallet implementation needs to be able to handle an invalid_proof (or invalid_or_missing_proof b/c the draft currently has both) error/challenge in any case so calling it cumbersome there is a misnomer. When a c_nonce from the token endpoint is an option, the wallet needs to deal with possibly getting the nonce value from two places. It's not making the wallet implementation less cumbersome. It's only optimizing around one http request / response.

‌

OIDF-automation commented 1 year ago

Imported from AB/Connect bitbucket - Original Commenter: pedro-felix

I agree that we are not simplifying the wallet by having the c_nonce on the token endpoint, namely because: 1) it is optional; 2) it may already be invalid when the wallet uses it, so the wallet needs to still correctly react to an error response and use the freshly provided c_nonce on a retry.
OTOH, the c_nonce on the token endpoint is currently optional, so some AS may decide to not implement that support on the token endpoint, keeping its implementation simpler; while leaving the door open for other AS to implement it.
So IMO, the question is: is the additional spec complexity (not necessarily AS complexity) worthwhile just to enable an optimization that some AS may decide to implement?

‌

OIDF-automation commented 1 year ago

Imported from AB/Connect bitbucket - Original Commenter: KristinaYasuda

(or invalid_or_missing_proof b/c the draft currently has both

that’s editorial error. I will do a PR removing references to invalid_or_missing_proof

‌

OIDF-automation commented 1 year ago

Imported from AB/Connect bitbucket - Original Commenter: KristinaYasuda

Having read the arguments, I am increasingly in favor of removing the c_nonce from the token endpoint

OIDF-automation commented 1 year ago

Imported from AB/Connect bitbucket - Original Commenter: tlodderstedt

I would like to hear more feedback from implementers (both wallets and issuers) before we decide.

OIDF-automation commented 1 year ago

Imported from AB/Connect bitbucket - Original Commenter: authlete-taka

I tried writing a virtual wallet-side code for “c_nonce” handling. (Its error handling is not perfect, though.)

// Make a token request and receive a token response.
token_response = make_token_request();

// The access token issued from the token endpoint.
access_token = token_response.access_token;

// The token response MAY contain "c_nonce".
c_nonce = token_response.c_nonce;

// Repeat credential requests until a credential or a transaction ID is issued.
while (true)
{
    // Make a credential request and receive a credential response.
    // It is assumed that the "make_credential_request" method here
    // internally generates a key proof with the given "c_nonce" and
    // includes the key proof in the credential request.
    credential_response = make_credential_request(access_token, c_nonce);

    // If either a credential or a transaction ID has been issued.
    if (credential_response.credential     != null ||
        credential_response.transaction_id != null)
    {
        // Stop repeating credential requests.
        break;
    }

    // If the reason of the error is "invalid_proof" and the value
    // of "c_nonce" in the credential response is different from
    // the one used in the credential request, it is worth trying
    // to make a credential request again with the new "c_nonce".
    if (credential_response.error   == invalid_proof &&
        credential_response.c_nonce != c_nonce)
    {
        // Make a credential request again with the new "c_nonce".
        c_nonce = credential_response.c_nonce;
        continue;
    }

    // Give up.
    break;
}

‌

The virtual code above indicates that whether the token response contains the c_nonce parameter or not is not a significant issue for wallet implementers. (Some people may not agree with this view, though.)

My conclusion is that it is acceptable to allow authorization server implementations to issue c_nonce from their token endpoints if they wish to do so. However, others may reach different conclusions and prefer wallets to make credential requests at least twice (the first one to obtain c_nonce and the second one to get a credential or a transaction ID).

A credential issuer validates an access token that has been issued by an authorization server. Therefore, there exists a relationship between a credential issuer and an authorization server to some degree. If such a relationship is assumed for acess tokens, it should not be a major concern to allow a similar relationship to be established between a credential issuer and an authorization server for c_nonces as well. If you implement an authorization server and a credential issuer, you will notice that a c_nonce always appears with an access token. However, others may have different thoughts and want to prevent a credential issuer and an authorization server from establishing a relationship for c_nonces.

aarmam commented 9 months ago

@OIDF-automation

Could c_nonce be returned by Token Endpoint as Access Token claim? So Credential Issuer can use/trust this initial nonce?

Sakurann commented 9 months ago

so that Issuer can use c_nonce from the access token in the credential error response when telling the wallet which c_nonce to use? that is an implementation detail, i don't think anything in the spec prohibits it, but the spec will be silent about this.

aarmam commented 9 months ago

@Sakurann

Token Endpoint returns Access Token that is signed by Authorization Server - so the claims can be trusted by Credential Issuer and therefore the c_nonce claim could be trusted and the extra roundtrip to ask the first nonce from Credential Issuer can be avoided. You are correct the the spec allows any extra claims needed to access the Protected Resource (Credential Endpoint).

Sakurann commented 9 months ago

Access Token is opaque to the Wallet, it is consumed by the Issuer. the wallet cannot look into the AT and extract any claims.

aarmam commented 9 months ago

@Sakurann Does it have to be? OAuth 2.0 does not dictate the format of AT and OpenID4VCI also not. 1) If opaque AT is returned from token endpoint the Credential Issuer must request token introspection anyway and could verify the c_nonce claim from it. 2) If signed AT jwt is returned from token endpoint the Credential Issuer has all the required info and does not have to do any more requests to Nonce endpoint at Credential Issuer.

Either way c_nonce could be used as AT claim?

aarmam commented 9 months ago

@peppelinux What kind of approach are you considering for c_nonce in Italy's implementation? I see you use opaque AT and therefore token introspection request.

peppelinux commented 9 months ago

Ciao @aarmam

token introspection is used in the SPID CIE OIDC Profile, while for the wallet solution we didn't have put this on the requirements of the credential issuer. In SPID CIE OIDC the access token is a signed JWT.

For the wallet solution we don't have specified yet the format of the AT and I'll back to you soon with a PR here and a comment in this thread

peppelinux commented 9 months ago

@aarmam my bad, it is already defined here https://italia.github.io/eudi-wallet-it-docs/versione-corrente/en/pid-eaa-issuance.html#access-token

the AT is a DPoP token, then a signed JWT, in the specs you find the claims within in

aarmam commented 9 months ago

@peppelinux

the AT is a DPoP token, then a signed JWT, in the specs you find the claims within in

This means it is a opaque AT and token introspection request must be used to get information about AT.

What kind of approach are you considering for c_nonce? Right now I see it is returned by the token endpoint, that means the Authorization Server will request Credential Issuer nonce endpoint? Have you considered the solution I proposed to include c_nonce as AT claim instead, so that Credential Issuer can trust the Authorization Server generated c_nonce?

Or you havent considered this as issue at all because Authorization Server and Credential Issuer are the same entities in Italy's implementation?

peppelinux commented 9 months ago

AT is a signed jwt, then it is not opaque. For the wallet solution we don't use introspection endpoint

Formally the credential endpoint is an RS, no matter if It is within the same node or not.

We have a concrete proposal with the nonce endpoint as well, if confirmed we'll have It in a PR. The sole concern Is that we want to issue a single nonce and that's good for auth code flow but not for pre-authz code flow (we don't use, but It would be better using a solution usable even in other implementation profiles).

Please reach us on GitHub Italia eudi-wallet-it-docs for any related to the italian implementation, and please insist if anything is missing and needs better answers. Thank you for the interest

peppelinux commented 9 months ago

@aarmam I forgot to reply on your proposal!

OAuth 2.0 doesn't require AT in JWT format, then the flow must be designed as the AT was opaque. Thus, the (un)security of the AT as a bearer token, having the c_nonce within It would reduce the security of the token endpoint, where the distinguished AT and c_nonce mades something you have (bearer) and something you know (c_nonce)

Sakurann commented 5 months ago

does anyone remember why we chose c_nonce as opposed to putting access token hash into the proof? if access token is sender-constrained, wouldn't that be stronger than c_nonce, so maybe we can drop the c_nonce option altogether..?

peppelinux commented 5 months ago

OAuth 2.0 doesn't mandate the access token be sender constrained.

issuing different c_nonce in the credential response allows to reuse the same access token, with different c_nonces, to obtain more tha a single credential without using the batch endpoint

jogu commented 1 month ago

does anyone remember why we chose c_nonce as opposed to putting access token hash into the proof? if access token is sender-constrained, wouldn't that be stronger than c_nonce, so maybe we can drop the c_nonce option altogether..?

Annoyingly those two alternatives have slightly different security properties, but it's rather hard to reason about because of all the optionality:

If the access token was sender-constrained and the token and credential endpoint require DPoP nonces then it's definitely at least as strong as c_nonce (but actually probably stronger). If instead dpop nonces aren't in use at all and the access token was long lived, then that's probably weaker (in particular it's vulnerable to the pre-generation attacks that dpop nonces prevent).

I was a bit ambivalent on whether we should remove c_nonce from the token endpoint or not, but after today's working group meeting I think I'm more in favour of removing c_nonce at the token endpoint from the spec.

The only advantage of c_nonce from the token endpoint that, if supported by the AS, it avoids one http call, i.e. it's potentially a performance gain, albeit I believe probably a fairly minor one.

In theory, the AS returning c_nonce at the token endpoint is completely optional and hence it doesn't impose a burden on authorization servers and they're free to implement or not implement at the token endpoint (as they can always return a c_nonce at the credential endpoint instead). And some authorization servers just won't implement it, because it introduces a coupling between the AS & the RS that's not always easy to do.

In practice this does complicate the spec (which is complicated enough even without this part), and worse it was mentioned on today's working group call that some wallet implementors end up assuming that they will get a c_nonce from the token endpoint and hence it's causing interoperability issues with authorization servers that don't issue one.

bc-pi commented 1 month ago

In practice this does complicate the spec (which is complicated enough even without this part), and worse it was mentioned on today's working group call that some wallet implementors end up assuming that they will get a c_nonce from the token endpoint and hence it's causing interoperability issues with authorization servers that don't issue one.

I tried to mention some of the potential mismatched expectations and issues that'll likely result in this https://github.com/openid/OpenID4VCI/pull/199#issuecomment-1879763498 on the rejected PR to Remove c_nonce* from the token response but I didn't anticipate wallets being unable to interoperate due to not implementing mandatory nonce handling at the credential endpoint. Which is not a good thing.

nemqe commented 1 month ago

By doing interop with wallet providers I've noticed that there is a bit of confusion regarding the proof / no-proof flows and how the nonces are communicated to the wallet.

We found that some wallets and issuers thought that the nonce should be by default returned from the token endpoint leading to some discussion whether the nonce in the error response is a valid way to start the flow or just a fallback crutch to enable ASs that can't serve the nonce through the token response.

This reminded me of 'the early' days where some implementers wanted to save http calls and started moving token_endpoint and similar attributes to the `openid-credential-issuer - was a cool looking optimization but we ended up with so much optional flows and variety that in the end it just muddied the understanding of the flow.

I like optimizations and less round trips, but I would keep it simple for the beginning so we have one way of doing things that is stable and can evolve, and later we can optimize.

I just give this as a feedback to highlight that we might need additional clarifications regarding this topic as it is causing some confusion on the implementer side leading to interoperability issues that stem from misunderstanding the spec text.

That being said, I do find it a bit unnatural to force errors in order to get a nonce, but I prefer that compared to multiple optional paths (although my opinion regarding this is not strong). I think a piece of text that clearly states what is mandatory and what is an optimization, how the first credential request is sent, is it with a proof with null nonce, or without proof at all is required so we loose the either/or but make it into a must/if you really want to.

I vaguely remember that a "nonce generation" endpoint was once discussed but don't know where that conversation ended up.

Also, not a cryptographer here, but can we have a no-negotiation simple nonce value that is deterministic but resistant to fight replays and prevent same values like hash of the token value + a nonce that is a timestamp that is not in the past but not more than x seconds in the future or something similar? Non really proposing the last one, just curious, although I see that in this case the issuer would have to ensure that same nonce values are not used.

babisRoutis commented 1 month ago

That being said, I do find it a bit unnatural to force errors in order to get a nonce, but I prefer that compared to multiple optional paths (although my opinion regarding this is not strong). I think a piece of text that clearly states what is mandatory and what is an optimization, how the first credential request is sent, is it with a proof with null nonce, or without proof at all is required so we loose the either/or but make it into a must/if you really want to.

:+1:

In case proofs are required, it is clear that the credential endpoint must provide c_nonce both for successful or error responses, regardless of the token endpoint.

For this reason, c_nonce provided by token endpoint is an optional feature. If present, allows wallet to save a http call. On the other hand, it adds some complexity to an already complex specification.

By keeping only the mandatory requirement (c_nonce from credential endpoint) we can lower specification complexity, make it easier for both issuers and wallet and increase interoperability,

Finally, perhaps credential endpoint should always return a c_nonce regardless if the request corresponds to a credential configuration that requires/supports proofs. Reason for this is that with the same access_token wallet can place consecutive requests, for instance one for a credential type that doesn't require proofs and another that requires proofs.

peppelinux commented 1 month ago

My comments here https://github.com/openid/OpenID4VCI/issues/331#issuecomment-2195821640

peppelinux commented 4 weeks ago

It has been a year, and I find myself reflecting more on the design than on the solution itself.

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 credential endpoint (RS) , should not be handled in the token endpoint response, even if optional, which serves a broader purpose in token issuance.

The token endpoint only returns tokens and metadata about the tokens, not parameters optionally required for RS endpoints.

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, as well as creating difficulties in traditional OAuth 2.0 implementations.

Masking a breaking change with optionality, and at the same time saying that this optionality is vital for the security of the solution, can consequently represent two things, one or the other or both at the same time:

design error
breaking change due to security requirement

therefore I support the removal of the c_nonce issuance in the token endpoint response.

About the way we secure the RS we're working on several others issues and therefore I try to not blat too much this one.

awoie commented 4 weeks ago

I support removing c_nonce from token endpoint since it in a lot of cases, this is quite challenging to implement. Especially, if AS != Issuer, or if existing software for AS is used by the issuer. Having this option just leads to more complexity which could be removed. Also conceptually it is weird that AS and Issuer have to share the responsibility to issue c_nonce tokens.

TakahikoKawasaki commented 3 weeks ago

Well, if there is such strong resistance to including c_nonce in the token response, there should logically be similar resistance to including credential_identifiers in the token response as well... (cf. PR #346)

Who manages the correspondences between credential_identifiers and the represented datasets? The authorization server or the credential issuer? If the authorization server manages the correspondences, how does the credential issuer know the correspondences? If the credential issuer manages the correspondences, why does the authorization server issue credential_identifiers?

Even if it is difficult to implement including c_nonce in the token response, is it easier to implement including credential_identifiers in the token response? When an access token and a credential_identifier are presented to the credential endpoint, how does the credential endpoint implementation obtain information about the dataset represented by that credential_identifier? Is it possible to obtain that information in a non-proprietary way?

If the same logic for removing c_nonce from the token response applies to credential_identifier, the initial credential request (which is destined to fail) should be made to obtain not only a valid c_nonce but also valid credential_identifiers with information about correspondences between the credential_identifiers and the represented datasets.

babisRoutis commented 3 weeks ago

Well, if there is such strong resistance to including c_nonce in the token response, there should logically be similar resistance to including credential_identifiers in the token response as well... (cf. PR #346)

Who manages the correspondences between credential_identifiers and the represented datasets? The authorization server or the credential issuer? If the authorization server manages the correspondences, how does the credential issuer know the correspondences? If the credential issuer manages the correspondences, why does the authorization server issue credential_identifiers?

Even if it is difficult to implement including c_nonce in the token response, is it easier to implement including credential_identifiers in the token response? When an access token and a credential_identifier are presented to the credential endpoint, how does the credential endpoint implementation obtain information about the dataset represented by that credential_identifier? Is it possible to obtain that information in a non-proprietary way?

If the same logic for removing c_nonce from the token response applies to credential_identifier, the initial credential request (which is destined to fail) should be made to obtain not only a valid c_nonce but also valid credential_identifiers with information about correspondences between the credential_identifiers and the represented datasets.

Dear @TakahikoKawasaki
Fully agree on that !

credential_identifiers have to do with the nature of the credential being issued, thus it is - or it should have been - a credential issuer's concern.

Having to work in a use case with a legacy authorization server, I ended up exposing from the credential issuer a protected endpoint that would return to the wallet the credential_identifiers that correspond to a given offer.

I have the feeling that this is broader though. For every state-full credential offer (with issuer_state in authorized code flow, or if it is pre-authorized code), the credential issuer should be aware of the context, mainly :

Which credential configurations and
if applicable, which credential_identifiers
if applicable, c_nonce

openid / OpenID4VCI