Merge pull request #1257 from lsst-sqre/tickets/DM-49329

DM-49329: Always authorize OPTIONS requests

Author: rra

changelog.d/20250305_171727_rra_DM_49315.md

@@ -0,0 +1,3 @@
+### Backwards-incompatible changes
+
+- Always authorize `OPTIONS` requests rather than expecting them to contain authentication credentials. This is required for proper CORS handling since the CORS pre-flight specification says that credentials are not included. Gafaelfawr previously effectively blocked all `OPTIONS` requests by replying with 302 or 401.

src/gafaelfawr/handlers/ingress.py

@@ -299,9 +299,18 @@ async def authenticate_with_type(
             examples=["basic"],
         ),
     ] = AuthType.Bearer,
+    x_original_method: Annotated[str, Header()],
     context: Annotated[RequestContext, Depends(context_dependency)],
-) -> TokenData:
-    """Set authentication challenge based on auth_type parameter."""
+) -> TokenData | None:
+    """Handle ``OPTIONS`` and user authentication.
+
+    If the request method is ``OPTIONS``, no authentication information will
+    be included. Return `None` in that case. Otherwise, authenticate the user
+    or, if they are not authenticated, send an authentication challenge based
+    on the ``auth_type`` parameter.
+    """
+    if x_original_method == "OPTIONS":
+        return None
     authenticate = AuthenticateRead(
         allow_cookies=allow_cookies, auth_type=auth_type, ajax_forbidden=True
     )
@@ -321,11 +330,25 @@ async def authenticate_with_type(
 )
 async def get_auth(
     *,
+    x_original_method: Annotated[str, Header()],
     auth_config: Annotated[AuthConfig, Depends(auth_config)],
-    token_data: Annotated[TokenData, Depends(authenticate_with_type)],
+    token_data: Annotated[TokenData | None, Depends(authenticate_with_type)],
     context: Annotated[RequestContext, Depends(context_dependency)],
     response: Response,
 ) -> dict[str, str]:
+    # If token_data is None, this is an OPTIONS requests. Those requests are
+    # treated identically to anonymous requests: we strip any authentication
+    # credentials (which should not be included anyway) and always return
+    # success. We cannot really do anything else, since the standard says CORS
+    # pre-flight checks should not include credentials.
+    #
+    # Redo the verification that the method really is OPTIONS out of paranoia.
+    if not token_data:
+        if x_original_method != "OPTIONS":
+            raise RuntimeError("Unauthenticated request to /ingress/auth")
+        return await get_anonymous(context=context, response=response)
+
+    # Do some basic checks and analysis of the authentication data.
     check_lifetime(context, auth_config, token_data)
     token_scopes = set(token_data.scopes)
 

src/gafaelfawr/services/kubernetes.py

@@ -128,7 +128,7 @@ def _build_annotations(self, ingress: GafaelfawrIngress) -> dict[str, str]:
         }
         if ingress.config.auth_cache_duration:
             annotations["nginx.ingress.kubernetes.io/auth-cache-key"] = (
-                "$http_cookie$http_authorization"
+                "$request_method$http_cookie$http_authorization"
             )
             annotations["nginx.ingress.kubernetes.io/auth-cache-duration"] = (
                 f"200 202 401 {ingress.config.auth_cache_duration}"

tests/conftest.py

@@ -74,7 +74,10 @@ async def client(app: FastAPI) -> AsyncIterator[AsyncClient]:
     """Return an ``httpx.AsyncClient`` configured to talk to the test app."""
     async with AsyncClient(
         base_url=f"https://{TEST_HOSTNAME}",
-        headers={"X-Original-URL": "https://foo.example.com/bar"},
+        headers={
+            "X-Original-Method": "GET",
+            "X-Original-URL": "https://foo.example.com/bar",
+        },
         transport=ASGITransport(app=app),
     ) as client:
         yield client

tests/data/kubernetes/output/ingresses.yaml

@@ -45,7 +45,7 @@ metadata:
   namespace: {namespace}
   annotations:
     another.annotation.example.com: bar
-    nginx.ingress.kubernetes.io/auth-cache-key: "$http_cookie$http_authorization"
+    nginx.ingress.kubernetes.io/auth-cache-key: "$request_method$http_cookie$http_authorization"
     nginx.ingress.kubernetes.io/auth-cache-duration: "200 202 401 5m"
     nginx.ingress.kubernetes.io/auth-method: "GET"
     nginx.ingress.kubernetes.io/auth-response-headers: "Authorization,Cookie,X-Auth-Request-Email,X-Auth-Request-Service,X-Auth-Request-Token,X-Auth-Request-User"

tests/handlers/ingress_test.py

@@ -1229,3 +1229,40 @@ async def test_user_domain(client: AsyncClient, factory: Factory) -> None:
         assert isinstance(authenticate, AuthErrorChallenge)
         assert authenticate.auth_type == AuthType.Bearer
         assert authenticate.error == AuthError.insufficient_scope
+
+
+@pytest.mark.asyncio
+async def test_options(client: AsyncClient, factory: Factory) -> None:
+    """Test special handling of OPTIONS."""
+    token_data = await create_session_token(
+        factory, group_names=["admin"], scopes={"read:all"}
+    )
+
+    # An OPTIONS request should always be allowed, even if there are no
+    # credentials provided at all and scopes are required.
+    r = await client.get(
+        "/ingress/auth",
+        params={"scope": "read:all"},
+        headers={
+            "X-Original-Method": "OPTIONS",
+            "X-Original-URL": "https://example.com/foo",
+        },
+    )
+    assert r.status_code == 200
+
+    # If credentials are included, they should be stripped.
+    await set_session_cookie(client, token_data.token)
+    client.cookies.set("_other", "somevalue", domain=TEST_HOSTNAME)
+    r = await client.get(
+        "/ingress/auth",
+        params={"scope": "read:all"},
+        headers=[
+            ("Authorization", "token some-other-token"),
+            ("Authorization", f"bearer {Token()}"),
+            ("X-Original-Method", "OPTIONS"),
+            ("X-Original-URL", "https://example.com/foo"),
+        ],
+    )
+    assert r.status_code == 200
+    assert r.headers["Authorization"] == "token some-other-token"
+    assert r.headers["Cookie"] == "_other=somevalue"

tests/selenium/tokens_test.py

@@ -57,6 +57,7 @@ async def test_token_info(
             params={"scope": "exec:test", "notebook": "true"},
             headers={
                 "Cookie": f"{COOKIE_NAME}={cookie}",
+                "X-Original-Method": "GET",
                 "X-Original-URL": "https://foo.example.com/bar",
             },
         )
@@ -67,6 +68,7 @@ async def test_token_info(
             params={"scope": "exec:test", "delegate_to": "service"},
             headers={
                 "Cookie": f"{COOKIE_NAME}={cookie}",
+                "X-Original-Method": "GET",
                 "X-Original-URL": "https://foo.example.com/bar",
             },
         )