update netbird server modules - #487367
Conversation
nixpkgs-ci
Bot
added
10.rebuild-linux: 1-10
9091206 to
a73ec87
Compare
nixpkgs-ci
Bot
added
8.has: documentation
3861769 to
4ec6429
Compare
|
I was about to actually take the time and review this insanely huge, single commit PR when I found my name in there. I don't know what happened but I strongly think you took my PR piped it through your LLM of choice and then posted it. I'm not generally against the usage of LLM however when it's this obvious I would have liked at least a notice. Anyway what I am really against is setting me as a maintainer without asking, or even notifying me. Please do not do that. As I've stated in the other netbird PR I am not interested in maintaining it anymore. If you actually want to adopt/modernize the netbird module:
|
4ec6429 to
c2ca54f
Compare
nixpkgs-ci
Bot
added
the
8.has: maintainer-list (update)
b42d5bc to
22fe74f
Compare
|
@PatrickDaG apologies, I'm new to nixpkg submissions and forgot to update the maintainer. I've since removed you in lieu of myself. This PR is based on main, where you're the listed maintainer. No PR of yours was run through an LLM, and though I did leverage Claude for assistance, the PR isn't 100% vibe-coded. As far as I can see, that's fine by the contribution guidelines, but if a heads-up on this is helpful to reviewers then sure, Ican include one in future! I've re-read the contribution guidelines in-depth, received some advice on a few key RFCs on another PR of mine here and re-written this PR in accordance. Notably, I've split this PR into several smaller commits (each detailed in the description) and limited the addition of new typed options. Hopefully this is more parseable for the reviewer :) if there's any additional feedback, I'll do my best to implement it ASAP! |
|
@shuuri-labs thanks for contributing this, I have been itching to re-write the On that note, I looked through changes and noticed you decided to keep nginx. Is there any consideration for adding a If there's no time, I may add this option myself at a later date once this gets merged. [1] https://docs.netbird.io/selfhosted/migration/enable-reverse-proxy |
|
Hey @debtquity, you're welcome :) Two reasons for not making the switch to Traefik just yet:
Definitely not done with the NetBird module after this gets merged, don't worry! |
|
This Caddy configuration worked for me: https://github.com/bct/nix-config/blob/a147a7172527af8ae9998f6ecaede7a86cbd1956/nixos/cloud/megahost-one/netbird.nix#L70-L119 |
nixpkgs-ci
Bot
added
the
2.status: merge conflict
PatrickDaG
left a comment
PatrickDaG
left a comment
There was a problem hiding this comment.
Hope I didn't come of too direct in the last message. Sorry about that. I should rather thank you that someone finally adopted the netbird module and will hopefully modernize it and add support for all the new things.
The multiple commits make it much more readable, thank you for that. I've added some small questions/suggestion, but all in all looks promising.
Oh, if you want you can of course take a look at my PR #354032, or have claude take a look, then I can at least feel like all the time spend on it wasn't completely wasted.
There was a problem hiding this comment.
I'm not sure what the official nixpkgs stance is on this but I feel like this basically just lists the options and doesn't really add anything apart from a whole bunch of text to the manual.
There was a problem hiding this comment.
Yeah, fair enough. I've trimmed the docs to remove sections that just listed individual options
| description = "Anonymize sensitive information in logs."; | ||
| }; | ||
|
|
||
| extraEnvironment = mkOption { |
There was a problem hiding this comment.
Is there a reason why you declare all these options? If not I would think an generic RFC 42 style ´settings´ option would keep the module smaller.
There was a problem hiding this comment.
I'venow replaced them with the existing extraEnvironment (freeform attrsOf str) for NB_* env vars and config (freeform JSON) for config.json overrides. Removed just under 200 lines in the process :) hope this is more RFC 42 compliant!
|
|
||
| for name in instances: | ||
| node.wait_for_unit(f"{name}.service") | ||
| node.wait_for_file(f"/var/run/{name}/sock") | ||
|
|
||
| for name in instances: | ||
| wait_until_rcode(node, f"{name} status |& grep -C20 Disconnected", 0, retries=5) | ||
|
|
||
| # Verify environment variables are set correctly for advanced client |
There was a problem hiding this comment.
I feel like testing for the environment here is not really useful since it just tests the module options we have created instead of the interaction with the actual program that we want to test.
There was a problem hiding this comment.
I've removed them along with the extra test clients. The test now checks core functionality (services start and reach 'Disconnected' state)
| stopIfChanged = false; | ||
| }; | ||
|
|
||
| systemd.services.netbird-relay.serviceConfig.EnvironmentFile = "/run/netbird-relay/env"; |
There was a problem hiding this comment.
Does this work? According to ´systemd.exec´ it should fail because the file will be accessed befor preStart runs?
There was a problem hiding this comment.
Yep, this is a bug. Fixed now and tested on my actual NetBird self-hosted instance. I found a fixed a couple of extra bugs which I've detailed in my comment below
The signal server's RuntimeDirectory, StateDirectory, and WorkingDirectory were incorrectly set to "netbird-mgmt" (copied from the management module). Fix them to use "netbird-signal".
25d56de to
bd29632
Compare
The relay module defaulted to pkgs.netbird (the client package), which does not contain the netbird-relay binary. Use pkgs.netbird-relay to match the pattern of the other server modules (signal, management, dashboard).
Both relay and management servers default to port 9090 for metrics, causing a bind conflict when colocated. Add an explicit metricsPort option to the relay module defaulting to 9091.
The management module set IdpManagerConfig.ManagerType to "integrated" which is not a valid type in the netbird binary. The embedded IDP uses a separate top-level EmbeddedIdP config section, not IdpManagerConfig. Set ManagerType to "none" when embedded IDP is enabled and use the correct EmbeddedIdP key with Enabled, LocalAddress, and nested Storage.Config fields matching the upstream Go struct.
Avoids port clash with signal server which defaults to 9091. Management=9090, signal=9091, relay=9092.
|
Apologies, I've been out sick for the past month. I'm back now have been working on/testing this for the past couple days. I've addressed your comments and tested this module on my personal VPS which runs my own self-hosted NetBird instance. I found (and fixed) the following issues:
Other than that, everything is working well - I can connect clients, connect to and pass traffic via Relay, access the dashboard and change settings etc. Here's the basic config that uses the new module. I've commented what needs replacing should anyone want to test it themselves: https://gist.github.com/shuuri-labs/2aeb3a7229723620375a1396765d513a |
|
Remove oidcConfigEndpoint from management test nodes since idp.test doesn't resolve in the test VM. Check all iptables chains for firewall rules since NixOS puts them in nixos-fw, not INPUT directly.
d77ef1b to
a10f5d6
Compare
|
Thanks for running the tests @PatrickDaG! pushed a fix for both failures:
|
|
Can you make sure the tests run on your end? |
|
My netbird dashboard is broken at the moment, and I came across this when trying to fix it. Thought I'd chip in regarding the traefik configuration. I've written the code that eventually became #490985, which is a WIP redesign to the traefik module. As you said it's probably best to leave adding a traefik proxy option to the netbird module for a separate PR; I would also recommend waiting until the aforementioned PR is merged as well. I can give pointers on how to write the traefik config in a way that takes advantage of some of the new features coming up, since the approach will be a little different. As far as the actual traefik options, this should have the options you need: https://docs.netbird.io/selfhosted/external-reverse-proxy#combined-container-setup-v0-65-0 |
| CredentialsTTL = "12h"; | ||
| Secret = "not-secure-secret"; | ||
| TimeBasedCredentials = false; | ||
| }; |
There was a problem hiding this comment.
Should we not remove this from the default if the builtin relay is now recommended? Enabling both by default seems wrong to me.
Sync the server module from NixOS#487367 (head) onto release-26.05: introduces the netbird native relay module, wires relay into management, and adds the embedded IdP option. Verified compatible with the netbird 0.74.2 binary (flags and management.json schema unchanged 0.69 -> 0.74.2).
The derived EmbeddedIdP settings seed the initial owner via an Owner.Password field, but netbird reads the bcrypt hash from Owner.Hash. OwnerConfig in management/server/idp/embedded.go has fields Email/Hash/Username and no Password, so the admin is never created and cannot log in. Fix the derived block and the server.md example, and switch the doc from htpasswd to mkpasswd (idiomatic, no apacheHttpd dependency, native $2b$ variant; -R 10 because the mkpasswd default cost is 5). Verified against netbird 0.69.0 (the version this module targets) and 0.74.2. Fix for NixOS#487367 - feel free to cherry-pick.
The TURNConfig.Secret world-readable check fired whenever the value is a string, regardless of whether TURN is used. On the relay path (useRelay = true, no coturn) TURNConfig.Turns is empty and the secret is never read, yet the warning still fired, including for the relay example in server.md. Gate the check on Turns being non-empty, mirroring the Relay.Secret check which is already gated on relayAddresses. Fix for NixOS#487367 - feel free to cherry-pick.
Sync the server module from NixOS#487367 (head) onto release-26.05: introduces the netbird native relay module, wires relay into management, and adds the embedded IdP option. Verified compatible with the netbird 0.74.2 binary (flags and management.json schema unchanged 0.69 -> 0.74.2).
RafaelKr
left a comment
RafaelKr
left a comment
There was a problem hiding this comment.
Thanks for putting this together — the native relay and embedded IdP are exactly what a self-hosted NetBird needs (no external IdP, no coturn), and it's great to have server-side tests to go with them. I've been running this module for my small home-network since last weekend, rebased onto release-26.05 and with netbird bumped to 0.74.2; the module works unchanged against that newer binary (management.json keys and CLI flags are unchanged from 0.69), which is a good forward-compat signal. The rebased branch is here: https://github.com/RafaelKr/nixpkgs/tree/nixos-26.05-netbird — each point below is already implemented as a commit there, so feel free to cherry-pick.
A few things worth addressing before merge:
- Functional bug: the embedded-IdP owner is seeded with an
Owner.Passwordfield, but netbird only readsOwner.Hash(bcrypt) — withPasswordthe initial admin cannot log in. This is not version drift: the embedded IdP was introduced in netbird v0.62.0, and itsOwnerConfighas hadHash(neverPassword) from that first release through the current v0.74.2. Same key in theserver.mdexample. - Invalid default secret:
DataStoreEncryptionKeydefaults to"very-insecure-key", which is not valid base64 — netbird base64-decodes it andlog.Fatalfs on startup, so any deployment that doesn't override it already fails at runtime. Better to require it (defaultnull+ assertion), turning that runtime crash into a clear build-time error. - Redundant default TURN: the defaults ship a TURN server plus secret, enabled alongside the now-recommended native relay. @b12f already flagged this (#487367 (comment)). Emptying
TURNConfig.Turnsand nullingTURNConfig.Secretgives relay/no-TURN setups a clean config; coturn is unaffected (it overridesTurnsviaserver.nix). Note theStunslist should stay — STUN is needed for direct P2P even with the relay (the relay ships an embedded STUN on:3478, which the defaultStunsentry targets); onlyTurnsis relay-redundant.
Nothing blocking beyond the owner-field bug; the rest is security/UX hardening of the defaults.
| @@ -75,7 +85,17 @@ | |||
| Datadir = "${stateDir}/data"; | |||
| DataStoreEncryptionKey = "very-insecure-key"; | |||
There was a problem hiding this comment.
"very-insecure-key" is not valid base64 (it contains -). netbird base64-decodes this key (util/crypt/crypt.go, NewFieldEncrypt) and log.Fatalfs at startup, so any deployment that doesn't override the default already fails at runtime with a cryptic base64 error. Defaulting to null and asserting it is set turns that into a clear build-time error. (Also worth noting in the docs: netbird has no key-rotation command — the store is single-key AES-GCM — so the key must be chosen once with e.g. openssl rand -base64 32 and kept stable.)
| DataStoreEncryptionKey = "very-insecure-key"; | |
| DataStoreEncryptionKey = null; |
and, in the assertions list:
{
assertion = managementConfig.DataStoreEncryptionKey != null;
message = ''
services.netbird.server: settings.DataStoreEncryptionKey must be set.
Generate a key with `openssl rand -base64 32` and provide it as a secret, e.g.
settings.DataStoreEncryptionKey._secret = "/run/secrets/netbird-datastore-key".
'';
}Implemented in ad57cf1 — cherry-pick-ready.
| Owner = { | ||
| Email = ""; | ||
| Password = ""; | ||
| Username = ""; | ||
| }; |
There was a problem hiding this comment.
The embedded IdP's owner is seeded here with a Password field.
The embedded IdP reads the owner's bcrypt hash from Owner.Hash, not Owner.Password. OwnerConfig in management/server/idp/embedded.go has exactly three fields — Email, Hash, Username (no Password) — and the owner is only seeded when c.Owner.Hash != "", so an Owner.Password key is silently dropped by the JSON decoder and the admin is never created (login fails). Renaming the key to Hash fixes it.
This is not version drift — Password has never existed. The embedded IdP was introduced in netbird v0.62.0 (embedded.go is absent in v0.61.0 and earlier), and OwnerConfig has used Hash from that first release through the current v0.74.2. The value the PR supplies is already a bcrypt hash (per the htpasswd comment), so only the key name is wrong.
Likely origin of the name: the owner is mapped into a Dex dex.Password{ Hash: []byte(c.Owner.Hash) } where it's consumed — the Dex type is named Password, but its credential field is Hash. Easy to conflate the type name with the config key.
| Owner = { | |
| Email = ""; | |
| Password = ""; | |
| Username = ""; | |
| }; | |
| Owner = { | |
| Email = ""; | |
| Hash = ""; | |
| Username = ""; | |
| }; |
Implemented in 0e667c8 (with the server.md fix below) — cherry-pick-ready.
| TURNConfig = { | ||
| Turns = [ | ||
| { | ||
| Proto = "udp"; | ||
| URI = "turn:${cfg.turnDomain}:${toString cfg.turnPort}"; | ||
| Username = "netbird"; | ||
| Password = "netbird"; | ||
| } | ||
| ]; | ||
|
|
||
| CredentialsTTL = "12h"; | ||
| Secret = "not-secure-secret"; | ||
| TimeBasedCredentials = false; | ||
| }; |
There was a problem hiding this comment.
As @b12f correctly noted, shipping a default TURN server alongside the recommended native relay is wrong — and it also drags in two placeholder secrets (the Turns plaintext password and Secret) that trip the world-readable warnings on every relay-only setup, including the relay example in this PR's own server.md:
The TURNConfig.Secret is world-readable in the Nix Store, you should provide it as a _secret.
A TURNConfig.Turns password is world-readable in the Nix Store, you should provide it as a _secret.
Emptying Turns and nulling Secret fixes both: coturn overrides Turns via server.nix (mkDefault), and relay/no-TURN setups get a clean config. With Secret = null the existing isString world-readable check is already correct, so no warning-gating is needed. (For TimeBasedCredentials, add a small assertion that Secret is set when it is enabled.) A fuller alternative is to drop the whole TURNConfig block from the defaults and populate it only under the coturn branch — cleaner separation, but more invasive. The Stuns list must stay: STUN is needed for direct P2P even with the relay (the relay ships an embedded STUN on :3478, which the default Stuns entry targets).
| }; | |
| TURNConfig = { | |
| Turns = [ ]; | |
| CredentialsTTL = "12h"; | |
| Secret = null; | |
| TimeBasedCredentials = false; | |
| }; |
Implemented in 2a9d1aa — cherry-pick-ready.
There was a problem hiding this comment.
Somehow the suggestion code-range in the preview is off here (should replace full 51-64). I don't know why this is. But it's correct in my commit, I you descide to cherry-pick it as is.
| # Generate with: htpasswd -bnBC 10 "" 'your-password' | tr -d ':\n' | ||
| Password._secret = "/run/secrets/admin-password-hash"; |
There was a problem hiding this comment.
Two things here:
- The key must be
Hash, notPassword— same bug as the derived block inmanagement.nix(the value is already a bcrypt hash, per the comment). As written, copy-pasting this leaves the admin unable to log in. mkpasswdis the more idiomatic generator for a NixOS doc thanhtpasswd: it's a small standalone package (htpasswdpulls inapacheHttpd), prints just the hash (notr -d ':\n'), emits the native$2b$bcrypt variant, and can read the password from a prompt instead ofargv. Its default cost is only 5, so pass-R 10. (htpasswd's$2y$also works — Go's bcrypt gates on the major version only — so this is ergonomics, not a correctness fix.)
| # Generate with: htpasswd -bnBC 10 "" 'your-password' | tr -d ':\n' | |
| Password._secret = "/run/secrets/admin-password-hash"; | |
| # Generate with: mkpasswd -m bcrypt -R 10 (type the password when prompted) | |
| Hash._secret = "/run/secrets/admin-password-hash"; |
Implemented in 0e667c8 (same commit as the management.nix owner fix) — cherry-pick-ready.
|
I'm no longer with netbird, and since I've switched to Tailscale in my own homelab, so I'll be ceasing work on this PR. @RafaelKr has integrated my work into his PR, so that should serve as the successor to this one. Thanks for all the reviews/contributions, everyone! Closing. |
|
That's unfortunate. But thank you very much for your initial effort, I really appreciate it! I probably won't have the time to become the lone maintainer of the netbird module, especially because it's pre-1.0 and moving pretty fast right now. Still here are some notes that might be helpful:
|
|
I'd be up to comaintain the module, but I currently don't have time to help with the inital creation |
|
For me it's the same. I use netbird currently in a dockerized setup managed through |
|
Just to let you know, I'm working on it and making good progress. I already implemented my proposed usage of I'll do incremental PRs for easy reviewability. So first doing a polished version of the current PR state. Then do another PR for implementing traefik on top. Also for traefik we might wait on #490985. And then the proxy module in another separate PR. In another PR Caddy could also be added, as it's the third wesberver option that upstream netbird supports, if there's a need for it. For my setup I don't need it. |
When you say traefik as a webserver, do you mean as a proxy? I don't think traefik can actually host content directly, usually that requires a dedicated webserver such as nginx
That would be ideal, yes. When the time comes please put any routing configuration under |
|
Hey @RafaelKr are you still going to publish a PR? Instead of a big PR I want to start uploading chunks. Maybe the proxy next. |
|
Yes, I would like to. Hopefully I'll find some time on the next weekend. When talking about "proxy" we need to be careful what we're actually talking about. Because there are two things that can be called reverse proxy:
The second one depends on the first one, as it requires traefik as its ingress proxy. (The So actually we need to rework the ingress proxy first, to be able to use traefik and only then can introduce the reverse proxy service. And some people suggested to wait for the traefik refactor in #490985 (or maybe #553100 which might become its successor) before doing it here. So actually we should implement in the following order:
I'll tag you as soon as I open the PR. |
Agreed, I generally assume reverse proxies are an external component of the netbird service, and proxy means the netbird-proxy itself.
Do we need an IdP? I've been running the
Can't we just start with something small? let the netbird-proxy start under systemd, and let users configure their reverse proxies? It could be explained in the markdown. Shouldn't we follow a more single-responsability-principle and focus on netbird only? And start with systemd, and if later it's needed, we can add the modular service, but at least we would have a clear and well-defined interface. Maybe in the future, if RFC 189 is ever approved, we'll be able to specify "we need any reverse proxy with x and y features". |
10 participants
Motivation
The existing NetBird server modules have limited configurability - management and signal lack TLS options, there's no relay server module, and the embedded IDP isn't exposed. Essentially, the NixOS module's options lag pretty far behind the latest versions of NetBird's various components
Changes
Bug fix: signal state directory
The signal server's RuntimeDirectory, StateDirectory, and WorkingDirectory all pointed to netbird-mgmt instead of netbird-signal. This caused signal and management to share (and potentially corrupt) each other's files when both ran on the same machine.
New module: services.netbird.server.relay
Full relay server module with:
The orchestrator module (server.nix) gains useRelay and relayAuthSecretFile options to wire relay + management together automatically.
Expanded: services.netbird.server.management
settings.ProviderConfig.Owner.Email)
Expanded: services.netbird.server.signal
Expanded: services.netbird.clients (client module)
Commits
IDs in redirects.json
orchestration and unconditional turnDomain default
Tests
Documentation
How to test
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.