hscontrol: satisfy golangci-lint on changed lines

Sentinel ErrNodeKeyInUse (err113); key the visible-peer set by tailcfg.NodeID
to drop an int64->uint64 cast (gosec G115); NewRequestWithContext (noctx); wsl.
This commit is contained in:
Kristoffer Dalby 2026-06-09 10:23:23 +00:00 committed by Kristoffer Dalby
parent 5e05652a78
commit 88044f43ff
5 changed files with 44 additions and 17 deletions

View file

@ -458,7 +458,7 @@ func (m *mapper) buildFromChange(
// //
// ok is false when the node or its matchers cannot be resolved; callers must // ok is false when the node or its matchers cannot be resolved; callers must
// then fail closed (emit nothing) rather than risk leaking forbidden peers. // then fail closed (emit nothing) rather than risk leaking forbidden peers.
func (m *mapper) visiblePeerIDs(nodeID types.NodeID) (map[types.NodeID]struct{}, bool) { func (m *mapper) visiblePeerIDs(nodeID types.NodeID) (map[tailcfg.NodeID]struct{}, bool) {
node, ok := m.state.GetNodeByID(nodeID) node, ok := m.state.GetNodeByID(nodeID)
if !ok { if !ok {
return nil, false return nil, false
@ -477,9 +477,11 @@ func (m *mapper) visiblePeerIDs(nodeID types.NodeID) (map[types.NodeID]struct{},
peers = policy.ReduceNodes(node, peers, matchers) peers = policy.ReduceNodes(node, peers, matchers)
} }
visible := make(map[types.NodeID]struct{}, peers.Len()) // Key by tailcfg.NodeID so the peer-patch path can look up by patch.NodeID
// directly, avoiding an unchecked int64->uint64 conversion.
visible := make(map[tailcfg.NodeID]struct{}, peers.Len())
for _, peer := range peers.All() { for _, peer := range peers.All() {
visible[peer.ID()] = struct{}{} visible[peer.ID().NodeID()] = struct{}{}
} }
return visible, true return visible, true
@ -504,8 +506,9 @@ func (m *mapper) filterVisiblePeerPatches(
} }
var filtered []*tailcfg.PeerChange var filtered []*tailcfg.PeerChange
for _, patch := range patches { for _, patch := range patches {
if _, vis := visible[types.NodeID(patch.NodeID)]; vis { if _, vis := visible[patch.NodeID]; vis {
filtered = append(filtered, patch) filtered = append(filtered, patch)
} }
} }
@ -528,8 +531,9 @@ func (m *mapper) filterVisibleNodes(
} }
var filtered []types.NodeView var filtered []types.NodeView
for _, peer := range peers.All() { for _, peer := range peers.All() {
if _, vis := visible[peer.ID()]; vis { if _, vis := visible[peer.ID().NodeID()]; vis {
filtered = append(filtered, peer) filtered = append(filtered, peer)
} }
} }

View file

@ -273,11 +273,13 @@ func TestBuildFromChangeFiltersPeerPatchesByVisibility(t *testing.T) {
require.NotNil(t, resp2) require.NotNil(t, resp2)
var gotVisible bool var gotVisible bool
for _, p := range resp2.PeersChangedPatch { for _, p := range resp2.PeersChangedPatch {
if p.NodeID == n1b.ID.NodeID() { if p.NodeID == n1b.ID.NodeID() {
gotVisible = true gotVisible = true
} }
} }
assert.True(t, gotVisible, assert.True(t, gotVisible,
"n1 must receive the online patch for visible same-user peer n1b") "n1 must receive the online patch for visible same-user peer n1b")
} }
@ -346,7 +348,7 @@ func TestBuildFromChangeFiltersUserProfilesByVisibility(t *testing.T) {
require.NotNil(t, resp) require.NotNil(t, resp)
for _, up := range resp.UserProfiles { for _, up := range resp.UserProfiles {
assert.NotEqual(t, tailcfg.UserID(user2.ID), up.ID, assert.NotEqual(t, user2.TailscaleUserProfile().ID, up.ID,
"n1 must not receive user2's profile; n2 is not ACL-visible to n1") "n1 must not receive user2's profile; n2 is not ACL-visible to n1")
} }
} }
@ -389,6 +391,7 @@ func TestBuildFromChangeVisibilityMatchesFullMap(t *testing.T) {
database, err := db.NewHeadscaleDatabase(cfg) database, err := db.NewHeadscaleDatabase(cfg)
require.NoError(t, err) require.NoError(t, err)
user1 := database.CreateUserForTest("u1") user1 := database.CreateUserForTest("u1")
user2 := database.CreateUserForTest("u2") user2 := database.CreateUserForTest("u2")
n1 := database.CreateRegisteredNodeForTest(user1, "n1") n1 := database.CreateRegisteredNodeForTest(user1, "n1")
@ -406,62 +409,77 @@ func TestBuildFromChangeVisibilityMatchesFullMap(t *testing.T) {
// fullVisible returns the peer IDs n1 sees in the full map. // fullVisible returns the peer IDs n1 sees in the full map.
fullVisible := func(t *testing.T) map[tailcfg.NodeID]bool { fullVisible := func(t *testing.T) map[tailcfg.NodeID]bool {
t.Helper() t.Helper()
resp, err := m.fullMapResponse(n1.ID, capVer) resp, err := m.fullMapResponse(n1.ID, capVer)
require.NoError(t, err) require.NoError(t, err)
got := map[tailcfg.NodeID]bool{} got := map[tailcfg.NodeID]bool{}
for _, p := range resp.Peers { for _, p := range resp.Peers {
got[p.ID] = true got[p.ID] = true
} }
return got return got
} }
// patchReaches reports whether a NodeOnline patch for id is delivered to n1. // patchReaches reports whether a NodeOnline patch for id is delivered to n1.
patchReaches := func(t *testing.T, id types.NodeID) bool { patchReaches := func(t *testing.T, id types.NodeID) bool {
t.Helper() t.Helper()
c := change.NodeOnline(id) c := change.NodeOnline(id)
resp, err := m.buildFromChange(n1.ID, capVer, &c) resp, err := m.buildFromChange(n1.ID, capVer, &c)
require.NoError(t, err) require.NoError(t, err)
if resp == nil { if resp == nil {
return false return false
} }
for _, p := range resp.PeersChangedPatch { for _, p := range resp.PeersChangedPatch {
if p.NodeID == id.NodeID() { if p.NodeID == id.NodeID() {
return true return true
} }
} }
return false return false
} }
// changedReaches reports whether a NodeAdded changed-peer for id reaches n1. // changedReaches reports whether a NodeAdded changed-peer for id reaches n1.
changedReaches := func(t *testing.T, id types.NodeID) bool { changedReaches := func(t *testing.T, id types.NodeID) bool {
t.Helper() t.Helper()
c := change.NodeAdded(id) c := change.NodeAdded(id)
resp, err := m.buildFromChange(n1.ID, capVer, &c) resp, err := m.buildFromChange(n1.ID, capVer, &c)
require.NoError(t, err) require.NoError(t, err)
if resp == nil { if resp == nil {
return false return false
} }
for _, p := range resp.PeersChanged { for _, p := range resp.PeersChanged {
if p.ID == id.NodeID() { if p.ID == id.NodeID() {
return true return true
} }
} }
return false return false
} }
// profileReaches reports whether userID's profile is delivered to n1 when n // profileReaches reports whether want's profile is delivered to n1 when n
// is added. Use a cross-user node so the result is not masked by n1's own // is added. Use a cross-user node so the result is not masked by n1's own
// always-present user profile. // always-present user profile.
profileReaches := func(t *testing.T, n *types.Node, userID uint) bool { profileReaches := func(t *testing.T, n *types.Node, want tailcfg.UserID) bool {
t.Helper() t.Helper()
c := change.NodeAdded(n.ID) c := change.NodeAdded(n.ID)
resp, err := m.buildFromChange(n1.ID, capVer, &c) resp, err := m.buildFromChange(n1.ID, capVer, &c)
require.NoError(t, err) require.NoError(t, err)
if resp == nil { if resp == nil {
return false return false
} }
for _, up := range resp.UserProfiles { for _, up := range resp.UserProfiles {
if up.ID == tailcfg.UserID(userID) { if up.ID == want {
return true return true
} }
} }
return false return false
} }
@ -501,6 +519,7 @@ func TestBuildFromChangeVisibilityMatchesFullMap(t *testing.T) {
full := fullVisible(t) full := fullVisible(t)
require.Lenf(t, full, tt.wantFull, require.Lenf(t, full, tt.wantFull,
"%s: unexpected full-map visible peer count", tt.name) "%s: unexpected full-map visible peer count", tt.name)
for _, peer := range []*types.Node{n1b, n2} { for _, peer := range []*types.Node{n1b, n2} {
want := full[peer.ID.NodeID()] want := full[peer.ID.NodeID()]
assert.Equalf(t, want, patchReaches(t, peer.ID), assert.Equalf(t, want, patchReaches(t, peer.ID),
@ -511,7 +530,7 @@ func TestBuildFromChangeVisibilityMatchesFullMap(t *testing.T) {
tt.name, peer.Hostname) tt.name, peer.Hostname)
} }
// Cross-user profile (user2) must appear iff n2 is visible to n1. // Cross-user profile (user2) must appear iff n2 is visible to n1.
assert.Equalf(t, full[n2.ID.NodeID()], profileReaches(t, n2, user2.ID), assert.Equalf(t, full[n2.ID.NodeID()], profileReaches(t, n2, user2.TailscaleUserProfile().ID),
"%s: user2 profile must be sent iff n2 is visible to n1", tt.name) "%s: user2 profile must be sent iff n2 is visible to n1", tt.name)
}) })
} }

View file

@ -184,7 +184,7 @@ func TestDoOIDCAuthorization(t *testing.T) {
// browsers that do not default to Lax sending it on cross-site requests. // browsers that do not default to Lax sending it on cross-site requests.
func TestSetCSRFCookieSameSite(t *testing.T) { func TestSetCSRFCookieSameSite(t *testing.T) {
w := httptest.NewRecorder() w := httptest.NewRecorder()
r := httptest.NewRequest(http.MethodGet, "/auth/abcdef0123456789", nil) r := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/auth/abcdef0123456789", nil)
_, err := setCSRFCookie(w, r, "state") _, err := setCSRFCookie(w, r, "state")
require.NoError(t, err) require.NoError(t, err)

View file

@ -229,6 +229,7 @@ func TestRegistrationRejectsNodeKeyClaimedByAnotherMachine(t *testing.T) {
database, err := db.NewHeadscaleDatabase(cfg) database, err := db.NewHeadscaleDatabase(cfg)
require.NoError(t, err) require.NoError(t, err)
user := database.CreateUserForTest("nk-user") user := database.CreateUserForTest("nk-user")
require.NoError(t, database.Close()) require.NoError(t, database.Close())
@ -273,6 +274,7 @@ func TestReauthRejectsNodeKeyClaimedByAnotherMachine(t *testing.T) {
database, err := db.NewHeadscaleDatabase(cfg) database, err := db.NewHeadscaleDatabase(cfg)
require.NoError(t, err) require.NoError(t, err)
attacker := database.CreateUserForTest("attacker") attacker := database.CreateUserForTest("attacker")
victim := database.CreateUserForTest("victim") victim := database.CreateUserForTest("victim")
require.NoError(t, database.Close()) require.NoError(t, database.Close())

View file

@ -112,6 +112,11 @@ var nodeUpdateColumns = []string{
// ErrRegistrationExpired is returned when a registration has expired. // ErrRegistrationExpired is returned when a registration has expired.
var ErrRegistrationExpired = errors.New("registration expired") var ErrRegistrationExpired = errors.New("registration expired")
// ErrNodeKeyInUse is returned when a registration or re-auth claims a NodeKey
// already bound to a different machine, enforcing the 1:1 NodeKey<->MachineKey
// binding.
var ErrNodeKeyInUse = errors.New("node key already in use by another machine")
// sshCheckPair identifies a (source, destination) node pair for // sshCheckPair identifies a (source, destination) node pair for
// SSH check auth tracking. // SSH check auth tracking.
type sshCheckPair struct { type sshCheckPair struct {
@ -523,6 +528,7 @@ func (s *State) persistNodeRowToDB(node types.NodeView) (types.NodeView, error)
// SetNodeExpiry calls or re-registration, not during MapRequest updates. // SetNodeExpiry calls or re-registration, not during MapRequest updates.
err := s.db.DB.Select(nodeUpdateColumns).Omit("Expiry").Updates(nodePtr).Error err := s.db.DB.Select(nodeUpdateColumns).Omit("Expiry").Updates(nodePtr).Error
s.persistMu.Unlock() s.persistMu.Unlock()
if err != nil { if err != nil {
return types.NodeView{}, fmt.Errorf("saving node: %w", err) return types.NodeView{}, fmt.Errorf("saving node: %w", err)
} }
@ -1572,9 +1578,7 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView
// the NodeStore NodeKey index (denying the victim service). // the NodeStore NodeKey index (denying the victim service).
if existing, ok := s.nodeStore.GetNodeByNodeKey(regData.NodeKey); ok && if existing, ok := s.nodeStore.GetNodeByNodeKey(regData.NodeKey); ok &&
existing.MachineKey() != regData.MachineKey { existing.MachineKey() != regData.MachineKey {
return types.NodeView{}, fmt.Errorf( return types.NodeView{}, ErrNodeKeyInUse
"node key already in use by another machine",
)
} }
// Update existing node in [NodeStore] - validation passed, safe to mutate // Update existing node in [NodeStore] - validation passed, safe to mutate
@ -1715,9 +1719,7 @@ func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, erro
// time and reject before allocating any resources. // time and reject before allocating any resources.
if existing, ok := s.nodeStore.GetNodeByNodeKey(params.NodeKey); ok && if existing, ok := s.nodeStore.GetNodeByNodeKey(params.NodeKey); ok &&
existing.MachineKey() != params.MachineKey { existing.MachineKey() != params.MachineKey {
return types.NodeView{}, fmt.Errorf( return types.NodeView{}, ErrNodeKeyInUse
"node key already in use by another machine",
)
} }
// Prepare the node for registration // Prepare the node for registration