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

make consistent the use of prefixes in the client_id scheme #401

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

bc-pi
Copy link
Member

@bc-pi bc-pi commented Jan 29, 2025

with a default allowance

to help with #376

@bc-pi bc-pi linked an issue Jan 29, 2025 that may be closed by this pull request
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.

please separate this PR into two PRs: one that includes what helps with #376 and editorial changes; and the other with what is a normative change we have not agreed in the WG (openid-federation and decentralzied-identifier). otherwise, will close.

Co-authored-by: Tim Cappalli <[email protected]>
@bc-pi
Copy link
Member Author

bc-pi commented Jan 30, 2025

please separate this PR into two PRs: one that includes what helps with #376 and editorial changes; and the other with what is a normative change we have not agreed in the WG (openid-federation and decentralzied-identifier). otherwise, will close.

I understand that you don't see this as important and are not happy to see it appear given the many competing priorities and constrained timelines from many external factors. But it is no less deserving of the WGs attention than many other items that have been brought to the front of the queue. I will respectfully suggest that "otherwise, will close" is an understandable but wholly inappropriate response. Please reconsider.

@bc-pi
Copy link
Member Author

bc-pi commented Feb 5, 2025

and even though it shouldn't work in this direction, whatever happens here in OpenID4VP will likely have ramifications in regular old OAuth. And it's not okay to push special treatment of DIDs and OpenID Federation onto that layer.

@aaronpk
Copy link

aaronpk commented Feb 5, 2025

Sorry for being so blunt, but if the DCP WG wants to do things the OpenID4VP way while ignoring feedback from the underlying OAuth perspective, that's fine with me, but then I expect that those decisions will not be brought back to the OAuth WG at the IETF. Making a decision here that is knowingly not compatible with work happening at the IETF is a decision the DCP WG will have to live with as things develop independently at the IETF.

If the changes in this PR are not made, then I expect to hear no complaints when this topic is brought back up at the IETF.

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.

This PR proposes breaking changes to the hard-won consensus position on Client ID prefixes developed in PR #263 . It would break existing client_id values using OpenID Federation and existing ones that are DIDs. That seems ill-advised.

aaronpk added a commit to aaronpk/oauth-client-id-scheme that referenced this pull request Feb 6, 2025
@aaronpk
Copy link

aaronpk commented Feb 6, 2025

I would like this to be a DCP working group decision that it's acceptable to diverge from what the OAuth WG may do with client ID schemes in the IETF. Can we get this decision onto the agenda for the next call?

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 decentralized-identifier:did: sounds very weird, and it's only DIDs that use did: anyway, unlike https: that is used not just by openid federation, so why did: is a preferential treatment? I support openid_federation: but am less sure about decentralized-identifier:

@Sakurann Sakurann self-requested a review February 7, 2025 10:12
@bc-pi
Copy link
Member Author

bc-pi commented Feb 7, 2025

I think decentralized-identifier:did: sounds very weird, and it's only DIDs that use did: anyway, unlike https: that is used not just by openid federation, so why did: is a preferential treatment? I support openid_federation: but am less sure about decentralized-dentifier:

The idea here is to have all client id schemes use a prefix. That's the consistency part. The text prior to this PR has a prefix for the client id schemes except for Federation and DIDs, which used the URI scheme as both the client id scheme and part of the client id value. This PR adjusts things so that all client id schemes consistently have a prefix part that is distinct from the value part. I chose decentralized_identifier: for Decentralized Identifiers (DIDs) v1.0 because I assumed someone would say did:did sounded weird or was confusing. It doesn't have to be decentralized_identifier: though. But it does need to be something. Would w3c_did: or w3c_decentralized_identifiers_v1_0:or justdid: (but that would then have double "did:did") or decentralized_id: or w3c_did_core_architecture_data_model_and_representations: or did_core: or did_core_2021: or something along those lines be less weird to your eye?

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.

One of the core principles of the OpenID Foundation is that we collaboratively support each other's work. This results in OpenID specifications being able to be seamlessly used together, both for specs in the same working group and specs across working groups.

This PR does not live up to that principle, since it would create a normative conflict between two OpenID specifications. OpenID Federation requires that the Client ID be the Entity Identifier (which begins with https:) when using Automatic Registration. This PR would require that it be a different value. This would hinder the use of the specifications together. In fact, it would create a contradiction within the OpenID4VP spec itself, because both the language in this PR and the language in the normatively referenced OpenID Federation specification would apply.

I therefore respectfully request that you withdraw this PR for those reasons.

@aaronpk
Copy link

aaronpk commented Feb 7, 2025

This change originally stemmed from a security issue with the possible ambiguous interpretation of the client identifier value when used in conjunction with other specifications.

There is of course precedent for making breaking changes to OpenID specs when security issues are found, as demonstrated by the current changes being made across a number of OpenID and OAuth specifications regarding private key JWT client authentication.

That said, the security issue in question is only relevant if an AS intends on supporting multiple different client metadata resolution methods. I would actually prefer plain HTTPS URLs to be valid client IDs, since many deployments will only ever have one method of resolving metadata from a URL, such as the one described in the draft Client ID Metadata Document that I presented at the last IETF meeting in November.

So if there is going to be an exception to the client ID prefix, I would prefer that exception be a blanket exception for https URLs, not explicitly for OpenID Federation, which is just one metadata resolution method.

It is particularly not collaborative or cooperative to have an OpenID Federation exception backported into an IETF draft, while blocking non-OpenID uses of the mechanism.

@bc-pi
Copy link
Member Author

bc-pi commented Feb 7, 2025

One of the core principles of the OpenID Foundation is that we collaboratively support each other's work.

That's a lovely sounding platitude on the surface of things but is actually absurd and untenable.

@selfissued
Copy link
Member

@aaronpk, the security vulnerability was closed by PR #263 unambiguously defining that Client IDs beginning with https: are OpenID Federation Entity Identifiers. Relaxing that to leave the interpretation up to implementations would re-open the security vulnerability.

It's not a special case. It's a clear definition to remove ambiguity.

@aaronpk
Copy link

aaronpk commented Feb 7, 2025

That's pretty effectively demonstrating the exact problem I mentioned before.

Defining https schemes as OpenID Federation identifiers is an OpenID-centric view of the world. Doing that is fine in an OpenID context of course. But someone not using OpenID Federation should not be prevented from also using the https scheme for client IDs. So when a draft is brought to the IETF that establishes https scheme client IDs, the response cannot be that this scheme is already defined by OpenID Federation so can't be used elsewhere.

@selfissued
Copy link
Member

For context, what IETF draft are you referring to, Aaron? OpenID4VP is an OpenID draft.

@aaronpk
Copy link

aaronpk commented Feb 7, 2025

I presented Client ID Metadata Document at IETF 121, and there were comments during the session about the potential conflict with the OpenID Federation use of https URLs. That led to a discussion between myself, Dr Fett, and Joseph resulting in presenting OAuth 2.0 Client ID Scheme at an interim meeting in January. In both cases, I mentioned that there are a number of deployments of systems that currently use https URLs as client IDs that do not use (and likely never will use) OpenID Federation.

@selfissued
Copy link
Member

Thanks. I wasn't aware of the OAuth 2.0 Client ID Scheme doc. I'll have a look at it.

That said, this issue isn't about the proposed IETF specs. It's about whether to introduce inconsistencies between two OpenID specs that are used together, and indeed, whether to thereby introduce a contradiction within the OpenID4VP spec itself.

@aaronpk
Copy link

aaronpk commented Feb 7, 2025

Right, which is why I said I'm fine with either outcome for OpenID4VP. The problem is that whatever gets decided here will ultimately trickle back up to OAuth IETF specs. That's why I asked for a DCP working group decision that it is acceptable to make a decision here that will not be brought back to IETF.

@selfissued
Copy link
Member

Ok, that makes sense. Of course, both officially and practically, we all participate as individuals...

@selfissued
Copy link
Member

@bc-pi , in response to my comment:

One of the core principles of the OpenID Foundation is that we collaboratively support each other's work.

you wrote:

That's a lovely sounding platitude on the surface of things but is actually absurd and untenable.

At a personal level, I sure hope we can all commit to working collaboratively and supporting each other's work, because our lives (and specs) will all be better for it. This isn't abstract to me, nor a platitude. I've experienced the non-collaborative alternative in practice and so have you Brian - it's the way the VCWG works. I care too much about the OpenID Foundation and our work together to see it head in that direction. For what it's worth...

@peppelinux
Copy link
Member

peppelinux commented Feb 8, 2025

It makes no sense to define decentralized_identifier in addition to did. In this I agree with Kristina. did alone is quite telling.

I appreciate https and not because this necessarily has to correspond exclusively to openid federation. I like being able to implement an entity identification mechanism that involves an https URL, indipendently of the discovery mechanisms supported. I am aware that via https there may be different ways of making a discovery, considering different trust frameworks. I am and I imagine everyone is aware that this client scheme is about the client's unique identifier and not about the trust framework that involved several other aspects about discovery and trust evaluation.

with the proposal consolidated to date in the openid4vp draft I still see interesting, creative degrees of freedom. For example, an x509_san_uri can support hybrid discovery mechanisms, combining certificates and metadata resolution on well-known endpoints (and this does not actually exclude openid federation: please don't get excited to reduce these freedoms).

I would like mechanisms that scale with different trust frameworks and that allow a single implementation work in different contexts without requiring any particular modifications or specializations.

I regret destroying any expected technical arbitrariness aimed at simplifying our world, unfortunately or fortunately, the future is hybrid, the technical rules must not aim to solve the problems of trust frameworks but accommodate their implementations.

I like how Brian writes and I like some of this PR. I do not support the specialization of client schemes proposed in this PR, for what my opinion is worth I ask for its removal. I would keep only the editorials in this PR, reverting all about client id schemes parameters.

Copy link
Member

@peppelinux peppelinux left a comment

Choose a reason for hiding this comment

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

as per my previous comment

@aaronpk
Copy link

aaronpk commented Feb 10, 2025

@peppelinux you're completely missing the original issue that led to this proposal, described in #376.

Currently, the client ID values are mixed between being prefixed with a value that is not in the client ID value.

            "redirect_uri:https://client.example.org/cb",
            "https://federation-verifier.example.com",
            "did:example:123#1",
            "verifier_attestation:verifier.example",
            "x509_san_dns:client.example.org",
            "x509_san_uri:https://client.example.org/cb",
            "web-origin:https://verifier.example.com"

Giving these all to a URI parser results in errors because underscores are not allowed in URI schemes. The point brought up in prior discussions is that these aren't actually URIs, rather they are their own construct of prefix:value. You would have to write a bespoke parser for these, which is done easily by looking for the first colon and treating everything before the colon as the client ID scheme and everything after as the value. That results in this table:

image

Now you can see the two outliers here are did and https, where the value is missing some characters. So these then have to be special-cased to put back together the correct client ID value.

This PR makes these consistent with the others by adding a prefix to everything:

image

@peppelinux
Copy link
Member

@aaronpk thank you for having put me in the right direction.

I would not add decentralized_identifier to did because did means decentralized identifier and ity represents a repetition. The did URIs seem to work properly without a bespoke parser.

I would not add x509_dns_uri or openid_federation, because https alone is quite telling. Hinting the use of federation or X.509 can be achieved using trust_chain or x5c within the JWS headers. I would say the same about web-origins

but I don't want to bother you, in general I'm a little worried about the over-specification of every detail.

This way of doing things leaves no room for an implementation to use a client_id with more than one trust framework or trust evaluation mechanism. In general it doesn't scale... But this is an old discussion.

To carry out this PR in a positive way, it is only necessary to resolve the URIs that cause exceptions in the parsers without touching those that do not cause exceptions, also in consideration of the current implementations and the desire not to introduce breaking changes where not strictly necessary

@tplooker
Copy link
Contributor

Im in favour of this change from @bc-pi. I raised concerns around the in-consistent application of prefixes to client IDs when the original proposal was made and I think both @bc-pi and @aaronpk have further distilled arguments for why it would be a bad idea to let that continue.

@lj-raidiam
Copy link

So if there is going to be an exception to the client ID prefix, I would prefer that exception be a blanket exception for https URLs, not explicitly for OpenID Federation, which is just one metadata resolution method.

Prefixing dids with yet another did-like prefix just to stick to the convention is not worth it and looks weird.

Now you can see the two outliers here are did and https, where the value is missing some characters. So these then have to be special-cased to put back together the correct client ID value.

Dids and https are different than other schemes on that list. One more if prior to parsing clientid doesn't seem like a huge lift.

So if there is going to be an exception to the client ID prefix, I would prefer that exception be a blanket exception for https URLs, not explicitly for OpenID Federation, which is just one metadata resolution method.

Potential conflict with the Client ID Metadata draft is an issue, however the bigger issue with this PR is that when merged it would quite severely impact other OpenID specification that is in a quite advanced state. Why would we even do that?

Perhaps adding an optional metadata type parameter only for https identifiers, which when present would convey information about use of a scheme other than the default one (openid federation) could be a solution?

Copy link

@lj-raidiam lj-raidiam left a comment

Choose a reason for hiding this comment

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

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 Identifier Schemes violate RFC 3986
8 participants