diff --git a/COMPATIBILITY.md b/COMPATIBILITY.md index 7f4c854e2..c6f938fc1 100644 --- a/COMPATIBILITY.md +++ b/COMPATIBILITY.md @@ -224,9 +224,9 @@ The changes: - Opt-in token authentication, with the gateway's variables: `SIE_AUTH_MODE` (`none`, `token`, `auto`) and `SIE_AUTH_TOKENS` or `SIE_AUTH_TOKEN`. A configured token turns it on. Clients send - `Authorization: Bearer ` or `x-api-key: `; when a request - sends both, `Authorization` is used and `x-api-key` is ignored. `/healthz`, - `/livez` and `/readyz` stay open. + `Authorization: Bearer ` or `x-api-key: `; when both + carry a token, `Authorization` is used and `x-api-key` is ignored. + `/healthz`, `/livez` and `/readyz` stay open. - In token mode without a configured token, the server generates one and saves it with mode `0600` in `SIE_DATA_DIR`. It prints the token once when stderr is a terminal, and otherwise only the file's path. A container with @@ -248,15 +248,18 @@ The changes: - Rust/Candle worker: logs a notice when it binds `0.0.0.0` only because `SIE_SERVER_HOST` is unset. - Gateway: - - Accepts `x-api-key` alongside `Authorization`. When a request sends both, - `Authorization` is used and `x-api-key` is ignored, so a request that - passes today still passes, including one where a proxy in front of the - gateway adds an `x-api-key` of its own. Repeated `x-api-key` values that - carry different tokens get `401`. - - A repeated `Authorization` header still uses its first value. The gateway - logs a warning, once per process, the first time a request that passes + - Accepts `x-api-key` alongside `Authorization`. The first + `Authorization` value decides when it carries a token, and `x-api-key` is + then ignored, so a request that passes today still passes, including one + where a proxy in front of the gateway adds an `x-api-key` of its own. + When that value is empty or blank, `x-api-key` decides. An unreadable + first value is refused, and `Bearer` alone counts as the token `Bearer`. + Repeated `x-api-key` values that carry different tokens get `401`. + - Later `Authorization` values are still ignored. The gateway logs a + warning, once per process, the first time a request that passes authentication repeats `Authorization` with values that carry different - tokens or with a value it cannot read. A refused request never logs it. + tokens, with a value it cannot read, or with a token only after a first + value that carries none. A refused request never logs it. - `SIE_WORKER_AUTH_TOKEN` is the token the gateway presents to workers. - Logs a warning when `SIE_AUTH_MODE` is unset on a non-loopback bind. - Telemetry: `SIE_TELEMETRY_ENABLED=1` opts in explicitly. The default stays on, @@ -273,7 +276,7 @@ The changes: | `sie-server` beyond loopback | No authentication | A token is required. Without `SIE_AUTH_TOKEN`, one is generated, and printed once on a terminal | `SIE_AUTH_MODE=none` | | `sie-server` on loopback without authentication | A request whose `Host` does not name this machine is answered, with a warning | It gets `400`. So does a request whose `Origin` names another site, at least for WebSocket upgrades and methods other than `GET` | Token authentication, or a setting for more allowed `Host` names, designed together with this change | | Rust/Candle worker bind | `0.0.0.0` | `127.0.0.1` | `SIE_SERVER_HOST=0.0.0.0` (the image sets it) | -| Gateway in token mode, repeated `Authorization` values that carry different tokens | The first value is used, with a warning | `401` | Send one `Authorization` header | +| Gateway in token mode, a repeated `Authorization` header whose values carry different tokens, include one it cannot read, or carry a token only after the first value | The later values are ignored; a request that passes authentication logs a warning, once per process | `401` | Send one `Authorization` header | | Gateway binary beyond loopback with `SIE_AUTH_MODE` unset | Starts without authentication | Refuses to start | `SIE_AUTH_MODE=none`, or `token` with `SIE_AUTH_TOKENS` | | Telemetry | On unless disabled | Off unless `SIE_TELEMETRY_ENABLED=1` | `SIE_TELEMETRY_ENABLED=1` | | Helm `gateway.auth.mode` | `none` | `token`, with a generated Secret (needs the chart change above) | `gateway.auth.mode=none` | diff --git a/SECURITY.md b/SECURITY.md index 2804106ca..c0564b632 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -45,7 +45,7 @@ What a default install exposes, and how to close it. with `SIE_AUTH_TOKEN` (or comma-separated `SIE_AUTH_TOKENS`). Without a configured token, the server generates one and keeps it in `SIE_DATA_DIR`. Clients send it as `Authorization: Bearer ` or `x-api-key: `; - when both are sent, `Authorization` is used. `/healthz`, `/livez` and + when both carry a token, `Authorization` is used. `/healthz`, `/livez` and `/readyz` stay open. Token authentication covers the HTTP port; the IPC socket and the engines' ports listen on this machine only. - Keep it on this machine with `--host 127.0.0.1`. In Docker, publish the port @@ -64,10 +64,15 @@ What a default install exposes, and how to close it. `SIE_AUTH_MODE=token` and `SIE_AUTH_TOKENS`. Mutations of pools, admin and config routes also need `SIE_ADMIN_TOKEN`. - Clients send a token as `Authorization: Bearer ` or - `x-api-key: `. When both are sent, `Authorization` is used and - `x-api-key` is ignored. Until 0.13.0, a repeated `Authorization` header uses - its first value and logs a warning; from 0.13.0, repeated values that carry - different tokens are refused. + `x-api-key: `. The first `Authorization` value decides when it + carries a token, and `x-api-key` is then ignored. When that value is empty + or blank, `x-api-key` decides. An unreadable first value is refused, and + `Bearer` alone counts as the token `Bearer`. Later `Authorization` values + are ignored. Until 0.13.0, a request that passes authentication with a + repeated `Authorization` header whose values carry different tokens, include + one the gateway cannot read, or carry a token only after the first value logs + a warning, once per process; a refused request never logs it. From 0.13.0 + such a request is refused. - `/healthz`, `/readyz`, `/openapi.json` and `/docs` are always reachable. `/`, `/health` and `/ws/*` require a token unless `SIE_AUTH_EXEMPT_OPERATIONAL=true`. diff --git a/deploy/helm/sie-cluster/README.md b/deploy/helm/sie-cluster/README.md index 9ff8048ae..364612ccf 100644 --- a/deploy/helm/sie-cluster/README.md +++ b/deploy/helm/sie-cluster/README.md @@ -1563,12 +1563,17 @@ An Ingress or a `LoadBalancer`/`NodePort` gateway Service also needs TLS; see With `gateway.auth.mode` set to `token` (or `static`, which the gateway treats the same), the gateway requires a token on every request except its probes and API description. Clients send it as `Authorization: Bearer ` or -`x-api-key: `. When a request sends both, the gateway uses -`Authorization` and ignores `x-api-key`; until 0.13.0, a repeated -`Authorization` header uses its first value. SDK clients pass the token as -`api_key`/`apiKey`, or read it from `SIE_API_KEY` when their base URL comes -from `SIE_BASE_URL`. The tokens come from one Secret, generated or managed by -the operator: +`x-api-key: `. The first `Authorization` value decides when it +carries a token, and the gateway then ignores `x-api-key`. When that value is +empty or blank, `x-api-key` decides. An unreadable first value is refused, and +`Bearer` alone counts as the token `Bearer`. Later `Authorization` values are +ignored. Until 0.13.0, a request that passes authentication with a repeated +`Authorization` header whose values carry different tokens, include one the +gateway cannot read, or carry a token only after the first value logs a +warning, once per process; a refused request never logs it. From 0.13.0 such a +request gets `401`. SDK clients pass the token as `api_key`/`apiKey`, or read +it from `SIE_API_KEY` when their base URL comes from `SIE_BASE_URL`. The tokens +come from one Secret, generated or managed by the operator: - **Generated.** With `gateway.auth.tokenSecretName` empty and `gateway.auth.generateToken=true`, the chart creates the Secret diff --git a/examples/coding-agent-claude-code/README.md b/examples/coding-agent-claude-code/README.md index a80cc8c3d..ef0fab381 100644 --- a/examples/coding-agent-claude-code/README.md +++ b/examples/coding-agent-claude-code/README.md @@ -128,7 +128,7 @@ export API_TIMEOUT_MS=1800000 `/v1/messages`. - `ANTHROPIC_AUTH_TOKEN` arrives as `Authorization: Bearer ...`, and Claude Code sends `ANTHROPIC_API_KEY` as `x-api-key`. The gateway and - `sie-server` read either one; when a request carries both, they use + `sie-server` read either one; when both carry a token, they use `Authorization` and ignore `x-api-key`. - The model id is sent byte for byte, slash and colon included. The four model variables keep every request, background tasks and subagents diff --git a/packages/sie_gateway/README.md b/packages/sie_gateway/README.md index 65a249da3..ecf28dd0b 100644 --- a/packages/sie_gateway/README.md +++ b/packages/sie_gateway/README.md @@ -93,7 +93,7 @@ Each `--flag` above has a matching `SIE_*` environment variable (see next sectio | `SIE_NATS_USER` | | NATS user (Helm sets `sie-gateway`). Set together with `SIE_NATS_PASSWORD`; setting only one fails startup | | `SIE_NATS_PASSWORD` | | NATS password of `SIE_NATS_USER`. Redacted in `Config` debug output | | `SIE_AUTH_MODE` | `none` | Auth mode for inbound requests: `none` disables, `token` (alias `static`) enforces. Values are case-sensitive. An unknown value, including `sie-server`'s `auto`, refuses every request with `500` except the probes and the API description (`/healthz`, `/readyz`, `/openapi.json`, `/docs`), and `main` logs a startup error naming the bad value. While it is unset on a non-loopback bind and the configuration is otherwise valid, `main` logs a warning: from 0.13.0 the gateway refuses to start that way | -| `SIE_AUTH_TOKENS` | | CSV of valid tokens for inference and pool/config read endpoints, presented as `Authorization: Bearer ` or `x-api-key: ` (when both are sent, `Authorization` is used and `x-api-key` is ignored). A repeated `Authorization` header uses its first value, as before, and logs a warning once; from 0.13.0 repeated values that carry different tokens get `401`. Repeated `x-api-key` values that carry different tokens get `401`. If unset, the singular `SIE_AUTH_TOKEN` is used as a fallback. When auth is enabled and this list is empty, non-probe requests return `500` | +| `SIE_AUTH_TOKENS` | | CSV of valid tokens for inference and pool/config read endpoints, presented as `Authorization: Bearer ` or `x-api-key: ` (the first `Authorization` value decides when it carries a token, and `x-api-key` is then ignored; when that value is empty or blank, `x-api-key` decides; an unreadable first value is refused, and `Bearer` alone counts as the token `Bearer`). Later `Authorization` values are ignored, as before. A request that passes authentication with a repeated `Authorization` header whose values carry different tokens, include one the gateway cannot read, or carry a token only after the first value logs a warning once per process (a refused request never logs it); from 0.13.0 such a request gets `401`. Repeated `x-api-key` values that carry different tokens get `401`. If unset, the singular `SIE_AUTH_TOKEN` is used as a fallback. When auth is enabled and this list is empty, non-probe requests return `500` | | `SIE_AUTH_TOKEN` | | Singular alias for `SIE_AUTH_TOKENS` (fallback only; prefer the plural form) | | `SIE_ADMIN_TOKEN` | | Admin bearer token the gateway requires inbound for admin-gated mutations: `POST/PUT/DELETE` on `/v1/configs/*`, `/v1/admin/*`, `/v1/pools/*`. If empty and an inbound request targets one of those paths, the middleware fails closed with `403`. Sent to `sie-config` only when `SIE_CONFIG_SERVICE_TOKEN` is unset (deprecated fallback) | | `SIE_CONFIG_SERVICE_TOKEN` | | Bearer token the gateway presents **as a client** on its `sie-config` reads (`/v1/configs/bundles`, `/export`, `/epoch`): `sie-config`'s read-scoped `SIE_CONFIG_READ_TOKEN`. When set, it decides and a blank value sends no token; when unset, the gateway falls back to `SIE_ADMIN_TOKEN` and logs a deprecation warning | diff --git a/packages/sie_gateway/docs/architecture-guide.md b/packages/sie_gateway/docs/architecture-guide.md index e8e93f3fc..2a385e1c0 100644 --- a/packages/sie_gateway/docs/architecture-guide.md +++ b/packages/sie_gateway/docs/architecture-guide.md @@ -516,7 +516,7 @@ Gateway (`sie-gateway`): - `SIE_CONFIG_SERVICE_TOKEN` — bearer token the gateway-as-client presents on its `sie-config` reads (`GET /v1/configs/bundles`, `/bundles/{id}`, `/export`, `/epoch`). It should be `sie-config`'s read-scoped token (`SIE_CONFIG_READ_TOKEN` there), which the chart generates and always wires in (empty when the chart runs no `sie-config`). When the variable is set it decides, and a blank value sends no `Authorization` header (so `sie-config` must either be unauthenticated or the gateway will fail with `401`/`403`). When it is unset, the gateway falls back to `SIE_ADMIN_TOKEN` and, when `SIE_CONFIG_SERVICE_URL` is set, logs a deprecation warning at startup; it logs the same warning when the two are equal. `sie-config` accepts one read token at a time and every component reads it at start, so a rotation restarts `sie-config` first and then the gateways and worker sidecars; until they restart, they receive `403` on config reads. - `SIE_ADMIN_TOKEN` — the token that `AuthLayer` requires for admin-gated mutations on the gateway itself (`POST/PUT/DELETE` on `/v1/configs/*`, `/v1/admin/*`, `/v1/pools/*`). If empty and the matching inbound request targets an admin path, the middleware fails closed with `403`. It is not sent to `sie-config` unless `SIE_CONFIG_SERVICE_TOKEN` is unset (the deprecated fallback above). The chart wires it only from `gateway.auth.adminTokenSecretName`, never from the `sie-config` admin token. - `SIE_AUTH_TOKEN` / `SIE_AUTH_TOKENS` — tokens accepted by the gateway's own inference API (`/v1/encode`, `/v1/score`, `/v1/extract`) and for read-side pool/config routes. Not used for outbound calls to `sie-config`. When `SIE_AUTH_MODE` enables auth but this list is empty, every non-probe request returns `500`. -- `SIE_AUTH_MODE` — `token` (alias: `static`) enforces auth; `none` (default) disables it. A typo refuses every request with `500` except the probes and the API description (`/healthz`, `/readyz`, `/openapi.json`, `/docs`), and `audit_auth` logs a startup error naming the bad value so operators see it in `kubectl logs`. Requests present a token as `Authorization: Bearer` or `x-api-key`; when both are sent, `Authorization` is used and `x-api-key` is ignored. Only the first `Authorization` value is read; repeated values that disagree log a warning once and are refused from 0.13.0. Repeated `x-api-key` values that disagree are refused. +- `SIE_AUTH_MODE` — `token` (alias: `static`) enforces auth; `none` (default) disables it. A typo refuses every request with `500` except the probes and the API description (`/healthz`, `/readyz`, `/openapi.json`, `/docs`), and `audit_auth` logs a startup error naming the bad value so operators see it in `kubectl logs`. Requests present a token as `Authorization: Bearer` or `x-api-key`. The first `Authorization` value decides when it carries a token, and `x-api-key` is then ignored. When that value is empty or blank, `x-api-key` decides. An unreadable first value is refused, and `Bearer` alone counts as the token `Bearer`. Later `Authorization` values are ignored; a request that passes authentication with a repeated `Authorization` header whose values carry different tokens, include one that cannot be read, or carry a token only after the first value logs a warning once per process (a refused request never logs it), and such a request is refused from 0.13.0. Repeated `x-api-key` values that disagree are refused. - `SIE_AUTH_EXEMPT_OPERATIONAL` — when `true`, `/`, `/health`, and `/ws/*` are exempt from auth (they expose worker URLs, bundle assignments, queue depth, GPU inventory — treat as sensitive). Default `false`. **`/healthz` and `/readyz` are always exempt from auth** so kubelet probes never fail with **`401`/`403`** because of a missing bearer token. `/readyz` reports only process readiness and, when `SIE_CONFIG_SERVICE_URL` is set, first-bootstrap completion; worker health remains visible through `/health` and inference responses. - `SIE_NATS_CONFIG_TRUSTED_PRODUCERS` — comma-separated producer allowlist for `sie.config.models._all`. Binary default is `sie-config`; Helm sets the release-scoped config Deployment name. Matching is exact OR K8s pod-name prefix (see §4.3). Untrusted notifications are dropped; the `config_poller` still closes the gap. - `SIE_NATS_CONFIG_TRUST_ANY_PRODUCER` — `true` disables producer validation entirely. Intended for local/dev. `main.rs` emits a startup warning when on. diff --git a/packages/sie_gateway/openapi.json b/packages/sie_gateway/openapi.json index f8fb3a972..70418f705 100644 --- a/packages/sie_gateway/openapi.json +++ b/packages/sie_gateway/openapi.json @@ -4688,7 +4688,7 @@ }, "securitySchemes": { "apiKeyAuth": { - "description": "The same SIE token as bearerAuth, sent in the x-api-key header. When a request sends both, the Authorization header is used and x-api-key is ignored.", + "description": "The same SIE token as bearerAuth, sent in the x-api-key header. When the first Authorization value carries a token, that token is used and x-api-key is ignored.", "in": "header", "name": "x-api-key", "type": "apiKey" diff --git a/packages/sie_gateway/src/middleware/auth.rs b/packages/sie_gateway/src/middleware/auth.rs index 1ab6c77d8..850d1465f 100644 --- a/packages/sie_gateway/src/middleware/auth.rs +++ b/packages/sie_gateway/src/middleware/auth.rs @@ -303,10 +303,11 @@ pub enum RequestCredential { /// Read the request's token. The first `Authorization` value (`Bearer /// ` or a raw value, as [`extract_bearer_token`]) decides when it -/// carries a token, and `x-api-key` is then ignored. Otherwise `x-api-key` -/// decides, and its repeated values must carry the same token. Later -/// `Authorization` values are ignored, as they always were; see -/// [`authorization_values_disagree`]. +/// carries a token, and `x-api-key` is then ignored. A first value that +/// cannot be read is refused, and `Bearer` alone is read as the raw token +/// `Bearer`. When the first value is empty or blank, `x-api-key` decides, and +/// its repeated values must carry the same token. Later `Authorization` values +/// are ignored, as they always were; see [`authorization_values_disagree`]. pub fn extract_request_credential(headers: &axum::http::HeaderMap) -> RequestCredential { let authorization = match headers.get("authorization").map(|value| value.to_str()) { None => RequestCredential::Missing, @@ -346,14 +347,17 @@ fn header_credential<'a>( } /// Whether the request repeats `Authorization` with values that carry -/// different tokens, or one that cannot be read. Until 0.13.0 the gateway -/// ignores the later values; from 0.13.0 such a request is refused. +/// different tokens, with one that cannot be read, or with a token only after +/// a first value that carries none. Until 0.13.0 the gateway ignores the later +/// values; from 0.13.0 such a request is refused. pub fn authorization_values_disagree(headers: &axum::http::HeaderMap) -> bool { - headers.get_all("authorization").iter().nth(1).is_some() - && matches!( - header_credential(headers.get_all("authorization"), authorization_token), - RequestCredential::Conflicting | RequestCredential::Unreadable - ) + let values = headers.get_all("authorization"); + values.iter().nth(1).is_some() + && match header_credential(values, authorization_token) { + RequestCredential::Conflicting | RequestCredential::Unreadable => true, + RequestCredential::Token(_) => extract_bearer_token(headers).is_none(), + RequestCredential::Missing => false, + } } /// The masked request token for audit records, empty when the request @@ -1027,6 +1031,18 @@ mod tests { extract_request_credential(&unreadable_first), RequestCredential::Unreadable ); + unreadable_first.append("x-api-key", axum::http::HeaderValue::from_static("tok")); + assert_eq!( + extract_request_credential(&unreadable_first), + RequestCredential::Unreadable + ); + assert_eq!( + extract_request_credential(&headers(&[ + ("authorization", "Bearer "), + ("x-api-key", "tok") + ])), + RequestCredential::Token("Bearer".into()) + ); } #[test] @@ -1037,6 +1053,13 @@ mod tests { ("authorization", "Bearer other"), ], vec![("authorization", "Bearer tok"), ("authorization", "other")], + vec![("authorization", ""), ("authorization", "Bearer tok")], + vec![ + ("authorization", " "), + ("authorization", "tok"), + ("authorization", "Bearer tok"), + ("x-api-key", "other"), + ], ] { assert!(authorization_values_disagree(&headers(&pairs)), "{pairs:?}"); } @@ -1048,6 +1071,7 @@ mod tests { vec![("authorization", "Bearer tok")], vec![("authorization", "Bearer tok"), ("authorization", "tok")], vec![("authorization", "Bearer tok"), ("authorization", "")], + vec![("authorization", ""), ("authorization", " ")], vec![("x-api-key", "tok"), ("x-api-key", "other")], ] { assert!( @@ -1308,7 +1332,7 @@ mod tests { fn repeated_authorization_after_an_empty_one(x_api_keys: &[&str]) -> axum::http::HeaderMap { let mut pairs = vec![ ("authorization", ""), - ("authorization", "Bearer user"), + ("authorization", "Bearer later"), ("authorization", "Bearer bogus"), ]; pairs.extend(x_api_keys.iter().map(|key| ("x-api-key", *key))); @@ -1421,6 +1445,107 @@ mod tests { } } + #[tokio::test] + async fn middleware_lets_x_api_key_decide_after_an_empty_authorization_and_warns() { + use tracing::instrument::WithSubscriber; + + let cfg = cfg_for_middleware( + "token", + vec!["user-token-aaaa", "key-token-bbbb"], + "admin-token-zzzz", + false, + ); + for (path, pairs, expected, token, warnings) in [ + ( + "/v1/encode/any", + vec![ + ("authorization", ""), + ("authorization", "Bearer user-token-aaaa"), + ("x-api-key", "key-token-bbbb"), + ], + StatusCode::OK, + Some("key-token-bbbb"), + 1, + ), + ( + "/v1/encode/any", + vec![ + ("authorization", " "), + ("authorization", "Bearer unknown-cccc"), + ("x-api-key", "key-token-bbbb"), + ], + StatusCode::OK, + Some("key-token-bbbb"), + 1, + ), + ( + "/v1/pools", + vec![ + ("authorization", ""), + ("authorization", "Bearer user-token-aaaa"), + ("x-api-key", "admin-token-zzzz"), + ], + StatusCode::OK, + Some("admin-token-zzzz"), + 1, + ), + ( + "/v1/pools", + vec![ + ("authorization", ""), + ("authorization", "Bearer admin-token-zzzz"), + ("x-api-key", "user-token-aaaa"), + ], + StatusCode::FORBIDDEN, + Some("user-token-aaaa"), + 0, + ), + ( + "/v1/encode/any", + vec![ + ("authorization", ""), + ("authorization", "Bearer user-token-aaaa"), + ], + StatusCode::UNAUTHORIZED, + None, + 0, + ), + ] { + let request_headers = headers(&pairs); + assert_eq!( + request_token_id(&request_headers), + token.map(mask_token).unwrap_or_default(), + "{pairs:?}" + ); + let log = CapturedLog::default(); + let dispatch = tracing::Dispatch::new( + tracing_subscriber::fmt() + .with_writer(log.clone()) + .with_ansi(false) + .finish(), + ); + let mut request = HttpRequest::builder() + .method(Method::POST) + .uri(path) + .body(Body::empty()) + .unwrap(); + *request.headers_mut() = request_headers; + + let status = test_router(Arc::clone(&cfg)) + .oneshot(request) + .with_subscriber(dispatch) + .await + .unwrap() + .status(); + assert_eq!(status, expected, "{pairs:?}"); + assert_eq!( + log.count("The gateway ignored the later Authorization values"), + warnings, + "{pairs:?}" + ); + } + } + #[tokio::test] async fn middleware_refuses_repeated_x_api_key_that_disagrees() { let cfg = cfg_for_middleware("token", vec!["user", "other"], "admin", false); diff --git a/packages/sie_gateway/src/openapi.rs b/packages/sie_gateway/src/openapi.rs index 40d5e01df..4a786c25b 100644 --- a/packages/sie_gateway/src/openapi.rs +++ b/packages/sie_gateway/src/openapi.rs @@ -301,7 +301,7 @@ fn apply_gateway_openapi_overrides(value: &mut Value) { "type": "apiKey", "in": "header", "name": "x-api-key", - "description": "The same SIE token as bearerAuth, sent in the x-api-key header. When a request sends both, the Authorization header is used and x-api-key is ignored." + "description": "The same SIE token as bearerAuth, sent in the x-api-key header. When the first Authorization value carries a token, that token is used and x-api-key is ignored." } }); @@ -3587,7 +3587,7 @@ mod tests { "type": "apiKey", "in": "header", "name": "x-api-key", - "description": "The same SIE token as bearerAuth, sent in the x-api-key header. When a request sends both, the Authorization header is used and x-api-key is ignored." + "description": "The same SIE token as bearerAuth, sent in the x-api-key header. When the first Authorization value carries a token, that token is used and x-api-key is ignored." }) ); assert_eq!(spec["paths"]["/healthz"]["get"]["security"], json!([])); diff --git a/packages/sie_server/README.md b/packages/sie_server/README.md index 30e753116..67991e6ab 100644 --- a/packages/sie_server/README.md +++ b/packages/sie_server/README.md @@ -1083,12 +1083,12 @@ as a profile an operator chooses. | `SIE_DATA_DIR` | `$XDG_DATA_HOME/sie`, else `~/.local/share/sie` | Holds the generated token (`auth-token`, mode `0600`) and the telemetry worker id. Images set `/app/.cache/sie`. | Clients send the token as `Authorization: Bearer ` or `x-api-key: `; -when both are sent, `Authorization` is used and `x-api-key` is ignored, as on the -gateway. The SDKs send `SIE_API_KEY` +when both carry a token, `Authorization` is used and `x-api-key` is ignored, as on +the gateway. The SDKs send `SIE_API_KEY` as a Bearer token. Repeated headers differ from the gateway until 0.13.0: the server reads every value of a repeated header, skips empty ones, and refuses -values that carry different tokens, while the gateway uses only the first -`Authorization` value. +values that carry different tokens, while the gateway reads only the first +`Authorization` value and uses `x-api-key` when that value is empty or blank. Token auth covers the HTTP port. The other listeners are local: the IPC socket that a worker sidecar uses (`SIE_IPC_SOCKET_PATH`, mode `0600`) and the @@ -1106,7 +1106,10 @@ and the container runtime keeps that one print in the container's logs. In containers, set `SIE_AUTH_TOKEN` yourself instead. The server refuses a token directory that another user owns or can write to, and a token file that is a symbolic link, not a regular file, hard-linked, owned by another user, or -readable by others. To rotate a generated token, delete `auth-token` from +readable by others. On macOS it also refuses a token directory or file whose +access control list has an entry that allows access (`ls -le` lists the +entries); deny entries, such as the one macOS puts on the home folder, are +accepted. To rotate a generated token, delete `auth-token` from `SIE_DATA_DIR` and restart the server. In Docker, keep the token across containers with a volume: diff --git a/packages/sie_server/src/sie_server/core/server_auth.py b/packages/sie_server/src/sie_server/core/server_auth.py index 95456db95..958b4b5e4 100644 --- a/packages/sie_server/src/sie_server/core/server_auth.py +++ b/packages/sie_server/src/sie_server/core/server_auth.py @@ -3,15 +3,18 @@ One vocabulary with the gateway: ``SIE_AUTH_MODE`` selects the mode, ``SIE_AUTH_TOKENS`` (or ``SIE_AUTH_TOKEN``) holds comma-separated tokens, and a request presents one in ``Authorization: Bearer`` or ``x-api-key``, with -``Authorization`` taking precedence when both are sent. Repeated headers are -read differently until 0.13.0: the gateway uses only the first -``Authorization`` value, while this server reads every value, skips empty ones -and refuses values that carry different tokens. +``Authorization`` taking precedence when both carry a token. Repeated headers +are read differently until 0.13.0: the gateway reads only the first +``Authorization`` value and uses ``x-api-key`` when that value is empty or +blank, while this server reads every value, skips empty ones and refuses values that +carry different tokens. """ from __future__ import annotations +import ctypes import errno +import functools import hmac import ipaddress import logging @@ -19,6 +22,7 @@ import re import secrets import stat +import sys from collections.abc import Iterable, Mapping, Sequence from dataclasses import dataclass from pathlib import Path @@ -27,7 +31,13 @@ import orjson from starlette.types import ASGIApp, Receive, Scope, Send -from sie_server.core.data_dir import ensure_data_dir, owned_files, remove_stale_files, resolve_data_dir +from sie_server.core.data_dir import ( + DATA_DIR_ENV, + ensure_data_dir, + owned_files, + remove_stale_files, + resolve_data_dir, +) logger = logging.getLogger(__name__) @@ -49,6 +59,11 @@ _TEMP_PREFIX = f".{TOKEN_FILE_NAME}.sie-server-" _TEMP_NAME = re.compile(rf"{re.escape(_TEMP_PREFIX)}[0-9a-f]{{16}}") _READ_ONLY_ERRNOS = frozenset({errno.EACCES, errno.EROFS}) +_NO_ACL_ERRNOS = frozenset({errno.ENOENT, errno.ENOTSUP, errno.EOPNOTSUPP}) +_ACL_TYPE_EXTENDED = 0x100 +_ACL_FIRST_ENTRY = 0 +_ACL_NEXT_ENTRY = -1 +_ACL_EXTENDED_DENY = 2 _HOST_CHECK_RELEASE = "0.13.0" _MAX_WARNED_HOSTS = 64 _MAX_LOGGED_HOST_CHARS = 100 @@ -215,17 +230,68 @@ def load_or_create_token(directory: Path) -> StoredToken: def check_private_directory(directory: Path) -> None: - """Refuse a token directory that another user owns or can write to.""" + """Refuse a token directory that another user owns or can write to, including through a macOS access control list.""" try: - info = directory.stat() + fd = os.open(directory, os.O_RDONLY | os.O_DIRECTORY) + except NotADirectoryError as exc: + raise TokenFileError(f"{directory} must be a directory.") from exc except OSError as exc: raise TokenFileError(f"Cannot read {directory}: {exc.strerror or exc}") from exc - if not stat.S_ISDIR(info.st_mode): - raise TokenFileError(f"{directory} must be a directory.") - if info.st_uid != os.getuid(): - raise TokenFileError(f"{directory} must be owned by the user that runs sie-server.") - if stat.S_IMODE(info.st_mode) & 0o022: - raise TokenFileError(f"{directory} must not be writable by others (chmod 700 {directory}).") + try: + info = os.fstat(fd) + if info.st_uid != os.getuid(): + raise TokenFileError(f"{directory} must be owned by the user that runs sie-server.") + if stat.S_IMODE(info.st_mode) & 0o022: + raise TokenFileError(f"{directory} must not be writable by others (chmod 700 {directory}).") + if _acl_allows_access(fd, directory): + raise TokenFileError( + f"{directory} has an access control list entry that allows access, which its mode does not show. " + f"List the entries with ls -led {directory} and remove each allow entry with chmod -a# " + f"{directory}, or set {DATA_DIR_ENV} to a directory of its own." + ) + finally: + os.close(fd) + + +@functools.cache +def _libsystem() -> ctypes.CDLL: + libsystem = ctypes.CDLL(None, use_errno=True) + libsystem.acl_get_fd_np.restype = ctypes.c_void_p + libsystem.acl_get_fd_np.argtypes = (ctypes.c_int, ctypes.c_int) + libsystem.acl_get_entry.argtypes = (ctypes.c_void_p, ctypes.c_int, ctypes.POINTER(ctypes.c_void_p)) + libsystem.acl_get_tag_type.argtypes = (ctypes.c_void_p, ctypes.POINTER(ctypes.c_int)) + libsystem.acl_free.argtypes = (ctypes.c_void_p,) + return libsystem + + +def _acl_allows_access(fd: int, target: Path) -> bool: + """Whether the macOS access control list of ``fd`` has an entry other than a deny entry. + + Such an entry can grant access that the mode bits do not show. Deny entries, such as the one macOS puts on a + home folder, only take access away. An entry whose kind cannot be read counts as allowing access, and a list + that cannot be read is refused. + """ + if sys.platform != "darwin": + return False + libsystem = _libsystem() + ctypes.set_errno(0) + acl = libsystem.acl_get_fd_np(fd, _ACL_TYPE_EXTENDED) + if not acl: + code = ctypes.get_errno() + if code in _NO_ACL_ERRNOS: + return False + raise TokenFileError(f"Cannot read the access control list of {target}: {os.strerror(code)}") + try: + entry = ctypes.c_void_p() + tag = ctypes.c_int() + position = _ACL_FIRST_ENTRY + while libsystem.acl_get_entry(acl, position, ctypes.byref(entry)) == 0: + position = _ACL_NEXT_ENTRY + if libsystem.acl_get_tag_type(entry, ctypes.byref(tag)) != 0 or tag.value != _ACL_EXTENDED_DENY: + return True + return False + finally: + libsystem.acl_free(acl) def read_stored_token(directory: Path) -> str: @@ -251,6 +317,11 @@ def read_stored_token(directory: Path) -> str: raise TokenFileError(f"{path} must be owned by the user that runs sie-server.") if stat.S_IMODE(info.st_mode) & 0o077: raise TokenFileError(f"{path} must not be readable or writable by others (chmod 600 {path}).") + if _acl_allows_access(fd, path): + raise TokenFileError( + f"{path} has an access control list entry that allows access, so others may have read the token. " + f"Delete {path} to have sie-server generate a new one, or remove the entries with chmod -N {path}." + ) raw = os.read(fd, _MAX_TOKEN_FILE_BYTES + 1) finally: os.close(fd) @@ -271,38 +342,50 @@ def _drop_temporary_links(path: Path, fd: int, token: os.stat_result) -> bool: True when every extra hard link of the token is such a file, even if a read-only directory keeps them in place; any other link is left alone and the token is refused. The token's link count is read before and after each - scan: one link after the scan means the temporary links are gone, and an - unchanged count equal to the temporary links found plus one means they are - the only extra links. A count that changes during the scan means a - concurrent start is still removing its link, so the scan is repeated a - few times before the token is refused. + scan, and one link after the scan means the temporary links are gone. An + unchanged count equal to the temporary links found plus one means they may + be the only extra links, so they are removed and the count is read again. + It must then be one, because a link renamed, added or removed while the + directory is listed can balance the count; only when a read-only directory + keeps every one of them in place does the balanced count decide. A count + that changes during the scan means a concurrent start is still removing + its link, so the scan is repeated a few times before the token is refused. """ before = token.st_nlink for _ in range(_LINK_SCAN_ATTEMPTS): try: - linked = [ - entry + linked = { + entry.name: entry for entry, info in owned_files(path.parent, _TEMP_NAME) - if (info.st_dev, info.st_ino) == (token.st_dev, token.st_ino) - ] + if os.path.samestat(info, token) + } except OSError: return False after = os.fstat(fd).st_nlink if after == 1: return True - if after == before == len({entry.name for entry in linked}) + 1: - for entry in linked: - try: - entry.unlink(missing_ok=True) - except OSError: - continue - return True + if after == before == len(linked) + 1: + kept = [_unlink_token_link(entry, token) for entry in linked.values()] + remaining = os.fstat(fd).st_nlink + return remaining == 1 or (remaining == after and all(code in _READ_ONLY_ERRNOS for code in kept)) if after == before: return False before = after return False +def _unlink_token_link(entry: Path, token: os.stat_result) -> int | None: + """Unlink ``entry`` if it is still a link of the token; the error number that kept it in place, else ``None``.""" + try: + if os.path.samestat(entry.lstat(), token): + entry.unlink() + except FileNotFoundError: + return None + except OSError as exc: + return exc.errno + return None + + def server_tokens(env: Mapping[str, str]) -> tuple[str, ...]: """The tokens a server in ``token`` mode accepts: the configured ones, else the persisted one.""" tokens = configured_tokens(env) @@ -322,8 +405,9 @@ def request_credentials(headers: Iterable[tuple[bytes, bytes]]) -> set[bytes]: ignored. Otherwise the ``x-api-key`` values count. Every value of a repeated header is read and empty values are ignored, so an empty ``Authorization`` followed by a valid one is accepted here, while the - gateway reads only the first ``Authorization`` value until 0.13.0. More - than one distinct token means repeated headers that disagree. + gateway ignores the later value and uses ``x-api-key`` or refuses the + request. More than one distinct token means repeated headers that + disagree. """ authorization: set[bytes] = set() api_key: set[bytes] = set() diff --git a/packages/sie_server/tests/core/test_server_auth.py b/packages/sie_server/tests/core/test_server_auth.py index 3311f0e0a..c25524ab8 100644 --- a/packages/sie_server/tests/core/test_server_auth.py +++ b/packages/sie_server/tests/core/test_server_auth.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +import ctypes import errno import logging import os @@ -44,6 +45,7 @@ TOKEN = "s3cret-token-value" # noqa: S105 - test credential OTHER = "another-token-value" +_MACOS_ACL = pytest.mark.skipif(sys.platform != "darwin", reason="macOS access control lists, set with chmod +a") @pytest.fixture(autouse=True) @@ -229,7 +231,7 @@ def test_a_concurrent_start_that_removes_its_temporary_link_mid_read_is_accepted def fstat_then_the_other_start_unlinks(fd: int) -> os.stat_result: info = real_fstat(fd) - if not seen: + if not seen and stat.S_ISREG(info.st_mode): seen.append(info.st_nlink) staged.unlink() return info @@ -310,6 +312,88 @@ def scan_listing_each_entry_twice(directory: Path, name: re.Pattern[str]) -> Ite assert read_stored_token(tmp_path) == stored.token assert not staged.exists() + def test_a_temporary_link_listed_twice_in_a_directory_without_write_permission_counts_once( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + directory = tmp_path / "sie" + stored = load_or_create_token(directory) + os.link(stored.path, tmp_path / "elsewhere") + os.link(stored.path, directory / ".auth-token.sie-server-0123456789abcdef") + + def scan_listing_each_entry_twice(directory: Path, name: re.Pattern[str]) -> Iterator[Any]: + for found in list(owned_files(directory, name)): + yield found + yield found + + monkeypatch.setattr(server_auth, "owned_files", scan_listing_each_entry_twice) + directory.chmod(0o500) + try: + with pytest.raises(TokenFileError, match="hard links"): + read_stored_token(directory) + finally: + directory.chmod(0o700) + + def test_a_temporary_link_renamed_during_the_scan_does_not_hide_a_foreign_link( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + stored = load_or_create_token(tmp_path) + os.link(stored.path, tmp_path / "copy") + staged = tmp_path / ".auth-token.sie-server-aaaaaaaaaaaaaaaa" + renamed = tmp_path / ".auth-token.sie-server-bbbbbbbbbbbbbbbb" + os.link(stored.path, staged) + + def scan_while_the_link_is_renamed(directory: Path, name: re.Pattern[str]) -> Iterator[Any]: + for entry, info in list(owned_files(directory, name)): + yield entry, info + if entry == staged: + staged.rename(renamed) + yield renamed, renamed.lstat() + + monkeypatch.setattr(server_auth, "owned_files", scan_while_the_link_is_renamed) + + with pytest.raises(TokenFileError, match="hard links"): + read_stored_token(tmp_path) + assert (tmp_path / "copy").exists() + + def test_a_temporary_link_added_before_and_removed_after_the_listing_does_not_hide_a_foreign_link( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + stored = load_or_create_token(tmp_path) + os.link(stored.path, tmp_path / "copy") + staged = tmp_path / ".auth-token.sie-server-cccccccccccccccc" + + def scan_around_a_passing_link(directory: Path, name: re.Pattern[str]) -> Iterator[Any]: + os.link(stored.path, staged) + found = list(owned_files(directory, name)) + staged.unlink() + yield from found + + monkeypatch.setattr(server_auth, "owned_files", scan_around_a_passing_link) + + with pytest.raises(TokenFileError, match="hard links"): + read_stored_token(tmp_path) + assert (tmp_path / "copy").exists() + + def test_a_temporary_name_that_no_longer_holds_the_token_is_not_removed( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + stored = load_or_create_token(tmp_path) + staged = tmp_path / ".auth-token.sie-server-eeeeeeeeeeeeeeee" + os.link(stored.path, staged) + + def scan_then_the_name_is_replaced(directory: Path, name: re.Pattern[str]) -> Iterator[Any]: + found = list(owned_files(directory, name)) + staged.unlink() + staged.write_text("unrelated\n") + os.link(stored.path, tmp_path / ".auth-token.sie-server-ffffffffffffffff") + yield from found + + monkeypatch.setattr(server_auth, "owned_files", scan_then_the_name_is_replaced) + + with pytest.raises(TokenFileError, match="hard links"): + read_stored_token(tmp_path) + assert staged.read_text() == "unrelated\n" + def test_a_symbolic_link_named_like_a_temporary_file_does_not_hide_a_foreign_link(self, tmp_path: Path) -> None: stored = load_or_create_token(tmp_path) os.link(stored.path, tmp_path / "copy") @@ -331,6 +415,55 @@ def test_a_temporary_link_in_a_directory_without_write_permission_is_tolerated(s finally: directory.chmod(0o700) + def test_a_temporary_link_that_a_writable_directory_cannot_remove_is_refused( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + stored = load_or_create_token(tmp_path) + staged = tmp_path / ".auth-token.sie-server-0123456789abcdef" + os.link(stored.path, staged) + _refuse_unlink(staged, monkeypatch, errno.EPERM) + + with pytest.raises(TokenFileError, match="hard links"): + read_stored_token(tmp_path) + assert staged.exists() + + def test_a_read_only_directory_does_not_excuse_a_link_kept_by_another_error( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + directory = tmp_path / "sie" + stored = load_or_create_token(directory) + immutable = directory / ".auth-token.sie-server-0000000000000001" + os.link(stored.path, immutable) + os.link(stored.path, directory / ".auth-token.sie-server-0000000000000002") + _refuse_unlink(immutable, monkeypatch, errno.EPERM) + directory.chmod(0o500) + try: + with pytest.raises(TokenFileError, match="hard links"): + read_stored_token(directory) + finally: + directory.chmod(0o700) + + def test_a_link_added_elsewhere_while_a_read_only_directory_is_cleaned_is_refused( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + directory = tmp_path / "sie" + stored = load_or_create_token(directory) + os.link(stored.path, directory / ".auth-token.sie-server-0123456789abcdef") + unlink_token_link = server_auth._unlink_token_link + + def unlink_then_a_link_appears_elsewhere(entry: Path, token: os.stat_result) -> int | None: + code = unlink_token_link(entry, token) + os.link(stored.path, tmp_path / "elsewhere") + return code + + monkeypatch.setattr(server_auth, "_unlink_token_link", unlink_then_a_link_appears_elsewhere) + directory.chmod(0o500) + try: + with pytest.raises(TokenFileError, match="hard links"): + read_stored_token(directory) + finally: + directory.chmod(0o700) + @pytest.mark.parametrize("name", [".auth-token.bak", ".auth-token.0123456789abcdef"]) def test_a_users_link_named_like_a_temporary_file_is_kept_and_refused(self, tmp_path: Path, name: str) -> None: stored = load_or_create_token(tmp_path) @@ -419,6 +552,79 @@ def test_a_directory_others_can_write_is_refused(self, tmp_path: Path, mode: int load_or_create_token(directory) assert not token_path(directory).exists() + @_MACOS_ACL + def test_a_directory_with_an_access_control_list_entry_that_allows_access_is_refused(self, tmp_path: Path) -> None: + existing = tmp_path / "existing" + load_or_create_token(existing) + empty = tmp_path / "empty" + empty.mkdir(mode=0o700) + for directory in (existing, empty): + _add_acl_entry(directory, "everyone deny delete") + _add_acl_entry(directory, "everyone allow add_file,add_subdirectory,delete_child") + + with pytest.raises(TokenFileError, match=re.escape(f"chmod -a# {existing}, or set SIE_DATA_DIR")): + read_stored_token(existing) + with pytest.raises(TokenFileError, match="entry that allows access"): + load_or_create_token(empty) + assert not token_path(empty).exists() + + @_MACOS_ACL + def test_deny_entries_like_the_home_folder_default_are_accepted(self, tmp_path: Path) -> None: + directory = tmp_path / "home" + directory.mkdir(mode=0o700) + _add_acl_entry(directory, "everyone deny delete") + + stored = load_or_create_token(directory) + _add_acl_entry(stored.path, "everyone deny delete") + + assert stored.created + assert read_stored_token(directory) == stored.token + + @_MACOS_ACL + def test_a_token_file_with_an_access_control_list_entry_that_allows_access_is_refused(self, tmp_path: Path) -> None: + stored = load_or_create_token(tmp_path) + _add_acl_entry(stored.path, "everyone allow read") + + with pytest.raises(TokenFileError, match=re.escape(f"Delete {stored.path} to have sie-server generate")): + read_stored_token(tmp_path) + + @_MACOS_ACL + def test_a_token_file_that_inherited_an_entry_is_refused_after_the_directory_is_cleared( + self, tmp_path: Path + ) -> None: + directory = tmp_path / "sie" + directory.mkdir(mode=0o700) + _add_acl_entry(directory, "everyone allow read,file_inherit") + token_path(directory).write_text(f"{TOKEN}\n") + token_path(directory).chmod(0o600) + subprocess.run(["/bin/chmod", "-N", str(directory)], check=True) # noqa: S603 + + with pytest.raises(TokenFileError, match=re.escape(f"chmod -N {token_path(directory)}")): + read_stored_token(directory) + + @_MACOS_ACL + def test_an_access_control_list_that_cannot_be_read_is_refused( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + def failing_read(fd: int, kind: int) -> None: + ctypes.set_errno(errno.EIO) + + monkeypatch.setattr(server_auth._libsystem(), "acl_get_fd_np", failing_read) + + with pytest.raises(TokenFileError, match="Cannot read the access control list"): + load_or_create_token(tmp_path) + assert not token_path(tmp_path).exists() + + @_MACOS_ACL + def test_an_access_control_list_entry_of_unknown_kind_counts_as_allowing_access( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + _add_acl_entry(tmp_path, "everyone deny delete") + monkeypatch.setattr(server_auth._libsystem(), "acl_get_tag_type", lambda entry, tag: -1) + + with pytest.raises(TokenFileError, match="entry that allows access"): + load_or_create_token(tmp_path) + def test_a_directory_of_another_user_is_refused(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: load_or_create_token(tmp_path) monkeypatch.setattr(os, "getuid", lambda: tmp_path.stat().st_uid + 1) @@ -907,6 +1113,22 @@ def refusing_open(path: Any, flags: int, *args: Any, **kwargs: Any) -> int: monkeypatch.setattr(os, "open", refusing_open) +def _refuse_unlink(target: Path, monkeypatch: pytest.MonkeyPatch, code: int) -> None: + """Make unlinking ``target`` fail with ``code``, as an immutable file makes it fail with EPERM.""" + real_unlink = Path.unlink + + def refusing_unlink(self: Path, missing_ok: bool = False) -> None: + if self == target: + raise OSError(code, os.strerror(code), str(self)) + real_unlink(self, missing_ok=missing_ok) + + monkeypatch.setattr(Path, "unlink", refusing_unlink) + + +def _add_acl_entry(path: Path, entry: str) -> None: + subprocess.run(["/bin/chmod", "+a", entry, str(path)], check=True) # noqa: S603 + + def _capture(call: Callable[[], object], outcome: list[BaseException]) -> None: try: call()