From f453502ef1048e402e48be4655f5c9f0858fb50c Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Mon, 2 Nov 2026 15:53:59 +0000 Subject: [PATCH] hscontrol: name policy, registration and map delivery decisions --- hscontrol/api/v2/devices.go | 3 +- hscontrol/api/v2/keys.go | 14 +- hscontrol/api/v2/oauth.go | 15 +- hscontrol/debug.go | 12 +- hscontrol/mapper/batcher.go | 4 +- hscontrol/mapper/node_conn.go | 13 +- hscontrol/oidc.go | 27 ++- hscontrol/policy/policyutil/reduce.go | 14 +- hscontrol/policy/v2/canaccess_equiv_test.go | 6 +- hscontrol/policy/v2/compiled.go | 9 +- hscontrol/policy/v2/filter.go | 2 +- hscontrol/policy/v2/policy.go | 55 +++-- hscontrol/policy/v2/sshtest.go | 2 +- hscontrol/policy/v2/types.go | 16 +- hscontrol/servertest/ha_property_test.go | 4 +- hscontrol/state/node_store.go | 16 +- hscontrol/state/state.go | 219 +++++++++++++------- hscontrol/types/change/change.go | 33 +-- 18 files changed, 312 insertions(+), 152 deletions(-) diff --git a/hscontrol/api/v2/devices.go b/hscontrol/api/v2/devices.go index 2eba40215..d8c6d39a4 100644 --- a/hscontrol/api/v2/devices.go +++ b/hscontrol/api/v2/devices.go @@ -476,7 +476,8 @@ func deviceFromView(view types.NodeView, allFields bool, baseDomain string) Devi } // LastSeen is reported only when the device is offline, matching tailcfg. - if view.LastSeen().Valid() && view.IsOnline().Valid() && !view.IsOnline().Get() { + hasOfflineLastSeen := view.LastSeen().Valid() && view.IsOnline().Valid() && !view.IsOnline().Get() + if hasOfflineLastSeen { ls := view.LastSeen().Get() d.LastSeen = &ls } diff --git a/hscontrol/api/v2/keys.go b/hscontrol/api/v2/keys.go index bd4a9ec6a..83daa7e55 100644 --- a/hscontrol/api/v2/keys.go +++ b/hscontrol/api/v2/keys.go @@ -3,7 +3,6 @@ package apiv2 import ( "context" "net/http" - "slices" "strconv" "time" @@ -204,7 +203,7 @@ func registerKeys(api huma.API, b Backend) { // As Tailscale lists them: never an invalid key, and to a token // without all=true only the keys tokens made. for i := range keys { - if keys[i].ValidAt(now) != nil || isOAuth && !in.All && !keys[i].CreatedByToken { + if !keyVisibleInList(&keys[i], now, isOAuth, in.All) { continue } @@ -326,6 +325,14 @@ func registerKeys(api huma.API, b Backend) { }) } +func keyVisibleInList(key *types.PreAuthKey, now time.Time, isOAuth, all bool) bool { + if key.ValidAt(now) != nil { + return false + } + + return !isOAuth || all || key.CreatedByToken +} + // requireKeyScope authorizes a keys operation for an OAuth access token. An admin // API key carries no OAuth scopes and is all-access, so it always passes. func requireKeyScope(ctx context.Context, need scope.Scope) error { @@ -483,8 +490,7 @@ func createOAuthClient(ctx context.Context, b Backend, body CreateKeyRequest) (* // An untagged "all" client may mint tokens for any tag (narrowTags), so a // tag-limited token would escape its tags one client away. - if len(tokenTags) > 0 && len(body.Tags) == 0 && - slices.Contains(body.Scopes, string(scope.All)) { + if len(tokenTags) > 0 && canAssignAnyTag(body.Tags, body.Scopes) { return nil, huma.Error403Forbidden( "client may not be granted scope all without tags by a tagged token", ) diff --git a/hscontrol/api/v2/oauth.go b/hscontrol/api/v2/oauth.go index 738d5c32c..9f8ea0bef 100644 --- a/hscontrol/api/v2/oauth.go +++ b/hscontrol/api/v2/oauth.go @@ -121,8 +121,7 @@ func oauthTokenHandler(b Backend) http.HandlerFunc { } granted := scope.Parse(token.Scopes) - if scope.Grants(granted, scope.AuthKeys) || scope.Grants(granted, scope.DevicesCore) || - scope.Grants(granted, scope.OAuthKeys) { + if tokenCanAssignTags(granted) { resp.Tags = strings.Join(token.Tags, " ") } @@ -130,6 +129,16 @@ func oauthTokenHandler(b Backend) http.HandlerFunc { } } +func tokenCanAssignTags(granted []scope.Scope) bool { + return scope.Grants(granted, scope.AuthKeys) || + scope.Grants(granted, scope.DevicesCore) || + scope.Grants(granted, scope.OAuthKeys) +} + +func canAssignAnyTag(tags, scopes []string) bool { + return len(tags) == 0 && slices.Contains(scopes, string(scope.All)) +} + // narrowScopes returns the requested scopes if each is granted by the client (an // empty request means "the client's full grant"), otherwise the offending scope // and false. A client holding a broad scope (e.g. "all") may mint a token limited @@ -157,7 +166,7 @@ func narrowTags(client *types.OAuthClient, requested []string) ([]string, string return client.Tags, "", true } - anyTag := len(client.Tags) == 0 && slices.Contains(client.Scopes, string(scope.All)) + anyTag := canAssignAnyTag(client.Tags, client.Scopes) for _, req := range requested { // Reject malformed tags at the trust boundary. A client's own tags are diff --git a/hscontrol/debug.go b/hscontrol/debug.go index 44eb4e10d..e89809235 100644 --- a/hscontrol/debug.go +++ b/hscontrol/debug.go @@ -104,8 +104,7 @@ func (h *Headscale) debugHostGuard(next http.Handler) http.Handler { host := normalize(r.Host) - _, err := netip.ParseAddr(host) - if host != "" && (err == nil || slices.Contains(allowed, host)) { + if isAllowedDebugHost(host, allowed) { next.ServeHTTP(w, r) return @@ -120,6 +119,15 @@ func (h *Headscale) debugHostGuard(next http.Handler) http.Handler { }) } +func isAllowedDebugHost(host string, allowed []string) bool { + _, err := netip.ParseAddr(host) + if host == "" { + return false + } + + return err == nil || slices.Contains(allowed, host) +} + func (h *Headscale) debugHTTPServer() *http.Server { debugMux := http.NewServeMux() debug := tsweb.Debugger(debugMux) diff --git a/hscontrol/mapper/batcher.go b/hscontrol/mapper/batcher.go index 5c3cb8652..593ed43d7 100644 --- a/hscontrol/mapper/batcher.go +++ b/hscontrol/mapper/batcher.go @@ -848,7 +848,7 @@ func (b *Batcher) cleanupOfflineNodes() { // Find nodes that have been offline for too long by scanning b.nodes // and checking each node's disconnectedAt timestamp. b.nodes.Range(func(nodeID types.NodeID, nc *multiChannelNodeConn) bool { - if nc != nil && !nc.hasActiveConnections() && nc.offlineDuration() > offlineNodeCleanupThreshold { + if nc.isOffline() && nc.offlineDuration() > offlineNodeCleanupThreshold { nodesToCleanup = append(nodesToCleanup, nodeID) } @@ -864,7 +864,7 @@ func (b *Batcher) cleanupOfflineNodes() { b.nodes.Compute( nodeID, func(conn *multiChannelNodeConn, loaded bool) (*multiChannelNodeConn, xsync.ComputeOp) { - if !loaded || conn == nil || conn.hasActiveConnections() { + if !loaded || !conn.isOffline() { return conn, xsync.CancelOp } diff --git a/hscontrol/mapper/node_conn.go b/hscontrol/mapper/node_conn.go index e5aebdf77..e941992c8 100644 --- a/hscontrol/mapper/node_conn.go +++ b/hscontrol/mapper/node_conn.go @@ -340,6 +340,10 @@ func (mc *multiChannelNodeConn) isConnected() bool { return mc.disconnectedAt.Load() == nil } +func (mc *multiChannelNodeConn) isOffline() bool { + return mc != nil && !mc.hasActiveConnections() +} + // offlineDuration returns how long the node has been disconnected. // Returns 0 if the node is connected or has never been marked as disconnected. func (mc *multiChannelNodeConn) offlineDuration() time.Duration { @@ -358,7 +362,7 @@ func (mc *multiChannelNodeConn) appendPending(changes ...change.Change) { mc.pending = append(mc.pending, changes...) mc.pendingSize += changesSize(changes) - fullAtHead := len(mc.pending) > 0 && mc.pending[0].IsFull() + fullAtHead := startsWithFullUpdate(mc.pending) // Pending drains only once the bundle in flight is delivered, so behind // workers it grew by every change; it renders folded anyway, and @@ -382,8 +386,7 @@ func (mc *multiChannelNodeConn) appendPending(changes ...change.Change) { // online/offline. func (mc *multiChannelNodeConn) prependPending(changes ...change.Change) { mc.pendingMu.Lock() - full := (len(mc.pending) > 0 && mc.pending[0].IsFull()) || - (len(changes) > 0 && changes[0].IsFull()) + full := startsWithFullUpdate(mc.pending) || startsWithFullUpdate(changes) mc.pending = append(changes, mc.pending...) mc.pendingSize += changesSize(changes) @@ -394,6 +397,10 @@ func (mc *multiChannelNodeConn) prependPending(changes ...change.Change) { mc.pendingMu.Unlock() } +func startsWithFullUpdate(changes []change.Change) bool { + return len(changes) > 0 && changes[0].IsFull() +} + // collapsePendingToFull replaces pending, together with incoming, by a // single full update and the pings it cannot carry ([change.CollapseToFull]). func (mc *multiChannelNodeConn) collapsePendingToFull(incoming []change.Change) { diff --git a/hscontrol/oidc.go b/hscontrol/oidc.go index e8412d990..b7c3de81e 100644 --- a/hscontrol/oidc.go +++ b/hscontrol/oidc.go @@ -349,17 +349,16 @@ func (a *AuthProviderOIDC) OIDCCallbackHandler( // The [oidc.UserInfo] type only decodes some fields (Subject, Profile, Email, EmailVerified). // We are interested in other fields too (e.g. groups are required for allowedGroups) so we // decode into our own [types.OIDCUserInfo] type using the underlying claims struct. - var userinfo2 types.OIDCUserInfo - if userinfo != nil && userinfo.Claims(&userinfo2) == nil && userinfo2.Sub == claims.Sub { + if userinfoClaims, ok := userInfoForSubject(userinfo, claims.Sub); ok { // Update the user with the userinfo claims (with id token claims as fallback). // TODO(kradalby): there might be more interesting fields here that we have not found yet. - claims.Email, claims.EmailVerified = mergeEmailClaims(&claims, &userinfo2) - claims.Username = cmp.Or(userinfo2.PreferredUsername, claims.Username) - claims.Name = cmp.Or(userinfo2.Name, claims.Name) + claims.Email, claims.EmailVerified = mergeEmailClaims(&claims, &userinfoClaims) + claims.Username = cmp.Or(userinfoClaims.PreferredUsername, claims.Username) + claims.Name = cmp.Or(userinfoClaims.Name, claims.Name) - claims.ProfilePictureURL = cmp.Or(userinfo2.Picture, claims.ProfilePictureURL) - if userinfo2.Groups != nil { - claims.Groups = userinfo2.Groups + claims.ProfilePictureURL = cmp.Or(userinfoClaims.Picture, claims.ProfilePictureURL) + if userinfoClaims.Groups != nil { + claims.Groups = userinfoClaims.Groups } } @@ -394,6 +393,15 @@ func (a *AuthProviderOIDC) OIDCCallbackHandler( a.beginConfirmation(writer, req, authInfo.AuthID, authInfo.Registration, user, nodeExpiry) } +func userInfoForSubject(userinfo *oidc.UserInfo, subject string) (types.OIDCUserInfo, bool) { + var claims types.OIDCUserInfo + if userinfo == nil || userinfo.Claims(&claims) != nil { + return claims, false + } + + return claims, claims.Sub == subject +} + // mergeEmailClaims keeps each verification flag with the address it describes. func mergeEmailClaims( claims *types.OIDCClaims, @@ -425,7 +433,8 @@ func extractCodeAndStateParamFromRequest( code := query.Get("code") state := query.Get("state") - if (code == "" && query.Get("error") == "") || state == "" { + hasAuthenticationResult := code != "" || query.Get("error") != "" + if !hasAuthenticationResult || state == "" { return "", "", NewHTTPError(http.StatusBadRequest, "missing code or state parameter", errEmptyOIDCCallbackParams) } diff --git a/hscontrol/policy/policyutil/reduce.go b/hscontrol/policy/policyutil/reduce.go index a6e803df0..020b8ddea 100644 --- a/hscontrol/policy/policyutil/reduce.go +++ b/hscontrol/policy/policyutil/reduce.go @@ -102,9 +102,7 @@ func appendOwnDests( nodeAddrs *netipx.IPSet, ) []tailcfg.NetPortRange { pfx, err := netip.ParsePrefix(dest.IP) - if err != nil || pfx.IsSingleIP() || nodeAddrs == nil || !nodeAddrs.ContainsPrefix(pfx) || - slices.ContainsFunc(subnetRoutes, pfx.Overlaps) || - slices.ContainsFunc(exitRoutes, pfx.Overlaps) { + if err != nil || !destinationNeedsOwnAddressExpansion(pfx, subnetRoutes, exitRoutes, nodeAddrs) { return append(dests, dest) } @@ -119,6 +117,16 @@ func appendOwnDests( return dests } +func destinationNeedsOwnAddressExpansion( + pfx netip.Prefix, + subnetRoutes, exitRoutes []netip.Prefix, + nodeAddrs *netipx.IPSet, +) bool { + return !pfx.IsSingleIP() && nodeAddrs != nil && nodeAddrs.ContainsPrefix(pfx) && + !slices.ContainsFunc(subnetRoutes, pfx.Overlaps) && + !slices.ContainsFunc(exitRoutes, pfx.Overlaps) +} + // reduceCapGrantRule filters a [tailcfg.CapGrant] rule to only include // [tailcfg.CapGrant] entries whose Dsts match the given node's IPs. When a // broad prefix (e.g. 100.64.0.0/10 from dst:*) contains a node's IP, it is diff --git a/hscontrol/policy/v2/canaccess_equiv_test.go b/hscontrol/policy/v2/canaccess_equiv_test.go index a9d65d1ac..24b4fb9c5 100644 --- a/hscontrol/policy/v2/canaccess_equiv_test.go +++ b/hscontrol/policy/v2/canaccess_equiv_test.go @@ -188,10 +188,12 @@ func peerMapPrefixReference( for i, a := range nodes { for _, b := range nodes[i+1:] { mi, mj := nodeMatchers[a.ID], nodeMatchers[b.ID] - if canAccessPrefixReference(a, b, mi) || + + linkedByEitherPolicy := canAccessPrefixReference(a, b, mi) || canAccessPrefixReference(b, a, mi) || canAccessPrefixReference(a, b, mj) || - canAccessPrefixReference(b, a, mj) { + canAccessPrefixReference(b, a, mj) + if linkedByEitherPolicy { peers[a.ID] = append(peers[a.ID], b.ID) peers[b.ID] = append(peers[b.ID], a.ID) } diff --git a/hscontrol/policy/v2/compiled.go b/hscontrol/policy/v2/compiled.go index f7f941251..fddc11cae 100644 --- a/hscontrol/policy/v2/compiled.go +++ b/hscontrol/policy/v2/compiled.go @@ -130,7 +130,7 @@ func buildUserNodeIndex( idx := make(userNodeIndex) for _, n := range nodes.All() { - if !n.IsTagged() && n.User().Valid() { + if nodeHasUserIdentity(n) { uid := n.User().ID() idx[uid] = append(idx[uid], n) } @@ -139,6 +139,11 @@ func buildUserNodeIndex( return idx } +// A user association alone does not establish ownership: tagged nodes may retain their creator. +func nodeHasUserIdentity(node types.NodeView) bool { + return !node.IsTagged() && node.User().Valid() +} + // nodeAddrSet is the addresses of every node. func nodeAddrSet(nodes views.Slice[types.NodeView]) *netipx.IPSet { var b netipx.IPSetBuilder @@ -852,7 +857,7 @@ func exitNodeSelfRules( for _, uid := range slices.Sorted(maps.Keys(userIdx)) { // compileAutogroupSelf already covers an untagged node's own user. - if !node.IsTagged() && node.User().Valid() && node.User().ID() == uid { + if nodeHasUserIdentity(node) && node.User().ID() == uid { continue } diff --git a/hscontrol/policy/v2/filter.go b/hscontrol/policy/v2/filter.go index a5ce1d86a..8b9124095 100644 --- a/hscontrol/policy/v2/filter.go +++ b/hscontrol/policy/v2/filter.go @@ -512,7 +512,7 @@ func sshPolicyForNode( // Handle autogroup:self destinations. // Tagged nodes can't match autogroup:self. - if cr.hasSelfDest && !node.IsTagged() && node.User().Valid() { + if cr.hasSelfDest && nodeHasUserIdentity(node) { rules = append(rules, cr.userRules[node.User().ID()]...) } diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index dd2ae2fe1..67ed60f13 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -695,7 +695,7 @@ func (pm *PolicyManager) sshRuleAllowsUser(rule SSH, srcNode types.NodeView, loc return true } - if srcNode.IsTagged() || !srcNode.User().Valid() || !rule.Users.ContainsLocalpart() { + if !nodeHasUserIdentity(srcNode) || !rule.Users.ContainsLocalpart() { return false } @@ -863,12 +863,11 @@ func (pm *PolicyManager) BuildPeerMap(nodes views.Slice[types.NodeView]) map[typ } ips, ri := n.IPs(), routeInfo[n.ID()] - routed := len(ri.subnet) > 0 for k := range pm.matchers { m := &pm.matchers[k] - if m.SrcsContainsIPs(ips...) || (routed && m.SrcsOverlapsRanges(ri.subnet)) { + if matcherReachesSource(m, ips, ri.subnet) { if srcOf[i] == nil { srcOf[i] = make([]uint64, words) } @@ -876,8 +875,7 @@ func (pm *PolicyManager) BuildPeerMap(nodes views.Slice[types.NodeView]) map[typ srcOf[i][k/64] |= 1 << (k % 64) //nolint:mnd } - if m.DestsContainsIP(ips...) || (routed && m.DestsOverlapsRanges(ri.subnet)) || - (ri.isExit && m.DestsIsTheInternet()) { + if matcherReachesDestination(m, ips, ri.subnet, ri.isExit) { if dstOf[i] == nil { dstOf[i] = make([]uint64, words) } @@ -971,6 +969,20 @@ func (pm *PolicyManager) BuildPeerMap(nodes views.Slice[types.NodeView]) map[typ return ret } +func matcherReachesSource(m *matcher.Match, ips []netip.Addr, routes []netipx.IPRange) bool { + return m.SrcsContainsIPs(ips...) || (len(routes) > 0 && m.SrcsOverlapsRanges(routes)) +} + +func matcherReachesDestination( + m *matcher.Match, + ips []netip.Addr, + routes []netipx.IPRange, + isExit bool, +) bool { + return m.DestsContainsIP(ips...) || (len(routes) > 0 && m.DestsOverlapsRanges(routes)) || + (isExit && m.DestsIsTheInternet()) +} + // filterRulesForNodeLocked returns the unreduced compiled filter rules // for a node, combining pre-compiled global rules with per-node self // and via rules from the stored compiled grants. @@ -1128,14 +1140,18 @@ func (pm *PolicyManager) SetUsers(users []types.User) (bool, bool, error) { // SSH rules and per-node filters embed user identity outside the filter // hash, so a user change needs a client refresh even when it did not move. - if hadPerNodeFilter || pm.needsPerNodeFilter || - (pm.pol != nil && len(pm.pol.SSHs) > 0) { + if pm.userChangeNeedsPolicyRefreshLocked(hadPerNodeFilter) { policyChanged = true } return policyChanged, true, nil } +func (pm *PolicyManager) userChangeNeedsPolicyRefreshLocked(hadPerNodeFilter bool) bool { + return hadPerNodeFilter || pm.needsPerNodeFilter || + (pm.pol != nil && len(pm.pol.SSHs) > 0) +} + // equalUsers compares user lists ignoring order and every field the policy // does not read, so a row touch such as an OIDC login is not a change. func equalUsers(a, b []types.User) bool { @@ -1736,8 +1752,7 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via } } - if viaPrimaryID == 0 || since.Before(viaPrimarySince) || - (since.Equal(viaPrimarySince) && node.ID() < viaPrimaryID) { + if isPreferredViaPrimary(node, since, viaPrimaryID, viaPrimarySince) { viaPrimaryID, viaPrimarySince = node.ID(), since } } @@ -1784,11 +1799,7 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via // routes the via grant added to Include. for _, dstPrefix := range resolvedDstPrefixes[i] { for _, p := range result.Include { - // Only a grant to the internet covers an exit route, - // as [types.Node.CanAccessRoutes] decides. - covers := dstPrefix.Overlaps(p) && - (reachesInternet || !tsaddr.IsExitRoute(p)) - if covers && !slices.Contains(result.UsePrimary, p) { + if grantCoversRoute(dstPrefix, p, reachesInternet) && !slices.Contains(result.UsePrimary, p) { result.UsePrimary = append(result.UsePrimary, p) } } @@ -1811,6 +1822,22 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via return result } +func isPreferredViaPrimary( + node types.NodeView, + since time.Time, + primaryID types.NodeID, + primarySince time.Time, +) bool { + return primaryID == 0 || since.Before(primarySince) || + (since.Equal(primarySince) && node.ID() < primaryID) +} + +// Only a grant to the internet covers an exit route, +// as [types.Node.CanAccessRoutes] decides. +func grantCoversRoute(dst, route netip.Prefix, reachesInternet bool) bool { + return dst.Overlaps(route) && (reachesInternet || !tsaddr.IsExitRoute(route)) +} + // grantReachesInternet reports whether a grant's destinations, resolved // to dsts, reach the internet by the test the grant's filter rule gets // ([matcher.Match.DestsIsTheInternet]). Neither the wildcard nor diff --git a/hscontrol/policy/v2/sshtest.go b/hscontrol/policy/v2/sshtest.go index d4c643754..05e0bb459 100644 --- a/hscontrol/policy/v2/sshtest.go +++ b/hscontrol/policy/v2/sshtest.go @@ -575,7 +575,7 @@ func sshTestDstNodes( } for _, n := range nodes.All() { - if !n.IsTagged() && n.User().Valid() && n.User().ID() == srcUserID { + if nodeHasUserIdentity(n) && n.User().ID() == srcUserID { out = append(out, n) } } diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index 9bbdcaf94..977520d1d 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -2635,12 +2635,7 @@ func (p *Policy) validate() error { // A check asks the source's user to log in again, and a tagged // node has none; Tailscale SaaS refuses this too. - if ssh.Action == SSHActionCheck && slices.ContainsFunc(ssh.Sources, func(a Alias) bool { - _, isTag := a.(*Tag) - ag, _ := a.(*AutoGroup) - - return isTag || ag.Is(AutoGroupTagged) - }) { + if sshCheckHasTaggedSource(ssh) { errs = append(errs, ErrSSHCheckTagSource) } @@ -3055,6 +3050,15 @@ type SSH struct { AcceptEnv []string `json:"acceptEnv,omitempty"` } +func sshCheckHasTaggedSource(ssh SSH) bool { + return ssh.Action == SSHActionCheck && slices.ContainsFunc(ssh.Sources, func(a Alias) bool { + _, isTag := a.(*Tag) + ag, _ := a.(*AutoGroup) + + return isTag || ag.Is(AutoGroupTagged) + }) +} + // SSHSrcAliases is a list of aliases that can be used as sources in an [SSH] rule. // It can be a list of usernames, groups, tags or autogroups. type SSHSrcAliases []Alias diff --git a/hscontrol/servertest/ha_property_test.go b/hscontrol/servertest/ha_property_test.go index cad6213f8..35a229203 100644 --- a/hscontrol/servertest/ha_property_test.go +++ b/hscontrol/servertest/ha_property_test.go @@ -296,8 +296,8 @@ func checkHAInvariants( ) } - if !prevStillApproves && hasPrimary && owner != prevOwner && - !st.IsNodeHealthy(owner) { + selectedReplacement := !prevStillApproves && hasPrimary && owner != prevOwner + if selectedReplacement && !st.IsNodeHealthy(owner) { rt.Fatalf( "prefix %s with no router available moved to %d, "+ "which the prober declared unreachable", diff --git a/hscontrol/state/node_store.go b/hscontrol/state/node_store.go index 95f0243c2..13c165d96 100644 --- a/hscontrol/state/node_store.go +++ b/hscontrol/state/node_store.go @@ -742,10 +742,15 @@ func resolveGivenName( taken[strings.ToLower(n.GivenName)] = struct{}{} } + labelAvailable := func(candidate string) bool { + _, busy := taken[strings.ToLower(candidate)] + + return !busy && (reservedLabel == nil || !reservedLabel(candidate)) + } + candidate := base for i := 1; ; i++ { - _, busy := taken[strings.ToLower(candidate)] - if !busy && (reservedLabel == nil || !reservedLabel(candidate)) { + if labelAvailable(candidate) { return candidate } @@ -907,6 +912,10 @@ func electPrimaryRoutes( return servesRoutes(&n) && !n.Unhealthy } healthy := func(id types.NodeID) bool { return !nodes[id].Unhealthy } + hasOlderAvailableSession := func(id, oldest types.NodeID) bool { + return available(id) && (oldest == 0 || + nodes[id].SessionStart.Before(nodes[oldest].SessionStart)) + } routes := make(map[netip.Prefix]types.NodeID, len(approvers)) for prefix, ids := range approvers { @@ -914,8 +923,7 @@ func electPrimaryRoutes( // ids ascend, so a tie keeps the lower ID. for _, id := range ids { - if available(id) && (oldest == 0 || - nodes[id].SessionStart.Before(nodes[oldest].SessionStart)) { + if hasOlderAvailableSession(id, oldest) { oldest = id } } diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index fb19edb9f..08a68270a 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -632,8 +632,7 @@ func (s *State) UpdateUser(userID types.UserID, updateFn func(*types.User) error owner := node.User() credential := node.AuthKey() owns := owner.Valid() && owner.ID() == user.ID - createdKey := credential.Valid() && credential.User().Valid() && - credential.User().ID() == user.ID + createdKey := credentialCreatedByUser(credential, user.ID) if !owns && !createdKey { continue @@ -647,7 +646,7 @@ func (s *State) UpdateUser(userID types.UserID, updateFn func(*types.User) error ownerRefreshed = ownerRefreshed || !n.IsTagged() } - if n.AuthKey != nil && n.AuthKey.User != nil && n.AuthKey.User.ID == user.ID { + if credentialCreatedByUser(n.AuthKey.View(), user.ID) { n.AuthKey = n.AuthKey.Clone() n.AuthKey.User = user.Clone() } @@ -675,6 +674,15 @@ func (s *State) UpdateUser(userID types.UserID, updateFn func(*types.User) error return user, c, nil } +func credentialCreatedByUser(credential types.CredentialView, userID uint) bool { + return credential.Valid() && credential.User().Valid() && credential.User().ID() == userID +} + +func taggedCredentialCreatedByUser(credential types.CredentialView, userID types.UserID) bool { + return credential.Valid() && credential.Tags().Len() > 0 && credential.UserID().Valid() && + types.UserID(credential.UserID().Get()) == userID +} + // DeleteUser permanently removes a user and all associated data (nodes, API keys, etc). // This operation is irreversible. // It also updates the policy manager to ensure ACL policies referencing the deleted @@ -689,7 +697,7 @@ func (s *State) DeleteUser(userID types.UserID) (change.Change, error) { // must stop naming it, as the database already does. dropCreator := func(n *types.Node) { k := n.AuthKey - if k == nil || !k.IsTagged() || k.UserID == nil || types.UserID(*k.UserID) != userID { + if !taggedCredentialCreatedByUser(k.View(), userID) { return } @@ -701,8 +709,7 @@ func (s *State) DeleteUser(userID types.UserID) (change.Change, error) { updates := make(map[types.NodeID]UpdateNodeFunc) for _, n := range s.nodeStore.ListNodes().All() { - if k := n.AuthKey(); k.Valid() && k.Tags().Len() > 0 && k.UserID().Valid() && - types.UserID(k.UserID().Get()) == userID { + if taggedCredentialCreatedByUser(n.AuthKey(), userID) { updates[n.ID()] = dropCreator } } @@ -1206,11 +1213,15 @@ func (s *State) BringNodeExpiryForward( expiry time.Time, ) (types.NodeView, change.Change, error) { return s.setNodeExpiry(nodeID, &expiry, func(node *types.Node) bool { - return node.NodeKey == nodeKey && - (node.Expiry == nil || node.Expiry.IsZero() || expiry.Before(*node.Expiry)) + return canBringExpiryForward(node, nodeKey, expiry) }) } +func canBringExpiryForward(node *types.Node, nodeKey key.NodePublic, expiry time.Time) bool { + return node.NodeKey == nodeKey && + (node.Expiry == nil || node.Expiry.IsZero() || expiry.Before(*node.Expiry)) +} + // setNodeExpiry sets the node's expiry. A non-nil allow decides on the node // as the writer holds it; a refusal writes nothing. func (s *State) setNodeExpiry( @@ -2010,7 +2021,7 @@ func (s *State) BatchSetNodeHealth(updates map[types.NodeID]bool) bool { for id := range fns { nv, ok := s.nodeStore.GetNode(id) - if ok && nv.Unhealthy() == !updates[id] && s.polMan.NodeIsViaTarget(nv) { + if ok && s.viaNodeHasHealth(nv, updates[id]) { return true } } @@ -2018,6 +2029,10 @@ func (s *State) BatchSetNodeHealth(updates map[types.NodeID]bool) bool { return false } +func (s *State) viaNodeHasHealth(node types.NodeView, healthy bool) bool { + return node.Unhealthy() == !healthy && s.polMan.NodeIsViaTarget(node) +} + // healthSetter returns an UpdateNodeFunc that flips n.Unhealthy to // the inverse of healthy, with the same gate as [State.SetNodeHealth]: // an unhealthy mark only sticks when the node is still online and @@ -2319,8 +2334,7 @@ func (s *State) SetAuthCacheEntry(id types.AuthID, entry *types.AuthRequest) err s.authCache.Remove(oldest) } - if (!entry.IsSSHCheck() && s.authCacheRegularEntries.Load() >= int64(s.authCacheMaxEntries)) || - s.authCache.Len() >= s.authCacheMaxEntries+authCacheSSHReserve { + if s.authCacheFullFor(entry) { return ErrAuthCacheFull } @@ -2338,6 +2352,12 @@ func (s *State) SetAuthCacheEntry(id types.AuthID, entry *types.AuthRequest) err return nil } +// authCacheFullFor is called with authCacheMu held. +func (s *State) authCacheFullFor(entry *types.AuthRequest) bool { + return (!entry.IsSSHCheck() && s.authCacheRegularEntries.Load() >= int64(s.authCacheMaxEntries)) || + s.authCache.Len() >= s.authCacheMaxEntries+authCacheSSHReserve +} + // DeleteAuthCacheEntry releases a consumed authentication transaction. func (s *State) DeleteAuthCacheEntry(id types.AuthID) { s.authCache.Remove(id) @@ -2588,10 +2608,7 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView // Apply default node expiry for non-tagged nodes when the // resolved expiry is still nil or zero (e.g., CLI registration // where the client did not request a specific expiry). - needsDefaultExpiry := !node.IsTagged() && - (node.Expiry == nil || node.Expiry.IsZero()) && - s.cfg.Node.Expiry > 0 - if needsDefaultExpiry { + if s.needsDefaultNodeExpiry(node) { exp := time.Now().Add(s.cfg.Node.Expiry) node.Expiry = &exp } @@ -2747,10 +2764,7 @@ func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, erro // Apply default node expiry for non-tagged nodes when the client // did not request a specific expiry. // Tagged nodes are exempt — they never expire. - needsDefaultExpiry := !nodeToRegister.IsTagged() && - (nodeToRegister.Expiry == nil || nodeToRegister.Expiry.IsZero()) && - s.cfg.Node.Expiry > 0 - if needsDefaultExpiry { + if s.needsDefaultNodeExpiry(&nodeToRegister) { exp := time.Now().Add(s.cfg.Node.Expiry) nodeToRegister.Expiry = &exp } @@ -2839,6 +2853,15 @@ func (s *State) validateRequestTags(node types.NodeView, requestTags []string) [ return rejectedTags } +func (s *State) needsDefaultNodeExpiry(node *types.Node) bool { + return !node.IsTagged() && (node.Expiry == nil || node.Expiry.IsZero()) && s.cfg.Node.Expiry > 0 +} + +func (s *State) canApplyReauthTag(node types.NodeView, authUser types.UserView, tag string) bool { + return s.polMan.NodeCanHaveTag(node, tag) || + (authUser.Valid() && s.polMan.UserCanHaveTag(authUser, tag)) +} + // validateRequestTagsForReauth authorises re-auth request tags against the // existing node OR the authenticating user. A tag-owned node (#3374) has no // user and its IP is in no owner set, so NodeCanHaveTag alone rejects every @@ -2853,11 +2876,7 @@ func (s *State) validateRequestTagsForReauth(node types.NodeView, authUser types var rejectedTags []string for _, tag := range requestTags { - if s.polMan.NodeCanHaveTag(node, tag) { - continue - } - - if authUser.Valid() && s.polMan.UserCanHaveTag(authUser, tag) { + if s.canApplyReauthTag(node, authUser, tag) { continue } @@ -2926,8 +2945,7 @@ func (s *State) processReauthTags( var approvedTags, rejectedTags []string for _, tag := range requestTags { - if s.polMan.NodeCanHaveTag(node.View(), tag) || - (authUser.Valid() && s.polMan.UserCanHaveTag(authUser, tag)) { + if s.canApplyReauthTag(node.View(), authUser, tag) { approvedTags = append(approvedTags, tag) } else { rejectedTags = append(rejectedTags, tag) @@ -3038,7 +3056,7 @@ func (s *State) HandleNodeFromAuthPath( existingNodeOwnedByOtherUser := false for uid, n := range all { - if uid != 0 && uid != types.UserID(user.ID) && !n.IsTagged() { + if isOtherUserOwnedNode(n, uid, types.UserID(user.ID)) { existingNodeOtherUser = n existingNodeOwnedByOtherUser = true } @@ -3226,7 +3244,7 @@ func (s *State) findExistingNodeForPAK( // key under its own node key is the device restarting (#3312); // pakReregister lets that key only wake it. node, ok = all[0] - ok = ok && node.IsTagged() && (pak.CanReauthenticate(node) || node.NodeKey() == nodeKey) + ok = ok && canReuseTaggedNodeWithKey(node, pak, nodeKey) } return node, ok, nil @@ -3252,14 +3270,25 @@ func loginNode(all map[types.UserID]types.NodeView, userID types.UserID) (types. return node, true } - if node, ok := all[0]; ok && node.IsTagged() && - (node.ClaimableBy(userID) || node.RegisterMethod() != util.RegisterMethodAuthKey) { + if node, ok := all[0]; ok && canClaimTaggedNode(node, userID) { return node, true } return types.NodeView{}, false } +func isOtherUserOwnedNode(node types.NodeView, ownerID, userID types.UserID) bool { + return ownerID != 0 && ownerID != userID && !node.IsTagged() +} + +func canReuseTaggedNodeWithKey(node types.NodeView, pak *types.PreAuthKey, nodeKey key.NodePublic) bool { + return node.IsTagged() && (pak.CanReauthenticate(node) || node.NodeKey() == nodeKey) +} + +func canClaimTaggedNode(node types.NodeView, userID types.UserID) bool { + return node.IsTagged() && (node.ClaimableBy(userID) || node.RegisterMethod() != util.RegisterMethodAuthKey) +} + // pakSkipsValidation reports whether re-registering n with pak may skip key // validation at now. The machine key proves identity and a pre-auth key only // authorises the initial join, so a plain restart (container "tailscale up @@ -3315,6 +3344,50 @@ func pakReregister(n types.NodeView, pak *types.PreAuthKey, nodeKey key.NodePubl return false, err } +// requestedTagsForPreAuthKey returns an authorised replacement set, or nil +// when the key's tags should be kept. +// Advertising a subset of the key's tags is redundant, not an escalation, +// and the tailscale client's OAuth authkey flow always does it. Other +// requested tags replace the key's when a tagged key's tags own each one, +// or a user's key's user owns it directly, as on Tailscale SaaS; otherwise +// they are refused, ahead of the key's own error, so new nodes and +// re-registrations are held to the same rule. +func (s *State) requestedTagsForPreAuthKey(hostinfo *tailcfg.Hostinfo, pak *types.PreAuthKey) ([]string, error) { + if !requestsTagsOutsideKey(hostinfo, pak.Tags) { + return nil, nil + } + + var refused []string + + for _, tag := range hostinfo.RequestTags { + if !s.preAuthKeyCanOwnTag(pak, tag) { + refused = append(refused, tag) + } + } + + if len(refused) > 0 { + return nil, fmt.Errorf("%w %v are invalid or not permitted", ErrRequestedTagsInvalidOrNotPermitted, refused) + } + + // Sort a copy: RequestTags is the node's reported Hostinfo. + return slices.Compact(slices.Sorted(slices.Values(hostinfo.RequestTags))), nil +} + +func requestsTagsOutsideKey(hostinfo *tailcfg.Hostinfo, keyTags []string) bool { + return hostinfo != nil && slices.ContainsFunc(hostinfo.RequestTags, func(tag string) bool { + return !slices.Contains(keyTags, tag) + }) +} + +func (s *State) preAuthKeyCanOwnTag(pak *types.PreAuthKey, tag string) bool { + return s.polMan.TagOwnedByTags(tag, pak.Tags) || + (!pak.IsTagged() && pak.User != nil && s.polMan.UserCanHaveTag(pak.User.View(), tag)) +} + +func requestedTagsErrorTakesPrecedence(tagsErr, keyErr error, reauth bool) bool { + return tagsErr != nil && !errors.Is(keyErr, ErrNodeKeyExpired) && (reauth || keyErr != nil) +} + //nolint:gocyclo // sequential validation/update/create paths with security-sensitive ordering func (s *State) HandleNodeFromPreAuthKey( regReq tailcfg.RegisterRequest, @@ -3332,7 +3405,7 @@ func (s *State) HandleNodeFromPreAuthKey( return types.NodeView{}, s.policyChangeSince(genBefore), err } - if !pak.Reusable && !pak.Used { + if shouldConsumePreAuthKey(pak) { mu := &s.singleUseKeyLocks[pak.ID%uint64(len(s.singleUseKeyLocks))] mu.Lock() defer mu.Unlock() @@ -3344,37 +3417,7 @@ func (s *State) HandleNodeFromPreAuthKey( } } - // Advertising a subset of the key's tags is redundant, not an escalation, - // and the tailscale client's OAuth authkey flow always does it. Other - // requested tags replace the key's when a tagged key's tags own each one, - // or a user's key's user owns it directly, as on Tailscale SaaS; otherwise - // they are refused, ahead of the key's own error, so new nodes and - // re-registrations are held to the same rule. - var ( - requestedTags []string - tagsErr error - ) - - if regReq.Hostinfo != nil && slices.ContainsFunc(regReq.Hostinfo.RequestTags, func(tag string) bool { - return !slices.Contains(pak.Tags, tag) - }) { - var refused []string - - for _, tag := range regReq.Hostinfo.RequestTags { - owned := s.polMan.TagOwnedByTags(tag, pak.Tags) || - (!pak.IsTagged() && pak.User != nil && s.polMan.UserCanHaveTag(pak.User.View(), tag)) - if !owned { - refused = append(refused, tag) - } - } - - if len(refused) > 0 { - tagsErr = fmt.Errorf("%w %v are invalid or not permitted", ErrRequestedTagsInvalidOrNotPermitted, refused) - } else { - // Sort a copy: RequestTags is the node's reported Hostinfo. - requestedTags = slices.Compact(slices.Sorted(slices.Values(regReq.Hostinfo.RequestTags))) - } - } + requestedTags, tagsErr := s.requestedTagsForPreAuthKey(regReq.Hostinfo, pak) existingNodeSameUser, existsSameUser, err := s.findExistingNodeForPAK(machineKey, pak, regReq.NodeKey) if err != nil { @@ -3392,7 +3435,7 @@ func (s *State) HandleNodeFromPreAuthKey( } // A wake-up writes no tags, so the tags it asks for are not refused. - if tagsErr != nil && !errors.Is(err, ErrNodeKeyExpired) && (reauth || err != nil) { + if requestedTagsErrorTakesPrecedence(tagsErr, err, reauth) { return types.NodeView{}, s.policyChangeSince(genBefore), tagsErr } @@ -3450,7 +3493,7 @@ func (s *State) HandleNodeFromPreAuthKey( if pak.User != nil { for uid, node := range s.nodeStore.GetNodesByMachineKeyAllUsers(machineKey) { - if uid != 0 && !node.IsTagged() && uid != types.UserID(pak.User.ID) { + if isOtherUserOwnedNode(node, uid, types.UserID(pak.User.ID)) { differentUserNode = node belongsToDifferentUser = true } @@ -3551,8 +3594,7 @@ func (s *State) approveRoutesAfterRegistration( before, node types.NodeView, c change.Change, ) (types.NodeView, change.Change, error) { - if before.Valid() && before.NodeKey() == node.NodeKey() && - slices.Equal(sortedPrefixes(before.AnnouncedRoutes()), sortedPrefixes(node.AnnouncedRoutes())) { + if registrationPreservesRouteInputs(before, node) { return node, c, nil } @@ -3566,6 +3608,19 @@ func (s *State) approveRoutesAfterRegistration( return node, c, nil } +func registrationPreservesRouteInputs(before, node types.NodeView) bool { + return before.Valid() && before.NodeKey() == node.NodeKey() && + slices.Equal(sortedPrefixes(before.AnnouncedRoutes()), sortedPrefixes(node.AnnouncedRoutes())) +} + +func preAuthKeyClaimsTaggedNode(node *types.Node, pak *types.PreAuthKey, requestedTags []string, previousNodeKey key.NodePublic) bool { + return requestedTags == nil && node.IsTagged() && !pak.IsTagged() && previousNodeKey != node.NodeKey +} + +func shouldConsumePreAuthKey(pak *types.PreAuthKey) bool { + return !pak.Reusable && !pak.Used +} + // reregisterNodeWithPAK updates an existing node in place for a pre-auth key // re-registration: NodeStore first, then the database. // @@ -3686,8 +3741,7 @@ func (s *State) reregisterNodeWithPAK( if wasUserOwned || node.IsExpiredAt(now) { node.Expiry = nil } - } else if requestedTags == nil && node.IsTagged() && !pak.IsTagged() && - prior.NodeKey != regReq.NodeKey { + } else if preAuthKeyClaimsTaggedNode(node, pak, requestedTags, prior.NodeKey) { // A user key that speaks for a tagged node, with a new node key, // takes it back as that user's device, as on Tailscale SaaS. A // relogin under the same node key keeps it tagged. As at a login @@ -3703,7 +3757,7 @@ func (s *State) reregisterNodeWithPAK( // If this registration will consume a single-use key (the tx below // calls UsePreAuthKey under the same condition), reflect that in the // cached AuthKey so the NodeStore copy matches the database. - if !pak.Reusable && !pak.Used { + if shouldConsumePreAuthKey(pak) { node.AuthKey.Used = true } @@ -3766,7 +3820,7 @@ func (s *State) reregisterNodeWithPAK( // atomic compare-and-set in [hsdb.UsePreAuthKey] would otherwise // reject it as "authkey already used", locking out containers // that restart with the same one-shot key. - if reauth && !pak.Reusable && !pak.Used { + if reauth && shouldConsumePreAuthKey(pak) { err := hsdb.UsePreAuthKey(tx, pak) if err != nil { return fmt.Errorf("using pre auth key: %w", err) @@ -3793,6 +3847,11 @@ func (s *State) reregisterNodeWithPAK( return updatedNodeView, before, nil } +func registrationOwnershipStillMatches(node, written *types.Node) bool { + return slices.Equal(node.Tags, written.Tags) && + reflect.DeepEqual(node.UserID, written.UserID) && reflect.DeepEqual(node.User, written.User) +} + // undoReregistration puts back the fields of a failed re-registration that // still hold what it wrote, so later admin and map writes survive, and saves // the result. LastSeen stays because the node did contact us; sessions, @@ -3815,9 +3874,7 @@ func (s *State) undoReregistration(prior, written *types.Node) { n.RegisterMethod = prior.RegisterMethod } - if slices.Equal(n.Tags, written.Tags) && - reflect.DeepEqual(n.UserID, written.UserID) && - reflect.DeepEqual(n.User, written.User) { + if registrationOwnershipStillMatches(n, written) { n.Tags = prior.Tags n.UserID = prior.UserID n.User = prior.User @@ -4061,6 +4118,14 @@ func isAutoDerivedGivenName(given, hostname string) bool { return err == nil && given == bumpedLabel(base, n) } +func dnsMetadataChanged(previous, reported *tailcfg.Hostinfo) bool { + return reported != nil && (previous == nil || previous.Hostname != reported.Hostname || previous.OS != reported.OS) +} + +func hasReportedHostnameChange(hostname string, reported *tailcfg.Hostinfo) bool { + return reported != nil && reported.Hostname != "" && reported.Hostname != hostname +} + // UpdateNodeFromMapRequest is the sync point where Hostinfo changes, // endpoint updates, and route advertisements from a [tailcfg.MapRequest] // land in the [NodeStore]. It produces a [change.Change] summarising @@ -4154,10 +4219,7 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest !hostinfoEqual(currentNode.Hostinfo, newHostinfo) delta.peerHostinfoChanged = newHostinfo != nil && !peerHostinfoEqual(currentNode.Hostinfo.View(), newHostinfo.View()) - delta.dnsMetadataChanged = newHostinfo != nil && - (currentNode.Hostinfo == nil || - currentNode.Hostinfo.Hostname != newHostinfo.Hostname || - currentNode.Hostinfo.OS != newHostinfo.OS) + delta.dnsMetadataChanged = dnsMetadataChanged(currentNode.Hostinfo, newHostinfo) // A change carrying only an updated LastSeen is not worth a // full-row database UPDATE plus the O(n) policy rescan @@ -4181,8 +4243,7 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest // Only a real hostname change may re-derive GivenName: it is peer // visible, so the whole node is resent and peers learn the name. - if newHostinfo != nil && newHostinfo.Hostname != "" && - newHostinfo.Hostname != currentNode.Hostname { + if hasReportedHostnameChange(currentNode.Hostname, newHostinfo) { // Preserve an admin-renamed GivenName: only auto-derive // when the current GivenName is still what // SanitizeHostname of the old Hostname would produce diff --git a/hscontrol/types/change/change.go b/hscontrol/types/change/change.go index 4e1561ffc..97bbe55c3 100644 --- a/hscontrol/types/change/change.go +++ b/hscontrol/types/change/change.go @@ -131,8 +131,7 @@ func (r Change) Merge(other Change) Change { } func (r Change) IsEmpty() bool { - if r.IncludeSelf || r.IncludeDERPMap || r.IncludeDNS || - r.IncludeDomain || r.IncludePolicy || r.SendAllPeers { + if r.IncludeSelf || r.hasConfigUpdate() || r.SendAllPeers { return false } @@ -144,10 +143,7 @@ func (r Change) IsEmpty() bool { return false } - return len(r.PeersChanged) == 0 && - len(r.PeersRemoved) == 0 && - len(r.DeletedNodes) == 0 && - len(r.PeerPatches) == 0 + return !r.hasPeerChanges() && len(r.PeerPatches) == 0 } func (r Change) IsSelfOnly() bool { @@ -155,8 +151,7 @@ func (r Change) IsSelfOnly() bool { return false } - if r.SendAllPeers || len(r.PeersChanged) > 0 || len(r.PeersRemoved) > 0 || - len(r.DeletedNodes) > 0 || len(r.PeerPatches) > 0 { + if r.SendAllPeers || r.hasPeerChanges() || len(r.PeerPatches) > 0 { return false } @@ -195,16 +190,15 @@ func (r Change) Type() string { return "policy" } - if len(r.PeerPatches) > 0 && len(r.PeersChanged) == 0 && len(r.PeersRemoved) == 0 && !r.SendAllPeers { + if len(r.PeerPatches) > 0 && !r.hasPeerRefresh() { return "patch" } - if len(r.PeersChanged) > 0 || len(r.PeersRemoved) > 0 || - len(r.DeletedNodes) > 0 || r.SendAllPeers { + if r.hasPeerChanges() || r.SendAllPeers { return "peers" } - if r.IncludeDERPMap || r.IncludeDNS || r.IncludeDomain || r.IncludePolicy { + if r.hasConfigUpdate() { return "config" } @@ -215,6 +209,18 @@ func (r Change) Type() string { return "unknown" } +func (r Change) hasConfigUpdate() bool { + return r.IncludeDERPMap || r.IncludeDNS || r.IncludeDomain || r.IncludePolicy +} + +func (r Change) hasPeerChanges() bool { + return len(r.PeersChanged) > 0 || len(r.PeersRemoved) > 0 || len(r.DeletedNodes) > 0 +} + +func (r Change) hasPeerRefresh() bool { + return len(r.PeersChanged) > 0 || len(r.PeersRemoved) > 0 || r.SendAllPeers +} + // ShouldSendToNode determines if this response should be sent to nodeID. // It handles self-only targeting and filtering out self-updates for non-origin nodes. func (r Change) ShouldSendToNode(nodeID types.NodeID) bool { @@ -315,8 +321,7 @@ func DedupePolicyChanges(changes []Change) []Change { // isPeerDelta reports whether r only adds, refreshes or patches peers other // than nodeID: [CoalescePeerDeltas] can fold such changes together. func (r Change) isPeerDelta(nodeID types.NodeID) bool { - if r.IncludeSelf || r.IncludeDERPMap || r.IncludeDNS || r.IncludeDomain || - r.IncludePolicy || r.SendAllPeers || r.RequiresRuntimePeerComputation { + if r.IncludeSelf || r.hasConfigUpdate() || r.SendAllPeers || r.RequiresRuntimePeerComputation { return false }