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

@@ -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:<profile-id>" 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:<profile>`
-// 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
 		}
 

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")
+}

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 {

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:<profile-id>" 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

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.