From cb47fdff36c0dfe3e5714b71031e845b26ddddd2 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 21 Oct 2026 23:37:47 +0000 Subject: [PATCH] policy/v2, mapper: align NextDNS warnings and device metadata Warn about malformed attributes once at policy load, not on each map. Include the node's stable ID as device_id in resolver queries. --- hscontrol/mapper/mapper.go | 35 ++++++---------------- hscontrol/mapper/mapper_test.go | 12 ++------ hscontrol/policy/v2/nodeattrs_test.go | 42 +++++++++++++++++++++++++++ hscontrol/policy/v2/policy.go | 21 ++++++++++++++ hscontrol/policy/v2/types.go | 35 ++++++++++++++++++++++ hscontrol/util/zlog/zf/fields.go | 1 + 6 files changed, 111 insertions(+), 35 deletions(-) diff --git a/hscontrol/mapper/mapper.go b/hscontrol/mapper/mapper.go index c3fb74faa..9053f3459 100644 --- a/hscontrol/mapper/mapper.go +++ b/hscontrol/mapper/mapper.go @@ -8,19 +8,18 @@ import ( "net/url" "os" "path" - "regexp" "slices" "strconv" "strings" "time" + policyv2 "github.com/juanfont/headscale/hscontrol/policy/v2" "github.com/juanfont/headscale/hscontrol/state" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/types/change" "github.com/rs/zerolog/log" "tailscale.com/envknob" "tailscale.com/tailcfg" - "tailscale.com/tailcfg/nodecap" "tailscale.com/types/dnstype" "tailscale.com/types/views" ) @@ -101,21 +100,6 @@ func generateUserProfiles( return profiles } -// nextDNSAttrPrefix is the form Tailscale uses for per-node NextDNS profile -// selection: an "attr" entry of "nextdns:" overrides the resolver -// path, and "nextdns:no-device-info" suppresses the metadata-appending step. -// See https://tailscale.com/docs/integrations/nextdns. -const ( - nextDNSAttrPrefix = "nextdns:" - nextDNSAttrNoInfo nodecap.Cap = "nextdns:no-device-info" -) - -// nextDNSProfileRE bounds the characters accepted in a `nextdns:` -// suffix. NextDNS profile IDs are short alphanumeric strings; restricting -// to that charset prevents a policy author from injecting `?`, `/`, `@`, -// or `..` into the resolver URL via a crafted cap name. -var nextDNSProfileRE = regexp.MustCompile(`^[A-Za-z0-9._-]{1,64}$`) - func generateDNSConfig( cfg *types.Config, node types.NodeView, @@ -137,7 +121,7 @@ func generateDNSConfig( } } - if _, suppressMetadata := capMap[nextDNSAttrNoInfo]; !suppressMetadata { + if _, suppressMetadata := capMap[policyv2.NextDNSAttrNoInfo]; !suppressMetadata { addNextDNSMetadata(dnsConfig.Resolvers, node) addNextDNSMetadata(dnsConfig.FallbackResolvers, node) @@ -165,7 +149,7 @@ func generateDNSConfig( // order in Go is randomised, so taking the literal first match would // cause the chosen profile to flip between reloads when a node has // multiple `nextdns:` caps. The profile string is also validated -// against [nextDNSProfileRE] so a crafted cap cannot inject path or +// against [policyv2.NextDNSProfileRE] so a crafted cap cannot inject path or // query characters into the resolver URL. func nextDNSProfileFromCapMap(capMap tailcfg.NodeCapMap) string { if len(capMap) == 0 { @@ -175,20 +159,18 @@ func nextDNSProfileFromCapMap(capMap tailcfg.NodeCapMap) string { candidates := make([]string, 0, len(capMap)) for attr := range capMap { - if attr == nextDNSAttrNoInfo { + if attr == policyv2.NextDNSAttrNoInfo { continue } - profile, ok := strings.CutPrefix(string(attr), nextDNSAttrPrefix) + profile, ok := strings.CutPrefix(string(attr), policyv2.NextDNSAttrPrefix) if !ok || profile == "" { continue } - if !nextDNSProfileRE.MatchString(profile) { - log.Warn(). - Str("cap", string(attr)). - Msg("nextdns profile rejected: must match [A-Za-z0-9._-]{1,64}") - + // Skipped in silence: the policy warns once when it loads, and this + // check guards the URL splice. + if !policyv2.NextDNSProfileRE.MatchString(profile) { continue } @@ -244,6 +226,7 @@ func addNextDNSMetadata(resolvers []*dnstype.Resolver, node types.NodeView) { } q := u.Query() + q.Set("device_id", string(node.ID().StableID())) q.Set("device_name", node.Hostname()) // Guard Hostinfo().Valid() before dereferencing OS(): a node loaded diff --git a/hscontrol/mapper/mapper_test.go b/hscontrol/mapper/mapper_test.go index d81d0317f..603ada442 100644 --- a/hscontrol/mapper/mapper_test.go +++ b/hscontrol/mapper/mapper_test.go @@ -154,7 +154,7 @@ func TestNextDNSCapMapRendering(t *testing.T) { nil, ) - want := "https://dns.nextdns.io/abc?device_ip=100.64.0.1&device_model=linux&device_name=node1" + want := "https://dns.nextdns.io/abc?device_id=1&device_ip=100.64.0.1&device_model=linux&device_name=node1" if addr := resolverAddr(t, got); addr != want { t.Errorf("addr = %q, want %q", addr, want) } @@ -173,7 +173,7 @@ func TestNextDNSCapMapRendering(t *testing.T) { capMap, ) - want := "https://dns.nextdns.io/override?device_ip=100.64.0.1&device_model=linux&device_name=node1" + want := "https://dns.nextdns.io/override?device_id=1&device_ip=100.64.0.1&device_model=linux&device_name=node1" if addr := resolverAddr(t, got); addr != want { t.Errorf("addr = %q, want %q", addr, want) } @@ -221,13 +221,7 @@ func TestNextDNSCapMapRendering(t *testing.T) { // nextDNSResolverSkipReasons lists checks where headscale's DoH address // differs from SaaS, keyed /. -var nextDNSResolverSkipReasons = map[string]string{ - "nodeattrs-nextdns-d10-resolver-default/device_id": nextDNSDeviceID, - "nodeattrs-nextdns-d11-resolver-profile/device_id": nextDNSDeviceID, - "nodeattrs-nextdns-d13-resolver-malformed/device_id": nextDNSDeviceID, -} - -const nextDNSDeviceID = "NEXTDNS_DEVICE_ID: SaaS adds the node's stable ID to the DoH query as device_id; headscale does not" +var nextDNSResolverSkipReasons = map[string]string{} // TestNextDNSResolverCompat compares the NextDNS DoH address every node gets // with what SaaS sent: the global profile, a per-node profile, and a diff --git a/hscontrol/policy/v2/nodeattrs_test.go b/hscontrol/policy/v2/nodeattrs_test.go index 4a70164a5..3cd4b31b9 100644 --- a/hscontrol/policy/v2/nodeattrs_test.go +++ b/hscontrol/policy/v2/nodeattrs_test.go @@ -1,13 +1,17 @@ package v2 import ( + "bytes" "net/netip" "slices" + "strings" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/juanfont/headscale/hscontrol/types" + "github.com/rs/zerolog" + "github.com/rs/zerolog/log" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "tailscale.com/tailcfg" @@ -364,3 +368,41 @@ func TestNodesWithChangedCapMap(t *testing.T) { assert.Empty(t, pm.NodesWithChangedCapMap(), "reloading the same policy must not produce CapMap diffs") } + +// TestMalformedNextDNSAttrWarnsOnceAtLoad pins that a nextdns: attr whose +// profile the resolver skips still loads and stays in the CapMap, as on +// Tailscale SaaS, and is logged once per policy load. Not parallel: it swaps +// the global logger and level. +func TestMalformedNextDNSAttrWarnsOnceAtLoad(t *testing.T) { + var buf bytes.Buffer + + prev, prevLevel := log.Logger, zerolog.GlobalLevel() + log.Logger = zerolog.New(&buf) + + zerolog.SetGlobalLevel(zerolog.WarnLevel) + + t.Cleanup(func() { + log.Logger = prev + + zerolog.SetGlobalLevel(prevLevel) + }) + + users := nodeAttrsTestUsers() + nodes := nodeAttrsTestNodes(users) + + pol := []byte(`{ + "tagOwners": {` + nodeAttrsTagOwners + `}, + "nodeAttrs": [{"target": ["*"], "attr": ["nextdns:a/b?c", "nextdns:abc123"]}] + }`) + + pm, err := NewPolicyManager(pol, users, nodes.ViewSlice()) + require.NoError(t, err) + + // The write path reloads the bytes it just set. + _, err = pm.SetPolicy(pol) + require.NoError(t, err) + + assert.Contains(t, pm.NodeCapMap(1), nodecap.Cap("nextdns:a/b?c")) + assert.Equal(t, 1, strings.Count(buf.String(), "nextdns:a/b?c"), buf.String()) + assert.NotContains(t, buf.String(), "nextdns:abc123") +} diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 8b53c5c4e..1faa25781 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -17,6 +17,7 @@ import ( "github.com/juanfont/headscale/hscontrol/policy/policyutil" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" + "github.com/juanfont/headscale/hscontrol/util/zlog/zf" "github.com/puzpuzpuz/xsync/v4" "github.com/rs/zerolog/log" "go4.org/netipx" @@ -248,6 +249,8 @@ func NewPolicyManager(b []byte, users []types.User, nodes views.Slice[types.Node return nil, err } + warnMalformedNextDNSAttrs(policy) + // Boot path: log a warning if the stored policy's tests would // fail against the current users and nodes, but keep the server // running. A stale stored policy (e.g. referencing a user that @@ -752,9 +755,27 @@ func (pm *PolicyManager) SetPolicy(polB []byte) (bool, error) { return false, err } + // Reloading the same bytes leaves changed false, so each policy warns once. + if changed { + warnMalformedNextDNSAttrs(pol) + } + return changed, nil } +// warnMalformedNextDNSAttrs logs, once per policy load, the nextdns: attrs +// the resolver skips, which the mapper does in silence on every DNS build. +func warnMalformedNextDNSAttrs(pol *Policy) { + bad := pol.malformedNextDNSAttrs() + if len(bad) == 0 { + return + } + + log.Warn(). + Strs(zf.NodeAttrs, bad). + Msg("nextdns attrs have no effect: a profile must match [A-Za-z0-9._-]{1,64}") +} + // Filter returns the current filter rules for the entire tailnet and the associated matchers. func (pm *PolicyManager) Filter() ([]tailcfg.FilterRule, []matcher.Match) { if pm == nil { diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index fe4bab998..3852bc963 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -6,6 +6,7 @@ import ( "fmt" "iter" "net/netip" + "regexp" "slices" "strconv" "strings" @@ -111,6 +112,40 @@ var ( ErrNodeAttrTargetUnsupported = errors.New("nodeAttrs target alias type is not supported") ) +// NextDNSAttrPrefix is the form Tailscale uses for per-node NextDNS profile +// selection: an attr of "nextdns:" overrides the resolver path, +// and [NextDNSAttrNoInfo] suppresses the device metadata. +// See https://tailscale.com/docs/integrations/nextdns. +const ( + NextDNSAttrPrefix = "nextdns:" + NextDNSAttrNoInfo nodecap.Cap = "nextdns:no-device-info" +) + +// NextDNSProfileRE bounds a profile ID, which is spliced into the resolver +// URL path, so it cannot carry `?`, `/` or `@`. +var NextDNSProfileRE = regexp.MustCompile(`^[A-Za-z0-9._-]{1,64}$`) + +// malformedNextDNSAttrs returns the nextdns: attrs whose profile the resolver +// skips. They load, as on Tailscale SaaS, and stay in the CapMap. +func (p *Policy) malformedNextDNSAttrs() []string { + if p == nil { + return nil + } + + var bad []string + + for _, na := range p.NodeAttrs { + for _, attr := range na.Attrs { + profile, ok := strings.CutPrefix(string(attr), NextDNSAttrPrefix) + if ok && attr != NextDNSAttrNoInfo && !NextDNSProfileRE.MatchString(profile) { + bad = append(bad, string(attr)) + } + } + } + + return bad +} + // nodeAttrUnsupportedCaps lists caps that headscale parses but cannot act on // today. Each entry maps to the tracking issue an operator can follow. The // caps are accepted by Tailscale SaaS, but delivering them via headscale diff --git a/hscontrol/util/zlog/zf/fields.go b/hscontrol/util/zlog/zf/fields.go index c3f1f5ff0..33ec8b33d 100644 --- a/hscontrol/util/zlog/zf/fields.go +++ b/hscontrol/util/zlog/zf/fields.go @@ -137,6 +137,7 @@ const ( AutoApproveHashNew = "autoApprove.hash.new" ExitSetHashOld = "exitSet.hash.old" ExitSetHashNew = "exitSet.hash.new" + NodeAttrs = "nodeAttrs" ) // Connection/Channel fields.