control/controlclient, tailcfg: add Node.Expired field, set for expired nodes #6929
Conversation
control/controlclient/map.go
Outdated
| if mapRes.ControlTime == nil { | ||
| log.Printf("netmap: removeExpiredPeers: MapResponse.ControlTime not present; skipping") | ||
| return | ||
| } |
There was a problem hiding this comment.
this is going to be spammy and also insufficient: most map responses don't have ControlTime set.
only the rare full path adds that.
and we don't really want those extra bytes all the time for every little update. ControlTime is supposed to be an occasional adjustment thing so the client can learn its offset.
There was a problem hiding this comment.
Storing the difference between control time and local time whenever the field is set may be a close enough approximation, especially if we arrange for it to be sent at least once a day or something. If the difference is small enough (e.g. less than a minute), we could also simply say the local clock is fine and use local time.
There was a problem hiding this comment.
If we never get a ControlTime (does headscale send it?) then I'd recommend just assuming the local time is good.
There was a problem hiding this comment.
Okay, I reworked this; we now store a delta on the mapSession and use that to calculate the adjusted current time before checking expiry. Thoughts?
Also: @maisem thinks that we shouldn't drop peers here, and should instead mutate their keys to be invalid; if nobody has any objections I can do that next.
There was a problem hiding this comment.
I view mutating keys to be invalid as mostly a server-side hack for old clients.
You can also do that for defense in depth, but I'd like to set a "expired bool" on the peer in the netmap so things like "tailscale ping" and magicsock and debug can say/log explicit things about peers being expired rather than obscure wireguard key mismatch errors.
There was a problem hiding this comment.
dropping peers from the netmap would mean that "tailscale status" and the likes would just omit them. That's probably going to result in more support noise. It also has weird implications in terms of subnet routers.
I'd very much prefer if we just somehow blocked traffic to expired peers instead of completely dropping them.
There was a problem hiding this comment.
Okay, we now replicate what control does and clear Endpoints and DERP, clear the node's Key, and add an Expired boolean and set it to true in this codepath.
andrew-d
force-pushed
the
andrew/netmap-drop-expired
branch
from
bee593d
to
bae0575
Compare
andrew-d
force-pushed
the
andrew/netmap-drop-expired
branch
3 times, most recently
from
ba19b86
to
f69c7af
Compare
andrew-d
force-pushed
the
andrew/netmap-drop-expired
branch
2 times, most recently
from
ec62f0e
to
5f71f39
Compare
| delta := mapRes.ControlTime.Sub(localNow) | ||
| if delta.Abs() > minClockDelta { | ||
| log.Printf("[v1] netmap: flagExpiredPeers: setting clock delta to %v", delta) | ||
| ms.clockDelta = delta |
There was a problem hiding this comment.
do we have to worry about resetting this to 0?
There was a problem hiding this comment.
Hm, for safety's sake, yes, added.
|
Also per @maisem's request, we now mutate the node's key instead of replacing it with |
andrew-d
force-pushed
the
andrew/netmap-drop-expired
branch
from
5f71f39
to
8ebfb8a
Compare
| @@ -85,6 +91,7 @@ func (ms *mapSession) addUserProfile(userID tailcfg.UserID) { | |||
| // information from prior MapResponse values. | |||
| func (ms *mapSession) netmapForResponse(resp *tailcfg.MapResponse) *netmap.NetworkMap { | |||
| undeltaPeers(resp, ms.previousPeers) | |||
| ms.flagExpiredPeers(resp) | |||
There was a problem hiding this comment.
we probably need to call this with a timer for cases where control doesn't send netmap updates for a long time (or is unreachable).
There was a problem hiding this comment.
Yep, planning on following up with that in another PR; it's a bit tricky since I think we need to create a new NetMap, since we can't mutate the existing one
andrew-d
force-pushed
the
andrew/netmap-drop-expired
branch
3 times, most recently
from
3669423
to
1f79477
Compare
bradfitz
changed the title
andrew-d
force-pushed
the
andrew/netmap-drop-expired
branch
from
1f79477
to
9dd3ac6
Compare
…ed nodes Nodes that are expired, taking into account the time delta calculated from MapResponse.ControlTime have the newly-added Expired boolean set. For additional defense-in-depth, also replicate what control does and clear the Endpoints and DERP fields, and additionally set the node key to a bogus value. Updates #6932 Signed-off-by: Andrew Dunham <andrew@du.nham.ca> Change-Id: Ia2bd6b56064416feee28aef5699ca7090940662a
andrew-d
force-pushed
the
andrew/netmap-drop-expired
branch
from
9dd3ac6
to
e91fdf0
Compare
|
Okay, looked at this with fresh eyes this morning ✅ |
Signed-off-by: Andrew Dunham andrew@du.nham.ca
Change-Id: Ia2bd6b56064416feee28aef5699ca7090940662a