fix: security and quality audit — 21 findings addressed#376
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a broad security/quality audit remediation for the NetBird Home Assistant add-on, focusing on secret handling, CI hardening, and runtime reliability/AppArmor compatibility.
Changes:
- Hardens the add-on startup script (safer config migration, secret handling via
NB_SETUP_KEY, redacted env var logging, atomicresolv.confupdate, andexecfor proper signal handling). - Strengthens supply-chain/CI security (pins helper actions to a SHA, adds explicit job permissions, replaces archived changed-files action usage with
git diff, and reduces expression-injection exposure viaenv:). - Adds an AppArmor profile and pins the upstream NetBird image by digest.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
netbird/rootfs/etc/s6-overlay/s6-rc.d/netbird/run |
Improves startup security/reliability (setup key handling, env var validation/redaction, atomic resolv.conf update, exec). |
netbird/rootfs/etc/s6-overlay/s6-rc.d/netbird/finish |
Removes dead code in the finish handler. |
netbird/Dockerfile |
Pins NetBird base image by digest for supply-chain integrity. |
netbird/config.yaml |
Tightens schema validation for URLs/hostname and restricts env vars to NB_ prefix. |
netbird/apparmor.txt |
Introduces an AppArmor profile with broader execution/path/network permissions needed for the add-on. |
docs/reviews/CODEBASE_REVIEW_2026_04_11.md |
Adds the audit report documenting findings and remediations. |
.github/workflows/update-changelog.yaml |
Moves interpolated values into env: to reduce injection risk and improves robustness. |
.github/workflows/lint.yaml |
Adds explicit permissions and pins helper action to a SHA. |
.github/workflows/builder.yaml |
Adds explicit permissions, pins helper actions, uses git diff for changed files, and avoids direct ${{ }} interpolation in bash logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bashio::log.info "Setup Key configured (hidden for security)" | ||
| options+=(--setup-key "${setup_key}") | ||
| export NB_SETUP_KEY="${setup_key}" | ||
| fi |
There was a problem hiding this comment.
Fixed: Added null check to setup_key (matching the existing log_level pattern on line 82). Now checks [ "${setup_key}" = "" ] || [ "${setup_key}" = "null" ].
| admin_url: url? | ||
| management_url: url? | ||
| setup_key: str? | ||
| hostname: match(^[a-zA-Z0-9]([a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])?$)? |
There was a problem hiding this comment.
Fixed: Added null checks to admin_url and management_url. Both now check for both empty string and "null" literal, consistent with how log_level is handled.
| env_vars: | ||
| - name: match(^[A-Z_][A-Z0-9_]*$) | ||
| - name: match(^NB_[A-Z0-9_]+$) | ||
| value: str |
There was a problem hiding this comment.
Fixed: Updated DOCS.md env_vars section to document the NB_ prefix requirement and explain the restriction.
| if ! grep -q '# systemd-resolved' /etc/resolv.conf 2>/dev/null; then | ||
| RESOLV_TMP=$(mktemp /tmp/resolv.conf.XXXXXX) | ||
| if { printf '# systemd-resolved\n'; cat /etc/resolv.conf; } > "${RESOLV_TMP}" \ | ||
| && mv -f "${RESOLV_TMP}" /etc/resolv.conf; then | ||
| bashio::log.debug "Prepended systemd-resolved marker to /etc/resolv.conf" | ||
| else | ||
| bashio::log.error "Failed to update /etc/resolv.conf for systemd-resolved workaround" | ||
| rm -f "${RESOLV_TMP}" 2>/dev/null |
There was a problem hiding this comment.
Fixed: Added mktemp failure guard. If mktemp fails, RESOLV_TMP is set to empty and the inner block is skipped with an error log.
|
Nice PR, maybe after it HA safety rating of this app will be raised from "orange" 3 to be closer to "green" Tailscale's 6. |
Address 21 findings from codebase audit: - SEC-01: Move setup key from CLI arg to NB_SETUP_KEY env var - SEC-02/CI-04: Fix GitHub Actions expression injection vectors - SEC-03: Pin home-assistant/actions to commit SHA - AA-03: Add /sbin/ to AppArmor for iptables execution - SEC-05/06: Validate URLs, enforce NB_ prefix for env vars - SEC-07: Pin Docker image by digest - CQ-01/02/03: Error handling, atomic resolv.conf, exec for signals - AA-04/05/06/07: AppArmor path and network rule gaps - CI-01/03/05: Replace archived action, add permissions, fix patterns Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add null checks for optional config fields (setup_key, admin_url, management_url, hostname) matching existing log_level pattern - Guard mktemp failure in resolv.conf workaround - Document NB_ prefix requirement for env_vars in DOCS.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
lfarkas
force-pushed
the
audit/security-quality-2026-04-11
branch
from
2938fbb to
8c2b2a5
Compare
📝 WalkthroughWalkthroughThe changes pin reusable workflow actions, add an AppArmor profile for NetBird, and revise startup handling for configuration migration, options, secrets, DNS configuration, and process execution. ChangesNetBird hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
netbird/apparmor.txt (2)
27-28: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider narrowing broad filesystem access paths.
/data/** rwand/config/** rwgrant read-write access to entire directory trees. The run script only accesses/config/config.json(line 14) and/homeassistant/netbird/config.json(line 13). Narrowing these to the specific paths NetBird actually uses reduces the attack surface if the process is compromised.For example, if only
config.jsonneeds to be read/written under/config/:- /config/** rw, + /config/config.json rw,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbird/apparmor.txt` around lines 27 - 28, Replace the broad `/data/** rw` and `/config/** rw` AppArmor rules with permissions limited to the specific paths used by the run script: `/config/config.json` and `/homeassistant/netbird/config.json`, preserving only the required read/write access.
56-56: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider narrowing
/proc/sys/net/** rwto specific sysctl paths.Write access to all of
/proc/sys/net/**allows modifying every network-related kernel parameter. If NetBird only writes to specific sysctls (e.g.,/proc/sys/net/ipv4/ip_forward), restricting to those paths would limit unintended kernel parameter changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbird/apparmor.txt` at line 56, The AppArmor rule grants overly broad write access to `/proc/sys/net/**`; replace it with write permissions for only the specific sysctl paths NetBird modifies, such as `/proc/sys/net/ipv4/ip_forward`, and remove the wildcard rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@netbird/apparmor.txt`:
- Around line 27-28: Replace the broad `/data/** rw` and `/config/** rw`
AppArmor rules with permissions limited to the specific paths used by the run
script: `/config/config.json` and `/homeassistant/netbird/config.json`,
preserving only the required read/write access.
- Line 56: The AppArmor rule grants overly broad write access to
`/proc/sys/net/**`; replace it with write permissions for only the specific
sysctl paths NetBird modifies, such as `/proc/sys/net/ipv4/ip_forward`, and
remove the wildcard rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8256e45a-1df1-488c-bae1-e94e136a9ff0
📒 Files selected for processing (4)
.github/workflows/builder.yaml.github/workflows/lint.yamlnetbird/apparmor.txtnetbird/rootfs/etc/s6-overlay/s6-rc.d/netbird/run
Summary
Key Changes
NB_SETUP_KEYenv var; expression injection vectors fixed withenv:blocks; actions pinned to SHAf6f29a7; Docker image pinned by digestexec netbird upfor proper S6 signal delivery/sbin/**for iptables,/config/**for state files,network inet rawfor ICMP,network unix streamfor daemon socketjitterbit/get-changed-fileswith native git diff; explicit permissions on all jobsurl?type; hostname regex enforced; env_vars requireNB_prefix (breaking change)Breaking Changes
env_varsname regex changed from^[A-Z_][A-Z0-9_]*$to^NB_[A-Z0-9_]+$— users with non-NB_prefixed variables must rename themNot Fixed (2 findings)
Test Plan
shellcheckpasses onrunscript (clean)shellcheckonfinishscript shows only pre-existing warningsFull audit report:
docs/reviews/CODEBASE_REVIEW_2026_04_11.mdGenerated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Maintenance