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

closes #2311 - wellknown is missing pkce related info #2547

Merged
merged 1 commit into from
Jun 3, 2021

Conversation

gbolo
Copy link
Contributor

@gbolo gbolo commented May 27, 2021

Related issue

#2311 - wellknown is missing pkce related info

Proposed changes

adds additional parameters to wellknown response as defined in RFC 8414

Checklist

  • I have read the contributing guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

@CLAassistant
Copy link

CLAassistant commented May 27, 2021

CLA assistant check
All committers have signed the CLA.

@aeneasr
Copy link
Member

aeneasr commented May 28, 2021

Thank you! The idea here being that we follow https://datatracker.ietf.org/doc/html/rfc8414 ?

@gbolo
Copy link
Contributor Author

gbolo commented May 28, 2021

Thank you! The idea here being that we follow https://datatracker.ietf.org/doc/html/rfc8414 ?

Hi @aeneasr. Just pointing out section-2 of the above RFC that describes the required parameter to inform the oidc client that this authorization server supports PKCE. I'm assuming that PKCE (as described in RFC7636) is already implemented. If this is true then we must advertise the "PKCE Code Challenge Methods".

   code_challenge_methods_supported
      OPTIONAL.  JSON array containing a list of Proof Key for Code
      Exchange (PKCE) [RFC7636] code challenge methods supported by this
      authorization server.  Code challenge method values are used in
      the "code_challenge_method" parameter defined in Section 4.3 of
      [RFC7636].  The valid code challenge method values are those
      registered in the IANA "PKCE Code Challenge Methods" registry
      [IANA.OAuth.Parameters].  If omitted, the authorization server
      does not support PKCE.

@aeneasr
Copy link
Member

aeneasr commented May 28, 2021

I see, thank you! As far as I can tell, it appears that the linked RFC would assume providing this information at a different endpoint (not /.well-known/openid-connect but instead /.well-known/oauth-authorization-server).

So I think it would make sense to open a new endpoint to follow this RFC and then add the values there?

@gbolo
Copy link
Contributor Author

gbolo commented May 28, 2021

I see, thank you! As far as I can tell, it appears that the linked RFC would assume providing this information at a different endpoint (not /.well-known/openid-connect but instead /.well-known/oauth-authorization-server).

So I think it would make sense to open a new endpoint to follow this RFC and then add the values there?

I believe that we can use the existing URL as many other providers do it (such as google: https://accounts.google.com/.well-known/openid-configuration)

@aeneasr
Copy link
Member

aeneasr commented May 28, 2021

I checked the RFC and it looks to me as they strictly require the document to be discoverable at /.well-known/oauth-authorization-server. See also Section 3 of RFC 8414.

However, as you said, this also appears to be exposed at openid-configuration. Do you know any client libraries which would respect this field at openid-configuration?

@gbolo
Copy link
Contributor Author

gbolo commented May 28, 2021

I checked the RFC and it looks to me as they strictly require the document to be discoverable at /.well-known/oauth-authorization-server. See also Section 3 of RFC 8414.

However, as you said, this also appears to be exposed at openid-configuration. Do you know any client libraries which would respect this field at openid-configuration?

Hi @aeneasr, I haven't used any OIDC sdks. However, I have written my own for educational purposes ONLY (https://github.com/gbolo/muggle-oidc/blob/main/oidc/discovery.go#L21) which respects this parameter at openid-configuration

I quickly looked around for any popular openid sdks and i found this one for js which seems to do something similar: https://github.com/panva/node-openid-client

If you would prefer to make a separate endpoint, I can try to update the PR to accomplish that

@aeneasr
Copy link
Member

aeneasr commented May 28, 2021

As it has come up before for other endpoints (e.g. revokation) I think it would make sense to implement RFC 8414 according to spec so we can address the other things as well then :) I think that would be the most straight forward way & spec compliant. If there is indeed a popular library expecting this at openid-configuration we could still add it there (as i. this PR:) )

@gbolo
Copy link
Contributor Author

gbolo commented May 28, 2021

As it has come up before for other endpoints (e.g. revokation) I think it would make sense to implement RFC 8414 according to spec so we can address the other things as well then :) I think that would be the most straight forward way & spec compliant. If there is indeed a popular library expecting this at openid-configuration we could still add it there (as i. this PR:) )

hey @aeneasr after reading the section 3 you pointed out, it seems like we are fine keeping it the same:

   Some OAuth applications will choose to use the well-known URI suffix
   "openid-configuration".  As described in Section 5, despite the
   identifier "/.well-known/openid-configuration", appearing to be
   OpenID specific, its usage in this specification is actually
   referring to a general OAuth 2.0 feature that is not specific to
   OpenID Connect.

@aeneasr
Copy link
Member

aeneasr commented Jun 3, 2021

I finally found the relevant line in the spec (RFC 8414):

Additional authorization server metadata parameters MAY also be used.
Some are defined by other specifications, such as OpenID Connect
Discovery 1.0 [OpenID.Discovery].

And OpenID Provider Metadata says

Additional OpenID Provider Metadata parameters MAY also be used. Some are defined by other specifications, such as OpenID Connect Session Management 1.0 [OpenID.Session].

So adding RFC 8414 to OpenID PM should be allowed :) Thus not requiring a new endpoint!

@aeneasr aeneasr merged commit 9693168 into ory:master Jun 3, 2021
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.

None yet

3 participants