fix(fxconfig)!: enable TLS by default to prevent plaintext fallback vulnerability - #214
Hidden character warning
Conversation
There was a problem hiding this comment.
Pull request overview
Enables TLS by default in fxconfig configurations to prevent insecure plaintext gRPC connections unless users explicitly opt out (tls.enabled: false), addressing the plaintext fallback vulnerability described in #108.
Changes:
- Flip TLS default to enabled via
TLSConfigdefaults andNormalize()behavior. - Add a dedicated config-load unit test asserting TLS is enabled by default when unspecified; update existing tests and integration fixtures to explicitly disable TLS where needed.
- Add a centralized CLI warning when any service has TLS disabled; document the breaking change.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/fxconfig/internal/config/config.go | Changes TLS default to enabled and normalizes missing enabled to true. |
| tools/fxconfig/internal/config/config_test.go | Updates normalization test expectations for the new TLS default. |
| tools/fxconfig/internal/config/load_test.go | Updates TLS flag tests and adds a new test asserting TLS is enabled by default. |
| tools/fxconfig/internal/cli/v1/root.go | Emits a centralized warning at config load time if any service has TLS disabled. |
| tools/fxconfig/integration/helpers_test.go | Updates generated integration configs to explicitly disable TLS. |
| tools/fxconfig/docs/README.md | Documents the breaking change (TLS enabled by default). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| > **Breaking Change (v0.4.0+):** TLS is now **enabled by default**. If you upgrade to this version and your config doesn't specify TLS certificates, you may see the error: `rootCertPaths must not be empty`. To restore the previous behavior (no TLS), explicitly set `tls.enabled: false` in your config or per service. | ||
|
|
||
| ### TLS Configuration | ||
|
|
||
| - **No TLS**: `enabled: false` or all TLS fields empty |
|
@MayankSharmaCSE @mbrandenburger Is there a specific reason to default to TLS enabled? Isn't it counterintuitive? I think about |
|
@pasquale95 , i have few thoughts:
|
BREAKING CHANGE: TLS is now enabled by default. - TLS now defaults to enabled (secure-by-default) - Normalize() sets Enabled=true when nil - Add TestLoad_TLSEnabledByDefault assertion - Move TLS warning to root.go (CLI layer) - Integration tests disable TLS explicitly (--insecure mode) - Add assert.False message for Queries TLS assertion consistency - Document breaking change in README Signed-off-by: mayanksharmaCSE <mayanksharmacse1@gmail.com>
00a1782 to
bb5792b
Compare
@pasquale95 valid point. I strongly believe the TLS-enabled default is the right thing to move forward - however, it should be as easy as possible. What if we use the system root ca pool by default. If ca "custom" ca cert is needed; the corresponding error message will tell the user that ca server cert cannot be validated; hinting that custom certs can be set via WDYT? |
|
@mbrandenburger I believe this is a good approach. If TLS is enabled by default, then at least we try to use the system root CA pool if rootCerts is undefined. |
Summary
Enable TLS by default across all service configurations (orderer, queries, notifications). Previously TLS was disabled by default.
Breaking Change
TLS is now enabled by default.
If you upgrade to this version and don't have TLS configured, you may see:
rootCertPaths must not be empty
New users must either:
rootCerts, ortls.enabled: falseExisting users can add
tls.enabled: falseto restore previous behavior.Changes
enabled: trueinstead offalseNormalize()setsEnabled=truewhen niltls.enabled: false(testcontainers use--insecureflag)assert.Falsemessage for Queries TLS assertion for consistencyMigration
Enable TLS (recommended for production):
Fixes #108
Test Plan