Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds a PreSharedKey configuration option and threads it from the CLI into the Server and NetBird client configuration via a new ClientConfig struct and updated NewNetBird signature; tests and call sites adjusted to the new parameter shape. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (root.go)
participant Server as Server (proxy/server.go)
participant RT as roundtrip.NewNetBird
participant NB as NetBird (client)
participant MGMT as Management API
CLI->>Server: start with --pre-shared-key
Server->>RT: NewNetBird(proxyID, proxyAddr, ClientConfig{MgmtAddr, WGPort, PreSharedKey}, ...)
RT->>NB: init NetBird with clientCfg (includes PreSharedKey)
NB->>MGMT: use MgmtAddr / WGPort / PreSharedKey when creating embedded client
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@proxy/cmd/proxy/cmd/root.go`:
- Line 88: Remove the CLI flag registration that exposes the PSK
(rootCmd.Flags().StringVar(&preSharedKey, "pre-shared-key", ...)) and stop using
the package-level preSharedKey variable; instead read the pre-shared key
exclusively from the environment using
envStringOrDefault("NB_PROXY_PRE_SHARED_KEY", "") at runtime (same pattern used
for ProxyToken) and use that value inside runServer and any other consumers;
also delete the package-level preSharedKey declaration so there is no flag or
global var exposing the secret.
| rootCmd.Flags().StringVar(&certLockMethod, "cert-lock-method", envStringOrDefault("NB_PROXY_CERT_LOCK_METHOD", "auto"), "Certificate lock method for cross-replica coordination: auto, flock, or k8s-lease") | ||
| rootCmd.Flags().IntVar(&wgPort, "wg-port", envIntOrDefault("NB_PROXY_WG_PORT", 0), "WireGuard listen port (0 = random). Fixed port only works with single-account deployments") | ||
| rootCmd.Flags().BoolVar(&proxyProtocol, "proxy-protocol", envBoolOrDefault("NB_PROXY_PROXY_PROTOCOL", false), "Enable PROXY protocol on TCP listeners to preserve client IPs behind L4 proxies") | ||
| rootCmd.Flags().StringVar(&preSharedKey, "pre-shared-key", envStringOrDefault("NB_PROXY_PRE_SHARED_KEY", ""), "Define a pre-shared key for the tunnel between proxy and peers") |
There was a problem hiding this comment.
PSK exposed in process listing via CLI flag.
WireGuard's own tooling (wg(8)) explicitly states that "command line arguments are not considered private on most systems" for pre-shared keys — they are visible to other processes via ps aux and /proc/<pid>/cmdline.
The existing ProxyToken deliberately has no CLI flag (lines 109–112) and is env-var-only precisely to avoid this leak. The WireGuard PSK is a cryptographic secret and warrants the same treatment. NB_PROXY_PRE_SHARED_KEY is already available as a safe alternative.
Consider removing the --pre-shared-key flag entirely and reading the PSK exclusively from the environment variable, mirroring the ProxyToken pattern:
🔒 Proposed fix: env-var-only PSK
- rootCmd.Flags().StringVar(&preSharedKey, "pre-shared-key", envStringOrDefault("NB_PROXY_PRE_SHARED_KEY", ""), "Define a pre-shared key for the tunnel between proxy and peers")In runServer:
+ preSharedKey := os.Getenv("NB_PROXY_PRE_SHARED_KEY")
srv := proxy.Server{And remove the package-level preSharedKey variable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@proxy/cmd/proxy/cmd/root.go` at line 88, Remove the CLI flag registration
that exposes the PSK (rootCmd.Flags().StringVar(&preSharedKey, "pre-shared-key",
...)) and stop using the package-level preSharedKey variable; instead read the
pre-shared key exclusively from the environment using
envStringOrDefault("NB_PROXY_PRE_SHARED_KEY", "") at runtime (same pattern used
for ProxyToken) and use that value inside runServer and any other consumers;
also delete the package-level preSharedKey declaration so there is no flag or
global var exposing the secret.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
proxy/server.go (1)
117-118: Consider supporting env-var or file-based sourcing forPreSharedKey.A WireGuard PSK passed via a
--pre-shared-keyCLI flag will be visible in process listings (ps aux) and may persist in shell history. The standard mitigation is to accept the secret via an environment variable or a file path (e.g.,--pre-shared-key-file). If the CLI flag inroot.gocurrently uses a plain--flagbinding, this is worth addressing before the feature ships.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/server.go` around lines 117 - 118, PreSharedKey is currently exposed via a CLI flag which can leak in process listings; update the flag handling (where --pre-shared-key is bound in root.go) and the PreSharedKey field usage to support secure sourcing: add a new --pre-shared-key-file flag and an environment variable (e.g., PRE_SHARED_KEY) and implement a loader function (e.g., loadPreSharedKey) that prioritizes file contents if --pre-shared-key-file is set, otherwise reads PRE_SHARED_KEY from the environment, and only falls back to the CLI --pre-shared-key value; ensure the loader reads the file securely, trims whitespace, avoids logging the secret, and that PreSharedKey is populated from this loader instead of directly from the plain flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@proxy/server.go`:
- Around line 117-118: PreSharedKey is currently exposed via a CLI flag which
can leak in process listings; update the flag handling (where --pre-shared-key
is bound in root.go) and the PreSharedKey field usage to support secure
sourcing: add a new --pre-shared-key-file flag and an environment variable
(e.g., PRE_SHARED_KEY) and implement a loader function (e.g., loadPreSharedKey)
that prioritizes file contents if --pre-shared-key-file is set, otherwise reads
PRE_SHARED_KEY from the environment, and only falls back to the CLI
--pre-shared-key value; ensure the loader reads the file securely, trims
whitespace, avoids logging the secret, and that PreSharedKey is populated from
this loader instead of directly from the plain flag.
|
1 participant
Describe your changes
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