Fix godoclint, exptostd, gocritic, inamedparam, ineffassign,
and embeddedstructfieldcheck lint issues:
- Fix documentation comments to start with symbol name
- Replace golang.org/x/exp/maps and slices with stdlib equivalents
- Simplify redundant closures (unlambda)
- Flatten nested conditionals (elseif)
- Use type switch with assignment (typeSwitchVar)
- Add parameter names to interface method signatures
- Fix ineffectual assignments
- Add empty line after embedded struct fields
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Run golangci-lint --fix and manually correct the resulting compilation
errors where := was incorrectly used instead of = for already-declared
variables.
Also fix lint issues:
- Use static errors instead of dynamic errors.New() (err113)
- Define constants for magic numbers (mnd)
- Use http.NewRequestWithContext instead of http.NewRequest (noctx)
Errors should not start capitalised and they should not contain the word error
or state that they "failed" as we already know it is an error
Signed-off-by: Kristoffer Dalby <kristoffer@dalby.cc>
Update integration test expectations to match current policy behavior:
1. IPProto defaults include all four protocols (TCP, UDP, ICMPv4,
ICMPv6) for port-range ACL rules, not just TCP and UDP.
2. Filter rules with identical SrcIPs and IPProto are now merged
into a single rule with combined DstPorts, so the subnet router
receives one filter rule instead of two.
Updates #3036
Add TestTagsAuthKeyConvertToUserViaCLIRegister that reproduces the
exact panic from #3038: register a node with a tags-only PreAuthKey
(no user), force reauth with empty tags, then register via CLI with
a user. The mapper panics on node.Owner().Model().ID when User is nil.
The critical detail is using a tags-only PreAuthKey (User: nil). When
the key is created under a user, the node inherits the User pointer
from createAndSaveNewNode and the bug is masked.
Also add Owner() validity assertions to the existing unit test
TestTaggedNodeWithoutUserToDifferentUser to catch the nil pointer
at the unit test level.
Updates #3038
Update integration tests to use valid SSH patterns:
- TestSSHOneUserToAll: use autogroup:member and autogroup:tagged
instead of wildcard destination
- TestSSHMultipleUsersAllToAll: use autogroup:self instead of
username destinations for group-to-user SSH access
Updates #3009
Updates #3010
Extend TestApiKeyCommand to test the new --id flag for expire and
delete commands, verifying that API keys can be managed by their
database ID in addition to the existing --prefix method.
Updates #2986
Make DeleteUser call updatePolicyManagerUsers() to refresh the policy
manager's cached user list after user deletion. This ensures consistency
with CreateUser, UpdateUser, and RenameUser which all update the policy
manager.
Previously, DeleteUser only removed the user from the database without
updating the policy manager. This could leave stale user references in
the cached user list, potentially causing issues when policy is
re-evaluated.
The gRPC handler now uses the change returned from DeleteUser instead of
manually constructing change.UserRemoved().
Fixes#2967
Add DeleteUser method to ControlServer interface and implement it in
HeadscaleInContainer to enable testing user deletion scenarios.
Add two integration tests for issue #2967:
- TestACLGroupWithUnknownUser: tests that valid users can communicate
when a group references a non-existent user
- TestACLGroupAfterUserDeletion: tests connectivity after deleting a
user that was referenced in an ACL group
These tests currently pass but don't fully reproduce the reported issue
where deleted users break connectivity for the entire group.
Updates #2967
- Rename TestTagsAuthKeyWithoutUserIgnoresAdvertisedTags to
TestTagsAuthKeyWithoutUserRejectsAdvertisedTags to reflect actual
behavior (PreAuthKey registrations reject advertised tags)
- Fix TestTagsAuthKeyWithoutUserInheritsTags to use ListNodes() without
user filter since tags-only nodes don't have a user association
Updates #2977
HandleNodeFromPreAuthKey assumed pak.User was always set, but
tags-only PreAuthKeys have nil User. This caused nil pointer
dereference when registering nodes with tags-only keys.
Also updates integration tests to use GetTags() instead of the
removed GetValidTags() method.
Updates #2977
Update 8 tests that involve admin tag assignment via SetNodeTags()
to verify both server-side state and node self view updates:
- TestTagsAuthKeyWithTagAdminOverrideReauthPreserves
- TestTagsAuthKeyWithTagCLICannotModifyAdminTags
- TestTagsAuthKeyWithoutTagCLINoOpAfterAdminWithReset
- TestTagsAuthKeyWithoutTagCLINoOpAfterAdminWithEmptyAdvertise
- TestTagsAuthKeyWithoutTagCLICannotReduceAdminMultiTag
- TestTagsUserLoginCLINoOpAfterAdminAssignment
- TestTagsUserLoginCLICannotRemoveAdminTags
- TestTagsAdminAPICanSetUnownedTag
Each test now validates that tag updates propagate to the node's
own self view using assertNodeSelfHasTagsWithCollect, addressing
the issue #2978 scenario where tag changes were observed to
propagate to peers but not to the node itself.
Updates #2978
Add TestTagsIssue2978ReproTagReplacement that specifically tests the
scenario from issue #2978:
- Register node with tag:foo via web auth with --advertise-tags
- Admin changes tag to tag:bar via SetNodeTags
- Verify client's self view updates (not just server-side)
The test performs multiple tag replacements with timing checks to
verify whether tag updates propagate to the node's self view after
the first call (fixed behavior) or only after a redundant second
call (bug behavior).
Add helper functions for test validation:
- assertNodeSelfHasTagsWithCollect: validates client's status.Self.Tags
- assertNetmapSelfHasTagsWithCollect: validates client's netmap.SelfNode.Tags
Updates #2978
Add TestTagsUserLoginReauthWithEmptyTagsRemovesAllTags to validate that
nodes can be untagged via `tailscale up --advertise-tags= --force-reauth`.
The test verifies:
- Node starts with tags and is owned by tagged-devices
- After reauth with empty tags, all tags are removed
- Node ownership returns to the authenticating user
Updates #2979
Add run ID-based isolation to container naming and network setup to
enable multiple integration tests to run concurrently on the same
Docker daemon without conflicts.
Changes:
- hsic: Add run ID prefix to headscale container names and use dynamic
port allocation for metrics endpoint (port 0 lets kernel assign)
- tsic: Add run ID prefix to tailscale container names
- dsic: Add run ID prefix to DERP container names
- scenario: Use run ID-aware test suite container name for network setup
Container naming now follows: {type}-{runIDShort}-{identifier}-{hash}
Example: ts-mdjtzx-1-74-fgdyls, hs-mdjtzx-pingallbyip-abc123
The run ID is obtained from HEADSCALE_INTEGRATION_RUN_ID environment
variable via dockertestutil.GetIntegrationRunID().
This commit adds tests to validate that there are
issues with how we propagate tag changes in the system.
This replicates #2389
Signed-off-by: Kristoffer Dalby <kristoffer@dalby.cc>
This PR restructures the integration tests and prebuilds all common assets used in all tests:
Headscale and Tailscale HEAD image
hi binary that is used to run tests
go cache is warmed up for compilation of the test
This essentially means we spend 6-10 minutes building assets before any tests starts, when that is done, all tests can just sprint through.
It looks like we are saving 3-9 minutes per test, and since we are limited to running max 20 concurrent tests across the repo, that means we had a lot of double work.
There is currently 113 checks, so we have to do five runs of 20, and the saving should be quite noticeable! I think the "worst case" saving would be 20+min and "best case" probably towards an hour.
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.
This PR changes tags to be something that exists on nodes in addition to users, to being its own thing. It is part of moving our tags support towards the correct tailscale compatible implementation.
There are probably rough edges in this PR, but the intention is to get it in, and then start fixing bugs from 0.28.0 milestone (long standing tags issue) to discover what works and what doesnt.
Updates #2417Closes#2619
Skip auth key validation for existing nodes re-registering with the same
NodeKey. Pre-auth keys are only required for initial authentication.
NodeKey rotation still requires a valid auth key as it is a security-sensitive
operation that changes the node's cryptographic identity.
Fixes#2830
On Windows, if the user clicks the Tailscale icon in the system tray,
it opens a login URL in the browser.
When the login URL is opened, `state/nonce` cookies are set for that particular URL.
If the user clicks the icon again, a new login URL is opened in the browser,
and new cookies are set.
If the user proceeds with auth in the first tab,
the redirect results in a "state did not match" error.
This patch ensures that each opened login URL sets an individual cookie
that remains valid on the `/oidc/callback` page.
`TestOIDCMultipleOpenedLoginUrls` illustrates and tests this behavior.
