From 24d6eee07bc004dc852184af3d5f553f2f916ffc Mon Sep 17 00:00:00 2001
From: Kenneth Jenkins <51246568+kenjenkins@users.noreply.github.com>
Date: Thu, 29 Aug 2024 12:22:59 -0700
Subject: [PATCH] config: generate fallback cert only as last resort

Currently Pomerium will generate a self-signed wildcard certificate for
use as a fallback certificate.

If any other certificate is configured, this self-signed certificate
will not normally be presented, except in the case of a TLS connection
where the client does not include the Server Name Indication (SNI)
extension. All modern browsers support SNI, so in practice this
certificate should never be presented to end users.

However, some network scanning tools will probe connections by IP
addresses, and so this self-signed certificate may be presented. The
presence of a self-signed certificate may be flagged as a problem.

Let's avoid generating this self-signed certificate if Pomerium has any
other certificate configured. This should prevent false positive reports
from these particular vulnerability scans.
---
 config/envoyconfig/tls.go      | 14 ++++---
 config/envoyconfig/tls_test.go | 71 ++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 5 deletions(-)

diff --git a/config/envoyconfig/tls.go b/config/envoyconfig/tls.go
index abf1d51c7..d35a13080 100644
--- a/config/envoyconfig/tls.go
+++ b/config/envoyconfig/tls.go
@@ -219,12 +219,16 @@ func getAllCertificates(cfg *config.Config) ([]tls.Certificate, error) {
 		return nil, fmt.Errorf("error collecting all certificates: %w", err)
 	}
 
-	wc, err := cfg.GenerateCatchAllCertificate()
-	if err != nil {
-		return nil, fmt.Errorf("error getting wildcard certificate: %w", err)
+	// Generate a fallback certificate only if explicitly requested, or if there
+	// are no other available certificates.
+	if cfg.Options.DeriveInternalDomainCert != nil || len(allCertificates) == 0 {
+		wc, err := cfg.GenerateCatchAllCertificate()
+		if err != nil {
+			return nil, fmt.Errorf("error generating wildcard certificate: %w", err)
+		}
+		allCertificates = append(allCertificates, *wc)
 	}
-
-	return append(allCertificates, *wc), nil
+	return allCertificates, nil
 }
 
 // validateCertificate validates that a certificate can be used with Envoy's TLS stack.
diff --git a/config/envoyconfig/tls_test.go b/config/envoyconfig/tls_test.go
index e8bd264ab..2b1129157 100644
--- a/config/envoyconfig/tls_test.go
+++ b/config/envoyconfig/tls_test.go
@@ -396,3 +396,74 @@ func Test_clientCABundle(t *testing.T) {
 	actual := clientCABundle(context.Background(), cfg)
 	assert.Equal(t, expected, actual)
 }
+
+func Test_getAllCertificates(t *testing.T) {
+	t.Run("fallback cert", func(t *testing.T) {
+		// If no certificate is configured, a fallback certificate should be generated.
+		cfg := &config.Config{Options: &config.Options{
+			SharedKey: base64.StdEncoding.EncodeToString([]byte("ABCDEFGHIJKLMNOPQRSTUVWXYZ123456")),
+		}}
+		certs, err := getAllCertificates(cfg)
+
+		require.NoError(t, err)
+		require.Len(t, certs, 1)
+		parsed, err := x509.ParseCertificate(certs[0].Certificate[0])
+		require.NoError(t, err)
+		assert.Equal(t, "CN=Pomerium PSK CA,O=Pomerium", parsed.Issuer.String())
+		assert.Equal(t, "O=Pomerium", parsed.Subject.String())
+	})
+	t.Run("no fallback cert", func(t *testing.T) {
+		// If some certificate is configured, the fallback certificate should not be generated.
+		cfg := &config.Config{Options: &config.Options{
+			Cert: base64.StdEncoding.EncodeToString([]byte(testServerCert)),
+			Key:  base64.StdEncoding.EncodeToString([]byte(testServerKey)),
+		}}
+		certs, err := getAllCertificates(cfg)
+
+		require.NoError(t, err)
+		require.Len(t, certs, 1)
+		parsed, err := x509.ParseCertificate(certs[0].Certificate[0])
+		require.NoError(t, err)
+		assert.Equal(t, "CN=Test Root CA", parsed.Issuer.String())
+		assert.Equal(t, "CN=server cert 1", parsed.Subject.String())
+	})
+	t.Run("derive internal domain cert", func(t *testing.T) {
+		// If the generated certificate is explicitly configured, then it should still be added.
+		cfg := &config.Config{Options: &config.Options{
+			Cert:      base64.StdEncoding.EncodeToString([]byte(testServerCert)),
+			Key:       base64.StdEncoding.EncodeToString([]byte(testServerKey)),
+			SharedKey: base64.StdEncoding.EncodeToString([]byte("ABCDEFGHIJKLMNOPQRSTUVWXYZ123456")),
+
+			DeriveInternalDomainCert: ptr("example.com"),
+		}}
+		certs, err := getAllCertificates(cfg)
+
+		require.NoError(t, err)
+		require.Len(t, certs, 2)
+		parsed, err := x509.ParseCertificate(certs[0].Certificate[0])
+		require.NoError(t, err)
+		assert.Equal(t, "CN=Test Root CA", parsed.Issuer.String())
+		assert.Equal(t, "CN=server cert 1", parsed.Subject.String())
+		parsed, err = x509.ParseCertificate(certs[1].Certificate[0])
+		require.NoError(t, err)
+		assert.Equal(t, "CN=Pomerium PSK CA,O=Pomerium", parsed.Issuer.String())
+		assert.Equal(t, "O=Pomerium", parsed.Subject.String())
+	})
+}
+
+var testServerCert = `-----BEGIN CERTIFICATE-----
+MIIBezCCASCgAwIBAgICEAEwCgYIKoZIzj0EAwIwFzEVMBMGA1UEAxMMVGVzdCBS
+b290IENBMCIYDzAwMDEwMTAxMDAwMDAwWhgPMDAwMTAxMDEwMDAwMDBaMBgxFjAU
+BgNVBAMTDXNlcnZlciBjZXJ0IDEwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAARI
+J6Cnzb7wY/C7DMxbZT3UFhEsUHq6hP80dtzmK3ix5v47q30wuYBXOwZckvvMSTXv
+h8vYNLDRk2Zk8FF4rP9Ro1cwVTATBgNVHSUEDDAKBggrBgEFBQcDATAfBgNVHSME
+GDAWgBT25A7+YE2uHr8pRVFJzt8xHsdPtzAdBgNVHREEFjAUghJzZXJ2ZXIuZXhh
+bXBsZS5jb20wCgYIKoZIzj0EAwIDSQAwRgIhAJwuu9y6AP9GGdo88YmB14uWC/fx
+ZNhtP7zjrvgObX7UAiEA4gFcmeZnWbcpuVSZDEFfMIfd/Nys8bpg3S8N/PSnJng=
+-----END CERTIFICATE-----`
+
+var testServerKey = `-----BEGIN EC PRIVATE KEY-----
+MHcCAQEEINl/ONFeqvjTLCPaIkcUnEdqXhQ8P3M/3qCjNNYfuJKvoAoGCCqGSM49
+AwEHoUQDQgAESCegp82+8GPwuwzMW2U91BYRLFB6uoT/NHbc5it4seb+O6t9MLmA
+VzsGXJL7zEk174fL2DSw0ZNmZPBReKz/UQ==
+-----END EC PRIVATE KEY-----`