mirror of
https://github.com/netbirdio/netbird.git
synced 2026-10-07 05:59:06 +02:00
d56e6fc5f4ae07e352ada103fd6354d95d1715c3
223
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> |
||
|
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. |
||
|
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. |
||
|
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.
|
||
|
Viktor Liu
|
63c26be72f | [client] Add local Prometheus metrics endpoint (#6689) | ||
|
Pascal FischerandDmitri Dolguikh
|
e06c17cf59 |
[management] network map from nmap data type (#6919)
Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io> Co-authored-by: Dmitri Dolguikh <dmitri.external@netbird.io> |
||
|
Viktor Liu
|
e4b8bf39d2 |
[client] Fix staticcheck findings from the updated golangci-lint (#7266)
* Fix staticcheck findings reported by the updated golangci-lint * Skip the receive error log when the local context is done |
||
|
dmitri-netbird
|
a144e8c144 |
[client, management] switch to go.uber.org/mock (#7253)
* switch to go.uber.org/mock/gomock Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io> * updated go:generate commands + regenerated mocks Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io> * update go:generate mockgen commands Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io> * removed duplicate import Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io> * fix go:generate Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io> --------- Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io> |
||
|
Viktor Liu
|
e290769df1 | [client] Take the graphical session answer from the caller instead of the daemon environment (#7187) | ||
|
Viktor Liu
|
5584f8ef0a | [client] Add strict anonymization level and MAC anonymization to debug bundles (#7102) | ||
|
Zoltan Papp
|
f2d13b884a |
[client] Fix session expired relogin (#7055)
## Describe your changes After the SSO session expires, the daemon tears the engine down permanently (management returns `PermissionDenied` → `runCancel()` → the retry loop exits for good). The "Session expired" dialog's Login button still drove the extend-session flow, which requires a live engine: the user completed the full browser SSO + 2FA round trip only to get `Failed to extend the session — engine is not initialised`, with no way out other than quitting and relaunching the client. Reproduce: 1. Log in on a desktop client with session expiration enabled (e.g. 16h TTL). 2. Let the session expire (e.g. leave the machine asleep overnight). 3. Wake it, click **Login** on the "Session expired" dialog, complete SSO + 2FA. 4. The error dialog appears and every retry fails the same way. Changes: - The expired branch of the session-expiration dialog now emits `trigger-login`, driving the full `Login → SSO → Up` sequence that rebuilds the client, instead of the extend flow (an expired session can no longer be extended). - `RequestExtendAuthSession` fails fast when the engine is already gone, so the browser/2FA round trip is not wasted on a doomed extend. - The expired tray row navigated the main window to `/#/login`, a route that does not exist and fell through to the main page without starting a login; it now emits `trigger-login` as well. ## Issue ticket number and link <!-- Required for anything that changes behavior. Link the issue (or the validated discussion it came from) that the NetBird team already agreed on. See https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#ticket-first-pr-second --> ## 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) - [ ] I ran and tested this change locally — I did not rely on CI to find out whether it works - [ ] This PR has a single purpose (not a fix + refactor + feature in one) - [ ] This change is a trivial fix, **OR** it links an issue the NetBird team agreed on beforehand. Changes to the public API, gRPC protocols, functionality behavior, CLI / service flags, or new features always need that agreement first. See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#ticket-first-pr-second). > 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/__ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved session extension handling when the client engine is unavailable by prompting users to log in again. - Updated expired-session behavior to trigger the standard login flow, providing a more consistent sign-in experience. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Zoltan Papp
|
78c1c2fc32 |
[client] Probe the daemon login with IsLoginRequired (#7052)
## Describe your changes
Probe the daemon login with IsLoginRequired
The Login probe attemptLogin(ctx, "", "") on an unregistered peer ends
in registerPeer with no setup key and no JWT, which fails locally with
InvalidArgument before reaching Management. Since #6983 classified that
as StatusLoginFailed and returned early, every setup-key enrolment and
every expired-session SSO re-login aborted before using its credentials,
breaking all netbird-cloud e2e runs from commit
|
||
|
Viktor Liu
|
e90be36cd5 | [client] Don't ask for an SSO login when the login never reached management (#6983) | ||
|
Viktor Liu
|
feecb993f4 | [client] Restrict debug bundle log path and upload destinations (#6975) | ||
|
Viktor Liu
|
aed60a2432 | [client] Fix daemon lock order inversion between SetConfig and login (#6978) | ||
|
Viktor Liu
|
0f5d2d91fb | [client] Authorize daemon IPC callers by their local identity (#6967) | ||
|
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
|
6fc05efa6c |
[client] Disconnect daemon on GUI quit via async Down (#6796)
The tray Quit menu now disconnects the daemon before exiting instead of only tearing down the GUI. A new DownAsync RPC lets the daemon start the teardown and return immediately: beginDown cancels the connection under the mutex (so it cannot reconnect), then finishDown (the retry-goroutine wait and status reset) runs on a background goroutine. handleQuit aborts any in-flight profile switch first (so a queued Up cannot reconnect during teardown) and calls DownAsync so quitting never blocks on the engine shutdown. ## Describe your changes ## Issue ticket number and link ## Stack <!-- branch-stack --> ### Checklist - [ ] Is it a bug fix - [ ] Is a typo/documentation fix - [x] 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/6796"><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=1786785261&installation_id=146802194&pr_number=6796&repository=netbirdio%2Fnetbird&return_to=https%3A%2F%2Fgithub.com%2Fnetbirdio%2Fnetbird%2Fpull%2F6796&signature=4fc95ebfed320240170c8318a2bc8750acfa5ad131454e3011c43bc8a5a09ed1"><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 shutdown reliability by continuing teardown even when stopping the service fails (stop errors are logged but not returned). * Refined connection shutdown to return “service not up” errors directly for clearer, more immediate RPC behavior. * Prevented shutdown hangs by making the tray Quit disconnect time-bounded (5 seconds). * Ensured any in-flight profile switch is cancelled before exiting, with quit serialized to avoid races. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
David Fry
|
e70a69bbcf |
[client] Restore residual state in foreground mode before login (#6707)
* Improved residual state restoration during foreground startup and foreground login, ensuring consistent recovery with stale states. * Foreground flows now initialize advanced routing so stale routes are bypassed during login. |
||
|
Maycon Santos
|
8e02154bf5 |
[client] Add SSO login flow timing instrumentation (#6717)
Users reported long delays between finishing browser authentication and the client connecting. Logs could not attribute the time: the PKCE and device flows were silent between issuing the auth URL and returning the token, and nothing recorded when the GUI issued the Up request after WaitSSOLogin completed. Add log lines covering the full chain: PKCE callback arrival and token exchange duration, device-flow polling and approval timing, GUI-side brackets around WaitSSOLogin and Up, daemon-side Up arrival and WaitSSOLogin return, and a frontend stall detector that reports when webview timers were suspended (macOS App Nap / hidden-window throttling), which delays the WaitSSOLogin-to-Up handoff. |
||
|
|
91acb8147c |
[management,client] 0.75.0 release with new desktop UI (#6473)
- **Wails v3 application** (`client/ui`) with a React + TypeScript + Tailwind frontend replacing the Fyne UI: main connection view, exit-node switcher, networks/peers browser with detail panels, profile management, settings (general, network, SSH, security, troubleshooting, appearance), debug-bundle creation, and a first-run welcome flow. - **Internationalization**: go-i18n bundle with 9 locales (en, de, es, fr, hu, it, pt, ru, zh-CN) shared between the tray and the frontend. - **New system tray** implementation with per-platform theme-aware icons, including a native XEmbed host for Linux (`xembed_tray_linux.c`) and a Linux theme watcher. - **Session handling**: auth session watcher (`client/internal/auth/sessionwatch`), pending login flow, session-expiration dialog and tray notifications, and `netbird login` improvements. - **Daemon API extensions** (`daemon.proto`): status stream subscription, event stream, networks/exit-node selection endpoints, and richer full status — with probe throttling on the daemon side to protect against UI-driven request storms. - **UI preferences store** persisted per profile, autostart management via the daemon (single source of truth in HKCU on Windows). - **Build system**: Taskfile-based builds per platform (macOS, Linux, Windows), Docker cross-compilation images, MSIX/NSIS/nfpm/AppImage packaging, and a new `frontend-ui` CI workflow. Co-authored-by: Zoltan Papp <zoltan.pmail@gmail.com> Co-authored-by: Eduard Gert <kontakt@eduardgert.de> Co-authored-by: braginini <bangvalo@gmail.com> Co-authored-by: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Co-authored-by: riccardom <riccardomanfrin@gmail.com> |
||
|
Riccardo Manfrin
|
c9d387bd0d |
[client] fix MDM managementURL conflict on default-port URL echo (#6672)
* Adds failing test * Fixes Management URL normalized compare on MDM |
||
|
Viktor Liu
|
1d8b5f6e5c | [client] Make lazy connections opt-out via NB_LAZY_CONN (#6617) | ||
|
Zoltan Pappandcoderabbitai[bot]
|
2d7b309004 |
[client] Categorize privileged tests behind a build tag and run them in Docker (#6425)
* [client] categorize root/system-mutating tests behind a privileged build tag Tests that need root or mutate host state (nftables/iptables/DNS, TUN/WireGuard interfaces, routes, eBPF, SSH/service install) are now gated behind a //go:build privileged tag. The default `go test ./client/...` runs as a non-root user with no sudo and leaves host networking untouched; mixed files were split so pure-logic tests stay in the default suite. A self-hosting ory/dockertest/v4 harness (client/testutil/privileged) runs the privileged suite inside a --privileged --cap-add=NET_ADMIN container via `make test-privileged`; a DOCKER_CI=true guard skips the spawn when already inside the container. Added `make test-unit` for the host-safe run. * [client] add PRIV_RUN/PRIV_PKGS filters to the privileged test harness The dockertest harness now reads two optional env vars when building the in-container `go test` command: PRIV_RUN adds a -run test-name filter and PRIV_PKGS overrides the package list. Both empty reproduce the full privileged suite, so CI and `make test-privileged` behave as before. Lets a developer run a single privileged test in the container, e.g.: PRIV_RUN=TestNftablesManager PRIV_PKGS=./client/firewall/nftables/... make test-privileged * [client] fix unused-helper lint after the privileged test split Splitting privileged tests into *_privileged_test.go left their shared helpers in the untagged files, so in the default (no-tag) build they had no callers and golangci-lint flagged them as unused. Moved the privileged-only helpers into the privileged files next to their callers (generateDummyHandler; createEngine/startSignal/startManagement/getConnectedPeers/ getPeers + kaep/kasp; (*mockDaemon).setJWTToken). Annotated the shared routing-test fixtures that must stay untagged for cross-platform compilation with //nolint:unused (systemops_bsd expected* vars, ensureIPv6DefaultRoute on bsd/windows, loopbackIfaceWindows), matching the existing linux variant. * [client] fix privileged test CI failures and run the harness on macOS The host-safe unit run dropped sudo but two privileged test groups were never tagged, and the Docker privileged job silently never ran the suite: - Gate the ssh/server PrivilegeDropper command-construction tests behind the privileged tag (they require root to target a different UID); split them into executor_unix_privileged_test.go. - Tag sharedsock raw-socket tests privileged (need CAP_NET_RAW). - Fix the Docker job command: nested single quotes around the build tags closed the sh -c wrapper early, dropping the go list package set and the privileged tag, so go test ran on the empty repo root. Use double quotes. Make the self-hosting harness usable from a dev Mac: - Build it on darwin as well as linux; it only drives Docker. - Resolve the active docker context endpoint into DOCKER_HOST when the default /var/run/docker.sock is absent (Docker Desktop, Colima, OrbStack). - Rename the misspelled containerGoModache constant to containerGoModCache. * Update client/internal/engine_privileged_test.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update client/internal/routemanager/systemops/systemops_linux_test.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update client/internal/routemanager/systemops/systemops_windows_test.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update client/server/server_privileged_test.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * [ci] Run privileged-tagged tests on darwin, windows and freebsd The privileged build tag split moved root/system-mutating tests behind //go:build privileged, but only the linux docker job was given the tag. The native darwin (sudo), windows (PsExec64 -s) and freebsd VM runners already have the required privileges, so add the privileged tag there too to keep CI running the same set of tests as before the split. * [ci] Exclude dockertest harness from the darwin privileged run The privileged tag now compiles client/testutil/privileged on darwin, whose TestRunPrivilegedSuiteInDocker spawns a container the macOS runner has no Docker for. Exclude the harness package from the darwin list, matching the linux job, so the privileged tests run in place without a container spawn. --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> |
||
|
Zoltan Papp
|
ac9529ea8c |
[client] Fix engine lifecyrcle race (#6443)
* [client] always clean up on Engine.Start failure via defer The rosenpass init paths (NewManager/Run) returned without calling e.close(), leaking the WireGuard interface and other partially initialized state on failure. Per-branch cleanup was easy to miss when adding new early returns. Convert Start to a named error return and tear down via a single defer that calls e.close() whenever err != nil, removing the scattered per-branch close() calls (including the redundant one in initFirewall). * [client] make Engine single-use and guard against double Start Create the run context once in NewEngine instead of in Start. This keeps e.cancel valid for the engine's whole lifetime, so Stop can cancel a Start that is blocked waiting on the network while holding syncMsgMux: Stop now cancels before taking the lock, unblocking that Start so it can release the mutex. Reject re-entry into Start: a non-nil wgInterface means a prior Start already ran (ErrEngineAlreadyStarted), and a cancelled run context means the engine was stopped (ErrEngineAlreadyStopped). Both checks run before the cleanup defer so a duplicate call cannot tear down the running engine's state. * [client] let engine context unblock WaitStreamConnected WaitStreamConnected only watched the signal client's own context, which derives from the parent engineCtx rather than the engine's run context. A Start blocked here (signal stream not yet up) could therefore not be released by Engine.Stop, since Stop only cancels the engine's run context. Pass a context into WaitStreamConnected and select on it too, and have the engine pass e.ctx, so Stop cancelling e.ctx unblocks a parked Start. Update the Client interface, the mock, and callers accordingly. * [client] fix Start/Stop race by making the run loop own engine shutdown ConnectClient.Stop stopped the engine directly while the run loop's backoff cycle could still be starting an engine, so Engine.close raced Engine.Start (e.g. firewall setup reading wgInterface while close nils it). embed.Client.Start's rollback only avoided a deadlock by cancelling before Stop; the race itself remained and was caught by -race. Make the run loop the sole owner of engine shutdown: derive the run context in NewConnectClient, and have Stop cancel it and wait for the loop to exit (skipping the wait when the loop never ran) instead of calling engine.Stop. The loop now always stops the engine on its way out, dropping the unsynchronised wgInterface check it used to guard that call. Self-calls from within the loop use runCancel to avoid waiting on themselves. embed keeps a defensive pre-Stop cancel(); the daemon's cleanupConnection gets a TODO to adopt Stop() rather than stopping the engine in parallel. * [client] init context state in engine tests Engine tests built the engine context with context.WithCancel( context.Background()), omitting CtxInitState. Now that the run context is created in the constructor, the wgIfaceMonitor goroutine can reach triggerClientRestart during teardown, which calls CtxGetState and panics on the missing state. Real entry points (up, embed, service) always CtxInitState; only the tests skipped it. * [client] interrupt connect backoff on context cancel The run loop retried with a raw ExponentialBackOff, so a backoff sleep ignored context cancellation. Now that ConnectClient.Stop waits for the run loop to exit, a cancel landing during a sleep would block Stop for the full interval (up to MaxInterval). Wrap the backoff with the run context so Retry returns promptly on cancel; the retry budget itself (MaxElapsedTime) is unchanged. * [client] bound WaitStreamConnected in signal client tests The tests waited on WaitStreamConnected with context.Background() and the client's own context was also Background, so a stream that never connects would hang until the suite timeout. Pass a 5s timeout context and assert StreamConnected afterwards so the tests fail fast with a clear reason. * [client] fix WaitStreamConnected stale-channel race The StreamConnected check and the wait-channel creation took the mutex separately, so notifyStreamConnected could set the status and close/clear connectedCh in between: the waiter then created a fresh channel nobody would ever close and blocked forever. Also, the status read was unlocked while notify wrote it under the mutex (a data race). Do the check and the channel fetch in one locked section; drop the now-unused getStreamStatusChan helper. Pre-existing bug, not introduced by this branch. * [client] abort Start if context cancelled while waiting for signal stream receiveSignalEvents blocks in WaitStreamConnected until the signal stream connects or the context is cancelled. If Stop cancelled e.ctx while Start was parked there, Start kept going: it started the remaining subsystems on a cancelled context and marked a shutting-down engine as started. Return the context error from receiveSignalEvents and propagate it from Start, so the deferred cleanup runs and the cancellation reaches the caller. * [client] clean up all started components on Start failure Start's failure defer only called close(), which covers the wg interface, firewall, rosenpass and port forwarding but leaves connMgr, srWatcher, route/DNS/flow/state managers and the monitor goroutines running. A late failure (e.g. the context-cancelled check after the signal stream) thus leaked them. Extract Stop's locked teardown into stopLocked (caller holds syncMsgMux, does not wait on shutdownWg) and call it from both Stop and Start's defer. The defer also cancels the run context first so goroutines started before the failure unwind. Teardown order is unchanged. |
||
|
Theodor Midtlien
|
ee360963f9 |
[client] Migrate profile identity from display name to ID and allow renaming of profiles (#6367)
* Migrate to profile ids * Migrate android profile manager * Clean up * Fix review * Add ID type * Fix test and runes in ShortID() * Fix profile switch on up and android comments * Revert android profile to string id * Fix feedback * Fix UI feedback * Fix id assignment * Add renaming of profiles * Fix review * Remove ui binary * Fix getProfileConfigPath not validating id * Change resolve handle order and fix server merge problems * Fix mdm test |
||
|
Riccardo Manfrin
|
2bcea9d582 |
[client] add MDM configuration profile support (Windows registry + macOS plist) (#6374)
* Initial scaffolding * Applies MDM override * Unit tests * Helpers business logic * Return error if trying to modify any config that is gated by MDM * Add ManagedFields to returned config over GetConfig * Adds initial 101 MDM policy business logic testing * gRPC MDM changes * MDM Name scoping for clarity * Implements windows loading of MDM policy * Adds missing WGPort config * Cleanup setupKey to align to linear * Align split tunnel code * Adds some log * Prefix every log with MDM * Adds debug config cobra command This can be useful for troubleshooting and checking config now that its resolution is not trivial defaults > config > env cars > CLI/UI > MDM * Adds MDM 1m diff checker & reloader * Adds also up/start after cancel * Publishes event for UI to sync upon MDM changes * Add events to resync UI to actual config This also provide fixup for UI no aligning to changed config when coming from cli up with config flags. * UI behavior conflicts relaxation UI sends full config snapshot with all values. It doesn't make sense to block it if the values are aligned with the values constrained by the MDM policy. It's just simplier to allow values that are compliant. (this goes for the CLI as well at this point) * Lock toggle Settngs * Advanced Settings locking * Fixup presharedkey * Apply MDM locks * Toggle gray in/out for Advanced Settings * Adds support for disabling of Profiles and UpdateSettings feature flags * Adds Gate Login as well when --disable-update-settings=true is given to service This commit tries to settle things with an old PR-4237 which had relaxed the case where the SetConfig returned an `Unavailable` code error. Under this circumnstance the PR allowed the upFunc to just emit a warning and progress further with the login gRPC. Since the login call is consuming the --management-url coming from the `up` command, it might be possible to abuse the "Unavailable" code to inject a management URL that is different from the configured one even though the --disable-update-settings is set to true (?) * Evaluate disable-update-settings errors only when there's an actual override * [UI] Fixup advanced Settings * [UI] Fixup for preshared key * [UI] Fixup for profile enable/disable toggle We need to align the initial state to evaluate the delta in case. The initial state has to be "true" since the profile starts visible. Then we receive MDM and transition the cache bool value to the actual MDM imposed state * Enforces disable networks * [UI] Aligns to "enable/disable once on change only" * Fixup: MDM wins. always * Removes --disable-advanced-settings It was a typo in our meetings. the actual thing is --disable-update-settings * [PROTO] Removes --disable-advanced-settings * [UI] Removes --disable-advanced-settings * Pins feat profile retrieval to notif event * [UI] Fix for "hide" not working when propagating to parent with children * Adds dep for reading plist files * Introduces support for darwing plist loading * Tests MDM config reload via ticker * [PROVISIONING] ADMX/ADML/PS/bash scripts/templates * CI fixes - Add docstrings to `mdm_integration` - refactor for cognitive complexity - mod tidy * Linting * Add docstrings to `mdm_integration` * nil,nil is no policy and no error. Allow it * nil,nil is no policy and no error. Allow it * exclude MDM profile adminstrated keys data from debug bundle * Fixes Rosenpass left disable after MDM unlock * Partial revert coderabbit added docstrings * Renaming fix * Avoid locking on clientRunning bool when the connection is aborted for whatever reason We want to just signal this through the giveUpChan, we will manage the signal from the waiter side and in case set it to false there. THis way we avoid locking, which should allow the MDM down+wait_for_term_chan_signal_+up procedure clientRunning is used to signal two different conditions here: 1. the initialization procedure is over (we have an engine) 2. the connection being up (or being attempted) Probably these two functionalities should not alias, and the failure of the second condition (because of any error) should just drive a reconnection (currently it's not happening, and we silently go idle). OR, mor probably, the two things are the SAME and there should not exist a case where we did the "Up" initialization and connection attempt but we are not still attempting it. * Moves test helper at te very bottom * Addresses github comments * No lock no copy * Prevents engine not stopping within 10 secs from being paired by another instance We instead juts SKIP updating the policy, so 1. the MDM ticker will kick in 1 minute time, 2. find the policy misaligned, 3. enter the onMDMPolicyChange, 4. find the s.clientRunning == true (because it is set to false only in server cleanupConnection, and not by s.actCancel()) 5. call s.actCancel() again if not nil 6. immediately return from <-s.clientGiveUpChan 7. finally call s.restartEngineForMDMLocked() * Since we ARE running there should be a config If the config was cancelled midflight, connect will abort later on * DisableAutoConnect should not stop a running connection. DisableAutoConnect should just avoid the connection attempts *when the service starts*. If we are started and we are up and running, DisableAutoConnect should not kick in. Another PR will follow about this topic * Removes unused vars * Moves callback into Run method arg * align comment to removal of DisableAutoConnect DisableAutoConnect should just avoid the connection attempts *when the service starts*. If we are started and we are up and running, DisableAutoConnect should not kick in * Removes unused managed_fields data. This was initially used to drive the UI but approach changed to reload config/features upon notifications which makes this data redundant. * Reorder stuff * Unexport unrequired vars/functions PoliciesEqual → policiesEqual AllKeys → allKeys * Adds list of MDM managed fields in the debug bundle |
||
|
Theodor Midtlien
|
512899d82d |
[client] Prevent corruption from competing log rotation and improve debug bundle (#6214)
* Adds heuristic to detect an edge case on Linux where a system has configured logrotate as a separate service to rotate log files which would mangle our client log files. If we detect logrotate being configured for netbird, we disable our rotation. * Adds new env var to disable log rotation: NB_LOG_DISABLE_ROTATION * Adds compressed and plain logrotate files to debug bundle. * Replaces lumberjack with timberjack (maintained fork with bug fixes and extra features). * Clarifies which daemon version is running in the bundle stats. * Change logging for client service status to console |
||
|
Bethuel Mmbaga
|
14af179556 | [management] Refactor management server bootstrap (#6256) | ||
|
Viktor Liu
|
1224d6e1ee | [client] Persist management URL and pre-shared key overrides on login (#6065) | ||
|
Viktor Liu
|
205ebcfda2 | [management, client] Add IPv6 overlay support (#5631) | ||
|
Viktor Liu
|
057d651d2e | [client, proxy] Add packet capture to debug bundle and CLI (#5891) | ||
|
Bethuel Mmbaga
|
df197d5001 | [management] Prevent JWT reuse during peer login (#6002) | ||
|
Viktor Liu
|
407e9d304b | [client] Move macOS sleep detection into the daemon (purego) (#5926) | ||
|
Viktor Liu
|
2e0e3a3601 | [client] Replace exclusion routes with scoped default + IP_BOUND_IF on macOS (#5918) | ||
|
Maycon Santos
|
53b04e512a |
[management] Reuse a single cache store across all management server consumers (#5889)
* Add support for legacy IDP cache environment variable * Centralize cache store creation to reuse a single Redis connection pool Each cache consumer (IDP cache, token store, PKCE store, secrets manager, EDR validator) was independently calling NewStore, creating separate Redis clients with their own connection pools — up to 1400 potential connections from a single management server process. Introduce a shared CacheStore() singleton on BaseServer that creates one store at boot and injects it into all consumers. Consumer constructors now receive a store.StoreInterface instead of creating their own. For Redis mode, all consumers share one connection pool (1000 max conns). For in-memory mode, all consumers share one GoCache instance. * Update management-integrations module to latest version * sync go.sum * Export `GetAddrFromEnv` to allow reuse across packages * Update management-integrations module version in go.mod and go.sum * Update management-integrations module version in go.mod and go.sum |
||
|
Viktor Liu
|
6b540d145c | [client] Add --disable-networks flag to block network selection (#5896) | ||
|
Viktor Liu
|
94a36cb53e | [client] Handle UPnP routers that only support permanent leases (#5826) | ||
|
Viktor Liu
|
d33cd4c95b | [client] Add NAT-PMP/UPnP support (#5202) | ||
|
Viktor Liu
|
aba5d6f0d2 | [client] Error out on netbird expose when block inbound is enabled (#5818) | ||
|
Zoltan PappandViktor Liu
|
91f0d5cefd |
[client] Feature/client metrics (#5512)
* Add client metrics * Add client metrics system with OpenTelemetry and VictoriaMetrics support Implements a comprehensive client metrics system to track peer connection stages and performance. The system supports multiple backend implementations (OpenTelemetry, VictoriaMetrics, and no-op) and tracks detailed connection stage durations from creation through WireGuard handshake. Key changes: - Add metrics package with pluggable backend implementations - Implement OpenTelemetry metrics backend - Implement VictoriaMetrics metrics backend - Add no-op metrics implementation for disabled state - Track connection stages: creation, semaphore, signaling, connection ready, and WireGuard handshake - Move WireGuard watcher functionality to conn.go - Refactor engine to integrate metrics tracking - Add metrics export endpoint in debug server * Add signaling metrics tracking for initial and reconnection attempts * Reset connection stage timestamps during reconnections to exclude unnecessary metrics tracking * Delete otel lib from client * Update unit tests * Invoke callback on handshake success in WireGuard watcher * Add Netbird version tracking to client metrics Integrate Netbird version into VictoriaMetrics backend and metrics labels. Update `ClientMetrics` constructor and metric name formatting to include version information. * Add sync duration tracking to client metrics Introduce `RecordSyncDuration` for measuring sync message processing time. Update all metrics implementations (VictoriaMetrics, no-op) to support the new method. Refactor `ClientMetrics` to use `AgentInfo` for static agent data. * Remove no-op metrics implementation and simplify ClientMetrics constructor Eliminate unused `noopMetrics` and refactor `ClientMetrics` to always use the VictoriaMetrics implementation. Update associated logic to reflect these changes. * Add total duration tracking for connection attempts Calculate total duration for both initial connections and reconnections, accounting for different timestamp scenarios. Update `Export` method to include Prometheus HELP comments. * Add metrics push support to VictoriaMetrics integration * [client] anchor connection metrics to first signal received * Remove creation_to_semaphore connection stage metric The semaphore queuing stage (Created → SemaphoreAcquired) is no longer tracked. Connection metrics now start from SignalingReceived. Updated docs and Grafana dashboard accordingly. * [client] Add remote push config for metrics with version-based eligibility Introduce remoteconfig.Manager that fetches a remote JSON config to control metrics push interval and restrict pushing to a specific agent version range. When NB_METRICS_INTERVAL is set, remote config is bypassed entirely for local override. * [client] Add WASM-compatible NewClientMetrics implementation Replace NewClientMetrics in metrics.go with a WASM-specific stub in metrics_js.go, returning nil for compatibility with JS builds. Simplify method usage for WASM targets. * Add missing file * Update default case in DeploymentType.String to return "unknown" instead of "selfhosted" * [client] Rework metrics to use timestamped samples instead of histograms Replace cumulative Prometheus histograms with timestamped point-in-time samples that are pushed once and cleared. This fixes metrics for sparse events (connections/syncs that happen once at startup) where rate() and increase() produced incorrect or empty results. Changes: - Switch from VictoriaMetrics histogram library to raw Prometheus text format with explicit millisecond timestamps - Reset samples after successful push (no resending stale data) - Rename connection_to_handshake → connection_to_wg_handshake - Add netbird_peer_connection_count metric for ICE vs Relay tracking - Simplify dashboard: point-based scatter plots, donut pie chart - Add maxStalenessInterval=1m to VictoriaMetrics to prevent forward-fill - Fix deployment_type Unknown returning "selfhosted" instead of "unknown" - Fix inverted shouldPush condition in push.go * [client] Add InfluxDB metrics backend alongside VictoriaMetrics Add influxdb.go with timestamped line protocol export for sparse one-shot events. Restore victoria.go to use proper Prometheus histograms. Update Grafana dashboards, add InfluxDB datasource, and update docs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * [client] Fix metrics issues and update dev docker setup - Fix StopPush not clearing push state, preventing restart - Fix race condition reading currentConnPriority without lock in recordConnectionMetrics - Fix stale comment referencing old metrics server URL - Update docker-compose for InfluxDB: add scoped tokens, .env config, init scripts - Rename docker-compose.victoria.yml to docker-compose.yml * [client] Add anonymised peer tracking to pushed metrics Introduce peer_id and connection_pair_id tags to InfluxDB metrics. Public keys are hashed (truncated SHA-256) for anonymisation. The connection pair ID is deterministic regardless of which side computes it, enabling deduplication of reconnections in the ICE vs Relay dashboard. Also pin Grafana to v11.6.0 for file-based provisioning and fix datasource UID references. * Remove unused dependencies from go.mod and go.sum * Refactor InfluxDB ingest pipeline: extract validation logic - Move line validation logic to `validateLine` and `validateField` helper functions. - Improve error handling with structured validation and clearer separation of concerns. - Add stderr redirection for error messages in `create-tokens.sh`. * Set non-root user in Dockerfile for Ingest service * Fix Windows CI: command line too long * Remove Victoria metrics * Add hashed peer ID as Authorization header in metrics push * Revert influxdb in docker compose * Enable gzip compression and authorization validation for metrics push and ingest * Reducate code of complexity * Update debug documentation to include metrics.txt description * Increase `maxBodySize` limit to 50 MB and update gzip reader wrapping logic * Refactor deployment type detection to use URL parsing for improved accuracy * Update readme * Throttle remote config retries on fetch failure * Preserve first WG handshake timestamp, ignore rekeys * Skip adding empty metrics.txt to debug bundle in debug mode * Update default metrics server URL to https://ingest.netbird.io * Atomic metrics export-and-reset to prevent sample loss between Export and Reset calls * Fix doc * Refactor Push configuration to improve clarity and enforce minimum push interval * Remove `minPushInterval` and update push interval validation logic * Revert ExportAndReset, it is acceptable data loss * Fix metrics review issues: rename env var, remove stale infra, add tests - Rename NB_METRICS_ENABLED to NB_METRICS_PUSH_ENABLED to clarify that collection is always active (for debug bundles) and only push is opt-in - Change default config URL from staging to production (ingest.netbird.io) - Delete broken Prometheus dashboard (used non-existent metric names) - Delete unused VictoriaMetrics datasource config - Replace committed .env with .env.example containing placeholder values - Wire Grafana admin credentials through env vars in docker-compose - Make metricsStages a pointer to prevent reset-vs-write race on reconnect - Fix typed-nil interface in debug bundle path (GetClientMetrics) - Use deterministic field order in InfluxDB Export (sorted keys) - Replace Authorization header with X-Peer-ID for metrics push - Fix ingest server timeout to use time.Second instead of float - Fix gzip double-close, stale comments, trim log levels - Add tests for influxdb.go and MetricsStages * Add login duration metric, ingest tag validation, and duration bounds - Add netbird_login measurement recording login/auth duration to management server, with success/failure result tag - Validate InfluxDB tags against per-measurement allowlists in ingest server to prevent arbitrary tag injection - Cap all duration fields (*_seconds) at 300s instead of only total_seconds - Add ingest server tests for tag/field validation, bounds, and auth * Add arch tag to all metrics * Fix Grafana dashboard: add arch to drop columns, add login panels * Validate NB_METRICS_SERVER_URL is an absolute HTTP(S) URL * Address review comments: fix README wording, update stale comments * Clarify env var precedence does not bypass remote config eligibility * Remove accidentally committed pprof files --------- Co-authored-by: Viktor Liu <viktor@netbird.io> |
||
|
Viktor Liu
|
3e6baea405 | [management,proxy,client] Add L4 capabilities (TLS/TCP/UDP) (#5530) | ||
|
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 |
||
|
Maycon Santos
|
15aa6bae1b |
[client] Fix exit node menu not refreshing on Windows (#5553)
* [client] Fix exit node menu not refreshing on Windows TrayOpenedCh is not implemented in the systray library on Windows, so exit nodes were never refreshed after the initial connect. Combined with the management sync not having populated routes yet when the Connected status fires, this caused the exit node menu to remain empty permanently after disconnect/reconnect cycles. Add a background poller on Windows that refreshes exit nodes while connected, with fast initial polling to catch routes from management sync followed by a steady 10s interval. On macOS/Linux, TrayOpenedCh continues to handle refreshes on each tray open. Also fix a data race on connectClient assignment in the server's connect() method and add nil checks in CleanState/DeleteState to prevent panics when connectClient is nil. * Remove unused exitNodeIDs * Remove unused exitNodeState struct |
||
|
Zoltan Papp
|
3acd86e346 |
[client] "reset connection" error on wake from sleep (#5522)
Capture engine reference before actCancel() in cleanupConnection(). After actCancel(), the connectWithRetryRuns goroutine sets engine to nil, causing connectClient.Stop() to skip shutdown. This allows the goroutine to set ErrResetConnection on the shared state after Down() clears it, causing the next Up() to fail. |
||
|
Zoltan Papp
|
c2c4d9d336 |
[client] Fix Server mutex held across waitForUp in Up() (#5460)
Up() acquired s.mutex with a deferred unlock, then called waitForUp() while still holding the lock. waitForUp() blocks for up to 50 seconds waiting on clientRunningChan/clientGiveUpChan, starving all concurrent gRPC calls that require the same mutex (Status, ListProfiles, etc.). Replace the deferred unlock with explicit s.mutex.Unlock() on every early-return path and immediately before waitForUp(), matching the pattern already used by the clientRunning==true branch. |
||
|
Maycon Santos
|
63c83aa8d2 |
[client,management] Feature/client service expose (#5411)
CLI: new expose command to publish a local port with flags for PIN, password, user groups, custom domain, name prefix and protocol (HTTP default). Management/API: create/renew/stop expose sessions (streamed status), automatic naming/domain, TTL renewals, background expiration, new management RPCs and client methods. UI/API: account settings now include peer_expose_enabled and peer_expose_groups; new activity codes for peer expose events. |
||
|
Zoltan Papp
|
37f025c966 |
Fix a race condition where a concurrent user-issued Up or Down command (#5418)
could interleave with a sleep/wake event causing out-of-order state transitions. The mutex now covers the full duration of each handler including the status check, the Up/Down call, and the flag update. Note: if Up or Down commands are triggered in parallel with sleep/wake events, the overall ordering of up/down/sleep/wake operations is still not guaranteed beyond what the mutex provides within the handler itself. |
