fftls: added ClientTLSConfigMergingSystemCAs for HTTP clients that ne…

pkg/fftls/config.go

@@ -44,7 +44,13 @@ const (
 	// HTTPConfTLSRequiredDNAttributes provides a set of regular expressions, to match against the DN of the client. Requires HTTPConfTLSClientAuth
 	HTTPConfTLSRequiredDNAttributes = "requiredDNAttributes"
 
-	defaultHTTPTLSEnabled = false
+	// HTTPConfTLSIncludeSystemCAs when true and a custom CA (caFile/ca) is configured, merges that CA
+	// into the system certificate pool instead of replacing RootCAs. Useful for outbound HTTP clients
+	// that talk to a private CA endpoint and may follow redirects to publicly trusted hosts (e.g. S3).
+	HTTPConfTLSIncludeSystemCAs = "includeSystemCAs"
+
+	defaultHTTPTLSEnabled          = false
+	defaultHTTPTLSIncludeSystemCAs = false
 )
 
 type Config struct {
@@ -57,6 +63,7 @@ type Config struct {
 	KeyFile                string                 `ffstruct:"tlsconfig" json:"keyFile,omitempty"`
 	Key                    string                 `ffstruct:"tlsconfig" json:"key,omitempty"`
 	InsecureSkipHostVerify bool                   `ffstruct:"tlsconfig" json:"insecureSkipHostVerify"`
+	IncludeSystemCAs       bool                   `ffstruct:"tlsconfig" json:"includeSystemCAs,omitempty"`
 	RequiredDNAttributes   map[string]interface{} `ffstruct:"tlsconfig" json:"requiredDNAttributes,omitempty"`
 }
 
@@ -71,6 +78,7 @@ func InitTLSConfig(conf config.Section) {
 	conf.AddKnownKey(HTTPConfTLSKey)
 	conf.AddKnownKey(HTTPConfTLSRequiredDNAttributes)
 	conf.AddKnownKey(HTTPConfTLSInsecureSkipHostVerify)
+	conf.AddKnownKey(HTTPConfTLSIncludeSystemCAs, defaultHTTPTLSIncludeSystemCAs)
 }
 
 func GenerateConfig(conf config.Section) *Config {
@@ -84,6 +92,7 @@ func GenerateConfig(conf config.Section) *Config {
 		KeyFile:                conf.GetString(HTTPConfTLSKeyFile),
 		Key:                    conf.GetString(HTTPConfTLSKey),
 		InsecureSkipHostVerify: conf.GetBool(HTTPConfTLSInsecureSkipHostVerify),
+		IncludeSystemCAs:       conf.GetBool(HTTPConfTLSIncludeSystemCAs),
 		RequiredDNAttributes:   conf.GetObject(HTTPConfTLSRequiredDNAttributes),
 	}
 }

pkg/fftls/fftls.go

@@ -101,12 +101,14 @@ func NewTLSConfig(ctx context.Context, config *Config, tlsType TLSType) (*tls.Co
 	}
 
 	var err error
-	// Support custom CA file
+	// Support custom CA file. By default a configured CA replaces RootCAs. When IncludeSystemCAs
+	// is set, the custom CA is merged into the system pool (e.g. private CA + public CAs for
+	// HTTP clients that follow redirects to S3).
 	var rootCAs *x509.CertPool
 	switch {
 	case config.CAFile != "":
-		log.L(ctx).Tracef("Loading CA file at %s", config.CAFile)
-		rootCAs = x509.NewCertPool()
+		log.L(ctx).Tracef("Loading CA file at %s (includeSystemCAs=%t)", config.CAFile, config.IncludeSystemCAs)
+		rootCAs = newRootCAPool(ctx, config.IncludeSystemCAs)
 		var caBytes []byte
 		caBytes, err = os.ReadFile(config.CAFile)
 		if err == nil {
@@ -119,7 +121,8 @@ func NewTLSConfig(ctx context.Context, config *Config, tlsType TLSType) (*tls.Co
 			}
 		}
 	case config.CA != "":
-		rootCAs = x509.NewCertPool()
+		log.L(ctx).Tracef("Loading inline CA PEM (includeSystemCAs=%t)", config.IncludeSystemCAs)
+		rootCAs = newRootCAPool(ctx, config.IncludeSystemCAs)
 		ok := rootCAs.AppendCertsFromPEM([]byte(config.CA))
 		if !ok {
 			err = i18n.NewError(ctx, i18n.MsgInvalidCAFile)
@@ -197,6 +200,19 @@ func NewTLSConfig(ctx context.Context, config *Config, tlsType TLSType) (*tls.Co
 
 }
 
+// newRootCAPool returns an empty pool, or the system pool when includeSystemCAs is true.
+func newRootCAPool(ctx context.Context, includeSystemCAs bool) *x509.CertPool {
+	if !includeSystemCAs {
+		return x509.NewCertPool()
+	}
+	rootCAs, err := x509.SystemCertPool()
+	if err != nil {
+		log.L(ctx).Warnf("Unable to load system cert pool; starting with empty pool: %s", err)
+		return x509.NewCertPool()
+	}
+	return rootCAs
+}
+
 // recordCACertExpiryMetrics decodes a PEM bundle (which may contain one or more certificates) and
 // records a CA certificate expiry gauge for each. It is best-effort and never fails the TLS config
 // construction: it is a no-op if metrics are not enabled, and certificates that cannot be parsed are

pkg/fftls/fftls_test.go

@@ -25,13 +25,13 @@ import (
 	"crypto/x509/pkix"
 	"encoding/pem"
 	"github.com/stretchr/testify/require"
+	"io"
 	"math/big"
 	"net"
 	"os"
 	"strings"
 	"testing"
 	"time"
-	"io"
 
 	"github.com/hyperledger-firefly/common/pkg/config"
 	"github.com/stretchr/testify/assert"
@@ -542,3 +542,140 @@ func TestConnectSkipVerification(t *testing.T) {
 	_ = conn.Close()
 
 }
+
+func TestIncludeSystemCAs_NilIfNotEnabled(t *testing.T) {
+	config.RootConfigReset()
+	conf := config.RootSection("fftls_client_merge")
+	InitTLSConfig(conf)
+
+	tlsConfig, err := ConstructTLSConfig(context.Background(), conf, ClientType)
+	assert.NoError(t, err)
+	assert.Nil(t, tlsConfig)
+}
+
+func TestIncludeSystemCAs_DefaultsWithoutCustomCA(t *testing.T) {
+	config.RootConfigReset()
+	conf := config.RootSection("fftls_client_merge")
+	InitTLSConfig(conf)
+	conf.Set(HTTPConfTLSEnabled, true)
+	conf.Set(HTTPConfTLSIncludeSystemCAs, true)
+
+	tlsConfig, err := ConstructTLSConfig(context.Background(), conf, ClientType)
+	assert.NoError(t, err)
+	require.NotNil(t, tlsConfig)
+	assert.False(t, tlsConfig.InsecureSkipVerify)
+	assert.Equal(t, uint16(tls.VersionTLS12), tlsConfig.MinVersion)
+	assert.Nil(t, tlsConfig.GetClientCertificate)
+
+	systemPool, err := x509.SystemCertPool()
+	if err == nil && systemPool != nil {
+		assert.True(t, tlsConfig.RootCAs.Equal(systemPool))
+	}
+}
+
+func TestIncludeSystemCAs_CAFileMergesSystemPool(t *testing.T) {
+	config.RootConfigReset()
+	caFile, _ := buildSelfSignedTLSKeyPairFiles(t, pkix.Name{CommonName: "test-ca.example.com"})
+
+	conf := config.RootSection("fftls_client_merge")
+	InitTLSConfig(conf)
+	conf.Set(HTTPConfTLSEnabled, true)
+	conf.Set(HTTPConfTLSCAFile, caFile)
+	conf.Set(HTTPConfTLSIncludeSystemCAs, true)
+
+	merged, err := ConstructTLSConfig(context.Background(), conf, ClientType)
+	assert.NoError(t, err)
+	require.NotNil(t, merged)
+	require.NotNil(t, merged.RootCAs)
+
+	conf.Set(HTTPConfTLSIncludeSystemCAs, false)
+	replaced, err := ConstructTLSConfig(context.Background(), conf, ClientType)
+	assert.NoError(t, err)
+	require.NotNil(t, replaced)
+	require.NotNil(t, replaced.RootCAs)
+
+	systemPool, sysErr := x509.SystemCertPool()
+	if sysErr != nil || systemPool == nil {
+		t.Skip("system cert pool unavailable")
+	}
+	expected := systemPool.Clone()
+	caBytes, err := os.ReadFile(caFile)
+	require.NoError(t, err)
+	require.True(t, expected.AppendCertsFromPEM(caBytes))
+
+	assert.True(t, merged.RootCAs.Equal(expected), "custom CA should be merged into the system pool")
+	assert.False(t, replaced.RootCAs.Equal(expected), "without includeSystemCAs, RootCAs should be only the custom CA")
+}
+
+func TestIncludeSystemCAs_InlineCAMergesSystemPool(t *testing.T) {
+	config.RootConfigReset()
+	caPEM, _ := buildSelfSignedTLSKeyPair(t, pkix.Name{CommonName: "inline-ca.example.com"})
+
+	conf := config.RootSection("fftls_client_merge")
+	InitTLSConfig(conf)
+	conf.Set(HTTPConfTLSEnabled, true)
+	conf.Set(HTTPConfTLSCA, caPEM)
+	conf.Set(HTTPConfTLSIncludeSystemCAs, true)
+
+	merged, err := ConstructTLSConfig(context.Background(), conf, ClientType)
+	assert.NoError(t, err)
+	require.NotNil(t, merged.RootCAs)
+
+	systemPool, sysErr := x509.SystemCertPool()
+	if sysErr != nil || systemPool == nil {
+		t.Skip("system cert pool unavailable")
+	}
+	expected := systemPool.Clone()
+	require.True(t, expected.AppendCertsFromPEM([]byte(caPEM)))
+	assert.True(t, merged.RootCAs.Equal(expected))
+}
+
+func TestIncludeSystemCAs_WithClientCert(t *testing.T) {
+	config.RootConfigReset()
+	certFile, keyFile := buildSelfSignedTLSKeyPairFiles(t, pkix.Name{CommonName: "client.example.com"})
+	caFile, _ := buildSelfSignedTLSKeyPairFiles(t, pkix.Name{CommonName: "ca.example.com"})
+
+	conf := config.RootSection("fftls_client_merge")
+	InitTLSConfig(conf)
+	conf.Set(HTTPConfTLSEnabled, true)
+	conf.Set(HTTPConfTLSCertFile, certFile)
+	conf.Set(HTTPConfTLSKeyFile, keyFile)
+	conf.Set(HTTPConfTLSCAFile, caFile)
+	conf.Set(HTTPConfTLSIncludeSystemCAs, true)
+	conf.Set(HTTPConfTLSInsecureSkipHostVerify, true)
+
+	tlsConfig, err := ConstructTLSConfig(context.Background(), conf, ClientType)
+	assert.NoError(t, err)
+	require.NotNil(t, tlsConfig)
+	assert.True(t, tlsConfig.InsecureSkipVerify)
+	require.NotNil(t, tlsConfig.GetClientCertificate)
+	cert, err := tlsConfig.GetClientCertificate(nil)
+	assert.NoError(t, err)
+	assert.NotNil(t, cert)
+}
+
+func TestIncludeSystemCAs_InvalidCAFile(t *testing.T) {
+	config.RootConfigReset()
+	_, notTheCAFileTheKey := buildSelfSignedTLSKeyPairFiles(t, pkix.Name{CommonName: "server.example.com"})
+
+	conf := config.RootSection("fftls_client_merge")
+	InitTLSConfig(conf)
+	conf.Set(HTTPConfTLSEnabled, true)
+	conf.Set(HTTPConfTLSCAFile, notTheCAFileTheKey)
+	conf.Set(HTTPConfTLSIncludeSystemCAs, true)
+
+	_, err := ConstructTLSConfig(context.Background(), conf, ClientType)
+	assert.Regexp(t, "FF00152", err)
+}
+
+func TestIncludeSystemCAs_CAFileNotFound(t *testing.T) {
+	config.RootConfigReset()
+	conf := config.RootSection("fftls_client_merge")
+	InitTLSConfig(conf)
+	conf.Set(HTTPConfTLSEnabled, true)
+	conf.Set(HTTPConfTLSCAFile, "/nonexistent/ca.pem")
+	conf.Set(HTTPConfTLSIncludeSystemCAs, true)
+
+	_, err := ConstructTLSConfig(context.Background(), conf, ClientType)
+	assert.Error(t, err)
+}

pkg/i18n/en_base_config_descriptions.go

@@ -78,6 +78,7 @@ var (
 	ConfigGlobalTLSKeyFile                = ffc("config.global.tls.keyFile", "The path to the private key file for TLS on this API", StringType)
 	ConfigGlobalTLSRequiredDNAttributes   = ffc("config.global.tls.requiredDNAttributes", "A set of required subject DN attributes. Each entry is a regular expression, and the subject certificate must have a matching attribute of the specified type (CN, C, O, OU, ST, L, STREET, POSTALCODE, SERIALNUMBER are valid attributes)", MapStringStringType)
 	ConfigGlobalTLSInsecureSkipHostVerify = ffc("config.global.tls.insecureSkipHostVerify", "When to true in unit test development environments to disable TLS verification. Use with extreme caution", BooleanType)
+	ConfigGlobalTLSIncludeSystemCAs       = ffc("config.global.tls.includeSystemCAs", "When true and a custom CA (caFile/ca) is configured, merge that CA into the system certificate pool instead of replacing it. Useful for outbound HTTP clients that may follow redirects to publicly trusted hosts", BooleanType)
 	ConfigGlobalTLSHandshakeTimeout       = ffc("config.global.tlsHandshakeTimeout", "The maximum amount of time to wait for a successful TLS handshake", TimeDurationType)
 
 	ConfigGlobalBodyTemplate              = ffc("config.global.bodyTemplate", "The body go template string to use when making HTTP requests", GoTemplateType)