diff --git a/README.md b/README.md index 1bc8541..79b4973 100644 --- a/README.md +++ b/README.md @@ -90,12 +90,23 @@ Configuration is by environment variable, and `example.env` lists them: | `BIOCHEF_RUN_TIMEOUT` | `900` | seconds before a run's whole process group is killed | | `BIOCHEF_KEEP_WORKSPACE` | `false` | leave a run's directory behind, for debugging | | `BIOCHEF_MAX_UPLOAD_BYTES` | `536870912` | largest request body accepted, in bytes | +| `BIOCHEF_AUTH` | `none` | who may call it: `none` or `bearer` | +| `BIOCHEF_AUTH_TOKEN` | | the shared token, required when `BIOCHEF_AUTH=bearer` | | `BIOCHEF_RUNNER` | `subprocess` | how a workflow executes: `subprocess` or `apptainer` | | `BIOCHEF_CONTAINER_IMAGE` | `docker://debian:stable-slim` | image each step runs in, under the `apptainer` runner | | `BIOCHEF_APPTAINER_CACHE` | `apptainer-cache` | where pulled container images are kept between runs | | `BIOCHEF_APPTAINER_ARGS` | `--contain` | extra flags for apptainer itself | -Three of those decide how isolated a run is, and are worth reading twice before +`BIOCHEF_AUTH` defaults to `none`, which means **any caller that can open a +socket to this service can make it execute tool binaries**. That is a reasonable +default on a laptop and the wrong one anywhere else. `bearer` requires +`Authorization: Bearer ` matching `BIOCHEF_AUTH_TOKEN`; a shared secret is +not identity -- every holder is the same caller -- but it is the difference +between an open endpoint and a closed one. Selecting `bearer` without a token +stops the service from starting rather than letting it run with a token nobody +has to guess. + +Three more decide how isolated a run is, and are worth reading twice before changing. `BIOCHEF_RUNNER` defaults to `subprocess`, which runs every step **on the host, @@ -141,15 +152,22 @@ run's data**. Emptying this variable turns that off deliberately. ## Before deploying this -**It is not ready to be exposed.** There is no authentication of any kind, and -several open issues describe defects reachable by anyone who can reach the port. -The most significant are tracked in the issue tracker; read them before putting -this anywhere a stranger can send it a request. +**It is not ready to be exposed.** Several open issues describe defects reachable +by anyone who can reach the port. The most significant are tracked in the issue +tracker; read them before putting this anywhere a stranger can send it a request. + +Authentication now exists but is **off by default**. `BIOCHEF_AUTH=none` is the +default, and it means what it says: any caller that can open a socket can make +this service execute tool binaries. Setting `BIOCHEF_AUTH=bearer` closes that, +and a deployment that does not is choosing to leave it open. + +Even set, a shared token is not identity — every holder is the same caller, and +nothing yet decides what a given caller may run or read. That is F3 (Passports), +and it does not exist. That matters more here than the sentence usually implies. The environments this is aimed at are the ones where it would do the most damage: an agent inside a -TRE sits next to data that is there precisely because it may not leave. Deciding -what a caller may run and read is C2 and F3, and neither exists yet. +TRE sits next to data that is there precisely because it may not leave. Development is organised as numbered workstreams (A–G) in the issues: the converter and its intermediate model, asynchronous runs, authentication and diff --git a/auth.py b/auth.py new file mode 100644 index 0000000..c0077e7 --- /dev/null +++ b/auth.py @@ -0,0 +1,179 @@ +"""Who may ask this service to run something (#10). + +The same shape as runner.py, and for the same reason. An interface, providers +selected by name, and the policy that every provider shares written once. F3 +(Passports) should be a third provider here rather than a rewrite of the second. + +What a provider decides is narrow on purpose: given a request, either it names a +caller or it refuses. It does not decide what that caller may do -- there is no +authorisation model yet, and pretending otherwise by returning roles nobody +consults would be worse than the gap. +""" + +import hmac +import json +import os +from typing import Optional + +from fastapi import HTTPException, Request + + +class Unauthenticated(HTTPException): + """401, with the challenge the specification requires. + + A 401 without WWW-Authenticate is not a well-formed refusal: RFC 9110 makes + the header mandatory on 401, and it is what tells a client which scheme to + try. 403 would be the wrong code -- it means "you are known and still may + not", which is a statement this service is not yet in a position to make. + """ + + def __init__(self, detail: str, scheme: str = "Bearer"): + super().__init__(status_code=401, detail=detail, + headers={"WWW-Authenticate": scheme}) + + +AUTH_TOKEN = os.getenv("BIOCHEF_AUTH_TOKEN", "") +"""The shared secret, when BIOCHEF_AUTH=bearer. + +Read from the environment rather than a file or an argument, so it does not end +up in a process listing or in shell history. +""" + + +class AuthProvider: + """Decides whether a request may proceed, and on whose behalf.""" + + name = "provider" + + def authenticate(self, request: Request) -> Optional[str]: + """Return an identity for the caller, or raise Unauthenticated. + + The return value is deliberately just a name. Nothing consults it yet; + it exists so that a provider which knows more about the caller -- a + Passport carries claims -- has somewhere to put it without changing the + signature of everything that calls this. + """ + raise NotImplementedError + + def describe(self) -> str: + return self.name + + +class NoAuth(AuthProvider): + """Current behaviour, kept as an explicit choice rather than an absence. + + Naming it matters. "No authentication" as a configured provider appears in + the settings, can be logged, and can be seen to be wrong in a deployment + review. The same state as an unconfigured service, but visible. + """ + + name = "none" + + def authenticate(self, request: Request) -> Optional[str]: + return None + + +class BearerAuth(AuthProvider): + """A single shared token, presented as `Authorization: Bearer `. + + A shared secret is not identity, and this does not pretend to be: every + holder is the same caller. It is the smallest thing that stops an open + endpoint being open, and the step before F3. + """ + + name = "bearer" + + def __init__(self, token: str = None): + token = AUTH_TOKEN if token is None else token + # Fail at startup, not on the first request. A deployment that asked for + # bearer and supplied no token would otherwise start, look configured, + # and refuse every request -- or worse, if this compared against an empty + # string, admit anyone who sent an empty one. + if not token or not token.strip(): + raise ValueError( + "BIOCHEF_AUTH=bearer needs BIOCHEF_AUTH_TOKEN set to a " + "non-empty value. Refusing to start rather than run with a " + "token nobody has to guess." + ) + self._token = token + + def authenticate(self, request: Request) -> Optional[str]: + header = request.headers.get("authorization") + if not header: + raise Unauthenticated("no credentials were presented") + + scheme, _, presented = header.partition(" ") + if scheme.lower() != "bearer" or not presented: + raise Unauthenticated("expected an Authorization: Bearer ") + + # compare_digest, not ==. String comparison returns as soon as it finds a + # difference, so how long it takes leaks how much of the token was right, + # and a token can be recovered a character at a time. + if not hmac.compare_digest(presented, self._token): + raise Unauthenticated("the token presented is not the one configured") + + return "bearer-token" + + +class AuthenticationMiddleware: + """Refuse before the body is read, not after. + + A route dependency would be the obvious place, and it is the wrong one: + starlette parses and spools the whole multipart payload before the endpoint + is entered, so an anonymous caller would still have uploaded up to + BIOCHEF_MAX_UPLOAD_BYTES before anything asked who they were. The same + reason bodylimit.py is middleware. + + Headers are in the ASGI scope from the start, so this costs nothing and can + answer before a single byte of body is accepted. It must therefore sit + OUTSIDE the body limit -- added last, since starlette makes the last-added + middleware outermost. + """ + + def __init__(self, app, provider: AuthProvider): + self.app = app + self.provider = provider + + async def __call__(self, scope, receive, send): + if scope["type"] != "http": + await self.app(scope, receive, send) + return + + try: + self.provider.authenticate(Request(scope)) + except HTTPException as refusal: + body = json.dumps({"detail": refusal.detail}).encode() + headers = [(b"content-type", b"application/json"), + (b"content-length", str(len(body)).encode())] + for key, value in (refusal.headers or {}).items(): + headers.append((key.encode().lower(), value.encode())) + await send({"type": "http.response.start", + "status": refusal.status_code, "headers": headers}) + await send({"type": "http.response.body", "body": body}) + return + + await self.app(scope, receive, send) + + +PROVIDERS = { + NoAuth.name: NoAuth, + BearerAuth.name: BearerAuth, +} + + +def get_auth(name: str) -> AuthProvider: + """Resolve a provider by name, refusing to start on one that is not there. + + Deliberately the same shape as runner.get_runner. A name that is not a + provider must stop the process: silently falling back would mean a typo in + BIOCHEF_AUTH turns an authenticated deployment into an open one, which is + the single worst way for this setting to fail. + """ + try: + provider = PROVIDERS[name] + except KeyError: + raise ValueError( + f"BIOCHEF_AUTH={name!r} is not an authentication provider. " + f"Available: {', '.join(sorted(PROVIDERS))}." + ) from None + return provider() diff --git a/example.env b/example.env index 19850b5..d2466dc 100644 --- a/example.env +++ b/example.env @@ -26,3 +26,9 @@ BIOCHEF_RUN_TIMEOUT=900 BIOCHEF_KEEP_WORKSPACE=false BIOCHEF_MAX_UPLOAD_BYTES=536870912 BIOCHEF_TOOL_CACHE=tool-cache + +# Who may call this service: none (anyone who can reach it) or bearer. +# With bearer, requests need Authorization: Bearer . +# Selecting bearer without a token stops the service from starting. +BIOCHEF_AUTH=none +BIOCHEF_AUTH_TOKEN= diff --git a/main.py b/main.py index d0ec16c..6085d8d 100644 --- a/main.py +++ b/main.py @@ -11,6 +11,7 @@ import base64 from workspace import UnsafeName, check_name, make_workspace +from auth import AuthenticationMiddleware, NoAuth, get_auth from bodylimit import BodySizeLimitMiddleware, MAX_UPLOAD_BYTES from runner import SubprocessRunner, get_runner @@ -21,6 +22,18 @@ # refusing bytes that are already on disk (#11). app.add_middleware(BodySizeLimitMiddleware) +AUTH = get_auth(os.getenv("BIOCHEF_AUTH", NoAuth.name)) +"""Who may ask this service to run something. + +Resolved at import so a deployment naming a provider it does not have, or asking +for bearer without a token, fails to start rather than accepting work. +""" + +# Added last, so it is OUTERMOST and runs before the body limit -- and therefore +# before any of the body is accepted. An anonymous caller should not be able to +# make this service buffer half a gigabyte before being told no (#10). +app.add_middleware(AuthenticationMiddleware, provider=AUTH) + @app.exception_handler(UnsafeName) async def unusable_name(request, exc): diff --git a/tests/test_auth_provider.py b/tests/test_auth_provider.py new file mode 100644 index 0000000..51ee7fa --- /dev/null +++ b/tests/test_auth_provider.py @@ -0,0 +1,235 @@ +"""Who is allowed to make this service run a tool (#10). + +Before this, anyone who could reach it. There was no authentication of any kind +-- no dependency on /convert, no header read, no token compared, and no setting +that could switch one on. + +There is no authentication of any kind: no dependency on /convert, no header +read, no token compared, nothing in the settings that could switch one on. The +endpoint accepts a workflow, pulls binaries from a registry, executes them, and +returns their output, to any caller that can open a socket to it. + +That is a defensible default for something on a laptop. It is the wrong one for +a service whose reason to exist is dispatching work into a Trusted Research +Environment, where the whole point is that not everyone may ask. + +C2 asks for the interface plus two providers, so that F3 (Passports) is a third +provider rather than a rewrite. +""" + +import inspect +import sys +import types +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(REPO_ROOT)) + +if "oras" not in sys.modules: + oras = types.ModuleType("oras") + client_mod = types.ModuleType("oras.client") + + class _Client: + def __init__(self, *a, **k): + pass + + def login(self, *a, **k): + pass + + def pull(self, *a, **k): + raise AssertionError("a test reached the registry") + + client_mod.OrasClient = _Client + oras.client = client_mod + sys.modules["oras"] = oras + sys.modules["oras.client"] = client_mod + +import pytest + +import main + + +def test_the_service_resolves_a_provider_and_defaults_to_none(): + """Unchanged behaviour by default, but as a named choice rather than a gap. + + C2's acceptance is explicit that provider `none` leaves behaviour unchanged, + so the default must not start refusing anyone. + """ + from auth import NoAuth + + assert isinstance(main.AUTH, NoAuth) + assert "BIOCHEF_AUTH" in Path(REPO_ROOT / "main.py").read_text() + + +def test_a_request_with_no_credentials_is_served_under_none(monkeypatch): + """The other half of "unchanged": nothing is refused for lack of a token.""" + from fastapi.testclient import TestClient + + client = TestClient(main.app, raise_server_exceptions=False) + response = client.post( + "/convert", + data={"biochef_workflow": '{"nodes": [], "edges": []}'}, + files=[("files", ("input-1-out", b"x", "application/octet-stream"))], + ) + assert response.status_code not in (401, 403), response.status_code + + +# -------------------------------------------------------------------------- +# bearer + + +def _bearer_app(token="s3cret"): + from auth import AuthenticationMiddleware, BearerAuth + + return AuthenticationMiddleware(main.app, provider=BearerAuth(token=token)) + + +def test_a_request_without_a_token_is_refused(): + from fastapi.testclient import TestClient + + response = TestClient(_bearer_app(), raise_server_exceptions=False).post( + "/convert", data={"biochef_workflow": "{}"}, + files=[("files", ("input-1-out", b"x", "application/octet-stream"))]) + + assert response.status_code == 401 + # RFC 9110 makes this mandatory on a 401, and it is what tells a client + # which scheme to try. + assert response.headers.get("www-authenticate") == "Bearer" + + +def test_the_right_token_is_let_through(): + """The control. Refusing everything would pass every other test here.""" + from fastapi.testclient import TestClient + + response = TestClient(_bearer_app(), raise_server_exceptions=False).post( + "/convert", headers={"Authorization": "Bearer s3cret"}, + data={"biochef_workflow": "not json"}, + files=[("files", ("input-1-out", b"x", "application/octet-stream"))]) + + assert response.status_code != 401, response.text + + +@pytest.mark.parametrize("header", [ + "Bearer wrong", + "Bearer ", + "Bearer", + "Basic s3cret", + "s3cret", + "bearer wrong-case-but-wrong-token", + "Bearer s3cre", # a prefix of the real token + "Bearer s3cretx", # the real token with more after it +]) +def test_anything_other_than_the_configured_token_is_refused(header): + from fastapi.testclient import TestClient + + response = TestClient(_bearer_app(), raise_server_exceptions=False).post( + "/convert", headers={"Authorization": header}, + data={"biochef_workflow": "{}"}, + files=[("files", ("input-1-out", b"x", "application/octet-stream"))]) + + assert response.status_code == 401, f"{header!r} was accepted" + + +def test_a_lowercase_scheme_is_accepted(): + """The scheme is case-insensitive per the specification; the token is not.""" + from fastapi.testclient import TestClient + + response = TestClient(_bearer_app(), raise_server_exceptions=False).post( + "/convert", headers={"Authorization": "bearer s3cret"}, + data={"biochef_workflow": "not json"}, + files=[("files", ("input-1-out", b"x", "application/octet-stream"))]) + + assert response.status_code != 401 + + +def test_the_token_is_compared_in_constant_time(): + """Not ==. + + String comparison returns at the first difference, so how long it takes + leaks how much of the token was right, and a token can be recovered a + character at a time. Structural because timing assertions are flaky by + nature, and the property is "which function is called". + """ + import auth + + source = inspect.getsource(auth.BearerAuth.authenticate) + assert "hmac.compare_digest" in source + assert "== self._token" not in source + + +def test_bearer_without_a_token_refuses_to_start(): + """Not "starts and rejects everyone", and above all not "accepts everyone". + + An empty configured token compared against an empty presented one would + admit any caller who sent `Authorization: Bearer` with nothing after it. + """ + from auth import BearerAuth + + for empty in ("", " ", None): + with pytest.raises(ValueError, match="BIOCHEF_AUTH_TOKEN"): + BearerAuth(token=empty) if empty is not None else BearerAuth(token="") + + +def test_an_unknown_provider_stops_the_process(): + """A typo in BIOCHEF_AUTH must not quietly leave the service open. + + Falling back to `none` would be the single worst way for this setting to + fail: the deployment looks configured and is not. + """ + from auth import get_auth + + with pytest.raises(ValueError) as exc: + get_auth("passports") + assert "BIOCHEF_AUTH" in str(exc.value) + assert "none" in str(exc.value) and "bearer" in str(exc.value) + + +# -------------------------------------------------------------------------- +# where the check happens, which decides what an anonymous caller can cost + + +def test_an_anonymous_request_is_refused_before_its_body_is_read(): + """A route dependency would run AFTER starlette spooled the whole payload. + + So an unauthenticated caller could still make the service buffer up to + BIOCHEF_MAX_UPLOAD_BYTES before being told no. Headers are in the ASGI scope + from the start, so the refusal costs nothing -- provided it sits outside the + body handling, which is what this pins. + """ + from fastapi.testclient import TestClient + + consumed = {"chunks": 0} + + def body(): + boundary = "----authtest" + head = (f"--{boundary}\r\n" + 'Content-Disposition: form-data; name="files"; ' + 'filename="input-1-out"\r\n\r\n').encode() + yield head + consumed["chunks"] += 1 + for _ in range(64): + yield b"x" * 4096 + consumed["chunks"] += 1 + yield f"\r\n--{boundary}--\r\n".encode() + + response = TestClient(_bearer_app(), raise_server_exceptions=False).post( + "/convert", + headers={"Content-Type": "multipart/form-data; boundary=----authtest"}, + content=body()) + + assert response.status_code == 401 + assert consumed["chunks"] <= 1, ( + f"{consumed['chunks']} chunks were consumed before the refusal; the " + f"check is running after the body has been read" + ) + + +def test_authentication_wraps_the_body_limit_not_the_other_way_round(): + """Order is the whole point, so it is asserted rather than assumed.""" + names = [m.cls.__name__ for m in main.app.user_middleware if hasattr(m, "cls")] + assert "AuthenticationMiddleware" in names + assert "BodySizeLimitMiddleware" in names + # starlette applies user_middleware outermost-first in this list. + assert names.index("AuthenticationMiddleware") < names.index("BodySizeLimitMiddleware"), ( + f"middleware order is {names}; authentication must be outermost" + )