[server] Add an option to disable cert verification for external client requests#5216
[server] Add an option to disable cert verification for external client requests#5216anudeepND wants to merge 4 commits intonetbirdio:mainfrom
Conversation
|
|
📝 WalkthroughWalkthroughThis PR centralizes HTTP client creation via a new util/http.go (NewTransport, NewHTTPClient), injects HTTP clients into geolocation and IDP components, and replaces direct net/http calls with the new clients across multiple modules. Function signatures for geolocation constructors and internal helpers were updated to accept *http.Client. Changes
Sequence Diagram(s)sequenceDiagram
participant Init as App Init
participant Geo as Geolocation Service
participant Util as util.NewHTTPClient / Transport
participant CDN as External Host (pkgs.netbird.io)
Init->>Util: NewTransport() / NewHTTPClient()
Init->>Geo: NewGeolocation(ctx,dataDir,autoUpdate, *http.Client)
Geo->>Util: client.Head/Get(database URL)
Util->>CDN: TCP/TLS request
CDN-->>Util: TLS response / file
Util-->>Geo: HTTP response
Geo-->>Init: geolocation ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/server/geolocation/geolocation.go (1)
3-8:⚠️ Potential issue | 🟡 MinorAdd nil guard for HTTP client to prevent panics.
If a caller passes nil, downstream calls to
client.Head()andclient.Get()will panic. Default tohttp.DefaultClientto keep the API safe.🛡️ Suggested safeguard
func NewGeolocation(ctx context.Context, dataDir string, autoUpdate bool, httpClient *http.Client) (Geolocation, error) { + if httpClient == nil { + httpClient = http.DefaultClient + } mmdbGlobPattern := filepath.Join(dataDir, mmdbPattern)
🧹 Nitpick comments (4)
util/http.go (2)
19-25: Set explicit TLS MinVersion when creating tls.Config.When
TLSClientConfigisniland you create an empty&tls.Config{}, you lose the default transport's TLS settings. While Go defaults to TLS 1.2 for clients, explicitly settingMinVersionensures consistent security posture and satisfies security scanners.Also consider checking case-insensitively (e.g.,
strings.EqualFold) for the environment variable value since users might setTRUEorTrue.🔒 Proposed fix
func NewTransport() *http.Transport { tr := http.DefaultTransport.(*http.Transport).Clone() - if os.Getenv(disableCertValidationKey) == "true" { + if strings.EqualFold(os.Getenv(disableCertValidationKey), "true") { log.Warnf("HTTP client certificate validation is disabled") if tr.TLSClientConfig == nil { - tr.TLSClientConfig = &tls.Config{} + tr.TLSClientConfig = &tls.Config{MinVersion: tls.VersionTLS12} } tr.TLSClientConfig.InsecureSkipVerify = true } return tr }You'll also need to add
"strings"to the imports.
20-20: Warning log fires on every call to NewTransport().Multiple IDP managers and other components call
NewTransport(), so this warning will appear repeatedly in logs when cert validation is disabled. Consider logging once at startup (e.g., usingsync.Once) or at a lower log level after the first occurrence.management/server/geolocation/utils.go (1)
179-194: Consider adding defensive check for Content-Disposition header.The function accesses
resp.Header["Content-Disposition"][0]directly on Line 187, which will panic if the header is missing or empty. While this is pre-existing code, the function signature change provides an opportunity to add defensive handling.🛡️ Optional: Add defensive check for missing header
func getFilenameFromURL(client *http.Client, url string) (string, error) { resp, err := client.Head(url) if err != nil { return "", err } defer resp.Body.Close() + cdHeader := resp.Header.Get("Content-Disposition") + if cdHeader == "" { + return "", fmt.Errorf("Content-Disposition header not found") + } + - _, params, err := mime.ParseMediaType(resp.Header["Content-Disposition"][0]) + _, params, err := mime.ParseMediaType(cdHeader) if err != nil { return "", err } filename := params["filename"] return filename, nil }management/internals/server/modules.go (1)
3-9: Prefer util.NewHTTPClient() to centralize HTTP client setup.Keeps transport defaults in one place and avoids duplicating client wiring here.
♻️ Suggested change
import ( "context" - "net/http" "os" "github.com/netbirdio/netbird/util" @@ - transport := util.NewTransport() - httpClient := &http.Client{ - Transport: transport, - } + httpClient := util.NewHTTPClient()Also applies to: 43-48
|
|
@anudeepND What I don't understand is how accessing websites even work in this setting? It looks like some sort of malware |
|
@braginini this kind of traffic inspection is pretty common I guess. I have seen this in multiple places with Fortient, Cisco and Paloalto which is mentioned in the issue. They do DPI (deep packet inspection) by performing authorised MITM. (The VM will be auto configured to trust the root CA generated in the firewall) Basically, whatever the request I send to external clients is intercepted by the firewall, the firewall will connect to the external endpoint on my behalf with the actual CA cert, it then creates a new certificate for the external end point signed by its own internal CA. The actual MITM attack is also done in a similar manner so that is the reason the flag is false by default. Users can set it to true for deploying netbird in this kind of strict environment. Let me know your POV on this |
|
I understand the point but I think that it could be solved with using a proxy on a VM or docker. We can describe this case in our docs, would you mind testing this? P.S. Don't get me wrong, I appreciate your contribution, but I am a bit hesitant to commit code that explicitly promotes not secure behaviour. That being said, I'm excited about your use case and would love to have a chat. Could you please ping me (Misha) on slack and we set up a call: https://docs.netbird.io/slack-url |
|
@braginini I agree with your concerns, will ping you in slack to have a conversation about this. I will also be able to show my use case as well |
|
@anudeepND can you try setting the proxy via HTTP_PROXY and HTTPS_PROXY environment variables in the management container? |
|
@mlsmaycon Docker proxy will not work in this case, the root CA is installed at the OS level. So even when Docker tries to perform SSL handshake through the tunnel, the firewall will intercept the traffic which will cause the same issue where cert verification fails in netbird Also, is there any other communication channel other than slack? |
4 participants
Describe your changes
This PR creates a central HTTP client with an option to disable SSL cert validation when making external requests (for example, downloading GeoIP database, check release versions) when
NB_DISABLE_CERT_VALIDATIONenv variable is set totrue.This is useful for self-hosted environments using internal CAs or self-signed certificates which is common when using firewalls that intercepts all the traffic.
Changes:
util.NewTransport()andutil.NewHTTPClient()to standardize HTTP transport configurationNB_DISABLE_CERT_VALIDATION=true, the transport is configured withInsecureSkipVerify: truewhich will disable certificate validationIssue ticket number and link
Fixes #5200
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:
netbirdio/docs#582
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.