From 62279d469da3a241826715d5b9df7b2e1a0cbfe2 Mon Sep 17 00:00:00 2001 From: maximilize <3752128+maximilize@users.noreply.github.com> Date: Mon, 13 Jul 2026 21:30:54 +0000 Subject: [PATCH] fix(policy): validate reauth tags against the authenticating user A tagged node re-authenticating with --advertise-tags was always rejected, even when the authenticating user owned every requested tag. applyAuthNodeUpdate validated the tags against the existing tag-owned node, which has no user of its own and whose IP is not in any tag owner's set, so NodeCanHaveTag could never approve them; the authenticating user who actually owns the tags was never consulted. Fresh registration works because it assigns that user to the node before validating. Add PolicyManager.UserCanHaveTag (the user half of NodeCanHaveTag) and validate reauth tags against the authenticating user as well as the node, so an owned tag is permitted while unowned tags are still rejected. Fixes #3374 --- CHANGELOG.md | 1 + hscontrol/policy/pm.go | 4 ++ hscontrol/policy/v2/policy.go | 31 +++++++++ hscontrol/state/issue_3374_test.go | 102 +++++++++++++++++++++++++++++ hscontrol/state/state.go | 38 ++++++++++- 5 files changed, 175 insertions(+), 1 deletion(-) create mode 100644 hscontrol/state/issue_3374_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 785d7d855..1e0e9cc32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ keys remain all-access. - Expiring or deleting a non-existent pre-auth key now returns an error instead of silently succeeding [#3324](https://github.com/juanfont/headscale/pull/3324) - Improve systemd service file hardening [#3341](https://github.com/juanfont/headscale/pull/3341) +- Re-authenticating a tagged node with `--advertise-tags` now authorises the requested tags against the authenticating user, instead of rejecting every tag [#3374](https://github.com/juanfont/headscale/issues/3374) ## 0.29.2 (2026-07-01) diff --git a/hscontrol/policy/pm.go b/hscontrol/policy/pm.go index ffde6361a..680c11f56 100644 --- a/hscontrol/policy/pm.go +++ b/hscontrol/policy/pm.go @@ -30,6 +30,10 @@ type PolicyManager interface { // NodeCanHaveTag reports whether the given node can have the given tag. NodeCanHaveTag(node types.NodeView, tag string) bool + // UserCanHaveTag reports whether the given user owns the given tag and may + // therefore apply it, e.g. when re-authenticating a tag-owned node. + UserCanHaveTag(user types.UserView, tag string) bool + // TagExists reports whether the given tag is defined in the policy. TagExists(tag string) bool diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index ae76ad267..6bc605add 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -972,6 +972,37 @@ func (pm *PolicyManager) NodeCanHaveTag(node types.NodeView, tag string) bool { return false } +// UserCanHaveTag reports whether user is authorised to apply tag, i.e. the user +// is one of the tag's owners in the policy (directly or through a group). This +// is the user half of [PolicyManager.NodeCanHaveTag]: it is used to authorise +// tags supplied by an authenticating user (e.g. re-auth with --advertise-tags) +// when the target node is tag-owned and thus carries no user of its own. +func (pm *PolicyManager) UserCanHaveTag(user types.UserView, tag string) bool { + if pm == nil || !user.Valid() { + return false + } + + pm.mu.RLock() + defer pm.mu.RUnlock() + + if pm.pol == nil { + return false + } + + owners, exists := pm.pol.TagOwners[Tag(tag)] + if !exists { + return false + } + + for _, owner := range owners { + if pm.userMatchesOwner(user, owner) { + return true + } + } + + return false +} + // TagOwnedByTags reports whether a credential holding ownerTags is authorised to // apply tag. It is true when tag is one of ownerTags, or when tag's tagOwners // chain (tag-to-tag ownership) transitively includes one of ownerTags. This is diff --git a/hscontrol/state/issue_3374_test.go b/hscontrol/state/issue_3374_test.go new file mode 100644 index 000000000..fe66e417c --- /dev/null +++ b/hscontrol/state/issue_3374_test.go @@ -0,0 +1,102 @@ +package state + +import ( + "fmt" + "testing" + + "github.com/juanfont/headscale/hscontrol/db" + "github.com/juanfont/headscale/hscontrol/types" + "github.com/juanfont/headscale/hscontrol/util" + "github.com/stretchr/testify/require" + "tailscale.com/tailcfg" +) + +// TestTaggedReauthValidatesAgainstAuthUser reproduces issue #3374: an already +// tagged node (tag-owned, so neither UserID nor User is set and its IP is not +// in any tag owner's node set) cannot re-advertise a tag through the auth path +// — even a tag it already holds — when the authenticating user owns that tag. +// +// applyAuthNodeUpdate validates RequestTags against the existing tagged node +// instead of the authenticating user, so both NodeCanHaveTag paths (node IP in +// the tag-owner set, node's own user owns the tag) dead-end. The +// fresh-registration path validates against the authenticating user and works, +// so tagged->tagged re-auth is the only rejected case. The fix must still +// reject tags the authenticating user does not own. +// +// https://github.com/juanfont/headscale/issues/3374 +func TestTaggedReauthValidatesAgainstAuthUser(t *testing.T) { + dbPath := t.TempDir() + "/headscale.db" + cfg := persistTestConfig(dbPath) + + database, err := db.NewHeadscaleDatabase(cfg) + require.NoError(t, err) + + // tagger owns tag:foo and performs the re-auth. The node is created under a + // different user and then tag-owned, so its IP is not in tagger's node set — + // the realistic tag-owned state, unlike a node that is still user-owned. + tagger := database.CreateUserForTest("tagger") + other := database.CreateUserForTest("other") + node := database.CreateRegisteredNodeForTest(other, "tagged-node") + machineKey := node.MachineKey + nodeID := node.ID + + require.NoError(t, database.Close()) + + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + // Make it genuinely tag-owned: tagged, neither UserID nor User set, as a node + // registered with a tagged pre-auth key ends up. UserID nil routes the re-auth + // through the convert-from-tag lookup. + seeded, ok := s.nodeStore.UpdateNode(nodeID, func(n *types.Node) { + n.Tags = []string{"tag:foo"} + n.UserID = nil + n.User = nil + n.Expiry = nil + }) + require.True(t, ok) + require.True(t, seeded.IsTagged(), "precondition: node must be tagged") + + // tagger owns tag:foo but not tag:bar. + policy := fmt.Sprintf(`{"tagOwners":{"tag:foo":["%s@"],"tag:bar":["%s@"]}}`, tagger.Name, other.Name) + _, err = s.SetPolicy([]byte(policy)) + require.NoError(t, err) + + reauth := func(t *testing.T, tags []string) (types.NodeView, error) { + t.Helper() + + regData := &types.RegistrationData{ + MachineKey: machineKey, + NodeKey: node.NodeKey, + DiscoKey: node.DiscoKey, + Hostname: "tagged-node", + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "tagged-node", + RequestTags: tags, + }, + } + + authID := types.MustAuthID() + s.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(regData)) + + n, _, err := s.HandleNodeFromAuthPath(authID, types.UserID(tagger.ID), nil, util.RegisterMethodOIDC) + + return n, err + } + + // tagger owns tag:foo, so re-advertising it must be permitted. + finalNode, err := reauth(t, []string{"tag:foo"}) + require.NoError(t, err, + "tag owner must be allowed to re-auth a tagged node re-advertising a tag they own") + require.True(t, finalNode.Valid()) + require.True(t, finalNode.IsTagged(), + "node should remain tagged after re-advertising a permitted tag") + require.Contains(t, finalNode.Tags().AsSlice(), "tag:foo") + + // tag:bar is owned by other, not tagger, so it must still be rejected — the + // fix authorises the authenticating user, it does not skip authorisation. + _, err = reauth(t, []string{"tag:bar"}) + require.Error(t, err, + "tags the authenticating user does not own must still be rejected") +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index ddb410eb7..b146850a7 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -1707,7 +1707,15 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView // Validate tags BEFORE calling [NodeStore.UpdateNode] to ensure we don't modify // [NodeStore] if validation fails. This maintains consistency between [NodeStore] // and database. - rejectedTags := s.validateRequestTags(params.ExistingNode, requestTags) + // + // #3374: a tag-owned node carries no user of its own, so authorise the + // requested tags against the authenticating user as well (mirroring the + // fresh-registration path, which validates a node already assigned that user). + var authUser types.UserView + if params.User != nil { + authUser = params.User.View() + } + rejectedTags := s.validateRequestTagsForReauth(params.ExistingNode, authUser, requestTags) if len(rejectedTags) > 0 { return types.NodeView{}, fmt.Errorf( "%w %v are invalid or not permitted", @@ -2028,6 +2036,34 @@ func (s *State) validateRequestTags(node types.NodeView, requestTags []string) [ return rejectedTags } +// validateRequestTagsForReauth is like validateRequestTags but also authorises +// tags owned by the authenticating user, not just by the node being +// re-authenticated. A tag-owned node has no user of its own, so on re-auth the +// node alone can never authorise a tag; the user completing the interactive +// login is the one who owns it (#3374). A tag is permitted when the existing +// node may hold it OR the authenticating user owns it; unowned tags are still +// rejected. +func (s *State) validateRequestTagsForReauth(node types.NodeView, authUser types.UserView, requestTags []string) []string { + // Empty tags = clear tags, always permitted + if len(requestTags) == 0 { + return nil + } + + var rejectedTags []string + + for _, tag := range requestTags { + if s.polMan.NodeCanHaveTag(node, tag) { + continue + } + if authUser.Valid() && s.polMan.UserCanHaveTag(authUser, tag) { + continue + } + rejectedTags = append(rejectedTags, tag) + } + + return rejectedTags +} + // processReauthTags handles tag changes during node re-authentication. // It processes RequestTags from the client and updates node tags accordingly. // Returns rejected tags (if any) for post-validation error handling.