From ed9a93fe5b9b9288e6be99da6d3a0b6a2cde470a Mon Sep 17 00:00:00 2001
From: Kenneth Jenkins <51246568+kenjenkins@users.noreply.github.com>
Date: Thu, 10 Aug 2023 16:15:11 -0700
Subject: [PATCH] config: extra CA and CRL validation (#4455)

Return an error from DownstreamMTLSSettings.validate() if both CA and
CAFile are populated, or if both CRL and CRLFile are populated.
---
 config/mtls.go         |  7 ++++++-
 config/mtls_test.go    |  4 ++++
 config/options_test.go | 41 ++++++++++++++++++++++++++++++-----------
 3 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/config/mtls.go b/config/mtls.go
index e4886f887..765af6b81 100644
--- a/config/mtls.go
+++ b/config/mtls.go
@@ -116,10 +116,15 @@ func (s *DownstreamMTLSSettings) GetMaxVerifyDepth() uint32 {
 }
 
 func (s *DownstreamMTLSSettings) validate() error {
-	if _, err := s.GetCA(); err != nil {
+	if s.CA != "" && s.CAFile != "" {
+		return errors.New("cannot set both ca and ca_file")
+	} else if _, err := s.GetCA(); err != nil {
 		return err
 	}
 
+	if s.CRL != "" && s.CRLFile != "" {
+		return errors.New("cannot set both crl and crl_file")
+	}
 	crl, err := s.GetCRL()
 	if err != nil {
 		return err
diff --git a/config/mtls_test.go b/config/mtls_test.go
index 1f4cc7f98..0be3c47b1 100644
--- a/config/mtls_test.go
+++ b/config/mtls_test.go
@@ -122,10 +122,14 @@ func TestDownstreamMTLSSettingsValidate(t *testing.T) {
 		errorMsg string
 	}{
 		{"not set", DownstreamMTLSSettings{}, ""},
+		{"both CA and CA file", DownstreamMTLSSettings{CA: "CA", CAFile: "CAFile"},
+			"cannot set both ca and ca_file"},
 		{"bad CA", DownstreamMTLSSettings{CA: "not%valid%base64%data"},
 			"CA: illegal base64 data at input byte 3"},
 		{"bad CA file", DownstreamMTLSSettings{CAFile: "-"},
 			"CA file: open -: no such file or directory"},
+		{"both CRL and CRL file", DownstreamMTLSSettings{CRL: "CRL", CRLFile: "CRLFile"},
+			"cannot set both crl and crl_file"},
 		{"bad CRL", DownstreamMTLSSettings{CRL: "dGhpc2lzZmluZQo="},
 			"CRL: cryptutil: invalid crl, no X509 CRL block found"},
 		{"bad CRL file", DownstreamMTLSSettings{CRLFile: "-"},
diff --git a/config/options_test.go b/config/options_test.go
index 23168c9e0..3e88e3e85 100644
--- a/config/options_test.go
+++ b/config/options_test.go
@@ -705,18 +705,37 @@ func TestDeprecatedClientCAOptions(t *testing.T) {
 	zl := zerolog.New(&logOutput)
 	testutil.SetLogger(t, &zl)
 
-	o := NewDefaultOptions()
-	o.ClientCA = "LS0tIEZBS0UgQ0EgQ0VSVCAtLS0="
-	o.ClientCAFile = caFile
-	o.AutocertOptions.Enable = true // suppress an unrelated warning
+	t.Run("CA", func(t *testing.T) {
+		logOutput.Reset()
 
-	err := o.Validate()
-	require.NoError(t, err)
-	assert.Equal(t, "LS0tIEZBS0UgQ0EgQ0VSVCAtLS0=", o.DownstreamMTLS.CA)
-	assert.Equal(t, caFile, o.DownstreamMTLS.CAFile)
-	assert.Equal(t, `{"level":"warn","message":"config: client_ca is deprecated, set downstream_mtls.ca instead"}
-{"level":"warn","message":"config: client_ca_file is deprecated, set downstream_mtls.ca_file instead"}
-`, logOutput.String())
+		o := NewDefaultOptions()
+		o.AutocertOptions.Enable = true // suppress an unrelated warning
+		o.ClientCA = "LS0tIEZBS0UgQ0EgQ0VSVCAtLS0="
+
+		err := o.Validate()
+
+		require.NoError(t, err)
+		assert.Equal(t, "LS0tIEZBS0UgQ0EgQ0VSVCAtLS0=", o.DownstreamMTLS.CA)
+		assert.Equal(t, `{"level":"warn","message":"config: client_ca is deprecated, set downstream_mtls.ca instead"}
+`,
+			logOutput.String())
+	})
+
+	t.Run("CAFile", func(t *testing.T) {
+		logOutput.Reset()
+
+		o := NewDefaultOptions()
+		o.AutocertOptions.Enable = true // suppress an unrelated warning
+		o.ClientCAFile = caFile
+
+		err := o.Validate()
+
+		require.NoError(t, err)
+		assert.Equal(t, caFile, o.DownstreamMTLS.CAFile)
+		assert.Equal(t, `{"level":"warn","message":"config: client_ca_file is deprecated, set downstream_mtls.ca_file instead"}
+`,
+			logOutput.String())
+	})
 }
 
 func TestOptions_DefaultURL(t *testing.T) {