[client] Add client-side support for mTLS.#5416
Open
reimarstier wants to merge 2 commits intonetbirdio:mainfrom
Open
[client] Add client-side support for mTLS.#5416reimarstier wants to merge 2 commits intonetbirdio:mainfrom
reimarstier wants to merge 2 commits intonetbirdio:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThreads an optional management client TLS certificate through configuration, mgmt/signal/relay client constructors, gRPC/WebSocket dialers, and relay manager to optionally enable mTLS when a certificate is provided. Changes
Sequence Diagram(s)sequenceDiagram
participant ProfileMgr as ProfileManager
participant Engine as Client Engine
participant RelayMgr as Relay Manager/Picker
participant Dialer as nbgrpc/WS Dialer
participant Server as Management/Signal Server
ProfileMgr->>Engine: provide MgmtClientCertKeyPair (*tls.Certificate)
Engine->>RelayMgr: NewManager / NewClient (pass clientCert)
RelayMgr->>Dialer: CreateConnection / Dial (addr, tlsEnabled, component, mgmtClientCert)
Note right of Dialer: build tls.Config with RootCAs\nattach mgmtClientCert if non-nil
Dialer->>Server: establish TLS / mTLS connection
Server-->>Engine: TLS handshake / gRPC or WS established
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
shared/signal/client/grpc.go (1)
57-62: Consider renamingmgmtClientCerttoclientCertin the signal client.The parameter is named
mgmtClientCerthere, but this is the signal client — the certificate serves the same mTLS purpose for signal connections, not management specifically. Using a generic name likeclientCert(as done in the relay client) would be clearer and more consistent.The same naming concern applies to
CreateConnectioninclient/grpc/dialer.gowhere the parameter is also calledmgmtClientCert.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/signal/client/grpc.go` around lines 57 - 62, Rename the parameter mgmtClientCert to clientCert for clarity and consistency: update NewClient(ctx context.Context, addr string, key wgtypes.Key, tlsEnabled bool, mgmtClientCert *tls.Certificate) to use clientCert and change all usages within NewClient (including the nbgrpc.CreateConnection call) to pass clientCert; likewise update the CreateConnection signature and its implementation in the dialer from mgmtClientCert to clientCert and adjust any local variable names, documentation/comments, and call sites to match the new identifier (e.g., references in nbgrpc.CreateConnection, dialer.go, and other callers).client/internal/profilemanager/config.go (2)
573-591: Consider warning when only one of cert/key path is provided, and failing hard on load errors.Two observations:
If a user sets
MgmtClientCertPathbut notMgmtClientKeyPath(or vice versa), nothing happens and there's no warning — the incomplete configuration is silently ignored. A log warning would help troubleshoot misconfiguration.When
tls.LoadX509KeyPairfails (line 584-586), the error is only logged and execution continues. The client will then silently connect without mTLS, which may violate the user's security expectations. Consider returning an error fromapply()in this case, or at minimum logging atWarnlevel instead ofError(since the currentErrorlog suggests something is wrong but doesn't prevent the insecure fallback).I recognize both patterns are consistent with the existing
ClientCertPathhandling above (lines 563-571), so this could be addressed as a follow-up for both cases.Proposed improvement
+ if (input.MgmtClientCertPath != "") != (input.MgmtClientKeyPath != "") { + log.Warnf("only one of MgmtClientCertPath/MgmtClientKeyPath is set; both are required for mTLS") + } + if config.MgmtClientCertPath != "" && config.MgmtClientKeyPath != "" { cert, err := tls.LoadX509KeyPair(config.MgmtClientCertPath, config.MgmtClientKeyPath) if err != nil { - log.Error("Failed to load mTLS cert/key pair for management: ", err) + return false, fmt.Errorf("failed to load mTLS cert/key pair for management: %w", err) } else { config.MgmtClientCertKeyPair = &cert log.Info("Loaded client mTLS cert/key pair for management.") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/profilemanager/config.go` around lines 573 - 591, Detect and warn on incomplete mTLS config and fail on load errors: when only one of config.MgmtClientCertPath or config.MgmtClientKeyPath is set, emit a warning log (e.g., log.Warn) so the user knows the config is incomplete; when both are set and tls.LoadX509KeyPair(config.MgmtClientCertPath, config.MgmtClientKeyPath) returns an error, do not silently continue — return that error from apply() (or the enclosing function) instead of merely logging, and only set config.MgmtClientCertKeyPair on success; apply the same warning/return-on-load-error pattern used for ClientCertPath/ClientKeyPath handling above so behavior is consistent.
163-166: Verify thatMgmtClientCertPathandMgmtClientKeyPathare not logged or written to sensitive locations.The certificate and key file paths will be persisted to the JSON config file (only the
MgmtClientCertKeyPairfield hasjson:"-"). This is fine for paths, but worth confirming that the key file itself has appropriate permissions. Consider documenting the expected file permissions for the key file (e.g., 0600).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/profilemanager/config.go` around lines 163 - 166, The MgmtClientCertPath and MgmtClientKeyPath fields must not be logged or written to insecure places and the key file itself must have restrictive permissions; add a comment beside MgmtClientKeyPath documenting the required permission (e.g., 0600) and update the config load/validate path (where the struct is parsed or saved) to (1) avoid emitting these values to logs or any exported JSON (keep MgmtClientCertKeyPair json:"-"), (2) validate that the file at MgmtClientKeyPath exists and has mode 0600 (or stricter) and return an error otherwise, and (3) ensure any config serialization/Save function explicitly omits or redacts the key path when writing to shared locations or logs; reference the MgmtClientCertPath, MgmtClientKeyPath, and MgmtClientCertKeyPair fields when making these checks and updates.client/grpc/dialer.go (1)
46-51: Log message says "management" but this function serves both management and signal connections.
CreateConnectionis called for both management (viawsproxy.ManagementComponent) and signal (viawsproxy.SignalComponent) connections. The log messages on lines 47 and 50 reference "management" specifically, which would be misleading when establishing a signal connection.Consider using the
componentparameter in the log message for clarity:♻️ Suggested fix
if mgmtClientCert != nil { - log.Debugf("Using client certificate for management connection. mTLS enabled.") + log.Debugf("Using client certificate for %s connection. mTLS enabled.", component) tlsConfig.Certificates = []tls.Certificate{*mgmtClientCert} } else { - log.Debugf("No client certificate for management provided. Disabling mTLS.") + log.Debugf("No client certificate provided for %s connection. mTLS disabled.", component) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/grpc/dialer.go` around lines 46 - 51, The log messages in CreateConnection hardcode "management" but this function is used for both management and signal connections; update the Debugf calls that reference "management" (around the mgmtClientCert and tlsConfig handling) to include the function's component parameter (e.g., component or wsproxy.ManagementComponent/wsproxy.SignalComponent) so the logs reflect which connection is being established; ensure both the "Using client certificate..." and "No client certificate..." messages incorporate the component variable for accurate, non-misleading logging.client/internal/connect.go (1)
219-219: Consider renaming config fields to reflect use across all backends, not just management.The configuration field
MgmtClientCertKeyPairis intentionally passed to all three backends—management (line 219), signal (line 268), and relay (line 294)—as confirmed by the config documentation stating: "Path to client certificate used for mTLS authentication that is used to connect to management/signal/relay backend."However, the field name and parameter names (
mgmtClientCert) across signal and management clients are misleading since the certificate serves all three services, not just management. This inconsistency is compounded by relay using a genericClientCertnaming.Renaming to
ClientCertKeyPair(without the "Mgmt" prefix) in the config and updating corresponding parameter names would improve clarity without changing functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/connect.go` at line 219, Rename the config field and parameter names that imply "Mgmt" to a generic name used by all backends: change MgmtClientCertKeyPair to ClientCertKeyPair in the config struct and docs, update any references and constructor parameters (e.g., the mgm.NewClient call that currently passes c.config.MgmtClientCertKeyPair, the signal client constructor that accepts mgmtClientCert, and the relay client usage of ClientCert) to use the new ClientCertKeyPair name, and rename parameter identifiers (e.g., mgmtClientCert -> clientCert) in function signatures for mgm.NewClient, signal.NewClient and relay.NewClient so names consistently reflect the shared client certificate across management/signal/relay backends.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shared/relay/client/dialers_generic.go`:
- Around line 16-18: The QUIC dialer isn't given the mTLS client certificate so
QUIC handshakes fail when mTLS is required; update the quic.Dialer type to
accept a ClientCert (mirror ws.Dialer.ClientCert), set quic.Dialer{ClientCert:
c.clientCert} where the race dialer is returned, and ensure the
ClientQUICTLSConfig (or the TLS config builder used by quic.Dialer) uses that
ClientCert when building the tls.Config so QUIC supports mTLS; alternatively, if
you prefer not to change quic.Dialer, explicitly document that mTLS forces
WS-only transport and keep the existing behavior.
---
Nitpick comments:
In `@client/grpc/dialer.go`:
- Around line 46-51: The log messages in CreateConnection hardcode "management"
but this function is used for both management and signal connections; update the
Debugf calls that reference "management" (around the mgmtClientCert and
tlsConfig handling) to include the function's component parameter (e.g.,
component or wsproxy.ManagementComponent/wsproxy.SignalComponent) so the logs
reflect which connection is being established; ensure both the "Using client
certificate..." and "No client certificate..." messages incorporate the
component variable for accurate, non-misleading logging.
In `@client/internal/connect.go`:
- Line 219: Rename the config field and parameter names that imply "Mgmt" to a
generic name used by all backends: change MgmtClientCertKeyPair to
ClientCertKeyPair in the config struct and docs, update any references and
constructor parameters (e.g., the mgm.NewClient call that currently passes
c.config.MgmtClientCertKeyPair, the signal client constructor that accepts
mgmtClientCert, and the relay client usage of ClientCert) to use the new
ClientCertKeyPair name, and rename parameter identifiers (e.g., mgmtClientCert
-> clientCert) in function signatures for mgm.NewClient, signal.NewClient and
relay.NewClient so names consistently reflect the shared client certificate
across management/signal/relay backends.
In `@client/internal/profilemanager/config.go`:
- Around line 573-591: Detect and warn on incomplete mTLS config and fail on
load errors: when only one of config.MgmtClientCertPath or
config.MgmtClientKeyPath is set, emit a warning log (e.g., log.Warn) so the user
knows the config is incomplete; when both are set and
tls.LoadX509KeyPair(config.MgmtClientCertPath, config.MgmtClientKeyPath) returns
an error, do not silently continue — return that error from apply() (or the
enclosing function) instead of merely logging, and only set
config.MgmtClientCertKeyPair on success; apply the same
warning/return-on-load-error pattern used for ClientCertPath/ClientKeyPath
handling above so behavior is consistent.
- Around line 163-166: The MgmtClientCertPath and MgmtClientKeyPath fields must
not be logged or written to insecure places and the key file itself must have
restrictive permissions; add a comment beside MgmtClientKeyPath documenting the
required permission (e.g., 0600) and update the config load/validate path (where
the struct is parsed or saved) to (1) avoid emitting these values to logs or any
exported JSON (keep MgmtClientCertKeyPair json:"-"), (2) validate that the file
at MgmtClientKeyPath exists and has mode 0600 (or stricter) and return an error
otherwise, and (3) ensure any config serialization/Save function explicitly
omits or redacts the key path when writing to shared locations or logs;
reference the MgmtClientCertPath, MgmtClientKeyPath, and MgmtClientCertKeyPair
fields when making these checks and updates.
In `@shared/signal/client/grpc.go`:
- Around line 57-62: Rename the parameter mgmtClientCert to clientCert for
clarity and consistency: update NewClient(ctx context.Context, addr string, key
wgtypes.Key, tlsEnabled bool, mgmtClientCert *tls.Certificate) to use clientCert
and change all usages within NewClient (including the nbgrpc.CreateConnection
call) to pass clientCert; likewise update the CreateConnection signature and its
implementation in the dialer from mgmtClientCert to clientCert and adjust any
local variable names, documentation/comments, and call sites to match the new
identifier (e.g., references in nbgrpc.CreateConnection, dialer.go, and other
callers).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
client/grpc/dialer.goclient/internal/auth/auth.goclient/internal/connect.goclient/internal/engine_test.goclient/internal/profilemanager/config.goclient/server/server.gorelay/test/benchmark_test.gorelay/testec2/relay.goshared/management/client/client_test.goshared/management/client/grpc.goshared/relay/client/client.goshared/relay/client/client_test.goshared/relay/client/dialer/ws/dialopts_generic.goshared/relay/client/dialer/ws/dialopts_js.goshared/relay/client/dialer/ws/ws.goshared/relay/client/dialers_generic.goshared/relay/client/dialers_js.goshared/relay/client/manager.goshared/relay/client/manager_test.goshared/relay/client/picker.goshared/signal/client/grpc.go
reimarstier
force-pushed
the
424-mtls-for-netbird-client
branch
from
657db57 to
84c8c85
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/grpc/dialer.go (1)
45-51: Log message is overly specific for a generic function.The
CreateConnectionfunction is used for multiple components (management, signal) as indicated by thecomponentparameter, but the log messages specifically reference "management connection" and "management provided". This could cause confusion when reviewing logs for signal connections.Consider making the log messages generic or including the component name:
♻️ Suggested improvement
// Only add client certificate if provided if mgmtClientCert != nil { - log.Debugf("Using client certificate for management connection. mTLS enabled.") + log.Debugf("Using client certificate for %s connection. mTLS enabled.", component) tlsConfig.Certificates = []tls.Certificate{*mgmtClientCert} } else { - log.Debugf("No client certificate for management provided. Disabling mTLS.") + log.Debugf("No client certificate provided for %s. mTLS disabled.", component) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/grpc/dialer.go` around lines 45 - 51, The log messages in CreateConnection are too specific to "management" while the function is generic; update the log.Debugf calls that reference "management connection" and "management provided" to either use the component parameter (e.g., include component name) or make them generic (e.g., "Using client certificate for connection" / "No client certificate provided, disabling mTLS"), leaving the existing behavior that sets tlsConfig.Certificates from mgmtClientCert unchanged; modify the calls where mgmtClientCert is checked and where tlsConfig.Certificates is set so log messages reference component or are generic, and ensure you adjust both debug branches consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/internal/profilemanager/config.go`:
- Around line 573-591: The current block that attempts to load the management
mTLS keypair (checking config.MgmtClientCertPath and config.MgmtClientKeyPath
and calling tls.LoadX509KeyPair) only logs errors and continues; change it to
fail fast by returning an error when tls.LoadX509KeyPair returns an error, and
ensure you clear any in-memory config.MgmtClientCertKeyPair (set it to nil)
before returning so no unusable keypair remains; update the surrounding function
signature to propagate the error up (return the error from the function that
handles config updates) and on success keep the existing assignment of
config.MgmtClientCertKeyPair and the info log.
---
Nitpick comments:
In `@client/grpc/dialer.go`:
- Around line 45-51: The log messages in CreateConnection are too specific to
"management" while the function is generic; update the log.Debugf calls that
reference "management connection" and "management provided" to either use the
component parameter (e.g., include component name) or make them generic (e.g.,
"Using client certificate for connection" / "No client certificate provided,
disabling mTLS"), leaving the existing behavior that sets tlsConfig.Certificates
from mgmtClientCert unchanged; modify the calls where mgmtClientCert is checked
and where tlsConfig.Certificates is set so log messages reference component or
are generic, and ensure you adjust both debug branches consistently.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
client/grpc/dialer.goclient/internal/auth/auth.goclient/internal/connect.goclient/internal/engine_test.goclient/internal/profilemanager/config.goclient/server/server.gorelay/test/benchmark_test.gorelay/testec2/relay.goshared/management/client/client_test.goshared/management/client/grpc.goshared/relay/client/client.goshared/relay/client/client_test.goshared/relay/client/dialer/ws/dialopts_generic.goshared/relay/client/dialer/ws/dialopts_js.goshared/relay/client/dialer/ws/ws.goshared/relay/client/dialers_generic.goshared/relay/client/dialers_js.goshared/relay/client/manager.goshared/relay/client/manager_test.goshared/relay/client/picker.goshared/signal/client/grpc.go
🚧 Files skipped from review as they are similar to previous changes (5)
- client/server/server.go
- shared/relay/client/client_test.go
- shared/signal/client/grpc.go
- shared/relay/client/manager_test.go
- shared/relay/client/client.go
reimarstier
force-pushed
the
424-mtls-for-netbird-client
branch
from
84c8c85 to
c192683
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
client/internal/profilemanager/config.go (1)
573-581: Consider skipping updates when management cert/key paths are unchanged.
updatedis set to true whenever non-empty input is present, even if the value is identical. This can trigger unnecessary config rewrites and cert reloads.Suggested refinement
- if input.MgmtClientKeyPath != "" { + if input.MgmtClientKeyPath != "" && input.MgmtClientKeyPath != config.MgmtClientKeyPath { config.MgmtClientKeyPath = input.MgmtClientKeyPath updated = true } - if input.MgmtClientCertPath != "" { + if input.MgmtClientCertPath != "" && input.MgmtClientCertPath != config.MgmtClientCertPath { config.MgmtClientCertPath = input.MgmtClientCertPath updated = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/profilemanager/config.go` around lines 573 - 581, The code unconditionally sets updated=true when input.MgmtClientKeyPath or input.MgmtClientCertPath are non-empty, causing needless rewrites; change the logic in the block that touches config.MgmtClientKeyPath and config.MgmtClientCertPath to only assign and set updated=true when the incoming value differs from the existing one (compare input.MgmtClientKeyPath != config.MgmtClientKeyPath and input.MgmtClientCertPath != config.MgmtClientCertPath before assigning). Ensure you use the same variable names (input.MgmtClientKeyPath, config.MgmtClientKeyPath, input.MgmtClientCertPath, config.MgmtClientCertPath, updated) so unchanged paths do not flip the updated flag.shared/relay/client/client_test.go (1)
71-71: Add one positive mTLS test case.These updates validate only the
nilcertificate path. Please add at least one test that passes a non-nil*tls.Certificateand verifies cert plumbing is actually exercised.Also applies to: 79-79, 87-87, 147-147, 187-187, 230-230, 242-242, 322-322, 370-370, 376-376, 398-398, 473-473, 479-479, 537-537, 543-543, 593-593, 651-651, 704-704, 716-716
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/relay/client/client_test.go` at line 71, Add a positive mTLS test that passes a real non-nil *tls.Certificate into NewClient to exercise cert plumbing: generate or load a test cert/key pair (e.g., tls.X509KeyPair with PEM blobs or a helper that creates a self-signed cert), pass &tlsCert into NewClient (where code currently calls NewClient(..., nil)), perform an actual dial/handshake against the test server (reuse serverCfg/StartTestServer or the existing test harness), then assert the server observed the client certificate (e.g., check server side ConnectionState.PeerCertificates or a stubbed TLSConfig.GetConfigForClient/GetCertificate was invoked) and that the client connection succeeded; apply this change to the NewClient calls referenced in the test (the instances around the given lines) so each nil-certificate case has at least one corresponding non-nil-certificate positive test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/grpc/dialer.go`:
- Around line 47-50: The debug messages in dialer.go hardcode "management
connection" — update them to be component-agnostic by using a neutral phrase
(e.g., "connection" or include a passed-in connectionName/connType variable if
available) so the logs apply to all dialer usages; specifically change the two
log.Debugf calls near tlsConfig.Certificates and the else branch that reference
"management" to use the generic wording, keeping the existing behavior that sets
tlsConfig.Certificates = []tls.Certificate{*mgmtClientCert} when mgmtClientCert
is present.
---
Nitpick comments:
In `@client/internal/profilemanager/config.go`:
- Around line 573-581: The code unconditionally sets updated=true when
input.MgmtClientKeyPath or input.MgmtClientCertPath are non-empty, causing
needless rewrites; change the logic in the block that touches
config.MgmtClientKeyPath and config.MgmtClientCertPath to only assign and set
updated=true when the incoming value differs from the existing one (compare
input.MgmtClientKeyPath != config.MgmtClientKeyPath and input.MgmtClientCertPath
!= config.MgmtClientCertPath before assigning). Ensure you use the same variable
names (input.MgmtClientKeyPath, config.MgmtClientKeyPath,
input.MgmtClientCertPath, config.MgmtClientCertPath, updated) so unchanged paths
do not flip the updated flag.
In `@shared/relay/client/client_test.go`:
- Line 71: Add a positive mTLS test that passes a real non-nil *tls.Certificate
into NewClient to exercise cert plumbing: generate or load a test cert/key pair
(e.g., tls.X509KeyPair with PEM blobs or a helper that creates a self-signed
cert), pass &tlsCert into NewClient (where code currently calls NewClient(...,
nil)), perform an actual dial/handshake against the test server (reuse
serverCfg/StartTestServer or the existing test harness), then assert the server
observed the client certificate (e.g., check server side
ConnectionState.PeerCertificates or a stubbed
TLSConfig.GetConfigForClient/GetCertificate was invoked) and that the client
connection succeeded; apply this change to the NewClient calls referenced in the
test (the instances around the given lines) so each nil-certificate case has at
least one corresponding non-nil-certificate positive test.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
client/grpc/dialer.goclient/internal/auth/auth.goclient/internal/connect.goclient/internal/engine_test.goclient/internal/profilemanager/config.goclient/server/server.gorelay/test/benchmark_test.gorelay/testec2/relay.goshared/management/client/client_test.goshared/management/client/grpc.goshared/relay/client/client.goshared/relay/client/client_test.goshared/relay/client/dialer/ws/dialopts_generic.goshared/relay/client/dialer/ws/dialopts_js.goshared/relay/client/dialer/ws/ws.goshared/relay/client/dialers_generic.goshared/relay/client/dialers_js.goshared/relay/client/manager.goshared/relay/client/manager_test.goshared/relay/client/picker.goshared/signal/client/grpc.go
✅ Files skipped from review due to trivial changes (1)
- shared/relay/client/dialers_js.go
🚧 Files skipped from review as they are similar to previous changes (7)
- shared/relay/client/client.go
- shared/management/client/client_test.go
- shared/management/client/grpc.go
- client/internal/auth/auth.go
- relay/test/benchmark_test.go
- shared/relay/client/dialer/ws/dialopts_generic.go
- client/server/server.go
reimarstier
force-pushed
the
424-mtls-for-netbird-client
branch
from
c192683 to
f1615f2
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shared/relay/client/manager_test.go (1)
26-34: Consider adding test coverage for the mTLS path.All existing tests pass
nilfor the newclientCertparameter. Consider adding at least one test that exercises the mTLS code path with a non-nil certificate to verify the feature works end-to-end through the manager layer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/relay/client/manager_test.go` around lines 26 - 34, Add a new unit test (or extend TestEmptyURL) that exercises the mTLS path by passing a non-nil clientCert into NewManager so the code path that handles client certificates is exercised; create a test TLS certificate using tls.X509KeyPair or load a small test cert/key pair (e.g., from testdata) and pass that certificate as the clientCert argument to NewManager, then call Serve() and assert expected behavior (error or success) to verify the manager uses the certificate end-to-end through the Serve method.client/internal/profilemanager/config.go (1)
563-571: Consider aligning SSO certificate error handling with the new fail-fast pattern.The existing SSO client certificate loading (lines 563-571) only logs errors and continues, while the new management certificate loading (lines 583-595) fails fast on errors. This inconsistency could lead to silent failures for SSO mTLS.
Consider applying the same fail-fast pattern here for consistency:
♻️ Suggested refactor
- if config.ClientCertPath != "" && config.ClientCertKeyPath != "" { - cert, err := tls.LoadX509KeyPair(config.ClientCertPath, config.ClientCertKeyPath) - if err != nil { - log.Error("Failed to load mTLS cert/key pair: ", err) - } else { - config.ClientCertKeyPair = &cert - log.Info("Loaded client mTLS cert/key pair") - } - } + config.ClientCertKeyPair = nil + if (config.ClientCertPath == "") != (config.ClientCertKeyPath == "") { + return updated, fmt.Errorf("both ClientCertPath and ClientCertKeyPath must be set together") + } + if config.ClientCertPath != "" { + cert, err := tls.LoadX509KeyPair(config.ClientCertPath, config.ClientCertKeyPath) + if err != nil { + return updated, fmt.Errorf("failed to load SSO mTLS cert/key pair: %w", err) + } + config.ClientCertKeyPair = &cert + log.Info("Loaded client mTLS cert/key pair for SSO") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/profilemanager/config.go` around lines 563 - 571, The SSO mTLS block currently swallows tls.LoadX509KeyPair errors (using log.Error) whereas the management cert code fails fast; change the SSO block so that on error from tls.LoadX509KeyPair(config.ClientCertPath, config.ClientCertKeyPath) you fail fast in the same way the management cert handling does (i.e., return the error from the enclosing function or call the same fatal/logging path used for management certs) instead of just logging, and still set config.ClientCertKeyPair = &cert and log.Info("Loaded client mTLS cert/key pair") on success; update callers if needed to propagate the returned error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/internal/profilemanager/config.go`:
- Around line 563-571: The SSO mTLS block currently swallows tls.LoadX509KeyPair
errors (using log.Error) whereas the management cert code fails fast; change the
SSO block so that on error from tls.LoadX509KeyPair(config.ClientCertPath,
config.ClientCertKeyPath) you fail fast in the same way the management cert
handling does (i.e., return the error from the enclosing function or call the
same fatal/logging path used for management certs) instead of just logging, and
still set config.ClientCertKeyPair = &cert and log.Info("Loaded client mTLS
cert/key pair") on success; update callers if needed to propagate the returned
error.
In `@shared/relay/client/manager_test.go`:
- Around line 26-34: Add a new unit test (or extend TestEmptyURL) that exercises
the mTLS path by passing a non-nil clientCert into NewManager so the code path
that handles client certificates is exercised; create a test TLS certificate
using tls.X509KeyPair or load a small test cert/key pair (e.g., from testdata)
and pass that certificate as the clientCert argument to NewManager, then call
Serve() and assert expected behavior (error or success) to verify the manager
uses the certificate end-to-end through the Serve method.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
client/grpc/dialer.goclient/internal/auth/auth.goclient/internal/connect.goclient/internal/engine_test.goclient/internal/profilemanager/config.goclient/server/server.gorelay/test/benchmark_test.gorelay/testec2/relay.goshared/management/client/client_test.goshared/management/client/grpc.goshared/relay/client/client.goshared/relay/client/client_test.goshared/relay/client/dialer/ws/dialopts_generic.goshared/relay/client/dialer/ws/dialopts_js.goshared/relay/client/dialer/ws/ws.goshared/relay/client/dialers_generic.goshared/relay/client/dialers_js.goshared/relay/client/manager.goshared/relay/client/manager_test.goshared/relay/client/picker.goshared/signal/client/grpc.go
🚧 Files skipped from review as they are similar to previous changes (5)
- shared/relay/client/client.go
- shared/relay/client/manager.go
- relay/test/benchmark_test.go
- shared/signal/client/grpc.go
- shared/relay/client/dialer/ws/dialopts_js.go
reimarstier
force-pushed
the
424-mtls-for-netbird-client
branch
from
f1615f2 to
5897b4b
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/internal/engine_test.go (1)
1512-1517: Consider adding one positive-path test with a non-nil client certificate.Current updates validate API compatibility (
nilcert), but a single test that exercises non-nilcert plumbing would better protect the new mTLS path.Also applies to: 1548-1548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/engine_test.go` around lines 1512 - 1517, Add a positive-path unit test in engine_test.go that exercises the mTLS client-cert plumbing by calling mgmt.NewClient and signal.NewClient with a non-nil client certificate instead of nil; create or reuse a test TLS client certificate (e.g., a helper that returns *tls.Certificate or x509 cert + key), pass it as the certificate argument to mgmt.NewClient and signal.NewClient, assert no error is returned and the returned client is non-nil, and optionally verify that the client attempts an mTLS handshake (mock server/assert on dial options) to ensure the non-nil cert path is exercised; reference mgmt.NewClient and signal.NewClient when adding the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/internal/engine_test.go`:
- Around line 1512-1517: Add a positive-path unit test in engine_test.go that
exercises the mTLS client-cert plumbing by calling mgmt.NewClient and
signal.NewClient with a non-nil client certificate instead of nil; create or
reuse a test TLS client certificate (e.g., a helper that returns
*tls.Certificate or x509 cert + key), pass it as the certificate argument to
mgmt.NewClient and signal.NewClient, assert no error is returned and the
returned client is non-nil, and optionally verify that the client attempts an
mTLS handshake (mock server/assert on dial options) to ensure the non-nil cert
path is exercised; reference mgmt.NewClient and signal.NewClient when adding the
test.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
client/grpc/dialer.goclient/internal/auth/auth.goclient/internal/connect.goclient/internal/engine_test.goclient/internal/profilemanager/config.goclient/server/server.gorelay/test/benchmark_test.gorelay/testec2/relay.goshared/management/client/client_test.goshared/management/client/grpc.goshared/relay/client/client.goshared/relay/client/client_test.goshared/relay/client/dialer/ws/dialopts_generic.goshared/relay/client/dialer/ws/dialopts_js.goshared/relay/client/dialer/ws/ws.goshared/relay/client/dialers_generic.goshared/relay/client/dialers_js.goshared/relay/client/manager.goshared/relay/client/manager_test.goshared/relay/client/picker.goshared/signal/client/grpc.go
🚧 Files skipped from review as they are similar to previous changes (7)
- relay/testec2/relay.go
- shared/management/client/client_test.go
- shared/signal/client/grpc.go
- shared/management/client/grpc.go
- shared/relay/client/dialer/ws/ws.go
- client/internal/profilemanager/config.go
- shared/relay/client/manager_test.go
reimarstier
force-pushed
the
424-mtls-for-netbird-client
branch
2 times, most recently
from
184783a to
b56a472
Compare
reimarstier
force-pushed
the
424-mtls-for-netbird-client
branch
3 times, most recently
from
3b96023 to
a28441b
Compare
Author
|
Hi @mlsmaycon, I kindly asked for a review. Is there a maintainer group that I can tag? I'd like to have feedback on the following:
This renames
|
reimarstier
force-pushed
the
424-mtls-for-netbird-client
branch
from
a28441b to
5355884
Compare
reimarstier
force-pushed
the
424-mtls-for-netbird-client
branch
3 times, most recently
from
0d8429f to
8f67a91
Compare
Author
|
@lixmal Can you take another look? |
reimarstier
force-pushed
the
424-mtls-for-netbird-client
branch
from
8f67a91 to
5a437f4
Compare
reimarstier
force-pushed
the
424-mtls-for-netbird-client
branch
4 times, most recently
from
08d418d to
da1622b
Compare
Client supports sending certificate to management/signal/relay backend for mutual authentication.
reimarstier
force-pushed
the
424-mtls-for-netbird-client
branch
from
da1622b to
543b4b5
Compare
|
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
1 participant
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.
This PR adds client support for sending a certificate to management/signal/relay backend for mutual authentication (mTLS).
Background
We've been using NetBird in eclipse-opendut for over two years now and are grateful for the VPN management that it provides. For internet facing services, we are required to use client certificates (mutual TLS aka mTLS). At the moment we use NetBird behind traefik and are able to configure client certificate verification here.
Changes
I would like to add support for client certificates in the NetBird client.
This pull request adds two fields to the configuration input:
If you configure a certificate path and key path, then the NetBird client will send the client certificate when making requests to the backend (management service, relay service, signal service).
There are already configuration fields present to configure client certificates for the identity provider, contributed in #2188. To avoid breaking existing configuration, I've opted for a descriptive longer name in the configuration.
Let me know if this is to your liking.
I have already run tests for v0.67.1:
Apart from that I've tested integration within our backend. I haven't tested on other operating systems than linux x86_64.
There are others who have requested more general support for client certificates and a PKI: mTLS Auth for Proxy Services.
Adding client-side support is a first step in that direction.
Checklist
Legal notice:
Reimar Stier, reimar.stier@mercedes-benz.com, Mercedes-Benz Tech Innovation GmbH, provider information
Summary by CodeRabbit
New Features
Tests