oauthenticator icon indicating copy to clipboard operation
oauthenticator copied to clipboard

[Generic] client_id / client_secret not passed in POST request to get an access token

Open chucklay opened this issue 6 years ago • 5 comments

In generic.py, the POST made to exchange the auth code for an auth token does not include the client_id and client_secret by default: https://github.com/jupyterhub/oauthenticator/blob/master/oauthenticator/generic.py#L89-L93

client_id and client_secret are required by oauth spec when making this exchange: https://developer.okta.com/blog/2018/04/10/oauth-authorization-code-grant-type#exchange-the-authorization-code-for-an-access-token

It seems like they could be passed in via self.extra_params, but so far I can't find where those extra_params should be set.

chucklay avatar Jun 13 '19 17:06 chucklay

Is there a legitimate reason not to include the client_id and client_secret values? The quick fix is to simply include them in the POST parameters. Examples where this is done in the other service specific POST params:

Github: https://github.com/jupyterhub/oauthenticator/blob/master/oauthenticator/github.py#L102-L103

Gitlab: https://github.com/jupyterhub/oauthenticator/blob/master/oauthenticator/gitlab.py#L100-L101

Bitbucket: https://github.com/jupyterhub/oauthenticator/blob/master/oauthenticator/bitbucket.py#L64-L65

Azure: https://github.com/jupyterhub/oauthenticator/blob/master/oauthenticator/azuread.py#L73-L74

inviscid avatar Jun 13 '19 18:06 inviscid

Is this still an issue for you?

manics avatar Oct 13 '20 19:10 manics

Reading in https://tools.ietf.org/html/rfc6749#section-3.2 I conclude that the specification say that we MAY send client_id, but it doesn't mention sending along client_secret, so it seem like a practice not part of the OAuth2 standard when interacting with the Access Token endpoint.

Apparently though, some Identity providers require it, like Okta, GitHub, .

I think it makes sense at least to pass along client_id because that is not a problem security wise to include I think, but I think since its not part of the OAuth2 standard I dislike sending it along to the Access Token endpoint alongside the code which should be enough. I wonder if we really need to send along the client_secret to GitHub's OAauth2 along with the provided code...

Anyhow, this is how you would configure it I think.

c.GenericOAuthenticator.client_secret = "my secret"
c.GenericOAuthenticator.extra_params = {"client_secret": c.GenericOAuthenticator.client_secret}

Reading again in the RFC specs under the Token Endpoint in section 3.2 I find...

Confidential clients or other clients issued client credentials MUST authenticate with the authorization server as described in Section 2.3 when making requests to the token endpoint.

In section 2.3 I find... Hmm indications that it is required? I'm confused...

If this is concluded to be required according to the RFC for OAuth2, then i'd say let's go for it and consider it a bug that we don't have it already, otherwise I'm not 100% on what to do.

Action point

Help with interpretation, is it correct that https://tools.ietf.org/html/rfc6749#section-3.2 sais that we are required to send client_id and client_secret when not using http basic authentication?

consideRatio avatar Oct 25 '20 23:10 consideRatio

Sorry for the late response

That quote from section 3.2 makes note of the fact that clients must authenticate with the authorization server in this part of the handshake. However, the RFC leaves the method of authentication up to the authorization server to implement. In practice, as long as a client is able to keep a client secret confidential from the end user, this authentication is almost always done in one of two ways: either HTTP basic auth (an Authorization header), or by simply including the client secret in the request body. Theoretically you could use any client validation method out there, but I'd be extremely surprised to see anything other than these two.

Looking through generic.py, it seems like it uses HTTP basic auth exclusively. However, since a lot of authorization servers use the in-body method, it would be nice if that was supported as well.

Also, while it's probably not relevant since Jupyter is server-based, it's probably worth mentioning the PKCE extensions to the Authorization Code flow, designed to replace the now-deprecated implicit flow: RFC Plain-English explanation

chucklay avatar Oct 26 '20 13:10 chucklay

https://tools.ietf.org/html/rfc6749#section-2.3.1

Clients in possession of a client password MAY use the HTTP Basic authentication scheme as defined in [RFC2617] to authenticate with the authorization server. The client identifier is encoded using the "application/x-www-form-urlencoded" encoding algorithm per Appendix B, and the encoded value is used as the username; the client password is encoded using the same algorithm and used as the password. The authorization server MUST support the HTTP Basic authentication scheme for authenticating clients that were issued a client password.

For example (with extra line breaks for display purposes only):

Authorization: Basic czZCaGRSa3F0Mzo3RmpmcDBaQnIxS3REUmJuZlZkbUl3

Alternatively, the authorization server MAY support including the client credentials in the request-body using the following parameters:

client_id REQUIRED. The client identifier issued to the client during the registration process described by Section 2.2.

client_secret REQUIRED. The client secret. The client MAY omit the parameter if the client secret is an empty string.

Including the client credentials in the request-body using the two parameters is NOT RECOMMENDED and SHOULD be limited to clients unable to directly utilize the HTTP Basic authentication scheme (or other password-based HTTP authentication schemes). The parameters can only be transmitted in the request-body and MUST NOT be included in the request URI.

I think this means if the client includes client credentials in the body then it must include both. Therefore I think as long as this is explicitly requested by the user it's OK to add.

manics avatar Nov 21 '20 16:11 manics

Hey everyone! Thanks a lot for the useful discussions in this thread.

I will close this issue now because since https://github.com/jupyterhub/oauthenticator/pull/526 got merged, the opposite is actually happening now (sending the client id and secret even when basic auth is used) and I opened https://github.com/jupyterhub/oauthenticator/issues/565 to track it.

Thank you!

GeorgianaElena avatar Jan 17 '23 12:01 GeorgianaElena