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.
bee593d to
bae0575
Compare
ba19b86 to
f69c7af
Compare
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 |
5f71f39 to
8ebfb8a
Compare
| // 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
3669423 to
1f79477
Compare
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
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