Conversation
- dialer_generic.go: Change FailedPrecondition to Internal, add docs for Linux root check - dialer.go: Preserve both native and WebSocket errors for diagnostics - dialer.go: Document timeout rationale (10s native, 30s WebSocket) - fallback.go: Remove codes.Internal, add comments explaining each error code - flow/client.go: Document why simpler fallback strategy is acceptable
- dialer_generic.go: Use fmt.Errorf for consistent error wrapping, remove unused imports - fallback.go: Simplify by removing manual unwrap loop (status.FromError handles it)
- --enable-stun requires non-empty --stun-ports - Ports must be in range 1-65535 - No duplicate ports allowed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@combined/cmd/root.go`:
- Around line 526-529: The code trusts X-Real-Ip / X-Real-Port unconditionally
when building connRemoteAddr (variables/functions: connRemoteAddr, r.RemoteAddr,
r.Header.Get("X-Real-Ip")/X-Real-Port); change it to only honor those headers
when the immediate peer (r.RemoteAddr) is a configured/trusted reverse proxy
(e.g., check membership against TrustedHTTPProxies or the existing
trust-checking helper used elsewhere), otherwise fall back to r.RemoteAddr;
preserve use of net.JoinHostPort to compose the address when headers are
accepted and silently ignore/validate malformed header values.
- Around line 305-311: Replace the fatal exit inside the healthcheck goroutine
so failures don't bypass shutdown: instead of calling log.Fatalf in the
goroutine that calls httpHealthcheck.ListenAndServe(), capture the error and
send it to a dedicated error channel (or call a provided cancel function) so the
main routine can initiate graceful shutdown; update the goroutine around
httpHealthcheck.ListenAndServe() to check errors.Is(err, http.ErrServerClosed)
and on any other error send that err over the channel (or log.Errorf+call
cancel) rather than invoking log.Fatalf so wg, management, relay, and STUN
shutdown logic can run.
🧹 Nitpick comments (2)
combined/cmd/config.go (1)
242-266: Edge case:exposedAddresswithout an explicit port.If a user sets
exposedAddress: "https://example.com"(no port),net.SplitHostPortwill fail andhostPortwill be"example.com". Auto-generated STUN URIs becomestun:example.com:3478(correct), but relay/signal URIs becomerels://example.comandhttps://example.com— which technically work using default ports but may confuse users or downstream parsers expecting explicit ports.Consider documenting that
exposedAddressshould include the port, or defaulting to:443/:80based on protocol when no port is present.combined/cmd/root.go (1)
222-226: Port extraction fromListenAddressis duplicated.The same
net.SplitHostPort+strconv.Atoipattern appears in bothcreateManagementServer(method, Line 222) andcreateManagementServer(function, Line 444). Consider extracting a helper likelistenPort(addr string) intto avoid drift.Also applies to: 444-449
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
| if err := httpHealthcheck.ListenAndServe(); !errors.Is(err, http.ErrServerClosed) { | ||
| log.Fatalf("failed to start healthcheck server: %v", err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
log.Fatalf in healthcheck goroutine will os.Exit(1) bypassing graceful shutdown.
If the healthcheck listener fails (e.g., port conflict), log.Fatalf kills the process immediately without running the shutdown sequence for management, relay, or STUN servers. Consider sending the error through a channel or using log.Errorf + cancellation instead.
🤖 Prompt for AI Agents
In `@combined/cmd/root.go` around lines 305 - 311, Replace the fatal exit inside
the healthcheck goroutine so failures don't bypass shutdown: instead of calling
log.Fatalf in the goroutine that calls httpHealthcheck.ListenAndServe(), capture
the error and send it to a dedicated error channel (or call a provided cancel
function) so the main routine can initiate graceful shutdown; update the
goroutine around httpHealthcheck.ListenAndServe() to check errors.Is(err,
http.ErrServerClosed) and on any other error send that err over the channel (or
log.Errorf+call cancel) rather than invoking log.Fatalf so wg, management,
relay, and STUN shutdown logic can run.
| connRemoteAddr := r.RemoteAddr | ||
| if r.Header.Get("X-Real-Ip") != "" && r.Header.Get("X-Real-Port") != "" { | ||
| connRemoteAddr = net.JoinHostPort(r.Header.Get("X-Real-Ip"), r.Header.Get("X-Real-Port")) | ||
| } |
There was a problem hiding this comment.
X-Real-Ip / X-Real-Port headers are trusted without validation against TrustedHTTPProxies.
These headers are used to construct the remote address for the relay connection. If the server is directly exposed (no reverse proxy), a client could spoof them. The impact is low since the relay authenticates via HMAC and these are primarily used for connection tracking/logging, but it's inconsistent with the management server's reverse proxy trust model.
🤖 Prompt for AI Agents
In `@combined/cmd/root.go` around lines 526 - 529, The code trusts X-Real-Ip /
X-Real-Port unconditionally when building connRemoteAddr (variables/functions:
connRemoteAddr, r.RemoteAddr, r.Header.Get("X-Real-Ip")/X-Real-Port); change it
to only honor those headers when the immediate peer (r.RemoteAddr) is a
configured/trusted reverse proxy (e.g., check membership against
TrustedHTTPProxies or the existing trust-checking helper used elsewhere),
otherwise fall back to r.RemoteAddr; preserve use of net.JoinHostPort to compose
the address when headers are accepted and silently ignore/validate malformed
header values.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@infrastructure_files/getting-started.sh`:
- Line 198: The docker-compose log commands use the wrong service name "netbird"
and should use the actual service name "netbird-server"; locate the calls to
$DOCKER_COMPOSE_COMMAND logs --tail=20 netbird (look for the
$DOCKER_COMPOSE_COMMAND invocation and the logs --tail=20 pattern) and change
the service argument to netbird-server in both occurrences (the one around the
health-check timeout and the other later at the second logs call) so
docker-compose prints the server logs instead of failing with "no such service".
🧹 Nitpick comments (1)
infrastructure_files/getting-started.sh (1)
800-802: Consider addingkeepaliveto thenetbird_serverupstream.The
netbird_dashboardupstream (line 797-798) haskeepalive 10for connection reuse, butnetbird_serverdoes not. For the API and OAuth2 routes (short-lived HTTP requests), a keepalive pool would reduce connection churn to the backend.Proposed fix
upstream netbird_server { server ${server_addr}; + keepalive 10; }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@combined/cmd/root.go`:
- Around line 631-650: The logEnvVars function currently masks values only when
the env var key contains "secret", "key", or "password" so NB_ vars containing
"token" are logged in cleartext; update the conditional in logEnvVars to also
check for "token" (case-insensitive) so keys like "NB_API_TOKEN" are masked by
maskSecret, ensuring you reference the logEnvVars function and the maskSecret
helper when making the change.
In `@signal/cmd/run.go`:
- Line 44: signalLetsencryptEmail is declared and exposed as the
--letsencrypt-email flag but never used; either thread it into the cert manager
creation or remove it. To fix, locate where encryption.CreateCertManager is
called (and any related cert manager setup in main/run), and add
signalLetsencryptEmail as an argument (and propagate it through to
CreateCertManager and its callers) so the LetsEncrypt email is passed through,
or if you prefer to drop the option, remove the signalLetsencryptEmail variable
and its flag binding for --letsencrypt-email to eliminate the dead flag.
🧹 Nitpick comments (2)
combined/cmd/root.go (1)
223-227: Silently ignoringstrconv.Atoierror on port string.If
portStris non-numeric (unlikely afterSplitHostPort, but possible with malformed config),mgmtPortsilently becomes0. The same pattern repeats at line 450. Consider returning an error or at least logging a warning.Suggested fix
- mgmtPort, _ := strconv.Atoi(portStr) + mgmtPort, err := strconv.Atoi(portStr) + if err != nil { + return fmt.Errorf("invalid port in listen address %q: %w", cfg.Server.ListenAddress, err) + }Apply the same pattern at line 450.
signal/cmd/run.go (1)
329-331: Both--letsencrypt-data-dirand deprecated--ssl-dirbind to the same variable.This is a clean deprecation approach. However, both flags have an empty string default. If a user enables Let's Encrypt (
--letsencrypt-domain) but forgets to specify a data directory,encryption.CreateCertManager("")will attempt to usefilepath.Join("", "letsencrypt")as the cert cache directory. Consider validating that the data dir is non-empty when Let's Encrypt is enabled.#!/bin/bash # Check what happens when CreateCertManager receives an empty datadir rg -n 'func CreateCertManager' --type=go -A 15
| // logEnvVars logs all NB_ environment variables that are currently set | ||
| func logEnvVars() { | ||
| log.Info("=== Environment Variables ===") | ||
| found := false | ||
| for _, env := range os.Environ() { | ||
| if strings.HasPrefix(env, "NB_") { | ||
| key, _, _ := strings.Cut(env, "=") | ||
| value := os.Getenv(key) | ||
| if strings.Contains(strings.ToLower(key), "secret") || strings.Contains(strings.ToLower(key), "key") || strings.Contains(strings.ToLower(key), "password") { | ||
| value = maskSecret(value) | ||
| } | ||
| log.Infof(" %s=%s", key, value) | ||
| found = true | ||
| } | ||
| } | ||
| if !found { | ||
| log.Info(" (none set)") | ||
| } | ||
| log.Info("=== End Environment Variables ===") | ||
| } |
There was a problem hiding this comment.
Environment variable masking misses common sensitive key patterns like "token".
The mask check on line 639 covers "secret", "key", and "password", but NB_ prefixed env vars containing "token" (e.g., API tokens) would be logged in cleartext.
Suggested fix
- if strings.Contains(strings.ToLower(key), "secret") || strings.Contains(strings.ToLower(key), "key") || strings.Contains(strings.ToLower(key), "password") {
+ if strings.Contains(strings.ToLower(key), "secret") || strings.Contains(strings.ToLower(key), "key") || strings.Contains(strings.ToLower(key), "password") || strings.Contains(strings.ToLower(key), "token") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // logEnvVars logs all NB_ environment variables that are currently set | |
| func logEnvVars() { | |
| log.Info("=== Environment Variables ===") | |
| found := false | |
| for _, env := range os.Environ() { | |
| if strings.HasPrefix(env, "NB_") { | |
| key, _, _ := strings.Cut(env, "=") | |
| value := os.Getenv(key) | |
| if strings.Contains(strings.ToLower(key), "secret") || strings.Contains(strings.ToLower(key), "key") || strings.Contains(strings.ToLower(key), "password") { | |
| value = maskSecret(value) | |
| } | |
| log.Infof(" %s=%s", key, value) | |
| found = true | |
| } | |
| } | |
| if !found { | |
| log.Info(" (none set)") | |
| } | |
| log.Info("=== End Environment Variables ===") | |
| } | |
| // logEnvVars logs all NB_ environment variables that are currently set | |
| func logEnvVars() { | |
| log.Info("=== Environment Variables ===") | |
| found := false | |
| for _, env := range os.Environ() { | |
| if strings.HasPrefix(env, "NB_") { | |
| key, _, _ := strings.Cut(env, "=") | |
| value := os.Getenv(key) | |
| if strings.Contains(strings.ToLower(key), "secret") || strings.Contains(strings.ToLower(key), "key") || strings.Contains(strings.ToLower(key), "password") || strings.Contains(strings.ToLower(key), "token") { | |
| value = maskSecret(value) | |
| } | |
| log.Infof(" %s=%s", key, value) | |
| found = true | |
| } | |
| } | |
| if !found { | |
| log.Info(" (none set)") | |
| } | |
| log.Info("=== End Environment Variables ===") | |
| } |
🤖 Prompt for AI Agents
In `@combined/cmd/root.go` around lines 631 - 650, The logEnvVars function
currently masks values only when the env var key contains "secret", "key", or
"password" so NB_ vars containing "token" are logged in cleartext; update the
conditional in logEnvVars to also check for "token" (case-insensitive) so keys
like "NB_API_TOKEN" are masked by maskSecret, ensuring you reference the
logEnvVars function and the maskSecret helper when making the change.
| signalPort int | ||
| metricsPort int | ||
| signalLetsencryptDomain string | ||
| signalLetsencryptEmail string |
There was a problem hiding this comment.
signalLetsencryptEmail is declared and has a CLI flag but is never used.
The variable is bound to --letsencrypt-email (line 332) but is never passed to encryption.CreateCertManager or any other function. Either wire it into the cert manager flow or remove the dead flag to avoid confusing users who set it expecting it to take effect.
#!/bin/bash
# Verify signalLetsencryptEmail is truly unused
rg -n 'signalLetsencryptEmail' --type=goAlso applies to: 218-219, 332-332
🤖 Prompt for AI Agents
In `@signal/cmd/run.go` at line 44, signalLetsencryptEmail is declared and exposed
as the --letsencrypt-email flag but never used; either thread it into the cert
manager creation or remove it. To fix, locate where encryption.CreateCertManager
is called (and any related cert manager setup in main/run), and add
signalLetsencryptEmail as an argument (and propagate it through to
CreateCertManager and its callers) so the LetsEncrypt email is passed through,
or if you prefer to drop the option, remove the signalLetsencryptEmail variable
and its flag binding for --letsencrypt-email to eliminate the dead flag.
|
4 participants
Describe your changes
Add a combined version of NetBird's control plane.
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit