make sure client CA and auth type are set if CA is explicitly specified. (#2825)

* make sure client CA and auth type are set if CA is explicitly specified.

added some simple tests to confirm the effect.

* test certificates (forgot to add them in the previous commit)

* made client auth policy configurable with new client_auth option.

README has been updated accordingly.

* fix editorial in README

plugin/tls/README.md

@@ -24,6 +24,16 @@ tls CERT KEY [CA]
 
 Parameter CA is optional. If not set, system CAs can be used to verify the client certificate
 
+~~~ txt
+tls CERT KEY [CA] {
+    client_auth nocert|request|require|verify_if_given|require_and_verify
+}
+~~~
+
+If client_auth option is specified, it controls the client authentication policy.
+The option value corresponds to the [ClientAuthType values of the Go tls package](https://golang.org/pkg/crypto/tls/#ClientAuthType): NoClientCert, RequestClientCert, RequireAnyClientCert, VerifyClientCertIfGiven, and RequireAndVerifyClientCert, respectively.
+The default is "nocert".  Note that it makes no sense to specify parameter CA unless this option is set to verify_if_given or require_and_verify.
+
 ## Examples
 
 Start a DNS-over-TLS server that picks up incoming DNS-over-TLS queries on port 5553 and uses the

plugin/tls/test_ca.pem

@@ -0,0 +1,20 @@
+-----BEGIN CERTIFICATE-----
+MIIDPzCCAiegAwIBAgIJAPjCWTu1wGapMA0GCSqGSIb3DQEBCwUAMDUxCzAJBgNV
+BAYTAlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMREwDwYDVQQKDAhJbmZvYmxveDAg
+Fw0xOTA1MTEwMDI3NDRaGA8yMTE5MDQxNzAwMjc0NFowNTELMAkGA1UEBhMCVVMx
+EzARBgNVBAgMCkNhbGlmb3JuaWExETAPBgNVBAoMCEluZm9ibG94MIIBIjANBgkq
+hkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEArAYiw1UjlYj+nITRUlj5hA7j8U2qWcyN
+YcDfqQnt173Z8yR7NJokqt3Bd3PlrBZS2XtYSNohxRr4qeJu/g7UBre/fSEU/ZOM
+Gl7NjBGKQEymJ0d8rBg52iiGNwU+ERI9pcQRA6DCEjVbOmjDiUd5yzuVotG/Sxep
+GUJ2puJ0p0gWCMEL9sdqY6HHd/hdj6B6+u2xD9NUCkX9pLC7CPFJHnP0vLO4WIWL
+z5C7yzpeLO9r7Nfnu+2HcRLmuFZVPNxkMq7UymqR1w5ZYJQ5p9E7pyxDVXxHnTqQ
+yLaAS2/9umrOwVnD1NaN3OdAhDedXbH0cF08GcIQD9rnlkLMW4CKtwIDAQABo1Aw
+TjAdBgNVHQ4EFgQUHcxJPBmHF0nSv+FJJI/kwrSThf8wHwYDVR0jBBgwFoAUHcxJ
+PBmHF0nSv+FJJI/kwrSThf8wDAYDVR0TBAUwAwEB/zANBgkqhkiG9w0BAQsFAAOC
+AQEAByItgyhlXDv2wnnMVXHHlUCbsKCOtBJZ8EumvKjeOx5G4gqJpQIQPNeBv1Od
+QT7d15HfT7RQqHSL0uAoGuNuyGjZGWWbLMkVt8T0tXY2v9Dd8eWC/lFaaA0vkqTG
+GpADSmH+SoFAdPPcYN/sXmEHvZcIQ0wUxuF48ZMwOh7ZOcrZggxlA9+BKHU4fO03
+o7krzpQZQmEDXNN8bt1R0DIhVADw/G2oJAzK0LGhh4eu6hj6k/cAWS6ujRBGqN0Z
+fURCrMEyjzbNybhkU1KqSr7eSJOWkl4UJ5Ns/dt9/yw2BBrKH3Mijch7UA8mlbEE
+29M28u2W7GMXLSSwmtCqDBRNhg==
+-----END CERTIFICATE-----

plugin/tls/test_cert.pem

@@ -0,0 +1,20 @@
+-----BEGIN CERTIFICATE-----
+MIIDPzCCAiegAwIBAgIJAPezzzshGRiTMA0GCSqGSIb3DQEBCwUAMDUxCzAJBgNV
+BAYTAlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMREwDwYDVQQKDAhJbmZvYmxveDAg
+Fw0xOTA1MTEwMDI2MjNaGA8yMTE5MDQxNzAwMjYyM1owNTELMAkGA1UEBhMCVVMx
+EzARBgNVBAgMCkNhbGlmb3JuaWExETAPBgNVBAoMCEluZm9ibG94MIIBIjANBgkq
+hkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEArAYiw1UjlYj+nITRUlj5hA7j8U2qWcyN
+YcDfqQnt173Z8yR7NJokqt3Bd3PlrBZS2XtYSNohxRr4qeJu/g7UBre/fSEU/ZOM
+Gl7NjBGKQEymJ0d8rBg52iiGNwU+ERI9pcQRA6DCEjVbOmjDiUd5yzuVotG/Sxep
+GUJ2puJ0p0gWCMEL9sdqY6HHd/hdj6B6+u2xD9NUCkX9pLC7CPFJHnP0vLO4WIWL
+z5C7yzpeLO9r7Nfnu+2HcRLmuFZVPNxkMq7UymqR1w5ZYJQ5p9E7pyxDVXxHnTqQ
+yLaAS2/9umrOwVnD1NaN3OdAhDedXbH0cF08GcIQD9rnlkLMW4CKtwIDAQABo1Aw
+TjAdBgNVHQ4EFgQUHcxJPBmHF0nSv+FJJI/kwrSThf8wHwYDVR0jBBgwFoAUHcxJ
+PBmHF0nSv+FJJI/kwrSThf8wDAYDVR0TBAUwAwEB/zANBgkqhkiG9w0BAQsFAAOC
+AQEAQyN9nLImdtufuSjXcrCJ3alt/vffHJIzlPgDsNw8+tjI7aRX7CzuurOOEQUC
+fJ9A6O+dat5k5yqVb9hDcD42HXtOjRQDYpQ6dOGirLFThIFSMC/7RiqHk0YtxojM
+ZNBbgXo4o1d+P9b25oc/+pRDzlOvqNL7IzW/LDHnJ4j6tBNguujCB5QFUF5dOa1z
+UR5rupMvv2KpEgRcfW/d3kwcAxH9nI0SHKJenhtweyajUgInK88TC+aT4909c2XA
+EADYyWxj1DMz3/sMpvGegHsfTPegNoDgz2yEKdu53dr4BUpF6E+eoCX9Hv78SWH3
+/rAlkbffzCL5d+I8y0jzEpLEqA==
+-----END CERTIFICATE-----

plugin/tls/test_key.pem

@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----
+MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQCsBiLDVSOViP6c
+hNFSWPmEDuPxTapZzI1hwN+pCe3XvdnzJHs0miSq3cF3c+WsFlLZe1hI2iHFGvip
+4m7+DtQGt799IRT9k4waXs2MEYpATKYnR3ysGDnaKIY3BT4REj2lxBEDoMISNVs6
+aMOJR3nLO5Wi0b9LF6kZQnam4nSnSBYIwQv2x2pjocd3+F2PoHr67bEP01QKRf2k
+sLsI8Ukec/S8s7hYhYvPkLvLOl4s72vs1+e77YdxEua4VlU83GQyrtTKapHXDllg
+lDmn0TunLENVfEedOpDItoBLb/26as7BWcPU1o3c50CEN51dsfRwXTwZwhAP2ueW
+QsxbgIq3AgMBAAECggEAF3FCnYHltoQTxnqnF+S+JAvvbjvaQiCJB9BD6oJK4kKi
+B+tpytJSuuI7ci7eFqR4J+ESN+NaBMVXK7eKzp5wsHWr575xYNkRl6phsnvVbkvD
+vMiWKdGnWJ57I9ZYDfWBZyyf8PGgYODajMwoEXYnF9YH30dcHTydM68GAloL8Zu9
+CtGCmlu4TER0BvG+rK2OD5lt8ORK56eMwzTTqMy0hCkP5VEq8j9RmekEzrgtWKm8
+OI3i8VnpOA0RCVhJ0q5a5jt/xbKRjFNsUNmy9HBRYg7Iw3SCEHmDtz1R9A9rvaJC
+WXqwKbGZPY8W69h8BhKcJ5RrKt2PZyJxw+LB610XSQKBgQDR/LIGXdJR/90epiGC
+p68W9Vc3eWxJlAtLDQCSULphLi6j7D+jesmhD3z2woBPjxkd4TaZa2t94Q1MzSeC
+ON/Aux1huto9ddxvijUQJN3Ep4zPkHdNzHfRwIZsgGH8u77VY/5I4V7IgxKjWlJ6
+Ii8ez8xpWj1rnQ0azSaYIcVl7QKBgQDRt+J+iRjKxHWuXoBFfv8oMfl+iYaMdJxu
+PELWb3RLsZ92hobSAmNR/gC3T7p8NFJlQVCoxZr8zt/Rvqh4aK3aSOuKeUvYAjs1
+/YbPcdSn6uTTIOi6CcHaJ8ZUXNvY5FuoT0+Q9Eb8fw5NGzxsgsfhScELLgbFKb5E
+Tkw43ZqeswKBgQCxXBgZnIEaVVw0mOlQ68TNRWfnKR23f92SBGdpLdpeXp1yQwb1
+U66d5PENkvbBPAJg5GozZzGhXsbXCajHKraCmQiWFTZkFvqbE0cCXcEaatJaNpEu
+GvdRKKXhWwZoa0MiBZUvhXuDLII/iviCxAC8q5LhoSCjlkENVB22/T83eQKBgQC4
+c3wRALG+fWZns5QsC5ONnc6rXXfqhxGi3vuGMMbfYF05WP6xLQp/7eBhWg1R+o7R
+oc24cvxrB+TRTFhOdvsZtvL7es2bMfMz/EUapSp9edpCW3p1Temi30LPplByhf6b
+nQ4FFuRsZa+FX8QYSDpWypCwLY4k0R8YYqklhrrcgwKBgFiM/GnRc230nj0GGWf1
++Ve2M/TQCgS6ufr2F0vU7QkEWfeiN9iunhmhsggqWxOEOU77FhCkQRtztm93hG0K
+eKoHNh/1HvHGBWsR0TaMDw3n8t7Yg5NmQb617nBELZbxxpd358muLiHDoix86W9Q
+xM6hB159G1gOEJsi8exm5AlZ
+-----END PRIVATE KEY-----

plugin/tls/tls.go

@@ -1,6 +1,8 @@
 package tls
 
 import (
+	ctls "crypto/tls"
+
 	"github.com/coredns/coredns/core/dnsserver"
 	"github.com/coredns/coredns/plugin"
 	"github.com/coredns/coredns/plugin/pkg/tls"
@@ -16,6 +18,14 @@ func init() {
 }
 
 func setup(c *caddy.Controller) error {
+	err := parseTLS(c)
+	if err != nil {
+		return plugin.Error("tls", err)
+	}
+	return nil
+}
+
+func parseTLS(c *caddy.Controller) error {
 	config := dnsserver.GetConfig(c)
 
 	if config.TLSConfig != nil {
@@ -27,10 +37,39 @@ func setup(c *caddy.Controller) error {
 		if len(args) < 2 || len(args) > 3 {
 			return plugin.Error("tls", c.ArgErr())
 		}
+		clientAuth := ctls.NoClientCert
+		for c.NextBlock() {
+			switch c.Val() {
+			case "client_auth":
+				authTypeArgs := c.RemainingArgs()
+				if len(authTypeArgs) != 1 {
+					return c.ArgErr()
+				}
+				switch authTypeArgs[0] {
+				case "nocert":
+					clientAuth = ctls.NoClientCert
+				case "request":
+					clientAuth = ctls.RequestClientCert
+				case "require":
+					clientAuth = ctls.RequireAnyClientCert
+				case "verify_if_given":
+					clientAuth = ctls.VerifyClientCertIfGiven
+				case "require_and_verify":
+					clientAuth = ctls.RequireAndVerifyClientCert
+				default:
+					return c.Errf("unknown authentication type '%s'", authTypeArgs[0])
+				}
+			default:
+				return c.Errf("unknown option '%s'", c.Val())
+			}
+		}
 		tls, err := tls.NewTLSConfigFromArgs(args...)
 		if err != nil {
-			return plugin.Error("tls", err)
+			return err
 		}
+		tls.ClientAuth = clientAuth
+		// NewTLSConfigFromArgs only sets RootCAs, so we need to let ClientCAs refer to it.
+		tls.ClientCAs = tls.RootCAs
 		config.TLSConfig = tls
 	}
 	return nil

plugin/tls/tls_test.go

@@ -1,9 +1,12 @@
 package tls
 
 import (
+	"crypto/tls"
 	"strings"
 	"testing"
 
+	"github.com/coredns/coredns/core/dnsserver"
+
 	"github.com/mholt/caddy"
 )
 
@@ -16,6 +19,11 @@ func TestTLS(t *testing.T) {
 	}{
 		// positive
 		// negative
+		{"tls test_cert.pem test_key.pem test_ca.pem {\nunknown\n}", true, "", "unknown option"},
+		// client_auth takes exactly one parameter, which must be one of known keywords.
+		{"tls test_cert.pem test_key.pem test_ca.pem {\nclient_auth\n}", true, "", "Wrong argument"},
+		{"tls test_cert.pem test_key.pem test_ca.pem {\nclient_auth none bogus\n}", true, "", "Wrong argument"},
+		{"tls test_cert.pem test_key.pem test_ca.pem {\nclient_auth bogus\n}", true, "", "unknown authentication type"},
 	}
 
 	for i, test := range tests {
@@ -38,3 +46,37 @@ func TestTLS(t *testing.T) {
 		}
 	}
 }
+
+func TestTLSClientAuthentication(t *testing.T) {
+	// Invalid configurations are tested in the general test case.  In this test we only look into specific details of valid client_auth options.
+	tests := []struct {
+		option string			// tls plugin option(s)
+		expectedType tls.ClientAuthType // expected authentication type.
+	}{
+		// By default, or if 'nocert' is specified, no cert should be requested.
+		// Other cases should be a straightforward mapping from the keyword to the type value.
+		{"", tls.NoClientCert},
+		{"{\nclient_auth nocert\n}", tls.NoClientCert},
+		{"{\nclient_auth request\n}", tls.RequestClientCert},
+		{"{\nclient_auth require\n}", tls.RequireAnyClientCert},
+		{"{\nclient_auth verify_if_given\n}", tls.VerifyClientCertIfGiven},
+		{"{\nclient_auth require_and_verify\n}", tls.RequireAndVerifyClientCert},
+	}
+
+	for i, test := range tests {
+		input := "tls test_cert.pem test_key.pem test_ca.pem " + test.option
+		c := caddy.NewTestController("dns", input)
+		err := setup(c)
+		if err != nil {
+			t.Errorf("Test %d: TLS config is unexpectedly rejected: %v", i, err)
+			continue // there's no point in the rest of the tests.
+		}
+		cfg := dnsserver.GetConfig(c)
+		if cfg.TLSConfig.ClientCAs == nil {
+			t.Errorf("Test %d: Client CA is not configured", i)
+		}
+		if cfg.TLSConfig.ClientAuth != test.expectedType {
+			t.Errorf("Test %d: Unexpected client auth type: %d", i, cfg.TLSConfig.ClientAuth)
+		}
+	}
+}