diff --git a/client/internal/engine.go b/client/internal/engine.go index d517d1d68..1613bbbc9 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -1061,7 +1061,11 @@ func (e *Engine) handleSync(update *mgmProto.SyncResponse) error { // back to empty if the FQDN doesn't have the expected shape. dnsName = extractDNSDomainFromFQDN(pc.GetFqdn()) } - result, err := nbnetworkmap.EnvelopeToNetworkMap(e.ctx, envelope, localKey, dnsName) + // With the firewall disabled there is no ACL manager to program, so + // RoutesFirewallRules would be built and then dropped. On a peer that + // routes many network resources that is the single most expensive + // step of the sync. + result, err := nbnetworkmap.EnvelopeToNetworkMap(e.ctx, envelope, localKey, dnsName, e.config.DisableFirewall) if err != nil { return fmt.Errorf("decode network map envelope: %w", err) } diff --git a/management/internals/controllers/network_map/nmaptest/runner.go b/management/internals/controllers/network_map/nmaptest/runner.go index 0d6ac9c18..ffce6483e 100644 --- a/management/internals/controllers/network_map/nmaptest/runner.go +++ b/management/internals/controllers/network_map/nmaptest/runner.go @@ -245,7 +245,7 @@ func computeMode(t *testing.T, ctx context.Context, mode Mode, nmData *networkma peerGroups := maps.Keys(nmData.GetPeerGroups(peerID)) resp := mgmtgrpc.ToComponentSyncResponse(ctx, nil, nil, nil, peer, nil, nil, components, nil, dnsDomain, nil, nmData.AccountSettings, nil, peerGroups, dnsFwdPort) - res, err := networkmap.EnvelopeToNetworkMap(ctx, resp.NetworkMapEnvelope, peer.Key, dnsDomain) + res, err := networkmap.EnvelopeToNetworkMap(ctx, resp.NetworkMapEnvelope, peer.Key, dnsDomain, false) require.NoError(t, err, "expand envelope") return res.NetworkMap default: diff --git a/management/server/types/networkmap_components_test.go b/management/server/types/networkmap_components_test.go index f6d542609..9e76de775 100644 --- a/management/server/types/networkmap_components_test.go +++ b/management/server/types/networkmap_components_test.go @@ -175,6 +175,63 @@ func TestNetworkMapComponents_NetworkResourceRoutes_RouterPeer(t *testing.T) { assert.NotEmpty(t, nm.RoutesFirewallRules, "router peer should have route firewall rules for the resource") } +// A receiver without a firewall asks Calculate to skip the route firewall +// rules. Everything the rest of the sync consumes — routes, peers, peer +// firewall rules — must come out unchanged. +func TestNetworkMapComponents_SkipRouteFirewallRules(t *testing.T) { + ctx := context.Background() + account := createComponentTestAccount() + + // The shared fixture leaves peer-router-1 out of every peer ACL, so its + // FirewallRules would be empty and the comparison below vacuous. Give the + // router a policy of its own. + account.Policies = append(account.Policies, &types.Policy{ + ID: "policy-router", Name: "Router connectivity", Enabled: true, + Rules: []*types.PolicyRule{{ + ID: "rule-router", Name: "Allow all <-> router", Enabled: true, + Action: types.PolicyTrafficActionAccept, Protocol: types.PolicyRuleProtocolALL, + Bidirectional: true, + Sources: []string{"group-all"}, Destinations: []string{"group-all"}, + }}, + }) + + validated := allPeersValidated(account) + + components := account.GetPeerNetworkMapComponents( + ctx, + "peer-router-1", + account.GetPeersCustomZone(ctx, "netbird.io"), + nil, + validated, + account.GetResourcePoliciesMap(), + account.GetResourceRoutersMap(), + account.GetActiveGroupUsers(), + ) + + full := components.Calculate(ctx) + require.NotEmpty(t, full.RoutesFirewallRules, "baseline: router peer must get route firewall rules") + require.NotEmpty(t, full.FirewallRules, "baseline: router peer must get peer firewall rules") + + components.SkipRouteFirewallRules = true + skipped := components.Calculate(ctx) + + assert.Empty(t, skipped.RoutesFirewallRules, "route firewall rules must not be computed when skipped") + assert.ElementsMatch(t, routeNetworks(full.Routes), routeNetworks(skipped.Routes), + "skipping route firewall rules must not change the routes") + assert.ElementsMatch(t, peerIDs(full.Peers), peerIDs(skipped.Peers), + "skipping route firewall rules must not change the peers to connect") + assert.Equal(t, full.FirewallRules, skipped.FirewallRules, + "peer firewall rules are unrelated and must come out unchanged") +} + +func routeNetworks(routes []*nmdata.Route) []string { + networks := make([]string, 0, len(routes)) + for _, r := range routes { + networks = append(networks, r.Network.String()) + } + return networks +} + func TestNetworkMapComponents_NetworkResourceRoutes_UnrelatedPeer(t *testing.T) { account := createComponentTestAccount() validated := allPeersValidated(account) diff --git a/shared/management/networkmap/envelope.go b/shared/management/networkmap/envelope.go index e7961fd7b..fd9dd6bbd 100644 --- a/shared/management/networkmap/envelope.go +++ b/shared/management/networkmap/envelope.go @@ -35,7 +35,12 @@ type EnvelopeResult struct { // // dnsName is the account's DNS domain ("netbird.cloud" etc.); used when // rebuilding the per-peer FQDNs that proto.RemotePeerConfig carries. -func EnvelopeToNetworkMap(ctx context.Context, env *proto.NetworkMapEnvelope, localPeerKey, dnsName string) (*EnvelopeResult, error) { +// +// skipRouteFirewallRules leaves RoutesFirewallRules empty. Callers that have +// no firewall to program pass true: the rules are the most expensive part of +// Calculate on a peer that routes many network resources, and nothing reads +// them afterwards. +func EnvelopeToNetworkMap(ctx context.Context, env *proto.NetworkMapEnvelope, localPeerKey, dnsName string, skipRouteFirewallRules bool) (*EnvelopeResult, error) { components, err := DecodeEnvelope(ctx, env) if err != nil { return nil, fmt.Errorf("decode envelope: %w", err) @@ -53,6 +58,7 @@ func EnvelopeToNetworkMap(ctx context.Context, env *proto.NetworkMapEnvelope, lo return nil, fmt.Errorf("receiving peer (wg_key prefix %q) not found among %d decoded peers — components have no PeerID, Calculate would return empty", trimKey(localPeerKey), len(components.Peers)) } components.PeerID = canonicalKey + components.SkipRouteFirewallRules = skipRouteFirewallRules includeIPv6 := localPeer.SupportsIPv6() && localPeer.IPv6.IsValid() useSourcePrefixes := localPeer.SupportsSourcePrefixes() diff --git a/shared/management/networkmap/envelope_test.go b/shared/management/networkmap/envelope_test.go index 7fe2a5277..98333a3c8 100644 --- a/shared/management/networkmap/envelope_test.go +++ b/shared/management/networkmap/envelope_test.go @@ -9,6 +9,7 @@ import ( "net/netip" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" goproto "google.golang.org/protobuf/proto" @@ -37,7 +38,7 @@ func TestEnvelopeToNetworkMap_RoundTrip(t *testing.T) { var decoded proto.NetworkMapEnvelope require.NoError(t, goproto.Unmarshal(wire, &decoded), "unmarshal envelope") - result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud") + result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud", false) require.NoError(t, err, "EnvelopeToNetworkMap") require.NotNil(t, result) require.NotNil(t, result.NetworkMap, "decoded NetworkMap must be non-nil") @@ -78,7 +79,7 @@ func TestCalculate_FirewallRuleProtocol_NeverNetbirdSSH(t *testing.T) { var decoded proto.NetworkMapEnvelope require.NoError(t, goproto.Unmarshal(wire, &decoded)) - result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud") + result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud", false) require.NoError(t, err) require.NotEmpty(t, result.NetworkMap.FirewallRules, "ssh policy should produce firewall rules") for i, fr := range result.NetworkMap.FirewallRules { @@ -88,13 +89,13 @@ func TestCalculate_FirewallRuleProtocol_NeverNetbirdSSH(t *testing.T) { } func TestEnvelopeToNetworkMap_NilEnvelope(t *testing.T) { - _, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), nil, "key", "netbird.cloud") + _, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), nil, "key", "netbird.cloud", false) require.Error(t, err, "nil envelope must produce an error rather than panic") } func TestEnvelopeToNetworkMap_FullPayloadMissing(t *testing.T) { env := &proto.NetworkMapEnvelope{} - _, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), env, "key", "netbird.cloud") + _, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), env, "key", "netbird.cloud", false) require.Error(t, err, "envelope with no Full payload must produce an error") } @@ -126,7 +127,7 @@ func TestDecodeEnvelope_MalformedWgKeyPeerSkipped(t *testing.T) { var decoded proto.NetworkMapEnvelope require.NoError(t, goproto.Unmarshal(wire, &decoded), "unmarshal envelope") - result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud") + result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud", false) require.NoError(t, err, "EnvelopeToNetworkMap must tolerate one bad peer key") require.NotNil(t, result) require.NotNil(t, result.Components) @@ -195,7 +196,7 @@ func TestEnvelopeRoundTrip_AllGroupShortCircuitParity(t *testing.T) { var decodedEnv proto.NetworkMapEnvelope require.NoError(t, goproto.Unmarshal(wire, &decodedEnv), "unmarshal envelope") - result, err := nbnetworkmap.EnvelopeToNetworkMap(ctx, &decodedEnv, peers["peer-T"].Key, "netbird.cloud") + result, err := nbnetworkmap.EnvelopeToNetworkMap(ctx, &decodedEnv, peers["peer-T"].Key, "netbird.cloud", false) require.NoError(t, err, "EnvelopeToNetworkMap") clientNM := result.NetworkMap @@ -253,7 +254,7 @@ func TestEnvelopeToNetworkMap_EmptyComponents(t *testing.T) { var decoded proto.NetworkMapEnvelope require.NoError(t, goproto.Unmarshal(wire, &decoded), "unmarshal envelope") - result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud") + result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud", false) require.NoError(t, err, "EnvelopeToNetworkMap must degrade gracefully on empty components") require.Equal(t, uint64(7), result.NetworkMap.Serial) require.Empty(t, result.NetworkMap.RemotePeers, "unvalidated peer connects to nobody") @@ -276,7 +277,7 @@ func TestEnvelopeToNetworkMap_MissingNetwork(t *testing.T) { var decoded proto.NetworkMapEnvelope require.NoError(t, goproto.Unmarshal(wire, &decoded), "unmarshal envelope") - result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud") + result, err := nbnetworkmap.EnvelopeToNetworkMap(context.Background(), &decoded, localPeerKey, "netbird.cloud", false) require.NoError(t, err, "a missing AccountNetwork must not panic the client") require.NotNil(t, result.Components.Network) require.NotEmpty(t, result.NetworkMap.RemotePeers, "the rest of the snapshot stays usable") @@ -353,3 +354,110 @@ func randomWgKey(t *testing.T) string { require.NoError(t, err) return base64.StdEncoding.EncodeToString(raw[:]) } + +// TestEnvelopeToNetworkMap_SkipRouteFirewallRules covers the flag end to end, +// through the envelope rather than by poking Calculate directly. The +// RoutesFirewallRulesIsEmpty derivation is the part that matters: the client's +// legacy-management probe reads an empty rule list together with that bit, so +// skipping the rules must set it rather than leave it false. +func TestEnvelopeToNetworkMap_SkipRouteFirewallRules(t *testing.T) { + ctx := context.Background() + c, routerKey := buildRoutedResourceComponents(t) + + envelope := mgmtgrpc.EncodeNetworkMapEnvelope(mgmtgrpc.ComponentsEnvelopeInput{ + Components: c, + DNSDomain: "netbird.cloud", + }) + wire, err := goproto.Marshal(envelope) + require.NoError(t, err, "marshal envelope") + var decoded proto.NetworkMapEnvelope + require.NoError(t, goproto.Unmarshal(wire, &decoded), "unmarshal envelope") + + full, err := nbnetworkmap.EnvelopeToNetworkMap(ctx, &decoded, routerKey, "netbird.cloud", false) + require.NoError(t, err, "EnvelopeToNetworkMap without skip") + require.NotEmpty(t, full.NetworkMap.RoutesFirewallRules, + "baseline: the router peer must receive route firewall rules") + require.False(t, full.NetworkMap.RoutesFirewallRulesIsEmpty, + "baseline: the empty bit must be false when rules are present") + + var decodedSkip proto.NetworkMapEnvelope + require.NoError(t, goproto.Unmarshal(wire, &decodedSkip), "unmarshal envelope") + skipped, err := nbnetworkmap.EnvelopeToNetworkMap(ctx, &decodedSkip, routerKey, "netbird.cloud", true) + require.NoError(t, err, "EnvelopeToNetworkMap with skip") + + assert.Empty(t, skipped.NetworkMap.RoutesFirewallRules, + "route firewall rules must not be computed when skipped") + assert.True(t, skipped.NetworkMap.RoutesFirewallRulesIsEmpty, + "the empty bit must be derived from the skipped list, or the client misreads it as legacy management") + assert.Len(t, skipped.NetworkMap.Routes, len(full.NetworkMap.Routes), + "skipping route firewall rules must not change the routes") + assert.Len(t, skipped.NetworkMap.RemotePeers, len(full.NetworkMap.RemotePeers), + "skipping route firewall rules must not change the remote peers") +} + +// buildRoutedResourceComponents returns components in which the local peer is +// the routing peer for one enabled network resource, reachable by a second +// peer through a resource policy — the minimum shape that yields a non-empty +// RoutesFirewallRules. It also returns the local peer's WG key. +func buildRoutedResourceComponents(t *testing.T) (*types.NetworkMapComponents, string) { + t.Helper() + + routerKey := randomWgKey(t) + peers := map[string]*nmdata.Peer{ + "peer-R": { + ID: "peer-R", Key: routerKey, DNSLabel: "router", + IP: netip.AddrFrom4([4]byte{100, 64, 0, 1}), + Meta: nmdata.PeerSystemMeta{WtVersion: "0.40.0"}, + }, + "peer-S": { + ID: "peer-S", Key: randomWgKey(t), DNSLabel: "source", + IP: netip.AddrFrom4([4]byte{100, 64, 0, 2}), + Meta: nmdata.PeerSystemMeta{WtVersion: "0.40.0"}, + }, + } + + resourcePolicy := &nmdata.Policy{ + ID: "pol-res", PublicID: "10", Enabled: true, + Rules: []*nmdata.PolicyRule{{ + ID: "rule-res", + Enabled: true, + Action: string(types.PolicyTrafficActionAccept), + Protocol: string(types.PolicyRuleProtocolALL), + Sources: []string{"g-src"}, + }}, + } + + c := &types.NetworkMapComponents{ + PeerID: "peer-R", + Network: &nmdata.Network{ + Identifier: "net-routed-resource", + Net: net.IPNet{IP: net.IP{100, 64, 0, 0}, Mask: net.CIDRMask(10, 32)}, + Serial: 1, + }, + AccountSettings: &nmdata.AccountSettingsInfo{}, + DNSSettings: &nmdata.DNSSettings{}, + Peers: peers, + Groups: map[string]*nmdata.Group{ + "g-src": {PublicID: "1", Name: "sources", Peers: []string{"peer-S"}}, + "g-routers": {PublicID: "2", Name: "routers", Peers: []string{"peer-R"}}, + }, + NetworkResources: []*nmdata.NetworkResource{{ + ID: "res-1", NetworkID: "netid-1", PublicID: "100", Name: "res1", + Type: "subnet", + Prefix: netip.MustParsePrefix("10.200.0.0/24"), + Enabled: true, + }}, + RoutersMap: map[string]map[string]*nmdata.NetworkRouter{ + "netid-1": {"peer-R": { + PublicID: "200", PeerGroups: []string{"g-routers"}, Metric: 9999, Enabled: true, + }}, + }, + ResourcePoliciesMap: map[string][]*nmdata.Policy{ + "res-1": {resourcePolicy}, + }, + Policies: []*nmdata.Policy{resourcePolicy}, + NetworkXIDToPublicID: map[string]string{"netid-1": "1"}, + } + + return c, routerKey +} diff --git a/shared/management/types/networkmap_components.go b/shared/management/types/networkmap_components.go index e18db4ec0..7742ca244 100644 --- a/shared/management/types/networkmap_components.go +++ b/shared/management/types/networkmap_components.go @@ -58,6 +58,13 @@ type NetworkMapComponents struct { // domain targets. ForceRoutingPeerDNSResolution bool + // SkipRouteFirewallRules drops the route firewall rule computation from + // Calculate. A receiver without a firewall manager never reads + // RoutesFirewallRules, and on a routing peer with many network resources + // building them dominates the cost of a sync. Defaults to false so the + // management server keeps producing them. + SkipRouteFirewallRules bool + routesByPeerOnce sync.Once routesByPeerIdx map[string][]routeIndexEntry @@ -149,11 +156,15 @@ func (c *NetworkMapComponents) Calculate(ctx context.Context) *NetworkMap { includeIPv6 = p.SupportsIPv6() && p.IPv6.IsValid() } routesUpdate := filterAndExpandRoutes(c.getRoutesToSync(targetPeerID, peersToConnect, peerGroups), includeIPv6) - routesFirewallRules := c.getPeerRoutesFirewallRules(ctx, targetPeerID, includeIPv6) + + var routesFirewallRules []*RouteFirewallRule + if !c.SkipRouteFirewallRules { + routesFirewallRules = c.getPeerRoutesFirewallRules(ctx, targetPeerID, includeIPv6) + } isRouter, networkResourcesRoutes, sourcePeers := c.getNetworkResourcesRoutesToSync(targetPeerID) var networkResourcesFirewallRules []*RouteFirewallRule - if isRouter { + if isRouter && !c.SkipRouteFirewallRules { networkResourcesFirewallRules = c.getPeerNetworkResourceFirewallRules(ctx, targetPeerID, networkResourcesRoutes, includeIPv6) }