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

Change client_id_scheme to a prefix #263

Merged
merged 18 commits into from
Oct 3, 2024
Merged

Conversation

danielfett
Copy link
Contributor

@danielfett danielfett commented Sep 13, 2024

Solves #124 by removing client_id_scheme as a separate parameter and making it a prefix instead. entity_id was renamed to https in order to avoid breaking OpenID Federation.

Fixes #124
Fixes #29

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.

I think this is the cleanest approach that solves main security problem while not breaking other specifications like OpenID Federation and DIDs, while following OAuth 2.0 principle of client_id/aud uniquely identifying the client throughout the transaction

openid-4-verifiable-presentations-1_0.md Outdated Show resolved Hide resolved
openid-4-verifiable-presentations-1_0.md Outdated Show resolved Hide resolved
@jogu
Copy link
Collaborator

jogu commented Sep 13, 2024

Thanks Daniel!

One final thought: I am dubious about adding the prefix to pre-registered client ids. I don't think it actually really matters for VP, but I think it creates a clear gap as there's no way it could be accepted in normal OAuth, and if might be better if we chose something that could potentially eventually make it into an OAuth standard. Would removing the prefix for pre-registered clients and saying this work:

pre-registered client ids MUST NOT contain :

@@ -457,9 +471,9 @@ Location: https://client.example.org/universal-link?
8%22%5D%7D%7D%7D
```

* `entity_id`: This value indicates that the Client Identifier is an Entity Identifier defined in OpenID Federation [@!OpenID.Federation]. Processing rules given in [@!OpenID.Federation] MUST be followed. Automatic Registration as defined in [@!OpenID.Federation] MUST be used. The Authorization Request MAY also contain a `trust_chain` parameter. The final Verifier metadata is obtained from the Trust Chain after applying the policies, according to [@!OpenID.Federation]. The `client_metadata` parameter, if present in the Authorization Request, MUST be ignored when this Client Identifier scheme is used.
* `https`: This value indicates that the Client Identifier is an Entity Identifier defined in OpenID Federation [@!OpenID.Federation]. Since the Entity Identifier already is defined to start with `https:`, the `https` does not need to be prefixed additionally. Processing rules given in [@!OpenID.Federation] MUST be followed. Automatic Registration as defined in [@!OpenID.Federation] MUST be used. The Authorization Request MAY also contain a `trust_chain` parameter. The final Verifier metadata is obtained from the Trust Chain after applying the policies, according to [@!OpenID.Federation]. The `client_metadata` parameter, if present in the Authorization Request, MUST be ignored when this Client Identifier scheme is used. Example Client Identifier: `https://federation-verifier.example.com`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `https`: This value indicates that the Client Identifier is an Entity Identifier defined in OpenID Federation [@!OpenID.Federation]. Since the Entity Identifier already is defined to start with `https:`, the `https` does not need to be prefixed additionally. Processing rules given in [@!OpenID.Federation] MUST be followed. Automatic Registration as defined in [@!OpenID.Federation] MUST be used. The Authorization Request MAY also contain a `trust_chain` parameter. The final Verifier metadata is obtained from the Trust Chain after applying the policies, according to [@!OpenID.Federation]. The `client_metadata` parameter, if present in the Authorization Request, MUST be ignored when this Client Identifier scheme is used. Example Client Identifier: `https://federation-verifier.example.com`.
* `https`: This value indicates that the Client Identifier is an Entity Identifier defined in OpenID Federation [@!OpenID.Federation]. Since the Entity Identifier already is defined to start with `https:`, the `https` is not prefixed additionally. Processing rules given in [@!OpenID.Federation] MUST be followed. Automatic Registration as defined in [@!OpenID.Federation] MUST be used. The Authorization Request MAY also contain a `trust_chain` parameter. The final Verifier metadata is obtained from the Trust Chain after applying the policies, according to [@!OpenID.Federation]. The `client_metadata` parameter, if present in the Authorization Request, MUST be ignored when this Client Identifier scheme is used. Example Client Identifier: `https://federation-verifier.example.com`.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to prescribe OpenID Federation as the only way metadata could be resolved from the client ID when it is an HTTPS url. There are multiple well-known metadata locations proposed for client metadata and a similar precedent exists within OAuth/OpenID for IDP / Authorisation Server metadata.

Copy link
Collaborator

@jogu jogu Sep 16, 2024

Choose a reason for hiding this comment

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

The intention is that federation is the only thing that uses the https client id scheme, and this doesn't stop a well-known based client id scheme being created, it would just use a client_id like wellknown:https://rp.example.com/.

If you think the text isn't conveying this could you suggest alternative wording please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `https`: This value indicates that the Client Identifier is an Entity Identifier defined in OpenID Federation [@!OpenID.Federation]. Since the Entity Identifier already is defined to start with `https:`, the `https` does not need to be prefixed additionally. Processing rules given in [@!OpenID.Federation] MUST be followed. Automatic Registration as defined in [@!OpenID.Federation] MUST be used. The Authorization Request MAY also contain a `trust_chain` parameter. The final Verifier metadata is obtained from the Trust Chain after applying the policies, according to [@!OpenID.Federation]. The `client_metadata` parameter, if present in the Authorization Request, MUST be ignored when this Client Identifier scheme is used. Example Client Identifier: `https://federation-verifier.example.com`.
* `https`: This value indicates that the Clients metadata is available at a well-known location such as defined in [@!OpenID.Federation]. Processing rules for retrieving and validating the metadata for the well-known location being used, such as that given in [@!OpenID.Federation] MUST be followed. The `client_metadata` parameter, if present in the Authorization Request, MUST be ignored when this Client Identifier scheme is used. Example Client Identifier: `https://federation-verifier.example.com`.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That doesn't reflect the intention though. If people want to use https urls with a different resolution method, they use a client id scheme other than https, e.g. wellknown:https://rp.example.com/. The "https" client-id scheme is reserved for federation only.

Avoiding the vagueness of "I'm going to somehow authenticate with you using this url, you go figure out how you think I'm trying to authenticate with you" is very much the point. If you think of the browser API, using the https client id scheme to reflect multiple ways the wallet can authenticate, can result in lots of deadends as the sandboxed wallet matcher is now unable to know if it supports the method the wallet is using to authenticate, because it literally doesn't know how the wallet is trying to authenticate and has no way to find out because it can't access the network.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@selfissued I'll change this to a "MUST NOT be prefixed" to be very clear.

@tplooker @jogu I agree with Joseph here. This is an opportunity to have a clearly defined mechanism both from a security point of view and from an implementation perspective. Unless there is a good reason to not prefix other https-based client id schemes, we should avoid it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Federation is an overly complicated mechanism for a client to simply convey its metadata at a well-known location, I don't believe we should be promoting it as the preferred mechanism for clients that are choosing to identify via an HTTPS url which is what this proposal does. It relegates other HTTPS based url based client metadata discovery mechanism to requiring a double prefix style client id which I don't think is a clean solution.

If we would prefer to have less ambiguity then the text I suggested, I propose we have a seperate parameter to convey the metadata location for the client such as client_metadata_location=federation.

Choose a reason for hiding this comment

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

@tplooker Is it going to be any different than client_id_scheme for https-based urls?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to be clear, we are not promoting federation as a preferred mechanism. We are merely acknowledging that federation is already the long established way of client's using self-allocated client_ids beginning with https: and that there's not really any option open to us of changing that position.

The alternative (following in the vein of this PR) is that federation is also prefixed, but that introduces interoperability issues / confusion and still doesn't solve the 'clean solution' problem.

I think if we do want to do something that is a generic scheme based on https urls (and I think that could be a good idea), we need to do that with a prefix, otherwise we're going to overlap and/or conflict with at least two other different specifications. It's simply not sustainable or sensible to have multiple specs trying to define self-assert client ids that are pure https urls. Even federation probably shouldn't be doing it, but that ship has mostly sailed.

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.

Approved with requests for clarifying changes.

@jogu jogu added this to the Final 1.0 milestone Sep 13, 2024
Copy link
Member

@c2bo c2bo left a comment

Choose a reason for hiding this comment

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

There are quite a few files in the diagrams folder that use client_id_scheme & client_id - should those be changed as well?

@Sakurann Sakurann requested review from jogu and c2bo September 19, 2024 13:13
Copy link
Member

@c2bo c2bo left a comment

Choose a reason for hiding this comment

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

I do believe that this is the best/cleanest path forward to guarantee security / prevent downgrading attacks.

That being said, I do understand the concerns that have been brought up, but do believe we should be dealing with that and especially the optimization for re-using the same key for different trust mechanisms in a different issue/PR.

@danielfett
Copy link
Contributor Author

FYI, I restructured the section on client id schemes to tighten the exp... improve readability.

Copy link
Member

@bc-pi bc-pi left a comment

Choose a reason for hiding this comment

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

I am philosophically opposed to special treatment of DIDs and Federation.

If we're gonna prefix, prefix all the new stuff equally. And ":" needn't be the prefix indicator.

Also it causes breakage in the Appendix A. OpenID4VP profile for the W3C Digital Credentials API #263 (comment)

@Sakurann
Copy link
Collaborator

Sakurann commented Sep 24, 2024

discussed on the WG call. need to clarify the behavior of client id in an unsigned request in the browser API since that is where client id is an https: just like in openid federation. please make suggestions.

there were multiple opinions that DIDs are not a special treatment. did: is defined as an URI scheme.

@Sakurann
Copy link
Collaborator

I think this PR also resolves #29?

@jogu
Copy link
Collaborator

jogu commented Sep 26, 2024

I think this PR also resolves #29?

I don't think so. I'll comment on the issue.

@danielfett
Copy link
Contributor Author

It indeed resolves #29 as it changes the title of the section that @jogu identified as confusing.

@Sakurann Sakurann requested a review from bc-pi October 1, 2024 18:25
Copy link
Member

@bc-pi bc-pi left a comment

Choose a reason for hiding this comment

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

I still object to special treatment of OpenID Federation and W3C Decentralized identifiers but am approving in the interest of making progress.

@Sakurann
Copy link
Collaborator

Sakurann commented Oct 3, 2024

I note that I relate to Brian's uncomfortableness to OpenID Federation's "special" treatment, while recognizing that changing openid federation document to enable openidfederation:https://... is, unfortunately, probably much much harder...

@jogu
Copy link
Collaborator

jogu commented Oct 3, 2024

This is quite a significant change, and it's taken well over 6 months to get to this point - but we now have 8 approvals on it and no request for changes. We've discussed this particular approach I believe on all the working group meetings for at least 2 weeks.

Not everyone is happy but, other than/despite the above debate over whether federation should have the "https" prefix or not (and similar for 'did'), I believe we've reached a solid consensus that prefixing is a workable approach. We did ask Oliver for feedback too, but he's not added anything more, and I also sent a message on the mailing list on 23rd September asking for feedback by 1st Oct, and (other than comments on this PR) we've not received anything further - so as discussed on today's WG call - merging!

A big thank you to everyone that contributed here - even if the WG didn't go with your suggestions I genuinely believe that all the different ideas we've had helped guide the working group towards what we ended up with!

@jogu jogu merged commit ee26876 into main Oct 3, 2024
2 checks passed
@jogu jogu deleted the danielfett/client-id-scheme-prefixed branch October 3, 2024 17:15
@bc-pi
Copy link
Member

bc-pi commented Oct 3, 2024

changing openid federation document to enable openidfederation:https://... is, unfortunately, probably much much harder...

Is it though? That openid federation document has been under development for more than eight years now and is on its 39th draft. And apparently a federation wallet architecture specification is needed for it to federation to work with wallets anyway. Changes aren't possible there? While introducing a breaking change to everything else (except for DIDs of course)...

I honestly don't see any justification for special treatment.

But I've approved anyway #263 (review)

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.

client_id_scheme security considerations Unclear what happens if client_id_scheme appears twice