Merge pull request #1141 from lsst-sqre/tickets/DM-47148

DM-47148: Add support for `client_secret_basic`

changelog.d/20241028_133552_rra_DM_47148.md

@@ -0,0 +1,3 @@
+### New features
+
+- Add support for `client_secret_basic` to the token endpoint for the OpenID Connect server. This is the recommended default authentication strategy and some clients don't support negotiating `client_secret_post` instead.

docs/user-guide/openid-connect.rst

@@ -37,12 +37,15 @@ For clients that require more manual configuration, the OpenID Connect routes ar
 - userinfo endpoint: ``/auth/openid/userinfo``.
 - JWKS endpoint: ``/.well-known/jwks.json``.
 
-As with any other protected service, the client must run on the same URL host as Gafaelfawr.
-These endpoints are all at that shared host (and should be specified using ``https``).
+The hostname for those routes is whatever host Gafaelfawr itself is configured to use.
+(Generally this will be the default domain of the Phalanx cluster.)
 
 The client must use the authentication code OpenID Connect flow (see `OpenID Connect Core 1.0 section 3.1 <https://openid.net/specs/openid-connect-core-1_0.html#CodeFlowAuth>`__).
 The other authentication flows are not supported.
 
+The authentication methods ``client_secret_basic`` and ``client_secret_post`` are supported.
+Gafaelfawr does not register a specific authentication method for a client and supports either authentication method for any client.
+
 OpenID scopes
 -------------
 

src/gafaelfawr/handlers/oidc.py

@@ -8,6 +8,7 @@
 
 from fastapi import APIRouter, Depends, Form, Query, Response, status
 from fastapi.responses import JSONResponse, RedirectResponse
+from fastapi.security import HTTPBasic, HTTPBasicCredentials
 from safir.models import ErrorModel
 from safir.slack.webhook import SlackRouteErrorHandler
 
@@ -40,6 +41,11 @@
     require_session=True, redirect_if_unauthenticated=True
 )
 authenticate_token = AuthenticateRead(require_bearer_token=True)
+client_basic = HTTPBasic(
+    scheme_name="oidc",
+    description="OpenID Connect client authentication",
+    auto_error=False,
+)
 
 
 @router.get(
@@ -236,6 +242,7 @@ async def post_token(
             examples=["gc-W74I5HltJZRc0fOUAapgVQ.3T1xQQgeD063KgmNinw-tA"],
         ),
     ] = None,
+    credentials: Annotated[HTTPBasicCredentials | None, Depends(client_basic)],
     redirect_uri: Annotated[
         str | None,
         Form(
@@ -248,6 +255,9 @@ async def post_token(
     response: Response,
 ) -> OIDCTokenReply | JSONResponse:
     oidc_service = context.factory.create_oidc_service()
+    if credentials:
+        client_id = credentials.username
+        client_secret = credentials.password
     async with context.session.begin():
         try:
             reply = await oidc_service.redeem_code(

src/gafaelfawr/models/oidc.py

@@ -456,8 +456,10 @@ class OIDCConfig(BaseModel):
     )
 
     token_endpoint_auth_methods_supported: list[str] = Field(
-        ["client_secret_post"],
+        ["client_secret_basic", "client_secret_post"],
         title="Supported client auth methods",
-        description="``client_secret_post`` is the only supported auth method",
-        examples=[["client_secret_post"]],
+        description=(
+            "``client_secret_basic`` and ``client_secret_post`` are supported"
+        ),
+        examples=[["client_secret_basic", "client_secret_post"]],
     )

tests/handlers/oidc_test.py

@@ -10,7 +10,7 @@
 from urllib.parse import parse_qs, urlencode, urlparse
 
 import pytest
-from httpx import AsyncClient
+from httpx import AsyncClient, BasicAuth
 from safir.datetime import current_datetime, format_datetime_for_logging
 from safir.testing.slack import MockSlackWebhook
 
@@ -47,6 +47,7 @@ async def authenticate(
     *,
     client_secret: str,
     expires: datetime,
+    use_basic_auth: bool = False,
 ) -> OIDCTokenReply:
     """Authenticate to Gafaelfawr with OpenID Connect.
 
@@ -62,6 +63,9 @@ async def authenticate(
         Secret used to authenticate to the token endpoint.
     expires
         Expected expiration of ID token.
+    use_basic_auth
+        Whether to use HTTP Basic Authentication instead of POSTing the
+        credentials.
 
     Returns
     -------
@@ -87,16 +91,29 @@ async def authenticate(
     code = query["code"][0]
 
     # Redeem the code for a token and check the result.
-    r = await client.post(
-        "/auth/openid/token",
-        data={
-            "grant_type": "authorization_code",
-            "client_id": request["client_id"],
-            "client_secret": client_secret,
-            "code": code,
-            "redirect_uri": request["redirect_uri"],
-        },
-    )
+    if use_basic_auth:
+        r = await client.post(
+            "/auth/openid/token",
+            auth=BasicAuth(
+                username=request["client_id"], password=client_secret
+            ),
+            data={
+                "grant_type": "authorization_code",
+                "code": code,
+                "redirect_uri": request["redirect_uri"],
+            },
+        )
+    else:
+        r = await client.post(
+            "/auth/openid/token",
+            data={
+                "grant_type": "authorization_code",
+                "client_id": request["client_id"],
+                "client_secret": client_secret,
+                "code": code,
+                "redirect_uri": request["redirect_uri"],
+            },
+        )
     assert r.status_code == 200
     assert r.headers["Cache-Control"] == "no-store"
     assert r.headers["Pragma"] == "no-cache"
@@ -862,7 +879,10 @@ async def test_well_known_oidc(
         "grant_types_supported": ["authorization_code"],
         "subject_types_supported": ["public"],
         "id_token_signing_alg_values_supported": [ALGORITHM],
-        "token_endpoint_auth_methods_supported": ["client_secret_post"],
+        "token_endpoint_auth_methods_supported": [
+            "client_secret_basic",
+            "client_secret_post",
+        ],
     }
 
 
@@ -1005,3 +1025,44 @@ async def test_data_rights(
         "preferred_username": token_data.username,
         "sub": token_data.username,
     }
+
+
+@pytest.mark.asyncio
+async def test_basic_auth(
+    client: AsyncClient, factory: Factory, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    redirect_uri = "https://example.org/"
+    clients = [build_oidc_client("some-id", "some-secret", redirect_uri)]
+    config = await reconfigure(
+        "github-oidc-server", factory, monkeypatch, oidc_clients=clients
+    )
+    assert config.oidc_server
+    token_data = await create_session_token(factory)
+    assert token_data.expires
+    await set_session_cookie(client, token_data.token)
+    oidc_service = factory.create_oidc_service()
+
+    reply = await authenticate(
+        factory,
+        client,
+        {
+            "response_type": "code",
+            "scope": "openid",
+            "client_id": "some-id",
+            "state": "random-state",
+            "redirect_uri": redirect_uri,
+        },
+        client_secret="some-secret",
+        expires=token_data.expires,
+        use_basic_auth=True,
+    )
+    id_token = oidc_service.verify_token(OIDCToken(encoded=reply.id_token))
+    assert id_token.claims == {
+        "aud": "some-id",
+        "exp": int(token_data.expires.timestamp()),
+        "iat": ANY,
+        "iss": str(config.oidc_server.issuer),
+        "jti": ANY,
+        "scope": "openid",
+        "sub": token_data.username,
+    }