diff --git a/Cargo.lock b/Cargo.lock index eae96fd..d9ef5a6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -236,6 +236,7 @@ dependencies = [ "chrono", "deadpool-postgres", "flate2", + "futures-util", "gcp_auth", "http-body-util", "hyper", diff --git a/crates/aster-srv/Cargo.toml b/crates/aster-srv/Cargo.toml index 090f236..74ad4d3 100644 --- a/crates/aster-srv/Cargo.toml +++ b/crates/aster-srv/Cargo.toml @@ -15,6 +15,7 @@ bytes = "1" chrono = { version = "0.4", default-features = false, features = ["clock", "std"] } deadpool-postgres = "0.14" flate2 = "1" +futures-util = { version = "0.3", default-features = false, features = ["std"] } http-body-util = "0.1" hyper = { version = "1", features = ["http1", "server"] } hyper-util = { version = "0.1", features = ["http1", "server", "server-graceful", "tokio"] } diff --git a/crates/aster-srv/README.md b/crates/aster-srv/README.md index 0c12939..0558200 100644 --- a/crates/aster-srv/README.md +++ b/crates/aster-srv/README.md @@ -100,9 +100,18 @@ the classifier updates `test_results.quarantine`, in this order: 3. A test whose final results since it entered end in `ASTER_SRV_QUARANTINE_RELEASE_PASSES` (default 20) consecutive passes is **released**. Any failure resets the count. -4. A test that is not broken **enters** once definite flake events from - `ASTER_SRV_QUARANTINE_RUNS` (default 2) different failing runs fall within - the last 7 days. Only events after it last left count, so a released or +4. A test that is not broken **enters** once **trusted** definite flake + events from `ASTER_SRV_QUARANTINE_RUNS` (default 2) different failing runs + fall within the last 7 days. An event is trusted when both of its runs are + default-branch runs by their upload's credentials: an OIDC token whose + `ref` is the default branch and whose `event_name` is `push`, + `workflow_dispatch` or `schedule`, or a static token's run on the default + branch. Pull request jobs run their author's code, so their token proves + where results came from, not that they are true: their flakes still score + (and show on `/ui`) but never quarantine a test, and a pull request's pass + cannot make a real main failure count. A trusted run is also compared + with the nearest earlier trusted run of its inputs, so pull request runs + in between do not hide a flake main saw itself. Only events after it last left count, so a released or expired test is judged afresh. `0` turns automatic quarantine off. A run holding definite events of more than `ASTER_SRV_QUARANTINE_RUN_CAP` (default 10) tests in those days counts toward none of them: many tests @@ -420,6 +429,7 @@ Pages are sent with a strict `Content-Security-Policy`, `nosniff`, | `ASTER_SRV_DEFAULT_BRANCH` | `main` | The branch whose runs decide broken and likely-flaky tests. | | `ASTER_SRV_BROKEN_RUNS` | `3` | Consecutive default-branch runs failing the same way that make a test broken; at least 3. | | `ASTER_SRV_MAX_BODY_BYTES` | 64 MiB | Largest upload, before and after decompression. | +| `ASTER_SRV_MAX_LARGE_BODIES` | `8` | Large request bodies (a cache commit's manifest, a test-results upload) one replica reads at once, 1 to 1024. Past it a request gets `503` with `Retry-After: 1`. | | `ASTER_SRV_RAW_BUCKET` | unset | A GCS bucket that keeps each upload as sent, using the pod's Workload Identity. Give the bucket a lifecycle rule that deletes objects after the retention period. | | `ASTER_SRV_IAP_AUDIENCE` | unset | The Google IAP audience the dashboard accepts: exactly `/projects/NUMBER/global/backendServices/ID`. A project-wide prefix is refused at startup, since it would accept an assertion for any other backend service of the project. Unset turns the dashboard off. See [Dashboard](#dashboard). | | `ASTER_SRV_UI_EMAIL_DOMAINS` | unset | Comma-separated domains (`example.com`, `*.example.com`) whose emails may view the dashboard. Unset allows anyone who passes IAP. | @@ -439,14 +449,19 @@ Pages are sent with a strict `Content-Security-Policy`, `nosniff`, | `ASTER_SRV_DEV_BLOB_ROUTE` | unset | `1` makes the in-memory blob store answer `PUT\|GET //cas/`, the test stand-in for a signed GCS URL. Tests and local development only; no deploy sets it and the route does not exist without it. | | `ASTER_SRV_PUBLIC_URL` | unset | The https URL people reach the server at (`https://aster-srv.example.com`); the `iss` and `aud` of person tokens. Required for [Laptop sign-in](#laptop-sign-in). With an https URL every response carries `Strict-Transport-Security: max-age=31536000; includeSubDomains`. An `http://` URL is refused at startup unless `ASTER_SRV_ALLOW_HTTP_PUBLIC_URL=1` (development and tests only). | | `ASTER_SRV_CLI_TOKEN_KEYS` | unset | `kid:secret` pairs, comma-separated. A secret is hex (64+ digits) or base64 (43+ characters) for at least 32 random bytes, as `openssl rand -base64 32` makes; passphrases, short keys and patterned keys (one byte repeated, a repeated block, a counting run, fewer than 16 distinct bytes) stop startup. The first signs person tokens and form MACs; all verify. Setting it also requires `ASTER_SRV_IAP_AUDIENCE` and `ASTER_SRV_UI_EMAIL_DOMAINS`, and forbids `ASTER_SRV_UI_DEV_EMAIL`. Keep it in a secret store. | +| `ASTER_SRV_PERSON_TOKENS` | `on` | `off` is the kill switch for laptop sign-in: every person token is refused and `/cli-auth` and `/v1/auth/token` answer `404`, whatever else is set. CI tokens are unaffected. Any value but `on` or `off` stops startup. See [Rotating the signing keys](#rotating-the-signing-keys). | | `ASTER_SRV_CLI_TOKEN_TTL` | `43200` | Seconds a person token lives, 60 to 43200 (12 hours; verification enforces the same cap). | -| `ASTER_SRV_API_ADDR` | unset | A second listener, such as `0.0.0.0:8081`, that serves only `/v1/*`. See [Hardening](#hardening-the-v1-channel). | +| `ASTER_SRV_API_ADDR` | unset | A second listener, such as `0.0.0.0:8081`, that serves only `/v1/*`. It refuses to start unless `ASTER_SRV_STATIC_TOKENS` and `ASTER_SRV_DEV_BLOB_ROUTE` are unset, `ASTER_SRV_REPOSITORY_OWNER_IDS` pins every `ASTER_SRV_REPOSITORIES` entry, `ASTER_SRV_CACHE_WRITER_WORKFLOWS` is set and `ASTER_SRV_TRUSTED_PROXY_HOPS` is at least 1. See [Hardening](#hardening-the-v1-channel). | | `ASTER_SRV_TRUSTED_PROXY_HOPS` | `1` | `X-Forwarded-For` entries from the right that infrastructure appended. See [Client IP](#client-ip-and-rate-limits). | | `ASTER_SRV_RATE_TOKEN_IP_PER_MIN` | `10` | `POST /v1/auth/token` per client IP per minute, per replica. | | `ASTER_SRV_RATE_API_IP_PER_MIN` | `1200` | All `/v1/*` per client IP per minute, per replica. | | `ASTER_SRV_RATE_API_SUBJECT_PER_MIN` | `3000` | Authenticated `/v1/*` per subject (a person, or a CI repository) per minute, per replica. | | `ASTER_SRV_HEADER_TIMEOUT_SECS` | `20` | Time to send request headers; also how long an idle connection lives. | | `ASTER_SRV_BODY_TIMEOUT_SECS` | `60` | Time for a request body to arrive in full (`408`). | +| `ASTER_SRV_UNAUTH_BODY_TIMEOUT_SECS` | `8` | Time for the body of an unauthenticated route (`POST /v1/auth/token`, `POST /cli-auth`, 8 KiB at most) to arrive (`408`); never more than `ASTER_SRV_BODY_TIMEOUT_SECS`, which the authenticated uploads keep. | +| `ASTER_SRV_MAX_CONNECTIONS_PER_IP` | `128` | Requests in flight from one client IP (resolved as in [Client IP](#client-ip-and-rate-limits); IPv6 by /64), per replica; more get `429`. With `ASTER_SRV_TRUSTED_PROXY_HOPS=0` the peer is the client, so it also caps that peer's open connections (extra ones are closed unserved). | +| `ASTER_SRV_LOG_SAMPLE_FIRST` | `5` | Failure audit lines (`auth_failure`, `rate_limited`, a failed `token_exchange`) logged per event, reason, route and client IP in each window; the rest are counted and summarised. | +| `ASTER_SRV_LOG_SAMPLE_WINDOW_SECS` | `60` | The sampling window, and how often `audit_summary` lines are written. | | `ASTER_SRV_HANDLER_TIMEOUT_SECS` | `120` | Time from the end of the headers to the reply, body included (`504`). | | `ASTER_SRV_MAX_CONNECTIONS` | `1024` | Open connections per listener; more wait in the accept queue. | | `ASTER_SRV_DB_WAIT_SECS` | `5` | How long a request waits for a database connection before failing. | @@ -463,7 +478,16 @@ none. Everything below applies to it. `/v1/*`. `/ui`, `/cli-auth`, `/healthz`, `/readyz`, `/metrics` and anything else answer `404` there, whatever the load balancer's URL map says, so a routing mistake cannot expose the dashboard or sign-in pages without IAP. The main -port is unchanged and still answers `/v1/*`. Give the non-IAP backend service +port is unchanged and still answers `/v1/*`. The listener has no IAP in front of +it, so it starts only with the settings that make a CI token mean what it +says: no static tokens, no `ASTER_SRV_DEV_BLOB_ROUTE`, an owner-id pin +(`ASTER_SRV_REPOSITORY_OWNER_IDS`) covering every `ASTER_SRV_REPOSITORIES` +entry (`owner/*=id` covers `owner/*` and `owner/name`; `owner/name=id` covers +only that repository), a writer-workflow list +(`ASTER_SRV_CACHE_WRITER_WORKFLOWS`; `ASTER_SRV_ALLOW_ANY_WRITER_WORKFLOW` +does not apply) and `ASTER_SRV_TRUSTED_PROXY_HOPS` of at least 1. Startup +fails naming every missing one. With the listener on, a token for a +repository no pin covers is refused too. Give the non-IAP backend service the API port; keep IAP on the main port. The API port's backend health check uses `GET /v1/healthz`, which answers `200 ok` and nothing else, is not authenticated (a health check carries no credentials) and reveals only that @@ -480,7 +504,9 @@ server answers `429` with `Retry-After`. | `POST /v1/auth/token` | client IP | 10 per minute | | all `/v1/*` | client IP | 1200 per minute | | authenticated `/v1/*` | person `sub`, or CI repository | 3000 per minute | +| requests in flight | client IP | 128 (more get `429`) | | concurrent exchanges | the replica | 4 (more get `503`, `Retry-After: 1`) | +| concurrent large bodies (cache commit, test-results upload) | the replica | 8, `ASTER_SRV_MAX_LARGE_BODIES` (more get `503`, `Retry-After: 1`) | Google's load balancer sets `X-Forwarded-For` to `, , `. Everything left of the last two entries is under the @@ -492,12 +518,34 @@ ignores the header. IPv6 clients share a limit per /64. This holds only while pods are reachable through the balancer alone; a caller reaching a pod directly could write the whole header (network policy must prevent it). +**Writers are checked before the body.** The route gate holds a CI token to +the route's writer rule before reading any body: for `/v1/cache/upload` and +`/v1/cache/commit` the cache rule (ref from `ASTER_SRV_CACHE_WRITERS`, a +`push` event, a `job_workflow_ref` prefix from +`ASTER_SRV_CACHE_WRITER_WORKFLOWS`), so a pull-request job's token gets `403` +without the server reading up to 64 MiB first. `POST /v1/test-results/runs` +still accepts pull-request runs (a retry that passes there is the flake +signal) and refuses `pull_request_target` and foreign workflows; only the +large-body limit applies to it beyond that. The repository allowlist and +owner-id pin are applied when the token is verified, which is also before any +body. + +Each limiter tracks at most 100,000 keys. Past that, idle keys are dropped +(at most once a second) and a newcomer shares one of 256 overflow buckets +chosen by a keyed hash, so a flood of minted client IPs does not take a real +client's whole allowance, and it never touches the per-subject limiter, which +has its own table. + **Size, time and connection limits.** `POST /cli-auth` and `/v1/auth/token` take at most 8 KiB, uploads and cache writes `ASTER_SRV_MAX_BODY_BYTES`, and every other route takes no body (`413`, decided from `Content-Length` before reading and while reading chunked bodies). Header, body and whole-request -times are capped (`408`, `504`), and each listener serves at most -`ASTER_SRV_MAX_CONNECTIONS` connections. Database connections are capped too: +times are capped (`408`, `504`); the unauthenticated bodies get the short +`ASTER_SRV_UNAUTH_BODY_TIMEOUT_SECS`. The header timeout also closes an idle +keep-alive connection. Each listener serves at most +`ASTER_SRV_MAX_CONNECTIONS` connections and one client IP holds at most +`ASTER_SRV_MAX_CONNECTIONS_PER_IP` requests open, so a few addresses +trickling bodies cannot take every slot. Database connections are capped too: the shared pool waits `ASTER_SRV_DB_WAIT_SECS` and fails, and `POST /v1/auth/token` has its own 2-connection pool so a flood of sign-in attempts cannot take connections from CI commits and uploads. @@ -506,7 +554,14 @@ attempts cannot take connections from CI commits and uploads. `token_mint` (a code issued at `/cli-auth`), `token_exchange` (`outcome` and `reason`), `token_revoke` (`audit::token_revoke`, called by the revoke endpoint), `auth_failure` (every 401 and 403: `route`, `client_ip`, `subject` -when known, `reason`) and `rate_limited`. Tokens, codes, verifiers and IAP +when known, `reason`) and `rate_limited`. The three failure events are +sampled so a flood of 401s does not become a flood of log lines: the first +`ASTER_SRV_LOG_SAMPLE_FIRST` (5) per event, reason, route and client IP in each +`ASTER_SRV_LOG_SAMPLE_WINDOW_SECS` (60) window are logged, then one +`audit_summary` line (`of`, `reason`, `route`, `client_ip`, `suppressed`) +when the window ends. The counters below stay exact, and `token_mint`, a +successful `token_exchange` and `token_revoke` are never sampled. Alert on +the counters, not on line counts. Tokens, codes, verifiers and IAP assertions are never fields, and the logger redacts credential-named fields and JWT-shaped text anyway. @@ -566,16 +621,44 @@ signs them with IAM Credentials `signBlob` as its own service account | Route | | |---|---| -| `POST /v1/cache/lookup` | `{repo, keys[]}` → `{hits: {key: {manifest, blobs}}}`. `manifest` is base64. `blobs` maps each sha256 the manifest names to a signed GET URL, and only when that object exists. An unknown key is absent from `hits`, not an error. A lookup updates `last_used_at`. | +| `POST /v1/cache/lookup` | `{repo, keys[]}` → `{hits: {key: {manifest, blobs}}}`. `manifest` is base64. `blobs` maps each sha256 the manifest names to a signed GET URL, and only when that object exists. An unknown key is absent from `hits`, not an error. A key whose blobs the server could not resolve (over the blob cap, past the time budget, or GCS or IAM failed or was busy) is also absent, so the client treats it as a miss and rebuilds. See [Lookup bounds](#lookup-bounds). A writer CI token's lookup updates `last_used_at`; see below. | | `POST /v1/cache/upload` | `{repo, digests[]}` → `{missing: {sha256: signed PUT URL}}` for blobs not yet in the bucket. Writers only. | | `POST /v1/cache/commit` | `{repo, key, manifest}` stores the manifest once every blob it names exists, and records the writer's `ref`, `run_id`, `sha` and `workflow_ref` claims (null for a static token). A blob uploaded through a signed URL is hashed on its first commit; bytes that do not match their sha256 are deleted and reported missing. `201` stored, `409` the key is already stored, `422` a named blob is missing or corrupt. Writers only. | | `GET /v1/cache/summary?repo=` | `{repo, manifests, manifest_bytes, blobs, blob_bytes, last_write_at, last_writer: {ref, run_id, sha, workflow} or null, lookups: [{day, hits, misses}]}`. `lookups` are the last 14 UTC days that had a lookup, newest first. `blobs` and `blob_bytes` are global: blobs are content-addressed and shared across repositories. | | `GET /v1/cache/entries?repo=&limit=<1..200, default 50>&before=&target=` | `{entries: [{key, target, size_bytes, output_files, exit_code, duration_ms, aster_version, created_at, last_used_at, hits, last_hit_at, writer: {ref, run_id, sha, workflow}}], next}` ordered by `created_at` descending, then key. `next` is an opaque cursor to pass as `before`, or null on the last page. `target` is a case-insensitive literal substring filter (`%`, `_` and `\` match themselves), applied in SQL, so a page holds `limit` entries whenever more match. Commit stores `target`, `output_files`, `exit_code`, `duration_ms` and `aster_version` beside the manifest, so paging never reads manifest bytes. A manifest that does not decode is listed with an empty `target` and zeroed fields. | -A lookup counts one hit per stored key (`hits`, `last_hit_at` on the manifest) -and adds the request's hits and misses to today's row in -`cache.lookup_stats`. Counting is best effort: a failure is logged and the -lookup still answers. Keys repeated in one request count once. +A lookup by a CI token that may write the cache (the same writer rules as +commit: allowed repository and owner id, writer ref, `push` event, writer +workflow) counts one hit per stored key it serves (`hits`, `last_hit_at` on +the manifest) and refreshes `last_used_at` on the manifest and its blobs, +which keeps them past retention. A pull-request or other non-writer CI +token's lookup, and a person's, changes none of these, so it cannot hold an +entry in the cache. Every CI lookup, writer or not, adds the request's hits +and misses to today's row in `cache.lookup_stats`; that write is best effort: +a failure is logged and the lookup still answers. Keys repeated in one +request count once. + +### Lookup bounds + +One lookup's GCS and IAM work is bounded, so a wide or cold lookup answers +inside the client's 5s request timeout with fewer hits rather than timing out +and losing them all: + +- **Blob cap.** One response names at most 4096 distinct blobs + (`MAX_LOOKUP_BLOBS`). Entries are admitted in the order the request lists + them; an entry whose blobs would push the total over the cap is absent + from `hits` (a miss) and is not counted. +- **Time budget.** Blobs are resolved 16 at a time per request, for at most + 3 seconds. An entry with a blob still unresolved then is a miss. +- **Remembered answers.** A positive HEAD is remembered for an hour (blobs + are content-addressed and a blob a stored manifest names is not deleted by + retention); a miss is never remembered. A signed GET URL is reused while + at least 5 of its 15 minutes remain. Concurrent lookups of one blob share + one HEAD and one signature. This memory is per replica. +- **IAM `signBlob` limit.** Per replica, at most 16 signatures in flight and + a token bucket of 200 per second with a burst of 512. A signature that + would wait more than 2 seconds fails with "signing is busy" and its entry + is a miss. HEADs are capped at 64 in flight per replica. Every route needs a bearer token; an IAP assertion alone is never accepted here, so a request with neither answers `401`. Reads accept any authenticated @@ -606,7 +689,7 @@ a `/v1/` path the table does not list is `401` without a valid token. |---|---|---| | `Public` | `GET /healthz`, `GET /readyz`, `POST /v1/auth/token` | anyone; the exchange needs a valid code | | `Iap` | `/cli-auth`, `/ui/*` | a person behind IAP | -| `CiOrPerson` | `GET /v1/test-results/{tests,tests/:id,failures,quarantine,clusters,durations,runs/:id/failures}`, `GET /v1/cache/{summary,entries}`, `POST /v1/cache/lookup` | a CI token for its own repository, or a person's token for any served repository named with `?repo=` (read-only; a person's lookups do not count hits) | +| `CiOrPerson` | `GET /v1/test-results/{tests,tests/:id,failures,quarantine,clusters,durations,runs/:id/failures}`, `GET /v1/cache/{summary,entries}`, `POST /v1/cache/lookup` | a CI token for its own repository, or a person's token for any served repository named with `?repo=` (read-only; only a writer CI token's lookups count hits) | | `CiWriter` | `POST /v1/test-results/runs`, `POST /v1/cache/{upload,commit}` | a CI token only, then the writer rules | A lookup or upload request body is capped at 1 MiB and read only after the @@ -638,8 +721,7 @@ keys only and RS256 to the GitHub OIDC verifier only; any other algorithm is Sign-in is off, failing closed, unless both `ASTER_SRV_CLI_TOKEN_KEYS` and `ASTER_SRV_PUBLIC_URL` are set and IAP (`ASTER_SRV_IAP_AUDIENCE`) is configured: `/cli-auth` and `/v1/auth/token` answer `404` and person tokens are -refused. Rotate keys by adding a new first pair, waiting out the token -lifetime, then dropping the old pair. +refused. Every six hours one replica drops manifests unused for `ASTER_SRV_CACHE_DAYS` (default 14) and blobs no remaining manifest names, and deletes those objects. @@ -653,3 +735,70 @@ A lookup answers before its day's `cache.lookup_stats` row is written: that write runs afterwards with a 3 second limit, and a failure is logged and the request's counts are lost. The counts are lookup requests (each distinct key counts once), not successful restores. + +### Rotating the signing keys + +The deploying repository sets `ASTER_SRV_CLI_TOKEN_KEYS` from its secret store; +these are the steps on the server's side. + +`ASTER_SRV_CLI_TOKEN_KEYS` is a comma-separated list of `kid:secret` pairs, +such as `k2:,k1:`. The first pair signs new person +tokens (its `kid` goes in the token header) and the `/cli-auth` form MACs; +every pair verifies. Key ids use letters, digits, `-`, `_` and `.`, and may +not repeat. + +A secret is at least 32 random bytes, written as base64 (43+ characters) or +hex (64+ digits): `openssl rand -base64 32` or `openssl rand -hex 32`. +Passphrases, short keys and patterned keys stop startup. + +Planned rotation, with nobody signed out: + +1. Generate a new secret and put its pair first: `k2:,k1:`. Deploy. + New tokens are signed with `k2`; tokens signed with `k1` still verify. +2. Wait a little over 12 hours after every replica runs the new list (the + longest a token lives, whatever `ASTER_SRV_CLI_TOKEN_TTL` says, plus 30 + seconds of clock skew), so every `k1` token has expired. +3. Drop the old pair: `k2:`. Deploy. + +Emergency rotation, when a secret leaked: replace the whole list with one new +pair, `k3:`, and deploy. Every token signed with an older key is refused +at once, so everyone signs in again with `aster cache login`; a `/cli-auth` +page open during the deploy fails its MAC and has to be reloaded. + +Kill switch: `ASTER_SRV_PERSON_TOKENS=off` refuses every person token and +answers `404` on `/cli-auth` and `/v1/auth/token`, whatever the keys say. CI +tokens are unaffected. Set it back to `on` (or unset it) to re-enable sign-in; +tokens still within their lifetime and signed by a listed key work again. + +### Revoking a departed person's tokens + +Removing someone from IAP stops new sign-ins, but a token they already hold +works until it expires (up to 12 hours). `aster cache logout` revokes a +person's own tokens; an operator does the same for someone else by writing +their row in `auth.revocations` directly. There is no admin route for this. + +1. Remove their IAP access (or narrow `ASTER_SRV_UI_EMAIL_DOMAINS`), so they + cannot sign in again. +2. Find the person's subject, IAP's stable `sub` (`accounts.google.com:`), + not their email. Every sign-in writes an audit line with both: + `jsonPayload.audit=true AND jsonPayload.event="token_mint" AND + jsonPayload.email="ada@example.com"` in Cloud Logging, and read + `jsonPayload.subject`. (`auth.cli_codes` does not help: a code row is + deleted when it is redeemed and is only good for 60 seconds.) Revoke + every distinct subject the email shows. +3. Connect to the server's database with `psql` as a role that can write the + `auth` schema (for Cloud SQL, through the Cloud SQL Auth Proxy or however + your deployment reaches the database) and run, with the subject from step 2: + + +```sql +\set subject 'accounts.google.com:1234567890' +INSERT INTO auth.revocations (subject, not_before) VALUES (:'subject', now()) +ON CONFLICT (subject) DO UPDATE SET not_before = greatest(auth.revocations.not_before, now()); +``` + + +Every replica checks `auth.revocations` on each request, so the person's +tokens issued up to that second get `401` from then on; nothing needs a +restart. This write is not audited by the server: record who ran it +and why. The `tests/operator_revoke.rs` test runs this snippet from this file. diff --git a/crates/aster-srv/src/audit.rs b/crates/aster-srv/src/audit.rs index e7d75aa..bd29e43 100644 --- a/crates/aster-srv/src/audit.rs +++ b/crates/aster-srv/src/audit.rs @@ -11,6 +11,15 @@ //! | `auth_failure` | any 401 or 403 | `subject` if known, `reason`, `detail` | //! | `rate_limited` | a limit refused the request | `scope`, `subject` if known | //! +//! The three failure events (`auth_failure`, `rate_limited`, a failed +//! `token_exchange`) are sampled: the first `ASTER_SRV_LOG_SAMPLE_FIRST` +//! (default 5) lines per `(event, reason, route, client_ip)` in each +//! `ASTER_SRV_LOG_SAMPLE_WINDOW_SECS` window (default 60) are logged, and when +//! the window ends one `audit_summary` line per key says how many more +//! happened (`suppressed`). The counters in [`crate::metrics`] are bumped +//! before sampling and stay exact. Mints, successful exchanges and revokes +//! are never sampled. +//! //! Each carries `route` (a fixed template, see [`crate::routes::label`]) and //! `client_ip` from the request. Tokens, codes, verifiers and assertions are //! never fields: callers pass reasons and subjects only, and [`crate::log`] @@ -24,9 +33,11 @@ //! `jsonPayload.audit=true AND jsonPayload.event="auth_failure"`, or with //! Prometheus: `sum(rate(aster_srv_auth_failures_total[5m])) > 1`. +use std::collections::HashMap; use std::future::Future; use std::sync::atomic::{AtomicBool, Ordering}; -use std::sync::Arc; +use std::sync::{Arc, Mutex}; +use std::time::{Duration, Instant}; use serde_json::{json, Map, Value}; @@ -37,22 +48,198 @@ pub struct RequestCtx { pub route: &'static str, pub client_ip: String, pub metrics: Arc, + pub sampler: Arc, /// Set once a failure was audited, so the server's catch-all does not /// log the same 401 or 403 twice. audited: AtomicBool, } impl RequestCtx { - pub fn new(route: &'static str, client_ip: String, metrics: Arc) -> Self { + pub fn new( + route: &'static str, + client_ip: String, + metrics: Arc, + sampler: Arc, + ) -> Self { Self { route, client_ip, metrics, + sampler, audited: AtomicBool::new(false), } } } +/// Distinct `(event, reason, route, client_ip)` keys sampled at once. Past +/// it, a new key is counted under `client_ip = "*"`, so a flood of spoofed +/// addresses cannot grow the table. +const MAX_SAMPLE_KEYS: usize = 10_000; + +type SampleKey = (&'static str, &'static str, &'static str, String); + +struct Seen { + logged: u32, + suppressed: u64, +} + +struct Window { + started: Instant, + seen: HashMap, +} + +/// Logs the first few failures per key in a window and a summary of the +/// rest. See the module docs. +pub struct Sampler { + first: u32, + window: Duration, + state: Mutex, +} + +impl Sampler { + pub fn new(first: u32, window: Duration) -> Self { + Self { + first, + window, + state: Mutex::new(Window { + started: Instant::now(), + seen: HashMap::new(), + }), + } + } + + /// Whether to log this occurrence. Also closes a window that has ended + /// and logs its summaries. + pub fn admit( + &self, + event: &'static str, + reason: &'static str, + route: &'static str, + ip: &str, + ) -> bool { + self.admit_at(event, reason, route, ip, Instant::now()) + } + + pub fn admit_at( + &self, + event: &'static str, + reason: &'static str, + route: &'static str, + ip: &str, + now: Instant, + ) -> bool { + let (closed, admitted) = { + let mut window = self.state.lock().unwrap(); + let closed = self.roll(&mut window, now); + let ip = if window.seen.len() >= MAX_SAMPLE_KEYS + && !window + .seen + .contains_key(&(event, reason, route, ip.to_string())) + { + "*" + } else { + ip + }; + let seen = window + .seen + .entry((event, reason, route, ip.to_string())) + .or_insert(Seen { + logged: 0, + suppressed: 0, + }); + let admitted = seen.logged < self.first; + if admitted { + seen.logged += 1; + } else { + seen.suppressed += 1; + } + (closed, admitted) + }; + self.log_summaries(closed); + admitted + } + + /// Close the window if it has ended and log what it suppressed. The + /// server calls this on a timer, so a quiet period still gets its summary. + pub fn flush(&self) { + self.flush_at(Instant::now()); + } + + pub fn flush_at(&self, now: Instant) { + let closed = self.roll(&mut self.state.lock().unwrap(), now); + self.log_summaries(closed); + } + + /// Log every pending summary now, whether or not the window ended. For + /// shutdown. + pub fn flush_all(&self) { + let closed = { + let mut window = self.state.lock().unwrap(); + window.started = Instant::now(); + std::mem::take(&mut window.seen) + }; + self.log_summaries(closed); + } + + /// Start a new window if this one has ended, returning its entries. + fn roll(&self, window: &mut Window, now: Instant) -> HashMap { + if now.saturating_duration_since(window.started) < self.window { + return HashMap::new(); + } + window.started = now; + std::mem::take(&mut window.seen) + } + + fn log_summaries(&self, closed: HashMap) { + let mut lines: Vec<_> = closed + .into_iter() + .filter(|(_, seen)| seen.suppressed > 0) + .collect(); + lines.sort_by(|a, b| a.0.cmp(&b.0)); + for ((event, reason, route, ip), seen) in lines { + crate::log::info( + "audit", + json!({ + "audit": true, + "event": "audit_summary", + "of": event, + "reason": reason, + "route": route, + "client_ip": ip, + "suppressed": seen.suppressed, + "logged": seen.logged, + "window_secs": self.window.as_secs(), + }), + ); + } + } + + /// Flush on a timer for as long as `sampler` is alive. + pub fn spawn_flush(sampler: &Arc) { + let weak = Arc::downgrade(sampler); + let every = sampler.window; + tokio::spawn(async move { + let mut tick = tokio::time::interval(every); + tick.tick().await; + loop { + tick.tick().await; + match weak.upgrade() { + Some(sampler) => sampler.flush(), + None => break, + } + } + }); + } +} + +/// Whether the current request should log a failure of this kind: the +/// first few per `(event, reason, route, client_ip)` per window. Always true +/// outside a request. +pub fn admit(event: &'static str, reason: &'static str) -> bool { + CTX.try_with(|ctx| ctx.sampler.admit(event, reason, ctx.route, &ctx.client_ip)) + .unwrap_or(true) +} + tokio::task_local! { static CTX: RequestCtx; } @@ -100,6 +287,9 @@ fn subject_field(subject: Option<&str>) -> Value { pub fn auth_failure(subject: Option<&str>, reason: &'static str, detail: &str) { let _ = CTX.try_with(|ctx| ctx.audited.store(true, Ordering::Relaxed)); metrics(|m, route| m.auth_failure(route, reason)); + if !admit("auth_failure", reason) { + return; + } let detail: String = detail.chars().take(200).collect(); emit( "auth_failure", @@ -111,6 +301,9 @@ pub fn auth_failure(subject: Option<&str>, reason: &'static str, detail: &str) { /// A limit refused the request. pub fn rate_limited(scope: &'static str, subject: Option<&str>) { metrics(|m, route| m.rate_limited(route, scope)); + if !admit("rate_limited", scope) { + return; + } emit( "rate_limited", None, @@ -140,6 +333,9 @@ pub fn exchange_succeeded(subject: &str) { pub fn exchange_failed(subject: Option<&str>, reason: &'static str) { let _ = CTX.try_with(|ctx| ctx.audited.store(true, Ordering::Relaxed)); metrics(|m, route| m.auth_failure(route, reason)); + if !admit("token_exchange", reason) { + return; + } emit( "token_exchange", None, @@ -152,3 +348,68 @@ pub fn exchange_failed(subject: Option<&str>, reason: &'static str) { pub fn token_revoke(subject: &str, by: &str) { emit("token_revoke", None, json!({"subject": subject, "by": by})); } + +#[cfg(test)] +mod tests { + use super::*; + + fn summaries() -> Vec { + crate::log::captured() + .iter() + .filter_map(|line| serde_json::from_str::(line).ok()) + .filter(|line| line["event"] == "audit_summary") + .collect() + } + + #[test] + fn the_first_few_per_key_are_admitted_and_the_rest_summarised_when_the_window_ends() { + crate::log::start_capture(); + let sampler = Sampler::new(3, Duration::from_secs(60)); + let start = Instant::now(); + let admit = |ip: &str, at: u64| { + sampler.admit_at( + "auth_failure", + "n15_unit_reason", + "/v1/cache", + ip, + start + Duration::from_secs(at), + ) + }; + let admitted = (0..10).filter(|n| admit("203.0.113.1", *n)).count(); + assert_eq!(admitted, 3); + // Another client IP, and another reason, have their own budget. + assert!(admit("203.0.113.2", 11)); + assert!(sampler.admit_at("auth_failure", "other", "/v1/cache", "203.0.113.1", start)); + // Nothing is summarised while the window is open. + sampler.flush_at(start + Duration::from_secs(59)); + assert!(summaries().iter().all(|s| s["reason"] != "n15_unit_reason")); + // The window ends: one summary for the key that suppressed lines. + sampler.flush_at(start + Duration::from_secs(61)); + let ours: Vec<_> = summaries() + .into_iter() + .filter(|s| s["reason"] == "n15_unit_reason") + .collect(); + assert_eq!(ours.len(), 1, "{ours:?}"); + assert_eq!(ours[0]["client_ip"], "203.0.113.1"); + assert_eq!(ours[0]["suppressed"], 7); + assert_eq!(ours[0]["logged"], 3); + assert_eq!(ours[0]["of"], "auth_failure"); + // A new window starts with a fresh budget. + assert!(admit("203.0.113.1", 62)); + } + + #[test] + fn a_flood_of_distinct_addresses_cannot_grow_the_table() { + let sampler = Sampler::new(1, Duration::from_secs(60)); + let now = Instant::now(); + for n in 0..(MAX_SAMPLE_KEYS + 500) { + sampler.admit_at("auth_failure", "r", "/v1/cache", &format!("ip-{n}"), now); + } + let window = sampler.state.lock().unwrap(); + assert!(window.seen.len() <= MAX_SAMPLE_KEYS + 1); + // The overflow is counted under one aggregate key. + assert!(window + .seen + .contains_key(&("auth_failure", "r", "/v1/cache", "*".to_string()))); + } +} diff --git a/crates/aster-srv/src/auth.rs b/crates/aster-srv/src/auth.rs index 238920e..0b6370e 100644 --- a/crates/aster-srv/src/auth.rs +++ b/crates/aster-srv/src/auth.rs @@ -4,8 +4,9 @@ //! An OIDC token is accepted when it is signed by the issuer's current key, //! is unexpired, names this server's audience, and comes from a repository //! on the allowlist. Keys are fetched from the issuer's JWKS and refetched -//! when a token names a key not yet seen, at most once a minute; callers -//! that arrive during a fetch wait for it and share its keys. +//! when a token names a key not yet seen, at most once every ten seconds +//! (see [`crate::keyfetch`] for the rules); callers that arrive during a +//! fetch wait for it and share its keys. //! //! [`authenticate`] is the one entry point for `/v1/*`. It reads the //! token's `alg` and sends HS256 to the server's own person-token keys and @@ -21,6 +22,7 @@ use serde::Deserialize; use tokio::sync::{Mutex, RwLock}; use crate::config::{Config, OwnerIds, RepositoryAllowlist, StaticToken}; +use crate::keyfetch::{RefetchPolicy, RefetchState}; use crate::person::{Person, TokenIssuer}; /// Who a bearer token names. @@ -82,6 +84,8 @@ pub struct Identity { pub git_ref: Option, /// The `run_id` claim. `None` for a static token. pub run_id: Option, + /// The `run_attempt` claim. `None` for a static token. + pub run_attempt: Option, /// The `sha` claim. `None` for a static token. pub sha: Option, /// The `workflow_ref` claim (`owner/repo/.github/workflows/ci.yml@refs/heads/main`): @@ -133,8 +137,12 @@ impl AuthError { } else { 401 }; + // Sampled with the audit lines: a flood of bad tokens is one line + // per client and reason per window, then a summary. if let AuthError::Invalid(reason) = &self { - crate::log::warn("token refused", serde_json::json!({"reason": reason})); + if crate::audit::admit("token_refused", self.reason_code()) { + crate::log::warn("token refused", serde_json::json!({"reason": reason})); + } } crate::Reply::error(status, self.to_string()) } @@ -160,6 +168,8 @@ struct Claims { git_ref: Option, #[serde(default, deserialize_with = "string_or_number")] run_id: Option, + #[serde(default, deserialize_with = "string_or_number")] + run_attempt: Option, #[serde(default)] sha: Option, #[serde(default)] @@ -191,8 +201,6 @@ fn non_empty(value: Option) -> Option { value.filter(|value| !value.is_empty()) } -const REFETCH_INTERVAL: Duration = Duration::from_secs(60); - pub struct Verifier { audience: String, issuer: String, @@ -202,9 +210,11 @@ pub struct Verifier { /// Where keys come from; `None` uses only keys added with [`Self::with_key`]. jwks_url: Option, keys: RwLock>, - /// When keys were last fetched. Held for the whole fetch, so a caller - /// that arrives mid-fetch waits and then finds the fetched keys. - last_fetch: Mutex>, + /// When keys may be refetched (see [`crate::keyfetch`]). Held for the + /// whole fetch, so a caller that arrives mid-fetch waits and then finds + /// the fetched keys. + refetch: Mutex, + refetch_policy: RefetchPolicy, client: reqwest::Client, } @@ -221,7 +231,8 @@ impl Verifier { config.issuer.trim_end_matches('/') )), keys: RwLock::new(HashMap::new()), - last_fetch: Mutex::new(None), + refetch: Mutex::new(RefetchState::default()), + refetch_policy: RefetchPolicy::default(), client: crate::http_client() .timeout(Duration::from_secs(10)) .build() @@ -236,6 +247,14 @@ impl Verifier { self } + /// Fetch keys from `url` instead of the issuer's, spacing refetches by + /// `policy`. For tests. + pub fn with_jwks(mut self, url: &str, policy: RefetchPolicy) -> Self { + self.jwks_url = Some(url.to_string()); + self.refetch_policy = policy; + self + } + /// Whether `token` is a configured static token. fn is_static(&self, token: &str) -> bool { self.static_tokens @@ -257,6 +276,7 @@ impl Verifier { repository: found.repository.clone(), git_ref: None, run_id: None, + run_attempt: None, sha: None, workflow: None, job_workflow: None, @@ -298,6 +318,7 @@ impl Verifier { repository: claims.repository, git_ref: non_empty(claims.git_ref), run_id: non_empty(claims.run_id), + run_attempt: non_empty(claims.run_attempt), sha: non_empty(claims.sha), job_workflow: non_empty(claims.job_workflow_ref).or_else(|| workflow.clone()), workflow, @@ -316,14 +337,14 @@ impl Verifier { // One fetch at a time, with the lock held across it. Callers that // queued behind a fetch look again once they hold the lock, so they // get the key it fetched instead of failing while it was in flight. - let mut last = self.last_fetch.lock().await; + let mut refetch = self.refetch.lock().await; if let Some(key) = self.keys.read().await.get(kid) { return Ok(key.clone()); } - if last.is_some_and(|at| at.elapsed() < REFETCH_INTERVAL) { + if !refetch.may_fetch(kid, Instant::now(), self.refetch_policy) { return Err(AuthError::Invalid("unknown signing key".to_string())); } - *last = Some(Instant::now()); + refetch.attempted(Instant::now()); let set: JwkSet = self .client .get(url) @@ -344,9 +365,16 @@ impl Verifier { } let mut keys = self.keys.write().await; *keys = fetched; - keys.get(kid) - .cloned() - .ok_or_else(|| AuthError::Invalid("unknown signing key".to_string())) + match keys.get(kid).cloned() { + Some(key) => { + refetch.found(kid); + Ok(key) + } + None => { + refetch.missing(kid, Instant::now()); + Err(AuthError::Invalid("unknown signing key".to_string())) + } + } } } @@ -470,6 +498,7 @@ mod tests { let mut full = claims("ArchAstro/firstlanding"); full["ref"] = json!("refs/heads/main"); full["run_id"] = json!("12345"); + full["run_attempt"] = json!("2"); full["sha"] = json!("abc123"); full["workflow_ref"] = json!("ArchAstro/firstlanding/.github/workflows/ci.yml@refs/heads/main"); @@ -477,6 +506,7 @@ mod tests { let identity = verifier().verify(Some(&bearer)).await.unwrap(); assert_eq!(identity.git_ref.as_deref(), Some("refs/heads/main")); assert_eq!(identity.run_id.as_deref(), Some("12345")); + assert_eq!(identity.run_attempt.as_deref(), Some("2")); assert_eq!(identity.sha.as_deref(), Some("abc123")); assert_eq!( identity.workflow.as_deref(), @@ -485,10 +515,12 @@ mod tests { // A numeric run_id is kept; empty claims become None. full["run_id"] = json!(678); + full["run_attempt"] = json!(3); full["sha"] = json!(""); let bearer = format!("Bearer {}", token(full, "k1")); let identity = verifier().verify(Some(&bearer)).await.unwrap(); assert_eq!(identity.run_id.as_deref(), Some("678")); + assert_eq!(identity.run_attempt.as_deref(), Some("3")); assert_eq!(identity.sha, None); // Static tokens carry none of them. diff --git a/crates/aster-srv/src/cache/mod.rs b/crates/aster-srv/src/cache/mod.rs index 87987c6..12f391b 100644 --- a/crates/aster-srv/src/cache/mod.rs +++ b/crates/aster-srv/src/cache/mod.rs @@ -20,7 +20,7 @@ mod store; use crate::db::Migration; -pub use policy::{results_writer_allowed, workflow_allowed}; +pub use policy::{cache_writer_allowed, results_writer_allowed, workflow_allowed}; /// Whether a signed-in person may read `repo`'s data: their email domain is /// allowed and the server serves the repository. @@ -31,9 +31,9 @@ pub fn person_may_browse( ) -> Result<(), &'static str> { policy::person_browser(&app.iap, person, repo, &app.config.repositories) } -pub use routes::{handle, Headers}; -pub use signing::{expires_at, FixedSigner, Signer, URL_TTL}; -pub use store::{run_retention, spawn, BlobStore, Report}; +pub use routes::{handle, Headers, MAX_LOOKUP_BLOBS}; +pub use signing::{expires_at, FixedSigner, SignLimits, Signer, URL_TTL}; +pub use store::{run_retention, spawn, BlobStore, FanoutConfig, Report}; pub const MIGRATIONS: &[Migration] = &[ Migration { diff --git a/crates/aster-srv/src/cache/policy.rs b/crates/aster-srv/src/cache/policy.rs index 4e31417..2f3dd0c 100644 --- a/crates/aster-srv/src/cache/policy.rs +++ b/crates/aster-srv/src/cache/policy.rs @@ -94,18 +94,25 @@ pub fn results_writer_allowed(config: &Config, identity: &Identity) -> bool { && (identity.static_token || identity.event_name.as_deref() != Some("pull_request_target")) } -/// A CI caller. Static tokens may write; OIDC tokens write only when their -/// ref matches the writer patterns, their workflow is allowed and the run -/// was triggered by a push. +/// Whether a CI token may write the cache. Static tokens may; OIDC tokens +/// only when their ref matches the writer patterns, their workflow is +/// allowed and the run was triggered by a push. The route gate calls this +/// before it reads a body, and `ci_reader` calls it again in the handler: +/// this is the one implementation. +pub fn cache_writer_allowed(config: &Config, identity: &Identity) -> bool { + identity.static_token + || (config + .cache_writers + .allows(&identity.repository, identity.git_ref.as_deref()) + && workflow_allowed(config, identity) + && pushed(identity)) +} + +/// A CI caller, with `may_write` from [`cache_writer_allowed`]. pub fn ci_reader(config: &Config, identity: &Identity) -> Reader { Reader { repository: identity.repository.clone(), - may_write: identity.static_token - || (config - .cache_writers - .allows(&identity.repository, identity.git_ref.as_deref()) - && workflow_allowed(config, identity) - && pushed(identity)), + may_write: cache_writer_allowed(config, identity), writer: Writer::from_identity(identity), caller: Caller::Ci, } @@ -192,6 +199,7 @@ mod tests { repository: "ArchAstro/firstlanding".into(), git_ref: git_ref.map(str::to_string), run_id: Some("9".into()), + run_attempt: Some("1".into()), sha: Some("abc".into()), workflow: Some("wf".into()), job_workflow: Some( diff --git a/crates/aster-srv/src/cache/routes.rs b/crates/aster-srv/src/cache/routes.rs index fe8fffe..fd65516 100644 --- a/crates/aster-srv/src/cache/routes.rs +++ b/crates/aster-srv/src/cache/routes.rs @@ -146,9 +146,14 @@ async fn lookup(app: &App, headers: &Headers<'_>, body: Value) -> Reply { if let Err(reply) = same_repo(&reader, &request.repo) { return reply; } - // Only CI's lookups count and keep entries alive; a person reading must - // not be able to hold an entry in the cache by asking for it. - let counts = reader.caller == Caller::Ci; + // Only a writer-eligible CI caller's lookups bump `hits`, `last_hit_at` and + // `last_used_at` and so keep entries alive. A person, and a pull-request + // or other non-push CI token (`Reader::may_write` is false, from + // `policy::ci_reader`), must not be able to hold an entry in the cache by + // asking for it. The same policy decides who may commit; it is not repeated. + let counts = reader.caller == Caller::Ci && reader.may_write; + // Aggregate lookup stats stay a hit-rate view over every CI lookup. + let records_stats = reader.caller == Caller::Ci; match lookup_hits(app, &reader.repository, &request.keys, counts).await { Ok(hits) => { let requested = request @@ -156,7 +161,7 @@ async fn lookup(app: &App, headers: &Headers<'_>, body: Value) -> Reply { .iter() .collect::>() .len(); - if counts { + if records_stats { record_lookup(app, &reader.repository, hits.len(), requested); } Reply::json(200, json!({"hits": hits})) @@ -218,44 +223,149 @@ async fn write_stats( Ok(()) } +/// Blobs one lookup response may name, across all its hits. An entry whose +/// blobs would not fit is answered as a miss, so the client rebuilds that +/// target instead of the server doing unbounded GCS and IAM work. An entry +/// that alone names more than this is never served from the cache. Entries are +/// admitted in the order the client asked for them. +pub const MAX_LOOKUP_BLOBS: usize = 4096; + +/// Blobs resolved (HEAD and signature) at once for one lookup. The store +/// bounds the same work across all requests. +const LOOKUP_CONCURRENCY: usize = 16; + +/// What resolving one digest gave. +enum Resolved { + Url(String), + /// Not in GCS: the blob is left out and the client uploads it again. + Absent, + /// GCS or signing failed or was busy: the entry is answered as a miss. + /// A blob still unresolved when the lookup budget runs out is treated + /// the same way. + Failed, +} + async fn lookup_hits( app: &App, repo: &str, keys: &[String], counts: bool, ) -> anyhow::Result> { + use futures_util::stream::{self, StreamExt}; let client = app.pool.get().await?; - let sql = if counts { - "UPDATE cache.manifests \ - SET last_used_at = now(), hits = hits + 1, last_hit_at = now() \ - WHERE repo = $1 AND key = ANY($2) \ - RETURNING key, manifest" - } else { - "SELECT key, manifest FROM cache.manifests WHERE repo = $1 AND key = ANY($2)" - }; - let rows = client.query(sql, &[&repo, &keys]).await?; + let rows = client + .query( + "SELECT key, manifest FROM cache.manifests WHERE repo = $1 AND key = ANY($2)", + &[&repo, &keys], + ) + .await?; + let mut found: std::collections::HashMap> = rows + .into_iter() + .map(|row| (row.get::<_, String>(0), row.get::<_, Vec>(1))) + .collect(); + + // Admit entries in request order until the blob budget is spent. + let mut admitted: Vec<(String, Vec, Vec)> = Vec::new(); + let mut digests: Vec = Vec::new(); + let mut seen = std::collections::HashSet::new(); + for key in keys { + let Some(manifest) = found.remove(key) else { + continue; + }; + let refs = blob_refs(&manifest); + let new = refs.iter().filter(|digest| !seen.contains(*digest)).count(); + if digests.len() + new > MAX_LOOKUP_BLOBS { + crate::log::warn( + "cache lookup over the blob cap; answered as a miss", + json!({"repo": repo, "key": key, "blobs": refs.len(), "cap": MAX_LOOKUP_BLOBS}), + ); + continue; + } + for digest in &refs { + if seen.insert(digest.clone()) { + digests.push(digest.clone()); + } + } + admitted.push((key.clone(), manifest, refs)); + } + + // Resolve at most LOOKUP_CONCURRENCY blobs at once, and stop at the + // store's lookup budget: a blob still unresolved then leaves its entry a + // miss, so the reply beats the client's request timeout. Without a + // bucket every blob resolves at once as absent and there is no budget: + // a zero one would race the stream and drop entries at random. + let budget = app + .cache_store + .lookup_budget() + .unwrap_or(std::time::Duration::MAX); + let wanted = digests.len(); + let mut pending = stream::iter(digests) + .map(|digest| async move { + let result = match app.cache_store.readable_url(&digest).await { + Ok(Some(url)) => Resolved::Url(url), + Ok(None) => Resolved::Absent, + Err(error) => { + crate::log::warn( + "cache lookup could not resolve a blob", + json!({"error": format!("{error:#}"), "digest": digest}), + ); + Resolved::Failed + } + }; + (digest, result) + }) + .buffer_unordered(LOOKUP_CONCURRENCY); + let mut resolved: std::collections::HashMap = + std::collections::HashMap::with_capacity(wanted); + let deadline = tokio::time::sleep(budget); + tokio::pin!(deadline); + loop { + tokio::select! { + next = pending.next() => match next { + Some((digest, result)) => { + resolved.insert(digest, result); + } + None => break, + }, + () = &mut deadline => { + crate::log::warn( + "cache lookup ran out of time; unresolved entries are misses", + json!({ + "repo": repo, + "budget_ms": budget.as_millis() as u64, + "resolved": resolved.len(), + "blobs": wanted, + }), + ); + break; + } + } + } + // Dropping the stream cancels the HEADs and signatures still in flight. + drop(pending); + let mut hits = serde_json::Map::new(); - for row in rows { - let key: String = row.get(0); - let manifest: Vec = row.get(1); + let mut served_keys: Vec = Vec::new(); + let mut served_blobs: Vec = Vec::new(); + for (key, manifest, refs) in admitted { + // A blob that failed, or was not resolved within the budget, makes + // the whole entry a miss: the server cannot say whether it exists. + if refs.iter().any(|digest| { + !matches!( + resolved.get(digest), + Some(Resolved::Url(_) | Resolved::Absent) + ) + }) { + continue; + } let mut blobs = serde_json::Map::new(); - for digest in blob_refs(&manifest) { - if !app.cache_store.exists(&digest).await? { - continue; - } - if counts { - client - .execute( - "UPDATE cache.blobs SET last_used_at = now() WHERE sha256 = $1", - &[&digest], - ) - .await?; + for digest in refs { + if let Some(Resolved::Url(url)) = resolved.get(&digest) { + blobs.insert(digest.clone(), Value::String(url.clone())); + served_blobs.push(digest); } - blobs.insert( - digest.clone(), - Value::String(app.cache_store.signed_url(&digest, Method::Get).await?), - ); } + served_keys.push(key.clone()); hits.insert( key, json!({ @@ -264,6 +374,25 @@ async fn lookup_hits( }), ); } + if counts && !served_keys.is_empty() { + // Only what the response serves counts and is kept alive. + client + .execute( + "UPDATE cache.manifests \ + SET last_used_at = now(), hits = hits + 1, last_hit_at = now() \ + WHERE repo = $1 AND key = ANY($2)", + &[&repo, &served_keys], + ) + .await?; + served_blobs.sort(); + served_blobs.dedup(); + client + .execute( + "UPDATE cache.blobs SET last_used_at = now() WHERE sha256 = ANY($1)", + &[&served_blobs], + ) + .await?; + } Ok(hits) } diff --git a/crates/aster-srv/src/cache/signing.rs b/crates/aster-srv/src/cache/signing.rs index 203e094..3679576 100644 --- a/crates/aster-srv/src/cache/signing.rs +++ b/crates/aster-srv/src/cache/signing.rs @@ -119,6 +119,96 @@ impl Signer for IamSigner { } } +/// Bounds on `signBlob` calls, which share one IAM quota for the whole +/// project. Callers wait for a slot and a token; one that would wait longer +/// than `max_wait` fails with "signing is busy" instead of queueing without +/// bound, and the lookup route turns that into a miss. +#[derive(Debug, Clone, Copy)] +pub struct SignLimits { + /// `signBlob` calls in flight at once. + pub concurrency: usize, + /// Calls per second once the burst is spent. + pub per_second: f64, + /// Calls that may go back to back. + pub burst: f64, + /// Longest a call waits for a token. + pub max_wait: Duration, +} + +impl Default for SignLimits { + fn default() -> Self { + Self { + concurrency: 16, + per_second: 200.0, + burst: 512.0, + max_wait: Duration::from_secs(2), + } + } +} + +/// A semaphore plus a token bucket over [`SignLimits`]. +pub struct SignLimiter { + limits: SignLimits, + slots: tokio::sync::Semaphore, + bucket: std::sync::Mutex<(f64, std::time::Instant)>, +} + +impl SignLimiter { + pub fn new(limits: SignLimits) -> Self { + Self { + slots: tokio::sync::Semaphore::new(limits.concurrency.max(1)), + bucket: std::sync::Mutex::new((limits.burst.max(1.0), std::time::Instant::now())), + limits, + } + } + + /// Take one token, waiting up to `max_wait` for it. + async fn take_token(&self) -> Result<()> { + loop { + let wait = { + let mut bucket = self.bucket.lock().expect("sign bucket lock"); + let now = std::time::Instant::now(); + let refill = now.duration_since(bucket.1).as_secs_f64() * self.limits.per_second; + bucket.0 = (bucket.0 + refill).min(self.limits.burst.max(1.0)); + bucket.1 = now; + if bucket.0 >= 1.0 { + bucket.0 -= 1.0; + return Ok(()); + } + Duration::from_secs_f64((1.0 - bucket.0) / self.limits.per_second.max(0.001)) + }; + if wait > self.limits.max_wait { + anyhow::bail!("signing is busy"); + } + tokio::time::sleep(wait).await; + } + } +} + +/// A [`Signer`] that goes through a [`SignLimiter`]. +pub struct Limited<'a> { + pub inner: &'a dyn Signer, + pub limiter: &'a SignLimiter, +} + +impl Signer for Limited<'_> { + fn sign<'a>(&'a self, payload: &'a [u8]) -> BoxFuture<'a, Result>> { + Box::pin(async move { + let _slot = + tokio::time::timeout(self.limiter.limits.max_wait, self.limiter.slots.acquire()) + .await + .map_err(|_| anyhow::anyhow!("signing is busy"))? + .map_err(|_| anyhow::anyhow!("signing is closed"))?; + self.limiter.take_token().await?; + self.inner.sign(payload).await + }) + } + + fn service_account_email(&self) -> &str { + self.inner.service_account_email() + } +} + /// A URL the client uses directly against GCS. pub async fn signed_url( signer: &dyn Signer, @@ -315,6 +405,111 @@ impl Signer for FixedSigner { } } +#[cfg(test)] +mod limiter_tests { + use super::*; + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + + struct Slow { + running: AtomicUsize, + peak: AtomicUsize, + calls: AtomicUsize, + } + + impl Signer for Slow { + fn sign<'a>(&'a self, _: &'a [u8]) -> BoxFuture<'a, Result>> { + Box::pin(async move { + let now = self.running.fetch_add(1, Ordering::SeqCst) + 1; + self.peak.fetch_max(now, Ordering::SeqCst); + self.calls.fetch_add(1, Ordering::SeqCst); + tokio::time::sleep(Duration::from_millis(20)).await; + self.running.fetch_sub(1, Ordering::SeqCst); + Ok(vec![1]) + }) + } + fn service_account_email(&self) -> &str { + "a@b.iam.gserviceaccount.com" + } + } + + fn slow() -> Arc { + Arc::new(Slow { + running: AtomicUsize::new(0), + peak: AtomicUsize::new(0), + calls: AtomicUsize::new(0), + }) + } + + #[tokio::test] + async fn concurrent_signatures_never_exceed_the_limit() { + let inner = slow(); + let limiter = Arc::new(SignLimiter::new(SignLimits { + concurrency: 3, + per_second: 10_000.0, + burst: 10_000.0, + max_wait: Duration::from_secs(5), + })); + let mut tasks = Vec::new(); + for _ in 0..12 { + let (inner, limiter) = (inner.clone(), limiter.clone()); + tasks.push(tokio::spawn(async move { + let limited = Limited { + inner: inner.as_ref(), + limiter: &limiter, + }; + limited.sign(b"x").await.unwrap(); + })); + } + for task in tasks { + task.await.unwrap(); + } + assert_eq!(inner.calls.load(Ordering::SeqCst), 12); + assert!(inner.peak.load(Ordering::SeqCst) <= 3); + } + + #[tokio::test] + async fn an_exhausted_bucket_fails_fast_instead_of_queueing() { + let inner = slow(); + let limiter = SignLimiter::new(SignLimits { + concurrency: 4, + per_second: 0.5, + burst: 2.0, + max_wait: Duration::from_millis(100), + }); + let limited = Limited { + inner: inner.as_ref(), + limiter: &limiter, + }; + limited.sign(b"x").await.unwrap(); + limited.sign(b"x").await.unwrap(); + let started = std::time::Instant::now(); + let error = limited.sign(b"x").await.unwrap_err(); + assert!(error.to_string().contains("busy"), "{error}"); + assert!(started.elapsed() < Duration::from_millis(100)); + assert_eq!(inner.calls.load(Ordering::SeqCst), 2); + } + + #[tokio::test] + async fn the_bucket_refills_over_time() { + let inner = slow(); + let limiter = SignLimiter::new(SignLimits { + concurrency: 4, + per_second: 100.0, + burst: 1.0, + max_wait: Duration::from_secs(1), + }); + let limited = Limited { + inner: inner.as_ref(), + limiter: &limiter, + }; + // The second call waits about 10ms for a token rather than failing. + limited.sign(b"x").await.unwrap(); + limited.sign(b"x").await.unwrap(); + assert_eq!(inner.calls.load(Ordering::SeqCst), 2); + } +} + #[cfg(test)] mod tests { //! Google's published V4 signing vectors diff --git a/crates/aster-srv/src/cache/store.rs b/crates/aster-srv/src/cache/store.rs index 77c8de2..319c0d4 100644 --- a/crates/aster-srv/src/cache/store.rs +++ b/crates/aster-srv/src/cache/store.rs @@ -8,14 +8,14 @@ use std::collections::{HashMap, HashSet}; use std::sync::Arc; -use std::time::{Duration, SystemTime}; +use std::time::{Duration, Instant, SystemTime}; use anyhow::{Context, Result}; use bytes::Bytes; use chrono::{DateTime, Utc}; use tokio::sync::{Mutex, OnceCell}; -use super::signing::{self, Method, Signer}; +use super::signing::{self, Limited, Method, SignLimiter, SignLimits, Signer}; const STORAGE_SCOPE: &str = "https://www.googleapis.com/auth/devstorage.read_write"; const RETENTION_LOCK: i64 = 0x6173_7465_722d_6363; // "aster-cc" @@ -25,6 +25,88 @@ const RETENTION_LOCK: i64 = 0x6173_7465_722d_6363; // "aster-cc" /// deleted before the manifest naming it is visible. pub(crate) const BLOB_LOCK: i64 = 0x6173_7465_722d_6362; // "aster-cb" +/// Tuning for the lookup fan-out: what the store remembers about a blob and +/// how hard it may push GCS and IAM. +#[derive(Debug, Clone, Copy)] +pub struct FanoutConfig { + /// How long a successful HEAD is trusted. Blobs are content-addressed and + /// immutable, and a blob a manifest names is never retained away, so a + /// positive answer can be long-lived. Misses are never cached: a blob + /// that was just uploaded must be seen at once. + pub present_ttl: Duration, + /// A cached signed GET URL is handed out only while at least this much of + /// its [`signing::URL_TTL`] is left, so the client has time to download. + pub reuse_margin: Duration, + /// HEAD requests in flight at once, across all requests. + pub head_concurrency: usize, + /// Digests remembered; idle ones are dropped when this is exceeded. + pub max_slots: usize, + /// Longest one lookup spends resolving blobs. An entry with a blob not + /// resolved by then is answered as a miss, so a cold or throttled lookup + /// still replies inside the client's request timeout (5s since 0.22) + /// instead of timing out and losing every hit in the batch. + pub lookup_budget: Duration, + pub sign: SignLimits, +} + +impl Default for FanoutConfig { + fn default() -> Self { + Self { + present_ttl: Duration::from_secs(60 * 60), + reuse_margin: Duration::from_secs(5 * 60), + head_concurrency: 64, + max_slots: 20_000, + lookup_budget: Duration::from_secs(3), + sign: SignLimits::default(), + } + } +} + +/// What is remembered about one digest. One async lock per digest also makes +/// concurrent lookups of the same blob share a single HEAD and signature. +#[derive(Default)] +struct Slot { + present_until: Option, + /// A signed GET URL and when it stops being valid. + url: Option<(String, Instant)>, +} + +/// State shared by every request: the per-digest memory and the limits on +/// calls to GCS and IAM. +pub struct Fanout { + config: FanoutConfig, + slots: std::sync::Mutex>>>, + heads: tokio::sync::Semaphore, + signing: SignLimiter, + /// HEAD requests actually issued. For tests. + head_calls: std::sync::atomic::AtomicU64, +} + +impl Fanout { + fn new(config: FanoutConfig) -> Self { + Self { + heads: tokio::sync::Semaphore::new(config.head_concurrency.max(1)), + signing: SignLimiter::new(config.sign), + slots: Default::default(), + head_calls: Default::default(), + config, + } + } + + fn slot(&self, sha256: &str) -> Arc> { + let mut slots = self.slots.lock().expect("slot map lock"); + if slots.len() >= self.config.max_slots && !slots.contains_key(sha256) { + // Drop what no request is using; they refill on demand. + slots.retain(|_, slot| Arc::strong_count(slot) > 1); + } + Arc::clone(slots.entry(sha256.to_string()).or_default()) + } + + fn forget(&self, sha256: &str) { + self.slots.lock().expect("slot map lock").remove(sha256); + } +} + /// What one retention pass removed. `None` from [`run_retention`] means /// another replica held the lock. #[derive(Debug, Default, PartialEq, Eq)] @@ -44,12 +126,16 @@ pub enum BlobStore { auth: OnceCell>, signer_email: String, signer: OnceCell>, + fanout: Fanout, }, /// Objects in memory, signed by `signer`. For tests. Memory { bucket: String, objects: Mutex)>>, signer: Arc, + fanout: Fanout, + /// Added to every HEAD, to stand in for GCS latency. For tests. + head_latency: Duration, }, } @@ -67,6 +153,7 @@ impl BlobStore { auth: OnceCell::new(), signer_email, signer: OnceCell::new(), + fanout: Fanout::new(FanoutConfig::default()), }, _ => BlobStore::Disabled, } @@ -74,13 +161,46 @@ impl BlobStore { /// An in-memory bucket. For tests. pub fn memory(bucket: impl Into, signer: Arc) -> Self { + Self::memory_with(bucket, signer, FanoutConfig::default(), Duration::ZERO) + } + + /// An in-memory bucket with chosen fan-out limits, and `head_latency` + /// added to every HEAD. For tests. + pub fn memory_with( + bucket: impl Into, + signer: Arc, + config: FanoutConfig, + head_latency: Duration, + ) -> Self { BlobStore::Memory { bucket: bucket.into(), objects: Mutex::new(HashMap::new()), signer, + fanout: Fanout::new(config), + head_latency, + } + } + + fn fanout(&self) -> Option<&Fanout> { + match self { + BlobStore::Disabled => None, + BlobStore::Gcs { fanout, .. } | BlobStore::Memory { fanout, .. } => Some(fanout), } } + /// How long one lookup may spend resolving blobs; see + /// [`FanoutConfig::lookup_budget`]. `None` without a bucket. + pub fn lookup_budget(&self) -> Option { + self.fanout().map(|fanout| fanout.config.lookup_budget) + } + + /// HEAD requests issued so far (cache hits excluded). For tests. + pub fn head_calls(&self) -> u64 { + self.fanout().map_or(0, |fanout| { + fanout.head_calls.load(std::sync::atomic::Ordering::SeqCst) + }) + } + /// Whether this is the in-memory store tests use in place of GCS. pub fn is_memory(&self) -> bool { matches!(self, BlobStore::Memory { .. }) @@ -197,11 +317,31 @@ impl BlobStore { !matches!(self, BlobStore::Disabled) } - /// Whether `cas/` exists. + /// Whether `cas/` exists, asked of GCS now. pub async fn exists(&self, sha256: &str) -> Result { + let Some(fanout) = self.fanout() else { + return Ok(false); + }; + let _permit = fanout + .heads + .acquire() + .await + .context("HEAD limiter closed")?; + fanout + .head_calls + .fetch_add(1, std::sync::atomic::Ordering::SeqCst); match self { BlobStore::Disabled => Ok(false), - BlobStore::Memory { objects, .. } => Ok(objects.lock().await.contains_key(sha256)), + BlobStore::Memory { + objects, + head_latency, + .. + } => { + if !head_latency.is_zero() { + tokio::time::sleep(*head_latency).await; + } + Ok(objects.lock().await.contains_key(sha256)) + } BlobStore::Gcs { client, .. } => { let (bucket, token) = self.gcs_auth().await?; let response = client @@ -221,6 +361,38 @@ impl BlobStore { } } + /// A signed GET URL for `cas/` when the object exists, else + /// `None`. For lookups: a positive HEAD is remembered for + /// [`FanoutConfig::present_ttl`] and a signed URL is reused while enough + /// of its lifetime is left, so a warm digest costs no GCS or IAM call. + /// Callers asking for the same digest at once share one HEAD and one + /// signature. Commit and upload keep using [`BlobStore::exists`]. + pub async fn readable_url(&self, sha256: &str) -> Result> { + let Some(fanout) = self.fanout() else { + return Ok(None); + }; + let slot = fanout.slot(sha256); + let mut slot = slot.lock().await; + let now = Instant::now(); + if slot.present_until.is_none_or(|until| until <= now) { + slot.present_until = None; + slot.url = None; + if !self.exists(sha256).await? { + return Ok(None); + } + slot.present_until = Some(Instant::now() + fanout.config.present_ttl); + } + if let Some((url, valid_until)) = &slot.url { + if *valid_until > Instant::now() + fanout.config.reuse_margin { + return Ok(Some(url.clone())); + } + } + let signed_at = Instant::now(); + let url = self.signed_url(sha256, Method::Get).await?; + slot.url = Some((url.clone(), signed_at + signing::URL_TTL)); + Ok(Some(url)) + } + /// The sha256 and length of `cas/`'s bytes, or `None` when there /// is no such object. pub async fn hash(&self, name: &str) -> Result> { @@ -313,6 +485,9 @@ impl BlobStore { /// Delete `cas/`. Missing is success. pub async fn delete(&self, sha256: &str) -> Result<()> { + if let Some(fanout) = self.fanout() { + fanout.forget(sha256); + } match self { BlobStore::Disabled => Ok(()), BlobStore::Memory { objects, .. } => { @@ -341,8 +516,16 @@ impl BlobStore { /// A signed URL for `cas/`, valid for [`signing::URL_TTL`]. pub async fn signed_url(&self, sha256: &str, method: Method) -> Result { let (bucket, signer) = self.signer().await?; + let limiter = &self + .fanout() + .context("no cache bucket is configured")? + .signing; + let limited = Limited { + inner: signer.as_ref(), + limiter, + }; signing::signed_url( - signer.as_ref(), + &limited, &bucket, &object_name(sha256), method, @@ -521,3 +704,143 @@ pub fn spawn(app: Arc) { } }); } + +#[cfg(test)] +mod tests { + use super::*; + use crate::cache::FixedSigner; + + const A: &str = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"; + + fn store(config: FanoutConfig, latency: Duration) -> (BlobStore, Arc) { + let signer = Arc::new(FixedSigner { + email: "t@example.iam.gserviceaccount.com".into(), + signature: b"sig".to_vec(), + signed: Default::default(), + }); + let store = BlobStore::memory_with("bucket", signer.clone(), config, latency); + (store, signer) + } + + fn signatures(signer: &FixedSigner) -> usize { + signer.signed.lock().unwrap().len() + } + + #[tokio::test] + async fn a_warm_digest_costs_no_head_and_no_signature() { + let (store, signer) = store(FanoutConfig::default(), Duration::ZERO); + store + .put_unrecorded(A, Bytes::from_static(b"x")) + .await + .unwrap(); + let first = store.readable_url(A).await.unwrap().unwrap(); + let second = store.readable_url(A).await.unwrap().unwrap(); + assert_eq!(first, second); + assert_eq!(store.head_calls(), 1); + assert_eq!(signatures(&signer), 1); + } + + #[tokio::test] + async fn a_missing_blob_is_never_cached() { + let (store, signer) = store(FanoutConfig::default(), Duration::ZERO); + assert_eq!(store.readable_url(A).await.unwrap(), None); + assert_eq!(store.readable_url(A).await.unwrap(), None); + assert_eq!(store.head_calls(), 2); + assert_eq!(signatures(&signer), 0); + // Uploaded after the miss, it is seen at once. + store + .put_unrecorded(A, Bytes::from_static(b"x")) + .await + .unwrap(); + assert!(store.readable_url(A).await.unwrap().is_some()); + } + + #[tokio::test] + async fn a_url_without_enough_lifetime_left_is_signed_again() { + let config = FanoutConfig { + reuse_margin: signing::URL_TTL + Duration::from_secs(1), + ..FanoutConfig::default() + }; + let (store, signer) = store(config, Duration::ZERO); + store + .put_unrecorded(A, Bytes::from_static(b"x")) + .await + .unwrap(); + store.readable_url(A).await.unwrap().unwrap(); + store.readable_url(A).await.unwrap().unwrap(); + // The HEAD is still remembered; only the signature is redone. + assert_eq!(store.head_calls(), 1); + assert_eq!(signatures(&signer), 2); + } + + #[tokio::test] + async fn a_remembered_blob_is_asked_about_again_after_its_ttl() { + let config = FanoutConfig { + present_ttl: Duration::from_millis(30), + ..FanoutConfig::default() + }; + let (store, _) = store(config, Duration::ZERO); + store + .put_unrecorded(A, Bytes::from_static(b"x")) + .await + .unwrap(); + assert!(store.readable_url(A).await.unwrap().is_some()); + if let BlobStore::Memory { objects, .. } = &store { + objects.lock().await.remove(A); + } + tokio::time::sleep(Duration::from_millis(60)).await; + assert_eq!(store.readable_url(A).await.unwrap(), None); + assert_eq!(store.head_calls(), 2); + } + + #[tokio::test] + async fn delete_forgets_a_remembered_blob() { + let (store, _) = store(FanoutConfig::default(), Duration::ZERO); + store + .put_unrecorded(A, Bytes::from_static(b"x")) + .await + .unwrap(); + assert!(store.readable_url(A).await.unwrap().is_some()); + store.delete(A).await.unwrap(); + assert_eq!(store.readable_url(A).await.unwrap(), None); + } + + #[tokio::test] + async fn concurrent_lookups_of_one_digest_share_one_head_and_signature() { + let (store, signer) = store(FanoutConfig::default(), Duration::from_millis(20)); + store + .put_unrecorded(A, Bytes::from_static(b"x")) + .await + .unwrap(); + let store = Arc::new(store); + let mut tasks = Vec::new(); + for _ in 0..20 { + let store = store.clone(); + tasks.push(tokio::spawn(async move { store.readable_url(A).await })); + } + for task in tasks { + assert!(task.await.unwrap().unwrap().is_some()); + } + assert_eq!(store.head_calls(), 1); + assert_eq!(signatures(&signer), 1); + } + + #[tokio::test] + async fn idle_slots_are_dropped_past_the_cap() { + let config = FanoutConfig { + max_slots: 4, + ..FanoutConfig::default() + }; + let (store, _) = store(config, Duration::ZERO); + for index in 0..20 { + let digest = format!("{index:064x}"); + store + .put_unrecorded(&digest, Bytes::from_static(b"x")) + .await + .unwrap(); + store.readable_url(&digest).await.unwrap().unwrap(); + } + let slots = store.fanout().unwrap().slots.lock().unwrap().len(); + assert!(slots <= 4, "{slots} slots kept"); + } +} diff --git a/crates/aster-srv/src/cli_auth.rs b/crates/aster-srv/src/cli_auth.rs index 43a433c..b0a93b2 100644 --- a/crates/aster-srv/src/cli_auth.rs +++ b/crates/aster-srv/src/cli_auth.rs @@ -349,6 +349,12 @@ fn assertion_is_recent(assertion: Option<&str>) -> bool { iat.is_some_and(|iat| iat <= now + 60 && now - iat <= IAP_MAX_AGE_SECONDS) } +/// Shown on the confirm page in copy mode, where the next page hands the +/// person a code to type into a terminal. +const COPY_CONFIRM_WARNING: &str = "

Only continue if you started this command \ + yourself, in a terminal you are using right now. A code from this page signs that \ + terminal in as you.

"; + /// `GET /cli-auth`: ask the person to confirm. pub async fn confirm(app: &App, browser: &Browser, query: Option<&str>) -> Reply { let Some(tokens) = app.person_tokens.as_ref().filter(|_| enabled(app)) else { @@ -385,9 +391,18 @@ pub async fn confirm(app: &App, browser: &Browser, query: Option<&str>) -> Reply }; let origin_details = format!( "
    {}{}{}
  • Request: {}
", - detail("From address", Some(browser.client_ip.to_string())), - detail("Browser", short_user_agent(browser.user_agent.as_deref())), - detail("Command run on", request.host.clone()), + detail( + "Your browser's address (seen by this server)", + Some(browser.client_ip.to_string()), + ), + detail( + "Your browser (as it describes itself)", + short_user_agent(browser.user_agent.as_deref()), + ), + detail( + "Device name (reported by the requesting computer, not verified)", + request.host.clone(), + ), escape(request_id(&request.state)), ); let mut fields = vec![ @@ -410,11 +425,18 @@ pub async fn confirm(app: &App, browser: &Browser, query: Option<&str>) -> Reply {origin_details}\

It can read cache entries only, and the sign-in ends after a \ few hours. If you did not just run aster cache login, or these details \ - are not yours, close this tab.

\ + are not yours, close this tab. The device name comes from the command that sent you \ + here, so anyone can make it say anything. Trust the code above, not the name.

\ + {copy_warning}\
{fields}\
", user_code = escape(&display_user_code(&request.user_code)), email = escape(&viewer.email), + copy_warning = if request.mode == Mode::Copy { + COPY_CONFIRM_WARNING + } else { + "" + }, fields = fields.concat(), ); Reply::html(200, bare_page("Authorize aster CLI", &body)) @@ -496,6 +518,8 @@ pub async fn authorize(app: &App, browser: &Browser, body: &[u8]) -> Reply { "

Paste this code into the terminal where you ran \ aster cache login (request {id}):

\ 
{code}
\ +

Never send this code to anyone. Whoever has it can \ + sign in as you until it expires. Paste it only into your own terminal.

\

It works once and expires in {CODE_SECONDS} seconds. \ It is not your token and gives nothing without that terminal.

", id = escape(request_id(&request.state)), diff --git a/crates/aster-srv/src/config.rs b/crates/aster-srv/src/config.rs index 41aeb08..30ed3ec 100644 --- a/crates/aster-srv/src/config.rs +++ b/crates/aster-srv/src/config.rs @@ -72,6 +72,11 @@ pub struct Config { /// `ASTER_SRV_MAX_BODY_BYTES`: the largest upload accepted, compressed /// or not. Default 64 MiB. pub max_body_bytes: usize, + /// `ASTER_SRV_MAX_LARGE_BODIES`: how many large request bodies (a cache + /// commit's manifest, a test-results upload) one replica reads at once. + /// Each can weigh `max_body_bytes`; past the limit a request gets `503` + /// with `Retry-After`. Default 8. + pub max_large_bodies: usize, /// `ASTER_SRV_IAP_AUDIENCE`: the Google IAP audience the dashboard (`/ui`) /// accepts: exactly `/projects/NUMBER/global/backendServices/ID`. A /// project-wide prefix is refused at startup, since it would accept an @@ -128,7 +133,7 @@ pub struct Config { /// rotated out by dropping its pair. Unset turns person login off. pub cli_token_keys: Vec, /// `ASTER_SRV_CLI_TOKEN_TTL`: seconds a person token lives, 60 to - /// 604800. Default 43200 (12 hours). + /// 43200 ([`crate::person::MAX_TTL_SECONDS`]). Default 43200 (12 hours). pub cli_token_ttl: u64, /// `ASTER_SRV_API_ADDR`: a second listener that serves only `/v1/*`, for /// the load balancer backend that has no IAP in front of it. Unset @@ -153,6 +158,23 @@ pub struct Config { /// `ASTER_SRV_BODY_TIMEOUT_SECS`: how long a request body may take to /// arrive in full. Default 60. pub body_timeout: Duration, + /// `ASTER_SRV_UNAUTH_BODY_TIMEOUT_SECS`: how long the body of an + /// unauthenticated route (`POST /v1/auth/token`, `POST /cli-auth`, both + /// at most 8 KiB) may take to arrive, at most `body_timeout`. Default 8. + pub unauth_body_timeout: Duration, + /// `ASTER_SRV_MAX_CONNECTIONS_PER_IP`: requests in flight at once from + /// one client IP (as [`crate::limits::client_ip`] resolves it; IPv6 by + /// /64), per replica; more get `429`. With `trusted_proxy_hops` 0, where + /// the TCP peer is the client, it also caps open connections from one + /// peer. Default 128. + pub max_connections_per_ip: usize, + /// `ASTER_SRV_LOG_SAMPLE_FIRST`: failure audit lines logged per + /// `(event, reason, route, client_ip)` in each window before the rest + /// are only counted and summarised. Default 5. + pub log_sample_first: u32, + /// `ASTER_SRV_LOG_SAMPLE_WINDOW_SECS`: the length of that window, and + /// how often a summary line is written. Default 60. + pub log_sample_window: Duration, /// `ASTER_SRV_HANDLER_TIMEOUT_SECS`: how long a request may run from the /// end of its headers to its reply, body read included. Default 120. pub handler_timeout: Duration, @@ -359,7 +381,7 @@ impl Config { that may write); set ASTER_SRV_ALLOW_ANY_WRITER_WORKFLOW=1 only in development" ); } - let owner_ids = OwnerIds::parse(var("ASTER_SRV_REPOSITORY_OWNER_IDS").as_deref())?; + let mut owner_ids = OwnerIds::parse(var("ASTER_SRV_REPOSITORY_OWNER_IDS").as_deref())?; let public_url = match var("ASTER_SRV_PUBLIC_URL") { Some(url) => { let url = url.trim().trim_end_matches('/').to_string(); @@ -413,6 +435,7 @@ impl Config { if api_listen.is_some_and(|api| api.port() != 0 && api == listen) { bail!("ASTER_SRV_API_ADDR must differ from ASTER_SRV_LISTEN"); } + let dev_blob_route = var("ASTER_SRV_DEV_BLOB_ROUTE").as_deref() == Some("1"); let number = |name: &str, default: u64, range: std::ops::RangeInclusive| match var(name) { Some(value) => value @@ -437,13 +460,76 @@ impl Config { let rate_api_subject_per_min = number("ASTER_SRV_RATE_API_SUBJECT_PER_MIN", 3000, 1..=1_000_000)? as u32; let max_connections = number("ASTER_SRV_MAX_CONNECTIONS", 1024, 1..=1_000_000)? as usize; + let max_large_bodies = number("ASTER_SRV_MAX_LARGE_BODIES", 8, 1..=1024)? as usize; let db_wait = Duration::from_secs(number("ASTER_SRV_DB_WAIT_SECS", 5, 1..=300)?); let header_timeout = Duration::from_secs(number("ASTER_SRV_HEADER_TIMEOUT_SECS", 20, 1..=300)?); let body_timeout = Duration::from_secs(number("ASTER_SRV_BODY_TIMEOUT_SECS", 60, 1..=3600)?); + let unauth_body_timeout = + Duration::from_secs(number("ASTER_SRV_UNAUTH_BODY_TIMEOUT_SECS", 8, 1..=60)?) + .min(body_timeout); + let log_sample_first = number("ASTER_SRV_LOG_SAMPLE_FIRST", 5, 1..=1000)? as u32; + let log_sample_window = + Duration::from_secs(number("ASTER_SRV_LOG_SAMPLE_WINDOW_SECS", 60, 1..=3600)?); + let max_connections_per_ip = + number("ASTER_SRV_MAX_CONNECTIONS_PER_IP", 128, 1..=1_000_000)? as usize; let handler_timeout = Duration::from_secs(number("ASTER_SRV_HANDLER_TIMEOUT_SECS", 120, 1..=3600)?); + // The `/v1` listener is reachable without IAP. It starts only with the + // settings that make a CI token mean what it says, and says every + // missing one at once. + if api_listen.is_some() { + let mut missing: Vec = Vec::new(); + if !static_tokens.is_empty() { + missing.push( + "ASTER_SRV_STATIC_TOKENS must be unset: a static token skips the OIDC claim \ + checks" + .into(), + ); + } + if dev_blob_route { + missing.push( + "ASTER_SRV_DEV_BLOB_ROUTE must be unset: it serves unauthenticated blob \ + reads and writes" + .into(), + ); + } + let unpinned: Vec<&str> = repositories + .entries() + .iter() + .map(String::as_str) + .filter(|entry| !owner_ids.covers_entry(entry)) + .collect(); + if !unpinned.is_empty() { + missing.push(format!( + "ASTER_SRV_REPOSITORY_OWNER_IDS must pin every ASTER_SRV_REPOSITORIES entry \ + (owner/name=id or owner/*=id); not pinned: {}", + unpinned.join(", ") + )); + } + if writer_workflows.is_none() { + missing.push( + "ASTER_SRV_CACHE_WRITER_WORKFLOWS must name the workflows that may write \ + (ASTER_SRV_ALLOW_ANY_WRITER_WORKFLOW does not apply here)" + .into(), + ); + } + if trusted_proxy_hops == 0 { + missing.push( + "ASTER_SRV_TRUSTED_PROXY_HOPS must be at least 1: behind the load balancer \ + every client would share its address and its rate limits" + .into(), + ); + } + if !missing.is_empty() { + bail!( + "ASTER_SRV_API_ADDR serves /v1 without IAP and needs:\n - {}", + missing.join("\n - ") + ); + } + owner_ids = owner_ids.failing_closed(); + } // Sign-in hands out tokens to whoever IAP names, so it needs IAP to be // real and the dashboard's domain allowlist to be set. Without them a // development viewer (no IAP) or any Google identity would get one. @@ -478,6 +564,7 @@ impl Config { default_branch, broken_runs, max_body_bytes, + max_large_bodies, iap_audience, ui_email_domains, ui_dev_email: var("ASTER_SRV_UI_DEV_EMAIL").map(|email| email.trim().to_string()), @@ -489,7 +576,7 @@ impl Config { cache_writers, writer_workflows, owner_ids, - dev_blob_route: var("ASTER_SRV_DEV_BLOB_ROUTE").as_deref() == Some("1"), + dev_blob_route, public_url, cli_token_keys, cli_token_ttl, @@ -499,6 +586,10 @@ impl Config { rate_api_ip_per_min, rate_api_subject_per_min, body_timeout, + unauth_body_timeout, + max_connections_per_ip, + log_sample_first, + log_sample_window, handler_timeout, header_timeout, max_connections, @@ -658,6 +749,11 @@ impl RepositoryAllowlist { Ok(Self(entries)) } + /// The entries, lowercased: `owner/name` or `owner/*`. + pub fn entries(&self) -> &[String] { + &self.0 + } + /// Whether `repository` (`owner/name`, any case) is allowed. pub fn allows(&self, repository: &str) -> bool { let repository = repository.to_ascii_lowercase(); @@ -675,7 +771,13 @@ impl RepositoryAllowlist { /// `repository_owner_id` pins: `owner/name` or `owner/*` to a GitHub id. #[derive(Debug, Clone, Default, PartialEq, Eq)] -pub struct OwnerIds(Vec<(String, String)>); +pub struct OwnerIds { + pins: Vec<(String, String)>, + /// A repository no pin covers is refused. Set when the `/v1` listener + /// (`ASTER_SRV_API_ADDR`) is on, so a gap in the pins cannot let a + /// recreated repository in. + require_pin: bool, +} impl OwnerIds { pub fn parse(value: Option<&str>) -> Result { @@ -701,21 +803,46 @@ impl OwnerIds { } pins.push((repo, id.to_string())); } - Ok(Self(pins)) + Ok(Self { + pins, + require_pin: false, + }) + } + + /// The same pins, with a repository no pin covers refused. + pub fn failing_closed(mut self) -> Self { + self.require_pin = true; + self } /// Whether `repository` with this `repository_owner_id` claim is - /// acceptable: no pin covers it, or the claim equals the pin. + /// acceptable: the claim equals every pin that covers it, and some pin + /// covers it when pins are required. pub fn accepts(&self, repository: &str, claim: Option<&str>) -> bool { let repository = repository.to_ascii_lowercase(); let owner = repository.split_once('/').map(|(owner, _)| owner); - self.0 + let mut covering = self + .pins .iter() .filter(|(pattern, _)| { *pattern == repository || pattern.strip_suffix("/*").is_some_and(|o| Some(o) == owner) }) - .all(|(_, id)| claim == Some(id.as_str())) + .peekable(); + if covering.peek().is_none() { + return !self.require_pin; + } + covering.all(|(_, id)| claim == Some(id.as_str())) + } + + /// Whether an allowlist entry (`owner/name` or `owner/*`) is pinned for + /// every repository it names: an exact pin for `owner/name`, or an + /// `owner/*` pin for either form. + fn covers_entry(&self, entry: &str) -> bool { + let owner = entry.split_once('/').map(|(owner, _)| owner); + self.pins.iter().any(|(pattern, _)| { + pattern == entry || pattern.strip_suffix("/*").is_some_and(|o| Some(o) == owner) + }) } } @@ -1101,6 +1228,9 @@ mod tests { assert_eq!(config.max_connections, 1024); assert_eq!(config.header_timeout, Duration::from_secs(20)); assert_eq!(config.db_wait, Duration::from_secs(5)); + assert_eq!(config.unauth_body_timeout, Duration::from_secs(8)); + assert_eq!(config.max_connections_per_ip, 128); + assert_eq!(config.max_large_bodies, 8); let with = |extra: &[(&str, &str)]| { let mut pairs = REQUIRED.to_vec(); pairs.extend_from_slice(extra); @@ -1108,6 +1238,11 @@ mod tests { }; let config = with(&[ ("ASTER_SRV_API_ADDR", "0.0.0.0:8081"), + ("ASTER_SRV_REPOSITORY_OWNER_IDS", "ArchAstro/*=4242"), + ( + "ASTER_SRV_CACHE_WRITER_WORKFLOWS", + "ArchAstro/x/.github/workflows/ci.yml@", + ), ("ASTER_SRV_TRUSTED_PROXY_HOPS", "2"), ("ASTER_SRV_RATE_TOKEN_IP_PER_MIN", "5"), ("ASTER_SRV_MAX_CONNECTIONS", "50"), @@ -1131,6 +1266,159 @@ mod tests { } } + /// Everything `ASTER_SRV_API_ADDR` needs, so a test can take one away. + const API_READY: [(&str, &str); 5] = [ + ("ASTER_SRV_API_ADDR", "127.0.0.1:8081"), + ("ASTER_SRV_REPOSITORY_OWNER_IDS", "ArchAstro/*=4242"), + ( + "ASTER_SRV_CACHE_WRITER_WORKFLOWS", + "ArchAstro/firstlanding/.github/workflows/ci.yml@", + ), + ("ASTER_SRV_TRUSTED_PROXY_HOPS", "1"), + ("ASTER_SRV_REPOSITORIES", "ArchAstro/*"), + ]; + + fn api_config(without: &[&str], extra: &[(&str, &str)]) -> Result { + let mut pairs = vec![("ASTER_SRV_DATABASE_URL", "postgres://localhost/aster")]; + pairs.extend(API_READY.iter().filter(|(name, _)| !without.contains(name))); + pairs.extend_from_slice(extra); + Config::from_env(env_of(&pairs)) + } + + /// N4: the listener that has no IAP in front of it starts only when + /// each of these holds, and one run names every missing one. + #[test] + fn the_api_listener_refuses_to_start_without_each_hardening_setting() { + let ok = api_config(&[], &[]).unwrap(); + assert!(ok.api_listen.is_some()); + let refused = |without: &[&str], extra: &[(&str, &str)], wanted: &str| { + let err = format!("{:#}", api_config(without, extra).unwrap_err()); + assert!(err.contains(wanted), "{wanted}: {err}"); + assert!(err.contains("ASTER_SRV_API_ADDR"), "{err}"); + }; + refused( + &[], + &[( + "ASTER_SRV_STATIC_TOKENS", + "ArchAstro/dev=dev-token-0123456789", + )], + "ASTER_SRV_STATIC_TOKENS", + ); + refused( + &[], + &[("ASTER_SRV_DEV_BLOB_ROUTE", "1")], + "ASTER_SRV_DEV_BLOB_ROUTE", + ); + refused( + &["ASTER_SRV_REPOSITORY_OWNER_IDS"], + &[], + "not pinned: archastro/*", + ); + refused( + &["ASTER_SRV_CACHE_WRITER_WORKFLOWS"], + &[], + "ASTER_SRV_CACHE_WRITER_WORKFLOWS", + ); + // The development escape hatch does not apply to this listener. + refused( + &["ASTER_SRV_CACHE_WRITER_WORKFLOWS"], + &[("ASTER_SRV_ALLOW_ANY_WRITER_WORKFLOW", "1")], + "ASTER_SRV_CACHE_WRITER_WORKFLOWS", + ); + refused( + &["ASTER_SRV_TRUSTED_PROXY_HOPS"], + &[("ASTER_SRV_TRUSTED_PROXY_HOPS", "0")], + "ASTER_SRV_TRUSTED_PROXY_HOPS", + ); + // Several at once are all listed. + let err = format!( + "{:#}", + api_config( + &[ + "ASTER_SRV_REPOSITORY_OWNER_IDS", + "ASTER_SRV_CACHE_WRITER_WORKFLOWS" + ], + &[("ASTER_SRV_DEV_BLOB_ROUTE", "1")], + ) + .unwrap_err() + ); + for name in [ + "ASTER_SRV_DEV_BLOB_ROUTE", + "ASTER_SRV_REPOSITORY_OWNER_IDS", + "ASTER_SRV_CACHE_WRITER_WORKFLOWS", + ] { + assert!(err.contains(name), "{name}: {err}"); + } + // Without the listener none of this is required. + let mut pairs = REQUIRED.to_vec(); + pairs.push(( + "ASTER_SRV_STATIC_TOKENS", + "ArchAstro/dev=dev-token-0123456789", + )); + pairs.push(("ASTER_SRV_DEV_BLOB_ROUTE", "1")); + pairs.push(("ASTER_SRV_TRUSTED_PROXY_HOPS", "0")); + assert!(Config::from_env(env_of(&pairs)).is_ok()); + } + + /// N4: every allowlist entry needs a pin that covers all it names: an + /// exact pin for `owner/name`, an `owner/*` pin for either form, and an + /// exact pin does not cover `owner/*`. + #[test] + fn every_allowlisted_repository_needs_an_owner_id_pin_when_the_api_listener_is_on() { + let with = |repositories: &str, pins: &str| { + api_config( + &["ASTER_SRV_REPOSITORIES", "ASTER_SRV_REPOSITORY_OWNER_IDS"], + &[ + ("ASTER_SRV_REPOSITORIES", repositories), + ("ASTER_SRV_REPOSITORY_OWNER_IDS", pins), + ], + ) + }; + assert!(with("ArchAstro/a,ArchAstro/b", "ArchAstro/a=1,ArchAstro/b=1").is_ok()); + assert!(with("ArchAstro/a,ArchAstro/b", "ArchAstro/*=1").is_ok()); + assert!(with("ArchAstro/*", "ArchAstro/*=1").is_ok()); + let err = format!( + "{:#}", + with("ArchAstro/a,ArchAstro/b", "ArchAstro/a=1").unwrap_err() + ); + assert!( + err.contains("archastro/b") && !err.contains("archastro/a,"), + "{err}" + ); + assert!(with("ArchAstro/*", "ArchAstro/a=1").is_err()); + assert!(with("ArchAstro/a,Other/b", "ArchAstro/*=1").is_err()); + // No pins at all. + assert!(api_config(&["ASTER_SRV_REPOSITORY_OWNER_IDS"], &[]).is_err()); + } + + /// N4: with the listener on, `OwnerIds::accepts` refuses a repository no + /// pin covers, instead of accepting any `repository_owner_id`. + #[test] + fn owner_ids_fail_closed_for_an_unpinned_repository_when_the_api_listener_is_on() { + let pins = OwnerIds::parse(Some("ArchAstro/a=1,Other/*=2")).unwrap(); + // Unpinned servers keep today's behaviour. + assert!(pins.accepts("ArchAstro/b", None)); + assert!(pins.accepts("ArchAstro/b", Some("9"))); + let strict = pins.clone().failing_closed(); + assert!(!strict.accepts("ArchAstro/b", None)); + assert!(!strict.accepts("ArchAstro/b", Some("1"))); + assert!(!strict.accepts("elsewhere/x", Some("1"))); + // Pinned repositories behave as before, case-insensitively. + assert!(strict.accepts("archastro/A", Some("1"))); + assert!(!strict.accepts("ArchAstro/a", Some("2"))); + assert!(!strict.accepts("ArchAstro/a", None)); + assert!(strict.accepts("Other/anything", Some("2"))); + // The config applies it exactly when the listener is on. + assert!(!api_config(&[], &[]) + .unwrap() + .owner_ids + .accepts("ArchAstro/x", None)); + let mut pairs = REQUIRED.to_vec(); + pairs.push(("ASTER_SRV_REPOSITORY_OWNER_IDS", "Other/*=2")); + let config = Config::from_env(env_of(&pairs)).unwrap(); + assert!(config.owner_ids.accepts("ArchAstro/x", None)); + } + #[test] fn reads_the_quarantine_rules() { let mut pairs = REQUIRED.to_vec(); diff --git a/crates/aster-srv/src/http.rs b/crates/aster-srv/src/http.rs index 583fff7..2cdcc66 100644 --- a/crates/aster-srv/src/http.rs +++ b/crates/aster-srv/src/http.rs @@ -213,6 +213,7 @@ async fn serve_surface( // One permit per open connection: past the cap, new ones wait in the // kernel's accept queue instead of costing a task and a socket each. let connections = Arc::new(Semaphore::new(app.config.max_connections)); + let per_peer = limits::Concurrency::new(app.config.max_connections_per_ip); let hsts = app .config .public_url @@ -233,6 +234,21 @@ async fn serve_surface( continue; } }; + // With no trusted proxy the peer is the client, so its open + // connections can be capped here. Behind a load balancer the + // peer is the balancer; the per-client cap is on in-flight + // requests in `handle`, by the forwarded address. + let connection_slot = if app.config.trusted_proxy_hops == 0 { + match per_peer.try_acquire(&limits::ip_key(peer.ip().to_canonical())) { + Some(slot) => Some(slot), + None => { + app.metrics.rate_limited("other", "connections_per_ip"); + continue; + } + } + } else { + None + }; let header_timeout = app.config.header_timeout; let app = Arc::clone(&app); let service = service_fn(move |request| { @@ -250,6 +266,7 @@ async fn serve_surface( let connection = graceful.watch(connection); tokio::spawn(async move { let _ = connection.await; + drop(connection_slot); drop(permit); }); } @@ -268,25 +285,32 @@ async fn handle( peer: std::net::IpAddr, request: Request, ) -> Reply { - let forwarded = request - .headers() - .get_all("x-forwarded-for") - .iter() - .filter_map(|value| value.to_str().ok()) - .collect::>() - .join(","); - let client = limits::client_ip( - Some(forwarded.as_str()).filter(|f| !f.is_empty()), - peer, - app.limits.trusted_proxy_hops, + let forwarded = limits::forwarded_for( + request + .headers() + .get_all("x-forwarded-for") + .iter() + .filter_map(|value| value.to_str().ok()), ); + let client = limits::client_ip(forwarded.as_deref(), peer, app.limits.trusted_proxy_hops); let path = request.uri().path().to_string(); let ctx = RequestCtx::new( routes::label(request.method().as_str(), &path), client.to_string(), Arc::clone(&app.metrics), + Arc::clone(&app.audit_sampler), ); audit::scope(ctx, async { + // A client IP holds only so many requests open at once, so slow + // bodies from one address cannot take every connection slot. + let Some(_slot) = app + .limits + .requests_per_ip + .try_acquire(&limits::ip_key(client)) + else { + audit::rate_limited("ip_concurrency", None); + return Reply::too_many(1); + }; let work = guarded(app, surface, client, request); let reply = tokio::time::timeout(app.config.handler_timeout, work) .await @@ -457,7 +481,7 @@ async fn route(app: &App, client: std::net::IpAddr, request: Request) let body = match read_body( request, crate::cli_auth::MAX_FORM_BYTES, - app.config.body_timeout, + app.config.unauth_body_timeout, ) .await { @@ -476,7 +500,7 @@ async fn route(app: &App, client: std::net::IpAddr, request: Request) let body = match read_body( request, crate::cli_auth::MAX_FORM_BYTES, - app.config.body_timeout, + app.config.unauth_body_timeout, ) .await { @@ -496,10 +520,11 @@ async fn route(app: &App, client: std::net::IpAddr, request: Request) let Some(Principal::Ci(identity)) = principal else { return Reply::error(403, "a person's token is read-only"); }; - // Uploads, like cache writes, may be limited to named workflows. - if !crate::cache::results_writer_allowed(&app.config, &identity) { - return Reply::error(403, "this workflow may not upload test results"); - } + // The gate has applied the results writer rule (named workflows, + // no `pull_request_target`) before this point. + let Some(_large) = large_body_permit(app) else { + return busy(); + }; let gzip = header(&request, CONTENT_ENCODING).is_some_and(|e| e.eq_ignore_ascii_case("gzip")); let body = match read_body(request, app.config.max_body_bytes, app.config.body_timeout) @@ -528,6 +553,16 @@ async fn route(app: &App, client: std::net::IpAddr, request: Request) query::handle(app, &identity, route, request.uri().query()).await } Handler::CacheRead | Handler::CacheLookup | Handler::CacheWrite => { + // A commit carries a manifest of up to the body cap; only a few + // run at once. The permit is held until the reply is built. + let _large = if route.handler == Handler::CacheWrite && path == "/v1/cache/commit" { + match large_body_permit(app) { + Some(permit) => Some(permit), + None => return busy(), + } + } else { + None + }; cache_request( app, request, @@ -564,6 +599,23 @@ async fn route(app: &App, client: std::net::IpAddr, request: Request) } } +/// A permit to read one large body (a commit's manifest or a test-results +/// upload), or `None` when [`crate::Config::max_large_bodies`] are already +/// running. Held for the rest of the request. +fn large_body_permit(app: &App) -> Option { + let permit = Arc::clone(&app.limits.large_bodies) + .try_acquire_owned() + .ok(); + if permit.is_none() { + audit::rate_limited("large_body", None); + } + permit +} + +fn busy() -> Reply { + Reply::error(503, "busy; retry shortly").with_header("retry-after", "1".into()) +} + /// Who is reading, for the GET routes over test results. A CI token reads /// its own repository. A signed-in person reads any repository on the /// allowlist, named by `?repo=owner/name`, so the identity returned for them diff --git a/crates/aster-srv/src/iap.rs b/crates/aster-srv/src/iap.rs index ff50569..6fe54c4 100644 --- a/crates/aster-srv/src/iap.rs +++ b/crates/aster-srv/src/iap.rs @@ -19,14 +19,13 @@ use serde::Deserialize; use tokio::sync::{Mutex, RwLock}; use crate::config::{Config, EmailDomains, IapAudience}; +use crate::keyfetch::{RefetchPolicy, RefetchState}; pub const IAP_ISSUER: &str = "https://cloud.google.com/iap"; pub const IAP_JWKS_URL: &str = "https://www.gstatic.com/iap/verify/public_key-jwk"; /// The header IAP puts its assertion in. pub const IAP_HEADER: &str = "x-goog-iap-jwt-assertion"; -const REFETCH_INTERVAL: Duration = Duration::from_secs(60); - /// Who is viewing. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Viewer { @@ -76,9 +75,11 @@ pub struct IapVerifier { /// Where keys come from; `None` uses only keys added with [`Self::with_key`]. jwks_url: Option, keys: RwLock>, - /// When keys were last fetched. Held for the whole fetch, so a caller - /// that arrives mid-fetch waits and then finds the fetched keys. - last_fetch: Mutex>, + /// When keys may be refetched (see [`crate::keyfetch`]). Held for the + /// whole fetch, so a caller that arrives mid-fetch waits and then finds + /// the fetched keys. + refetch: Mutex, + refetch_policy: RefetchPolicy, client: reqwest::Client, } @@ -94,7 +95,8 @@ impl IapVerifier { domains: config.ui_email_domains.clone(), jwks_url: Some(IAP_JWKS_URL.to_string()), keys: RwLock::new(HashMap::new()), - last_fetch: Mutex::new(None), + refetch: Mutex::new(RefetchState::default()), + refetch_policy: RefetchPolicy::default(), client: crate::http_client() .timeout(Duration::from_secs(10)) .build() @@ -102,6 +104,14 @@ impl IapVerifier { } } + /// Fetch keys from `url` instead of Google's, spacing refetches by + /// `policy`. For tests. + pub fn with_jwks(mut self, url: &str, policy: RefetchPolicy) -> Self { + self.jwks_url = Some(url.to_string()); + self.refetch_policy = policy; + self + } + /// Trust one signing key and fetch no others. For tests. pub fn with_key(mut self, kid: &str, key: DecodingKey) -> Self { self.jwks_url = None; @@ -196,14 +206,14 @@ impl IapVerifier { // One fetch at a time, with the lock held across it. Callers that // queued behind a fetch look again once they hold the lock, so they // get the key it fetched instead of failing while it was in flight. - let mut last = self.last_fetch.lock().await; + let mut refetch = self.refetch.lock().await; if let Some(key) = self.keys.read().await.get(kid) { return Ok(key.clone()); } - if last.is_some_and(|at| at.elapsed() < REFETCH_INTERVAL) { + if !refetch.may_fetch(kid, Instant::now(), self.refetch_policy) { return Err(unknown()); } - *last = Some(Instant::now()); + refetch.attempted(Instant::now()); let set: JwkSet = self .client .get(url) @@ -224,7 +234,16 @@ impl IapVerifier { } let mut keys = self.keys.write().await; *keys = fetched; - keys.get(kid).cloned().ok_or_else(unknown) + match keys.get(kid).cloned() { + Some(key) => { + refetch.found(kid); + Ok(key) + } + None => { + refetch.missing(kid, Instant::now()); + Err(unknown()) + } + } } } diff --git a/crates/aster-srv/src/keyfetch.rs b/crates/aster-srv/src/keyfetch.rs new file mode 100644 index 0000000..5c9fff1 --- /dev/null +++ b/crates/aster-srv/src/keyfetch.rs @@ -0,0 +1,160 @@ +//! When a verifier may refetch its issuer's signing keys. +//! +//! A token that names a key the verifier has not seen is either a real key +//! rotation (the key is in the issuer's JWK set) or a probe (it is not). +//! The verifier cannot tell before it fetches, and fetching on every unknown +//! key would let a caller turn requests into fetches against the issuer. +//! +//! The rules, shared by the GitHub OIDC verifier ([`crate::auth`]) and the +//! IAP verifier ([`crate::iap`]): +//! +//! 1. A fetch of any outcome, success or failure, is followed by at least +//! [`RefetchPolicy::min_interval`] (10 seconds) before the next one. Every +//! unknown key in that window is refused without a fetch, so a flood of +//! random keys costs the issuer at most one fetch per interval. +//! 2. A key a successful fetch did not find is remembered for +//! [`RefetchPolicy::absent_for`] (60 seconds): the same unknown key does +//! not trigger another fetch inside that time, however often it arrives. +//! 3. Neither a failed fetch nor a probe for a *different* key blocks a real +//! rotation for more than `min_interval`. Before this, the attempt time +//! was recorded before the fetch, so one failed fetch or one probe held +//! off a real rotation for a full minute. +//! +//! Callers that arrive while a fetch runs wait for it (the state is behind +//! one lock held across the fetch) and share its result. + +use std::collections::HashMap; +use std::time::{Duration, Instant}; + +/// The refetch timings. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct RefetchPolicy { + /// The least time between two fetches, whatever their outcome. + pub min_interval: Duration, + /// How long a key a successful fetch did not find is not looked for again. + pub absent_for: Duration, +} + +impl Default for RefetchPolicy { + fn default() -> Self { + Self { + min_interval: Duration::from_secs(10), + absent_for: Duration::from_secs(60), + } + } +} + +/// Unknown keys remembered at once. A flood of distinct random keys cannot +/// grow this: past the cap the oldest entries are dropped. +const MAX_ABSENT: usize = 64; + +/// What the verifier remembers between fetches. Lives behind the lock that +/// serialises fetches. +#[derive(Default)] +pub struct RefetchState { + last_attempt: Option, + absent: HashMap, +} + +impl RefetchState { + /// Whether a token naming `kid`, which is not in the key set, may cause + /// a fetch now. + pub fn may_fetch(&self, kid: &str, now: Instant, policy: RefetchPolicy) -> bool { + let since = |at: Instant| now.saturating_duration_since(at); + if self + .last_attempt + .is_some_and(|at| since(at) < policy.min_interval) + { + return false; + } + self.absent + .get(kid) + .is_none_or(|at| since(*at) >= policy.absent_for) + } + + /// A fetch starts now. Recorded before the request so that a fetch that + /// fails still spaces the next one. + pub fn attempted(&mut self, now: Instant) { + self.last_attempt = Some(now); + } + + /// A fetch succeeded and its key set held `kid`: forget any record that + /// it was absent. + pub fn found(&mut self, kid: &str) { + self.absent.remove(kid); + } + + /// A fetch succeeded and its key set did not hold `kid`. + pub fn missing(&mut self, kid: &str, now: Instant) { + if self.absent.len() >= MAX_ABSENT && !self.absent.contains_key(kid) { + let oldest = self + .absent + .iter() + .min_by_key(|(_, at)| **at) + .map(|(kid, _)| kid.clone()); + if let Some(oldest) = oldest { + self.absent.remove(&oldest); + } + } + self.absent.insert(kid.to_string(), now); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + const POLICY: RefetchPolicy = RefetchPolicy { + min_interval: Duration::from_secs(10), + absent_for: Duration::from_secs(60), + }; + + #[test] + fn a_first_unknown_key_fetches() { + assert!(RefetchState::default().may_fetch("k", Instant::now(), POLICY)); + } + + #[test] + fn a_failed_fetch_blocks_the_next_for_the_short_interval_only() { + let start = Instant::now(); + let mut state = RefetchState::default(); + state.attempted(start); + // No success was recorded: the real rotation waits ten seconds, not a minute. + assert!(!state.may_fetch("rotated", start + Duration::from_secs(9), POLICY)); + assert!(state.may_fetch("rotated", start + Duration::from_secs(10), POLICY)); + } + + #[test] + fn a_probe_blocks_itself_for_a_minute_and_a_different_key_for_ten_seconds() { + let start = Instant::now(); + let mut state = RefetchState::default(); + state.attempted(start); + state.missing("probe", start); + let later = |s| start + Duration::from_secs(s); + assert!(!state.may_fetch("probe", later(30), POLICY)); + assert!(state.may_fetch("rotated", later(10), POLICY)); + assert!(state.may_fetch("probe", later(60), POLICY)); + } + + #[test] + fn a_key_found_later_is_forgotten_as_absent() { + let start = Instant::now(); + let mut state = RefetchState::default(); + state.missing("k", start); + state.found("k"); + assert!(state.may_fetch("k", start, POLICY)); + } + + #[test] + fn remembered_absent_keys_are_bounded() { + let start = Instant::now(); + let mut state = RefetchState::default(); + for n in 0..1000u64 { + state.missing(&format!("k{n}"), start + Duration::from_millis(n)); + } + assert_eq!(state.absent.len(), MAX_ABSENT); + // The newest are kept, the oldest dropped. + assert!(state.absent.contains_key("k999")); + assert!(!state.absent.contains_key("k0")); + } +} diff --git a/crates/aster-srv/src/lib.rs b/crates/aster-srv/src/lib.rs index dc74a9f..3a13cf2 100644 --- a/crates/aster-srv/src/lib.rs +++ b/crates/aster-srv/src/lib.rs @@ -16,6 +16,7 @@ pub mod config; pub mod db; mod http; pub mod iap; +pub mod keyfetch; pub mod limits; pub mod log; pub mod metrics; @@ -79,6 +80,8 @@ pub struct App { pub auth_pool: deadpool_postgres::Pool, pub limits: limits::Limits, pub metrics: Arc, + /// Samples the failure audit lines; see [`audit`]. + pub audit_sampler: Arc, pub verifier: auth::Verifier, pub iap: iap::IapVerifier, /// Mints and checks person tokens. `None` when @@ -187,6 +190,10 @@ impl App { let auth_pool = db::pool_sized(&config.database_url, limits::AUTH_POOL_SIZE, config.db_wait)?; let limits = limits::Limits::new(&config); + let audit_sampler = Arc::new(audit::Sampler::new( + config.log_sample_first, + config.log_sample_window, + )); let person_tokens = person::TokenIssuer::from_config(&config); let raw_store = raw_store.unwrap_or_else(|| raw_store::RawStore::new(config.raw_bucket.clone())); @@ -199,6 +206,7 @@ impl App { auth_pool, limits, metrics: Default::default(), + audit_sampler, verifier, iap, person_tokens, diff --git a/crates/aster-srv/src/limits.rs b/crates/aster-srv/src/limits.rs index 690c61e..dc08302 100644 --- a/crates/aster-srv/src/limits.rs +++ b/crates/aster-srv/src/limits.rs @@ -33,10 +33,17 @@ use tokio::sync::Semaphore; use crate::Config; /// Most keys a limiter tracks. Past it, idle keys are dropped, and if the -/// table is still full the newcomer shares one overflow bucket: attackers -/// that mint keys cannot grow memory, they only share a limit. +/// table is still full the newcomer shares an overflow bucket: attackers +/// that mint keys cannot grow memory, they only share limits. const MAX_KEYS: usize = 100_000; -const OVERFLOW_KEY: &str = "*overflow*"; +/// How many overflow buckets a limiter has. A newcomer lands in one by a +/// keyed hash (the seed is per limiter and per process, so a caller cannot +/// aim at a bucket), so a flood of minted keys has to drain all of them to +/// starve a real client, not one shared bucket. +const OVERFLOW_SHARDS: u64 = 256; +/// The table is swept for idle keys at most this often, so a flood of new +/// keys against a full table cannot make every request scan 100,000 entries. +const SWEEP_INTERVAL: Duration = Duration::from_secs(1); /// Exchanges running at once. The exchange holds one of the auth pool's /// connections ([`AUTH_POOL_SIZE`]); a few more wait their turn. pub const MAX_CONCURRENT_EXCHANGES: usize = 4; @@ -48,20 +55,43 @@ struct Bucket { last: Instant, } +struct Table { + buckets: HashMap, + last_sweep: Option, + #[cfg(test)] + sweeps: usize, +} + /// A token bucket per key: `per_minute` requests, refilled continuously. +/// +/// Each limiter has its own table and its own overflow buckets, so keys +/// minted against one (client IPs, before authentication) never spend +/// another's (the authenticated subjects). pub struct RateLimiter { per_minute: u32, - buckets: Mutex>, + table: Mutex, + seed: std::collections::hash_map::RandomState, } impl RateLimiter { pub fn new(per_minute: u32) -> Self { Self { per_minute, - buckets: Mutex::new(HashMap::new()), + table: Mutex::new(Table { + buckets: HashMap::new(), + last_sweep: None, + #[cfg(test)] + sweeps: 0, + }), + seed: Default::default(), } } + fn overflow_key(&self, key: &str) -> String { + use std::hash::BuildHasher; + format!("*overflow-{}*", self.seed.hash_one(key) % OVERFLOW_SHARDS) + } + /// Take one request from `key`'s bucket, or learn how many seconds /// until one is available. pub fn check(&self, key: &str) -> Result<(), u64> { @@ -71,17 +101,27 @@ impl RateLimiter { pub fn check_at(&self, key: &str, now: Instant) -> Result<(), u64> { let capacity = f64::from(self.per_minute); let rate = capacity / 60.0; - let mut buckets = self.buckets.lock().unwrap(); - let mut key = key; - if !buckets.contains_key(key) && buckets.len() >= MAX_KEYS { - buckets.retain(|_, b| { - b.tokens + now.saturating_duration_since(b.last).as_secs_f64() * rate < capacity - }); - if buckets.len() >= MAX_KEYS { - key = OVERFLOW_KEY; + let mut table = self.table.lock().unwrap(); + let mut key = key.to_string(); + if !table.buckets.contains_key(&key) && table.buckets.len() >= MAX_KEYS { + let due = table + .last_sweep + .is_none_or(|at| now.saturating_duration_since(at) >= SWEEP_INTERVAL); + if due { + table.last_sweep = Some(now); + #[cfg(test)] + { + table.sweeps += 1; + } + table.buckets.retain(|_, b| { + b.tokens + now.saturating_duration_since(b.last).as_secs_f64() * rate < capacity + }); + } + if table.buckets.len() >= MAX_KEYS { + key = self.overflow_key(&key); } } - let bucket = buckets.entry(key.to_string()).or_insert(Bucket { + let bucket = table.buckets.entry(key).or_insert(Bucket { tokens: capacity, last: now, }); @@ -98,7 +138,66 @@ impl RateLimiter { #[cfg(test)] fn len(&self) -> usize { - self.buckets.lock().unwrap().len() + self.table.lock().unwrap().buckets.len() + } + + #[cfg(test)] + fn sweeps(&self) -> usize { + self.table.lock().unwrap().sweeps + } +} + +/// How many things one key may hold at once: a client IP's in-flight +/// requests or open connections. A held slot is a [`SlotGuard`]; the table +/// holds only keys with a live slot, so it is as small as the open work. +pub struct Concurrency { + max: usize, + held: Arc>>, +} + +impl Concurrency { + pub fn new(max: usize) -> Self { + Self { + max, + held: Arc::default(), + } + } + + /// A slot for `key`, or `None` when it already holds the maximum. + pub fn try_acquire(&self, key: &str) -> Option { + let mut held = self.held.lock().unwrap(); + let count = held.entry(key.to_string()).or_default(); + if *count >= self.max { + return None; + } + *count += 1; + Some(SlotGuard { + key: key.to_string(), + held: Arc::clone(&self.held), + }) + } + + #[cfg(test)] + fn keys(&self) -> usize { + self.held.lock().unwrap().len() + } +} + +/// One slot of a [`Concurrency`], released on drop. +pub struct SlotGuard { + key: String, + held: Arc>>, +} + +impl Drop for SlotGuard { + fn drop(&mut self) { + let mut held = self.held.lock().unwrap(); + if let Some(count) = held.get_mut(&self.key) { + *count -= 1; + if *count == 0 { + held.remove(&self.key); + } + } } } @@ -112,6 +211,11 @@ pub struct Limits { pub api_subject: RateLimiter, /// Exchanges allowed to run at once. pub exchanges: Arc, + /// Large request bodies (a cache commit, a test-results upload) read at + /// once: each can weigh `ASTER_SRV_MAX_BODY_BYTES` in memory. + pub large_bodies: Arc, + /// Requests in flight, per client IP ([`ip_key`]). + pub requests_per_ip: Concurrency, pub trusted_proxy_hops: usize, } @@ -122,11 +226,22 @@ impl Limits { api_ip: RateLimiter::new(config.rate_api_ip_per_min), api_subject: RateLimiter::new(config.rate_api_subject_per_min), exchanges: Arc::new(Semaphore::new(MAX_CONCURRENT_EXCHANGES)), + large_bodies: Arc::new(Semaphore::new(config.max_large_bodies)), + requests_per_ip: Concurrency::new(config.max_connections_per_ip), trusted_proxy_hops: config.trusted_proxy_hops, } } } +/// The `X-Forwarded-For` value of a request: every header line of that name, +/// in order, joined into one list (RFC 9110: repeated lines are one +/// comma-separated list). A line that is not text is skipped. `None` when +/// there is nothing to read. +pub fn forwarded_for<'a>(lines: impl IntoIterator) -> Option { + let joined = lines.into_iter().collect::>().join(","); + (!joined.is_empty()).then_some(joined) +} + /// The client address: see the module docs for the trust model. pub fn client_ip(forwarded_for: Option<&str>, peer: IpAddr, trusted_hops: usize) -> IpAddr { let from_header = || { @@ -192,24 +307,82 @@ mod tests { assert_eq!(slow.check_at("x", start), Err(6)); } - #[test] - fn the_table_is_bounded_and_overflow_keys_share_one_bucket() { - let limiter = RateLimiter::new(1); - let now = Instant::now(); + /// A limiter whose table is full of spent buckets, none idle. + fn full_limiter(per_minute: u32, now: Instant) -> RateLimiter { + let limiter = RateLimiter::new(per_minute); for n in 0..MAX_KEYS { - limiter.check_at(&format!("k{n}"), now).unwrap(); + for _ in 0..per_minute { + limiter.check_at(&format!("k{n}"), now).unwrap(); + } } assert_eq!(limiter.len(), MAX_KEYS); - // Every bucket is spent, so none is idle to drop: newcomers share. - assert!(limiter.check_at("new-1", now).is_ok()); - assert!(limiter.check_at("new-2", now).is_err()); - assert_eq!(limiter.len(), MAX_KEYS + 1); - // Later, idle buckets are dropped and real keys come back. + limiter + } + + /// N7: the table stays bounded, and a flood of minted keys that fills it + /// does not take a real newcomer's whole allowance: the overflow is many + /// buckets chosen by a keyed hash, not one shared bucket. + #[test] + fn a_flood_of_minted_keys_does_not_starve_a_real_newcomer() { + let now = Instant::now(); + let limiter = full_limiter(60, now); + // 1,000 minted keys arrive at once. One shared bucket of 60 would be + // empty after 60 of them; 256 of them leave each about 4 spent. + for n in 0..1000 { + let _ = limiter.check_at(&format!("mint-{n}"), now); + } + assert!(limiter.len() <= MAX_KEYS + OVERFLOW_SHARDS as usize); + // A real client arriving now still has its allowance, and the same + // client lands in the same overflow bucket each time. + for _ in 0..40 { + assert!(limiter.check_at("real-client", now).is_ok()); + } + // Another limiter's keys are never affected. + let other = RateLimiter::new(60); + assert!(other.check_at("real-client", now).is_ok()); + // Later, idle buckets are dropped and real keys come back with a + // table of their own. let later = now + Duration::from_secs(120); assert!(limiter.check_at("new-3", later).is_ok()); assert!(limiter.len() < 10); } + /// N7: a full table is swept for idle keys at most once per interval, + /// however many new keys arrive, so a flood cannot make each request + /// scan the whole table. + #[test] + fn a_full_table_is_swept_at_most_once_per_interval() { + let now = Instant::now(); + let limiter = full_limiter(1, now); + for n in 0..5000 { + let _ = limiter.check_at(&format!("mint-{n}"), now + Duration::from_millis(n % 900)); + } + assert_eq!(limiter.sweeps(), 1); + let _ = limiter.check_at("later", now + Duration::from_millis(1500)); + assert_eq!(limiter.sweeps(), 2); + } + + /// N7: the subject limiter, keyed by verified subjects, is a different + /// table from the per-IP limiters minted keys fill, so an unauthenticated + /// flood cannot spend an authenticated subject's allowance. + #[test] + fn a_flood_against_the_ip_limiter_leaves_subjects_untouched() { + let config = crate::Config::from_env(|name| match name { + "ASTER_SRV_DATABASE_URL" => Some("postgres://localhost/x".into()), + "ASTER_SRV_REPOSITORIES" => Some("ArchAstro/*".into()), + _ => None, + }) + .unwrap(); + let limits = Limits::new(&config); + let now = Instant::now(); + for n in 0..(MAX_KEYS + 5000) { + let _ = limits.api_ip.check_at(&format!("203.0.{n}"), now); + } + for _ in 0..100 { + assert!(limits.api_subject.check_at("ci:ArchAstro/x", now).is_ok()); + } + } + #[test] fn the_client_is_the_right_most_address_we_do_not_own() { let peer = ip("10.0.0.9"); @@ -238,6 +411,77 @@ mod tests { ); } + #[test] + fn a_key_holds_at_most_its_cap_and_a_dropped_slot_frees_it() { + let slots = Concurrency::new(2); + let a = slots.try_acquire("a").unwrap(); + let _a2 = slots.try_acquire("a").unwrap(); + assert!(slots.try_acquire("a").is_none()); + // Another key is untouched. + let b = slots.try_acquire("b").unwrap(); + drop(a); + assert!(slots.try_acquire("a").is_some()); + // The table keeps only keys with a live slot. + drop(b); + assert_eq!(slots.keys(), 1); + } + + /// N12: a client may send any number of `X-Forwarded-For` lines; they + /// are one list, in order, and the load balancer's entries are the last. + #[test] + fn repeated_forwarded_for_lines_are_one_list_and_cannot_move_the_client() { + let peer = ip("10.0.0.9"); + let lb = "203.0.113.7, 34.8.0.1"; + // The caller's own lines come before the balancer's. + let one_line = forwarded_for(["6.6.6.6, 7.7.7.7, 203.0.113.7, 34.8.0.1"]); + let two_lines = forwarded_for(["6.6.6.6", "7.7.7.7", lb]); + let spoof_first = forwarded_for(["6.6.6.6, 5.5.5.5", lb]); + for header in [&one_line, &two_lines, &spoof_first] { + assert_eq!(client_ip(header.as_deref(), peer, 1), ip("203.0.113.7")); + } + // Lines are joined in order, so hops walk across a line boundary. + assert_eq!(forwarded_for(["a", "b, c"]).as_deref(), Some("a,b, c")); + assert_eq!( + client_ip( + forwarded_for(["9.9.9.9", "203.0.113.7", "34.8.0.1"]).as_deref(), + peer, + 1 + ), + ip("203.0.113.7") + ); + // No lines, or empty ones: the peer. + assert_eq!(forwarded_for([]), None); + assert_eq!(forwarded_for([""]), None); + assert_eq!(client_ip(forwarded_for([]).as_deref(), peer, 1), peer); + // A spoofed line added after the balancer's entries takes the + // trusted position: the balancer must be the last to append, which + // is why the pods must be reachable through it alone. + assert_eq!( + client_ip(forwarded_for([lb, "6.6.6.6"]).as_deref(), peer, 1), + ip("34.8.0.1") + ); + } + + /// N12: each trusted hop moves the client one entry left, and an entry + /// that is not an address, or a header too short for the hops, falls back + /// to the peer. + #[test] + fn the_client_moves_left_one_entry_per_trusted_hop() { + let peer = ip("10.0.0.9"); + let header = forwarded_for(["1.1.1.1, 2.2.2.2, 3.3.3.3, 4.4.4.4"]); + let at = |hops| client_ip(header.as_deref(), peer, hops); + assert_eq!(at(0), peer); + assert_eq!(at(1), ip("3.3.3.3")); + assert_eq!(at(2), ip("2.2.2.2")); + assert_eq!(at(3), ip("1.1.1.1")); + assert_eq!(at(4), peer); + assert_eq!( + client_ip(Some("1.1.1.1, not-an-ip, 4.4.4.4"), peer, 1), + peer + ); + assert_eq!(client_ip(Some("1.1.1.1,,4.4.4.4"), peer, 1), peer); + } + #[test] fn ipv6_clients_share_a_limit_per_64() { assert_eq!( diff --git a/crates/aster-srv/src/main.rs b/crates/aster-srv/src/main.rs index 91e8622..9daf623 100644 --- a/crates/aster-srv/src/main.rs +++ b/crates/aster-srv/src/main.rs @@ -40,6 +40,7 @@ async fn run() -> anyhow::Result<()> { let listen = config.listen; let api_listen = config.api_listen; let app = App::connect(config)?; + aster_srv::audit::Sampler::spawn_flush(&app.audit_sampler); let listener = tokio::net::TcpListener::bind(listen).await?; log::info( "aster-srv listening", @@ -91,10 +92,11 @@ async fn run() -> anyhow::Result<()> { shutdown_signal().await; let _ = stop.send(true); }); - aster_srv::serve(app, listener, wait(stopped)).await?; + aster_srv::serve(app.clone(), listener, wait(stopped)).await?; if let Some(api) = api { api.await??; } + app.audit_sampler.flush_all(); log::info("aster-srv stopped", json!({})); Ok(()) } diff --git a/crates/aster-srv/src/routes.rs b/crates/aster-srv/src/routes.rs index 4e56af7..b613689 100644 --- a/crates/aster-srv/src/routes.rs +++ b/crates/aster-srv/src/routes.rs @@ -13,7 +13,7 @@ //! | `Person` | a signed-in person's token only (sign out); a CI token is refused. With person sign-in switched off (`ASTER_SRV_PERSON_TOKENS=off`) the route answers 404 before any token is read | //! | `Ci` | a CI token (GitHub OIDC or a development static token); a person's token is refused | //! | `CiOrPerson` | a CI token for its own repository, or a signed-in person for any served repository: the read-only routes | -//! | `CiWriter` | a CI token only; the handler also checks the writer rules (ref, workflow, event) | +//! | `CiWriter` | a CI token only, held at the gate to the route's writer rule before any body is read: the cache's (ref, workflow, push event) for cache writes, the results rule for test-results upload | use crate::audit; use crate::auth::{AuthError, Identity, Principal}; @@ -303,6 +303,21 @@ pub async fn gate( ); return Err(Reply::error(403, "a person's token is read-only")); } + // A CI token that is not allowed to write is refused here, before + // any body is read: a pull-request job's token must not make the + // server read and parse up to 64 MiB only to refuse it. + if class == Auth::CiWriter { + if let (Some(route), Principal::Ci(identity)) = (route, &principal) { + if let Some(message) = writer_refusal(app, route.handler, identity) { + audit::auth_failure( + Some(&identity.repository), + "writer_not_allowed", + message, + ); + return Err(Reply::error(403, message)); + } + } + } if class == Auth::Person && matches!(principal, Principal::Ci(_)) { audit::auth_failure( Some(&person_subject(&principal)), @@ -316,6 +331,24 @@ pub async fn gate( } } +/// Why a CI token may not call a `CiWriter` route, or `None` when it may. +/// The cache writes use the cache's own writer rule (ref, event, workflow). +/// Test-results upload deliberately accepts pull-request runs (a retry that +/// passes there is the flake signal), so it applies only the results rule. +/// The repository allowlist and the owner-id pin were applied when the token +/// was verified, before this runs. +fn writer_refusal(app: &App, handler: Handler, identity: &Identity) -> Option<&'static str> { + match handler { + Handler::CacheWrite if !crate::cache::cache_writer_allowed(&app.config, identity) => { + Some("this caller may not write the cache") + } + Handler::ResultsUpload if !crate::cache::results_writer_allowed(&app.config, identity) => { + Some("this workflow may not upload test results") + } + _ => None, + } +} + fn person_subject(principal: &Principal) -> String { match principal { Principal::Person(person) => person.subject.clone(), @@ -344,6 +377,7 @@ pub fn identity_for(principal: Principal, repo: Option<&str>) -> Result String { + format!( + "(CASE WHEN {r}.source = 'oidc' \ + THEN COALESCE({r}.git_ref = {git_ref} \ + AND {r}.event_name IN ('push', 'workflow_dispatch', 'schedule'), \ + false) \ + ELSE COALESCE({branch}, false) END)", + git_ref = text(&format!("refs/heads/{}", self.on_default)), + branch = self.branch_is_default(&format!("{r}.branch")), + ) + } + + /// Whether the run `r` is evidence quarantine may act on: a + /// default-branch run whose source is known. A pull request job runs + /// its author's code, so its token proves where the results came from + /// but not that they are true. + pub(super) fn trusted(&self, r: &str) -> String { + format!("({r}.source IS NOT NULL AND {})", self.on_default(r)) + } + /// Each run's final result of one test: `filter` picks the test and /// bounds the runs. `strict` leaves out `partial` targets. pub(super) fn finals(&self, filter: &str, strict: bool) -> String { @@ -369,7 +396,7 @@ async fn pass(app: &App, now: DateTime) -> Result> { } const EVENT_COLUMNS: &str = "observed_on, repo, test_id, kind, reason, run_id, prev_run_id, \ - target, input_hash, observed_at"; + target, input_hash, observed_at, trusted"; const EVENT_KEY: &str = "(repo, test_id, kind, run_id, observed_on)"; async fn classify( @@ -388,7 +415,8 @@ async fn classify( WITH attempts AS ( \ SELECT tr.repo, tr.run_id, tr.test_id, tr.target, tr.outcome, \ tr.failure_hash, tr.received_on, r.received_at, r.sha, \ - {on_default} AS on_default, t.input_hash, t.confidence, \ + {on_default} AS on_default, {trusted} AS trusted, \ + t.input_hash, t.confidence, \ row_number() OVER w AS rn, \ bool_or(tr.outcome IN ('fail', 'error')) OVER (\ PARTITION BY tr.run_id, tr.test_id) AS any_fail \ @@ -402,11 +430,12 @@ async fn classify( AND tr.outcome <> 'skip' AND NOT t.cached \ WINDOW w AS (PARTITION BY tr.run_id, tr.test_id ORDER BY tr.attempt DESC)) \ SELECT repo, run_id, test_id, target, outcome IN ('fail', 'error') AS is_fail, \ - failure_hash, received_on, received_at, sha, on_default, input_hash, \ - confidence IS NOT DISTINCT FROM 'partial' AS partial, \ + failure_hash, received_on, received_at, sha, on_default, trusted, \ + input_hash, confidence IS NOT DISTINCT FROM 'partial' AS partial, \ any_fail AND outcome = 'pass' AS retried \ FROM attempts WHERE rn = 1", - on_default = window.branch_is_default("r.branch"), + on_default = window.on_default("r"), + trusted = window.trusted("r"), from = at(window.from), to = at(window.to), from_day = day(from_day), @@ -469,7 +498,7 @@ async fn classify( &format!( "INSERT INTO test_results.test_flake_events ({EVENT_COLUMNS}) \ SELECT w.received_on, w.repo, w.test_id, 'definite', 'retry', w.run_id, \ - NULL, w.target, w.input_hash, w.received_at \ + NULL, w.target, w.input_hash, w.received_at, w.trusted \ FROM cls_win w WHERE w.retried \ AND NOT EXISTS (SELECT 1 FROM test_results.target_incidents i \ WHERE i.run_id = w.run_id AND i.target = w.target) \ @@ -495,7 +524,8 @@ async fn classify( .execute( &format!( "CREATE TEMP TABLE cls_hash ON COMMIT DROP AS \ - SELECT t.repo, t.address, t.input_hash, t.run_id, t.received_on, r.received_at \ + SELECT t.repo, t.address, t.input_hash, t.run_id, t.received_on, r.received_at, \ + {trusted} AS trusted \ FROM (SELECT repo, target, input_hash, max(received_on) AS until \ FROM cls_win WHERE input_hash IS NOT NULL \ GROUP BY repo, target, input_hash) k \ @@ -504,7 +534,8 @@ async fn classify( AND t.input_hash = k.input_hash \ AND t.received_on BETWEEN {history} AND k.until \ JOIN test_results.runs r ON r.run_id = t.run_id \ - WHERE NOT t.cached" + WHERE NOT t.cached", + trusted = window.trusted("r"), ), &[], ) @@ -512,14 +543,18 @@ async fn classify( transaction.batch_execute("ANALYZE cls_hash").await?; // The event is the failed run's, whichever of the two runs that was, so // a pass, a failure and a pass with one hash make one event: the second - // comparison finds the first one's key. + // comparison finds the first one's key. A trusted run is also compared + // with the nearest earlier trusted run of its hash, so pull request runs + // in between cannot hide a flake main saw for itself; the event is + // trusted when both its runs are, and a trusted comparison found later + // upgrades one found first. events += transaction .execute( &format!( "WITH earlier AS MATERIALIZED ( \ SELECT DISTINCT ON (tr.run_id, tr.test_id) \ tr.repo, tr.test_id, h.address, h.input_hash, tr.run_id, \ - h.received_at, h.received_on, \ + h.received_at, h.received_on, h.trusted, \ tr.outcome IN ('fail', 'error') AS is_fail \ FROM cls_hash h \ JOIN test_results.test_results tr \ @@ -530,27 +565,47 @@ async fn classify( nearest AS ( \ SELECT DISTINCT ON (w.run_id, w.test_id) w.*, \ p.run_id AS prev_run_id, p.is_fail AS prev_is_fail, \ - p.received_at AS prev_received_at, p.received_on AS prev_received_on \ + p.received_at AS prev_received_at, p.received_on AS prev_received_on, \ + p.trusted AS prev_trusted \ FROM cls_win w \ JOIN earlier p \ ON p.repo = w.repo AND p.test_id = w.test_id \ AND p.address = w.target AND p.input_hash = w.input_hash \ AND (p.received_at, p.run_id) < (w.received_at, w.run_id) \ ORDER BY w.run_id, w.test_id, p.received_at DESC, p.run_id DESC), \ + nearest_trusted AS ( \ + SELECT DISTINCT ON (w.run_id, w.test_id) w.*, \ + p.run_id AS prev_run_id, p.is_fail AS prev_is_fail, \ + p.received_at AS prev_received_at, p.received_on AS prev_received_on, \ + p.trusted AS prev_trusted \ + FROM cls_win w \ + JOIN earlier p \ + ON p.repo = w.repo AND p.test_id = w.test_id \ + AND p.address = w.target AND p.input_hash = w.input_hash \ + AND (p.received_at, p.run_id) < (w.received_at, w.run_id) \ + WHERE w.trusted AND p.trusted \ + ORDER BY w.run_id, w.test_id, p.received_at DESC, p.run_id DESC), \ flips AS ( \ SELECT repo, test_id, target, input_hash, \ CASE WHEN is_fail THEN run_id ELSE prev_run_id END AS failed_run, \ CASE WHEN is_fail THEN prev_run_id ELSE run_id END AS passed_run, \ CASE WHEN is_fail THEN received_on ELSE prev_received_on END AS failed_on, \ - CASE WHEN is_fail THEN received_at ELSE prev_received_at END AS failed_at \ - FROM nearest WHERE prev_is_fail <> is_fail) \ + CASE WHEN is_fail THEN received_at ELSE prev_received_at END AS failed_at, \ + trusted AND prev_trusted AS trusted \ + FROM (SELECT * FROM nearest UNION ALL SELECT * FROM nearest_trusted) n \ + WHERE prev_is_fail <> is_fail), \ + kept AS ( \ + SELECT DISTINCT ON (repo, test_id, failed_run, failed_on) * FROM flips f \ + WHERE NOT EXISTS (SELECT 1 FROM test_results.target_incidents i \ + WHERE i.run_id = f.failed_run AND i.target = f.target) \ + ORDER BY repo, test_id, failed_run, failed_on, trusted DESC, passed_run) \ INSERT INTO test_results.test_flake_events ({EVENT_COLUMNS}) \ SELECT f.failed_on, f.repo, f.test_id, 'definite', 'input_hash', f.failed_run, \ - f.passed_run, f.target, f.input_hash, f.failed_at \ - FROM flips f \ - WHERE NOT EXISTS (SELECT 1 FROM test_results.target_incidents i \ - WHERE i.run_id = f.failed_run AND i.target = f.target) \ - ON CONFLICT {EVENT_KEY} DO NOTHING" + f.passed_run, f.target, f.input_hash, f.failed_at, f.trusted \ + FROM kept f \ + ON CONFLICT {EVENT_KEY} DO UPDATE \ + SET trusted = true, prev_run_id = EXCLUDED.prev_run_id \ + WHERE EXCLUDED.trusted AND NOT test_results.test_flake_events.trusted" ), &[], ) @@ -606,7 +661,7 @@ async fn classify( &format!( "INSERT INTO test_results.test_flake_events ({EVENT_COLUMNS}) \ SELECT m.prev_received_on, w.repo, w.test_id, 'likely', 'next_main_run', \ - m.prev_run_id, w.run_id, f.target, t.input_hash, m.prev_received_at \ + m.prev_run_id, w.run_id, f.target, t.input_hash, m.prev_received_at, false \ FROM cls_win w \ JOIN cls_main m \ ON m.repo = w.repo AND m.test_id = w.test_id AND m.run_id = w.run_id \ @@ -815,7 +870,7 @@ async fn rebuild_main( history = day(window.history_from), through_day = day(window.from.date_naive()), through = at(window.from), - main = window.branch_is_default("r.branch"), + main = window.on_default("r"), ); transaction .execute( diff --git a/crates/aster-srv/src/test_results/durations.rs b/crates/aster-srv/src/test_results/durations.rs index d23b76d..136f1d0 100644 --- a/crates/aster-srv/src/test_results/durations.rs +++ b/crates/aster-srv/src/test_results/durations.rs @@ -220,6 +220,7 @@ mod tests { repository: "ArchAstro/firstlanding".into(), git_ref: None, run_id: None, + run_attempt: None, sha: None, workflow: None, job_workflow: None, diff --git a/crates/aster-srv/src/test_results/ingest.rs b/crates/aster-srv/src/test_results/ingest.rs index 8d4c146..e317b03 100644 --- a/crates/aster-srv/src/test_results/ingest.rs +++ b/crates/aster-srv/src/test_results/ingest.rs @@ -47,18 +47,13 @@ pub async fn handle(app: &App, identity: &Identity, upload: Upload) -> Reply { Ok(parts) => parts, Err(message) => return Reply::error(400, message), }; - if !run.repo.eq_ignore_ascii_case(&identity.repository) { - return Reply::error( - 403, - format!( - "the token is for {} but the results are for {}", - identity.repository, run.repo - ), - ); - } + let (run, source) = match bind(identity, run) { + Ok(bound) => bound, + Err(message) => return Reply::error(403, message), + }; let received_at = Utc::now(); - let stored = match store(app, &run, &targets, &tests, received_at).await { + let stored = match store(app, &run, &source, &targets, &tests, received_at).await { Ok(stored) => stored, Err(error) => { crate::log::error( @@ -125,6 +120,119 @@ fn decode(upload: &Upload, limit: usize) -> Result { String::from_utf8(bytes).map_err(|_| Reply::error(400, "the results are not UTF-8")) } +/// Where a run came from, as its token says. The classifier trusts these +/// columns, never the run record's own `branch`, to tell default-branch +/// runs from the rest. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Source { + /// `oidc` or `static`. + pub kind: &'static str, + /// The OIDC `ref` claim. + pub git_ref: Option, + /// The OIDC `event_name` claim. + pub event_name: Option, +} + +/// Check a run record against the token that uploaded it, and replace what +/// the token knows better. The record's fields are the client's to choose, +/// so without this one job could file results under another job's run id +/// (taking it first, so the real upload is a duplicate), another repository's +/// id, or a commit and branch it never ran. +/// +/// - An OIDC token's run id must be `gh::::...` +/// for the token's own repository, run and attempt (what Aster's client +/// builds in GitHub Actions); its `sha` must be the token's; `branch`, +/// `pr` and `attempt` come from the `ref` and `run_attempt` claims. +/// - A static token's run id names its repository: a `gh:` id must be +/// `gh::...`, and any other id is stored as +/// `static::`. +/// +/// Repositories compare without case, as GitHub's do. So no two +/// repositories' run ids can collide. +pub fn bind(identity: &Identity, mut run: RunRecord) -> Result<(RunRecord, Source), String> { + if !run.repo.eq_ignore_ascii_case(&identity.repository) { + return Err(format!( + "the token is for {} but the results are for {}", + identity.repository, run.repo + )); + } + let starts_with = |id: &str, prefix: &str| { + id.get(..prefix.len()) + .is_some_and(|head| head.eq_ignore_ascii_case(prefix)) + }; + let repo_prefix = format!("gh:{}:", identity.repository); + if identity.static_token { + if starts_with(&run.run_id, "gh:") { + if !starts_with(&run.run_id, &repo_prefix) { + return Err(format!( + "the run id {} is not one of {}'s", + run.run_id, identity.repository + )); + } + } else { + run.run_id = format!("static:{}:{}", identity.repository, run.run_id); + } + return Ok(( + run, + Source { + kind: "static", + git_ref: None, + event_name: None, + }, + )); + } + + let Some(github_run) = identity.run_id.as_deref() else { + return Err("the token has no run_id claim".into()); + }; + let prefix = match identity.run_attempt.as_deref() { + Some(attempt) => format!("{repo_prefix}{github_run}:{attempt}:"), + None => format!("{repo_prefix}{github_run}:"), + }; + if !starts_with(&run.run_id, &prefix) { + return Err(format!( + "the run id {} is not this job's: it must start with {prefix}", + run.run_id + )); + } + if let Some(sha) = identity.sha.as_deref() { + if run.sha != sha { + return Err(format!( + "the results are for commit {} but the token is for {sha}", + run.sha + )); + } + } + if let Some(attempt) = identity.run_attempt.as_deref() { + run.attempt = attempt.parse().ok(); + } + if let Some(git_ref) = identity.git_ref.as_deref() { + run.pr = pull_request(git_ref); + if let Some(branch) = git_ref.strip_prefix("refs/heads/") { + run.branch = Some(branch.to_string()); + } + } + Ok(( + run, + Source { + kind: "oidc", + git_ref: identity.git_ref.clone(), + event_name: identity.event_name.clone(), + }, + )) +} + +/// The pull request a ref belongs to: `refs/pull/123/merge`, or a merge +/// queue branch `refs/heads/gh-readonly-queue/main/pr-123-`. +fn pull_request(reference: &str) -> Option { + if let Some(rest) = reference.strip_prefix("refs/pull/") { + return rest.split('/').next()?.parse().ok(); + } + let queue = reference.strip_prefix("refs/heads/gh-readonly-queue/")?; + let entry = queue.rsplit('/').next()?; + entry.strip_prefix("pr-")?.split('-').next()?.parse().ok() +} + type Parts = (RunRecord, Vec, Vec); /// The run record (first and only one), then its targets and tests. @@ -160,6 +268,7 @@ fn enum_text(value: &T) -> String { pub async fn store( app: &App, run: &RunRecord, + source: &Source, targets: &[TargetRecord], tests: &[TestRecord], received_at: DateTime, @@ -177,8 +286,9 @@ pub async fn store( let inserted = transaction .execute( "INSERT INTO test_results.runs (run_id, repo, sha, branch, pr, workflow, job, \ - attempt, lane, runner, aster_version, started_at, received_at) \ - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13) \ + attempt, lane, runner, aster_version, started_at, received_at, \ + source, git_ref, event_name) \ + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16) \ ON CONFLICT (run_id) DO NOTHING", &[ &run.run_id, @@ -194,6 +304,9 @@ pub async fn store( &run.aster_version, &started_at, &received_at, + &source.kind, + &source.git_ref, + &source.event_name, ], ) .await?; @@ -385,6 +498,104 @@ mod tests { } } + fn oidc(git_ref: &str, event: &str) -> Identity { + Identity { + repository: "ArchAstro/firstlanding".into(), + git_ref: Some(git_ref.into()), + run_id: Some("1".into()), + run_attempt: Some("1".into()), + sha: Some("abc".into()), + workflow: None, + job_workflow: None, + event_name: Some(event.into()), + static_token: false, + } + } + + #[test] + fn an_oidc_upload_is_bound_to_its_own_run_commit_and_ref() { + // A pull request job: its branch and number come from `ref`, not the + // record, which claims to be main. + let mut claimed = run(); + claimed.branch = Some("main".into()); + claimed.pr = Some(999); + let (bound, source) = bind(&oidc("refs/pull/7/merge", "pull_request"), claimed).unwrap(); + assert_eq!(bound.pr, Some(7)); + assert_eq!(bound.attempt, Some(1)); + assert_eq!( + source, + Source { + kind: "oidc", + git_ref: Some("refs/pull/7/merge".into()), + event_name: Some("pull_request".into()), + } + ); + // A push: the branch is the ref's. + let (bound, _) = bind(&oidc("refs/heads/main", "push"), run()).unwrap(); + assert_eq!((bound.branch.as_deref(), bound.pr), (Some("main"), None)); + // Repository case does not matter. + let mut lower = run(); + lower.run_id = "gh:archastro/FIRSTLANDING:1:1:x".into(); + assert!(bind(&oidc("refs/heads/main", "push"), lower).is_ok()); + } + + #[test] + fn an_oidc_upload_cannot_take_another_runs_id_or_commit() { + let job = oidc("refs/pull/7/merge", "pull_request"); + for (run_id, why) in [ + // A later run of the repository: taking it first would make the + // real upload a duplicate. + ("gh:ArchAstro/firstlanding:2:1:test-ci:x", "another run"), + ("gh:ArchAstro/firstlanding:1:2:test-ci:x", "another attempt"), + ( + "gh:ArchAstro/firstlanding:12:1:test-ci:x", + "a run id it prefixes", + ), + ("gh:ArchAstro/other:1:1:test-ci:x", "another repository"), + ("static:ArchAstro/firstlanding:x", "a static token's"), + ("local:test:ab", "not a GitHub run"), + ] { + let mut forged = run(); + forged.run_id = run_id.into(); + assert!(bind(&job, forged).is_err(), "{why}"); + } + let mut other_commit = run(); + other_commit.sha = "def".into(); + assert!(bind(&job, other_commit).is_err()); + let mut no_run = job.clone(); + no_run.run_id = None; + assert!(bind(&no_run, run()).is_err()); + let mut other_repo = run(); + other_repo.repo = "ArchAstro/other".into(); + assert!(bind(&job, other_repo).is_err()); + } + + #[test] + fn a_static_tokens_run_ids_name_its_repository() { + let token = Identity { + repository: "ArchAstro/dev".into(), + git_ref: None, + run_id: None, + run_attempt: None, + sha: None, + workflow: None, + job_workflow: None, + event_name: None, + static_token: true, + }; + let mut local = run(); + local.repo = "ArchAstro/dev".into(); + local.run_id = "local:test:ab".into(); + let (bound, source) = bind(&token, local.clone()).unwrap(); + assert_eq!(bound.run_id, "static:ArchAstro/dev:local:test:ab"); + assert_eq!(source.kind, "static"); + // Its own repository's GitHub ids pass; another's do not. + local.run_id = "gh:ArchAstro/dev:5:1:x".into(); + assert_eq!(bind(&token, local.clone()).unwrap().0.run_id, local.run_id); + local.run_id = "gh:ArchAstro/firstlanding:5:1:x".into(); + assert!(bind(&token, local).is_err()); + } + #[test] fn raw_keys_are_safe_paths() { let day = NaiveDate::from_ymd_opt(2026, 10, 13).unwrap(); diff --git a/crates/aster-srv/src/test_results/mod.rs b/crates/aster-srv/src/test_results/mod.rs index 97d4a14..ba853e7 100644 --- a/crates/aster-srv/src/test_results/mod.rs +++ b/crates/aster-srv/src/test_results/mod.rs @@ -264,6 +264,24 @@ WHERE c.failures >= 20 OR (c.failures >= 5 AND c.failures * 2 >= c.tests) OR COALESCE(h.shared, 0) >= 10 ON CONFLICT (run_id, target) DO NOTHING; +"#, + }, + Migration { + feature: "test_results", + version: 7, + // Where each run came from, from the token that uploaded it rather + // than the run record: `source` (`oidc` or `static`) and the OIDC + // `ref` and `event_name` claims. Nullable columns on a small table; + // runs stored before have none and are never trusted. Each flake + // event says whether both of its runs are trusted, which + // quarantine requires; a constant default, so no rewrite. + sql: r#" +ALTER TABLE test_results.runs + ADD COLUMN source text, + ADD COLUMN git_ref text, + ADD COLUMN event_name text; +ALTER TABLE test_results.test_flake_events + ADD COLUMN trusted boolean NOT NULL DEFAULT false; "#, }, ]; diff --git a/crates/aster-srv/src/test_results/quarantine.rs b/crates/aster-srv/src/test_results/quarantine.rs index fe789ad..6fbe329 100644 --- a/crates/aster-srv/src/test_results/quarantine.rs +++ b/crates/aster-srv/src/test_results/quarantine.rs @@ -11,9 +11,12 @@ //! 3. A test whose final results since it entered end in //! `ASTER_SRV_QUARANTINE_RELEASE_PASSES` consecutive passes is released. //! Skips and cached targets' replayed results do not count. -//! 4. A test that is not broken enters when definite flake events from -//! `ASTER_SRV_QUARANTINE_RUNS` different failing runs fall in the last 7 -//! days. Only events after the test last left count, so a released or +//! 4. A test that is not broken enters when trusted definite flake events +//! from `ASTER_SRV_QUARANTINE_RUNS` different failing runs fall in the +//! last 7 days. An event is trusted when both its runs are default-branch +//! runs whose source is known (see `Window::trusted`): pull request jobs +//! run their author's code, so their results still score flakes but +//! never hide a failure. Only events after the test last left count, so a released or //! expired test is judged afresh. A run holding definite events of more //! than `ASTER_SRV_QUARANTINE_RUN_CAP` tests in those days counts for //! none: many tests flaking in one run point at the run, not the tests. @@ -226,7 +229,8 @@ pub(super) async fn update( recent AS ( \ SELECT e.repo, e.test_id, e.run_id, e.observed_at \ FROM test_results.test_flake_events e \ - WHERE e.kind = 'definite' AND e.observed_on >= '{from_day}'::date \ + WHERE e.kind = 'definite' AND e.trusted \ + AND e.observed_on >= '{from_day}'::date \ AND e.observed_at > {to} - {ENTRY_DAYS} * interval '24 hours' \ AND e.observed_at <= {to}), \ crowded AS ( \ diff --git a/crates/aster-srv/src/test_results/query.rs b/crates/aster-srv/src/test_results/query.rs index 68275bf..2f4b2d7 100644 --- a/crates/aster-srv/src/test_results/query.rs +++ b/crates/aster-srv/src/test_results/query.rs @@ -561,13 +561,11 @@ async fn run_failures( else { return Err(Reply::error(404, "no such run").into()); }; + // Another repository's run is indistinguishable from a missing one: the + // reply must not say that the id exists, or whose it is. let repo: String = run.get("repo"); if !repo.eq_ignore_ascii_case(&identity.repository) { - return Err(Reply::error( - 403, - format!("the run belongs to {repo}, not {}", identity.repository), - ) - .into()); + return Err(Reply::error(404, "no such run").into()); } let received_on: chrono::NaiveDate = run.get("received_on"); let after = params.cursor.clone().unwrap_or_default(); diff --git a/crates/aster-srv/tests/cache.rs b/crates/aster-srv/tests/cache.rs index 1526d5c..219324e 100644 --- a/crates/aster-srv/tests/cache.rs +++ b/crates/aster-srv/tests/cache.rs @@ -1552,3 +1552,92 @@ async fn retention_prunes_old_lookup_stats() { assert_eq!(left, [20, 0]); server.stop().await; } + +/// N13. A pull-request CI token may read the cache, but its lookups must not +/// bump `hits`, `last_hit_at` or `last_used_at`: any PR author could otherwise +/// keep an entry alive forever. A person's lookup does not count either. A +/// main push (writer-eligible) still counts. +#[tokio::test] +async fn a_pull_request_lookup_does_not_count_but_a_main_push_does() { + let Some((server, _)) = server().await else { + return; + }; + // Phase 1: main commits one entry. + put_blob(&server, DIGEST).await; + let writer = bearer(REPO, Some("refs/heads/main")); + commit_as(&server, &writer, "k1", "//a:b").await; + let used_before: chrono::DateTime = server + .client() + .await + .query_one( + "SELECT last_used_at FROM cache.manifests WHERE key = 'k1'", + &[], + ) + .await + .unwrap() + .get(0); + + // Phase 2: a pull_request token looks the key up. It hits, but nothing is + // counted in the manifest row. + let pull_request = format!( + "Bearer {}", + common::token_with_claims( + REPO, + json!({"event_name": "pull_request", "ref": "refs/pull/7/merge"}), + ) + ); + let (status, body) = server + .post_json( + "/v1/cache/lookup", + &json!({"repo": REPO, "keys": ["k1"]}), + Some(&pull_request), + None, + ) + .await; + assert_eq!(status, 200); + assert!(body["hits"]["k1"].is_object(), "{body}"); + let state = |client: deadpool_postgres::Client| async move { + let row = client + .query_one( + "SELECT hits, last_hit_at IS NOT NULL, last_used_at FROM cache.manifests \ + WHERE key = 'k1'", + &[], + ) + .await + .unwrap(); + ( + row.get::<_, i64>(0), + row.get::<_, bool>(1), + row.get::<_, chrono::DateTime>(2), + ) + }; + assert_eq!(state(server.client().await).await, (0, false, used_before)); + + // Phase 3: a signed-in person looks it up. People never count either. + let (status, body) = server + .post_json( + "/v1/cache/lookup", + &json!({"repo": REPO, "keys": ["k1"]}), + Some(&common::person_bearer(&server, VIEWER)), + None, + ) + .await; + assert_eq!(status, 200, "{body}"); + assert!(body["hits"]["k1"].is_object(), "{body}"); + assert_eq!(state(server.client().await).await, (0, false, used_before)); + + // Phase 4: a main push looks it up and is counted. + let (status, _) = server + .post_json( + "/v1/cache/lookup", + &json!({"repo": REPO, "keys": ["k1"]}), + Some(&writer), + None, + ) + .await; + assert_eq!(status, 200); + let (hits, hit_at, used) = state(server.client().await).await; + assert_eq!((hits, hit_at), (1, true)); + assert!(used >= used_before); + server.stop().await; +} diff --git a/crates/aster-srv/tests/classifier_model.rs b/crates/aster-srv/tests/classifier_model.rs index c9dff73..32201fa 100644 --- a/crates/aster-srv/tests/classifier_model.rs +++ b/crates/aster-srv/tests/classifier_model.rs @@ -246,11 +246,21 @@ impl Model { events.insert(event("definite", "retry", f)); definite.insert(f.key.1.clone()); } + // The nearest earlier run of the hash, and for a trusted + // (default-branch) run also the nearest earlier trusted one. let earlier = list[..i] .iter() .rev() .find(|p| p.input_hash == f.input_hash); - if let Some(p) = earlier.filter(|p| p.is_fail != f.is_fail) { + let earlier_trusted = list[..i] + .iter() + .rev() + .find(|p| p.input_hash == f.input_hash && p.on_default) + .filter(|_| f.on_default); + for p in [earlier, earlier_trusted].into_iter().flatten() { + if p.is_fail == f.is_fail { + continue; + } let failed = if f.is_fail { f } else { p }; if !incidents.contains(&failed.key.1) { events.insert(event("definite", "input_hash", failed)); diff --git a/crates/aster-srv/tests/cli_auth.rs b/crates/aster-srv/tests/cli_auth.rs index a56ad17..91f1303 100644 --- a/crates/aster-srv/tests/cli_auth.rs +++ b/crates/aster-srv/tests/cli_auth.rs @@ -1113,7 +1113,9 @@ fn manifest_body(key: &str) -> Value { } fn ci(git_ref: &str, job_workflow: Option<&str>) -> String { - let mut claims = json!({"ref": git_ref}); + // One GitHub run (1, attempt 1) of commit 9f1c, which the uploads below + // name. + let mut claims = json!({"ref": git_ref, "run_id": "1", "run_attempt": "1", "sha": "9f1c"}); if let Some(workflow) = job_workflow { claims["job_workflow_ref"] = json!(workflow); } @@ -1123,7 +1125,7 @@ fn ci(git_ref: &str, job_workflow: Option<&str>) -> String { async fn upload_results(server: &Server, bearer: &str) -> u16 { use aster_results_model::{to_ndjson, Record, RunRecord}; let body = to_ndjson(&[Record::Run(RunRecord { - run_id: "run-1".into(), + run_id: format!("gh:{REPO}:1:1:test:-"), repo: REPO.into(), sha: "9f1c".into(), branch: Some("main".into()), diff --git a/crates/aster-srv/tests/cli_auth_copy.rs b/crates/aster-srv/tests/cli_auth_copy.rs index 9083366..a55361f 100644 --- a/crates/aster-srv/tests/cli_auth_copy.rs +++ b/crates/aster-srv/tests/cli_auth_copy.rs @@ -215,6 +215,32 @@ async fn copy_mode_shows_a_one_time_code_and_never_redirects_or_shows_a_token() "{}", form.body ); + // Each detail says who vouches for it: the device name is typed by the + // requesting command, so the page says it is not verified. + assert!( + form.body.contains( + "Device name (reported by the requesting computer, not verified): devbox-7" + ), + "{}", + form.body + ); + assert!( + form.body + .contains("Your browser's address (seen by this server)"), + "{}", + form.body + ); + assert!( + form.body.contains("Your browser (as it describes itself)"), + "{}", + form.body + ); + assert!( + form.body + .contains("Only continue if you started this command yourself"), + "copy mode warns on the confirm page: {}", + form.body + ); assert!( !form.body.contains("name=\"port\"") && !form.body.contains("(port"), "copy mode has no port: {}", @@ -235,6 +261,11 @@ async fn copy_mode_shows_a_one_time_code_and_never_redirects_or_shows_a_token() let code = shown_code(&done.body); assert_eq!(code.len(), 43, "a 256-bit code, base64url"); assert!(done.body.contains("state-co"), "the request id is shown"); + assert!( + done.body.contains("Never send this code to anyone."), + "{}", + done.body + ); for (name, value) in [ ("cache-control", "no-store"), ("x-frame-options", "DENY"), diff --git a/crates/aster-srv/tests/common/mod.rs b/crates/aster-srv/tests/common/mod.rs index 116b918..ce0a5f6 100644 --- a/crates/aster-srv/tests/common/mod.rs +++ b/crates/aster-srv/tests/common/mod.rs @@ -181,7 +181,15 @@ async fn launch( { Some(CLI_TOKEN_KEYS.into()) } - "ASTER_SRV_DEV_BLOB_ROUTE" => Some("1".into()), + // The `/v1` listener refuses to start with the stand-in route, + // without owner-id pins, or without a writer-workflow list. + "ASTER_SRV_DEV_BLOB_ROUTE" if !has("ASTER_SRV_API_ADDR") => Some("1".into()), + "ASTER_SRV_REPOSITORY_OWNER_IDS" if has("ASTER_SRV_API_ADDR") => { + Some("ArchAstro/*=4242".into()) + } + "ASTER_SRV_CACHE_WRITER_WORKFLOWS" if has("ASTER_SRV_API_ADDR") => { + Some("ArchAstro/firstlanding/.github/workflows/ci.yml@".into()) + } _ => None, }), }) @@ -401,6 +409,7 @@ pub fn token_with_ref(repository: &str, workflow_ref: Option<&str>) -> String { "aud": "aster-srv", "exp": Utc::now().timestamp() + 300, "repository": repository, + "repository_owner_id": "4242", "event_name": "push", }); if let Some(workflow_ref) = workflow_ref { @@ -424,6 +433,7 @@ pub fn token_with_claims(repository: &str, extra: Value) -> String { "aud": "aster-srv", "exp": Utc::now().timestamp() + 300, "repository": repository, + "repository_owner_id": "4242", "event_name": "push", }); for (name, value) in extra.as_object().expect("claims are an object") { @@ -455,6 +465,12 @@ pub struct Run<'a> { pub lane: Option<&'a str>, /// The CI attempt. pub attempt: Option, + /// Who uploaded it: `static` (the default, judged by `branch`) or + /// `oidc` with `git_ref` and `event_name`; `None` for a run stored + /// before sources were recorded. + pub source: Option<&'a str>, + pub git_ref: Option<&'a str>, + pub event_name: Option<&'a str>, /// `(test_id, outcome, attempt, failure_hash)`. pub results: Vec<(&'a str, &'a str, i32, Option<&'a str>)>, } @@ -473,10 +489,21 @@ impl<'a> Run<'a> { job: None, lane: None, attempt: None, + source: Some("static"), + git_ref: None, + event_name: None, results: Vec::new(), } } + /// Uploaded with an OIDC token for `git_ref` and `event_name`. + pub fn oidc(mut self, git_ref: &'a str, event_name: &'a str) -> Self { + self.source = Some("oidc"); + self.git_ref = Some(git_ref); + self.event_name = Some(event_name); + self + } + pub fn hash(mut self, input_hash: &'a str) -> Self { self.input_hash = input_hash; self @@ -519,8 +546,8 @@ pub async fn seed(server: &Server, run: &Run<'_>) { .execute( "INSERT INTO test_results.runs \ (run_id, repo, sha, branch, job, lane, attempt, runner, aster_version, \ - started_at, received_at) \ - VALUES ($1, $2, $3, $4, $6, $7, $8, $9, '0', $5, $5)", + started_at, received_at, source, git_ref, event_name) \ + VALUES ($1, $2, $3, $4, $6, $7, $8, $9, '0', $5, $5, $10, $11, $12)", &[ &run.run_id, &run.repo, @@ -531,6 +558,9 @@ pub async fn seed(server: &Server, run: &Run<'_>) { &run.lane, &run.attempt, &runner, + &run.source, + &run.git_ref, + &run.event_name, ], ) .await diff --git a/crates/aster-srv/tests/hardening.rs b/crates/aster-srv/tests/hardening.rs index c5d12cc..451ed13 100644 --- a/crates/aster-srv/tests/hardening.rs +++ b/crates/aster-srv/tests/hardening.rs @@ -1256,3 +1256,410 @@ async fn https_deployments_send_hsts_on_every_response_and_auth_responses_are_ne assert_eq!(response.headers()["cache-control"], "no-store"); dev.stop().await; } + +// ------------------------------------------------- N3: unauthenticated slowloris + +/// A POST that declares 100 body bytes and sends one, from the client the +/// load balancer would report as `client` (`X-Forwarded-For` is +/// `, , `). The stream stays open, holding the +/// request in flight. +async fn stalled_post(url: &str, path: &str, client: &str) -> tokio::net::TcpStream { + let mut stream = tokio::net::TcpStream::connect(url.trim_start_matches("http://")) + .await + .unwrap(); + stream + .write_all( + format!( + "POST {path} HTTP/1.1\r\nHost: x\r\nx-forwarded-for: 6.6.6.6, {client}, 34.8.0.1\r\n\ + Content-Length: 100\r\n\r\n{{" + ) + .as_bytes(), + ) + .await + .unwrap(); + stream +} + +/// N3: an unauthenticated body gets its own short deadline, well under the +/// global body timeout that authenticated uploads keep. +#[tokio::test] +async fn unauthenticated_bodies_time_out_in_seconds_while_authenticated_ones_keep_the_long_timeout() +{ + let Some(server) = start_with(&[ + ("ASTER_SRV_API_ADDR", "127.0.0.1:0"), + ("ASTER_SRV_UNAUTH_BODY_TIMEOUT_SECS", "1"), + ("ASTER_SRV_BODY_TIMEOUT_SECS", "30"), + ]) + .await + else { + return; + }; + for url in [&server.url, server.api_url.as_ref().unwrap()] { + for path in ["/v1/auth/token", "/cli-auth"] { + let stalled = + format!("POST {path} HTTP/1.1\r\nHost: x\r\nContent-Length: 100\r\n\r\n{{"); + let started = Instant::now(); + let reply = raw(url, stalled.as_bytes(), Duration::from_secs(10)).await; + if path == "/cli-auth" && url != &server.url { + // Not served on the API port at all. + assert_eq!(status_of(&reply), Some(404), "{reply}"); + continue; + } + assert_eq!(status_of(&reply), Some(408), "{url}{path}: {reply}"); + assert!( + started.elapsed() < Duration::from_secs(5), + "{url}{path}: {:?}", + started.elapsed() + ); + } + } + // A CI upload with a valid token is still waited for: no reply in 3s. + let token = common::token_with_claims( + REPO, + json!({"job_workflow_ref": "ArchAstro/firstlanding/.github/workflows/ci.yml@refs/heads/main"}), + ); + let stalled = format!( + "POST /v1/test-results/runs HTTP/1.1\r\nHost: x\r\nauthorization: Bearer {token}\r\n\ + Content-Length: 100\r\n\r\n{{" + ); + let started = Instant::now(); + let reply = raw(&server.url, stalled.as_bytes(), Duration::from_secs(3)).await; + assert_eq!(reply, "", "an authenticated body keeps the long timeout"); + assert!(started.elapsed() >= Duration::from_secs(3)); + server.stop().await; +} + +/// N3: `header_read_timeout` also bounds an idle keep-alive connection: after +/// a completed request the connection is closed once no next request's +/// headers arrive in time. +#[tokio::test] +async fn an_idle_keep_alive_connection_is_closed_after_the_header_timeout() { + let Some(server) = start_with(&[("ASTER_SRV_HEADER_TIMEOUT_SECS", "1")]).await else { + return; + }; + let mut stream = tokio::net::TcpStream::connect(server.url.trim_start_matches("http://")) + .await + .unwrap(); + stream + .write_all(b"GET /healthz HTTP/1.1\r\nHost: x\r\n\r\n") + .await + .unwrap(); + let mut buffer = [0u8; 2048]; + let n = tokio::time::timeout(Duration::from_secs(5), stream.read(&mut buffer)) + .await + .expect("answered") + .unwrap(); + assert!(String::from_utf8_lossy(&buffer[..n]).starts_with("HTTP/1.1 200")); + // Keep-alive: the connection stays open for the next request, then goes. + let started = Instant::now(); + let closed = tokio::time::timeout(Duration::from_secs(10), stream.read(&mut buffer)) + .await + .expect("an idle connection must not stay open"); + assert!(matches!(closed, Ok(0) | Err(_)), "{closed:?}"); + assert!( + started.elapsed() < Duration::from_secs(5), + "idle for {:?}", + started.elapsed() + ); + server.stop().await; +} + +/// N3: behind the balancer one client IP, as `client_ip` resolves it, holds +/// at most `ASTER_SRV_MAX_CONNECTIONS_PER_IP` requests open; a spoofed +/// `X-Forwarded-For` prefix does not make a second client of it, and another +/// client is unaffected. +#[tokio::test] +async fn one_client_ip_cannot_hold_more_than_its_share_of_requests_open() { + let Some(server) = start_with(&[ + ("ASTER_SRV_MAX_CONNECTIONS_PER_IP", "2"), + ("ASTER_SRV_UNAUTH_BODY_TIMEOUT_SECS", "30"), + ("ASTER_SRV_BODY_TIMEOUT_SECS", "30"), + ]) + .await + else { + return; + }; + let url = &server.url; + let first = stalled_post(url, "/v1/auth/token", "203.0.113.7").await; + let _second = stalled_post(url, "/v1/auth/token", "203.0.113.7").await; + tokio::time::sleep(Duration::from_millis(300)).await; + // The third from the same client, whatever it prefixes, is turned away. + let third = "GET /healthz HTTP/1.1\r\nHost: x\r\nConnection: close\r\n\ + x-forwarded-for: 1.2.3.4, 203.0.113.7, 34.8.0.1\r\n\r\n"; + let reply = raw(url, third.as_bytes(), Duration::from_secs(5)).await; + assert_eq!(status_of(&reply), Some(429), "{reply}"); + assert!(reply.to_lowercase().contains("retry-after"), "{reply}"); + // Another client address is served. + let other = "GET /healthz HTTP/1.1\r\nHost: x\r\nConnection: close\r\n\ + x-forwarded-for: 1.2.3.4, 203.0.113.8, 34.8.0.1\r\n\r\n"; + let reply = raw(url, other.as_bytes(), Duration::from_secs(5)).await; + assert_eq!(status_of(&reply), Some(200), "{reply}"); + // The first slot frees when its request ends. + drop(first); + let mut status = None; + for _ in 0..50 { + let reply = raw(url, third.as_bytes(), Duration::from_secs(5)).await; + status = status_of(&reply); + if status == Some(200) { + break; + } + tokio::time::sleep(Duration::from_millis(100)).await; + } + assert_eq!(status, Some(200)); + let metrics = server.app.metrics.render(); + assert!(metrics.contains("scope=\"ip_concurrency\""), "{metrics}"); + server.stop().await; +} + +/// N3: with no trusted proxy the TCP peer is the client, so its open +/// connections are capped at accept: the extra one is closed unserved. +#[tokio::test] +async fn without_a_proxy_one_peer_cannot_hold_more_connections_than_its_share() { + let Some(server) = start_with(&[ + ("ASTER_SRV_TRUSTED_PROXY_HOPS", "0"), + ("ASTER_SRV_MAX_CONNECTIONS_PER_IP", "2"), + ("ASTER_SRV_HEADER_TIMEOUT_SECS", "60"), + ]) + .await + else { + return; + }; + let address = server.url.trim_start_matches("http://").to_string(); + let first = tokio::net::TcpStream::connect(&address).await.unwrap(); + let _second = tokio::net::TcpStream::connect(&address).await.unwrap(); + tokio::time::sleep(Duration::from_millis(300)).await; + let reply = raw( + &server.url, + b"GET /healthz HTTP/1.1\r\nHost: x\r\nConnection: close\r\n\r\n", + Duration::from_secs(5), + ) + .await; + assert_eq!( + reply, "", + "a third connection from the peer is closed unserved" + ); + drop(first); + let mut served = false; + for _ in 0..50 { + let reply = raw( + &server.url, + b"GET /healthz HTTP/1.1\r\nHost: x\r\nConnection: close\r\n\r\n", + Duration::from_secs(5), + ) + .await; + if status_of(&reply) == Some(200) { + served = true; + break; + } + tokio::time::sleep(Duration::from_millis(100)).await; + } + assert!(served, "a slot frees when a connection closes"); + server.stop().await; +} + +/// N4: on a server with the API listener, a token whose `repository_owner_id` +/// is not the pinned one, or is missing, is refused on both ports; the +/// pinned owner passes. (Startup refuses an unpinned allowlist entry, so the +/// fail-closed case itself is the `OwnerIds` unit test.) +#[tokio::test] +async fn the_api_listener_refuses_tokens_without_the_pinned_owner_id() { + let Some(server) = start_with(&[("ASTER_SRV_API_ADDR", "127.0.0.1:0")]).await else { + return; + }; + let wrong = format!( + "Bearer {}", + common::token_with_claims(REPO, json!({"repository_owner_id": "1"})) + ); + let missing = format!( + "Bearer {}", + common::token_with_claims(REPO, json!({"repository_owner_id": null})) + ); + for url in [&server.url, server.api_url.as_ref().unwrap()] { + assert_eq!(lookup(url, &ci_bearer()).await.status(), 200); + assert_eq!(lookup(url, &wrong).await.status(), 403, "{url}"); + assert_eq!(lookup(url, &missing).await.status(), 403, "{url}"); + } + server.stop().await; +} + +// ------------------------------------------------------ N15: sampled 401 logs + +/// N15: a flood of failures from one client logs the first few lines and a +/// summary, the counters stay exact, and the audit trail of successful +/// mints, exchanges and revokes is complete however many there are. +#[tokio::test] +async fn a_401_flood_is_sampled_and_summarised_while_counters_and_the_success_trail_stay_exact() { + let Some(server) = start_with(&[("ASTER_SRV_LOG_SAMPLE_FIRST", "5")]).await else { + return; + }; + let url = server.url.clone(); + let flooder = "198.51.100.77"; + // 200 bad bearer tokens from one client IP (a spoofed prefix is ignored). + for n in 0..200 { + let response = client() + .post(format!("{url}/v1/cache/lookup")) + .header("authorization", format!("Bearer flood-{n}-not-a-token")) + .header("x-forwarded-for", format!("6.6.6.6, {flooder}, 34.8.0.1")) + .json(&json!({"repo": REPO, "keys": ["k"]})) + .send() + .await + .unwrap(); + assert_eq!(response.status(), 401); + } + // Eight complete sign-ins, more than the sample size, then eight revokes. + let who = "n15-ada@example.com"; + let subject = iap_subject(who); + for _ in 0..8 { + let (code, verifier) = browser_login(&url, &iap_assertion(who)).await; + let (status, _) = redeem(&url, &code, &verifier).await; + assert_eq!(status, 200); + aster_srv::audit::token_revoke(&subject, "operator"); + } + server.app.audit_sampler.flush_all(); + + let lines: Vec = aster_srv::log::captured() + .iter() + .filter_map(|line| serde_json::from_str::(line).ok()) + .collect(); + let audit = |event: &str, f: &dyn Fn(&Value) -> bool| { + lines + .iter() + .filter(|l| l["audit"] == true && l["event"] == event && f(l)) + .count() + }; + // Sampled: at most 5 failure lines for the flooder, the rest summarised. + let from_flooder = |l: &Value| l["client_ip"] == flooder && l["route"] == "/v1/cache"; + assert_eq!(audit("auth_failure", &from_flooder), 5); + let summary: Vec<_> = lines + .iter() + .filter(|l| { + l["event"] == "audit_summary" && l["client_ip"] == flooder && l["of"] == "auth_failure" + }) + .collect(); + assert_eq!(summary.len(), 1, "{summary:?}"); + assert_eq!(summary[0]["suppressed"], 195); + assert_eq!(summary[0]["reason"], "invalid_token"); + // The server's own "token refused" warnings are sampled the same way. + let refused = lines + .iter() + .filter(|l| l["message"] == "token refused") + .count(); + assert!(refused <= 5 + 8, "{refused} token refused lines"); + // Counters are exact. + let metrics = server.app.metrics.render(); + let expected = + "aster_srv_auth_failures_total{route=\"/v1/cache\",reason=\"invalid_token\"} 200"; + assert!(metrics.contains(expected), "{expected}\n{metrics}"); + // The success trail is complete: every mint, exchange and revoke. + assert_eq!( + audit("token_mint", &|l| l["subject"] == subject.as_str()), + 8 + ); + assert_eq!( + audit("token_exchange", &|l| l["outcome"] == "success" + && l["subject"] == subject.as_str()), + 8 + ); + assert_eq!( + audit("token_revoke", &|l| l["subject"] == subject.as_str()), + 8 + ); + server.stop().await; +} + +// ------------------------------------------------------------ N12: the client IP + +/// N12: the client IP the server rate-limits and audits is the one +/// `limits::client_ip` picks from every `X-Forwarded-For` line together, for +/// the configured number of trusted hops: repeated lines are one list, a +/// spoofed prefix changes nothing, and more hops move the client left. +/// Each case uses its own address, so the shared log capture cannot mix cases. +#[tokio::test] +async fn the_client_ip_comes_from_all_forwarded_for_lines_and_the_trusted_hops() { + // A failed sign-in exchange sent with `lines`; the audit trail must name + // `expected`. (An exchange, not a bad bearer: a bad bearer also writes a + // "token refused" warning, which the N15 test counts across this binary.) + async fn assert_audited_as(server: &Server, lines: &[&str], expected: &str) { + let headers: String = lines + .iter() + .map(|line| format!("x-forwarded-for: {line}\r\n")) + .collect(); + let (verifier, _) = pkce(); + let body = json!({ + "code": BASE64URL.encode([7u8; 32]), + "verifier": verifier, + "port": 40123, + "user_code": USER_CODE, + }) + .to_string(); + let request = format!( + "POST /v1/auth/token HTTP/1.1\r\nHost: x\r\nConnection: close\r\n{headers}\ + content-type: application/json\r\ncontent-length: {}\r\n\r\n{body}", + body.len() + ); + let reply = raw(&server.url, request.as_bytes(), Duration::from_secs(5)).await; + assert_eq!(status_of(&reply), Some(400), "{reply}"); + let named = aster_srv::log::captured() + .iter() + .filter_map(|line| serde_json::from_str::(line).ok()) + .any(|l| { + l["audit"] == true + && l["event"] == "token_exchange" + && l["outcome"] == "failure" + && l["route"] == "/v1/auth/token" + && l["client_ip"] == expected + }); + assert!(named, "{lines:?} should be audited as {expected}"); + } + + // One trusted hop: the load balancer. + let Some(server) = start_with(&[("ASTER_SRV_LOG_SAMPLE_FIRST", "1000")]).await else { + return; + }; + assert_audited_as( + &server, + &["6.6.6.6, 203.0.113.91, 34.8.0.1"], + "203.0.113.91", + ) + .await; + assert_audited_as( + &server, + &["6.6.6.6, 203.0.113.93", "34.8.0.1"], + "203.0.113.93", + ) + .await; + assert_audited_as( + &server, + &["9.9.9.9", "5.5.5.5, 203.0.113.94, 34.8.0.1"], + "203.0.113.94", + ) + .await; + // Too few entries: the TCP peer, which is the test client. (Other tests + // also audit 127.0.0.1, so this only shows the request is served; the + // `limits` unit tests pin the fallback exactly.) + assert_audited_as(&server, &["34.8.0.1"], "127.0.0.1").await; + server.stop().await; + + // Two trusted hops (a proxy in front of the balancer): one entry further left. + let Some(server) = start_with(&[ + ("ASTER_SRV_TRUSTED_PROXY_HOPS", "2"), + ("ASTER_SRV_LOG_SAMPLE_FIRST", "1000"), + ]) + .await + else { + return; + }; + assert_audited_as( + &server, + &["6.6.6.6, 203.0.113.95, 10.0.0.5, 34.8.0.1"], + "203.0.113.95", + ) + .await; + assert_audited_as( + &server, + &["6.6.6.6", "203.0.113.96", "10.0.0.5, 34.8.0.1"], + "203.0.113.96", + ) + .await; + server.stop().await; +} diff --git a/crates/aster-srv/tests/incidents.rs b/crates/aster-srv/tests/incidents.rs index ba5f590..b8952ce 100644 --- a/crates/aster-srv/tests/incidents.rs +++ b/crates/aster-srv/tests/incidents.rs @@ -20,7 +20,7 @@ use aster_results_model::{ }; use aster_srv::test_results::classifier; use chrono::Utc; -use common::{iap_assertion, start, token, Server, IAP_AUDIENCE, REPO}; +use common::{iap_assertion, start, token_with_claims, Server, IAP_AUDIENCE, REPO}; use serde_json::{json, Value}; const SHARD_6: &str = "//src/elixir/core-test-shard-6:test-ci"; @@ -196,8 +196,41 @@ fn upload_stream( to_ndjson(&records) } +/// One CI job of that morning: its GitHub run, the ref and event that +/// started it, and the Aster run id its client builds. +struct Job { + github_run: &'static str, + git_ref: &'static str, + event: &'static str, +} + +impl Job { + fn run_id(&self) -> String { + format!( + "gh:{REPO}:{}:1:test:test-ci:source-elixir-core-6:ab", + self.github_run + ) + } +} + +const PR_RUN: Job = Job { + github_run: "39000054301", + git_ref: "refs/pull/18740/merge", + event: "pull_request", +}; +const CRASH_RUN: Job = Job { + github_run: "39000703969", + git_ref: "refs/heads/main", + event: "push", +}; +const PASS_RUN: Job = Job { + github_run: "39002241971", + git_ref: "refs/heads/main", + event: "workflow_dispatch", +}; + /// Upload a stream as CI does: gzipped NDJSON with the job's OIDC token. -async fn upload(server: &Server, ndjson: &str) { +async fn upload(server: &Server, job: &Job, ndjson: &str) { let mut gz = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::fast()); gz.write_all(ndjson.as_bytes()).unwrap(); let response = aster_srv::http_client() @@ -206,7 +239,16 @@ async fn upload(server: &Server, ndjson: &str) { .post(format!("{}/v1/test-results/runs", server.url)) .header("Content-Type", "application/x-ndjson") .header("Content-Encoding", "gzip") - .bearer_auth(token(REPO)) + .bearer_auth(token_with_claims( + REPO, + json!({ + "run_id": job.github_run, + "run_attempt": "1", + "sha": "78d67670d", + "ref": job.git_ref, + "event_name": job.event, + }), + )) .body(gz.finish().unwrap()) .send() .await @@ -237,7 +279,8 @@ async fn a_crashed_shard_on_unchanged_inputs_is_one_incident_and_quarantines_not // an ordinary flake: it fails once, in the main run below. upload( &server, - &upload_stream("gh:pr:39000054301", "fix/pr", 0, false), + &PR_RUN, + &upload_stream(&PR_RUN.run_id(), "fix/pr", 0, false), ) .await; classify(&server).await; @@ -247,7 +290,8 @@ async fn a_crashed_shard_on_unchanged_inputs_is_one_incident_and_quarantines_not // the next upload, as the five-minute loop did that morning. upload( &server, - &upload_stream("gh:main:39000703969", "main", 644, true), + &CRASH_RUN, + &upload_stream(&CRASH_RUN.run_id(), "main", 644, true), ) .await; let crash = classify(&server).await; @@ -256,7 +300,8 @@ async fn a_crashed_shard_on_unchanged_inputs_is_one_incident_and_quarantines_not // 04:26Z, Stable CI on main again, same inputs: everything passes. upload( &server, - &upload_stream("gh:main:39002241971", "main", 0, false), + &PASS_RUN, + &upload_stream(&PASS_RUN.run_id(), "main", 0, false), ) .await; classify(&server).await; @@ -273,7 +318,7 @@ async fn a_crashed_shard_on_unchanged_inputs_is_one_incident_and_quarantines_not .unwrap(); assert_eq!(incidents.len(), 1); let row = &incidents[0]; - assert_eq!(row.get::<_, String>("run_id"), "gh:main:39000703969"); + assert_eq!(row.get::<_, String>("run_id"), CRASH_RUN.run_id()); assert_eq!(row.get::<_, String>("target"), SHARD_6); assert_eq!(row.get::<_, i32>("failures"), 644); assert_eq!(row.get::<_, i32>("tests"), SHARD_6_TESTS as i32); @@ -284,20 +329,24 @@ async fn a_crashed_shard_on_unchanged_inputs_is_one_incident_and_quarantines_not // even though two passes on its inputs surround it: score 3, not 6. let events = client .query( - "SELECT test_id, kind, run_id FROM test_results.test_flake_events", + "SELECT test_id, kind, run_id, prev_run_id, trusted FROM test_results.test_flake_events", &[], ) .await .unwrap() .iter() - .map(|r| (r.get(0), r.get(1), r.get(2))) - .collect::>(); + .map(|r| (r.get(0), r.get(1), r.get(2), r.get(3), r.get(4))) + .collect::>(); + // It was first found against the pull request's pass (untrusted), then + // against main's own pass: the trusted comparison wins. assert_eq!( events, vec![( format!("{SHARD_2}#7"), "definite".to_string(), - "gh:main:39000703969".to_string() + CRASH_RUN.run_id(), + PASS_RUN.run_id(), + true )] ); let flaky = client diff --git a/crates/aster-srv/tests/jwks_refetch.rs b/crates/aster-srv/tests/jwks_refetch.rs new file mode 100644 index 0000000..755013b --- /dev/null +++ b/crates/aster-srv/tests/jwks_refetch.rs @@ -0,0 +1,249 @@ +//! When the verifiers refetch their issuer's signing keys, against a stub +//! JWKS server that counts fetches and can be made to fail. +//! +//! No database needed. + +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::{Arc, Mutex}; +use std::time::Duration; + +use aster_srv::auth::Verifier; +use aster_srv::iap::IapVerifier; +use aster_srv::keyfetch::RefetchPolicy; +use aster_srv::Config; +use base64::engine::general_purpose::URL_SAFE_NO_PAD as BASE64URL; +use base64::Engine; +use jsonwebtoken::{encode, Algorithm, EncodingKey, Header}; +use serde_json::json; +use tokio::io::{AsyncReadExt, AsyncWriteExt}; + +const PRIVATE: &[u8] = include_bytes!("fixtures/test-oidc-key.pem"); +const JWKS_N: &str = "855qXNm0Qs-azcPsy_QGxBFH4BPpxL3czNPvP4XV1XRv3tCIbacibo4Vbkvop-Z-4G70ZP-KNgMx_ItXqNpWN1erdKh_JWl0OVPMZpeYJH5T1jV2beTiflxah3pAAvv2lPC0m5QZR9dc6mE2XKcwyPPdhVq1CBeHmksnC8gQjM8iiQkWDuEU4FKbPxzQ6bQ0sau-STvZfy9eP8wQk1kxXE6acJaJaLd45Qqy3rTHVTOjCt3P_gjAbxY_bHY_zFZyn3hBrpuxgJN0fkIsAWHjWPKQzcGNnfbr4UI0R0YKiC69GkbWBUy1Jv-6d5o9cecUSVACA9SdH3A0TobNMwWXmQ"; +const IAP_AUDIENCE: &str = "/projects/1/global/backendServices/42"; + +/// Fast enough to test; the production defaults are 10s and 60s. +const POLICY: RefetchPolicy = RefetchPolicy { + min_interval: Duration::from_millis(400), + absent_for: Duration::from_secs(60), +}; + +/// A JWKS endpoint: it serves `kids` (as `rsa` or `ec` keys) while `healthy` +/// and a 500 otherwise, and counts every request. +struct Jwks { + url: String, + fetches: Arc, + healthy: Arc, + kids: Arc>>, +} + +async fn jwks(kind: &'static str) -> Jwks { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let url = format!("http://{}/jwks", listener.local_addr().unwrap()); + let stub = Jwks { + url, + fetches: Arc::default(), + healthy: Arc::new(AtomicBool::new(true)), + kids: Arc::default(), + }; + let (fetches, healthy, kids) = ( + Arc::clone(&stub.fetches), + Arc::clone(&stub.healthy), + Arc::clone(&stub.kids), + ); + tokio::spawn(async move { + loop { + let (mut stream, _) = listener.accept().await.unwrap(); + fetches.fetch_add(1, Ordering::SeqCst); + let ok = healthy.load(Ordering::SeqCst); + let keys: Vec<_> = kids + .lock() + .unwrap() + .iter() + .map(|kid| match kind { + "rsa" => json!({"kty": "RSA", "kid": kid, "use": "sig", "alg": "RS256", + "n": JWKS_N, "e": "AQAB"}), + _ => { + let point = &ec_pair().1; + json!({"kty": "EC", "crv": "P-256", "kid": kid, "use": "sig", "alg": "ES256", + "x": BASE64URL.encode(&point[1..33]), "y": BASE64URL.encode(&point[33..65])}) + } + }) + .collect(); + tokio::spawn(async move { + let mut buffer = [0u8; 2048]; + let _ = stream.read(&mut buffer).await; + let reply = if ok { + let body = json!({"keys": keys}).to_string(); + format!( + "HTTP/1.1 200 OK\r\ncontent-type: application/json\r\ncontent-length: {}\r\n\ + connection: close\r\n\r\n{body}", + body.len() + ) + } else { + "HTTP/1.1 500 Internal Server Error\r\ncontent-length: 0\r\nconnection: close\r\n\r\n" + .to_string() + }; + let _ = stream.write_all(reply.as_bytes()).await; + }); + } + }); + stub +} + +impl Jwks { + fn serve(&self, kids: &[&str]) { + *self.kids.lock().unwrap() = kids.iter().map(|k| k.to_string()).collect(); + } + fn fetches(&self) -> usize { + self.fetches.load(Ordering::SeqCst) + } +} + +fn ec_pair() -> &'static (Vec, Vec) { + use aws_lc_rs::rand::SystemRandom; + use aws_lc_rs::signature::{EcdsaKeyPair, KeyPair, ECDSA_P256_SHA256_FIXED_SIGNING}; + static PAIR: std::sync::OnceLock<(Vec, Vec)> = std::sync::OnceLock::new(); + PAIR.get_or_init(|| { + let pkcs8 = + EcdsaKeyPair::generate_pkcs8(&ECDSA_P256_SHA256_FIXED_SIGNING, &SystemRandom::new()) + .unwrap(); + let pair = + EcdsaKeyPair::from_pkcs8(&ECDSA_P256_SHA256_FIXED_SIGNING, pkcs8.as_ref()).unwrap(); + (pkcs8.as_ref().to_vec(), pair.public_key().as_ref().to_vec()) + }) +} + +fn config() -> Config { + Config::from_env(|name| match name { + "ASTER_SRV_DATABASE_URL" => Some("postgres://localhost/x".into()), + "ASTER_SRV_REPOSITORIES" => Some("ArchAstro/*".into()), + "ASTER_SRV_IAP_AUDIENCE" => Some(IAP_AUDIENCE.into()), + _ => None, + }) + .unwrap() +} + +fn oidc(kid: &str) -> String { + let mut header = Header::new(Algorithm::RS256); + header.kid = Some(kid.into()); + let token = encode( + &header, + &json!({ + "iss": aster_srv::config::GITHUB_ISSUER, "aud": "aster-srv", + "exp": chrono::Utc::now().timestamp() + 300, + "repository": "ArchAstro/firstlanding", + }), + &EncodingKey::from_rsa_pem(PRIVATE).unwrap(), + ) + .unwrap(); + format!("Bearer {token}") +} + +fn iap(kid: &str) -> String { + let mut header = Header::new(Algorithm::ES256); + header.kid = Some(kid.into()); + encode( + &header, + &json!({ + "iss": "https://cloud.google.com/iap", "aud": IAP_AUDIENCE, + "iat": chrono::Utc::now().timestamp(), + "exp": chrono::Utc::now().timestamp() + 300, + "email": "ada@example.com", "sub": "accounts.google.com:1", + }), + &EncodingKey::from_ec_der(&ec_pair().0), + ) + .unwrap() +} + +async fn pause() { + tokio::time::sleep(POLICY.min_interval + Duration::from_millis(100)).await; +} + +/// N9b: a failed fetch used to stamp the refetch time before the request, so +/// the next real rotation was refused for a full minute. Now it waits only +/// the short interval. +#[tokio::test] +async fn a_failed_key_fetch_does_not_block_the_next_real_rotation() { + let stub = jwks("rsa").await; + stub.healthy.store(false, Ordering::SeqCst); + let verifier = Verifier::new(&config()).with_jwks(&stub.url, POLICY); + // The issuer is down: the first token fails, and costs one fetch. + assert!(verifier.verify(Some(&oidc("rotated"))).await.is_err()); + assert_eq!(stub.fetches(), 1); + // Retries inside the short interval do not fetch again. + assert!(verifier.verify(Some(&oidc("rotated"))).await.is_err()); + assert_eq!(stub.fetches(), 1); + // The issuer recovers and has the rotated key: the next token after the + // short interval succeeds, not a minute later. + stub.healthy.store(true, Ordering::SeqCst); + stub.serve(&["rotated"]); + pause().await; + let identity = verifier.verify(Some(&oidc("rotated"))).await.unwrap(); + assert_eq!(identity.repository, "ArchAstro/firstlanding"); + assert_eq!(stub.fetches(), 2); +} + +/// N9b: a flood of unknown keys costs the issuer at most one fetch per +/// interval, the same unknown key is not looked for again for a minute, and +/// a real rotation behind the flood is still picked up after the short +/// interval. +#[tokio::test] +async fn an_unknown_key_flood_does_not_cause_a_refetch_storm_or_hide_a_rotation() { + let stub = jwks("rsa").await; + stub.serve(&["current"]); + let verifier = Arc::new(Verifier::new(&config()).with_jwks(&stub.url, POLICY)); + assert!(verifier.verify(Some(&oidc("current"))).await.is_ok()); + assert_eq!(stub.fetches(), 1); + pause().await; + // 200 distinct random keys at once (signed first, so the flood is + // faster than the refetch interval). + let probes: Vec = (0..200).map(|n| oidc(&format!("probe-{n}"))).collect(); + let mut flood = tokio::task::JoinSet::new(); + for probe in probes { + let verifier = Arc::clone(&verifier); + flood.spawn(async move { verifier.verify(Some(&probe)).await }); + } + while let Some(result) = flood.join_next().await { + assert!(result.unwrap().is_err()); + } + assert_eq!(stub.fetches(), 2, "200 probes cost one fetch"); + // One probe after the interval costs one fetch and is remembered as + // absent: the same key again, even after the interval, costs none. + pause().await; + assert!(verifier.verify(Some(&oidc("lone-probe"))).await.is_err()); + assert_eq!(stub.fetches(), 3); + pause().await; + assert!(verifier.verify(Some(&oidc("lone-probe"))).await.is_err()); + assert_eq!(stub.fetches(), 3, "a known-absent key is not fetched for"); + // A real rotation, still inside the minute the probes were remembered. + stub.serve(&["current", "rotated"]); + let identity = verifier.verify(Some(&oidc("rotated"))).await.unwrap(); + assert_eq!(identity.repository, "ArchAstro/firstlanding"); + assert_eq!(stub.fetches(), 4); +} + +/// N9b, IAP: the same rules on the dashboard's verifier. +#[tokio::test] +async fn the_iap_verifier_follows_the_same_refetch_rules() { + let stub = jwks("ec").await; + stub.healthy.store(false, Ordering::SeqCst); + let verifier = IapVerifier::new(&config()).with_jwks(&stub.url, POLICY); + assert!(verifier.verify(Some(&iap("rotated"))).await.is_err()); + assert!(verifier.verify(Some(&iap("rotated"))).await.is_err()); + assert_eq!(stub.fetches(), 1); + stub.healthy.store(true, Ordering::SeqCst); + stub.serve(&["rotated"]); + pause().await; + let viewer = verifier.verify(Some(&iap("rotated"))).await.unwrap(); + assert_eq!(viewer.email, "ada@example.com"); + assert_eq!(stub.fetches(), 2); + // A probe is remembered, a rotation behind it is not blocked. + pause().await; + assert!(verifier.verify(Some(&iap("probe"))).await.is_err()); + assert!(verifier.verify(Some(&iap("probe"))).await.is_err()); + assert_eq!(stub.fetches(), 3); + stub.serve(&["rotated", "next"]); + pause().await; + assert!(verifier.verify(Some(&iap("next"))).await.is_ok()); + assert_eq!(stub.fetches(), 4); +} diff --git a/crates/aster-srv/tests/lookup_fanout.rs b/crates/aster-srv/tests/lookup_fanout.rs new file mode 100644 index 0000000..cf31b02 --- /dev/null +++ b/crates/aster-srv/tests/lookup_fanout.rs @@ -0,0 +1,347 @@ +//! N5: a lookup's GCS and IAM work is bounded. +//! +//! Real Postgres and the real routes, an in-memory GCS with injected HEAD +//! latency and a signer with injected `signBlob` latency. Set +//! `ASTER_SRV_TEST_DATABASE_URL`; without it the tests skip. + +mod common; + +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::Arc; +use std::time::{Duration, Instant}; + +use aster_srv::cache::{BlobStore, FanoutConfig, SignLimits, Signer, MAX_LOOKUP_BLOBS}; +use serde_json::{json, Value}; + +const REPO: &str = "ArchAstro/firstlanding"; + +/// A signer that takes `delay` per call and counts calls. +struct SlowSigner { + delay: Duration, + calls: AtomicUsize, +} + +impl Signer for SlowSigner { + fn sign<'a>( + &'a self, + _payload: &'a [u8], + ) -> std::pin::Pin>> + Send + 'a>> + { + Box::pin(async move { + self.calls.fetch_add(1, Ordering::SeqCst); + tokio::time::sleep(self.delay).await; + Ok(b"sig".to_vec()) + }) + } + + fn service_account_email(&self) -> &str { + "cache-test@example.iam.gserviceaccount.com" + } +} + +fn digest(index: usize) -> String { + format!("{index:064x}") +} + +async fn server( + config: FanoutConfig, + head_latency: Duration, + sign_delay: Duration, +) -> Option<(common::Server, Arc)> { + let signer = Arc::new(SlowSigner { + delay: sign_delay, + calls: AtomicUsize::new(0), + }); + let store = BlobStore::memory_with( + "aster-cache-test", + signer.clone() as Arc, + config, + head_latency, + ); + let started = common::start_with( + &[ + ("ASTER_SRV_CACHE_BUCKET", "aster-cache-test"), + ( + "ASTER_SRV_CACHE_SIGNER", + "cache-test@example.iam.gserviceaccount.com", + ), + ("ASTER_SRV_CACHE_DAYS", "14"), + ], + Some(store), + ) + .await?; + Some((started, signer)) +} + +/// Entry `key` naming `blobs` (one per output), stored as if CI had committed it. +async fn seed(server: &common::Server, key: &str, blobs: &[String]) { + let outputs: Vec = blobs + .iter() + .enumerate() + .map(|(index, digest)| { + json!({"path": format!("out/{index}"), "kind": "file", "digest": digest, "mode": 420}) + }) + .collect(); + let manifest = serde_json::to_vec(&json!({ + "schema": 1, "key": key, "target": "//a:b", "outputs": outputs, + })) + .unwrap(); + let client = server.client().await; + client + .execute( + "INSERT INTO cache.manifests (repo, key, manifest, size_bytes, target) \ + VALUES ($1, $2, $3, $4, '//a:b')", + &[&REPO, &key, &manifest, &(manifest.len() as i64)], + ) + .await + .unwrap(); +} + +async fn put(server: &common::Server, blobs: &[String]) { + for digest in blobs { + server + .app + .cache_store + .put(&server.app.pool, digest, bytes::Bytes::from_static(b"b")) + .await + .unwrap(); + } +} + +fn main_push() -> String { + format!( + "Bearer {}", + common::token_with_ref(REPO, Some("refs/heads/main")) + ) +} + +async fn lookup(server: &common::Server, keys: &[String]) -> (u16, Value, Duration) { + let started = Instant::now(); + let (status, body) = server + .post_json( + "/v1/cache/lookup", + &json!({"repo": REPO, "keys": keys}), + Some(&main_push()), + None, + ) + .await; + (status, body, started.elapsed()) +} + +/// The latency story behind the observed remote-cache read misses: 64 keys of +/// 8 blobs each (512 blobs), 20ms per HEAD and 10ms per signBlob. One at a +/// time that is 512 * 30ms = 15.4s (17.8s measured before the fix), past the 0.22 client's 5s timeout. With +/// bounded concurrency a cold lookup takes about a second, and a repeat costs +/// no GCS or IAM call at all. +#[tokio::test] +async fn a_wide_lookup_finishes_in_bounded_time_and_a_repeat_is_free() { + let Some((server, signer)) = server( + FanoutConfig::default(), + Duration::from_millis(20), + Duration::from_millis(10), + ) + .await + else { + return; + }; + // Phase 1: 64 entries, 8 distinct blobs each, all present in GCS. + let keys: Vec = (0..64).map(|index| format!("k{index}")).collect(); + for (index, key) in keys.iter().enumerate() { + let blobs: Vec = (0..8).map(|blob| digest(index * 8 + blob)).collect(); + put(&server, &blobs).await; + seed(&server, key, &blobs).await; + } + + // Phase 2: a cold lookup resolves all 512 blobs concurrently. + let (status, body, cold) = lookup(&server, &keys).await; + assert_eq!(status, 200, "{body}"); + assert_eq!(body["hits"].as_object().unwrap().len(), 64); + assert!(body["hits"]["k63"]["blobs"][digest(511)] + .as_str() + .unwrap() + .starts_with("https://storage.googleapis.com/aster-cache-test/cas/")); + assert_eq!(server.app.cache_store.head_calls(), 512); + assert_eq!(signer.calls.load(Ordering::SeqCst), 512); + + // Phase 3: the same lookup again is served from memory. + let (status, again, warm) = lookup(&server, &keys).await; + assert_eq!(status, 200); + assert_eq!(again["hits"], body["hits"]); + assert_eq!(server.app.cache_store.head_calls(), 512); + assert_eq!(signer.calls.load(Ordering::SeqCst), 512); + + eprintln!( + "lookup_fanout: 64 keys x 8 blobs, HEAD 20ms, sign 10ms: cold {cold:?}, warm {warm:?} (before the fix, measured: 17.8s cold, 17.9s warm)" + ); + assert!(cold < Duration::from_secs(4), "cold lookup took {cold:?}"); + assert!( + warm < Duration::from_millis(500), + "warm lookup took {warm:?}" + ); + server.stop().await; +} + +/// Entries past the per-response blob cap are answered as misses, in request +/// order, and only the entries served are counted. +#[tokio::test] +async fn entries_over_the_blob_cap_are_misses_and_are_not_counted() { + // Only the cap is under test: lift the signing rate and the time budget, + // which would otherwise cut 4000 cold signatures short first. + let config = FanoutConfig { + lookup_budget: Duration::from_secs(60), + sign: SignLimits { + per_second: 1e9, + burst: 1e9, + ..SignLimits::default() + }, + ..FanoutConfig::default() + }; + let Some((server, _)) = server(config, Duration::ZERO, Duration::ZERO).await else { + return; + }; + // Five entries of 1000 distinct blobs: the first four (4000) fit in 4096. + let per_entry = MAX_LOOKUP_BLOBS / 4 - 24; + let keys: Vec = (0..5).map(|index| format!("big{index}")).collect(); + for (index, key) in keys.iter().enumerate() { + let blobs: Vec = (0..per_entry).map(|n| digest(index * 10_000 + n)).collect(); + for digest in &blobs { + let bytes = bytes::Bytes::from_static(b"b"); + server + .app + .cache_store + .put_unrecorded(digest, bytes) + .await + .unwrap(); + } + seed(&server, key, &blobs).await; + } + let (status, body, _) = lookup(&server, &keys).await; + assert_eq!(status, 200); + let hits = body["hits"].as_object().unwrap(); + assert_eq!(hits.len(), 4, "{:?}", hits.keys().collect::>()); + assert!(hits.get("big4").is_none()); + let counted: Vec<(String, i64)> = server + .client() + .await + .query("SELECT key, hits FROM cache.manifests ORDER BY key", &[]) + .await + .unwrap() + .iter() + .map(|row| (row.get(0), row.get(1))) + .collect(); + assert_eq!(counted.last().unwrap(), &("big4".to_string(), 0)); + assert!(counted[..4].iter().all(|(_, hits)| *hits == 1)); + server.stop().await; +} + +/// When IAM signing is saturated, entries that cannot be signed become +/// misses, the lookup still answers 200, and nothing hammers IAM. +#[tokio::test] +async fn a_saturated_signer_turns_entries_into_misses_not_errors() { + let config = FanoutConfig { + sign: SignLimits { + concurrency: 4, + per_second: 0.01, + burst: 3.0, + max_wait: Duration::from_millis(20), + }, + ..FanoutConfig::default() + }; + let Some((server, signer)) = server(config, Duration::ZERO, Duration::ZERO).await else { + return; + }; + let keys: Vec = (0..6).map(|index| format!("s{index}")).collect(); + for (index, key) in keys.iter().enumerate() { + let blobs = vec![digest(index)]; + put(&server, &blobs).await; + seed(&server, key, &blobs).await; + } + let (status, body, _) = lookup(&server, &keys).await; + assert_eq!(status, 200, "{body}"); + assert_eq!(body["hits"].as_object().unwrap().len(), 3, "{body}"); + assert_eq!(signer.calls.load(Ordering::SeqCst), 3); + // Only the three served entries were counted. + let counted: i64 = server + .client() + .await + .query_one("SELECT sum(hits)::bigint FROM cache.manifests", &[]) + .await + .unwrap() + .get(0); + assert_eq!(counted, 3); + server.stop().await; +} + +/// A blob that disappears from GCS is noticed once the remembered answer +/// expires, and a lookup then leaves it out. +#[tokio::test] +async fn a_blob_deleted_from_gcs_drops_out_after_the_remembered_answer_expires() { + let config = FanoutConfig { + present_ttl: Duration::from_millis(50), + ..FanoutConfig::default() + }; + let Some((server, _)) = server(config, Duration::ZERO, Duration::ZERO).await else { + return; + }; + let blobs = vec![digest(1), digest(2)]; + put(&server, &blobs).await; + seed(&server, "k", &blobs).await; + let keys = vec!["k".to_string()]; + let (_, first, _) = lookup(&server, &keys).await; + assert_eq!(first["hits"]["k"]["blobs"].as_object().unwrap().len(), 2); + server.app.cache_store.delete(&blobs[1]).await.unwrap(); + // delete forgets the digest at once; a stale positive would need the TTL. + let (_, second, _) = lookup(&server, &keys).await; + assert_eq!(second["hits"]["k"]["blobs"].as_object().unwrap().len(), 1); + server.stop().await; +} + +/// A lookup that cannot resolve every blob within its time budget answers on +/// time with the entries it did resolve; the rest are misses and are not +/// counted. Before the budget, a slow GCS made the whole batch outlive the +/// client's 5s timeout and every hit in it was lost. A repeat finds the +/// resolved blobs remembered and serves more. +#[tokio::test] +async fn a_slow_lookup_answers_within_its_budget_and_the_rest_are_misses() { + // 128 one-blob entries at 200ms per HEAD, 16 at a time: 1.6s of work + // against a 300ms budget. + let config = FanoutConfig { + lookup_budget: Duration::from_millis(300), + ..FanoutConfig::default() + }; + let Some((server, _)) = server(config, Duration::from_millis(200), Duration::ZERO).await else { + return; + }; + let keys: Vec = (0..128).map(|index| format!("slow{index}")).collect(); + for (index, key) in keys.iter().enumerate() { + let blobs = vec![digest(index)]; + put(&server, &blobs).await; + seed(&server, key, &blobs).await; + } + + // The cold lookup stops at the budget: some entries hit, the rest miss. + let (status, body, cold) = lookup(&server, &keys).await; + assert_eq!(status, 200, "{body}"); + let first = body["hits"].as_object().unwrap().len(); + eprintln!("lookup_fanout: 128 keys, HEAD 200ms, budget 300ms: {first} hits in {cold:?}"); + assert!(cold < Duration::from_secs(1), "cold lookup took {cold:?}"); + assert!(first > 0 && first < keys.len(), "{first} hits"); + for hit in body["hits"].as_object().unwrap().values() { + assert_eq!(hit["blobs"].as_object().unwrap().len(), 1, "{hit}"); + } + // Only the entries served were counted. + let counted: i64 = server + .client() + .await + .query_one("SELECT sum(hits)::bigint FROM cache.manifests", &[]) + .await + .unwrap() + .get(0); + assert_eq!(counted, first as i64); + + // The repeat reuses what the first lookup resolved and gets further. + let (status, again, _) = lookup(&server, &keys).await; + assert_eq!(status, 200); + assert!(again["hits"].as_object().unwrap().len() > first, "{again}"); + server.stop().await; +} diff --git a/crates/aster-srv/tests/operator_revoke.rs b/crates/aster-srv/tests/operator_revoke.rs new file mode 100644 index 0000000..3bc142c --- /dev/null +++ b/crates/aster-srv/tests/operator_revoke.rs @@ -0,0 +1,123 @@ +//! N8: the operator runbook for revoking a departed person's tokens. The SQL +//! is read from the README between the `revoke-sql` markers, so the test +//! runs what an operator would paste into `psql`, against real Postgres. +//! +//! Set `ASTER_SRV_TEST_DATABASE_URL`; without it the test skips. + +mod common; + +use std::sync::Arc; + +use aster_srv::cache::{BlobStore, FixedSigner, Signer}; +use common::{iap_subject, person_bearer, Server, IAP_AUDIENCE}; +use serde_json::json; + +const REPO: &str = "ArchAstro/firstlanding"; +const README: &str = include_str!("../README.md"); + +/// The documented statement with psql's `:'subject'` bound as `$1`, and the +/// example subject its `\set` line gives. psql sends `:'subject'` as a quoted +/// text literal, which is what a bound text parameter is. +fn documented_revoke_sql() -> (String, String) { + let block = README + .split("") + .nth(1) + .and_then(|rest| rest.split("").next()) + .expect("the README has the revoke-sql markers"); + let body = block + .trim() + .strip_prefix("```sql") + .and_then(|rest| rest.strip_suffix("```")) + .expect("the snippet is one sql code block") + .trim(); + let (set, statement) = body.split_once('\n').expect("a \\set line, then SQL"); + let example = set + .strip_prefix("\\set subject '") + .and_then(|rest| rest.strip_suffix('\'')) + .expect("the first line sets the subject") + .to_string(); + assert_eq!(statement.matches(":'subject'").count(), 1, "{statement}"); + assert!(!statement.contains('$'), "{statement}"); + (statement.replace(":'subject'", "$1"), example) +} + +async fn start() -> Option { + let signer = Arc::new(FixedSigner { + email: "cache-test@example.iam.gserviceaccount.com".into(), + signature: b"test-signature".to_vec(), + signed: Default::default(), + }); + let store = BlobStore::memory("aster-cache-test", signer as Arc); + common::start_with( + &[ + ("ASTER_SRV_CACHE_BUCKET", "aster-cache-test"), + ( + "ASTER_SRV_CACHE_SIGNER", + "cache-test@example.iam.gserviceaccount.com", + ), + ("ASTER_SRV_IAP_AUDIENCE", IAP_AUDIENCE), + ("ASTER_SRV_UI_EMAIL_DOMAINS", "example.com"), + ], + Some(store), + ) + .await +} + +#[tokio::test] +async fn the_documented_revoke_sql_logs_out_a_departed_person_and_nobody_else() { + let (sql, example) = documented_revoke_sql(); + assert!(example.starts_with("accounts.google.com:"), "{example}"); + let Some(server) = start().await else { + return; + }; + let lookup = |bearer: String| { + let server = &server; + async move { + server + .post_json( + "/v1/cache/lookup", + &json!({"repo": REPO, "keys": ["k"]}), + Some(&bearer), + None, + ) + .await + .0 + } + }; + + // Two people signed in; Mallory holds tokens on two laptops. + let departed = "mallory@example.com"; + let laptop = person_bearer(&server, departed); + let desktop = person_bearer(&server, departed); + let colleague = person_bearer(&server, "ada@example.com"); + for bearer in [&laptop, &desktop, &colleague] { + assert_eq!(lookup(bearer.clone()).await, 200); + } + + // The operator runs the README's SQL for Mallory's stable subject. + let subject = iap_subject(departed); + let client = server.client().await; + assert_eq!(client.execute(sql.as_str(), &[&subject]).await.unwrap(), 1); + + // Mallory's tokens are refused at once; Ada's still works. + assert_eq!(lookup(laptop.clone()).await, 401); + assert_eq!(lookup(desktop).await, 401); + assert_eq!(lookup(colleague.clone()).await, 200); + + // Running it again (the person was already revoked, by logout or an + // earlier run) updates the row instead of failing. + assert_eq!(client.execute(sql.as_str(), &[&subject]).await.unwrap(), 1); + let rows: i64 = client + .query_one( + "SELECT count(*) FROM auth.revocations WHERE subject = $1", + &[&subject], + ) + .await + .unwrap() + .get(0); + assert_eq!(rows, 1); + assert_eq!(lookup(laptop).await, 401); + assert_eq!(lookup(colleague).await, 200); + drop(client); + server.stop().await; +} diff --git a/crates/aster-srv/tests/quarantine.rs b/crates/aster-srv/tests/quarantine.rs index f3adbfe..ca1b9d0 100644 --- a/crates/aster-srv/tests/quarantine.rs +++ b/crates/aster-srv/tests/quarantine.rs @@ -26,7 +26,7 @@ async fn classify(server: &Server, now: DateTime) -> classifier::Report { /// A run in which `test` failed and then passed on its retry: one definite /// flake event. fn retried<'a>(run: &'a str, test: &'a str, at: DateTime) -> Run<'a> { - Run::new(run, "pr-1", at) + Run::new(run, "main", at) .attempt(test, "fail", 1, Some("f1")) .attempt(test, "pass", 2, None) } @@ -77,12 +77,12 @@ async fn definite_flakes_in_two_runs_quarantine_a_test() { // Two definite events, but from one run. seed( &server, - &Run::new("r3", "pr-2", ago(now, 45)).result("once", "pass", None), + &Run::new("r3", "main", ago(now, 45)).result("once", "pass", None), ) .await; seed( &server, - &Run::new("r4", "pr-2", ago(now, 40)) + &Run::new("r4", "main", ago(now, 40)) .attempt("once", "fail", 1, Some("f1")) .attempt("once", "pass", 2, None), ) @@ -230,7 +230,7 @@ async fn consecutive_passes_release_and_a_release_starts_afresh() { let failure = (outcome == "fail").then_some("f9"); seed( &server, - &Run::new(run, "pr-9", ago(now, minutes)) + &Run::new(run, "main", ago(now, minutes)) .hash(run) .result("t", outcome, failure), ) @@ -241,7 +241,7 @@ async fn consecutive_passes_release_and_a_release_starts_afresh() { seed( &server, - &Run::new("p6", "pr-9", ago(now, 40)) + &Run::new("p6", "main", ago(now, 40)) .hash("p6") .result("t", "pass", None), ) @@ -281,7 +281,7 @@ async fn replayed_passes_from_cached_targets_do_not_release() { assert_eq!(quarantined(&feed(&server).await), vec!["t"]); for (run, minutes) in [("c1", 70), ("c2", 65)] { - let mut replayed = Run::new(run, "pr-9", ago(now, minutes)).result("t", "pass", None); + let mut replayed = Run::new(run, "main", ago(now, minutes)).result("t", "pass", None); replayed.cached = true; seed(&server, &replayed).await; } @@ -291,7 +291,7 @@ async fn replayed_passes_from_cached_targets_do_not_release() { for (run, minutes) in [("p1", 55), ("p2", 50)] { seed( &server, - &Run::new(run, "pr-9", ago(now, minutes)).result("t", "pass", None), + &Run::new(run, "main", ago(now, minutes)).result("t", "pass", None), ) .await; } @@ -569,7 +569,7 @@ async fn a_run_flaking_more_tests_than_the_cap_quarantines_none_of_them() { // (three failures of ten tests), so each is a definite flake. let tests = ["a", "b", "c"]; let run = |id: &'static str, at| { - let mut seeded = Run::new(id, "pr-1", at); + let mut seeded = Run::new(id, "main", at); for test in tests { seeded = seeded .attempt(test, "fail", 1, Some("f1")) @@ -606,3 +606,130 @@ async fn a_run_flaking_more_tests_than_the_cap_quarantines_none_of_them() { server.stop().await; } } + +/// Flaky score and quarantine state of `test`. +async fn score_and_quarantine(server: &Server, test: &str) -> (i32, bool) { + let score = server + .client() + .await + .query_one( + "SELECT flake_score FROM test_results.tests WHERE test_id = $1", + &[&test], + ) + .await + .unwrap() + .get(0); + let held = quarantined(&feed(server).await).contains(&test.to_string()); + (score, held) +} + +#[tokio::test] +async fn pull_request_evidence_scores_flakes_but_never_quarantines() { + // Two retried flakes per test, each from a different kind of job. A + // pull request job runs its author's code: its token proves where the + // results came from, not that they are true. + // (test, how its job uploads, whether its flakes may quarantine it) + type Job = (&'static str, fn(Run<'static>) -> Run<'static>, bool); + let jobs: [Job; 5] = [ + ("pushed", |r| r.oidc("refs/heads/main", "push"), true), + ( + "dispatched", + |r| r.oidc("refs/heads/main", "workflow_dispatch"), + true, + ), + // Its record says main; its ref says pull request 7. + ("pr", |r| r.oidc("refs/pull/7/merge", "pull_request"), false), + // The default branch's ref, but run for a pull request. + ( + "pr-target", + |r| r.oidc("refs/heads/main", "pull_request_target"), + false, + ), + // A static token's runs go by branch. + ( + "static-branch", + |r| Run { + branch: "feature/x", + ..r + }, + false, + ), + ]; + let Some(server) = start(&[]).await else { + return; + }; + let now = Utc::now(); + // Run ids live as long as the runs that borrow them. + let leak = |id: String| -> &'static str { Box::leak(id.into_boxed_str()) }; + for (test, job, _) in &jobs { + let (first, second) = (leak(format!("{test}-1")), leak(format!("{test}-2"))); + seed(&server, &job(retried(first, test, ago(now, 20)))).await; + seed(&server, &job(retried(second, test, ago(now, 10)))).await; + } + classify(&server, now).await; + for (test, _, held) in jobs { + assert_eq!( + score_and_quarantine(&server, test).await, + (6, held), + "{test}" + ); + } + server.stop().await; +} + +#[tokio::test] +async fn a_forged_pass_cannot_make_a_real_main_failure_quarantinable() { + let Some(server) = start(&[]).await else { + return; + }; + let now = Utc::now(); + let main = |id: &'static str, at, outcome: &'static str| { + Run::new(id, "main", at) + .oidc("refs/heads/main", "push") + .result("real", outcome, (outcome == "fail").then_some("f1")) + }; + let pr = |id: &'static str, at| { + Run::new(id, "main", at) + .oidc("refs/pull/7/merge", "pull_request") + .result("real", "pass", None) + }; + // The test really fails on main twice. A pull request job uploads a + // pass on the same inputs after each failure: both failures now look + // like flakes, but only on the forger's word. + seed(&server, &main("m1", ago(now, 60), "fail")).await; + seed(&server, &pr("p1", ago(now, 55))).await; + seed(&server, &main("m2", ago(now, 50), "fail")).await; + seed(&server, &pr("p2", ago(now, 45))).await; + classify(&server, ago(now, 40)).await; + assert_eq!(score_and_quarantine(&server, "real").await, (6, false)); + + // A main run passes on those inputs: m2's failure is now a flake on + // main's own word. One trusted failing run is not two. + seed(&server, &main("t1", ago(now, 30), "pass")).await; + classify(&server, ago(now, 25)).await; + assert!(!score_and_quarantine(&server, "real").await.1); + let trusted: Vec<(String, bool)> = server + .client() + .await + .query( + "SELECT run_id, trusted FROM test_results.test_flake_events \ + WHERE kind = 'definite' ORDER BY run_id", + &[], + ) + .await + .unwrap() + .iter() + .map(|r| (r.get(0), r.get(1))) + .collect(); + assert_eq!( + trusted, + vec![("m1".to_string(), false), ("m2".to_string(), true)] + ); + + // A second failure and pass on main: two trusted failing runs. + seed(&server, &main("m3", ago(now, 20), "fail")).await; + seed(&server, &main("t2", ago(now, 10), "pass")).await; + classify(&server, now).await; + assert!(score_and_quarantine(&server, "real").await.1); + server.stop().await; +} diff --git a/crates/aster-srv/tests/query.rs b/crates/aster-srv/tests/query.rs index 1a42002..2c97d23 100644 --- a/crates/aster-srv/tests/query.rs +++ b/crates/aster-srv/tests/query.rs @@ -399,22 +399,23 @@ async fn reports_a_runs_failures_with_their_state() { // Errors. assert_eq!(server.get(&path, None).await.0, 401); - assert_eq!( - server - .get("/v1/test-results/runs/nope/failures", Some(REPO)) - .await - .0, - 404 - ); + let missing = server + .get("/v1/test-results/runs/nope/failures", Some(REPO)) + .await; + assert_eq!(missing.0, 404); let mut foreign = Run::new("o1", "main", ago(now, 5)).result("t", "fail", Some("x")); foreign.repo = OTHER; seed(&server, &foreign).await; - assert_eq!( - server - .get("/v1/test-results/runs/o1/failures", Some(REPO)) - .await - .0, - 403 + // N10: another repository's run answers exactly what a missing run does, + // so the id leaks neither its existence nor its owner. + let foreign_reply = server + .get("/v1/test-results/runs/o1/failures", Some(REPO)) + .await; + assert_eq!(foreign_reply, missing); + assert!( + !foreign_reply.1.to_string().contains(OTHER), + "{}", + foreign_reply.1 ); assert_eq!( server diff --git a/crates/aster-srv/tests/routes.rs b/crates/aster-srv/tests/routes.rs index 7d7f273..2359a0c 100644 --- a/crates/aster-srv/tests/routes.rs +++ b/crates/aster-srv/tests/routes.rs @@ -23,33 +23,35 @@ const PRIVATE_KEY: &[u8] = include_bytes!("fixtures/test-oidc-key.pem"); const DIGEST: &str = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"; async fn start() -> Option { + start_with(&[]).await +} + +async fn start_with(extra: &[(&str, &str)]) -> Option { let signer = Arc::new(FixedSigner { email: "cache-test@example.iam.gserviceaccount.com".into(), signature: b"test-signature".to_vec(), signed: Default::default(), }); let store = BlobStore::memory("aster-cache-test", signer as Arc); - common::start_with( - &[ - ("ASTER_SRV_CACHE_BUCKET", "aster-cache-test"), - ( - "ASTER_SRV_CACHE_SIGNER", - "cache-test@example.iam.gserviceaccount.com", - ), - ( - "ASTER_SRV_IAP_AUDIENCE", - "/projects/123456/global/backendServices/4242", - ), - ("ASTER_SRV_UI_EMAIL_DOMAINS", "example.com"), - ( - "ASTER_SRV_CACHE_WRITER_WORKFLOWS", - "ArchAstro/firstlanding/.github/workflows/ci.yml@", - ), - ("ASTER_SRV_REPOSITORY_OWNER_IDS", "ArchAstro/*=4242"), - ], - Some(store), - ) - .await + let mut env = vec![ + ("ASTER_SRV_CACHE_BUCKET", "aster-cache-test"), + ( + "ASTER_SRV_CACHE_SIGNER", + "cache-test@example.iam.gserviceaccount.com", + ), + ( + "ASTER_SRV_IAP_AUDIENCE", + "/projects/123456/global/backendServices/4242", + ), + ("ASTER_SRV_UI_EMAIL_DOMAINS", "example.com"), + ( + "ASTER_SRV_CACHE_WRITER_WORKFLOWS", + "ArchAstro/firstlanding/.github/workflows/ci.yml@", + ), + ("ASTER_SRV_REPOSITORY_OWNER_IDS", "ArchAstro/*=4242"), + ]; + env.extend_from_slice(extra); + common::start_with(&env, Some(store)).await } /// A route's pattern with each `:name` filled in. @@ -740,3 +742,206 @@ async fn the_signed_blob_stand_in_exists_only_when_the_dev_flag_is_set() { } server.stop().await; } + +/// POST `path` with `token`, declare `declared` body bytes, send only `{`, +/// and report what the server answers within `wait`: `Some(status line)` if +/// it replied while the body was still unsent, `None` if it kept waiting. +/// The stream is returned so the caller can keep the body open. +async fn post_with_unsent_body( + server: &Server, + path: &str, + token: &str, + wait: std::time::Duration, +) -> (Option, tokio::net::TcpStream) { + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + let mut stream = tokio::net::TcpStream::connect(server.url.trim_start_matches("http://")) + .await + .unwrap(); + stream + .write_all( + format!( + "POST {path} HTTP/1.1\r\nhost: x\r\nauthorization: Bearer {token}\r\n\ + content-type: application/json\r\ncontent-length: 1000\r\n\r\n{{" + ) + .as_bytes(), + ) + .await + .unwrap(); + let mut buffer = [0u8; 256]; + let reply = match tokio::time::timeout(wait, stream.read(&mut buffer)).await { + Ok(Ok(n)) if n > 0 => Some( + String::from_utf8_lossy(&buffer[..n]) + .lines() + .next() + .unwrap_or("") + .to_string(), + ), + _ => None, + }; + (reply, stream) +} + +fn ci_token(change: Value) -> String { + let mut claims = json!({ + "repository_owner_id": "4242", + "job_workflow_ref": CI_WORKFLOW, + "ref": "refs/heads/main", + "event_name": "push", + }); + for (name, value) in change.as_object().unwrap() { + claims[name] = value.clone(); + } + token_with_claims(REPO, claims) +} + +/// N1: the gate holds a CI token to the cache writer rule (ref, push event, +/// workflow prefix) before any body is read. The body is declared and never +/// sent, so a server that read it first would not answer within the wait. +#[tokio::test] +async fn a_non_writer_ci_token_is_refused_before_its_body_is_read() { + let Some(server) = start().await else { + return; + }; + let wait = std::time::Duration::from_secs(5); + let refused = [ + ( + "a pull-request ref", + ci_token(json!({"ref": "refs/pull/7/merge"})), + ), + ( + "a pull_request event on the default branch", + ci_token(json!({"event_name": "pull_request"})), + ), + ( + "another workflow", + ci_token( + json!({"job_workflow_ref": "ArchAstro/firstlanding/.github/workflows/evil.yml@refs/heads/main"}), + ), + ), + ("no ref claim", ci_token(json!({"ref": null}))), + ]; + for path in ["/v1/cache/commit", "/v1/cache/upload"] { + for (what, token) in &refused { + let (reply, _stream) = post_with_unsent_body(&server, path, token, wait).await; + let reply = reply.unwrap_or_else(|| panic!("{path} with {what}: waited for the body")); + assert!( + reply.starts_with("HTTP/1.1 403"), + "{path} with {what}: {reply}" + ); + } + // A token for a repository off the allowlist, or with the wrong + // owner id, is refused at verification, still before the body. + for (what, token) in [ + ( + "an unlisted repository", + token_with_claims("evil/fork", json!({"repository_owner_id": "4242"})), + ), + ( + "a wrong owner id", + ci_token(json!({"repository_owner_id": "1"})), + ), + ] { + let (reply, _stream) = post_with_unsent_body(&server, path, &token, wait).await; + let reply = reply.unwrap_or_else(|| panic!("{path} with {what}: waited for the body")); + assert!( + reply.starts_with("HTTP/1.1 403"), + "{path} with {what}: {reply}" + ); + } + // A real writer is let through to read its body: no early reply. + let (reply, _stream) = post_with_unsent_body( + &server, + path, + &ci_token(json!({})), + std::time::Duration::from_millis(700), + ) + .await; + assert_eq!(reply, None, "{path}: a writer's body is awaited"); + } + // Test-results upload deliberately accepts pull requests (accepted + // risk): a pull_request token is read, not refused early. Only + // pull_request_target and a foreign workflow are. + let short = std::time::Duration::from_millis(700); + let (reply, _stream) = post_with_unsent_body( + &server, + "/v1/test-results/runs", + &ci_token(json!({"ref": "refs/pull/7/merge", "event_name": "pull_request"})), + short, + ) + .await; + assert_eq!(reply, None, "a pull_request upload's body is awaited"); + let (reply, _stream) = post_with_unsent_body( + &server, + "/v1/test-results/runs", + &ci_token(json!({"event_name": "pull_request_target"})), + wait, + ) + .await; + assert!(reply.unwrap().starts_with("HTTP/1.1 403")); + server.stop().await; +} + +/// N1: only `ASTER_SRV_MAX_LARGE_BODIES` large bodies (a commit's manifest, +/// a test-results upload) are read at once; the rest get 503 with +/// `Retry-After`, and small routes are unaffected. A slot frees when the +/// request that held it ends. +#[tokio::test] +async fn the_large_body_semaphore_bounds_concurrent_commits_and_uploads() { + let Some(server) = start_with(&[("ASTER_SRV_MAX_LARGE_BODIES", "1")]).await else { + return; + }; + let token = ci_token(json!({})); + let wait = std::time::Duration::from_millis(500); + // The first commit holds the only slot, its body half sent. + let (reply, first) = post_with_unsent_body(&server, "/v1/cache/commit", &token, wait).await; + assert_eq!(reply, None); + // A second commit and an upload of results are both turned away. + for path in ["/v1/cache/commit", "/v1/test-results/runs"] { + let (reply, _stream) = post_with_unsent_body(&server, path, &token, wait).await; + let reply = reply.unwrap_or_else(|| panic!("{path}: queued instead of refused")); + assert!(reply.starts_with("HTTP/1.1 503"), "{path}: {reply}"); + } + let client = aster_srv::http_client().build().unwrap(); + let refused = client + .post(format!("{}/v1/cache/commit", server.url)) + .bearer_auth(&token) + .body("{}") + .send() + .await + .unwrap(); + assert_eq!(refused.status(), 503); + assert_eq!(refused.headers()["retry-after"], "1"); + // Lookups carry small bodies and do not use the slot. + let (status, _) = server + .post_json( + "/v1/cache/lookup", + &json!({"repo": REPO, "keys": ["k"]}), + Some(&format!("Bearer {token}")), + None, + ) + .await; + assert_eq!(status, 200); + // When the first request ends, its slot is free again. + drop(first); + let mut status = 503; + for _ in 0..50 { + status = client + .post(format!("{}/v1/cache/commit", server.url)) + .bearer_auth(&token) + .body("{}") + .send() + .await + .unwrap() + .status() + .as_u16(); + if status != 503 { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + } + assert_eq!( + status, 400, + "the manifest is not valid JSON, but it was read" + ); + server.stop().await; +} diff --git a/crates/aster-srv/tests/server.rs b/crates/aster-srv/tests/server.rs index 8c73351..5553984 100644 --- a/crates/aster-srv/tests/server.rs +++ b/crates/aster-srv/tests/server.rs @@ -107,6 +107,11 @@ async fn start(retention_days: u32) -> Option { } fn token(repository: &str) -> String { + job_token(repository, "16161", "9f1c") +} + +/// The OIDC token of a job in GitHub run `run` (attempt 1) at `sha`. +fn job_token(repository: &str, run: &str, sha: &str) -> String { let mut header = Header::new(Algorithm::RS256); header.kid = Some("test".into()); encode( @@ -116,12 +121,25 @@ fn token(repository: &str) -> String { "aud": "aster-srv", "exp": Utc::now().timestamp() + 300, "repository": repository, + // The job that uploads: pull request 16161's merge commit 9f1c, + // GitHub run 16161 attempt 1. Its run ids start + // `gh::16161:1:` (see `stream`). + "run_id": run, + "run_attempt": "1", + "sha": sha, + "ref": "refs/pull/16161/merge", + "event_name": "pull_request", }), &EncodingKey::from_rsa_pem(PRIVATE).unwrap(), ) .unwrap() } +/// The Aster run id `name` in the uploading job's GitHub run. +fn gh(name: &str) -> String { + format!("gh:{REPO}:16161:1:{name}") +} + fn stream(run_id: &str, repo: &str, tests: usize) -> Vec { let mut records = vec![ Record::Run(RunRecord { @@ -204,13 +222,13 @@ async fn stores_a_run_once_and_keeps_the_raw_upload() { let Some(server) = start(30).await else { return; }; - let body = gzip(&to_ndjson(&stream("run-1", REPO, 3))); + let body = gzip(&to_ndjson(&stream(&gh("run-1"), REPO, 3))); let (status, reply) = upload(&server, Some(&token(REPO)), body.clone()).await; assert_eq!(status, 201, "{reply}"); assert_eq!( reply, - json!({"run_id": "run-1", "duplicate": false, "targets": 1, "tests": 3}) + json!({"run_id": gh("run-1"), "duplicate": false, "targets": 1, "tests": 3}) ); assert_eq!( @@ -282,12 +300,76 @@ async fn stores_a_run_once_and_keeps_the_raw_upload() { server.stop().await; } +#[tokio::test] +async fn a_job_files_results_only_under_its_own_run_commit_and_repository() { + let Some(server) = start(30).await else { + return; + }; + // Setup: GitHub run 16162 has not uploaded yet. A job of run 16161 + // tries to take its run id first, which would make the real upload a + // "duplicate" and drop its results. + let next_run = format!("gh:{REPO}:16162:1:test-ci:-:ab"); + let body = gzip(&to_ndjson(&stream(&next_run, REPO, 2))); + let (status, reply) = upload(&server, Some(&token(REPO)), body.clone()).await; + assert_eq!(status, 403, "{reply}"); + + // The real job of run 16162 uploads its results and they are stored. + let real = job_token(REPO, "16162", "9f1c"); + let (status, reply) = upload(&server, Some(&real), body).await; + assert_eq!(status, 201, "{reply}"); + assert_eq!(reply["duplicate"], false); + + // A job cannot file results under a commit it did not run. + let other_commit = gzip(&to_ndjson(&stream(&gh("other-commit"), REPO, 1))); + let (status, _) = upload( + &server, + Some(&job_token(REPO, "16161", "dead")), + other_commit, + ) + .await; + assert_eq!(status, 403); + + // Another allowed repository cannot use this repository's run ids, + // with its own name in the record or this one's. + let theirs = format!("gh:{REPO}:16161:1:theirs"); + for repo in ["ArchAstro/aster", REPO] { + let body = gzip(&to_ndjson(&stream(&theirs, repo, 1))); + let (status, _) = upload(&server, Some(&token("ArchAstro/aster")), body).await; + assert_eq!(status, 403, "{repo}"); + } + assert_eq!( + count(&server, "SELECT count(*) FROM test_results.runs").await, + 1, + "only the real job's run is stored" + ); + let source: (String, String, String) = { + let client = server.app.pool.get().await.unwrap(); + let row = client + .query_one( + "SELECT source, git_ref, event_name FROM test_results.runs", + &[], + ) + .await + .unwrap(); + (row.get(0), row.get(1), row.get(2)) + }; + assert_eq!( + source, + ( + "oidc".to_string(), + "refs/pull/16161/merge".to_string(), + "pull_request".to_string() + ) + ); + server.stop().await; +} + #[tokio::test] async fn refuses_missing_bad_and_mismatched_credentials() { let Some(server) = start(30).await else { return; }; - let body = gzip(&to_ndjson(&stream("run-2", REPO, 1))); + let body = gzip(&to_ndjson(&stream(&gh("run-2"), REPO, 1))); assert_eq!(upload(&server, None, body.clone()).await.0, 401); assert_eq!( upload(&server, Some("not-a-token"), body.clone()).await.0, @@ -321,7 +403,7 @@ async fn rejects_malformed_uploads() { reply["error"].as_str().unwrap().contains("line 1"), "{reply}" ); - let targets_first = to_ndjson(&stream("run-3", REPO, 1)[1..]); + let targets_first = to_ndjson(&stream(&gh("run-3"), REPO, 1)[1..]); assert_eq!( upload(&server, Some(&bearer), gzip(&targets_first)).await.0, 400 @@ -352,7 +434,7 @@ async fn ingests_fifty_thousand_tests_quickly() { let Some(server) = start(30).await else { return; }; - let body = gzip(&to_ndjson(&stream("run-big", REPO, 50_000))); + let body = gzip(&to_ndjson(&stream(&gh("run-big"), REPO, 50_000))); let started = Instant::now(); let (status, reply) = upload(&server, Some(&token(REPO)), body).await; let took = started.elapsed(); diff --git a/docs/test-tracking.md b/docs/test-tracking.md index 68f7144..14a5af3 100644 --- a/docs/test-tracking.md +++ b/docs/test-tracking.md @@ -272,8 +272,11 @@ At start Aster prints the list it will use, for example: ``` **The server** quarantines a test that is not broken once definite flakes -from 2 different failing runs fall within 7 days. A run that flaked more than -10 tests counts toward none of them, and an incident's failures never count. It releases the test after 20 +from 2 different failing runs fall within 7 days, counting only flakes whose +runs are both default-branch runs by their credentials (a push, dispatch or +schedule of the default branch): a pull request job's results score flakes +but never quarantine. A run that flaked more than 10 tests counts toward none +of them, and an incident's failures never count. It releases the test after 20 consecutive passes, when it breaks, or after 30 days; a released test starts afresh. The list is fetched at start and cached in `.aster/quarantine.json`. If the server cannot be reached, a cache younger than 24 hours stands in; diff --git a/src/cache/aster_srv.rs b/src/cache/aster_srv.rs index 7207b4a..e815332 100644 --- a/src/cache/aster_srv.rs +++ b/src/cache/aster_srv.rs @@ -30,6 +30,7 @@ use ureq::tls::{RootCerts, TlsConfig, TlsProvider}; use super::artifact::{Digest, Manifest}; use super::backend::CacheBackend; use super::hasher::encode_hex; +use crate::srv_url::{checked_origin, scheme_policy}; use crate::test_results::upload::{proxy_token_from_env, Credentials}; use super::login; @@ -111,7 +112,10 @@ impl AsterSrvBackend { /// uses the token `aster cache login` stored for this server, and with /// no unexpired token the remote is skipped. `None` when the URL or repo /// is empty. - pub fn from_env(url: Option<&str>, repo: Option<&str>) -> Option { + /// + /// A URL that is plain http to another machine is an error: the + /// credentials would cross the network in the clear. + pub fn from_env(url: Option<&str>, repo: Option<&str>) -> Result> { let env = |name: &str| std::env::var(name).ok(); let credentials = Credentials::from_env(&env); let person_token = match (&credentials, url) { @@ -120,18 +124,21 @@ impl AsterSrvBackend { } _ => None, }; - let backend = Self::from_parts( + let Some(backend) = Self::from_parts( url, repo, credentials, proxy_token_from_env(&env), person_token, - )?; + )? + else { + return Ok(None); + }; if let Some(notice) = signature_exclusion_notice(super::signature::signing_key().is_some()) { eprintln!("{notice}"); } - Some(backend) + Ok(Some(backend)) } fn from_parts( @@ -140,16 +147,11 @@ impl AsterSrvBackend { credentials: Option, proxy_token: Option, person_token: Option, - ) -> Option { - let base_url = non_empty(url)?; - let repo = non_empty(repo)?; - Some(Self::new( - base_url, - repo, - credentials, - proxy_token, - person_token, - )) + ) -> Result> { + let (Some(base_url), Some(repo)) = (non_empty(url), non_empty(repo)) else { + return Ok(None); + }; + Self::new(base_url, repo, credentials, proxy_token, person_token).map(Some) } /// Rewrite `https://storage.googleapis.com` blob URLs to `base`. @@ -159,8 +161,15 @@ impl AsterSrvBackend { /// without changing the URL the server signs. pub fn redirect_gcs_to(mut self, base: impl Into) -> Self { let base = base.into().trim_end_matches('/').to_string(); - if !base.is_empty() { - self.gcs_base = Some(base); + if base.is_empty() { + return self; + } + match checked_origin(&base) { + Ok(origin) => { + self.blob_agent = build_agent(&origin, BLOB_TIMEOUT); + self.gcs_base = Some(base); + } + Err(error) => eprintln!("[aster] Warning: ignoring ASTER_SRV_GCS_BASE: {error:#}"), } self } @@ -169,7 +178,8 @@ impl AsterSrvBackend { /// reproduces a production timeout without a multi-second wait. #[cfg(test)] fn with_request_timeout(mut self, timeout: Duration) -> Self { - self.agent = build_agent(timeout); + let origin = checked_origin(&self.base_url).expect("checked in new"); + self.agent = build_agent(&origin, timeout); self } @@ -179,25 +189,28 @@ impl AsterSrvBackend { credentials: Option, proxy_token: Option, person_token: Option, - ) -> Self { + ) -> Result { + let origin = checked_origin(&base_url)?; // A person's token reads only; do not ask for upload URLs it cannot get. let person_only = credentials.is_none() && person_token.is_some(); - Self { + Ok(Self { base_url: base_url.trim_end_matches('/').to_string(), repo, credentials, proxy_token, person_token, gcs_base: None, - agent: build_agent(REQUEST_TIMEOUT), - blob_agent: build_agent(BLOB_TIMEOUT), + agent: build_agent(&origin, REQUEST_TIMEOUT), + // Signed blob URLs come from the server's reply, so they get the + // server's policy: https only, unless the server is this machine. + blob_agent: build_agent(&origin, BLOB_TIMEOUT), state: Mutex::new(State { hits: HashMap::new(), upload_urls: HashMap::new(), read_only: person_only, }), logged_read_only: AtomicBool::new(false), - } + }) } /// Ask lookup for every `keys` entry that is not already cached. @@ -658,7 +671,10 @@ fn brief(body: &str) -> String { message.replace('\n', " ") } -pub(super) fn build_agent(timeout: Duration) -> ureq::Agent { +/// An agent for `origin` (already through `checked_origin`): https only +/// unless `origin` is this machine, and no redirect can downgrade. +pub(crate) fn build_agent(origin: &str, timeout: Duration) -> ureq::Agent { + let (https_only, max_redirects) = scheme_policy(origin); let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); let tls = TlsConfig::builder() .provider(TlsProvider::Rustls) @@ -666,7 +682,8 @@ pub(super) fn build_agent(timeout: Duration) -> ureq::Agent { .root_certs(RootCerts::WebPki) .build(); ureq::Agent::config_builder() - .https_only(false) + .https_only(https_only) + .max_redirects(max_redirects) .http_status_as_error(false) .tls_config(tls) .user_agent(USER_AGENT) @@ -992,6 +1009,7 @@ mod tests { None, ) .unwrap() + .unwrap() } #[test] @@ -1078,7 +1096,11 @@ mod tests { #[test] fn from_env_is_none_without_a_url_or_repo() { - assert!(AsterSrvBackend::from_parts(None, Some("acme/app"), None, None, None).is_none()); + assert!( + AsterSrvBackend::from_parts(None, Some("acme/app"), None, None, None) + .unwrap() + .is_none() + ); assert!( AsterSrvBackend::from_parts( Some("http://127.0.0.1:9"), @@ -1087,6 +1109,7 @@ mod tests { None, None ) + .unwrap() .is_some(), "a person with no CI token still gets a backend, which stays local until signed in" ); @@ -1097,9 +1120,12 @@ mod tests { None, None, ) + .unwrap() .is_none()); assert!( - AsterSrvBackend::from_parts(Some("http://x"), Some(" "), None, None, None).is_none() + AsterSrvBackend::from_parts(Some("http://x"), Some(" "), None, None, None) + .unwrap() + .is_none() ); } @@ -1123,6 +1149,7 @@ mod tests { person_token.map(str::to_string), ) .unwrap() + .unwrap() } #[test] @@ -1166,6 +1193,7 @@ mod tests { Some("ci-proxy".into()), Some("person-token".into()), ) + .unwrap() .unwrap(); ci.get_manifest("anything").unwrap(); assert_eq!( @@ -1350,4 +1378,57 @@ mod tests { static LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); LOCK.lock().unwrap_or_else(|poisoned| poisoned.into_inner()) } + + #[test] + fn a_cleartext_url_on_another_host_is_refused_and_https_and_loopback_are_not() { + for bad in [ + "http://aster-srv.example.com", + "http://10.0.0.5:8080", + "ftp://x", + ] { + let error = AsterSrvBackend::from_parts( + Some(bad), + Some("acme/app"), + Some(Credentials::Static("t".into())), + None, + None, + ) + .err() + .unwrap_or_else(|| panic!("{bad} was accepted")); + assert!(format!("{error:#}").contains("ASTER_SRV_URL"), "{error:#}"); + } + for ok in [ + "https://aster-srv.example.com", + "http://127.0.0.1:9", + "http://localhost:9", + ] { + assert!( + AsterSrvBackend::from_parts( + Some(ok), + Some("acme/app"), + Some(Credentials::Static("t".into())), + None, + None, + ) + .unwrap() + .is_some(), + "{ok}" + ); + } + } + + #[test] + fn a_cleartext_blob_base_is_ignored() { + let backend = AsterSrvBackend::from_parts( + Some("https://aster-srv.example.com"), + Some("acme/app"), + Some(Credentials::Static("t".into())), + None, + None, + ) + .unwrap() + .unwrap() + .redirect_gcs_to("http://blobs.example.com"); + assert!(backend.gcs_base.is_none()); + } } diff --git a/src/cache/backend.rs b/src/cache/backend.rs index 5db4161..cc4a169 100644 --- a/src/cache/backend.rs +++ b/src/cache/backend.rs @@ -628,7 +628,7 @@ pub fn select_remote( .filter(|repo| !repo.is_empty()) .or_else(|| std::env::var("GITHUB_REPOSITORY").ok()) .filter(|repo| !repo.trim().is_empty()); - match super::aster_srv::AsterSrvBackend::from_env(aster_srv_url, repo.as_deref()) { + match super::aster_srv::AsterSrvBackend::from_env(aster_srv_url, repo.as_deref())? { Some(backend) => { let backend = match std::env::var("ASTER_SRV_GCS_BASE") { Ok(base) if !base.trim().is_empty() => backend.redirect_gcs_to(base), @@ -1387,4 +1387,18 @@ mod tests { } assert!(tier.remote_enabled()); } + #[test] + fn select_remote_refuses_a_cleartext_aster_srv_url_on_another_host() { + let dir = tempfile::tempdir().unwrap(); + let error = select_remote( + RemoteKind::AsterSrv, + Some("http://aster-srv.example.com"), + Some(dir.path()), + None, + None, + ) + .err() + .expect("a cleartext url must be refused"); + assert!(format!("{error:#}").contains("must be https"), "{error:#}"); + } } diff --git a/src/cache/login.rs b/src/cache/login.rs index 0837f9b..5c16000 100644 --- a/src/cache/login.rs +++ b/src/cache/login.rs @@ -63,12 +63,20 @@ use serde::{Deserialize, Serialize}; use serde_json::{json, Value}; use sha2::{Digest, Sha256}; +use crate::srv_url::{checked_origin, is_secure_origin, server_origin}; + /// How close to expiry a stored token is treated as already expired. const EXPIRY_SKEW: Duration = Duration::from_secs(60); /// How long `aster cache login` waits for the browser to come back. pub const LOGIN_TIMEOUT: Duration = Duration::from_secs(5 * 60); +/// Printed before the paste prompt. A code someone else minted signs this +/// terminal in as them, and a social-engineering attacker asks for exactly +/// that paste. +pub const PASTE_WARNING: &str = "Only paste a code you got from this browser yourself. \ +Never paste a code someone sent you; it would sign this terminal in as them."; + /// The one line printed when there is no usable sign-in. pub const SIGNED_OUT_MESSAGE: &str = "[aster] not signed in; run aster cache login"; @@ -84,39 +92,6 @@ pub struct StoredToken { pub email: Option, } -/// `scheme://authority` of `url`, with the path dropped and the host -/// lowercased. `None` unless the scheme is http or https. -fn server_origin(url: &str) -> Option { - let url = url.trim(); - let (scheme, rest) = url.split_once("://")?; - let scheme = scheme.to_ascii_lowercase(); - if scheme != "http" && scheme != "https" { - return None; - } - let authority = rest.split(['/', '?', '#']).next()?.to_ascii_lowercase(); - if authority.is_empty() || authority.contains('@') { - return None; - } - Some(format!("{scheme}://{authority}")) -} - -/// Whether `origin` (as [`server_origin`] returns it) may carry a sign-in: -/// https, or http to this machine. A code, a verifier and a bearer token -/// over plain http to another host could be read by anyone on the path. -fn is_secure_origin(origin: &str) -> bool { - let Some((scheme, authority)) = origin.split_once("://") else { - return false; - }; - if scheme == "https" { - return true; - } - let host = match authority.strip_prefix('[') { - Some(rest) => rest.split(']').next().unwrap_or(""), - None => authority.split(':').next().unwrap_or(""), - }; - matches!(host, "127.0.0.1" | "::1" | "localhost") -} - /// Where to keep sign-ins: `$XDG_CONFIG_HOME/aster`, else `~/.config/aster` /// (`%APPDATA%\aster` on Windows). pub fn config_dir() -> Option { @@ -495,7 +470,7 @@ fn logout_in(config_dir: &Path, server_url: &str) -> Result { } fn revoke(origin: &str, token: &str) -> std::result::Result<(), String> { - let agent = super::aster_srv::build_agent(Duration::from_secs(15)); + let agent = super::aster_srv::build_agent(origin, Duration::from_secs(15)); let response = agent .post(&format!("{origin}/v1/auth/revoke")) .header("Authorization", &format!("Bearer {token}")) @@ -531,19 +506,6 @@ fn read_pasted_code(input: &mut dyn BufRead) -> Result { Ok(code.to_string()) } -/// A normal origin for `server_url`, or why it cannot be used. -fn checked_origin(server_url: &str) -> Result { - let origin = server_origin(server_url) - .with_context(|| format!("ASTER_SRV_URL must be an http(s) URL, not '{server_url}'"))?; - if !is_secure_origin(&origin) { - bail!( - "ASTER_SRV_URL must be https (plain http is only for this machine); \ - '{origin}' would send the sign-in in the clear" - ); - } - Ok(origin) -} - /// Eight characters of the server's user-code alphabet (5 bits each). fn random_user_code() -> Result { const ALPHABET: &[u8] = b"ABCDEFGHJKLMNPQRSTUVWXYZ23456789"; @@ -691,7 +653,7 @@ fn exchange( user_code: &str, port: Option, ) -> Result { - let agent = super::aster_srv::build_agent(Duration::from_secs(15)); + let agent = super::aster_srv::build_agent(origin, Duration::from_secs(15)); let mut response = agent .post(&format!("{origin}/v1/auth/token")) .header("Content-Type", "application/json") @@ -1191,4 +1153,9 @@ mod tests { assert!(out.revoked.is_err()); assert!(!path.exists(), "the local token is gone either way"); } + #[test] + fn the_paste_prompt_warns_against_a_code_someone_sent() { + assert!(PASTE_WARNING.contains("Only paste a code you got from this browser yourself")); + assert!(PASTE_WARNING.contains("Never paste a code someone sent you")); + } } diff --git a/src/cache/mod.rs b/src/cache/mod.rs index eab01e5..9405575 100644 --- a/src/cache/mod.rs +++ b/src/cache/mod.rs @@ -37,7 +37,7 @@ pub use http::{HttpCache, REMOTE_CACHE_TOKEN_ENV, REMOTE_CACHE_URL_ENV}; pub use local::{LocalStore, PruneStats}; pub use login::{ login, logout, remote_session, stored_token, Announcement, CodeReason, Flow, LoginMode, - LoginOptions, SignedIn, SignedOut, LOGIN_TIMEOUT, SIGNED_OUT_MESSAGE, + LoginOptions, SignedIn, SignedOut, LOGIN_TIMEOUT, PASTE_WARNING, SIGNED_OUT_MESSAGE, }; pub use matcher::TargetInputMatcher; pub use outputs::{pack, restore}; diff --git a/src/lib.rs b/src/lib.rs index a5c5653..433d5f4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -8,6 +8,7 @@ pub mod executor; pub mod git; pub mod graph; pub mod plugins; +pub(crate) mod srv_url; pub mod targets; pub mod test_results; pub mod ui; diff --git a/src/main.rs b/src/main.rs index 13c19d9..ed5c5fe 100644 --- a/src/main.rs +++ b/src/main.rs @@ -369,6 +369,7 @@ fn run() -> Result<()> { ); if copy { println!("Request id: {}", announcement.request_id); + println!("{}", aster::cache::PASTE_WARNING); print!("Paste the code the page shows, then press Enter: "); let _ = std::io::Write::flush(&mut std::io::stdout()); } else { diff --git a/src/srv_url.rs b/src/srv_url.rs new file mode 100644 index 0000000..c202d21 --- /dev/null +++ b/src/srv_url.rs @@ -0,0 +1,140 @@ +//! Which `ASTER_SRV_URL` values Aster will talk to, and the HTTP policy for +//! them. +//! +//! Every client of aster-srv (the person sign-in, the cache backend, test +//! result uploads and reads, quarantine, balance, summary) goes through +//! [`checked_origin`] before it builds an agent or sends a request. A bearer +//! token, an OIDC token or a sign-in code over plain http to another host +//! could be read by anyone on the path, so that is refused. + +use anyhow::{bail, Context, Result}; + +/// `scheme://authority` of `url`, with the path dropped and the host +/// lowercased. `None` unless the scheme is http or https. +pub(crate) fn server_origin(url: &str) -> Option { + let url = url.trim(); + let (scheme, rest) = url.split_once("://")?; + let scheme = scheme.to_ascii_lowercase(); + if scheme != "http" && scheme != "https" { + return None; + } + let authority = rest.split(['/', '?', '#']).next()?.to_ascii_lowercase(); + if authority.is_empty() || authority.contains('@') { + return None; + } + Some(format!("{scheme}://{authority}")) +} + +/// Whether `origin` (as [`server_origin`] returns it) may carry credentials: +/// https, or http to this machine. +pub(crate) fn is_secure_origin(origin: &str) -> bool { + let Some((scheme, _)) = origin.split_once("://") else { + return false; + }; + scheme == "https" || is_loopback_origin(origin) +} + +/// Whether `origin` names this machine (`127.0.0.1`, `::1`, `localhost`). +fn is_loopback_origin(origin: &str) -> bool { + let Some((_, authority)) = origin.split_once("://") else { + return false; + }; + let host = match authority.strip_prefix('[') { + Some(rest) => rest.split(']').next().unwrap_or(""), + None => authority.split(':').next().unwrap_or(""), + }; + matches!(host, "127.0.0.1" | "::1" | "localhost") +} + +/// A normal origin for `server_url`, or why it cannot be used. Nothing has +/// been sent when this fails. +pub(crate) fn checked_origin(server_url: &str) -> Result { + let origin = server_origin(server_url) + .with_context(|| format!("ASTER_SRV_URL must be an http(s) URL, not '{server_url}'"))?; + if !is_secure_origin(&origin) { + bail!( + "ASTER_SRV_URL must be https (plain http is only for this machine); \ + '{origin}' would send credentials in the clear" + ); + } + Ok(origin) +} + +/// How an agent that talks to `origin` (already through [`checked_origin`]) +/// treats the scheme: `(https_only, max_redirects)`. +/// +/// An https origin only follows https redirects, so a redirect cannot +/// downgrade. A loopback http origin follows no redirects at all, so it +/// cannot be sent to another host. +pub(crate) fn scheme_policy(origin: &str) -> (bool, u32) { + if origin.starts_with("https://") { + (true, 10) + } else { + (!is_loopback_origin(origin), 0) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn origins_are_normalised_and_odd_urls_refused() { + assert_eq!( + server_origin("https://Aster-Srv.Example.com/some/path?x=1").as_deref(), + Some("https://aster-srv.example.com") + ); + assert_eq!( + server_origin("http://127.0.0.1:8080").as_deref(), + Some("http://127.0.0.1:8080") + ); + for bad in [ + "", + "ftp://x", + "example.com", + "https://", + "https://user@example.com", + "http://127.0.0.1@evil.com", + ] { + assert_eq!(server_origin(bad), None, "{bad}"); + } + } + + #[test] + fn only_https_or_this_machine_is_secure() { + for ok in [ + "https://aster-srv.example.com", + "http://127.0.0.1:8080", + "http://localhost:3000", + "http://[::1]:3000", + ] { + assert!(is_secure_origin(ok), "{ok}"); + } + for bad in [ + "http://aster-srv.example.com", + "http://10.0.0.5:8080", + "http://127.0.0.1.evil.com", + "http://localhost.evil.com", + ] { + assert!(!is_secure_origin(bad), "{bad}"); + } + } + + #[test] + fn the_scheme_policy_blocks_downgrades() { + assert_eq!(scheme_policy("https://aster-srv.example.com"), (true, 10)); + assert_eq!(scheme_policy("http://127.0.0.1:8080"), (false, 0)); + assert_eq!(scheme_policy("http://[::1]:8080"), (false, 0)); + // An origin that was never checked still cannot downgrade. + assert_eq!(scheme_policy("http://example.com"), (true, 0)); + } + + #[test] + fn checked_origin_refuses_cleartext_to_another_host() { + assert!(checked_origin("https://aster-srv.example.com/").is_ok()); + assert!(checked_origin("http://127.0.0.1:1").is_ok()); + let error = checked_origin("http://aster-srv.example.com").unwrap_err(); + assert!(format!("{error:#}").contains("must be https"), "{error:#}"); + assert!(checked_origin("ftp://x").is_err()); + } +} diff --git a/src/test_results/client.rs b/src/test_results/client.rs index 5e9dba5..eb6c5e3 100644 --- a/src/test_results/client.rs +++ b/src/test_results/client.rs @@ -8,6 +8,7 @@ use anyhow::{bail, Context, Result}; use serde_json::Value; use super::upload::{agent, bearer, Credentials}; +use crate::srv_url::checked_origin; /// The most pages `failures_for_github_run` follows. const MAX_PAGES: usize = 200; @@ -37,7 +38,8 @@ impl Server { proxy_token: Option, timeout: Duration, ) -> Result { - let agent = agent(timeout); + let origin = checked_origin(url)?; + let agent = agent(&origin, timeout); let token = bearer(&agent, credentials)?; Ok(Self { agent, @@ -88,13 +90,14 @@ impl Server { token: String, proxy_token: Option, timeout: Duration, - ) -> Self { - Self { - agent: agent(timeout), + ) -> Result { + let origin = checked_origin(url)?; + Ok(Self { + agent: agent(&origin, timeout), base: url.trim_end_matches('/').to_string(), token, proxy_token, - } + }) } /// `GET path` (with its query): the status and the JSON body, `Null` @@ -329,4 +332,23 @@ mod tests { .unwrap() .is_none()); } + + #[test] + fn a_cleartext_server_on_another_host_is_refused_before_any_request() { + let static_token = Credentials::Static("secret".into()); + for url in ["http://aster-srv.example.com", "http://10.0.0.5:8080/x"] { + let error = Server::connect(url, &static_token, None).err().unwrap(); + assert!(format!("{error:#}").contains("must be https"), "{error:#}"); + let error = Server::with_token(url, "t".into(), None, Duration::from_secs(1)) + .err() + .unwrap(); + assert!(format!("{error:#}").contains("must be https"), "{error:#}"); + } + // https and this machine are fine; a static token needs no request. + assert!(Server::connect("https://aster-srv.example.com/", &static_token, None).is_ok()); + assert!(Server::connect("http://127.0.0.1:1", &static_token, None).is_ok()); + assert!( + Server::with_token("http://[::1]:1", "t".into(), None, Duration::from_secs(1)).is_ok() + ); + } } diff --git a/src/test_results/read.rs b/src/test_results/read.rs index d7dd4be..429ecd6 100644 --- a/src/test_results/read.rs +++ b/src/test_results/read.rs @@ -90,7 +90,7 @@ pub fn get( None => { let stored = stored_token(url, now).ok_or_else(|| anyhow!(SIGNED_OUT_MESSAGE))?; ( - Server::with_token(url, stored.token, proxy_token, TIMEOUT), + Server::with_token(url, stored.token, proxy_token, TIMEOUT)?, true, ) } @@ -171,4 +171,17 @@ mod tests { let missing = get(&url, "/v1/absent", &env, now).unwrap_err().to_string(); assert!(missing.contains("not found"), "{missing}"); } + + #[test] + fn a_cleartext_server_on_another_host_is_refused_for_reads() { + let env = |name: &str| (name == "ASTER_SRV_TOKEN").then(|| "secret".to_string()); + let error = get( + "http://aster-srv.example.com", + "/v1/test-results/tests", + &env, + SystemTime::now(), + ) + .unwrap_err(); + assert!(format!("{error:#}").contains("must be https"), "{error:#}"); + } } diff --git a/src/test_results/upload.rs b/src/test_results/upload.rs index 3ee551f..b044cd6 100644 --- a/src/test_results/upload.rs +++ b/src/test_results/upload.rs @@ -60,11 +60,18 @@ impl Credentials { } /// An HTTP agent whose requests give up after `timeout`. -pub(super) fn agent(timeout: Duration) -> ureq::Agent { +/// +/// `origin` is the server's origin (through `checked_origin`): the agent is +/// https only unless it is this machine, and follows no redirect that could +/// downgrade. +pub(super) fn agent(origin: &str, timeout: Duration) -> ureq::Agent { + let (https_only, max_redirects) = crate::srv_url::scheme_policy(origin); // ureq is built without its own crypto provider; use the aws-lc-rs one // the rest of Aster uses. Installing twice is harmless. let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); ureq::Agent::config_builder() + .https_only(https_only) + .max_redirects(max_redirects) .timeout_connect(Some(Duration::from_secs(10))) .timeout_global(Some(timeout)) .user_agent(concat!("aster/", env!("CARGO_PKG_VERSION"))) @@ -123,7 +130,8 @@ pub fn upload( encoder.write_all(&ndjson)?; let body = encoder.finish()?; - let agent = agent(Duration::from_secs(60)); + let origin = crate::srv_url::checked_origin(server_url)?; + let agent = agent(&origin, Duration::from_secs(60)); let token = bearer(&agent, credentials)?; let url = format!("{}/v1/test-results/runs", server_url.trim_end_matches('/')); let mut last_error = None; @@ -360,4 +368,69 @@ mod tests { "no proxy token, no proxy header" ); } + + #[test] + fn upload_refuses_cleartext_to_another_host_before_any_request() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("r.ndjson"); + std::fs::write(&path, b"{}\n").unwrap(); + let error = upload( + "http://aster-srv.example.com", + &path, + &Credentials::Static("secret".into()), + None, + ) + .unwrap_err(); + assert!(format!("{error:#}").contains("must be https"), "{error:#}"); + assert!(!format!("{error:#}").contains("cannot reach"), "{error:#}"); + } + + #[test] + fn a_loopback_http_agent_does_not_follow_a_redirect_to_another_server() { + // `a` answers 302 to `b`; the agent must stop at the 302. + let b = TcpListener::bind("127.0.0.1:0").unwrap(); + b.set_nonblocking(true).unwrap(); + let a = TcpListener::bind("127.0.0.1:0").unwrap(); + let a_url = format!("http://{}", a.local_addr().unwrap()); + let location = format!("http://{}/stolen", b.local_addr().unwrap()); + let server = std::thread::spawn(move || { + let (mut stream, _) = a.accept().unwrap(); + let mut reader = BufReader::new(stream.try_clone().unwrap()); + let mut line = String::new(); + loop { + line.clear(); + if reader.read_line(&mut line).unwrap() <= 2 { + break; + } + } + use std::io::Write; + write!( + stream, + "HTTP/1.1 302 Found\r\nLocation: {location}\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" + ) + .unwrap(); + }); + let origin = crate::srv_url::checked_origin(&a_url).unwrap(); + let response = agent(&origin, Duration::from_secs(5)) + .get(&format!("{a_url}/x")) + .header("Authorization", "Bearer secret") + .call() + .unwrap(); + server.join().unwrap(); + assert_eq!(response.status().as_u16(), 302); + assert!(b.accept().is_err(), "the redirect target was contacted"); + } + + #[test] + fn an_https_agent_refuses_plain_http() { + let origin = crate::srv_url::checked_origin("https://aster-srv.example.com").unwrap(); + let error = agent(&origin, Duration::from_secs(1)) + .get("http://127.0.0.1:1/") + .call() + .unwrap_err(); + assert!( + format!("{error}").to_lowercase().contains("https"), + "{error}" + ); + } } diff --git a/tests/cache_login_e2e.rs b/tests/cache_login_e2e.rs index 8860d7a..725f67c 100644 --- a/tests/cache_login_e2e.rs +++ b/tests/cache_login_e2e.rs @@ -171,6 +171,12 @@ async fn start() -> Option { /// A stand-in for GitHub's OIDC token endpoint: any GET gets a token that /// names `acme/app` on `main`, from the `ci.yml` workflow. fn github_oidc_endpoint(stop: Arc) -> String { + github_oidc_endpoint_for(stop, "7001", "0123abc") +} + +/// [`github_oidc_endpoint`] for one GitHub run and commit: aster-srv binds +/// uploaded test results to the token's `run_id`, `run_attempt` and `sha`. +fn github_oidc_endpoint_for(stop: Arc, run_id: &str, sha: &str) -> String { let mut header = Header::new(Algorithm::RS256); header.kid = Some("test".into()); let jwt = encode( @@ -181,8 +187,9 @@ fn github_oidc_endpoint(stop: Arc) -> String { "exp": chrono::Utc::now().timestamp() + 600, "repository": REPO, "ref": "refs/heads/main", - "sha": "0123abc", - "run_id": "7001", + "sha": sha, + "run_id": run_id, + "run_attempt": "1", "event_name": "push", "workflow_ref": CI_WORKFLOW, "job_workflow_ref": CI_WORKFLOW, @@ -610,9 +617,20 @@ async fn srv(dir: &Path, url: &str, config_home: &Path, args: &[&str]) -> (bool, ) } +/// The GitHub run id, commit and Aster run id of CI run `run`. The run id has +/// the `gh::::` shape aster-srv requires of an +/// upload made with that run's OIDC token. +fn ci_run(run: usize) -> (String, String, String) { + let github_run = format!("{}", 7000 + run); + let sha = format!("{run:040}"); + let run_id = format!("gh:{REPO}:{github_run}:1:test"); + (github_run, sha, run_id) +} + /// One CI run of `//app:test`, as `--test-results` writes it: `flaky_test` /// fails and then passes on retry, `broken_test` fails on every attempt. fn ci_results(run: usize) -> String { + let (_, sha, run_id) = ci_run(run); let record = |value: Value| format!("{value}\n"); let test = |id: &str, outcome: &str, attempt: u32, hash: Option<&str>| { record(json!({ @@ -623,8 +641,8 @@ fn ci_results(run: usize) -> String { }; [ record(json!({ - "type": "run", "run_id": format!("e2e-run-{run}"), "repo": REPO, - "sha": format!("{run:040}"), "branch": "main", "aster_version": "0.0.0", + "type": "run", "run_id": run_id, "repo": REPO, + "sha": sha, "branch": "main", "aster_version": "0.0.0", "started_at": chrono::Utc::now().to_rfc3339(), })), record(json!({ @@ -656,10 +674,12 @@ async fn a_signed_in_laptop_reads_every_dashboard_dataset_with_aster_srv() { let url = server.url.clone(); // Phase 1: CI uploads three runs of a suite with a flaky test and a broken - // one, with the job's OIDC token, through `aster results upload`. + // one, each with its own job's OIDC token (aster-srv binds a run to the + // token's run id and commit), through `aster results upload`. let stop_oidc = Arc::new(AtomicBool::new(false)); - let oidc_url = github_oidc_endpoint(stop_oidc.clone()); for run in 1..=3 { + let (github_run, sha, _) = ci_run(run); + let oidc_url = github_oidc_endpoint_for(stop_oidc.clone(), &github_run, &sha); let file = root.path().join(format!("results-{run}.ndjson")); fs::write(&file, ci_results(run)).unwrap(); let uploaded = tokio::task::spawn_blocking({ @@ -749,7 +769,15 @@ async fn a_signed_in_laptop_reads_every_dashboard_dataset_with_aster_srv() { &laptop, &url, &config_home, - &["get", "/v1/test-results/runs/e2e-run-1/failures"], + // The run id holds the repository's `/`, so it is percent-encoded + // to stay one path segment. + &[ + "get", + &format!( + "/v1/test-results/runs/{}/failures", + ci_run(1).2.replace('/', "%2F") + ), + ], ) .await; assert!(ok, "{stderr}");