mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-12 17:59:06 +02:00
682b2de549c1844a4e9b3edf19b9730eb7053a51
3312
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 riccardom
|
682b2de549 |
[client] Fail netbird up when the daemon refuses the settings update
With the update-settings kill switch on, `netbird up --enable-rosenpass` connected and said almost nothing: SetConfig refused the change, the CLI downgraded that to a warning, and Login carries no rosenpass field to apply, so the flag was silently dropped. The setting stayed disabled, which is the point of the switch, but the caller was never told their request had been ignored. The refusal now travels as codes.FailedPrecondition instead of codes.Unavailable, and the CLI fails on it. Unavailable means "the daemon cannot serve this call", which is why the CLI downgraded it and why client/ui/services reads it as an unreachable daemon — both wrong for a daemon that answered and refused. FailedPrecondition also matches what the MDM gate already returns for a managed field, so both refusals are now one class of error, and it is added to the login backoff's early-exit codes so a refused login stops instead of retrying for 30s. This does not put the container back in the deadlock: with the value-aware gate, a client restating its own configuration is not refused at all, so nothing reaches this path unless a real change was asked for. |
||
|
riccardom
|
1d213dd4d4 |
[client] Treat a profile with no identity as already deregistered
Two findings on the same consequence of pure reads: a profile can legitimately carry no keys, because logging out clears them in place. - sendLogoutRequestWithConfig went straight to wgtypes.ParseKey and failed with "incorrect key size: 0" on the second logout of the same profile. There is nothing to deregister for a peer that was never registered, so it returns cleanly. Before pure reads this case was hidden: the read minted a key and the daemon dialed management with one it had never seen. - The mobile logout read the config with the generating reader right after checking the file exists. The two are not atomic, so a profile removed in between was resolved from the defaults and recreated by the write that follows. It uses the existing-file reader now. Reported by cubic-dev-ai and CodeRabbit on PR #7398. |
||
|
riccardom
|
29d03356fc |
[client] Keep the admin panel path part of its identity
The endpoint comparison introduced for the management URL was applied to the admin URL too, and that one is opened in a browser rather than dialed over gRPC: a panel served under /netbird is not the panel served at the root. So a config whose admin URL differed only by path reported no change, and the new path was never persisted — a custom panel URL could not be updated at all. SameServiceURLIncludingPath adds what a URL carries past its endpoint (path, query, fragment, userinfo) while still treating equivalent spellings as equal: a missing path and "/" are the same root, and so is a trailing slash. The management URL keeps the endpoint-only comparison, since only the endpoint is ever dialed. Ports are also normalized numerically now, so ":0443" and ":443" are one port. Reported by cubic-dev-ai on PR #7398 (two findings). |
||
|
riccardom
|
bc49b7249c |
[client] Stop the gate test from dialing the real management server
TestLogin_RestatingTheStoredConfigPassesTheGate asserts that the gate lets a no-op login through, and the handler then went on to do the login for real: isLoginRequired builds an auth client when isLoginRequiredFn is unset, so the test dialed the profile's management URL — api.netbird.io:443. It took 1.05s locally and would hang on a runner with no egress, for a fact about the gate that needs no network at all. Stubbed like the login_outcome tests do. The test now runs in 0.00s. Reported by cubic-dev-ai on PR #7398. |
||
|
riccardom
|
e027ba11f9 |
[client] Keep the peer identity out of a read that finds no file
ReadOrGenerateConfig resolves a default config when the profile has no file yet, and createNewConfig was minting the WireGuard and SSH keys while doing so. That defeated the provisioning pair it was meant to serve: the CLI's foreground login calls EnsureIdentity to find out whether it has to persist the keys, got generated == false because the read had already generated them, and so never wrote them out. The login then dialed management with an identity that only existed in memory, and the next login registered a second peer. createNewConfig no longer provisions. createProvisionedConfig is the variant that does, and the callers whose contract is "usable as it comes back" use it: CreateInMemoryConfig, whose callers connect with the result, and the two create-and-write branches. A read gets a config with no identity, so the caller's own EnsureIdentity reports the work and triggers the write. Reported by CodeRabbit and cubic-dev-ai on PR #7398, both on the same defect. |
||
|
riccardom
|
294fa0bcfd |
[client] Address the remaining bot findings on PR #7398
- Login logged the active-profile-state error and returned the same cause; the repo's guidelines call for one or the other, and the wrapped error is the one that carries context. (CodeRabbit) - `netbird up` reported a codes.Unavailable SetConfig failure as "the daemon refused the settings update", but that code also covers a daemon that became unreachable. It now reports what the daemon said without asserting why. (cubic-dev-ai) - TestLogin_ChangingTheManagementURLIsRefused asserted the error and nothing else, while "refused before it can touch daemon state" is the contract. It now checks the stored management URL, the in-progress login and the active profile, matching its SetConfig counterpart. (cubic-dev-ai) |
||
|
riccardom
|
c660fcaaac |
[client] Compare the client certificate paths before reporting a change
apply() assigned the incoming mTLS certificate and key paths and set updated unconditionally, without comparing them to what the config already held. It is the same presence-instead-of-value mistake this branch set out to fix, one layer down: a caller restating its own certificate paths was reported as changing them, which trips the value-aware update-settings gate. Reported by cubic-dev-ai on PR #7398. |
||
|
riccardom
|
911705e1c6 |
[client] Do not panic on a config with no sync message version
apply() wrote the incoming sync message version through the stored pointer, without checking it was there: a config that carries no version yet made it dereference nil. Reachable from the update-settings dry run, which runs inside a request handler — where failing closed is the worst acceptable outcome, and a panic is not one. The field is now reassigned like every other optional one, which also means apply() no longer mutates anything the caller still holds through a pointer, so the dry run's copy has one less field to detach. Reported by cubic-dev-ai on PR #7398. |
||
|
riccardom
|
844bf24a6f |
[client] Name the two config readers for what they do
ReadConfig and GetConfig differed in one thing — what happens when the file is absent — and neither name said which was which: - ReadConfig -> ReadOrGenerateConfig (reads it, or generates one in memory) - GetConfig -> GetExistingConfig (reads it, or fails) Three comments went with them: - GetConfig's said "return with Config and if it was created. Errors out if it does not exist", which described a bool it does not return and a creation it never performs. - ReadConfig's explained that it does not write, which is what a reader is supposed to do anyway. - Server.getConfig's said it "errors out if it does not exist", which it does not — it resolves a default config, and now provisions the identity too. |
||
|
riccardom
|
7a1f095eb5 |
[client] Make config reads pure and provision the identity explicitly
Reading a config wrote it back. profilemanager.readConfig persisted whatever apply() had filled in, and ReadConfig created and wrote the file outright when it was absent, so every reader was quietly a writer: a gate deciding whether to refuse a request, a UI listing profiles, a mobile getter reading one preference. The previous commit worked around that with a PeekConfig variant, which left two read functions with opposite side effects and the antipattern still there for everyone else. Only one thing in a read genuinely had to be persisted: apply() generated the WireGuard and SSH keys when it found them empty, and a generated key cannot be recomputed — losing it means the peer comes back with a different identity and registers again. Everything else apply() fills in is a deterministic default that the next read recomputes anyway. So identity provisioning is now its own step, Config.EnsureIdentity, and the callers that provision write the result out themselves, in the open: - Server.getConfig, the daemon's provisioning point; - the CLI's foreground login, which is about to dial management; - update() / directUpdate(), the config write paths — a stored profile can legitimately carry no identity, since a mobile logout clears the keys in place, and the next write is what has to mint a new one. ReadConfig and GetConfig no longer write anything, PeekConfig is gone, and the dry-run baseline no longer needs placeholder keys to keep apply() from minting real ones. One deliberate leftover: readConfig still calls util.EnforcePermission, which chmods a config file whose permissions are too broad. It changes no content and is idempotent, and dropping it would leave a legacy file world-readable until its first write. |
||
|
riccardom
|
7dbd5f8f56 |
[client] Drop an unreachable guard and fix two stale comments
- loginOverridesInput's nil-message guard cannot be reached: Login dereferences the message well before it, in storedLoginConfig. - The docstring above afterLoginPreCheck described persistLoginOverrides, which lives further down the file and now carries its own. - UpdateConfig's comment named DirectUpdateConfig; the function is DirectUpdateOrCreateConfig. |
||
|
riccardom
|
ee9a5c2e20 |
[client] Re-take the update-settings decision under the config lock
Login checks twice on purpose: the first check refuses the ordinary case early, and authorizeAndPrepareLogin re-takes the authoritative one under guardedConfigMu because the first is unsynchronized against a concurrent privileged request. The update-settings decision is now equally value-dependent — it compares the request against the stored config — but it was taken only in the first, unlocked check. So a login that was a no-op when it was checked could be written after a concurrent writer had repointed the profile, which is exactly the window the lock exists to close. The decision is now re-taken alongside the privilege one, which also makes it the last read before persistLoginOverrides writes. The test drives that interleaving through the existing afterLoginPreCheck seam and fails without the re-check. |
||
|
riccardom
|
ec30004241 |
[client] Cover the login the update-settings gate used to refuse
The gate's decision procedure was tested directly, but no test drove the Login RPC that the refusal actually broke: the CLI retries Login in a backoff loop, so a refused no-op login is what kept a client configured by environment from ever coming up. The handler-level coverage stopped at the refusal case, which passes on the pre-fix code too. This test fails on the pre-fix daemon with "update settings are disabled" and passes now. Past the gate the handler does real work the test does not stand up, so it asserts only that the refusal did not happen. |
||
|
riccardom
|
ea972f7847 |
[client] Stop the config dry run from generating throwaway keys
The dry run's baseline for a profile with no config file yet went through createNewConfig, and apply() generates a WireGuard and an SSH key whenever it finds those fields empty. The baseline is compared against and discarded, so every evaluation minted a keypair it threw away — and logged "generated new Wireguard key". The CLI retries Login in a backoff loop, so a first `netbird up` on a fresh profile filled the daemon log with what reads like peer-key rotation. The baseline now starts from the shared skeleton with placeholder keys, so apply() has nothing to generate. No ConfigInput field maps to either key, so the comparison is unaffected. |
||
|
riccardom
|
2a17bf0d55 |
[client] Compare service URLs as endpoints, not as strings
Three places in one request path each had their own notion of "same management URL": the config layer compared the parsed URLs as strings, the privileged-change gate compared scheme + host + effective port, and the MDM conflict check compared strings after filling in the default port. Only the middle one was right. A string comparison answers the wrong question. "https://api.netbird.io", "https://api.netbird.io/" and "https://API.netbird.io:443" are one endpoint written three ways, so a client restating its own management URL with a trailing slash — a normal way to write it — was still read as a client asking to be repointed, and the update-settings gate refused it. The MDM check had the same flaw against the enforced value. profilemanager.SameServiceURL is now the single comparison: same scheme, same host case-insensitively as DNS names are, same effective port. The config layer, the privileged-change gate and the MDM conflict check all defer to it, so there is one answer to "did this URL change?" instead of three. |
||
|
riccardom
|
0b969e2124 |
[client] Do not write the profile config while only reading it to decide
The update-settings gate needs the stored config to decide whether a request changes anything, so the previous commit moved that read ahead of the refusal. The read is not side-effect free: profilemanager.GetConfig writes the config back whenever apply() has to fill in a default the file was missing. A request that the gate then refuses had therefore already rewritten the profile file. PeekConfig is GetConfig without that write-back. The returned config is still normalized in memory, which is what the decision needs; the file is left exactly as it was found. Every caller of storedConfigAtPath feeds a gate that can refuse, so they all peek. Note for reviewers: the daemon still normalizes the file on startup and on every real update, so nothing depends on a read performing that write. |
||
|
riccardom
|
ef88c4de5f |
[client] Gate settings updates on value, not on field presence
The update-settings kill switch (--disable-update-settings /
NB_DISABLE_UPDATE_SETTINGS / the MDM DisableUpdateSettings key) forbids
changing settings, but it decided what a "change" was by looking at
whether a field was present in the request. The CLI fills the whole
config surface of SetConfigRequest and LoginRequest from its flags and
environment on every `netbird up` (setupSetConfigReq in cmd/up.go), so a
client configured by environment restates its own configuration on every
start and tripped the gate every time.
SetConfig only warned about that, but Login carries the same fields and
was gated the same way, and Login runs inside the CLI's backoff loop: the
daemon answered every attempt with codes.Unavailable, `netbird up` never
completed, and a container with NB_DISABLE_UPDATE_SETTINGS plus any
config env var (NB_MANAGEMENT_URL, for one) could not come up at all.
Both gates now compare values. Config.WouldChange is the dry-run half of
UpdateConfig: it runs the very same diff logic (Config.apply) against a
copy of the stored config, so the gate cannot drift from what an actual
update would do, nor go stale when a field is added. A request that
restates what the profile already holds changes nothing and is allowed; a
request that diverges is refused exactly as before, and a dry run that
cannot be evaluated fails closed. A profile with no config on disk yet is
judged against the config the daemon would create for it.
For Login, the compared input comes from loginOverridesInput, which
persistLoginOverrides also uses to perform the write, so the gate judges
precisely the two fields a login can persist (management URL, pre-shared
key) and no field it ignores.
Two adjacent defects surfaced while making the comparison exact:
- Config.apply compared URLs as raw strings, so the same endpoint spelled
without its default port ("https://api.netbird.io" vs
"https://api.netbird.io:443") counted as a new value and rewrote the
config. It now compares the parsed forms.
- UpdateConfig did not collapse the redacted pre-shared key, unlike
UpdateOrCreateConfig and DirectUpdateConfig, so a UI round-trip of the
mask replaced the stored key with asterisks.
The CLI warning for a refused SetConfig said the method was not available
in the daemon, which sent people looking for a version mismatch that was
not there; it now reports the refusal.
|
||
|
Zoltan Papp
|
c3cf7c0c37 |
[client] Clarify that metrics ingest X-Peer-ID is not a credential (#7363)
* [client] Clarify that metrics ingest X-Peer-ID is not a credential The ingest endpoint is intentionally unauthenticated: it accepts telemetry from peers of both cloud and self-hosted deployments, and for a self-hosted peer there is no shared trust anchor to authenticate against. The X-Peer-ID header is a correlation tag whose format check exists to bound InfluxDB tag cardinality. Both the function name (validateAuth) and the 401 response implied an authentication control that was never there, which invites the reading that the check can be bypassed. Rename it to validatePeerIDFormat and return 400, matching the other input validation failures in the same handler. Document the intent in the godoc and the infra README. No behavioural change for clients: push.go classifies responses by 2xx range rather than by status code, so 400 and 401 are handled identically. * [client] Reject metrics ingest bodies whose peer_id tag disagrees with the header validateTag checked tag names against the per-measurement allowlist but only bounded the value length, so the peer_id tag was free-form text up to 64 bytes and could differ from the X-Peer-ID header the request was accepted with. Tie the two together: the tag value must equal the header value. Since the header is already checked to be 16 hex characters, this transitively constrains the tag to the same shape. Every client sends the same value in both places (metrics.go feeds agentInfo.peerID to both push.SetPeerID and the body tags), so well-behaved clients are unaffected. The mismatch is rejected rather than silently overwritten: rewriting the value would re-serialize caller-controlled text back into line protocol and would hide misbehaving senders instead of surfacing them. Rejection also matches the other input validation failures in the same handler, which all return 400. This narrows the value space of the peer_id tag but does not by itself bound InfluxDB series cardinality: a sender that puts the same arbitrary 16 hex characters in both the header and the body still passes. Limiting that needs a per-source rate limit in front of the service. * [client] Document what the metrics ingest peer_id check does and does not bound The README described X-Peer-ID as the correlation tag, but grouping is done by the peer_id tag in the submitted line protocol: that is what is forwarded to InfluxDB, while the header only serves as the value each tag is checked against. It also claimed the format check bounds tag cardinality. It bounds the value space of the tag, not the number of distinct series, so state that explicitly and point out that series cardinality has to be limited outside this service. * [client] Set timeouts on the metrics ingest HTTP server The server ran on http.ListenAndServe with no timeouts, silenced with a nolint:gosec for G114. Without ReadHeaderTimeout a client can hold a connection open by sending headers slowly, and without ReadTimeout or IdleTimeout connections accumulate on an endpoint that takes unauthenticated requests. Construct an http.Server with explicit limits instead, which also drops the nolint. Handler stays nil so the existing DefaultServeMux registrations are unaffected. WriteTimeout is deliberately larger than the 10s upstream client timeout: the response is only written after the forward to InfluxDB completes, so a tighter value would cut off the server's own valid response. * Revert "[client] Document what the metrics ingest peer_id check does and does not bound" This reverts commit |
||
|
Zoltan Papp
|
ecbeba8e67 |
[management] Fix geolocation panics (#7382)
* [management] Return errors instead of panicking on malformed geolocation inputs * [management] Reject empty date suffix in geolocation database filename |
||
|
eYey
|
2f55965031 |
[client, android] Type the split tunnelling mode instead of storing a string (#7387)
gomobile carries only basic types, so the typed constants stay unexported and the exported SplitTunnelMode* ints are what the Java side gets. |
||
|
Maycon Santos
|
a1415dbc05 |
[client] Fix the ICEBind races that wedge interface creation (#7377)
* [client] Add tests for the ICEBind open and close races Running many embedded clients in one process intermittently wedges interface creation. A goroutine dump taken from 50 clients shows ten of them parked for seven minutes in Device.IpcSet, in closeBindLocked waiting on device.net.stopping.Wait, holding device.net while every other device goroutine queues behind it on Device.Up. Open writes s.closed and Close reads it with no synchronisation, and Close also closes s.closedChan without the mutex that Open swaps it under. Two Closes can both pass the check and close the same channel, and a Close racing an Open can mark the bind closed while a live channel and live receive functions remain, after which every later Close takes its early return and runs neither close(closedChan) nor StdNetBind.Close. The receive functions never stop, so stopping.Wait never returns. These tests do not fix that. The first pins the contract closeBindLocked depends on and passes today. The other two fail under -race, reporting the races at the three sites above, and pass again once closed and closedChan are guarded consistently. * [client] Release parked receivers so reopening a bind cannot stall receiveRelayed held closedChanMu for the whole of its blocking select, so a parked receiver kept the read lock indefinitely and Open could never take the write lock it needs to install a fresh closedChan. wireguard-go reaches Open from Device.IpcSet and Device.Up with device.net held, so the stall took the device lock with it: interface creation never finished, every other device goroutine queued behind Device.Up, and Engine.Start never returned. Callers now copy the channel under a short read lock and select on the copy. Copying alone would stand a new trap in the same place, because an Open that follows an Open leaves the previous generation parked on a channel no later Close can reach, so Open now closes the outgoing channel before swapping it. closed and closedChan are also updated together under that mutex. Read and written apart, Close could see a stale closed and skip both close(closedChan) and StdNetBind.Close, leaving every receive function running and wedging closeBindLocked on device.net.stopping.Wait, or two Close calls could pass the check together and close the same channel twice. TestICEBindOpenDoesNotBlockOnParkedReceiver fails without this change, without needing the race detector. The other three cover the surrounding contract and report the state races under -race. * [client] Make the bind lifecycle transition atomic and tighten its tests Review caught that the previous commit moved the torn transition rather than removing it. Open published the new generation before calling StdNetBind.Open, so an Open rejected because the bind was already open had already signalled the outgoing generation, and a Close arriving in that window could mark the bind closed while the same call went on to install live sockets. Every later Close then returned early and never shut them down. Open now calls StdNetBind.Open first, so a failure leaves the current generation untouched, and both Open and Close hold the lock across the whole transition. Ordering is safe: StdNetBind.Open reaches muUDPMux through createReceiverFn, and no path takes muUDPMux before closedChanMu. The tests were also weaker than they read. The stress test claimed to cover a stale channel but only ever raced two Closes, and the concurrency test left overlap to goroutine start order. Both now gate their goroutines on a common start, the stress test races an Open against the Closes, and both assert the surviving generation channel is actually closed. Waiting on receive functions to be entered replaces part of the sleep in the reopen probe, and teardown bounds its Close so a regression fails the assertion instead of hanging. Two of the four now fail without the fix and no race detector, the stress test by reproducing close of a closed channel at the Close early return. * [client] Fail the reopen probe when its teardown does not complete closeBounded swallowed its timeout and the cleanup discarded what receiversStopped returned, so the bounds added in the previous commit only stopped teardown hanging. A wedged Close or a parked receiver would have left the test green with a leaked goroutine, which is the failure this test exists to catch. closeBounded now reports whether Close returned, and cleanup fails the test on either bound. |
||
|
Pascal Fischer
|
e3d6c3d0eb | [management] fix private services calc on new db path (#7383) | ||
|
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. |
||
|
Maycon Santos
|
3027130f0f |
[management] Add Agent Network access roles and self-service endpoints (#7221)
Delegating Agent Network today means handing out full account admin, and regular users cannot see their own usage or how to connect a local tool. Add two roles on top of the existing agent_network permission submodules. agent_network_admin owns the whole area (providers, policies, guardrails, budgets, usage, logs, settings) with read-only users, groups, peers, and account info needed to build policies, and nothing else in the account. usage_viewer is the regular User baseline plus read on the aggregated usage and cost overview: no provider configuration, no policies, no request-level logs, which can contain captured prompts. billing_admin gets a proper permission-map entry with the User baseline so role resolution stops failing with role-not-found; its plan and invoice permissions stay enforced cloud-side. Add the self-service endpoints behind the "My Agent Network" view, available to every authenticated user because both answers are scoped strictly to the caller. GET /api/agent-network/me/setup returns the account endpoint plus the providers and models the caller's own groups authorize, computed with the same rules the proxy enforces: policy filtering as in policy selection, model allowlist union intersected with declared models, orphan and disabled providers omitted. Not set up and no access are deliberately indistinguishable, and the response carries display metadata only. GET /api/agent-network/me/consumption returns the caller's own user-dimension counters. |
||
|
evgeniyChepelevandClaude Opus 5
|
652d5f3c15 |
[client] Reuse the profile's account for iOS SSO logins (#7193)
* [client] Reuse the profile's account for iOS SSO logins Android reads the profile's stored account and passes it as the OIDC login_hint, and records it again after a successful login. iOS did neither: it called GetOAuthFlow with an empty hint, so a re-login was resolved by whatever session the browser's cookie jar held rather than by the account the profile belongs to. With a non-ephemeral browser session that is the wrong account as soon as more than one is signed in. Mirror client/android/login.go: hint from mobile.ReadProfileEmail before the flow, mobile.WriteProfileEmail after Login succeeds. Storing after Login and not before keeps a rejected token from leaving a hint that points at an account which cannot be used. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * [client] Persist the account email on tvOS and on the device flow Two paths left a profile with no account bound, so every later login went out without a login_hint — the case this change exists to remove. WriteProfileEmail went through util.WriteJsonWithRestrictedPermission, which writes a temp file and renames it over the target. The tvOS App Group sandbox blocks exactly that, which is why the config sitting next to this file is written with DirectWriteOutConfig. On tvOS the email write therefore failed and was dropped with a warning. Use DirectWriteJson: the file is rewritten whole from a single key, so the only thing atomicity buys here is surviving a crash mid-write, and a torn file reads back as "no email" and is replaced by the next login. The device authorization flow never populated TokenInfo.Email, unlike the PKCE flow, so a client driven through it — Android TV and tvOS — bound no account at all. Parse the ID token there too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * [client] Report a failed close from DirectWriteJson The deferred close assigned its error to err, but the return value was not named, so the assignment went nowhere: a close that failed was logged and the function still returned nil. The write is only durable once the file closes cleanly, so every caller — the management config, the profile configs and the profile account email — could be told the data landed when it had not. Name the return so the assignment does what its shape always intended, and report the failure once. When the body succeeded the close error is returned and the caller logs it. When the body already failed, that error is the one that explains the failure and is what the caller gets, which leaves the deferred log as the only place the close failure can surface — at debug, per the logging rules for close errors on writes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Daneyon Hansen
|
7a9582db16 |
[management,proxy] Add agentgateway integration (#7274)
* [management] Add agentgateway provider catalog entry Allow Agent Network providers to target an operator-supplied agentgateway proxy while stamping trusted NetBird identity headers. Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io> * [proxy] Allow trusted Agent Network identity headers Permit only the built-in identity injector to replace the two reserved agentgateway attribution headers while keeping them blocked for every other middleware. Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io> * [management,proxy] Add multi-vendor gateway routing Let one Agent Network route declare multiple parser surfaces while preserving the existing singular vendor wire field. Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io> * [management] Update router test for model policies Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io> * [proxy] Cover reserved header policy Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io> * [management] Add agentgateway model discovery Use agentgateway's OpenAI-compatible models endpoint and omit wildcard patterns until NetBird can authorize and price them consistently. Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io> --------- Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io> |
||
|
Zoltan Papp
|
4749005a50 |
[client] Resolve profiles for the sudo invoking user instead of root (#7238)
* [client] Resolve profiles for the sudo invoking user instead of root
The SSH server flags force `netbird up` through sudo, but the CLI resolved
every per-user path with the process user. As root that reads root's own
(empty) local state, so a `sudo netbird up` silently switched the daemon from
the user's profile to the default one — cancelling any login already waiting
in the browser — and then ran an SSO login for the default profile's config.
Whichever account that login returned, the default profile's peer belongs to
someone else, so every attempt ended in "peer is already registered by a
different User or a Setup Key", with nothing telling the user why.
Resolve the acting user through SUDO_USER when running as root: the active
profile, the profile config paths and the stored account email now come from
the invoking user's directories. Privilege decisions are untouched — they stay
on the kernel credentials of the daemon connection, which an environment
variable can never influence; a forged SUDO_USER only selects a profile root
could select anyway.
The invoking user's directories are strictly read-only under sudo. Anything
root wrote there would be root-owned and break the user's own runs, so instead
of chowning files back, the local writes are skipped: the active-profile
bookkeeping and the account-email state simply do not update from a sudo run
(the daemon records the switch on its side; a skipped email write costs at
most one extra account prompt later).
Plain root — no sudo context — has no user to act for, so the ambiguity is
refused instead of guessed at: when the daemon's active profile differs from
what root resolves and no --profile was given, up fails with a message naming
both profiles, instead of silently switching the daemon and failing later with
the ownership error.
* [client] Act on the daemon-resolved profile and fail closed in the root guard
Under sudo the local active-profile mirror is not updated, so up/login
re-reading it after a profile switch acted on the previous profile; use
the daemon-resolved ID directly instead. The plain-root guard now runs
after the readiness wait, denies on lookup errors and empty responses,
and matches the owning username as well; an unowned profile (fresh
install) and a daemon predating the RPC stay allowed. Write-skip
decisions key off the sudo environment alone so a transient user lookup
failure cannot turn a run into writing root-owned files into the user's
directory, and RemoveProfileState honors the read-only rule too.
* [client] Return a wrapped error instead of double-reporting the dial failure
* [client] Read the profile from the daemon when the local mirror is not authoritative
Under sudo without --profile, `up` took the active profile from the invoking
user's local active_profile.txt mirror and drove the daemon to it. But that
mirror is never written under sudo (the SwitchProfile write is a no-op), so it
goes stale after any --profile run and silently switches the daemon back to the
mirror's default. The plain-root guard was meant to refuse exactly this
ambiguity but only ran for plain root, never for the sudo case the fix targets.
When there is no --profile and the mirror is not authoritative (sudo or plain
root), take the profile the daemon already holds for the invoking user instead
of the stale mirror: stay on the user's current profile when the daemon owns it
(or it is unowned, as on a fresh install), and refuse with a --profile hint when
the daemon is on another user's profile. A daemon predating the RPC keeps the
mirror-derived profile.
Reproduce (before this change):
1. As a non-root user misha, with the daemon installed and running:
sudo netbird up --profile work
misha connects on the `work` profile.
2. Because the local mirror write is skipped under sudo,
~misha/.config/netbird/active_profile.txt still says `default` (or is still
absent, which also resolves to `default`).
3. Run a bare:
sudo netbird up
The CLI reads `default` from the frozen mirror and sends ProfileName=default;
the daemon silently switches away from `work` and brings the tunnel up on
`default` — a different account/peer than the one last chosen, with no
warning. After this change step 3 stays on `work`.
* [client] Return a sentinel error instead of nil-nil for the missing daemon RPC
* [client] Load the extend-session hint from the resolved profile
* [client] Fail closed instead of reading root's config when the sudo user lookup fails
* [client] Fail closed in InvokingUser when the sudo user lookup fails
A previous change made baseConfigDir fail closed when SUDO_USER cannot be
resolved, but InvokingUser still fell through to user.Current(). Those two
guards disagreed: the active-profile mirror and the email state refused to
read root's directory, while every profile-path caller happily resolved as
root.
The consequence of a transient NSS failure under sudo was that
Profile.FilePath resolved through getConfigDirForUser("root"), creating
/var/lib/netbird/root and reading the profile JSON from there, and the CLI
sent Username "root" to the daemon in SetConfig and ListProfiles, so the
daemon resolved the same phantom namespace. The invoking user was silently
moved onto a root-owned profile instead of being told the lookup failed.
Fail closed at the single source of the fallback. getConfigDirForUser is
left alone on purpose: it is a pure path helper that also serves
daemon-supplied usernames, and under sudo with a successful lookup it must
still create the invoking user's own profile directory.
|
||
|
Riccardo Manfrin
|
352a1d348a |
[client] Do not log the WireGuard key on a parse failure (#7379)
The error already says what went wrong: an invalid base64 payload reports the offending byte offset, and a wrong key size reports the length. Passing the key itself adds nothing an operator can act on, and the line is emitted at Error level, so it reaches every log sink and every debug bundle. |
||
|
Anton Groshev
|
922be0b8c2 | Fix docs link (#7352) | ||
|
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.
|
||
|
Maycon Santos
|
1081ca006d |
[management,client] Add anonymize level and upload URL to remote debug bundle jobs (#7147)
This extends the management-requested remote debug-bundle job with two new, optional parameters. anonymize_level selects how aggressively the bundle is scrubbed: "default" keeps internal (private) IP ranges readable, while "strict" also anonymizes private, CGNAT and link-local addresses; the value is trimmed and lowercased, and an unknown level is rejected at creation. upload_url lets an operator point the peer at a specific upload service instead of the default one; it must be a well-formed https URL with a host, and an empty value falls back to the default upload server. Both fields flow through the job workload API and are surfaced in the create-debug-job modal on the dashboard. Validation is shared so the client executor and the management boundary agree on what a valid upload URL is, preventing drift between the two checks. |
||
|
Maxim EgorovandRiccardo Manfrin
|
930a25319d |
[client] Keep the route selection on an invalid request and apply it on a partial one (#7292)
* [client] Keep the route selection when every requested ID is unavailable
A non-append SelectRoutes() wipes the current selection before applying
the requested one, but it validated the requested IDs only afterwards,
while already mutating. A request naming no available route at all left
every route deselected and returned an error - so a typo in a route ID
silently dropped the user's exit node, and the routes stayed applied
while the selector claimed nothing was selected.
Validate first and bail out before touching any state when nothing in
the request is available. A request with at least one available route
keeps applying the valid part and reporting the rest, and an empty
request still deselects everything, since that is the caller asking for
exactly that rather than a failed lookup.
* [client] Trim the new comments to the contributing guide's length budget
CONTRIBUTING.md caps comments at 90 characters per line and roughly 250
per comment. The three comments added by this PR were over both limits.
The test comments also restated their own test names, so they lose that
half and keep only the why.
* [client] Apply the route selection even when some IDs are unknown
SelectRoutes and DeselectRoutes returned the error before TriggerSelection,
so a request mixing valid and unknown network IDs changed the selector but
never reached the routing table. The valid routes read as selected while
`ip route` showed nothing.
Trigger the selection first and return the error afterwards. The inner
selectRoutes already applied the valid part of a partial request, only the
outer layer dropped it.
* [client] Publish the network selection event on a partial failure
Returning early on error was correct while an error meant nothing had
happened. A partial failure now changes the selection and the routing
table, so returning first left the change with no trace in the event log
or the UI, even though the new state had already been broadcast.
* [client] Cover the append and deselect-all paths of the selection guard
The append path was never destructive and behaves the same with or without
the early return, so that case is characterization rather than a regression
test. The deselect-all case is a real guard: the early return also skips
resetting deselectAll, so a typo no longer drops the "nothing selected,
including future networks" policy.
* [client] Pin that a fully invalid selection disturbs nothing
The selection is now applied on every request, including one where no ID is
known and the selector is left untouched. Nothing may be torn down or
reinstalled on that path.
* Revert "[client] Publish the network selection event on a partial failure"
This reverts commit
|
||
|
Theodor Midtlien
|
12e8874517 |
[client, relay, management] Bump go version to 1.26 and go-quic to v0.62.0 (#7359)
* Bump go version to 1.26 and go-quic to v0.62.0 * Replace deprecated ecdsa public key assembly and add tests for jwt * Update goversioninfo * Pin go toolchain to 1.26.7 |
||
|
Laotree
|
24959e1ed9 |
[client] Drop agentConnecting whenever ICE session state clears (#7327)
* [client] Drop agentConnecting whenever ICE session state clears Closing a WorkerICE raced a blocked dial goroutine: Close released the agent while connect() was still inside Dial, and the goroutine's own cleanup skipped its flag reset because w.agent no longer matched. With agentConnecting stuck on true, evalConnStatus read the peer as connected, the reconnection guard stopped sending offers and same-session offers were dropped, so the peer could not recover without a restart. An aborted recreate in OnNewOffer reaches the same wedged state without any race. Route every teardown path through one abandonNegotiation helper so the agent and flag fields always clear together; Close now also cleans up residual state left by an aborted recreate. * [client] Drive the ICE teardown race test through the real dial goroutine The regression test simulated the stale goroutine by calling closeAgent directly, so it pinned the symptom rather than the mechanism. Rework it to start a real negotiation, tear it down mid-flight and let the actual goroutine run its own cleanup: with no remote responder the dial can only fail once Close cancels it, so the interleaving stays deterministic without sleeps or injection points. Assert the full idle state that abandonNegotiation owns (agent nil, connecting false, remote session ID empty) instead of only InProgress, and make the stale-cleanup ownership test verify that the newer session survives field by field. * [client] Assert live remote session ID after stale ICE cleanup The stale-cleanup test compared a snapshot captured before closeAgent ran, so clearing the field during cleanup would have gone unnoticed. Read the field under the mutex after the cleanup instead. * [client] Give the ICE race tests a no-op signal client The candidate callback fires from a real gather and dereferences the signaler, so a nil one crashes the test package intermittently when gather wins the race against Close. Build the worker with a stub signal.Client instead. * [client] Read the ICE dial cancel func from an argument in connect The error paths read w.agentDialerCancel without holding muxAgent while OnNewOffer rewrites the field for a newer negotiation, a data race the new teardown test trips under -race. Reading a stale value also let an old goroutine cancel another session's dial. Capture the cancel func at goroutine spawn, like the dial context already is. * [client] Guard the ICE dial success path against stale negotiations The stale-cleanup guard in closeAgent only protected teardown. Its success-path counterpart was missing: an older negotiation could complete agentDial after a newer one replaced w.agent, then clear the newer session's agentConnecting, record lastSuccess and publish its dead connection via onICEConnectionIsReady. Verify ownership under muxAgent twice: right after the dial returns, so a stale goroutine drops its connection before touching a closed agent, and again at the state-commit point, atomic with the agentConnecting and lastSuccess writes, so a replacement arriving in the meantime cannot get its state clobbered. Both paths close the stale connection and return without modifying worker state. A regression test holds session A's dial open until session B is installed, then releases it; the stale connection must be discarded and B's agent, connecting flag and remote session ID must survive. * [client] Fix ICE teardown test leak and document the stale delivery window A code review of the stale-negotiation guard found a leftover resource leak in TestWorkerICE_StaleCloseAgentKeepsCurrentSession: session B is never closed, so its ICE sockets and blocked dial goroutine live as long as the test process. Register t.Cleanup(w.Close). The delivery race flagged after the success-path guard is pre-existing and self-correcting - the newer negotiation overwrites the transient endpoint - so document it in the existing todo instead of locking the callback, which would invert lock order against Conn.Close. Adjust the teardown test comment to match the now-synchronous Close flag clearing. |
||
|
Max
|
7ffbcb0016 | [client] Add Ukrainian localization for desktop client (#7035) | ||
|
Zoltan Papp
|
086d8ba507 |
[client] Close the session-expiration dialog only on renewal (#7337)
* [client] Close the session-expiration dialog only on an actual session renewal The dialog auto-closed on any Connected status snapshot, but the daemon emits Connected periodically regardless of session state, so the warning popup disappeared on the next snapshot (~30s) with no chance to re-authenticate. Close only when the snapshot's session deadline jumps past the one the dialog was opened for, meaning the session was renewed from another surface (tray action, CLI, main window). * [client] Compare session renewals against the exact deadline in the expiration dialog The dialog reconstructed its reference deadline from the relative seconds URL parameter, which carries up to a second of truncation and mount latency, forcing a renewal-detection margin wide enough to miss a renewal made shortly after the previous login. Pass the absolute deadline (unix ms) from both tray call sites - the extend flow's cached deadline and the final warning's event metadata - so any forward jump in the snapshot deadline closes the dialog; the seconds-derived fallback with a small tolerance remains for an unknown deadline. * [client] Derive the expiration dialog countdown from the deadline The per-second decrement assumed the interval fires once a second, but the webview's timers get suspended for tens of seconds under App Nap / hidden-window throttling, leaving the displayed countdown behind the wall clock by the suspended time. Recompute the remaining time from the absolute deadline on every tick so the first tick after a suspension shows the correct value. * [client] Tolerate the warning deadline's second precision in the renewal check The final-warning metadata formats the deadline as RFC3339 truncated to whole seconds while the status snapshot keeps millisecond precision, so an unchanged deadline could appear up to 999 ms newer than the exact URL value and close the dialog on the first snapshot. Allow a sub-second tolerance on the exact path; any real renewal jumps by at least seconds. |
||
|
eYey
|
945b0b6be2 |
Store the Android split tunnelling settings per profile (#7349)
Which applications the tunnel carries belongs with the rest of a profile's preferences rather than on the Android side, so the choice follows the profile the user is on. Adds a split tunnel store beside the SSH session store, over a "split-tunnel" namespace holding the mode and both selections. The two selections are kept apart because the platform applies an allow list or a deny list and never both, and so that switching mode does not throw away the picks made in the other one. Only the store itself needs the android build tag; the rest stays untagged so it is covered by the package's host tests. |
||
|
Bethuel Mmbaga
|
11733fd718 | [infrastructure] Improve domain, Docker Compose, and license validation in self-hosted scripts (#7339) | ||
|
Pascal Fischer
|
353251d886 | [management] fix posture check evaluation for direct peers in policy definition (#7348) | ||
|
Pascal Fischer
|
611a9291cd | [management] fix posture check flip evaluation for affected peers calc (#7347) | ||
|
Zoltan Papp
|
89c6e84a41 |
[client, ios] Fix context cancellation during restart (#7329)
* fix(mobile): stop the client synchronously so a restart cannot inherit a cancelled context
Original finding
----------------
A user reported that leaving home and switching from wifi to cellular killed
all Internet traffic until NetBird was turned off. A debug bundle captured the
failure (iOS, CLI 0.75.0, self-hosted management, generated 2026-08-18 01:17;
the incident is at 2026-08-17 22:37:38-51 UTC).
The bundle shows the whole sequence:
22:37:38.255 management sync stream drops (keepalive ACK timeout)
22:37:43.670 Swift: "Network type changed: wifi -> cellular" -> schedules a
restart with a 1s debounce
22:37:44.737 Go: "ensuring wg interface is removed, Netbird engine context
cancelled" - engineCtx dies, every peer gets context canceled
22:37:49.910 iface.go:238 "failed to remove WireGuard interface utun6:
timeout when waiting for interface utun6 to be removed"
-> the teardown stretches out for ~5s
22:37:50.710 Swift: "restartClient: starting client", needsLogin=false
(so this is NOT a login expiry)
22:37:51.013 Go: connect.go:476 "exiting client retry loop due to
unrecoverable error: context canceled" - the OLD run dies here
22:37:51.333 Go: grpc.go:135 "failed creating connection to Management
Service: context canceled" - the NEW start, 2ms after the old
run finally exited
22:37:51.334 Swift: "restartClient: start failed" -> widget disconnected
then nothing for 15 minutes
The tunnel stayed installed with no engine behind it, so every packet was
black-holed. status.txt, generated ~14 hours later, still reads Management:
Disconnected / Signal: Disconnected / Peers count: 0/0 - the client never
recovered on its own.
Root cause
----------
Client.Stop() cancelled a shared ctxCancel field and returned immediately,
without waiting for the run loop to exit. The Swift stop{} completion handler
therefore fired while the Go teardown was still running (stretched out by the
utun6 removal timeout), and the start that followed landed on a context that
the outgoing run was about to cancel.
Two further paths wrote the same shared field. IsLoginRequired() and
LoginForMobile() each overwrote c.ctxCancel, so any call to them during a live
session discarded the running engine's cancel function. restartClient() calls
needsLoginCached() on exactly this path.
Changes
-------
- Stop() now drives the stored ConnectClient: ConnectClient.Stop() cancels the
run context and blocks on runExited, so the caller's completion handler only
fires once the run loop has really finished. The ctxCancel path stays as a
fallback for when no ConnectClient exists yet (e.g. during LoginForMobile).
- Run() owns its cancel in a local variable, so a concurrent call that
overwrites the shared field can no longer cancel this run's context through
the deferred cleanup.
- IsLoginRequired() and LoginForMobile() use local cancels and leave the shared
field alone. LoginForMobile's cancel moves into the deferred cleanup of the
goroutine that outlives the call, so the OAuth token wait is not cut short.
- The Android SDK gets the same treatment. The structural defect is identical
there, but the trigger is absent: Android has no automatic engine restart on
a network type change, and no interface-removal timeout to stretch the
teardown. This part is preventive, not a fix for an observed failure.
* fix(mobile): do not let a superseded startup publish its client
Review found a window the previous commit left open. Run stored its cancel
function and only published the ConnectClient later, after loading config and
constructing the client. A Stop landing inside that window found no
ConnectClient, cancelled the run and returned immediately. A new Run could then
publish its own client, and the cancelled older run — still executing — would
overwrite it with a client that was already being torn down. The next Stop
stopped that stale client and left the live one running with nothing tracking
it.
Runs now carry a generation. Run claims one before doing any work and publishes
its client only while the generation is still current; a superseded run returns
without touching the shared state. Stop bumps the generation, so any startup
still in flight is invalidated, then cancels it and waits for the run to exit
before returning (20s cap so a wedged teardown cannot block the caller
forever).
setState is gone: publishState replaces it at both call sites on each platform.
* fix(ios): add a non-waiting Stop for callers on a deadline
Stop now waits for the run loop to exit, which is what a restart needs but
wrong for stopTunnel: iOS gives NEPacketTunnelProvider only a few seconds
there before it kills the extension, and the wait can run to its 20s cap.
Waiting past the deadline earns a SIGKILL, so the next start inherits a dirty
state instead of the orderly shutdown the wait was meant to buy.
StopWithoutWait tears the client down and returns. ConnectClient.Stop blocks on
runExited with no cap of its own, so the non-waiting path runs it detached
rather than only skipping the runDone wait.
Android keeps a single blocking Stop: it has no equivalent deadline.
* fix(mobile): guard the run lifecycle with a single lock
Stop and beginRun each touched the same lifecycle state across two locks in
sequence: take stateMu, release it, then take ctxCancelLock. A run starting in
that gap installed its own cancel before Stop reached it, so Stop cancelled the
fresh run and left its own target running — the same class of defect this branch
exists to fix, this time in the locking rather than the state.
ctxCancel moves into the stateMu group, and both sides take their snapshot in
one critical section. ctxCancelLock then guarded nothing and is gone.
* fix(mobile): drop the run-generation machinery for a serialized lifecycle
The platform callers (Swift/Kotlin) always stop before starting and coalesce
restarts, so the generation counter guarded against call patterns that cannot
occur. Replace it with a single-run contract:
- startRun refuses a second Run while the previous one has not exited
- finishRun clears the published state on every exit path, including errors
- Stop cancels and waits for the run loop with a bounded timeout; it no
longer calls ConnectClient.Stop, whose wait is unbounded
- concurrent Stops wait on the same exit channel instead of returning early
- a superseded startup no longer reports a clean nil exit
* revert(android): drop the run lifecycle changes
Android does not have the defect this PR fixes. On ux/ios-style-redesign the
EngineRestarter is gone: network changes are handled as events instead of an
engine restart, so nothing stops the client and starts it again.
The remaining stop() callers are all final teardowns on the main thread with a
framework deadline - the stop-engine broadcast receiver, onDestroy, onRevoke and
the binder's stopEngine. A Stop that waits for the run loop would risk an ANR
there for a race that cannot occur, so the fix stays iOS-only.
* fix(ios): make loginComplete race-free
The OAuth goroutine spawned by LoginForMobile sets loginComplete after the
call has returned to Swift, while the Swift side polls IsLoginComplete and
later calls ClearLoginComplete from its own thread. The plain bool made all
three unsynchronized: the store may never become visible to the poller, and
a Clear racing the store can be lost, leaving a stale true that makes the
next login look already complete.
Switch the field to atomic.Bool. It is a standalone flag rather than part of
the run lifecycle that stateMu guards, and it has to stay readable while the
login goroutine is still in flight.
|
||
|
Riccardo Manfrin
|
6620219939 |
[client] Add catch-all NRPT rule when NetBird is the primary DNS resolver (#7071)
* [client] Add catch-all NRPT rule when NetBird is the primary DNS resolver
* Remove obvious comments
* Install the catch-all rule where the adapter's DNS is set
addDNSSetupForAll makes us the peer's main DNS forwarder, and the catch-all NRPT
rule is the other half of that same job: without it the adapter's NameServer only
adds one more resolver to the set Windows queries in parallel. Having the two in
one place says that, where a separate block at the end of applyDNSConfig read as
an afterthought.
The block could not simply move up: removeDNSMatchPolicies deletes the catch-all
key too, so installing the rule before it ran would have had the rule deleted
moments later. The cleanup now runs first, which is what it was always for - it
clears what the previous apply installed before this one installs anything - and
keeps being unconditional, so a leftover rule from an earlier run cannot survive
into a config that no longer wants it.
* Name the escape hatch after the behaviour it restores
NB_DISABLE_DNS_CATCHALL_NRPT described the mechanism it switches off. What an
operator reaching for it wants is the behaviour they had before, so name it that:
NB_USE_LEGACY_DNS_RESOLUTION, matching NB_USE_LEGACY_ROUTING, the only other
legacy switch in the client.
Not NB_WIN_LEGACY_FULL_TUNNEL_DNS_RESOLVE, as first suggested: the rule follows a
primary nameserver group, not a full tunnel, and putting FULL_TUNNEL in a public
variable name would carry that confusion for as long as the variable lives. No
OS prefix either, since nothing else in the client has one and this switch is
inert anywhere but Windows by construction.
Behaviour and default are unchanged: the catch-all rule is on unless the variable
says otherwise.
* Exempt .local from the catch-all rule
RFC 6762 reserves .local for multicast DNS and says unicast resolvers must not
answer for it. The catch-all rule hands it to us anyway, we forward it to
whatever upstream the primary nameserver group points at, and the answer comes
back NXDOMAIN for hosts that do exist - printers, NAS boxes, anything
announcing itself on the link. Confirmed on a Win11Pro VM: laptop.local resolves
with the client down and returns "Nome DNS inesistente" with it up, and the
client log shows the query arriving on the catch-all handler and being forwarded
to 1.1.1.1.
An NRPT rule that names a namespace and lists no servers is an exemption: the
DNS client resolves those names as it would with no rule at all. What that looks
like in the registry is not what it sounds like. Writing no server value and
clearing ConfigOptions produces a rule Windows treats as a no-op - it never
appears in Get-DnsClientNrptPolicy -Effective and the catch-all keeps the query.
The value has to be present and empty, with ConfigOptions still 0x8: the flag
says the server list is the meaningful part of the rule, and an empty list then
means "no server, resolve normally". Verified both encodings on the VM.
Installed together with the catch-all, since without one nothing captures .local
in the first place, and removed with it.
Exclusivity is unaffected elsewhere, and a more specific rule still wins - a
match domain under .local keeps resolving through NetBird, which is what a
legacy Active Directory domain named corp.local needs. Verified separately that
a match domain does take precedence over the catch-all: declaring fritz.box
against the local router restored laptop.fritz.box while the catch-all was in
force.
* Treat the root namespace as a match domain, not a special case
The catch-all had a function, a registry key and a call site of its own, which
made it look like a different mechanism. It is not: "." is an NRPT namespace like
any other, it just happens to match every name. So it goes into the match domain
list, and addDNSMatchPolicy writes it along with the rest — batching, GPO
variant, volatile keys and cleanup all come for free.
The .local exemption stays a rule of its own, and not for symmetry: it is the one
rule with a different server list, an empty one. Putting it in the same Name
value would give it our resolver and exempt nothing.
Windows expands a rule's Name value into one effective namespace each, so a rule
carrying {.example.com, .} still shows both as separate rows in
Get-DnsClientNrptPolicy -Effective. Nothing is lost for diagnosis by dropping the
dedicated key.
Suggested by Vik in review.
* Do not report a failed NRPT cleanup as success
removeRegistryKeyFromDNSPolicyConfig returned nil for every OpenKey error, so a
permission or registry failure was indistinguishable from a key that was never
there. Cleanup then reported success while the rule stayed in force — which is
how a rule outlives the interface it points at and keeps sending every query to
an address that no longer answers.
Distinguish the two, the way listNRPTRuleKeys already does for the policy store
root: a missing key is nothing to do, anything else reaches the caller.
restoreHostDNS now propagates that error instead of logging it. applyDNSConfig
keeps logging on purpose: there we are about to write fresh rules over whatever
survived, while restore is the path where a rule left behind is the whole
problem.
Also addresses review nits on the tests: doc comments on the two added cases,
reported Close and DeleteKey errors so a failed cleanup cannot contaminate the
next registry test, and a context message on the exemption's namespace assertion.
|
||
|
Viktor Liu
|
63c26be72f | [client] Add local Prometheus metrics endpoint (#6689) | ||
|
Pascal FischerandDmitri Dolguikh
|
e06c17cf59 |
[management] network map from nmap data type (#6919)
Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io> Co-authored-by: Dmitri Dolguikh <dmitri.external@netbird.io> |
||
|
Viktor Liu
|
473392a935 | [client] Tolerate a still-locked updater binary when cleaning up after an update (#7286) | ||
|
Viktor Liu
|
0bd1147ff0 | [client] Keep NetBird traffic out of third-party fwmark rules (#7314) | ||
|
Viktor Liu
|
f221347c7a | [infrastructure] Trigger the dashboard wasm client bump on release tags (#7277) | ||
|
dmitri-netbird
|
0a9ce7f797 |
[client] fix a flake in TestResolver_ConcurrentStaleHitsCollapseRefresh test (#7326)
* fix a flake in TestResolver_ConcurrentStaleHitsCollapseRefresh test Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io> * use testify's eventually asserts Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io> --------- Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io> |
||
|
Viktor Liu
|
7e8b4e1417 |
[client, proxy] Remove lazy connection exclusions and run Rosenpass on the embedded proxy (#6763)
* Run lazy connection manager for rosenpass peers * Treat forward-target peers as normal lazy connections * Run Rosenpass in permissive mode on the embedded proxy |