From 84d15a58f340aadc99190bc4f8115f63b6668361 Mon Sep 17 00:00:00 2001 From: Nathan Fudenberg Date: Thu, 3 Oct 2024 13:26:24 -0400 Subject: [PATCH] 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> --- .../v1.18.0-beta25/strict-tls-validation.yaml | 9 +++ .../listener/gateway_listener_translator.go | 4 +- .../gateway2/translator/sslutils/ssl_utils.go | 60 +++++++------------ projects/gloo/pkg/utils/ssl.go | 37 +++++------- projects/gloo/pkg/utils/ssl_test.go | 23 ++++++- 5 files changed, 69 insertions(+), 64 deletions(-) create mode 100644 changelog/v1.18.0-beta25/strict-tls-validation.yaml diff --git a/changelog/v1.18.0-beta25/strict-tls-validation.yaml b/changelog/v1.18.0-beta25/strict-tls-validation.yaml new file mode 100644 index 00000000000..e26380f18c2 --- /dev/null +++ b/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. + \ No newline at end of file diff --git a/projects/gateway2/translator/listener/gateway_listener_translator.go b/projects/gateway2/translator/listener/gateway_listener_translator.go index 53a1f24c245..9bdc64453e5 100644 --- a/projects/gateway2/translator/listener/gateway_listener_translator.go +++ b/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 } diff --git a/projects/gateway2/translator/sslutils/ssl_utils.go b/projects/gateway2/translator/sslutils/ssl_utils.go index b3128d439cc..193e942b8c5 100644 --- a/projects/gateway2/translator/sslutils/ssl_utils.go +++ b/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 } diff --git a/projects/gloo/pkg/utils/ssl.go b/projects/gloo/pkg/utils/ssl.go index 3846622491c..b51e305d18a 100644 --- a/projects/gloo/pkg/utils/ssl.go +++ b/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,11 +432,7 @@ 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) } @@ -443,18 +440,17 @@ 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 { +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) { diff --git a/projects/gloo/pkg/utils/ssl_test.go b/projects/gloo/pkg/utils/ssl_test.go index c8ecdf4673d..e3a813c145b 100644 --- a/projects/gloo/pkg/utils/ssl_test.go +++ b/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 }),