fix(fxconfig)!: enable TLS by default to prevent plaintext fallback vulnerability by MayankSharmaCSE · Pull Request #138 · hyperledger/fabric-x

MayankSharmaCSE · 2026-04-16T16:50:50Z

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:

Configure TLS with rootCerts, or
Explicitly set tls.enabled: false

Existing users can add tls.enabled: false to restore previous behavior.

Changes

TLS now defaults to enabled: true instead of false
Normalize() sets Enabled=true when nil
TLS warning moved to config load time (single centralized warning instead of per-client)
Integration tests updated with explicit tls.enabled: false (testcontainers use --insecure flag)
Added assert.False message for Queries TLS assertion for consistency

Migration

Enable TLS (recommended for production):

tls:
  enabled: true
  rootCerts:
    - /path/to/ca.crt

Disable TLS (not recommended for production):
tls:
  enabled: false

Fixes #108
Test Plan

Unit tests pass
Integration tests pass

Copilot

Pull request overview

This PR addresses a security issue in fxconfig where gRPC clients could silently downgrade to plaintext when TLS was disabled or not explicitly configured, enabling a man-in-the-middle risk.

Changes:

Flip TLSConfig.Enabled default from false to true so TLS is enabled unless explicitly disabled.
Emit a warning log when TLS is disabled to improve visibility of insecure configurations.
Update load tests to reflect the new secure default behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
tools/fxconfig/internal/config/config.go	Changes TLS enabled default to `true` via struct tag.
tools/fxconfig/internal/client/client.go	Adds a warning log when TLS is disabled in `createSecOpts`.
tools/fxconfig/internal/config/load_test.go	Updates test expectation so Notifications TLS is enabled by default.

Comments suppressed due to low confidence (1)

tools/fxconfig/internal/config/config.go:67

The new default default:"true" suggests TLS should be enabled when Enabled is unset, but TLSConfig.Normalize() still sets Enabled to false when nil and IsEnabled() still returns false when Enabled is nil. That means TLS can still silently fall back to plaintext if a TLSConfig is constructed without viper defaults (or if YAML explicitly sets enabled: null), which undermines the secure-by-default intent. Consider changing Normalize()/IsEnabled() to default to true when Enabled is nil (and update the related comments) so the behavior is consistent across all code paths.

	Enabled            *bool    `mapstructure:"enabled" yaml:"enabled,omitempty" desc:"Enable/disable TLS" default:"true"`
	ClientKeyPath      string   `mapstructure:"clientKey" yaml:"clientKey,omitempty" desc:"Path to TLS client private key"`
	ClientCertPath     string   `mapstructure:"clientCert" yaml:"clientCert,omitempty" desc:"Path to TLS client certificate"`
	RootCertPaths      []string `mapstructure:"rootCerts" yaml:"rootCerts,omitempty" desc:"Paths to TLS root certificates"`
	ServerNameOverride string   `mapstructure:"serverNameOverride" yaml:"serverNameOverride,omitempty" desc:"Override TLS server name"`

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-16T16:55:29Z


 	// let's see if we use TLS
 	if !tlsConfig.IsEnabled() {
+		logger.Warn("TLS is disabled — connections will be unencrypted and unauthenticated. This is insecure and not recommended for production environments.")


This warning string appears to exceed the repository’s lll max line length (120) enforced by golangci-lint, which will likely fail CI. Consider splitting the message across concatenated string literals (or otherwise shortening it) so the line length stays within the configured limit.

Suggested change

logger.Warn("TLS is disabled — connections will be unencrypted and unauthenticated. This is insecure and not recommended for production environments.")

logger.Warn(

"TLS is disabled — connections will be unencrypted and unauthenticated. " +

"This is insecure and not recommended for production environments.",

)

Copilot · 2026-04-16T16:55:30Z

 	assert.True(t, cfg.Orderer.TLS.IsEnabled())
 	assert.Equal(t, []string{"/path/to/ca.pem"}, cfg.Orderer.TLS.RootCertPaths)
 	assert.False(t, cfg.Queries.TLS.IsEnabled())
-	assert.False(t, cfg.Notifications.TLS.IsEnabled())
+	assert.True(t, cfg.Notifications.TLS.IsEnabled())


With the new secure default, this test now expects Notifications TLS to be enabled even though the YAML in this test does not provide any root certs for Notifications (and no global tls.rootCerts). That configuration would fail TLSConfig.Validate() (rootCertPaths must not be empty when TLS is enabled), so the test is no longer representative of a valid config. Consider adding a top-level tls.rootCerts (or notifications-specific rootCerts) to keep the test focused on the enabled flag while still producing a valid configuration.

We need to update the tests now to disable TLS explicitly. Also update the documentation accordingly.

mbrandenburger · 2026-04-17T09:08:11Z

 	assert.True(t, cfg.Orderer.TLS.IsEnabled())
 	assert.Equal(t, []string{"/path/to/ca.pem"}, cfg.Orderer.TLS.RootCertPaths)
 	assert.False(t, cfg.Queries.TLS.IsEnabled())
-	assert.False(t, cfg.Notifications.TLS.IsEnabled())
+	assert.True(t, cfg.Notifications.TLS.IsEnabled())


We need to update the tests now to disable TLS explicitly. Also update the documentation accordingly.

mbrandenburger · 2026-04-22T09:37:29Z

hi @MayankSharmaCSE it seems that the unit tests are still failing.

MayankSharmaCSE · 2026-04-22T17:53:56Z

@mbrandenburger, fixed the issue .

mbrandenburger

Thank you @MayankSharmaCSE for working on this PR. We are making good progress here. I have a few more comments below.

Moreover, we should better indicate that this is a breaking change. Please make sure that the PR description highlights this. New users need to configure TLS correctly or disable it explicitly.

The commit message should therefore also contain fix(fxconfig)! per conventional commits.

mbrandenburger · 2026-04-24T20:20:38Z

 	assert.False(t, cfg.Queries.TLS.IsEnabled())
-	assert.False(t, cfg.Notifications.TLS.IsEnabled())
+	assert.False(t, cfg.Notifications.TLS.IsEnabled(), "Notifications explicitly has enabled: false in config")


Should we add a similar comment to assert.False(t, cfg.Queries.TLS.IsEnabled()) as well?

Yeh , I missed that during the refactor. i will add the assertion for cfg.Queries.TLS.IsEnabled() to ensure consistency across all three service configs in the test.

mbrandenburger · 2026-04-24T20:25:09Z

+		logger.Warn("TLS is disabled — connections will be unencrypted and " +
+			"unauthenticated. This is insecure and not recommended for production.")


I am wondering if this is the right place to throw the logging as this would lead to multiple warning for each client (orderer, queries, notifications). Without additional information which service has TLS disabled.

Maybe we should handle the warning somewhere else ... during config loading?

Fair point. Logging in each client would produce redundant warnings without context. I agree a single warning during config load would be cleaner. However, to keep this PR focused, I did prefer to address this as a follow-up item rather than scope creep this fix.

mbrandenburger · 2026-04-24T20:28:23Z

 ### TLS Configuration

- **No TLS**: `enabled: false` or all TLS fields empty
+- **No TLS**: `enabled: false`
 - **Server TLS**: `enabled: true` with only `rootCerts` set (server authentication only)
 - **Mutual TLS**: `enabled: true` with `clientKey`, `clientCert`, and `rootCerts` all set (mutual authentication)
 - **Service-specific TLS**: Each service (orderer, queries, notifications) can override the parent `tls` section
 - **SNI Override**: Use `serverNameOverride` for IP-based connections or custom hostname verification


I think we need to document this better and explain that by default TLS is enabled and requires at least a rootCert otherwise the user will get an error. Or the user explicitly turns off TLS.

I will add clear documentation explaining:

TLS is now enabled by default and requires TLS certificates.

Users can set tls.enabled: false to disable (not recommended for production).

The "rootCertPaths must not be empty" error indicates misconfiguration. This will be added to the README and/or inline comments in the config example file.

MayankSharmaCSE · 2026-04-27T13:27:25Z

@mbrandenburger Thanks for the review you had! I have addressed all your comments:

Breaking change — Updated commit prefix to fix(fxconfig)!: and added a prominent Breaking Change section in the PR description warning users that TLS is now enabled by default.
Queries assertion — Added the missing assert.False message for cfg.Queries.TLS.IsEnabled() for consistency with Orderer and Notifications.
Warning logging location — Moved the TLS warning to ResolveTLS() in config.go so it logs once at config load time instead of multiple times per client. Happy to address this as a follow-up if you'd prefer a different approach.
Documentation — Added a Breaking Change notice in the README explaining the new default, the error users may see (rootCertPaths must not be empty), and how to migrate.

mbrandenburger

Thank you @MayankSharmaCSE Great work! See my comments below.

mbrandenburger · 2026-04-29T14:57:52Z

 	require.NoError(t, err)
 	require.NotNil(t, cfg)
 	assert.Equal(t, 30*time.Second, cfg.Orderer.ConnectionTimeout)
 	assert.Equal(t, 30*time.Second, cfg.Queries.ConnectionTimeout)
 	assert.Equal(t, 30*time.Second, cfg.Notifications.ConnectionTimeout)


Let's add TLS enabled as assertions as it's our new default.

For instance:

assert.True(t, cfg.Orderer.TLS.IsEnabled()) // ... (and Queries/Notifications)

Makes sense. I will add assert.True for when TLS is enabled by default (not explicitly disabled).

mbrandenburger · 2026-04-29T15:08:52Z

+
+	// Warn once if any service has TLS disabled
+	if !c.Orderer.TLS.IsEnabled() || !c.Queries.TLS.IsEnabled() || !c.Notifications.TLS.IsEnabled() {
+		configLogger.Warn("TLS is disabled for one or more services — " +
+			"connections will be unencrypted. This is insecure and not recommended for production.")
+	}


I have the feeling we should not do the logging here as the "application" should decide if this particular configuration should log a warning or not. Thus, checking the config and print a log could happen in

fabric-x/tools/fxconfig/internal/cli/v1/root.go

Line 55 in 321cab8

cliCtx.Printer = cliio.NewCLIPrinter(cmd.OutOrStdout(), cmd.ErrOrStderr(), cliio.FormatTable)

WDYT?

Agreed, the config package shouldn't dictate logging. I will move the TLS warning from config.goto root.go so the application controls when/how to log. Does that sound right to you?

mbrandenburger · 2026-04-29T15:11:17Z

@pasquale95 Can you please have a look at this PR, which changes the "default TLS behavior" for fxconfig to always enabled - if not explicitly disabled.

- Change tls.enabled default from false to true - Normalize() sets Enabled=true when nil - Add TestLoad_TLSEnabledByDefault to assert default behavior - Move TLS warning to root.go (CLI layer) - Update integration tests with explicit tls.enabled: false - Improve assert messages for Queries TLS consistency - Document breaking change in README Signed-off-by: mayanksharmaCSE <mayanksharmacse1@gmail.com>

mbrandenburger · 2026-04-30T10:48:37Z

This PR was re-opened in #214

MayankSharmaCSE · 2026-04-30T10:52:27Z

@mbrandenburger,yes,I have followed up on the changes in #214 because i messed up my git history with some wrong commands.

Copilot AI review requested due to automatic review settings April 16, 2026 16:50

Copilot started reviewing on behalf of MayankSharmaCSE April 16, 2026 16:51 View session

Copilot AI reviewed Apr 16, 2026

View reviewed changes

mbrandenburger requested changes Apr 17, 2026

View reviewed changes

mbrandenburger added the fxconfig label Apr 22, 2026

mbrandenburger self-assigned this Apr 22, 2026

mbrandenburger requested changes Apr 24, 2026

View reviewed changes

mbrandenburger requested changes Apr 29, 2026

View reviewed changes

mbrandenburger changed the title ~~fix(fxconfig): enable TLS by default to prevent plaintext fallback vulnerability~~ fix(fxconfig)!: enable TLS by default to prevent plaintext fallback vulnerability Apr 29, 2026

MayankSharmaCSE force-pushed the fix/tls-secure-by-default branch 3 times, most recently from 82ad38c to f01ca25 Compare April 29, 2026 18:49

MayankSharmaCSE force-pushed the fix/tls-secure-by-default branch from f01ca25 to f5e8e5d Compare April 29, 2026 19:20

MayankSharmaCSE closed this Apr 29, 2026

		logger.Warn("TLS is disabled — connections will be unencrypted and " +
		"unauthenticated. This is insecure and not recommended for production.")

Uh oh!

Conversation

MayankSharmaCSE commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Breaking Change

Changes

Migration

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbrandenburger commented Apr 22, 2026

Uh oh!

MayankSharmaCSE commented Apr 22, 2026

Uh oh!

mbrandenburger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MayankSharmaCSE commented Apr 27, 2026

Uh oh!

mbrandenburger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbrandenburger commented Apr 29, 2026

Uh oh!

mbrandenburger commented Apr 30, 2026

Uh oh!

MayankSharmaCSE commented Apr 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MayankSharmaCSE commented Apr 16, 2026 •

edited

Loading