mirror of
https://github.com/netbirdio/netbird.git
synced 2026-10-07 05:59:06 +02:00
a8dff998ef3cb38f435c1e4c9648dcd129ce48c1
40
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Riccardo Manfrin
|
a8dff998ef |
[client] Gate settings updates on value, not on field presence (#7398)
* [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.
* [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.
* [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.
* [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.
* [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.
* [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.
* [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.
* [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.
* [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.
* [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.
* [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.
* [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)
* [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.
* [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.
* [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).
* [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.
* [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.
* [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.
* [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.
* [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.
* [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.
* [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.
* [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.
* [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.
* [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.
* [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.
* [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.
* [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.
* [client] Resolve the merge conflicts left in the tree
|
||
|
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> |
||
|
Zoltan Papp
|
89c6e84a41 |
[client, ios] Fix context cancellation during restart (#7329)
* fix(mobile): stop the client synchronously so a restart cannot inherit a cancelled context
Original finding
----------------
A user reported that leaving home and switching from wifi to cellular killed
all Internet traffic until NetBird was turned off. A debug bundle captured the
failure (iOS, CLI 0.75.0, self-hosted management, generated 2026-08-18 01:17;
the incident is at 2026-08-17 22:37:38-51 UTC).
The bundle shows the whole sequence:
22:37:38.255 management sync stream drops (keepalive ACK timeout)
22:37:43.670 Swift: "Network type changed: wifi -> cellular" -> schedules a
restart with a 1s debounce
22:37:44.737 Go: "ensuring wg interface is removed, Netbird engine context
cancelled" - engineCtx dies, every peer gets context canceled
22:37:49.910 iface.go:238 "failed to remove WireGuard interface utun6:
timeout when waiting for interface utun6 to be removed"
-> the teardown stretches out for ~5s
22:37:50.710 Swift: "restartClient: starting client", needsLogin=false
(so this is NOT a login expiry)
22:37:51.013 Go: connect.go:476 "exiting client retry loop due to
unrecoverable error: context canceled" - the OLD run dies here
22:37:51.333 Go: grpc.go:135 "failed creating connection to Management
Service: context canceled" - the NEW start, 2ms after the old
run finally exited
22:37:51.334 Swift: "restartClient: start failed" -> widget disconnected
then nothing for 15 minutes
The tunnel stayed installed with no engine behind it, so every packet was
black-holed. status.txt, generated ~14 hours later, still reads Management:
Disconnected / Signal: Disconnected / Peers count: 0/0 - the client never
recovered on its own.
Root cause
----------
Client.Stop() cancelled a shared ctxCancel field and returned immediately,
without waiting for the run loop to exit. The Swift stop{} completion handler
therefore fired while the Go teardown was still running (stretched out by the
utun6 removal timeout), and the start that followed landed on a context that
the outgoing run was about to cancel.
Two further paths wrote the same shared field. IsLoginRequired() and
LoginForMobile() each overwrote c.ctxCancel, so any call to them during a live
session discarded the running engine's cancel function. restartClient() calls
needsLoginCached() on exactly this path.
Changes
-------
- Stop() now drives the stored ConnectClient: ConnectClient.Stop() cancels the
run context and blocks on runExited, so the caller's completion handler only
fires once the run loop has really finished. The ctxCancel path stays as a
fallback for when no ConnectClient exists yet (e.g. during LoginForMobile).
- Run() owns its cancel in a local variable, so a concurrent call that
overwrites the shared field can no longer cancel this run's context through
the deferred cleanup.
- IsLoginRequired() and LoginForMobile() use local cancels and leave the shared
field alone. LoginForMobile's cancel moves into the deferred cleanup of the
goroutine that outlives the call, so the OAuth token wait is not cut short.
- The Android SDK gets the same treatment. The structural defect is identical
there, but the trigger is absent: Android has no automatic engine restart on
a network type change, and no interface-removal timeout to stretch the
teardown. This part is preventive, not a fix for an observed failure.
* fix(mobile): do not let a superseded startup publish its client
Review found a window the previous commit left open. Run stored its cancel
function and only published the ConnectClient later, after loading config and
constructing the client. A Stop landing inside that window found no
ConnectClient, cancelled the run and returned immediately. A new Run could then
publish its own client, and the cancelled older run — still executing — would
overwrite it with a client that was already being torn down. The next Stop
stopped that stale client and left the live one running with nothing tracking
it.
Runs now carry a generation. Run claims one before doing any work and publishes
its client only while the generation is still current; a superseded run returns
without touching the shared state. Stop bumps the generation, so any startup
still in flight is invalidated, then cancels it and waits for the run to exit
before returning (20s cap so a wedged teardown cannot block the caller
forever).
setState is gone: publishState replaces it at both call sites on each platform.
* fix(ios): add a non-waiting Stop for callers on a deadline
Stop now waits for the run loop to exit, which is what a restart needs but
wrong for stopTunnel: iOS gives NEPacketTunnelProvider only a few seconds
there before it kills the extension, and the wait can run to its 20s cap.
Waiting past the deadline earns a SIGKILL, so the next start inherits a dirty
state instead of the orderly shutdown the wait was meant to buy.
StopWithoutWait tears the client down and returns. ConnectClient.Stop blocks on
runExited with no cap of its own, so the non-waiting path runs it detached
rather than only skipping the runDone wait.
Android keeps a single blocking Stop: it has no equivalent deadline.
* fix(mobile): guard the run lifecycle with a single lock
Stop and beginRun each touched the same lifecycle state across two locks in
sequence: take stateMu, release it, then take ctxCancelLock. A run starting in
that gap installed its own cancel before Stop reached it, so Stop cancelled the
fresh run and left its own target running — the same class of defect this branch
exists to fix, this time in the locking rather than the state.
ctxCancel moves into the stateMu group, and both sides take their snapshot in
one critical section. ctxCancelLock then guarded nothing and is gone.
* fix(mobile): drop the run-generation machinery for a serialized lifecycle
The platform callers (Swift/Kotlin) always stop before starting and coalesce
restarts, so the generation counter guarded against call patterns that cannot
occur. Replace it with a single-run contract:
- startRun refuses a second Run while the previous one has not exited
- finishRun clears the published state on every exit path, including errors
- Stop cancels and waits for the run loop with a bounded timeout; it no
longer calls ConnectClient.Stop, whose wait is unbounded
- concurrent Stops wait on the same exit channel instead of returning early
- a superseded startup no longer reports a clean nil exit
* revert(android): drop the run lifecycle changes
Android does not have the defect this PR fixes. On ux/ios-style-redesign the
EngineRestarter is gone: network changes are handled as events instead of an
engine restart, so nothing stops the client and starts it again.
The remaining stop() callers are all final teardowns on the main thread with a
framework deadline - the stop-engine broadcast receiver, onDestroy, onRevoke and
the binder's stopEngine. A Stop that waits for the run loop would risk an ANR
there for a race that cannot occur, so the fix stays iOS-only.
* fix(ios): make loginComplete race-free
The OAuth goroutine spawned by LoginForMobile sets loginComplete after the
call has returned to Swift, while the Swift side polls IsLoginComplete and
later calls ClearLoginComplete from its own thread. The plain bool made all
three unsynchronized: the store may never become visible to the poller, and
a Clear racing the store can be lost, leaving a stale true that makes the
next login look already complete.
Switch the field to atomic.Bool. It is a standalone flag rather than part of
the run lifecycle that stateMu guards, and it has to stay readable while the
login goroutine is still in flight.
|
||
|
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. |
||
|
Zoltan Papp
|
070a0a7bf1 |
[client, android] Handle network changes without restarting the engine (#7144)
On network changes the client restarted the whole engine. That is heavy-handed and slow: it tears down working state to recover from a transition the engine could handle itself. This replaces the restart with proper network event handling. Suspend the retry loops while no network is available. Instead of burning through backoff intervals against an unreachable network, the reconnection loops park until the OS reports a usable network again. Reconnect immediately on a network switch. When the OS hands us a new network, connections bound to the old one are swept and re-dialed right away, rather than waiting for a timeout to notice they are dead. |
||
|
Viktor Liu
|
5584f8ef0a | [client] Add strict anonymization level and MAC anonymization to debug bundles (#7102) | ||
|
evgeniyChepelevandZoltan Papp
|
f2318a8fef |
[client] iOS - Remove duplicate Login RPCs from the iOS SDK (#6931)
## Describe your changes Removes redundant `Login` RPCs from the iOS SDK bindings. ## Issue ticket number and link ## Stack <!-- branch-stack --> ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [ ] Created tests that fail without the change (if possible) - [ ] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) ### Docs PR URL (required if "docs added" is checked) Paste the PR link from https://github.com/netbirdio/docs here: https://github.com/netbirdio/docs/pull/__ <!-- codesmith:footer --> --- <a href="https://app.blacksmith.sh/netbirdio/codesmith/netbird/pr/6931"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v2.svg"><img alt="View with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"></picture></a> <a href="https://backend.blacksmith.sh/track/enable-autofix?expires=1787779607&installation_model_id=427504&pr_number=6931&repository=netbirdio%2Fnetbird&return_to=https%3A%2F%2Fgithub.com%2Fnetbirdio%2Fnetbird%2Fpull%2F6931&signature=ce1631be2a5ffdba58c44b4669b0d480dc9f956102cf10a0c7b5845a800cdc68"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"></picture></a> <sup>Need help on this PR? Tag <code>@codesmith-bot</code> with what you need. Autofix is disabled.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added an interactive iOS login option that starts authentication directly when needed. * Improved login flow handling, including clearer error reporting and successful-login notifications. * Login configuration is now saved automatically after successful authentication when applicable. * **Bug Fixes** * Prevented duplicate login requests during iOS startup. * Improved startup behavior and error propagation when the management service is unavailable. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Removes redundant `Login` RPCs from the iOS SDK bindings. Both changed files are behind the `ios` build tag — Android, desktop and the shared core are not affected. ### Problem `auth.Auth.IsLoginRequired()` is not a cheap probe: it calls `doMgmLogin()` and classifies the resulting error, so every "is login required?" check costs a **full `Login` RPC**. There is no lighter way to ask. As a result the iOS client issued ~7 `Login` requests before the first `Sync`, where Android issues ~3, and the extra ones were indistinguishable from real logins in the management logs. Three of those came from this package: 1. `Run()` called `LoginSync()` before starting the engine, which performs `IsLoginRequired` **and** `Login` — two RPCs. This duplicated the engine's own `loginToManagement` (`client/internal/connect.go`), which runs immediately before the first `Sync` and is the authoritative login. The `Login(ctx, "", "")` inside `LoginSync` could not even establish anything: with an empty setup key and empty JWT, a registration attempt fails by construction, so it was a pure check. 2. `Auth.login()` called `IsLoginRequired()` again before opening the browser, even when the caller had already determined that login is needed. This is not only wasted traffic: - **It pushes peers toward the server-side login ban.** In `management/internals/shared/grpc/loginfilter.go`, every login with unchanged metadata increments `sessionCounter`, and exceeding `reconnLimitForBan` (30) within `reconnThreshold` (5 min) bans the peer for `baseBlockDuration` (10 min), doubling on repeat. Redundant logins carry identical metadata, so they count against exactly this budget. At 7 logins per connect the budget is exhausted after ~4 reconnects instead of ~10 — reachable on flaky mobile networks. - **Each redundant check is a potential 2-minute stall.** `IsLoginRequired` retries with backoff up to `MaxElapsedTime` (2 min) and returns `true` on failure, so an unreachable server was reported as "login required" rather than as a timeout, and the `LoginSync` pre-flight could abort engine startup on that basis. ### Changes **`client/ios/NetBirdSDK/client.go`** — `Run()` no longer performs the `LoginSync()` pre-flight. The engine's `loginToManagement` remains the single authoritative login. **`client/ios/NetBirdSDK/login.go`** — new exported `LoginInteractive`, which skips the `IsLoginRequired()` pre-flight and goes straight to the browser / device-code flow, for callers that have already established login is required. `LoginWithDeviceName` keeps the check for callers where the auth state is unknown (tvOS). Both now delegate to a shared `startLogin()`. ### Why this is safe An expired or revoked session still fails the connection, one step later and through a single path: `loginToManagement` returns `PermissionDenied` → the deferred `MarkManagementDisconnected` records it on the shared status recorder → `ClientStop` fires the listener's disconnect callback, where `IsLoginRequiredCached()` reports login-required → the client tears the tunnel down. The error is also returned out of `Run()`. Where the server is unreachable, the engine now retries with backoff and recovers on its own instead of aborting the start. Co-authored-by: Zoltan Papp <zoltan.pmail@gmail.com> |
||
|
Viktor Liu
|
feecb993f4 | [client] Restrict debug bundle log path and upload destinations (#6975) | ||
|
Zoltan Papp
|
2ef457be95 |
[client] Unify route selection in the route manager (#6928)
## Describe your changes Move route select/deselect handling from the daemon server into exported routemanager methods (SelectRoutes, DeselectRoutes, SelectAllRoutes, DeselectAllRoutes) so every consumer shares one implementation: v4/v6 exit-pair expansion, exit-node mutual exclusion, and selection triggering. Previously the exit-node exclusivity lived only in the daemon's SelectNetworks RPC, so the Android and iOS bindings could leave two exit nodes selected until the next network map reconciliation. Both bindings now call the shared manager methods and enforce exclusivity at toggle time, matching the desktop behavior. ## Issue ticket number and link ## Stack <!-- branch-stack --> ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [ ] Created tests that fail without the change (if possible) - [ ] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) ### Docs PR URL (required if "docs added" is checked) Paste the PR link from https://github.com/netbirdio/docs here: https://github.com/netbirdio/docs/pull/__ <!-- codesmith:footer --> --- <a href="https://app.blacksmith.sh/netbirdio/codesmith/netbird/pr/6928"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v2.svg"><img alt="View with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"></picture></a> <a href="https://backend.blacksmith.sh/track/enable-autofix?expires=1787772098&installation_model_id=427504&pr_number=6928&repository=netbirdio%2Fnetbird&return_to=https%3A%2F%2Fgithub.com%2Fnetbirdio%2Fnetbird%2Fpull%2F6928&signature=31ad59e1483e1582cd447a8db2fe21e5309230e631cbd0cad0f977cd15fb7b9b"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"></picture></a> <sup>Need help on this PR? Tag <code>@codesmith-bot</code> with what you need. Autofix is disabled.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Route selection/deselection is now handled through shared route-manager APIs for both individual routes and “all routes”. * Exit-node selections automatically enforce mutual exclusivity while keeping non-exit routes unaffected. * **Bug Fixes** * Unknown or unavailable route IDs now return errors, and exclusivity is preserved even when some route IDs fail. * **Tests** * Added route-selection tests covering exclusivity (including IPv4/IPv6), select-all behavior, partial errors, and invalid IDs. * **Refactor / Chores** * Simplified Android, iOS, and server routing flows to delegate to the shared manager; updated mocks and removed redundant routing command logic/dependencies. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Zoltan Papp
|
ca80e49aa0 |
[client] Refresh WireGuard stats in mobile debug bundles (#6814)
The iOS and Android DebugBundle paths built GeneratorDependencies without setting RefreshStatus, so the bundle's status.txt read the cached peer state instead of live WireGuard interface stats. When the periodic health probe had not run yet, connected relayed peers showed "handshake: -" and "0 B/0 B" even though the interface was passing traffic. Wire RefreshStatus to RunHealthProbes on both platforms, matching the desktop daemon path in client/server/debug.go. The engine reference is already available in the cc.Engine() block used for client metrics. ## Describe your changes ## Issue ticket number and link ## Stack <!-- branch-stack --> ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [ ] Created tests that fail without the change (if possible) - [ ] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) ### Docs PR URL (required if "docs added" is checked) Paste the PR link from https://github.com/netbirdio/docs here: https://github.com/netbirdio/docs/pull/__ <!-- codesmith:footer --> --- <a href="https://app.blacksmith.sh/netbirdio/codesmith/netbird/pr/6814"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v2.svg"><img alt="View with Codesmith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"></picture></a> <a href="https://backend.blacksmith.sh/track/enable-autofix?expires=1786881440&installation_id=146802194&pr_number=6814&repository=netbirdio%2Fnetbird&return_to=https%3A%2F%2Fgithub.com%2Fnetbirdio%2Fnetbird%2Fpull%2F6814&signature=c07cbcef1028a366312f7743e6479f2fddafd568aac498ab95828a372ac6843c"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with Codesmith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"></picture></a> <sup>Need help on this PR? Tag <code>/codesmith</code> with what you need. Autofix is disabled.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved debug bundle generation on Android and iOS by refreshing connection health status before collecting diagnostic information. * Ensured debug bundles include more current health-related data for troubleshooting. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Zoltan Papp
|
6df0175607 |
[client] Add IsLoginRequiredCached for iOS mobile client (#6447)
Expose a network-free login-required check backed by the in-memory status recorder. Unlike IsLoginRequired(), which creates a fresh auth client and performs a blocking network call, IsLoginRequiredCached() reports whether the LAST observed management error was an auth failure (PermissionDenied/ InvalidArgument). This lets the iOS connection listener detect a mid-session token expiry from within onDisconnected during teardown without blocking on a slow or unavailable network. |
||
|
|
3c23700e56 |
[client] Add iOS debug bundle support in Go (#6270)
* Add iOS debug bundle support in Go Thread cacheDir through NewClient -> RunOniOS -> MobileDependency.TempDir so the iOS client can pass its sandbox-writable cache directory for debug bundle zip file creation instead of os.TempDir(). Move log collection into platform-dispatched addPlatformLog(): - iOS: adds the file-based Go client log (with rotation, stderr/stdout companions and anonymization handled by addLogfile) plus the Swift app log (swift-log.log) written by the iOS app into the same log directory - Other non-Android platforms: existing file-based log + systemd fallback Narrow the debug_nonandroid.go build tag to !android && !ios so iOS no longer attempts the systemd journal fallback. Add a DebugBundle() entry point to the iOS Go client that generates a bundle, uploads it and returns the upload key. It works with or without a running engine: when the engine is up it reuses the live config, sync response and client metrics; otherwise it loads the config from disk (or the preloaded tvOS config). Guard the live config/ConnectClient behind a state mutex since DebugBundle may run on a different thread. * Include the iOS state file in the debug bundle addStateFile() resolved the state path via ServiceManager.GetStatePath(), which on iOS points at a hard-coded default that does not exist in the app sandbox, so the state file was silently skipped. Add an optional StatePath to GeneratorDependencies and use it when set, falling back to the ServiceManager default otherwise. The iOS DebugBundle passes the client's actual state file path (the App Group profile state), matching the Android bundle which includes the state file. * ios: enable sync response persistence for debug bundle Turn on sync response persistence before starting the engine so DebugBundle can include the network map. On iOS the store is disk-backed (see syncstore) to keep the map out of the constrained process memory. * ios: pass log file path through NewClient constructor (#6393) Add logFilePath field to Client struct and expose it as a parameter in NewClient so callers provide the Go log path at construction time. Wire it into DebugBundle via GeneratorDependencies.LogPath so the debug bundle includes client.log and swift-log.log regardless of whether the bundle is triggered by the app or the management server. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * ios: pass log file path to engine for remote debug bundles RunOniOS started the engine with an empty LogPath, so EngineConfig.LogPath was never set. Management-triggered (jobs) debug bundles read the log path from the engine config, so they collected no client logs (client.log, rotated logs, swift-log.log). The GUI path was unaffected because it passes c.logFilePath directly to the bundle generator. Thread c.logFilePath through RunOniOS into the engine config so remote bundles include the client logs too. --------- Co-authored-by: evgeniyChepelev <68751844+evgeniyChepelev@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Zoltan Papp
|
08a2b63675 |
[client] propagate exit-node deselect to synthesized v6 (::/0) route (#6296)
* [client] propagate exit-node deselect to synthesized v6 (::/0) route
When a client deselects an IPv4 exit node, the auto-generated IPv6 default
route (::/0) was still selected and pushed onto the tunnel interface, even
though the user disabled the exit node. On an exit node without a real IPv6
egress this blackholes IPv6 traffic, and because clients prefer IPv6 (happy
eyeballs) it can break general connectivity.
Root cause: the synthesized v6 route gets a different NetID than its v4 base
(base + "-v6"). The route selector keys deselects by NetID and defaults
unknown NetIDs to selected, so the "-v6" entry was never matched by the v4
deselect. The effectiveNetID() mirror that solves exactly this is used by
HasUserSelectionForRoute and FilterSelectedExitNodes, but categorizeUserSelection
called the raw IsSelected(), bypassing it and mis-categorizing the v6 pair as
user-selected.
Add RouteSelector.IsSelectedForExitNode(), which applies effectiveNetID before
the selection check, and use it in categorizeUserSelection. IsSelected() is left
untouched so non-exit code paths don't make unrelated "*-v6" routes inherit v4
state. Adds regression tests for the v4/v6 deselect mirror and explicit-v6
override.
* [client] add DIAG logging to trace exit-node v6 (::/0) route filtering
Temporary diagnostics to find why a deselected v4 exit node's synthesized
::/0 route still reaches the tunnel. Logs the full install path: incoming
client networks, route-selector state before/after the management-driven
update, what updateExitNodeSelections deselects/selects, and per-route
KEEP/SKIP/DROP decisions in FilterSelectedExitNodes and applyExitNodeFilter.
To be reverted once the real root cause is confirmed from a client log.
* [client] clear orphaned v6 exit selection when v4 pair is toggled
Root cause of the leaking ::/0 route, confirmed from client logs: the
synthesized "-v6" exit route could stay explicitly selected in the persisted
route-selector state while its v4 base was deselected (selected=[...-v6],
deselected=[...v4base]). Because the v6 entry then has its own explicit state,
effectiveNetID stops mirroring the v4 base, so FilterSelectedExitNodes keeps
::/0 and it is installed on the tunnel even though the user disabled the exit
node. This happened because the iOS SDK's deselect only pairs the "-v6" sibling
via ExpandV6ExitPairs when the v6 route is present in the current routesMap; a
deselect at a moment it wasn't expanded left the v6 selection orphaned.
Fix at the selector write path so it is independent of routesMap timing: when a
v4 exit NetID is selected or deselected, clear any orphaned explicit state on
its "-v6" sibling (clearPairedV6Locked), unless the sibling is part of the same
batch (the deliberate ExpandV6ExitPairs case). The v6 then falls back to
inheriting the v4 base via effectiveNetID, so a v4 deselect also drops ::/0 and
a v4 select brings both back.
Adds regression tests: a stale explicit v6 selection is cleared by a later v4
deselect, and an explicit v6 select made in the same batch is preserved.
* [ios] compute route connection status in the bridge
The iOS bridge exposed a route's Network as a possibly comma-joined string
("0.0.0.0/0, ::/0" for a merged exit node) but no connection status, forcing
the UI to infer status by string-matching that joined value against peer
routes — which never matched for the merged exit node, leaving it stuck as
not-connected. Android already computes status in the core (findBestRoutePeer).
Mirror that here: add a Status field to RoutesSelectionInfo and compute it from
the connected peers' route tables, matching the route's primary prefix, a merged
exit node's extra v6 prefix, or a dynamic route's domain pattern (the key the
route manager records). The UI can now read the status directly.
* [client] remove exit-node v6 DIAG logging and tidy routeselector
Drop the temporary DIAG diagnostics added to trace the leaking ::/0 route
(the root cause is fixed and confirmed). Also reorganize routeselector.go so
the exit-node helpers (clearPairedV6Locked, isExitNode) sit next to the
exit-node code paths and MarshalJSON/UnmarshalJSON are grouped together.
* [client] mirror v4 exit selection onto v6 pair at write time
The synthesized "-v6" exit route shares its v4 base's NetID plus a "-v6"
suffix. Selection state was reconciled at read time via effectiveNetID, a
mirror that could only be applied on exit-node code paths, which forced a
parallel IsSelectedForExitNode() alongside IsSelected() and a clearPairedV6Locked()
orphan cleanup on every toggle. That machinery still missed the case observed
in the field: a persisted state with the v4 base deselected but its "-v6"
sibling explicitly selected (orphaned). Because effectiveNetID returns the v6
entry itself once it carries explicit state, and clearPairedV6Locked only fires
on a live toggle, the loaded orphan survived and the ::/0 route leaked onto the
tunnel despite the exit node being disabled, breaking IPv6 (happy eyeballs).
Treat the v4/v6 exit pair as a single toggle and keep state consistent at write
time instead. RouteSelector.SyncPairedSelection forces the "-v6" entry to match
its v4 base unconditionally, resetting any orphaned explicit state. The route
manager, which knows the route prefixes, computes the pairs (V6ExitMergeSet) and
calls it from updateRouteSelectorFromManagement before selection is read, so both
collectExitNodeInfo and FilterSelectedExitNodes see consistent state, including
pairs loaded from persisted selector state.
This removes effectiveNetID, IsSelectedForExitNode and clearPairedV6Locked; the
selector is literal again and no longer needs the "exit-node paths only" caveat.
HasUserSelectionForRoute and applyExitNodeFilter use the raw NetID.
Adds a selector test for SyncPairedSelection (including the orphaned-v6 case) and
a route-manager test reproducing the persisted-orphan scenario from the field log.
* [client] add DIAG logging to trace v6 exit-pair mirror
The write-time mirror did not eliminate the leak in field testing. Re-add the
DIAG diagnostics around the exit-node selection flow to capture a fresh trace:
- UpdateRoutes: incoming client networks, selector state before/after the
management update, and the networks remaining after FilterSelectedExitNodes.
- mirrorV6ExitPairSelections: the NetIDs present in this update and the v6 pairs
V6ExitMergeSet derives from them (reveals whether the v4 base and its ::/0 pair
are present in the same update so the pair can be matched).
- SyncPairedSelection: the base/paired state before and after the sync.
- FilterSelectedExitNodes / applyExitNodeFilter: per-route SKIP/KEEP/DROP and the
selection lookups behind each decision.
- updateExitNodeSelections / logExitNodeUpdate: categorization and deselect set.
Temporary; to be removed once the root cause is confirmed.
* [client] remove v6 exit-pair mirror DIAG logging
Drop the temporary DIAG diagnostics added to trace the v4/v6 exit-pair mirror.
The field log confirmed the write-time mirror keeps the pair consistent (the
::/0 route is only ever applied alongside its v4 base and is dropped on deselect),
so the diagnostics are no longer needed.
|
||
|
Viktor Liu
|
9ed2e2a5b4 | [client] Drop DNS probes for passive health projection (#5971) | ||
|
Viktor Liu
|
205ebcfda2 | [management, client] Add IPv6 overlay support (#5631) | ||
|
Zoltan Papp
|
f23aaa9ae7 |
[client] iOS: structured ResolvedIPs collection for domain routes (#6090)
* [client] iOS: structured ResolvedIPs collection for domain routes Replace comma-joined ResolvedIPs string with a gomobile-friendly ResolvedIPs collection (Add/Get/Size), mirroring the Android bridge in client/android/network_domains.go. This allows the iOS app to match domain-route resolved IPs against connected peer routes without parsing CSV strings, fixing the route status indicator for dynamic (DNS) routes. * [client] iOS: align dynamic route exposure with Android bridge For dynamic (DNS) routes the Swift side previously received "invalid Prefix" as the Network value, forcing UI code to special-case that sentinel. The Android bridge uses Domains.SafeString() instead so peer.routes entries (which also derive from Domains.SafeString()) match directly. Mirror that here. Also fix the resolved IP lookup: resolvedDomains is keyed by the resolved domain (e.g. api.ipify.org), not the configured pattern (e.g. *.ipify.org). Group entries by ParentDomain like the daemon does in client/server/network.go, so wildcard route patterns get their resolved IPs populated. |
||
|
Maycon Santos
|
e2c2f64be7 |
[client] Fix iOS DNS upstream routing for deselected exit nodes (#5803)
- Add GetSelectedClientRoutes() to the route manager that filters through FilterSelectedExitNodes, returning only active routes instead of all management routes - Use GetSelectedClientRoutes() in the DNS route checker so deselected exit nodes' 0.0.0.0/0 no longer matches upstream DNS IPs — this prevented the resolver from switching away from the utun-bound socket after exit node deselection - Initialize iOS DNS server with host DNS fallback addresses (1.1.1.1:53, 1.0.0.1:53) and a permanent root zone handler, matching Android's behavior — without this, unmatched DNS queries arriving via the 0.0.0.0/0 tunnel route had no handler and were silently dropped |
||
|
Zoltan Papp
|
fe9b844511 |
[client] refactor auto update workflow (#5448)
Auto-update logic moved out of the UI into a dedicated updatemanager.Manager service that runs in the connection layer. The UI no longer polls or checks for updates independently. The update manager supports three modes driven by the management server's auto-update policy: No policy set by mgm: checks GitHub for the latest version and notifies the user (previous behavior, now centralized) mgm enforces update: the "About" menu triggers installation directly instead of just downloading the file — user still initiates the action mgm forces update: installation proceeds automatically without user interaction updateManager lifecycle is now owned by daemon, giving the daemon server direct control via a new TriggerUpdate RPC Introduces EngineServices struct to group external service dependencies passed to NewEngine, reducing its argument count from 11 to 4 |
||
|
Zoltan Papp
|
ded04b7627 |
[client] Consolidate authentication logic (#5010)
* Consolidate authentication logic - Moving auth functions from client/internal to client/internal/auth package - Creating unified auth.Auth client with NewAuth() constructor - Replacing direct auth function calls with auth client methods - Refactoring device flow and PKCE flow implementations - Updating iOS/Android/server code to use new auth client API * Refactor PKCE auth and login methods - Remove unnecessary internal package reference in PKCE flow test - Adjust context assignment placement in iOS and Android login methods |
||
|
Zoltan Papp
|
9c9d8e17d7 |
Revert "Revert "[relay] Update GO version and QUIC version (#4736)" (#5055)" (#5071)
This reverts commit
|
||
|
Maycon Santos
|
24df442198 |
Revert "[relay] Update GO version and QUIC version (#4736)" (#5055)
This reverts commit
|
||
|
Zoltan Papp
|
8722b79799 |
[relay] Update GO version and QUIC version (#4736)
- Go 1.25.5 - QUIC 0.55.0 |
||
|
shuuri-labs
|
96cdd56902 |
Feat/add support for forcing device auth flow on ios (#4944)
* updates to client file writing * numerous * minor * - Align OnLoginSuccess behavior with Android (only call on nil error) - Remove verbose debug logging from WaitToken in device_flow.go - Improve TUN FD=0 fallback comments and warning messages - Document why config save after login differs from Android * Add nolint directive for staticcheck SA1029 in login.go * Fix CodeRabbit review issues for iOS/tvOS SDK - Remove goroutine from OnLoginSuccess callback, invoke synchronously - Stop treating PermissionDenied as success, propagate as permanent error - Replace context.TODO() with bounded timeout context (30s) in RequestAuthInfo - Handle DirectUpdateOrCreateConfig errors in IsLoginRequired and LoginForMobile - Add permission enforcement to DirectUpdateOrCreateConfig for existing configs - Fix variable shadowing in device_ios.go where err was masked by := in else block * Address additional CodeRabbit review issues for iOS/tvOS SDK - Make tunFd == 0 a hard error with exported ErrInvalidTunnelFD (remove dead fallback code) - Apply defaults in ConfigFromJSON to prevent partially-initialized configs - Add nil guards for listener/urlOpener interfaces in public SDK entry points - Reorder config save before OnLoginSuccess to prevent teardown race - Add explanatory comment for urlOpener.Open goroutine * Make urlOpener.Open() synchronous in device auth flow |
||
|
Zoltan Papp
|
011cc81678 | [client, management] auto-update (#4732) | ||
|
Diego Romar
|
08f31fbcb3 |
[iOS] Add force relay connection on iOS (#4928)
* [ios] Add a bogus test to check iOS behavior when setting environment variables * [ios] Revert "Add a bogus test to check iOS behavior when setting environment variables" This reverts commit 90ca01105a6b0f4471aac07a63fc95e5d4eaef9b. * [ios] Add EnvList struct to export and import environment variables * [ios] Add envList parameter to the iOS Client Run method * [ios] Add some debug logging to exportEnvVarList * Add "//go:build ios" to client/ios/NetBirdSDK files |
||
|
shuuri-labs
|
7285fef0f0 |
feat: Add support for displaying device code (UserCode) on Android TV SSO flow (#4800)
- Modified URLOpener interface to pass userCode alongside URL in login.go - added ability to force device auth flow |
||
|
Viktor Liu
|
d71a82769c | [client,management] Rewrite the SSH feature (#4015) | ||
|
Viktor Liu
|
75327d9519 | [client] Add login_hint to oidc flows (#4724) | ||
|
Viktor Liu
|
1d5e871bdf |
[misc] Move shared components to shared directory (#4286)
Moved the following directories: ``` - management/client → shared/management/client - management/domain → shared/management/domain - management/proto → shared/management/proto - signal/client → shared/signal/client - signal/proto → shared/signal/proto - relay/client → shared/relay/client - relay/auth → shared/relay/auth ``` and adjusted import paths |
||
|
hakansa
|
cb8b6ca59b |
[client] Feat: Support Multiple Profiles (#3980)
[client] Feat: Support Multiple Profiles (#3980) |
||
|
Viktor Liu
|
bc7b2c6ba3 | [client] Report client system flags to management server on login (#3187) | ||
|
|
ddc365f7a0 |
[client, management] Add new network concept (#3047)
--------- Co-authored-by: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Co-authored-by: bcmmbaga <bethuelmbaga12@gmail.com> Co-authored-by: Maycon Santos <mlsmaycon@gmail.com> Co-authored-by: Zoltan Papp <zoltan.pmail@gmail.com> |
||
|
Pascal Fischer
|
e40a29ba17 | [client] Add support for state manager on iOS (#2996) | ||
|
Viktor Liu
|
869537c951 | [client] Cleanup dns and route states on startup (#2757) | ||
|
Zoltan Papp
|
0c039274a4 |
[relay] Feature/relay integration (#2244)
This update adds new relay integration for NetBird clients. The new relay is based on web sockets and listens on a single port. - Adds new relay implementation with websocket with single port relaying mechanism - refactor peer connection logic, allowing upgrade and downgrade from/to P2P connection - peer connections are faster since it connects first to relay and then upgrades to P2P - maintains compatibility with old clients by not using the new relay - updates infrastructure scripts with new relay service |
||
|
pascal-fischer
|
855fba8fac | On iOS add error handling for getRouteselector (#2394) | ||
|
pascal-fischer
|
47752e1573 | Support DNS routes on iOS (#2254) | ||
|
pascal-fischer
|
272ade07a8 | Add route selection to iOS (#1944) | ||
|
Misha Bragin
|
52fd9a575a | Add quantum resistance status output (#1608) | ||
|
pascal-fischerandMaycon Santos
|
818c6b885f |
Feature/add iOS support (#1244)
* starting engine by passing file descriptor on engine start * inject logger that does not compile * logger and first client * first working connection * support for routes and working connection * small refactor for better code quality in swift * trying to add DNS * fix * updated * fix route deletion * trying to bind the DNS resolver dialer to an interface * use dns.Client.Exchange * fix metadata send on startup * switching between client to query upstream * fix panic on no dns response * fix after merge changes * add engine ready listener * replace engine listener with connection listener * disable relay connection for iOS until proxy is refactored into bind * Extract private upstream for iOS and fix function headers for other OS * Update mock Server * Fix dns server and upstream tests * Fix engine null pointer with mobile dependencies for other OS * Revert back to disabling upstream on no response * Fix some of the remarks from the linter * Fix linter * re-arrange duration calculation * revert exported HostDNSConfig * remove unused engine listener * remove development logs * refactor dns code and interface name propagation * clean dns server test * disable upstream deactivation for iOS * remove files after merge * fix dns server darwin * fix server mock * fix build flags * move service listen back to initialize * add wgInterface to hostManager initialization on android * fix typo and remove unused function * extract upstream exchange for ios and rest * remove todo * separate upstream logic to ios file * Fix upstream test * use interface and embedded struct for upstream * set properly upstream client * remove placeholder * remove ios specific attributes * fix upstream test * merge ipc parser and wg configurer for mobile * fix build annotation * use json for DNS settings handover through gomobile * add logs for DNS json string * bring back check on ios for private upstream * remove wrong (and unused) line * fix wrongly updated comments on DNSSetting export --------- Co-authored-by: Maycon Santos <mlsmaycon@gmail.com> |
