[calvin-archastro]

Review on ArchCode

Merge order: this is the only 0.24.1 PR. After it merges, a version-bump PR, then tag v0.24.1. v0.24.0 will not be deployed.

Problem and intent

The second independent security review of v0.23.1..v0.24.0, plus firstlanding #19856, returned a conditional GO with no critical or high findings. Calvin asked for every finding to be fixed before aster-srv's /v1 channel goes live without IAP. This PR fixes N1 and N2-N15 on top of v0.24.0, one commit per finding (or a small group), so it can be reviewed commit by commit.

The two medium findings that matter most:

1. N1, pre-auth resource use on writer routes. A CI token that may not write the cache, such as an in-repo pull_request job, still made the server read and parse up to 64 MiB on /v1/cache/commit before require_writer refused it.
2. N5, lookup fan-out. lookup_hits ran a GCS HEAD and an IAM signBlob one blob at a time for up to 256 keys. Reply time grew with the bucket. This is also the likely server-side cause of the observed remote-cache read misses: the 0.22 client times out after 5s and, before feat(cache): one-line remote cache summary; batch lookup timeout no longer disables the remote #183, silently turned the remote off for the whole run (Stable CI job 120151277785: 0 cached of 127 targets whose keys hadn't changed).

What changed (before → after)

N	Before	After
N1	The gate admitted any CI token on writer routes; the writer check ran after reading the body	The gate applies the full writer policy (repo allowlist, owner-id pin, writer ref, event_name=push, ci.yml@ prefix) before reading any body, using one shared function, policy::cache_writer_allowed. A semaphore bounds concurrent large-body requests.
N2 (tab 7)	Uploaded runs trusted run_id, sha and ref as the client claimed them	Runs are tied to the uploading job's OIDC claims (gh:<repo>:<run>:<attempt>: prefix, sha must match the token, branch/attempt taken from the claims). Quarantine uses only trusted default-branch evidence. Migration test_results v7.
N3	Unauthenticated bodies had the 60s timeout; 1024 connections per listener, no per-IP cap	A short body timeout on unauthenticated routes, a per-IP connection cap, and idle keep-alive bounded by header_read_timeout (tested)
N4	The API listener started with unsafe settings; unpinned repos passed the owner check	With ASTER_SRV_API_ADDR set, the server refuses to start on static tokens, the dev blob route, an unpinned allowlisted repo, an unset writer-workflow list, or trusted_proxy_hops=0. The owner check fails closed.
N5	Sequential HEAD and signBlob per blob	Blob cap 4096 per lookup, 16 concurrent per request, a 3s time budget (entries over the cap or the budget are misses), positive existence memory for 1h, signed URLs reused while 5 of their 15 min remain, and a global signBlob bound (16 in flight, 200/s, burst 512, fail fast)
N6	CI, upload and aster srv clients accepted http:// to any host	Every aster-srv client uses checked_origin: https unless loopback. Agents are https-only, and loopback agents follow no redirects.
N7	One overflow bucket could starve authenticated subjects	Sharded overflow per limiter, with a throttled sweep
N8	No way to revoke a departed person	An operator runbook: SQL between README markers; a test runs exactly that SQL
N9	Key rotation undocumented; kill switch missing from the README	A rotation runbook for ASTER_SRV_CLI_TOKEN_KEYS; ASTER_SRV_PERSON_TOKENS added to the env table
N9b	A failed fetch or unknown-kid probe set last and suppressed a real rotation for 60s	last is set only on success; failures back off separately (keyfetch::may_fetch, in both auth.rs and iap.rs)
N10	A run_id from another repo revealed which repo owns it	Plain 404
N11	The device name on the confirm page looked verified; copy mode had no warning about pasted codes	Each detail is labelled by its source; copy mode and the CLI paste prompt warn against codes someone else sent
N12	Repeated X-Forwarded-For lines weren't treated as one list	Lines are joined in order (RFC 9110) before choosing the trusted hop; unit and e2e tests
N13	CI pull_request lookups bumped hits and last-used	Only writer-eligible CI lookups count; person lookups never count
N15	Every 401 logged a line	The first few per (reason, route, client IP) per window are logged and the rest summarised; counters and the success trail stay exact

Unchanged: the CI OIDC write path for main pushes, the person read allowlist, the accepted risks recorded in #190, and the client cache protocol.

Also 12e7198: the ASTER_SRV_CLI_TOKEN_TTL doc comment said 604800s; the code caps it at 43200.

Security checklist

Paths are under crates/aster-srv/ unless they start with src/ (the client crate).

N	Commit(s)	Code	Tests
N1	efdd1ad, f57b378	src/routes.rs writer_refusal; src/cache/policy.rs cache_writer_allowed; src/http.rs large_body_permit	tests/routes.rs: a_non_writer_ci_token_is_refused_before_its_body_is_read, the_large_body_semaphore_bounds_concurrent_commits_and_uploads
N2	a002489, 90fe648, 3901bc5	src/auth.rs run_attempt; src/test_results/ingest.rs bind; migration test_results v7; src/test_results/classifier.rs on_default/trusted; src/test_results/quarantine.rs	tests/server.rs, tests/incidents.rs, tests/classifier_model.rs, tests/quarantine.rs, tests/cli_auth.rs (tab 7)
N3	2ac7a37, f57b378	src/http.rs unauth_body_timeout; src/limits.rs Concurrency (per-IP cap)	tests/hardening.rs: unauthenticated_bodies_time_out_in_seconds_while_authenticated_ones_keep_the_long_timeout, an_idle_keep_alive_connection_is_closed_after_the_header_timeout, one_client_ip_cannot_hold_more_than_its_share_of_requests_open, without_a_proxy_one_peer_cannot_hold_more_connections_than_its_share; unit limits::a_key_holds_at_most_its_cap_and_a_dropped_slot_frees_it
N4	42a1617	src/config.rs API-listener checks; OwnerIds::accepts fails closed	unit config::the_api_listener_refuses_to_start_without_each_hardening_setting, every_allowlisted_repository_needs_an_owner_id_pin_when_the_api_listener_is_on, owner_ids_fail_closed_for_an_unpinned_repository_when_the_api_listener_is_on; tests/hardening.rs: the_api_listener_refuses_tokens_without_the_pinned_owner_id
N5	4c6248f, 18fbab7, eba438e	src/cache/routes.rs MAX_LOOKUP_BLOBS, per-request concurrency and budget; src/cache/store.rs readable_url, existence memory, digest cap; src/cache/signing.rs signBlob bound	tests/lookup_fanout.rs: entries_over_the_blob_cap_are_misses_and_are_not_counted, a_wide_lookup_finishes_in_bounded_time_and_a_repeat_is_free, a_slow_lookup_answers_within_its_budget_and_the_rest_are_misses; store units a_warm_digest_costs_no_head_and_no_signature, a_missing_blob_is_never_cached, a_remembered_blob_is_asked_about_again_after_its_ttl, delete_forgets_a_remembered_blob, a_url_without_enough_lifetime_left_is_signed_again, concurrent_lookups_of_one_digest_share_one_head_and_signature, idle_slots_are_dropped_past_the_cap; signing units concurrent_signatures_never_exceed_the_limit, an_exhausted_bucket_fails_fast_instead_of_queueing, the_bucket_refills_over_time, a_saturated_signer_turns_entries_into_misses_not_errors; V4 conformance matches_googles_v4_signing_vectors still passes
N6	08288ec	src/srv_url.rs checked_origin, scheme_policy; src/cache/aster_srv.rs from_env/from_parts/build_agent; src/cache/backend.rs select_remote; src/test_results/{client,read,upload}.rs	srv_url::tests::{checked_origin_refuses_cleartext_to_another_host, the_scheme_policy_blocks_downgrades, only_https_or_this_machine_is_secure}; aster_srv::tests::{a_cleartext_url_on_another_host_is_refused_and_https_and_loopback_are_not, a_cleartext_blob_base_is_ignored}; backend::tests::select_remote_refuses_a_cleartext_aster_srv_url_on_another_host; client::tests::a_cleartext_server_on_another_host_is_refused_before_any_request; read::tests::a_cleartext_server_on_another_host_is_refused_for_reads; upload::tests::{upload_refuses_cleartext_to_another_host_before_any_request, an_https_agent_refuses_plain_http, a_loopback_http_agent_does_not_follow_a_redirect_to_another_server}
N7	b1d193c	src/limits.rs overflow_key	units limits::a_flood_of_minted_keys_does_not_starve_a_real_newcomer, a_full_table_is_swept_at_most_once_per_interval, a_flood_against_the_ip_limiter_leaves_subjects_untouched
N8	cccb944	README.md "Revoking a departed person's tokens" (SQL between revoke-sql markers)	tests/operator_revoke.rs: the_documented_revoke_sql_logs_out_a_departed_person_and_nobody_else
N9	199d5eb	README.md "Rotating the signing keys"; ASTER_SRV_PERSON_TOKENS env row	docs; the kill switch is covered by the existing config units and tests/cli_auth_copy.rs
N9b	d5fc8c7, f57b378	src/keyfetch.rs may_fetch, used in src/auth.rs and src/iap.rs	tests/jwks_refetch.rs: a_failed_key_fetch_does_not_block_the_next_real_rotation, an_unknown_key_flood_does_not_cause_a_refetch_storm_or_hide_a_rotation, the_iap_verifier_follows_the_same_refetch_rules; keyfetch::tests
N10	67af47a	src/test_results/query.rs run failures	tests/query.rs reports_a_runs_failures_with_their_state (asserts 404)
N11	e7d5334	src/cli_auth.rs confirm and code pages; src/cache/login.rs PASTE_WARNING	tests/cli_auth_copy.rs copy_mode_shows_a_one_time_code_and_never_redirects_or_shows_a_token; login::tests::the_paste_prompt_warns_against_a_code_someone_sent
N12	8ad9bdb	src/limits.rs forwarded_for, client_ip; src/http.rs handle	units limits::repeated_forwarded_for_lines_are_one_list_and_cannot_move_the_client, limits::the_client_moves_left_one_entry_per_trusted_hop; tests/hardening.rs the_client_ip_comes_from_all_forwarded_for_lines_and_the_trusted_hops
N13	c67c9ae, 0e6434f	src/cache/routes.rs (counts = Ci && may_write)	tests/cache.rs a_pull_request_lookup_does_not_count_but_a_main_push_does (pull_request, person, main push)
N14	—	firstlanding infra: Cloud Armor rule 900	firstlanding #19927
N15	065d531	src/audit.rs Sampler, admit	tests/hardening.rs a_401_flood_is_sampled_and_summarised_while_counters_and_the_success_trail_stay_exact; units audit::the_first_few_per_key_are_admitted_and_the_rest_summarised_when_the_window_ends, a_flood_of_distinct_addresses_cannot_grow_the_table

Live verification after the ingress flip (N12, multiple X-Forwarded-For headers, client IP taken from the right hop) is added to firstlanding's live-negative-checks.py in #19927.

Scope

Server (aster-srv) and the aster client. Backend only.

Risk: medium

These are auth-boundary and resource-limit changes, so they're the riskiest part of the release, but every finding has a test that fails without its fix (spot-checked per commit by the authors).

• New startup refusals (N4) apply only when ASTER_SRV_API_ADDR is set. firstlanding #19856 sets every required setting (owner-id pin, writer workflows, trusted hops).
• N5's budget turns slow lookups into misses rather than errors, so the worst case is a cache miss, never a failed build.
• Migration test_results v7 is additive.

User impact

Laptop and CI users shouldn't see a change, apart from clearer confirm-page labels and the paste warning. A cold lookup on a large CI build now answers within about 3s, with partial hits instead of timing out.

Testing

Canonical end-to-end proofs:

• tests/cache_login_e2e.rs: a_laptop_signs_in_through_aster_srv_and_restores_what_ci_wrote, a_remote_shell_signs_in_by_pasting_a_code_and_restores_what_ci_wrote, and a_signed_in_laptop_reads_every_dashboard_dataset_with_aster_srv. These run the real aster binary, server and Postgres.
• tests/cache_remote_e2e.rs remote_restore_across_checkouts_through_aster_srv.
• Each finding's own boundary test is listed in the checklist.

Latency (N5), measured on real routes and Postgres, with in-memory GCS at 20ms per HEAD and 10ms per signBlob:

Case	v0.24.0	This PR
64 keys × 8 blobs, cold	17.5s	1.11s, 64 hits
same, warm	18.9s	23ms
256 × 16 (4096 blobs), cold	504 after 120s	3.05s, partial hits, the rest misses

Integration matrix:

Entry point	Variation	Covered by
/v1/cache/commit, /upload	writer main push / PR token / wrong workflow / unpinned owner; huge body before auth	tests/routes.rs
/v1/cache/lookup	cold / warm / over the cap / over the budget / saturated signer; PR vs main vs person counting	tests/lookup_fanout.rs, tests/cache.rs
/v1/test-results/runs	OIDC run binding, static namespace, trusted default branch	tests/server.rs, incidents.rs, quarantine.rs
/v1/auth/token, /cli-auth	slow body, per-IP cap, idle keep-alive, 401 flood sampling	tests/hardening.rs
JWKS (auth, iap)	failed fetch, unknown-kid flood, real rotation	tests/jwks_refetch.rs
Startup	each API-listener refusal	config units
Client	https / loopback http / cleartext remote refused / redirect downgrade	client units
Operator	the documented revoke SQL	tests/operator_revoke.rs

Fidelity:

• Real: Postgres, server code, the aster binary, JWT signing.
• Faked:
  • IAP is a signed header injected by the test, with no Google key fetch.
  • GCS is the in-memory store, with injected latency for the N5 measurements.
  • GitHub OIDC uses a test RSA key.
  • The signBlob limits are per replica.

Not proven here:

• Real Google load-balancer X-Forwarded-For shape, Cloud Armor, and cross-replica limits. firstlanding #19927's live checks cover these after the flip.
• Real IAM signBlob quota behaviour.
• Rollback of migration v7: it's additive, with no down migration.

Local checks on the assembled head (3db4301): fmt; clippy -D warnings; rustdoc -D warnings; full cargo test -p aster-srv against Postgres 17; cache_login_e2e (3) and cache_remote_e2e (2); root lib and bins (916 + 20). All pass.

3db4301 (test only): the dashboard e2e now gives each simulated CI run its own OIDC token and a matching gh:<repo>:<run>:<attempt>: run id, because N2 rightly refused the old e2e-run-N uploads with 403.

Follow-ups

1. The firstlanding imageRef bump and applies after the third review (v0.24.0..v0.24.1).
2. The N15 sampling test counts warnings across the whole test binary, so new bad-bearer tests in hardening.rs can push it over its limit.
3. Commits efdd1ad through 199d5eb fail CI clippy/rustdoc until f57b378; all of them compile.

🤖 Generated with Claude Code