core/config: remove support for base64 encoded certificates (#4718)

* core/config: update file watcher source to handle base64 encoded certificates * fix data race * core/config: only allow files in certificates * remove test * re-add test
pomerium · Nov 8, 2023 · 9598c99 · 9598c99
1 parent ffca3b3
commit 9598c99
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 24 deletions.
diff --git a/config/options.go b/config/options.go
@@ -96,6 +96,7 @@ type Options struct {
 	// If this setting is not specified, the value defaults to V4_PREFERRED.
 	DNSLookupFamily string `mapstructure:"dns_lookup_family" yaml:"dns_lookup_family,omitempty"`
 
+	CertificateData  []*config.Settings_Certificate
 	CertificateFiles []certificateFilePair `mapstructure:"certificates" yaml:"certificates,omitempty"`
 
 	// Cert and Key is the x509 certificate used to create the HTTPS server.
@@ -666,13 +667,18 @@ func (o *Options) Validate() error {
 		hasCert = true
 	}
 
-	for _, c := range o.CertificateFiles {
-		_, err := cryptutil.CertificateFromBase64(c.CertFile, c.KeyFile)
+	for _, c := range o.CertificateData {
+		_, err := tls.X509KeyPair(c.GetCertBytes(), c.GetKeyBytes())
 		if err != nil {
-			_, err = cryptutil.CertificateFromFile(c.CertFile, c.KeyFile)
+			return fmt.Errorf("config: bad cert entry, cert is invalid: %w", err)
 		}
+		hasCert = true
+	}
+
+	for _, c := range o.CertificateFiles {
+		_, err := cryptutil.CertificateFromFile(c.CertFile, c.KeyFile)
 		if err != nil {
-			return fmt.Errorf("config: bad cert entry, base64 or file reference invalid. %w", err)
+			return fmt.Errorf("config: bad cert entry, file reference invalid. %w", err)
 		}
 		hasCert = true
 	}
@@ -1020,14 +1026,18 @@ func (o *Options) GetCertificates() ([]tls.Certificate, error) {
 		certs = append(certs, *cert)
 	}
 	for _, c := range o.CertificateFiles {
-		cert, err := cryptutil.CertificateFromBase64(c.CertFile, c.KeyFile)
+		cert, err := cryptutil.CertificateFromFile(c.CertFile, c.KeyFile)
 		if err != nil {
-			cert, err = cryptutil.CertificateFromFile(c.CertFile, c.KeyFile)
+			return nil, fmt.Errorf("config: invalid certificate entry: %w", err)
 		}
+		certs = append(certs, *cert)
+	}
+	for _, c := range o.CertificateData {
+		cert, err := tls.X509KeyPair(c.GetCertBytes(), c.GetKeyBytes())
 		if err != nil {
 			return nil, fmt.Errorf("config: invalid certificate entry: %w", err)
 		}
-		certs = append(certs, *cert)
+		certs = append(certs, cert)
 	}
 	if o.CertFile != "" && o.KeyFile != "" {
 		cert, err := cryptutil.CertificateFromFile(o.CertFile, o.KeyFile)
@@ -1041,7 +1051,12 @@ func (o *Options) GetCertificates() ([]tls.Certificate, error) {
 
 // HasCertificates returns true if options has any certificates.
 func (o *Options) HasCertificates() bool {
-	return o.Cert != "" || o.Key != "" || len(o.CertificateFiles) > 0 || o.CertFile != "" || o.KeyFile != ""
+	return o.Cert != "" ||
+		o.Key != "" ||
+		len(o.CertificateFiles) > 0 ||
+		o.CertFile != "" ||
+		o.KeyFile != "" ||
+		len(o.CertificateData) > 0
 }
 
 // GetX509Certificates gets all the x509 certificates from the options. Invalid certificates are ignored.
@@ -1063,11 +1078,17 @@ func (o *Options) GetX509Certificates() []*x509.Certificate {
 		}
 	}
 
-	for _, c := range o.CertificateFiles {
-		cert, err := cryptutil.ParsePEMCertificateFromBase64(c.CertFile)
+	for _, c := range o.CertificateData {
+		cert, err := cryptutil.ParsePEMCertificate(c.GetCertBytes())
 		if err != nil {
-			cert, err = cryptutil.ParsePEMCertificateFromFile(c.CertFile)
+			log.Error(context.Background()).Err(err).Msg("invalid certificate")
+		} else {
+			certs = append(certs, cert)
 		}
+	}
+
+	for _, c := range o.CertificateFiles {
+		cert, err := cryptutil.ParsePEMCertificateFromFile(c.CertFile)
 		if err != nil {
 			log.Error(context.Background()).Err(err).Msg("invalid certificate_file")
 		} else {
@@ -1373,21 +1394,18 @@ func (o *Options) Checksum() uint64 {
 
 func (o *Options) applyExternalCerts(ctx context.Context, certsIndex *cryptutil.CertificatesIndex, certs []*config.Settings_Certificate) {
 	for _, c := range certs {
-		cfp := certificateFilePair{}
-		cfp.CertFile = base64.StdEncoding.EncodeToString(c.CertBytes)
-		cfp.KeyFile = base64.StdEncoding.EncodeToString(c.KeyBytes)
-
-		cert, err := cryptutil.ParsePEMCertificateFromBase64(cfp.CertFile)
+		cert, err := cryptutil.ParsePEMCertificate(c.GetCertBytes())
 		if err != nil {
 			log.Error(ctx).Err(err).Msg("parsing cert from databroker: skipped")
 			continue
 		}
+
 		if overlaps, name := certsIndex.OverlapsWithExistingCertificate(cert); overlaps {
 			log.Error(ctx).Err(err).Str("domain", name).Msg("overlaps with local certs: skipped")
 			continue
 		}
 
-		o.CertificateFiles = append(o.CertificateFiles, cfp)
+		o.CertificateData = append(o.CertificateData, c)
 	}
 }
 

diff --git a/config/options_test.go b/config/options_test.go
@@ -628,17 +628,12 @@ func TestCertificatesArrayParsing(t *testing.T) {
 
 	testCertFileRef := "./testdata/example-cert.pem"
 	testKeyFileRef := "./testdata/example-key.pem"
-	testCertFile, _ := os.ReadFile(testCertFileRef)
-	testKeyFile, _ := os.ReadFile(testKeyFileRef)
-	testCertAsBase64 := base64.StdEncoding.EncodeToString(testCertFile)
-	testKeyAsBase64 := base64.StdEncoding.EncodeToString(testKeyFile)
 
 	tests := []struct {
 		name             string
 		certificateFiles []certificateFilePair
 		wantErr          bool
 	}{
-		{"Handles base64 string as params", []certificateFilePair{{KeyFile: testKeyAsBase64, CertFile: testCertAsBase64}}, false},
 		{"Handles file reference as params", []certificateFilePair{{KeyFile: testKeyFileRef, CertFile: testCertFileRef}}, false},
 		{"Returns an error otherwise", []certificateFilePair{{KeyFile: "abc", CertFile: "abc"}}, true},
 	}
@@ -936,8 +931,11 @@ func TestOptions_ApplySettings(t *testing.T) {
 		options := NewDefaultOptions()
 		cert1, err := cryptutil.GenerateCertificate(nil, "example.com")
 		require.NoError(t, err)
+		cert1path := filepath.Join(t.TempDir(), "example.com.pem")
+		err = os.WriteFile(cert1path, cert1.Certificate[0], 0o600)
+		require.NoError(t, err)
 		options.CertificateFiles = append(options.CertificateFiles, certificateFilePair{
-			CertFile: base64.StdEncoding.EncodeToString(encodeCert(cert1)),
+			CertFile: cert1path,
 		})
 		cert2, err := cryptutil.GenerateCertificate(nil, "example.com")
 		require.NoError(t, err)
@@ -955,7 +953,7 @@ func TestOptions_ApplySettings(t *testing.T) {
 			},
 		}
 		options.ApplySettings(ctx, certsIndex, settings)
-		assert.Len(t, options.CertificateFiles, 2, "should prevent adding duplicate certificates")
+		assert.Len(t, options.CertificateData, 1, "should prevent adding duplicate certificates")
 	})
 }