projects/utils/ssl: Add a check for ssl secrets that will be rejected by envoy but not go (#10047)

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Bernie Birnbaum <[email protected]>

Author: 3 people

changelog/v1.18.0-beta22/tls-secret-validation.yaml

@@ -0,0 +1,8 @@
+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.

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 (
@@ -30,6 +32,10 @@ func validatedCertData(sslSecret *corev1.Secret) error {
 	privateKey := sslSecret.Data[corev1.TLSPrivateKeyKey]
 	rootCa := sslSecret.Data[corev1.ServiceAccountRootCAKey]
 
+	// we always return an error when the certChain and/or privateKey are invalid
+	// in theory we could propagate only the valid blocks of the certChain (ie the output of cert.ParseCertsPEM(certChain))º
+	// and this would be accepted by Envoy, however we choose to maintain consistency between the secret at rest and in
+	// Envoy, which also maintains consistency with existing UX
 	err := isValidSslKeyPair(certChain, privateKey, rootCa)
 	if err != nil {
 		return InvalidTlsSecretError(sslSecret, err)
@@ -38,12 +44,34 @@ func validatedCertData(sslSecret *corev1.Secret) error {
 	return nil
 }
 
+// isValidSslKeyPair validates that the cert and key are a valid pair
+// It previously only checked in go but now also checks that nothing is lost in cert encoding
 func isValidSslKeyPair(certChain, privateKey, rootCa []byte) error {
 
 	if len(certChain) == 0 || len(privateKey) == 0 {
 		return NoCertificateFoundError
 	}
 
-	_, err := tls.X509KeyPair([]byte(certChain), []byte(privateKey))
+	_, err := tls.X509KeyPair(certChain, 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
+	}
+	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
 }

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
@@ -429,6 +431,10 @@ func getSslSecrets(ref core.ResourceRef, secrets v1.SecretList) (string, string,
 	rootCa := sslSecret.Tls.GetRootCa()
 	ocspStaple := sslSecret.Tls.GetOcspStaple()
 
+	// we always return an error when the certChain and/or privateKey are invalid
+	// in theory we could propagate only the valid blocks of the certChain (ie the output of cert.ParseCertsPEM(certChain))º
+	// and this would be accepted by Envoy, however we choose to maintain consistency between the secret at rest and in
+	// Envoy, which also maintains consistency with existing UX
 	err = isValidSslKeyPair(certChain, privateKey, rootCa)
 	if err != nil {
 		return "", "", "", nil, InvalidTlsSecretError(secret.GetMetadata().Ref(), err)
@@ -437,13 +443,37 @@ func getSslSecrets(ref core.ResourceRef, secrets v1.SecretList) (string, string,
 	return certChain, privateKey, rootCa, ocspStaple, nil
 }
 
+// isValidSslKeyPair validates that the cert and key are a valid pair
+// It previously only checked in go but now also checks that nothing is lost in cert encoding
 func isValidSslKeyPair(certChain, privateKey, rootCa string) error {
 	// in the case where we _only_ provide a rootCa, we do not want to validate tls.key+tls.cert
 	if (certChain == "") && (privateKey == "") && (rootCa != "") {
 		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
+	}
+	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
 }
 

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 = ""