From bb6da35417679ea5e584d2a41f98d14275e352da Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 21 Oct 2026 23:37:47 +0000 Subject: [PATCH] policy/v2: warn once at load about malformed nextdns attrs SaaS loads them and its resolver skips them, as headscale does, but the mapper also logged a warning per node on every DNS build. --- hscontrol/mapper/mapper.go | 34 +++++----------------- 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 + 5 files changed, 107 insertions(+), 26 deletions(-) diff --git a/hscontrol/mapper/mapper.go b/hscontrol/mapper/mapper.go index c3fb74faa..2c90ed8ff 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 } 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.