tls: Allow client auth configs if CA filenames match (#2648)

* verify client certs

* move client cert compatible checker to an independent function

* unexport client cert compatible checker

* rename functions and add comment

* gofmt code

* add test

* add back the comment

3 files changed, 102 insertions, 12 deletions

diff --git a/caddytls/config.go b/caddytls/config.go
index 6ae1f85ec..ed2b53d4e 100644
--- a/caddytls/config.go
+++ b/caddytls/config.go
@@ -394,16 +394,9 @@ func assertConfigsCompatible(cfg1, cfg2 *Config) error {
 	if c1.MaxVersion != c2.MaxVersion {
 		return fmt.Errorf("maximum TLS version mismatch")
 	}
-	if c1.ClientAuth != c2.ClientAuth {
-		return fmt.Errorf("client authentication policy mismatch")
-	}
-	if c1.ClientAuth != tls.NoClientCert && c2.ClientAuth != tls.NoClientCert && c1.ClientCAs != c2.ClientCAs {
-		// Two hosts defined on the same listener are not compatible if they
-		// have ClientAuth enabled, because there's no guarantee beyond the
-		// hostname which config will be used (because SNI only has server name).
-		// To prevent clients from bypassing authentication, require that
-		// ClientAuth be configured in an unambiguous manner.
-		return fmt.Errorf("multiple hosts requiring client authentication ambiguously configured")
+
+	if err := assertClientCertsCompatible(cfg1, cfg2); err != nil {
+		return err
 	}
 
 	return nil
@@ -550,6 +543,37 @@ func getPreferredDefaultCiphers() []uint16 {
 	return defaultCiphersNonAESNI
 }
 
+func assertClientCertsCompatible(cfg1, cfg2 *Config) error {
+	c1, c2 := cfg1.tlsConfig, cfg2.tlsConfig
+	if c1.ClientAuth != c2.ClientAuth {
+		return fmt.Errorf("client authentication policy mismatch")
+	}
+
+	if c1.ClientAuth == tls.NoClientCert || c2.ClientAuth == tls.NoClientCert {
+		return nil
+	}
+
+	ccerts1, ccerts2 := cfg1.ClientCerts, cfg2.ClientCerts
+
+	if len(ccerts1) != len(ccerts2) {
+		return fmt.Errorf("number of client certs differs")
+	}
+
+	// The order of client CAs matters
+	for i, v := range ccerts1 {
+		if v != ccerts2[i] {
+			// Two hosts defined on the same listener are not compatible if they
+			// have ClientAuth enabled, because there's no guarantee beyond the
+			// hostname which config will be used (because SNI only has server name).
+			// To prevent clients from bypassing authentication, require that
+			// ClientAuth be configured in an unambiguous manner.
+			return fmt.Errorf("multiple hosts requiring client authentication ambiguously configured")
+		}
+	}
+
+	return nil
+}
+
 // Map of supported curves
 // https://golang.org/pkg/crypto/tls/#CurveID
 var supportedCurvesMap = map[string]tls.CurveID{
diff --git a/caddytls/config_test.go b/caddytls/config_test.go
index a0af13740..1d64c3390 100644
--- a/caddytls/config_test.go
+++ b/caddytls/config_test.go
@@ -108,3 +108,25 @@ func TestGetPreferredDefaultCiphers(t *testing.T) {
 		}
 	}
 }
+
+func TestAssertTLSConfigCompatibleClientCert(t *testing.T) {
+	configs := []*Config{
+		{Enabled: true, ClientAuth: tls.RequestClientCert, ClientCerts: []string{}},
+		{Enabled: true, ClientAuth: tls.RequestClientCert, ClientCerts: []string{"ca_cert.crt"}},
+	}
+
+	_, err := MakeTLSConfig(configs)
+	if err == nil {
+		t.Fatalf("Expected an error, but got %v", err)
+	}
+
+	configs = []*Config{
+		{Enabled: true, ClientAuth: tls.RequestClientCert, ClientCerts: []string{"ca_cert.crt"}},
+		{Enabled: true, ClientAuth: tls.RequestClientCert, ClientCerts: []string{"ca_cert.crt"}},
+	}
+
+	_, err = MakeTLSConfig(configs)
+	if err != nil {
+		t.Fatalf("Expected no error, but got %v", err)
+	}
+}
diff --git a/caddytls/setup_test.go b/caddytls/setup_test.go
index 67de597ad..b0c7b0628 100644
--- a/caddytls/setup_test.go
+++ b/caddytls/setup_test.go
@@ -38,11 +38,18 @@ func TestMain(m *testing.M) {
 		os.Remove(certFile)
 		log.Fatal(err)
 	}
+	err = ioutil.WriteFile(caCertFile, caCert, 0644)
+	if err != nil {
+		os.Remove(keyFile)
+		os.Remove(certFile)
+		log.Fatal(err)
+	}
 
 	result := m.Run()
 
 	os.Remove(certFile)
 	os.Remove(keyFile)
+	os.Remove(caCertFile)
 	os.Exit(result)
 }
 
@@ -438,8 +445,9 @@ func TestSetupParseWithEmail(t *testing.T) {
 }
 
 const (
-	certFile = "test_cert.pem"
-	keyFile  = "test_key.pem"
+	certFile   = "test_cert.pem"
+	keyFile    = "test_key.pem"
+	caCertFile = "ca_cert.crt"
 )
 
 var testCert = []byte(`-----BEGIN CERTIFICATE-----
@@ -464,3 +472,39 @@ AwEHoUQDQgAEs22MtnG79K1mvIyjEO9GLx7BFD0tBbGnwQ0VPsuCxC6IeVuXbQDL
 SiVQvFZ6lUszTlczNxVkpEfqrM6xAupB7g==
 -----END EC PRIVATE KEY-----
 `)
+
+var caCert = []byte(`-----BEGIN CERTIFICATE-----
+MIIF2DCCA8CgAwIBAgIQTKr5yttjb+Af907YWwOGnTANBgkqhkiG9w0BAQwFADCB
+hTELMAkGA1UEBhMCR0IxGzAZBgNVBAgTEkdyZWF0ZXIgTWFuY2hlc3RlcjEQMA4G
+A1UEBxMHU2FsZm9yZDEaMBgGA1UEChMRQ09NT0RPIENBIExpbWl0ZWQxKzApBgNV
+BAMTIkNPTU9ETyBSU0EgQ2VydGlmaWNhdGlvbiBBdXRob3JpdHkwHhcNMTAwMTE5
+MDAwMDAwWhcNMzgwMTE4MjM1OTU5WjCBhTELMAkGA1UEBhMCR0IxGzAZBgNVBAgT
+EkdyZWF0ZXIgTWFuY2hlc3RlcjEQMA4GA1UEBxMHU2FsZm9yZDEaMBgGA1UEChMR
+Q09NT0RPIENBIExpbWl0ZWQxKzApBgNVBAMTIkNPTU9ETyBSU0EgQ2VydGlmaWNh
+dGlvbiBBdXRob3JpdHkwggIiMA0GCSqGSIb3DQEBAQUAA4ICDwAwggIKAoICAQCR
+6FSS0gpWsawNJN3Fz0RndJkrN6N9I3AAcbxT38T6KhKPS38QVr2fcHK3YX/JSw8X
+pz3jsARh7v8Rl8f0hj4K+j5c+ZPmNHrZFGvnnLOFoIJ6dq9xkNfs/Q36nGz637CC
+9BR++b7Epi9Pf5l/tfxnQ3K9DADWietrLNPtj5gcFKt+5eNu/Nio5JIk2kNrYrhV
+/erBvGy2i/MOjZrkm2xpmfh4SDBF1a3hDTxFYPwyllEnvGfDyi62a+pGx8cgoLEf
+Zd5ICLqkTqnyg0Y3hOvozIFIQ2dOciqbXL1MGyiKXCJ7tKuY2e7gUYPDCUZObT6Z
++pUX2nwzV0E8jVHtC7ZcryxjGt9XyD+86V3Em69FmeKjWiS0uqlWPc9vqv9JWL7w
+qP/0uK3pN/u6uPQLOvnoQ0IeidiEyxPx2bvhiWC4jChWrBQdnArncevPDt09qZah
+SL0896+1DSJMwBGB7FY79tOi4lu3sgQiUpWAk2nojkxl8ZEDLXB0AuqLZxUpaVIC
+u9ffUGpVRr+goyhhf3DQw6KqLCGqR84onAZFdr+CGCe01a60y1Dma/RMhnEw6abf
+Fobg2P9A3fvQQoh/ozM6LlweQRGBY84YcWsr7KaKtzFcOmpH4MN5WdYgGq/yapiq
+crxXStJLnbsQ/LBMQeXtHT1eKJ2czL+zUdqnR+WEUwIDAQABo0IwQDAdBgNVHQ4E
+FgQUu69+Aj36pvE8hI6t7jiY7NkyMtQwDgYDVR0PAQH/BAQDAgEGMA8GA1UdEwEB
+/wQFMAMBAf8wDQYJKoZIhvcNAQEMBQADggIBAArx1UaEt65Ru2yyTUEUAJNMnMvl
+wFTPoCWOAvn9sKIN9SCYPBMtrFaisNZ+EZLpLrqeLppysb0ZRGxhNaKatBYSaVqM
+4dc+pBroLwP0rmEdEBsqpIt6xf4FpuHA1sj+nq6PK7o9mfjYcwlYRm6mnPTXJ9OV
+2jeDchzTc+CiR5kDOF3VSXkAKRzH7JsgHAckaVd4sjn8OoSgtZx8jb8uk2Intzna
+FxiuvTwJaP+EmzzV1gsD41eeFPfR60/IvYcjt7ZJQ3mFXLrrkguhxuhoqEwWsRqZ
+CuhTLJK7oQkYdQxlqHvLI7cawiiFwxv/0Cti76R7CZGYZ4wUAc1oBmpjIXUDgIiK
+boHGhfKppC3n9KUkEEeDys30jXlYsQab5xoq2Z0B15R97QNKyvDb6KkBPvVWmcke
+jkk9u+UJueBPSZI9FoJAzMxZxuY67RIuaTxslbH9qh17f4a+Hg4yRvv7E491f0yL
+S0Zj/gA0QHDBw7mh3aZw4gSzQbzpgJHqZJx64SIDqZxubw5lT2yHh17zbqD5daWb
+QOhTsiedSrnAdyGN/4fy3ryM7xfft0kL0fJuMAsaDk527RH89elWsn2/x20Kk4yl
+0MC2Hb46TpSi125sC8KKfPog88Tk5c0NqMuRkrF8hey1FGlmDoLnzc7ILaZRfyHB
+NVOFBkpdn627G190
+-----END CERTIFICATE-----
+`)