-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: block incoming endpoints from call-me-maybe #55
base: main
Are you sure you want to change the base?
Changes from all commits
f852531
b22cdd1
5beb71a
6f25043
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -217,6 +217,8 @@ type Conn struct { | |
// blockEndpoints is whether to avoid capturing, storing and sending | ||
// endpoints gathered from local interfaces or STUN. Only DERP endpoints | ||
// will be sent. | ||
// This will also block incoming endpoints received via call-me-maybe disco | ||
// packets. | ||
blockEndpoints bool | ||
// endpointsUpdateActive indicates that updateEndpoints is | ||
// currently running. It's used to deduplicate concurrent endpoint | ||
|
@@ -843,10 +845,10 @@ func (c *Conn) DiscoPublicKey() key.DiscoPublic { | |
return c.discoPublic | ||
} | ||
|
||
// SetBlockEndpoints sets the blockEndpoints field. If changed, endpoints will | ||
// be updated to apply the new settings. Existing connections may continue to | ||
// use the old setting until they are reestablished. Disabling endpoints does | ||
// not affect the UDP socket or portmapper. | ||
// SetBlockEndpoints sets the blockEndpoints field. If enabled, all peer | ||
// endpoints will be cleared from the peer map and every connection will | ||
// immediately switch to DERP. Disabling endpoints does not affect the UDP | ||
// socket or portmapper. | ||
func (c *Conn) SetBlockEndpoints(block bool) { | ||
c.mu.Lock() | ||
defer c.mu.Unlock() | ||
|
@@ -856,6 +858,7 @@ func (c *Conn) SetBlockEndpoints(block bool) { | |
return | ||
} | ||
|
||
// Re-gather local endpoints. | ||
const why = "SetBlockEndpoints" | ||
if c.endpointsUpdateActive { | ||
if c.wantEndpointsUpdate != why { | ||
|
@@ -866,6 +869,13 @@ func (c *Conn) SetBlockEndpoints(block bool) { | |
c.endpointsUpdateActive = true | ||
go c.updateEndpoints(why) | ||
} | ||
|
||
// Clear any endpoints from the peerMap if block is true. | ||
if block { | ||
c.peerMap.forEachEndpoint(func(ep *endpoint) { | ||
ep.clearEndpoints("SetBlockEndpoints") | ||
}) | ||
} | ||
} | ||
|
||
// determineEndpoints returns the machine's endpoint addresses. It | ||
|
@@ -1523,6 +1533,14 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netip.AddrPort, derpNodeSrc ke | |
c.logf("[unexpected] CallMeMaybe from peer via DERP whose netmap discokey != disco source") | ||
return | ||
} | ||
if c.blockEndpoints { | ||
c.dlogf("[v1] magicsock: disco: %v<-%v (%v, %v) got call-me-maybe with %d endpoints, but endpoints blocked", | ||
c.discoShort, epDisco.short, | ||
ep.publicKey.ShortString(), derpStr(src.String()), | ||
len(dm.MyNumber), | ||
) | ||
dm.MyNumber = []netip.AddrPort{} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are we even delivering the CallMeMaybe? We clear out any previously learned endpoints when we set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is for incoming call-me-maybe packets. If only one side has "--disable-direct-connections" then the other side will still send call-me-maybe packets populated with endpoints to the other peer, which is why they still connect directly if they are on the same LAN. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clearing the endpoints once won't prevent them from being readded to the peer map when the peer sends us another call-me-maybe packet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realize this is for incoming call-me-maybe. I'm asking why we strip out the endpoints and then process the empty call-me-maybe, rather than just drop the call-me-maybe packets on the floor without further processing. What benefit do we get from sending an empty call-me-maybe thru |
||
} | ||
c.dlogf("[v1] magicsock: disco: %v<-%v (%v, %v) got call-me-maybe, %d endpoints", | ||
c.discoShort, epDisco.short, | ||
ep.publicKey.ShortString(), derpStr(src.String()), | ||
|
@@ -1921,6 +1939,13 @@ func (c *Conn) SetNetworkMap(nm *netmap.NetworkMap) { | |
// we'll fall through to the next pass, which allocates but can | ||
// handle full set updates. | ||
for _, n := range nm.Peers { | ||
if c.blockEndpoints { | ||
c.dlogf("[v1] magicsock: SetNetworkingMap: %v (%v, %v) received %d endpoints, but endpoints blocked", | ||
n.DiscoKey.ShortString(), n.Key.ShortString(), | ||
len(n.Endpoints), | ||
) | ||
n.Endpoints = []string{} | ||
} | ||
if ep, ok := c.peerMap.endpointForNodeKey(n.Key); ok { | ||
if n.DiscoKey.IsZero() && !n.IsWireGuardOnly { | ||
// Discokey transitioned from non-zero to zero? This should not | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 onendpoint
that could add an endpoint. There are 3 of these (updateFromNode
,handleCallMeMaybe
andaddCandidateEndpoint
), 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 ablockEndpoints
field that we sync withConn
, that way we can always directly check whether we are blocking endpoints at the moment we are about to add one?