From 6afd9202bd7799040e9cf353ef94289c70d355ac Mon Sep 17 00:00:00 2001
From: reesporte <reesedporter@gmail.com>
Date: Mon, 27 Jun 2022 10:44:26 -0500
Subject: [PATCH] add guardrails to TLS configs

We catch some possible states that don't make sense or are insecure:
1. If we're passed a nil tlsConfig to parse, return an error so we don't panic.
2. If we have a root CA, but we're skipping server cert verification, return an error.
3. If we have a TLS cert, but we're skipping server cert verification, return an error.
---
 server/tlsconfig.go      | 23 +++++++++++++--
 server/tlsconfig_test.go | 60 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+), 2 deletions(-)
 create mode 100644 server/tlsconfig_test.go

diff --git a/server/tlsconfig.go b/server/tlsconfig.go
index 4976fa6e5..adf233f92 100644
--- a/server/tlsconfig.go
+++ b/server/tlsconfig.go
@@ -36,6 +36,7 @@ package server
 import (
 	"crypto/tls"
 	"crypto/x509"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"os/signal"
@@ -104,11 +105,27 @@ func (kpr *keypairReloader) GetClientCertificateFunc() func(*tls.CertificateRequ
 }
 
 func GetTLSConfig(tlsConfig *TLSConfig, logger logger.Logger) (TLSConfig *tls.Config, err error) {
-	if tlsConfig.CertificatePath != "" && tlsConfig.CertificateKeyPath != "" {
+	if tlsConfig == nil {
+		return nil, fmt.Errorf("cannot parse nil tls config")
+	}
+
+	hasCA := len(tlsConfig.CACertPath) > 0
+	hasCert := len(tlsConfig.CertificatePath) > 0 && len(tlsConfig.CertificateKeyPath) > 0
+
+	if hasCA && tlsConfig.SkipVerify {
+		return nil, fmt.Errorf("cannot specify root certificate and disable server certificate verification")
+	}
+
+	if hasCert && tlsConfig.SkipVerify {
+		return nil, fmt.Errorf("cannot specify TLS certificate and disable server certificate verification")
+	}
+
+	if hasCert {
 		kpr, err := NewKeypairReloader(tlsConfig.CertificatePath, tlsConfig.CertificateKeyPath, logger)
 		if err != nil {
 			return nil, errors.Wrap(err, "loading keypair")
 		}
+
 		TLSConfig = &tls.Config{
 			InsecureSkipVerify:       tlsConfig.SkipVerify,
 			PreferServerCipherSuites: true,
@@ -116,7 +133,8 @@ func GetTLSConfig(tlsConfig *TLSConfig, logger logger.Logger) (TLSConfig *tls.Co
 			GetCertificate:           kpr.GetCertificateFunc(),
 			GetClientCertificate:     kpr.GetClientCertificateFunc(),
 		}
-		if tlsConfig.CACertPath != "" {
+
+		if hasCA {
 			b, err := ioutil.ReadFile(tlsConfig.CACertPath)
 			if err != nil {
 				return nil, errors.Wrap(err, "loading tls ca key")
@@ -130,6 +148,7 @@ func GetTLSConfig(tlsConfig *TLSConfig, logger logger.Logger) (TLSConfig *tls.Co
 			TLSConfig.ClientCAs = certPool
 			TLSConfig.RootCAs = certPool
 		}
+
 		if tlsConfig.EnableClientVerification {
 			TLSConfig.ClientAuth = tls.RequireAndVerifyClientCert
 		}
diff --git a/server/tlsconfig_test.go b/server/tlsconfig_test.go
new file mode 100644
index 000000000..9cc5d3d83
--- /dev/null
+++ b/server/tlsconfig_test.go
@@ -0,0 +1,60 @@
+package server_test
+
+import (
+	"crypto/tls"
+	"fmt"
+	"reflect"
+	"testing"
+
+	"github.com/molecula/featurebase/v3/logger"
+	"github.com/molecula/featurebase/v3/server"
+)
+
+func TestGetTLSConfig(t *testing.T) {
+	type testCase struct {
+		config *server.TLSConfig
+		exp    *tls.Config
+		err    error
+	}
+	for name, test := range map[string]testCase{
+		"nil": {
+			config: nil,
+			exp:    nil,
+			err:    fmt.Errorf("cannot parse nil tls config"),
+		},
+		"hasCASkip": {
+			config: &server.TLSConfig{
+				CACertPath: "blah",
+				SkipVerify: true,
+			},
+			exp: nil,
+			err: fmt.Errorf("cannot specify root certificate and disable server certificate verification"),
+		},
+		"hasCertSkip": {
+			config: &server.TLSConfig{
+				CertificatePath:    "blah",
+				CertificateKeyPath: "blah",
+				SkipVerify:         true,
+			},
+			exp: nil,
+			err: fmt.Errorf("cannot specify TLS certificate and disable server certificate verification"),
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			got, err := server.GetTLSConfig(test.config, logger.NopLogger)
+			if errStr(err) != errStr(test.err) {
+				t.Fatalf("expected %v, got %v", test.err, err)
+			}
+			if !reflect.DeepEqual(got, test.exp) {
+				t.Fatalf("expected %v, got %v", test.exp, got)
+			}
+		})
+	}
+}
+
+func errStr(err error) string {
+	if err == nil {
+		return ""
+	}
+	return err.Error()
+}