From 3a1bbeba906ed913cd8c382b4004ce069812131f Mon Sep 17 00:00:00 2001 From: crn4 Date: Tue, 19 May 2026 20:27:50 +0200 Subject: [PATCH] review comments --- client/internal/engine.go | 8 +- .../network_map/controller/controller.go | 2 +- .../shared/grpc/components_encoder.go | 61 ++++++------ .../grpc/components_envelope_response.go | 27 ++++-- .../networks/resources/types/resource.go | 23 ++--- management/server/networks/routers/manager.go | 14 ++- .../server/networks/routers/types/router.go | 17 ++-- management/server/store/sql_store.go | 93 +++++++++++++++++-- management/server/types/account_components.go | 8 +- .../types/networkmap_wire_benchmark_test.go | 12 ++- .../types/networkmap_wire_breakdown_test.go | 7 ++ management/server/types/policy.go | 1 + route/route.go | 1 + shared/management/client/client_test.go | 59 ++++++++++-- shared/management/networkmap/decode.go | 31 ++++++- shared/management/networkmap/encode.go | 2 +- shared/management/networkmap/envelope.go | 10 +- 17 files changed, 289 insertions(+), 87 deletions(-) diff --git a/client/internal/engine.go b/client/internal/engine.go index f63840aa1..acbe94f22 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -874,8 +874,12 @@ func (e *Engine) handleSync(update *mgmProto.SyncResponse) error { return e.ctx.Err() } - if update.NetworkMap != nil && update.NetworkMap.PeerConfig != nil { - e.handleAutoUpdateVersion(update.NetworkMap.PeerConfig.AutoUpdate) + // Envelope sync responses carry PeerConfig at the top level; legacy + // NetworkMap syncs carry it under NetworkMap.PeerConfig. + if pc := update.GetPeerConfig(); pc != nil { + e.handleAutoUpdateVersion(pc.GetAutoUpdate()) + } else if nm := update.GetNetworkMap(); nm != nil && nm.GetPeerConfig() != nil { + e.handleAutoUpdateVersion(nm.GetPeerConfig().GetAutoUpdate()) } if update.GetNetbirdConfig() != nil { diff --git a/management/internals/controllers/network_map/controller/controller.go b/management/internals/controllers/network_map/controller/controller.go index 2f9274a06..938df5539 100644 --- a/management/internals/controllers/network_map/controller/controller.go +++ b/management/internals/controllers/network_map/controller/controller.go @@ -223,7 +223,7 @@ func (c *Controller) sendUpdateAccountPeers(ctx context.Context, accountID strin c.metrics.CountCalcPeerNetworkMapDuration(time.Since(start)) - proxyNetworkMap := proxyNetworkMaps[peer.ID] + proxyNetworkMap := proxyNetworkMaps[p.ID] if result.NetworkMap != nil && proxyNetworkMap != nil { result.NetworkMap.Merge(proxyNetworkMap) } diff --git a/management/internals/shared/grpc/components_encoder.go b/management/internals/shared/grpc/components_encoder.go index e9c01d56b..af91b008c 100644 --- a/management/internals/shared/grpc/components_encoder.go +++ b/management/internals/shared/grpc/components_encoder.go @@ -264,38 +264,47 @@ func (e *componentEncoder) encodePolicies(policies []*types.Policy) ([]*proto.Po if r == nil || !r.Enabled { continue } - pc := &proto.PolicyCompact{ - Id: pol.AccountSeqID, - Action: networkmap.GetProtoAction(string(r.Action)), - Protocol: networkmap.GetProtoProtocol(string(r.Protocol)), - Bidirectional: r.Bidirectional, - Ports: portsToUint32(r.Ports), - PortRanges: portRangesToProto(r.PortRanges), - SourceGroupIds: make([]uint32, 0, len(r.Sources)), - DestinationGroupIds: make([]uint32, 0, len(r.Destinations)), - AuthorizedUser: r.AuthorizedUser, - AuthorizedGroups: e.encodeAuthorizedGroups(r.AuthorizedGroups), - SourceResource: e.resourceToProto(r.SourceResource), - DestinationResource: e.resourceToProto(r.DestinationResource), - SourcePostureCheckSeqIds: e.postureCheckSeqs(pol.SourcePostureChecks), - } - for _, gid := range r.Sources { - if seq, ok := e.groupSeq(gid); ok { - pc.SourceGroupIds = append(pc.SourceGroupIds, seq) - } - } - for _, gid := range r.Destinations { - if seq, ok := e.groupSeq(gid); ok { - pc.DestinationGroupIds = append(pc.DestinationGroupIds, seq) - } - } idxByPolicy[pol] = append(idxByPolicy[pol], uint32(len(out))) - out = append(out, pc) + out = append(out, e.encodePolicyRule(pol, r)) } } return out, idxByPolicy } +// encodePolicyRule maps a single PolicyRule under pol to a PolicyCompact entry. +func (e *componentEncoder) encodePolicyRule(pol *types.Policy, r *types.PolicyRule) *proto.PolicyCompact { + return &proto.PolicyCompact{ + Id: pol.AccountSeqID, + Action: networkmap.GetProtoAction(string(r.Action)), + Protocol: networkmap.GetProtoProtocol(string(r.Protocol)), + Bidirectional: r.Bidirectional, + Ports: portsToUint32(r.Ports), + PortRanges: portRangesToProto(r.PortRanges), + SourceGroupIds: e.groupSeqIDs(r.Sources), + DestinationGroupIds: e.groupSeqIDs(r.Destinations), + AuthorizedUser: r.AuthorizedUser, + AuthorizedGroups: e.encodeAuthorizedGroups(r.AuthorizedGroups), + SourceResource: e.resourceToProto(r.SourceResource), + DestinationResource: e.resourceToProto(r.DestinationResource), + SourcePostureCheckSeqIds: e.postureCheckSeqs(pol.SourcePostureChecks), + } +} + +// groupSeqIDs maps the xid group IDs in src to their per-account seq ids, +// dropping any group that has no seq id assigned. +func (e *componentEncoder) groupSeqIDs(src []string) []uint32 { + if len(src) == 0 { + return nil + } + out := make([]uint32, 0, len(src)) + for _, gid := range src { + if seq, ok := e.groupSeq(gid); ok { + out = append(out, seq) + } + } + return out +} + // unionPolicies merges c.Policies with every policy referenced by // c.ResourcePoliciesMap, deduplicating by pointer identity. Resource-only // policies (relevant to a NetworkResource but not to peer-pair traffic) diff --git a/management/internals/shared/grpc/components_envelope_response.go b/management/internals/shared/grpc/components_envelope_response.go index dfd7b5ad4..5a6f8d066 100644 --- a/management/internals/shared/grpc/components_envelope_response.go +++ b/management/internals/shared/grpc/components_envelope_response.go @@ -152,16 +152,7 @@ func computeSSHEnabledForPeer(c *types.NetworkMapComponents, peer *nbpeer.Peer) continue } for _, rule := range policy.Rules { - if rule == nil || !rule.Enabled { - continue - } - if !peerInDestinations(c, rule, peer.ID) { - continue - } - if rule.Protocol == types.PolicyRuleProtocolNetbirdSSH { - return true - } - if peer.SSHEnabled && types.PolicyRuleImpliesLegacySSH(rule) { + if ruleEnablesSSHForPeer(c, rule, peer) { return true } } @@ -169,6 +160,22 @@ func computeSSHEnabledForPeer(c *types.NetworkMapComponents, peer *nbpeer.Peer) return false } +// ruleEnablesSSHForPeer returns true when rule is active, targets peer, and +// either explicitly authorises SSH or covers the legacy TCP/22 path while the +// peer itself has SSH enabled locally. +func ruleEnablesSSHForPeer(c *types.NetworkMapComponents, rule *types.PolicyRule, peer *nbpeer.Peer) bool { + if rule == nil || !rule.Enabled { + return false + } + if !peerInDestinations(c, rule, peer.ID) { + return false + } + if rule.Protocol == types.PolicyRuleProtocolNetbirdSSH { + return true + } + return peer.SSHEnabled && types.PolicyRuleImpliesLegacySSH(rule) +} + // peerInDestinations reports whether peerID is in any of rule.Destinations' // groups (or matches DestinationResource if it's a peer-typed resource — // for non-peer types Calculate falls through to group lookup, so we mirror diff --git a/management/server/networks/resources/types/resource.go b/management/server/networks/resources/types/resource.go index 1ced3ab91..454ca4162 100644 --- a/management/server/networks/resources/types/resource.go +++ b/management/server/networks/resources/types/resource.go @@ -96,17 +96,18 @@ func (n *NetworkResource) FromAPIRequest(req *api.NetworkResourceRequest) { func (n *NetworkResource) Copy() *NetworkResource { return &NetworkResource{ - ID: n.ID, - AccountID: n.AccountID, - NetworkID: n.NetworkID, - Name: n.Name, - Description: n.Description, - Type: n.Type, - Address: n.Address, - Domain: n.Domain, - Prefix: n.Prefix, - GroupIDs: n.GroupIDs, - Enabled: n.Enabled, + ID: n.ID, + AccountID: n.AccountID, + NetworkID: n.NetworkID, + AccountSeqID: n.AccountSeqID, + Name: n.Name, + Description: n.Description, + Type: n.Type, + Address: n.Address, + Domain: n.Domain, + Prefix: n.Prefix, + GroupIDs: n.GroupIDs, + Enabled: n.Enabled, } } diff --git a/management/server/networks/routers/manager.go b/management/server/networks/routers/manager.go index 3a985a5b0..4f60b82ce 100644 --- a/management/server/networks/routers/manager.go +++ b/management/server/networks/routers/manager.go @@ -173,10 +173,20 @@ func (m *managerImpl) UpdateRouter(ctx context.Context, userID string, router *t } oldRouter, err := transaction.GetNetworkRouterByID(ctx, store.LockingStrengthNone, router.AccountID, router.ID) - if err != nil { + if err == nil { + router.AccountSeqID = oldRouter.AccountSeqID + } else if e, ok := status.FromError(err); ok && e.Type() == status.NotFound { + // PUT-as-upsert: caller may target a brand-new router id (used by + // the dashboard's "save" flow). Allocate a fresh account_seq_id so + // the upsert behaves the same as Create(). + seq, allocErr := transaction.AllocateAccountSeqID(ctx, router.AccountID, serverTypes.AccountSeqEntityNetworkRouter) + if allocErr != nil { + return fmt.Errorf("failed to allocate network router seq id: %w", allocErr) + } + router.AccountSeqID = seq + } else { return fmt.Errorf("failed to get existing network router: %w", err) } - router.AccountSeqID = oldRouter.AccountSeqID err = transaction.SaveNetworkRouter(ctx, router) if err != nil { diff --git a/management/server/networks/routers/types/router.go b/management/server/networks/routers/types/router.go index 7325599b2..d4c55eaf3 100644 --- a/management/server/networks/routers/types/router.go +++ b/management/server/networks/routers/types/router.go @@ -81,14 +81,15 @@ func (n *NetworkRouter) FromAPIRequest(req *api.NetworkRouterRequest) { func (n *NetworkRouter) Copy() *NetworkRouter { return &NetworkRouter{ - ID: n.ID, - NetworkID: n.NetworkID, - AccountID: n.AccountID, - Peer: n.Peer, - PeerGroups: n.PeerGroups, - Masquerade: n.Masquerade, - Metric: n.Metric, - Enabled: n.Enabled, + ID: n.ID, + NetworkID: n.NetworkID, + AccountID: n.AccountID, + AccountSeqID: n.AccountSeqID, + Peer: n.Peer, + PeerGroups: n.PeerGroups, + Masquerade: n.Masquerade, + Metric: n.Metric, + Enabled: n.Enabled, } } diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index 40cdc7c36..05ba3fd71 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -3659,11 +3659,24 @@ func allocateAccountSeqIDMysql(db *gorm.DB, accountID string, entity types.Accou // the in-memory account whose AccountSeqID is zero. Called from SaveAccount so // the canonical "save the whole account" path produces the same persisted seq // ids that the manager-level Create paths produce. Update flows that go -// through SaveAccount preserve existing non-zero values. +// through SaveAccount preserve existing non-zero values; for those, the +// per-entity counter is bumped so subsequent AllocateAccountSeqID calls don't +// hand out a colliding id. func (s *SqlStore) assignAccountSeqIDs(ctx context.Context, tx *gorm.DB, account *types.Account) error { + maxByEntity := make(map[types.AccountSeqEntity]uint32, 8) + bump := func(entity types.AccountSeqEntity, seq uint32) { + if seq > maxByEntity[entity] { + maxByEntity[entity] = seq + } + } + for i := range account.GroupsG { g := account.GroupsG[i] - if g == nil || g.AccountSeqID != 0 { + if g == nil { + continue + } + if g.AccountSeqID != 0 { + bump(types.AccountSeqEntityGroup, g.AccountSeqID) continue } seq, err := allocateAccountSeqID(ctx, tx, s.storeEngine, account.Id, types.AccountSeqEntityGroup) @@ -3673,7 +3686,11 @@ func (s *SqlStore) assignAccountSeqIDs(ctx context.Context, tx *gorm.DB, account g.AccountSeqID = seq } for _, p := range account.Policies { - if p == nil || p.AccountSeqID != 0 { + if p == nil { + continue + } + if p.AccountSeqID != 0 { + bump(types.AccountSeqEntityPolicy, p.AccountSeqID) continue } seq, err := allocateAccountSeqID(ctx, tx, s.storeEngine, account.Id, types.AccountSeqEntityPolicy) @@ -3685,6 +3702,7 @@ func (s *SqlStore) assignAccountSeqIDs(ctx context.Context, tx *gorm.DB, account for i := range account.RoutesG { r := &account.RoutesG[i] if r.AccountSeqID != 0 { + bump(types.AccountSeqEntityRoute, r.AccountSeqID) continue } seq, err := allocateAccountSeqID(ctx, tx, s.storeEngine, account.Id, types.AccountSeqEntityRoute) @@ -3696,6 +3714,7 @@ func (s *SqlStore) assignAccountSeqIDs(ctx context.Context, tx *gorm.DB, account for i := range account.NameServerGroupsG { ng := &account.NameServerGroupsG[i] if ng.AccountSeqID != 0 { + bump(types.AccountSeqEntityNameserverGroup, ng.AccountSeqID) continue } seq, err := allocateAccountSeqID(ctx, tx, s.storeEngine, account.Id, types.AccountSeqEntityNameserverGroup) @@ -3705,7 +3724,11 @@ func (s *SqlStore) assignAccountSeqIDs(ctx context.Context, tx *gorm.DB, account ng.AccountSeqID = seq } for _, nr := range account.NetworkResources { - if nr == nil || nr.AccountSeqID != 0 { + if nr == nil { + continue + } + if nr.AccountSeqID != 0 { + bump(types.AccountSeqEntityNetworkResource, nr.AccountSeqID) continue } seq, err := allocateAccountSeqID(ctx, tx, s.storeEngine, account.Id, types.AccountSeqEntityNetworkResource) @@ -3715,7 +3738,11 @@ func (s *SqlStore) assignAccountSeqIDs(ctx context.Context, tx *gorm.DB, account nr.AccountSeqID = seq } for _, nr := range account.NetworkRouters { - if nr == nil || nr.AccountSeqID != 0 { + if nr == nil { + continue + } + if nr.AccountSeqID != 0 { + bump(types.AccountSeqEntityNetworkRouter, nr.AccountSeqID) continue } seq, err := allocateAccountSeqID(ctx, tx, s.storeEngine, account.Id, types.AccountSeqEntityNetworkRouter) @@ -3725,7 +3752,11 @@ func (s *SqlStore) assignAccountSeqIDs(ctx context.Context, tx *gorm.DB, account nr.AccountSeqID = seq } for _, n := range account.Networks { - if n == nil || n.AccountSeqID != 0 { + if n == nil { + continue + } + if n.AccountSeqID != 0 { + bump(types.AccountSeqEntityNetwork, n.AccountSeqID) continue } seq, err := allocateAccountSeqID(ctx, tx, s.storeEngine, account.Id, types.AccountSeqEntityNetwork) @@ -3735,7 +3766,11 @@ func (s *SqlStore) assignAccountSeqIDs(ctx context.Context, tx *gorm.DB, account n.AccountSeqID = seq } for _, pc := range account.PostureChecks { - if pc == nil || pc.AccountSeqID != 0 { + if pc == nil { + continue + } + if pc.AccountSeqID != 0 { + bump(types.AccountSeqEntityPostureCheck, pc.AccountSeqID) continue } seq, err := allocateAccountSeqID(ctx, tx, s.storeEngine, account.Id, types.AccountSeqEntityPostureCheck) @@ -3744,9 +3779,53 @@ func (s *SqlStore) assignAccountSeqIDs(ctx context.Context, tx *gorm.DB, account } pc.AccountSeqID = seq } + for entity, maxSeq := range maxByEntity { + if err := ensureAccountSeqCounter(tx, s.storeEngine, account.Id, entity, maxSeq+1); err != nil { + return fmt.Errorf("seed counter for %s: %w", entity, err) + } + } return nil } +// ensureAccountSeqCounter raises the per-account counter for entity to at +// least target. Used when SaveAccount persists components that already carry +// AccountSeqIDs (e.g. test bulk-load from sqlite to postgres, or migrations +// running before component data lands) so that the next AllocateAccountSeqID +// call returns a fresh id beyond what was just written. +func ensureAccountSeqCounter(db *gorm.DB, engine types.Engine, accountID string, entity types.AccountSeqEntity, target uint32) error { + switch engine { + case types.PostgresStoreEngine, types.SqliteStoreEngine: + const sqlStr = ` + INSERT INTO account_seq_counters (account_id, entity, next_id) + VALUES (?, ?, ?) + ON CONFLICT (account_id, entity) DO UPDATE + SET next_id = GREATEST(account_seq_counters.next_id, EXCLUDED.next_id) + ` + // sqlite's UPSERT understands max() but the migration uses GREATEST + // for postgres and max() for sqlite. We collapse to dialect-specific + // statements only when needed. + if engine == types.SqliteStoreEngine { + const sqliteSQL = ` + INSERT INTO account_seq_counters (account_id, entity, next_id) + VALUES (?, ?, ?) + ON CONFLICT (account_id, entity) DO UPDATE + SET next_id = max(account_seq_counters.next_id, excluded.next_id) + ` + return db.Exec(sqliteSQL, accountID, string(entity), target).Error + } + return db.Exec(sqlStr, accountID, string(entity), target).Error + case types.MysqlStoreEngine: + const sqlStr = ` + INSERT INTO account_seq_counters (account_id, entity, next_id) + VALUES (?, ?, ?) + ON DUPLICATE KEY UPDATE next_id = GREATEST(next_id, VALUES(next_id)) + ` + return db.Exec(sqlStr, accountID, string(entity), target).Error + default: + return fmt.Errorf("unsupported store engine for account_seq counter: %v", engine) + } +} + // transaction wraps a GORM transaction with MySQL-specific FK checks handling // Use this instead of db.Transaction() directly to avoid deadlocks on MySQL/Aurora func (s *SqlStore) transaction(fn func(*gorm.DB) error) error { diff --git a/management/server/types/account_components.go b/management/server/types/account_components.go index 45f7189b0..8f6ffb6ba 100644 --- a/management/server/types/account_components.go +++ b/management/server/types/account_components.go @@ -348,9 +348,11 @@ func (a *Account) getPeersGroupsPoliciesRoutes( for _, groupID := range r.Groups { relevantGroupIDs[groupID] = a.GetGroup(groupID) } - for _, groupID := range r.AccessControlGroups { - relevantGroupIDs[groupID] = a.GetGroup(groupID) - routeAccessControlGroups[groupID] = struct{}{} + if r.Enabled { + for _, groupID := range r.AccessControlGroups { + relevantGroupIDs[groupID] = a.GetGroup(groupID) + routeAccessControlGroups[groupID] = struct{}{} + } } relevantRoutes = append(relevantRoutes, r) } diff --git a/management/server/types/networkmap_wire_benchmark_test.go b/management/server/types/networkmap_wire_benchmark_test.go index eb18dd04e..43c9e1fbf 100644 --- a/management/server/types/networkmap_wire_benchmark_test.go +++ b/management/server/types/networkmap_wire_benchmark_test.go @@ -43,7 +43,7 @@ func populateAccountSeqIDs(account *types.Account) { } // assignValidWgKeys overwrites every peer's Key with a valid base64-encoded -// 32-byte string. The default scalableTestAccount uses unparseable strings +// 32-byte string. The default scalableTestAccount uses unparsable strings // like "key-peer-0", which makes the components encoder emit a nil WgPubKey // and the legacy encoder ship 10-char placeholders — both shrink the wire // size in unrealistic ways. Production peers always have valid 44-char base64 @@ -154,14 +154,20 @@ func BenchmarkNetworkMapWireSize(b *testing.B) { settings := &types.Settings{} legacyResp := mgmtgrpc.ToSyncResponse(ctx, nil, nil, nil, peer, nil, nil, networkMap, "netbird.cloud", nil, dnsCache, settings, nil, nil, 0) - legacyBytes, _ := goproto.Marshal(legacyResp.NetworkMap) + legacyBytes, err := goproto.Marshal(legacyResp.NetworkMap) + if err != nil { + b.Fatalf("marshal legacy networkmap: %v", err) + } env := mgmtgrpc.EncodeNetworkMapEnvelope(mgmtgrpc.ComponentsEnvelopeInput{ Components: components, PeerConfig: legacyResp.NetworkMap.PeerConfig, DNSDomain: "netbird.cloud", }) - envBytes, _ := goproto.Marshal(env) + envBytes, err := goproto.Marshal(env) + if err != nil { + b.Fatalf("marshal envelope: %v", err) + } b.Run(fmt.Sprintf("size/%s", scale.name), func(b *testing.B) { b.ReportMetric(float64(len(legacyBytes)), "legacy_bytes") diff --git a/management/server/types/networkmap_wire_breakdown_test.go b/management/server/types/networkmap_wire_breakdown_test.go index d76e73fb9..39b0b81c4 100644 --- a/management/server/types/networkmap_wire_breakdown_test.go +++ b/management/server/types/networkmap_wire_breakdown_test.go @@ -3,6 +3,7 @@ package types_test import ( "context" "fmt" + "os" "testing" goproto "google.golang.org/protobuf/proto" @@ -23,6 +24,9 @@ func TestNetworkMapWireBreakdown(t *testing.T) { if testing.Short() { t.Skip("size diagnostic, skipped with -short") } + if os.Getenv("NB_RUN_WIRE_BREAKDOWN") != "1" { + t.Skip("set NB_RUN_WIRE_BREAKDOWN=1 to run wire breakdown diagnostic") + } const peerCount, groupCount = 5000, 100 account, validatedPeers := scalableTestAccount(peerCount, groupCount) @@ -74,6 +78,9 @@ func TestNetworkMapWireBreakdown(t *testing.T) { } full := envelope.GetFull() + if full == nil { + t.Fatalf("expected full network map envelope payload, got nil") + } t.Logf("\n=== COMPONENTS NetworkMapEnvelope (%d peers, %d groups) ===", peerCount, groupCount) t.Logf(" Total: %d bytes (%.1f%% of legacy)\n", componentsTotal, pct(componentsTotal, legacyTotal)) diff --git a/management/server/types/policy.go b/management/server/types/policy.go index 69c7c9762..fdda30b66 100644 --- a/management/server/types/policy.go +++ b/management/server/types/policy.go @@ -91,6 +91,7 @@ func (p *Policy) Copy() *Policy { c := &Policy{ ID: p.ID, AccountID: p.AccountID, + AccountSeqID: p.AccountSeqID, Name: p.Name, Description: p.Description, Enabled: p.Enabled, diff --git a/route/route.go b/route/route.go index 4a8c342b2..8a26cc3bb 100644 --- a/route/route.go +++ b/route/route.go @@ -131,6 +131,7 @@ func (r *Route) Copy() *Route { route := &Route{ ID: r.ID, AccountID: r.AccountID, + AccountSeqID: r.AccountSeqID, Description: r.Description, NetID: r.NetID, Network: r.Network, diff --git a/shared/management/client/client_test.go b/shared/management/client/client_test.go index a8e8172dc..37daabde5 100644 --- a/shared/management/client/client_test.go +++ b/shared/management/client/client_test.go @@ -316,27 +316,74 @@ func TestClient_Sync(t *testing.T) { select { case resp := <-ch: - if resp.GetPeerConfig() == nil { + if resp.GetPeerConfig() == nil && resp.GetNetworkMap().GetPeerConfig() == nil { t.Error("expecting non nil PeerConfig got nil") } if resp.GetNetbirdConfig() == nil { t.Error("expecting non nil NetbirdConfig got nil") } - if len(resp.GetRemotePeers()) != 1 { - t.Errorf("expecting RemotePeers size %d got %d", 1, len(resp.GetRemotePeers())) + // Component-capable clients receive a NetworkMapEnvelope; the + // remote-peers list is encoded inside it. Decode it and check the + // envelope's peers slice. Legacy peers populate the top-level + // RemotePeers; both shapes must surface exactly one remote peer. + remotePeerKeys := remotePeerKeysFromSync(resp, testKey.PublicKey().String()) + if len(remotePeerKeys) != 1 { + t.Errorf("expecting RemotePeers size %d got %d", 1, len(remotePeerKeys)) return } - if resp.GetRemotePeersIsEmpty() == true { + if resp.GetNetworkMap() != nil && resp.GetRemotePeersIsEmpty() { t.Error("expecting RemotePeers property to be false, got true") } - if resp.GetRemotePeers()[0].GetWgPubKey() != remoteKey.PublicKey().String() { - t.Errorf("expecting RemotePeer public key %s got %s", remoteKey.PublicKey().String(), resp.GetRemotePeers()[0].GetWgPubKey()) + if remotePeerKeys[0] != remoteKey.PublicKey().String() { + t.Errorf("expecting RemotePeer public key %s got %s", remoteKey.PublicKey().String(), remotePeerKeys[0]) } case <-time.After(3 * time.Second): t.Error("timeout waiting for test to finish") } } +// remotePeerKeysFromSync extracts the remote-peer WG keys from either the +// legacy NetworkMap.RemotePeers list or the components NetworkMapEnvelope's +// inner peers slice (filtering out the local receiving peer identified by +// localKey, since the envelope's peers list is index-addressed and includes +// the local peer alongside remotes). +func remotePeerKeysFromSync(resp *mgmtProto.SyncResponse, localKey string) []string { + if rp := resp.GetRemotePeers(); len(rp) > 0 { + out := make([]string, 0, len(rp)) + for _, p := range rp { + out = append(out, p.GetWgPubKey()) + } + return out + } + env := resp.GetNetworkMapEnvelope().GetFull() + if env == nil { + return nil + } + out := make([]string, 0, len(env.GetPeers())) + for _, p := range env.GetPeers() { + key := wgKeyFromBytes(p.GetWgPubKey()) + if key == "" || key == localKey { + continue + } + out = append(out, key) + } + return out +} + +// wgKeyFromBytes mirrors the client-side decoder: the envelope ships raw 32 +// bytes; reconstruct the standard base64 key the test compares against. +func wgKeyFromBytes(raw []byte) string { + if len(raw) == 0 { + return "" + } + var k wgtypes.Key + if len(raw) != len(k) { + return "" + } + copy(k[:], raw) + return k.String() +} + func Test_SystemMetaDataFromClient(t *testing.T) { s, lis, mgmtMockServer, serverKey := startMockManagement(t) defer s.GracefulStop() diff --git a/shared/management/networkmap/decode.go b/shared/management/networkmap/decode.go index 84b104986..4d4a41c88 100644 --- a/shared/management/networkmap/decode.go +++ b/shared/management/networkmap/decode.go @@ -80,6 +80,9 @@ func DecodeEnvelope(env *proto.NetworkMapEnvelope) (*types.NetworkMapComponents, // for peers (and never has). peerIDByIndex := make([]string, len(full.Peers)) for idx, pc := range full.Peers { + if pc == nil { + return nil, fmt.Errorf("invalid envelope: peers[%d] is nil", idx) + } peerID := synthPeerID(uint32(idx)) peer := decodePeerCompact(pc, peerID, full.AgentVersions) c.Peers[peerID] = peer @@ -88,7 +91,10 @@ func DecodeEnvelope(env *proto.NetworkMapEnvelope) (*types.NetworkMapComponents, // Phase 2: groups. AccountSeqID becomes both the synthesized string ID // and the GroupCompact.id wire value. - for _, gc := range full.Groups { + for i, gc := range full.Groups { + if gc == nil { + return nil, fmt.Errorf("invalid envelope: groups[%d] is nil", i) + } groupID := synthGroupID(gc.Id) peerIDs := make([]string, 0, len(gc.PeerIndexes)) for _, idx := range gc.PeerIndexes { @@ -108,23 +114,35 @@ func DecodeEnvelope(env *proto.NetworkMapEnvelope) (*types.NetworkMapComponents, // model is 1 rule per policy). Policy.ID is synthesized from the // per-account seq id; proto.FirewallRule.PolicyID downstream carries // the same synth string (no xid on the wire). - for _, pc := range full.Policies { + for i, pc := range full.Policies { + if pc == nil { + return nil, fmt.Errorf("invalid envelope: policies[%d] is nil", i) + } policyID := synthPolicyID(pc.Id) c.Policies = append(c.Policies, decodePolicyCompact(pc, policyID, peerIDByIndex)) } // Phase 4: routes. - for _, rr := range full.Routes { + for i, rr := range full.Routes { + if rr == nil { + return nil, fmt.Errorf("invalid envelope: routes[%d] is nil", i) + } c.Routes = append(c.Routes, decodeRouteRaw(rr, peerIDByIndex)) } // Phase 5: NSGs. - for _, nsg := range full.NameserverGroups { + for i, nsg := range full.NameserverGroups { + if nsg == nil { + return nil, fmt.Errorf("invalid envelope: nameserver_groups[%d] is nil", i) + } c.NameServerGroups = append(c.NameServerGroups, decodeNameServerGroupRaw(nsg)) } // Phase 6: network resources. - for _, nr := range full.NetworkResources { + for i, nr := range full.NetworkResources { + if nr == nil { + return nil, fmt.Errorf("invalid envelope: network_resources[%d] is nil", i) + } c.NetworkResources = append(c.NetworkResources, decodeNetworkResource(nr)) } @@ -513,6 +531,9 @@ func portRangesFromProto(ranges []*proto.PortInfo_Range) []types.RulePortRange { } out := make([]types.RulePortRange, 0, len(ranges)) for _, r := range ranges { + if r == nil || r.Start > 65535 || r.End > 65535 { + continue + } out = append(out, types.RulePortRange{ Start: uint16(r.Start), End: uint16(r.End), diff --git a/shared/management/networkmap/encode.go b/shared/management/networkmap/encode.go index ebaede64a..1b1c3380e 100644 --- a/shared/management/networkmap/encode.go +++ b/shared/management/networkmap/encode.go @@ -307,7 +307,7 @@ func BuildAuthorizedUsersProto(ctx context.Context, authorizedUsers map[string]m if !exists { hash, err := sshauth.HashUserID(userID) if err != nil { - log.WithContext(ctx).Errorf("failed to hash user id %s: %v", userID, err) + log.WithContext(ctx).WithError(err).Error("failed to hash user id") continue } idx = uint32(len(hashedUsers)) diff --git a/shared/management/networkmap/envelope.go b/shared/management/networkmap/envelope.go index 295c26b7a..e90cb5982 100644 --- a/shared/management/networkmap/envelope.go +++ b/shared/management/networkmap/envelope.go @@ -52,8 +52,8 @@ func EnvelopeToNetworkMap(ctx context.Context, env *proto.NetworkMapEnvelope, lo } components.PeerID = localPeerID - includeIPv6 := localPeer != nil && localPeer.SupportsIPv6() && localPeer.IPv6.IsValid() - useSourcePrefixes := localPeer != nil && localPeer.SupportsSourcePrefixes() + includeIPv6 := localPeer.SupportsIPv6() && localPeer.IPv6.IsValid() + useSourcePrefixes := localPeer.SupportsSourcePrefixes() typedNM := components.Calculate(ctx) @@ -149,9 +149,15 @@ func appendUniquePeers(dst, extra []*proto.RemotePeerConfig) []*proto.RemotePeer } seen := make(map[string]struct{}, len(dst)) for _, p := range dst { + if p == nil { + continue + } seen[p.WgPubKey] = struct{}{} } for _, p := range extra { + if p == nil { + continue + } if _, ok := seen[p.WgPubKey]; ok { continue }