From 8ad7b9e5a2362bac31353c8b2a5c00269ccc012d Mon Sep 17 00:00:00 2001 From: Scott Trinh Date: Tue, 12 Mar 2024 13:51:00 -0400 Subject: [PATCH] Fallback to PKCE RFC parameter names (#7034) --- docs/guides/auth/built_in_ui.rst | 14 ++++++++++++ docs/guides/auth/email_password.rst | 14 ++++++++++++ docs/guides/auth/oauth.rst | 14 ++++++++++++ edb/server/protocol/auth_ext/http.py | 32 +++++++++++++++++++++------- tests/test_http_ext_auth.py | 4 ++-- 5 files changed, 68 insertions(+), 10 deletions(-) diff --git a/docs/guides/auth/built_in_ui.rst b/docs/guides/auth/built_in_ui.rst index e3709718b8..4c5d9d4237 100644 --- a/docs/guides/auth/built_in_ui.rst +++ b/docs/guides/auth/built_in_ui.rst @@ -55,6 +55,20 @@ end of the flow. Take this ``verifier`` string, hash it with SHA256, and then base64url encode the resulting string. This new string is called the ``challenge``. +.. note:: + + If you are familiar with PKCE, you will notice some differences from how RFC + 7636 defines PKCE. Our authentication flow is not an OAuth flow, but rather a + strict server-to-server flow with Proof Key of Code Exchange added for + additional security to avoid leaking the authentication token. Here are some + differences between PKCE as defined in RFC 7636 and our implementation: + + - We do not support the ``plain`` value for ``code_challenge_method``, and + therefore do not read that value if provided in requests. + - Our parameters omit the ``code_`` prefix, however we do support + ``code_challenge`` and ``code_verifier`` as aliases, preferring + ``challenge`` and ``verifier`` if present. + .. code-block:: javascript import http from "node:http"; diff --git a/docs/guides/auth/email_password.rst b/docs/guides/auth/email_password.rst index 405edebc23..d464997cc9 100644 --- a/docs/guides/auth/email_password.rst +++ b/docs/guides/auth/email_password.rst @@ -58,6 +58,20 @@ end of the flow. Take this ``verifier`` string, hash it with SHA256, and then base64url encode the resulting string. This new string is called the ``challenge``. +.. note:: + + If you are familiar with PKCE, you will notice some differences from how RFC + 7636 defines PKCE. Our authentication flow is not an OAuth flow, but rather a + strict server-to-server flow with Proof Key of Code Exchange added for + additional security to avoid leaking the authentication token. Here are some + differences between PKCE as defined in RFC 7636 and our implementation: + + - We do not support the ``plain`` value for ``code_challenge_method``, and + therefore do not read that value if provided in requests. + - Our parameters omit the ``code_`` prefix, however we do support + ``code_challenge`` and ``code_verifier`` as aliases, preferring + ``challenge`` and ``verifier`` if present. + .. lint-off .. code-block:: javascript diff --git a/docs/guides/auth/oauth.rst b/docs/guides/auth/oauth.rst index 9a18063514..b6cdb122c6 100644 --- a/docs/guides/auth/oauth.rst +++ b/docs/guides/auth/oauth.rst @@ -58,6 +58,20 @@ end of the flow. Take this ``verifier`` string, hash it with SHA256, and then base64url encode the resulting string. This new string is called the ``challenge``. +.. note:: + + If you are familiar with PKCE, you will notice some differences from how RFC + 7636 defines PKCE. Our authentication flow is not an OAuth flow, but rather a + strict server-to-server flow with Proof Key of Code Exchange added for + additional security to avoid leaking the authentication token. Here are some + differences between PKCE as defined in RFC 7636 and our implementation: + + - We do not support the ``plain`` value for ``code_challenge_method``, and + therefore do not read that value if provided in requests. + - Our parameters omit the ``code_`` prefix, however we do support + ``code_challenge`` and ``code_verifier`` as aliases, preferring + ``challenge`` and ``verifier`` if present. + .. code-block:: javascript import http from "node:http"; diff --git a/edb/server/protocol/auth_ext/http.py b/edb/server/protocol/auth_ext/http.py index 2f7e5ab8c2..cc5c9698de 100644 --- a/edb/server/protocol/auth_ext/http.py +++ b/edb/server/protocol/auth_ext/http.py @@ -253,7 +253,9 @@ async def handle_authorize(self, request: Any, response: Any): raise errors.InvalidData( "Redirect URL does not match any allowed URLs.", ) - challenge = _get_search_param(query, "challenge") + challenge = _get_search_param( + query, "challenge", fallback_keys=["code_challenge"] + ) oauth_client = oauth.Client( db=self.db, provider_name=provider_name, base_url=self.test_url ) @@ -386,7 +388,9 @@ async def handle_token(self, request: Any, response: Any): request.url.query.decode("ascii") if request.url.query else "" ) code = _get_search_param(query, "code") - verifier = _get_search_param(query, "verifier") + verifier = _get_search_param( + query, "verifier", fallback_keys=["code_verifier"] + ) verifier_size = len(verifier) @@ -1338,7 +1342,9 @@ async def handle_ui_forgot_password(self, request: Any, response: Any): query = urllib.parse.parse_qs( request.url.query.decode("ascii") if request.url.query else "" ) - challenge = _get_search_param(query, "challenge") + challenge = _get_search_param( + query, "challenge", fallback_keys=["code_challenge"] + ) app_details_config = self._get_app_details_config() response.status = http.HTTPStatus.OK @@ -1660,9 +1666,7 @@ def _make_session_token(self, identity_id: str) -> str: claims=claims, ) session_token.make_signed_token(signing_key) - metrics.auth_successful_logins.inc( - 1.0, self.tenant.get_instance_name() - ) + metrics.auth_successful_logins.inc(1.0, self.tenant.get_instance_name()) return session_token.serialize() def _get_from_claims(self, state: str, key: str) -> str: @@ -1987,8 +1991,18 @@ def _maybe_get_search_param( return params[0] if params else None -def _get_search_param(query_dict: dict[str, list[str]], key: str) -> str: +def _get_search_param( + query_dict: dict[str, list[str]], + key: str, + *, + fallback_keys: Optional[list[str]] = None, +) -> str: val = _maybe_get_search_param(query_dict, key) + if val is None and fallback_keys is not None: + for fallback_key in fallback_keys: + val = _maybe_get_search_param(query_dict, fallback_key) + if val is not None: + break if val is None: raise errors.InvalidData(f"Missing query parameter: {key}") return val @@ -2010,7 +2024,9 @@ def _get_pkce_challenge( query_dict: dict[str, list[str]], ) -> str | None: cookie_name = 'edgedb-pkce-challenge' - challenge: str | None = _maybe_get_search_param(query_dict, 'challenge') + challenge: str | None = _maybe_get_search_param( + query_dict, 'challenge' + ) or _maybe_get_search_param(query_dict, "code_challenge") if challenge is not None: _set_cookie(response, cookie_name, challenge) else: diff --git a/tests/test_http_ext_auth.py b/tests/test_http_ext_auth.py index 6e8fb9beb0..eacf71b21f 100644 --- a/tests/test_http_ext_auth.py +++ b/tests/test_http_ext_auth.py @@ -1071,7 +1071,7 @@ async def test_http_auth_ext_discord_authorize_01(self): query = { "provider": provider_name, "redirect_to": redirect_to, - "challenge": challenge, + "code_challenge": challenge, } _, headers, status = self.http_con_request( @@ -3042,7 +3042,7 @@ async def test_http_auth_ext_token_01(self): http_con, { "code": pkce.id, - "verifier": base64.urlsafe_b64encode(os.urandom(43)) + "code_verifier": base64.urlsafe_b64encode(os.urandom(43)) .rstrip(b"=") .decode(), },