-
Notifications
You must be signed in to change notification settings - Fork 21
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
Comments
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 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 |
Each credential format has to define how to use client_id and client_id_scheme in the response. W3C VCDM with Data Integrity:
SD-JWT VC:
W3C VCDM with JWT:
ISO mdoc:
|
IMO, if |
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. |
@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. |
in-person WG:
|
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. |
There is a similar ambiguity between entity_id and x509_san_uri
That would result in client authentication failing as far as I can see.
The root cause of the problem is the introduction of |
@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. |
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? |
Just a note related to @Sakurann's comment above, I will provide another proposal related to this issue soon which could help simplify things. |
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
One possible proposal here would be to do the following
How would this look, a standard request would look like HTTP/1.1 302 Found 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
Payload
When the client wants to use the Header
Payload
|
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. |
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. |
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? |
@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.) |
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. |
You would have to explain to me how this could possibly happen. |
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. |
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). |
|
I've been thinking about this for a while. It seems to me that introducing Some have advocated in this thread that if 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 |
Isn't that how all security works? and patches are introduced when new facts and bugs come to light |
x509_san_dns - indicates the trust scheme |
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. |
My observation is that Or do we have other examples in the specs, where two parameters must always be used together and they are not merged? |
WG discussion: there seems to be an agreement on the problem. but there still seems to be a disagreement on the solution:
|
Based on the discussion on the call today, I continue to believe that we should remove |
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?
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. |
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. |
My proposal (if we agree to namespace) is to add the client id scheme as a URI scheme in front of the client id:
|
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 |
The fact that this is acknowledged as breaking things reinforces my conclusion that |
@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. |
The proposal is to remove |
Remove client_id_scheme to solve #124RationaleOriginally, the concept of a client ID scheme was introduced to govern the following:
However, the primary function of
Additional verifier metadata was a secondary use case since there are not many relevant ones pertaining to OpenID4VP. The problem that
Attempting to solve #124 by prefixing 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 ObservationGiven 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. ProposalI propose the following:
Regarding 2), I propose the following:
ConclusionRemoving 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 The proposal also allows the decoupling the verification of the linkage of public verification keys for request objects and the |
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. |
I like the direction @awoie provided. The rationale is sound and the proposed solution is simplified and provides flexibility. One remark while reading.
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. |
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. |
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. |
Proposal dropping |
On a call today, we identified a potential solution to this issue:
This would mean that an Federation entity ID can be used without any changes (effectively without a prefix). For example, for The "internal" OAuth client ID would be For Federation, the request would be |
With this proposal, was the idea to drop the |
Yes, |
Here is the latest proposal in the form of a PR: #263 |
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 theclient_id
, it should appear everywhere whereclient_id
appears, including inaud
values and in thesub
value of the Verifier Attestation JWT.In the credentials, the
client_id_scheme
should be included besides theclient_id
as the audience; e.g., in an SD-JWT KB-JWT, there should be a separate claim besidesaud
.In Section 12.1, the following sentences need to be adapted as well:
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.
The text was updated successfully, but these errors were encountered: