diff --git a/CHANGELOG.md b/CHANGELOG.md index 176260a91..930de5aa0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -130,6 +130,8 @@ clients, and how to run the same setup without Nix. - A registration request from a client below the minimum supported version is now rejected before it can log a node out, use a pre-auth key or start a login [#3519](https://github.com/juanfont/headscale/pull/3519) - Fix SSH check accepting a repeated follow-up for an already-decided session, even after a rejection [#3526](https://github.com/juanfont/headscale/pull/3526) +- A node re-registering with a spent, expired or revoked pre-auth key is now rejected if it expired or changed node key while the re-registration was in flight + ## 0.29.5 (202x-xx-xx) **Minimum supported Tailscale client version: v1.80.0** diff --git a/hscontrol/state/auth_tagged_expiry_test.go b/hscontrol/state/auth_tagged_expiry_test.go index 60288f6d7..bd94a5d78 100644 --- a/hscontrol/state/auth_tagged_expiry_test.go +++ b/hscontrol/state/auth_tagged_expiry_test.go @@ -2,6 +2,9 @@ package state import ( "fmt" + "runtime" + "strings" + "sync" "testing" "time" @@ -9,6 +12,7 @@ import ( "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" "github.com/stretchr/testify/require" + "gorm.io/gorm" "tailscale.com/tailcfg" "tailscale.com/types/key" ) @@ -1614,3 +1618,585 @@ func TestTaggingPreservesNodeExpiry(t *testing.T) { require.NotNil(t, tagged.AsStruct().Expiry, "tag change must not clear expiry") require.Equal(t, expiry.Unix(), tagged.AsStruct().Expiry.Unix()) } + +// oldPAKSkipExpr is the inline skip expression pakSkipsValidation replaced, +// with the lookup's existsSameUser taken as true. It is the oracle for nodes +// findExistingNodeForPAK can return, away from the expiry boundary. +func oldPAKSkipExpr(n types.NodeView, pak *types.PreAuthKey, nodeKey key.NodePublic) bool { + isExistingNodeReregistering := n.Valid() + isNodeKeyRotation := n.Valid() && n.NodeKey() != nodeKey + isExpired := n.Valid() && !n.IsTagged() && n.IsExpired() + isOwnershipConversion := n.Valid() && pak.IsTagged() && !n.IsTagged() + isRetag := n.Valid() && pak.IsTagged() && n.IsTagged() && + (!n.AuthKeyID().Valid() || n.AuthKeyID().Get() != pak.ID) + + return isExistingNodeReregistering && !isNodeKeyRotation && !isExpired && + !isOwnershipConversion && !isRetag +} + +func TestPAKSkipsValidation(t *testing.T) { + now := time.Now() + past := now.Add(-time.Hour) + future := now.Add(time.Hour) + zero := time.Time{} + oneNanoAgo := now.Add(-time.Nanosecond) + + nodeKey := key.NewNode().Public() + otherNodeKey := key.NewNode().Public() + ownerID, otherUserID := uint(1), uint(2) + keyID, otherKeyID := uint64(10), uint64(11) + + userKey := &types.PreAuthKey{ID: keyID, User: &types.User{ID: ownerID}} + taggedKey := &types.PreAuthKey{ID: keyID, Tags: []string{"tag:a"}} + + userNode := func(uid uint, expiry *time.Time) *types.Node { + return &types.Node{ + ID: 1, + NodeKey: nodeKey, + UserID: &uid, + User: &types.User{ID: uid}, + Expiry: expiry, + } + } + taggedNode := func(expiry *time.Time, authKeyID *uint64) *types.Node { + return &types.Node{ + ID: 1, + NodeKey: nodeKey, + Tags: []string{"tag:a"}, + Expiry: expiry, + AuthKeyID: authKeyID, + } + } + + tests := []struct { + name string + node *types.Node + pak *types.PreAuthKey + nodeKey key.NodePublic + // lookup marks nodes findExistingNodeForPAK can return for pak, away + // from the expiry boundary, where the old expression is the oracle. + lookup bool + want bool + }{ + {"plain restart", userNode(ownerID, &future), userKey, nodeKey, true, true}, + {"rotation", userNode(ownerID, &future), userKey, otherNodeKey, true, false}, + {"expired user node", userNode(ownerID, &past), userKey, nodeKey, true, false}, + {"zero expiry", userNode(ownerID, &zero), userKey, nodeKey, true, true}, + {"nil expiry", userNode(ownerID, nil), userKey, nodeKey, true, true}, + {"expired tagged node, same key", taggedNode(&past, &keyID), taggedKey, nodeKey, true, true}, + {"conversion", userNode(ownerID, &future), taggedKey, nodeKey, true, false}, + {"retag, AuthKeyID nil", taggedNode(nil, nil), taggedKey, nodeKey, true, false}, + {"retag, other AuthKeyID", taggedNode(nil, &otherKeyID), taggedKey, nodeKey, true, false}, + {"retag, same AuthKeyID", taggedNode(nil, &keyID), taggedKey, nodeKey, true, true}, + {"tagged key, rotation", taggedNode(nil, &keyID), taggedKey, otherNodeKey, true, false}, + {"user key on a tagged node", taggedNode(nil, &otherKeyID), userKey, nodeKey, true, true}, + {"user key on an expired tagged node", taggedNode(&past, nil), userKey, nodeKey, true, true}, + {"user key on another user's node", userNode(otherUserID, &future), userKey, nodeKey, false, false}, + {"user node expiry equals now", userNode(ownerID, &now), userKey, nodeKey, false, true}, + {"user node expired one nanosecond before now", userNode(ownerID, &oneNanoAgo), userKey, nodeKey, false, false}, + {"invalid view", nil, userKey, nodeKey, false, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var view types.NodeView + if tt.node != nil { + view = tt.node.View() + } + + require.Equal(t, tt.want, pakSkipsValidation(view, tt.pak, tt.nodeKey, now)) + + if tt.lookup { + require.Equal(t, tt.want, oldPAKSkipExpr(view, tt.pak, tt.nodeKey), + "must match the expression it replaced") + } + }) + } +} + +// pakReregCase is a node registered with a pre-auth key as hostname "h0", and +// regReq, a re-registration of it with the same node key as hostname "h1". +type pakReregCase struct { + s *State + machineKey key.MachinePublic + node types.NodeView + regReq tailcfg.RegisterRequest +} + +func registerForPAKRereg(t *testing.T, s *State, k *types.PreAuthKeyNew) pakReregCase { + t.Helper() + + machineKey := key.NewMachine().Public() + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: k.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "h0"}, + Expiry: time.Now().Add(24 * time.Hour), + } + + node, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey) + require.NoError(t, err) + + regReq.Hostinfo = &tailcfg.Hostinfo{Hostname: "h1"} + + return pakReregCase{s: s, machineKey: machineKey, node: node, regReq: regReq} +} + +// lookup runs the lookup half of a re-registration with keyStr, as +// HandleNodeFromPreAuthKey does, and requires that it skips key validation. +// The returned view and key row go stale once the caller writes concurrently. +func (c pakReregCase) lookup(t *testing.T, keyStr string) (types.NodeView, *types.PreAuthKey) { + t.Helper() + + pak, err := c.s.GetPreAuthKey(keyStr) + require.NoError(t, err) + + view, ok, err := c.s.findExistingNodeForPAK(c.machineKey, pak) + require.NoError(t, err) + require.True(t, ok) + require.True(t, pakSkipsValidation(view, pak, c.regReq.NodeKey, time.Now()), + "precondition: the lookup skips key validation") + + return view, pak +} + +// reregister runs the mutation half with the lookup's view and key row. +func (c pakReregCase) reregister( + view types.NodeView, + pak *types.PreAuthKey, + regReq tailcfg.RegisterRequest, +) (types.NodeView, error) { + hi := regReq.Hostinfo.Clone() + + return c.s.reregisterNodeWithPAK(view, pak, regReq, c.machineKey, hi.Hostname, hi) +} + +// pakNodeFields are the node fields a re-registration writes. Expiry is in +// Unix nanoseconds (0 for none) so NodeStore and database values compare. +type pakNodeFields struct { + NodeKey key.NodePublic + Hostname string + AuthKeyID *uint64 + Expiry int64 + Tags []string + UserID *uint +} + +func pakFieldsOf(n *types.Node) pakNodeFields { + f := pakNodeFields{ + NodeKey: n.NodeKey, + Hostname: n.Hostname, + AuthKeyID: n.AuthKeyID, + Tags: n.Tags, + UserID: n.UserID, + } + if n.Expiry != nil { + f.Expiry = n.Expiry.UnixNano() + } + + return f +} + +// fields returns the node's fields in the NodeStore and in the database. +func (c pakReregCase) fields(t *testing.T) (pakNodeFields, pakNodeFields) { + t.Helper() + + ns, ok := c.s.GetNodeByID(c.node.ID()) + require.True(t, ok) + + dbNode, err := c.s.db.GetNodeByID(c.node.ID()) + require.NoError(t, err) + + return pakFieldsOf(ns.AsStruct()), pakFieldsOf(dbNode) +} + +// waitParkedOnWriteQueue waits until a goroutine running fn is blocked handing +// a write to the NodeStore writer, which proves that everything fn does before +// that write has already run. +func waitParkedOnWriteQueue(t *testing.T, fn string) { + t.Helper() + + buf := make([]byte, 1<<22) + + require.Eventually(t, func() bool { + stacks := string(buf[:runtime.Stack(buf, true)]) + for g := range strings.SplitSeq(stacks, "\n\n") { + if strings.Contains(g, "[select") && + strings.Contains(g, "(*NodeStore).UpdateNodes(") && + strings.Contains(g, "."+fn+"(") { + return true + } + } + + return false + }, 5*time.Second, time.Millisecond) +} + +// TestPAKReregisterRevalidatesAtMutation: a re-registration whose lookup +// skipped key validation must decide again on the node the NodeStore writer +// holds, because a concurrent write can land between lookup and mutation. +func TestPAKReregisterRevalidatesAtMutation(t *testing.T) { + t.Run("admin_expire_spent_key", func(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("pakuser") + + k, err := s.CreatePreAuthKey(user.TypedID(), false, false, nil, nil) + require.NoError(t, err) + + c := registerForPAKRereg(t, s, k) + view, pak := c.lookup(t, k.Key) + require.True(t, pak.Used, "precondition: the single-use key is spent") + + tA := time.Now() + _, _, err = s.SetNodeExpiry(c.node.ID(), &tA) + require.NoError(t, err) + + nsBefore, dbBefore := c.fields(t) + + _, err = c.reregister(view, pak, c.regReq) + require.ErrorIs(t, err, types.PAKError("authkey already used")) + + nsAfter, dbAfter := c.fields(t) + require.Equal(t, nsBefore, nsAfter, "NodeStore node must be untouched") + require.Equal(t, dbBefore, dbAfter, "database node must be untouched") + require.Equal(t, tA.UnixNano(), nsAfter.Expiry) + require.Equal(t, tA.UnixNano(), dbAfter.Expiry) + require.Equal(t, "h0", nsAfter.Hostname) + + stored, err := s.GetPreAuthKey(k.Key) + require.NoError(t, err) + require.True(t, stored.Used, "the key is not consumed again") + }) + + t.Run("admin_expire_reusable_key", func(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("pakuser") + + k, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + + c := registerForPAKRereg(t, s, k) + view, pak := c.lookup(t, k.Key) + + tA := time.Now() + _, _, err = s.SetNodeExpiry(c.node.ID(), &tA) + require.NoError(t, err) + + got, err := c.reregister(view, pak, c.regReq) + require.NoError(t, err, "a valid key re-authorises whatever raced") + require.False(t, got.IsExpired()) + + ns, dbNode := c.fields(t) + require.Equal(t, c.regReq.Expiry.UnixNano(), ns.Expiry, "expiry is extended") + require.Equal(t, ns, dbNode, "database must equal the NodeStore") + }) + + t.Run("nodekey_changed_spent_key", func(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("pakuser") + + k, err := s.CreatePreAuthKey(user.TypedID(), false, false, nil, nil) + require.NoError(t, err) + + c := registerForPAKRereg(t, s, k) + view, pak := c.lookup(t, k.Key) + + // A MapRequest reconcile moves the node to a new node key. + reconciled := key.NewNode().Public() + _, ok := s.nodeStore.UpdateNode(c.node.ID(), func(n *types.Node) { + n.NodeKey = reconciled + }) + require.True(t, ok) + + nsBefore, dbBefore := c.fields(t) + + _, err = c.reregister(view, pak, c.regReq) + require.ErrorIs(t, err, types.PAKError("authkey already used")) + + nsAfter, dbAfter := c.fields(t) + require.Equal(t, nsBefore, nsAfter, "NodeStore node must be untouched") + require.Equal(t, dbBefore, dbAfter, "database node must be untouched") + require.Equal(t, reconciled, nsAfter.NodeKey) + }) + + t.Run("retag_same_tagged_key", func(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("pakuser") + + _, err := s.SetPolicy(fmt.Appendf(nil, + `{"tagOwners":{"tag:a":["%s@"],"tag:b":["%s@"]}}`, user.Name, user.Name)) + require.NoError(t, err) + + k, err := s.CreatePreAuthKey(nil, false, false, nil, []string{"tag:a"}) + require.NoError(t, err) + + c := registerForPAKRereg(t, s, k) + view, pak := c.lookup(t, k.Key) + + _, _, err = s.SetNodeTags(c.node.ID(), []string{"tag:b"}) + require.NoError(t, err) + + _, err = c.reregister(view, pak, c.regReq) + require.NoError(t, err) + + ns, dbNode := c.fields(t) + require.Equal(t, []string{"tag:b"}, ns.Tags, "the admin's tags are kept") + require.Equal(t, []string{"tag:b"}, dbNode.Tags) + }) + + t.Run("owner_to_tagged_user_key", func(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("pakuser") + + _, err := s.SetPolicy(fmt.Appendf(nil, `{"tagOwners":{"tag:a":["%s@"]}}`, user.Name)) + require.NoError(t, err) + + k, err := s.CreatePreAuthKey(user.TypedID(), false, false, nil, nil) + require.NoError(t, err) + + c := registerForPAKRereg(t, s, k) + view, pak := c.lookup(t, k.Key) + require.False(t, view.IsTagged(), "precondition: the lookup saw a user-owned node") + + _, _, err = s.SetNodeTags(c.node.ID(), []string{"tag:a"}) + require.NoError(t, err) + + got, err := c.reregister(view, pak, c.regReq) + require.NoError(t, err, "a user key on a tagged node skips validation") + require.True(t, got.IsTagged()) + + ns, dbNode := c.fields(t) + require.Nil(t, ns.UserID) + require.Nil(t, dbNode.UserID) + require.Equal(t, []string{"tag:a"}, dbNode.Tags) + }) + + t.Run("deleted", func(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("pakuser") + + k, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + + c := registerForPAKRereg(t, s, k) + view, pak := c.lookup(t, k.Key) + + _, err = s.DeleteNode(view) + require.NoError(t, err) + + _, err = c.reregister(view, pak, c.regReq) + require.ErrorIs(t, err, ErrNodeNotInNodeStore) + + _, err = s.db.GetNodeByID(c.node.ID()) + require.ErrorIs(t, err, gorm.ErrRecordNotFound, "no database row may be written") + }) + + // A stopped NodeStore drops the write, so the writer never decides and + // UpdateNode still reports the node from the last snapshot. + t.Run("store_stopped", func(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("pakuser") + + k0, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + + c := registerForPAKRereg(t, s, k0) + + k, err := s.CreatePreAuthKey(user.TypedID(), false, false, nil, nil) + require.NoError(t, err) + + regReq := c.regReq + regReq.Auth = &tailcfg.RegisterResponseAuth{AuthKey: k.Key} + + view, pak := c.lookup(t, k.Key) + require.False(t, pak.Used, "precondition: the lookup read a fresh single-use key") + + _, dbBefore := c.fields(t) + + s.nodeStore.Stop() + + _, err = c.reregister(view, pak, regReq) + require.ErrorIs(t, err, ErrNodeNotInNodeStore) + + _, dbAfter := c.fields(t) + require.Equal(t, dbBefore, dbAfter, "database node must be untouched") + + stored, err := s.GetPreAuthKey(k.Key) + require.NoError(t, err) + require.False(t, stored.Used, "the key must not be consumed") + }) + + // The node is expired after the lookup, then the key expires, then the + // writer runs. The decision must be taken at the writer's own clock: any + // validity or clock read taken before the NodeStore write lets the expired + // key revive the node. + t.Run("admin_expire_then_key_expires", func(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("pakuser") + + k0, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + + c := registerForPAKRereg(t, s, k0) + + // Wide enough that setup on a slow disk still stalls the writer + // well before the key expires. + expiration := time.Now().Add(2 * time.Second) + k, err := s.CreatePreAuthKey(user.TypedID(), true, false, &expiration, nil) + require.NoError(t, err) + + regReq := c.regReq + regReq.Auth = &tailcfg.RegisterResponseAuth{AuthKey: k.Key} + + view, pak := c.lookup(t, k.Key) + require.NoError(t, pak.ValidAt(time.Now())) + + tA := time.Now() + _, _, err = s.SetNodeExpiry(c.node.ID(), &tA) + require.NoError(t, err) + + // Stall the writer so the re-registration's write is applied later. + entered, release := make(chan struct{}), make(chan struct{}) + + releaseOnce := sync.OnceFunc(func() { close(release) }) + defer releaseOnce() + + stalled := make(chan struct{}) + + go func() { + defer close(stalled) + + s.nodeStore.UpdateNode(c.node.ID(), func(*types.Node) { + close(entered) + <-release + }) + }() + + <-entered + require.Greater(t, time.Until(expiration), time.Second, + "setup must stall the writer well before the key expires") + + type result struct { + node types.NodeView + err error + } + + done := make(chan result, 1) + + go func() { + n, err := c.reregister(view, pak, regReq) + done <- result{n, err} + }() + + waitParkedOnWriteQueue(t, "reregisterNodeWithPAK") + require.True(t, time.Now().Before(expiration), + "the re-registration must reach its NodeStore write before the key expires") + + // Only now let the key expire, then let the writer run. + require.Eventually(t, func() bool { + return pak.ValidAt(time.Now()) != nil + }, time.Until(expiration)+time.Second, time.Millisecond) + releaseOnce() + <-stalled + + res := <-done + require.ErrorIs(t, res.err, types.PAKError("authkey expired")) + + ns, dbNode := c.fields(t) + for _, f := range []pakNodeFields{ns, dbNode} { + require.Equal(t, tA.UnixNano(), f.Expiry) + require.Equal(t, c.regReq.NodeKey, f.NodeKey) + require.Equal(t, &k0.ID, f.AuthKeyID) + require.Equal(t, "h0", f.Hostname) + } + }) +} + +// TestPAKReregisterRollbackKeepsPriorWrite: when the database rejects a +// re-registration, the NodeStore rollback must restore the node the writer +// replaced, not the lookup's older view, so an admin expiry landed in between +// survives. +func TestPAKReregisterRollbackKeepsPriorWrite(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("pakuser") + + k0, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + + c := registerForPAKRereg(t, s, k0) + + k, err := s.CreatePreAuthKey(user.TypedID(), false, false, nil, nil) + require.NoError(t, err) + + regReq := c.regReq + regReq.Auth = &tailcfg.RegisterResponseAuth{AuthKey: k.Key} + + view, pak := c.lookup(t, k.Key) + require.False(t, pak.Used, "precondition: the lookup read a fresh single-use key") + + tA := time.Now() + _, _, err = s.SetNodeExpiry(c.node.ID(), &tA) + require.NoError(t, err) + + // Another registration consumes the key after the lookup read its row, so + // the transaction's compare-and-set fails. + err = s.db.Write(func(tx *gorm.DB) error { + return db.UsePreAuthKey(tx, &types.PreAuthKey{ID: pak.ID}) + }) + require.NoError(t, err) + + _, err = c.reregister(view, pak, regReq) + require.ErrorIs(t, err, types.PAKError("authkey already used")) + + ns, _ := c.fields(t) + require.Equal(t, tA.UnixNano(), ns.Expiry, "rollback must keep the admin expiry") + require.Equal(t, &k0.ID, ns.AuthKeyID) + require.Equal(t, "h0", ns.Hostname) +} + +// TestPAKReregisterConcurrentAdminExpire races a same-key restart with a spent +// key against an admin expiry. Either serial order leaves the node expired. +// Only the NodeStore is checked: the order of the two database writes is not +// serialised with it. +func TestPAKReregisterConcurrentAdminExpire(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("pakuser") + + k, err := s.CreatePreAuthKey(user.TypedID(), false, false, nil, nil) + require.NoError(t, err) + + c := registerForPAKRereg(t, s, k) + future := time.Now().Add(time.Hour) + + for i := range 100 { + _, _, err := s.SetNodeExpiry(c.node.ID(), &future) + require.NoError(t, err) + + var ( + wg sync.WaitGroup + expireErr error + ) + + start := make(chan struct{}) + + wg.Go(func() { + <-start + + _, _, _ = s.HandleNodeFromPreAuthKey(c.regReq, c.machineKey) + }) + wg.Go(func() { + <-start + + now := time.Now() + _, _, expireErr = s.SetNodeExpiry(c.node.ID(), &now) + }) + close(start) + wg.Wait() + + require.NoError(t, expireErr) + + n, ok := s.GetNodeByID(c.node.ID()) + require.True(t, ok) + require.True(t, n.IsExpired(), "iteration %d: the spent key revived an expired node", i) + } +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 90d5b0a3f..e57a849d7 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -2612,6 +2612,47 @@ func (s *State) findExistingNodeForPAK( return types.NodeView{}, false, nil } +// pakSkipsValidation reports whether re-registering n with pak may skip key +// validation at now. The machine key proves identity and a pre-auth key only +// authorises the initial join, so a plain restart (container "tailscale up +// --authkey=KEY") re-presents a spent key legitimately. Anything that +// authorises something new must present a valid key: +// - a node key rotation; +// - an expired user-owned node, which is re-authenticating rather than +// waking up. Tagged nodes never expire, so their past expiry is only a +// stale logout stamp and does not count; +// - a tagged key converting a user-owned node to tagged; +// - a tagged key other than the one the node last authed with, which retags +// it. +// +// It is a pure function of the node so the NodeStore writer can decide again +// on the node it actually replaces. +func pakSkipsValidation(n types.NodeView, pak *types.PreAuthKey, nodeKey key.NodePublic, now time.Time) bool { + if !n.Valid() { + return false + } + + isNodeKeyRotation := n.NodeKey() != nodeKey + isExpiredUserOwned := !n.IsTagged() && n.IsExpiredAt(now) + isOwnershipConversion := pak.ConvertsNodeToTagged(n) + isRetag := pak.RetagsNode(n) + ownershipMatches := n.IsTagged() || + (pak.User != nil && n.UserID().Valid() && n.UserID().Get() == pak.User.ID) + + return ownershipMatches && !isNodeKeyRotation && !isExpiredUserOwned && + !isOwnershipConversion && !isRetag +} + +// pakReregisterErr is the authorisation decision for re-registering n with +// pak at now: nil when validation may be skipped, else the key's validity. +func pakReregisterErr(n types.NodeView, pak *types.PreAuthKey, nodeKey key.NodePublic, now time.Time) error { + if pakSkipsValidation(n, pak, nodeKey, now) { + return nil + } + + return pak.ValidAt(now) +} + //nolint:gocyclo // sequential validation/update/create paths with security-sensitive ordering func (s *State) HandleNodeFromPreAuthKey( regReq tailcfg.RegisterRequest, @@ -2648,72 +2689,15 @@ func (s *State) HandleNodeFromPreAuthKey( } } - // Helper to get username for logging (handles nil User for tags-only keys) - pakUsername := func() string { - if pak.User != nil { - return pak.User.Username() - } - - return types.TaggedDevices.Name - } - existingNodeSameUser, existsSameUser, err := s.findExistingNodeForPAK(machineKey, pak) if err != nil { return types.NodeView{}, s.policyChangeSince(genBefore), err } - // For existing nodes, skip validation if: - // 1. MachineKey matches (cryptographic proof of machine identity) - // 2. User/tag ownership matches (from the PAK being used) - // 3. Not a NodeKey rotation (rotation requires fresh validation) - // - // Security: MachineKey is the cryptographic identity. If someone has the MachineKey, - // they control the machine. The PAK was only needed to authorize initial join. - // We don't check which specific PAK was used originally because: - // - Container restarts may use different PAKs (e.g., env var changed) - // - Original PAK may be deleted - // - MachineKey + ownership is sufficient to prove this is the same node - isExistingNodeReregistering := existsSameUser && existingNodeSameUser.Valid() - - // Check if this is a NodeKey rotation (different NodeKey) - isNodeKeyRotation := existsSameUser && existingNodeSameUser.Valid() && - existingNodeSameUser.NodeKey() != regReq.NodeKey - - // An expired node is genuinely re-authenticating, not just waking up, so it - // must present a valid key. Without this a node that re-uses its NodeKey - // after expiry would skip validation and be re-authorised with a spent or - // expired key; the boundary must not depend on the client rotating its key. - // - // Tagged nodes are excluded: they never expire (KB 1068), so an - // IsExpired() tagged node only reflects a stale logout stamp left by an - // older headscale (#3371). Forcing it down the re-validation path burns its - // fresh key and blocks re-auth forever; treat it as a plain re-registration - // and clear the stale expiry in the update below. - isExpired := existsSameUser && existingNodeSameUser.Valid() && - !existingNodeSameUser.IsTagged() && - existingNodeSameUser.IsExpired() - - // A tagged key presented for a currently user-owned node converts that node - // to tagged. That is an ownership change, not a plain refresh, so it must - // present a valid key rather than ride the skip-validation fast-path. - isOwnershipConversion := existsSameUser && existingNodeSameUser.Valid() && - pak.IsTagged() && !existingNodeSameUser.IsTagged() - - // A tagged key that differs from the one the node last authed with retags - // the node (see the in-place update below). Applying a key's tags is an - // authorisation decision, so the key must be validated rather than ride the - // skip-validation fast-path; otherwise a spent, revoked or expired tagged - // key could still retag a node that reuses its node key. Like isExpired, - // this boundary must not depend on the client rotating its key. - isRetag := existsSameUser && existingNodeSameUser.Valid() && - pak.IsTagged() && existingNodeSameUser.IsTagged() && - (!existingNodeSameUser.AuthKeyID().Valid() || existingNodeSameUser.AuthKeyID().Get() != pak.ID) - - if isExistingNodeReregistering && !isNodeKeyRotation && !isExpired && !isOwnershipConversion && !isRetag { - // Existing, still-valid node re-registering with same NodeKey: skip - // validation. Pre-auth keys are only needed for initial authentication. - // Critical for containers that run "tailscale up --authkey=KEY" on every - // restart. + // This decision is only an early reject that keeps a key error ahead of + // ErrNodeKeyInUse. An in-place re-registration decides again in the + // NodeStore writer, on the node it replaces; see reregisterNodeWithPAK. + if existsSameUser && pakSkipsValidation(existingNodeSameUser, pak, regReq.NodeKey, time.Now()) { log.Debug(). Caller(). Uint64(zf.NodeID, existingNodeSameUser.ID().Uint64()). @@ -2725,10 +2709,8 @@ func (s *State) HandleNodeFromPreAuthKey( Bool(zf.AuthKeyUsed, pak.Used). Bool(zf.AuthKeyExpired, pak.Expiration != nil && pak.Expiration.Before(time.Now())). Bool(zf.AuthKeyReusable, pak.Reusable). - Bool(zf.NodeKeyRotation, isNodeKeyRotation). - Msg("Existing node re-registering with same NodeKey and auth key, skipping validation") + Msg("Existing node re-registering with same NodeKey; lookup permits skipping key validation, writer re-decides") } else { - // New node or NodeKey rotation: require valid auth key. err = pak.Validate() if err != nil { return types.NodeView{}, s.policyChangeSince(genBefore), err @@ -2752,7 +2734,7 @@ func (s *State) HandleNodeFromPreAuthKey( Str(zf.NodeName, hostname). Str(zf.MachineKey, machineKey.ShortString()). Str(zf.NodeKey, regReq.NodeKey.ShortString()). - Str(zf.UserName, pakUsername()). + Str(zf.UserName, pak.Username()). Msg("Registering node with pre-auth key") var finalNode types.NodeView @@ -2762,184 +2744,10 @@ func (s *State) HandleNodeFromPreAuthKey( // a tagged node (findExistingNodeForPAK matches it under UserID 0); for a // user-owned key it is true when the same user already has the node. if existsSameUser && existingNodeSameUser.Valid() { - log.Trace(). - Caller(). - Str(zf.NodeName, existingNodeSameUser.Hostname()). - Uint64(zf.NodeID, existingNodeSameUser.ID().Uint64()). - Str(zf.MachineKey, machineKey.ShortString()). - Str(zf.NodeKey, existingNodeSameUser.NodeKey().ShortString()). - Str(zf.UserName, pakUsername()). - Msg("Node re-registering with existing machine key and user, updating in place") - - // Re-registration rotates the NodeKey to the client-supplied value. - // Enforce the same 1:1 NodeKey<->MachineKey binding the auth path - // (applyAuthNodeUpdate) and poll-time validation enforce: a NodeKey - // already bound to a different machine must not be claimed here, or a - // re-registering node could rotate its key to a victim's and poison the - // NodeStore NodeKey index, denying the victim service. - if existing, ok := s.nodeStore.GetNodeByNodeKey(regReq.NodeKey); ok && - existing.MachineKey() != machineKey { - return types.NodeView{}, s.policyChangeSince(genBefore), ErrNodeKeyInUse - } - - // Snapshot the pre-update node so the NodeStore can be rolled back if - // the database write below fails. The view points at the immutable - // pre-update snapshot (UpdateNode swaps in a new one), so this stays - // valid after the mutation. - priorNode := existingNodeSameUser.AsStruct() - - // Update existing node - NodeStore first, then database - updatedNodeView, ok := s.nodeStore.UpdateNode(existingNodeSameUser.ID(), func(node *types.Node) { - node.NodeKey = regReq.NodeKey - node.Hostname = hostname - - // TODO(kradalby): We should ensure we use the same hostinfo and node merge semantics - // when a node re-registers as we do when it sends a map request (UpdateNodeFromMapRequest). - - // Preserve NetInfo from existing node when re-registering - node.Hostinfo = validHostinfo - node.Hostinfo.NetInfo = preserveNetInfo(existingNodeSameUser, existingNodeSameUser.ID(), validHostinfo) - - node.RegisterMethod = util.RegisterMethodAuthKey - - // Tags from a PreAuthKey are applied on initial registration and - // re-applied whenever a *different* key is presented on - // re-registration: re-keying is Tailscale's documented way to change - // an auth-key device's tags (KB 1068 - "generate a new auth key with - // the new set of tags ... doing so replaces the device's existing - // tags"). Presenting the SAME key again (container restart, - // #2830/#3312) preserves the node's current tags and any admin - // override. A tagged key presented for a user-owned node also converts - // it, dropping user ownership. - // - // node.AuthKeyID still holds the prior key's ID here (it is reassigned - // below), and SetNodeTags leaves AuthKeyID intact, so an admin retag - // cannot masquerade as a new key. - keyChanged := node.AuthKeyID == nil || *node.AuthKeyID != pak.ID - if pak.IsTagged() && (!node.IsTagged() || keyChanged) { - wasUserOwned := !node.IsTagged() - - node.Tags = pak.Tags - node.UserID = nil - node.User = nil - - // Converting a user-owned node to tagged drops the user's key - // expiry (tagged nodes never expire). But retagging an - // already-tagged node must preserve a deliberate FUTURE expiry - // set via `headscale nodes expire` - that is a node property, not - // tied to the auth key - and only clear a stale PAST expiry. This - // keeps the retag path symmetric with the same-key relogin path - // (#3371) rather than silently overriding an admin decision. - if wasUserOwned || node.IsExpired() { - node.Expiry = nil - } - } - - node.AuthKey = pak.AsCredential() - node.AuthKeyID = &pak.ID - // If this registration will consume a single-use key (the tx below - // calls UsePreAuthKey under the same condition), reflect that in the - // cached AuthKey so the NodeStore copy matches the database. - if !pak.Reusable && !pak.Used { - node.AuthKey.Used = true - } - // Preserve online state during re-registration so a live node does - // not appear offline before the client restarts its map stream. - node.LastSeen = new(time.Now()) - - // Tagged nodes keep their existing expiry (disabled). - // User-owned nodes update expiry from the client request, - // falling back to the configured default if the client - // did not request a specific expiry. If neither is set, - // clear the expiry so the database holds NULL instead of - // a pointer to zero time. - if !node.IsTagged() { - if !regReq.Expiry.IsZero() { - node.Expiry = ®Req.Expiry - } else if s.cfg.Node.Expiry > 0 { - exp := time.Now().Add(s.cfg.Node.Expiry) - node.Expiry = &exp - } else { - node.Expiry = nil - } - } else if node.IsExpired() { - // #3371: a tagged node must never carry key expiry. Clear a - // stale PAST expiry left by a logout (older headscale) so - // re-auth is not permanently blocked. A deliberate future - // expiry (headscale nodes expire) has IsExpired() == false and - // is left untouched. - node.Expiry = nil - } - }) - - if !ok { - return types.NodeView{}, s.policyChangeSince(genBefore), fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, existingNodeSameUser.ID()) - } - - _, err = hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.Node, error) { - // Explicitly select all node columns so GORM includes nil/zero-value fields - // (see nodeUpdateColumns comment). AuthKeyID is normally excluded to - // avoid persisting a deleted key's stale reference on MapRequest - // (#2862), but re-registration presents a freshly-validated key, so - // its ID must be persisted here — otherwise a restart reloads the old - // key and any key-scoped property (e.g. Ephemeral) silently reverts. - reregColumns := append(slices.Clone(nodeUpdateColumns), "AuthKeyID") - - err := tx.Select(reregColumns).Updates(updatedNodeView.AsStruct()).Error - if err != nil { - return nil, fmt.Errorf("saving node: %w", err) - } - - // Only mark the key used on the *first* registration. On - // re-registration the same key is already used and the - // atomic compare-and-set in [hsdb.UsePreAuthKey] would otherwise - // reject it as "authkey already used". This is the path - // behind issue #2830 where containers restart with the - // same one-shot key. - if !pak.Reusable && !pak.Used { - err = hsdb.UsePreAuthKey(tx, pak) - if err != nil { - return nil, fmt.Errorf("using pre auth key: %w", err) - } - } - - return nil, nil //nolint:nilnil // intentional: transaction success - }) + finalNode, err = s.reregisterNodeWithPAK(existingNodeSameUser, pak, regReq, machineKey, hostname, validHostinfo) if err != nil { - // The NodeStore was updated before the database write. Roll it back - // so it does not advertise a registration the database rejected - // (e.g. a node key that a restart would not reload). Restore only - // the fields the update above wrote: sessions, endpoints and health - // may have moved since priorNode was taken. LastSeen stays, as the - // node did contact us. - if priorNode != nil { - s.nodeStore.UpdateNode(priorNode.ID, func(n *types.Node) { - n.NodeKey = priorNode.NodeKey - n.Hostname = priorNode.Hostname - n.Hostinfo = priorNode.Hostinfo - n.RegisterMethod = priorNode.RegisterMethod - n.Tags = priorNode.Tags - n.UserID = priorNode.UserID - n.User = priorNode.User - n.Expiry = priorNode.Expiry - n.AuthKey = priorNode.AuthKey - n.AuthKeyID = priorNode.AuthKeyID - }) - } - - return types.NodeView{}, s.policyChangeSince(genBefore), fmt.Errorf("writing node to database: %w", err) + return types.NodeView{}, s.policyChangeSince(genBefore), err } - - log.Trace(). - Caller(). - Str(zf.NodeName, updatedNodeView.Hostname()). - Uint64(zf.NodeID, updatedNodeView.ID().Uint64()). - Str(zf.MachineKey, machineKey.ShortString()). - Str(zf.NodeKey, updatedNodeView.NodeKey().ShortString()). - Str(zf.UserName, pakUsername()). - Msg("Node re-authorized") - - finalNode = updatedNodeView } else { // Node does not exist for this user with this machine key. // For a user-owned key, check whether the machine key is already held @@ -2976,7 +2784,7 @@ func (s *State) HandleNodeFromPreAuthKey( Uint64(zf.ExistingNodeID, differentUserNode.ID().Uint64()). Str(zf.MachineKey, machineKey.ShortString()). Str(zf.OldUser, oldUserName). - Str(zf.NewUser, pakUsername()). + Str(zf.NewUser, pak.Username()). Msg("Creating new node for different user (same machine key exists for another user)") } @@ -3036,6 +2844,219 @@ func (s *State) HandleNodeFromPreAuthKey( return finalNode, reauthChange(finalNode, existsSameUser, policyChanged), nil } +// reregisterNodeWithPAK updates an existing node in place for a pre-auth key +// re-registration: NodeStore first, then the database. +// +// The caller's lookup view goes stale before the NodeStore writer runs: an +// admin or logout expiry, a node key reconcile, a retag or a deletion can land +// in between, and the key or the node can expire. So the writer decides +// whether the key must be valid on the node it replaces, at its own clock, and +// leaves the node untouched when it is not. existingNodeSameUser is used only +// for its ID and for logging. +func (s *State) reregisterNodeWithPAK( + existingNodeSameUser types.NodeView, + pak *types.PreAuthKey, + regReq tailcfg.RegisterRequest, + machineKey key.MachinePublic, + hostname string, + validHostinfo *tailcfg.Hostinfo, +) (types.NodeView, error) { + log.Trace(). + Caller(). + Str(zf.NodeName, existingNodeSameUser.Hostname()). + Uint64(zf.NodeID, existingNodeSameUser.ID().Uint64()). + Str(zf.MachineKey, machineKey.ShortString()). + Str(zf.NodeKey, existingNodeSameUser.NodeKey().ShortString()). + Str(zf.UserName, pak.Username()). + Msg("Node re-registering with existing machine key and user, updating in place") + + // Re-registration rotates the NodeKey to the client-supplied value. + // Enforce the same 1:1 NodeKey<->MachineKey binding the auth path + // (applyAuthNodeUpdate) and poll-time validation enforce: a NodeKey + // already bound to a different machine must not be claimed here, or a + // re-registering node could rotate its key to a victim's and poison the + // NodeStore NodeKey index, denying the victim service. + if existing, ok := s.nodeStore.GetNodeByNodeKey(regReq.NodeKey); ok && + existing.MachineKey() != machineKey { + return types.NodeView{}, ErrNodeKeyInUse + } + + // prior is the node the writer replaced: the NetInfo source and the + // rollback target if the database write below fails. + var ( + prior *types.Node + authErr error + ) + + // Update existing node - NodeStore first, then database + updatedNodeView, ok := s.nodeStore.UpdateNode(existingNodeSameUser.ID(), func(node *types.Node) { + // One clock read, so the whole mutation is decided at one instant. + now := time.Now() + + prior = node.Clone() + + authErr = pakReregisterErr(node.View(), pak, regReq.NodeKey, now) + if authErr != nil { + return + } + + node.NodeKey = regReq.NodeKey + node.Hostname = hostname + + // TODO(kradalby): We should ensure we use the same hostinfo and node merge semantics + // when a node re-registers as we do when it sends a map request (UpdateNodeFromMapRequest). + + // Preserve NetInfo from existing node when re-registering + node.Hostinfo = validHostinfo + node.Hostinfo.NetInfo = preserveNetInfo(prior.View(), prior.ID, validHostinfo) + + node.RegisterMethod = util.RegisterMethodAuthKey + + // Tags from a PreAuthKey are applied on initial registration and + // re-applied whenever a *different* key is presented on + // re-registration: re-keying is Tailscale's documented way to change + // an auth-key device's tags, replacing its existing ones. Presenting + // the SAME key again (a container restart) preserves the node's + // current tags and any admin override. A tagged key presented for a + // user-owned node also converts it, dropping user ownership. + // See https://tailscale.com/kb/1068/tags#apply-a-tag-to-a-device-with-the-cli. + // + // node.AuthKeyID still holds the prior key's ID here (it is reassigned + // below), and SetNodeTags leaves AuthKeyID intact, so an admin retag + // cannot masquerade as a new key. + if pak.ConvertsNodeToTagged(node.View()) || pak.RetagsNode(node.View()) { + wasUserOwned := !node.IsTagged() + + node.Tags = pak.Tags + node.UserID = nil + node.User = nil + + // Converting a user-owned node to tagged drops the user's key + // expiry (tagged nodes never expire). But retagging an + // already-tagged node must preserve a deliberate FUTURE expiry + // set via `headscale nodes expire` - that is a node property, not + // tied to the auth key - and only clear a stale PAST expiry. This + // keeps the retag path symmetric with the same-key relogin path + // below rather than silently overriding an admin decision. + if wasUserOwned || node.IsExpiredAt(now) { + node.Expiry = nil + } + } + + node.AuthKey = pak.AsCredential() + node.AuthKeyID = &pak.ID + // If this registration will consume a single-use key (the tx below + // calls UsePreAuthKey under the same condition), reflect that in the + // cached AuthKey so the NodeStore copy matches the database. + if !pak.Reusable && !pak.Used { + node.AuthKey.Used = true + } + // Preserve online state during re-registration so a live node does + // not appear offline before the client restarts its map stream. + node.LastSeen = new(now) + + // Tagged nodes keep their existing expiry (disabled). + // User-owned nodes update expiry from the client request, + // falling back to the configured default if the client + // did not request a specific expiry. If neither is set, + // clear the expiry so the database holds NULL instead of + // a pointer to zero time. + if !node.IsTagged() { + if !regReq.Expiry.IsZero() { + node.Expiry = ®Req.Expiry + } else if s.cfg.Node.Expiry > 0 { + exp := now.Add(s.cfg.Node.Expiry) + node.Expiry = &exp + } else { + node.Expiry = nil + } + } else if node.IsExpiredAt(now) { + // A tagged node must never carry key expiry. Clear a stale + // PAST logout stamp so re-auth is not permanently blocked. A + // deliberate future expiry (headscale nodes expire) is not yet + // expired and is left untouched. + node.Expiry = nil + } + }) + + // A nil prior means the writer never saw the node (deleted, or the store + // stopped), so nothing was decided and nothing may be persisted. + if !ok || prior == nil { + return types.NodeView{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, existingNodeSameUser.ID()) + } + + if authErr != nil { + log.Debug(). + Caller(). + Uint64(zf.NodeID, existingNodeSameUser.ID().Uint64()). + Str(zf.MachineKey, machineKey.ShortString()). + Uint64(zf.AuthKeyID, pak.ID). + Err(authErr). + Msg("Re-registration needs a valid auth key when it applies, rejecting") + + return types.NodeView{}, authErr + } + + _, err := hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.Node, error) { + // Explicitly select all node columns so GORM includes nil/zero-value fields + // (see nodeUpdateColumns comment). AuthKeyID is excluded on MapRequest + // so a deleted key's stale reference is not persisted, but + // re-registration presents a key just loaded from the database, so + // its ID must be persisted here — otherwise a restart reloads the old + // key and any key-scoped property (e.g. Ephemeral) silently reverts. + reregColumns := append(slices.Clone(nodeUpdateColumns), "AuthKeyID") + + err := tx.Select(reregColumns).Updates(updatedNodeView.AsStruct()).Error + if err != nil { + return nil, fmt.Errorf("saving node: %w", err) + } + + // Only mark the key used on the *first* registration. On + // re-registration the same key is already used and the + // atomic compare-and-set in [hsdb.UsePreAuthKey] would otherwise + // reject it as "authkey already used", locking out containers + // that restart with the same one-shot key. + if !pak.Reusable && !pak.Used { + err = hsdb.UsePreAuthKey(tx, pak) + if err != nil { + return nil, fmt.Errorf("using pre auth key: %w", err) + } + } + + return nil, nil //nolint:nilnil // intentional: transaction success + }) + if err != nil { + // Restore the registration fields from the node the writer replaced. + // Sessions, endpoints and health may have moved since that snapshot; + // preserve them. LastSeen stays because the node did contact us. + s.nodeStore.UpdateNode(prior.ID, func(n *types.Node) { + n.NodeKey = prior.NodeKey + n.Hostname = prior.Hostname + n.Hostinfo = prior.Hostinfo + n.RegisterMethod = prior.RegisterMethod + n.Tags = prior.Tags + n.UserID = prior.UserID + n.User = prior.User + n.Expiry = prior.Expiry + n.AuthKey = prior.AuthKey + n.AuthKeyID = prior.AuthKeyID + }) + + return types.NodeView{}, fmt.Errorf("writing node to database: %w", err) + } + + log.Trace(). + Caller(). + Str(zf.NodeName, updatedNodeView.Hostname()). + Uint64(zf.NodeID, updatedNodeView.ID().Uint64()). + Str(zf.MachineKey, machineKey.ShortString()). + Str(zf.NodeKey, updatedNodeView.NodeKey().ShortString()). + Str(zf.UserName, pak.Username()). + Msg("Node re-authorized") + + return updatedNodeView, nil +} + // reauthChange returns the [change.Change] to broadcast after an authentication // that updated or created a node. // diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index 3059b8a59..257b9ca81 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -219,6 +219,11 @@ func (ns Nodes) ViewSlice() views.Slice[NodeView] { // IsExpired returns whether the node registration has expired. func (node *Node) IsExpired() bool { + return node.IsExpiredAt(time.Now()) +} + +// IsExpiredAt reports whether the node registration has expired at now. +func (node *Node) IsExpiredAt(now time.Time) bool { // If Expiry is not set, the client has not indicated that // it wants an expiry time, it is therefore considered // to mean "not expired" @@ -226,7 +231,7 @@ func (node *Node) IsExpired() bool { return false } - return time.Since(*node.Expiry) > 0 + return now.After(*node.Expiry) } // Online reports the node's last known connectivity. Unknown counts as @@ -933,6 +938,15 @@ func (nv NodeView) IsExpired() bool { return nv.ж.IsExpired() } +// IsExpiredAt reports whether the node registration has expired at now. +func (nv NodeView) IsExpiredAt(now time.Time) bool { + if !nv.Valid() { + return true + } + + return nv.ж.IsExpiredAt(now) +} + // Online reports the node's last known connectivity. func (nv NodeView) Online() bool { if !nv.Valid() { diff --git a/hscontrol/types/node_test.go b/hscontrol/types/node_test.go index 07af84278..9a4b537db 100644 --- a/hscontrol/types/node_test.go +++ b/hscontrol/types/node_test.go @@ -1043,3 +1043,33 @@ func TestHasPolicyChangeFields(t *testing.T) { }) } } + +func TestNodeIsExpiredAt(t *testing.T) { + now := time.Now() + zero := time.Time{} + justBefore := now.Add(-time.Nanosecond) + + tests := []struct { + name string + expiry *time.Time + want bool + }{ + {name: "nil expiry", expiry: nil, want: false}, + {name: "zero expiry", expiry: &zero, want: false}, + {name: "expiry equals now", expiry: &now, want: false}, + {name: "now one nanosecond after expiry", expiry: &justBefore, want: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + n := &Node{Expiry: tt.expiry} + require.Equal(t, tt.want, n.IsExpiredAt(now)) + require.Equal(t, tt.want, n.View().IsExpiredAt(now)) + }) + } + + t.Run("invalid view", func(t *testing.T) { + require.True(t, NodeView{}.IsExpiredAt(now), + "an invalid view counts as expired, like IsExpired") + }) +} diff --git a/hscontrol/types/preauth_key.go b/hscontrol/types/preauth_key.go index 0f76efea3..6adad5dd4 100644 --- a/hscontrol/types/preauth_key.go +++ b/hscontrol/types/preauth_key.go @@ -98,11 +98,21 @@ func (pak *PreAuthKey) Validate() error { EmbedObject(pak). Msg("PreAuthKey.Validate: checking key") + return pak.ValidAt(time.Now()) +} + +// ValidAt is [PreAuthKey.Validate] at a caller-chosen instant, without +// logging, so a NodeStore writer can decide at its own clock. +func (pak *PreAuthKey) ValidAt(now time.Time) error { + if pak == nil { + return PAKError("invalid authkey") + } + if pak.Revoked != nil { return PAKError("authkey revoked") } - if pak.Expiration != nil && pak.Expiration.Before(time.Now()) { + if pak.Expiration != nil && pak.Expiration.Before(now) { return PAKError("authkey expired") } @@ -124,6 +134,29 @@ func (pak *PreAuthKey) IsTagged() bool { return len(pak.Tags) > 0 } +// Username returns the associated user's name, or TaggedDevices for a key +// without a user. For tagged keys the associated user is the creator. +func (pak *PreAuthKey) Username() string { + if pak.User != nil { + return pak.User.Username() + } + + return TaggedDevices.Name +} + +// ConvertsNodeToTagged reports whether this key changes a user-owned node +// to tag ownership. +func (pak *PreAuthKey) ConvertsNodeToTagged(node NodeView) bool { + return node.Valid() && pak.IsTagged() && !node.IsTagged() +} + +// RetagsNode reports whether this key replaces an already-tagged node's +// tags. Reusing the last key preserves subsequent admin tag changes. +func (pak *PreAuthKey) RetagsNode(node NodeView) bool { + return node.IsTagged() && pak.IsTagged() && + (!node.AuthKeyID().Valid() || node.AuthKeyID().Get() != pak.ID) +} + // maskedPrefix returns the key prefix in masked format for safe logging. // SECURITY: Never log the full key or hash, only the masked prefix. func (pak *PreAuthKey) maskedPrefix() string { diff --git a/hscontrol/types/preauth_key_test.go b/hscontrol/types/preauth_key_test.go index 1b280149b..c7e33af8b 100644 --- a/hscontrol/types/preauth_key_test.go +++ b/hscontrol/types/preauth_key_test.go @@ -6,6 +6,7 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" ) func TestCanUsePreAuthKey(t *testing.T) { @@ -14,11 +15,32 @@ func TestCanUsePreAuthKey(t *testing.T) { future := now.Add(time.Hour) tests := []struct { - name string - pak *PreAuthKey + name string + pak *PreAuthKey + // at, when set, checks ValidAt(at) instead of Validate. + at time.Time wantErr bool err PAKError }{ + { + name: "valid at the instant of expiration", + pak: &PreAuthKey{ + Reusable: true, + Expiration: &now, + }, + at: now, + wantErr: false, + }, + { + name: "expired one nanosecond after expiration", + pak: &PreAuthKey{ + Reusable: true, + Expiration: &now, + }, + at: now.Add(time.Nanosecond), + wantErr: true, + err: PAKError("authkey expired"), + }, { name: "valid reusable key", pak: &PreAuthKey{ @@ -105,7 +127,16 @@ func TestCanUsePreAuthKey(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := tt.pak.Validate() + var err error + if tt.at.IsZero() { + err = tt.pak.Validate() + if diff := cmp.Diff(err, tt.pak.ValidAt(time.Now())); diff != "" { + t.Errorf("Validate and ValidAt(now) disagree (-Validate +ValidAt):\n%s", diff) + } + } else { + err = tt.pak.ValidAt(tt.at) + } + if tt.wantErr { if err == nil { t.Errorf("expected error but got none") @@ -127,3 +158,44 @@ func TestCanUsePreAuthKey(t *testing.T) { }) } } + +func TestPreAuthKeyUsername(t *testing.T) { + user := &User{Name: "creator", Email: "creator@example.com"} + for _, pak := range []*PreAuthKey{ + {User: user}, + {User: user, Tags: []string{"tag:server"}}, + } { + require.Equal(t, user.Username(), pak.Username()) + } + + require.Equal(t, TaggedDevices.Name, (&PreAuthKey{Tags: []string{"tag:server"}}).Username()) +} + +func TestPreAuthKeyTagChanges(t *testing.T) { + keyID := uint64(1) + userID := uint(1) + pak := &PreAuthKey{ID: keyID, Tags: []string{"tag:original"}, User: &User{ID: userID}} + + userNode := (&Node{ID: 1, UserID: &userID}).View() + require.True(t, pak.ConvertsNodeToTagged(userNode)) + require.False(t, pak.RetagsNode(userNode)) + + // An admin changed the tags, but the key identity is unchanged. The + // creator's UserID does not make this tagged node user-owned. + taggedNode := (&Node{ + ID: 1, Tags: []string{"tag:admin"}, AuthKeyID: &keyID, UserID: &userID, + }).View() + require.False(t, pak.ConvertsNodeToTagged(taggedNode)) + require.False(t, pak.RetagsNode(taggedNode)) + + otherKey := &PreAuthKey{ID: 2, Tags: []string{"tag:replacement"}} + require.True(t, otherKey.RetagsNode(taggedNode)) + require.False(t, otherKey.ConvertsNodeToTagged(taggedNode)) + require.True(t, pak.RetagsNode((&Node{ID: 1, Tags: []string{"tag:admin"}}).View())) + + userKey := &PreAuthKey{User: &User{ID: userID}} + require.False(t, userKey.ConvertsNodeToTagged(userNode)) + require.False(t, userKey.RetagsNode(taggedNode)) + require.False(t, pak.ConvertsNodeToTagged(NodeView{})) + require.False(t, pak.RetagsNode(NodeView{})) +} diff --git a/hscontrol/util/zlog/zf/fields.go b/hscontrol/util/zlog/zf/fields.go index 45edd1da0..156d156c7 100644 --- a/hscontrol/util/zlog/zf/fields.go +++ b/hscontrol/util/zlog/zf/fields.go @@ -78,7 +78,6 @@ const ( AuthKeyUsed = "authkey.used" AuthKeyExpired = "authkey.expired" AuthKeyReusable = "authkey.reusable" - NodeKeyRotation = "nodekey.rotation" ) // APIKey fields.