fix: use SecretBytes type for cert values to prevent accidental print…

…ing. Fixes 145
nginxinc · Dec 15, 2023 · beea17c · beea17c
1 parent 537b14a
commit beea17c
Show file tree

Hide file tree

Showing 7 changed files with 95 additions and 32 deletions.
diff --git a/.tool-versions 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)
+	}
+}