fix(bridge): refuse writes when the device header is absent - #28
Conversation
deviceAuth() treated a request that carries no device header as an authorised
on-host operator:
if (!device) return { enforced: true, device: null, authorized: true };
The comment above the branch documents the intent as "an absent header is the
on-host loopback operator", but the request's origin was never checked, so the
branch granted full write access to any caller that reached the bridge without
the header. README.md already documented the consequence, in the Variant B
warning: an absent header means full access, so the gate is a no-op.
Configuring COLLIE_DEVICE_HEADER is the operator asserting that their proxy
sets the header on every request. A request without one did not come through
that proxy, and is now treated exactly like a device that is not on the
allowlist: read-only.
Read-only is the accurate word, not "rejected". guard() consults deviceAuth
only for writes, so a header-less caller still reads panes, as an unlisted
device always could. That is the existing design and this change does not touch
it; what changes is that a missing header no longer counts as the operator. The
README now says so explicitly, including that pane text is therefore not behind
this gate.
There is deliberately no loopback exemption, which is worth recording because
it looks like the obvious way to keep the on-host convenience. Every supported
front door is a proxy co-located with the bridge: collie-ctl.sh maps tailscale
serve to 127.0.0.1:PORT, and the documented Variant B and C proxies use a
loopback proxy_pass. A header-less remote request therefore arrives with a
loopback peer just like a local one, so neither the peer address nor the Host
header separates the operator on the host from a remote client whose proxy
failed to inject the header. An exemption would have re-opened the very path
this closes.
The cost is that the bridge's own loopback URL is read-only while device auth
is on, since it bypasses the proxy that would inject the header. The README
states that, and gives the by-hand alternative: curl the loopback bridge with
an allowlisted id, not the public URL, where the proxy's mandatory override
would replace it.
guard() is exported so the wiring is tested, not just deviceAuth in isolation:
the gate lives in the write/read asymmetry there, and deviceAuth being correct
on its own would not catch that regressing. Both halves are asserted.
This closes the absent-header half of the documented hole. The other half, a
client setting the header itself on plain tailscale serve, is unchanged and
still needs a proxy that overrides it; the README warning now says just that.
A deployment that does not set COLLIE_DEVICE_HEADER is unaffected.
|
Merged and shipped in 0.15.0. Thanks, this was the most valuable thing in the queue. Your reasoning about why the peer address is not the fix is correct, and it is the part that made the change obviously right. I verified the hole was live rather than theoretical: on a deployment where a proxy injects the header, requests through the front door were attributed correctly, while requests sent straight to the bridge's own tailnet URL came back Two notes. The PR description does not match the diff. It describes The tests are the best part. Pinning the read/write asymmetry and explicitly refusing a loopback-Host exemption stops the obvious future regression. 318 backend tests pass on merged main. Flagged as a breaking change in the changelog, since anyone with the header configured and no injector in front loses write access on upgrade. |
|
You're right. The peer-address version came first, and I wrote the description for it. Review killed Sorry. Reconciling those accounts cost you review time for no good reason. I've rewritten the #29 has the same problem: the description says the region comparison collapses runs of whitespace, |
2 participants
The problem
deviceAuth()treats a request that carries no device header as an authorised on-host operator:The comment above the branch documents the intent as "an absent header is the on-host loopback
operator", and that intent is reasonable. The request's origin is never checked, though, so the
branch grants full write access to any caller that reaches the bridge without the header.
The README already documents the consequence, in the Variant B warning: an absent header means full
access, so on plain
tailscale servethe gate is a no-op. This PR closes that half.It matters most under
COLLIE_SKIP_SERVE=1, whereCOLLIE_TRUSTED_USERis inert (the bridgealready warns about exactly that at startup) and the device header is the only authorisation left.
Why checking the peer address is not the fix
This is worth recording, because it is the obvious first move and it does not work.
Every supported front door is a proxy co-located with the bridge.
collie-ctl.shmapstailscale serveto127.0.0.1:PORT, and the documented Variant B and C proxies use a loopbackproxy_pass. A header-less remote request therefore still arrives with a loopback peer. The peeraddress cannot separate the operator on the host from a remote client whose proxy failed to inject
the header, so gating on it would look like a fix while changing nothing for either documented
deployment.
What this does instead
Setting
COLLIE_DEVICE_HEADERis the operator asserting that their proxy sets the header on everyrequest. A request without one did not come through that proxy, and is now treated the same as a
device that is not on the allowlist.
unknownsentinelRead-only is the whole scope of this gate, deliberately.
guard()consultsdeviceAuth()only forwrites, so a header-less caller still reads panes. That is the existing design, and this change does
not touch it. What changes is that a missing header no longer counts as the operator.
The absent-header case deliberately gets no loopback exemption, for the reason above. Driving a pane
from the host is one flag away: send an allowlisted id yourself, against the loopback bridge.
guard()is now exported, so the wiring can be tested rather than onlydeviceAuth()in isolation.What this does not fix
The Variant B warning has a second half, and it stays:
tailscale serveforwards a client-suppliedX-Device-Iduntouched, so a client that sets an allowlisted id itself is still trusted. Only aproxy that overrides the header closes that, exactly as the README already requires. I reworded
the warning to say just that rather than deleting it.
A deployment that does not set
COLLIE_DEVICE_HEADERis completely unaffected.Tests
Seven new cases, and
bun run typecheckclean.Two on
deviceAuth()directly: the absent header refused, and a loopbackHostexplicitly notbuying an exemption, so the tempting fix above cannot creep back in unnoticed.
Five on
guard(), pinning the read/write asymmetry that the gate stands or falls on: a write withno header and a write from a non-allowlisted device both 403, a write from an allowlisted device
proceeds, a read with no header still proceeds rather than being rejected, and with the feature off
a write with no header proceeds.
Docs
README updated in three places: the security bullet, the Variant B section, and the reverse-proxy
note. The Variant B section now spells out what read-only covers, that the bridge's own loopback URL
becomes read-only because it bypasses the proxy, and how to drive a pane from the host by hand.