diff --git a/hscontrol/mapper/mapper.go b/hscontrol/mapper/mapper.go index d61c097b..a9a43073 100644 --- a/hscontrol/mapper/mapper.go +++ b/hscontrol/mapper/mapper.go @@ -458,7 +458,7 @@ func (m *mapper) buildFromChange( // // 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. -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) if !ok { return nil, false @@ -477,9 +477,11 @@ func (m *mapper) visiblePeerIDs(nodeID types.NodeID) (map[types.NodeID]struct{}, 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() { - visible[peer.ID()] = struct{}{} + visible[peer.ID().NodeID()] = struct{}{} } return visible, true @@ -504,8 +506,9 @@ func (m *mapper) filterVisiblePeerPatches( } var filtered []*tailcfg.PeerChange + for _, patch := range patches { - if _, vis := visible[types.NodeID(patch.NodeID)]; vis { + if _, vis := visible[patch.NodeID]; vis { filtered = append(filtered, patch) } } @@ -528,8 +531,9 @@ func (m *mapper) filterVisibleNodes( } var filtered []types.NodeView + for _, peer := range peers.All() { - if _, vis := visible[peer.ID()]; vis { + if _, vis := visible[peer.ID().NodeID()]; vis { filtered = append(filtered, peer) } } diff --git a/hscontrol/mapper/mapper_test.go b/hscontrol/mapper/mapper_test.go index a740da72..94880c78 100644 --- a/hscontrol/mapper/mapper_test.go +++ b/hscontrol/mapper/mapper_test.go @@ -273,11 +273,13 @@ func TestBuildFromChangeFiltersPeerPatchesByVisibility(t *testing.T) { require.NotNil(t, resp2) var gotVisible bool + for _, p := range resp2.PeersChangedPatch { if p.NodeID == n1b.ID.NodeID() { gotVisible = true } } + assert.True(t, gotVisible, "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) 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") } } @@ -389,6 +391,7 @@ func TestBuildFromChangeVisibilityMatchesFullMap(t *testing.T) { database, err := db.NewHeadscaleDatabase(cfg) require.NoError(t, err) + user1 := database.CreateUserForTest("u1") user2 := database.CreateUserForTest("u2") 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 := func(t *testing.T) map[tailcfg.NodeID]bool { t.Helper() + resp, err := m.fullMapResponse(n1.ID, capVer) require.NoError(t, err) + got := map[tailcfg.NodeID]bool{} for _, p := range resp.Peers { got[p.ID] = true } + return got } // patchReaches reports whether a NodeOnline patch for id is delivered to n1. patchReaches := func(t *testing.T, id types.NodeID) bool { t.Helper() + c := change.NodeOnline(id) resp, err := m.buildFromChange(n1.ID, capVer, &c) require.NoError(t, err) + if resp == nil { return false } + for _, p := range resp.PeersChangedPatch { if p.NodeID == id.NodeID() { return true } } + return false } // changedReaches reports whether a NodeAdded changed-peer for id reaches n1. changedReaches := func(t *testing.T, id types.NodeID) bool { t.Helper() + c := change.NodeAdded(id) resp, err := m.buildFromChange(n1.ID, capVer, &c) require.NoError(t, err) + if resp == nil { return false } + for _, p := range resp.PeersChanged { if p.ID == id.NodeID() { return true } } + 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 // 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() + c := change.NodeAdded(n.ID) resp, err := m.buildFromChange(n1.ID, capVer, &c) require.NoError(t, err) + if resp == nil { return false } + for _, up := range resp.UserProfiles { - if up.ID == tailcfg.UserID(userID) { + if up.ID == want { return true } } + return false } @@ -501,6 +519,7 @@ func TestBuildFromChangeVisibilityMatchesFullMap(t *testing.T) { full := fullVisible(t) require.Lenf(t, full, tt.wantFull, "%s: unexpected full-map visible peer count", tt.name) + for _, peer := range []*types.Node{n1b, n2} { want := full[peer.ID.NodeID()] assert.Equalf(t, want, patchReaches(t, peer.ID), @@ -511,7 +530,7 @@ func TestBuildFromChangeVisibilityMatchesFullMap(t *testing.T) { tt.name, peer.Hostname) } // 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) }) } diff --git a/hscontrol/oidc_test.go b/hscontrol/oidc_test.go index a77ba1bc..0472f01c 100644 --- a/hscontrol/oidc_test.go +++ b/hscontrol/oidc_test.go @@ -184,7 +184,7 @@ func TestDoOIDCAuthorization(t *testing.T) { // browsers that do not default to Lax sending it on cross-site requests. func TestSetCSRFCookieSameSite(t *testing.T) { 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") require.NoError(t, err) diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index cecc17dc..0adac6d5 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -229,6 +229,7 @@ func TestRegistrationRejectsNodeKeyClaimedByAnotherMachine(t *testing.T) { database, err := db.NewHeadscaleDatabase(cfg) require.NoError(t, err) + user := database.CreateUserForTest("nk-user") require.NoError(t, database.Close()) @@ -273,6 +274,7 @@ func TestReauthRejectsNodeKeyClaimedByAnotherMachine(t *testing.T) { database, err := db.NewHeadscaleDatabase(cfg) require.NoError(t, err) + attacker := database.CreateUserForTest("attacker") victim := database.CreateUserForTest("victim") require.NoError(t, database.Close()) diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index c6568f36..a59219a9 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -112,6 +112,11 @@ var nodeUpdateColumns = []string{ // ErrRegistrationExpired is returned when a registration has 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 // SSH check auth tracking. 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. err := s.db.DB.Select(nodeUpdateColumns).Omit("Expiry").Updates(nodePtr).Error s.persistMu.Unlock() + if err != nil { 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). if existing, ok := s.nodeStore.GetNodeByNodeKey(regData.NodeKey); ok && existing.MachineKey() != regData.MachineKey { - return types.NodeView{}, fmt.Errorf( - "node key already in use by another machine", - ) + return types.NodeView{}, ErrNodeKeyInUse } // 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. if existing, ok := s.nodeStore.GetNodeByNodeKey(params.NodeKey); ok && existing.MachineKey() != params.MachineKey { - return types.NodeView{}, fmt.Errorf( - "node key already in use by another machine", - ) + return types.NodeView{}, ErrNodeKeyInUse } // Prepare the node for registration