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.

Sign up for GitHub

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

Jump to bottom

[All] Add enable_pkce config, True by default #765

Merged
merged 5 commits into from
Oct 1, 2024
Merged

[All] Add enable_pkce config, True by default #765

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
01d153e
Add PKCE
renan-r-santos Sep 28, 2024
d9f96d2
Address PR comments
renan-r-santos Sep 28, 2024
f65acce
Rename pkce config to require_pkce
renan-r-santos Sep 29, 2024
2ee67ae
fix docs
renan-r-santos Sep 29, 2024
73c23d1
Rename config from require_pkce to enable_pkce
renan-r-santos Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 41 additions & 1 deletion oauthenticator/oauth2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
"""

import base64
import hashlib
import json
import os
import secrets
import uuid
from functools import reduce
from inspect import isawaitable
Expand Down Expand Up @@ -115,7 +117,22 @@ def get(self):
state_id = self._generate_state_id()
next_url = self._get_next_url()

cookie_state = _serialize_state({"state_id": state_id, "next_url": next_url})
state = {"state_id": state_id, "next_url": next_url}

if self.authenticator.require_pkce:
# https://datatracker.ietf.org/doc/html/rfc7636#section-4
code_verifier = secrets.token_urlsafe(43)
renan-r-santos marked this conversation as resolved.
Show resolved Hide resolved
code_challenge = hashlib.sha256(code_verifier.encode("utf-8")).digest()
code_challenge_base64 = (
base64.urlsafe_b64encode(code_challenge).decode("utf-8").rstrip("=")
)

token_params["code_challenge"] = code_challenge_base64
token_params["code_challenge_method"] = "S256"

state["code_verifier"] = code_verifier

cookie_state = _serialize_state(state)
self.set_state_cookie(cookie_state)

authorize_state = _serialize_state({"state_id": state_id})
Expand Down Expand Up @@ -663,6 +680,17 @@ def _allowed_scopes_validation(self, proposal):
""",
)

require_pkce = Bool(
False,
config=True,
help="""
Require Proof Key for Code Exchange (PKCE) for OAuth2 authorization code flow
consideRatio marked this conversation as resolved.
Show resolved Hide resolved

Only the S256 code challenge method is supported.
`RFC 7636 <https://datatracker.ietf.org/doc/html/rfc7636>`.
""",
)

client_id_env = ""
client_id = Unicode(
config=True,
Expand Down Expand Up @@ -980,6 +1008,18 @@ def build_access_tokens_request_params(self, handler, data=None):
"data": data,
}

if self.require_pkce:
# https://datatracker.ietf.org/doc/html/rfc7636#section-4.5
cookie_state = handler.get_state_cookie()
if not cookie_state:
raise web.HTTPError(400, "OAuth state missing from cookies")

code_verifier = _deserialize_state(cookie_state).get("code_verifier")
if not code_verifier:
raise web.HTTPError(400, "Missing code_verifier")

params.update([("code_verifier", code_verifier)])

# the client_id and client_secret should not be included in the access token request params
# when basic authentication is used
# ref: https://www.rfc-editor.org/rfc/rfc6749#section-2.3.1
Expand Down
52 changes: 52 additions & 0 deletions oauthenticator/tests/test_oauth2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ async def test_serialize_state():

TEST_STATE_ID = '123'
TEST_NEXT_URL = '/ABC'
TEST_CODE_VERIFIER = 'code_verifier123'


async def test_login_states():
Expand Down Expand Up @@ -206,3 +207,54 @@ async def test_add_user_override(
assert added_user.name in authenticator.allowed_users
else:
assert added_user.name not in authenticator.allowed_users


async def test_login_handler_pkce():
authenticator = OAuthenticator(require_pkce=True)
login_handler = mock_handler(OAuthLoginHandler, authenticator=authenticator)
login_handler.authorize_redirect = Mock()

login_handler.get() # no await, since authorize_redirect is mocked

# Check that PKCE parameters are included
assert (
"code_challenge"
in login_handler.authorize_redirect.call_args.kwargs["extra_params"]
)
assert (
login_handler.authorize_redirect.call_args.kwargs["extra_params"][
"code_challenge_method"
]
== "S256"
)


async def test_callback_handler_pkce():
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add a test to verify an error is returned if PKCE is requested but the server doesn't support it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I guess the way an Oauth server tells it doesn't support PKCE would be by returning a 403 when the client tries to exchange the code for a token.

Copy link
Member

Choose a reason for hiding this comment

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

Won't it work but just not be checked for validity if the provider doesn't support it (ignored extra parameters)? If that's true, should it be on by default?

Copy link
Member

Choose a reason for hiding this comment

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

If the parameters are definitely ignored then we could always send them, and change the property name to require_pkce to enforce it on the client side.

Copy link
Member

Choose a reason for hiding this comment

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

@renan-r-santos Sorry, I realise now I was unclear in my request. I was thinking of testing that raise web.HTTPError(400, "Missing code_verifier") is raised when the server silently ignores the PKCE request.

Regarding whether or not to always send the PKCE request, how about if we rename the parameter require_pkce instead of pkce, but keep the current implementation (only send the PKCE field when require_pkce = True? That lets us switch to always sending PKCE in future if we want, without having to change or add any parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding whether or not to always send the PKCE request, how about if we rename the parameter require_pkce instead of pkce, but keep the current implementation (only send the PKCE field when require_pkce = True?

That sounds like a good plan to me. I've updated the PR to reflect that.

I was thinking of testing that raise web.HTTPError(400, "Missing code_verifier") is raised when the server silently ignores the PKCE request.

raise web.HTTPError(400, "Missing code_verifier") won't get raised if the server silently ignores the PKCE request. code_verifier is data that we store in a cookie together with state_id and next_url in the login handler, but code_verifier isn't sent to or returned from the OAuth provider during login. It is only during code exchange that the client grabs the code_verifier previously stored in a cookie and sends it to the server so it can hash it and compare it with the code_challenge.

So, the error you mentioned can only happen if the cookie got somehow deleted or corrupted between login and callback handlers. If you still think it is worth adding a test for that, let me know and I'll update the PR. I could be wrong, but I don't think there's a way for a client to know if a server ignores PKCE parameters. On the other hand, a server can enforce that clients use PKCE.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think PKCE is only for the provider to check; clients only provide information. So the only reason to disable it that I can see is if some provider raises on unrecognized arguments, which is officially wrong:

The authorization server MUST ignore unrecognized request parameters.

So if we're only talking about valid OAuth providers, we don't even need to make it optional.

From the PKCE spec:

As the OAuth 2.0 [RFC6749] server responses are unchanged by this
specification, client implementations of this specification do not
need to know if the server has implemented this specification or not
and SHOULD send the additional parameters as defined in Section 4 to
all servers.

i.e. it's always right to send PKCE, and it's entirely up to the provider to decide whether to validate or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much for the references, especially

The authorization server MUST ignore unrecognized request parameters.

which I didn't know and makes a lot of difference.

url_state = _serialize_state({'state_id': TEST_STATE_ID})
callback_request_uri = f"http://myhost/callback?code=123&state={url_state}"

cookie_state = _serialize_state(
{
'state_id': TEST_STATE_ID,
'next_url': TEST_NEXT_URL,
'code_verifier': TEST_CODE_VERIFIER,
}
)

authenticator = OAuthenticator(require_pkce=True)
callback_handler = mock_handler(
OAuthCallbackHandler,
uri=callback_request_uri,
authenticator=authenticator,
)

callback_handler.get_secure_cookie = Mock(return_value=cookie_state.encode('utf8'))
callback_handler.login_user = Mock(return_value=mock_login_user_coro())
callback_handler.redirect = Mock()

await callback_handler.get()

callback_handler.redirect.assert_called_once_with('/ABC')
params = authenticator.build_access_tokens_request_params(callback_handler)

assert params['code_verifier'] == TEST_CODE_VERIFIER