[PR #5458] [CLOSED] Add TLS certificate skip verification option for Reverse Proxy backend connections #27984

Open
opened 2026-08-05 07:09:30 -04:00 by saavagebueno · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/netbirdio/netbird/pull/5458
Author: @samanebi
Created: 2/26/2026
Status: Closed

Base: mainHead: feature/5347-proxy-skip-tls-verify


📝 Commits (6)

  • b065f2a [proxy] add TLS skip verification option for backend connections (#5347)
  • 6f4fa1a [proxy] fix review suggestions
  • fd89a83 [proxy] Use a single source of truth for proxySkipTLSVerify to avoid insecure precedence surprises
  • f3d4a88 Merge branch 'main' into feature/5347-proxy-skip-tls-verify
  • 67f5685 [proxy] fix tests according to latest merge
  • 11ba889 [proxy] remove redundant env var

📊 Changes

6 files changed (+140 additions, -67 deletions)

View changed files

📝 proxy/README.md (+21 -14)
📝 proxy/cmd/proxy/cmd/root.go (+23 -20)
📝 proxy/internal/roundtrip/netbird.go (+34 -20)
📝 proxy/internal/roundtrip/netbird_test.go (+52 -9)
📝 proxy/internal/roundtrip/transport.go (+3 -1)
📝 proxy/server.go (+7 -3)

📄 Description

PR Description

Describe your changes

This PR adds a new configuration option --proxy-skip-tls-verify (env: NB_PROXY_SKIP_TLS_VERIFY) that allows the NetBird Reverse Proxy to skip TLS certificate verification when connecting to backend services. This is useful for internal services using self-signed certificates or private certificate authorities.

Key changes:

  • Added new CLI flag --proxy-skip-tls-verify and environment variable NB_PROXY_SKIP_TLS_VERIFY
  • Modified NewNetBird() function to accept skipTLSVerify parameter
  • Updated HTTP transport configuration to set InsecureSkipVerify: true when enabled
  • Added comprehensive unit tests for the new functionality
  • Updated proxy README.md with configuration documentation and security warnings
  • Added security warning log message when TLS verification is disabled

Security considerations:

  • Disabled by default (secure by default principle)
  • Logs a warning message when enabled to ensure operators are aware
  • Includes //nolint:gosec comment with justification for the security scanner
  • Documentation clearly warns about the security implications

Fixes #5347

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

Docs PR URL (required if "docs added" is checked)

N/A - Documentation was updated in the proxy/README.md file within this PR. No separate docs repository changes are required as the proxy documentation is maintained in the main repository.


Technical Implementation Details

Files Modified

  1. proxy/cmd/proxy/cmd/root.go

    • Added proxySkipTLSVerify flag variable
    • Added CLI flag --proxy-skip-tls-verify with environment variable NB_PROXY_SKIP_TLS_VERIFY
    • Pass the flag value to proxy.Server struct
  2. proxy/server.go

    • Added ProxySkipTLSVerify field to Server struct with documentation
    • Pass the flag value to roundtrip.NewNetBird() function
  3. proxy/internal/roundtrip/netbird.go

    • Modified NewNetBird() signature to accept skipTLSVerify bool parameter
    • Updated transport configuration logic to set TLSClientConfig.InsecureSkipVerify when enabled
    • Added warning log message when TLS verification is disabled
    • Updated all existing callers in tests
  4. proxy/internal/roundtrip/transport.go

    • Added proxySkipTLSVerify bool field to transportConfig struct
    • Added loading from environment variable NB_PROXY_SKIP_TLS_VERIFY
    • Added field to debug logging output
  5. proxy/internal/roundtrip/netbird_test.go

    • Updated all mockNetBird() calls to include the new parameter
    • Updated all direct NewNetBird() calls in tests
    • Added new test TestNetBird_SkipTLSVerify_ConfiguresTransport to verify the feature works correctly
    • Added new test TestNetBird_SkipTLSVerify_DefaultIsFalse to verify secure defaults
  6. proxy/README.md

    • Added new "Backend TLS Verification" section explaining the feature and security implications
    • Added the new flag to the configuration table
    • Included clear security warning about when this should be used

Testing Strategy

The implementation includes comprehensive unit tests that verify:

  • Default behavior (TLS verification enabled)
  • TLS verification can be disabled via the flag
  • Transport configuration is correctly applied
  • TLS client config is properly set when skip verify is enabled

Security Analysis

Threat Model:

  • Default Secure: The feature is disabled by default, maintaining secure behavior
  • Explicit Opt-in: Requires explicit configuration by system administrator
  • Visibility: Logs a warning message when enabled so it's visible in logs
  • Limited Scope: Only affects backend connections, not client-facing TLS
  • Use Case: Designed for internal services with self-signed certificates in trusted networks

Mitigation:

  • Clear documentation warnings
  • Runtime warning logs
  • Code comment explaining the security tradeoff
  • gosec linter suppression with justification

Testing Checklist

  • Unit tests added and passing
  • No linter errors
  • Documentation updated
  • Security implications documented
  • Default behavior remains secure
  • Manual testing with self-signed certificate (requires environment setup)

Commit Message (for reference)

Add TLS skip verification option for reverse proxy backend connections

This adds support for skipping TLS certificate verification when the
reverse proxy connects to backend services. This is useful for internal
services using self-signed certificates or private CAs.

The feature is disabled by default and can be enabled via:
- CLI flag: --proxy-skip-tls-verify
- Environment variable: NB_PROXY_SKIP_TLS_VERIFY

When enabled, a warning is logged to ensure operators are aware of the
security implications.

Fixes #5347

Additional Notes

  • All existing tests have been updated to work with the new function signature
  • The change is backward compatible in behavior (disabled by default)
  • The implementation follows Go security best practices with explicit opt-in
  • Code follows the project's contribution guidelines regarding comments, tests, and simplicity

Summary by CodeRabbit

  • New Features

    • Added a flag and environment variable to optionally disable TLS certificate verification for backend service connections.
  • Documentation

    • Added a Backend TLS Verification section and updated the deployment flags table with clearer headers, formatting, and the new flag row.
  • Tests

    • Added tests and updated test setup to validate TLS-skip behavior and defaults.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/netbirdio/netbird/pull/5458 **Author:** [@samanebi](https://github.com/samanebi) **Created:** 2/26/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `feature/5347-proxy-skip-tls-verify` --- ### 📝 Commits (6) - [`b065f2a`](https://github.com/netbirdio/netbird/commit/b065f2ad3136dac58cda9ddecf8bcadfc21d2b0a) [proxy] add TLS skip verification option for backend connections (#5347) - [`6f4fa1a`](https://github.com/netbirdio/netbird/commit/6f4fa1a49e6fabed02faa2dfb67ee0fdec939bd1) [proxy] fix review suggestions - [`fd89a83`](https://github.com/netbirdio/netbird/commit/fd89a83233ef5905c74c64ce1ca756ebff3142d8) [proxy] Use a single source of truth for proxySkipTLSVerify to avoid insecure precedence surprises - [`f3d4a88`](https://github.com/netbirdio/netbird/commit/f3d4a88bd51d85f0f0c70fc2829a06567adb4efa) Merge branch 'main' into feature/5347-proxy-skip-tls-verify - [`67f5685`](https://github.com/netbirdio/netbird/commit/67f56855fba32a667fbda4326df4f3a3b801394b) [proxy] fix tests according to latest merge - [`11ba889`](https://github.com/netbirdio/netbird/commit/11ba889a38c24ef06f690ea9b22e5b1b037c1b9b) [proxy] remove redundant env var ### 📊 Changes **6 files changed** (+140 additions, -67 deletions) <details> <summary>View changed files</summary> 📝 `proxy/README.md` (+21 -14) 📝 `proxy/cmd/proxy/cmd/root.go` (+23 -20) 📝 `proxy/internal/roundtrip/netbird.go` (+34 -20) 📝 `proxy/internal/roundtrip/netbird_test.go` (+52 -9) 📝 `proxy/internal/roundtrip/transport.go` (+3 -1) 📝 `proxy/server.go` (+7 -3) </details> ### 📄 Description ## PR Description ### Describe your changes This PR adds a new configuration option `--proxy-skip-tls-verify` (env: `NB_PROXY_SKIP_TLS_VERIFY`) that allows the NetBird Reverse Proxy to skip TLS certificate verification when connecting to backend services. This is useful for internal services using self-signed certificates or private certificate authorities. **Key changes:** - Added new CLI flag `--proxy-skip-tls-verify` and environment variable `NB_PROXY_SKIP_TLS_VERIFY` - Modified `NewNetBird()` function to accept `skipTLSVerify` parameter - Updated HTTP transport configuration to set `InsecureSkipVerify: true` when enabled - Added comprehensive unit tests for the new functionality - Updated proxy README.md with configuration documentation and security warnings - Added security warning log message when TLS verification is disabled **Security considerations:** - Disabled by default (secure by default principle) - Logs a warning message when enabled to ensure operators are aware - Includes `//nolint:gosec` comment with justification for the security scanner - Documentation clearly warns about the security implications ### Issue ticket number and link Fixes #5347 ### Stack <!-- branch-stack --> ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [x] Is a feature enhancement - [ ] It is a refactor - [x] Created tests that fail without the change (if possible) > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [x] I added/updated documentation for this change - [ ] Documentation is **not needed** for this change (explain why) ### Docs PR URL (required if "docs added" is checked) N/A - Documentation was updated in the proxy/README.md file within this PR. No separate docs repository changes are required as the proxy documentation is maintained in the main repository. --- ## Technical Implementation Details ### Files Modified 1. **proxy/cmd/proxy/cmd/root.go** - Added `proxySkipTLSVerify` flag variable - Added CLI flag `--proxy-skip-tls-verify` with environment variable `NB_PROXY_SKIP_TLS_VERIFY` - Pass the flag value to `proxy.Server` struct 2. **proxy/server.go** - Added `ProxySkipTLSVerify` field to `Server` struct with documentation - Pass the flag value to `roundtrip.NewNetBird()` function 3. **proxy/internal/roundtrip/netbird.go** - Modified `NewNetBird()` signature to accept `skipTLSVerify bool` parameter - Updated transport configuration logic to set `TLSClientConfig.InsecureSkipVerify` when enabled - Added warning log message when TLS verification is disabled - Updated all existing callers in tests 4. **proxy/internal/roundtrip/transport.go** - Added `proxySkipTLSVerify bool` field to `transportConfig` struct - Added loading from environment variable `NB_PROXY_SKIP_TLS_VERIFY` - Added field to debug logging output 5. **proxy/internal/roundtrip/netbird_test.go** - Updated all `mockNetBird()` calls to include the new parameter - Updated all direct `NewNetBird()` calls in tests - Added new test `TestNetBird_SkipTLSVerify_ConfiguresTransport` to verify the feature works correctly - Added new test `TestNetBird_SkipTLSVerify_DefaultIsFalse` to verify secure defaults 6. **proxy/README.md** - Added new "Backend TLS Verification" section explaining the feature and security implications - Added the new flag to the configuration table - Included clear security warning about when this should be used ### Testing Strategy The implementation includes comprehensive unit tests that verify: - Default behavior (TLS verification enabled) - TLS verification can be disabled via the flag - Transport configuration is correctly applied - TLS client config is properly set when skip verify is enabled ### Security Analysis **Threat Model:** - **Default Secure**: The feature is disabled by default, maintaining secure behavior - **Explicit Opt-in**: Requires explicit configuration by system administrator - **Visibility**: Logs a warning message when enabled so it's visible in logs - **Limited Scope**: Only affects backend connections, not client-facing TLS - **Use Case**: Designed for internal services with self-signed certificates in trusted networks **Mitigation:** - Clear documentation warnings - Runtime warning logs - Code comment explaining the security tradeoff - gosec linter suppression with justification ### Testing Checklist - [x] Unit tests added and passing - [x] No linter errors - [x] Documentation updated - [x] Security implications documented - [x] Default behavior remains secure - [x] Manual testing with self-signed certificate (requires environment setup) ### Commit Message (for reference) ``` Add TLS skip verification option for reverse proxy backend connections This adds support for skipping TLS certificate verification when the reverse proxy connects to backend services. This is useful for internal services using self-signed certificates or private CAs. The feature is disabled by default and can be enabled via: - CLI flag: --proxy-skip-tls-verify - Environment variable: NB_PROXY_SKIP_TLS_VERIFY When enabled, a warning is logged to ensure operators are aware of the security implications. Fixes #5347 ``` ### Additional Notes - All existing tests have been updated to work with the new function signature - The change is backward compatible in behavior (disabled by default) - The implementation follows Go security best practices with explicit opt-in - Code follows the project's contribution guidelines regarding comments, tests, and simplicity <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a flag and environment variable to optionally disable TLS certificate verification for backend service connections. * **Documentation** * Added a Backend TLS Verification section and updated the deployment flags table with clearer headers, formatting, and the new flag row. * **Tests** * Added tests and updated test setup to validate TLS-skip behavior and defaults. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
saavagebueno added the pull-request label 2026-08-05 07:09:30 -04:00
Sign in to join this conversation.
No Label pull-request
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DYNR/netbird#27984