feat(acl): grant node_exporter egress proxies scrape access (TIN-3970) - #22
feat(acl): grant node_exporter egress proxies scrape access (TIN-3970)#22Jesssullivan wants to merge 1 commit into
Conversation
The Tailscale k8s operator creates egress proxy devices for the
ExternalName Services in tinyland-staging that front node_exporter on
relay-1 and petting-zoo-mini. Those proxies inherit the operator's
PROXY_TAGS (tag:k8s), and no ACL permits tag:k8s to reach tag:dollhouse
or tag:dev, so their flows are dropped. Verified live 2026-08-22:
ts-relay-node-exporter-egress: flow TCP 100.65.23.86:35180 >
100.102.229.122:9100 rejected due to acl
ts-pzm-node-exporter-egress: flow TCP 100.69.142.65:51002 >
100.111.5.80:9100 rejected due to acl
Rather than widen tag:k8s, which 85 devices hold including 23 ephemeral
GitHub runner VMs, introduce a dedicated identity for these proxies,
mirroring the tag:mcp-proxy precedent from TIN-2940:
* tag:k8s-egress-nodeexporter, owned by tag:k8s-operator so the
operator can mint it for the proxies it creates
* a grant scoped to tcp:9100 on three pinned host aliases
tinyland-neo is included to prevent a regression. The neo egress proxy
reaches neo today only because neo itself holds tag:k8s and the
tag:k8s to tag:k8s rule covers it; once that proxy moves to the
dedicated tag it would lose the path without this grant.
Reachability is expressed only as a port-scoped grant, never as an ACL
src, so it cannot silently widen past tcp:9100. Contract tests pin the
owner, the port, the host IPs, and the absence of a broad ACL rule; all
four were mutation-tested to confirm they can fail.
This is the tailnet half. The proxies do not carry the new tag yet;
the blahaj-side Service annotations are a required follow-up.
Refs: TIN-2626, TIN-3970
Reviewer note: residual risk — no server-side policy validation was possible locallyFlagging this explicitly rather than letting it be discovered at deploy time. The gap. What I tried. Both local credential paths failed:
The specific construct at risk. This grant is the repo's first use of a host alias in a grant "dst": ["tinyland-relay-1", "tinyland-petting-zoo-mini", "tinyland-neo"]The existing Why I still believe it is correct. Grants and ACL rules share one destination grammar — grants just move the port out of Suggested gate before merge — run once with a credential that has ACL read/write: nix develop --command just build
python3 - <<'PY'
import os, sys, json, urllib.request, urllib.error
sys.path.insert(0, "scripts")
from ts_auth import resolve_bearer
tok = resolve_bearer(os.environ["TAILSCALE_API_KEY"])
req = urllib.request.Request(
"https://api.tailscale.com/api/v2/tailnet/taila4c78d.ts.net/acl/validate",
data=json.dumps(json.load(open("generated/policy.json"))).encode(),
method="POST")
req.add_header("Authorization", f"Bearer {tok}")
req.add_header("Content-Type", "application/json")
try:
with urllib.request.urlopen(req) as r:
print("HTTP", r.status, "->", r.read().decode() or "(empty = VALID)")
except urllib.error.HTTPError as e:
print("HTTP", e.code, "->", e.read().decode()[:1000])
PYAn empty 200 body means the policy parses. If it rejects host aliases in a grant Worth fixing separately (out of scope here): that this repo auto-deploys on merge while its only pre-merge check is a local diff is a structural gap, not specific to this PR. Adding an |
|
Two findings from verification, both pre-existing and neither caused by this PRRaising these here because the first one blocks this PR from actually deploying, and the second is direct evidence that the follow-up half of this change is the step that historically gets dropped. 1. BLOCKING — the ACL deploy credential looks expired; merging will not deployThe This is not the advisory "local policy differs from live" path — the workflow catches that string and downgrades it to a warning. This is a hard auth failure before any diff happens. It is new, and not mine. The same check passed on the previous two PRs:
Last successful CD run was 2026-07-27. Admin API keys cap at 90 days, so an expiry in the intervening ~4 weeks fits exactly. Why this blocks the merge. Evidence the secret is a direct key, not the OAuth client secret. Suggested fix, in order:
2. The
|
Jesssullivan
left a comment
Jesssullivan
left a comment
There was a problem hiding this comment.
Adversarial review — CHANGES NEEDED
Posted as a review comment: GitHub refuses
REQUEST_CHANGESfrom the PR author’s own token. Treat this as a blocking review — do not merge until items 1–4 below are cleared.
Reviewed from an independent detached worktree at 34271d7, rebuilt with the repo's own tooling, and cross-checked against the live tailnet and the live cluster. Not an approval — the operator approves.
Verdict: the diff is clean and correctly scoped, but the change is a no-op on merge, and the only path that makes it work is undocumented here and fleet-breaking if done the obvious way. Two of the PR's stated findings are also mis-stated in a way that would cause harm if acted on.
1. Reproduction — delta confirmed, no smuggled changes
Rebuilt generated/policy.json at a44dc90 (main) and at 34271d7 (head) with just build, using the flake toolchain (dhall 1.42.3, dhall-json 1.7.12). The diff is exactly what the PR claims and nothing more:
tagOwners 25 -> 26 + tag:k8s-egress-nodeexporter: [tag:k8s-operator, autogroup:admin, group:dollhouse-admins]
grants 9 -> 10 + {src:[tag:k8s-egress-nodeexporter], dst:[tinyland-relay-1, tinyland-petting-zoo-mini, tinyland-neo], ip:[tcp:9100]}
hosts 6 -> 9 + tinyland-neo / tinyland-petting-zoo-mini / tinyland-relay-1
acls 41 -> 41 (unchanged)
ssh 8 -> 8 (unchanged)
groups/nodeAttrs/autoApprovers unchanged
dhall type OK · dhall format --check exit 0 · 7/7 contract tests pass · structural validation passes. Independently confirmed.
Host pinning is correct. I resolved all three against live MagicDNS:
| alias | pinned | live | device tags |
|---|---|---|---|
tinyland-relay-1 |
100.102.229.122 | ✅ relay-1.taila4c78d.ts.net |
tag:dollhouse |
tinyland-petting-zoo-mini |
100.111.5.80 | ✅ | tag:dev, tag:dollhouse |
tinyland-neo |
100.67.93.34 | ✅ | includes tag:k8s |
Root cause in the PR body is correct. Verified live: all three egress proxies (tinyland-staging-{neo,pzm,relay}-node-exporter-egress) carry exactly ['tag:k8s']; relay-1 and pzm do not hold tag:k8s; ACL rule #8 is tag:k8s -> tag:k8s:*, which is precisely why neo scrapes and the other two don't. Rejecting src=[tag:k8s] was right — census says 85 devices hold it.
Including tinyland-neo in dst is not speculative — it is required. Once a proxy is retagged it loses tag:k8s and therefore loses rule #8, so without neo in dst the retag would break the one target that works today. Correct call.
Public-repo hygiene: clean. Only 100.64/10 CGNAT addresses (9) plus 10.0.0.0 / 192.168.0.0 network placeholders. No key material, no credentials, no external hostnames. gitleaks green.
Dirty-tree disposition: verified, claim holds. wip/dirty-worktree-20260822 = b22420e, GPG-signed (G), on origin, parented at 15d37c9 on fix/emergency-ssh-admin-access. 13 paths (10 tracked-modified + generated/policy.json + tofu/main.tf + tofu/variables.tf) — matches the report. Nothing was lost.
2. BLOCKING — on merge this changes nothing, and the follow-up is a fleet hazard
Live tag census across the whole tailnet:
tag:k8s-egress-nodeexporter 0 devices
tag:mcp-proxy 0 devices
Merging this alters zero reachability. That alone would just be "part one of two". The problem is what part two actually requires:
- Operator is
tailscale/k8s-operator:v1.94.2withPROXY_TAGS = tag:k8s— a single, global env var on the operator Deployment. - All 27 tailscale-managed Services in the cluster share the one ProxyClass
honey-sting-tailnet:ingress-nginx-controller,massageithaca-prod,nix-cache/bazel-cache-grpc,gf-rbe/gf-reapi-cell, the entire LGTM stack (loki/grafana/mimir/tempo/pyroscope/otlp), all fivemcp-services, and the three node_exporter egress proxies. - I checked the live
proxyclasses.tailscale.comCRD:ProxyClassv1alpha1 has notagsfield. There is no per-class lever. The only per-proxy lever is thetailscale.com/tagsService annotation.
So the two available paths are:
- Change
PROXY_TAGS— the obvious move, and it is catastrophic. 85 devices losetag:k8s, ACL rule #8 (tag:k8s -> tag:k8s:*) collapses fleet-wide, and every ingress above goes dark. - Annotate only the three egress Services with
tailscale.com/tags: tag:k8s-egress-nodeexporter— the correct path, but this PR never states it, and its behavior on egress Services (type: ExternalName+tailscale.com/tailnet-fqdn, notailscale.com/expose) is not demonstrated anywhere in this PR. Those Services also live in a different repo, so nothing links the two halves.
The PR contains no warning about path 1. Given the PR's own evidence that part two gets dropped here — tag:mcp-proxy has been inert since TIN-2940 landed 2026-07-16, confirmed 0 devices — shipping the ACL half with an unstated, fleet-breaking obvious-path is the thing to fix before merge.
To clear this: state the annotation explicitly in the PR body (or a repo note), prove tailscale.com/tags is honored on one egress Service on v1.94.2 before relying on it, add an explicit "do not touch PROXY_TAGS" warning, and link the manifest change so part two is tracked rather than remembered.
3. Finding #1 in the PR body is misdiagnosed — and acting on it as written would break CD
The PR concludes from the CI 401 that "the deploy credential appears expired… merging yields a failed CD run". CI and CD do not read the same secret.
TAILSCALE_API_KEY is defined twice:
| scope | created | updated |
|---|---|---|
| repo-level | 2026-02-24T18:43:17Z | 2026-06-24T21:39:43Z |
production environment |
2026-02-24T18:43:31Z | 2026-06-24T21:39:41Z |
ci.yml's validate job has no environment:, so it reads the repo-level secret. cd.yml's deploy job has environment: production, so the environment secret overrides it. The 401 you observed is evidence about the repo-level secret only. It is not evidence about the credential CD uses. (They were co-rotated 2s apart and are plausibly the same value — but that was never established, and the PR reasons as if the two scopes were one.)
And the implied remediation is a live landmine. cd.yml passes TS_OAUTH_CLIENT_ID: ${{ vars.TS_OAUTH_CLIENT_ID }}. I checked both scopes:
repo variables: {"variables": [], "total_count": 0}
production env variables: {"variables": [], "total_count": 0}
TS_OAUTH_CLIENT_ID is set nowhere. So the non-expiring-OAuth path built by #15/#16 has never actually been exercised — every CD run since has succeeded because the secret is a direct tskey-api- key that resolve_bearer passes through untouched. If someone reads "rotate before merging" and drops in a tskey-client- OAuth secret, ts_auth.resolve_bearer raises RuntimeError, push.py returns 1, and CD hard-fails — the exact silent non-deploy the PR is warning about, caused by the fix.
Do this instead: read the actual expiry off the Tailscale admin keys page rather than inferring it from a 401 in a different secret scope. If rotating, either mint a fresh tskey-api- key, or set the TS_OAUTH_CLIENT_ID repo variable first and only then swap in the client secret.
Separately, ci.yml is structurally wrong and will stay red even after a correct rotation. Its validate job never maps TS_OAUTH_CLIENT_ID at all. Post-OAuth-rotation it would raise RuntimeError, which does not emit the Validation FAILED: local policy differs from live. string the workflow greps for, so it falls through to exit "$EXIT_CODE" = 1. Fix ci.yml to mirror cd.yml (environment: production + TS_OAUTH_CLIENT_ID) so the two stop diverging.
4. Scope analysis of the grant — tight, with two caveats
Attacked for over-breadth; it holds up.
srcbreadth.tag:k8s-egress-nodeexporteris mintable only bytag:k8s-operator,autogroup:admin,group:dollhouse-admins— same owner set astag:mcp-proxy. Tailscale tags do not inherit, so no other tag picks this up transitively. Bounded and correct.dstbreadth. Three host aliases, each a bare/32(no CIDR, no wildcard, noautogroup:). Grants matchdstby IP, not by identity — if a device is deleted and its 100.x address later recycled, the grant silently follows the address. Low probability; worth knowing.ipbreadth.tcp:9100only. No UDP, no ranges, no*. Grants are independent ofacls, so this cannot be widened by an ACL rule elsewhere.- Net effect is a narrowing. Today the neo path runs through
tag:k8s -> tag:k8s:*— all ports to 85 devices. After retag it becomes tcp:9100 to three hosts. Strictly better. - No future-device surprise beyond the intended semantic: anything the operator tags with this gets tcp:9100 to those three, nothing else.
Grant-dst grammar — better evidence than "unproven". The live policy already carries {"src":["tinyland-honey"], "dst":["tag:mcp-proxy"], "ip":["tcp:8080"]}, in which a host alias is used in grant src and was accepted by a real POST /acl (CD run for #18, 2026-07-16, success). Host aliases are therefore proven valid in grant selectors on this tailnet; dst symmetry is near-certain. Downgrade that residual-risk note.
MagicDNS drift trap (pre-existing, worth an issue). relay-1 is a collision-suffixed name: there is a stale device relay at 100.114.248.77, untagged, offline since 2026-04-10. If that stale device is deleted, Tailscale frees the base name and today's relay-1 can become relay. The ACL is safe (it pins the IP), but the egress Service is annotated tailnet-fqdn: relay-1.taila4c78d.ts.net — that name is unstable, so the proxy would silently repoint while the grant kept pointing at 100.102.229.122. Reaping the dead relay device is the cheap fix.
5. CD safety on merge
Answering "could a bad policy lock the operator out": not from this diff. It touches no ssh rule, no admin ACL, and is purely additive; Tailscale rejects unparseable policies server-side before applying. The systemic risks are real but pre-existing:
push.pydoes a full-policyPOSTwith noIf-Match/ETag — a blind overwrite of live.- CD's
Validate against live ACLiscontinue-on-error: trueandShow diffends in|| true, so drift is never a gate anywhere in the pipeline. If live has been hand-edited in the admin console since the last CD success (2026-07-27,a44dc903), merging silently reverts it — and right now nobody can tell, because validation can't authenticate. - CI's
validateis advisory by construction, not a gate: it catches theValidation FAILEDstring and downgrades to a warning. The only thing that reddens it is a hard error — which is why the 401 is visible at all.
Adding If-Match and making CD's validate blocking on drift (while staying advisory on expected diff) is the durable fix. Out of scope for this PR; worth an issue.
6. Test quality — one weak assertion
test_node_exporter_egress_tag_gets_no_broad_acl_access compares whole src lists:
srcs = [r["src"] for r in self.policy["acls"]]
self.assertNotIn(["tag:k8s-egress-nodeexporter"], srcs)This only catches a rule whose src is exactly that one-element list. An ACL rule with src: ["tag:dollhouse", "tag:k8s-egress-nodeexporter"] grants the broad access the test exists to forbid and the test still passes. Make it a membership check:
self.assertFalse(any("tag:k8s-egress-nodeexporter" in r["src"] for r in self.policy["acls"]))The other three new tests are sound regression guards and I confirmed they fail under mutation. Note they pin repo state against literals — they cannot detect the tailnet drifting out from under the pinned IPs; only a live check can, which is the gate that's currently broken.
Summary — what to change before merge
- State the part-two mechanism in the PR: the per-Service
tailscale.com/tagsannotation on the three egress Services, plus an explicit "do not changePROXY_TAGS" warning (85 devices, all 27 proxies, one shared ProxyClass, notagsfield on ProxyClass v1alpha1). - Prove
tailscale.com/tagsis honored on an egress Service on operator v1.94.2 before depending on it. - Link part two so it can't go the way of
tag:mcp-proxy. - Correct finding #1: CI reads the repo-level secret, CD reads the
productionenvironment secret. Check the real expiry in the Tailscale admin console. If rotating to OAuth, setTS_OAUTH_CLIENT_IDfirst — it is currently unset in both scopes and CD will hard-fail without it. - Fix
ci.yml'svalidatejob to mirrorcd.ymlso CI and CD stop authenticating differently. - Tighten
test_node_exporter_egress_tag_gets_no_broad_acl_accessto a membership check.
Items 1–4 are pre-merge. 5, 6 and the If-Match / dead-relay-device items can be follow-ups.
The Dhall, the generated delta, the host pinning, the grant scope, and the WIP preservation are all correct and independently verified. The objection is to landing this as a completed fix while the half that does the work is unstated, unproven, and one obvious wrong move away from taking down every ingress on the cluster.
Part 2 is up: tinyland-inc/tinyland.dev#821The review here flagged that this grant matches zero devices as written, because nothing tags the egress proxies. That half now exists: https://github.com/tinyland-inc/tinyland.dev/pull/821 adds Both PRs are held. This one merges first — see ordering below. The annotation mechanism is now proven, not assumedThe open question was whether
Two things the review should know1. Ordering is load-bearing, and this PR is the hard prerequisite. The operator applies tags by minting a pre-authorized auth key naming them. The Tailscale API rejects a key requesting a tag that does not exist or is not owned by 2. Tagging is create-time only — neither PR is self-executing. Merging both PRs and applying still leaves the devices on Sequence
Nothing has been merged or applied. Refs TIN-2626, TIN-3970. |
Blocked on a credential-wiring problem, not on this PR — see #23This PR's Investigating that turned up a wiring trap worth fixing before this merges:
#23 puts both jobs on one scope and one variable set, adds a fail-fast Suggested order: merge #23 → rotate the credential into the Nothing in #23 touches |
Status update: #23 merged, credential rotation still pending — no code change needed here#23 merged 2026-08-22 20:31 UTC (unifies both jobs onto the Checked whether the operator has rotated Unchanged — still the same value that returned 401 in both this PR's run Nothing pending here is a code change. The remaining step is purely |
|
Hygiene-sweep check on the #23 follow-through: rotation is still pending.
Per #23's own rotation table, both are still the dead Precisely what's pending before this can go green and merge, per #23:
Not rotating this myself (operator credential action). No merge action taken here — holding per #23's stated order (#23 → rotate → green #22 → merge #22). |
1 participant
Problem
The Tailscale k8s operator creates egress-proxy devices for the
ExternalNameServices intinyland-stagingthat frontnode_exporteronrelay-1andpetting-zoo-mini. Their flows are dropped by ACL. Verified live on the cluster, 2026-08-22:Root cause
The operator assigns proxies its
PROXY_TAGS, which istag:k8s:All three egress proxies therefore carry exactly
['tag:k8s']. The destinations do not:relay-1100.102.229.122tag:dollhousepetting-zoo-mini100.111.5.80tag:dev,tag:dollhouseneo100.67.93.34tag:dev,tag:dollhouse,tag:k8s,tag:k8s-operator, …No rule permits
tag:k8s→tag:dollhouseortag:dev. Thetag:k8s-operatorrule infragments/kubernetes.dhalldoes reach those tags, but it applies to the operator pod itself, not to the proxies it creates.Why neo already passes: neo holds
tag:k8sitself, so the existingtag:k8s → tag:k8s:*rule covers it. It is not evidence that the policy is correct — it is a coincidence of neo's tag set.Why not simply allow
tag:k8stag:k8sis held by 85 devices, including 23 ephemeralgithub-runnervmeorf1-*CI runners, every MassageIthaca PR preview, and every MCP proxy. Granting that set scrape access to lab hosts is the broad allow this repo has repeatedly refused (cf.test_honey_does_not_receive_broad_kubernetes_acl_access).Instead this follows the
tag:mcp-proxyprecedent from TIN-2940 / #18: a dedicated tag for operator-created proxies, owned bytag:k8s-operatorso the operator can mint it, plus a port-scoped grant rather than an ACL rule.Generated JSON delta
generated/policy.json, built withnix develop --command just build. This is the complete diff — no ACL rules change (41 before, 41 after):Counts:
tagOwners 25 → 26,grants 9 → 10,hosts 6 → 9,acls 41 → 41,ssh 8 → 8.Scope rationale
ip: ["tcp:9100"]— TCP only, single port.node_exporteris read-only metrics.tag:dollhouseortag:devdevice.dstashost:portand is easy to widen to:*in a later edit. A grant carries the port in a separateipfield that a contract test pins.test_node_exporter_egress_tag_gets_no_broad_acl_accessfails if the tag ever appears as an ACLsrc.Why
tinyland-neois in the grantNot a new capability — the neo proxy can already reach
neo:9100today viatag:k8s → tag:k8s. It is included to prevent a regression: once the blahaj follow-up moves that proxy ontotag:k8s-egress-nodeexporter, it losestag:k8sand would lose the path. Omitting it would break a currently-working (if unproductive — see below) route.Required follow-up — this PR alone does not flip the metrics
The proxies do not carry the new tag yet. The tailnet side must land first so the tag exists and is owned by
tag:k8s-operator; only then can the operator mint it.The egress Services are not in blahaj. They live in the
tinyland.devrepo:infra/staging/observability-tailnet-services.yaml— three Services, each needing one added annotation:relay-node-exporter-egressneo-node-exporter-egresspzm-node-exporter-egressThe operator then recreates the proxy devices with the new tag and the grant takes effect.
Note this is a Service annotation, not a ProxyClass field — the
honey-sting-tailnetProxyClass these Services reference carries only scheduling config (statefulSet.podaffinity/tolerations/spread), and the ProxyClass CRD has no tags field. Tagging is per-Service.Apply all three together:
neo-node-exporter-egressmust be annotated in the same change, or it keepstag:k8swhile the others move, leaving the fleet inconsistent.Verification plan
Current baseline, from inside the prometheus pod:
After this PR and the blahaj annotation:
up{job="node-relay"}→ 1. High confidence. From an ACL-permitted device,relay-1:9100is open and serving genuine node_exporter — 450node_*metric lines. Only the ACL is in the way.up{job="node-pzm"}→ exporter state becomes verifiable. The ACL block is removed, but a probe from an ACL-permitted device gets connection refused on100.111.5.80:9100, which indicates no listener. Expect this job to stay at 0 and need a pzm-sidenode_exporterdeployment. That is a host-config gap, not an ACL gap.up{job="node-neo"}→ unchanged at 0, and this PR does not claim to fix it. neo's 9100 is held bytcfsdbound to127.0.0.1only, so tailnet peers get RST. It is also not node_exporter. Thenode-neoscrape job is misconfigured independently of the tailnet policy and needs a separate fix in the owning repo.Post-merge the CD workflow pushes the policy; confirm the live diff matches the delta above, then re-check the proxy logs for absence of
rejected due to acl.Validation evidence
All run locally against the repo's own toolchain via
nix develop:dhall type --file policy.dhallfind . -name '*.dhall' -exec dhall format --check {} +just buildgenerated/policy.json, delta as quotedpython3 -m unittest discover -s testsThe four new contract tests were mutation-tested — each was re-run against a deliberately corrupted
generated/policy.jsonto prove it can fail rather than passing vacuously:tag:k8s-operatorfrom the tag's ownersiptotcp:*tinyland-relay-1to10.0.0.1srcRestored, all 7 pass.
One authoring note for future edits: dhall 1.42.3's formatter discards every comment except the file header, so the provenance comments live in the
core.dhall/kubernetes.dhallheaders. Inline comments inside list literals are silently dropped and then fail CI'sdhall format --check.Dirty-tree disposition — set aside, not incorporated
The worktree was dirty on arrival, but not with a half-finished version of this grant, so per the operator ruling it was preserved untouched rather than folded in.
It was 10 modified files (not 5) on branch
fix/emergency-ssh-admin-access, 7 commits behindorigin/main, plus untrackedgenerated/andtofu/. The edits would have reverted merged work: guttingfragments/kubernetes.dhallandfragments/core.dhall, deletingscripts/ts_auth.pyandtests/, and re-adding thetofu/path that #9 deliberately removed.Preserved verbatim as a signed commit on
wip/dirty-worktree-20260822(b22420e), pushed to origin. Nothing was stashed or discarded; this branch is available for operator triage. This PR then branched from a cleanorigin/main(a44dc90).Not merging
Held for the ratified adversarial-review gate: agent review, then operator review.
Refs: TIN-2626, TIN-3970