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

client_id_scheme security considerations #124

Closed
danielfett opened this issue Mar 5, 2024 · 48 comments · Fixed by #263
Closed

client_id_scheme security considerations #124

danielfett opened this issue Mar 5, 2024 · 48 comments · Fixed by #263

Comments

@danielfett
Copy link
Contributor

danielfett commented Mar 5, 2024

When client_id_scheme is used, there can be multiple client_ids in the same ecosystem that belong to different clients. One of those clients could be malicious, compromised or the client_id scheme could allow for spoofing/impersonating client ids. One such insecure/spoofable client must not endanger the security of other clients in the same ecosystem.

In the protocol, since the client_id_scheme parameter namespaces the client_id, it should appear everywhere where client_id appears, including in aud values and in the sub value of the Verifier Attestation JWT.

In the credentials, the client_id_scheme should be included besides the client_id as the audience; e.g., in an SD-JWT KB-JWT, there should be a separate claim besides aud.

In Section 12.1, the following sentences need to be adapted as well:

The Wallet MUST link every Verifiable Presentation returned to the Verifier in the VP Token to the client_id and the nonce values of the respective Authentication Request.

The Verifier MUST validate every individual Verifiable Presentation in an Authorization Response and ensure that it is linked to the values of the client_id and the nonce parameter it had used for the respective Authorization Request.

The client_id is used to detect the presentation of Verifiable Credentials to a party other than the one intended.

In the security considerations there should be considerations about confusing two client IDs with different client ID schemes.

Everywhere where a party checks a client id (especially the AS and the client), it must check the tuple (client_id, client_id_scheme) instead. This also applies if client_id_scheme is not used by one of the parties (in which case client_id_scheme must be replaced by, e.g., null).

Edit 2023-05-17: Fixed a mistake in the third paragraph.

@Sakurann
Copy link
Collaborator

Sakurann commented Mar 5, 2024

sorry, I am probably missing something, if client_id belongs to a compromised or malicious client that is part of the ecosystem using the same client_id_scheme, how does including client_id_scheme help detect that.

@jogu
Copy link
Collaborator

jogu commented Mar 5, 2024

sorry, I am probably missing something, if client_id belongs to a compromised or malicious client that is part of the ecosystem using the same client_id_scheme, how does including client_id_scheme help detect that.

It's the equivalent of suggesting instead of "aud": "https://www.example.com" in a JWT for a client we instead have "aud": "redirect_uri:https://www.example.com" if the client_id uses the redirect_uri client_id_scheme (but instead of having a structured aud value Daniel is suggesting having a separate claim that recipients of the JWT will need to check).

I'm not sure if there is actually a practical issue unless a client_id_scheme that allows the attacker to select a completely arbitrary client_id is supported (which I don't think any of the currently defined client_id_schemes do, I think they pretty much all require a url or hostname or an AS-assigned client_id).

It does feel like there might be an issue when client_id_schemes result in what is nominally the same client having the same client_id but with different security levels. So for exactly where a client intends to have client_id_scheme=entity_id&client_id=https://www.example.com but the attacker instead uses client_id_scheme=redirect_uri&client_id=https://www.example.com and is then able to obtain a VC with an aud of https://www.example.com that it can attempt to inject into a flow for the legitimate client.

@awoie
Copy link
Contributor

awoie commented Mar 18, 2024

In the credentials, the client_id should be included as the audience; e.g., in an SD-JWT KB-JWT, there should be a separate claim besides aud.

Each credential format has to define how to use client_id and client_id_scheme in the response.

W3C VCDM with Data Integrity:

  • domain parameter has to include the fully qualified client_id (FQCID), i.e., <client_id_scheme>:<client_id>

SD-JWT VC:

  • KB-JWT could have an additional claim, client_id_scheme, or use the FQCID as well for the aud value.

W3C VCDM with JWT:

  • aud parameter that contains the FQCID, or use the same mechanism as SD-JWT VC.

ISO mdoc:

  • OID4VPHandover structure has to include the FQCID if other client ID schemes than x509_san_dns are used (x509_san_dns is the default since ISO 18013-7 only allows this). ISO 23220-4 could potentially allow other client ID schemes.

@awoie
Copy link
Contributor

awoie commented Mar 18, 2024

This also applies if client_id_scheme is not used by one of the parties (in which case client_id_scheme must be replaced by, e.g., null).

IMO, if client_id_scheme is not present, it is pre-registered (default value if omitted).

@jogu jogu added the id3 label Mar 21, 2024
@danielfett
Copy link
Contributor Author

I'm not sure if there is actually a practical issue unless a client_id_scheme that allows the attacker to select a completely arbitrary client_id is supported (which I don't think any of the currently defined client_id_schemes do, I think they pretty much all require a url or hostname or an AS-assigned client_id).

The problem is that this security is accidental and neither guaranteed in all deployments nor for any future extensions that define new client id schemes.

@jogu
Copy link
Collaborator

jogu commented Mar 26, 2024

@danielfett yes, exactly. For clarity whilst I can't identify a practical attack with the current client id schemes I do agree there's a real issue here we need to look at fixing.

@Sakurann
Copy link
Collaborator

in-person WG:

  • no clear direction...
  • @tplooker volunteered to try provide a text/mechanism that does not use client_id_scheme parameter but achieves what the spec enables now (flexibility of verifier authentication mechanisms)
  • if removing client id schemes is not an option, namespacing client id sounded like the best option

@peppelinux
Copy link
Member

I've come to believe that the security concerns discussed in this thread primarily pertain to the redirect_uri client_id_scheme. If other client id schemes do not share this vulnerability, my recommendation - even if it might sound very unpopular - is to discontinue support for the redirect_uri scheme and remove it from the specifications. Personally, I have always perceived the redirect_uri scheme as a security risk and have been critical of its implementation.

Regarding the potential security issues with new client id schemes mentioned by @danielfett and @jogu, I believe such concerns should be addressed by those future extensions that have not yet identified their security weaknesses. Something that seems pretty out of scope for openid4vci.

I agree with @awoie with the approach that if the parameter is absent, the Wallet MUST follow the behavior outlined in RFC6749.

In the context of OpenID Federation, the client id by itself is sufficient to initiate trust discovery, unless it is an HTTPS URL. While the client id scheme serves as a useful indicator for determining the pattern of client id evaluation.

It's important to note that even with namespaced client ids, an attacker could potentially replace the entire client_id value, redirecting to any values.

Therefore, I suggest that our focus should be more on addressing the root of the problem—the redirect_uri—rather than the representation of the client identifier itself.

@jogu
Copy link
Collaborator

jogu commented Apr 21, 2024

I've come to believe that the security concerns discussed in this thread primarily pertain to the redirect_uri client_id_scheme. If other client id schemes do not share this vulnerability

There is a similar ambiguity between entity_id and x509_san_uri

It's important to note that even with namespaced client ids, an attacker could potentially replace the entire client_id value, redirecting to any values.

That would result in client authentication failing as far as I can see.

Therefore, I suggest that our focus should be more on addressing the root of the problem—the redirect_uri—rather than the representation of the client identifier itself.

The root cause of the problem is the introduction of client_id_scheme in the VCI spec, hence I believe the issue needs to be addressed in the VCI spec.

@peppelinux
Copy link
Member

@jogu formally you are completely right, as usual. At the same time, from my perspective, a trust discovery using federation or x509 can be achieved by using the client_id alone.

I don't have any expectations about not taking this in the scope of openid4vci, therefore I hope that my comment won't block the conversation about the solution we may have to improve the current specs.

@tplooker
Copy link
Contributor

In the protocol, since the client_id_scheme parameter namespaces the client_id, it should appear everywhere where client_id appears, including in aud values and in the sub value of the Verifier Attestation JWT.

This is interesting, I wouldn't have described the client_id_scheme as namespacing the client_id. I would have described the purpose of the client_id_scheme parameter as telling you how to interpret the client_id. I can see however the security of client authentication becomes tied to the weakest client_id_scheme supported by the party receiving the request is that a correct way to interpret it @danielfett? e.g you could potentially have the same client_id across two different client_id_schemes?

@tplooker
Copy link
Contributor

Just a note related to @Sakurann's comment above, I will provide another proposal related to this issue soon which could help simplify things.

@tplooker
Copy link
Contributor

As said above, here is my proposal

While we consider this issue, I think we should also decide whether all of the client_id_schemes we currently have defined are worth persisting with. IMO, if we consider the original intent of the client_id_scheme it was really about devising a way for a client to identify itself and communicate its metadata with the wallet in a way that doesn't require pre-registration. For a variety of reasons we've ended up with perhaps more client_id_schemes then are practically needed, for instance below is the current set of client id schemes supported and used

  • x509_san_dns
  • x509_san_uri
  • redirect_uri
  • verifier_attestation
  • did
  • pre-registered

One possible proposal here would be to do the following

  • Simplify to have one client ID scheme of uri where when used, the URI needs to have a scheme of either https or did.
  • To cater for the current behaviour of the verifier_attestation x509_san_dns and x509_san_uri client id schemes, which IMO are more about how the client authenticates rather then how it identifies itself we shift to a model which relies on the presence of certain parameters in the signed request object header, such as x5c.

How would this look, a standard request would look like

HTTP/1.1 302 Found
Location: https://client.example.org/universal-link?
response_type=vp_token
&client_id=https%3A%2F%2Fclient.example.org
&client_id_scheme=uri
&redirect_uri=https%3A%2F%2Fclient.example.org%2Fcb
&presentation_definition=...
&nonce=n-0S6_WzA2Mj
&client_metadata=...

In a scenario where the requested is signed and the client/verifier's signing keys need to be traced via a chain of x509 certificates (e.g how x509_san_dns works), the signed request would look as it does now, for example decoded to

Header

{
    "typ": "oauth-authz-req+jwt",
    "alg": "RS256",
    "x5c": ["...", "..."]
}

Payload

{
   "client_id": "https://client.example.org",
   "client_id_scheme": "uri",
   "response_type": "vp_token",
   "redirect_uri": "https://client.example.org/cb",
   "nonce":"n-0S6_WzA2Mj",
   "presentation_definition": { ... },
   "client_metadata": { ... }
}

When the client wants to use the verifier_attestation style behaviour the signed request would look as follows

Header

{
    "typ": "oauth-authz-req+jwt",
    "alg": "RS256",
    "jwt": "..."
}

Payload

{
   "client_id": "https://client.example.org",
   "client_id_scheme": "uri",
   "response_type": "vp_token",
   "redirect_uri": "https://client.example.org/cb",
   "nonce":"n-0S6_WzA2Mj",
   "presentation_definition": { ... },
   "client_metadata": { ... }
}

@David-Chadwick
Copy link
Contributor

If we go back in time to when the client_id_scheme parameter was introduced, it was as a result of issue 1551 that I introduced. This was essentially "How can the wallet trust the verifier". I wanted this new parameter to define the trust scheme being used, so that the wallet could look up the client_id in its trust infrastructure to determine if the client_id was trusted. But this is not how the parameter has evolved since then. However if we return to the original issue, then redefining client_id_scheme to indicate the trust scheme in operation (which in fact some of the values already do e.g. entity_id indicates OpenID Federation) then we might have a solution to this issue.

@jogu
Copy link
Collaborator

jogu commented May 17, 2024

@David-Chadwick

then redefining client_id_scheme to indicate the trust scheme in operation (which in fact some of the values already do e.g. entity_id indicates OpenID Federation) then we might have a solution to this issue.

I don't think I'm following how this solves the problem, can you expand please? It seems like the fundamental issue of client id's not being unique to a trust scheme and a client id alone not being sufficient to indicate how the client wants to be / was authenticated would remain.

@jogu
Copy link
Collaborator

jogu commented May 17, 2024

@tplooker

  • Simplify to have one client ID scheme of uri where when used, the URI needs to have a scheme of either https or did.
  • To cater for the current behaviour of the verifier_attestation x509_san_dns and x509_san_uri client id schemes, which IMO are more about how the client authenticates rather then how it identifies itself we shift to a model which relies on the presence of certain parameters in the signed request object header, such as x5c.

Can you expand on how this scheme would solve the "In the credentials, the client_id_scheme should be included besides the client_id as the audience; e.g., in an SD-JWT KB-JWT, there should be a separate claim besides aud." part of Daniel's original problem statement?

@David-Chadwick
Copy link
Contributor

@jogu Indicating the trust scheme that should be used to validate the client_id solves the problem because the wallet can determine if the RP is trusted or not by communicating with its trust scheme and root(s) of trust. The assumption is that every trust scheme will not have duplicate client_ids (because if it did have, then no-one could determine if a specific client_id was the intended trusted one or not.)
It does not mater if a different trust scheme use the same client_ids (as another trust scheme) to refer to different RPs, because the wallet is operating in its own trust scheme and determines if the RP is trusted under its own trust scheme rules. So if RP1 and RP2 have the same client_id but these are issued under different trust schemes, TS1 and TS2 respectively, the wallet will know which RP is trusted dependent upon the asserted trust scheme. It is very important that the trust scheme provides all the important parameters of the RP (the ones provided in the protocol should not be trusted because they do not need to be signed and they could be fraudulent). So say RP2 fraudulently says its client-id has been allocated by TS1, the wallet will nevertheless believe it is talking to RP1 and will proceed on that basis. RP2 will never receive any credentials from the wallet because the trust scheme will have provided RP1's URL. The worst thing that can happen to the user is that RP1 (which is trusted) receives unexpected credentials from the wallet when it had not requested them. Alternatively the wallet may detect anomalies between what the trust scheme says about RP1 and what RP2 said about itself, and therefore decides to terminate the connection.

@jogu
Copy link
Collaborator

jogu commented May 18, 2024

@David-Chadwick

The worst thing that can happen to the user is that RP1 (which is trusted) receives unexpected credentials from the wallet when it had not requested them.

I don't think that's the worst thing that can happen. The verifier RP1 can potentially receive credentials for one user in a session started by a different user.

@David-Chadwick
Copy link
Contributor

You would have to explain to me how this could possibly happen.
Wouldn't it require two different users to be colluding with a rogue RP (RP2) in order to gain access to a trustworthy RP (RP1)? In which case the two users could collude much more easily than needing to involve RP2.
But if the two users are not colluding, then the user with the high value credentials would need to be tricked into sending their credentials to RP1 when the user with no or low value credentials is trying to access it. So it sounds highly improbably to me, unless I have missed something.

@jogu
Copy link
Collaborator

jogu commented May 19, 2024

Wouldn't it require two different users to be colluding with a rogue RP (RP2) in order to gain access to a trustworthy RP (RP1)? In which case the two users could collude much more easily than needing to involve RP2.

An example would be one user is an attacker, the second user is being phished.

We're probably veering of the main point as we don't have a full attack that works with what's defined today. The problem is that both the spec and in particular client_id_scheme have extension points, so we're leaving a potential foot gun that might result in unintended consequences when people add/change things in the future.

What we do know is that in general signed messages that are received are normally validated to be to and from the expected parties, and by not including & checking client_id_scheme (whilst also having existing and potential future client id schemes that will assign client ids that have the same value but are either different clients or have been authenticated in different potentially much less trustworthy ways) in those messages we have broken that property.

@David-Chadwick
Copy link
Contributor

This is precisely why I am suggesting that client_id_scheme should refer to the trust scheme and the client_id should be evaluation with respect to this trust mechanism (which it already does for X.509 for example, so make it do this for all the other schemes as well. Currently it has imprecise semantics).

@peppelinux
Copy link
Member

@tplooker

for instance below is the current set of client id schemes supported and used

x509_san_dns
x509_san_uri
redirect_uri
verifier_attestation
did
pre-registered

entity_id should be taken in consideration and therefore included in the list.

@selfissued
Copy link
Member

I've been thinking about this for a while. It seems to me that introducing client_id_scheme may have created more problems than it solves. I would therefore support proposals to remove it.

Some have advocated in this thread that if client_id_scheme isn't present, that the default should be that the Client ID is allocated by the Authorization Server. I think the answer should be closer to "the default is defined by the ecosystem rules".

For instance, in OpenID Federation, we have years of experience with Client IDs that are Entity Identifier URLs. They work great. In those deployments, no client_id_scheme is needed or used, and they're not allocated by ASs. In this case, the default is that the Client ID is an Entity ID.

@David-Chadwick
Copy link
Contributor

David-Chadwick commented May 20, 2024

"it's okay for everything we know about today"

Isn't that how all security works? and patches are introduced when new facts and bugs come to light

@David-Chadwick
Copy link
Contributor

x509_san_dns - indicates the trust scheme
x509_san_uri - indicates the trust scheme
redirect_uri - does not say anything about the trust scheme
verifier_attestation - not sure what this indicates
did - some dids might indicate the trust scheme and others do not
pre-registered - indicates the trust scheme
entity_id - indicates the trust scheme but might be better named "federation"

@tplooker
Copy link
Contributor

tplooker commented May 21, 2024

Can you expand on how this scheme would solve the "In the credentials, the client_id_scheme should be included besides the client_id as the audience; e.g., in an SD-JWT KB-JWT, there should be a separate claim besides aud." part of Daniel's original problem statement?

Well if there is really only "one" type of client id scheme which is "uri" do we even need to include the client_id_scheme parameter in the response anymore? I would posit we don't.

@paulbastian
Copy link
Contributor

paulbastian commented Jun 13, 2024

My observation is that client_id and client_id_scheme should always be used in conjunction, therefore this is a good indicator that these two parameters should be merged, i.e. client_id_scheme should be the scheme for client_id and we should find ways how to solve potentially evolving issues.

Or do we have other examples in the specs, where two parameters must always be used together and they are not merged?

@Sakurann
Copy link
Collaborator

WG discussion: there seems to be an agreement on the problem. but there still seems to be a disagreement on the solution:

  1. it is not a severe enough of a problem, keep things as-is
  2. namespace client_ids (especially, if those two things always have to be used together; would require an extra processing steps for the wallet to remove the namespace, before processing the client_id itself; this approach deviates from oauth, where client_id is not namespaced, which is also kind of due to the nature of wallet space being different)
  3. whenever client_id is used, mandate to also include client_id_scheme

@selfissued
Copy link
Member

Based on the discussion on the call today, I continue to believe that we should remove client_id_scheme. It creates more problems than it solves.

@c2bo
Copy link
Member

c2bo commented Jun 14, 2024

Based on the discussion on the call today, I continue to believe that we should remove client_id_scheme. It creates more problems than it solves.

How would we convey the mechanism to establish trust in the RP if we remove the client_id_scheme ? Wouldn't we require something very similar that likely shares the same problems?

My observation is that client_id and client_id_scheme should always be used in conjunction, therefore this is a good indicator that these two parameters should be merged, i.e. client_id_scheme should be the scheme for client_id and we should find ways how to solve potentially evolving issues.
Or do we have other examples in the specs, where two parameters must always be used together and they are not merged?

I am with Paul on this one but I do feel I don't fully understand the problems that possibly prefixing client_id would create.

@David-Chadwick
Copy link
Contributor

Well if there is really only "one" type of client id scheme which is "uri" do we even need to include the client_id_scheme parameter in the response anymore? I would posit we don't.

Since I started the issue over a year ago which led to client_id_scheme being adopted, I have never agreed with the resolution as it currently stands. I have always maintained that what we need is a method to indicate the trust scheme that the client_id should be used with. Client_id_scheme was the resolution that the group adopted, but as we can see, it is not a resolution of the original problem that I raised. So I propose that we remove client_id_scheme and introduce another parameter trust_scheme, which is an optional parameter that RPs may use if they need to indicate to the wallet what trust scheme the client_id should be used with. If the client_id on its own indicates the trust scheme that it is to be used with, then the parameter is not needed, but if there is no indication of the trust scheme, or it is ambiguous which trust scheme should be used, then the RP will indicate this in the trust_scheme parameter. The trust_scheme parameter should be a freeform string, and we can define certain values for it, but other users may define additional values in future.

@danielfett
Copy link
Contributor Author

danielfett commented Jul 25, 2024

My proposal (if we agree to namespace) is to add the client id scheme as a URI scheme in front of the client id:

  • x509_san_dns:client.example.org
  • redirect_uri:https://client.example.com
  • did:did:example:123 (the first did: can be dropped here, so did:example:123)

@tlodderstedt
Copy link
Collaborator

I originally was opposing against the change as I believed it would break existing OpenID federation implementations. However, I realized the way we use federation in OID4VP today, it already requires changes as federation implementation need to add processing of the client_id_scheme parameter. So it does not matter whether we have an additional parameter or add a prefix to the client id. Both require changes.

@Sakurann
Copy link
Collaborator

WG discussion

  • @awoie and @tplooker to make a proposal on august 6th DCP WG call.
  • would like to get implementation experience since it is a breaking change. especially for openid federation folks.

@selfissued
Copy link
Member

The fact that this is acknowledged as breaking things reinforces my conclusion that client_id_scheme was a mistake in the first place. We should remove it.

@David-Chadwick
Copy link
Contributor

@selfissued. client_id_scheme as currently defined is a mistake. We can agree on that. But a parameter is needed to indicate the trust scheme as I explained above. @danielfett 's proposal may do this if the semantics are clearly defined.

@awoie
Copy link
Contributor

awoie commented Aug 6, 2024

WG discussion

  • @awoie and @tplooker to make a proposal on august 6th DCP WG call.
  • would like to get implementation experience since it is a breaking change. especially for openid federation folks.

The proposal is to remove client_id_scheme and replace it by the following mechanism.
If a client_id is a URL, one can look up metadata using a small number of selected .well-known suffixes. For example, one of them could be .well-known/openid-federation to enable trust establishment. Other .well-known can be defined by the core spec and by profiles. E.g. there might be a .well-known based on web domains, see #82.

@awoie
Copy link
Contributor

awoie commented Aug 20, 2024

Remove client_id_scheme to solve #124

Rationale

Originally, the concept of a client ID scheme was introduced to govern the following:

  • How the client ID is interpreted as an identifier in a certain trust method.
  • Whether the request MUST, MAY, or MUST NOT be signed.
  • How the wallet MUST determine the verifier metadata.

However, the primary function of client_id_scheme in OID4VP seems to be:

  • Verifying the linkage between the client_id and the verification public key of the request object.
  • Verifying the request object (or the signer of the request object, e.g., the key) to trust the verifier metadata passed in the client_metadata parameter, e.g., encryption keys. The public key that signed the request object is typically not included in the verifier metadata itself, e.g., X.509 schemes, verifier attestation, DID.
  • Verifying the linkage between the redirect_uri (response_uri) and the client ID, which was attested to the client. This is the case for verifier attestations, redirect_uri client ID scheme and for X.509 schemes if SAN was used. Not all client ID schemes support this type of linkage verification, e.g., DID. Note, in OpenID Federation and pre-registered clients, this linkage is provided by the redirect_uris client metadata parameter.

Additional verifier metadata was a secondary use case since there are not many relevant ones pertaining to OpenID4VP. client_name might be one of the few.

The problem that client_id_scheme created:

  • The need to carry a second parameter everywhere client_id is used.
  • A very invasive set of changes to existing protocols.
  • It created a security issue when used with VPs (this issue client_id_scheme security considerations #124), i.e., client_id + client_id_scheme is effectively the audience. This has to be considered for every credential format.
  • Tight coupling between client_id_scheme and how the linkage between the verification public key of the request object and the client_id has to be verified. It is hard to combine existing linkage methods (e.g., X.509 with OpenID Federation, verifier attestation with OpenID Federation entity IDs, etc.), or define alternative options as fallbacks (e.g., X.509 root certs are not trusted but verifiers want to provide an alternative option based on HTTPS .well-known).
  • The need to keep track of a registry of client_id_scheme values.
  • Ecosystems might want to define their own rules especially on X.509 certificate profiles which might introduce requirements competing with how certain client ID schemes are defined in OpenID4VP.
  • Inconsistent security gurantees across client ID schemes, i.e., which of the linkages is verified. For example, some verify redirect_uri (or response_uri) linkage, some don't.

Attempting to solve #124 by prefixing client_id values with the client_id_scheme does not solve all the problems that were described above. Furthermore, this would be a breaking change to other specifications OpenID4VP is used with and to existing deployments of OpenID4VP.

Removing the concept of client ID scheme would address all of the issues above. Note that this would still allow for verifier metadata being passed in the client_metadata parameter, or obtained via other mechanisms.

Observation

Given the small number of client ID schemes, these linkages can also be verified by inspection while being more flexible and less limiting in how to resolve verifier metadata by even allowing for combining different methods.

Proposal

I propose the following:

  1. Remove client_id_scheme and the concept of client ID scheme from OID4VP.
  2. Keep some of the language that is related to specific client ID schemes and focus on how these linkages can be verified based on inspection. Potentially, move some of the language to the Implementation or Security Consideration sections.

Regarding 2), I propose the following:

  • x509_san_dns and x509_san_uri

    Add text to the Security Considerations section on how to trust the client_id and redirect_uri (or response_uri) using X.509 certificates included in the request object, i.e., validate the X.509 SAN entries in case the certificate was issued by a public/general purpose root CA that verifies SAN entries; or having a registry of trusted root CAs that attest the genuineness of clients possessing X.509 certificates to sign request objects.

  • did

    Note, the DID client ID scheme doesn't enable verification of the linkage between the redirect_uri (or response_uri) and the client ID today. These are self-asserted by the DID controller that signs the request object.

    Add text to Security or Implementation Considerations section how to verify a request object using DIDs, i.e., by checking the client_id is a DID and the keys from the kid of the JWT (request object). Add to Security Considerations that redirect_uri, even if the request object is signed, is not linked to the DID or client_id, which is missing today.

  • entity_id

    Add language to the Implementation Considerations section on performing .well-known lookups (e.g., OpenID Federation) to retrieve verifier metadata (including redirect_uris) if the client_id scheme is a HTTPS URI.

  • verifier_attestation

    Keep the verifier attestation section, i.e., include the verifier attestation JWT in the jwt header of the authorization request object and perform all the checks on cnf keys as well as that client_id matches the sub claim of the verifier attestation. The typ header parameter allows the wallet to determine whether the JWT in the jwt header is a verifier attestation and check whether client_id matches the sub of the verifier attestation. Because we have the typ header parameter, there is no need to indicate the use of verifier attestation in another parameter such as client_id_scheme.

  • redirect_uri

    Add language to the Security Considerations section on how to verify the linkage if client_id matches redirect_uri.

  • pre-registered:

    Remove this paragraph and update the Security/Implementation Considerations section, if required.

Conclusion

Removing the concept of client ID scheme specifically solves the original security issue (#124) and all the issues introduced in the rationale while also simplifying the specification, being more flexible and less limiting and not introducing any potential backwards compatibility issues with how the client_id value was previously defined and used in OAuth2 and OpenID-related specifications (e.g., OpenID Federation).

The proposal also allows the decoupling the verification of the linkage of public verification keys for request objects and the client_id value and/or verifier metadata. For example, a verifier attestation might be used with an entity ID value for the client ID and if the verifier attestation cannot be verified because the issuer is not trusted, .well-known lookup based on OpenID Federation could be performed.

@David-Chadwick
Copy link
Contributor

It should not be mandatory to sign the request object, in which case the wallet/holder will not have a public key for the verifier. Nevertheless the holder should still be able to determine if the asserted client_id is trustworthy (or not) by interrogating its trust infrastructure. In which case it may need an indication of which trust method and/or root of trust to use, as suggested by the verifier. This is why an optional trust_scheme parameter may be needed instead of client_id_scheme.

@javereec
Copy link
Contributor

I like the direction @awoie provided. The rationale is sound and the proposed solution is simplified and provides flexibility. One remark while reading.

Add text to Security or Implementation Considerations section how to verify a request object using DIDs, i.e., by checking the client_id is a DID and the keys from the kid of the JWT (request object). Add to Security Considerations that redirect_uri, even if the request object is signed, is not linked to the DID or client_id, which is missing today.

It could be argued that there likely IS a way to add redirect_uri to a DID Document, therefore providing a linkage between the two.

@jogu
Copy link
Collaborator

jogu commented Aug 20, 2024

Oliver talked through his above comment made today on today's working group call.

Several working group members commented that they didn't see exactly how a wallet would be expected to process an incoming request to figure out the client authentication method being used, and there were similar comments about wanting to understand exactly what would go in the spec.

Oliver/Tobias agreed to provide more details, there was some discussion about whether it's best to provide more detail on the issue or as a PR, with Joseph/Kristina preferring it on the issue, however on reflection after the call the chairs felt a PR would be fine if Oliver/Tobias feel that's a better/easier way to express their thoughts.

I'm not marking with the 'ready-for-pr' yet as the working group has not yet reached consensus on adopting this approach or not, and any PR would just be to make the suggested approach clearer.

@marcoscaceres
Copy link
Contributor

I tend to agree that dropping client_id_scheme would be a good idea (particularly if there's situations where client_id there's a mismatch with client_id_scheme). If we can derive everything we need just from client_id, then that would be great.

awoie added a commit that referenced this issue Aug 27, 2024
@awoie
Copy link
Contributor

awoie commented Sep 10, 2024

Proposal dropping client_id_scheme and fixing the security issue described in this issue is here #237

@danielfett
Copy link
Contributor Author

On a call today, we identified a potential solution to this issue:

  • We use the prefixing approach that I proposed initially.
  • The OpenID Federation scheme is renamed to https

This would mean that an Federation entity ID can be used without any changes (effectively without a prefix).


For example, for x509_san_dns, the authorization request would look like this:
... client_id="x509_san_dns:client.example.com" ...

The "internal" OAuth client ID would be x509_san_dns:client.example.com. This would also be the value that appears in the aud claim in the presentation.


For Federation, the request would be ... client_id="https://client.example.com" ... and the aud value in the presentation would be https://client.example.com.

@awoie
Copy link
Contributor

awoie commented Sep 12, 2024

On a call today, we identified a potential solution to this issue:

  • We use the prefixing approach that I proposed initially.
  • The OpenID Federation scheme is renamed to https

This would mean that an Federation entity ID can be used without any changes (effectively without a prefix).

For example, for x509_san_dns, the authorization request would look like this: ... client_id="x509_san_dns:client.example.com" ...

The "internal" OAuth client ID would be x509_san_dns:client.example.com. This would also be the value that appears in the aud claim in the presentation.

For Federation, the request would be ... client_id="https://client.example.com" ... and the aud value in the presentation would be https://client.example.com.

With this proposal, was the idea to drop the client_id_scheme parameter since you would have that info in the pre-fix?

@danielfett
Copy link
Contributor Author

Yes, client_id_scheme would be dropped.

@danielfett
Copy link
Contributor Author

Here is the latest proposal in the form of a PR: #263

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

Successfully merging a pull request may close this issue.