Repository navigation
tailcat, internal/buildtags: link the port mapper in native builds - #152
Open
pedramamini wants to merge 1 commit into
Open
pedramamini wants to merge 1 commit into
pedramamini wants to merge 1 commit into
Conversation
magicsock creates a port mapper only when feature/portmapper registers one, and nothing in tailcat imported it; release builds also omitted it with ts_omit_portmapper. So neither side ever asked its router for a UPnP, NAT-PMP, or PCP mapping, and the only public endpoint a peer advertised was its STUN-learned one. That is enough when one side's NAT keeps the same public port for every destination, but not when a client sits behind a NAT that allocates a new public port per destination (common on hotel, office, and carrier networks). The client's endpoint is unpredictable, and a server behind an ordinary home router drops its packets because they arrive from a port the server never sent to, so the connection stays on DERP for its whole life. A router mapping accepts packets from any source, which gives the server an endpoint such a client can reach directly. Import feature/condregister/portmapper from the tailcat package, as tsnet does, and keep portmapper in releaseKeep so ts_omit_portmapper drops out of build-tags.txt and .goreleaser.yaml. The wasm build still omits it: a browser has no UDP sockets. TS_DISABLE_PORTMAPPER=1 and TS_DISABLE_UPNP=1 turn it off at runtime. The new test runs go list on cmd/tailcat with ReleaseTags instead of checking the hook in the test binary, which links the port mapper on its own through feature/condregister. Updates tailscale#143 Signed-off-by: Pedram Amini <pedram.amini@gmail.com>
This branch has not been deployed
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
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.
Updates #143
magicsock creates a port mapper only when
feature/portmapperregisters one, and nothing in tailcat imports it; release builds also setts_omit_portmapper. So neither side ever asks its router for a UPnP, NAT-PMP, or PCP mapping, and a peer's only public endpoint is the one STUN learned. (The author of #143 assumed tailcat already relies on UPnP/NAT-PMP/PCP; it doesn't.)That fails when a client sits behind a NAT that allocates a new public port per destination (common on hotel, office, and carrier networks) and the server sits behind an ordinary home router: the client's endpoint is unpredictable, the server's router drops its packets, and the connection stays on DERP for its whole life. A router mapping accepts packets from any source, which gives such a client an endpoint it can reach directly.
This imports
feature/condregister/portmapperfrom the tailcat package, as tsnet does, and keepsportmapperinreleaseKeep, sots_omit_portmapperdrops out ofbuild-tags.txtand.goreleaser.yaml. The wasm build still omits it: a browser has no UDP sockets.TS_DISABLE_PORTMAPPER=1turns it off at runtime,TS_DISABLE_UPNP=1only UPnP; the README and CHANGELOG say so. It adds 352 KB (1.9%) to the stripped linux/amd64 binary.The new test,
internal/buildtags/deps_test.go, runsgo list -depsoncmd/tailcatwithReleaseTags()and oncmd/tailcat-webwithWasmTags(). It checks the binaries rather than the test binary on purpose: test builds link the port mapper on their own throughfeature/condregister, so a hook check in the package's tests passes even without this change. The new test fails if either the import or the keep-list entry is removed.Testing:
go build ./...,go vet ./...andgo test -count=1 -timeout 600s ./...pass (darwin/arm64, go1.26.5).make tidyleavesgo.mod,go.sumandflakehashes.jsonunchanged. Release-tag cross-builds pass for linux amd64/arm64/arm, windows amd64/arm64, darwin, freebsd and openbsd, as doGOOS=js GOARCH=wasm go build ./weband the libtailcat c-shared smoke test.MASQUERADE --random-fully(STUN saw mapped ports 28674, 28674 and 28677 for three destinations) and rantailcat ping --until-direct --timeout 45sagainst servers behind a NETGEAR Orbi with UPnP, NAT-PMP and PCP enabled:The live tests used this commit cherry-picked onto v0.7.0, to match v0.7.0 clients; the PR is against main.