security/netbird: make the daemon configuration write survivable, and fix a setting that never applied - #5673
Open
BxnnyG wants to merge 2 commits into
Open
security/netbird: make the daemon configuration write survivable, and fix a setting that never applied#5673BxnnyG wants to merge 2 commits into
BxnnyG wants to merge 2 commits into
Conversation
/var/db/netbird/config.json holds the peer's WireGuard private key. Applying settings rewrote it with file_put_contents, which truncates and then writes: an interrupted write - a full /var, a power cut - leaves it short, and there is no other copy of that key on the box. The cost is not a wrong setting, it is re-enrolling the firewall by hand from the console. The new content now goes to a sibling temporary file, is flushed to disk and renamed over the target, which POSIX requires to be atomic, and the previous content is kept as config.json.bak. Both are created under umask 0077 and then given the target's own mode, so the key never widens on the way through - a chmod afterwards would be closing a door that was already open. The read path now says which failure it hit. A missing configuration and a corrupt one shared one log line; they are different problems and only one of them is recoverable by hand. Verified on a 26.7 router: Apply leaves a config.json.bak byte-for-byte the size of the previous configuration, both 0600, and the daemon keeps its identity. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The setting validates, saves, survives Apply and lands in config.json as "IpMapping". NetBird has no such field - not in v0.74.4, not in v0.60.0, v0.45.0 or v0.30.0, and a code search over netbirdio/netbird matches the substring only inside the function name parseNATExternalIPMappings. Go's encoding/json drops unknown keys, so the daemon has been reading the file and ignoring that line ever since the setting was added. Read as names and types only, the key set of a live config.json and the persisted fields of NetBird's Config struct at v0.74.4 agree on 36 of 37 names. IpMapping is the one the daemon does not know. The field the help text describes is NATExternalIPs, set on the CLI with --external-ip-map. The help text gives its three forms correctly, down to the examples, so the feature was understood - only its destination was not. Two things were wrong, not one: the name and the type. The daemon wants a list of strings, so a comma-separated field becomes a list, and an empty field becomes an empty list rather than null or a list holding one empty string - to a Go []string those are three different values. IpMapping is removed rather than left beside the new key, so the next reader does not have to work out which of the two the daemon reads. Anyone who has filled this field in has had a setting that did nothing. After this it does something, which is worth knowing before upgrading. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important notices
If AI was used, please disclose:
Co-Authored-Bytrailers.I did not open an issue first and should have — I am happy to do that now, or to split anything here that is too large to review as a single change. Apologies for submitting without this template filled in; that was my mistake, not a deliberate omission.
Two defects in the same function, both about
/var/db/netbird/config.json— the file that holds the peer's WireGuard private key.1. The write was not atomic
syncConfig()read the file, changed sixteen keys and wrote it back withfile_put_contents(), which truncates and then writes. Any interrupted write — a full/var, a power cut — leaves the file short, and there is no other copy of that key on the box. The cost is not a wrong setting; it is re-enrolling the firewall by hand from the console.The new content now goes to a sibling temporary file, is flushed with
fsync()and renamed over the target. Both the temporary file and theconfig.json.bakit leaves behind are created underumask 0077and then given the target's own mode, so the key never widens on the way through — achmod()afterwards would be closing a door that was already open.The read path also says which failure it hit. A missing configuration and a corrupt one shared one log line; they are different problems and only one of them is recoverable by hand.
2.
IpMappingis not a NetBird configuration keySettings → General → Force IP Mapping validates, saves, survives Apply and lands in
config.jsonas"IpMapping". NetBird has no such field — not inv0.74.4, not inv0.60.0,v0.45.0orv0.30.0— and a code search overnetbirdio/netbirdmatches the substring only inside the function nameparseNATExternalIPMappings. Go'sencoding/jsondrops unknown keys, so the daemon has been reading the file and ignoring that line ever since the setting was added.Read as names and types only, the key set of a live
config.jsonand the persisted fields of NetBird'sConfigstruct atv0.74.4agree on 36 of 37 names.IpMappingis the one the daemon does not know.The field the help text describes is
NATExternalIPs, set on the CLI with--external-ip-map. The help text gives its three forms correctly, down to the examples, so the feature was understood — only its destination was not. Two things were wrong: the name and the type, since the daemon wants a list of strings rather than a comma-separated one.Worth flagging for the changelog: anyone who has filled that field in has had a setting that did nothing, and after this it does something.
Testing
Both changes were verified on a live 26.7 router (
os-netbirdbuilt from this branch, daemon 0.74.4):config.json.bakbyte-for-byte the size of the previous configuration, both0600, andnetbird statusafterwards shows management and signal connected — the daemon keeps its identity.jq '{IpMapping, NATExternalIPs}' /var/db/netbird/config.json→{"IpMapping": null, "NATExternalIPs": []}.ConfigFileis deliberately free of OPNsense classes so it can be exercised without the framework, and I have unit tests for it — the atomic replacement, the backup, the mode, the failure paths and the list splitting. They are not in this diff, because no plugin in this repository ships atests/directory and there is no CI to run them; I did not want to introduce a convention as a side effect of a bugfix. Happy to add them in whatever shape you would want them, or to leave them out.Checked against
phpcs --standard=<core>/ruleset.xml: no new errors or warnings.