mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-19 05:09:06 +02:00
262ce8c3b215f5776bcf22914b0c387723e11952
1459
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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
|
15c0a2903d |
[client] Return the context error when the SSH handshake fails with it (#7426)
* [client] Return the context error when the SSH handshake fails on a context deadline The handshake mapped the context deadline onto the socket but returned the raw socket error. Which error surfaces depends on a race between the x/crypto ssh readLoop and kexLoop goroutines: the kexLoop write fails with i/o timeout and closes the conn, and the readLoop then reports use of closed network connection. Callers checking errors.Is(err, context.DeadlineExceeded) never matched, and TestSSHClient_ContextCancellation flaked on the FreeBSD job. Handshake now wraps the context error when the context is done or its deadline has passed. The deadline comparison is needed because the socket deadline and the context timer fire independently, so ctx.Err() can still be nil when the deadline-triggered socket error arrives. * [client] Close the silent test server conn without racing t.Cleanup The accept goroutine registered the conn close via t.Cleanup, which can run after the test's cleanup list has already been drained, leaving the accepted connection open. The goroutine now holds the conn until a cleanup-closed channel signals the end of the test and closes it on the way out. * [client] Bind the SSH handshake to the context instead of a socket deadline Mapping only the context deadline onto the socket left context cancellation unobserved: an in-flight handshake kept running until the deadline, and the error classification had to guess whether a raw socket error was caused by the deadline. Closing the conn from context.AfterFunc covers both deadline and cancellation, and ctx.Err() is already set by the time the close-induced error surfaces, so the time-based DeadlineExceeded attribution is no longer needed. The stop() result guards the window between a successful handshake and the AfterFunc firing so a closed conn is never handed back as a client. |
||
|
Brad Ison
|
76ea72237f |
[management] Add Agent Network managed proxy to the API spec (#7433)
Defines the cloud-side managed gateway provisioning surface (POST/GET /api/integrations/agent-network/managed-proxy) and its response objects so clients consume generated types instead of hand-written ones. POST is idempotent: 202 when the call starts (or restarts) provisioning, 200 when a deployment already exists; 409 names an already-assigned endpoint the managed flow does not own and 503 signals temporarily exhausted endpoint allocation. |
||
|
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) | ||
|
Zoltan Papp
|
fbd4730f0b |
[client] Expose the remote jobs opt-in in the Android and iOS SDK preferences (#7406)
Remote jobs (debug bundle requests from management) are gated behind Config.RemoteJobsAllowed, which defaults to false and could only be enabled through the CLI flag or an MDM policy. The mobile SDKs had no way to set it, so the mobile clients always refused the job. Add GetRemoteJobsAllowed/SetRemoteJobsAllowed to both mobile Preferences types, following the existing ServerSSHAllowed accessors, so the apps can offer a settings toggle for it. |
||
|
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
|
0bfe2f6ead |
[client] Answer terminalLoginError's nil case on its own terms
A successful Login reaches terminalLoginError with a nil error, and nothing covered that. It happens to work on grpc v1.80.0 — gstatus.FromError(nil) answers (nil, true), and Status.Code tolerates a nil receiver by returning codes.OK, which is not in the terminal set — but that is a chain of internal details to be relying on for the common path, and none of it was asserted. Now the nil error is handled where it is obvious, and the table covers it. Reported by CodeRabbit on PR #7398, which called it a panic; measured on v1.80.0 it is not one. The gap was the untested reliance, not a crash. |
||
|
riccardom
|
34cae56ee3 |
[client] Stop netbird login from retrying a refusal for 30 seconds
`netbird up` and `netbird login` both run Login through the backoff cycle, and each carried its own copy of the list of codes that end it. Only up.go learned about codes.FailedPrecondition, so a refused `netbird login` kept retrying and then reported "login backoff cycle failed" instead of what the daemon said. terminalLoginError is now that list, once, next to WithBackOff — the duplicated copies are what let the two commands disagree in the first place. Reported by cubic-dev-ai on PR #7398. |
||
|
riccardom
|
6790c34b08 |
[client] Let an unprivileged caller log out a profile with no identity
The empty-key check sat behind requirePrivilegeForDeregistration, so an unprivileged logout of an identity-less profile was refused with PermissionDenied instead of completing as the no-op it is. And it was refused for most profiles, not a corner case: the gate arms whenever the SSH server is enabled, and sshServerEnabled reads an absent ServerSSHAllowed as enabled, so every legacy profile qualifies. The check now runs first. What the gate protects against is handing this machine's registered key to another management server; with no key there is nothing to hand over and nothing to protect. Reported by CodeRabbit and cubic-dev-ai on PR #7398, both on the same defect. |
||
|
riccardom
|
cbeda854cf |
[client] Restore the gofmt alignment of the error constants
The comment added above errUpdateSettingsDisabled in the previous commit split the const block's alignment group, so gofmt wants the two constants above it re-aligned. CI runs gofmt, so this would have failed the lint job. |
||
|
riccardom
|
70a15f709c |
[client] Name the reader storedConfigAtPath actually calls
The purity note still said profilemanager.GetConfig, which the rename two commits later turned into GetExistingConfig. Reported by cubic-dev-ai on PR #7398. |
||
|
riccardom
|
682b2de549 |
[client] Fail netbird up when the daemon refuses the settings update
With the update-settings kill switch on, `netbird up --enable-rosenpass` connected and said almost nothing: SetConfig refused the change, the CLI downgraded that to a warning, and Login carries no rosenpass field to apply, so the flag was silently dropped. The setting stayed disabled, which is the point of the switch, but the caller was never told their request had been ignored. The refusal now travels as codes.FailedPrecondition instead of codes.Unavailable, and the CLI fails on it. Unavailable means "the daemon cannot serve this call", which is why the CLI downgraded it and why client/ui/services reads it as an unreachable daemon — both wrong for a daemon that answered and refused. FailedPrecondition also matches what the MDM gate already returns for a managed field, so both refusals are now one class of error, and it is added to the login backoff's early-exit codes so a refused login stops instead of retrying for 30s. This does not put the container back in the deadlock: with the value-aware gate, a client restating its own configuration is not refused at all, so nothing reaches this path unless a real change was asked for. |
||
|
riccardom
|
1d213dd4d4 |
[client] Treat a profile with no identity as already deregistered
Two findings on the same consequence of pure reads: a profile can legitimately carry no keys, because logging out clears them in place. - sendLogoutRequestWithConfig went straight to wgtypes.ParseKey and failed with "incorrect key size: 0" on the second logout of the same profile. There is nothing to deregister for a peer that was never registered, so it returns cleanly. Before pure reads this case was hidden: the read minted a key and the daemon dialed management with one it had never seen. - The mobile logout read the config with the generating reader right after checking the file exists. The two are not atomic, so a profile removed in between was resolved from the defaults and recreated by the write that follows. It uses the existing-file reader now. Reported by cubic-dev-ai and CodeRabbit on PR #7398. |
||
|
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
|
bc49b7249c |
[client] Stop the gate test from dialing the real management server
TestLogin_RestatingTheStoredConfigPassesTheGate asserts that the gate lets a no-op login through, and the handler then went on to do the login for real: isLoginRequired builds an auth client when isLoginRequiredFn is unset, so the test dialed the profile's management URL — api.netbird.io:443. It took 1.05s locally and would hang on a runner with no egress, for a fact about the gate that needs no network at all. Stubbed like the login_outcome tests do. The test now runs in 0.00s. Reported by cubic-dev-ai on PR #7398. |
||
|
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
|
294fa0bcfd |
[client] Address the remaining bot findings on PR #7398
- Login logged the active-profile-state error and returned the same cause; the repo's guidelines call for one or the other, and the wrapped error is the one that carries context. (CodeRabbit) - `netbird up` reported a codes.Unavailable SetConfig failure as "the daemon refused the settings update", but that code also covers a daemon that became unreachable. It now reports what the daemon said without asserting why. (cubic-dev-ai) - TestLogin_ChangingTheManagementURLIsRefused asserted the error and nothing else, while "refused before it can touch daemon state" is the contract. It now checks the stored management URL, the in-progress login and the active profile, matching its SetConfig counterpart. (cubic-dev-ai) |
||
|
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
|
ee9a5c2e20 |
[client] Re-take the update-settings decision under the config lock
Login checks twice on purpose: the first check refuses the ordinary case early, and authorizeAndPrepareLogin re-takes the authoritative one under guardedConfigMu because the first is unsynchronized against a concurrent privileged request. The update-settings decision is now equally value-dependent — it compares the request against the stored config — but it was taken only in the first, unlocked check. So a login that was a no-op when it was checked could be written after a concurrent writer had repointed the profile, which is exactly the window the lock exists to close. The decision is now re-taken alongside the privilege one, which also makes it the last read before persistLoginOverrides writes. The test drives that interleaving through the existing afterLoginPreCheck seam and fails without the re-check. |
||
|
riccardom
|
ec30004241 |
[client] Cover the login the update-settings gate used to refuse
The gate's decision procedure was tested directly, but no test drove the Login RPC that the refusal actually broke: the CLI retries Login in a backoff loop, so a refused no-op login is what kept a client configured by environment from ever coming up. The handler-level coverage stopped at the refusal case, which passes on the pre-fix code too. This test fails on the pre-fix daemon with "update settings are disabled" and passes now. Past the gate the handler does real work the test does not stand up, so it asserts only that the refusal did not happen. |
||
|
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 |
||
|
eYey
|
2f55965031 |
[client, android] Type the split tunnelling mode instead of storing a string (#7387)
gomobile carries only basic types, so the typed constants stay unexported and the exported SplitTunnelMode* ints are what the Java side gets. |
||
|
Maycon Santos
|
a1415dbc05 |
[client] Fix the ICEBind races that wedge interface creation (#7377)
* [client] Add tests for the ICEBind open and close races Running many embedded clients in one process intermittently wedges interface creation. A goroutine dump taken from 50 clients shows ten of them parked for seven minutes in Device.IpcSet, in closeBindLocked waiting on device.net.stopping.Wait, holding device.net while every other device goroutine queues behind it on Device.Up. Open writes s.closed and Close reads it with no synchronisation, and Close also closes s.closedChan without the mutex that Open swaps it under. Two Closes can both pass the check and close the same channel, and a Close racing an Open can mark the bind closed while a live channel and live receive functions remain, after which every later Close takes its early return and runs neither close(closedChan) nor StdNetBind.Close. The receive functions never stop, so stopping.Wait never returns. These tests do not fix that. The first pins the contract closeBindLocked depends on and passes today. The other two fail under -race, reporting the races at the three sites above, and pass again once closed and closedChan are guarded consistently. * [client] Release parked receivers so reopening a bind cannot stall receiveRelayed held closedChanMu for the whole of its blocking select, so a parked receiver kept the read lock indefinitely and Open could never take the write lock it needs to install a fresh closedChan. wireguard-go reaches Open from Device.IpcSet and Device.Up with device.net held, so the stall took the device lock with it: interface creation never finished, every other device goroutine queued behind Device.Up, and Engine.Start never returned. Callers now copy the channel under a short read lock and select on the copy. Copying alone would stand a new trap in the same place, because an Open that follows an Open leaves the previous generation parked on a channel no later Close can reach, so Open now closes the outgoing channel before swapping it. closed and closedChan are also updated together under that mutex. Read and written apart, Close could see a stale closed and skip both close(closedChan) and StdNetBind.Close, leaving every receive function running and wedging closeBindLocked on device.net.stopping.Wait, or two Close calls could pass the check together and close the same channel twice. TestICEBindOpenDoesNotBlockOnParkedReceiver fails without this change, without needing the race detector. The other three cover the surrounding contract and report the state races under -race. * [client] Make the bind lifecycle transition atomic and tighten its tests Review caught that the previous commit moved the torn transition rather than removing it. Open published the new generation before calling StdNetBind.Open, so an Open rejected because the bind was already open had already signalled the outgoing generation, and a Close arriving in that window could mark the bind closed while the same call went on to install live sockets. Every later Close then returned early and never shut them down. Open now calls StdNetBind.Open first, so a failure leaves the current generation untouched, and both Open and Close hold the lock across the whole transition. Ordering is safe: StdNetBind.Open reaches muUDPMux through createReceiverFn, and no path takes muUDPMux before closedChanMu. The tests were also weaker than they read. The stress test claimed to cover a stale channel but only ever raced two Closes, and the concurrency test left overlap to goroutine start order. Both now gate their goroutines on a common start, the stress test races an Open against the Closes, and both assert the surviving generation channel is actually closed. Waiting on receive functions to be entered replaces part of the sleep in the reopen probe, and teardown bounds its Close so a regression fails the assertion instead of hanging. Two of the four now fail without the fix and no race detector, the stress test by reproducing close of a closed channel at the Close early return. * [client] Fail the reopen probe when its teardown does not complete closeBounded swallowed its timeout and the cleanup discarded what receiversStopped returned, so the bounds added in the previous commit only stopped teardown hanging. A wedged Close or a parked receiver would have left the test green with a leaked goroutine, which is the failure this test exists to catch. closeBounded now reports whether Close returned, and cleanup fails the test on either bound. |
||
|
Theodor Midtlien
|
e5c0cdf958 | [client] Stay connected with login command (#7384) | ||
|
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. |
||
|
Riccardo Manfrin
|
c170905bc9 |
[client] Allow logging out of the active profile when profiles are disabled (#7360)
* [client] Allow logging out of the active profile when profiles are disabled
A profile-addressed logout was refused outright when the profiles feature is
disabled: handleProfileLogout ran validateProfileOperation, which returned
Unavailable ("profiles are disabled, you cannot use this feature without
profiles enabled") before looking at which profile was targeted.
The desktop UI always addresses logout by profile — both the profile menu and
the session-expiration dialog send the active profile's ID — so a client with
profiles disabled could not log out at all; only a plain `netbird logout`,
which takes the profile-less path, still worked. Logging out of the profile the
daemon is already running is a deregistration, not profile management, and with
profiles disabled there is a single profile anyway, so every profile-addressed
logout is by definition an active-profile logout.
Replace validateProfileOperation with validateProfileLogout, which skips the
profiles-disabled check when the target is the active profile and keeps gating
logout of any other profile. This mirrors switchProfileIfNeeded, which already
gates only the branch that actually manages profiles. The dropped
allowActiveProfile parameter was always true, leaving canRemoveProfile
unreachable, so both are removed.
* [client] Compare the username and propagate state errors on profile logout
Review follow-ups on the logout gate:
Propagate the GetActiveProfileState failure instead of discarding it. A failed
lookup made the target look non-active, so a caller with profiles disabled got
"profiles are disabled" in place of the real error.
Compare the username along with the ID when deciding whether the target is the
active profile, matching switchProfileIfNeeded. Legacy profile IDs are display
names, so two users can hold the same ID in their own profile directories, and
an ID-only match let one user's logout pass the gate against the other user's
active profile. The default profile is shared and carries no username, so it
keeps matching on the ID alone.
Re-read the active profile before the connection teardown rather than reusing
the pre-flight snapshot. Login switches profiles under guardedConfigMu, which
the logout path does not hold, so a login that landed while the deregistration
was in flight would otherwise lose its fresh connection to a stale flag.
* [client] Address review on the profile logout gate
Pass the username down to logoutFromProfile and reuse the running config only
when the target is the active profile for that username. On an ID-only match a
legacy profile ID shared between two users made the connected-client path
deregister the active peer while its connection stayed up, which the gate fix
alone did not cover.
Split the setup-key-less branch of Login into beginSSOLogin, with the
reuse-the-pending-flow decision in pendingOAuthFlowResponse. Login's cognitive
complexity drops from 37 to 21 (gocognit), clearing the SonarQube report on
this file with no behaviour change.
Point the test fixture at an https URL, since the profiles a gated logout must
not touch only need to be unreachable, not plaintext.
|