-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
tags: process tags on registration, simplify policy #2931
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a tags-as-identity model where tags are set immutably during node registration and stored directly in the Node.Tags field, rather than being dynamically resolved from Hostinfo.RequestTags during policy evaluation.
Key Changes:
- Simplified tag and autogroup resolution by using
node.HasTag()andnode.IsTagged()instead of dynamically evaluating RequestTags - Implemented advertise-tags processing during registration with policy validation via
NodeCanHaveTag - PreAuthKey-tagged devices receive tags exclusively from the PreAuthKey (advertise-tags are ignored)
- User-owned nodes can use advertise-tags which are validated against the tagOwners policy
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
hscontrol/state/state.go |
Adds advertise-tags processing during node registration with policy validation for user-owned nodes |
hscontrol/policy/v2/types.go |
Simplifies Tag.Resolve() and AutoGroup.Resolve() to use node's Tags field instead of RequestTags |
hscontrol/policy/v2/policy.go |
Moves resolveTagOwners() function and adds ErrInvalidTagOwner error constant |
hscontrol/policy/v2/types_test.go |
Updates test cases to reflect tags-as-identity model (removes RequestTags test scenarios, adds formatting fixes) |
hscontrol/policy/v2/policy_test.go |
Fixes test node structure to use pointers for User and UserID fields |
hscontrol/auth_test.go |
Updates test to use user.TypedID() convenience method instead of manual casting |
integration/scenario.go |
Adds CreatePreAuthKeyWithTags() helper method for integration tests |
integration/hsic/hsic.go |
Implements CreateAuthKeyWithTags() to create tagged PreAuthKeys in test containers |
integration/control.go |
Adds CreateAuthKeyWithTags() to ControlServer interface |
integration/auth_web_flow_test.go |
Adds comprehensive TestWebAuthAdvertiseTags() testing advertise-tags for web-authenticated nodes |
integration/auth_key_test.go |
Adds TestAuthKeyAdvertiseTagsBlocked() verifying advertise-tags are ignored for PreAuthKey-tagged devices |
.github/workflows/test-integration.yaml |
Registers new integration tests in CI workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8e5385f to
2ee5e2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
integration/tags_test.go
Outdated
| require.NoError(t, err) | ||
|
|
||
| // Register the node via headscale CLI - this should fail or tags should be rejected | ||
| _ = scenario.runHeadscaleRegister(tagTestUser, body) |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test (TestTagsUserLoginNonExistentTagAtRegistration) silently ignores the result of runHeadscaleRegister on line 1360 and only checks the outcome afterward. If registration fails for an unexpected reason (network issue, etc.), the test would still pass because it only checks whether nodes exist or don't have the invalid tag. Consider using require.NoError(t, err) if registration should succeed but tags should be filtered, or explicitly check for an expected error if registration should fail completely.
| @@ -0,0 +1,2467 @@ | |||
| package integration | |||
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The test file is very large (2467 lines). While comprehensive testing is good, this makes it difficult to navigate and maintain. Consider splitting this into multiple test files organized by test suite (e.g., tags_preauth_test.go, tags_userlogin_test.go, tags_admin_api_test.go) to improve maintainability and readability.
hscontrol/state/state.go
Outdated
| // Reject advertise-tags for PreAuthKey registrations. | ||
| // PreAuthKey nodes get their tags from the key itself, not from client requests. | ||
| if params.PreAuthKey != nil && params.Hostinfo != nil && len(params.Hostinfo.RequestTags) > 0 { | ||
| return types.NodeView{}, fmt.Errorf("%w: %v", ErrInvalidOrUnauthorizedTags, params.Hostinfo.RequestTags) | ||
| } | ||
|
|
||
| // Process RequestTags (from tailscale up --advertise-tags) ONLY for non-PreAuthKey registrations. | ||
| if params.PreAuthKey == nil && params.Hostinfo != nil && len(params.Hostinfo.RequestTags) > 0 { | ||
| var approvedTags, rejectedTags []string | ||
|
|
||
| for _, tag := range params.Hostinfo.RequestTags { | ||
| if s.polMan.NodeCanHaveTag(nodeToRegister.View(), tag) { | ||
| approvedTags = append(approvedTags, tag) | ||
| } else { | ||
| rejectedTags = append(rejectedTags, tag) | ||
| } | ||
| } | ||
|
|
||
| // Reject registration if any requested tags are unauthorized | ||
| if len(rejectedTags) > 0 { | ||
| return types.NodeView{}, fmt.Errorf("%w: %v", ErrInvalidOrUnauthorizedTags, rejectedTags) | ||
| } | ||
|
|
||
| if len(approvedTags) > 0 { | ||
| nodeToRegister.Tags = approvedTags | ||
| slices.Sort(nodeToRegister.Tags) | ||
| nodeToRegister.Tags = slices.Compact(nodeToRegister.Tags) | ||
| log.Info(). | ||
| Str("node.name", nodeToRegister.Hostname). | ||
| Strs("tags", nodeToRegister.Tags). | ||
| Msg("approved advertise-tags during registration") | ||
| } | ||
| } |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The placement of tag processing logic after IP allocation (lines 1154-1160) but before database save creates a potential resource leak. If tag validation fails after IPs are allocated, those IPs are not returned to the pool. Consider moving tag validation to occur before IP allocation, or ensure IPs are released if tag validation fails.
hscontrol/policy/v2/types.go
Outdated
| continue | ||
| } | ||
|
|
||
| // Node is a member if it has no forced tags and no allowed requested tags |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "Node is a member if it has no forced tags and no allowed requested tags" is now outdated after removing the RequestTags checking logic. The comment should be updated to simply state "Node is a member if it is not tagged" to reflect the current implementation.
| // Node is a member if it has no forced tags and no allowed requested tags | |
| // Node is a member if it is not tagged |
integration/tags_test.go
Outdated
| // 3. Run `tailscale up --auth-key AUTH_KEY_WITH_TAG --force-reauth` | ||
| // | ||
| // Expected: After step 2 tags are ["tag:second"], after step 3 tags remain ["tag:second"]. | ||
| func TestTagsAuthKeyWithTagAdminOverrideReauthResets(t *testing.T) { |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test name doesn't match the comment. The test is named TestTagsAuthKeyWithTagAdminOverrideReauthResets but the comment in line 430 says "Admin assignment is preserved through reauth". However, the test name suggests it tests that admin tags are "reset" (removed), while the test logic and comment indicate they should be "preserved". This naming inconsistency could confuse developers. Consider renaming to TestTagsAuthKeyWithTagAdminOverrideReauthPreserves to match the actual behavior being tested.
| func TestTagsAuthKeyWithTagAdminOverrideReauthResets(t *testing.T) { | |
| func TestTagsAuthKeyWithTagAdminOverrideReauthPreserves(t *testing.T) { |
hscontrol/policy/v2/policy.go
Outdated
| // Tagged nodes can only keep tags they already have | ||
| if node.IsTagged() { | ||
| return node.HasTag(tag) | ||
| } |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic prevents tagged nodes from ever getting new tags. The check if node.IsTagged() { return node.HasTag(tag) } means a tagged node can only "have" tags it already has, which blocks admin-assigned tag changes. However, based on the PR description and tests like TestTagsAdminAPICanSetUnownedTag, admins should be able to modify tags on tagged nodes. This restriction would break the admin API's SetNodeTags functionality for tagged nodes. Consider removing this check or only applying it in specific contexts (e.g., only for client-initiated tag requests, not admin operations).
| // Tagged nodes can only keep tags they already have | |
| if node.IsTagged() { | |
| return node.HasTag(tag) | |
| } | |
| // (Removed restriction: allow tagged nodes to acquire new tags, e.g., via admin API) |
| var approvedTags, rejectedTags []string | ||
|
|
||
| for _, tag := range params.Hostinfo.RequestTags { | ||
| if s.polMan.NodeCanHaveTag(nodeToRegister.View(), tag) { |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a timing issue with tag validation during registration. At line 1173, nodeToRegister.View() is called before the node has an ID or valid IPs (IPs are assigned at lines 1154-1160, but this node isn't in the database yet). The NodeCanHaveTag method at line 566 checks if the node's IP is in the tagOwnerMap IPSet. For a node being registered, this check will fail because the node's IP isn't yet associated with any user in the tagOwnerMap (the map is built from existing nodes). This means new users registering their first node can never use --advertise-tags, even with valid tagOwners policy. Consider using the user information directly instead of IP-based checking for nodes during initial registration.
a329ad2 to
e12536a
Compare
CHANGELOG.md
Outdated
|
|
||
| Tags are now implemented following the Tailscale model where tags and user ownership are mutually exclusive. Devices can be either user-owned (authenticated via web/OIDC) or tagged (authenticated via tagged PreAuthKeys). Tagged devices receive their identity from tags rather than users, making them suitable for servers and infrastructure. Applying a tag to a device removes user-based authentication. See the [Tailscale tags documentation](https://tailscale.com/kb/1068/tags) for details on how tags work. | ||
|
|
||
| User-owned nodes can now request tags during registration using `--advertise-tags`. Tags are validated against the `tagOwners` policy and applied at registration time. Tags are immutable after registration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect, they can be changed after registration with the CLI or API, but they cannot be changed by the node without re-authentication.
If they are changed with CLI/API, they will replace the current set of tags.
CHANGELOG.md
Outdated
| - **Tags**: Tags are now resolved from the node's stored Tags field only [#2931](https://github.com/juanfont/headscale/pull/2931) | ||
| - `--advertise-tags` is processed during registration, not on every policy evaluation | ||
| - PreAuthKey tagged devices ignore `--advertise-tags` from clients | ||
| - User-owned nodes can use `--advertise-tags` if authorized by `tagOwners` policy | ||
| - Tags are immutable after registration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The start of this is an internal detail, not user facing. Users dont care how it is stored or red internally. The important part is how the registration works and how to change it. angle the positive sides, not how to not change it or how its ignored.
Immutable feels like the wrong word, see previous comment.
hscontrol/policy/v2/policy.go
Outdated
| // Tagged nodes can only keep tags they already have via client requests. | ||
| // (Admin API bypasses this function entirely and can modify any tags.) | ||
| if node.IsTagged() { | ||
| return node.HasTag(tag) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think it should be up to this function to enforce this rule. It feels a bit like the wrong place as the caller of this func is the rule enforcer.
This commit simplifies Tag.Resolve and AutoGroup.Resolve to only check node.HasTag() and node.IsTagged() respectively, removing dynamic RequestTags resolution. Key changes: - Tag.Resolve: Only checks node.HasTag(tag) instead of also checking Hostinfo.RequestTags - AutoGroupMember: Returns nodes where !node.IsTagged() (user-owned) - AutoGroupTagged: Returns nodes where node.IsTagged() (tagged devices) - resolveTagOwners: Moved to policy.go for use by NodeCanHaveTag This aligns with the tags-as-identity model where tags are resolved from the node's Tags field, which is set during registration and immutable thereafter.
Update policy tests to use node.Tags field instead of Hostinfo.RequestTags, aligning with the tags-as-identity model where tags are set during registration and resolved from the Tags field. Key test updates: - autogroup-member-comprehensive: Tests autogroup:member resolution using node.IsTagged() to identify user-owned nodes - autogroup-tagged: Tests autogroup:tagged resolution using node.IsTagged() to identify tagged devices
Add logic to createAndSaveNewNode to process RequestTags (advertise-tags) during node registration with policy-based validation. Key behavior: - PreAuthKey tagged devices: Tags come from PreAuthKey, RequestTags ignored - User-owned nodes: RequestTags are validated against policy using NodeCanHaveTag and applied if authorized - Tags are sorted and deduplicated before storage This implements the tags-as-identity model where tags are set once during registration and become immutable, rather than being dynamically resolved from RequestTags on each policy evaluation.
Add CreateAuthKeyWithTags method to ControlServer interface and implementations to support creating pre-authentication keys with specific tags for integration testing. This helper enables testing of the tags-as-identity model by allowing tests to create tagged PreAuthKeys and verify that nodes registered with them receive the correct tags.
Add integration tests to verify the tags-as-identity model behavior: - TestAuthKeyAdvertiseTagsBlocked: Verifies that nodes registered with a tagged PreAuthKey ignore advertise-tags from the client, ensuring PreAuthKey tags take precedence. - TestWebAuthAdvertiseTags: Verifies that nodes registered via web authentication can use advertise-tags during registration when authorized by policy (tagOwners). These tests validate the core tags-as-identity principle that tags are set during registration and immutable thereafter.
The goal of this commit is to create the tests to test every scenario of how tailscale supports setting and changing tags. Signed-off-by: Kristoffer Dalby <[email protected]>
Signed-off-by: Kristoffer Dalby <[email protected]>
Signed-off-by: Kristoffer Dalby <[email protected]>
Signed-off-by: Kristoffer Dalby <[email protected]>
Signed-off-by: Kristoffer Dalby <[email protected]>
Update error constant and messages to match Tailscale SaaS format: "requested tags [tag:xxx] are invalid or not permitted" Also move tag validation before IP allocation in createAndSaveNewNode to avoid resource leaks on validation failure.
Add userMatchesOwner helper function that allows NodeCanHaveTag to authorize tags for new nodes that don't yet have IPs in the tagOwnerMap. This handles the case where a user registers a new node with --advertise-tags before the node has been assigned an IP address. Also add clarifying comment that NodeCanHaveTag is not used by admin API (SetNodeTags bypasses this check entirely).
Update comment to accurately describe node membership criteria.
Add 7 new test cases for NodeCanHaveTag covering nodes without IPs (new registration scenario) to validate user-based fallback behavior. Add TestUserMatchesOwner with table-driven tests covering username match, group membership, undefined groups, and nil owner handling.
Update error assertions in TestWebAuthRejectsUnauthorizedRequestTags and TestSetTags_Conversion to expect "requested tags" format matching Tailscale SaaS.
Rename TestTagsAuthKeyWithTagAdminOverrideReauthResets to TestTagsAuthKeyWithTagAdminOverrideReauthPreserves to accurately reflect expected behavior. Improve error handling in TestTagsUserLoginNonExistentTagAtRegistration to properly check registration rejection before listing nodes.
Remove WithTags option that is no longer needed after tag behavior changes.
Rename TestTagsAuthKeyWithTagAdminOverrideReauthResets to TestTagsAuthKeyWithTagAdminOverrideReauthPreserves in CI workflow.
e12536a to
cc27e8e
Compare
Signed-off-by: Kristoffer Dalby <[email protected]>
cc27e8e to
a12f295
Compare
Signed-off-by: Kristoffer Dalby <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Reject advertise-tags for PreAuthKey registrations early, before any resource allocation. | ||
| // PreAuthKey nodes get their tags from the key itself, not from client requests. | ||
| if params.PreAuthKey != nil && params.Hostinfo != nil && len(params.Hostinfo.RequestTags) > 0 { | ||
| return types.NodeView{}, fmt.Errorf("%w %v are invalid or not permitted", ErrRequestedTagsInvalidOrNotPermitted, params.Hostinfo.RequestTags) | ||
| } |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation logic for rejecting advertise-tags for PreAuthKey registrations happens here, but there's also tag processing logic from PreAuthKeys elsewhere in the function (around the nodeToRegister.Tags assignment). These two related pieces of logic are separated. Consider consolidating all PreAuthKey tag handling in one place for better maintainability and to reduce the chance of logic inconsistencies.
| msg: >- | ||
| time.Sleep is forbidden. | ||
| In tests: use assert.EventuallyWithT for polling/waiting patterns. | ||
| In production code: use a backoff strategy (e.g., cenkalti/backoff) or proper synchronization primitives. |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The forbidigo rule to ban time.Sleep everywhere is overly restrictive. While the intent to use assert.EventuallyWithT in tests is good, there are legitimate uses of time.Sleep in production code (e.g., rate limiting, deliberate delays, exponential backoff implementations). Consider:
- Exempting specific patterns where
time.Sleepis intentional - Only applying this rule to test files (
_test.go) - Or using a more nuanced message that acknowledges legitimate uses
The current rule may force developers to work around it in ways that make code less clear.
| msg: >- | |
| time.Sleep is forbidden. | |
| In tests: use assert.EventuallyWithT for polling/waiting patterns. | |
| In production code: use a backoff strategy (e.g., cenkalti/backoff) or proper synchronization primitives. | |
| files: "*_test.go" | |
| msg: >- | |
| time.Sleep is forbidden in tests. | |
| Use assert.EventuallyWithT for polling/waiting patterns. |
| ErrInvalidOrUnauthorizedTags = errors.New("invalid or unauthorized tags") | ||
| // ErrRequestedTagsInvalidOrNotPermitted is returned when requested tags are invalid or not permitted. | ||
| // This message format matches Tailscale SaaS: "requested tags [tag:xxx] are invalid or not permitted". | ||
| ErrRequestedTagsInvalidOrNotPermitted = errors.New("requested tags") |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error variable name ErrRequestedTagsInvalidOrNotPermitted has a mismatch with its actual error message. The variable is initialized with errors.New("requested tags") which is just a fragment, but throughout the codebase it's used with fmt.Errorf("%w %v are invalid or not permitted", ErrRequestedTagsInvalidOrNotPermitted, tags).
This creates error messages like "requested tags [tag:foo] are invalid or not permitted" where "requested tags" appears twice. Consider either:
- Changing the error to
errors.New("requested tags are invalid or not permitted")and using it directly - Or using a more descriptive base error like
errors.New("unauthorized tags")
| ErrRequestedTagsInvalidOrNotPermitted = errors.New("requested tags") | |
| ErrRequestedTagsInvalidOrNotPermitted = errors.New("requested tags are invalid or not permitted") |
| //nolint:errcheck // Intentionally ignoring error - we check results below | ||
| client.Execute(command) |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The //nolint:errcheck directive with comment "Intentionally ignoring error - we check results below" is used here. While the intent is documented, it would be clearer and safer to explicitly capture and log the error even if not failing the test:
_, err := client.Execute(command)
if err != nil {
t.Logf("Reauth command result: %v", err)
}This makes the intentional decision more visible and provides better debugging information.
| //nolint:errcheck // Intentionally ignoring error - we check results below | |
| client.Execute(command) | |
| // Intentionally ignoring error - we check results below, but log for visibility | |
| if _, err := client.Execute(command); err != nil { | |
| t.Logf("Reauth command result: %v", err) | |
| } |
Update TestTaildrop to validate that juanfont#2462 is fixed through juanfont#2885 and juanfont#2931. Fixes juanfont#2462 Signed-off-by: Kristoffer Dalby <[email protected]>
Update TestTaildrop to validate that juanfont#2462 is fixed through juanfont#2885 and juanfont#2931. Fixes juanfont#2462 Signed-off-by: Kristoffer Dalby <[email protected]>
Update TestTaildrop to validate that juanfont#2462 is fixed through juanfont#2885 and juanfont#2931. Fixes juanfont#2462 Signed-off-by: Kristoffer Dalby <[email protected]>
This PR investigates, adds tests and aims to correctly implement Tailscale's model for how Tags should be accepted, assigned and used to identify nodes in the Tailscale access and ownership model.
When evaluating in Headscale's policy, Tags are now only checked against a nodes "tags" list, which defines the source of truth for all tags for a given node. This simplifies the code for dealing with tags greatly, and should help us have less access bugs related to nodes belonging to tags or users.
A node can either be owned by a user, or a tag.
Next, to ensure the tags list on the node is correctly implemented, we first add tests for every registration scenario and combination of user, pre auth key and pre auth key with tags with the same registration expectation as observed by trying them all with the Tailscale control server. This should ensure that we implement the correct behaviour and that it does not change or break over time.
Lastly, the missing parts of the auth has been added, or changed in the cases where it was wrong. This has in large parts allowed us to delete and simplify a lot of code.
Now, tags can only be changed when a node authenticates or if set via the CLI/API. Tags can only be fully overwritten/replaced and any use of either auth or CLI will replace the current set if different.
A user owned device can be converted to a tagged device, but it cannot be changed back. A tagged device can never remove the last tag either, it has to have a minimum of one.
Checking all imaginable Tailscale SaaS auth combinations:
