From 5a4d366ce1636d818afb70a3950634cc0c7fa035 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 28 Oct 2026 13:36:14 +0000 Subject: [PATCH] auth, state: keep only hostname, OS and tags in pending registrations The rest arrives with the first MapRequest. A relogin keeps the node's current Hostinfo, endpoints and disco key; an oversized merge is a 400. Fixes ANT-2026-RQS7JAPP --- CHANGELOG.md | 2 + hscontrol/api/v1/errors.go | 1 + hscontrol/auth.go | 22 ++- hscontrol/auth_test.go | 77 ++++++++--- hscontrol/noise_test.go | 36 +++++ hscontrol/oidc.go | 5 + hscontrol/oidc_test.go | 43 ++++++ hscontrol/servertest/lifecycle_test.go | 5 +- hscontrol/state/persist_test.go | 180 ++++++++++++++++++++++++- hscontrol/state/state.go | 76 +++++------ hscontrol/types/registration.go | 19 +-- 11 files changed, 375 insertions(+), 91 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e70c4538b..d2a4372d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -160,6 +160,8 @@ the ACME DNS-01 challenge through a built-in DNS provider: `acmedns`, - Refuse a key expiry outside the years 0 to 9999 in UTC on registration, on the OIDC token-expiry path and in `headscale nodes expire` / `POST /api/v1/node/{nodeId}/expire` (400); the latest accepted expiry no longer fails node listings on PostgreSQL in a zone east of UTC - Keep the first 256 endpoints and 4096 announced routes from client reports; oversized hostnames, requested tags or other metadata return a registration error or HTTP 413 on map updates. Zoned IPv6 endpoints are refused - At startup, trim stored hostnames, endpoints and routes to the client-report limits and remove zoned endpoints. Hostinfo exceeding its size or tag limit is dropped until the next valid report +- Pending interactive registrations keep only the hostname, OS and requested tags; other device details arrive with the first map request. Reauthentication preserves routes, services and endpoints +- Web reauthentication preserves the discovery key so peers can still reach the device ## 0.29.7 (202x-xx-xx) diff --git a/hscontrol/api/v1/errors.go b/hscontrol/api/v1/errors.go index 87ef93de2..71ccfe035 100644 --- a/hscontrol/api/v1/errors.go +++ b/hscontrol/api/v1/errors.go @@ -35,6 +35,7 @@ func mapError(msg string, err error) error { errors.Is(err, state.ErrNodeHasNeitherUserNorTags), errors.Is(err, state.ErrRequestedTagsInvalidOrNotPermitted), errors.Is(err, types.ErrExpiryOutOfRange), + errors.Is(err, types.ErrHostinfoTooLarge), errors.Is(err, db.ErrUserStillHasNodes), errors.Is(err, db.ErrCannotChangeOIDCUser), errors.Is(err, db.ErrPreAuthKeyNotTaggedOrOwned), diff --git a/hscontrol/auth.go b/hscontrol/auth.go index 99b8720dd..837bb4f1e 100644 --- a/hscontrol/auth.go +++ b/hscontrol/auth.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/url" + "slices" "strings" "time" @@ -388,25 +389,32 @@ func (h *Headscale) reqToNewRegisterResponse( }, nil } -// registrationDataFromRequest builds the [types.RegistrationData] payload stored -// in the auth cache for a pending registration. The original [tailcfg.Hostinfo] is -// retained so that consumers (auth callback, observability) see the -// fields the client originally announced; the bounded-LRU cap on the -// cache is what bounds the unauthenticated cache-fill DoS surface. +// registrationDataFromRequest keeps only auth and confirmation metadata; +// operational metadata arrives with the node's first MapRequest. func registrationDataFromRequest( req tailcfg.RegisterRequest, machineKey key.MachinePublic, ) *types.RegistrationData { - var hostname string + var ( + hostname string + hostinfo *tailcfg.Hostinfo + ) + if req.Hostinfo != nil { hostname = req.Hostinfo.Hostname + + hostinfo = &tailcfg.Hostinfo{ + Hostname: hostname, + OS: req.Hostinfo.OS, + RequestTags: slices.Clone(req.Hostinfo.RequestTags), + } } regData := &types.RegistrationData{ MachineKey: machineKey, NodeKey: req.NodeKey, Hostname: hostname, - Hostinfo: req.Hostinfo, + Hostinfo: hostinfo, } if !req.Expiry.IsZero() { diff --git a/hscontrol/auth_test.go b/hscontrol/auth_test.go index 053471b92..f21b9c66a 100644 --- a/hscontrol/auth_test.go +++ b/hscontrol/auth_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/http" + "net/netip" "net/url" "strings" "testing" @@ -1638,15 +1639,8 @@ func TestAuthenticationFlows(t *testing.T) { }, // === AUTH PROVIDER EDGE CASES === - // TEST: Interactive workflow preserves custom hostinfo - // WHAT: Tests that custom hostinfo fields are preserved through interactive flow - // INPUT: Interactive registration with detailed hostinfo (OS, version, model) - // EXPECTED: Node registers with all hostinfo fields preserved - // WHY: Ensures interactive flow doesn't lose custom hostinfo data - // NOTE: RequestTags are NOT tested here because tag authorization via - // advertise-tags requires the user to have existing nodes (for IP-based - // ownership verification). New users registering their first node cannot - // claim tags via RequestTags - they must use a tagged PreAuthKey instead. + // Interactive registration retains auth metadata. The first map request + // supplies the remaining Hostinfo fields. { name: "interactive_workflow_with_custom_hostinfo", setupFunc: func(t *testing.T, app *Headscale) (string, error) { //nolint:thelper @@ -1673,15 +1667,29 @@ func TestAuthenticationFlows(t *testing.T) { validateCompleteResponse: true, expectedAuthURLPattern: "/register/", validate: func(t *testing.T, resp *tailcfg.RegisterResponse, app *Headscale) { //nolint:thelper - // Verify custom hostinfo was preserved through interactive workflow + // Confirm the minimal cached report, then restore the operational fields. node, found := app.state.GetNodeByNodeKey(nodeKey1.Public()) assert.True(t, found, "node should be found after interactive registration") if found { assert.Equal(t, "custom-interactive-node", node.Hostname()) assert.Equal(t, "linux", node.Hostinfo().OS()) - assert.Equal(t, "20.04", node.Hostinfo().OSVersion()) - assert.Equal(t, "server", node.Hostinfo().DeviceModel()) + assert.Empty(t, node.Hostinfo().OSVersion()) + assert.Empty(t, node.Hostinfo().DeviceModel()) + + _, err := app.state.UpdateNodeFromMapRequest(node.ID(), tailcfg.MapRequest{ + NodeKey: node.NodeKey(), DiscoKey: node.DiscoKey(), + Hostinfo: &tailcfg.Hostinfo{ + Hostname: node.Hostname(), OS: "linux", + OSVersion: "20.04", DeviceModel: "server", + }, + }) + require.NoError(t, err) + + updated, ok := app.state.GetNodeByID(node.ID()) + require.True(t, ok) + assert.Equal(t, "20.04", updated.Hostinfo().OSVersion()) + assert.Equal(t, "server", updated.Hostinfo().DeviceModel()) } }, }, @@ -4340,14 +4348,14 @@ func TestHandleNodeFromAuthPath_ReloginWholeNode(t *testing.T) { tests := []struct { name string expired bool - services []tailcfg.Service + hostname string wantPatch bool }{ {name: "keys only", wantPatch: true}, {name: "expired", expired: true}, { - name: "peer-visible hostinfo", - services: []tailcfg.Service{{Proto: tailcfg.PeerAPI4, Port: 4242}}, + name: "peer-visible hostname", + hostname: "renamed-node", }, } @@ -4365,15 +4373,19 @@ func TestHandleNodeFromAuthPath_ReloginWholeNode(t *testing.T) { app.state.PutNodeInStoreForTest(*node) + hostname := node.Hostname + if tt.hostname != "" { + hostname = tt.hostname + } + newNodeKey := key.NewNode() authID := types.MustAuthID() app.state.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{ MachineKey: node.MachineKey, NodeKey: newNodeKey.Public(), - Hostname: node.Hostname, + Hostname: hostname, Hostinfo: &tailcfg.Hostinfo{ - Hostname: node.Hostname, - Services: tt.services, + Hostname: hostname, }, })) @@ -4598,3 +4610,32 @@ func TestRegisterRejectsUnencodableExpiry(t *testing.T) { require.NoError(t, err) assert.True(t, latest.Equal(*stored.Expiry)) } + +// TestPendingRegistrationKeepsAuthMetadata pins that operational reports do +// not stay in the auth cache before the first MapRequest. +func TestPendingRegistrationKeepsAuthMetadata(t *testing.T) { + app := createTestApp(t) + hostinfo := &tailcfg.Hostinfo{ + Hostname: "pending-router", OS: "linux", OSVersion: "6.12.0", + RequestTags: []string{"tag:router"}, + RoutableIPs: []netip.Prefix{netip.MustParsePrefix("10.0.0.0/24")}, + Services: []tailcfg.Service{{Proto: tailcfg.PeerAPI4, Port: 41641}}, + NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, + SSH_HostKeys: []string{"ssh-ed25519 key"}, + } + resp, err := app.handleRegister(context.Background(), tailcfg.RegisterRequest{ + NodeKey: key.NewNode().Public(), Hostinfo: hostinfo, + }, key.NewMachine().Public()) + require.NoError(t, err) + authID, err := extractRegistrationIDFromAuthURL(resp.AuthURL) + require.NoError(t, err) + + entry, ok := app.state.GetAuthCacheEntry(authID) + require.True(t, ok) + assert.Equal(t, &tailcfg.Hostinfo{ + Hostname: "pending-router", OS: "linux", RequestTags: []string{"tag:router"}, + }, entry.RegistrationData().Hostinfo) + hostinfo.RequestTags[0] = "tag:changed" + assert.Equal(t, []string{"tag:router"}, entry.RegistrationData().Hostinfo.RequestTags) + assert.Len(t, hostinfo.RoutableIPs, 1, "the request itself must be left alone") +} diff --git a/hscontrol/noise_test.go b/hscontrol/noise_test.go index 2ee46e9fe..05a6913c4 100644 --- a/hscontrol/noise_test.go +++ b/hscontrol/noise_test.go @@ -1936,6 +1936,42 @@ func TestPollNetMapHandler_UnknownNodeBeforeReportValidation(t *testing.T) { func TestRegistrationHandler_ValidatesClientReport(t *testing.T) { t.Parallel() + t.Run("pending registration retains auth metadata", func(t *testing.T) { + t.Parallel() + + app := createTestApp(t) + ns := &noiseServer{headscale: app, machineKey: key.NewMachine().Public()} + + rec := serveRegister(t, ns, tailcfg.RegisterRequest{ + Version: tailcfg.CurrentCapabilityVersion, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "report-pending-node", + OS: "linux", + OSVersion: "6.12", + RequestTags: []string{"tag:test"}, + Services: manyServices(300), + RoutableIPs: []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24")}, + NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, + }, + }) + require.Equal(t, http.StatusOK, rec.Code, "body=%q", rec.Body.String()) + + var resp tailcfg.RegisterResponse + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) + + authID, err := extractRegistrationIDFromAuthURL(resp.AuthURL) + require.NoError(t, err, "resp=%+v", resp) + + entry, ok := app.state.GetAuthCacheEntry(authID) + require.True(t, ok) + assert.Equal(t, &tailcfg.Hostinfo{ + Hostname: "report-pending-node", + OS: "linux", + RequestTags: []string{"tag:test"}, + }, entry.RegistrationData().Hostinfo) + }) + t.Run("oversized report", func(t *testing.T) { t.Parallel() diff --git a/hscontrol/oidc.go b/hscontrol/oidc.go index 898b1842d..fa09f8f6d 100644 --- a/hscontrol/oidc.go +++ b/hscontrol/oidc.go @@ -995,6 +995,11 @@ func (a *AuthProviderOIDC) handleRegistration( if err != nil { a.h.Change(nodeChange) + if errors.Is(err, types.ErrHostinfoTooLarge) { + return false, newHTTPUserError(http.StatusBadRequest, "node report too large", + "This device reports too much data. Ask your administrator to reduce its reported metadata, then sign in again.", err) + } + return false, fmt.Errorf("registering node: %w", err) } diff --git a/hscontrol/oidc_test.go b/hscontrol/oidc_test.go index 171d6ae22..c62b4e467 100644 --- a/hscontrol/oidc_test.go +++ b/hscontrol/oidc_test.go @@ -2,12 +2,14 @@ package hscontrol import ( "context" + "encoding/json" "io" "net/http" "net/http/cookiejar" "net/http/httptest" "net/url" "regexp" + "strings" "testing" "time" @@ -16,6 +18,7 @@ import ( "github.com/oauth2-proxy/mockoidc" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "tailscale.com/tailcfg" "tailscale.com/types/key" ) @@ -617,3 +620,43 @@ func TestRouterMethodNotAllowedIncludesAllow(t *testing.T) { assert.Equal(t, http.StatusMethodNotAllowed, resp.StatusCode) assert.Equal(t, []string{http.MethodGet}, resp.Header.Values("Allow")) } + +func TestOIDCRegistrationRejectsOversizedMergedHostinfo(t *testing.T) { + t.Parallel() + + app := createTestApp(t) + user := app.state.CreateUserForTest("oidc-report-user") + node := app.state.CreateNodeForTest(user, "oidc-report-node") + node.Hostinfo = &tailcfg.Hostinfo{Hostname: node.Hostname, OSVersion: "v"} + encoded, err := json.Marshal(node.Hostinfo) + require.NoError(t, err) + + node.Hostinfo.OSVersion = strings.Repeat("v", types.MaxHostinfoSize-len(encoded)) + require.NoError(t, types.ValidateHostinfo(node.Hostinfo)) + before := app.state.PutNodeInStoreForTest(*node) + + hostname := strings.Repeat("h", types.MaxHostnameLength) + authID := types.MustAuthID() + app.state.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: node.MachineKey, NodeKey: key.NewNode().Public(), + Hostname: hostname, Hostinfo: &tailcfg.Hostinfo{Hostname: hostname}, + })) + + provider := &AuthProviderOIDC{h: app} + changed, err := provider.handleRegistration(user, authID, nil) + require.ErrorIs(t, err, types.ErrHostinfoTooLarge) + + var httpErr HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, http.StatusBadRequest, httpErr.Code) + assert.Contains(t, strings.ToLower(httpErr.UserMsg), "reduce") + assert.Contains(t, httpErr.UserMsg, "metadata") + assert.Contains(t, httpErr.UserMsg, "sign") + assert.False(t, changed) + + after, ok := app.state.GetNodeByID(node.ID) + require.True(t, ok) + assert.Equal(t, before.NodeKey(), after.NodeKey()) + assert.Equal(t, before.Hostname(), after.Hostname()) + assert.True(t, before.Hostinfo().AsStruct().Equal(after.Hostinfo().AsStruct())) +} diff --git a/hscontrol/servertest/lifecycle_test.go b/hscontrol/servertest/lifecycle_test.go index 2ea9f3906..17d61e6d3 100644 --- a/hscontrol/servertest/lifecycle_test.go +++ b/hscontrol/servertest/lifecycle_test.go @@ -324,9 +324,8 @@ func TestReloginOfExpiredNodeClearsPeerExpiry(t *testing.T) { } // TestReloginPeerVisibleHostinfoReachesPeers covers the rest of the #3531 -// class: a relogin stores the RegisterRequest's Hostinfo, so the following -// MapRequest carries no Hostinfo delta, and a peer-visible change made at -// relogin reaches peers only if the relogin itself sends the whole node. +// class: peers must see changed services after the first MapRequest following +// relogin. A key-only patch cannot carry that change. func TestReloginPeerVisibleHostinfoReachesPeers(t *testing.T) { t.Parallel() diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index 104322856..182e9e8af 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -1,6 +1,7 @@ package state import ( + "encoding/json" "errors" "net/netip" "slices" @@ -363,7 +364,6 @@ func TestReauthRejectsNodeKeyClaimedByAnotherMachine(t *testing.T) { Hostname: "attacker", Hostinfo: &tailcfg.Hostinfo{}, }, - ValidHostinfo: &tailcfg.Hostinfo{}, Hostname: "attacker", User: attacker, RegisterMethod: util.RegisterMethodCLI, @@ -424,7 +424,6 @@ func TestReauthPreservesEndpointsWhenClientOmitsThem(t *testing.T) { Hostinfo: &tailcfg.Hostinfo{}, Endpoints: nil, }, - ValidHostinfo: &tailcfg.Hostinfo{}, Hostname: "node", User: user, RegisterMethod: util.RegisterMethodCLI, @@ -1484,7 +1483,7 @@ func TestNewStateDiscardsOversizedHostinfo(t *testing.T) { "oversized Hostinfo is dropped until the node reports again") } -// TestBootKeepsHostinfoTheMapPathStored pins that the startup bound keeps +// TestBootKeepsHostinfoTheMapPathStored pins that the startup check keeps // what the map path accepted. A report without NetInfo keeps the stored one, // so a stored Hostinfo can combine fields from two reports. func TestBootKeepsHostinfoTheMapPathStored(t *testing.T) { @@ -1517,10 +1516,183 @@ func TestBootKeepsHostinfoTheMapPathStored(t *testing.T) { reloaded, ok := persistTestReopen(t, dbPath).GetNodeByID(nodeID) require.True(t, ok) require.True(t, reloaded.Hostinfo().Valid(), - "the startup bound must keep a Hostinfo the map path stored") + "the startup check must keep a Hostinfo the map path stored") assert.Equal(t, tailcfg.DERPRegionID(1), reloaded.Hostinfo().NetInfo().PreferredDERP()) osVersionLen := len(reloaded.Hostinfo().OSVersion()) assert.Equal(t, 60<<10, osVersionLen) } + +// TestReauthKeepsOperationalMetadata covers a relogin before the next map request. +// Pending registrations carry only auth metadata; live connectivity must survive. +func TestReauthKeepsOperationalMetadata(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + route := netip.MustParsePrefix("10.0.0.0/24") + + node, ok := s.nodeStore.UpdateNode(nodeID, func(n *types.Node) { + n.Hostinfo = &tailcfg.Hostinfo{ + Hostname: n.Hostname, RoutableIPs: []netip.Prefix{route}, + OS: "linux", OSVersion: "6.12.0", + Services: []tailcfg.Service{{Proto: tailcfg.PeerAPI4, Port: 41641}}, + SSH_HostKeys: []string{"ssh-ed25519 key"}, + NetInfo: &tailcfg.NetInfo{PreferredDERP: 1}, + } + n.DiscoKey = key.NewDisco().Public() + n.Endpoints = []netip.AddrPort{netip.MustParseAddrPort("192.0.2.1:41641")} + n.ApprovedRoutes = []netip.Prefix{route} + }) + require.True(t, ok) + + _, epoch := s.Connect(nodeID) + require.NotZero(t, epoch) + require.Equal(t, []netip.Prefix{route}, s.GetNodePrimaryRoutes(nodeID), + "precondition: the node serves its route") + + authID := types.MustAuthID() + s.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: node.MachineKey(), + NodeKey: key.NewNode().Public(), + Hostname: node.Hostname(), + Hostinfo: &tailcfg.Hostinfo{Hostname: node.Hostname()}, + })) + + relogged, _, err := s.HandleNodeFromAuthPath( + authID, node.TypedUserID(), nil, util.RegisterMethodOIDC, + ) + require.NoError(t, err) + require.Equal(t, nodeID, relogged.ID(), "relogin must update the existing node") + + assert.Equal(t, []netip.Prefix{route}, relogged.AnnouncedRoutes()) + assert.Equal(t, []netip.Prefix{route}, s.GetNodePrimaryRoutes(nodeID)) + assert.True(t, node.Hostinfo().Equal(relogged.Hostinfo())) + assert.Equal(t, node.DiscoKey(), relogged.DiscoKey()) + assert.Equal(t, node.Endpoints(), relogged.Endpoints()) +} + +func TestReauthRejectsOversizedMergedHostinfo(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + node, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + hi := &tailcfg.Hostinfo{Hostname: node.Hostname(), OSVersion: "v"} + encoded, err := json.Marshal(hi) + require.NoError(t, err) + + hi.OSVersion = strings.Repeat("v", types.MaxHostinfoSize-len(encoded)+1) + require.NoError(t, types.ValidateHostinfo(hi)) + _, err = s.UpdateNodeFromMapRequest(nodeID, tailcfg.MapRequest{ + NodeKey: node.NodeKey(), DiscoKey: node.DiscoKey(), Hostinfo: hi, + }) + require.NoError(t, err) + + before, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + rowBefore, err := s.DB().GetNodeByID(nodeID) + require.NoError(t, err) + + hostname := strings.Repeat("h", types.MaxHostnameLength) + require.NoError(t, types.ValidateHostinfo(&tailcfg.Hostinfo{Hostname: hostname})) + + authID := types.MustAuthID() + s.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: node.MachineKey(), NodeKey: key.NewNode().Public(), + Hostname: hostname, Hostinfo: &tailcfg.Hostinfo{Hostname: hostname}, + })) + _, _, err = s.HandleNodeFromAuthPath(authID, node.TypedUserID(), nil, util.RegisterMethodOIDC) + require.ErrorIs(t, err, types.ErrHostinfoTooLarge) + + after, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + assert.Equal(t, before.AsStruct(), after.AsStruct(), "rejected merge leaves the live node unchanged") + + rowAfter, err := s.DB().GetNodeByID(nodeID) + require.NoError(t, err) + assert.Equal(t, rowBefore, rowAfter, "rejected merge leaves the stored node unchanged") +} + +func TestReauthKeepsTaggedFutureExpiry(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + _, err := s.SetPolicy([]byte(`{"tagOwners":{"tag:ci":["persist-user@"]}}`)) + require.NoError(t, err) + + node, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + userID := node.TypedUserID() + _, _, err = s.SetNodeTags(nodeID, []string{"tag:ci"}) + require.NoError(t, err) + + future := time.Now().Add(time.Hour).UTC() + node, ok = s.nodeStore.UpdateNode(nodeID, func(n *types.Node) { + n.Expiry = &future + }) + require.True(t, ok) + require.True(t, node.IsTagged()) + + authID := types.MustAuthID() + s.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: node.MachineKey(), NodeKey: key.NewNode().Public(), + Hostname: node.Hostname(), + Hostinfo: &tailcfg.Hostinfo{Hostname: node.Hostname(), RequestTags: []string{"tag:ci"}}, + })) + relogged, _, err := s.HandleNodeFromAuthPath(authID, userID, nil, util.RegisterMethodOIDC) + require.NoError(t, err) + require.Equal(t, nodeID, relogged.ID()) + require.True(t, relogged.IsTagged()) + require.True(t, relogged.Expiry().Valid()) + assert.True(t, future.Equal(relogged.Expiry().Get())) + + row, err := s.DB().GetNodeByID(nodeID) + require.NoError(t, err) + require.NotNil(t, row.Expiry) + assert.True(t, future.Equal(*row.Expiry)) +} + +func TestReauthReplacesReportedTags(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + _, err := s.SetPolicy([]byte(`{"tagOwners":{"tag:a":["persist-user@"],"tag:b":["persist-user@"]}}`)) + require.NoError(t, err) + + node, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + userID := node.TypedUserID() + + for _, tags := range [][]string{{"tag:a"}, nil, {"tag:b"}} { + want := slices.Clone(tags) + authID := types.MustAuthID() + s.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: node.MachineKey(), NodeKey: key.NewNode().Public(), + Hostname: node.Hostname(), + Hostinfo: &tailcfg.Hostinfo{Hostname: node.Hostname(), RequestTags: tags}, + })) + relogged, _, err := s.HandleNodeFromAuthPath(authID, userID, nil, util.RegisterMethodOIDC) + require.NoError(t, err) + require.Equal(t, nodeID, relogged.ID()) + assert.Equal(t, want, relogged.Hostinfo().RequestTags().AsSlice()) + assert.Equal(t, len(want) > 0, relogged.IsTagged()) + + if len(tags) > 0 { + tags[0] = "tag:changed" + } + + stored, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + assert.Equal(t, want, stored.Hostinfo().RequestTags().AsSlice(), + "cached tags must not alias stored Hostinfo") + + row, err := s.DB().GetNodeByID(nodeID) + require.NoError(t, err) + assert.Equal(t, want, row.Hostinfo.RequestTags) + } +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index f2fd8013c..b19f55900 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -51,10 +51,8 @@ const ( registerCacheExpiration = time.Minute * 15 // defaultRegisterCacheMaxEntries is the default upper bound on the number - // of pending registration entries the auth cache will hold. With a 15-minute - // TTL and a stripped-down RegistrationData payload (~200 bytes per entry), - // 1024 entries cap the worst-case cache footprint at well under 1 MiB even - // under sustained unauthenticated cache-fill attempts. + // of pending registration entries the auth cache will hold. Entries keep + // only hostname, OS and requested tags, capped by [types.ValidateHostinfo]. defaultRegisterCacheMaxEntries = 1024 // defaultNodeStoreBatchSize is the default number of write operations to batch @@ -1973,8 +1971,6 @@ type authNodeUpdateParams struct { ExistingNode types.NodeView // Cached registration payload from the originating client request. RegData *types.RegistrationData - // Pre-validated hostinfo; NetInfo preserved from ExistingNode. - ValidHostinfo *tailcfg.Hostinfo // Hostname from hostinfo, or generated from keys if client omits it. Hostname string // Auth user; may differ from ExistingNode.User() on conversion. @@ -2051,19 +2047,27 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView return types.NodeView{}, false, ErrNodeKeyInUse } - // Update existing node in [NodeStore] - validation passed, safe to mutate - before, updatedNodeView, ok := s.nodeStore.UpdateNodeDiff(params.ExistingNode.ID(), func(node *types.Node) { - node.NodeKey = regData.NodeKey - node.DiscoKey = regData.DiscoKey - node.Hostname = params.Hostname + var hostinfoErr error - // Preserve NetInfo from existing node when re-registering - node.Hostinfo = params.ValidHostinfo - node.Hostinfo.NetInfo = preserveNetInfo( - params.ExistingNode, - params.ExistingNode.ID(), - params.ValidHostinfo, - ) + // Read current operational metadata while holding the NodeStore writer lock. + before, updatedNodeView, ok := s.nodeStore.UpdateNodeDiff(params.ExistingNode.ID(), func(node *types.Node) { + hostinfo := cmp.Or(node.Hostinfo.Clone(), &tailcfg.Hostinfo{}) + + hostinfo.Hostname = params.Hostname + hostinfo.RequestTags = slices.Clone(requestTags) + + hostinfoErr = types.ValidateHostinfo(hostinfo) + if hostinfoErr != nil { + return + } + + node.NodeKey = regData.NodeKey + if !regData.DiscoKey.IsZero() { + node.DiscoKey = regData.DiscoKey + } + + node.Hostname = params.Hostname + node.Hostinfo = hostinfo // Preserve the node's live endpoints when the register request carried // none. Web/OIDC relogins report endpoints via MapRequest, not register, @@ -2097,34 +2101,11 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView isTagged := node.IsTagged() switch { - case wasTagged && !isTagged: - // Tagged → Personal: set expiry from client request - if params.Expiry != nil { - node.Expiry = params.Expiry - } else { - node.Expiry = regData.Expiry - } - case !wasTagged && isTagged: - // Personal → Tagged: clear expiry; tagged nodes disable it by default. - node.Expiry = nil - case params.IsConvertFromTag && !isTagged: - // Explicit conversion from tagged to user-owned: set expiry from client request - if params.Expiry != nil { - node.Expiry = params.Expiry - } else { - node.Expiry = regData.Expiry - } case !isTagged: - // Personal → Personal: update expiry from client - if params.Expiry != nil { - node.Expiry = params.Expiry - } else { - node.Expiry = regData.Expiry - } - case isTagged && node.IsExpired(): - // Tagged → Tagged and expired: a person re-authenticated it, so clear - // the past expiry. A future administrative expiry has not taken - // effect and falls through to the no-op below. + // User-owned nodes take the expiry from this authentication. + node.Expiry = cmp.Or(params.Expiry, regData.Expiry) + case !wasTagged || node.IsExpired(): + // Becoming tagged disables expiry; reauth clears a past expiry. node.Expiry = nil } // Tagged → Tagged and not expired: keep existing expiry - no action. @@ -2145,6 +2126,10 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView return types.NodeView{}, false, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, params.ExistingNode.ID()) } + if hostinfoErr != nil { + return types.NodeView{}, false, hostinfoErr + } + keysOnly := onlyKeysChanged(before, updatedNodeView, time.Now()) // Persist to database. @@ -2585,7 +2570,6 @@ func (s *State) HandleNodeFromAuthPath( // Common params for update operations updateParams := authNodeUpdateParams{ RegData: regData, - ValidHostinfo: hostinfo, Hostname: hostname, User: user, Expiry: expiry, diff --git a/hscontrol/types/registration.go b/hscontrol/types/registration.go index 669114d97..be984b589 100644 --- a/hscontrol/types/registration.go +++ b/hscontrol/types/registration.go @@ -13,11 +13,9 @@ import ( // only the fields the registration callback path actually consumes when // promoting a pending registration to a real node. // -// Combined with the bounded-LRU cache that holds these entries, this caps -// the worst-case memory footprint of unauthenticated cache-fill attempts -// at (max_entries × per_entry_size). The cache is sized so that the -// product is bounded to a few MiB even with attacker-supplied 1 MiB -// Hostinfos (the Noise body limit). +// Any client can open a pending registration. The bounded-LRU cache that +// holds these entries caps their number. Hostinfo retains only auth and +// confirmation metadata, bounded by [ValidateHostinfo] before caching. type RegistrationData struct { // MachineKey is the cryptographic identity of the machine being // registered. Required. @@ -34,14 +32,9 @@ type RegistrationData struct { // Already validated/normalised by EnsureHostname at producer time. Hostname string - // Hostinfo is the original [tailcfg.Hostinfo] from the [tailcfg.RegisterRequest], - // stored so that the auth callback can populate the new node's - // initial [tailcfg.Hostinfo] (and so that observability/CLI consumers see - // fields like OS, OSVersion, and IPNVersion before the first - // [tailcfg.MapRequest] restores the live set). - // - // May be nil if the client did not send [tailcfg.Hostinfo] in the original - // [tailcfg.RegisterRequest]. + // Hostinfo carries hostname and requested tags for auth, and OS for the + // confirmation page. Other fields arrive in the first MapRequest. + // It may be nil when the registration did not include Hostinfo. Hostinfo *tailcfg.Hostinfo // Endpoints is the initial set of WireGuard endpoints the node