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.