From c34c729080ddd4427403c57b36f7e59701d147e7 Mon Sep 17 00:00:00 2001 From: Jess Sullivan Date: Sat, 22 Aug 2026 16:11:49 -0400 Subject: [PATCH 1/2] fix(ci): read Tailscale credentials from one scope, fail fast, validate server-side The CI `validate` job read the repository-scoped TAILSCALE_API_KEY while the CD `deploy` job, declaring `environment: production`, read the environment-scoped secret of the same name. Two different values behind one name, with nothing to show they differ: a red validate said nothing about the deploy path, and a rotation applied to one scope silently left the other dead. cd.yml also referenced `vars.TS_OAUTH_CLIENT_ID`, which is set in neither scope (repo variables and production environment variables are both empty), so any swap of the secret to an OAuth client secret would have failed the push with a RuntimeError from ts_auth.resolve_bearer. ci.yml never mapped the variable at all, so the OAuth path was unreachable from the pull request side. - Both jobs now declare `environment: production` and map TAILSCALE_API_KEY and TS_OAUTH_CLIENT_ID exactly once, at job level. A green validate is now evidence about the credential the deploy will use. - scripts/ci_preflight.py runs before the Nix install and fails with a message naming the exact secret or variable and the scope it must live in. It ends with a live auth probe, so an expired key reports as "rotate this, here" rather than an opaque `API error 401` several minutes into the job. - scripts/acl_validate.py adds the missing pre-merge gate: Tailscale type-checks the built policy via POST /acl/validate, which nothing did before (push.py --dry-run only diffs local against live). That endpoint reports policy errors with HTTP 200, so --prove first submits a policy the server must reject and refuses to claim a pass if it comes back clean. - docs/ci-credentials.md records the two-scope trap, both credential kinds and the rotation runbook for each. XDG_CACHE_HOME moves to a `Configure cache dir` step because the `runner` context is not available in job-level `env` (caught by actionlint). No policy change: no .dhall file and no grants.json entry is touched. --- .github/workflows/cd.yml | 33 ++-- .github/workflows/ci.yml | 31 +++- docs/ci-credentials.md | 132 ++++++++++++++++ scripts/acl_validate.py | 232 +++++++++++++++++++++++++++++ scripts/ci_preflight.py | 161 ++++++++++++++++++++ tests/test_ci_credential_wiring.py | 199 +++++++++++++++++++++++++ 6 files changed, 766 insertions(+), 22 deletions(-) create mode 100644 docs/ci-credentials.md create mode 100644 scripts/acl_validate.py create mode 100644 scripts/ci_preflight.py create mode 100644 tests/test_ci_credential_wiring.py diff --git a/.github/workflows/cd.yml b/.github/workflows/cd.yml index d32d03c..a63bfa3 100644 --- a/.github/workflows/cd.yml +++ b/.github/workflows/cd.yml @@ -15,37 +15,42 @@ jobs: deploy: name: Build, validate, push ACL runs-on: ubuntu-latest + # The CI `validate` job declares this same environment, so both jobs read + # one set of credentials from one scope. See docs/ci-credentials.md. environment: production + env: + TAILSCALE_API_KEY: ${{ secrets.TAILSCALE_API_KEY }} + TS_OAUTH_CLIENT_ID: ${{ vars.TS_OAUTH_CLIENT_ID }} steps: - uses: actions/checkout@v4 + # The runner context is unavailable in job-level env, so export the Nix + # cache dir once here instead of repeating it on every step. + - name: Configure cache dir + run: echo "XDG_CACHE_HOME=$RUNNER_TEMP/.cache" >> "$GITHUB_ENV" + + # Runs before the Nix install so a dead credential fails in seconds with + # a message naming the secret, the scope, and the fix. + - name: Preflight Tailscale credential wiring + run: python3 scripts/ci_preflight.py + - uses: DeterminateSystems/nix-installer-action@main - uses: DeterminateSystems/magic-nix-cache-action@main - name: Build policy JSON run: nix develop --command just build - env: - XDG_CACHE_HOME: ${{ runner.temp }}/.cache + + # Hard gate: never push a policy the API would reject. + - name: Server-side policy validation + run: python3 scripts/acl_validate.py --prove - name: Validate against live ACL continue-on-error: true run: nix develop --command python3 scripts/validate.py - env: - TAILSCALE_API_KEY: ${{ secrets.TAILSCALE_API_KEY }} - TS_OAUTH_CLIENT_ID: ${{ vars.TS_OAUTH_CLIENT_ID }} - XDG_CACHE_HOME: ${{ runner.temp }}/.cache - name: Show diff run: | nix develop --command python3 scripts/push.py --dry-run || true - env: - TAILSCALE_API_KEY: ${{ secrets.TAILSCALE_API_KEY }} - TS_OAUTH_CLIENT_ID: ${{ vars.TS_OAUTH_CLIENT_ID }} - XDG_CACHE_HOME: ${{ runner.temp }}/.cache - name: Push ACL to Tailscale run: nix develop --command python3 scripts/push.py --confirm - env: - TAILSCALE_API_KEY: ${{ secrets.TAILSCALE_API_KEY }} - TS_OAUTH_CLIENT_ID: ${{ vars.TS_OAUTH_CLIENT_ID }} - XDG_CACHE_HOME: ${{ runner.temp }}/.cache diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 91c7885..b189b72 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -92,16 +92,37 @@ jobs: runs-on: ubuntu-latest needs: [check, secrets] if: github.event_name == 'pull_request' + # Deliberately the same environment the CD deploy job uses, so this job + # reads the *same* TAILSCALE_API_KEY and TS_OAUTH_CLIENT_ID values the push + # will read. Without it, a green run here says nothing about the deploy + # path. See docs/ci-credentials.md. + environment: production + env: + TAILSCALE_API_KEY: ${{ secrets.TAILSCALE_API_KEY }} + TS_OAUTH_CLIENT_ID: ${{ vars.TS_OAUTH_CLIENT_ID }} steps: - uses: actions/checkout@v4 + # The runner context is unavailable in job-level env, so export the Nix + # cache dir once here instead of repeating it on every step. + - name: Configure cache dir + run: echo "XDG_CACHE_HOME=$RUNNER_TEMP/.cache" >> "$GITHUB_ENV" + + # Runs before the Nix install so a dead credential fails in seconds with + # a message naming the secret, the scope, and the fix. + - name: Preflight Tailscale credential wiring + run: python3 scripts/ci_preflight.py + - uses: DeterminateSystems/nix-installer-action@main - uses: DeterminateSystems/magic-nix-cache-action@main - name: Build policy run: nix develop --command just build - env: - XDG_CACHE_HOME: ${{ runner.temp }}/.cache + + # Server-side grammar check. push.py --dry-run only diffs locally; this + # asks Tailscale whether the policy would be accepted at all. + - name: Server-side policy validation + run: python3 scripts/acl_validate.py --prove - name: Validate against live id: validate @@ -127,9 +148,6 @@ jobs: fi exit "$EXIT_CODE" - env: - TAILSCALE_API_KEY: ${{ secrets.TAILSCALE_API_KEY }} - XDG_CACHE_HOME: ${{ runner.temp }}/.cache - name: Diff summary if: always() @@ -142,9 +160,6 @@ jobs: echo "$OUTPUT" echo 'EOF' } >> "$GITHUB_OUTPUT" - env: - TAILSCALE_API_KEY: ${{ secrets.TAILSCALE_API_KEY }} - XDG_CACHE_HOME: ${{ runner.temp }}/.cache - name: Comment on PR if: always() diff --git a/docs/ci-credentials.md b/docs/ci-credentials.md new file mode 100644 index 0000000..f5c11d1 --- /dev/null +++ b/docs/ci-credentials.md @@ -0,0 +1,132 @@ +# CI/CD credentials for the tailnet policy + +This repo pushes the live tailnet policy for `taila4c78d.ts.net`. Two GitHub +Actions jobs talk to the Tailscale API: + +| Job | Workflow | Trigger | What it does | +| --- | --- | --- | --- | +| `validate` | `.github/workflows/ci.yml` | `pull_request` | Reads the live ACL, diffs it, asks Tailscale to type-check the built policy | +| `deploy` | `.github/workflows/cd.yml` | `push` to `main` | Same checks, then `POST /acl` to apply | + +## One scope, one credential set + +Both jobs declare `environment: production`. That is deliberate, and it is the +whole point of this document. + +GitHub resolves `secrets.X` from the job's environment first and falls back to +the repository scope, so a repository secret and an environment secret with the +**same name** are two different values with no visible sign that they differ. +Before this was unified, `validate` read the repository-scoped +`TAILSCALE_API_KEY` and `deploy` read the `production`-scoped one. A red +`validate` therefore said nothing about whether the deploy would work, and a +rotation applied to one scope silently left the other dead. + +The rule now: **the `production` environment is the only scope that matters.** +A repository-level secret of the same name is dead weight and should be deleted +so it cannot be rotated by mistake. + +| Name | Type | Scope | Required | +| --- | --- | --- | --- | +| `TAILSCALE_API_KEY` | Environment **secret** | `production` | always | +| `TS_OAUTH_CLIENT_ID` | Environment **variable** | `production` | only when the secret is an OAuth client secret | + +`TS_OAUTH_CLIENT_ID` is a *variable*, not a secret — the client id is not +sensitive, and storing it as a secret makes it unreadable in logs for no gain. +Environment variables live under +*Settings → Environments → production → Environment variables*. + +## Two kinds of credential + +`scripts/ts_auth.py` accepts either form in `TAILSCALE_API_KEY` and branches on +the prefix: + +- **`tskey-api-…`** — a direct admin API key. Used as the bearer token as-is. + Needs no client id. **Expires after at most 90 days**, so it guarantees a + future outage on a timer. +- **`tskey-client-…`** — an OAuth client secret. Tailscale rejects it as a + bearer token (HTTP 403); it is exchanged for a short-lived access token via + the `client_credentials` grant. **Does not expire.** The exchange requires the + client id, so `TS_OAUTH_CLIENT_ID` becomes mandatory. + +Either kind must carry the `policy_file` scope. Without it the credential +authenticates but the ACL read returns HTTP 403. + +The OAuth pair is the preferred configuration: non-expiring, and scopeable to +`policy_file` alone rather than full admin. + +## Rotation runbook + +### Preferred: OAuth client pair + +1. Tailscale admin console → **Settings → OAuth clients → Generate OAuth + client**. Grant **only** the `policy_file` scope (read **and** write — the CD + job pushes). +2. Copy both halves. The secret (`tskey-client-…`) is shown once. +3. GitHub → **Settings → Environments → production**: + - **Environment secrets** → update `TAILSCALE_API_KEY` to the client secret. + - **Environment variables** → add/update `TS_OAUTH_CLIENT_ID` to the client + id. +4. Delete the old OAuth client in the Tailscale console. +5. Re-run CI on any open PR. `Preflight Tailscale credential wiring` proves the + new pair before anything else runs. + +### Interim: direct API key + +1. Tailscale admin console → **Settings → Keys → Generate access token**. +2. GitHub → **Settings → Environments → production → Environment secrets** → + update `TAILSCALE_API_KEY`. +3. Leave `TS_OAUTH_CLIENT_ID` unset (or accept the preflight warning that it is + ignored). +4. Diary the 90-day expiry, or move to the OAuth pair. + +Do **not** set the repository-scoped `TAILSCALE_API_KEY`. It is not read by +either job. + +## What the preflight tells you + +`scripts/ci_preflight.py` runs first in both jobs, on the runner's system +python3, before the Nix toolchain is installed — a dead credential costs +seconds, not a full dev-shell build followed by an opaque `API error 401`. It +never prints a credential value, only the kind inferred from the prefix. + +| Symptom | Meaning | Fix | +| --- | --- | --- | +| `Tailscale credential is not wired` | `TAILSCALE_API_KEY` unset in the `production` environment | Create the environment secret | +| `Tailscale OAuth client id is missing` | Secret is `tskey-client-…`, `TS_OAUTH_CLIENT_ID` unset | Add the environment **variable** | +| `Tailscale OAuth token exchange failed` | Client id and client secret are not a matching pair | Re-copy both halves from one OAuth client | +| `…present but rejected (HTTP 401)` | Expired or revoked credential | Rotate the value | +| `…lacks policy-file permission (HTTP 403)` | Credential lacks `policy_file` scope | Regenerate with the scope | + +## Server-side policy validation + +`scripts/acl_validate.py` POSTs the built policy to +`POST /api/v2/tailnet/{tailnet}/acl/validate`, which type-checks it without +applying anything. This is the only pre-merge check that can catch a policy +Tailscale will refuse — `push.py --dry-run` merely diffs the local build against +the live ACL and never asks whether the result is legal. + +The endpoint has a trap: **policy errors are returned with HTTP 200** and a +JSON body carrying `message` / `data`, matching +`tailscale.com/cmd/gitops-pusher`. A naive status-code check passes everything. +So the script treats a non-empty `message` or `data` as failure, and `--prove` +first submits a policy that must be rejected (unknown action plus an undefined +group reference). If that known-bad policy comes back clean, the checker is +blind and the step fails rather than reporting a pass it cannot justify. + +Without credentials the step emits a loud warning and skips, which only happens +locally — in CI the preflight has already failed the job. + +## Caveats of `environment:` on a pull-request job + +- **Fork PRs get no secrets.** This is GitHub behaviour for any scope, not a + consequence of using an environment. Policy changes have to come from a + branch on this repository. +- **Adding required reviewers to the `production` environment would gate every + PR**, because the `validate` job would then wait for a deployment approval. + If that protection is ever wanted for deploys only, split the environments and + update the `environment:` key plus the `SCOPE` string in + `scripts/ci_preflight.py` together. +- **A deployment-branch policy restricting `production` to `main` would break + PR validation** for the same reason. +- Each PR run records a `production` deployment in the Environments UI. That is + cosmetic noise; `validate` applies nothing. diff --git a/scripts/acl_validate.py b/scripts/acl_validate.py new file mode 100644 index 0000000..9745d00 --- /dev/null +++ b/scripts/acl_validate.py @@ -0,0 +1,232 @@ +#!/usr/bin/env python3 +"""Ask Tailscale to type-check generated/policy.json without applying it. + +``push.py --dry-run`` only diffs the local build against the live ACL. Nothing +in CI asks Tailscale whether the policy it is about to push is *acceptable*, so +a grammar or reference error survives review and first surfaces as a failed CD +push after merge. This closes that gap with + + POST /api/v2/tailnet/{tailnet}/acl/validate + +which validates a policy server-side and applies nothing. + +Response contract, mirroring tailscale.com/cmd/gitops-pusher (testNewACLs): + + - a non-2xx status is a failure; + - a 2xx response whose body carries a non-empty ``message`` or a non-empty + ``data`` array is ALSO a failure -- Tailscale reports policy errors with + HTTP 200; + - an empty body, or ``{}``, is a pass. + +Because "HTTP 200" does not mean "valid" here, ``--prove`` first submits a +policy Tailscale must reject. If that known-bad policy comes back clean, this +checker cannot see failures at all, and we fail loudly rather than report a +pass we have no basis for. +""" + +import argparse +import json +import os +import sys +import urllib.error +import urllib.request +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +from ts_auth import resolve_bearer # noqa: E402 + +REPO_ROOT = Path(__file__).resolve().parent.parent +GENERATED_POLICY = REPO_ROOT / "generated" / "policy.json" +TAILNET = "taila4c78d.ts.net" +API_BASE = f"https://api.tailscale.com/api/v2/tailnet/{TAILNET}" + +IN_ACTIONS = os.environ.get("GITHUB_ACTIONS") == "true" + +# Two independent grammar errors: an action Tailscale does not define, and a +# reference to a group that is not declared anywhere. Used by --prove to +# confirm this checker can actually observe a rejection. +KNOWN_BAD_POLICY = { + "acls": [ + { + "action": "definitely-not-a-valid-action", + "src": ["group:tailnet-acl-selftest-undefined-group"], + "dst": ["*:*"], + } + ] +} + + +def _annotate(level: str, title: str, lines: "list[str]") -> None: + sys.stdout.flush() + if IN_ACTIONS: + print(f"::{level} title={title}::{' '.join(lines)}") + print(title, file=sys.stderr) + for line in lines: + print(f" {line}", file=sys.stderr) + + +def post_validate(bearer: str, policy: dict) -> "tuple[int, str]": + """POST a policy to the validate endpoint. Returns (status, body).""" + data = json.dumps(policy).encode("utf-8") + req = urllib.request.Request(f"{API_BASE}/acl/validate", data=data, method="POST") + req.add_header("Authorization", f"Bearer {bearer}") + # gitops-pusher posts a whole policy file with this content type; strict + # JSON is valid HuJSON, so the body below is what upstream would send. + req.add_header("Content-Type", "application/hujson") + req.add_header("Accept", "application/json") + try: + with urllib.request.urlopen(req, timeout=60) as resp: + return resp.status, resp.read().decode(errors="replace") + except urllib.error.HTTPError as exc: + return exc.code, exc.read().decode(errors="replace") + + +def interpret(status: int, body: str) -> "tuple[bool, list[str]]": + """Decide whether a /acl/validate response is a pass, and why not.""" + problems: list[str] = [] + text = body.strip() + parsed = None + + if text: + try: + parsed = json.loads(text) + except json.JSONDecodeError: + problems.append(f"unparseable response body: {text[:500]}") + + if isinstance(parsed, dict): + message = parsed.get("message") or "" + if message: + problems.append(str(message)) + for entry in parsed.get("data") or []: + if not isinstance(entry, dict): + problems.append(str(entry)) + continue + prefix = f"user {entry['user']}: " if entry.get("user") else "" + for err in entry.get("errors") or []: + problems.append(f"{prefix}error: {err}") + for warning in entry.get("warnings") or []: + problems.append(f"{prefix}warning: {warning}") + elif parsed is not None: + problems.append(f"unexpected response shape: {text[:500]}") + + if status // 100 != 2: + problems.append(f"HTTP {status}") + + return (not problems), problems + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + parser.add_argument( + "--prove", + action="store_true", + help=( + "Before validating the real policy, submit a deliberately invalid one and " + "require that Tailscale rejects it. Guards against reporting a vacuous pass." + ), + ) + args = parser.parse_args() + + secret = os.environ.get("TAILSCALE_API_KEY", "").strip() + if not secret: + _annotate( + "warning", + "Server-side ACL validation SKIPPED", + [ + "TAILSCALE_API_KEY is not set, so the policy was not checked against the " + "Tailscale grammar.", + "In CI this cannot happen: scripts/ci_preflight.py fails the job first. " + "Locally, export TAILSCALE_API_KEY to enable this check.", + ], + ) + return 0 + + if not GENERATED_POLICY.exists(): + _annotate( + "error", + "Nothing to validate", + [f"{GENERATED_POLICY} does not exist. Run 'just build' first."], + ) + return 1 + + try: + bearer = resolve_bearer(secret) + except Exception as exc: # noqa: BLE001 -- surfaced verbatim to the operator + _annotate( + "error", + "Tailscale token could not be resolved", + [str(exc), "See docs/ci-credentials.md."], + ) + return 1 + + if args.prove: + status, body = post_validate(bearer, KNOWN_BAD_POLICY) + ok, problems = interpret(status, body) + if status in (404, 405): + # The endpoint is not available for this tailnet. That is not a + # policy problem and must not fail the build -- but say so loudly, + # because the pre-merge grammar gate is then absent. + _annotate( + "warning", + "Server-side ACL validation UNAVAILABLE", + [ + f"POST {API_BASE}/acl/validate returned HTTP {status}, so this " + "tailnet does not expose the validate endpoint.", + "Skipping the grammar gate. Policy errors will surface at CD push " + "time instead of on the pull request.", + ], + ) + return 0 + if status // 100 != 2: + # A transport or auth failure would also "reject" the known-bad + # policy, which would make the self-test vacuous. Say so plainly + # instead of claiming the validator works. + _annotate( + "error", + "Could not run the ACL validator self-test", + [ + f"The validate endpoint returned HTTP {status}, so the self-test " + "proves nothing about grammar checking.", + f"Body: {body.strip()[:300] or '(empty)'}", + "This is a credential or availability problem, not a policy problem. " + "See docs/ci-credentials.md.", + ], + ) + return 1 + if ok: + _annotate( + "error", + "Server-side ACL validation is blind", + [ + "Tailscale accepted a policy that is deliberately invalid (unknown " + "action plus an undefined group reference), so a clean result from " + "this endpoint proves nothing about the real policy.", + f"Endpoint returned HTTP {status} with body: {body.strip()[:300] or '(empty)'}", + "Refusing to report a pass. Re-check the endpoint contract before " + "trusting this step.", + ], + ) + return 1 + print("Self-test OK: the known-bad policy was rejected as expected.") + print(f" rejection: {problems[0][:300]}") + + policy = json.loads(GENERATED_POLICY.read_text(encoding="utf-8")) + print(f"Validating generated/policy.json against {TAILNET} ...") + status, body = post_validate(bearer, policy) + ok, problems = interpret(status, body) + + if ok: + print(f"Server-side validation PASSED (HTTP {status}).") + return 0 + + _annotate( + "error", + "Tailscale rejected the generated policy", + [f"HTTP {status}."] + problems, + ) + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/ci_preflight.py b/scripts/ci_preflight.py new file mode 100644 index 0000000..664cb29 --- /dev/null +++ b/scripts/ci_preflight.py @@ -0,0 +1,161 @@ +#!/usr/bin/env python3 +"""Fail fast, and legibly, when the Tailscale credential wiring is wrong. + +This runs on the runner's system python3 before the Nix toolchain is installed, +so a missing or dead credential costs seconds rather than a full dev-shell +build followed by an opaque ``API error 401`` a few minutes later. + +Every failure names the exact secret or variable, and the exact scope it has to +live in, so a rotation lands in the right place on the first attempt. + +No credential value is ever printed -- only its kind, inferred from the prefix. +""" + +import os +import sys +import urllib.error +import urllib.request +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +from ts_auth import resolve_bearer # noqa: E402 + +TAILNET = "taila4c78d.ts.net" +API_BASE = f"https://api.tailscale.com/api/v2/tailnet/{TAILNET}" + +SECRET_NAME = "TAILSCALE_API_KEY" +CLIENT_ID_VAR = "TS_OAUTH_CLIENT_ID" + +# The CI `validate` job and the CD `deploy` job both declare +# `environment: production`, so both resolve their credentials from this one +# scope. Keep this string in sync with the `environment:` key in +# .github/workflows/ci.yml and .github/workflows/cd.yml. +SCOPE = 'the "production" environment (repo Settings -> Environments -> production)' + +IN_ACTIONS = os.environ.get("GITHUB_ACTIONS") == "true" + + +def _annotate(level: str, title: str, lines: "list[str]") -> None: + """Emit a GitHub annotation (single line) plus readable stderr output.""" + sys.stdout.flush() + if IN_ACTIONS: + # Annotation bodies cannot contain raw newlines. + print(f"::{level} title={title}::{' '.join(lines)}") + print(f"{title}", file=sys.stderr) + for line in lines: + print(f" {line}", file=sys.stderr) + + +def fail(title: str, *lines: str) -> int: + _annotate("error", title, list(lines)) + return 1 + + +def warn(title: str, *lines: str) -> None: + _annotate("warning", title, list(lines)) + + +def main() -> int: + secret = os.environ.get(SECRET_NAME, "").strip() + client_id = os.environ.get(CLIENT_ID_VAR, "").strip() + + if not secret: + return fail( + "Tailscale credential is not wired", + f"{SECRET_NAME} is unset or empty in this job.", + f"Create it as an ENVIRONMENT secret named {SECRET_NAME} in {SCOPE}.", + "A repository-level secret of the same name is NOT what this job reads: " + "the job declares `environment: production`, and an environment secret of " + "the same name shadows the repository secret.", + ) + + if secret.startswith("tskey-client-"): + kind = "OAuth client secret (tskey-client-...)" + if not client_id: + return fail( + "Tailscale OAuth client id is missing", + f"{SECRET_NAME} holds an OAuth client secret, which cannot be used as a " + "bearer token on its own -- it has to be exchanged for an access token, " + "and that exchange requires the client id.", + f"Set a variable (not a secret; the client id is not sensitive) named " + f"{CLIENT_ID_VAR} in {SCOPE}, under 'Environment variables'.", + f"Alternatively, swap {SECRET_NAME} back to a direct API key " + "(tskey-api-...), which needs no client id.", + ) + elif secret.startswith("tskey-api-"): + kind = "direct API key (tskey-api-...)" + if client_id: + warn( + "TS_OAUTH_CLIENT_ID is set but unused", + f"{SECRET_NAME} holds a direct API key, so {CLIENT_ID_VAR} is ignored. " + "This is harmless, but it means the OAuth path is untested.", + ) + else: + kind = "unrecognised prefix" + warn( + "Tailscale credential has an unrecognised prefix", + f"{SECRET_NAME} starts with neither 'tskey-api-' nor 'tskey-client-'. " + "This is usually a truncated or mis-pasted value. The live probe below is " + "the authority.", + ) + + print(f"Credential scope: {SCOPE}") + print(f"Credential kind: {kind}") + print(f"{CLIENT_ID_VAR}: {'set' if client_id else 'unset'}") + + try: + bearer = resolve_bearer(secret) + except urllib.error.HTTPError as exc: + detail = exc.read().decode(errors="replace")[:400] + return fail( + "Tailscale OAuth token exchange failed", + f"The client_credentials grant returned HTTP {exc.code}: {detail}", + f"Check that {CLIENT_ID_VAR} in {SCOPE} is the client id belonging to the " + f"client secret currently stored in {SECRET_NAME} -- a mismatched pair fails " + "here.", + ) + except Exception as exc: # noqa: BLE001 -- surfaced verbatim to the operator + return fail("Tailscale token could not be resolved", str(exc)) + + req = urllib.request.Request(f"{API_BASE}/acl") + req.add_header("Authorization", f"Bearer {bearer}") + req.add_header("Accept", "application/json") + try: + with urllib.request.urlopen(req, timeout=30) as resp: + resp.read() + status = resp.status + except urllib.error.HTTPError as exc: + detail = exc.read().decode(errors="replace")[:400] + if exc.code == 401: + return fail( + "Tailscale credential is present but rejected (HTTP 401)", + f"The API returned: {detail}", + f"Rotate the value of the {SECRET_NAME} environment secret in {SCOPE}.", + "Direct API keys (tskey-api-...) expire after at most 90 days. An " + "ACL-scoped OAuth client secret (tskey-client-...) does not expire and is " + f"the preferred replacement, but it additionally needs its client id in " + f"the {CLIENT_ID_VAR} variable in {SCOPE}.", + "See docs/ci-credentials.md for the rotation runbook.", + ) + if exc.code == 403: + return fail( + "Tailscale credential is valid but lacks policy-file permission (HTTP 403)", + f"The API returned: {detail}", + "The API key or OAuth client must carry the 'policy_file' scope " + "(Tailscale admin console -> Settings -> Keys / OAuth clients).", + ) + return fail( + f"Tailscale API probe failed (HTTP {exc.code})", + detail, + f"Probed {API_BASE}/acl.", + ) + except urllib.error.URLError as exc: + return fail("Could not reach the Tailscale API", str(exc.reason)) + + print(f"Preflight OK: the credential authenticates against {TAILNET} (HTTP {status}).") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_ci_credential_wiring.py b/tests/test_ci_credential_wiring.py new file mode 100644 index 0000000..4082930 --- /dev/null +++ b/tests/test_ci_credential_wiring.py @@ -0,0 +1,199 @@ +"""Contract tests for the CI/CD credential wiring. + +Two things are pinned here: + +1. The response contract of Tailscale's ``POST /acl/validate`` endpoint, which + reports policy errors with **HTTP 200**. A status-code check alone passes + everything, so ``acl_validate.interpret`` is the load-bearing logic and it + cannot be exercised live from a test. +2. That ``validate`` (ci.yml) and ``deploy`` (cd.yml) read their credentials + from one scope and one variable set. Splitting them again is the specific + regression this repo already suffered. +""" + +import json +import os +import re +import sys +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +REPO_ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(REPO_ROOT / "scripts")) + +import acl_validate # noqa: E402 + + +class ValidateResponseContractTest(unittest.TestCase): + def test_empty_body_is_a_pass(self) -> None: + ok, problems = acl_validate.interpret(200, "") + self.assertTrue(ok) + self.assertEqual(problems, []) + + def test_empty_object_is_a_pass(self) -> None: + ok, problems = acl_validate.interpret(200, "{}") + self.assertTrue(ok) + self.assertEqual(problems, []) + + def test_message_on_http_200_is_a_failure(self) -> None: + # The trap: Tailscale reports grammar errors with a 200 status. + ok, problems = acl_validate.interpret( + 200, '{"message": "line 3, column 5: unknown action"}' + ) + self.assertFalse(ok) + self.assertIn("line 3, column 5: unknown action", problems) + + def test_data_errors_on_http_200_are_a_failure(self) -> None: + ok, problems = acl_validate.interpret( + 200, '{"data": [{"user": "alice", "errors": ["cannot reach tag:x"]}]}' + ) + self.assertFalse(ok) + self.assertIn("user alice: error: cannot reach tag:x", problems) + + def test_data_warnings_on_http_200_are_a_failure(self) -> None: + # Parity with tailscale.com/cmd/gitops-pusher, which fails on any + # non-empty data array. + ok, problems = acl_validate.interpret( + 200, '{"data": [{"user": "bob", "warnings": ["unused group"]}]}' + ) + self.assertFalse(ok) + self.assertIn("user bob: warning: unused group", problems) + + def test_unauthorized_is_a_failure(self) -> None: + ok, problems = acl_validate.interpret(401, '{"message": "API token invalid"}') + self.assertFalse(ok) + self.assertIn("API token invalid", problems) + self.assertIn("HTTP 401", problems) + + def test_non_2xx_with_empty_body_is_still_a_failure(self) -> None: + ok, problems = acl_validate.interpret(503, "") + self.assertFalse(ok) + self.assertEqual(problems, ["HTTP 503"]) + + def test_unparseable_body_is_a_failure(self) -> None: + ok, problems = acl_validate.interpret(200, "gateway") + self.assertFalse(ok) + self.assertTrue(any("unparseable" in p for p in problems)) + + def test_known_bad_policy_is_actually_malformed(self) -> None: + # --prove is only meaningful if the probe policy really is invalid. + rule = acl_validate.KNOWN_BAD_POLICY["acls"][0] + self.assertNotIn(rule["action"], {"accept", "check"}) + self.assertTrue(rule["src"][0].startswith("group:")) + self.assertIn("undefined", rule["src"][0]) + + +class ProveGateTest(unittest.TestCase): + """The --prove self-test decides whether a clean result may be trusted.""" + + def _run(self, responses: "list[tuple[int, str]]") -> "tuple[int, int]": + calls = {"n": 0} + + def fake_post(_bearer: str, _policy: dict) -> "tuple[int, str]": + response = responses[calls["n"]] + calls["n"] += 1 + return response + + with tempfile.TemporaryDirectory() as tmp: + policy_path = Path(tmp) / "policy.json" + policy_path.write_text(json.dumps({"acls": []}), encoding="utf-8") + with mock.patch.object(acl_validate, "post_validate", fake_post), mock.patch.object( + acl_validate, "resolve_bearer", lambda secret: "bearer" + ), mock.patch.object( + acl_validate, "GENERATED_POLICY", policy_path + ), mock.patch.dict( + os.environ, {"TAILSCALE_API_KEY": "tskey-api-stub"}, clear=False + ), mock.patch.object( + sys, "argv", ["acl_validate.py", "--prove"] + ): + return acl_validate.main(), calls["n"] + + def test_known_bad_rejected_then_real_policy_clean_passes(self) -> None: + code, calls = self._run( + [(200, '{"message": "group:... is not defined"}'), (200, "")] + ) + self.assertEqual(code, 0) + self.assertEqual(calls, 2) + + def test_known_bad_accepted_means_blind_and_fails(self) -> None: + # The endpoint said a deliberately invalid policy is fine. A clean + # result on the real policy would then be worthless. + code, calls = self._run([(200, "{}")]) + self.assertEqual(code, 1) + self.assertEqual(calls, 1, "must not go on to validate the real policy") + + def test_endpoint_absent_skips_without_failing(self) -> None: + code, calls = self._run([(404, '{"message": "404 page not found"}')]) + self.assertEqual(code, 0) + self.assertEqual(calls, 1) + + def test_auth_failure_during_self_test_fails(self) -> None: + code, calls = self._run([(401, '{"message": "API token invalid"}')]) + self.assertEqual(code, 1) + self.assertEqual(calls, 1) + + def test_real_policy_rejection_fails(self) -> None: + code, calls = self._run( + [ + (200, '{"message": "unknown action"}'), + (200, '{"message": "line 12, column 3: tag:nope is not defined"}'), + ] + ) + self.assertEqual(code, 1) + self.assertEqual(calls, 2) + + +class WorkflowScopeTest(unittest.TestCase): + CI = REPO_ROOT / ".github" / "workflows" / "ci.yml" + CD = REPO_ROOT / ".github" / "workflows" / "cd.yml" + + @classmethod + def setUpClass(cls) -> None: + cls.ci_text = cls.CI.read_text(encoding="utf-8") + cls.cd_text = cls.CD.read_text(encoding="utf-8") + + def test_both_jobs_declare_the_production_environment(self) -> None: + for name, text in (("ci.yml", self.ci_text), ("cd.yml", self.cd_text)): + with self.subTest(workflow=name): + self.assertIn("environment: production", text) + + def test_both_jobs_map_the_same_credential_pair(self) -> None: + secret = "TAILSCALE_API_KEY: ${{ secrets.TAILSCALE_API_KEY }}" + client_id = "TS_OAUTH_CLIENT_ID: ${{ vars.TS_OAUTH_CLIENT_ID }}" + for name, text in (("ci.yml", self.ci_text), ("cd.yml", self.cd_text)): + with self.subTest(workflow=name): + self.assertIn(secret, text) + self.assertIn(client_id, text) + + def test_credentials_are_mapped_exactly_once_per_workflow(self) -> None: + # Job-level env only. A per-step remap is how the two scopes drifted + # apart in the first place. + for name, text in (("ci.yml", self.ci_text), ("cd.yml", self.cd_text)): + with self.subTest(workflow=name): + self.assertEqual( + len(re.findall(r"secrets\.TAILSCALE_API_KEY", text)), 1 + ) + self.assertEqual( + len(re.findall(r"vars\.TS_OAUTH_CLIENT_ID", text)), 1 + ) + + def test_preflight_runs_in_both_workflows(self) -> None: + for name, text in (("ci.yml", self.ci_text), ("cd.yml", self.cd_text)): + with self.subTest(workflow=name): + self.assertIn("scripts/ci_preflight.py", text) + + def test_server_side_validation_runs_with_prove_in_both_workflows(self) -> None: + for name, text in (("ci.yml", self.ci_text), ("cd.yml", self.cd_text)): + with self.subTest(workflow=name): + self.assertIn("scripts/acl_validate.py --prove", text) + + def test_preflight_scope_string_matches_the_declared_environment(self) -> None: + import ci_preflight + + self.assertIn("production", ci_preflight.SCOPE) + + +if __name__ == "__main__": + unittest.main() From 4440c193309bcc795fd1ebeaaad7a1e04b229b6e Mon Sep 17 00:00:00 2001 From: Jess Sullivan Date: Sat, 22 Aug 2026 16:16:36 -0400 Subject: [PATCH 2/2] fix(ci): skip the PR comment when the build never ran The first run of this branch proved the preflight works -- and exposed a wart. `Diff summary` and `Comment on PR` carried a bare `if: always()`, so when the preflight fails before the Nix install they still ran, and posted a PR comment whose entire content was: /home/runner/work/_temp/....sh: line 2: nix: command not found Gate both on `steps.build.outcome == 'success'`. Fast-failing on a dead credential now leaves the PR clean instead of adding noise that reads like a policy problem. Pinned by a contract test that also refuses a bare `if: always()` in ci.yml. --- .github/workflows/ci.yml | 8 ++++++-- tests/test_ci_credential_wiring.py | 9 +++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b189b72..3bf44f8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -117,6 +117,7 @@ jobs: - uses: DeterminateSystems/magic-nix-cache-action@main - name: Build policy + id: build run: nix develop --command just build # Server-side grammar check. push.py --dry-run only diffs locally; this @@ -149,8 +150,11 @@ jobs: exit "$EXIT_CODE" + # Only meaningful once the toolchain and policy exist. When the preflight + # fails fast, Nix is never installed and these steps would otherwise post + # a PR comment whose entire content is "nix: command not found". - name: Diff summary - if: always() + if: always() && steps.build.outcome == 'success' id: diff run: | set +e @@ -162,7 +166,7 @@ jobs: } >> "$GITHUB_OUTPUT" - name: Comment on PR - if: always() + if: always() && steps.build.outcome == 'success' uses: actions/github-script@v7 with: script: | diff --git a/tests/test_ci_credential_wiring.py b/tests/test_ci_credential_wiring.py index 4082930..22ae7c4 100644 --- a/tests/test_ci_credential_wiring.py +++ b/tests/test_ci_credential_wiring.py @@ -189,6 +189,15 @@ def test_server_side_validation_runs_with_prove_in_both_workflows(self) -> None: with self.subTest(workflow=name): self.assertIn("scripts/acl_validate.py --prove", text) + def test_pr_comment_steps_are_gated_on_a_successful_build(self) -> None: + # A bare `if: always()` here posts a PR comment whose whole content is + # "nix: command not found" whenever the preflight fails before Nix is + # installed, which is now the common failure mode. + self.assertEqual( + len(re.findall(r"steps\.build\.outcome == 'success'", self.ci_text)), 2 + ) + self.assertNotIn(" if: always()\n", self.ci_text) + def test_preflight_scope_string_matches_the_declared_environment(self) -> None: import ci_preflight