Repository navigation
Close tokenless remote admin, and make the safe tailnet topology installable on macOS - #181
Closed
discordwell wants to merge 4 commits into
Closed
discordwell wants to merge 4 commits into
discordwell wants to merge 4 commits into
Conversation
authorizeAdmin's fallback granted every REMOTE caller the full admin surface — accounts, transcripts, account-import, drain — whenever neither AdminToken nor AccountImportToken was configured. That is the default state of a bare `subrouter serve`, so the moment such a daemon bound a non-loopback address (the tailnet bind that team sharing calls for, where this proxy deliberately allows plaintext HTTP), every host the ACLs admit held admin. Two layers close it: serve now refuses a non-loopback --addr with no admin token, at startup, naming the fixes: --admin-token / SUBROUTER_ADMIN_TOKEN[_FILE], a loopback bind, or the new --allow-unauthenticated-admin opt-in for a deliberately unsecured deployment. Unparseable addresses are left for net.Listen to report; hostnames are treated as non-loopback because their resolution cannot be trusted at validation time. authorizeAdmin itself now grants tokenless REMOTE admin only behind the same explicit opt-in, so no other construction of Server can fail open. The one legitimate consumer of the old fallback — per-tenant scoped servers, where the MultiTenant router has already validated the tenant key before routing only the tenant-visible control paths — carries that delegation through the new internal tenantControlAuthorized, mirroring tenantAccountImportAuthorized. Loopback stays unconditionally admin (it is how the sr CLI works), and the spoofed-forwarded-header case is pinned: the socket peer decides, X-Forwarded-For cannot claim loopback. Tests that drove admin handlers through httptest.NewRequest were riding the open fallback unnoticed — its default peer, 192.0.2.1, is remote. They now say which peer they mean via loopbackAdminRequest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…min token `serve` has allowed a non-loopback bind since e7839e5, on the condition that an admin token is configured -- `bindAddrRequiresAdminToken` refuses to start otherwise. `install-systemd` mirrors that with its own --admin-token. macOS `install-daemon` did not: it rejected every address that was not localhost, so the one topology this is for -- serving the pool over a tailnet -- was reachable on Linux and unreachable on a Mac, even though the daemon it installs would have run it safely. Reuse serve's own predicate rather than writing a second spelling of "is this loopback". Two gates that decide the same question from different code is how C1 happened: authorizeAdmin granted full admin to any remote caller whenever no token was set, which is exactly the state a bare `serve` starts in. The token reaches the daemon two ways, and the difference is not cosmetic: --admin-token-file writes SUBROUTER_ADMIN_TOKEN_FILE; the secret stays in a file the operator controls and the plist stays 0644. --admin-token writes SUBROUTER_ADMIN_TOKEN inline, and the plist is then chmod 0600. Neither puts the token in ProgramArguments. launchd argv shows up in `ps` for every local user, so an argv token would publish the admin credential to the audience the token exists to gate. TestLaunchAgentPlistKeepsAdminTokenOutOf ProgramArguments pins that. Tests cover the refusal across tailnet IPv4 / MagicDNS / bare :31415, both loopback spellings still installing untokened, both token forms, their mutual exclusion, and the argv leak. Verified in the container only (./deploy/docker/test.sh) -- ./internal/proxy and ./cmd/subrouter green. Nothing was run against this host's daemon.
…undary
The tailnet lane had no onboarding doc, and the README described the macOS
daemon as localhost-only -- true until the previous commit, misleading now.
The load-bearing paragraph is the one saying the tailnet is not the security
boundary. Tailscale decides who can reach the port; it does not decide what
they may do there, and Subrouter deliberately does not treat node identity as
authorization. Loopback is the only implicitly trusted peer. That distinction
is the whole of C1: tokenless remote admin was once the silent default, so a
tailnet bind published accounts, transcripts and account-import to every host
the ACLs admitted. Someone reasoning "it's on my tailnet, so it's fine" is
reconstructing that bug from first principles, which is why the doc argues
the point rather than just listing flags.
Claims here were checked against the code, not inferred:
- drain 403s a non-loopback peer even WITH a valid admin token
(handleDrain re-checks isLoopbackRemote inside the handler, after
requireAdmin) -- so `serve`'s own flag help, which lists drain among what
a token gates, overstates it slightly.
- requireAdmin returns 401, so a stale client token fails loudly rather
than degrading to unauthenticated.
- secretFromEnvironment treats both SUBROUTER_ADMIN_TOKEN and
SUBROUTER_ADMIN_TOKEN_FILE being set as fatal, which is what makes the
two install flags mutually exclusive rather than merely redundant.
- the flag matrix is real: --admin-token-file is macOS-only,
--admin-token-stdin is systemd-only.
Recommends binding the 100.x address rather than a MagicDNS name: the
listener resolves once at bind time and a LaunchAgent can beat tailscaled's
MagicDNS to it. KeepAlive means that race self-heals, so the doc also gives
the tell that separates it from a genuine bind failure -- restart churn in
subrouter.err.log that stops on its own versus churn that does not.
📝 WalkthroughWalkthroughThe daemon now requires an admin token for non-loopback binds unless explicit unauthenticated access is enabled. Install commands support inline or file-based tokens. Proxy authorization distinguishes loopback, admin-token, tenant, and unauthenticated access. ChangesAdmin authentication and daemon exposure
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant Daemon
participant ProxyServer
participant AdminEndpoint
Operator->>Daemon: start with listen address and token settings
Daemon->>ProxyServer: configure admin authentication
ProxyServer->>AdminEndpoint: receive control request
AdminEndpoint-->>ProxyServer: authorize loopback, token, tenant, or explicit opt-in
ProxyServer-->>Operator: return control response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/tailscale.md`:
- Line 11: Update the authorizeAdmin description in the documentation to remove
/_subrouter/account-import and /_subrouter/drain from the remote admin-token
paths, state that account import uses its separate scoped token, and state that
drain is loopback-only regardless of token.
- Around line 34-38: Update the inline-token example in the daemon installation
instructions to define TOKEN before invoking install-daemon. Generate or assign
TOKEN using the documented token-generation command, while preserving the
existing --admin-token "$TOKEN" usage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d78303b6-ddcd-4aea-83d5-9c586f87d8d6
📒 Files selected for processing (13)
README.mdcmd/subrouter/install_daemon.gocmd/subrouter/install_daemon_test.gocmd/subrouter/main.gocmd/subrouter/serve_admin_gate_test.godocs/tailscale.mdinternal/proxy/admin_gate_test.gointernal/proxy/dashboard_test.gointernal/proxy/multitenant.gointernal/proxy/multitenant_test.gointernal/proxy/proxy.gointernal/proxy/proxy_websocket_test.gointernal/proxy/rate_limit_reset_test.go
|
|
||
| Tailscale decides which hosts can *reach* the port. It does not decide what they can *do* once they reach it, and Subrouter does not treat tailnet membership as authorization. | ||
|
|
||
| `authorizeAdmin` trusts exactly one thing implicitly: a loopback peer. Every other caller — including one that is unambiguously on your tailnet, with a valid node key, admitted by your ACLs — must present the admin token to touch `/_subrouter/accounts`, `/_subrouter/transcripts`, `/_subrouter/account-import`, or `/_subrouter/drain`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the remote authorization description.
Line 11 lists /_subrouter/account-import and /_subrouter/drain as remote admin-token paths. Line 64 gives account import its separate scoped token. Line 68 makes drain loopback-only regardless of token. Remove both endpoints from this admin-token statement and state their separate restrictions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/tailscale.md` at line 11, Update the authorizeAdmin description in the
documentation to remove /_subrouter/account-import and /_subrouter/drain from
the remote admin-token paths, state that account import uses its separate scoped
token, and state that drain is loopback-only regardless of token.
Comment on lines
+34
to
+38
| The alternative form puts the token inline in the plist's `EnvironmentVariables`, and the installer then writes the plist `0600` instead of `0644`: | ||
|
|
||
| ```bash | ||
| ./bin/subrouter install-daemon --addr 100.x.y.z:31415 --admin-token "$TOKEN" | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Define $TOKEN before the inline-token example.
The preceding instructions create a token file but do not assign $TOKEN. This command expands to an empty token in a new shell and the installer rejects the non-loopback bind. Add a token-generation assignment before this example.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/tailscale.md` around lines 34 - 38, Update the inline-token example in
the daemon installation instructions to define TOKEN before invoking
install-daemon. Generate or assign TOKEN using the documented token-generation
command, while preserving the existing --admin-token "$TOKEN" usage.
Review of the previous commit turned up two ways it fails late instead of early, both specific to the token-file form. **An unreadable token file installed cleanly.** Nothing checked that the path existed, was a file, or held a non-empty secret. Install succeeded, launchd started the daemon, secretFromEnvironment failed, serve exited -- and because the plist sets KeepAlive, that is not one legible failure but an endless restart loop whose only symptom is churn in subrouter.err.log. The operator gets no message connecting it to the flag they just passed. **A relative path silently pointed somewhere else.** launchd resolves it against the LaunchAgent's WorkingDirectory, not the shell that ran the installer, so `--admin-token-file admin-token` reads a different file or none. The plist now always carries an absolute path. The readability check runs through the *same* readSecretFile the daemon uses, extracted from secretFromEnvironment for the purpose. An installer that validated its own way could accept a file serve then rejects, which is the same two-gates-one-question shape as the tokenless-remote-admin bug this branch fixes -- worth not reintroducing one commit later. Also warns when the token file is group- or world-readable. Not fatal, since an operator may have their reasons, but readable-by-everyone gives back the only thing --admin-token-file buys over --admin-token, so it should not pass in silence. Three tests added, and the existing ones now write real temp files instead of asserting against a path that merely looked plausible. Negative-controlled: with the new check stubbed out, TestValidateDaemonConfigRejectsUnusableAdmin TokenFile fails, so it is load-bearing rather than decorative. Container-only verification; ./cmd/subrouter and ./internal/proxy green.
Author
|
Added a fourth commit after re-reviewing the token-file path. Two ways it failed late rather than early, both worth knowing about if you review the flag surface:
The install-time check runs through the same Negative-controlled: with the new check stubbed out the test fails, so it is load-bearing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sharing one Subrouter across a few machines over a tailnet was documented as a supported shape and was, in practice, either unsafe or impossible depending on the OS. This makes it safe on both, and writes down the trust boundary so the unsafe version does not get rebuilt by someone reasoning from the tailnet inward.
What changes for an operator
A tailnet bind no longer silently publishes the admin surface. Previously a daemon bound to a tailnet address with no admin token handed every reachable host the accounts, transcripts, account-import and drain endpoints — and that was the default state of a bare
serve, not a misconfiguration you had to opt into.servenow refuses to start in that state.macOS can now serve the pool at all.
install-daemonrejected every non-loopback address, so the topology was reachable on Linux viainstall-systemdand simply unavailable on a Mac — even though the daemon it installs would have run it safely. It now accepts a tailnet address and requires a token, matching whatservealready enforced.The token stays out of places that leak it.
--admin-token-filekeeps it out of the plist entirely; the inline--admin-tokenform writes the plist0600instead of0644. Neither puts it inProgramArguments, because launchd argv is readable throughpsby every local user on the box — precisely the audience the token exists to exclude.Why the doc argues rather than lists
docs/tailscale.mdspends its first section on a single claim: the tailnet is not the security boundary. Tailscale decides who can reach the port; it does not decide what they may do once they get there, and Subrouter deliberately does not treat node identity as authorization — loopback is the only implicitly trusted peer.That is the whole of the bug fixed here. Anyone thinking "it is on my tailnet, so it is fine" is reconstructing it from first principles, and a flag list would not stop them.
Review notes — verification, and two things the existing help text gets slightly wrong
One predicate, not two.
install-daemonreusesserve's ownbindAddrRequiresAdminTokenrather than spelling "is this loopback" a second time. Two gates answering one question from separate code is how the original hole opened:authorizeAdminreturnedtruefor any remote caller whenever no token was configured.Claims in the doc were checked against the code, not inferred:
drain403s a non-loopback peer even with a valid admin token —handleDrainre-checksisLoopbackRemoteinside the handler, afterrequireAdmin.serve's flag help lists drain among what the token gates, which overstates it.requireAdminreturns 401, so a stale client token fails loudly rather than degrading to unauthenticated.secretFromEnvironmenttreatsSUBROUTER_ADMIN_TOKENandSUBROUTER_ADMIN_TOKEN_FILEboth being set as fatal. That is what makes the two install flags mutually exclusive rather than merely redundant — accepting both would install a plist that validates and then fails at daemon boot.--admin-token-fileis macOS-only,--admin-token-stdinis systemd-only.Bind the 100.x address, not the MagicDNS name. The listener resolves once at bind time and a LaunchAgent can beat
tailscaled's MagicDNS to it.KeepAlivemakes that race self-heal, so the doc gives the tell that separates it from a genuine bind failure: restart churn insubrouter.err.logthat stops on its own, versus churn that does not.Testing. Five new tests cover the refusal across tailnet IPv4 / MagicDNS / bare
:31415, both loopback spellings still installing untokened, both token forms, their mutual exclusion, and the argv leak. Verified hermetically in a container with the repo mounted read-only and--network none;./cmd/subrouterand./internal/proxygreen on top ofmain. Nothing was run against a live daemon.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Secures remote admin and makes safe tailnet installs work on macOS.
servenow fails closed on non-loopback binds without a token, andinstall-daemonaccepts tailnet addresses with a validated token while keeping secrets out of places that leak.New Features
install-daemonaccepts tailnet IP/MagicDNS with--admin-token-file(preferred) or--admin-token; token is set via env (never inProgramArguments); plist perms are0644with token-file and0600with inline token.serveadds--allow-unauthenticated-admin; otherwise a non-loopback bind requires--admin-token/SUBROUTER_ADMIN_TOKEN[_FILE](shared gate); docs indocs/tailscale.md, README updated.Bug Fixes
/_subrouter/drainis loopback-only; hostnames are treated as non-loopback at validation time.Written for commit e11d16e. Summary will update on new commits.