Skip to content

fix(remote): connect --e2ee actually bypasses the plaintext-address check - #1019

Open
knaoe wants to merge 1 commit into
fujibee:mainfrom
knaoe:fix/connect-e2ee-bypass
Open

fix(remote): connect --e2ee actually bypasses the plaintext-address check#1019
knaoe wants to merge 1 commit into
fujibee:mainfrom
knaoe:fix/connect-e2ee-bypass

Conversation

@knaoe

@knaoe knaoe commented Aug 31, 2026

Copy link
Copy Markdown

Fixes #1018.

What

cmd_connect in scripts/remote.sh called _remote_validate_endpoint unconditionally, before $e2ee was ever consulted. So the refusal text's own claim — "connect with --e2ee so the contents are sealed before they leave this machine" — didn't hold: --e2ee never actually bypassed the plaintext-address check.

Fix

Skip the check when --e2ee is set:

if [ "$e2ee" -eq 0 ]; then
  _remote_validate_endpoint "$endpoint" || exit 1
fi

cmd_pull and cmd_set_endpoint call the same validator unconditionally too, but that's correct as-is — neither takes an --e2ee flag, since encryption is a per-team property fixed at connect time. cmd_connect is the only place that parses --e2ee and then didn't act on it for this decision.

I deliberately left connectedBinding() (scripts/internal/remote-sync.mjs:579, the per-sync re-check for an already-connected binding) as-is. It has no e2ee parameter, and I wasn't confident rewiring it to accept one — and getting that wiring wrong — was worth doing without a maintainer's steer, given a connected team's binding is exactly the state a bug there could put in a broken loop. Flagged in the issue; happy to take a pass at it here if that's the right call.

Tests

Added two cases to tests/test_remote.bats, right after the existing loopback-acceptance test:

  • a control: without --e2ee, an address outside the allowlist (http://0.0.0.0:1) is still refused with the existing message
  • with --e2ee, the same address gets past that refusal and fails at the network instead (asserted via the absence of the refusal text plus the presence of remote.sh's own connect failed wrapper, not a specific curl/errno string, since the exact transport error for an unreachable address is not something I wanted to pin across platforms)

0.0.0.0:1 was picked over a real unreachable public address so the case stays fast and hermetic (no network dependency, no timeout to wait out) — nothing listens on port 1 there, and it resolves like loopback for an outbound connection on both Linux and macOS, while still being outside the validator's private-range allowlist.

Ran locally (bats tests/test_remote.bats tests/test_endpoint_scheme.bats tests/test_endpoint_table_node.bats): all passing, including the two new cases and the full existing suite in those files.

Environment

Found this while connecting two of my own machines over Tailscale — its CGNAT range (100.64.0.0/10) isn't in the hardcoded private-range list, so a Tailscale-only pair (no shared LAN) had no working plaintext path, and --e2ee, the documented way out, didn't do anything.

…heck

cmd_connect validated the endpoint unconditionally before --e2ee was
ever consulted, so the refusal text's own claim ("connect with --e2ee
so the contents are sealed before they leave this machine") did not
hold. Skip the check when e2ee is requested, matching the documented
and advertised behavior.

Adds a control test (same address, no --e2ee, still refused) and the
positive case (with --e2ee, gets past the refusal and fails at the
network instead) to tests/test_remote.bats.
@knaoe

knaoe commented Aug 31, 2026

Copy link
Copy Markdown
Author

Found a second gate while actually driving this end to end against a real Tailscale address, so flagging before this looks more finished than it is.

cmd_connect's registration POST now goes through fine with this fix. But right after the binding is recorded, connect tries to export the initial age-v1 snapshot, which goes through connectedBinding() (scripts/internal/remote-sync.mjs:579) — and that function has no e2ee (or cipher-aware) parameter at all, so it applies the same plaintext-address rule unconditionally. Repro against a live server:

$ remote.sh connect --endpoint http://100.x.x.x:8787 --e2ee my-team
Generated a new key for team 'my-team'.
...
Connecting team 'my-team' to http://100.x.x.x:8787 ...
{"...":"fatal","message":"connected team endpoint must use HTTPS, or HTTP to a private IP address (10/8, 172.16/12, 192.168/16, 169.254/16, 127/8, ::1, fc00::/7)",...}
agmsg: could not export the initial age-v1 snapshot for team 'my-team'; sync was not started.
agmsg: the remote binding was recorded, but no sync engine was started.

So this PR as it stands fixes the cmd_connect refusal (verified, tested) but does not deliver a working --e2ee connect to a non-private address end to end — it just moves the failure one step later, to a worse spot (the binding is already recorded server-side by then).

I didn't attempt a fix for this one here, for the reason I gave in the PR description: connectedBinding() is also the per-cycle check continued sync relies on, and I wasn't confident about the right way to give it cipher awareness without a maintainer's steer — whether that's threading a parameter from both call sites (connect's snapshot export, and continued sync), or having it read the binding's own recorded cipher/security state instead of taking a caller-supplied flag. The second reads more robust to me (the binding already has to carry that state for other reasons?) but I don't know this codebase's data model well enough to be sure, and getting this wrong for the continued-sync path risks a worse failure mode than the one already reported.

Happy to take a pass at whichever shape you'd prefer, or to leave this PR scoped to just the cmd_connect fix and file a separate issue for connectedBinding() if that's cleaner to review and land independently.

@fujibee

fujibee commented Sep 13, 2026

Copy link
Copy Markdown
Owner

@knaoe

Thanks — the cmd_connect change is correct, and your two tests reproduce cleanly here (red on main, green on your head).

On the second gate you flagged: we traced it and agree, with one addition. connectedBinding() in scripts/internal/remote-sync.mjs is re-read and re-validated on every sync cycle (six call sites), not just once after connect. So with only the cmd_connect half, connect --e2ee <public plaintext address> succeeds and records the binding, and then every engine cycle fails on the same rule — a harder-to-diagnose state than today's immediate refusal. For that reason we'd rather not merge the first half alone.

Since you offered: yes, please extend the fix to connectedBinding() in this PR — make the plaintext-address check conditional on the team's cipher profile (age-v1 seals the body before it leaves the machine, which is the rationale the refusal text already gives). A test that drives connect + one engine cycle against an out-of-allowlist address with --e2ee would pin it. Happy to review as soon as it's up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

connect --e2ee doesn't bypass the plaintext-address refusal it's documented to bypass

2 participants