fix: block incoming endpoints from call-me-maybe #55
Conversation
Previously, (*magicsock.Conn).SetBlockEndpoints(true) would only prevent local endpoints from being sent to other peers. This setting is primarily used to prevent any direct connection from forming, regardless of which side initiated it, but any connections initiated locally to endpoints received from the other peer would still work. If endpoints are blocked, we will now drop any endpoints we already know of (via call-me-maybe) as well as block any future endpoints received via call-me-maybe. Endpoints received via coordination are not impacted (and should be blocked using a different mechanism).
|
I tested this on coder by setting |
wgengine/magicsock/endpoint.go
Outdated
| When: time.Now(), | ||
| What: "clearEndpoints-" + why, | ||
| }) | ||
| de.endpointState = map[netip.AddrPort]*endpointState{} |
There was a problem hiding this comment.
I like this, but I think we should pair it with a lower-level implementation of blockEndpoints
Right now, we check this field on the magicsock Conn before calling into methods on endpoint that could add an endpoint. There are 3 of these (updateFromNode, handleCallMeMaybe and addCandidateEndpoint), but you've only directly blocked the first two.
So, there is still a narrow race condition where you can set block endpoints, clear them all out, and a Disco Ping could come in and add an endpoint back in.
What do you think about extending endpoint to also have a blockEndpoints field that we sync with Conn, that way we can always directly check whether we are blocking endpoints at the moment we are about to add one?
3 participants
Previously, (*magicsock.Conn).SetBlockEndpoints(true) would only prevent local endpoints from being sent to other peers.
This setting is primarily used to prevent any direct connection from forming, regardless of which side initiated it, but any connections initiated locally to endpoints received from the other peer would still work.
If endpoints are blocked, we will now drop any endpoints we already know of (via call-me-maybe) as well as block any future endpoints received via call-me-maybe.
Endpoints received via coordination are not impacted (and should be blocked using a different mechanism).
Note; it was difficult to write a test that would fail with the previous behavior but pass with the current behavior because of the test utilities available in Tailscale, but I did get it to work. Notably, there's a single
time.Sleep(time.Second), but after trying various other ways to accomplish the same thing I couldn't get anything to work.