diff --git a/libs/hackbot-client/hackbot_client/client.py b/libs/hackbot-client/hackbot_client/client.py index e9db069f88..1e0e8e996d 100644 --- a/libs/hackbot-client/hackbot_client/client.py +++ b/libs/hackbot-client/hackbot_client/client.py @@ -2,24 +2,58 @@ from __future__ import annotations +import asyncio from collections.abc import Mapping from typing import Any import httpx +from google.auth.transport import requests as google_requests +from google.oauth2 import id_token from hackbot_client.models import RunRef class HackbotClient: - """Call the public, API-key-authenticated Hackbot endpoints.""" + """Call the public Hackbot API with either API key or service account auth.""" def __init__( - self, base_url: str, api_key: str, timeout_seconds: float = 30.0 + self, + base_url: str, + api_key: str = "", + audience: str = "", + timeout_seconds: float = 30.0, ) -> None: + """Initialize the client. + + Args: + base_url: Base URL of the Hackbot API + api_key: API key for legacy authentication (optional) + audience: Audience for service account OIDC token minting. + Required if api_key is not set. + timeout_seconds: HTTP request timeout + """ self._base_url = base_url.rstrip("/") self._api_key = api_key + self._audience = audience self._timeout_seconds = timeout_seconds + if not api_key and not audience: + raise ValueError( + "Either api_key or audience must be provided for authentication" + ) + + async def _get_headers(self) -> dict[str, str]: + """Return auth headers: API key or service account token.""" + if self._api_key: + return {"X-API-Key": self._api_key} + + token = await asyncio.to_thread( + id_token.fetch_id_token, + google_requests.Request(), + self._audience, + ) + return {"Authorization": f"Bearer {token}"} + async def trigger_run( self, agent_name: str, @@ -28,7 +62,7 @@ async def trigger_run( on_behalf_of: str | None = None, ) -> RunRef: """Create an agent run and return the API's typed run reference.""" - headers = {"X-API-Key": self._api_key} + headers = await self._get_headers() if on_behalf_of is not None: headers["X-On-Behalf-Of"] = on_behalf_of diff --git a/libs/hackbot-client/pyproject.toml b/libs/hackbot-client/pyproject.toml index b63b8433b0..df2e8af07b 100644 --- a/libs/hackbot-client/pyproject.toml +++ b/libs/hackbot-client/pyproject.toml @@ -4,6 +4,7 @@ version = "0.1.0" description = "Small shared Hackbot API client (httpx-based)" requires-python = ">=3.12" dependencies = [ + "google-auth[requests]>=2.25.0", "httpx>=0.26.0", "pydantic>=2.6.0", ] diff --git a/libs/hackbot-client/tests/test_client.py b/libs/hackbot-client/tests/test_client.py index 44d022f525..5551f89cd9 100644 --- a/libs/hackbot-client/tests/test_client.py +++ b/libs/hackbot-client/tests/test_client.py @@ -12,11 +12,12 @@ def _client(**kwargs) -> HackbotClient: - return HackbotClient( - base_url=kwargs.pop("base_url", "https://hackbot.example"), - api_key=kwargs.pop("api_key", "secret"), - **kwargs, - ) + defaults = { + "base_url": "https://hackbot.example", + "api_key": "secret", + } + defaults.update(kwargs) + return HackbotClient(**defaults) def _capture_post(monkeypatch, response: httpx.Response) -> dict: @@ -95,3 +96,51 @@ async def test_trigger_run_rejects_an_invalid_success_response(monkeypatch): with pytest.raises(ValidationError): await _client().trigger_run("bug-fix", {"bug_id": 1234}) + + +def test_client_requires_either_api_key_or_audience(): + """Client must have at least one auth method configured.""" + with pytest.raises(ValueError, match="Either api_key or audience must be provided"): + HackbotClient(base_url="https://example.com") + + +async def test_trigger_run_with_service_account_auth(monkeypatch): + """Client uses Bearer token when api_key is not set but audience is.""" + captured = _capture_post( + monkeypatch, + httpx.Response( + 201, + json={"run_id": RUN_ID, "agent": "bug-fix", "status": "pending"}, + ), + ) + + def mock_fetch_id_token(request, audience): + return "fake.jwt.token" + + monkeypatch.setattr(client_module.id_token, "fetch_id_token", mock_fetch_id_token) + + client = _client(api_key="", audience="https://hackbot.example") + await client.trigger_run("bug-fix", {"bug_id": 1234}) + + assert captured["headers"] == {"Authorization": "Bearer fake.jwt.token"} + + +async def test_trigger_run_prefers_api_key_over_service_account(monkeypatch): + """When both api_key and audience are set, use the API key.""" + captured = _capture_post( + monkeypatch, + httpx.Response( + 201, + json={"run_id": RUN_ID, "agent": "bug-fix", "status": "pending"}, + ), + ) + + def mock_fetch_id_token(request, audience, credentials=None): + raise AssertionError("Should not call fetch_id_token when api_key is set") + + monkeypatch.setattr(client_module.id_token, "fetch_id_token", mock_fetch_id_token) + + client = _client(api_key="secret", audience="https://hackbot.example") + await client.trigger_run("bug-fix", {"bug_id": 1234}) + + assert captured["headers"] == {"X-API-Key": "secret"} diff --git a/services/hackbot-api/app/auth.py b/services/hackbot-api/app/auth.py index 903b4f4e66..2ae9fc16c6 100644 --- a/services/hackbot-api/app/auth.py +++ b/services/hackbot-api/app/auth.py @@ -3,6 +3,7 @@ import logging from fastapi import Header, HTTPException, Request, status +from google.auth import exceptions as google_auth_exceptions from google.auth.transport import requests as google_requests from google.oauth2 import id_token from slack_sdk.signature import SignatureVerifier @@ -79,18 +80,44 @@ async def require_slack_signature( ) -async def require_api_key(x_api_key: str | None = Header(default=None)) -> None: - if not settings.external_api_key: +def require_api_key( + x_api_key: str | None = Header(default=None), + authorization: str | None = Header(default=None), +) -> None: + """Accept either API key (X-API-Key) or service account token (Bearer).""" + if x_api_key is not None: + if settings.external_api_key and hmac.compare_digest( + x_api_key, settings.external_api_key + ): + return raise HTTPException( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - detail="API key not configured", + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Invalid X-API-Key", ) - if x_api_key is None or not hmac.compare_digest( - x_api_key, settings.external_api_key - ): + + if not authorization or not authorization.startswith("Bearer "): + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Invalid or missing credentials", + ) + + try: + claims = id_token.verify_oauth2_token( + authorization.removeprefix("Bearer "), + google_requests.Request(), + audience=settings.api_audience, + ) + except (ValueError, google_auth_exceptions.GoogleAuthError): + log.warning("Rejected request with invalid OIDC credentials") + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Invalid credentials", + ) from None + + if claims.get("email") not in settings.allowed_service_accounts: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, - detail="Invalid or missing X-API-Key", + detail="Service account not authorized", ) diff --git a/services/hackbot-api/app/config.py b/services/hackbot-api/app/config.py index b7673dc339..70b14ed681 100644 --- a/services/hackbot-api/app/config.py +++ b/services/hackbot-api/app/config.py @@ -70,6 +70,8 @@ class Settings(BaseSettings): # API auth external_api_key: str = "" + api_audience: str = "" + allowed_service_accounts: list[str] = [] phabricator: PhabricatorSettings diff --git a/services/hackbot-api/app/routers/webhooks.py b/services/hackbot-api/app/routers/webhooks.py index f096babcff..37638a6886 100644 --- a/services/hackbot-api/app/routers/webhooks.py +++ b/services/hackbot-api/app/routers/webhooks.py @@ -37,6 +37,7 @@ def get_hackbot_client() -> HackbotClient: return HackbotClient( base_url=settings.hackbot_api_url, api_key=settings.external_api_key, + audience=settings.api_audience, ) diff --git a/services/hackbot-api/tests/test_auth.py b/services/hackbot-api/tests/test_auth.py new file mode 100644 index 0000000000..8e504ac7c7 --- /dev/null +++ b/services/hackbot-api/tests/test_auth.py @@ -0,0 +1,37 @@ +import inspect + +import pytest +from app import auth +from fastapi import HTTPException +from google.auth import exceptions as google_auth_exceptions +from google.oauth2 import id_token + + +@pytest.mark.parametrize( + "error", + [ + ValueError(), + google_auth_exceptions.GoogleAuthError(), + google_auth_exceptions.TransportError(), + ], +) +def test_google_token_errors_return_401(monkeypatch, error): + def fail(*_args, **_kwargs): + raise error + + monkeypatch.setattr(id_token, "verify_oauth2_token", fail) + monkeypatch.setattr(auth.settings, "api_audience", "https://hackbot.example") + monkeypatch.setattr( + auth.settings, + "allowed_service_accounts", + ["listener@example.iam.gserviceaccount.com"], + ) + + with pytest.raises(HTTPException) as exc: + auth.require_api_key(x_api_key=None, authorization="Bearer bad-token") + + assert exc.value.status_code == 401 + + +def test_google_auth_dependencies_are_synchronous(): + assert not inspect.iscoroutinefunction(auth.require_api_key) diff --git a/uv.lock b/uv.lock index c4b7e1f0a7..26d3fd796a 100644 --- a/uv.lock +++ b/uv.lock @@ -2766,6 +2766,7 @@ name = "hackbot-client" version = "0.1.0" source = { editable = "libs/hackbot-client" } dependencies = [ + { name = "google-auth", extra = ["requests"] }, { name = "httpx" }, { name = "pydantic" }, ] @@ -2778,6 +2779,7 @@ dev = [ [package.metadata] requires-dist = [ + { name = "google-auth", extras = ["requests"], specifier = ">=2.25.0" }, { name = "httpx", specifier = ">=0.26.0" }, { name = "pydantic", specifier = ">=2.6.0" }, { name = "pytest", marker = "extra == 'dev'", specifier = ">=8.0.0" },