mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-12 17:59:06 +02:00
fix_update_settings_value_aware
955
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 riccardom
|
36bbef21cd |
[client] Fold the scheme case here too, like util does (review item 6)
profilemanager.SameServiceURL compared the scheme with ==, util.SameServiceURL with EqualFold. No observable difference — net/url lowercases the scheme when it parses, and both functions take parsed URLs — but two functions of the same name with two different rules is a trap for whoever reads one and assumes the other. |
||
|
riccardom
|
07dc9cb1c8 |
[client] Let a logged-out profile deserialize again (review item 2)
ConfigFromJSON refused a document with no WireGuard or SSH key. A config legitimately has none between a logout and the next login: mobile LogoutProfile clears both in place and writes the profile back, so the peer re-registers on the next login instead of returning as itself. So the refusal broke the mobile flows it was meant to protect. On iOS and tvOS the stored JSON of a logged-out profile stopped loading through Client.SetConfigFromJSON and Auth.SetConfigFromJSON, and copyConfig — which round-trips a Config through JSON to take an in-memory copy before applying the MDM overlay — failed on the same document. Where the old code silently minted a key, this returned an error, which is worse for logout and profile switching alike: neither is asking to connect. The deserializer now stays out of the identity question in both directions: it does not generate one (a read cannot hand back keys nothing will write down) and does not refuse one that is absent. Whoever goes on to connect is where an absent identity has to be answered — and it already is, by the login path that provisions and persists. ErrConfigWithoutIdentity goes with it; nothing else used it. |
||
|
riccardom
|
222ad91c4d |
[client] Give a newly added profile its identity (review item 1)
AddProfile writes the config it builds straight to disk, but built it with createNewConfig, which stopped generating the peer's keys when identity generation moved out of apply() into EnsureIdentity. The profile file landed with an empty PrivateKey and SSHKey. Nothing lost the keys permanently — the daemon's own getConfig provisions and persists them on first use — but every reader that does not write got a config that cannot connect in the meantime, which is exactly the set this branch grew: the update-settings gate deciding whether to refuse a request, and the mobile SDKs loading a stored profile. createProvisionedConfig exists for callers that persist or connect, and this is one; before the split, createNewConfig produced the keys here too. |
||
|
riccardom
|
050c2ba7d4 |
[client] Reuse util's service-URL comparison instead of a second copy
The endpoint-comparison rules this branch introduced now live in util (PR #7472 moved them there so the MDM conflict check could stop comparing URLs as strings). Keeping a copy here is what produced that bug in the first place: two implementations of "is this the same endpoint?" drift, and the one that drifts starts refusing a URL that addresses the very server it already points at. So SameServiceURL delegates the port normalization to util.ServiceURLPort and drops the local one, and SameServiceURLIncludingPath — endpoint plus path, for the admin panel URL, which is opened rather than dialed — is util.SameServiceURL plus the query, fragment and userinfo it adds on top, so the local path normalization goes too. What stays here is the distinction util does not make: SameServiceURL is endpoint-only, because a management URL is dialed and only its host and port are, while util.SameServiceURL includes the path. Pure refactor. Verified as one: all 198 pairs of a 14-spelling matrix (default and zero-padded ports, host case, trailing slash, path, query, fragment, userinfo, both schemes, nil operands) answer identically for both functions before and after. |
||
|
riccardom
|
262ce8c3b2 |
Merge remote-tracking branch 'origin/main' into fix_update_settings_value_aware
# Conflicts: # client/ios/NetBirdSDK/client.go # client/server/mdm.go # client/server/server.go |
||
|
Riccardo ManfrinandZoltan Papp
|
e14006ddc1 |
[client] mobile MDM bridge — iOS + Android setMDMPolicyFetcher entrypoint (#6435)
* MDM Android mobile wiring * Removes dead code * Removes static vars * Now we need to apply MDM in the GetConfig * You now need to explicitly call these around * Adds iOS wiring * Resolve merge conflicts from main - login.go: keep both new imports (mdm + nbnet + server) - ios/NetBirdSDK/client.go: additive struct-field merge (mdmLoader + stateMu/connectClient/config) - setconfig_mdm_test.go: adopt new withMDMPolicy(t, s, policy) signature; fix stray old-signature call in TestSetConfig_MDMAllow_ManagementURLPortNormalized * Convey MDM overlay config to Debug Bundle output Aligns to other clients OSes behavior * Solved conflict in client.go * Fixup helper withMDMPolicy -> configWithMDM * Fixup after merge * Resolve merge conflicts * [client] Move MDM enforcement logic into a shared Go layer (#7319) The mobile bridges only carried the policy fetcher, leaving every enforcement decision to the native apps: the desktop derived its UI restrictions in the Wails service layer, the daemon kept the conflict machinery in the server package, and both mobile bridges duplicated the JSON fetch adapter. Anything the native side had to reimplement was a place for iOS and Android to drift apart. Enforcement now lives in client/mdm and is consumed identically by all three platforms: - conflicts.go holds the value-aware conflict checks lifted out of the daemon, so the same normalization (canonical URLs, PSK sentinel echo) applies wherever a config change is validated. - restrictions.go derives the UI enforcement snapshot from a policy and renders it in the JSON shape the desktop frontend already consumes. The service-layer types become aliases, keeping one source of truth. - jsonloader.go replaces the adapter that was copy-pasted into both bridges. - changedetector.go moves change detection off the native side: the caller forwards the OS notification and asks whether the managed configuration actually changed, instead of diffing dictionaries itself. The mobile bridges gain the enforcement the daemon already had. The Preferences getters resolve managed keys from the policy, so a naive UI shows the enforced value; Commit rejects a staged change that diverges from a managed key; NewAuth resolves the managed management URL before persisting the config and overlays the policy on it, so a login can no longer run against a URL the policy forbids. Android's profile mutations fail closed when disableProfiles is set. NewAuth takes the fetcher as a required argument rather than keeping a policy-blind overload: the apps consume this code as a submodule, so a compile error at the bump is the point. The mobile PSK getter is replaced by a presence check — the key has no reason to cross the bridge, and not returning it means the native side needs no redaction sentinel of its own. * [client] Resolve the main merge conflicts in the MDM integration The merge commit was recorded with the conflict markers still in the tree. Resolve them so the branch builds again: - client/ios/NetBirdSDK: keep both the mdm and mobile imports, and keep the mdmLoader/mdmDetector fields next to main's stateMu documentation. - client/server/mdm.go: drop the conflict helpers main added locally, they already live in the client/mdm package on this branch, and keep the new checks main introduced (allowRemoteJobs, enableLocalMetrics, localMetricsAddress) as calls into the package-level helpers. - client/mdm/conflicts.go: add ConflictStringPtr, the presence-aware string check main needs for the optional localMetricsAddress field. - Port the two tests main added over the per-Server loader helper and the configWithMDM helper, both of which replaced the package-level policy injection this branch removed. * [client] Reject explicit empty PSK when MDM enforces a pre-shared key The SetConfig, Login and mobile Commit conflict checks collapsed the PSK to a plain string, so an explicit empty value was indistinguishable from an unset field and slipped past the MDM gate, clearing the persisted key. Carry the optional field as a pointer through ConflictStringPtr, treating only the redaction sentinel as a no-op echo. ConflictString had no other callers and is removed. * [client] Apply MDM overlay on the preloaded iOS config in Run Run only overlaid the MDM policy when the config was loaded from file, so the tvOS path fed by SetConfigFromJSON started with unmanaged settings. Apply the overlay after the config source is selected, as the other resolution sites already do. * [client] Gate non-active profile logout behind the MDM profiles switch The mobile ProfileManager let LogoutProfile clear credentials of any profile even when disableProfiles was enforced. Follow the daemon's validateProfileLogout semantics: logging out of the active profile is a plain logout and stays allowed, logging out of any other profile is profile management and is rejected under the policy. * [client] Resolve the managed management URL through the MDM overlay on mobile NewAuth on Android and iOS replaced the caller URL with the raw policy value before persisting, so a malformed managed URL failed config validation and blocked the login instead of being skipped with a warning like the overlay does. Preferences.GetManagementURL likewise echoed the raw policy string to the native UI even when the overlay had rejected it. Follow the daemon: persist the caller URL, overlay the policy on the resolved config, and report the overlaid ManagementURL as the effective value. * [client] Clean up MDM review leftovers Drop the unused ChangeDetector.Current, point the stale LoadPolicy comment references at Loader.Load, and move the profileEmail godoc back above its function. * [client] Check remote jobs and local metrics keys in the mobile MDM conflict gate MDMConflicts skipped allowRemoteJobs, enableLocalMetrics and localMetricsAddress even though the overlay applies all three and the daemon gate already checks them, so a mobile Commit could persist values diverging from the enforced policy. Align the list with the daemon. * [client] Silence the deprecated PreSharedKey lint in the login conflict test The legacy LoginRequest.PreSharedKey field is deliberately exercised by the test, matching the nolint already carried by the production path. * [client] Publish the mobile MDM loader and detector atomically SetMDMPolicyFetcher wrote the loader and change detector as two plain fields that Run, the OS-change callback and the restrictions getter read from other threads without synchronization. Hold both behind a single atomic pointer so a registration is published as one unit and readers always observe a matching loader and detector pair; Preferences gets the same treatment for its loader. Exported signatures are unchanged. * [client] Report the MDM-overlaid remote jobs value from mobile Preferences GetRemoteJobsAllowed returned the staged or persisted value even when the policy manages allowRemoteJobs, so the native settings UI could show a value the Commit gate would reject. Resolve it through the overlay like GetManagementURL does. * [client] Stop persisting the MDM-overlaid config after mobile logins NewAuth already writes the config through UpdateOrCreateConfig before the MDM policy is overlaid, and the login itself never mutates the Config. The post-login WriteOutConfig calls therefore only rewrote the same file with the enforced ManagementURL and PreSharedKey in it, so a removed or changed policy kept acting through the persisted values. * [client] Document that the MDM overlay on Config is not reversible ApplyMDMPolicy promised that an empty Policy clears a prior overlay, but applyMDMPolicy only resets the enforcement metadata and the runtime-only upload URL; the enforced ManagementURL, PreSharedKey and flags stay. Every lifecycle owner resolves the base Config again before applying, so state that contract instead of the reversibility that was never implemented. * [client] Re-resolve the tvOS preloaded config before every MDM overlay The iOS Client kept the config parsed from SetConfigFromJSON and applied the MDM overlay onto that same instance on every Run, IsLoginRequired and DebugBundle, so a key removed from the policy stayed enforced. Store the JSON instead and parse it per load through one loadConfig path. Auth serialized the overlaid config from GetConfigJSON, which tvOS then persisted to UserDefaults and fed back as the preload. Keep the resolved config as the base, run the login on a JSON round-trip copy with the overlay, and return the base from GetConfigJSON. * [client] Serve the MDM-managed management URL without touching the config file on mobile Preferences.GetManagementURL resolved a managed URL by reading and overlaying the persisted config, so a corrupt file or the tvOS sandbox turned an enforced URL into a read error. Return the canonical managed value directly, the same string BuildRestrictions already hands to the UI, and only fall back to the staged or persisted value when MDM does not manage the key. NewAuth validated the caller-supplied management URL before the overlay ran, so a malformed or echoed value blocked or persisted under an MDM policy that already dictates the URL. Ignore the caller value while the key is managed; the login runs against the overlay either way. * [client] Align the MDM loader docs with the fetcher precedence and make disableAdvancedView a tristate NewLoader, PolicyFetcher and the darwin/windows loadPlatform docs claimed the fetcher is unused on desktop, while every loader returns its values when one is injected. That precedence is the seam the server tests rely on across platforms, so the docs now describe it; production desktop callers still pass nil and keep the registry / plist authoritative. Fields.DisableAdvancedView collapsed "managed and false" into the same JSON as "not managed", unlike AllowServerSSH and the daemon's optional proto field. Carry it as a *bool so the UIs can tell the two apart; the desktop reflect loop skips pointer fields already, and the mobile decoders treat null as not managed. * [client] Clean up MDM review nits - ResolveConflicts treats a managed key whose ConflictCheck has no Check as a conflict instead of dereferencing nil. - Ticker.Run and ChangeDetector.Changed share policyChanged so the diff semantics and the log line cannot drift apart. - TestLoader_NilFetcherReturnsEmpty skips on windows/darwin, where a nil fetcher reads the real registry / plist. - The profilemanager test loader checks GetInt before GetBool so integer keys survive the round trip, and the PSK tests use the exported redaction sentinel. * [client] Fix int policy values coercing to bool in the MDM test helper withMDMPolicy rebuilt the policy map by trying GetString, then GetBool, then GetInt. Policy.GetBool accepts native ints (non-zero means true), so an int-valued key such as wireguardPort round-tripped through the helper as the bool true and GetInt was never reached. Try GetInt before GetBool, as the profilemanager helper already does; GetInt does not coerce bools, so booleans still fall through to GetBool. No test sets an int key today, so this was latent: the first test to exercise the wireguardPort conflict gate would have seen ConflictInt64 report a conflict for every value, including a matching one. --------- Co-authored-by: Zoltan Papp <zoltan.pmail@gmail.com> |
||
|
riccardom
|
3fd28885e1 |
Merge branch 'main' into fix_update_settings_value_aware
Resolves a semantic conflict the merge introduces without a textual one: main added Preferences.GetRemoteJobsAllowed to the Android and iOS SDKs, reading through profilemanager.ReadConfig, while this branch renamed that function to ReadOrGenerateConfig. Neither side is broken alone, so only the merge CI builds for a pull request caught it: client/android/preferences.go:334:29: undefined: profilemanager.ReadConfig Both new call sites now use ReadOrGenerateConfig, which is what the getters around them already do. |
||
|
riccardom
|
ea8e64e6ef |
[client] Gather the optional-field defaults into one function
Resolving an unset optional field was spread over five places: the two values newConfigSkeleton pre-sets, the block this branch added for the SSH toggles, the network monitor's own if, the `else if` tails of ServerSSHAllowed and RemoteJobsAllowed, and a trailing if for DisableNotifications several hundred lines further down. Reading apply() left no single answer to "what does this field default to, and who decides". They now live in Config.resolveUnsetDefaults, which apply() calls before it compares anything — the ordering being the point, since it is what lets every comparison below diff values instead of presence. The comparisons for ServerSSHAllowed, RemoteJobsAllowed and DisableNotifications lose their `config.X == nil ||` clauses accordingly, as the other six already had. newConfigSkeleton keeps its two, and that is the one asymmetry worth naming: ServerSSHAllowed defaults to false for a new profile and to true for a legacy one, and it only works because the skeleton runs first. The doc comment says so, where before it was implied by the order of two distant blocks. Pure refactor. Verified as one: for the four fields whose branches moved, plus two that did not and the JWT TTL, all 63 combinations of stored value (nil/false/true) against input value (absent/false/true) produce byte- identical resolved values and `updated` verdicts before and after. |
||
|
riccardom
|
9ff4e6ce70 |
[client] Say that the null-on-disk fixture is synthesized, not written
The test comment described the null state in the present tense — "the config a plain login writes leaves every one of them unset" — which was true before this branch and is not any more: apply() now resolves those fields, so a login writes them set. unsetOnDisk puts the null state back deliberately, to stand in for a profile an older client wrote. Comments only. |
||
|
riccardom
|
155ced4ab9 |
[client] Refuse a serialized config that carries no peer identity
ConfigFromJSON still promised a "fully initialized" config after this PR moved key generation out of apply() into EnsureIdentity, but identity stopped being one of the defaults it applies. Its two callers both connect with what they get back: the iOS SDK's Client.SetConfigFromJSON keeps it as the preloaded config Run() uses on tvOS, and Auth.SetConfigFromJSON as the config it authenticates with. No caller feeds it a document without keys today — every stored document comes from Auth.GetConfigJSON, whose config is provisioned by DirectUpdateOrCreateConfig or CreateInMemoryConfig, and the tvOS app only ever edits fields of a document it already has. This is a safety net for the next caller, not a live bug. Provisioning the identity here would be the wrong net. Neither caller can hand a generated key back to the store the document came from — Client exports no config at all — so the peer would connect under an identity nothing persists and register anew on every launch, which is the failure the EnsureIdentity split exists to prevent. A document with no identity means nobody has logged in yet, and saying so is the only useful answer. Both keys are required because both are dead ends when missing: an empty WireGuard key fails the management login on its size, and an empty SSH key fails ssh.GeneratePublicKey in ConnectClient before the engine starts. |
||
|
riccardom
|
70167821bb |
[client] Stop the last config write that skipped normalization
Every path that creates or updates a profile config goes through apply(), which resolves an optional field to its default — except RenameProfile, which read the file with a bare json.Unmarshal, set the name, and wrote it straight back. That copied whatever the file held, so a config written by a client that stored these fields as null kept them null. It could not introduce a null, only carry one forward, but renaming a profile is a poor place to leave a half-resolved config behind. It now reads through GetExistingConfig, which normalizes what it hands out. The tests state the invariant the fix completes, over the *bool fields of Config listed by reflection so a field added later is covered without touching them: none may come out of apply() unset, and no write may store one as null. An optional bool that can be nil, true or false forces every reader to invent the meaning of nil, and makes a diff of the config compare presence rather than value — which is exactly what refused `netbird up` for a client restating its own defaults. SyncMessageVersion stays a genuine three-state field and is not covered: it is an *int whose absence means the client pins no version, and it travels to management that way. |
||
|
riccardom
|
e998887977 |
[client] Normalize the config before diffing it in WouldChange
apply() reports two different things through one bool: an input that changed a value, and a field it had to fill in because the config carried none. The update-settings gate reads that bool as "the caller asked for a change", so any config still missing a default answered a request that asks for nothing with a refusal. Readers already hand out normalized configs — readConfig applies an empty input for exactly this reason — which is why the gate got away with it. But a handler that refuses a request must not depend on where its caller obtained the config, and it must not start reading "this profile predates a field" as "the caller asked for a change" the day someone adds one with a default. WouldChange now runs the filling-in as a pass of its own and discards its verdict, so the pass that answers the caller measures only what the input did. |
||
|
riccardom
|
aac936a810 |
[client] Treat an unset optional field as its default when diffing a config
Seven Config fields mean "the effective default" when they hold no value: the five SSH toggles, the SSH JWT cache TTL, and the network monitor. Every consumer already reads a nil as that default, but apply() diffed them by presence — `config.X == nil || *input.X != *config.X` — so an input restating the default counted as a change. That made the update-settings gate refuse `netbird up` outright. The CLI sends every flag whose value came from an environment variable (SetFlagsFromEnvVars goes through pflag's FlagSet.Set, which marks the flag Changed), and the config a plain login writes leaves all seven unset, so a container configured with, say, NB_ENABLE_SSH_ROOT=false restated a default the file held as null on every start and was answered with FailedPrecondition. apply() now resolves the seven up front, the way it already did for ServerSSHAllowed and RemoteJobsAllowed, which also repairs such a profile on its next write. With the values named, the comparisons below diff values instead of presence, so their nil branches are gone. The network monitor keeps its platform default — on for windows and darwin — and naming it as false elsewhere is what createEngineConfig already read a nil to be. getJWTCacheTTL reaches the same 0 through its own default, and Android's GetEnableSSH* getters already answered nil with false. |
||
|
Zoltan Papp
|
5cb6b0d33b |
[client] Assign the Android TUN address as a host prefix (#7414)
Android 16+ local network protection derives the blocked prefixes from the interface address prefix. A /16 address turns the whole overlay into a local network, so apps without ACCESS_LOCAL_NETWORK cannot reach any peer. Pass the address as /32 and /128 and add the overlay networks to the route list that the Android side turns into VPN routes, on both the initial create and the renew path. |
||
|
Zoltan Papp
|
825389818c |
[client] Gather fresh system info on every management sync stream connect (#7409)
* Gather fresh system info on every management sync stream connect The engine collected the peer meta once at start and reused the same Info for every Sync stream reconnect, so a mobile network switch that redials management kept reporting the old local network addresses. The peer network range posture check was then evaluated against stale data until the client restarted. Sync now takes a gatherer that runs at each stream connect. The gatherer is cheap: GetInfo plus the cached posture check file results, kept in the new system.InfoSource, which the engine refreshes whenever the checks list changes. No process enumeration runs on the reconnect path. Also fix the management mock server calling itself instead of SyncFunc. * Evaluate the login response posture checks before the first sync connect The engine starts with the checks the login response carried, and the first sync stream request used to send their evaluated file results. After moving the gather into InfoSource, the stream opened with an empty cache and the first sync response did not refill it, because its checks equal the ones the engine already holds. Desktop peers therefore never reported process or file posture results. Seed the cache once before the first connect, where the old gather ran, so a timed out evaluation still falls through to the address-only info. * Harden the sync info source against nil callbacks and shared slices A nil getInfo opens the stream without metadata, as a nil sysInfo did before. The cached posture results are a copy, so the Info returned by Refresh cannot alias the snapshot later Current calls report. The exclusion test asserts the remaining address count so it cannot pass vacuously on a single-address host. * Retry a posture check refresh that timed out or failed to sync The checks list was recorded before the gather ran, so once the gather timed out or SyncMeta failed, the next sync response carrying the same list matched the recorded one and nothing retried. The peer kept reporting the previous posture results until the list changed again. Record the checks only after the meta reached management, so a failed cycle is repeated on the next sync response. * Log the skipped posture refresh, let the mock Sync return errors and deflake the reconnect test * Drop the nil guard around the sync info callback * Send the refreshed info on the first sync connect instead of gathering it twice |
||
|
Viktor Liu
|
7c1253004b | [client] Renew the Android TUN only when the routes it carries change (#7396) | ||
|
Viktor Liu
|
bb233c72b6 | [client] Rebuild the overlay listeners when the TUN is renewed (#7397) | ||
|
Maxim Egorov
|
b0e03038ed | [client] Pick the probe port from the system in Test_freePort (#7404) | ||
|
Viktor Liu
|
778b3b3264 |
[client] Unify peer and route ACL filtering with multi-source rules (#6322)
* Unify peer and route ACL filtering with multi-source peer rules * Remove partial userspace firewall mode and open foreign chains via a table-less allower * Snapshot iptables rule maps before persisting state * Scope userspace firewall wildcard source rules per address family * Install nftables peer filter and mangle rules in a single transaction * Share the iptables jump rule spec between install and cleanup * Fix legacy ACL source wildcard and keep rollback tracking on delete failure * Fix CI: recognize multi-value port set lookups in tests and correct PeerIP lint suppression * Fall back to per-prefix filter rules when ipset is unavailable * Annotate legacy PeerIP usages in ACL tests and fix import formatting * Keep firewall rule bookkeeping in step with the kernel on replace and teardown * Release the routing reference when the route manager shuts down * Keep set references and rule tracking consistent when a routing rule fails |
||
|
riccardom
|
29d03356fc |
[client] Keep the admin panel path part of its identity
The endpoint comparison introduced for the management URL was applied to the admin URL too, and that one is opened in a browser rather than dialed over gRPC: a panel served under /netbird is not the panel served at the root. So a config whose admin URL differed only by path reported no change, and the new path was never persisted — a custom panel URL could not be updated at all. SameServiceURLIncludingPath adds what a URL carries past its endpoint (path, query, fragment, userinfo) while still treating equivalent spellings as equal: a missing path and "/" are the same root, and so is a trailing slash. The management URL keeps the endpoint-only comparison, since only the endpoint is ever dialed. Ports are also normalized numerically now, so ":0443" and ":443" are one port. Reported by cubic-dev-ai on PR #7398 (two findings). |
||
|
riccardom
|
e027ba11f9 |
[client] Keep the peer identity out of a read that finds no file
ReadOrGenerateConfig resolves a default config when the profile has no file yet, and createNewConfig was minting the WireGuard and SSH keys while doing so. That defeated the provisioning pair it was meant to serve: the CLI's foreground login calls EnsureIdentity to find out whether it has to persist the keys, got generated == false because the read had already generated them, and so never wrote them out. The login then dialed management with an identity that only existed in memory, and the next login registered a second peer. createNewConfig no longer provisions. createProvisionedConfig is the variant that does, and the callers whose contract is "usable as it comes back" use it: CreateInMemoryConfig, whose callers connect with the result, and the two create-and-write branches. A read gets a config with no identity, so the caller's own EnsureIdentity reports the work and triggers the write. Reported by CodeRabbit and cubic-dev-ai on PR #7398, both on the same defect. |
||
|
riccardom
|
c660fcaaac |
[client] Compare the client certificate paths before reporting a change
apply() assigned the incoming mTLS certificate and key paths and set updated unconditionally, without comparing them to what the config already held. It is the same presence-instead-of-value mistake this branch set out to fix, one layer down: a caller restating its own certificate paths was reported as changing them, which trips the value-aware update-settings gate. Reported by cubic-dev-ai on PR #7398. |
||
|
riccardom
|
911705e1c6 |
[client] Do not panic on a config with no sync message version
apply() wrote the incoming sync message version through the stored pointer, without checking it was there: a config that carries no version yet made it dereference nil. Reachable from the update-settings dry run, which runs inside a request handler — where failing closed is the worst acceptable outcome, and a panic is not one. The field is now reassigned like every other optional one, which also means apply() no longer mutates anything the caller still holds through a pointer, so the dry run's copy has one less field to detach. Reported by cubic-dev-ai on PR #7398. |
||
|
riccardom
|
844bf24a6f |
[client] Name the two config readers for what they do
ReadConfig and GetConfig differed in one thing — what happens when the file is absent — and neither name said which was which: - ReadConfig -> ReadOrGenerateConfig (reads it, or generates one in memory) - GetConfig -> GetExistingConfig (reads it, or fails) Three comments went with them: - GetConfig's said "return with Config and if it was created. Errors out if it does not exist", which described a bool it does not return and a creation it never performs. - ReadConfig's explained that it does not write, which is what a reader is supposed to do anyway. - Server.getConfig's said it "errors out if it does not exist", which it does not — it resolves a default config, and now provisions the identity too. |
||
|
riccardom
|
7a1f095eb5 |
[client] Make config reads pure and provision the identity explicitly
Reading a config wrote it back. profilemanager.readConfig persisted whatever apply() had filled in, and ReadConfig created and wrote the file outright when it was absent, so every reader was quietly a writer: a gate deciding whether to refuse a request, a UI listing profiles, a mobile getter reading one preference. The previous commit worked around that with a PeekConfig variant, which left two read functions with opposite side effects and the antipattern still there for everyone else. Only one thing in a read genuinely had to be persisted: apply() generated the WireGuard and SSH keys when it found them empty, and a generated key cannot be recomputed — losing it means the peer comes back with a different identity and registers again. Everything else apply() fills in is a deterministic default that the next read recomputes anyway. So identity provisioning is now its own step, Config.EnsureIdentity, and the callers that provision write the result out themselves, in the open: - Server.getConfig, the daemon's provisioning point; - the CLI's foreground login, which is about to dial management; - update() / directUpdate(), the config write paths — a stored profile can legitimately carry no identity, since a mobile logout clears the keys in place, and the next write is what has to mint a new one. ReadConfig and GetConfig no longer write anything, PeekConfig is gone, and the dry-run baseline no longer needs placeholder keys to keep apply() from minting real ones. One deliberate leftover: readConfig still calls util.EnforcePermission, which chmods a config file whose permissions are too broad. It changes no content and is idempotent, and dropping it would leave a legacy file world-readable until its first write. |
||
|
riccardom
|
7dbd5f8f56 |
[client] Drop an unreachable guard and fix two stale comments
- loginOverridesInput's nil-message guard cannot be reached: Login dereferences the message well before it, in storedLoginConfig. - The docstring above afterLoginPreCheck described persistLoginOverrides, which lives further down the file and now carries its own. - UpdateConfig's comment named DirectUpdateConfig; the function is DirectUpdateOrCreateConfig. |
||
|
riccardom
|
ea972f7847 |
[client] Stop the config dry run from generating throwaway keys
The dry run's baseline for a profile with no config file yet went through createNewConfig, and apply() generates a WireGuard and an SSH key whenever it finds those fields empty. The baseline is compared against and discarded, so every evaluation minted a keypair it threw away — and logged "generated new Wireguard key". The CLI retries Login in a backoff loop, so a first `netbird up` on a fresh profile filled the daemon log with what reads like peer-key rotation. The baseline now starts from the shared skeleton with placeholder keys, so apply() has nothing to generate. No ConfigInput field maps to either key, so the comparison is unaffected. |
||
|
riccardom
|
2a17bf0d55 |
[client] Compare service URLs as endpoints, not as strings
Three places in one request path each had their own notion of "same management URL": the config layer compared the parsed URLs as strings, the privileged-change gate compared scheme + host + effective port, and the MDM conflict check compared strings after filling in the default port. Only the middle one was right. A string comparison answers the wrong question. "https://api.netbird.io", "https://api.netbird.io/" and "https://API.netbird.io:443" are one endpoint written three ways, so a client restating its own management URL with a trailing slash — a normal way to write it — was still read as a client asking to be repointed, and the update-settings gate refused it. The MDM check had the same flaw against the enforced value. profilemanager.SameServiceURL is now the single comparison: same scheme, same host case-insensitively as DNS names are, same effective port. The config layer, the privileged-change gate and the MDM conflict check all defer to it, so there is one answer to "did this URL change?" instead of three. |
||
|
riccardom
|
0b969e2124 |
[client] Do not write the profile config while only reading it to decide
The update-settings gate needs the stored config to decide whether a request changes anything, so the previous commit moved that read ahead of the refusal. The read is not side-effect free: profilemanager.GetConfig writes the config back whenever apply() has to fill in a default the file was missing. A request that the gate then refuses had therefore already rewritten the profile file. PeekConfig is GetConfig without that write-back. The returned config is still normalized in memory, which is what the decision needs; the file is left exactly as it was found. Every caller of storedConfigAtPath feeds a gate that can refuse, so they all peek. Note for reviewers: the daemon still normalizes the file on startup and on every real update, so nothing depends on a read performing that write. |
||
|
Riccardo Manfrin
|
7b22d55bf6 |
[client] Bind the cached SSH JWT to the local caller that obtained it (#7378)
* [client] Bind the cached SSH JWT to the local caller that obtained it Record the identity that obtained the token and return it only to that same identity, comparing the account alone: the group set and the elevation flag describe what a token may do rather than who it belongs to, and the same user may call once elevated and once not. A control channel that carries no caller identity gets a miss on read and stores nothing on write, matching how the other ipcauth consumers fail closed. Clear the entry when the session it speaks for ends: logout, down and profile switch. * [client] Cover the profile-switch path of the SSH JWT cache The cache being correct buys nothing if a handler around it forgets to clear it, and SwitchProfile had no test at all. Point the profile globals at a temp dir holding a single default profile, which is the one ActiveProfileState.FilePath resolves without consulting the current OS user, and call SwitchProfile with no request so neither the switch itself nor the profile-list event is involved. * [client] Report the SSH JWT cache in the no-identity startup warning daemonServerOptions already warns once, at startup, about what a control channel with no caller identity gives up. Name the SSH JWT cache there too, on both the TCP and the no-peer-identity-primitive paths. The per-request logs in cachedJWT and WaitJWTToken drop to Debug: the condition is expected and handled on such a channel, the caller simply re-authenticates, and repeating it on every SSH authentication buried the one message that is actionable. * [client] Stop the local-metrics manager leaking out of the profile test localmetrics.NewManager runs a goroutine until its context is done, and the test handed it context.Background(), so the manager outlived the test and stayed in the test binary for every case that followed. * [client] Keep the cached SSH JWT across a down/up cycle Clearing the cache in cleanupConnection also caught Down, which ends the connection and not the session: the peer stays enrolled, `up` reconnects without going back to the IdP, and the token still belongs to the same NetBird identity. With a long cache TTL that cost the owner a fresh device-code flow for nothing, since the owner binding is what keeps the token away from other local accounts. Clear it on the two paths where the session really ends and the next one may belong to a different NetBird user: profile logout when the profile is the active one, and active-profile logout. SwitchProfile already cleared it on its own. * [client] Resolve the merge conflict in the profile-logout cleanup main extracted the inline profile-logout cleanup into cleanupAfterProfileLogout, which this branch had edited in place to clear the SSH JWT cache. Take main's helper and move the clear inside it. The helper returns early when the profile that was deregistered is not the active one, so the cache is still only cleared when the session that owns the token actually ends. * [client] Do not cache an SSH JWT obtained under a session that ended WaitJWTToken polls the IdP with s.mutex released, and that wait can run for as long as the user takes in the browser. A logout or a profile switch in the meantime clears the cache, but the poll then completed and stored its token anyway, so the entry the next session read belonged to the previous one. Give the cache a generation that clear advances. WaitJWTToken takes the generation before the wait and hands it back to store, which keeps the token only while the generation still matches. The two mutexes are distinct, so this was never a data race and the race detector could not have found it: the window is between two separately locked sections. * [client] Make the profile-switch test switch a profile SwitchProfile with a nil request skips switchProfileIfNeeded, so the test only covered the no-op path and would have passed with profile-transition invalidation broken. Create a second profile and name it in the request, then assert the active profile actually moved before checking the cache. Also correct the comment on the Down test: the logout handlers do call cleanupConnection. What changed is that clearing the cache is no longer one of the things cleanupConnection does. * [client] Take the SSH JWT cache generation when the flow is created WaitJWTToken read the generation after validating the device code, but the flow it belongs to is created earlier, in RequestJWTAuth, and SwitchProfile does not reset s.oauthAuthFlow. A profile switch between the two therefore advanced the generation before it was ever read: the guard compared the new session against itself and let the token through, which is the case it exists to stop. Record the generation on the flow when RequestJWTAuth creates it, and read it from there. The whole span from the request to the IdP answering now counts as one session for the cache. * [client] Correct two test comments the clear-on-Down change invalidated Moving the clear out of cleanupConnection left two comments describing the old behaviour: newTestServer said cleanupConnection clears the cache, and the comment above TestJWTCache_ClearDropsTheEntry listed Down among the callers of clear. Neither is true any more. * [client] Read the SSH JWT cache generation before the IdP round trip RequestJWTAuth read the generation where it stored the flow, which is after RequestAuthInfo has talked to the IdP. A logout or a profile switch during that call advanced the generation first, so the flow recorded the new session's value and the later store was accepted: the window moved rather than closed. Read it with the config, under the same s.mutex section. SwitchProfile holds that mutex across its own clear(), so the config and the generation cannot be torn apart by a switch. |
||
|
riccardom
|
ef88c4de5f |
[client] Gate settings updates on value, not on field presence
The update-settings kill switch (--disable-update-settings /
NB_DISABLE_UPDATE_SETTINGS / the MDM DisableUpdateSettings key) forbids
changing settings, but it decided what a "change" was by looking at
whether a field was present in the request. The CLI fills the whole
config surface of SetConfigRequest and LoginRequest from its flags and
environment on every `netbird up` (setupSetConfigReq in cmd/up.go), so a
client configured by environment restates its own configuration on every
start and tripped the gate every time.
SetConfig only warned about that, but Login carries the same fields and
was gated the same way, and Login runs inside the CLI's backoff loop: the
daemon answered every attempt with codes.Unavailable, `netbird up` never
completed, and a container with NB_DISABLE_UPDATE_SETTINGS plus any
config env var (NB_MANAGEMENT_URL, for one) could not come up at all.
Both gates now compare values. Config.WouldChange is the dry-run half of
UpdateConfig: it runs the very same diff logic (Config.apply) against a
copy of the stored config, so the gate cannot drift from what an actual
update would do, nor go stale when a field is added. A request that
restates what the profile already holds changes nothing and is allowed; a
request that diverges is refused exactly as before, and a dry run that
cannot be evaluated fails closed. A profile with no config on disk yet is
judged against the config the daemon would create for it.
For Login, the compared input comes from loginOverridesInput, which
persistLoginOverrides also uses to perform the write, so the gate judges
precisely the two fields a login can persist (management URL, pre-shared
key) and no field it ignores.
Two adjacent defects surfaced while making the comparison exact:
- Config.apply compared URLs as raw strings, so the same endpoint spelled
without its default port ("https://api.netbird.io" vs
"https://api.netbird.io:443") counted as a new value and rewrote the
config. It now compares the parsed forms.
- UpdateConfig did not collapse the redacted pre-shared key, unlike
UpdateOrCreateConfig and DirectUpdateConfig, so a UI round-trip of the
mask replaced the stored key with asterisks.
The CLI warning for a refused SetConfig said the method was not available
in the daemon, which sent people looking for a version mismatch that was
not there; it now reports the refusal.
|
||
|
Zoltan Papp
|
c3cf7c0c37 |
[client] Clarify that metrics ingest X-Peer-ID is not a credential (#7363)
* [client] Clarify that metrics ingest X-Peer-ID is not a credential The ingest endpoint is intentionally unauthenticated: it accepts telemetry from peers of both cloud and self-hosted deployments, and for a self-hosted peer there is no shared trust anchor to authenticate against. The X-Peer-ID header is a correlation tag whose format check exists to bound InfluxDB tag cardinality. Both the function name (validateAuth) and the 401 response implied an authentication control that was never there, which invites the reading that the check can be bypassed. Rename it to validatePeerIDFormat and return 400, matching the other input validation failures in the same handler. Document the intent in the godoc and the infra README. No behavioural change for clients: push.go classifies responses by 2xx range rather than by status code, so 400 and 401 are handled identically. * [client] Reject metrics ingest bodies whose peer_id tag disagrees with the header validateTag checked tag names against the per-measurement allowlist but only bounded the value length, so the peer_id tag was free-form text up to 64 bytes and could differ from the X-Peer-ID header the request was accepted with. Tie the two together: the tag value must equal the header value. Since the header is already checked to be 16 hex characters, this transitively constrains the tag to the same shape. Every client sends the same value in both places (metrics.go feeds agentInfo.peerID to both push.SetPeerID and the body tags), so well-behaved clients are unaffected. The mismatch is rejected rather than silently overwritten: rewriting the value would re-serialize caller-controlled text back into line protocol and would hide misbehaving senders instead of surfacing them. Rejection also matches the other input validation failures in the same handler, which all return 400. This narrows the value space of the peer_id tag but does not by itself bound InfluxDB series cardinality: a sender that puts the same arbitrary 16 hex characters in both the header and the body still passes. Limiting that needs a per-source rate limit in front of the service. * [client] Document what the metrics ingest peer_id check does and does not bound The README described X-Peer-ID as the correlation tag, but grouping is done by the peer_id tag in the submitted line protocol: that is what is forwarded to InfluxDB, while the header only serves as the value each tag is checked against. It also claimed the format check bounds tag cardinality. It bounds the value space of the tag, not the number of distinct series, so state that explicitly and point out that series cardinality has to be limited outside this service. * [client] Set timeouts on the metrics ingest HTTP server The server ran on http.ListenAndServe with no timeouts, silenced with a nolint:gosec for G114. Without ReadHeaderTimeout a client can hold a connection open by sending headers slowly, and without ReadTimeout or IdleTimeout connections accumulate on an endpoint that takes unauthenticated requests. Construct an http.Server with explicit limits instead, which also drops the nolint. Handler stays nil so the existing DefaultServeMux registrations are unaffected. WriteTimeout is deliberately larger than the 10s upstream client timeout: the response is only written after the forward to InfluxDB completes, so a tighter value would cut off the server's own valid response. * Revert "[client] Document what the metrics ingest peer_id check does and does not bound" This reverts commit |
||
|
Maycon Santos
|
ebc259e30b |
[management,client] Gate remote jobs behind an admin opt-in with MDM support (#7153)
This introduces a disabled-by-default allow-remote-jobs setting that controls whether the management server may run jobs (such as debug bundles) on a peer. The flag propagates end to end: through client configuration, the daemon SetConfig and Login requests, authentication, and system info, up to management, where it is stored on the peer and exposed on the peers API as remote_jobs_allowed. The client refuses any management-requested job unless the peer has opted in. Because enabling remote jobs crosses the user-to-root boundary, turning it on requires privilege, mirroring the SSH-server gate. Administrators can enforce the setting through MDM policy on both macOS and Windows, and MDM can also override the debug-bundle upload URL. The change ships policy documentation and generated profile templates, and adds configuration, conflict, and enforcement tests covering the opt-in, privilege, and MDM paths. |
||
|
evgeniyChepelevandClaude Opus 5
|
652d5f3c15 |
[client] Reuse the profile's account for iOS SSO logins (#7193)
* [client] Reuse the profile's account for iOS SSO logins Android reads the profile's stored account and passes it as the OIDC login_hint, and records it again after a successful login. iOS did neither: it called GetOAuthFlow with an empty hint, so a re-login was resolved by whatever session the browser's cookie jar held rather than by the account the profile belongs to. With a non-ephemeral browser session that is the wrong account as soon as more than one is signed in. Mirror client/android/login.go: hint from mobile.ReadProfileEmail before the flow, mobile.WriteProfileEmail after Login succeeds. Storing after Login and not before keeps a rejected token from leaving a hint that points at an account which cannot be used. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * [client] Persist the account email on tvOS and on the device flow Two paths left a profile with no account bound, so every later login went out without a login_hint — the case this change exists to remove. WriteProfileEmail went through util.WriteJsonWithRestrictedPermission, which writes a temp file and renames it over the target. The tvOS App Group sandbox blocks exactly that, which is why the config sitting next to this file is written with DirectWriteOutConfig. On tvOS the email write therefore failed and was dropped with a warning. Use DirectWriteJson: the file is rewritten whole from a single key, so the only thing atomicity buys here is surviving a crash mid-write, and a torn file reads back as "no email" and is replaced by the next login. The device authorization flow never populated TokenInfo.Email, unlike the PKCE flow, so a client driven through it — Android TV and tvOS — bound no account at all. Parse the ID token there too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * [client] Report a failed close from DirectWriteJson The deferred close assigned its error to err, but the return value was not named, so the assignment went nowhere: a close that failed was logged and the function still returned nil. The write is only durable once the file closes cleanly, so every caller — the management config, the profile configs and the profile account email — could be told the data landed when it had not. Name the return so the assignment does what its shape always intended, and report the failure once. When the body succeeded the close error is returned and the caller logs it. When the body already failed, that error is the one that explains the failure and is what the caller gets, which leaves the deferred log as the only place the close failure can surface — at debug, per the logging rules for close errors on writes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Zoltan Papp
|
4749005a50 |
[client] Resolve profiles for the sudo invoking user instead of root (#7238)
* [client] Resolve profiles for the sudo invoking user instead of root
The SSH server flags force `netbird up` through sudo, but the CLI resolved
every per-user path with the process user. As root that reads root's own
(empty) local state, so a `sudo netbird up` silently switched the daemon from
the user's profile to the default one — cancelling any login already waiting
in the browser — and then ran an SSO login for the default profile's config.
Whichever account that login returned, the default profile's peer belongs to
someone else, so every attempt ended in "peer is already registered by a
different User or a Setup Key", with nothing telling the user why.
Resolve the acting user through SUDO_USER when running as root: the active
profile, the profile config paths and the stored account email now come from
the invoking user's directories. Privilege decisions are untouched — they stay
on the kernel credentials of the daemon connection, which an environment
variable can never influence; a forged SUDO_USER only selects a profile root
could select anyway.
The invoking user's directories are strictly read-only under sudo. Anything
root wrote there would be root-owned and break the user's own runs, so instead
of chowning files back, the local writes are skipped: the active-profile
bookkeeping and the account-email state simply do not update from a sudo run
(the daemon records the switch on its side; a skipped email write costs at
most one extra account prompt later).
Plain root — no sudo context — has no user to act for, so the ambiguity is
refused instead of guessed at: when the daemon's active profile differs from
what root resolves and no --profile was given, up fails with a message naming
both profiles, instead of silently switching the daemon and failing later with
the ownership error.
* [client] Act on the daemon-resolved profile and fail closed in the root guard
Under sudo the local active-profile mirror is not updated, so up/login
re-reading it after a profile switch acted on the previous profile; use
the daemon-resolved ID directly instead. The plain-root guard now runs
after the readiness wait, denies on lookup errors and empty responses,
and matches the owning username as well; an unowned profile (fresh
install) and a daemon predating the RPC stay allowed. Write-skip
decisions key off the sudo environment alone so a transient user lookup
failure cannot turn a run into writing root-owned files into the user's
directory, and RemoveProfileState honors the read-only rule too.
* [client] Return a wrapped error instead of double-reporting the dial failure
* [client] Read the profile from the daemon when the local mirror is not authoritative
Under sudo without --profile, `up` took the active profile from the invoking
user's local active_profile.txt mirror and drove the daemon to it. But that
mirror is never written under sudo (the SwitchProfile write is a no-op), so it
goes stale after any --profile run and silently switches the daemon back to the
mirror's default. The plain-root guard was meant to refuse exactly this
ambiguity but only ran for plain root, never for the sudo case the fix targets.
When there is no --profile and the mirror is not authoritative (sudo or plain
root), take the profile the daemon already holds for the invoking user instead
of the stale mirror: stay on the user's current profile when the daemon owns it
(or it is unowned, as on a fresh install), and refuse with a --profile hint when
the daemon is on another user's profile. A daemon predating the RPC keeps the
mirror-derived profile.
Reproduce (before this change):
1. As a non-root user misha, with the daemon installed and running:
sudo netbird up --profile work
misha connects on the `work` profile.
2. Because the local mirror write is skipped under sudo,
~misha/.config/netbird/active_profile.txt still says `default` (or is still
absent, which also resolves to `default`).
3. Run a bare:
sudo netbird up
The CLI reads `default` from the frozen mirror and sends ProfileName=default;
the daemon silently switches away from `work` and brings the tunnel up on
`default` — a different account/peer than the one last chosen, with no
warning. After this change step 3 stays on `work`.
* [client] Return a sentinel error instead of nil-nil for the missing daemon RPC
* [client] Load the extend-session hint from the resolved profile
* [client] Fail closed instead of reading root's config when the sudo user lookup fails
* [client] Fail closed in InvokingUser when the sudo user lookup fails
A previous change made baseConfigDir fail closed when SUDO_USER cannot be
resolved, but InvokingUser still fell through to user.Current(). Those two
guards disagreed: the active-profile mirror and the email state refused to
read root's directory, while every profile-path caller happily resolved as
root.
The consequence of a transient NSS failure under sudo was that
Profile.FilePath resolved through getConfigDirForUser("root"), creating
/var/lib/netbird/root and reading the profile JSON from there, and the CLI
sent Username "root" to the daemon in SetConfig and ListProfiles, so the
daemon resolved the same phantom namespace. The invoking user was silently
moved onto a root-owned profile instead of being told the lookup failed.
Fail closed at the single source of the fallback. getConfigDirForUser is
left alone on purpose: it is a pure path helper that also serves
daemon-supplied usernames, and under sudo with a successful lookup it must
still create the invoking user's own profile directory.
|
||
|
Riccardo Manfrin
|
352a1d348a |
[client] Do not log the WireGuard key on a parse failure (#7379)
The error already says what went wrong: an invalid base64 payload reports the offending byte offset, and a wrong key size reports the length. Passing the key itself adds nothing an operator can act on, and the line is emitted at Error level, so it reaches every log sink and every debug bundle. |
||
|
Maycon Santos
|
1081ca006d |
[management,client] Add anonymize level and upload URL to remote debug bundle jobs (#7147)
This extends the management-requested remote debug-bundle job with two new, optional parameters. anonymize_level selects how aggressively the bundle is scrubbed: "default" keeps internal (private) IP ranges readable, while "strict" also anonymizes private, CGNAT and link-local addresses; the value is trimmed and lowercased, and an unknown level is rejected at creation. upload_url lets an operator point the peer at a specific upload service instead of the default one; it must be a well-formed https URL with a host, and an empty value falls back to the default upload server. Both fields flow through the job workload API and are surfaced in the create-debug-job modal on the dashboard. Validation is shared so the client executor and the management boundary agree on what a valid upload URL is, preventing drift between the two checks. |
||
|
Maxim EgorovandRiccardo Manfrin
|
930a25319d |
[client] Keep the route selection on an invalid request and apply it on a partial one (#7292)
* [client] Keep the route selection when every requested ID is unavailable
A non-append SelectRoutes() wipes the current selection before applying
the requested one, but it validated the requested IDs only afterwards,
while already mutating. A request naming no available route at all left
every route deselected and returned an error - so a typo in a route ID
silently dropped the user's exit node, and the routes stayed applied
while the selector claimed nothing was selected.
Validate first and bail out before touching any state when nothing in
the request is available. A request with at least one available route
keeps applying the valid part and reporting the rest, and an empty
request still deselects everything, since that is the caller asking for
exactly that rather than a failed lookup.
* [client] Trim the new comments to the contributing guide's length budget
CONTRIBUTING.md caps comments at 90 characters per line and roughly 250
per comment. The three comments added by this PR were over both limits.
The test comments also restated their own test names, so they lose that
half and keep only the why.
* [client] Apply the route selection even when some IDs are unknown
SelectRoutes and DeselectRoutes returned the error before TriggerSelection,
so a request mixing valid and unknown network IDs changed the selector but
never reached the routing table. The valid routes read as selected while
`ip route` showed nothing.
Trigger the selection first and return the error afterwards. The inner
selectRoutes already applied the valid part of a partial request, only the
outer layer dropped it.
* [client] Publish the network selection event on a partial failure
Returning early on error was correct while an error meant nothing had
happened. A partial failure now changes the selection and the routing
table, so returning first left the change with no trace in the event log
or the UI, even though the new state had already been broadcast.
* [client] Cover the append and deselect-all paths of the selection guard
The append path was never destructive and behaves the same with or without
the early return, so that case is characterization rather than a regression
test. The deselect-all case is a real guard: the early return also skips
resetting deselectAll, so a typo no longer drops the "nothing selected,
including future networks" policy.
* [client] Pin that a fully invalid selection disturbs nothing
The selection is now applied on every request, including one where no ID is
known and the selector is left untouched. Nothing may be torn down or
reinstalled on that path.
* Revert "[client] Publish the network selection event on a partial failure"
This reverts commit
|
||
|
Laotree
|
24959e1ed9 |
[client] Drop agentConnecting whenever ICE session state clears (#7327)
* [client] Drop agentConnecting whenever ICE session state clears Closing a WorkerICE raced a blocked dial goroutine: Close released the agent while connect() was still inside Dial, and the goroutine's own cleanup skipped its flag reset because w.agent no longer matched. With agentConnecting stuck on true, evalConnStatus read the peer as connected, the reconnection guard stopped sending offers and same-session offers were dropped, so the peer could not recover without a restart. An aborted recreate in OnNewOffer reaches the same wedged state without any race. Route every teardown path through one abandonNegotiation helper so the agent and flag fields always clear together; Close now also cleans up residual state left by an aborted recreate. * [client] Drive the ICE teardown race test through the real dial goroutine The regression test simulated the stale goroutine by calling closeAgent directly, so it pinned the symptom rather than the mechanism. Rework it to start a real negotiation, tear it down mid-flight and let the actual goroutine run its own cleanup: with no remote responder the dial can only fail once Close cancels it, so the interleaving stays deterministic without sleeps or injection points. Assert the full idle state that abandonNegotiation owns (agent nil, connecting false, remote session ID empty) instead of only InProgress, and make the stale-cleanup ownership test verify that the newer session survives field by field. * [client] Assert live remote session ID after stale ICE cleanup The stale-cleanup test compared a snapshot captured before closeAgent ran, so clearing the field during cleanup would have gone unnoticed. Read the field under the mutex after the cleanup instead. * [client] Give the ICE race tests a no-op signal client The candidate callback fires from a real gather and dereferences the signaler, so a nil one crashes the test package intermittently when gather wins the race against Close. Build the worker with a stub signal.Client instead. * [client] Read the ICE dial cancel func from an argument in connect The error paths read w.agentDialerCancel without holding muxAgent while OnNewOffer rewrites the field for a newer negotiation, a data race the new teardown test trips under -race. Reading a stale value also let an old goroutine cancel another session's dial. Capture the cancel func at goroutine spawn, like the dial context already is. * [client] Guard the ICE dial success path against stale negotiations The stale-cleanup guard in closeAgent only protected teardown. Its success-path counterpart was missing: an older negotiation could complete agentDial after a newer one replaced w.agent, then clear the newer session's agentConnecting, record lastSuccess and publish its dead connection via onICEConnectionIsReady. Verify ownership under muxAgent twice: right after the dial returns, so a stale goroutine drops its connection before touching a closed agent, and again at the state-commit point, atomic with the agentConnecting and lastSuccess writes, so a replacement arriving in the meantime cannot get its state clobbered. Both paths close the stale connection and return without modifying worker state. A regression test holds session A's dial open until session B is installed, then releases it; the stale connection must be discarded and B's agent, connecting flag and remote session ID must survive. * [client] Fix ICE teardown test leak and document the stale delivery window A code review of the stale-negotiation guard found a leftover resource leak in TestWorkerICE_StaleCloseAgentKeepsCurrentSession: session B is never closed, so its ICE sockets and blocked dial goroutine live as long as the test process. Register t.Cleanup(w.Close). The delivery race flagged after the success-path guard is pre-existing and self-correcting - the newer negotiation overwrites the transient endpoint - so document it in the existing todo instead of locking the callback, which would invert lock order against Conn.Close. Adjust the teardown test comment to match the now-synchronous Close flag clearing. |
||
|
Riccardo Manfrin
|
6620219939 |
[client] Add catch-all NRPT rule when NetBird is the primary DNS resolver (#7071)
* [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.
|
||
|
Viktor Liu
|
63c26be72f | [client] Add local Prometheus metrics endpoint (#6689) | ||
|
Pascal FischerandDmitri Dolguikh
|
e06c17cf59 |
[management] network map from nmap data type (#6919)
Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io> Co-authored-by: Dmitri Dolguikh <dmitri.external@netbird.io> |
||
|
Viktor Liu
|
473392a935 | [client] Tolerate a still-locked updater binary when cleaning up after an update (#7286) | ||
|
dmitri-netbird
|
0a9ce7f797 |
[client] fix a flake in TestResolver_ConcurrentStaleHitsCollapseRefresh test (#7326)
* fix a flake in TestResolver_ConcurrentStaleHitsCollapseRefresh test Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io> * use testify's eventually asserts Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io> --------- Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io> |
||
|
Viktor Liu
|
7e8b4e1417 |
[client, proxy] Remove lazy connection exclusions and run Rosenpass on the embedded proxy (#6763)
* Run lazy connection manager for rosenpass peers * Treat forward-target peers as normal lazy connections * Run Rosenpass in permissive mode on the embedded proxy |
||
|
Viktor Liu
|
51095cb986 |
[client, management] Support per-peer lazy connection state and default proxy peers to lazy (#6762)
* Support per-peer lazy connection state and default proxy peers to lazy * Classify forward targets from incoming config in lazy exclusion * Set IsUserspaceBind mock so lazy manager starts in engine test * Skip lazy exclude reconciliation when the set is unchanged * Keep cached lazy flag when a sync carries no peer config |
||
|
Viktor Liu
|
ccf8f43cb1 | [client] Ask the OS for privileges when a guarded SSH setting is changed (#7066) | ||
|
Zoltan Papp
|
15fff4c164 |
[client] Sweep connections on network loss via a shared netevents manager (#7254)
Losing the last network only flipped the availability state: the dead management, signal and relay sockets stayed silently connected until their own timeouts, so the client kept reporting Connected with no network at all. Introduce client/netevents with a Manager that ties the availability state, the connection sweeper and the status recorder together, and move the netstate and netsweep packages under it (netsweep renamed to sweep). SetNetworkAvailable(false) now also sweeps the registered connections so their owners redial and the listener reaches the NoNetwork state. The Android and iOS bindings own a Manager instance and inject it through the constructors; consumers hold the concrete *Manager whose nil zero value reports always-online and never sweeps, with interfaces kept only as parameter contracts. The relay guard settle wait moved into the Manager as WaitSettled, removing the netevents import from the relay package. |
||
|
Viktor Liu
|
5fc191167d | [client] Revert declaring multi-buffer support for the loopback XDP program (#7303) | ||
|
Viktor Liu
|
7f03a2e86f | [client] Hold a peer offer or answer that arrives before the handshaker starts listening (#7255) |