work

Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>

.golangci.yaml

@@ -1,53 +1,39 @@
 ---
-run:
-  timeout: 10m
-  build-tags:
-    - ts2019
-
-issues:
-  skip-dirs:
-    - gen
+version: "2"
 linters:
-  enable-all: true
+  default: all
   disable:
-    - revive
-    - lll
-    - gofmt
+    - cyclop
+    - depguard
+    - dupl
+    - exhaustruct
+    - funlen
     - gochecknoglobals
     - gochecknoinits
     - gocognit
-    - funlen
-    - tagliatelle
     - godox
-    - ireturn
-    - execinquery
-    - exhaustruct
-    - nolintlint
-    - musttag # causes issues with imported libs
-    - depguard
-    - exportloopref
-    - tenv
-
-    # We should strive to enable these:
-    - wrapcheck
-    - dupl
-    - makezero
-    - maintidx
-
-    # Limits the methods of an interface to 10. We have more in integration tests
     - interfacebloat
-
-    # We might want to enable this, but it might be a lot of work
-    - cyclop
+    - ireturn
+    - lll
+    - maintidx
+    - makezero
+    - musttag
     - nestif
-    - wsl # might be incompatible with gofumpt
-    - testpackage
+    - nolintlint
     - paralleltest
-
-linters-settings:
+    - revive
+    - tagliatelle
+    - testpackage
+    - wrapcheck
+    - wsl
+  settings:
+    gocritic:
+      disabled-checks:
+        - appendAssign
+        - ifElseChain
+    nlreturn:
+      block-size: 4
     varnamelen:
-    ignore-type-assert-ok: true
-    ignore-map-index-ok: true
       ignore-names:
         - err
         - db
@@ -63,12 +49,31 @@ linters-settings:
         - pr
         - p
         - p2
+      ignore-type-assert-ok: true
+      ignore-map-index-ok: true
+  exclusions:
+    generated: lax
+    presets:
+      - comments
+      - common-false-positives
+      - legacy
+      - std-error-handling
+    paths:
+      - third_party$
+      - builtin$
+      - examples$
+      - gen
 
-  gocritic:
-    disabled-checks:
-      - appendAssign
-      # TODO(kradalby): Remove this
-      - ifElseChain
-
-  nlreturn:
-    block-size: 4
+formatters:
+  enable:
+    - gci
+    - gofmt
+    - gofumpt
+    - goimports
+  exclusions:
+    generated: lax
+    paths:
+      - third_party$
+      - builtin$
+      - examples$
+      - gen

hscontrol/auth.go

@@ -285,9 +285,6 @@ func (h *Headscale) handleRegisterInteractive(
 		nodeToRegister.Node.Expiry = &regReq.Expiry
 	}
 
-	// Ensure any auto approved routes are handled before saving.
-	policy.AutoApproveRoutes(h.polMan, &nodeToRegister.Node)
-
 	h.registrationCache.Set(
 		registrationId,
 		nodeToRegister,

hscontrol/policy/policy.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/juanfont/headscale/hscontrol/types"
 	"github.com/juanfont/headscale/hscontrol/util"
+	"github.com/rs/zerolog/log"
 	"github.com/samber/lo"
 	"tailscale.com/net/tsaddr"
 	"tailscale.com/tailcfg"
@@ -87,9 +88,13 @@ func AutoApproveRoutes(pm PolicyManager, node *types.Node) bool {
 	if pm == nil {
 		return false
 	}
+
+	log.Trace().Msgf("AUTO APPROVE: node %s %d", node.Hostname, node.ID)
 	var newApproved []netip.Prefix
 	for _, route := range node.AnnouncedRoutes() {
+		log.Trace().Msgf("AUTO APPROVE: node %s %d, checking %s", node.Hostname, node.ID, route.String())
 		if pm.NodeCanApproveRoute(node, route) {
+			log.Trace().Msgf("AUTO APPROVE: node %s %d, checking %s, %v", node.Hostname, node.ID, route.String(), true)
 			newApproved = append(newApproved, route)
 		}
 	}

hscontrol/policy/v1/policy.go

@@ -7,6 +7,9 @@ import (
 	"os"
 	"sync"
 
+	"slices"
+
+	"github.com/davecgh/go-spew/spew"
 	"github.com/juanfont/headscale/hscontrol/types"
 	"github.com/rs/zerolog/log"
 	"tailscale.com/tailcfg"
@@ -145,13 +148,7 @@ func (pm *PolicyManager) NodeCanHaveTag(node *types.Node, tag string) bool {
 	tags, invalid := pm.pol.TagsOfNode(pm.users, node)
 	log.Debug().Strs("authorised_tags", tags).Strs("unauthorised_tags", invalid).Uint64("node.id", node.ID.Uint64()).Msg("tags provided by policy")
 
-	for _, t := range tags {
-		if t == tag {
-			return true
-		}
-	}
-
-	return false
+	return slices.Contains(tags, tag)
 }
 
 func (pm *PolicyManager) NodeCanApproveRoute(node *types.Node, route netip.Prefix) bool {
@@ -163,18 +160,27 @@ func (pm *PolicyManager) NodeCanApproveRoute(node *types.Node, route netip.Prefi
 	defer pm.mu.Unlock()
 
 	approvers, _ := pm.pol.AutoApprovers.GetRouteApprovers(route)
+	log.Trace().Msgf("AUTO APPROVE: node %d, checking %s, approvers: %v", node.ID, route.String(), approvers)
 
 	for _, approvedAlias := range approvers {
 		if approvedAlias == node.User.Username() {
 			return true
 		} else {
+			log.Trace().Msgf("AUTO APPROVE: node %d, checking %s, expanding: %s", node.ID, route.String(), approvedAlias)
+
 			ips, err := pm.pol.ExpandAlias(pm.nodes, pm.users, approvedAlias)
 			if err != nil {
 				return false
 			}
+			log.Trace().Msgf("AUTO APPROVE: node %d, checking %s, ips: %v", node.ID, route.String(), ips.Prefixes())
+			log.Trace().Msgf("AUTO APPROVE: node %d, checking %s, contains? %v", node.ID, route.String(), node.IPs())
+			// log.Trace().Msgf("AUTO APPROVE: node %d, checking %s, users %v", node.ID, route.String(), pm.users)
+			// log.Trace().Msgf("AUTO APPROVE: node %d, checking %s, nodes %v", node.ID, route.String(), pm.nodes)
+			spew.Dump(pm.users)
+			spew.Dump(pm.nodes)
 
 			// approvedIPs should contain all of node's IPs if it matches the rule, so check for first
-			if ips.Contains(*node.IPv4) {
+			if ips != nil && ips.Contains(*node.IPv4) {
 				return true
 			}
 		}

hscontrol/policy/v2/policy.go

@@ -7,7 +7,10 @@ import (
 	"strings"
 	"sync"
 
+	"slices"
+
 	"github.com/juanfont/headscale/hscontrol/types"
+	"github.com/rs/zerolog/log"
 	"go4.org/netipx"
 	"tailscale.com/net/tsaddr"
 	"tailscale.com/tailcfg"
@@ -83,10 +86,12 @@ func (pm *PolicyManager) updateLocked() (bool, error) {
 	pm.tagOwnerMap = tagMap
 	pm.tagOwnerMapHash = tagOwnerMapHash
 
+	log.Printf("AUTO APP: BUILDING AUTO APProvers")
 	autoMap, err := resolveAutoApprovers(pm.pol, pm.users, pm.nodes)
 	if err != nil {
 		return false, fmt.Errorf("resolving auto approvers map: %w", err)
 	}
+	log.Printf("AUTO APP: BUILDING AUTO APProvers DONE")
 
 	autoApproveMapHash := deephash.Hash(&autoMap)
 	autoApproveChanged := autoApproveMapHash != pm.autoApproveMapHash
@@ -174,12 +179,10 @@ func (pm *PolicyManager) NodeCanHaveTag(node *types.Node, tag string) bool {
 	defer pm.mu.Unlock()
 
 	if ips, ok := pm.tagOwnerMap[Tag(tag)]; ok {
-		for _, nodeAddr := range node.IPs() {
-			if ips.Contains(nodeAddr) {
+		if slices.ContainsFunc(node.IPs(), ips.Contains) {
 			return true
 		}
 	}
-	}
 
 	return false
 }
@@ -192,16 +195,16 @@ func (pm *PolicyManager) NodeCanApproveRoute(node *types.Node, route netip.Prefi
 	pm.mu.Lock()
 	defer pm.mu.Unlock()
 
+	log.Debug().Msg(pm.DebugString())
+
 	// The fast path is that a node requests to approve a prefix
 	// where there is an exact entry, e.g. 10.0.0.0/8, then
 	// check and return quickly
 	if _, ok := pm.autoApproveMap[route]; ok {
-		for _, nodeAddr := range node.IPs() {
-			if pm.autoApproveMap[route].Contains(nodeAddr) {
+		if slices.ContainsFunc(node.IPs(), pm.autoApproveMap[route].Contains) {
 			return true
 		}
 	}
-	}
 
 	// The slow path is that the node tries to approve
 	// 10.0.10.0/24, which is a part of 10.0.0.0/8, then we
@@ -220,13 +223,11 @@ func (pm *PolicyManager) NodeCanApproveRoute(node *types.Node, route netip.Prefi
 		// Check if prefix is larger (so containing) and then overlaps
 		// the route to see if the node can approve a subset of an autoapprover
 		if prefix.Bits() <= route.Bits() && prefix.Overlaps(route) {
-			for _, nodeAddr := range node.IPs() {
-				if approveAddrs.Contains(nodeAddr) {
+			if slices.ContainsFunc(node.IPs(), approveAddrs.Contains) {
 				return true
 			}
 		}
 	}
-	}
 
 	return false
 }
@@ -279,5 +280,8 @@ func (pm *PolicyManager) DebugString() string {
 		}
 	}
 
+	sb.WriteString("\n\n")
+	sb.WriteString(pm.nodes.DebugString())
+
 	return sb.String()
 }

hscontrol/policy/v2/types.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
+	"log"
 	"net/netip"
 	"strings"
 	"time"
@@ -160,6 +161,10 @@ func (g Group) CanBeAutoApprover() bool {
 	return true
 }
 
+func (g Group) String() string {
+	return string(g)
+}
+
 func (g Group) Resolve(p *Policy, users types.Users, nodes types.Nodes) (*netipx.IPSet, error) {
 	var ips netipx.IPSetBuilder
 	var errs []error
@@ -209,15 +214,24 @@ func (t Tag) Resolve(p *Policy, users types.Users, nodes types.Nodes) (*netipx.I
 		return nil, err
 	}
 
+	log.Printf("AUTO APP: TAGMAP: %+v", tagMap)
+	for tag, ips := range tagMap {
+		log.Printf("AUTO APP: TAG %s, %v", tag, ips.Prefixes())
+	}
+
 	for _, node := range nodes {
 		if node.HasTag(string(t)) {
+			log.Printf("AUTO APP: NODE %d %q HAS TAG %s", node.ID, node.Hostname, t)
 			node.AppendToIPSet(&ips)
 		}
 
 		// TODO(kradalby): remove as part of #2417, see comment above
 		if tagMap != nil {
+			log.Printf("AUTO APP: NODE %d %q CHECKING REQUESTED TAGS: %v", node.ID, node.Hostname, node.Hostinfo.RequestTags)
 			if tagips, ok := tagMap[t]; ok && node.InIPSet(tagips) && node.Hostinfo != nil {
+				log.Printf("AUTO APP: NODE %d %q CHECKING tagips %v", node.ID, node.Hostname, tagips.Prefixes())
 				for _, tag := range node.Hostinfo.RequestTags {
+					log.Printf("AUTO APP: NODE %d %q CHECKING requested tag %s", node.ID, node.Hostname, tag)
 					if tag == string(t) {
 						node.AppendToIPSet(&ips)
 					}
@@ -233,12 +247,16 @@ func (t Tag) CanBeAutoApprover() bool {
 	return true
 }
 
+func (t Tag) String() string {
+	return string(t)
+}
+
 // Host is a string that represents a hostname.
 type Host string
 
 func (h Host) Validate() error {
 	if isHost(string(h)) {
-		fmt.Errorf("Hostname %q is invalid", h)
+		return fmt.Errorf("Hostname %q is invalid", h)
 	}
 	return nil
 }
@@ -591,6 +609,7 @@ func unmarshalPointer[T any](
 type AutoApprover interface {
 	CanBeAutoApprover() bool
 	UnmarshalJSON([]byte) error
+	String() string
 }
 
 type AutoApprovers []AutoApprover
@@ -826,6 +845,7 @@ func resolveAutoApprovers(p *Policy, users types.Users, nodes types.Nodes) (map[
 			}
 			// If it does not resolve, that means the autoApprover is not associated with any IP addresses.
 			ips, _ := aa.Resolve(p, users, nodes)
+			log.Printf("AUTO APP RESOLVED: tag: %q pref: %s ips: %v", autoApprover.String(), prefix, ips.Prefixes())
 			routes[prefix].AddSet(ips)
 		}
 	}

hscontrol/poll.go

@@ -462,6 +462,7 @@ func (m *mapSession) handleEndpointUpdate() {
 		// auto approved. Any change here is not important as any
 		// actual state change will be detected when the route manager
 		// is updated.
+		log.Trace().Msgf("AUTO APPROVE ROUTES CHANGE")
 		policy.AutoApproveRoutes(m.h.polMan, m.node)
 
 		// Update the routes of the given node in the route manager to

hscontrol/types/node.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net/netip"
 	"slices"
+	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -190,19 +191,26 @@ func (node *Node) IsTagged() bool {
 // Currently, this function only handles tags set
 // via CLI ("forced tags" and preauthkeys)
 func (node *Node) HasTag(tag string) bool {
-	if slices.Contains(node.ForcedTags, tag) {
-		return true
-	}
+	return slices.Contains(node.Tags(), tag)
+}
 
-	if node.AuthKey != nil && slices.Contains(node.AuthKey.Tags, tag) {
-		return true
+func (node *Node) Tags() []string {
+	var tags []string
+
+	if node.AuthKey != nil {
+		tags = append(tags, node.AuthKey.Tags...)
 	}
 
 	// TODO(kradalby): Figure out how tagging should work
 	// and hostinfo.requestedtags.
 	// Do this in other work.
+	// #2417
 
-	return false
+	tags = append(tags, node.ForcedTags...)
+	sort.Strings(tags)
+	tags = slices.Compact(tags)
+
+	return tags
 }
 
 func (node *Node) RequestTags() []string {
@@ -404,9 +412,9 @@ func (node *Node) SubnetRoutes() []netip.Prefix {
 	return routes
 }
 
-func (node *Node) String() string {
-	return node.Hostname
-}
+// func (node *Node) String() string {
+// 	return node.Hostname
+// }
 
 // PeerChangeFromMapRequest takes a MapRequest and compares it to the node
 // to produce a PeerChange struct that can be used to updated the node and
@@ -526,15 +534,15 @@ func (node *Node) ApplyPeerChange(change *tailcfg.PeerChange) {
 	node.LastSeen = change.LastSeen
 }
 
-func (nodes Nodes) String() string {
-	temp := make([]string, len(nodes))
+// func (nodes Nodes) String() string {
+// 	temp := make([]string, len(nodes))
 
-	for index, node := range nodes {
-		temp[index] = node.Hostname
-	}
+// 	for index, node := range nodes {
+// 		temp[index] = node.Hostname
+// 	}
 
-	return fmt.Sprintf("[ %s ](%d)", strings.Join(temp, ", "), len(temp))
-}
+// 	return fmt.Sprintf("[ %s ](%d)", strings.Join(temp, ", "), len(temp))
+// }
 
 func (nodes Nodes) IDMap() map[NodeID]*Node {
 	ret := map[NodeID]*Node{}
@@ -545,3 +553,25 @@ func (nodes Nodes) IDMap() map[NodeID]*Node {
 
 	return ret
 }
+
+func (nodes Nodes) DebugString() string {
+	var sb strings.Builder
+	sb.WriteString("Nodes:\n")
+	for _, node := range nodes {
+		sb.WriteString(node.DebugString())
+		sb.WriteString("\n")
+	}
+	return sb.String()
+}
+
+func (node Node) DebugString() string {
+	var sb strings.Builder
+	fmt.Fprintf(&sb, "%s(%s):\n", node.Hostname, node.ID)
+	fmt.Fprintf(&sb, "\tUser: %s (%d, %q)\n", node.User.Display(), node.User.ID, node.User.Username())
+	fmt.Fprintf(&sb, "\tTags: %v\n", node.Tags())
+	fmt.Fprintf(&sb, "\tIPs: %v\n", node.IPs())
+	fmt.Fprintf(&sb, "\tApprovedRoutes: %v\n", node.ApprovedRoutes)
+	fmt.Fprintf(&sb, "\tSubnetRoutes: %v\n", node.SubnetRoutes())
+	sb.WriteString("\n")
+	return sb.String()
+}

integration/auth_oidc_test.go

@@ -19,6 +19,20 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+func oidcHSICOpts(s *Scenario) []hsic.Option {
+	oidcMap := map[string]string{
+		"HEADSCALE_OIDC_ISSUER":             s.mockOIDC.Issuer(),
+		"HEADSCALE_OIDC_CLIENT_ID":          s.mockOIDC.ClientID(),
+		"CREDENTIALS_DIRECTORY_TEST":        "/tmp",
+		"HEADSCALE_OIDC_CLIENT_SECRET_PATH": "${CREDENTIALS_DIRECTORY_TEST}/hs_client_oidc_secret",
+	}
+	return []hsic.Option{
+		hsic.WithConfigEnv(oidcMap),
+		hsic.WithFileInContainer("/tmp/hs_client_oidc_secret", []byte(s.mockOIDC.ClientSecret())),
+		hsic.WithTLS(),
+	}
+}
+
 func TestOIDCAuthenticationPingAll(t *testing.T) {
 	IntegrationSkip(t)
 	t.Parallel()
@@ -40,19 +54,9 @@ func TestOIDCAuthenticationPingAll(t *testing.T) {
 
 	defer scenario.ShutdownAssertNoPanics(t)
 
-	oidcMap := map[string]string{
-		"HEADSCALE_OIDC_ISSUER":             scenario.mockOIDC.Issuer(),
-		"HEADSCALE_OIDC_CLIENT_ID":          scenario.mockOIDC.ClientID(),
-		"CREDENTIALS_DIRECTORY_TEST":        "/tmp",
-		"HEADSCALE_OIDC_CLIENT_SECRET_PATH": "${CREDENTIALS_DIRECTORY_TEST}/hs_client_oidc_secret",
-	}
-
 	err = scenario.CreateHeadscaleEnvWithLoginURL(
 		nil,
-		hsic.WithTestName("oidcauthping"),
-		hsic.WithConfigEnv(oidcMap),
-		hsic.WithTLS(),
-		hsic.WithFileInContainer("/tmp/hs_client_oidc_secret", []byte(scenario.mockOIDC.ClientSecret())),
+		append(oidcHSICOpts(scenario), hsic.WithTestName("oidcauthping"))...,
 	)
 	assertNoErrHeadscaleEnv(t, err)
 

integration/route_test.go

@@ -1367,9 +1367,16 @@ func TestSubnetRouterMultiNetworkExitNode(t *testing.T) {
 //   - Verify that routes can now be seen by peers.
 func TestAutoApproveMultiNetwork(t *testing.T) {
 	IntegrationSkip(t)
-	t.Parallel()
 
-	spec := ScenarioSpec{
+	tests := []struct {
+		name     string
+		spec     ScenarioSpec
+		withURL  bool
+		withOIDC bool
+	}{
+		{
+			name: "authkey",
+			spec: ScenarioSpec{
 				NodesPerUser: 3,
 				Users:        []string{"user1", "user2"},
 				Networks: map[string][]string{
@@ -1382,15 +1389,65 @@ func TestAutoApproveMultiNetwork(t *testing.T) {
 				// We build the head image with curl and traceroute, so only use
 				// that for this test.
 				Versions: []string{"head"},
+			},
+		},
+		{
+			name: "webauth",
+			spec: ScenarioSpec{
+				NodesPerUser: 3,
+				Users:        []string{"user1", "user2"},
+				Networks: map[string][]string{
+					"usernet1": {"user1"},
+					"usernet2": {"user2"},
+				},
+				ExtraService: map[string][]extraServiceFunc{
+					"usernet1": {Webservice},
+				},
+				// We build the head image with curl and traceroute, so only use
+				// that for this test.
+				Versions: []string{"head"},
+			},
+			withURL: true,
+		},
+		// TODO(kradalby): multinetwork isnt really working on the oidc
+		// {
+		// 	name: "oidc",
+		// 	spec: ScenarioSpec{
+		// 		NodesPerUser: 3,
+		// 		Users:        []string{"user1", "user2"},
+		// 		OIDCUsers: []mockoidc.MockUser{
+		// 			oidcMockUser("user1", false),
+		// 			oidcMockUser("user1", false),
+		// 			oidcMockUser("user1", false),
+		// 			oidcMockUser("user2", false),
+		// 			oidcMockUser("user2", false),
+		// 			oidcMockUser("user2", false),
+		// 		},
+		// 		Networks: map[string][]string{
+		// 			"usernet1": {"user1"},
+		// 			"usernet2": {"user2"},
+		// 		},
+		// 		ExtraService: map[string][]extraServiceFunc{
+		// 			"usernet1": {Webservice},
+		// 		},
+		// 		// We build the head image with curl and traceroute, so only use
+		// 		// that for this test.
+		// 		Versions: []string{"head"},
+		// 	},
+		// 	withURL:  true,
+		// 	withOIDC: true,
+		// },
 	}
 
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
 			rootRoute := netip.MustParsePrefix("10.42.0.0/16")
 			subRoute := netip.MustParsePrefix("10.42.7.0/24")
 			notApprovedRoute := netip.MustParsePrefix("192.168.0.0/24")
 
-	scenario, err := NewScenario(spec)
+			scenario, err := NewScenario(tt.spec)
 			require.NoErrorf(t, err, "failed to create scenario: %s", err)
-	defer scenario.ShutdownAssertNoPanics(t)
+			// defer scenario.ShutdownAssertNoPanics(t)
 
 			pol := &policyv1.ACLPolicy{
 				ACLs: []policyv1.ACL{
@@ -1411,15 +1468,23 @@ func TestAutoApproveMultiNetwork(t *testing.T) {
 				},
 			}
 
-	err = scenario.CreateHeadscaleEnv([]tsic.Option{
-		tsic.WithAcceptRoutes(),
-		tsic.WithTags([]string{"tag:approve"}),
-	},
+			opts := []hsic.Option{
 				hsic.WithTestName("clienableroute"),
 				hsic.WithEmbeddedDERPServerOnly(),
 				hsic.WithTLS(),
 				hsic.WithACLPolicy(pol),
 				hsic.WithPolicyMode(types.PolicyModeDB),
+			}
+
+			if tt.withOIDC {
+				opts = append(opts, oidcHSICOpts(scenario)...)
+			}
+
+			err = scenario.createHeadscaleEnv(tt.withURL, []tsic.Option{
+				tsic.WithAcceptRoutes(),
+				tsic.WithTags([]string{"tag:approve"}),
+			},
+				opts...,
 			)
 			assertNoErrHeadscaleEnv(t, err)
 
@@ -1721,6 +1786,8 @@ func TestAutoApproveMultiNetwork(t *testing.T) {
 					requirePeerSubnetRoutes(t, peerStatus, nil)
 				}
 			}
+		})
+	}
 }
 
 func assertTracerouteViaIP(t *testing.T, tr util.Traceroute, ip netip.Addr) {