diff --git a/hscontrol/servertest/sshcheck_test.go b/hscontrol/servertest/sshcheck_test.go index 020ded5e5..493834a50 100644 --- a/hscontrol/servertest/sshcheck_test.go +++ b/hscontrol/servertest/sshcheck_test.go @@ -245,3 +245,42 @@ func TestSSHCheckFollowsReturnedHoldURL(t *testing.T) { expandHoldURL(initial.HoldAndDelegate, srcID, dstID, "root")) assert.True(t, followUp.Accept, "authenticated root login must be accepted, got %+v", followUp) } + +// TestSSHCheckApprovalDroppedAfterSourceTagConversion: an approval does not +// outlive its source's owner. A check rule cannot name a tag source, so the +// converted source is not let in at all. +func TestSSHCheckApprovalDroppedAfterSourceTagConversion(t *testing.T) { + t.Parallel() + h := servertest.NewHarness(t, 2) + srcID := types.NodeID(h.Client(0).Netmap().SelfNode.ID()) //nolint:gosec + dstID := types.NodeID(h.Client(1).Netmap().SelfNode.ID()) //nolint:gosec + h.ChangePolicy(t, []byte(`{ +"tagOwners":{"tag:ci":["harness-default@"],"tag:server":["harness-default@"]}, +"ssh":[{"action":"check","src":["harness-default@"], +"dst":["tag:server"],"users":["autogroup:nonroot"],"checkPeriod":"1h"}]}`)) + + _, c, err := h.Server.State().SetNodeTags(dstID, []string{"tag:server"}) + require.NoError(t, err) + h.Server.App.Change(c) + + initial := pollSSHAction(t, h.Server.URL, h.Client(1), srcID, dstID, "") + require.NotEmpty(t, initial.HoldAndDelegate) + authID := authIDFromHoldURL(t, initial.HoldAndDelegate) + auth, ok := h.Server.State().GetAuthCacheEntry(authID) + require.True(t, ok) + auth.FinishAuth(types.AuthVerdict{}) + + approved := pollSSHAction(t, h.Server.URL, h.Client(1), srcID, dstID, authID.String()) + require.True(t, approved.Accept) + cached := pollSSHAction(t, h.Server.URL, h.Client(1), srcID, dstID, "") + require.True(t, cached.Accept) + + _, c, err = h.Server.State().SetNodeTags(srcID, []string{"tag:ci"}) + require.NoError(t, err) + h.Server.App.Change(c) + _, checkFound := h.Server.State().SSHCheckParams(srcID, dstID, sshCheckLocalUser) + require.False(t, checkFound) + after := pollSSHAction(t, h.Server.URL, h.Client(1), srcID, dstID, "") + assert.False(t, after.Accept) + assert.Empty(t, after.HoldAndDelegate) +} diff --git a/hscontrol/state/ssh_check_test.go b/hscontrol/state/ssh_check_test.go index 269484a21..a2eb015e0 100644 --- a/hscontrol/state/ssh_check_test.go +++ b/hscontrol/state/ssh_check_test.go @@ -1,6 +1,7 @@ package state import ( + "fmt" "sync" "testing" "time" @@ -8,16 +9,26 @@ import ( "github.com/juanfont/headscale/hscontrol/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "tailscale.com/types/key" ) -func newTestStateForSSHCheck() *State { - return &State{ - sshCheckAuth: make(map[sshCheckPair]time.Time), +func newTestStateForSSHCheck(t *testing.T) *State { + t.Helper() + + s := newRetagTestState(t) + + user := s.CreateUserForTest("ssh-user") + for i := range 10 { + node := s.CreateRegisteredNodeForTest(user, fmt.Sprintf("ssh-node-%d", i)) + node.User = user + s.nodeStore.PutNode(*node) } + + return s } func TestSSHCheckAuth(t *testing.T) { - s := newTestStateForSSHCheck() + s := newTestStateForSSHCheck(t) src := types.NodeID(1) dst := types.NodeID(2) @@ -46,7 +57,7 @@ func TestSSHCheckAuth(t *testing.T) { } func TestSSHCheckAuthClear(t *testing.T) { - s := newTestStateForSSHCheck() + s := newTestStateForSSHCheck(t) s.SetLastSSHAuth(types.NodeID(1), types.NodeID(2)) s.SetLastSSHAuth(types.NodeID(1), types.NodeID(3)) @@ -68,13 +79,13 @@ func TestSSHCheckAuthClear(t *testing.T) { } func TestSSHCheckAuthConcurrent(t *testing.T) { - s := newTestStateForSSHCheck() + s := newTestStateForSSHCheck(t) var wg sync.WaitGroup for i := range 100 { wg.Go(func() { - src := types.NodeID(uint64(i % 10)) //nolint:gosec + src := types.NodeID(uint64(i%10 + 1)) //nolint:gosec dst := types.NodeID(uint64(i%5 + 10)) //nolint:gosec s.SetLastSSHAuth(src, dst) @@ -95,3 +106,79 @@ func TestSSHCheckAuthConcurrent(t *testing.T) { wg.Wait() } + +func TestSSHCheckAuthFollowsSourceOwner(t *testing.T) { + for _, tt := range []struct { + name string + tagged bool + kept bool + }{ + {"convert", false, false}, + {"retag", true, false}, + {"user_change", false, false}, + {"delete", false, false}, + {"destination_change", false, true}, + {"rekey", false, true}, + {"same_tags", true, true}, + } { + t.Run(tt.name, func(t *testing.T) { + s := newTestStateForSSHCheck(t) + _, err := s.SetPolicy([]byte(`{"tagOwners":{"tag:ci":["ssh-user@"],"tag:ops":["ssh-user@"]}}`)) + require.NoError(t, err) + + src, dst := types.NodeID(1), types.NodeID(2) + if tt.tagged { + _, _, err = s.SetNodeTags(src, []string{"tag:ci", "tag:ops"}) + require.NoError(t, err) + } + + s.SetLastSSHAuth(src, dst) + at, ok := s.GetLastSSHAuth(src, dst) + require.True(t, ok) + + switch tt.name { + case "convert", "destination_change": + id := src + if tt.name == "destination_change" { + id = dst + } + + _, _, err = s.SetNodeTags(id, []string{"tag:ci"}) + require.NoError(t, err) + case "retag": + _, _, err = s.SetNodeTags(src, []string{"tag:ops"}) + require.NoError(t, err) + case "user_change": + other := s.CreateUserForTest("other-user") + _, ok = s.nodeStore.UpdateNode(src, func(n *types.Node) { + n.UserID = &other.ID + n.User = other + }) + require.True(t, ok) + case "delete": + node, ok := s.GetNodeByID(src) + require.True(t, ok) + + _, err = s.DeleteNode(node) + require.NoError(t, err) + case "rekey": + _, ok = s.nodeStore.UpdateNode(src, func(n *types.Node) { n.NodeKey = key.NewNode().Public() }) + require.True(t, ok) + case "same_tags": + _, ok = s.nodeStore.UpdateNode(src, func(n *types.Node) { + n.Tags = []string{"tag:ops", "tag:ci"} + }) + require.True(t, ok) + } + + last, ok := s.GetLastSSHAuth(src, dst) + assert.Equal(t, tt.kept, ok) + + if tt.kept { + assert.Equal(t, at, last) + } else { + assert.True(t, last.IsZero()) + } + }) + } +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index df459b389..56471d6a7 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -147,6 +147,11 @@ type sshCheckPair struct { Dst types.NodeID } +type sshCheckAuthRecord struct { + at time.Time + src types.NodeView +} + // State manages Headscale's core state, coordinating between database, policy management, // IP allocation, and DERP routing. All methods are thread-safe. // @@ -199,7 +204,7 @@ type State struct { // // Ref: https://github.com/tailscale/tailscale/issues/10480 // Ref: https://github.com/tailscale/tailscale/issues/7125 - sshCheckAuth map[sshCheckPair]time.Time + sshCheckAuth map[sshCheckPair]sshCheckAuthRecord sshCheckMu sync.RWMutex // persistMu serialises node-row persistence and deletion so the database @@ -353,7 +358,7 @@ func NewState(cfg *types.Config) (*State, error) { nodeStore: nodeStore, pings: newPingTracker(), - sshCheckAuth: make(map[sshCheckPair]time.Time), + sshCheckAuth: make(map[sshCheckPair]sshCheckAuthRecord), registerLocks: xsync.NewMap[key.MachinePublic, *registerLock](), } @@ -1993,18 +1998,33 @@ func (s *State) SetLastSSHAuth(src, dst types.NodeID) { s.sshCheckMu.Lock() defer s.sshCheckMu.Unlock() - s.sshCheckAuth[sshCheckPair{Src: src, Dst: dst}] = time.Now() + node, ok := s.nodeStore.GetNode(src) + if !ok { + return + } + + s.sshCheckAuth[sshCheckPair{Src: src, Dst: dst}] = sshCheckAuthRecord{ + at: time.Now(), src: node, + } } // GetLastSSHAuth returns when src last authenticated for SSH check -// to dst. +// to dst, while the source still has the owner that authenticated. func (s *State) GetLastSSHAuth(src, dst types.NodeID) (time.Time, bool) { s.sshCheckMu.RLock() defer s.sshCheckMu.RUnlock() - t, ok := s.sshCheckAuth[sshCheckPair{Src: src, Dst: dst}] + record, ok := s.sshCheckAuth[sshCheckPair{Src: src, Dst: dst}] + if !ok { + return time.Time{}, false + } - return t, ok + node, exists := s.nodeStore.GetNode(src) + if !exists || !sameOwner(record.src, node) { + return time.Time{}, false + } + + return record.at, true } // ClearSSHCheckAuth clears all recorded SSH check auth times. @@ -2013,7 +2033,7 @@ func (s *State) ClearSSHCheckAuth() { s.sshCheckMu.Lock() defer s.sshCheckMu.Unlock() - s.sshCheckAuth = make(map[sshCheckPair]time.Time) + s.sshCheckAuth = make(map[sshCheckPair]sshCheckAuthRecord) } // preserveNetInfo preserves NetInfo from an existing node for faster DERP connectivity. @@ -2066,6 +2086,20 @@ type authNodeUpdateParams struct { IsConvertFromTag bool } +// sameOwner reports whether a and b have one owner: the same tag set for +// tagged nodes, the same user otherwise. +func sameOwner(a, b types.NodeView) bool { + if !a.Valid() || !b.Valid() || a.IsTagged() != b.IsTagged() { + return false + } + + if a.IsTagged() { + return views.SliceEqualAnyOrder(a.Tags(), b.Tags()) + } + + return a.UserID().Valid() && b.UserID().Valid() && a.UserID().Get() == b.UserID().Get() +} + // applyAuthNodeUpdate applies common update logic for re-authenticating or converting // an existing node. It updates the node in [NodeStore], processes RequestTags, and // persists changes to the database. The bool reports whether peers read the @@ -2223,6 +2257,9 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView exp := time.Now().Add(s.cfg.Node.Expiry) node.Expiry = &exp } + + // Route approvals stay on an owner change, as on Tailscale SaaS; + // autoApprovers only add. }) if !ok { @@ -3266,6 +3303,9 @@ func (s *State) reregisterNodeWithPAK( default: node.Expiry = nil } + + // Route approvals stay on an owner change, as on Tailscale SaaS; + // autoApprovers only add. }) // A nil prior means the writer never saw the node (deleted, or the store