Sandcat netbird integration

[wiacekm]

No description provided.

[adamw]

Automated review. Generated by Claude Code from seven reviewer agents (functionality, security, structure, readability, simplicity, tests, performance) plus a verification pass. Line numbers refer to the PR head (d26bb2d). Findings marked verified were reproduced locally.

Blockers

• Entrypoint replacement drops master's behavior (cli/lib/composefile.bash:446). enable_netbird deletes the compose entrypoint and replaces it with mitmproxy-init.sh, which only re-does rm -f dns.conf. Master's entrypoint also chowns /mitmproxy-public and publishes the CA cert there, and master's healthcheck gates on that cert. After rebase, mitmproxy never becomes healthy and wg-client never starts. The PR currently conflicts with master in five files. Verified.
• Healthcheck budget is ~30s but startup can take minutes (compose-proxy.yml:71). netbird_verify_host_management_reachable alone is ~95s worst case (16 × curl --max-time 5 + sleeps), plus daemon wait (30s), wt0 wait (30s), and the addon's new _pass_cli_warmup and per-secret retries (3 × 60s each). Only wg-client got a start_period. The comment in main() says enrollment must not block the proxy, but it runs synchronously before exec.
• Digit-only or boolean-looking secrets are rejected (cli/lib/netbird.bash:56). yq -o json -r strips quotes, and netbird_flatten_secret_setting re-parses the text as YAML. A token of 0123456789 types as !!float, the flatten fails, and compose/run/attach/restart-proxy all abort. A value of null is silently treated as unset. Verified.
• Two existing pytest tests fail (test_mitmproxy_addon.py:1132, :1163). They assert timeout=30; the code now uses 60. The unpatched time.sleep in the new retry loop also adds ~20s to the suite. Clean at the merge base. Verified.
• Stale dns.conf removed too late (mitmproxy-init.sh:377). It is deleted after start_netbird instead of first. On restart with the persisted volume, the healthcheck passes on the previous run's file and wg-client reads a stale upstream.

Security (high)

• mitmweb is reachable from the whole mesh with the fixed password mitmproxy. enable_netbird puts wt0 in the mitmproxy netns; only OUTPUT rules are added. NetBird's default policy is all-to-all. Any peer can open port 8081 and read intercepted traffic, including resolved secrets. Add an INPUT drop on wt0 for 8081 and the WG port, and generate the password.
• Repo-controlled peer name triggers an authenticated DELETE (netbird-peer-lifecycle.sh:278-302). netbird_peer_name_proxy is read from the project's committed .sandcat/settings.json. Cloning a hostile repo and starting the stack deletes whatever peer that repo names from the operator's account. Derive the name from the project instead of trusting settings.
• PAT and setup key are readable from the agent container. netbird_api_token / netbird_enrollment_key are layered over .sandcat/settings*.json, and .sandcat is mounted into the agent. The README recommends this layering. Either drop the project layers for these keys or exclude them from the mount.

Important

• Peer FQDNs from the management server go into dnsmasq with only a substring match on the domain (mitmproxy-init.sh:281). A name containing / becomes a multi-domain address= line and hijacks a whole TLD inside the sandbox. Anchor the suffix and validate the charset.
• local=/<domain>/ (mitmproxy-init.sh:303) cancels the server=/<domain>/<ns> line at :272, so wildcard mesh resolution never works once any peer exists.
• Alias regex ^(.+)-[0-9]{1,3}-[0-9]{1,3}$ (mitmproxy-init.sh:289) strips two of four octets: x-100-64-0-5 yields alias x-100-64. Verified.
• mv "$tmp" "$profile_file" runs even when jq failed (netbird-peer-lifecycle.sh:179-200), leaving an empty default.json. set -e does not help because the call chain runs inside if start_netbird.
• Cloud enrollment inherits a stale self-hosted URL from global settings.json (mitmproxy-init.sh:338); the cloud path never clears it.
• find_repo_root uses $PWD, so sandcat init --path other --netbird reads the current directory's .sandcat/settings.json and bakes that server into the other project (cli/lib/netbird.bash:25).
• No trap in either script. exec gosu orphans the NetBird daemon and both supervisor loops under mitmweb, so docker stop never runs netbird down. supervise_netbird_daemon also respawns netbird service run without checking the previous one is alive, and re-enrolls every 10s with no backoff, each time fetching the full peer list and possibly deleting the peer.
• netbird up has no timeout (mitmproxy-init.sh:193, :231). Setup key and PAT are passed on argv to netbird and curl; use --setup-key-file and curl -H @-.
• netbird-peers.conf is never truncated when there are no peers and the cp is not atomic (mitmproxy-init.sh:308-315).
• The two-call retry in netbird_resolve_secret_ref (netbird-peer-lifecycle.sh:59-63) writes both attempts to stdout, so a partial first output corrupts the captured secret.
• restart_dnsmasq starts a second dnsmasq even if the old one never exited, and logs success regardless (wg-client-init.sh:158-175). Every peer change restarts dnsmasq, giving the agent up to ~10s of DNS outage.
• _volume_timestamp_epoch (cli/lib/volume.bash:13) only strips a trailing Z; a numeric offset from a non-UTC daemon fails on GNU date and is silently wrong on BSD date. Regression from master's date -d.
• "${COMPOSE_ARGS[@]}" under set -u fails on macOS bash 3.2 when empty (start.sh:149). The CLI already uses the ${a[@]+"${a[@]}"} guard elsewhere.
• start.sh writes generated server secrets into the tracked netbird-server.env with default umask.
• wg-client-init.sh re-does the full merge every 5s even when the mtime it computes is unchanged; a one-line early return fixes it.

Cleanup

• docs/examples/proxy-peer/scripts/proxy-peer-init.sh duplicates ~150 lines of mitmproxy-init.sh and has already drifted. Both source netbird-peer-lifecycle.sh, so the shared home exists. The four tests that grep both copies for the same string go away with it.
• ~200 lines of NetBird compose logic live in composefile.bash, which sources netbird.bash mid-function. init defines three functions inside init(). apply_secret_provider now branches on the mitmproxy dockerfile.
• The eight idempotency guards in enable_netbird are dead: the template is copied fresh immediately before.
• ~26 tests grep source text or assert line order (netbird_contract.bats, netbird_peer_lifecycle.bats:245-290, proxy_peer_compose.bats, wg-client/netbird_dns.bats:138-153). No test covers a failing GET or DELETE in the deregistration path. Several tests are duplicates (three idempotence tests, two --netbird-management-url tests, two --capability tests).
• Four unrelated changes ride along: the yq SIGSEGV retry in require.bash, BSD date in volume.bash, the pass-cli warmup, and the enabled flag parser (~35 lines for a boolean).
• Every attach/compose/run/restart-proxy now spawns ~10 yq processes even with NetBird disabled.
• docs/examples/netbird-server (426 lines) is not used by the CLI. docs/ is now a Sphinx source tree on master; runnable examples belong outside it.
• netbird_mgmt_delete_peer_by_name and apply_netbird_build_args's wg-client default are only used by their own tests. netbird.env says wg-client consumes it; the consumer is Dockerfile.mitmproxy.
• New bats files use spaces; .editorconfig mandates tabs.

Test run

In a worktree at the PR head: bats 389 passed, 5 failed (2 pre-existing on master, 3 in netbird_server_start.bats from SHELLOPTS leaking nounset into the pyenv shim). pytest 329 passed, 2 failed (above).