From 6620219939739eb298a3f98fb43ece7f9306450f Mon Sep 17 00:00:00 2001 From: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:18:49 +0200 Subject: [PATCH 1/5] [client] Add catch-all NRPT rule when NetBird is the primary DNS resolver (#7071) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [client] Add catch-all NRPT rule when NetBird is the primary DNS resolver * Remove obvious comments * Install the catch-all rule where the adapter's DNS is set addDNSSetupForAll makes us the peer's main DNS forwarder, and the catch-all NRPT rule is the other half of that same job: without it the adapter's NameServer only adds one more resolver to the set Windows queries in parallel. Having the two in one place says that, where a separate block at the end of applyDNSConfig read as an afterthought. The block could not simply move up: removeDNSMatchPolicies deletes the catch-all key too, so installing the rule before it ran would have had the rule deleted moments later. The cleanup now runs first, which is what it was always for - it clears what the previous apply installed before this one installs anything - and keeps being unconditional, so a leftover rule from an earlier run cannot survive into a config that no longer wants it. * Name the escape hatch after the behaviour it restores NB_DISABLE_DNS_CATCHALL_NRPT described the mechanism it switches off. What an operator reaching for it wants is the behaviour they had before, so name it that: NB_USE_LEGACY_DNS_RESOLUTION, matching NB_USE_LEGACY_ROUTING, the only other legacy switch in the client. Not NB_WIN_LEGACY_FULL_TUNNEL_DNS_RESOLVE, as first suggested: the rule follows a primary nameserver group, not a full tunnel, and putting FULL_TUNNEL in a public variable name would carry that confusion for as long as the variable lives. No OS prefix either, since nothing else in the client has one and this switch is inert anywhere but Windows by construction. Behaviour and default are unchanged: the catch-all rule is on unless the variable says otherwise. * Exempt .local from the catch-all rule RFC 6762 reserves .local for multicast DNS and says unicast resolvers must not answer for it. The catch-all rule hands it to us anyway, we forward it to whatever upstream the primary nameserver group points at, and the answer comes back NXDOMAIN for hosts that do exist - printers, NAS boxes, anything announcing itself on the link. Confirmed on a Win11Pro VM: laptop.local resolves with the client down and returns "Nome DNS inesistente" with it up, and the client log shows the query arriving on the catch-all handler and being forwarded to 1.1.1.1. An NRPT rule that names a namespace and lists no servers is an exemption: the DNS client resolves those names as it would with no rule at all. What that looks like in the registry is not what it sounds like. Writing no server value and clearing ConfigOptions produces a rule Windows treats as a no-op - it never appears in Get-DnsClientNrptPolicy -Effective and the catch-all keeps the query. The value has to be present and empty, with ConfigOptions still 0x8: the flag says the server list is the meaningful part of the rule, and an empty list then means "no server, resolve normally". Verified both encodings on the VM. Installed together with the catch-all, since without one nothing captures .local in the first place, and removed with it. Exclusivity is unaffected elsewhere, and a more specific rule still wins - a match domain under .local keeps resolving through NetBird, which is what a legacy Active Directory domain named corp.local needs. Verified separately that a match domain does take precedence over the catch-all: declaring fritz.box against the local router restored laptop.fritz.box while the catch-all was in force. * Treat the root namespace as a match domain, not a special case The catch-all had a function, a registry key and a call site of its own, which made it look like a different mechanism. It is not: "." is an NRPT namespace like any other, it just happens to match every name. So it goes into the match domain list, and addDNSMatchPolicy writes it along with the rest — batching, GPO variant, volatile keys and cleanup all come for free. The .local exemption stays a rule of its own, and not for symmetry: it is the one rule with a different server list, an empty one. Putting it in the same Name value would give it our resolver and exempt nothing. Windows expands a rule's Name value into one effective namespace each, so a rule carrying {.example.com, .} still shows both as separate rows in Get-DnsClientNrptPolicy -Effective. Nothing is lost for diagnosis by dropping the dedicated key. Suggested by Vik in review. * Do not report a failed NRPT cleanup as success removeRegistryKeyFromDNSPolicyConfig returned nil for every OpenKey error, so a permission or registry failure was indistinguishable from a key that was never there. Cleanup then reported success while the rule stayed in force — which is how a rule outlives the interface it points at and keeps sending every query to an address that no longer answers. Distinguish the two, the way listNRPTRuleKeys already does for the policy store root: a missing key is nothing to do, anything else reaches the caller. restoreHostDNS now propagates that error instead of logging it. applyDNSConfig keeps logging on purpose: there we are about to write fresh rules over whatever survived, while restore is the path where a rule left behind is the whole problem. Also addresses review nits on the tests: doc comments on the two added cases, reported Close and DeleteKey errors so a failed cleanup cannot contaminate the next registry test, and a context message on the exemption's namespace assertion. --- client/internal/dns/host_windows.go | 134 ++++++++++++++++++++-- client/internal/dns/host_windows_test.go | 139 +++++++++++++++++++++++ 2 files changed, 261 insertions(+), 12 deletions(-) diff --git a/client/internal/dns/host_windows.go b/client/internal/dns/host_windows.go index 53380b2aa..948000a3d 100644 --- a/client/internal/dns/host_windows.go +++ b/client/internal/dns/host_windows.go @@ -6,8 +6,10 @@ import ( "fmt" "io" "net/netip" + "os" "os/exec" "slices" + "strconv" "strings" "syscall" "time" @@ -34,10 +36,16 @@ var ( // Registry locations of the host DNS configuration this package programs, // exported so a diagnostic reader reports the same locations that are written. const ( - // NRPTKeyPrefix starts the name of every NRPT rule key this client creates. - // Older versions used different layouts under the same prefix: a single - // unsuffixed key, then one key per domain, now one key per batch of domains. - NRPTKeyPrefix = "NetBird-Match" + // NRPTKeyPrefix starts the name of every NRPT rule key this client creates: + // the match rules, the catch-all, and the .local exemption. Cleanup + // enumerates by this prefix, so a new kind of rule is removed by existing + // code as long as its key starts here. + NRPTKeyPrefix = "NetBird-" + + // nrptMatchKeyName names the match-domain rules. Older versions used + // different layouts under the same name: a single unsuffixed key, then one + // key per domain, now one key per batch of domains. + nrptMatchKeyName = NRPTKeyPrefix + "Match" // DNSPolicyConfigRoot holds the NRPT rules of the local policy store. DNSPolicyConfigRoot = `SYSTEM\CurrentControlSet\Services\Dnscache\Parameters\DnsPolicyConfig` @@ -53,8 +61,24 @@ const ( ) const ( - dnsPolicyConfigMatchPath = DNSPolicyConfigRoot + `\` + NRPTKeyPrefix - gpoDnsPolicyConfigMatchPath = GPODNSPolicyConfigRoot + `\` + NRPTKeyPrefix + dnsPolicyConfigMatchPath = DNSPolicyConfigRoot + `\` + nrptMatchKeyName + gpoDnsPolicyConfigMatchPath = GPODNSPolicyConfigRoot + `\` + nrptMatchKeyName + + dnsPolicyConfigExemptLocalPath = DNSPolicyConfigRoot + `\` + NRPTKeyPrefix + `ExemptLocal` + gpoDnsPolicyConfigExemptLocalPath = GPODNSPolicyConfigRoot + `\` + NRPTKeyPrefix + `ExemptLocal` + + nrptCatchAllNamespace = "." + // nrptLocalNamespace is reserved for multicast DNS by RFC 6762: a unicast + // resolver must not answer for it. The catch-all rule would hand it to us + // anyway, so it gets an exemption rule of its own. + nrptLocalNamespace = ".local" + + // envLegacyDNSResolution restores the pre-catch-all behaviour: the adapter's + // NameServer alone, leaving the OS free to query other adapters' resolvers in + // parallel. An escape hatch for setups that depend on a resolver of theirs + // still being reachable while connected, at the cost of the leak and of the + // race the catch-all rule exists to close. + envLegacyDNSResolution = "NB_USE_LEGACY_DNS_RESOLUTION" dnsPolicyConfigVersionKey = "Version" dnsPolicyConfigVersionValue = 2 @@ -293,6 +317,13 @@ func (r *registryConfigurator) disableWINSForInterface() error { } func (r *registryConfigurator) applyDNSConfig(config HostDNSConfig, stateManager *statemanager.Manager) error { + // Clear every rule the previous apply installed before installing any new + // one, including a leftover catch-all: removal is unconditional so a rule + // from an earlier run cannot survive into a config that no longer wants it. + if err := r.removeDNSMatchPolicies(); err != nil { + log.Errorf("cleanup old dns match policies: %s", err) + } + if config.RouteAll { if err := r.addDNSSetupForAll(config.ServerIP); err != nil { return fmt.Errorf("add dns setup: %w", err) @@ -318,8 +349,22 @@ func (r *registryConfigurator) applyDNSConfig(config HostDNSConfig, stateManager matchDomains = append(matchDomains, "."+strings.TrimSuffix(dConf.Domain, ".")) } - if err := r.removeDNSMatchPolicies(); err != nil { - log.Errorf("cleanup old dns match policies: %s", err) + // The root namespace is a match domain like any other: it just happens to + // match every name. Without it the adapter's NameServer only adds one more + // resolver to the set Windows queries in parallel, keeping whichever answer + // comes back first — which leaks every query to the local network and lets a + // resolver other than ours answer for a name we are authoritative for. + if config.RouteAll { + if parseBoolEnv(envLegacyDNSResolution) { + log.Infof("%s is set, leaving DNS resolution shared with the other adapters' resolvers instead of forcing it through %s", envLegacyDNSResolution, config.ServerIP) + } else { + matchDomains = append(matchDomains, nrptCatchAllNamespace) + log.Infof("routing every namespace through %s: DNS resolution is now exclusive to NetBird", config.ServerIP) + + if err := r.addDNSExemptLocalPolicy(); err != nil { + return fmt.Errorf("add dns exempt policy: %w", err) + } + } } if len(matchDomains) != 0 { @@ -397,6 +442,42 @@ func (r *registryConfigurator) addDNSMatchPolicy(domains []string, ip netip.Addr return nil } +// addDNSExemptLocalPolicy carves .local back out of the catch-all. RFC 6762 +// reserves it for multicast DNS, so forwarding those names to a unicast +// upstream answers NXDOMAIN for hosts that do exist - printers, NAS boxes, and +// anything else announcing itself on the link - and the answer is authoritative +// enough that Windows stops looking. A rule naming the namespace with no +// servers hands it back to the DNS client untouched. A more specific rule still +// wins, so a match domain under .local keeps going through us. +func (r *registryConfigurator) addDNSExemptLocalPolicy() error { + var noServers netip.Addr + + if err := r.configureDNSPolicy(dnsPolicyConfigExemptLocalPath, []string{nrptLocalNamespace}, noServers); err != nil { + return fmt.Errorf("configure exempt policy for %s: %w", nrptLocalNamespace, err) + } + + if r.gpo { + if err := r.configureDNSPolicy(gpoDnsPolicyConfigExemptLocalPath, []string{nrptLocalNamespace}, noServers); err != nil { + return fmt.Errorf("configure gpo exempt policy for %s: %w", nrptLocalNamespace, err) + } + if err := refreshGroupPolicy(); err != nil { + log.Warnf("failed to refresh group policy: %v", err) + } + } + + log.Infof("added NRPT exemption for %s, leaving it to the OS resolver", nrptLocalNamespace) + return nil +} + +// configureDNSPolicy writes one NRPT rule. An invalid ip writes an exemption +// rule: the namespace with an empty server list, which tells the DNS client to +// resolve those names the way it would without any rule at all. +// +// The empty string is the whole difference, and it has to be written: dropping +// the value and clearing ConfigOptions instead produces a rule Windows treats +// as a no-op, keeps out of Get-DnsClientNrptPolicy -Effective, and ignores in +// favour of the catch-all. 0x8 says the server list is the meaningful part of +// the rule, and an empty list then means "no server, resolve normally". func (r *registryConfigurator) configureDNSPolicy(policyPath string, domains []string, ip netip.Addr) error { if err := removeRegistryKeyFromDNSPolicyConfig(policyPath); err != nil { return fmt.Errorf("remove existing dns policy: %w", err) @@ -416,7 +497,11 @@ func (r *registryConfigurator) configureDNSPolicy(policyPath string, domains []s return fmt.Errorf("set %s: %w", dnsPolicyConfigNameKey, err) } - if err := regKey.SetStringValue(dnsPolicyConfigGenericDNSServersKey, ip.String()); err != nil { + var servers string + if ip.IsValid() { + servers = ip.String() + } + if err := regKey.SetStringValue(dnsPolicyConfigGenericDNSServersKey, servers); err != nil { return fmt.Errorf("set %s: %w", dnsPolicyConfigGenericDNSServersKey, err) } @@ -514,8 +599,11 @@ func (r *registryConfigurator) getInterfaceRegistryKey() (registry.Key, error) { } func (r *registryConfigurator) restoreHostDNS() error { + // Propagated, unlike in applyDNSConfig: there we are about to write fresh + // rules over whatever survived, here we are leaving, and a rule left behind + // keeps sending every query to an address that is about to disappear. if err := r.removeDNSMatchPolicies(); err != nil { - log.Errorf("remove dns match policies: %s", err) + return fmt.Errorf("remove dns match policies: %w", err) } if err := r.deleteInterfaceRegistryKeyProperty(interfaceConfigSearchListKey); err != nil { @@ -598,9 +686,17 @@ func listNRPTRuleKeys(root string) ([]string, error) { func removeRegistryKeyFromDNSPolicyConfig(regKeyPath string) error { k, err := registry.OpenKey(registry.LOCAL_MACHINE, regKeyPath, registry.QUERY_VALUE) - if err != nil { - log.Debugf("failed to open HKEY_LOCAL_MACHINE\\%s: %v", regKeyPath, err) + switch { + case errors.Is(err, registry.ErrNotExist), errors.Is(err, syscall.ERROR_PATH_NOT_FOUND): + // nothing to remove, which is the normal case for a rule this config + // never installed + log.Debugf("HKEY_LOCAL_MACHINE\\%s does not exist", regKeyPath) return nil + case err != nil: + // anything else has to reach the caller: reporting success here would + // leave the rule in force while claiming it was removed, which is how a + // stale rule outlives the interface it points at + return fmt.Errorf("open HKEY_LOCAL_MACHINE\\%s: %w", regKeyPath, err) } closer(k) @@ -636,6 +732,20 @@ func refreshGroupPolicy() error { return nil } +func parseBoolEnv(key string) bool { + val := os.Getenv(key) + if val == "" { + return false + } + + parsed, err := strconv.ParseBool(val) + if err != nil { + log.Warnf("failed to parse %s=%q: %v", key, val, err) + return false + } + return parsed +} + func closer(closer io.Closer) { if err := closer.Close(); err != nil { log.Errorf("failed to close: %s", err) diff --git a/client/internal/dns/host_windows_test.go b/client/internal/dns/host_windows_test.go index 861613c95..7aef64590 100644 --- a/client/internal/dns/host_windows_test.go +++ b/client/internal/dns/host_windows_test.go @@ -94,6 +94,145 @@ func TestNRPTEntriesCleanupOnConfigChange(t *testing.T) { assert.False(t, exists, "NRPT rule 2 should NOT exist after reducing to 75 domains") } +// TestNRPTCatchAllRule verifies that RouteAll adds the root namespace to the +// match rule instead of a rule of its own, that .local is carved back out with +// an empty server list, and that both go away when RouteAll is cleared or the +// host DNS is restored. +func TestNRPTCatchAllRule(t *testing.T) { + if testing.Short() { + t.Skip("skipping registry integration test in short mode") + } + + defer cleanupRegistryKeys(t) + cleanupRegistryKeys(t) + + testIP := netip.MustParseAddr("100.64.0.1") + testGUID := "{12345678-1234-1234-1234-123456789ABC}" + interfacePath := InterfaceConfigPath + `\` + testGUID + testKey, _, err := registry.CreateKey(registry.LOCAL_MACHINE, interfacePath, registry.SET_VALUE) + require.NoError(t, err, "Should create test interface registry key") + require.NoError(t, testKey.Close(), "close test interface registry key") + defer func() { + assert.NoError(t, registry.DeleteKey(registry.LOCAL_MACHINE, interfacePath), "delete test interface registry key") + }() + + cfg := ®istryConfigurator{guid: testGUID} + + matchOnly := HostDNSConfig{ + ServerIP: testIP, + Domains: []DomainConfig{{Domain: "example.com", MatchOnly: true}}, + } + primary := HostDNSConfig{ + ServerIP: testIP, + RouteAll: true, + Domains: []DomainConfig{{Domain: "example.com", MatchOnly: true}}, + } + firstRule := fmt.Sprintf("%s-0", dnsPolicyConfigMatchPath) + + // The root namespace is not a rule of its own: it rides in the match rule, + // which is the point of it not being a special case. + require.NoError(t, cfg.applyDNSConfig(matchOnly, nil)) + names := ruleNamespaces(t, firstRule) + assert.Contains(t, names, ".example.com") + assert.NotContains(t, names, nrptCatchAllNamespace, "a match-only config must not claim every namespace") + + require.NoError(t, cfg.applyDNSConfig(primary, nil)) + names = ruleNamespaces(t, firstRule) + assert.Contains(t, names, ".example.com") + assert.Contains(t, names, nrptCatchAllNamespace, "RouteAll should add the root namespace to the match rule") + + k, err := registry.OpenKey(registry.LOCAL_MACHINE, firstRule, registry.QUERY_VALUE) + require.NoError(t, err) + servers, _, err := k.GetStringValue(dnsPolicyConfigGenericDNSServersKey) + require.NoError(t, err) + assert.Equal(t, testIP.String(), servers, "every namespace in the rule resolves through our resolver") + require.NoError(t, k.Close(), "close match rule key") + + // .local is carved back out: RFC 6762 reserves it for mDNS, so it needs a + // rule of its own — it is the one rule with a different server list. + ek, err := registry.OpenKey(registry.LOCAL_MACHINE, dnsPolicyConfigExemptLocalPath, registry.QUERY_VALUE) + require.NoError(t, err, "exemption rule should exist once the root namespace is claimed") + + exemptNames, _, err := ek.GetStringsValue(dnsPolicyConfigNameKey) + require.NoError(t, err) + assert.Equal(t, []string{nrptLocalNamespace}, exemptNames, "the exemption should name only the mDNS namespace") + + exemptServers, _, err := ek.GetStringValue(dnsPolicyConfigGenericDNSServersKey) + require.NoError(t, err, "the value has to be present, empty: without it Windows drops the rule") + assert.Empty(t, exemptServers, "an exemption rule lists no servers") + + exemptOpts, _, err := ek.GetIntegerValue(dnsPolicyConfigConfigOptionsKey) + require.NoError(t, err) + assert.EqualValues(t, dnsPolicyConfigConfigOptionsValue, exemptOpts, "same options as a normal rule; the empty server list is what makes it an exemption") + require.NoError(t, ek.Close(), "close exemption rule key") + + require.NoError(t, cfg.applyDNSConfig(matchOnly, nil)) + names = ruleNamespaces(t, firstRule) + assert.NotContains(t, names, nrptCatchAllNamespace, "clearing RouteAll should drop the root namespace") + + exists, err := registryKeyExists(dnsPolicyConfigExemptLocalPath) + require.NoError(t, err) + assert.False(t, exists, "exemption rule should go with the namespace it carves out of") + + require.NoError(t, cfg.applyDNSConfig(primary, nil)) + require.NoError(t, cfg.restoreHostDNS()) + exists, err = registryKeyExists(firstRule) + require.NoError(t, err) + assert.False(t, exists, "restore should leave no rule behind") +} + +// ruleNamespaces returns the namespaces an NRPT rule key claims. +func ruleNamespaces(t *testing.T, path string) []string { + t.Helper() + k, err := registry.OpenKey(registry.LOCAL_MACHINE, path, registry.QUERY_VALUE) + require.NoError(t, err, "rule key %s should exist", path) + defer k.Close() + + names, _, err := k.GetStringsValue(dnsPolicyConfigNameKey) + require.NoError(t, err) + return names +} + +// TestNRPTCatchAllRuleLegacyEnv verifies that NB_USE_LEGACY_DNS_RESOLUTION +// leaves the root namespace unclaimed, so no rule is written for a RouteAll +// config that carries no match domains. +func TestNRPTCatchAllRuleLegacyEnv(t *testing.T) { + if testing.Short() { + t.Skip("skipping registry integration test in short mode") + } + + defer cleanupRegistryKeys(t) + cleanupRegistryKeys(t) + + t.Setenv(envLegacyDNSResolution, "true") + + testGUID := "{12345678-1234-1234-1234-123456789ABC}" + interfacePath := InterfaceConfigPath + `\` + testGUID + testKey, _, err := registry.CreateKey(registry.LOCAL_MACHINE, interfacePath, registry.SET_VALUE) + require.NoError(t, err, "Should create test interface registry key") + require.NoError(t, testKey.Close(), "close test interface registry key") + defer func() { + assert.NoError(t, registry.DeleteKey(registry.LOCAL_MACHINE, interfacePath), "delete test interface registry key") + }() + + cfg := ®istryConfigurator{guid: testGUID} + config := HostDNSConfig{ + ServerIP: netip.MustParseAddr("100.64.0.1"), + RouteAll: true, + } + + require.NoError(t, cfg.applyDNSConfig(config, nil)) + + // RouteAll with no match domains and the switch set leaves nothing to write. + exists, err := registryKeyExists(fmt.Sprintf("%s-0", dnsPolicyConfigMatchPath)) + require.NoError(t, err) + assert.False(t, exists, "no rule should be written when the legacy env var is set") + + exists, err = registryKeyExists(dnsPolicyConfigExemptLocalPath) + require.NoError(t, err) + assert.False(t, exists, "no exemption without a claimed root namespace") +} + func registryKeyExists(path string) (bool, error) { k, err := registry.OpenKey(registry.LOCAL_MACHINE, path, registry.QUERY_VALUE) if err != nil { From 89c6e84a41486469c3244d7caed56d3e5db3e1c8 Mon Sep 17 00:00:00 2001 From: Zoltan Papp Date: Fri, 28 Aug 2026 09:33:08 +0200 Subject: [PATCH 2/5] [client, ios] Fix context cancellation during restart (#7329) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mobile): stop the client synchronously so a restart cannot inherit a cancelled context Original finding ---------------- A user reported that leaving home and switching from wifi to cellular killed all Internet traffic until NetBird was turned off. A debug bundle captured the failure (iOS, CLI 0.75.0, self-hosted management, generated 2026-08-18 01:17; the incident is at 2026-08-17 22:37:38-51 UTC). The bundle shows the whole sequence: 22:37:38.255 management sync stream drops (keepalive ACK timeout) 22:37:43.670 Swift: "Network type changed: wifi -> cellular" -> schedules a restart with a 1s debounce 22:37:44.737 Go: "ensuring wg interface is removed, Netbird engine context cancelled" - engineCtx dies, every peer gets context canceled 22:37:49.910 iface.go:238 "failed to remove WireGuard interface utun6: timeout when waiting for interface utun6 to be removed" -> the teardown stretches out for ~5s 22:37:50.710 Swift: "restartClient: starting client", needsLogin=false (so this is NOT a login expiry) 22:37:51.013 Go: connect.go:476 "exiting client retry loop due to unrecoverable error: context canceled" - the OLD run dies here 22:37:51.333 Go: grpc.go:135 "failed creating connection to Management Service: context canceled" - the NEW start, 2ms after the old run finally exited 22:37:51.334 Swift: "restartClient: start failed" -> widget disconnected then nothing for 15 minutes The tunnel stayed installed with no engine behind it, so every packet was black-holed. status.txt, generated ~14 hours later, still reads Management: Disconnected / Signal: Disconnected / Peers count: 0/0 - the client never recovered on its own. Root cause ---------- Client.Stop() cancelled a shared ctxCancel field and returned immediately, without waiting for the run loop to exit. The Swift stop{} completion handler therefore fired while the Go teardown was still running (stretched out by the utun6 removal timeout), and the start that followed landed on a context that the outgoing run was about to cancel. Two further paths wrote the same shared field. IsLoginRequired() and LoginForMobile() each overwrote c.ctxCancel, so any call to them during a live session discarded the running engine's cancel function. restartClient() calls needsLoginCached() on exactly this path. Changes ------- - Stop() now drives the stored ConnectClient: ConnectClient.Stop() cancels the run context and blocks on runExited, so the caller's completion handler only fires once the run loop has really finished. The ctxCancel path stays as a fallback for when no ConnectClient exists yet (e.g. during LoginForMobile). - Run() owns its cancel in a local variable, so a concurrent call that overwrites the shared field can no longer cancel this run's context through the deferred cleanup. - IsLoginRequired() and LoginForMobile() use local cancels and leave the shared field alone. LoginForMobile's cancel moves into the deferred cleanup of the goroutine that outlives the call, so the OAuth token wait is not cut short. - The Android SDK gets the same treatment. The structural defect is identical there, but the trigger is absent: Android has no automatic engine restart on a network type change, and no interface-removal timeout to stretch the teardown. This part is preventive, not a fix for an observed failure. * fix(mobile): do not let a superseded startup publish its client Review found a window the previous commit left open. Run stored its cancel function and only published the ConnectClient later, after loading config and constructing the client. A Stop landing inside that window found no ConnectClient, cancelled the run and returned immediately. A new Run could then publish its own client, and the cancelled older run — still executing — would overwrite it with a client that was already being torn down. The next Stop stopped that stale client and left the live one running with nothing tracking it. Runs now carry a generation. Run claims one before doing any work and publishes its client only while the generation is still current; a superseded run returns without touching the shared state. Stop bumps the generation, so any startup still in flight is invalidated, then cancels it and waits for the run to exit before returning (20s cap so a wedged teardown cannot block the caller forever). setState is gone: publishState replaces it at both call sites on each platform. * fix(ios): add a non-waiting Stop for callers on a deadline Stop now waits for the run loop to exit, which is what a restart needs but wrong for stopTunnel: iOS gives NEPacketTunnelProvider only a few seconds there before it kills the extension, and the wait can run to its 20s cap. Waiting past the deadline earns a SIGKILL, so the next start inherits a dirty state instead of the orderly shutdown the wait was meant to buy. StopWithoutWait tears the client down and returns. ConnectClient.Stop blocks on runExited with no cap of its own, so the non-waiting path runs it detached rather than only skipping the runDone wait. Android keeps a single blocking Stop: it has no equivalent deadline. * fix(mobile): guard the run lifecycle with a single lock Stop and beginRun each touched the same lifecycle state across two locks in sequence: take stateMu, release it, then take ctxCancelLock. A run starting in that gap installed its own cancel before Stop reached it, so Stop cancelled the fresh run and left its own target running — the same class of defect this branch exists to fix, this time in the locking rather than the state. ctxCancel moves into the stateMu group, and both sides take their snapshot in one critical section. ctxCancelLock then guarded nothing and is gone. * fix(mobile): drop the run-generation machinery for a serialized lifecycle The platform callers (Swift/Kotlin) always stop before starting and coalesce restarts, so the generation counter guarded against call patterns that cannot occur. Replace it with a single-run contract: - startRun refuses a second Run while the previous one has not exited - finishRun clears the published state on every exit path, including errors - Stop cancels and waits for the run loop with a bounded timeout; it no longer calls ConnectClient.Stop, whose wait is unbounded - concurrent Stops wait on the same exit channel instead of returning early - a superseded startup no longer reports a clean nil exit * revert(android): drop the run lifecycle changes Android does not have the defect this PR fixes. On ux/ios-style-redesign the EngineRestarter is gone: network changes are handled as events instead of an engine restart, so nothing stops the client and starts it again. The remaining stop() callers are all final teardowns on the main thread with a framework deadline - the stop-engine broadcast receiver, onDestroy, onRevoke and the binder's stopEngine. A Stop that waits for the run loop would risk an ANR there for a race that cannot occur, so the fix stays iOS-only. * fix(ios): make loginComplete race-free The OAuth goroutine spawned by LoginForMobile sets loginComplete after the call has returned to Swift, while the Swift side polls IsLoginComplete and later calls ClearLoginComplete from its own thread. The plain bool made all three unsynchronized: the store may never become visible to the poller, and a Clear racing the store can be lost, leaving a stale true that makes the next login look already complete. Switch the field to atomic.Bool. It is a standalone flag rather than part of the run lifecycle that stateMu guards, and it has to stay readable while the login goroutine is still in flight. --- client/ios/NetBirdSDK/client.go | 122 ++++++++++++++++++++++++-------- 1 file changed, 93 insertions(+), 29 deletions(-) diff --git a/client/ios/NetBirdSDK/client.go b/client/ios/NetBirdSDK/client.go index 8373e498a..bbbb969c9 100644 --- a/client/ios/NetBirdSDK/client.go +++ b/client/ios/NetBirdSDK/client.go @@ -4,12 +4,14 @@ package NetBirdSDK import ( "context" + "errors" "fmt" "net/netip" "os" "sort" "strings" "sync" + "sync/atomic" "time" log "github.com/sirupsen/logrus" @@ -37,6 +39,8 @@ const ( AnonymizeLevelStrict = nbAnonymize.LevelStrictString ) +var errClientAlreadyRunning = errors.New("client is already running") + // RouteListener export internal RouteListener for mobile type NetworkChangeListener interface { listener.NetworkChangeListener @@ -74,15 +78,13 @@ type Client struct { cacheDir string logFilePath string recorder *peer.Status - ctxCancel context.CancelFunc - ctxCancelLock *sync.Mutex deviceName string osName string osVersion string networkChangeListener listener.NetworkChangeListener onHostDnsFn func([]string) dnsManager dns.IosDnsManager - loginComplete bool + loginComplete atomic.Bool // netMgr outlives engine restarts: it mirrors the OS connectivity, not // the engine lifecycle. Run injects its state and sweeper into each new // ConnectClient. @@ -90,9 +92,16 @@ type Client struct { // preloadedConfig holds config loaded from JSON (used on tvOS where file writes are blocked) preloadedConfig *profilemanager.Config + // stateMu guards the run lifecycle as one unit: the cancel installed by + // the current run, the channel it closes on exit, and the state it + // published. One run at a time: startRun refuses a second Run while the + // previous one has not exited, and the platform serializes Stop before + // Start, so no generation tracking is needed. stateMu sync.RWMutex connectClient *internal.ConnectClient config *profilemanager.Config + runDone chan struct{} + ctxCancel context.CancelFunc } // NewClient instantiate a new Client @@ -107,7 +116,6 @@ func NewClient(cfgFile, stateFile, cacheDir, logFilePath, deviceName string, osV osName: osName, osVersion: osVersion, recorder: recorder, - ctxCancelLock: &sync.Mutex{}, networkChangeListener: networkChangeListener, dnsManager: dnsManager, netMgr: netevents.NewManager(recorder), @@ -156,17 +164,21 @@ func (c *Client) Run(fd int32, interfaceName string, envList *EnvList) error { c.recorder.UpdateManagementAddress(cfg.ManagementURL.String()) c.recorder.UpdateRosenpass(cfg.RosenpassEnabled, cfg.RosenpassPermissive) - var ctx context.Context //nolint ctxWithValues := context.WithValue(context.Background(), system.DeviceNameCtxKey, c.deviceName) //nolint ctxWithValues = context.WithValue(ctxWithValues, system.OsNameCtxKey, c.osName) //nolint ctxWithValues = context.WithValue(ctxWithValues, system.OsVersionCtxKey, c.osVersion) - c.ctxCancelLock.Lock() - ctx, c.ctxCancel = context.WithCancel(ctxWithValues) - defer c.ctxCancel() - c.ctxCancelLock.Unlock() + runCtx, runCancel := context.WithCancel(ctxWithValues) + defer runCancel() + + done, err := c.startRun(runCancel) + if err != nil { + return err + } + defer c.finishRun(done) + ctx := runCtx // No login pre-flight here. The engine's own loginToManagement (connect.go) performs // the authoritative Login immediately before the first Sync, so a LoginSync() call at @@ -215,16 +227,40 @@ func (c *Client) NotifyNetworkChange() { c.netMgr.NotifyNetworkChange() } -// Stop the internal client and free the resources +// Stop cancels the running client and waits for the run loop to exit, so a +// caller that restarts immediately cannot race the outgoing teardown. func (c *Client) Stop() { - c.ctxCancelLock.Lock() - defer c.ctxCancelLock.Unlock() - if c.ctxCancel == nil { + done := c.cancelRun() + if done == nil { return } - c.ctxCancel() - c.setState(nil, nil) + select { + case <-done: + case <-time.After(stopRunWaitTimeout): + log.Warnf("Stop: timed out waiting for the run loop to exit") + } +} + +// StopWithoutWait cancels the running client without waiting for the run loop. +// Use it where the caller is on a deadline the wait could overrun, such as +// NEPacketTunnelProvider.stopTunnel, which iOS gives only a few seconds +// before it kills the extension. +func (c *Client) StopWithoutWait() { + c.cancelRun() +} + +func (c *Client) cancelRun() chan struct{} { + c.stateMu.RLock() + done := c.runDone + cancel := c.ctxCancel + c.stateMu.RUnlock() + + if cancel != nil { + cancel() + } + + return done } // DebugBundle generates a debug bundle, uploads it and returns the upload key. @@ -376,16 +412,14 @@ func (c *Client) IsLoginRequiredCached() bool { } func (c *Client) IsLoginRequired() bool { - var ctx context.Context //nolint ctxWithValues := context.WithValue(context.Background(), system.DeviceNameCtxKey, c.deviceName) //nolint ctxWithValues = context.WithValue(ctxWithValues, system.OsNameCtxKey, c.osName) //nolint ctxWithValues = context.WithValue(ctxWithValues, system.OsVersionCtxKey, c.osVersion) - c.ctxCancelLock.Lock() - defer c.ctxCancelLock.Unlock() - ctx, c.ctxCancel = context.WithCancel(ctxWithValues) + ctx, cancel := context.WithCancel(ctxWithValues) + defer cancel() var cfg *profilemanager.Config var err error @@ -433,17 +467,22 @@ func (c *Client) IsLoginRequired() bool { // loginForMobileAuthTimeout is the timeout for requesting auth info from the server const loginForMobileAuthTimeout = 30 * time.Second +const stopRunWaitTimeout = 20 * time.Second + func (c *Client) LoginForMobile() string { - var ctx context.Context //nolint ctxWithValues := context.WithValue(context.Background(), system.DeviceNameCtxKey, c.deviceName) //nolint ctxWithValues = context.WithValue(ctxWithValues, system.OsNameCtxKey, c.osName) //nolint ctxWithValues = context.WithValue(ctxWithValues, system.OsVersionCtxKey, c.osVersion) - c.ctxCancelLock.Lock() - defer c.ctxCancelLock.Unlock() - ctx, c.ctxCancel = context.WithCancel(ctxWithValues) + ctx, cancel := context.WithCancel(ctxWithValues) + loginDone := false + defer func() { + if !loginDone { + cancel() + } + }() // Use DirectUpdateOrCreateConfig to avoid atomic file operations (temp file + rename) // which are blocked by the tvOS sandbox in App Group containers @@ -470,7 +509,9 @@ func (c *Client) LoginForMobile() string { } // This could cause a potential race condition with loading the extension which need to be handled on swift side + loginDone = true go func() { + defer cancel() tokenInfo, err := oAuthFlow.WaitToken(ctx, flowInfo) if err != nil { log.Errorf("LoginForMobile: WaitToken failed: %v", err) @@ -487,18 +528,18 @@ func (c *Client) LoginForMobile() string { log.Errorf("LoginForMobile: Login failed: %v", err) return } - c.loginComplete = true + c.loginComplete.Store(true) }() return flowInfo.VerificationURIComplete } func (c *Client) IsLoginComplete() bool { - return c.loginComplete + return c.loginComplete.Load() } func (c *Client) ClearLoginComplete() { - c.loginComplete = false + c.loginComplete.Store(false) } func (c *Client) GetRoutesSelectionDetails() (*RoutesSelectionDetails, error) { @@ -718,13 +759,36 @@ func (c *Client) DeselectRoute(id string) error { return nil } -// setState stores the running engine state so DebugBundle can reuse the live -// config and ConnectClient. It is cleared on Stop. -func (c *Client) setState(cfg *profilemanager.Config, cc *internal.ConnectClient) { +func (c *Client) startRun(cancel context.CancelFunc) (chan struct{}, error) { c.stateMu.Lock() defer c.stateMu.Unlock() + + if c.runDone != nil { + return nil, errClientAlreadyRunning + } + + done := make(chan struct{}) + c.runDone = done + c.ctxCancel = cancel + return done, nil +} + +func (c *Client) finishRun(done chan struct{}) { + c.stateMu.Lock() + c.connectClient = nil + c.config = nil + c.runDone = nil + c.ctxCancel = nil + c.stateMu.Unlock() + + close(done) +} + +func (c *Client) setState(cfg *profilemanager.Config, cc *internal.ConnectClient) { + c.stateMu.Lock() c.config = cfg c.connectClient = cc + c.stateMu.Unlock() } // stateSnapshot returns the current config and ConnectClient under the lock. From 611a9291cd99e4a16f26a68ef28d48bedf0edf40 Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Fri, 28 Aug 2026 15:39:48 +0200 Subject: [PATCH 3/5] [management] fix posture check flip evaluation for affected peers calc (#7347) --- .../network_map/controller/controller.go | 38 +--- .../controller/posture_twin_test.go | 38 ++++ .../controllers/network_map/interface.go | 6 +- .../controllers/network_map/interface_mock.go | 10 +- .../grpc/components_envelope_response.go | 3 +- .../internals/shared/grpc/conversion.go | 3 +- management/internals/shared/grpc/server.go | 12 +- management/server/account.go | 3 +- management/server/account/manager.go | 11 +- management/server/account/manager_mock.go | 17 +- management/server/account_test.go | 58 ++++- .../affected_peers_router_paths_test.go | 42 ++++ .../server/affected_peers_router_test.go | 6 + management/server/mock_server/account_mock.go | 17 +- management/server/peer.go | 25 ++- management/server/peer_posture_test.go | 183 ++++++++++++++++ management/server/peer_test.go | 10 +- .../server/posture/affects_posture_test.go | 202 ------------------ management/server/posture/checks.go | 41 ---- .../server/types/account_networkmapdata.go | 14 +- .../management/networkmap/nmdata/posture.go | 16 ++ .../networkmap/nmdata/posture_test.go | 54 +++++ 22 files changed, 476 insertions(+), 333 deletions(-) create mode 100644 management/internals/controllers/network_map/controller/posture_twin_test.go create mode 100644 management/server/peer_posture_test.go delete mode 100644 management/server/posture/affects_posture_test.go create mode 100644 shared/management/networkmap/nmdata/posture_test.go diff --git a/management/internals/controllers/network_map/controller/controller.go b/management/internals/controllers/network_map/controller/controller.go index e74b17638..f21f878a4 100644 --- a/management/internals/controllers/network_map/controller/controller.go +++ b/management/internals/controllers/network_map/controller/controller.go @@ -566,15 +566,13 @@ func NetworkMapFromData(ctx context.Context, nmData *networkmap.NetworkMapData, return nm } -// peerPostureChecksFromData mirrors getPeerPostureChecks on the twin store. The -// sync response only encodes process-check file paths, so only ProcessCheck is -// converted back to the server posture type. -func peerPostureChecksFromData(nmData *networkmap.NetworkMapData, peerID string) []*posture.Checks { +// peerPostureChecksFromData mirrors getPeerPostureChecks on the twin store. +func peerPostureChecksFromData(nmData *networkmap.NetworkMapData, peerID string) []*nmdata.PostureChecks { if len(nmData.PostureChecks) == 0 { return nil } - peerPostureChecks := make(map[string]*posture.Checks) + peerPostureChecks := make(map[string]*nmdata.PostureChecks) for _, policy := range nmData.Policies { if policy == nil || !policy.Enabled || len(policy.SourcePostureChecks) == 0 { continue @@ -583,11 +581,9 @@ func peerPostureChecksFromData(nmData *networkmap.NetworkMapData, peerID string) continue } for _, checkID := range policy.SourcePostureChecks { - twin := nmData.PostureChecks[checkID] - if twin == nil { - continue + if twin := nmData.PostureChecks[checkID]; twin != nil { + peerPostureChecks[checkID] = twin } - peerPostureChecks[checkID] = postureChecksFromTwin(twin) } } @@ -608,18 +604,6 @@ func isPeerInPolicySourceGroupsFromData(nmData *networkmap.NetworkMapData, peerI return false } -func postureChecksFromTwin(twin *nmdata.PostureChecks) *posture.Checks { - checks := &posture.Checks{ID: twin.ID} - if twin.Checks.ProcessCheck != nil { - processes := make([]posture.Process, 0, len(twin.Checks.ProcessCheck.Processes)) - for _, p := range twin.Checks.ProcessCheck.Processes { - processes = append(processes, posture.Process{LinuxPath: p.LinuxPath, MacPath: p.MacPath, WindowsPath: p.WindowsPath}) - } - checks.Checks.ProcessCheck = &posture.ProcessCheck{Processes: processes} - } - return checks -} - func (c *Controller) perAccountOrGlobalSupportedSyncMessageVersions(accountId string) sharedgrpc.SyncMessageVersion { if perAccount, ok := c.perAccountServerSupportedSyncMessageVersions[accountId]; ok { return perAccount @@ -967,7 +951,7 @@ func (c *Controller) BufferUpdateAccountPeers(ctx context.Context, accountID str // data the legacy server folds in via NetworkMap.Merge). The gRPC layer // encodes both into the wire envelope. Callers must gate on capability // themselves before dispatching here — this method does NOT branch on it. -func (c *Controller) GetValidatedPeerWithComponents(ctx context.Context, isRequiresApproval bool, accountID string, peer *nbpeer.Peer) (*nbpeer.Peer, *types.NetworkMapComponents, *types.NetworkMap, []*posture.Checks, int64, error) { +func (c *Controller) GetValidatedPeerWithComponents(ctx context.Context, isRequiresApproval bool, accountID string, peer *nbpeer.Peer) (*nbpeer.Peer, *types.NetworkMapComponents, *types.NetworkMap, []*nmdata.PostureChecks, int64, error) { if isRequiresApproval { network, err := c.repo.GetAccountNetwork(ctx, accountID) if err != nil { @@ -1032,7 +1016,7 @@ func (c *Controller) GetValidatedPeerWithComponents(ctx context.Context, isRequi // getValidatedPeerWithComponentsFromData is the account-free variant of // GetValidatedPeerWithComponents. The proxy network map fragment is omitted // like on the other nmdata paths. -func (c *Controller) getValidatedPeerWithComponentsFromData(ctx context.Context, accountID string, peer *nbpeer.Peer, nmData *networkmap.NetworkMapData) (*nbpeer.Peer, *types.NetworkMapComponents, *types.NetworkMap, []*posture.Checks, int64, error) { +func (c *Controller) getValidatedPeerWithComponentsFromData(ctx context.Context, accountID string, peer *nbpeer.Peer, nmData *networkmap.NetworkMapData) (*nbpeer.Peer, *types.NetworkMapComponents, *types.NetworkMap, []*nmdata.PostureChecks, int64, error) { postureChecks := peerPostureChecksFromData(nmData, peer.ID) dnsDomain := c.getDNSDomainFromData(nmData.AccountSettings) @@ -1142,7 +1126,7 @@ func (b *bufferAffectedUpdate) setTimer(d time.Duration, f func()) { b.next.Reset(d) } -func (c *Controller) GetValidatedPeerWithMap(ctx context.Context, isRequiresApproval bool, accountID string, peerID string) (*types.NetworkMap, []*posture.Checks, int64, error) { +func (c *Controller) GetValidatedPeerWithMap(ctx context.Context, isRequiresApproval bool, accountID string, peerID string) (*types.NetworkMap, []*nmdata.PostureChecks, int64, error) { if isRequiresApproval { network, err := c.repo.GetAccountNetwork(ctx, accountID) if err != nil { @@ -1209,7 +1193,7 @@ func (c *Controller) GetValidatedPeerWithMap(ctx context.Context, isRequiresAppr // getValidatedPeerWithMapFromData is the account-free variant of // GetValidatedPeerWithMap. The proxy network map fragment is omitted like on // the other nmdata paths. -func (c *Controller) getValidatedPeerWithMapFromData(ctx context.Context, accountID string, peerID string, nmData *networkmap.NetworkMapData) (*types.NetworkMap, []*posture.Checks, int64, error) { +func (c *Controller) getValidatedPeerWithMapFromData(ctx context.Context, accountID string, peerID string, nmData *networkmap.NetworkMapData) (*types.NetworkMap, []*nmdata.PostureChecks, int64, error) { postureChecks := peerPostureChecksFromData(nmData, peerID) dnsDomain := c.getDNSDomainFromData(nmData.AccountSettings) @@ -1234,7 +1218,7 @@ func (c *Controller) GetDNSDomain(settings *types.Settings) string { } // getPeerPostureChecks returns the posture checks applied for a given peer. -func (c *Controller) getPeerPostureChecks(account *types.Account, peerID string) ([]*posture.Checks, error) { +func (c *Controller) getPeerPostureChecks(account *types.Account, peerID string) ([]*nmdata.PostureChecks, error) { peerPostureChecks := make(map[string]*posture.Checks) if len(account.PostureChecks) == 0 { @@ -1251,7 +1235,7 @@ func (c *Controller) getPeerPostureChecks(account *types.Account, peerID string) } } - return maps.Values(peerPostureChecks), nil + return types.TwinPostureChecksList(maps.Values(peerPostureChecks)), nil } func (c *Controller) StartWarmup(ctx context.Context) { diff --git a/management/internals/controllers/network_map/controller/posture_twin_test.go b/management/internals/controllers/network_map/controller/posture_twin_test.go new file mode 100644 index 000000000..d5c9035e0 --- /dev/null +++ b/management/internals/controllers/network_map/controller/posture_twin_test.go @@ -0,0 +1,38 @@ +package controller + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/shared/management/networkmap" + "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" +) + +func TestPeerPostureChecksFromData_ReturnsTwinsUnchanged(t *testing.T) { + check := &nmdata.PostureChecks{ + ID: "pc1", + Checks: nmdata.ChecksDefinition{ + NBVersionCheck: &nmdata.NBVersionCheck{MinVersion: "0.30.0"}, + OSVersionCheck: &nmdata.OSVersionCheck{Linux: &nmdata.MinKernelVersionCheck{MinKernelVersion: "6.1"}}, + }, + } + nmData := &networkmap.NetworkMapData{ + Groups: map[string]*nmdata.Group{"g1": {ID: "g1", Peers: []string{"peer1"}}}, + Policies: []*nmdata.Policy{{ + ID: "policy1", + Enabled: true, + SourcePostureChecks: []string{"pc1"}, + Rules: []*nmdata.PolicyRule{{ID: "rule1", Enabled: true, Sources: []string{"g1"}}}, + }}, + PostureChecks: map[string]*nmdata.PostureChecks{"pc1": check}, + } + + got := peerPostureChecksFromData(nmData, "peer1") + require.Len(t, got, 1) + assert.Same(t, check, got[0]) + assert.Len(t, got[0].GetChecks(), 2) + + assert.Empty(t, peerPostureChecksFromData(nmData, "peer-outside-source-group")) +} diff --git a/management/internals/controllers/network_map/interface.go b/management/internals/controllers/network_map/interface.go index b535321d1..1e8c219b3 100644 --- a/management/internals/controllers/network_map/interface.go +++ b/management/internals/controllers/network_map/interface.go @@ -7,8 +7,8 @@ import ( nbdns "github.com/netbirdio/netbird/dns" nbpeer "github.com/netbirdio/netbird/management/server/peer" - "github.com/netbirdio/netbird/management/server/posture" "github.com/netbirdio/netbird/management/server/types" + "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" ) const ( @@ -23,8 +23,8 @@ type Controller interface { BufferUpdateAffectedPeers(ctx context.Context, accountID string, peerIDs []string, reason types.UpdateReason) error UpdateAccountPeer(ctx context.Context, accountId string, peerId string) error BufferUpdateAccountPeers(ctx context.Context, accountID string, reason types.UpdateReason) error - GetValidatedPeerWithMap(ctx context.Context, isRequiresApproval bool, accountID string, peerID string) (*types.NetworkMap, []*posture.Checks, int64, error) - GetValidatedPeerWithComponents(ctx context.Context, isRequiresApproval bool, accountID string, p *nbpeer.Peer) (*nbpeer.Peer, *types.NetworkMapComponents, *types.NetworkMap, []*posture.Checks, int64, error) + GetValidatedPeerWithMap(ctx context.Context, isRequiresApproval bool, accountID string, peerID string) (*types.NetworkMap, []*nmdata.PostureChecks, int64, error) + GetValidatedPeerWithComponents(ctx context.Context, isRequiresApproval bool, accountID string, p *nbpeer.Peer) (*nbpeer.Peer, *types.NetworkMapComponents, *types.NetworkMap, []*nmdata.PostureChecks, int64, error) GetDNSDomain(settings *types.Settings) string StartWarmup(context.Context) GetNetworkMap(ctx context.Context, peerID string) (*types.NetworkMap, error) diff --git a/management/internals/controllers/network_map/interface_mock.go b/management/internals/controllers/network_map/interface_mock.go index 42051f172..8b104dfa0 100644 --- a/management/internals/controllers/network_map/interface_mock.go +++ b/management/internals/controllers/network_map/interface_mock.go @@ -14,8 +14,8 @@ import ( reflect "reflect" peer "github.com/netbirdio/netbird/management/server/peer" - posture "github.com/netbirdio/netbird/management/server/posture" types "github.com/netbirdio/netbird/management/server/types" + nmdata "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" gomock "go.uber.org/mock/gomock" ) @@ -127,13 +127,13 @@ func (mr *MockControllerMockRecorder) GetNetworkMap(ctx, peerID any) *gomock.Cal } // GetValidatedPeerWithComponents mocks base method. -func (m *MockController) GetValidatedPeerWithComponents(ctx context.Context, isRequiresApproval bool, accountID string, p *peer.Peer) (*peer.Peer, *types.NetworkMapComponents, *types.NetworkMap, []*posture.Checks, int64, error) { +func (m *MockController) GetValidatedPeerWithComponents(ctx context.Context, isRequiresApproval bool, accountID string, p *peer.Peer) (*peer.Peer, *types.NetworkMapComponents, *types.NetworkMap, []*nmdata.PostureChecks, int64, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetValidatedPeerWithComponents", ctx, isRequiresApproval, accountID, p) ret0, _ := ret[0].(*peer.Peer) ret1, _ := ret[1].(*types.NetworkMapComponents) ret2, _ := ret[2].(*types.NetworkMap) - ret3, _ := ret[3].([]*posture.Checks) + ret3, _ := ret[3].([]*nmdata.PostureChecks) ret4, _ := ret[4].(int64) ret5, _ := ret[5].(error) return ret0, ret1, ret2, ret3, ret4, ret5 @@ -146,11 +146,11 @@ func (mr *MockControllerMockRecorder) GetValidatedPeerWithComponents(ctx, isRequ } // GetValidatedPeerWithMap mocks base method. -func (m *MockController) GetValidatedPeerWithMap(ctx context.Context, isRequiresApproval bool, accountID, peerID string) (*types.NetworkMap, []*posture.Checks, int64, error) { +func (m *MockController) GetValidatedPeerWithMap(ctx context.Context, isRequiresApproval bool, accountID, peerID string) (*types.NetworkMap, []*nmdata.PostureChecks, int64, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetValidatedPeerWithMap", ctx, isRequiresApproval, accountID, peerID) ret0, _ := ret[0].(*types.NetworkMap) - ret1, _ := ret[1].([]*posture.Checks) + ret1, _ := ret[1].([]*nmdata.PostureChecks) ret2, _ := ret[2].(int64) ret3, _ := ret[3].(error) return ret0, ret1, ret2, ret3 diff --git a/management/internals/shared/grpc/components_envelope_response.go b/management/internals/shared/grpc/components_envelope_response.go index c059b2248..cdd2a7f37 100644 --- a/management/internals/shared/grpc/components_envelope_response.go +++ b/management/internals/shared/grpc/components_envelope_response.go @@ -7,7 +7,6 @@ import ( "github.com/netbirdio/netbird/client/ssh/auth" nbconfig "github.com/netbirdio/netbird/management/internals/server/config" - "github.com/netbirdio/netbird/management/server/posture" "github.com/netbirdio/netbird/management/server/types" sharedgrpc "github.com/netbirdio/netbird/shared/management/grpc" "github.com/netbirdio/netbird/shared/management/networkmap" @@ -37,7 +36,7 @@ func ToComponentSyncResponse( components *types.NetworkMapComponents, proxyPatch *types.NetworkMap, dnsName string, - checks []*posture.Checks, + checks []*nmdata.PostureChecks, settings *nmdata.AccountSettingsInfo, extraSettings *types.ExtraSettings, peerGroups []string, diff --git a/management/internals/shared/grpc/conversion.go b/management/internals/shared/grpc/conversion.go index 5640127ca..96bd9f1f4 100644 --- a/management/internals/shared/grpc/conversion.go +++ b/management/internals/shared/grpc/conversion.go @@ -18,7 +18,6 @@ import ( "github.com/netbirdio/netbird/management/internals/controllers/network_map/controller/cache" nbconfig "github.com/netbirdio/netbird/management/internals/server/config" - "github.com/netbirdio/netbird/management/server/posture" "github.com/netbirdio/netbird/management/server/types" "github.com/netbirdio/netbird/shared/management/networkmap" "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" @@ -154,7 +153,7 @@ func toPeerConfig(peer *nmdata.Peer, network *nmdata.Network, dnsName string, se return peerConfig } -func ToSyncResponse(ctx context.Context, config *nbconfig.Config, httpConfig *nbconfig.HttpServerConfig, deviceFlowConfig *nbconfig.DeviceAuthorizationFlow, peer *nmdata.Peer, turnCredentials *Token, relayCredentials *Token, networkMap *types.NetworkMap, dnsName string, checks []*posture.Checks, dnsCache *cache.DNSConfigCache, settings *nmdata.AccountSettingsInfo, extraSettings *types.ExtraSettings, peerGroups []string, dnsFwdPort int64) *proto.SyncResponse { +func ToSyncResponse(ctx context.Context, config *nbconfig.Config, httpConfig *nbconfig.HttpServerConfig, deviceFlowConfig *nbconfig.DeviceAuthorizationFlow, peer *nmdata.Peer, turnCredentials *Token, relayCredentials *Token, networkMap *types.NetworkMap, dnsName string, checks []*nmdata.PostureChecks, dnsCache *cache.DNSConfigCache, settings *nmdata.AccountSettingsInfo, extraSettings *types.ExtraSettings, peerGroups []string, dnsFwdPort int64) *proto.SyncResponse { // IPv6 data in AllowedIPs and SourcePrefixes wildcard expansion depends on // whether the target peer supports IPv6. Routes and firewall rules are already // filtered at the source (network map builder). diff --git a/management/internals/shared/grpc/server.go b/management/internals/shared/grpc/server.go index 4435f6706..240243497 100644 --- a/management/internals/shared/grpc/server.go +++ b/management/internals/shared/grpc/server.go @@ -42,10 +42,10 @@ import ( "github.com/netbirdio/netbird/management/server/auth" nbContext "github.com/netbirdio/netbird/management/server/context" nbpeer "github.com/netbirdio/netbird/management/server/peer" - "github.com/netbirdio/netbird/management/server/posture" "github.com/netbirdio/netbird/management/server/settings" "github.com/netbirdio/netbird/management/server/telemetry" "github.com/netbirdio/netbird/management/server/types" + "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" "github.com/netbirdio/netbird/shared/management/proto" internalStatus "github.com/netbirdio/netbird/shared/management/status" ) @@ -902,7 +902,7 @@ func (s *Server) ExtendAuthSession(ctx context.Context, req *proto.EncryptedMess }, nil } -func (s *Server) prepareLoginResponse(ctx context.Context, peer *nbpeer.Peer, network *types.Network, postureChecks []*posture.Checks, enableSSH bool) (*proto.LoginResponse, error) { +func (s *Server) prepareLoginResponse(ctx context.Context, peer *nbpeer.Peer, network *types.Network, postureChecks []*nmdata.PostureChecks, enableSSH bool) (*proto.LoginResponse, error) { var relayToken *Token var err error if s.config.Relay != nil && len(s.config.Relay.Addresses) > 0 { @@ -990,7 +990,7 @@ func (s *Server) IsHealthy(ctx context.Context, req *proto.Empty) (*proto.Empty, } // sendInitialSync sends initial proto.SyncResponse to the peer requesting synchronization -func (s *Server) sendInitialSync(ctx context.Context, peerKey wgtypes.Key, peer *nbpeer.Peer, networkMap *types.NetworkMap, postureChecks []*posture.Checks, srv proto.ManagementService_SyncServer, dnsFwdPort int64) error { +func (s *Server) sendInitialSync(ctx context.Context, peerKey wgtypes.Key, peer *nbpeer.Peer, networkMap *types.NetworkMap, postureChecks []*nmdata.PostureChecks, srv proto.ManagementService_SyncServer, dnsFwdPort int64) error { var err error var turnToken *Token @@ -1301,7 +1301,7 @@ func (s *Server) Logout(ctx context.Context, req *proto.EncryptedMessage) (*prot } // toProtocolChecks converts posture checks to protocol checks. -func toProtocolChecks(ctx context.Context, postureChecks []*posture.Checks) []*proto.Checks { +func toProtocolChecks(ctx context.Context, postureChecks []*nmdata.PostureChecks) []*proto.Checks { protoChecks := make([]*proto.Checks, 0, len(postureChecks)) for _, postureCheck := range postureChecks { check := toProtocolCheck(postureCheck) @@ -1313,8 +1313,8 @@ func toProtocolChecks(ctx context.Context, postureChecks []*posture.Checks) []*p return protoChecks } -// toProtocolCheck converts a posture.Checks to a proto.Checks. -func toProtocolCheck(postureCheck *posture.Checks) *proto.Checks { +// toProtocolCheck converts posture checks to a proto.Checks. +func toProtocolCheck(postureCheck *nmdata.PostureChecks) *proto.Checks { protoCheck := &proto.Checks{} if check := postureCheck.Checks.ProcessCheck; check != nil { diff --git a/management/server/account.go b/management/server/account.go index 700dfa04d..4fe0e5338 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -52,6 +52,7 @@ import ( "github.com/netbirdio/netbird/management/server/util" "github.com/netbirdio/netbird/route" nbdomain "github.com/netbirdio/netbird/shared/management/domain" + "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" "github.com/netbirdio/netbird/shared/management/status" ) @@ -1920,7 +1921,7 @@ func domainIsUpToDate(domain string, domainCategory string, userAuth auth.UserAu // derived from syncTime (the moment the gRPC stream opened). Any // concurrent stream that started earlier loses the optimistic-lock race // in MarkPeerConnected and bails without writing. -func (am *DefaultAccountManager) SyncAndMarkPeer(ctx context.Context, accountID string, peerPubKey string, meta nbpeer.PeerSystemMeta, realIP net.IP, syncTime time.Time) (*nbpeer.Peer, *types.NetworkMap, []*posture.Checks, int64, error) { +func (am *DefaultAccountManager) SyncAndMarkPeer(ctx context.Context, accountID string, peerPubKey string, meta nbpeer.PeerSystemMeta, realIP net.IP, syncTime time.Time) (*nbpeer.Peer, *types.NetworkMap, []*nmdata.PostureChecks, int64, error) { peer, netMap, postureChecks, dnsfwdPort, err := am.SyncPeer(ctx, types.PeerSync{WireGuardPubKey: peerPubKey, Meta: meta, RealIP: realIP}, accountID) if err != nil { return nil, nil, nil, 0, fmt.Errorf("error syncing peer: %w", err) diff --git a/management/server/account/manager.go b/management/server/account/manager.go index f4b0408cf..154c9ab18 100644 --- a/management/server/account/manager.go +++ b/management/server/account/manager.go @@ -23,6 +23,7 @@ import ( "github.com/netbirdio/netbird/management/server/users" "github.com/netbirdio/netbird/route" "github.com/netbirdio/netbird/shared/management/domain" + "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" ) type ExternalCacheManager nbcache.UserDataCache @@ -70,7 +71,7 @@ type Manager interface { UpdatePeerIPv6(ctx context.Context, accountID, userID, peerID string, newIPv6 netip.Addr) error GetNetworkMap(ctx context.Context, peerID string) (*types.NetworkMap, error) GetPeerNetwork(ctx context.Context, peerID string) (*types.Network, error) - AddPeer(ctx context.Context, accountID, setupKey, userID string, p *nbpeer.Peer, temporary bool) (*nbpeer.Peer, *types.Network, []*posture.Checks, bool, error) + AddPeer(ctx context.Context, accountID, setupKey, userID string, p *nbpeer.Peer, temporary bool) (*nbpeer.Peer, *types.Network, []*nmdata.PostureChecks, bool, error) CreatePAT(ctx context.Context, accountID string, initiatorUserID string, targetUserID string, tokenName string, expiresIn int) (*types.PersonalAccessTokenGenerated, error) DeletePAT(ctx context.Context, accountID string, initiatorUserID string, targetUserID string, tokenID string) error GetPAT(ctx context.Context, accountID string, initiatorUserID string, targetUserID string, tokenID string) (*types.PersonalAccessToken, error) @@ -109,9 +110,9 @@ type Manager interface { GetPeer(ctx context.Context, accountID, peerID, userID string) (*nbpeer.Peer, error) UpdateAccountSettings(ctx context.Context, accountID, userID string, newSettings *types.Settings) (*types.Settings, error) UpdateAccountOnboarding(ctx context.Context, accountID, userID string, newOnboarding *types.AccountOnboarding) (*types.AccountOnboarding, error) - LoginPeer(ctx context.Context, login types.PeerLogin) (*nbpeer.Peer, *types.Network, []*posture.Checks, bool, error) // used by peer gRPC API - ExtendPeerSession(ctx context.Context, peerPubKey, userID string) (time.Time, error) // used by peer gRPC API for ExtendAuthSession - SyncPeer(ctx context.Context, sync types.PeerSync, accountID string) (*nbpeer.Peer, *types.NetworkMap, []*posture.Checks, int64, error) // used by peer gRPC API + LoginPeer(ctx context.Context, login types.PeerLogin) (*nbpeer.Peer, *types.Network, []*nmdata.PostureChecks, bool, error) // used by peer gRPC API + ExtendPeerSession(ctx context.Context, peerPubKey, userID string) (time.Time, error) // used by peer gRPC API for ExtendAuthSession + SyncPeer(ctx context.Context, sync types.PeerSync, accountID string) (*nbpeer.Peer, *types.NetworkMap, []*nmdata.PostureChecks, int64, error) // used by peer gRPC API GetExternalCacheManager() ExternalCacheManager GetPostureChecks(ctx context.Context, accountID, postureChecksID, userID string) (*posture.Checks, error) SavePostureChecks(ctx context.Context, accountID, userID string, postureChecks *posture.Checks, create bool) (*posture.Checks, error) @@ -121,7 +122,7 @@ type Manager interface { UpdateIntegratedValidator(ctx context.Context, accountID, userID, validator string, groups []string) error GroupValidation(ctx context.Context, accountId string, groups []string) (bool, error) GetValidatedPeers(ctx context.Context, accountID string) (map[string]struct{}, map[string]string, error) - SyncAndMarkPeer(ctx context.Context, accountID string, peerPubKey string, meta nbpeer.PeerSystemMeta, realIP net.IP, syncTime time.Time) (*nbpeer.Peer, *types.NetworkMap, []*posture.Checks, int64, error) + SyncAndMarkPeer(ctx context.Context, accountID string, peerPubKey string, meta nbpeer.PeerSystemMeta, realIP net.IP, syncTime time.Time) (*nbpeer.Peer, *types.NetworkMap, []*nmdata.PostureChecks, int64, error) OnPeerDisconnected(ctx context.Context, accountID string, peerPubKey string, streamStartTime time.Time) error SyncPeerMeta(ctx context.Context, peerPubKey string, meta nbpeer.PeerSystemMeta, realIP net.IP) error FindExistingPostureCheck(accountID string, checks *posture.ChecksDefinition) (*posture.Checks, error) diff --git a/management/server/account/manager_mock.go b/management/server/account/manager_mock.go index 9ac10cba0..f31f63d0e 100644 --- a/management/server/account/manager_mock.go +++ b/management/server/account/manager_mock.go @@ -29,6 +29,7 @@ import ( route "github.com/netbirdio/netbird/route" auth "github.com/netbirdio/netbird/shared/auth" domain "github.com/netbirdio/netbird/shared/management/domain" + nmdata "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" gomock "go.uber.org/mock/gomock" ) @@ -86,12 +87,12 @@ func (mr *MockManagerMockRecorder) AccountExists(ctx, accountID any) *gomock.Cal } // AddPeer mocks base method. -func (m *MockManager) AddPeer(ctx context.Context, accountID, setupKey, userID string, p *peer.Peer, temporary bool) (*peer.Peer, *types.Network, []*posture.Checks, bool, error) { +func (m *MockManager) AddPeer(ctx context.Context, accountID, setupKey, userID string, p *peer.Peer, temporary bool) (*peer.Peer, *types.Network, []*nmdata.PostureChecks, bool, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "AddPeer", ctx, accountID, setupKey, userID, p, temporary) ret0, _ := ret[0].(*peer.Peer) ret1, _ := ret[1].(*types.Network) - ret2, _ := ret[2].([]*posture.Checks) + ret2, _ := ret[2].([]*nmdata.PostureChecks) ret3, _ := ret[3].(bool) ret4, _ := ret[4].(error) return ret0, ret1, ret2, ret3, ret4 @@ -1323,12 +1324,12 @@ func (mr *MockManagerMockRecorder) ListUsers(ctx, accountID any) *gomock.Call { } // LoginPeer mocks base method. -func (m *MockManager) LoginPeer(ctx context.Context, login types.PeerLogin) (*peer.Peer, *types.Network, []*posture.Checks, bool, error) { +func (m *MockManager) LoginPeer(ctx context.Context, login types.PeerLogin) (*peer.Peer, *types.Network, []*nmdata.PostureChecks, bool, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "LoginPeer", ctx, login) ret0, _ := ret[0].(*peer.Peer) ret1, _ := ret[1].(*types.Network) - ret2, _ := ret[2].([]*posture.Checks) + ret2, _ := ret[2].([]*nmdata.PostureChecks) ret3, _ := ret[3].(bool) ret4, _ := ret[4].(error) return ret0, ret1, ret2, ret3, ret4 @@ -1568,12 +1569,12 @@ func (mr *MockManagerMockRecorder) StoreEvent(ctx, initiatorID, targetID, accoun } // SyncAndMarkPeer mocks base method. -func (m *MockManager) SyncAndMarkPeer(ctx context.Context, accountID, peerPubKey string, meta peer.PeerSystemMeta, realIP net.IP, syncTime time.Time) (*peer.Peer, *types.NetworkMap, []*posture.Checks, int64, error) { +func (m *MockManager) SyncAndMarkPeer(ctx context.Context, accountID, peerPubKey string, meta peer.PeerSystemMeta, realIP net.IP, syncTime time.Time) (*peer.Peer, *types.NetworkMap, []*nmdata.PostureChecks, int64, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "SyncAndMarkPeer", ctx, accountID, peerPubKey, meta, realIP, syncTime) ret0, _ := ret[0].(*peer.Peer) ret1, _ := ret[1].(*types.NetworkMap) - ret2, _ := ret[2].([]*posture.Checks) + ret2, _ := ret[2].([]*nmdata.PostureChecks) ret3, _ := ret[3].(int64) ret4, _ := ret[4].(error) return ret0, ret1, ret2, ret3, ret4 @@ -1586,12 +1587,12 @@ func (mr *MockManagerMockRecorder) SyncAndMarkPeer(ctx, accountID, peerPubKey, m } // SyncPeer mocks base method. -func (m *MockManager) SyncPeer(ctx context.Context, sync types.PeerSync, accountID string) (*peer.Peer, *types.NetworkMap, []*posture.Checks, int64, error) { +func (m *MockManager) SyncPeer(ctx context.Context, sync types.PeerSync, accountID string) (*peer.Peer, *types.NetworkMap, []*nmdata.PostureChecks, int64, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "SyncPeer", ctx, sync, accountID) ret0, _ := ret[0].(*peer.Peer) ret1, _ := ret[1].(*types.NetworkMap) - ret2, _ := ret[2].([]*posture.Checks) + ret2, _ := ret[2].([]*nmdata.PostureChecks) ret3, _ := ret[3].(int64) ret4, _ := ret[4].(error) return ret0, ret1, ret2, ret3, ret4 diff --git a/management/server/account_test.go b/management/server/account_test.go index a5a484c1a..b462cc2a6 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -10,16 +10,17 @@ import ( "os" "reflect" "strconv" + "strings" "sync" "testing" "time" - "go.uber.org/mock/gomock" "github.com/prometheus/client_golang/prometheus/push" log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/metric/noop" + "go.uber.org/mock/gomock" "golang.zx2c4.com/wireguard/wgctrl/wgtypes" "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain" @@ -37,6 +38,8 @@ import ( "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service" reverseproxymanager "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service/manager" "github.com/netbirdio/netbird/management/internals/modules/zones" + networkmapdb "github.com/netbirdio/netbird/management/internals/network_map_db" + networkmapdbfactory "github.com/netbirdio/netbird/management/internals/network_map_db/factory" "github.com/netbirdio/netbird/management/internals/server/config" nbgrpc "github.com/netbirdio/netbird/management/internals/shared/grpc" nbAccount "github.com/netbirdio/netbird/management/server/account" @@ -3293,13 +3296,33 @@ func createManager(t testing.TB) (*DefaultAccountManager, *update_channel.PeersU if err != nil { return nil, nil, err } - eventStore := &activity.InMemoryEventStore{} + return buildTestManager(t, store, nil) +} - metrics, err := telemetry.NewDefaultAppMetrics(context.Background()) - if err != nil { - return nil, nil, err +// createManagerWithNetworkMapStore builds a manager whose network map controller +// reads the twin (nmdata) store, the production path on sqlite and postgres. +func createManagerWithNetworkMapStore(t testing.TB) (*DefaultAccountManager, *update_channel.PeersUpdateManager) { + t.Helper() + + if engine := os.Getenv("NETBIRD_STORE_ENGINE"); engine != "" && !strings.EqualFold(engine, string(types.SqliteStoreEngine)) { + t.Skipf("network map store test needs the sqlite engine, got %s", engine) } + dataDir := t.TempDir() + store, err := createStoreAt(t, dataDir) + require.NoError(t, err) + + nmdataStore, err := networkmapdbfactory.NewNetworkMapDBStore(context.Background(), types.SqliteStoreEngine, dataDir, MockIntegratedValidator{}, newSettingsMockManager(t)) + require.NoError(t, err) + + manager, updateManager, err := buildTestManager(t, store, nmdataStore) + require.NoError(t, err) + return manager, updateManager +} + +func newSettingsMockManager(t testing.TB) *settings.MockManager { + t.Helper() + ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish) @@ -3312,6 +3335,23 @@ func createManager(t testing.TB) (*DefaultAccountManager, *update_channel.PeersU UpdateExtraSettings(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()). Return(false, nil). AnyTimes() + return settingsMockManager +} + +func buildTestManager(t testing.TB, store store.Store, nmdataStore *networkmapdb.NetworkMapDBStoreImpl) (*DefaultAccountManager, *update_channel.PeersUpdateManager, error) { + t.Helper() + + eventStore := &activity.InMemoryEventStore{} + + metrics, err := telemetry.NewDefaultAppMetrics(context.Background()) + if err != nil { + return nil, nil, err + } + + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + settingsMockManager := newSettingsMockManager(t) permissionsManager := permissions.NewManager(store) peersManager := peers.NewManager(store, permissionsManager) @@ -3331,7 +3371,7 @@ func createManager(t testing.TB) (*DefaultAccountManager, *update_channel.PeersU updateManager := update_channel.NewPeersUpdateManager(metrics) requestBuffer := NewAccountRequestBuffer(ctx, store) - networkMapController := controller.NewController(ctx, store, metrics, updateManager, requestBuffer, MockIntegratedValidator{}, settingsMockManager, "netbird.cloud", port_forwarding.NewControllerMock(), ephemeral_manager.NewEphemeralManager(store, peers.NewManager(store, permissionsManager)), &config.Config{}, nil) + networkMapController := controller.NewController(ctx, store, metrics, updateManager, requestBuffer, MockIntegratedValidator{}, settingsMockManager, "netbird.cloud", port_forwarding.NewControllerMock(), ephemeral_manager.NewEphemeralManager(store, peers.NewManager(store, permissionsManager)), &config.Config{}, nmdataStore) manager, err := BuildManager(ctx, &config.Config{}, store, networkMapController, job.NewJobManager(nil, store, peersManager), nil, "", eventStore, nil, false, MockIntegratedValidator{}, metrics, port_forwarding.NewControllerMock(), settingsMockManager, permissionsManager, false, cacheStore) if err != nil { return nil, nil, err @@ -3349,7 +3389,11 @@ func createManager(t testing.TB) (*DefaultAccountManager, *update_channel.PeersU func createStore(t testing.TB) (store.Store, error) { t.Helper() - dataDir := t.TempDir() + return createStoreAt(t, t.TempDir()) +} + +func createStoreAt(t testing.TB, dataDir string) (store.Store, error) { + t.Helper() store, cleanUp, err := store.NewTestStoreFromSQL(context.Background(), "", dataDir) if err != nil { return nil, err diff --git a/management/server/affected_peers_router_paths_test.go b/management/server/affected_peers_router_paths_test.go index 5d83367fd..7fef1ab35 100644 --- a/management/server/affected_peers_router_paths_test.go +++ b/management/server/affected_peers_router_paths_test.go @@ -12,6 +12,7 @@ import ( resourceTypes "github.com/netbirdio/netbird/management/server/networks/resources/types" routerTypes "github.com/netbirdio/netbird/management/server/networks/routers/types" "github.com/netbirdio/netbird/management/server/posture" + "github.com/netbirdio/netbird/management/server/store" "github.com/netbirdio/netbird/management/server/types" ) @@ -338,3 +339,44 @@ func TestAffectedPeers_PeerChange_RouterInOtherNetworkNotAffected(t *testing.T) assert.NotContains(t, affected, second.routerPeerID, "a router in an unrelated network must not be affected by a source-peer change for another resource") } + +// TestAffectedPeers_E2E_PostureFlip_RefreshesRoutingPeer drives the customer path +// on the twin store: the source peer's metadata flips a posture verdict on sync, +// and the routing peer serving the gated resource must be refreshed in both +// directions. Without the flip detection the deny direction takes the nmap +// shortcut (the denied peer's map holds no router) and the allow direction +// depends on which meta field moved, leaving the routers with a stale map. +func TestAffectedPeers_E2E_PostureFlip_RefreshesRoutingPeer(t *testing.T) { + manager, updateManager := createManagerWithNetworkMapStore(t) + s := buildRouterScenario(t, manager, updateManager, true) + ctx := context.Background() + + s.createPostureCheckGatedPolicy(t, ctx) + + source, err := s.manager.Store.GetPeerByID(ctx, store.LockingStrengthNone, s.accountID, s.sourcePeerID) + require.NoError(t, err) + + syncWithVersion := func(version string) { + meta := source.Meta + meta.WtVersion = version + _, _, _, _, err := s.manager.SyncPeer(ctx, types.PeerSync{WireGuardPubKey: source.Key, Meta: meta}, s.accountID) + require.NoError(t, err) + } + syncWithVersion("0.31.0") + + routerCh := s.updateManager.CreateChannel(ctx, s.routerPeerID) + unrelatedCh := s.updateManager.CreateChannel(ctx, s.unrelatedPeerID) + t.Cleanup(func() { + s.updateManager.CloseChannel(ctx, s.routerPeerID) + s.updateManager.CloseChannel(ctx, s.unrelatedPeerID) + }) + settleAffectedUpdates(routerCh, unrelatedCh) + + syncWithVersion("0.29.0") + peerShouldReceiveUpdate(t, routerCh) + peerShouldNotReceiveUpdate(t, unrelatedCh) + + syncWithVersion("0.31.0") + peerShouldReceiveUpdate(t, routerCh) + peerShouldNotReceiveUpdate(t, unrelatedCh) +} diff --git a/management/server/affected_peers_router_test.go b/management/server/affected_peers_router_test.go index cc9df0a6a..9ecfaed69 100644 --- a/management/server/affected_peers_router_test.go +++ b/management/server/affected_peers_router_test.go @@ -60,6 +60,12 @@ func setupRouterScenario(t *testing.T, directRouterPeer bool) *routerScenario { manager, updateManager, err := createManager(t) require.NoError(t, err) + return buildRouterScenario(t, manager, updateManager, directRouterPeer) +} + +func buildRouterScenario(t *testing.T, manager *DefaultAccountManager, updateManager *update_channel.PeersUpdateManager, directRouterPeer bool) *routerScenario { + t.Helper() + ctx := context.Background() account, err := createAccount(manager, "router_scenario", userID, "") diff --git a/management/server/mock_server/account_mock.go b/management/server/mock_server/account_mock.go index 071e3771b..2f871c3e2 100644 --- a/management/server/mock_server/account_mock.go +++ b/management/server/mock_server/account_mock.go @@ -24,6 +24,7 @@ import ( "github.com/netbirdio/netbird/management/server/users" "github.com/netbirdio/netbird/route" "github.com/netbirdio/netbird/shared/management/domain" + "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" ) var _ account.Manager = (*MockAccountManager)(nil) @@ -41,11 +42,11 @@ type MockAccountManager struct { GetPeersFunc func(ctx context.Context, accountID, userID, nameFilter, ipFilter string) ([]*nbpeer.Peer, error) MarkPeerConnectedFunc func(ctx context.Context, peerKey string, accountID string, sessionStartedAt int64, nmap *types.NetworkMap) error MarkPeerDisconnectedFunc func(ctx context.Context, peerKey string, accountID string, sessionStartedAt int64) error - SyncAndMarkPeerFunc func(ctx context.Context, accountID string, peerPubKey string, meta nbpeer.PeerSystemMeta, realIP net.IP, syncTime time.Time) (*nbpeer.Peer, *types.NetworkMap, []*posture.Checks, int64, error) + SyncAndMarkPeerFunc func(ctx context.Context, accountID string, peerPubKey string, meta nbpeer.PeerSystemMeta, realIP net.IP, syncTime time.Time) (*nbpeer.Peer, *types.NetworkMap, []*nmdata.PostureChecks, int64, error) DeletePeerFunc func(ctx context.Context, accountID, peerKey, userID string) error GetNetworkMapFunc func(ctx context.Context, peerKey string) (*types.NetworkMap, error) GetPeerNetworkFunc func(ctx context.Context, peerKey string) (*types.Network, error) - AddPeerFunc func(ctx context.Context, accountID string, setupKey string, userId string, peer *nbpeer.Peer, temporary bool) (*nbpeer.Peer, *types.Network, []*posture.Checks, bool, error) + AddPeerFunc func(ctx context.Context, accountID string, setupKey string, userId string, peer *nbpeer.Peer, temporary bool) (*nbpeer.Peer, *types.Network, []*nmdata.PostureChecks, bool, error) GetGroupFunc func(ctx context.Context, accountID, groupID, userID string) (*types.Group, error) GetAllGroupsFunc func(ctx context.Context, accountID, userID string) ([]*types.Group, error) GetGroupByNameFunc func(ctx context.Context, groupName, accountID, userID string) (*types.Group, error) @@ -98,9 +99,9 @@ type MockAccountManager struct { SaveDNSSettingsFunc func(ctx context.Context, accountID, userID string, dnsSettingsToSave *types.DNSSettings) error GetPeerFunc func(ctx context.Context, accountID, peerID, userID string) (*nbpeer.Peer, error) UpdateAccountSettingsFunc func(ctx context.Context, accountID, userID string, newSettings *types.Settings) (*types.Settings, error) - LoginPeerFunc func(ctx context.Context, login types.PeerLogin) (*nbpeer.Peer, *types.Network, []*posture.Checks, bool, error) + LoginPeerFunc func(ctx context.Context, login types.PeerLogin) (*nbpeer.Peer, *types.Network, []*nmdata.PostureChecks, bool, error) ExtendPeerSessionFunc func(ctx context.Context, peerPubKey, userID string) (time.Time, error) - SyncPeerFunc func(ctx context.Context, sync types.PeerSync, accountID string) (*nbpeer.Peer, *types.NetworkMap, []*posture.Checks, int64, error) + SyncPeerFunc func(ctx context.Context, sync types.PeerSync, accountID string) (*nbpeer.Peer, *types.NetworkMap, []*nmdata.PostureChecks, int64, error) InviteUserFunc func(ctx context.Context, accountID string, initiatorUserID string, targetUserEmail string) error ApproveUserFunc func(ctx context.Context, accountID, initiatorUserID, targetUserID string) (*types.UserInfo, error) RejectUserFunc func(ctx context.Context, accountID, initiatorUserID, targetUserID string) error @@ -230,7 +231,7 @@ func (am *MockAccountManager) DeleteSetupKey(ctx context.Context, accountID, use return status.Errorf(codes.Unimplemented, "method DeleteSetupKey is not implemented") } -func (am *MockAccountManager) SyncAndMarkPeer(ctx context.Context, accountID string, peerPubKey string, meta nbpeer.PeerSystemMeta, realIP net.IP, syncTime time.Time) (*nbpeer.Peer, *types.NetworkMap, []*posture.Checks, int64, error) { +func (am *MockAccountManager) SyncAndMarkPeer(ctx context.Context, accountID string, peerPubKey string, meta nbpeer.PeerSystemMeta, realIP net.IP, syncTime time.Time) (*nbpeer.Peer, *types.NetworkMap, []*nmdata.PostureChecks, int64, error) { if am.SyncAndMarkPeerFunc != nil { return am.SyncAndMarkPeerFunc(ctx, accountID, peerPubKey, meta, realIP, syncTime) } @@ -424,7 +425,7 @@ func (am *MockAccountManager) AddPeer( userId string, peer *nbpeer.Peer, temporary bool, -) (*nbpeer.Peer, *types.Network, []*posture.Checks, bool, error) { +) (*nbpeer.Peer, *types.Network, []*nmdata.PostureChecks, bool, error) { if am.AddPeerFunc != nil { return am.AddPeerFunc(ctx, accountID, setupKey, userId, peer, temporary) } @@ -862,7 +863,7 @@ func (am *MockAccountManager) UpdateAccountSettings(ctx context.Context, account } // LoginPeer mocks LoginPeer of the AccountManager interface -func (am *MockAccountManager) LoginPeer(ctx context.Context, login types.PeerLogin) (*nbpeer.Peer, *types.Network, []*posture.Checks, bool, error) { +func (am *MockAccountManager) LoginPeer(ctx context.Context, login types.PeerLogin) (*nbpeer.Peer, *types.Network, []*nmdata.PostureChecks, bool, error) { if am.LoginPeerFunc != nil { return am.LoginPeerFunc(ctx, login) } @@ -878,7 +879,7 @@ func (am *MockAccountManager) ExtendPeerSession(ctx context.Context, peerPubKey, } // SyncPeer mocks SyncPeer of the AccountManager interface -func (am *MockAccountManager) SyncPeer(ctx context.Context, sync types.PeerSync, accountID string) (*nbpeer.Peer, *types.NetworkMap, []*posture.Checks, int64, error) { +func (am *MockAccountManager) SyncPeer(ctx context.Context, sync types.PeerSync, accountID string) (*nbpeer.Peer, *types.NetworkMap, []*nmdata.PostureChecks, int64, error) { if am.SyncPeerFunc != nil { return am.SyncPeerFunc(ctx, sync, accountID) } diff --git a/management/server/peer.go b/management/server/peer.go index 579ff2708..87ca57c2b 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -23,7 +23,6 @@ import ( "github.com/netbirdio/netbird/shared/management/domain" "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" - "github.com/netbirdio/netbird/management/server/posture" "github.com/netbirdio/netbird/management/server/store" "github.com/netbirdio/netbird/management/server/types" @@ -741,7 +740,7 @@ func (am *DefaultAccountManager) handleSetupKeyAddedPeer(ctx context.Context, en // to it. We also add the User ID to the peer metadata to identify registrant. If no userID provided, then fail with status.PermissionDenied // Each new Peer will be assigned a new next net.IP from the Account.Network and Account.Network.LastIP will be updated (IP's are not reused). // The peer property is just a placeholder for the Peer properties to pass further -func (am *DefaultAccountManager) AddPeer(ctx context.Context, accountID, setupKey, userID string, peer *nbpeer.Peer, temporary bool) (*nbpeer.Peer, *types.Network, []*posture.Checks, bool, error) { +func (am *DefaultAccountManager) AddPeer(ctx context.Context, accountID, setupKey, userID string, peer *nbpeer.Peer, temporary bool) (*nbpeer.Peer, *types.Network, []*nmdata.PostureChecks, bool, error) { if setupKey == "" && userID == "" && !peer.ProxyMeta.Embedded { // no auth method provided => reject access return nil, nil, nil, false, status.ErrNoAuthMethodProvided @@ -1001,7 +1000,7 @@ func getPeerIPDNSLabel(ip netip.Addr, peerHostName string) (string, error) { } // SyncPeer checks whether peer is eligible for receiving NetworkMap (authenticated) and returns its NetworkMap if eligible -func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync types.PeerSync, accountID string) (*nbpeer.Peer, *types.NetworkMap, []*posture.Checks, int64, error) { +func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync types.PeerSync, accountID string) (*nbpeer.Peer, *types.NetworkMap, []*nmdata.PostureChecks, int64, error) { var peer *nbpeer.Peer var ipv6CapabilityChanged bool var metaDiff nbpeer.MetaDiff @@ -1065,7 +1064,7 @@ func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync types.PeerSy return nil, nil, nil, 0, err } - metaDiffAffectsPosture := posture.AffectsPosture(ctx, &metaDiff, resPostureChecks) + metaDiffAffectsPosture := metaDiffAffectsPosture(&metaDiff, resPostureChecks) if requiresPeerUpdate(ctx, isStatusChanged, sync.UpdateAccountPeers, ipv6CapabilityChanged, metaDiffAffectsPosture, metaDiff.VersionChanged(), metaDiff.HostnameChanged()) { changedPeerIDs := []string{peer.ID} affectedPeerIDs := am.syncPeerAffectedPeers(ctx, accountID, peer.ID, nmap, peerNotValid, metaDiffAffectsPosture) @@ -1077,6 +1076,14 @@ func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync types.PeerSy return peer, nmap, resPostureChecks, dnsFwdPort, nil } +// metaDiffAffectsPosture reports whether the meta change flips the verdict of any of +// the peer's posture checks, replaying them against the old and new state. +func metaDiffAffectsPosture(diff *nbpeer.MetaDiff, checks []*nmdata.PostureChecks) bool { + oldPeer := types.TwinPeer(&nbpeer.Peer{Meta: diff.OldMeta, Location: diff.OldLocation}) + newPeer := types.TwinPeer(&nbpeer.Peer{Meta: diff.NewMeta, Location: diff.NewLocation}) + return nmdata.PostureVerdictChanged(checks, oldPeer, newPeer) +} + func requiresPeerUpdate(ctx context.Context, isStatusChanged, updateAccountPeers, ipv6CapabilityChanged, metaDiffAffectsPosture, versionChanged, hostname bool) bool { var reason string switch { @@ -1128,7 +1135,7 @@ func (am *DefaultAccountManager) markConnectedAffectedPeers(ctx context.Context, return affectedPeerIDsFromNetworkMap(nmap, peerID) } -func (am *DefaultAccountManager) handlePeerLoginNotFound(ctx context.Context, login types.PeerLogin, err error) (*nbpeer.Peer, *types.Network, []*posture.Checks, bool, error) { +func (am *DefaultAccountManager) handlePeerLoginNotFound(ctx context.Context, login types.PeerLogin, err error) (*nbpeer.Peer, *types.Network, []*nmdata.PostureChecks, bool, error) { if errStatus, ok := status.FromError(err); ok && errStatus.Type() == status.NotFound { // we couldn't find this peer by its public key which can mean that peer hasn't been registered yet. // Try registering it. @@ -1149,7 +1156,7 @@ func (am *DefaultAccountManager) handlePeerLoginNotFound(ctx context.Context, lo // LoginPeer logs in or registers a peer. // If peer doesn't exist the function checks whether a setup key or a user is present and registers a new peer if so. -func (am *DefaultAccountManager) LoginPeer(ctx context.Context, login types.PeerLogin) (*nbpeer.Peer, *types.Network, []*posture.Checks, bool, error) { +func (am *DefaultAccountManager) LoginPeer(ctx context.Context, login types.PeerLogin) (*nbpeer.Peer, *types.Network, []*nmdata.PostureChecks, bool, error) { accountID, err := am.Store.GetAccountIDByPeerPubKey(ctx, login.WireGuardPubKey) if err != nil { return am.handlePeerLoginNotFound(ctx, login, err) @@ -1322,7 +1329,7 @@ func (am *DefaultAccountManager) ExtendPeerSession(ctx context.Context, peerPubK // getPeerLoginInfo computes the login/register response data (network, posture // checks, SSH) from the store without building the peer's full network map. -func getPeerLoginInfo(ctx context.Context, transaction store.Store, accountID string, peer *nbpeer.Peer, isValid bool) (*types.Network, []*posture.Checks, bool, error) { +func getPeerLoginInfo(ctx context.Context, transaction store.Store, accountID string, peer *nbpeer.Peer, isValid bool) (*types.Network, []*nmdata.PostureChecks, bool, error) { network, err := transaction.GetAccountNetwork(ctx, store.LockingStrengthNone, accountID) if err != nil { return nil, nil, false, fmt.Errorf("get account network: %w", err) @@ -1364,7 +1371,7 @@ func isPeerSSHEnabled(ctx context.Context, peer *nbpeer.Peer, policies []*types. } // getPeerPostureChecks returns the posture checks for the peer. -func getPeerPostureChecks(ctx context.Context, transaction store.Store, accountID string, peerGroupIDs []string, policies []*types.Policy) ([]*posture.Checks, error) { +func getPeerPostureChecks(ctx context.Context, transaction store.Store, accountID string, peerGroupIDs []string, policies []*types.Policy) ([]*nmdata.PostureChecks, error) { if len(policies) == 0 { return nil, nil } @@ -1385,7 +1392,7 @@ func getPeerPostureChecks(ctx context.Context, transaction store.Store, accountI return nil, err } - return maps.Values(peerPostureChecks), nil + return types.TwinPostureChecksList(maps.Values(peerPostureChecks)), nil } // processPeerPostureChecks checks if the peer is in the source group of the policy and returns the posture checks. diff --git a/management/server/peer_posture_test.go b/management/server/peer_posture_test.go new file mode 100644 index 000000000..6b298f5d1 --- /dev/null +++ b/management/server/peer_posture_test.go @@ -0,0 +1,183 @@ +package server + +import ( + "net" + "net/netip" + "testing" + + "github.com/stretchr/testify/assert" + + nbpeer "github.com/netbirdio/netbird/management/server/peer" + "github.com/netbirdio/netbird/management/server/posture" + "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" +) + +func diffFrom(oldMeta, newMeta nbpeer.PeerSystemMeta, oldLoc, newLoc nbpeer.Location) *nbpeer.MetaDiff { + return &nbpeer.MetaDiff{ + OldMeta: oldMeta, + NewMeta: newMeta, + OldLocation: oldLoc, + NewLocation: newLoc, + } +} + +func postureBundle(def nmdata.ChecksDefinition) []*nmdata.PostureChecks { + return []*nmdata.PostureChecks{{Checks: def}} +} + +func TestMetaDiffAffectsPosture_NBVersion(t *testing.T) { + c := postureBundle(nmdata.ChecksDefinition{NBVersionCheck: &nmdata.NBVersionCheck{MinVersion: "1.2.0"}}) + + tests := []struct { + name string + oldVer, newVer string + want bool + }{ + {"both above min, no flip", "1.3.0", "1.4.0", false}, + {"both below min, no flip", "1.0.0", "1.1.0", false}, + {"crosses up below->above", "1.1.0", "1.3.0", true}, + {"crosses down above->below", "1.3.0", "1.1.0", true}, + {"unparsable old only -> flip", "garbage", "1.3.0", true}, + {"unparsable both -> no flip", "garbage", "junk", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + diff := diffFrom( + nbpeer.PeerSystemMeta{WtVersion: tt.oldVer}, + nbpeer.PeerSystemMeta{WtVersion: tt.newVer}, + nbpeer.Location{}, nbpeer.Location{}, + ) + assert.Equal(t, tt.want, metaDiffAffectsPosture(diff, c)) + }) + } +} + +func TestMetaDiffAffectsPosture_OSVersion_KernelBumpWithinMin(t *testing.T) { + c := postureBundle(nmdata.ChecksDefinition{OSVersionCheck: &nmdata.OSVersionCheck{ + Linux: &nmdata.MinKernelVersionCheck{MinKernelVersion: "5.0.0"}, + }}) + + withinMin := diffFrom( + nbpeer.PeerSystemMeta{GoOS: "linux", KernelVersion: "5.10.0-arch1"}, + nbpeer.PeerSystemMeta{GoOS: "linux", KernelVersion: "5.15.0-arch2"}, + nbpeer.Location{}, nbpeer.Location{}, + ) + assert.False(t, metaDiffAffectsPosture(withinMin, c)) + + crossesDown := diffFrom( + nbpeer.PeerSystemMeta{GoOS: "linux", KernelVersion: "5.10.0-arch1"}, + nbpeer.PeerSystemMeta{GoOS: "linux", KernelVersion: "4.19.0-arch1"}, + nbpeer.Location{}, nbpeer.Location{}, + ) + assert.True(t, metaDiffAffectsPosture(crossesDown, c)) +} + +func TestMetaDiffAffectsPosture_OSVersion_GoOSSwitchFlipsVerdict(t *testing.T) { + c := postureBundle(nmdata.ChecksDefinition{OSVersionCheck: &nmdata.OSVersionCheck{ + Linux: &nmdata.MinKernelVersionCheck{MinKernelVersion: "6.0.0"}, + }}) + + diff := diffFrom( + nbpeer.PeerSystemMeta{GoOS: "freebsd"}, + nbpeer.PeerSystemMeta{GoOS: "linux", KernelVersion: "4.19.0"}, + nbpeer.Location{}, nbpeer.Location{}, + ) + assert.True(t, metaDiffAffectsPosture(diff, c)) +} + +func TestMetaDiffAffectsPosture_Process_GoOSSwitchFlipsVerdict(t *testing.T) { + c := postureBundle(nmdata.ChecksDefinition{ProcessCheck: &nmdata.ProcessCheck{ + Processes: []nmdata.Process{{LinuxPath: "/usr/bin/foo"}}, + }}) + + files := []nbpeer.File{{Path: "/usr/bin/foo", ProcessIsRunning: true}} + diff := diffFrom( + nbpeer.PeerSystemMeta{GoOS: "linux", Files: files}, + nbpeer.PeerSystemMeta{GoOS: "windows", Files: files}, + nbpeer.Location{}, nbpeer.Location{}, + ) + assert.True(t, metaDiffAffectsPosture(diff, c)) +} + +func TestMetaDiffAffectsPosture_Process_UnrelatedFileChange(t *testing.T) { + c := postureBundle(nmdata.ChecksDefinition{ProcessCheck: &nmdata.ProcessCheck{ + Processes: []nmdata.Process{{LinuxPath: "/usr/bin/foo"}}, + }}) + + diff := diffFrom( + nbpeer.PeerSystemMeta{GoOS: "linux", Files: []nbpeer.File{ + {Path: "/usr/bin/foo", ProcessIsRunning: true}, + }}, + nbpeer.PeerSystemMeta{GoOS: "linux", Files: []nbpeer.File{ + {Path: "/usr/bin/foo", ProcessIsRunning: true}, + {Path: "/usr/bin/bar", ProcessIsRunning: true}, + }}, + nbpeer.Location{}, nbpeer.Location{}, + ) + assert.False(t, metaDiffAffectsPosture(diff, c)) +} + +func TestMetaDiffAffectsPosture_GeoLocation(t *testing.T) { + c := postureBundle(nmdata.ChecksDefinition{GeoLocationCheck: &nmdata.GeoLocationCheck{ + Action: posture.CheckActionAllow, + Locations: []nmdata.GeoLocation{{CountryCode: "DE"}}, + }}) + + stayAllowed := diffFrom( + nbpeer.PeerSystemMeta{}, nbpeer.PeerSystemMeta{}, + nbpeer.Location{CountryCode: "DE", CityName: "Berlin"}, + nbpeer.Location{CountryCode: "DE", CityName: "Munich"}, + ) + assert.False(t, metaDiffAffectsPosture(stayAllowed, c)) + + moveOut := diffFrom( + nbpeer.PeerSystemMeta{}, nbpeer.PeerSystemMeta{}, + nbpeer.Location{CountryCode: "DE"}, + nbpeer.Location{CountryCode: "FR"}, + ) + assert.True(t, metaDiffAffectsPosture(moveOut, c)) +} + +func TestMetaDiffAffectsPosture_PeerNetworkRange_ConnectionIP(t *testing.T) { + c := postureBundle(nmdata.ChecksDefinition{PeerNetworkRangeCheck: &nmdata.PeerNetworkRangeCheck{ + Action: posture.CheckActionAllow, + Ranges: []netip.Prefix{netip.MustParsePrefix("10.0.0.0/8")}, + }}) + + movesOutOfRange := diffFrom( + nbpeer.PeerSystemMeta{}, nbpeer.PeerSystemMeta{}, + nbpeer.Location{ConnectionIP: net.ParseIP("10.1.2.3")}, + nbpeer.Location{ConnectionIP: net.ParseIP("8.8.8.8")}, + ) + assert.True(t, metaDiffAffectsPosture(movesOutOfRange, c)) + + staysInRange := diffFrom( + nbpeer.PeerSystemMeta{}, nbpeer.PeerSystemMeta{}, + nbpeer.Location{ConnectionIP: net.ParseIP("10.1.2.3")}, + nbpeer.Location{ConnectionIP: net.ParseIP("10.9.9.9")}, + ) + assert.False(t, metaDiffAffectsPosture(staysInRange, c)) +} + +func TestMetaDiffAffectsPosture_IrrelevantFieldChange(t *testing.T) { + c := postureBundle(nmdata.ChecksDefinition{ + NBVersionCheck: &nmdata.NBVersionCheck{MinVersion: "1.0.0"}, + GeoLocationCheck: &nmdata.GeoLocationCheck{Action: posture.CheckActionAllow, Locations: []nmdata.GeoLocation{{CountryCode: "DE"}}}, + }) + + diff := diffFrom( + nbpeer.PeerSystemMeta{Hostname: "old", WtVersion: "1.5.0"}, + nbpeer.PeerSystemMeta{Hostname: "new", WtVersion: "1.5.0"}, + nbpeer.Location{CountryCode: "DE"}, nbpeer.Location{CountryCode: "DE"}, + ) + assert.False(t, metaDiffAffectsPosture(diff, c)) +} + +func TestMetaDiffAffectsPosture_NoChecks(t *testing.T) { + diff := diffFrom( + nbpeer.PeerSystemMeta{WtVersion: "1.0.0"}, + nbpeer.PeerSystemMeta{WtVersion: "2.0.0"}, + nbpeer.Location{}, nbpeer.Location{}, + ) + assert.False(t, metaDiffAffectsPosture(diff, nil)) +} diff --git a/management/server/peer_test.go b/management/server/peer_test.go index 9a662bdbf..22f2b9b6f 100644 --- a/management/server/peer_test.go +++ b/management/server/peer_test.go @@ -16,11 +16,11 @@ import ( "testing" "time" - "go.uber.org/mock/gomock" "github.com/rs/xid" log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" "golang.org/x/exp/maps" "golang.zx2c4.com/wireguard/wgctrl/wgtypes" @@ -1170,11 +1170,11 @@ func TestToSyncResponse(t *testing.T) { }, } dnsName := "example.com" - checks := []*posture.Checks{ + checks := []*nmdata.PostureChecks{ { - Checks: posture.ChecksDefinition{ - ProcessCheck: &posture.ProcessCheck{ - Processes: []posture.Process{{LinuxPath: "/usr/bin/netbird"}}, + Checks: nmdata.ChecksDefinition{ + ProcessCheck: &nmdata.ProcessCheck{ + Processes: []nmdata.Process{{LinuxPath: "/usr/bin/netbird"}}, }, }, }, diff --git a/management/server/posture/affects_posture_test.go b/management/server/posture/affects_posture_test.go deleted file mode 100644 index 6aa54d892..000000000 --- a/management/server/posture/affects_posture_test.go +++ /dev/null @@ -1,202 +0,0 @@ -package posture - -import ( - "context" - "net" - "net/netip" - "testing" - - "github.com/stretchr/testify/assert" - - nbpeer "github.com/netbirdio/netbird/management/server/peer" -) - -// diffFrom builds a MetaDiff from the old/new snapshots AffectsPosture replays against. -func diffFrom(oldMeta, newMeta nbpeer.PeerSystemMeta, oldLoc, newLoc nbpeer.Location) *nbpeer.MetaDiff { - return &nbpeer.MetaDiff{ - OldMeta: oldMeta, - NewMeta: newMeta, - OldLocation: oldLoc, - NewLocation: newLoc, - } -} - -func checks(def ChecksDefinition) []*Checks { - return []*Checks{{Checks: def}} -} - -func TestAffectsPosture_NilDiff(t *testing.T) { - assert.False(t, AffectsPosture(context.Background(), nil, checks(ChecksDefinition{ - NBVersionCheck: &NBVersionCheck{MinVersion: "1.0.0"}, - }))) -} - -func TestAffectsPosture_NBVersion(t *testing.T) { - c := checks(ChecksDefinition{NBVersionCheck: &NBVersionCheck{MinVersion: "1.2.0"}}) - - tests := []struct { - name string - oldVer, newVer string - want bool - }{ - {"both above min, no flip", "1.3.0", "1.4.0", false}, - {"both below min, no flip", "1.0.0", "1.1.0", false}, - {"crosses up below->above", "1.1.0", "1.3.0", true}, - {"crosses down above->below", "1.3.0", "1.1.0", true}, - {"unparsable old only -> flip", "garbage", "1.3.0", true}, - {"unparsable both -> no flip", "garbage", "junk", false}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - diff := diffFrom( - nbpeer.PeerSystemMeta{WtVersion: tt.oldVer}, - nbpeer.PeerSystemMeta{WtVersion: tt.newVer}, - nbpeer.Location{}, nbpeer.Location{}, - ) - assert.Equal(t, tt.want, AffectsPosture(context.Background(), diff, c)) - }) - } -} - -func TestAffectsPosture_OSVersion_KernelBumpWithinMin(t *testing.T) { - c := checks(ChecksDefinition{OSVersionCheck: &OSVersionCheck{ - Linux: &MinKernelVersionCheck{MinKernelVersion: "5.0.0"}, - }}) - - // Kernel moves but stays above the minimum: verdict stays pass -> not affected. - withinMin := diffFrom( - nbpeer.PeerSystemMeta{GoOS: "linux", KernelVersion: "5.10.0-arch1"}, - nbpeer.PeerSystemMeta{GoOS: "linux", KernelVersion: "5.15.0-arch2"}, - nbpeer.Location{}, nbpeer.Location{}, - ) - assert.False(t, AffectsPosture(context.Background(), withinMin, c)) - - // Kernel drops below the minimum: verdict flips pass -> fail -> affected. - crossesDown := diffFrom( - nbpeer.PeerSystemMeta{GoOS: "linux", KernelVersion: "5.10.0-arch1"}, - nbpeer.PeerSystemMeta{GoOS: "linux", KernelVersion: "4.19.0-arch1"}, - nbpeer.Location{}, nbpeer.Location{}, - ) - assert.True(t, AffectsPosture(context.Background(), crossesDown, c)) -} - -func TestAffectsPosture_OSVersion_GoOSSwitchFlipsVerdict(t *testing.T) { - // Only Linux is constrained. An OS outside the switch (freebsd) passes; switching to a - // failing linux kernel flips the verdict pass -> fail. - c := checks(ChecksDefinition{OSVersionCheck: &OSVersionCheck{ - Linux: &MinKernelVersionCheck{MinKernelVersion: "6.0.0"}, - }}) - - diff := diffFrom( - nbpeer.PeerSystemMeta{GoOS: "freebsd"}, - nbpeer.PeerSystemMeta{GoOS: "linux", KernelVersion: "4.19.0"}, - nbpeer.Location{}, nbpeer.Location{}, - ) - assert.True(t, AffectsPosture(context.Background(), diff, c)) -} - -func TestAffectsPosture_Process_GoOSSwitchFlipsVerdict(t *testing.T) { - // Process runs at a linux path. Switching GoOS to windows (no WindowsPath configured) - // flips the verdict. - c := checks(ChecksDefinition{ProcessCheck: &ProcessCheck{ - Processes: []Process{{LinuxPath: "/usr/bin/foo"}}, - }}) - - files := []nbpeer.File{{Path: "/usr/bin/foo", ProcessIsRunning: true}} - diff := diffFrom( - nbpeer.PeerSystemMeta{GoOS: "linux", Files: files}, - nbpeer.PeerSystemMeta{GoOS: "windows", Files: files}, - nbpeer.Location{}, nbpeer.Location{}, - ) - assert.True(t, AffectsPosture(context.Background(), diff, c)) -} - -func TestAffectsPosture_Process_UnrelatedFileChange(t *testing.T) { - // A tracked process stays running while an unrelated file is added: the verdict does - // not move, so posture is not affected. - c := checks(ChecksDefinition{ProcessCheck: &ProcessCheck{ - Processes: []Process{{LinuxPath: "/usr/bin/foo"}}, - }}) - - diff := diffFrom( - nbpeer.PeerSystemMeta{GoOS: "linux", Files: []nbpeer.File{ - {Path: "/usr/bin/foo", ProcessIsRunning: true}, - }}, - nbpeer.PeerSystemMeta{GoOS: "linux", Files: []nbpeer.File{ - {Path: "/usr/bin/foo", ProcessIsRunning: true}, - {Path: "/usr/bin/bar", ProcessIsRunning: true}, - }}, - nbpeer.Location{}, nbpeer.Location{}, - ) - assert.False(t, AffectsPosture(context.Background(), diff, c)) -} - -func TestAffectsPosture_GeoLocation(t *testing.T) { - c := checks(ChecksDefinition{GeoLocationCheck: &GeoLocationCheck{ - Action: CheckActionAllow, - Locations: []Location{{CountryCode: "DE"}}, - }}) - - // Moving within allowed countries keeps the verdict; moving out flips it. - stayAllowed := diffFrom( - nbpeer.PeerSystemMeta{}, nbpeer.PeerSystemMeta{}, - nbpeer.Location{CountryCode: "DE", CityName: "Berlin"}, - nbpeer.Location{CountryCode: "DE", CityName: "Munich"}, - ) - assert.False(t, AffectsPosture(context.Background(), stayAllowed, c)) - - moveOut := diffFrom( - nbpeer.PeerSystemMeta{}, nbpeer.PeerSystemMeta{}, - nbpeer.Location{CountryCode: "DE"}, - nbpeer.Location{CountryCode: "FR"}, - ) - assert.True(t, AffectsPosture(context.Background(), moveOut, c)) -} - -func TestAffectsPosture_PeerNetworkRange_ConnectionIP(t *testing.T) { - // The check reads the connection IP. Moving out of the allowed range flips the verdict; - // moving within it does not. - _, allowed, _ := net.ParseCIDR("10.0.0.0/8") - c := checks(ChecksDefinition{PeerNetworkRangeCheck: &PeerNetworkRangeCheck{ - Action: CheckActionAllow, - Ranges: []netip.Prefix{netip.MustParsePrefix(allowed.String())}, - }}) - - movesOutOfRange := diffFrom( - nbpeer.PeerSystemMeta{}, nbpeer.PeerSystemMeta{}, - nbpeer.Location{ConnectionIP: net.ParseIP("10.1.2.3")}, - nbpeer.Location{ConnectionIP: net.ParseIP("8.8.8.8")}, - ) - assert.True(t, AffectsPosture(context.Background(), movesOutOfRange, c)) - - staysInRange := diffFrom( - nbpeer.PeerSystemMeta{}, nbpeer.PeerSystemMeta{}, - nbpeer.Location{ConnectionIP: net.ParseIP("10.1.2.3")}, - nbpeer.Location{ConnectionIP: net.ParseIP("10.9.9.9")}, - ) - assert.False(t, AffectsPosture(context.Background(), staysInRange, c)) -} - -func TestAffectsPosture_IrrelevantFieldChange(t *testing.T) { - // Hostname changes but no check reads it: not affected even with checks present. - c := checks(ChecksDefinition{ - NBVersionCheck: &NBVersionCheck{MinVersion: "1.0.0"}, - GeoLocationCheck: &GeoLocationCheck{Action: CheckActionAllow, Locations: []Location{{CountryCode: "DE"}}}, - }) - - diff := diffFrom( - nbpeer.PeerSystemMeta{Hostname: "old", WtVersion: "1.5.0"}, - nbpeer.PeerSystemMeta{Hostname: "new", WtVersion: "1.5.0"}, - nbpeer.Location{CountryCode: "DE"}, nbpeer.Location{CountryCode: "DE"}, - ) - assert.False(t, AffectsPosture(context.Background(), diff, c)) -} - -func TestAffectsPosture_NoChecks(t *testing.T) { - diff := diffFrom( - nbpeer.PeerSystemMeta{WtVersion: "1.0.0"}, - nbpeer.PeerSystemMeta{WtVersion: "2.0.0"}, - nbpeer.Location{}, nbpeer.Location{}, - ) - assert.False(t, AffectsPosture(context.Background(), diff, nil)) -} diff --git a/management/server/posture/checks.go b/management/server/posture/checks.go index 72b719252..c38136d1c 100644 --- a/management/server/posture/checks.go +++ b/management/server/posture/checks.go @@ -7,7 +7,6 @@ import ( "regexp" "github.com/hashicorp/go-version" - log "github.com/sirupsen/logrus" nbpeer "github.com/netbirdio/netbird/management/server/peer" "github.com/netbirdio/netbird/shared/management/http/api" @@ -55,46 +54,6 @@ type Checks struct { Checks ChecksDefinition `gorm:"serializer:json"` } -// AffectsPosture reports whether the change in diff flips the verdict of any check. It -// replays each check against the peer's old and new state and compares verdicts, so a -// change that moves a field but stays the right side of a threshold (e.g. a kernel bump -// still above the minimum) does not force a re-evaluation. See verdictChanged for how an -// evaluation error counts. -func AffectsPosture(ctx context.Context, diff *nbpeer.MetaDiff, checks []*Checks) bool { - if diff == nil { - return false - } - - oldPeer := nbpeer.Peer{Meta: diff.OldMeta, Location: diff.OldLocation} - newPeer := nbpeer.Peer{Meta: diff.NewMeta, Location: diff.NewLocation} - - for _, c := range checks { - for _, check := range c.GetChecks() { - if verdictChanged(ctx, check, oldPeer, newPeer) { - return true - } - } - } - return false -} - -// verdictChanged replays check against old and new state and reports whether the verdict -// differs. Like callers, it treats an evaluation error as deny: two errors are the same -// verdict (no change), an error on one side only is a flip. -func verdictChanged(ctx context.Context, check Check, oldPeer, newPeer nbpeer.Peer) bool { - oldPass, oldErr := check.Check(ctx, oldPeer) - newPass, newErr := check.Check(ctx, newPeer) - - oldVerdict := oldPass && (oldErr == nil) - newVerdict := newPass && (newErr == nil) - changed := oldVerdict != newVerdict - - log.WithContext(ctx).Tracef("posture check %s replay: verdict %t -> %t (changed=%t), errs: %v -> %v", - check.Name(), oldVerdict, newVerdict, changed, oldErr, newErr) - - return changed -} - // ChecksDefinition contains definition of actual check type ChecksDefinition struct { NBVersionCheck *NBVersionCheck `json:",omitempty"` diff --git a/management/server/types/account_networkmapdata.go b/management/server/types/account_networkmapdata.go index 8f2e03a10..d554bfe80 100644 --- a/management/server/types/account_networkmapdata.go +++ b/management/server/types/account_networkmapdata.go @@ -93,7 +93,7 @@ func (a *Account) toNetworkMapData( } for _, pc := range a.PostureChecks { if pc != nil { - nmd.PostureChecks[pc.ID] = twinPostureChecks(pc) + nmd.PostureChecks[pc.ID] = TwinPostureChecks(pc) nmd.PostureCheckXIDToPublicID[pc.ID] = pc.PublicID } } @@ -391,7 +391,17 @@ func TwinNetwork(n *Network) *nmdata.Network { } } -func twinPostureChecks(pc *posture.Checks) *nmdata.PostureChecks { +// TwinPostureChecksList converts posture checks to their slim nmdata twins. +func TwinPostureChecksList(checks []*posture.Checks) []*nmdata.PostureChecks { + out := make([]*nmdata.PostureChecks, 0, len(checks)) + for _, pc := range checks { + out = append(out, TwinPostureChecks(pc)) + } + return out +} + +// TwinPostureChecks converts posture checks to their slim nmdata twin. +func TwinPostureChecks(pc *posture.Checks) *nmdata.PostureChecks { if pc == nil { return nil } diff --git a/shared/management/networkmap/nmdata/posture.go b/shared/management/networkmap/nmdata/posture.go index dc1753791..6a6b028c7 100644 --- a/shared/management/networkmap/nmdata/posture.go +++ b/shared/management/networkmap/nmdata/posture.go @@ -45,6 +45,22 @@ func PassesChecks(checks []Check, peer *Peer) bool { return true } +// PostureVerdictChanged reports whether any check in the bundles gives a different +// verdict for newPeer than for oldPeer. Checks are replayed one by one, so a change +// that moves a field but stays on the same side of a threshold does not count. An +// evaluation error is a deny, like in PassesChecks. +func PostureVerdictChanged(checks []*PostureChecks, oldPeer, newPeer *Peer) bool { + for _, pc := range checks { + for _, c := range pc.GetChecks() { + single := []Check{c} + if PassesChecks(single, oldPeer) != PassesChecks(single, newPeer) { + return true + } + } + } + return false +} + // GetChecks returns the initialized checks in the same order as posture.Checks.GetChecks. func (pc *PostureChecks) GetChecks() []Check { var checks []Check diff --git a/shared/management/networkmap/nmdata/posture_test.go b/shared/management/networkmap/nmdata/posture_test.go new file mode 100644 index 000000000..13e5f268e --- /dev/null +++ b/shared/management/networkmap/nmdata/posture_test.go @@ -0,0 +1,54 @@ +package nmdata + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func bundle(def ChecksDefinition) []*PostureChecks { + return []*PostureChecks{{Checks: def}} +} + +func TestPostureVerdictChanged_ErrorCountsAsDeny(t *testing.T) { + c := bundle(ChecksDefinition{NBVersionCheck: &NBVersionCheck{MinVersion: "1.2.0"}}) + + tests := []struct { + name string + oldVer, newVer string + want bool + }{ + {"both above min, no flip", "1.3.0", "1.4.0", false}, + {"crosses up below->above", "1.1.0", "1.3.0", true}, + {"unparsable old only -> flip", "garbage", "1.3.0", true}, + {"unparsable both -> no flip", "garbage", "junk", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + oldPeer := &Peer{Meta: PeerSystemMeta{WtVersion: tt.oldVer}} + newPeer := &Peer{Meta: PeerSystemMeta{WtVersion: tt.newVer}} + assert.Equal(t, tt.want, PostureVerdictChanged(c, oldPeer, newPeer)) + }) + } +} + +func TestPostureVerdictChanged_ReplaysEachCheck(t *testing.T) { + // Old fails the version check, new fails the kernel check: the bundle denies on + // both sides, yet every single check flipped, so the posture must be re-evaluated. + c := bundle(ChecksDefinition{ + NBVersionCheck: &NBVersionCheck{MinVersion: "1.0.0"}, + OSVersionCheck: &OSVersionCheck{Linux: &MinKernelVersionCheck{MinKernelVersion: "5.0.0"}}, + }) + oldPeer := &Peer{Meta: PeerSystemMeta{WtVersion: "0.9.0", GoOS: "linux", KernelVersion: "6.0.0"}} + newPeer := &Peer{Meta: PeerSystemMeta{WtVersion: "1.1.0", GoOS: "linux", KernelVersion: "4.0.0"}} + + assert.False(t, c[0].Passes(oldPeer)) + assert.False(t, c[0].Passes(newPeer)) + assert.True(t, PostureVerdictChanged(c, oldPeer, newPeer)) +} + +func TestPostureVerdictChanged_NoChecks(t *testing.T) { + oldPeer := &Peer{Meta: PeerSystemMeta{WtVersion: "1.0.0"}} + newPeer := &Peer{Meta: PeerSystemMeta{WtVersion: "2.0.0"}} + assert.False(t, PostureVerdictChanged(nil, oldPeer, newPeer)) +} From 353251d88696255f77be399da696f29f1792ff5d Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Fri, 28 Aug 2026 16:46:42 +0200 Subject: [PATCH 4/5] [management] fix posture check evaluation for direct peers in policy definition (#7348) --- .../network_map/controller/controller.go | 17 ++-- .../controller/posture_twin_test.go | 78 ++++++++++++----- .../policy-direct-peer-unvalidated/case.json | 5 ++ .../golden/peer-a.json | 64 ++++++++++++++ .../golden/peer-c.json | 64 ++++++++++++++ .../nmdata.json | 63 ++++++++++++++ .../cases/posture-direct-source/case.json | 5 ++ .../posture-direct-source/golden/peer-b.json | 33 +++++++ .../posture-direct-source/golden/peer-c.json | 64 ++++++++++++++ .../cases/posture-direct-source/nmdata.json | 51 +++++++++++ .../affected_peers_router_paths_test.go | 24 ++++- .../server/affected_peers_router_test.go | 17 ++++ management/server/peer.go | 14 +-- management/server/peer_posture_test.go | 20 +++++ management/server/types/account.go | 28 +++--- .../networkmap_components_correctness_test.go | 83 ++++++++++++++++++ .../networkmap/networkmapcompute.go | 87 ++++++++++--------- .../networkmap/networkmapcompute_test.go | 68 +++++++++++++-- .../management/types/networkmap_components.go | 32 +++---- 19 files changed, 698 insertions(+), 119 deletions(-) create mode 100644 management/internals/controllers/network_map/nmaptest/testdata/cases/policy-direct-peer-unvalidated/case.json create mode 100644 management/internals/controllers/network_map/nmaptest/testdata/cases/policy-direct-peer-unvalidated/golden/peer-a.json create mode 100644 management/internals/controllers/network_map/nmaptest/testdata/cases/policy-direct-peer-unvalidated/golden/peer-c.json create mode 100644 management/internals/controllers/network_map/nmaptest/testdata/cases/policy-direct-peer-unvalidated/nmdata.json create mode 100644 management/internals/controllers/network_map/nmaptest/testdata/cases/posture-direct-source/case.json create mode 100644 management/internals/controllers/network_map/nmaptest/testdata/cases/posture-direct-source/golden/peer-b.json create mode 100644 management/internals/controllers/network_map/nmaptest/testdata/cases/posture-direct-source/golden/peer-c.json create mode 100644 management/internals/controllers/network_map/nmaptest/testdata/cases/posture-direct-source/nmdata.json diff --git a/management/internals/controllers/network_map/controller/controller.go b/management/internals/controllers/network_map/controller/controller.go index f21f878a4..d72ba439d 100644 --- a/management/internals/controllers/network_map/controller/controller.go +++ b/management/internals/controllers/network_map/controller/controller.go @@ -577,7 +577,7 @@ func peerPostureChecksFromData(nmData *networkmap.NetworkMapData, peerID string) if policy == nil || !policy.Enabled || len(policy.SourcePostureChecks) == 0 { continue } - if !isPeerInPolicySourceGroupsFromData(nmData, peerID, policy) { + if !isPeerInPolicySourcesFromData(nmData, peerID, policy) { continue } for _, checkID := range policy.SourcePostureChecks { @@ -590,11 +590,14 @@ func peerPostureChecksFromData(nmData *networkmap.NetworkMapData, peerID string) return maps.Values(peerPostureChecks) } -func isPeerInPolicySourceGroupsFromData(nmData *networkmap.NetworkMapData, peerID string, policy *nmdata.Policy) bool { +func isPeerInPolicySourcesFromData(nmData *networkmap.NetworkMapData, peerID string, policy *nmdata.Policy) bool { for _, rule := range policy.Rules { if rule == nil || !rule.Enabled { continue } + if rule.SourceResource.Type == string(types.ResourceTypePeer) && rule.SourceResource.ID == peerID { + return true + } for _, groupID := range rule.Sources { if group := nmData.Groups[groupID]; group != nil && slices.Contains(group.Peers, peerID) { return true @@ -1314,7 +1317,7 @@ func computeForwarderPortFromVersions(wtVersions []string, requiredVersion strin // addPolicyPostureChecks adds posture checks from a policy to the peer posture checks map if the peer is in the policy's source groups. func addPolicyPostureChecks(account *types.Account, peerID string, policy *types.Policy, peerPostureChecks map[string]*posture.Checks) error { - isInGroup, err := isPeerInPolicySourceGroups(account, peerID, policy) + isInGroup, err := isPeerInPolicySources(account, peerID, policy) if err != nil { return err } @@ -1334,13 +1337,17 @@ func addPolicyPostureChecks(account *types.Account, peerID string, policy *types return nil } -// isPeerInPolicySourceGroups checks if a peer is present in any of the policy rule source groups. -func isPeerInPolicySourceGroups(account *types.Account, peerID string, policy *types.Policy) (bool, error) { +// isPeerInPolicySources checks if a peer is a source of the policy, directly or through a source group. +func isPeerInPolicySources(account *types.Account, peerID string, policy *types.Policy) (bool, error) { for _, rule := range policy.Rules { if !rule.Enabled { continue } + if rule.SourceResource.Type == types.ResourceTypePeer && rule.SourceResource.ID == peerID { + return true, nil + } + for _, sourceGroup := range rule.Sources { group := account.GetGroup(sourceGroup) if group == nil { diff --git a/management/internals/controllers/network_map/controller/posture_twin_test.go b/management/internals/controllers/network_map/controller/posture_twin_test.go index d5c9035e0..98e0991d0 100644 --- a/management/internals/controllers/network_map/controller/posture_twin_test.go +++ b/management/internals/controllers/network_map/controller/posture_twin_test.go @@ -4,35 +4,65 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "github.com/netbirdio/netbird/shared/management/networkmap" "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" + "github.com/netbirdio/netbird/shared/management/types" ) -func TestPeerPostureChecksFromData_ReturnsTwinsUnchanged(t *testing.T) { - check := &nmdata.PostureChecks{ - ID: "pc1", - Checks: nmdata.ChecksDefinition{ - NBVersionCheck: &nmdata.NBVersionCheck{MinVersion: "0.30.0"}, - OSVersionCheck: &nmdata.OSVersionCheck{Linux: &nmdata.MinKernelVersionCheck{MinKernelVersion: "6.1"}}, +func postureSelectionData(policies ...*nmdata.Policy) *networkmap.NetworkMapData { + return &networkmap.NetworkMapData{ + Groups: map[string]*nmdata.Group{"g-src": {ID: "g-src", Peers: []string{"peer-group"}}}, + Policies: policies, + PostureChecks: map[string]*nmdata.PostureChecks{ + "pc1": {ID: "pc1", Checks: nmdata.ChecksDefinition{NBVersionCheck: &nmdata.NBVersionCheck{MinVersion: "0.30.0"}}}, }, } - nmData := &networkmap.NetworkMapData{ - Groups: map[string]*nmdata.Group{"g1": {ID: "g1", Peers: []string{"peer1"}}}, - Policies: []*nmdata.Policy{{ - ID: "policy1", - Enabled: true, - SourcePostureChecks: []string{"pc1"}, - Rules: []*nmdata.PolicyRule{{ID: "rule1", Enabled: true, Sources: []string{"g1"}}}, - }}, - PostureChecks: map[string]*nmdata.PostureChecks{"pc1": check}, - } - - got := peerPostureChecksFromData(nmData, "peer1") - require.Len(t, got, 1) - assert.Same(t, check, got[0]) - assert.Len(t, got[0].GetChecks(), 2) - - assert.Empty(t, peerPostureChecksFromData(nmData, "peer-outside-source-group")) +} + +func gatedPolicy(id string, rule *nmdata.PolicyRule, checkIDs ...string) *nmdata.Policy { + return &nmdata.Policy{ID: id, Enabled: true, SourcePostureChecks: checkIDs, Rules: []*nmdata.PolicyRule{rule}} +} + +func checkIDs(checks []*nmdata.PostureChecks) []string { + ids := make([]string, 0, len(checks)) + for _, c := range checks { + ids = append(ids, c.ID) + } + return ids +} + +func TestPeerPostureChecksFromData_SelectsPolicySourcePeers(t *testing.T) { + groupRule := &nmdata.PolicyRule{ID: "r-group", Enabled: true, Sources: []string{"g-src"}, Destinations: []string{"g-dst"}} + directRule := &nmdata.PolicyRule{ID: "r-direct", Enabled: true, SourceResource: nmdata.Resource{ID: "peer-direct", Type: string(types.ResourceTypePeer)}, Destinations: []string{"g-dst"}} + + t.Run("source group member and direct source peer both get the checks", func(t *testing.T) { + nmData := postureSelectionData(gatedPolicy("p1", groupRule, "pc1"), gatedPolicy("p2", directRule, "pc1")) + + assert.Equal(t, []string{"pc1"}, checkIDs(peerPostureChecksFromData(nmData, "peer-group"))) + assert.Equal(t, []string{"pc1"}, checkIDs(peerPostureChecksFromData(nmData, "peer-direct"))) + assert.Empty(t, peerPostureChecksFromData(nmData, "peer-elsewhere")) + }) + + t.Run("source resource of a non-peer type never matches a peer", func(t *testing.T) { + hostRule := &nmdata.PolicyRule{ID: "r-host", Enabled: true, SourceResource: nmdata.Resource{ID: "peer-direct", Type: string(types.ResourceTypeHost)}, Destinations: []string{"g-dst"}} + nmData := postureSelectionData(gatedPolicy("p1", hostRule, "pc1")) + + assert.Empty(t, peerPostureChecksFromData(nmData, "peer-direct")) + }) + + t.Run("same check through two policies is returned once", func(t *testing.T) { + nmData := postureSelectionData(gatedPolicy("p1", groupRule, "pc1"), gatedPolicy("p2", groupRule, "pc1")) + + assert.Equal(t, []string{"pc1"}, checkIDs(peerPostureChecksFromData(nmData, "peer-group"))) + }) + + t.Run("disabled policy, disabled rule and dangling check are ignored", func(t *testing.T) { + disabledPolicy := gatedPolicy("p-off", groupRule, "pc1") + disabledPolicy.Enabled = false + disabledRule := &nmdata.PolicyRule{ID: "r-off", Enabled: false, Sources: []string{"g-src"}} + nmData := postureSelectionData(disabledPolicy, gatedPolicy("p-rule-off", disabledRule, "pc1"), gatedPolicy("p-dangling", groupRule, "pc-missing")) + + assert.Empty(t, peerPostureChecksFromData(nmData, "peer-group")) + }) } diff --git a/management/internals/controllers/network_map/nmaptest/testdata/cases/policy-direct-peer-unvalidated/case.json b/management/internals/controllers/network_map/nmaptest/testdata/cases/policy-direct-peer-unvalidated/case.json new file mode 100644 index 000000000..cdf31c413 --- /dev/null +++ b/management/internals/controllers/network_map/nmaptest/testdata/cases/policy-direct-peer-unvalidated/case.json @@ -0,0 +1,5 @@ +{ + "description": "A peer named directly as a rule source or destination is subject to approval exactly like a group member: unvalidated peer-b is neither a source for peer-c nor a destination for peer-a, while the validated direct source peer-a reaches peer-c. Legacy mode is excluded: the frozen legacynmap copy still carries the direct-peer bypass.", + "peers": ["peer-a", "peer-c"], + "modes": ["full", "envelope"] +} diff --git a/management/internals/controllers/network_map/nmaptest/testdata/cases/policy-direct-peer-unvalidated/golden/peer-a.json b/management/internals/controllers/network_map/nmaptest/testdata/cases/policy-direct-peer-unvalidated/golden/peer-a.json new file mode 100644 index 000000000..e0605525f --- /dev/null +++ b/management/internals/controllers/network_map/nmaptest/testdata/cases/policy-direct-peer-unvalidated/golden/peer-a.json @@ -0,0 +1,64 @@ +{ + "Serial": "22", + "peerConfig": { + "address": "100.64.0.1/10", + "sshConfig": {}, + "fqdn": "peer-a.netbird.test", + "autoUpdate": {} + }, + "remotePeers": [ + { + "wgPubKey": "4deEImv8zGvsyBmmfC2G0eQkbyMzyGuz/YK7pcYETwM=", + "allowedIps": [ + "100.64.0.3/32" + ], + "sshConfig": {}, + "fqdn": "peer-c.netbird.test", + "agentVersion": "0.60.0" + } + ], + "DNSConfig": { + "ServiceEnable": true, + "CustomZones": [ + { + "Domain": "netbird.test.", + "Records": [ + { + "Name": "peer-a.netbird.test", + "Type": "1", + "Class": "IN", + "TTL": "300", + "RData": "100.64.0.1" + }, + { + "Name": "peer-c.netbird.test", + "Type": "1", + "Class": "IN", + "TTL": "300", + "RData": "100.64.0.3" + } + ] + } + ], + "ForwarderPort": "22054" + }, + "FirewallRules": [ + { + "PeerIP": "100.64.0.3", + "Protocol": "TCP", + "Port": "443", + "PolicyID": "cG9sLWRpcmVjdC1vaw==" + }, + { + "PeerIP": "100.64.0.3", + "Direction": "OUT", + "Protocol": "TCP", + "Port": "443", + "PolicyID": "cG9sLWRpcmVjdC1vaw==" + } + ], + "routesFirewallRulesIsEmpty": true, + "sshAuth": { + "UserIDClaim": "sub" + } +} diff --git a/management/internals/controllers/network_map/nmaptest/testdata/cases/policy-direct-peer-unvalidated/golden/peer-c.json b/management/internals/controllers/network_map/nmaptest/testdata/cases/policy-direct-peer-unvalidated/golden/peer-c.json new file mode 100644 index 000000000..f2b3e9357 --- /dev/null +++ b/management/internals/controllers/network_map/nmaptest/testdata/cases/policy-direct-peer-unvalidated/golden/peer-c.json @@ -0,0 +1,64 @@ +{ + "Serial": "22", + "peerConfig": { + "address": "100.64.0.3/10", + "sshConfig": {}, + "fqdn": "peer-c.netbird.test", + "autoUpdate": {} + }, + "remotePeers": [ + { + "wgPubKey": "vblMc9U8RAI6cVopcKEMTVT6lVC3D9nTTMSwot5d3L4=", + "allowedIps": [ + "100.64.0.1/32" + ], + "sshConfig": {}, + "fqdn": "peer-a.netbird.test", + "agentVersion": "0.60.0" + } + ], + "DNSConfig": { + "ServiceEnable": true, + "CustomZones": [ + { + "Domain": "netbird.test.", + "Records": [ + { + "Name": "peer-a.netbird.test", + "Type": "1", + "Class": "IN", + "TTL": "300", + "RData": "100.64.0.1" + }, + { + "Name": "peer-c.netbird.test", + "Type": "1", + "Class": "IN", + "TTL": "300", + "RData": "100.64.0.3" + } + ] + } + ], + "ForwarderPort": "22054" + }, + "FirewallRules": [ + { + "PeerIP": "100.64.0.1", + "Protocol": "TCP", + "Port": "443", + "PolicyID": "cG9sLWRpcmVjdC1vaw==" + }, + { + "PeerIP": "100.64.0.1", + "Direction": "OUT", + "Protocol": "TCP", + "Port": "443", + "PolicyID": "cG9sLWRpcmVjdC1vaw==" + } + ], + "routesFirewallRulesIsEmpty": true, + "sshAuth": { + "UserIDClaim": "sub" + } +} diff --git a/management/internals/controllers/network_map/nmaptest/testdata/cases/policy-direct-peer-unvalidated/nmdata.json b/management/internals/controllers/network_map/nmaptest/testdata/cases/policy-direct-peer-unvalidated/nmdata.json new file mode 100644 index 000000000..283df304c --- /dev/null +++ b/management/internals/controllers/network_map/nmaptest/testdata/cases/policy-direct-peer-unvalidated/nmdata.json @@ -0,0 +1,63 @@ +{ + "Network": {"Serial": 22}, + "Peers": { + "peer-a": {"IP": "100.64.0.1", "Meta": {"WtVersion": "0.60.0"}}, + "peer-b": {"IP": "100.64.0.2", "Meta": {"WtVersion": "0.60.0"}}, + "peer-c": {"IP": "100.64.0.3", "Meta": {"WtVersion": "0.60.0"}} + }, + "ValidatedPeers": {"peer-a": {}, "peer-c": {}}, + "Groups": { + "grp-dev": {"Peers": ["peer-a"]}, + "grp-ops": {"Peers": ["peer-c"]} + }, + "Policies": [ + { + "ID": "pol-direct-ok", + "PublicID": "pol-direct-ok-pub", + "Enabled": true, + "Rules": [ + { + "Enabled": true, + "Action": "accept", + "Protocol": "tcp", + "Ports": ["443"], + "Bidirectional": true, + "SourceResource": {"ID": "peer-a", "Type": "peer"}, + "Destinations": ["grp-ops"] + } + ] + }, + { + "ID": "pol-src-unval", + "PublicID": "pol-src-unval-pub", + "Enabled": true, + "Rules": [ + { + "Enabled": true, + "Action": "accept", + "Protocol": "tcp", + "Ports": ["8443"], + "Bidirectional": true, + "SourceResource": {"ID": "peer-b", "Type": "peer"}, + "Destinations": ["grp-ops"] + } + ] + }, + { + "ID": "pol-dst-unval", + "PublicID": "pol-dst-unval-pub", + "Enabled": true, + "Rules": [ + { + "Enabled": true, + "Action": "accept", + "Protocol": "tcp", + "Ports": ["9443"], + "Bidirectional": true, + "Sources": ["grp-dev"], + "DestinationResource": {"ID": "peer-b", "Type": "peer"} + } + ] + } + ] +} diff --git a/management/internals/controllers/network_map/nmaptest/testdata/cases/posture-direct-source/case.json b/management/internals/controllers/network_map/nmaptest/testdata/cases/posture-direct-source/case.json new file mode 100644 index 000000000..8d7460721 --- /dev/null +++ b/management/internals/controllers/network_map/nmaptest/testdata/cases/posture-direct-source/case.json @@ -0,0 +1,5 @@ +{ + "description": "A peer named directly as a rule source is gated by the policy's posture checks exactly like a group member: peer-b (0.40.0) fails the 0.45.0 minimum, so it gets no connectivity and peer-c must not see it, while the compliant direct source peer-a reaches peer-c. Legacy mode is excluded: the frozen legacynmap copy still carries the direct-peer bypass.", + "peers": ["peer-b", "peer-c"], + "modes": ["full", "envelope"] +} diff --git a/management/internals/controllers/network_map/nmaptest/testdata/cases/posture-direct-source/golden/peer-b.json b/management/internals/controllers/network_map/nmaptest/testdata/cases/posture-direct-source/golden/peer-b.json new file mode 100644 index 000000000..240358e40 --- /dev/null +++ b/management/internals/controllers/network_map/nmaptest/testdata/cases/posture-direct-source/golden/peer-b.json @@ -0,0 +1,33 @@ +{ + "Serial": "21", + "peerConfig": { + "address": "100.64.0.2/10", + "sshConfig": {}, + "fqdn": "peer-b.netbird.test", + "autoUpdate": {} + }, + "remotePeersIsEmpty": true, + "DNSConfig": { + "ServiceEnable": true, + "CustomZones": [ + { + "Domain": "netbird.test.", + "Records": [ + { + "Name": "peer-b.netbird.test", + "Type": "1", + "Class": "IN", + "TTL": "300", + "RData": "100.64.0.2" + } + ] + } + ], + "ForwarderPort": "5353" + }, + "firewallRulesIsEmpty": true, + "routesFirewallRulesIsEmpty": true, + "sshAuth": { + "UserIDClaim": "sub" + } +} diff --git a/management/internals/controllers/network_map/nmaptest/testdata/cases/posture-direct-source/golden/peer-c.json b/management/internals/controllers/network_map/nmaptest/testdata/cases/posture-direct-source/golden/peer-c.json new file mode 100644 index 000000000..85573ed35 --- /dev/null +++ b/management/internals/controllers/network_map/nmaptest/testdata/cases/posture-direct-source/golden/peer-c.json @@ -0,0 +1,64 @@ +{ + "Serial": "21", + "peerConfig": { + "address": "100.64.0.3/10", + "sshConfig": {}, + "fqdn": "peer-c.netbird.test", + "autoUpdate": {} + }, + "remotePeers": [ + { + "wgPubKey": "vblMc9U8RAI6cVopcKEMTVT6lVC3D9nTTMSwot5d3L4=", + "allowedIps": [ + "100.64.0.1/32" + ], + "sshConfig": {}, + "fqdn": "peer-a.netbird.test", + "agentVersion": "0.60.0" + } + ], + "DNSConfig": { + "ServiceEnable": true, + "CustomZones": [ + { + "Domain": "netbird.test.", + "Records": [ + { + "Name": "peer-a.netbird.test", + "Type": "1", + "Class": "IN", + "TTL": "300", + "RData": "100.64.0.1" + }, + { + "Name": "peer-c.netbird.test", + "Type": "1", + "Class": "IN", + "TTL": "300", + "RData": "100.64.0.3" + } + ] + } + ], + "ForwarderPort": "5353" + }, + "FirewallRules": [ + { + "PeerIP": "100.64.0.1", + "Protocol": "TCP", + "Port": "443", + "PolicyID": "cG9sLWRpcmVjdC1vaw==" + }, + { + "PeerIP": "100.64.0.1", + "Direction": "OUT", + "Protocol": "TCP", + "Port": "443", + "PolicyID": "cG9sLWRpcmVjdC1vaw==" + } + ], + "routesFirewallRulesIsEmpty": true, + "sshAuth": { + "UserIDClaim": "sub" + } +} diff --git a/management/internals/controllers/network_map/nmaptest/testdata/cases/posture-direct-source/nmdata.json b/management/internals/controllers/network_map/nmaptest/testdata/cases/posture-direct-source/nmdata.json new file mode 100644 index 000000000..e6b99bfdd --- /dev/null +++ b/management/internals/controllers/network_map/nmaptest/testdata/cases/posture-direct-source/nmdata.json @@ -0,0 +1,51 @@ +{ + "Network": {"Serial": 21}, + "Peers": { + "peer-a": {"IP": "100.64.0.1", "Meta": {"WtVersion": "0.60.0"}}, + "peer-b": {"IP": "100.64.0.2", "Meta": {"WtVersion": "0.40.0"}}, + "peer-c": {"IP": "100.64.0.3", "Meta": {"WtVersion": "0.60.0"}} + }, + "Groups": { + "grp-ops": {"Peers": ["peer-c"]} + }, + "PostureChecks": { + "chk-ver": {"Checks": {"NBVersionCheck": {"MinVersion": "0.45.0"}}} + }, + "PostureCheckXIDToPublicID": {"chk-ver": "chk-ver-pub"}, + "Policies": [ + { + "ID": "pol-direct-ok", + "PublicID": "pol-direct-ok-pub", + "Enabled": true, + "SourcePostureChecks": ["chk-ver"], + "Rules": [ + { + "Enabled": true, + "Action": "accept", + "Protocol": "tcp", + "Ports": ["443"], + "Bidirectional": true, + "SourceResource": {"ID": "peer-a", "Type": "peer"}, + "Destinations": ["grp-ops"] + } + ] + }, + { + "ID": "pol-direct-denied", + "PublicID": "pol-direct-denied-pub", + "Enabled": true, + "SourcePostureChecks": ["chk-ver"], + "Rules": [ + { + "Enabled": true, + "Action": "accept", + "Protocol": "tcp", + "Ports": ["8443"], + "Bidirectional": true, + "SourceResource": {"ID": "peer-b", "Type": "peer"}, + "Destinations": ["grp-ops"] + } + ] + } + ] +} diff --git a/management/server/affected_peers_router_paths_test.go b/management/server/affected_peers_router_paths_test.go index 7fef1ab35..d5868a5c1 100644 --- a/management/server/affected_peers_router_paths_test.go +++ b/management/server/affected_peers_router_paths_test.go @@ -146,7 +146,7 @@ func TestAffectedPeers_GroupAddResource_RefreshesRoutingPeer(t *testing.T) { assert.NotContains(t, affected, s.unrelatedPeerID, "unrelated peer must not be affected") } -func (s *routerScenario) createPostureCheckGatedPolicy(t *testing.T, ctx context.Context) string { +func (s *routerScenario) createPostureCheckGatedPolicy(t *testing.T, ctx context.Context, policy *types.Policy) string { t.Helper() check, err := s.manager.SavePostureChecks(ctx, s.accountID, userID, &posture.Checks{ @@ -157,7 +157,6 @@ func (s *routerScenario) createPostureCheckGatedPolicy(t *testing.T, ctx context }, true) require.NoError(t, err) - policy := peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID) policy.SourcePostureChecks = []string{check.ID} _, err = s.manager.SavePolicy(ctx, s.accountID, userID, policy, true) require.NoError(t, err) @@ -169,7 +168,7 @@ func TestAffectedPeers_E2E_SavePostureCheck_RefreshesRoutingPeer(t *testing.T) { s := setupRouterScenario(t, true) ctx := context.Background() - checkID := s.createPostureCheckGatedPolicy(t, ctx) + checkID := s.createPostureCheckGatedPolicy(t, ctx, peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID)) srcCh := s.updateManager.CreateChannel(ctx, s.sourcePeerID) routerCh := s.updateManager.CreateChannel(ctx, s.routerPeerID) @@ -347,11 +346,28 @@ func TestAffectedPeers_PeerChange_RouterInOtherNetworkNotAffected(t *testing.T) // shortcut (the denied peer's map holds no router) and the allow direction // depends on which meta field moved, leaving the routers with a stale map. func TestAffectedPeers_E2E_PostureFlip_RefreshesRoutingPeer(t *testing.T) { + runPostureFlipRefreshesRoutingPeer(t, func(s *routerScenario) *types.Policy { + return peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID) + }) +} + +// TestAffectedPeers_E2E_PostureFlip_DirectSourcePeer_RefreshesRoutingPeer is the same +// scenario with the source peer named directly in the rule: it must receive its posture +// checks and have its flips detected exactly like a group member. +func TestAffectedPeers_E2E_PostureFlip_DirectSourcePeer_RefreshesRoutingPeer(t *testing.T) { + runPostureFlipRefreshesRoutingPeer(t, func(s *routerScenario) *types.Policy { + return peerToResourcePolicyByPeer(s.sourcePeerID, s.resourceGroupID) + }) +} + +func runPostureFlipRefreshesRoutingPeer(t *testing.T, policyFor func(s *routerScenario) *types.Policy) { + t.Helper() + manager, updateManager := createManagerWithNetworkMapStore(t) s := buildRouterScenario(t, manager, updateManager, true) ctx := context.Background() - s.createPostureCheckGatedPolicy(t, ctx) + s.createPostureCheckGatedPolicy(t, ctx, policyFor(s)) source, err := s.manager.Store.GetPeerByID(ctx, store.LockingStrengthNone, s.accountID, s.sourcePeerID) require.NoError(t, err) diff --git a/management/server/affected_peers_router_test.go b/management/server/affected_peers_router_test.go index 9ecfaed69..7e3f02b27 100644 --- a/management/server/affected_peers_router_test.go +++ b/management/server/affected_peers_router_test.go @@ -173,6 +173,23 @@ func peerToResourcePolicyByGroup(sourceGroupID, resourceGroupID string) *types.P } } +// peerToResourcePolicyByPeer builds a policy naming the source peer directly via +// SourceResource rather than through a group. +func peerToResourcePolicyByPeer(sourcePeerID, resourceGroupID string) *types.Policy { + return &types.Policy{ + Enabled: true, + Name: "peer-to-resource-by-peer", + Rules: []*types.PolicyRule{ + { + Enabled: true, + SourceResource: types.Resource{ID: sourcePeerID, Type: types.ResourceTypePeer}, + Destinations: []string{resourceGroupID}, + Action: types.PolicyTrafficActionAccept, + }, + }, + } +} + // peerToResourcePolicyByResource builds a policy referencing the resource // directly via DestinationResource rather than its group. func peerToResourcePolicyByResource(sourceGroupID, resourceID string) *types.Policy { diff --git a/management/server/peer.go b/management/server/peer.go index 87ca57c2b..07619f51e 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -1349,7 +1349,7 @@ func getPeerLoginInfo(ctx context.Context, transaction store.Store, accountID st return nil, nil, false, err } - postureChecks, err := getPeerPostureChecks(ctx, transaction, accountID, peerGroupIDs, policies) + postureChecks, err := getPeerPostureChecks(ctx, transaction, accountID, peer.ID, peerGroupIDs, policies) if err != nil { return nil, nil, false, err } @@ -1371,7 +1371,7 @@ func isPeerSSHEnabled(ctx context.Context, peer *nbpeer.Peer, policies []*types. } // getPeerPostureChecks returns the posture checks for the peer. -func getPeerPostureChecks(ctx context.Context, transaction store.Store, accountID string, peerGroupIDs []string, policies []*types.Policy) ([]*nmdata.PostureChecks, error) { +func getPeerPostureChecks(ctx context.Context, transaction store.Store, accountID, peerID string, peerGroupIDs []string, policies []*types.Policy) ([]*nmdata.PostureChecks, error) { if len(policies) == 0 { return nil, nil } @@ -1383,7 +1383,7 @@ func getPeerPostureChecks(ctx context.Context, transaction store.Store, accountI continue } - postureChecksIDs := processPeerPostureChecks(policy, peerGroupIDs) + postureChecksIDs := processPeerPostureChecks(policy, peerID, peerGroupIDs) peerPostureChecksIDs = append(peerPostureChecksIDs, postureChecksIDs...) } @@ -1395,13 +1395,17 @@ func getPeerPostureChecks(ctx context.Context, transaction store.Store, accountI return types.TwinPostureChecksList(maps.Values(peerPostureChecks)), nil } -// processPeerPostureChecks checks if the peer is in the source group of the policy and returns the posture checks. -func processPeerPostureChecks(policy *types.Policy, peerGroupIDs []string) []string { +// processPeerPostureChecks returns the policy's posture checks when the peer is a source of the policy, directly or through a source group. +func processPeerPostureChecks(policy *types.Policy, peerID string, peerGroupIDs []string) []string { for _, rule := range policy.Rules { if !rule.Enabled { continue } + if rule.SourceResource.Type == types.ResourceTypePeer && rule.SourceResource.ID == peerID { + return policy.SourcePostureChecks + } + for _, sourceGroup := range rule.Sources { if slices.Contains(peerGroupIDs, sourceGroup) { return policy.SourcePostureChecks diff --git a/management/server/peer_posture_test.go b/management/server/peer_posture_test.go index 6b298f5d1..88662e2fa 100644 --- a/management/server/peer_posture_test.go +++ b/management/server/peer_posture_test.go @@ -9,6 +9,7 @@ import ( nbpeer "github.com/netbirdio/netbird/management/server/peer" "github.com/netbirdio/netbird/management/server/posture" + "github.com/netbirdio/netbird/management/server/types" "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" ) @@ -181,3 +182,22 @@ func TestMetaDiffAffectsPosture_NoChecks(t *testing.T) { ) assert.False(t, metaDiffAffectsPosture(diff, nil)) } + +func TestProcessPeerPostureChecks(t *testing.T) { + policy := &types.Policy{ + Enabled: true, + SourcePostureChecks: []string{"pc1"}, + Rules: []*types.PolicyRule{ + {Enabled: false, Sources: []string{"g-disabled"}, SourceResource: types.Resource{ID: "peer-disabled", Type: types.ResourceTypePeer}}, + {Enabled: true, Sources: []string{"g-src"}, Destinations: []string{"g-dst"}}, + {Enabled: true, SourceResource: types.Resource{ID: "peer-direct", Type: types.ResourceTypePeer}, Destinations: []string{"g-dst"}}, + {Enabled: true, SourceResource: types.Resource{ID: "peer-as-host", Type: types.ResourceTypeHost}, Destinations: []string{"g-dst"}}, + }, + } + + assert.Equal(t, []string{"pc1"}, processPeerPostureChecks(policy, "peer-in-group", []string{"g-src"}), "source group member") + assert.Equal(t, []string{"pc1"}, processPeerPostureChecks(policy, "peer-direct", nil), "direct source peer") + assert.Empty(t, processPeerPostureChecks(policy, "peer-elsewhere", []string{"g-dst"}), "destination-only peer") + assert.Empty(t, processPeerPostureChecks(policy, "peer-disabled", []string{"g-disabled"}), "disabled rule") + assert.Empty(t, processPeerPostureChecks(policy, "peer-as-host", nil), "source resource of a non-peer type") +} diff --git a/management/server/types/account.go b/management/server/types/account.go index 522bb8be6..d689b0175 100644 --- a/management/server/types/account.go +++ b/management/server/types/account.go @@ -909,13 +909,13 @@ func (a *Account) GetPeerConnectionResources(ctx context.Context, peer *nbpeer.P var peerInSources, peerInDestinations bool if rule.SourceResource.Type == ResourceTypePeer && rule.SourceResource.ID != "" { - sourcePeers, peerInSources = a.getPeerFromResource(rule.SourceResource, peer.ID) + sourcePeers, peerInSources = a.getPeerFromResource(ctx, rule.SourceResource, peer.ID, policy.SourcePostureChecks, validatedPeersMap) } else { sourcePeers, peerInSources = a.getAllPeersFromGroups(ctx, rule.Sources, peer.ID, policy.SourcePostureChecks, validatedPeersMap) } if rule.DestinationResource.Type == ResourceTypePeer && rule.DestinationResource.ID != "" { - destinationPeers, peerInDestinations = a.getPeerFromResource(rule.DestinationResource, peer.ID) + destinationPeers, peerInDestinations = a.getPeerFromResource(ctx, rule.DestinationResource, peer.ID, nil, validatedPeersMap) } else { destinationPeers, peerInDestinations = a.getAllPeersFromGroups(ctx, rule.Destinations, peer.ID, nil, validatedPeersMap) } @@ -1120,8 +1120,17 @@ func ruleHasDestination(rule *PolicyRule, peerID string, peerGroupIDs map[string // Important: Posture checks are applicable only to source group peers, // for destination group peers, call this method with an empty list of sourcePostureChecksIDs func (a *Account) getAllPeersFromGroups(ctx context.Context, groups []string, peerID string, sourcePostureChecksIDs []string, validatedPeersMap map[string]struct{}) ([]*nbpeer.Peer, bool) { + return a.filterPolicyPeers(ctx, a.getUniquePeerIDsFromGroupsIDs(ctx, groups), peerID, sourcePostureChecksIDs, validatedPeersMap) +} + +// getPeerFromResource resolves a rule side that names a peer directly, admitting it +// like a member of a group holding only that peer. +func (a *Account) getPeerFromResource(ctx context.Context, resource Resource, peerID string, sourcePostureChecksIDs []string, validatedPeersMap map[string]struct{}) ([]*nbpeer.Peer, bool) { + return a.filterPolicyPeers(ctx, []string{resource.ID}, peerID, sourcePostureChecksIDs, validatedPeersMap) +} + +func (a *Account) filterPolicyPeers(ctx context.Context, uniquePeerIDs []string, peerID string, sourcePostureChecksIDs []string, validatedPeersMap map[string]struct{}) ([]*nbpeer.Peer, bool) { peerInGroups := false - uniquePeerIDs := a.getUniquePeerIDsFromGroupsIDs(ctx, groups) filteredPeers := make([]*nbpeer.Peer, 0, len(uniquePeerIDs)) for _, p := range uniquePeerIDs { peer, ok := a.Peers[p] @@ -1150,19 +1159,6 @@ func (a *Account) getAllPeersFromGroups(ctx context.Context, groups []string, pe return filteredPeers, peerInGroups } -func (a *Account) getPeerFromResource(resource Resource, peerID string) ([]*nbpeer.Peer, bool) { - peer := a.GetPeer(resource.ID) - if peer == nil { - return []*nbpeer.Peer{}, false - } - - if peer.ID == peerID { - return []*nbpeer.Peer{}, true - } - - return []*nbpeer.Peer{peer}, false -} - // validatePostureChecksOnPeer validates the posture checks on a peer func (a *Account) validatePostureChecksOnPeer(ctx context.Context, sourcePostureChecksID []string, peerID string) bool { peer, ok := a.Peers[peerID] diff --git a/management/server/types/networkmap_components_correctness_test.go b/management/server/types/networkmap_components_correctness_test.go index eb3e4fe3b..35b5f7149 100644 --- a/management/server/types/networkmap_components_correctness_test.go +++ b/management/server/types/networkmap_components_correctness_test.go @@ -875,6 +875,89 @@ func TestComponents_PeerAsSourceResource(t *testing.T) { assert.True(t, has443, "peer-0 as source resource should have port 443 rule") } +func hasFirewallRuleTo(nm *types.NetworkMap, peerIP, port string) bool { + for _, rule := range nm.FirewallRules { + if rule.PeerIP == peerIP && rule.Port == port { + return true + } + } + return false +} + +// TestComponents_PeerAsSourceResource_PostureChecks verifies that a directly referenced +// source peer is gated by the policy's posture checks like a member of a group holding only +// that peer: peer-1 (0.25.0) fails the 0.26.0 minimum, peer-2 (0.40.0) passes. +func TestComponents_PeerAsSourceResource_PostureChecks(t *testing.T) { + account, validatedPeers := scalableTestAccountWithoutDefaultPolicy(20, 2) + + for _, sourcePeerID := range []string{"peer-1", "peer-2"} { + account.Policies = append(account.Policies, &types.Policy{ + ID: "policy-peer-src-" + sourcePeerID, Name: "Peer Source " + sourcePeerID, Enabled: true, AccountID: "test-account", + SourcePostureChecks: []string{"posture-check-ver"}, + Rules: []*types.PolicyRule{{ + ID: "rule-peer-src-" + sourcePeerID, Enabled: true, + Action: types.PolicyTrafficActionAccept, + Protocol: types.PolicyRuleProtocolTCP, + Bidirectional: true, + Ports: []string{"9443"}, + SourceResource: types.Resource{ID: sourcePeerID, Type: types.ResourceTypePeer}, + Destinations: []string{"group-0"}, + }}, + }) + } + + nm0 := componentsNetworkMap(account, "peer-0", validatedPeers) + require.NotNil(t, nm0) + assert.False(t, hasFirewallRuleTo(nm0, "100.64.0.1", "9443"), "destination must not see the direct source peer failing the posture check") + assert.True(t, hasFirewallRuleTo(nm0, "100.64.0.2", "9443"), "destination must see the direct source peer passing the posture check") + + nm1 := componentsNetworkMap(account, "peer-1", validatedPeers) + require.NotNil(t, nm1) + assert.False(t, hasFirewallRuleTo(nm1, "100.64.0.0", "9443"), "a direct source peer failing the posture check gets no policy connectivity") + + nm2 := componentsNetworkMap(account, "peer-2", validatedPeers) + require.NotNil(t, nm2) + assert.True(t, hasFirewallRuleTo(nm2, "100.64.0.0", "9443"), "a direct source peer passing the posture check gets policy connectivity") +} + +// TestComponents_PeerAsResource_Unvalidated verifies that a directly referenced peer is +// subject to approval like a group member, whether it is the rule's source or destination. +func TestComponents_PeerAsResource_Unvalidated(t *testing.T) { + account, validatedPeers := scalableTestAccountWithoutDefaultPolicy(20, 2) + delete(validatedPeers, "peer-2") + + account.Policies = append(account.Policies, + &types.Policy{ + ID: "policy-unval-src", Name: "Unvalidated Source", Enabled: true, AccountID: "test-account", + Rules: []*types.PolicyRule{{ + ID: "rule-unval-src", Enabled: true, + Action: types.PolicyTrafficActionAccept, Protocol: types.PolicyRuleProtocolTCP, Bidirectional: true, + Ports: []string{"9443"}, + SourceResource: types.Resource{ID: "peer-2", Type: types.ResourceTypePeer}, + Destinations: []string{"group-0"}, + }}, + }, + &types.Policy{ + ID: "policy-unval-dst", Name: "Unvalidated Destination", Enabled: true, AccountID: "test-account", + Rules: []*types.PolicyRule{{ + ID: "rule-unval-dst", Enabled: true, + Action: types.PolicyTrafficActionAccept, Protocol: types.PolicyRuleProtocolTCP, Bidirectional: true, + Ports: []string{"9444"}, + Sources: []string{"group-0"}, + DestinationResource: types.Resource{ID: "peer-2", Type: types.ResourceTypePeer}, + }}, + }, + ) + + nm0 := componentsNetworkMap(account, "peer-0", validatedPeers) + require.NotNil(t, nm0) + assert.False(t, hasFirewallRuleTo(nm0, "100.64.0.2", "9443"), "an unvalidated direct source peer must not be admitted") + assert.False(t, hasFirewallRuleTo(nm0, "100.64.0.2", "9444"), "an unvalidated direct destination peer must not be admitted") + for _, p := range nm0.Peers { + assert.NotEqual(t, "peer-2", p.ID, "an unvalidated direct peer must not be shipped as a remote peer") + } +} + // TestComponents_PeerAsDestinationResource verifies that a policy with DestinationResource.Type=Peer // targets only that specific peer as the destination. func TestComponents_PeerAsDestinationResource(t *testing.T) { diff --git a/shared/management/networkmap/networkmapcompute.go b/shared/management/networkmap/networkmapcompute.go index 1cf7aeef4..65e76d097 100644 --- a/shared/management/networkmap/networkmapcompute.go +++ b/shared/management/networkmap/networkmapcompute.go @@ -324,19 +324,13 @@ func (nmd *NetworkMapData) getPeersGroupsPoliciesRoutes( var peerInSources, peerInDestinations bool if rule.SourceResource.Type == string(types.ResourceTypePeer) && rule.SourceResource.ID != "" { - sourcePeers = []string{rule.SourceResource.ID} - if rule.SourceResource.ID == peerID { - peerInSources = true - } + sourcePeers, peerInSources = nmd.getPeerFromResource(rule.SourceResource, peerID, policy.SourcePostureChecks, postureFailedPeers) } else { sourcePeers, peerInSources = nmd.getPeersFromGroups(rule.Sources, peerID, policy.SourcePostureChecks, postureFailedPeers) } if rule.DestinationResource.Type == string(types.ResourceTypePeer) && rule.DestinationResource.ID != "" { - destinationPeers = []string{rule.DestinationResource.ID} - if rule.DestinationResource.ID == peerID { - peerInDestinations = true - } + destinationPeers, peerInDestinations = nmd.getPeerFromResource(rule.DestinationResource, peerID, nil, postureFailedPeers) } else { destinationPeers, peerInDestinations = nmd.getPeersFromGroups(rule.Destinations, peerID, nil, postureFailedPeers) } @@ -403,30 +397,16 @@ func (nmd *NetworkMapData) getPeersFromGroups(groups []string, peerID string, so filteredPeerIDs = make([]string, 0, len(group.Peers)) peerInGroups = false for _, pid := range group.Peers { - peer, ok := nmd.Peers[pid] - if !ok || peer == nil { + if !nmd.admitPolicyPeer(pid, sourcePostureChecksIDs, postureFailedPeers) { continue } - if _, ok := nmd.ValidatedPeers[peer.ID]; !ok { - continue - } - - isValid, pname := nmd.validatePostureChecksOnPeerGetFailed(sourcePostureChecksIDs, peer.ID) - if !isValid && len(pname) > 0 { - if _, ok := (*postureFailedPeers)[pname]; !ok { - (*postureFailedPeers)[pname] = make(map[string]struct{}) - } - (*postureFailedPeers)[pname][peer.ID] = struct{}{} - continue - } - - if peer.ID == peerID { + if pid == peerID { peerInGroups = true continue } - filteredPeerIDs = append(filteredPeerIDs, peer.ID) + filteredPeerIDs = append(filteredPeerIDs, pid) } return filteredPeerIDs, peerInGroups } @@ -436,36 +416,59 @@ func (nmd *NetworkMapData) getPeersFromGroups(groups []string, peerID string, so continue } seenPeerIds[pid] = struct{}{} - peer, ok := nmd.Peers[pid] - if !ok || peer == nil { + if !nmd.admitPolicyPeer(pid, sourcePostureChecksIDs, postureFailedPeers) { continue } - if _, ok := nmd.ValidatedPeers[peer.ID]; !ok { - continue - } - - isValid, pname := nmd.validatePostureChecksOnPeerGetFailed(sourcePostureChecksIDs, peer.ID) - if !isValid && len(pname) > 0 { - if _, ok := (*postureFailedPeers)[pname]; !ok { - (*postureFailedPeers)[pname] = make(map[string]struct{}) - } - (*postureFailedPeers)[pname][peer.ID] = struct{}{} - continue - } - - if peer.ID == peerID { + if pid == peerID { peerInGroups = true continue } - filteredPeerIDs = append(filteredPeerIDs, peer.ID) + filteredPeerIDs = append(filteredPeerIDs, pid) } } return filteredPeerIDs, peerInGroups } +// getPeerFromResource resolves a rule side that names a peer directly, admitting it +// like a member of a group holding only that peer. +func (nmd *NetworkMapData) getPeerFromResource(resource nmdata.Resource, peerID string, sourcePostureChecksIDs []string, + postureFailedPeers *map[string]map[string]struct{}) ([]string, bool) { + if !nmd.admitPolicyPeer(resource.ID, sourcePostureChecksIDs, postureFailedPeers) { + return nil, false + } + if resource.ID == peerID { + return nil, true + } + return []string{resource.ID}, false +} + +// admitPolicyPeer applies the per-peer admission of a rule side: the peer must exist, +// be validated and pass the rule's posture checks. A failed check is recorded in +// postureFailedPeers. +func (nmd *NetworkMapData) admitPolicyPeer(pid string, sourcePostureChecksIDs []string, postureFailedPeers *map[string]map[string]struct{}) bool { + peer, ok := nmd.Peers[pid] + if !ok || peer == nil { + return false + } + + if _, ok := nmd.ValidatedPeers[pid]; !ok { + return false + } + + isValid, pname := nmd.validatePostureChecksOnPeerGetFailed(sourcePostureChecksIDs, pid) + if !isValid && len(pname) > 0 { + if _, ok := (*postureFailedPeers)[pname]; !ok { + (*postureFailedPeers)[pname] = make(map[string]struct{}) + } + (*postureFailedPeers)[pname][pid] = struct{}{} + return false + } + return true +} + func (nmd *NetworkMapData) validatePostureChecksOnPeerGetFailed(sourcePostureChecksID []string, peerID string) (bool, string) { peer, ok := nmd.Peers[peerID] if !ok || peer == nil { diff --git a/shared/management/networkmap/networkmapcompute_test.go b/shared/management/networkmap/networkmapcompute_test.go index 8c9add8c1..ad21fd70f 100644 --- a/shared/management/networkmap/networkmapcompute_test.go +++ b/shared/management/networkmap/networkmapcompute_test.go @@ -448,10 +448,9 @@ func TestGetPeerNetworkMapComponents_PeerResourceRules(t *testing.T) { assert.ElementsMatch(t, []string{targetID, remote.ID}, peerIDSet(c.Peers)) }) - // Legacy parity: directly referenced peers bypass the ValidatedPeers gate - // and posture checks that group-derived peers go through; the client-side - // Calculate shares this behavior via getPeerFromResource. - t.Run("unvalidated source resource peer still connects", func(t *testing.T) { + // A directly referenced peer is admitted like a member of a group holding only + // that peer: the ValidatedPeers gate and the posture checks apply equally. + t.Run("unvalidated source resource peer is excluded", func(t *testing.T) { target := newPeer(targetID, 1) unval := newPeer("peer-unval", 2) nmd := newNMD(target, unval) @@ -463,10 +462,10 @@ func TestGetPeerNetworkMapComponents_PeerResourceRules(t *testing.T) { c := compute(nmd, targetID) - assert.ElementsMatch(t, []string{targetID, unval.ID}, peerIDSet(c.Peers)) + assert.ElementsMatch(t, []string{targetID}, peerIDSet(c.Peers)) }) - t.Run("source resource peer bypasses posture checks", func(t *testing.T) { + t.Run("source resource peer failing posture checks is excluded", func(t *testing.T) { target := newPeer(targetID, 1) failing := newPeer("peer-failing", 2) failing.Meta.WtVersion = failingVersion @@ -481,10 +480,65 @@ func TestGetPeerNetworkMapComponents_PeerResourceRules(t *testing.T) { c := compute(nmd, targetID) - assert.ElementsMatch(t, []string{targetID, failing.ID}, peerIDSet(c.Peers)) + assert.ElementsMatch(t, []string{targetID}, peerIDSet(c.Peers)) assert.Empty(t, c.PostureFailedPeers) }) + t.Run("direct source peer failure recorded when connected via another policy", func(t *testing.T) { + target := newPeer(targetID, 1) + failing := newPeer("peer-failing", 2) + failing.Meta.WtVersion = failingVersion + nmd := newNMD(target, failing) + addVersionCheck(nmd, "pc-1", postureMinVersion) + addGroup(nmd, "g-dst", targetID) + checkedRule := newRule(nil, []string{"g-dst"}) + checkedRule.SourceResource = peerResource(failing.ID) + checked := newPolicy("p-checked", checkedRule) + checked.SourcePostureChecks = []string{"pc-1"} + openRule := newRule(nil, []string{"g-dst"}) + openRule.SourceResource = peerResource(failing.ID) + nmd.Policies = []*nmdata.Policy{checked, newPolicy("p-open", openRule)} + + c := compute(nmd, targetID) + + assert.ElementsMatch(t, []string{targetID, failing.ID}, peerIDSet(c.Peers)) + assert.Equal(t, map[string]map[string]struct{}{"pc-1": {failing.ID: {}}}, c.PostureFailedPeers) + }) + + t.Run("target as source resource failing posture checks gets no policy", func(t *testing.T) { + target := newPeer(targetID, 1) + target.Meta.WtVersion = failingVersion + dst := newPeer("peer-dst", 2) + nmd := newNMD(target, dst) + addVersionCheck(nmd, "pc-1", postureMinVersion) + addGroup(nmd, "g-dst", dst.ID) + rule := newRule(nil, []string{"g-dst"}) + rule.SourceResource = peerResource(targetID) + p := newPolicy("p-1", rule) + p.SourcePostureChecks = []string{"pc-1"} + nmd.Policies = []*nmdata.Policy{p} + + c := compute(nmd, targetID) + + assert.Empty(t, policyIDs(c.Policies)) + assert.ElementsMatch(t, []string{targetID}, peerIDSet(c.Peers)) + }) + + t.Run("unvalidated destination resource peer is excluded", func(t *testing.T) { + target := newPeer(targetID, 1) + unval := newPeer("peer-unval", 2) + nmd := newNMD(target, unval) + delete(nmd.ValidatedPeers, unval.ID) + addGroup(nmd, "g-src", targetID) + rule := newRule([]string{"g-src"}, nil) + rule.DestinationResource = peerResource(unval.ID) + nmd.Policies = []*nmdata.Policy{newPolicy("p-1", rule)} + + c := compute(nmd, targetID) + + assert.ElementsMatch(t, []string{targetID}, peerIDSet(c.Peers)) + }) + t.Run("unrelated peer resource rule ignored", func(t *testing.T) { target := newPeer(targetID, 1) a := newPeer("peer-a", 2) diff --git a/shared/management/types/networkmap_components.go b/shared/management/types/networkmap_components.go index d008ece83..e18db4ec0 100644 --- a/shared/management/types/networkmap_components.go +++ b/shared/management/types/networkmap_components.go @@ -230,13 +230,13 @@ func (c *NetworkMapComponents) getPeerConnectionResources(targetPeerID string) ( var peerInSources, peerInDestinations bool if rule.SourceResource.Type == string(ResourceTypePeer) && rule.SourceResource.ID != "" { - sourcePeers, peerInSources = c.getPeerFromResource(rule.SourceResource, targetPeerID) + sourcePeers, peerInSources = c.getPeerFromResource(rule.SourceResource, targetPeerID, policy.SourcePostureChecks) } else { sourcePeers, peerInSources = c.getAllPeersFromGroups(rule.Sources, targetPeerID, policy.SourcePostureChecks) } if rule.DestinationResource.Type == string(ResourceTypePeer) && rule.DestinationResource.ID != "" { - destinationPeers, peerInDestinations = c.getPeerFromResource(rule.DestinationResource, targetPeerID) + destinationPeers, peerInDestinations = c.getPeerFromResource(rule.DestinationResource, targetPeerID, nil) } else { destinationPeers, peerInDestinations = c.getAllPeersFromGroups(rule.Destinations, targetPeerID, nil) } @@ -373,8 +373,21 @@ func (c *NetworkMapComponents) connResourcesGenerator(targetPeer *nmdata.Peer) ( } func (c *NetworkMapComponents) getAllPeersFromGroups(groups []string, peerID string, sourcePostureChecksIDs []string) ([]*nmdata.Peer, bool) { + return c.filterPolicyPeers(c.getUniquePeerIDsFromGroupsIDs(groups), peerID, sourcePostureChecksIDs) +} + +// getPeerFromResource resolves a rule side that names a peer directly. The peer is +// subject to the same admission as a group member, so a direct peer behaves exactly +// like a group holding only that peer. +func (c *NetworkMapComponents) getPeerFromResource(resource nmdata.Resource, peerID string, sourcePostureChecksIDs []string) ([]*nmdata.Peer, bool) { + return c.filterPolicyPeers([]string{resource.ID}, peerID, sourcePostureChecksIDs) +} + +// filterPolicyPeers admits the peers of one rule side: known to the components and +// passing the rule's posture checks. It reports the admitted peers other than peerID +// and whether peerID itself is admitted on that side. +func (c *NetworkMapComponents) filterPolicyPeers(uniquePeerIDs []string, peerID string, sourcePostureChecksIDs []string) ([]*nmdata.Peer, bool) { peerInGroups := false - uniquePeerIDs := c.getUniquePeerIDsFromGroupsIDs(groups) filteredPeers := make([]*nmdata.Peer, 0, len(uniquePeerIDs)) for _, p := range uniquePeerIDs { @@ -427,19 +440,6 @@ func (c *NetworkMapComponents) getUniquePeerIDsFromGroupsIDs(groups []string) [] return ids } -func (c *NetworkMapComponents) getPeerFromResource(resource nmdata.Resource, peerID string) ([]*nmdata.Peer, bool) { - if resource.ID == peerID { - return []*nmdata.Peer{}, true - } - - peerInfo := c.GetPeerInfo(resource.ID) - if peerInfo == nil { - return []*nmdata.Peer{}, false - } - - return []*nmdata.Peer{peerInfo}, false -} - func (c *NetworkMapComponents) filterPeersByLoginExpiration(aclPeers []*nmdata.Peer) ([]*nmdata.Peer, []*nmdata.Peer) { peersToConnect := make([]*nmdata.Peer, 0, len(aclPeers)) var expiredPeers []*nmdata.Peer From 11733fd718fb7889cfbf8b4e6498c0b4e74a1eb0 Mon Sep 17 00:00:00 2001 From: Bethuel Mmbaga Date: Fri, 28 Aug 2026 18:11:57 +0300 Subject: [PATCH 5/5] [infrastructure] Improve domain, Docker Compose, and license validation in self-hosted scripts (#7339) --- .../getting-started-enterprise.sh | 112 ++++++++++++++++- infrastructure_files/getting-started.sh | 79 +++++++++--- infrastructure_files/migrate-to-enterprise.sh | 113 +++++++++++++++++- 3 files changed, 279 insertions(+), 25 deletions(-) diff --git a/infrastructure_files/getting-started-enterprise.sh b/infrastructure_files/getting-started-enterprise.sh index 7418cb8e8..3f7cf6357 100755 --- a/infrastructure_files/getting-started-enterprise.sh +++ b/infrastructure_files/getting-started-enterprise.sh @@ -15,16 +15,25 @@ NETBIRD_EULA_URL="https://netbird.io/self-hosted-EULA" # server trusts X-Forwarded-* headers from this address only. TRAEFIK_IP="172.30.0.10" +LICENSE_VERDICT="unknown" +LICENSE_LOG_LINES="" + check_docker_compose() { - if command -v docker-compose &> /dev/null; then - echo "docker-compose" - return + if ! command -v docker &> /dev/null && ! command -v docker-compose &> /dev/null; then + echo "Docker is not installed or not in PATH. Please follow the steps from the official guide: https://docs.docker.com/engine/install/" > /dev/stderr + exit 1 fi - if docker compose --help &> /dev/null; then + + if docker compose version &> /dev/null; then echo "docker compose" return fi - echo "docker-compose is not installed or not in PATH. See https://docs.docker.com/engine/install/" > /dev/stderr + if command -v docker-compose &> /dev/null && docker-compose version &> /dev/null; then + echo "docker-compose" + return + fi + + echo "Docker Compose is not installed or not in PATH. Please follow the steps from the official guide: https://docs.docker.com/compose/install/" > /dev/stderr exit 1 } @@ -221,6 +230,90 @@ wait_postgres() { set -e } +wait_for_license_verdict() { + local counter=0 + local logs="" + + echo -n "Waiting for the server to validate the license" + while [[ $counter -lt 60 ]]; do + logs=$($DOCKER_COMPOSE_COMMAND logs --no-color --tail=all netbird-server 2>/dev/null || true) + + if grep -qi "license invalidated" <<< "$logs"; then + echo " rejected" + LICENSE_VERDICT="rejected" + LICENSE_LOG_LINES=$(grep -i "license" <<< "$logs" | tail -n 5 || true) + return 0 + fi + + if grep -qi "license validated" <<< "$logs"; then + echo " ok" + LICENSE_VERDICT="ok" + return 0 + fi + + echo -n " ." + sleep 2 + counter=$((counter + 1)) + done + + echo " no verdict in 120s" + LICENSE_VERDICT="unknown" + LICENSE_LOG_LINES=$(grep -iE "failed to validate license|error validating license" <<< "$logs" | tail -n 3 || true) + return 0 +} + +report_license_verdict() { + if [[ "$LICENSE_VERDICT" == "ok" ]]; then + return 0 + fi + + if [[ "$LICENSE_VERDICT" == "unknown" ]]; then + echo "" + echo " ⚠ The server logged no license verdict within 120s." + if [[ -n "$LICENSE_LOG_LINES" ]]; then + echo " It was still reporting validation errors:" + while IFS= read -r line; do + [[ -n "$line" ]] && echo " $line" + done <<< "$LICENSE_LOG_LINES" + fi + echo "" + echo " Check the verdict with:" + echo "" + echo " $DOCKER_COMPOSE_COMMAND logs netbird-server | grep -i license" + return 0 + fi + + local unreachable="false" + if grep -qi "couldn't be validated with the license server" <<< "$LICENSE_LOG_LINES"; then + unreachable="true" + fi + + echo "" + if [[ "$unreachable" == "true" ]]; then + echo " ⚠ The server could not validate the license:" + else + echo " ⚠ The server rejected the license key:" + fi + while IFS= read -r line; do + [[ -n "$line" ]] && echo " $line" + done <<< "$LICENSE_LOG_LINES" + echo "" + echo " The stack is up, and only the license check did not pass." + echo "" + if [[ "$unreachable" == "true" ]]; then + echo " The license server could not be reached, so the key itself was" + echo " never checked. Confirm this host has outbound access to the" + echo " license server, then restart:" + else + echo " Check the reason the server gave above, verify that" + echo " NETBIRD_LICENSE_KEY in .env matches the key you were issued," + echo " then restart:" + fi + echo "" + echo " $DOCKER_COMPOSE_COMMAND up -d" + return 0 +} + init_environment() { check_openssl DOCKER_COMPOSE_COMMAND=$(check_docker_compose) @@ -299,6 +392,9 @@ init_environment() { echo "Starting remaining services ..." $DOCKER_COMPOSE_COMMAND up -d + echo "" + wait_for_license_verdict + echo "" echo "Done." echo "" @@ -309,6 +405,12 @@ init_environment() { echo "" echo "Tail logs:" echo " cd $(pwd) && $DOCKER_COMPOSE_COMMAND logs -f netbird-server traefik" + + report_license_verdict + + if [[ "$LICENSE_VERDICT" == "rejected" ]]; then + exit 1 + fi } # ------------------------------------------------------------------ diff --git a/infrastructure_files/getting-started.sh b/infrastructure_files/getting-started.sh index 0fc5b23c5..5efc0181e 100755 --- a/infrastructure_files/getting-started.sh +++ b/infrastructure_files/getting-started.sh @@ -60,18 +60,21 @@ check_docker_sock_perms() { } check_docker_compose() { - if command -v docker-compose &> /dev/null - then - echo "docker-compose" - return - fi - if docker compose --help &> /dev/null - then - echo "docker compose" - return + if ! command -v docker &> /dev/null && ! command -v docker-compose &> /dev/null; then + echo "Docker is not installed or not in PATH. Please follow the steps from the official guide: https://docs.docker.com/engine/install/" > /dev/stderr + exit 1 fi - echo "docker-compose is not installed or not in PATH. Please follow the steps from the official guide: https://docs.docker.com/engine/install/" > /dev/stderr + if docker compose version &> /dev/null; then + echo "docker compose" + return + fi + if command -v docker-compose &> /dev/null && docker-compose version &> /dev/null; then + echo "docker-compose" + return + fi + + echo "Docker Compose is not installed or not in PATH. Please follow the steps from the official guide: https://docs.docker.com/compose/install/" > /dev/stderr exit 1 } @@ -98,19 +101,39 @@ get_main_ip_address() { } check_nb_domain() { - DOMAIN=$1 - if [[ "$DOMAIN-x" == "-x" ]]; then + local domain="$1" + + if [[ -z "$domain" ]]; then echo "The NETBIRD_DOMAIN variable cannot be empty." > /dev/stderr return 1 fi - - if [[ "$DOMAIN" == "netbird.example.com" ]]; then + if [[ "$domain" == "use-ip" ]]; then + return 0 + fi + if [[ "$domain" == "netbird.example.com" ]]; then echo "The NETBIRD_DOMAIN cannot be netbird.example.com" > /dev/stderr return 1 fi + if [[ "$domain" =~ ^[0-9.]+$ ]]; then + echo "'$domain' is an IP address. Use 'use-ip' to install on this host's IP over HTTP, or an FQDN to get a TLS certificate." > /dev/stderr + return 1 + fi + if [[ ! "$domain" =~ ^[A-Za-z0-9]([A-Za-z0-9-]*[A-Za-z0-9])?(\.[A-Za-z0-9]([A-Za-z0-9-]*[A-Za-z0-9])?)+$ ]]; then + echo "'$domain' is not a valid FQDN. It needs at least one dot (e.g. netbird.my-domain.com), with no scheme, port or trailing dot." > /dev/stderr + return 1 + fi return 0 } +check_domain_resolves() { + local domain="$1" + if command -v getent &> /dev/null && getent hosts "$domain" &> /dev/null; then return 0; fi + if command -v host &> /dev/null && host "$domain" &> /dev/null; then return 0; fi + if command -v dig &> /dev/null && [[ -n "$(dig +short "$domain" 2>/dev/null)" ]]; then return 0; fi + if command -v nslookup &> /dev/null && nslookup "$domain" &> /dev/null; then return 0; fi + return 1 +} + # Non-interactive configuration # ------------------------------ # Every prompt below can be pre-answered with an environment variable, so the @@ -170,7 +193,22 @@ read_nb_domain() { read -r READ_NETBIRD_DOMAIN < /dev/tty if ! check_nb_domain "$READ_NETBIRD_DOMAIN"; then read_nb_domain + return fi + + if [[ "$READ_NETBIRD_DOMAIN" != "use-ip" ]] && ! check_domain_resolves "$READ_NETBIRD_DOMAIN"; then + local confirm="" + echo "" > /dev/stderr + echo "Warning: '$READ_NETBIRD_DOMAIN' does not resolve via DNS from this host." > /dev/stderr + echo "TLS certificate issuance and client connections will fail until it does." > /dev/stderr + echo -n "Continue anyway? [y/N]: " > /dev/stderr + read -r confirm < /dev/tty + if [[ ! "$confirm" =~ ^[Yy]$ ]]; then + read_nb_domain + return + fi + fi + echo "$READ_NETBIRD_DOMAIN" return 0 } @@ -439,12 +477,23 @@ configure_domain() { # Domain is validated (not a free-form value), so it keeps its own guard # rather than going through resolve(): a valid NETBIRD_DOMAIN is used as-is, # otherwise we prompt, or abort when there is no terminal to prompt on. + local prompted="false" if ! check_nb_domain "$NETBIRD_DOMAIN"; then if ! tty_available; then - echo "NETBIRD_DOMAIN is required for a non-interactive install." > /dev/stderr + if [[ -n "$NETBIRD_DOMAIN" ]]; then + echo "NETBIRD_DOMAIN='$NETBIRD_DOMAIN' cannot be used for a non-interactive install." > /dev/stderr + else + echo "NETBIRD_DOMAIN is required for a non-interactive install." > /dev/stderr + fi exit 1 fi NETBIRD_DOMAIN=$(read_nb_domain) + prompted="true" + fi + + if [[ "$prompted" == "false" && "$NETBIRD_DOMAIN" != "use-ip" ]] && ! check_domain_resolves "$NETBIRD_DOMAIN"; then + echo "Warning: '$NETBIRD_DOMAIN' does not resolve via DNS from this host." > /dev/stderr + echo "TLS certificate issuance and client connections will fail until it does." > /dev/stderr fi if [[ "$NETBIRD_DOMAIN" == "use-ip" ]]; then diff --git a/infrastructure_files/migrate-to-enterprise.sh b/infrastructure_files/migrate-to-enterprise.sh index 744ba5375..2b10250c9 100755 --- a/infrastructure_files/migrate-to-enterprise.sh +++ b/infrastructure_files/migrate-to-enterprise.sh @@ -40,6 +40,10 @@ ENTERPRISE_CONFIG_FILE="config.yaml.enterprise" # completed successfully. ROLLBACK_STATE="disarmed" ENV_EXISTED="unknown" +# Verdict the server logs about the license key on startup: ok, rejected, or +# unknown when neither line appeared before the timeout. +LICENSE_VERDICT="unknown" +LICENSE_LOG_LINES="" ENV_BACKUP="" PG_VOLUME_NAME="" BACKUP_DIR="" @@ -59,15 +63,21 @@ ENTERPRISE_CONFIG="no" NETBIRD_EULA_URL="https://netbird.io/self-hosted-EULA" check_docker_compose() { - if command -v docker-compose &> /dev/null; then - echo "docker-compose" - return + if ! command -v docker &> /dev/null && ! command -v docker-compose &> /dev/null; then + echo "Docker is not installed or not in PATH. Please follow the steps from the official guide: https://docs.docker.com/engine/install/" > /dev/stderr + exit 1 fi - if docker compose --help &> /dev/null; then + + if docker compose version &> /dev/null; then echo "docker compose" return fi - echo "docker-compose is not installed or not in PATH." > /dev/stderr + if command -v docker-compose &> /dev/null && docker-compose version &> /dev/null; then + echo "docker-compose" + return + fi + + echo "Docker Compose is not installed or not in PATH. Please follow the steps from the official guide: https://docs.docker.com/compose/install/" > /dev/stderr exit 1 } @@ -1000,6 +1010,39 @@ init_migration() { check_stale_postgres_volume } +wait_for_license_verdict() { + local counter=0 + local logs="" + + echo -n "Waiting for the server to validate the license" + while [[ $counter -lt 60 ]]; do + + logs=$($DOCKER_COMPOSE_COMMAND logs --no-color --tail=all "$COMBINED_SERVICE" 2>/dev/null || true) + + if grep -qi "license invalidated" <<< "$logs"; then + echo " rejected" + LICENSE_VERDICT="rejected" + LICENSE_LOG_LINES=$(grep -i "license" <<< "$logs" | tail -n 5 || true) + return 0 + fi + + if grep -qi "license validated" <<< "$logs"; then + echo " ok" + LICENSE_VERDICT="ok" + return 0 + fi + + echo -n " ." + sleep 2 + counter=$((counter + 1)) + done + + echo " no verdict in 120s" + LICENSE_VERDICT="unknown" + LICENSE_LOG_LINES=$(grep -iE "failed to validate license|error validating license" <<< "$logs" | tail -n 3 || true) + return 0 +} + apply_changes() { # From here on a failure must roll the deployment back. ROLLBACK_STATE="armed" @@ -1100,9 +1143,57 @@ apply_changes() { echo "Bringing up all services ..." $DOCKER_COMPOSE_COMMAND up -d + echo "" + wait_for_license_verdict + echo "" echo "Migration complete." + if [[ "$LICENSE_VERDICT" == "rejected" ]]; then + local unreachable="false" + if grep -qi "couldn't be validated with the license server" <<< "$LICENSE_LOG_LINES"; then + unreachable="true" + fi + + echo "" + if [[ "$unreachable" == "true" ]]; then + echo " ⚠ The server could not validate the license:" + else + echo " ⚠ The server rejected the license key:" + fi + while IFS= read -r line; do + [[ -n "$line" ]] && echo " $line" + done <<< "$LICENSE_LOG_LINES" + echo "" + echo " The migration itself completed: the images and any migrated data" + echo " are in place, and only the license check did not pass." + echo "" + if [[ "$unreachable" == "true" ]]; then + echo " The license server could not be reached, so the key itself was" + echo " never checked. Confirm this host has outbound access to the" + echo " license server, then restart:" + else + echo " Check the reason the server gave above, verify that" + echo " NB_LICENSE_KEY in .env matches the key you were issued, then" + echo " restart:" + fi + echo "" + echo " $DOCKER_COMPOSE_COMMAND up -d" + elif [[ "$LICENSE_VERDICT" == "unknown" ]]; then + echo "" + echo " ⚠ The server logged no license verdict within 120s." + if [[ -n "$LICENSE_LOG_LINES" ]]; then + echo " It was still reporting validation errors:" + while IFS= read -r line; do + [[ -n "$line" ]] && echo " $line" + done <<< "$LICENSE_LOG_LINES" + fi + echo "" + echo " Check the verdict with:" + echo "" + echo " $DOCKER_COMPOSE_COMMAND logs $COMBINED_SERVICE | grep -i license" + fi + # Nothing left to undo. ROLLBACK_STATE="disarmed" } @@ -1122,6 +1213,11 @@ print_summary() { fi [[ "$ENABLE_FLOW" == "yes" ]] && echo " Traffic flow: enabled" [[ "$ENABLE_FLOW" != "yes" ]] && echo " Traffic flow: disabled" + case "$LICENSE_VERDICT" in + ok) echo " License: validated by the server" ;; + rejected) echo " License: REJECTED - see above, the install is not usable yet" ;; + *) echo " License: not confirmed (no verdict in the logs yet)" ;; + esac echo "" echo " Generated files (next to your docker-compose.yml):" echo " $OVERRIDE_FILE" @@ -1176,3 +1272,10 @@ trap 'exit 130' INT TERM init_migration apply_changes print_summary + +# A rejected license leaves a migrated but unusable install. Say so in the exit +# code too, or a wrapper script reads this run as a clean success. +if [[ "$LICENSE_VERDICT" == "rejected" ]]; then + exit 1 +fi +exit 0