scrubbedsecrets (#10120) (#10151)

Co-authored-by: ashish b <108897222+ashishb-solo@users.noreply.github.com>
Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>

changelog/v1.18.0-beta25/strict-tls-validation.yaml

@@ -0,0 +1,9 @@
+changelog:
+  - type: FIX
+    issueLink: https://github.com/solo-io/solo-projects/issues/6772
+    resolvesIssue: false
+    description: >-
+      Changes to downgrade the level of strictness added in 1.17.8 for TLS secret validation.
+      We still validate several pieces of the cert but also scrub down to usable bits. This means we avoid envoy nacks while allowing more non-useful or functional info in our TLS secrets.
+      This means we are RFC compliant but it may mean that there could exist some cert data in an edge case which we will elide from envoy but previously would be nacked.
+  

projects/gateway2/translator/listener/gateway_listener_translator.go

@@ -485,7 +485,9 @@ func translateSslConfig(
 		if err != nil {
 			return nil, err
 		}
-		if err := sslutils.ValidateTlsSecret(secret.(*corev1.Secret)); err != nil {
+		// The resulting sslconfig will still have to go through a real translation where we run through this again.
+		// This means that while its nice to still fail early here we dont need to scrub the actual contents of the secret.
+		if _, err := sslutils.ValidateTlsSecret(secret.(*corev1.Secret)); err != nil {
 			return nil, err
 		}
 

projects/gateway2/translator/sslutils/ssl_utils.go

@@ -3,7 +3,6 @@ package sslutils
 import (
 	"crypto/tls"
 	"fmt"
-	"strings"
 
 	"github.com/rotisserie/eris"
 	corev1 "k8s.io/api/core/v1"
@@ -19,59 +18,44 @@ var (
 	NoCertificateFoundError = eris.New("no certificate information found")
 )
 
-func ValidateTlsSecret(secret *corev1.Secret) error {
-	err := validatedCertData(secret)
-	if err != nil {
-		return err
-	}
-	return nil
-}
+// ValidateTlsSecret and return a cleaned cert
+func ValidateTlsSecret(sslSecret *corev1.Secret) (cleanedCertChain string, err error) {
 
-func validatedCertData(sslSecret *corev1.Secret) error {
-	certChain := sslSecret.Data[corev1.TLSCertKey]
-	privateKey := sslSecret.Data[corev1.TLSPrivateKeyKey]
-	rootCa := sslSecret.Data[corev1.ServiceAccountRootCAKey]
+	certChain := string(sslSecret.Data[corev1.TLSCertKey])
+	privateKey := string(sslSecret.Data[corev1.TLSPrivateKeyKey])
+	rootCa := string(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)
+	cleanedCertChain, err = cleanedSslKeyPair(certChain, privateKey, rootCa)
 	if err != nil {
-		return InvalidTlsSecretError(sslSecret, err)
+		err = InvalidTlsSecretError(sslSecret, err)
 	}
-
-	return nil
+	return cleanedCertChain, err
 }
 
-// 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 {
+func cleanedSslKeyPair(certChain, privateKey, rootCa string) (cleanedChain string, err error) {
 
-	if len(certChain) == 0 || len(privateKey) == 0 {
-		return NoCertificateFoundError
+	// in the case where we _only_ provide a rootCa, we do not want to validate tls.key+tls.cert
+	if (certChain == "") && (privateKey == "") && (rootCa != "") {
+		return certChain, nil
 	}
 
-	_, err := tls.X509KeyPair(certChain, privateKey)
+	// validate that the cert and key are a valid pair
+	_, err = tls.X509KeyPair([]byte(certChain), []byte(privateKey))
 	if err != nil {
-		return err
+		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...)
+	candidateCert, err := cert.ParseCertsPEM([]byte(certChain))
 	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 rather than sanitize. This is to maintain UX with older versions and to keep in line with gateway2 pkg.
+		return "", err
 	}
+	cleanedChainBytes, err := cert.EncodeCertificates(candidateCert...)
+	cleanedChain = string(cleanedChainBytes)
 
-	return err
+	return cleanedChain, err
 }

projects/gloo/pkg/utils/ssl.go

@@ -3,7 +3,6 @@ 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"
@@ -330,10 +329,12 @@ func (s *sslConfigTranslator) ResolveCommonSslConfig(cs CertSource, secrets v1.S
 		certChain, privateKey, rootCa = sslFiles.GetTlsCert(), sslFiles.GetTlsKey(), sslFiles.GetRootCa()
 		// Since ocspStaple is []byte, but we want the file path, we're storing it in a separate string variable
 		ocspStapleFile = sslFiles.GetOcspStaple()
-		err := isValidSslKeyPair(certChain, privateKey, rootCa)
+
+		cleanCertChain, err := cleanedSslKeyPair(certChain, privateKey, rootCa)
 		if err != nil {
 			return nil, InvalidTlsSecretError(nil, err)
 		}
+		certChain = cleanCertChain
 	} else if sslSds := cs.GetSds(); sslSds != nil {
 		tlsContext, err := s.handleSds(sslSds, VerifySanListToMatchSanList(cs.GetVerifySubjectAltName()))
 		if err != nil {
@@ -431,30 +432,25 @@ 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)
+	certChain, err = cleanedSslKeyPair(certChain, privateKey, rootCa)
 	if err != nil {
 		return "", "", "", nil, InvalidTlsSecretError(secret.GetMetadata().Ref(), err)
 	}
 
 	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 {
+func cleanedSslKeyPair(certChain, privateKey, rootCa string) (cleanedChain string, err 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
+		return certChain, nil
 	}
 
 	// validate that the cert and key are a valid pair
-	_, err := tls.X509KeyPair([]byte(certChain), []byte(privateKey))
+	_, err = tls.X509KeyPair([]byte(certChain), []byte(privateKey))
 	if err != nil {
-		return err
+		return "", err
 	}
 
 	// validate that the parsed piece is valid
@@ -463,18 +459,13 @@ func isValidSslKeyPair(certChain, privateKey, rootCa string) error {
 	// 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 rather than sanitize. This is to maintain UX with older versions and to keep in line with gateway2 pkg.
+		return "", err
 	}
+	cleanedChainBytes, err := cert.EncodeCertificates(candidateCert...)
+	cleanedChain = string(cleanedChainBytes)
 
-	return err
+	return cleanedChain, err
 }
 
 func (s *sslConfigTranslator) ResolveSslParamsConfig(params *ssl.SslParameters) (*envoyauth.TlsParameters, error) {

projects/gloo/pkg/utils/ssl_test.go

@@ -177,13 +177,32 @@ 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",
+		DescribeTable("should fail if a cert which is invalid for envoy 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())
+				Expect(err).NotTo(HaveOccurred())
+
+			},
+			Entry("upstreamCfg", func() utils.CertSource { return upstreamCfg }),
+			Entry("downstreamCfg", func() utils.CertSource { return downstreamCfg }),
+		)
+		DescribeTable("should scrub invalid non-certs like bad headers (no space after header definition) ",
+			func(c func() utils.CertSource) {
+				// invalid headers should not make envoy puke.
+				// when shared as is without scrubbing envoy nacks.
+				tlsSecret.CertChain += `# subject
+
+
+
+
+-----BEGIN HEADERS-----
+Header: 1
+-----END HEADERS-------`
+				_, err := resolveCommonSslConfig(c(), secrets)
+				Expect(err).NotTo(HaveOccurred())
 
 			},
 			Entry("upstreamCfg", func() utils.CertSource { return upstreamCfg }),