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

Fallback to PKCE RFC parameter names #7034

Merged
merged 4 commits into from Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions docs/guides/auth/built_in_ui.rst
Expand Up @@ -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";
Expand Down
14 changes: 14 additions & 0 deletions docs/guides/auth/email_password.rst
Expand Up @@ -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
Expand Down
14 changes: 14 additions & 0 deletions docs/guides/auth/oauth.rst
Expand Up @@ -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";
Expand Down
32 changes: 24 additions & 8 deletions edb/server/protocol/auth_ext/http.py
Expand Up @@ -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
)
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand All @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions tests/test_http_ext_auth.py
Expand Up @@ -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(
Expand Down Expand Up @@ -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(),
},
Expand Down