search

openid / OpenID4VP

57 stars 20 forks source link

Change `client_id_scheme` to a prefix #263

Closed danielfett closed 1 month ago

danielfett commented 2 months ago

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

jogu commented 2 months ago

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 :

danielfett commented 2 months ago

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

Sakurann commented 2 months ago

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 commented 2 months ago

I think this PR also resolves https://github.com/openid/OpenID4VP/issues/29?

jogu commented 2 months ago

I think this PR also resolves #29?

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

danielfett commented 2 months ago

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

Sakurann commented 1 month ago

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 commented 1 month ago

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!

bc-pi commented 1 month ago

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 https://github.com/openid/OpenID4VP/pull/263#pullrequestreview-2341135999