carverauto/serviceradar

Fork 0

CNPG pool sets sslmode=disable when TLS config is present but SSLMode unset, resulting in unencrypted connections #681

New issue

Closed

opened 2026-03-28 04:27:22 +00:00 by mfreeman451 · 1 comment

mfreeman451 commented

2026-03-28 04:27:22 +00:00

Owner

Imported from GitHub.

Original GitHub issue: #2143
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/issues/2143
Original created: 2025-12-16T05:15:41Z

Summary

Context: NewCNPGPool in pkg/db/cnpg_pool.go creates a pgx connection pool for connecting to CloudNativePG/Timescale databases, handling both the connection URL configuration and optional TLS setup.
Bug: When TLS configuration is provided but SSLMode is not explicitly set, the function defaults sslmode to "disable", which instructs PostgreSQL to completely disable SSL/TLS encryption.
Actual vs. expected: The connection URL has sslmode=disable even when a complete TLS configuration (certificates, keys, CA) is provided, causing the TLS configuration to be ignored; the expected behavior is that when TLS configuration is provided, an appropriate SSL mode like "require", "verify-ca", or "verify-full" should be used.
Impact: Database connections that should be encrypted remain unencrypted, exposing sensitive data in transit and violating security requirements for production deployments that rely on TLS/mTLS for database connections.

Code with bug

sslMode := cnpg.SSLMode
if sslMode == "" {
	sslMode = "disable"  // <-- BUG 🔴 Defaults to disable even when TLS config is provided
}
query.Set("sslmode", sslMode)

Later in the same function:

if tlsConfig, err := buildCNPGTLSConfig(&cnpg); err != nil {
	return nil, err
} else if tlsConfig != nil {
	poolConfig.ConnConfig.TLSConfig = tlsConfig  // TLS config is set but sslmode is already "disable"
}

Evidence

Example

Consider a configuration where a user provides TLS certificates to connect to a CNPG cluster:

cfg := &models.CNPGDatabase{
    Host:     "cnpg-rw.serviceradar",
    Port:     5432,
    Database: "serviceradar",
    Username: "app_user",
    Password: "secret",
    // SSLMode is not set (common when users assume TLS config implies SSL)
    CertDir:  "/etc/serviceradar/cnpg",
    TLS: &models.TLSConfig{
        CertFile: "client.crt",
        KeyFile:  "client.key",
        CAFile:   "ca.crt",
    },
}

Step-by-step execution:

Line 63-67: Since cfg.SSLMode is empty string, sslMode is set to "disable"
Line 67: The connection URL query gets sslmode=disable
Line 83: The connection string is parsed: postgres://app_user:secret@cnpg-rw.serviceradar:5432/serviceradar?sslmode=disable
Lines 121-125: The TLS config is built successfully and set on poolConfig.ConnConfig.TLSConfig
Result: pgx sees both sslmode=disable in the connection string AND a TLS config object. According to PostgreSQL/pgx behavior, sslmode=disable takes precedence, meaning the connection will NOT be encrypted despite having valid TLS certificates configured.

Inconsistency with codebase

Reference code

From openspec/changes/add-mtls-only-edge-onboarding/design.md:28:

CNPG now runs with TLS (server cert from compose CA) and compose clients default to `sslmode=verify-full`

From openspec/changes/update-helm-demo-chart/tasks.md:36:

DB event writer now mounts the `cnpg-ca` secret and points its CNPG TLS CA file to `Values.cnpg.caFile`
(defaults to `/etc/serviceradar/cnpg/ca.crt`) so SPIFFE Postgres connections can verify the CNPG server cert.

Current code

pkg/db/cnpg_pool.go:63-67

sslMode := cnpg.SSLMode
if sslMode == "" {
	sslMode = "disable"
}
query.Set("sslmode", sslMode)

Contradiction

The documentation explicitly states that CNPG connections use TLS and that clients should use sslmode=verify-full. However, the code defaults to sslmode=disable when no explicit SSL mode is provided. This creates a situation where:

Users who configure TLS by providing certificates (as documented) but omit the SSLMode field will have their connections silently downgraded to unencrypted
The TLS configuration (certificates, keys, CA) is successfully loaded but never used because sslmode=disable tells the driver to skip SSL entirely

Failing test

Test script

package main_test

import (
	"fmt"
	"net/url"
	"os"
	"path/filepath"
	"testing"

	"github.com/carverauto/serviceradar/pkg/models"
)

// This test demonstrates that when TLS configuration is provided but SSLMode is not set,
// the connection URL will have sslmode=disable, which conflicts with the TLS config.
func TestCNPGPoolSSLModeDefaultConflictsWithTLS(t *testing.T) {
	// Create a temporary directory for test certificates
	tmpDir := t.TempDir()

	// Create dummy cert files (they don't need to be valid for this test)
	certFile := filepath.Join(tmpDir, "client.crt")
	keyFile := filepath.Join(tmpDir, "client.key")
	caFile := filepath.Join(tmpDir, "ca.crt")

	// Write dummy content
	if err := os.WriteFile(certFile, []byte("dummy cert"), 0600); err != nil {
		t.Fatal(err)
	}
	if err := os.WriteFile(keyFile, []byte("dummy key"), 0600); err != nil {
		t.Fatal(err)
	}
	if err := os.WriteFile(caFile, []byte("dummy ca"), 0600); err != nil {
		t.Fatal(err)
	}

	cfg := &models.CNPGDatabase{
		Host:     "localhost",
		Port:     5432,
		Database: "testdb",
		Username: "testuser",
		Password: "testpass",
		// NOTE: SSLMode is intentionally not set, which is a valid scenario
		// when users want to use TLS configuration
		CertDir:  tmpDir,
		TLS: &models.TLSConfig{
			CertFile: "client.crt",
			KeyFile:  "client.key",
			CAFile:   "ca.crt",
		},
	}

	// Create a connection URL to examine what sslmode would be set
	connURL := url.URL{
		Scheme: "postgres",
		Host:   fmt.Sprintf("%s:%d", cfg.Host, cfg.Port),
		Path:   "/" + cfg.Database,
		User:   url.UserPassword(cfg.Username, cfg.Password),
	}

	query := connURL.Query()

	// This simulates the logic in NewCNPGPool lines 63-67
	sslMode := cfg.SSLMode
	if sslMode == "" {
		sslMode = "disable"
	}
	query.Set("sslmode", sslMode)

	connURL.RawQuery = query.Encode()

	// Check the connection string
	connStr := connURL.String()

	t.Logf("Connection string: %s", connStr)

	// The bug: sslmode is set to "disable" even though TLS config is provided
	if query.Get("sslmode") != "disable" {
		t.Errorf("Expected sslmode to be 'disable', got: %s", query.Get("sslmode"))
	}

	// The TLS config would be set (if we could construct the pool without connecting)
	if cfg.TLS == nil {
		t.Errorf("TLS config should be set")
	}

	// This is the contradiction: we have TLS config but sslmode=disable
	if cfg.TLS != nil && query.Get("sslmode") == "disable" {
		t.Errorf("BUG DETECTED: TLS configuration is provided but sslmode is 'disable'. " +
			"This will cause the TLS configuration to be ignored and the connection will not be encrypted.")
	}
}

func TestCNPGPoolSSLModeExplicitlySetWorksWithTLS(t *testing.T) {
	cfg := &models.CNPGDatabase{
		Host:     "localhost",
		Port:     5432,
		Database: "testdb",
		Username: "testuser",
		Password: "testpass",
		SSLMode:  "require", // Explicitly set
		TLS: &models.TLSConfig{
			CertFile: "client.crt",
			KeyFile:  "client.key",
			CAFile:   "ca.crt",
		},
	}

	// Create a connection URL to examine what sslmode would be set
	connURL := url.URL{
		Scheme: "postgres",
		Host:   fmt.Sprintf("%s:%d", cfg.Host, cfg.Port),
		Path:   "/" + cfg.Database,
		User:   url.UserPassword(cfg.Username, cfg.Password),
	}

	query := connURL.Query()

	// This simulates the logic in NewCNPGPool lines 63-67
	sslMode := cfg.SSLMode
	if sslMode == "" {
		sslMode = "disable"
	}
	query.Set("sslmode", sslMode)

	// When SSLMode is explicitly set, this works correctly
	if query.Get("sslmode") != "require" {
		t.Errorf("Expected sslmode to be 'require', got: %s", query.Get("sslmode"))
	}
}

Test output

=== RUN   TestCNPGPoolSSLModeDefaultConflictsWithTLS
    cnpg_sslmode_test.go:73: Connection string: postgres://testuser:testpass@localhost:5432/testdb?sslmode=disable
    cnpg_sslmode_test.go:87: BUG DETECTED: TLS configuration is provided but sslmode is 'disable'. This will cause the TLS configuration to be ignored and the connection will not be encrypted.
--- FAIL: TestCNPGPoolSSLModeDefaultConflictsWithTLS (0.00s)
=== RUN   TestCNPGPoolSSLModeExplicitlySetWorksWithTLS
--- PASS: TestCNPGPoolSSLModeExplicitlySetWorksWithTLS (0.00s)
FAIL
FAIL	command-line-arguments	0.006s
FAIL

Full context

The NewCNPGPool function is called during database initialization by several components:

Core service (pkg/db/db.go:73): The main application calls newCNPGPool during startup to establish the primary database connection pool used for all telemetry data, device updates, and service registry operations.
Migration tool (cmd/tools/cnpg-migrate/main.go:127): The cnpg-migrate CLI tool calls NewCNPGPool to connect to the database and apply schema migrations.
DB event writer: As referenced in the documentation, the db-event-writer service connects to CNPG to ingest OTEL logs, metrics, and traces.

The CNPG cluster is the primary system of record for ServiceRadar, storing:

Timescale hypertables for metrics (CPU, memory, disk, network)
Apache AGE graph data for device relationships
Service registry and configuration
OTEL telemetry data
Device discovery and inventory information

The codebase is designed to operate in production environments with SPIFFE mTLS enabled and TLS connections to the database. The compose-based deployments and Kubernetes/Helm configurations all provision TLS certificates for CNPG connections. Users following the documented deployment patterns would naturally provide TLS configuration without explicitly setting the SSLMode field, as the presence of certificates would imply that TLS should be used.

External documentation

PostgreSQL SSL Support Documentation

sslmode

This option determines whether or with what priority a secure SSL TCP/IP connection will be negotiated with the server. There are six modes:

disable
    only try a non-SSL connection

allow
    first try a non-SSL connection; if that fails, try an SSL connection

prefer (default)
    first try an SSL connection; if that fails, try a non-SSL connection

require
    only try an SSL connection. If a root certificate file is present, verify the certificate in the same way as if verify-ca was specified

verify-ca
    only try an SSL connection, and verify that the server certificate is issued by a trusted certificate authority (CA)

verify-full
    only try an SSL connection, verify that the server certificate is issued by a trusted CA and that the requested server host name matches that in the certificate

pgx Connection Config Documentation

TLSConfig is the configuration for TLS connections. If nil, TLS will not be used.

However, the sslmode parameter in the connection string takes precedence. When sslmode=disable,
no TLS connection will be attempted regardless of TLSConfig.

Why has this bug gone undetected?

This bug has gone undetected for several reasons:

Workaround in deployment configurations: Most deployments explicitly set the SSLMode field to "require", "verify-ca", or "verify-full" in their configuration files, which bypasses the buggy default. The documentation references (like "compose clients default to sslmode=verify-full") suggest that configurations are explicitly setting this value.
Local development with SSL disabled: Developers testing locally may run CNPG without TLS (using the default sslmode=disable), so they never configure TLS certificates and therefore never encounter the conflicting configuration scenario.
Silent failure mode: When sslmode=disable is set, the connection succeeds but is simply unencrypted. There's no error message or warning indicating that the TLS configuration was ignored. The application functions normally, just without the expected encryption.
Recent TLS adoption: Based on the git history and documentation references, TLS support for CNPG appears to be a relatively recent addition (references in "add-mtls-only-edge-onboarding" and related changes). The default to "disable" may have been established before TLS configuration was fully implemented.
Testing gap: There are no tests for the cnpg_pool.go file, and the integration tests that use CNPG either explicitly set SSLMode or run without TLS entirely.
Configuration validation gap: The code doesn't validate the relationship between TLS configuration and SSLMode. It accepts both being set in conflicting ways without warning.

Recommended fix

The function should check if TLS configuration is provided and automatically use an appropriate SSL mode if none is explicitly set:

sslMode := cnpg.SSLMode
if sslMode == "" {
	// When TLS config is provided, default to verify-full for maximum security
	// Otherwise, default to disable for backward compatibility with non-TLS setups
	if cnpg.TLS != nil {
		sslMode = "verify-full"  // <-- FIX 🟢 Use verify-full when TLS config exists
	} else {
		sslMode = "disable"
	}
}
query.Set("sslmode", sslMode)

Alternatively, for a more conservative fix that maintains stricter backward compatibility:

sslMode := cnpg.SSLMode
if sslMode == "" {
	if cnpg.TLS != nil {
		sslMode = "require"  // <-- FIX 🟢 Use require as minimum when TLS config exists
	} else {
		sslMode = "disable"
	}
}
query.Set("sslmode", sslMode)

The first approach ("verify-full") is recommended because:

It provides the highest security by validating both the certificate chain and the server hostname
It aligns with the documented expectation that "compose clients default to sslmode=verify-full"
When users provide TLS certificates, they typically intend to verify the server identity

Imported from GitHub. Original GitHub issue: #2143 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/issues/2143 Original created: 2025-12-16T05:15:41Z --- # Summary - **Context**: `NewCNPGPool` in `pkg/db/cnpg_pool.go` creates a pgx connection pool for connecting to CloudNativePG/Timescale databases, handling both the connection URL configuration and optional TLS setup. - **Bug**: When TLS configuration is provided but `SSLMode` is not explicitly set, the function defaults `sslmode` to `"disable"`, which instructs PostgreSQL to completely disable SSL/TLS encryption. - **Actual vs. expected**: The connection URL has `sslmode=disable` even when a complete TLS configuration (certificates, keys, CA) is provided, causing the TLS configuration to be ignored; the expected behavior is that when TLS configuration is provided, an appropriate SSL mode like `"require"`, `"verify-ca"`, or `"verify-full"` should be used. - **Impact**: Database connections that should be encrypted remain unencrypted, exposing sensitive data in transit and violating security requirements for production deployments that rely on TLS/mTLS for database connections. # Code with bug ```go sslMode := cnpg.SSLMode if sslMode == "" { sslMode = "disable" // <-- BUG 🔴 Defaults to disable even when TLS config is provided } query.Set("sslmode", sslMode) ``` Later in the same function: ```go if tlsConfig, err := buildCNPGTLSConfig(&cnpg); err != nil { return nil, err } else if tlsConfig != nil { poolConfig.ConnConfig.TLSConfig = tlsConfig // TLS config is set but sslmode is already "disable" } ``` # Evidence ## Example Consider a configuration where a user provides TLS certificates to connect to a CNPG cluster: ```go cfg := &models.CNPGDatabase{ Host: "cnpg-rw.serviceradar", Port: 5432, Database: "serviceradar", Username: "app_user", Password: "secret", // SSLMode is not set (common when users assume TLS config implies SSL) CertDir: "/etc/serviceradar/cnpg", TLS: &models.TLSConfig{ CertFile: "client.crt", KeyFile: "client.key", CAFile: "ca.crt", }, } ``` **Step-by-step execution:** 1. Line 63-67: Since `cfg.SSLMode` is empty string, `sslMode` is set to `"disable"` 2. Line 67: The connection URL query gets `sslmode=disable` 3. Line 83: The connection string is parsed: `postgres://app_user:secret@cnpg-rw.serviceradar:5432/serviceradar?sslmode=disable` 4. Lines 121-125: The TLS config is built successfully and set on `poolConfig.ConnConfig.TLSConfig` 5. **Result**: pgx sees both `sslmode=disable` in the connection string AND a TLS config object. According to PostgreSQL/pgx behavior, `sslmode=disable` takes precedence, meaning the connection will NOT be encrypted despite having valid TLS certificates configured. ## Inconsistency with codebase ### Reference code From `openspec/changes/add-mtls-only-edge-onboarding/design.md:28`: ``` CNPG now runs with TLS (server cert from compose CA) and compose clients default to `sslmode=verify-full` ``` From `openspec/changes/update-helm-demo-chart/tasks.md:36`: ``` DB event writer now mounts the `cnpg-ca` secret and points its CNPG TLS CA file to `Values.cnpg.caFile` (defaults to `/etc/serviceradar/cnpg/ca.crt`) so SPIFFE Postgres connections can verify the CNPG server cert. ``` ### Current code `pkg/db/cnpg_pool.go:63-67` ```go sslMode := cnpg.SSLMode if sslMode == "" { sslMode = "disable" } query.Set("sslmode", sslMode) ``` ### Contradiction The documentation explicitly states that CNPG connections use TLS and that clients should use `sslmode=verify-full`. However, the code defaults to `sslmode=disable` when no explicit SSL mode is provided. This creates a situation where: 1. Users who configure TLS by providing certificates (as documented) but omit the `SSLMode` field will have their connections silently downgraded to unencrypted 2. The TLS configuration (certificates, keys, CA) is successfully loaded but never used because `sslmode=disable` tells the driver to skip SSL entirely ## Failing test ### Test script ```go package main_test import ( "fmt" "net/url" "os" "path/filepath" "testing" "github.com/carverauto/serviceradar/pkg/models" ) // This test demonstrates that when TLS configuration is provided but SSLMode is not set, // the connection URL will have sslmode=disable, which conflicts with the TLS config. func TestCNPGPoolSSLModeDefaultConflictsWithTLS(t *testing.T) { // Create a temporary directory for test certificates tmpDir := t.TempDir() // Create dummy cert files (they don't need to be valid for this test) certFile := filepath.Join(tmpDir, "client.crt") keyFile := filepath.Join(tmpDir, "client.key") caFile := filepath.Join(tmpDir, "ca.crt") // Write dummy content if err := os.WriteFile(certFile, []byte("dummy cert"), 0600); err != nil { t.Fatal(err) } if err := os.WriteFile(keyFile, []byte("dummy key"), 0600); err != nil { t.Fatal(err) } if err := os.WriteFile(caFile, []byte("dummy ca"), 0600); err != nil { t.Fatal(err) } cfg := &models.CNPGDatabase{ Host: "localhost", Port: 5432, Database: "testdb", Username: "testuser", Password: "testpass", // NOTE: SSLMode is intentionally not set, which is a valid scenario // when users want to use TLS configuration CertDir: tmpDir, TLS: &models.TLSConfig{ CertFile: "client.crt", KeyFile: "client.key", CAFile: "ca.crt", }, } // Create a connection URL to examine what sslmode would be set connURL := url.URL{ Scheme: "postgres", Host: fmt.Sprintf("%s:%d", cfg.Host, cfg.Port), Path: "/" + cfg.Database, User: url.UserPassword(cfg.Username, cfg.Password), } query := connURL.Query() // This simulates the logic in NewCNPGPool lines 63-67 sslMode := cfg.SSLMode if sslMode == "" { sslMode = "disable" } query.Set("sslmode", sslMode) connURL.RawQuery = query.Encode() // Check the connection string connStr := connURL.String() t.Logf("Connection string: %s", connStr) // The bug: sslmode is set to "disable" even though TLS config is provided if query.Get("sslmode") != "disable" { t.Errorf("Expected sslmode to be 'disable', got: %s", query.Get("sslmode")) } // The TLS config would be set (if we could construct the pool without connecting) if cfg.TLS == nil { t.Errorf("TLS config should be set") } // This is the contradiction: we have TLS config but sslmode=disable if cfg.TLS != nil && query.Get("sslmode") == "disable" { t.Errorf("BUG DETECTED: TLS configuration is provided but sslmode is 'disable'. " + "This will cause the TLS configuration to be ignored and the connection will not be encrypted.") } } func TestCNPGPoolSSLModeExplicitlySetWorksWithTLS(t *testing.T) { cfg := &models.CNPGDatabase{ Host: "localhost", Port: 5432, Database: "testdb", Username: "testuser", Password: "testpass", SSLMode: "require", // Explicitly set TLS: &models.TLSConfig{ CertFile: "client.crt", KeyFile: "client.key", CAFile: "ca.crt", }, } // Create a connection URL to examine what sslmode would be set connURL := url.URL{ Scheme: "postgres", Host: fmt.Sprintf("%s:%d", cfg.Host, cfg.Port), Path: "/" + cfg.Database, User: url.UserPassword(cfg.Username, cfg.Password), } query := connURL.Query() // This simulates the logic in NewCNPGPool lines 63-67 sslMode := cfg.SSLMode if sslMode == "" { sslMode = "disable" } query.Set("sslmode", sslMode) // When SSLMode is explicitly set, this works correctly if query.Get("sslmode") != "require" { t.Errorf("Expected sslmode to be 'require', got: %s", query.Get("sslmode")) } } ``` ### Test output ``` === RUN TestCNPGPoolSSLModeDefaultConflictsWithTLS cnpg_sslmode_test.go:73: Connection string: postgres://testuser:testpass@localhost:5432/testdb?sslmode=disable cnpg_sslmode_test.go:87: BUG DETECTED: TLS configuration is provided but sslmode is 'disable'. This will cause the TLS configuration to be ignored and the connection will not be encrypted. --- FAIL: TestCNPGPoolSSLModeDefaultConflictsWithTLS (0.00s) === RUN TestCNPGPoolSSLModeExplicitlySetWorksWithTLS --- PASS: TestCNPGPoolSSLModeExplicitlySetWorksWithTLS (0.00s) FAIL FAIL command-line-arguments 0.006s FAIL ``` # Full context The `NewCNPGPool` function is called during database initialization by several components: 1. **Core service** (`pkg/db/db.go:73`): The main application calls `newCNPGPool` during startup to establish the primary database connection pool used for all telemetry data, device updates, and service registry operations. 2. **Migration tool** (`cmd/tools/cnpg-migrate/main.go:127`): The `cnpg-migrate` CLI tool calls `NewCNPGPool` to connect to the database and apply schema migrations. 3. **DB event writer**: As referenced in the documentation, the db-event-writer service connects to CNPG to ingest OTEL logs, metrics, and traces. The CNPG cluster is the primary system of record for ServiceRadar, storing: - Timescale hypertables for metrics (CPU, memory, disk, network) - Apache AGE graph data for device relationships - Service registry and configuration - OTEL telemetry data - Device discovery and inventory information The codebase is designed to operate in production environments with SPIFFE mTLS enabled and TLS connections to the database. The compose-based deployments and Kubernetes/Helm configurations all provision TLS certificates for CNPG connections. Users following the documented deployment patterns would naturally provide TLS configuration without explicitly setting the `SSLMode` field, as the presence of certificates would imply that TLS should be used. ## External documentation - [PostgreSQL SSL Support Documentation](https://www.postgresql.org/docs/current/libpq-ssl.html) ``` sslmode This option determines whether or with what priority a secure SSL TCP/IP connection will be negotiated with the server. There are six modes: disable only try a non-SSL connection allow first try a non-SSL connection; if that fails, try an SSL connection prefer (default) first try an SSL connection; if that fails, try a non-SSL connection require only try an SSL connection. If a root certificate file is present, verify the certificate in the same way as if verify-ca was specified verify-ca only try an SSL connection, and verify that the server certificate is issued by a trusted certificate authority (CA) verify-full only try an SSL connection, verify that the server certificate is issued by a trusted CA and that the requested server host name matches that in the certificate ``` - [pgx Connection Config Documentation](https://pkg.go.dev/github.com/jackc/pgx/v5#ConnConfig) ``` TLSConfig is the configuration for TLS connections. If nil, TLS will not be used. However, the sslmode parameter in the connection string takes precedence. When sslmode=disable, no TLS connection will be attempted regardless of TLSConfig. ``` # Why has this bug gone undetected? This bug has gone undetected for several reasons: 1. **Workaround in deployment configurations**: Most deployments explicitly set the `SSLMode` field to `"require"`, `"verify-ca"`, or `"verify-full"` in their configuration files, which bypasses the buggy default. The documentation references (like "compose clients default to `sslmode=verify-full`") suggest that configurations are explicitly setting this value. 2. **Local development with SSL disabled**: Developers testing locally may run CNPG without TLS (using the default `sslmode=disable`), so they never configure TLS certificates and therefore never encounter the conflicting configuration scenario. 3. **Silent failure mode**: When `sslmode=disable` is set, the connection succeeds but is simply unencrypted. There's no error message or warning indicating that the TLS configuration was ignored. The application functions normally, just without the expected encryption. 4. **Recent TLS adoption**: Based on the git history and documentation references, TLS support for CNPG appears to be a relatively recent addition (references in "add-mtls-only-edge-onboarding" and related changes). The default to `"disable"` may have been established before TLS configuration was fully implemented. 5. **Testing gap**: There are no tests for the `cnpg_pool.go` file, and the integration tests that use CNPG either explicitly set `SSLMode` or run without TLS entirely. 6. **Configuration validation gap**: The code doesn't validate the relationship between `TLS` configuration and `SSLMode`. It accepts both being set in conflicting ways without warning. # Recommended fix The function should check if TLS configuration is provided and automatically use an appropriate SSL mode if none is explicitly set: ```go sslMode := cnpg.SSLMode if sslMode == "" { // When TLS config is provided, default to verify-full for maximum security // Otherwise, default to disable for backward compatibility with non-TLS setups if cnpg.TLS != nil { sslMode = "verify-full" // <-- FIX 🟢 Use verify-full when TLS config exists } else { sslMode = "disable" } } query.Set("sslmode", sslMode) ``` Alternatively, for a more conservative fix that maintains stricter backward compatibility: ```go sslMode := cnpg.SSLMode if sslMode == "" { if cnpg.TLS != nil { sslMode = "require" // <-- FIX 🟢 Use require as minimum when TLS config exists } else { sslMode = "disable" } } query.Set("sslmode", sslMode) ``` The first approach (`"verify-full"`) is recommended because: - It provides the highest security by validating both the certificate chain and the server hostname - It aligns with the documented expectation that "compose clients default to `sslmode=verify-full`" - When users provide TLS certificates, they typically intend to verify the server identity

mfreeman451 commented

2026-03-28 04:27:23 +00:00

Author

Owner

Imported GitHub comment.

Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/issues/2143#issuecomment-3663542651
Original created: 2025-12-17T04:04:35Z

closing as completed

Imported GitHub comment. Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/issues/2143#issuecomment-3663542651 Original created: 2025-12-17T04:04:35Z --- closing as completed