Merge pull request #948 from lsst-sqre/tickets/DM-42384

DM-42384: Add OpenID Connect return URL registration

changelog.d/20240123_163221_rra_DM_42384.md

@@ -0,0 +1,3 @@
+### Backwards-incompatible changes
+
+- Clients of the Gafaelfawr OpenID Connect server now must have registered return URIs as well as client IDs and secrets. Each element of the `oidc-server-secrets` secret must, in addition to the previous `id` and `secret` keys, contain a `return_uri` key that matches the return URL of authentications from that client. Those return URLs are now allowed to be at any (matching) domain and are not constrained to the same domain as Gafaelfawr.

docs/user-guide/openid-connect.rst

@@ -11,9 +11,18 @@ To protect a service that uses OpenID Connect, first set ``oidcServer.enabled``
 Then, create (or add to, if already existing) an ``oidc-server-secrets`` secret for the ``gafaelfawr`` Phalanx application.
 
 The value of the secret must be a JSON list, with each list member representing one OpenID Connect client.
-Each list member must be an object with two keys: ``id`` and ``secret``.
-``id`` is the unique OpenID Connect client ID that the client will present during authentication.
-``secret`` should be a randomly-generated secret that the client will use to authenticate.
+Each list member must be an object with the following keys:
+
+``id``
+    The unique OpenID Connect client ID (the ``client_id`` parameter in the OpenID Connect protocol) that the client will present during authentication.
+
+``secret``
+    A randomly-generated secret that the client will use to authenticate via the ``client_secret`` POST parameter.
+
+``return_uri``
+    The acceptable return URL for this client.
+    The actual return URL (the ``redirect_uri`` parameter) of any authentication must exactly match this return URL except for query parameters and fragments.
+    The path portion of this URL may not contain semicolons (``;``) to avoid potentially confusing parsing as either part of the path or as path parameters.
 
 Configure the OpenID client
 ===========================

src/gafaelfawr/config.py

@@ -753,6 +753,9 @@ class OIDCClient:
     client_secret: str
     """Secret used to authenticate this client."""
 
+    return_uri: str
+    """Acceptable return URL when authenticating users for this client."""
+
 
 @dataclass(frozen=True, slots=True)
 class OIDCServerConfig:
@@ -1021,7 +1024,11 @@ def from_file(cls, path: Path) -> Self:  # noqa: PLR0912,PLR0915,C901
             oidc_secrets_json = cls._load_secret(path).decode()
             oidc_secrets = json.loads(oidc_secrets_json)
             oidc_clients = tuple(
-                OIDCClient(client_id=c["id"], client_secret=c["secret"])
+                OIDCClient(
+                    client_id=c["id"],
+                    client_secret=c["secret"],
+                    return_uri=c["return_uri"],
+                )
                 for c in oidc_secrets
             )
             data_rights_mapping = {

src/gafaelfawr/dependencies/return_url.py

@@ -8,6 +8,7 @@
 
 from __future__ import annotations
 
+from typing import Annotated
 from urllib.parse import ParseResult, urlparse
 
 from fastapi import Depends, Header, Query
@@ -16,7 +17,6 @@
 from .context import RequestContext, context_dependency
 
 __all__ = [
-    "parsed_redirect_uri",
     "return_url",
     "return_url_with_header",
 ]
@@ -64,13 +64,15 @@ def _check_url(url: str, param: str, context: RequestContext) -> ParseResult:
 
 
 async def return_url(
-    rd: (str | None) = Query(
-        None,
-        title="URL to return to",
-        description="User is sent here after operation",
-        examples=["https://example.com/"],
-    ),
-    context: RequestContext = Depends(context_dependency),
+    context: Annotated[RequestContext, Depends(context_dependency)],
+    rd: Annotated[
+        str | None,
+        Query(
+            title="URL to return to",
+            description="User is sent here after operation",
+            examples=["https://example.com/"],
+        ),
+    ] = None,
 ) -> str | None:
     """Validate a return URL in an ``rd`` parameter.
 
@@ -92,22 +94,26 @@ async def return_url(
 
 
 async def return_url_with_header(
-    rd: (str | None) = Query(
-        None,
-        title="URL to return to",
-        description=(
-            "User is sent here after successful authentication. Overrides"
-            " `X-Auth-Request-Redirect` if both are set."
+    context: Annotated[RequestContext, Depends(context_dependency)],
+    rd: Annotated[
+        str | None,
+        Query(
+            title="URL to return to",
+            description=(
+                "User is sent here after successful authentication. Overrides"
+                " `X-Auth-Request-Redirect` if both are set."
+            ),
+            examples=["https://example.com/"],
+        ),
+    ] = None,
+    x_auth_request_redirect: Annotated[
+        str | None,
+        Header(
+            title="URL to return to",
+            description="User is sent here after successful authentication",
+            examples=["https://example.com/"],
         ),
-        examples=["https://example.com/"],
-    ),
-    x_auth_request_redirect: (str | None) = Header(
-        None,
-        title="URL to return to",
-        description="User is sent here after successful authentication",
-        examples=["https://example.com/"],
-    ),
-    context: RequestContext = Depends(context_dependency),
+    ] = None,
 ) -> str | None:
     """Validate a return URL in an ``rd`` parameter or header.
 
@@ -127,34 +133,4 @@ async def return_url_with_header(
     """
     if not rd and x_auth_request_redirect:
         rd = x_auth_request_redirect
-    return await return_url(rd, context)
-
-
-async def parsed_redirect_uri(
-    redirect_uri: str = Query(
-        ...,
-        title="URL to return to",
-        description=(
-            "User is sent here after successful or failed authentication"
-        ),
-        examples=["https://example.com/"],
-    ),
-    context: RequestContext = Depends(context_dependency),
-) -> ParseResult:
-    """Validate a return URL in a ``redirect_uri`` parameter.
-
-    Same as `return_url` except expects the URL in a ``return_uri`` parameter
-    instead of ``rd`` and returns a parsed URL instead of the `str` form.
-
-    Returns
-    -------
-    urllib.parse.ParseResult
-        The verified, parsed redirect URI.
-
-    Raises
-    ------
-    fastapi.HTTPException
-        An appropriate error if the return URL was invalid.
-    """
-    context.rebind_logger(return_url=redirect_uri)
-    return _check_url(redirect_uri, "redirect_uri", context)
+    return await return_url(context, rd)

src/gafaelfawr/exceptions.py

@@ -24,6 +24,7 @@
     "InputValidationError",
     "InsufficientScopeError",
     "InvalidClientError",
+    "InvalidClientIdError",
     "InvalidCSRFError",
     "InvalidCursorError",
     "InvalidDelegateToError",
@@ -55,7 +56,7 @@
     "PermissionDeniedError",
     "ProviderError",
     "ProviderWebError",
-    "UnauthorizedClientError",
+    "ReturnUriMismatchError",
     "UnknownAlgorithmError",
     "UnknownKeyIdError",
     "UnsupportedGrantTypeError",
@@ -80,6 +81,16 @@ def __init__(self, message: str) -> None:
         super().__init__(message, ErrorLocation.body, ["token_name"])
 
 
+class InvalidClientIdError(InputValidationError):
+    """Invalid client ID for OpenID Connect server."""
+
+    error = "invalid_client"
+    status_code = status.HTTP_403_FORBIDDEN
+
+    def __init__(self, message: str) -> None:
+        super().__init__(message, ErrorLocation.query, ["client_id"])
+
+
 class InvalidCSRFError(InputValidationError):
     """Invalid or missing CSRF token."""
 
@@ -181,6 +192,16 @@ class PermissionDeniedError(InputValidationError):
     status_code = status.HTTP_403_FORBIDDEN
 
 
+class ReturnUriMismatchError(InputValidationError):
+    """Specified return URI does not match return URI of registered client."""
+
+    error = "return_uri_mismatch"
+    status_code = status.HTTP_403_FORBIDDEN
+
+    def __init__(self, message: str) -> None:
+        super().__init__(message, ErrorLocation.query, ["return_uri"])
+
+
 class OAuthError(Exception):
     """An OAuth-related error occurred.
 
@@ -390,13 +411,6 @@ class OIDCWebError(ProviderWebError):
     """A web request to the OpenID Connect provider failed."""
 
 
-class UnauthorizedClientError(Exception):
-    """The client is not authorized to request an authorization code.
-
-    This corresponds to the ``unauthorized_client`` error in RFC 6749.
-    """
-
-
 class VerifyTokenError(SlackException):
     """Base exception class for failure in verifying a token."""
 

src/gafaelfawr/handlers/oidc.py

@@ -5,24 +5,15 @@
 import time
 from collections.abc import Mapping
 from typing import Annotated, Any
-from urllib.parse import ParseResult, parse_qsl, urlencode
+from urllib.parse import parse_qsl, urlencode, urlparse
 
-from fastapi import (
-    APIRouter,
-    Depends,
-    Form,
-    HTTPException,
-    Query,
-    Response,
-    status,
-)
+from fastapi import APIRouter, Depends, Form, Query, Response, status
 from fastapi.responses import JSONResponse, RedirectResponse
 from safir.models import ErrorModel
 from safir.slack.webhook import SlackRouteErrorHandler
 
 from ..dependencies.auth import AuthenticateRead, verified_oidc_token
 from ..dependencies.context import RequestContext, context_dependency
-from ..dependencies.return_url import parsed_redirect_uri
 from ..exceptions import InvalidRequestError, OAuthError
 from ..models.oidc import (
     JWKS,
@@ -76,7 +67,16 @@ async def get_login(
             examples=["https://example.org/chronograf"],
         ),
     ],
-    parsed_redirect_uri: Annotated[ParseResult, Depends(parsed_redirect_uri)],
+    redirect_uri: Annotated[
+        str,
+        Query(
+            title="URL to return to",
+            description=(
+                "User is sent here after successful or failed authentication"
+            ),
+            examples=["https://example.com/"],
+        ),
+    ],
     token_data: Annotated[TokenData, Depends(authenticate)],
     context: Annotated[RequestContext, Depends(context_dependency)],
     response_type: Annotated[
@@ -124,17 +124,13 @@ async def get_login(
         ),
     ] = None,
 ) -> str:
+    context.rebind_logger(return_uri=redirect_uri)
     oidc_service = context.factory.create_oidc_service()
 
-    # Check the client_id first, since if it's not valid, we cannot continue
-    # or send any errors back to the client via redirect.
-    if not oidc_service.is_valid_client(client_id):
-        msg = f"Unknown client_id {client_id} in OpenID Connect request"
-        context.logger.warning("Invalid request", error=msg)
-        raise HTTPException(
-            status_code=status.HTTP_400_BAD_REQUEST,
-            detail=[{"type": "invalid_client", "msg": msg}],
-        )
+    # Check the client_id and redirect_uri first, since if either of them are
+    # not valid, we cannot continue or send any errors back to the client via
+    # redirect.
+    oidc_service.validate_client(client_id, redirect_uri)
 
     # Parse the authentication request.
     error = None
@@ -152,7 +148,7 @@ async def get_login(
         e = InvalidRequestError(error)
         context.logger.warning("%s", e.message, error=str(e))
         return build_return_url(
-            parsed_redirect_uri,
+            redirect_uri,
             state=state,
             error=e.error,
             error_description=str(e),
@@ -161,25 +157,23 @@ async def get_login(
     # Get an authorization code and return it.
     code = await oidc_service.issue_code(
         client_id=client_id,
-        redirect_uri=parsed_redirect_uri.geturl(),
+        redirect_uri=redirect_uri,
         token=token_data.token,
         scopes=scopes,
         nonce=nonce,
     )
-    return_url = build_return_url(
-        parsed_redirect_uri, state=state, code=str(code)
-    )
+    return_url = build_return_url(redirect_uri, state=state, code=str(code))
     context.logger.info("Returned OpenID Connect authorization code")
     return return_url
 
 
-def build_return_url(redirect_uri: ParseResult, **params: str | None) -> str:
+def build_return_url(redirect_uri: str, **params: str | None) -> str:
     """Construct a return URL for a redirect.
 
     Parameters
     ----------
     redirect_uri
-        The parsed return URI from the client.
+        Return URI from the client.
     **params
         Additional parameters to add to that URI to create the return URL.
         Any parameters set to `None` will be ignored.
@@ -189,9 +183,10 @@ def build_return_url(redirect_uri: ParseResult, **params: str | None) -> str:
     str
         The return URL to which the user should be redirected.
     """
-    query = parse_qsl(redirect_uri.query) if redirect_uri.query else []
+    parsed_uri = urlparse(redirect_uri)
+    query = parse_qsl(parsed_uri.query) if parsed_uri.query else []
     query.extend((k, v) for (k, v) in params.items() if v is not None)
-    return_url = redirect_uri._replace(query=urlencode(query))
+    return_url = parsed_uri._replace(query=urlencode(query))
     return return_url.geturl()