From beea17ca9e0e4b1b0873c3884efda505a9602644 Mon Sep 17 00:00:00 2001 From: Javier Evans Date: Tue, 28 Nov 2023 19:19:17 -0800 Subject: [PATCH] fix: use SecretBytes type for cert values to prevent accidental printing. Fixes 145 --- .tool-versions | 1 + cmd/tls-config-factory-test-harness/main.go | 21 +++++++------ internal/authentication/factory_test.go | 34 +++++++++++---------- internal/certification/certificates.go | 24 +++++++++++---- internal/configuration/settings.go | 1 + internal/core/secret_bytes.go | 15 +++++++++ internal/core/secret_bytes_test.go | 31 +++++++++++++++++++ 7 files changed, 95 insertions(+), 32 deletions(-) create mode 100644 .tool-versions create mode 100644 internal/core/secret_bytes.go create mode 100644 internal/core/secret_bytes_test.go diff --git a/.tool-versions b/.tool-versions new file mode 100644 index 0000000..09548d5 --- /dev/null +++ b/.tool-versions @@ -0,0 +1 @@ +golang 1.19.13 diff --git a/cmd/tls-config-factory-test-harness/main.go b/cmd/tls-config-factory-test-harness/main.go index b5fda78..3f46d4f 100644 --- a/cmd/tls-config-factory-test-harness/main.go +++ b/cmd/tls-config-factory-test-harness/main.go @@ -6,6 +6,7 @@ import ( "github.com/nginxinc/kubernetes-nginx-ingress/internal/authentication" "github.com/nginxinc/kubernetes-nginx-ingress/internal/certification" "github.com/nginxinc/kubernetes-nginx-ingress/internal/configuration" + "github.com/nginxinc/kubernetes-nginx-ingress/internal/core" "github.com/sirupsen/logrus" "os" ) @@ -82,7 +83,7 @@ func buildConfigMap() map[string]TlsConfiguration { } func ssTlsConfig() configuration.Settings { - certificates := make(map[string]map[string][]byte) + certificates := make(map[string]map[string]core.SecretBytes) certificates[CaCertificateSecretKey] = buildCaCertificateEntry(caCertificatePEM()) certificates[ClientCertificateSecretKey] = buildClientCertificateEntry(clientKeyPEM(), clientCertificatePEM()) @@ -95,7 +96,7 @@ func ssTlsConfig() configuration.Settings { } func ssMtlsConfig() configuration.Settings { - certificates := make(map[string]map[string][]byte) + certificates := make(map[string]map[string]core.SecretBytes) certificates[CaCertificateSecretKey] = buildCaCertificateEntry(caCertificatePEM()) certificates[ClientCertificateSecretKey] = buildClientCertificateEntry(clientKeyPEM(), clientCertificatePEM()) @@ -114,7 +115,7 @@ func caTlsConfig() configuration.Settings { } func caMtlsConfig() configuration.Settings { - certificates := make(map[string]map[string][]byte) + certificates := make(map[string]map[string]core.SecretBytes) certificates[ClientCertificateSecretKey] = buildClientCertificateEntry(clientKeyPEM(), clientCertificatePEM()) return configuration.Settings{ @@ -215,15 +216,15 @@ z/3KkMx4uqJXZyvQrmkolSg= ` } -func buildClientCertificateEntry(keyPEM, certificatePEM string) map[string][]byte { - return map[string][]byte{ - certification.CertificateKey: []byte(certificatePEM), - certification.CertificateKeyKey: []byte(keyPEM), +func buildClientCertificateEntry(keyPEM, certificatePEM string) map[string]core.SecretBytes { + return map[string]core.SecretBytes{ + certification.CertificateKey: core.SecretBytes([]byte(certificatePEM)), + certification.CertificateKeyKey: core.SecretBytes([]byte(keyPEM)), } } -func buildCaCertificateEntry(certificatePEM string) map[string][]byte { - return map[string][]byte{ - certification.CertificateKey: []byte(certificatePEM), +func buildCaCertificateEntry(certificatePEM string) map[string]core.SecretBytes { + return map[string]core.SecretBytes{ + certification.CertificateKey: core.SecretBytes([]byte(certificatePEM)), } } diff --git a/internal/authentication/factory_test.go b/internal/authentication/factory_test.go index 7ecd610..a535200 100644 --- a/internal/authentication/factory_test.go +++ b/internal/authentication/factory_test.go @@ -6,9 +6,11 @@ package authentication import ( + "testing" + "github.com/nginxinc/kubernetes-nginx-ingress/internal/certification" "github.com/nginxinc/kubernetes-nginx-ingress/internal/configuration" - "testing" + "github.com/nginxinc/kubernetes-nginx-ingress/internal/core" ) const ( @@ -34,7 +36,7 @@ func TestTlsFactory_UnspecifiedModeDefaultsToNoTls(t *testing.T) { } func TestTlsFactory_SelfSignedTlsMode(t *testing.T) { - certificates := make(map[string]map[string][]byte) + certificates := make(map[string]map[string]core.SecretBytes) certificates[CaCertificateSecretKey] = buildCaCertificateEntry(caCertificatePEM()) settings := configuration.Settings{ @@ -69,7 +71,7 @@ func TestTlsFactory_SelfSignedTlsMode(t *testing.T) { } func TestTlsFactory_SelfSignedTlsModeCertPoolError(t *testing.T) { - certificates := make(map[string]map[string][]byte) + certificates := make(map[string]map[string]core.SecretBytes) certificates[CaCertificateSecretKey] = buildCaCertificateEntry(invalidCertificatePEM()) settings := configuration.Settings{ @@ -90,7 +92,7 @@ func TestTlsFactory_SelfSignedTlsModeCertPoolError(t *testing.T) { } func TestTlsFactory_SelfSignedTlsModeCertPoolCertificateParseError(t *testing.T) { - certificates := make(map[string]map[string][]byte) + certificates := make(map[string]map[string]core.SecretBytes) certificates[CaCertificateSecretKey] = buildCaCertificateEntry(invalidCertificateDataPEM()) settings := configuration.Settings{ @@ -113,7 +115,7 @@ func TestTlsFactory_SelfSignedTlsModeCertPoolCertificateParseError(t *testing.T) } func TestTlsFactory_SelfSignedMtlsMode(t *testing.T) { - certificates := make(map[string]map[string][]byte) + certificates := make(map[string]map[string]core.SecretBytes) certificates[CaCertificateSecretKey] = buildCaCertificateEntry(caCertificatePEM()) certificates[ClientCertificateSecretKey] = buildClientCertificateEntry(clientKeyPEM(), clientCertificatePEM()) @@ -149,7 +151,7 @@ func TestTlsFactory_SelfSignedMtlsMode(t *testing.T) { } func TestTlsFactory_SelfSignedMtlsModeCertPoolError(t *testing.T) { - certificates := make(map[string]map[string][]byte) + certificates := make(map[string]map[string]core.SecretBytes) certificates[CaCertificateSecretKey] = buildCaCertificateEntry(invalidCertificatePEM()) certificates[ClientCertificateSecretKey] = buildClientCertificateEntry(clientKeyPEM(), clientCertificatePEM()) @@ -171,7 +173,7 @@ func TestTlsFactory_SelfSignedMtlsModeCertPoolError(t *testing.T) { } func TestTlsFactory_SelfSignedMtlsModeClientCertificateError(t *testing.T) { - certificates := make(map[string]map[string][]byte) + certificates := make(map[string]map[string]core.SecretBytes) certificates[CaCertificateSecretKey] = buildCaCertificateEntry(caCertificatePEM()) certificates[ClientCertificateSecretKey] = buildClientCertificateEntry(clientKeyPEM(), invalidCertificatePEM()) @@ -222,7 +224,7 @@ func TestTlsFactory_CaTlsMode(t *testing.T) { } func TestTlsFactory_CaMtlsMode(t *testing.T) { - certificates := make(map[string]map[string][]byte) + certificates := make(map[string]map[string]core.SecretBytes) certificates[ClientCertificateSecretKey] = buildClientCertificateEntry(clientKeyPEM(), clientCertificatePEM()) settings := configuration.Settings{ @@ -257,7 +259,7 @@ func TestTlsFactory_CaMtlsMode(t *testing.T) { } func TestTlsFactory_CaMtlsModeClientCertificateError(t *testing.T) { - certificates := make(map[string]map[string][]byte) + certificates := make(map[string]map[string]core.SecretBytes) certificates[CaCertificateSecretKey] = buildCaCertificateEntry(caCertificatePEM()) certificates[ClientCertificateSecretKey] = buildClientCertificateEntry(clientKeyPEM(), invalidCertificatePEM()) @@ -411,15 +413,15 @@ z/3KkMx4uqJXZyvQrmkolSg= ` } -func buildClientCertificateEntry(keyPEM, certificatePEM string) map[string][]byte { - return map[string][]byte{ - certification.CertificateKey: []byte(certificatePEM), - certification.CertificateKeyKey: []byte(keyPEM), +func buildClientCertificateEntry(keyPEM, certificatePEM string) map[string]core.SecretBytes { + return map[string]core.SecretBytes{ + certification.CertificateKey: core.SecretBytes([]byte(certificatePEM)), + certification.CertificateKeyKey: core.SecretBytes([]byte(keyPEM)), } } -func buildCaCertificateEntry(certificatePEM string) map[string][]byte { - return map[string][]byte{ - certification.CertificateKey: []byte(certificatePEM), +func buildCaCertificateEntry(certificatePEM string) map[string]core.SecretBytes { + return map[string]core.SecretBytes{ + certification.CertificateKey: core.SecretBytes([]byte(certificatePEM)), } } diff --git a/internal/certification/certificates.go b/internal/certification/certificates.go index ac31702..4cad4e2 100644 --- a/internal/certification/certificates.go +++ b/internal/certification/certificates.go @@ -11,11 +11,14 @@ package certification import ( "context" "fmt" + "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/informers" "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/cache" + + "github.com/nginxinc/kubernetes-nginx-ingress/internal/core" ) const ( @@ -30,7 +33,12 @@ const ( ) type Certificates struct { - Certificates map[string]map[string][]byte + // { + // ClientCertificateSecretKey: { + // "tls.crt": []byte + // } + // } + Certificates map[string]map[string]core.SecretBytes // Context is the context used to control the application. Context context.Context @@ -61,14 +69,14 @@ func NewCertificates(ctx context.Context, k8sClient kubernetes.Interface) *Certi } // GetCACertificate returns the Certificate Authority certificate. -func (c *Certificates) GetCACertificate() []byte { +func (c *Certificates) GetCACertificate() core.SecretBytes { bytes := c.Certificates[c.CaCertificateSecretKey][CertificateKey] return bytes } // GetClientCertificate returns the Client certificate and key. -func (c *Certificates) GetClientCertificate() ([]byte, []byte) { +func (c *Certificates) GetClientCertificate() (core.SecretBytes, core.SecretBytes) { keyBytes := c.Certificates[c.ClientCertificateSecretKey][CertificateKeyKey] certificateBytes := c.Certificates[c.ClientCertificateSecretKey][CertificateKey] @@ -81,7 +89,7 @@ func (c *Certificates) Initialize() error { var err error - c.Certificates = make(map[string]map[string][]byte) + c.Certificates = make(map[string]map[string]core.SecretBytes) informer, err := c.buildInformer() if err != nil { @@ -151,10 +159,14 @@ func (c *Certificates) handleAddEvent(obj interface{}) { return } - c.Certificates[secret.Name] = map[string][]byte{} + c.Certificates[secret.Name] = map[string]core.SecretBytes{} + // Input from the secret comes in the form + // tls.crt: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUVCVEN... + // tls.key: LS0tLS1CRUdJTiBQUklWQVRFIEtFWS0tLS0tCk1JSUV2Z0l... + // Where the keys are `tls.crt` and `tls.key` and the values are []byte for k, v := range secret.Data { - c.Certificates[secret.Name][k] = v + c.Certificates[secret.Name][k] = core.SecretBytes(v) } logrus.Debugf("Certificates::handleAddEvent: certificates (%d)", len(c.Certificates)) diff --git a/internal/configuration/settings.go b/internal/configuration/settings.go index d15ad84..72be6fa 100644 --- a/internal/configuration/settings.go +++ b/internal/configuration/settings.go @@ -179,6 +179,7 @@ func (s *Settings) Initialize() error { certificates := certification.NewCertificates(s.Context, s.K8sClient) + // q. Why is this a separate step? err = certificates.Initialize() if err != nil { return fmt.Errorf(`error occurred initializing certificates: %w`, err) diff --git a/internal/core/secret_bytes.go b/internal/core/secret_bytes.go new file mode 100644 index 0000000..fddd6cf --- /dev/null +++ b/internal/core/secret_bytes.go @@ -0,0 +1,15 @@ +package core + +import ( + "encoding/json" +) + +type SecretBytes []byte + +func (sb SecretBytes) String() string { + return "[REDACTED]" +} + +func (sb SecretBytes) MarshalJSON() ([]byte, error) { + return json.Marshal("[REDACTED]") +} diff --git a/internal/core/secret_bytes_test.go b/internal/core/secret_bytes_test.go new file mode 100644 index 0000000..8dd8024 --- /dev/null +++ b/internal/core/secret_bytes_test.go @@ -0,0 +1,31 @@ +/* + * Copyright 2023 F5 Inc. All rights reserved. + * Use of this source code is governed by the Apache License that can be found in the LICENSE file. + */ + +package core + +import ( + "encoding/json" + "fmt" + "testing" +) + +func TestSecretBytesToString(t *testing.T) { + sensitive := SecretBytes([]byte("If you can see this we have a problem")) + + expected := "foo [REDACTED] bar" + result := fmt.Sprintf("foo %v bar", sensitive) + if result != expected { + t.Errorf("Expected %s, got %s", expected, result) + } +} + +func TestSecretBytesToJSON(t *testing.T) { + sensitive, _ := json.Marshal(SecretBytes([]byte("If you can see this we have a problem"))) + expected := `foo "[REDACTED]" bar` + result := fmt.Sprintf("foo %v bar", string(sensitive)) + if result != expected { + t.Errorf("Expected %s, got %s", expected, result) + } +}