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

Add OAuth2 metadata url #3595

Closed
AxelNennker opened this issue Feb 20, 2024 · 13 comments
Closed

Add OAuth2 metadata url #3595

AxelNennker opened this issue Feb 20, 2024 · 13 comments
Labels
security: auth Authentication including overlap with authorization security
Milestone

Comments

@AxelNennker
Copy link
Contributor

OAuth2 servers have metadata too. RFC8414

This issue is about adding oauth2MetadataUrl to oauth2 allows the client to download the OAuth2 client to download the RFC8414 OAuth2 metadata. Please see #3594 for one variant of adding oauth2MetadataUrl to OAS.

The reading of the authorization server metadata enables clients to understand which features an authorization server supports.
E.g. which grant types are supported, which scopes are supported,
Some of which might improve over time. Reacting to the changed and hopefully improved metadata allows clients to e.g. improve their security if e.g. PKCE is newly supported.

See e.g. OAuth 2.0 Security Best Current Practice for features secure authorization servers should or must support.

oauth2 flows should be a subset of the grant_types_supported from the OAuth2 metadata.

@miqui
Copy link
Contributor

miqui commented Feb 20, 2024

Worth looking into. Perhaps this could be included in MoonWalk.

@AxelNennker
Copy link
Contributor Author

I suggest creating a PR to whatever branch you prefer.

Please consider putting the following next to the definition of openIdConnectUrl e.g. here 3.1.0.mc:

oauth2MetadataUrl | string | oauth2 | OPTIONAL. A HTTPS URL to discover OAuth2 configuration values. This must be in the form of a URL. See RFC8414

@lornajane
Copy link
Contributor

I'm in favour of adding support for the metadata URL in a security scheme. It looks as if the proposed oauth2MetadataUrl and existing openIdConnectUrl are the same thing, but used in different contexts, have I understood that correctly? But the point here is that it's a wider appeal than just openidconnect, all OAuth2 could (or maybe should!) be checking metadata to know how to proceed.

@AxelNennker
Copy link
Contributor Author

openid AZ/OP metadata was developed independently from oauth2 metadata.
OIDF developed the OP metadata in 2014 and refreshed that the spec in December 2023.
OAuth2 metadata was spec-ed in 2018, I think.

There is some overlap because OIDC is a "profile" of OAuth2.
The people writing the metadata specs are working together and there is convergence but the two specs are not the same.

@lornajane
Copy link
Contributor

Thanks for clarifying that, I think it's an important question that would probably come up in discussion.

The other question is where we can add it. I'm going to suggest adding it to the 3.1 branch. As an optional field, it's appropriate to add it in a patch release.

@AxelNennker
Copy link
Contributor Author

The other question is where we can add it. I'm going to suggest adding it to the 3.1 branch. As an optional field, it's appropriate to add it in a patch release.

To make sure, 3.1 means creating a PR to https://github.com/OAI/OpenAPI-Specification/blob/v3.1.1-dev/versions/3.1.1.md ?

From reading https://github.com/OAI/OpenAPI-Specification/blob/v3.1.1-dev/DEVELOPMENT.md#tracking-process I arrive at 3.0.x because I see this as a non-breaking change. Adding a new optional field does not break e.g. 3.0.3 API specs, I think.
Yes, adding a new field is not a typo or wording clarification. But typos etc are given as an example of non-breaking changes. So, my understanding was 3.0.x for a non-breaking change.

Anyway, if you say "change https://github.com/OAI/OpenAPI-Specification/blob/v3.1.1-dev/versions/3.1.1.md" then I'll do that

@handrews
Copy link
Member

handrews commented Mar 5, 2024

@lornajane

As an optional field, it's appropriate to add it in a patch release.

My understanding is that patch releases are only for bugfixes/clarifications (because implementations shouldn't have to care about the patch number), so new fields have to go in minor releases. Of course I might be wrong or we might want to change that policy, but I think right now it would have to go into 3.2.0.

@AxelNennker
Copy link
Contributor Author

AxelNennker commented Mar 5, 2024

What does "because implementations shouldn't have to care about the patch number" mean?

As an API designer using 3.0.x I do not have to care about a new optional field, because if I do not use the new field then nothing has changed. If I use the new field, then also there is no harm done. Implementers of my API who find the new field, take it as additional documentation that would otherwise be communicated out-of-band (where is the OAuth2 AZ metadata). Implementers can then take that documentation into account or not. Either way, no harm done.

Maybe create an issue for this discussion? And then a PR for https://github.com/OAI/OpenAPI-Specification/blob/main/DEVELOPMENT.md to write that down?

Anyway, I can create a PR for https://github.com/OAI/OpenAPI-Specification/blob/v3.2.0-dev/versions/3.2.0.md
Should I do that? Please advice.

@handrews
Copy link
Member

handrews commented Mar 5, 2024

@AxelNennker implementations == implementations of the OAS (tools) not the API. Implementations would need to add support for the optional field, so you couldn't use the exact same implementation for 3.0.4 as for 3.0.3 if 3.0.4 contained a new field, optional or otherwise.

There are already numerous issues filed around sorting all of this out and documenting it, including:

The limiting factor on these policy decisions is that we currently don't have a full-strength Technical Steering Committee. New members have been nominated and we are waiting to hear results, which have been delayed by difficulties in getting a quorum of existing members. My understanding is that a quorum has been achieved and I'm hoping we get a new lineup announced on Thursday.

Our biggest challenge as a project right now is getting the contributor guidance (blocked on the TSC) and infrastructure (mostly just needing someone to do them) issues resolved [you probably can't see those projects right now but hopefully someone will fix the permissions soon].

@AxelNennker
Copy link
Contributor Author

Ah, last time I mentioned tools somebody said that OpenAPI-Specification does not care too much about tools - I think, I remember that correctly.

The last time, half a year ago, I used java client codegen the field openIdConnectUrl did not show up anywhere in the generated client code. Not sure whether there was schema validation.
Anyway, a good implementers guide is to ignore unknown fields and generate code anyway.
I agree that schema validation should mark an unknown field as "unknown" or something.

Regarding, this issue #3595 : for which version should I write the PR. Or is this discussed in the next meeting and decided there?

@handrews
Copy link
Member

handrews commented Mar 5, 2024

@AxelNennker we close issues people open about tools here in the spec repo, and we make an effort to keep the spec independent of any particular tool. But we do care about tools.

Adding a field requires support from tools in order for them to be compliant, so it forces the choice between compliance and what you consider "good implementers guide" behavior (which I'm not disparaging, I'm just acknowledging that not everyone has the same notion of how implementations should handle such a choice, and that's not even getting into the topic of schema validation).

Given the confusion over what does and doesn't go in a patch release, I have no idea where to put a PR. We need to resolve the process question first. The Thursday meeting is always the best place to try to advance things.

@lornajane
Copy link
Contributor

We definitely want to avoid springing surprises on tooling vendors, so we've agreed that this is an excellent change and we'll accept it into the 3.2 branch.

@handrews
Copy link
Member

PR was merged a while back, closing!

@handrews handrews added this to the v3.2.0 milestone May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security: auth Authentication including overlap with authorization security
Projects
None yet
Development

No branches or pull requests

4 participants