From e658203d0a0b7b479cbb59cfc43832699d25fb1c Mon Sep 17 00:00:00 2001
From: Nathan Fudenberg <nathan.fudenberg@solo.io>
Date: Mon, 16 Sep 2024 19:37:06 -0400
Subject: [PATCH] 1.17: projects/utils/ssl: Add a check for ssl secrets that
 will be rejected by envoy but not go (#10048)

---
 changelog/v1.17.8/strict-tls-validation.yaml  |  9 +++++++
 .../gateway2/translator/sslutils/ssl_utils.go | 24 +++++++++++++++++
 projects/gloo/pkg/utils/ssl.go                | 26 +++++++++++++++++++
 projects/gloo/pkg/utils/ssl_test.go           | 12 +++++++++
 4 files changed, 71 insertions(+)
 create mode 100644 changelog/v1.17.8/strict-tls-validation.yaml

diff --git a/changelog/v1.17.8/strict-tls-validation.yaml b/changelog/v1.17.8/strict-tls-validation.yaml
new file mode 100644
index 00000000000..c7f6bcac029
--- /dev/null
+++ b/changelog/v1.17.8/strict-tls-validation.yaml
@@ -0,0 +1,9 @@
+changelog:
+  - type: FIX
+    issueLink: https://github.com/solo-io/solo-projects/issues/6772
+    resolvesIssue: false
+    description: >-
+      Plugs a gap where go would check a secret for validity per spec but envoy is more aggressive.
+      For example a tls secret with a certchain that contains an invalid pem block will be rejected by envoy but not go.
+      Prior to this pr these types of secrets would be accepted by gloo and nacked by envoy.
+  
\ No newline at end of file
diff --git a/projects/gateway2/translator/sslutils/ssl_utils.go b/projects/gateway2/translator/sslutils/ssl_utils.go
index 190d98b3b1c..368cf2ff2df 100644
--- a/projects/gateway2/translator/sslutils/ssl_utils.go
+++ b/projects/gateway2/translator/sslutils/ssl_utils.go
@@ -3,9 +3,11 @@ package sslutils
 import (
 	"crypto/tls"
 	"fmt"
+	"strings"
 
 	"github.com/rotisserie/eris"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/client-go/util/cert"
 )
 
 var (
@@ -45,5 +47,27 @@ func isValidSslKeyPair(certChain, privateKey, rootCa []byte) error {
 	}
 
 	_, err := tls.X509KeyPair([]byte(certChain), []byte(privateKey))
+	if err != nil {
+		return err
+	}
+	// validate that the parsed piece is valid
+	// this is still faster than a call out to openssl despite this second parsing pass of the cert
+	// pem parsing in go is permissive while envoy is not
+	// this might not be needed once we have larger envoy validation
+	candidateCert, err := cert.ParseCertsPEM(certChain)
+	if err != nil {
+		// return err rather than sanitize. This is to maintain UX with older versions and to prevent a large refactor
+		// for gatewayv2 secret validation code.
+		return err
+	}
+	reencoded, err := cert.EncodeCertificates(candidateCert...)
+	if err != nil {
+		return err
+	}
+	trimmedEncoded := strings.TrimSpace(string(reencoded))
+	if trimmedEncoded != strings.TrimSpace(string(certChain)) {
+		return fmt.Errorf("certificate chain does not match parsed certificate")
+	}
+
 	return err
 }
diff --git a/projects/gloo/pkg/utils/ssl.go b/projects/gloo/pkg/utils/ssl.go
index 37b49c3b1f6..3da89598d08 100644
--- a/projects/gloo/pkg/utils/ssl.go
+++ b/projects/gloo/pkg/utils/ssl.go
@@ -3,6 +3,7 @@ package utils
 import (
 	"crypto/tls"
 	"fmt"
+	"strings"
 
 	envoycore "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
 	envoygrpccredential "github.com/envoyproxy/go-control-plane/envoy/config/grpc_credential/v3"
@@ -14,6 +15,7 @@ import (
 	v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1"
 	"github.com/solo-io/gloo/projects/gloo/pkg/api/v1/ssl"
 	"github.com/solo-io/solo-kit/pkg/api/v1/resources/core"
+	"k8s.io/client-go/util/cert"
 )
 
 //go:generate mockgen -destination mocks/mock_ssl.go github.com/solo-io/gloo/projects/gloo/pkg/utils SslConfigTranslator
@@ -433,7 +435,31 @@ func isValidSslKeyPair(certChain, privateKey, rootCa string) error {
 		return nil
 	}
 
+	// validate that the cert and key are a valid pair
 	_, err := tls.X509KeyPair([]byte(certChain), []byte(privateKey))
+	if err != nil {
+
+		return err
+	}
+
+	// validate that the parsed piece is valid
+	// this is still faster than a call out to openssl despite this second parsing pass of the cert
+	// pem parsing in go is permissive while envoy is not
+	// this might not be needed once we have larger envoy validation
+	candidateCert, err := cert.ParseCertsPEM([]byte(certChain))
+	if err != nil {
+		// return err rather than sanitize. This is to maintain UX with older versions and to keep in line with gateway2 pkg.
+		return err
+	}
+	reencoded, err := cert.EncodeCertificates(candidateCert...)
+	if err != nil {
+		return err
+	}
+	trimmedEncoded := strings.TrimSpace(string(reencoded))
+	if trimmedEncoded != strings.TrimSpace(certChain) {
+		return fmt.Errorf("certificate chain does not match parsed certificate")
+	}
+
 	return err
 }
 
diff --git a/projects/gloo/pkg/utils/ssl_test.go b/projects/gloo/pkg/utils/ssl_test.go
index 17bce372b56..66cb0ab896e 100644
--- a/projects/gloo/pkg/utils/ssl_test.go
+++ b/projects/gloo/pkg/utils/ssl_test.go
@@ -177,6 +177,18 @@ var _ = Describe("Ssl", func() {
 			Entry("upstreamCfg", func() utils.CertSource { return upstreamCfg }),
 			Entry("downstreamCfg", func() utils.CertSource { return downstreamCfg }),
 		)
+		DescribeTable("should fail if invalid for envoy cert is provided",
+			func(c func() utils.CertSource) {
+				// unterminated cert in chain is valid for go b ut not envoy
+				tlsSecret.CertChain += `-----BEGIN CERTIFICATE-----
+MIID6TCCA1ICAQEwDQYJKoZIhvcNAQEFBQAwgYsxCzAJBgNVBAYTAlVTMRMwEQYD`
+				_, err := resolveCommonSslConfig(c(), secrets)
+				Expect(err).To(HaveOccurred())
+
+			},
+			Entry("upstreamCfg", func() utils.CertSource { return upstreamCfg }),
+			Entry("downstreamCfg", func() utils.CertSource { return downstreamCfg }),
+		)
 		DescribeTable("should not have validation context if no rootca",
 			func(c func() utils.CertSource) {
 				tlsSecret.RootCa = ""