caddytls: Refactor certificate selection policies (close #1575)

Certificate selection used to be a module, but this seems unnecessary,
especially since the built-in CustomSelectionPolicy allows quite complex
selection logic on a number of fields in certs. If we need to extend
that logic, we can, but I don't think there are SO many possibilities
that we need modules.

This update also allows certificate selection to choose between multiple
matching certs based on client compatibility and makes a number of other
improvements in the default cert selection logic, both here and in the
latest CertMagic.

The hardest part of this was the conn policy consolidation logic
(Caddyfile only, of course). We have to merge connection policies that
we can easily combine, because if two certs are manually loaded in a
Caddyfile site block, that produces two connection policies, and each
cert is tagged with a different tag, meaning only the first would ever
be selected. So given the same matchers, we can merge the two, but this
required improving the Tag selection logic to support multiple tags to
choose from, hence "tags" changed to "any_tag" or "all_tags" (but we
use any_tag in our Caddyfile logic).

Combining conn policies with conflicting settings is impossible, so
that should return an error if two policies with the exact same matchers
have non-empty settings that are not the same (the one exception being
any_tag which we can merge because the logic for them is to OR them).

It was a bit complicated. It seems to work in numerous tests I've
conducted, but we'll see how it pans out in the release candidates.

8 files changed, 195 insertions, 62 deletions

diff --git a/caddyconfig/caddyfile/adapter.go b/caddyconfig/caddyfile/adapter.go
index 02a951b56..5b4495eb7 100644
--- a/caddyconfig/caddyfile/adapter.go
+++ b/caddyconfig/caddyfile/adapter.go
@@ -68,6 +68,11 @@ func (a Adapter) Adapt(body []byte, options map[string]interface{}) ([]byte, []c
 // into JSON. Caddyfile-unmarshaled values
 // will not be used directly; they will be
 // encoded as JSON and then used from that.
+// Implementations must be able to support
+// multiple segments (instances of their
+// directive or batch of tokens); typically
+// this means wrapping all token logic in
+// a loop: `for d.Next() { ... }`.
 type Unmarshaler interface {
 	UnmarshalCaddyfile(d *Dispenser) error
 }
diff --git a/caddyconfig/httpcaddyfile/builtins.go b/caddyconfig/httpcaddyfile/builtins.go
index 26a421c9f..1efe5accc 100644
--- a/caddyconfig/httpcaddyfile/builtins.go
+++ b/caddyconfig/httpcaddyfile/builtins.go
@@ -71,6 +71,7 @@ func parseTLS(h Helper) ([]ConfigValue, error) {
 	cp := new(caddytls.ConnectionPolicy)
 	var fileLoader caddytls.FileLoader
 	var folderLoader caddytls.FolderLoader
+	var certSelector caddytls.CustomCertSelectionPolicy
 	var acmeIssuer *caddytls.ACMEIssuer
 	var internalIssuer *caddytls.InternalIssuer
 	var onDemand bool
@@ -135,8 +136,8 @@ func parseTLS(h Helper) ([]ConfigValue, error) {
 				// remember this for next time we see this cert file
 				tlsCertTags[certFilename] = tag
 			}
-			certSelector := caddytls.CustomCertSelectionPolicy{Tag: tag}
-			cp.CertSelection = caddyconfig.JSONModuleObject(certSelector, "policy", "custom", h.warnings)
+			certSelector.AnyTag = append(certSelector.AnyTag, tag)
+
 		default:
 			return nil, h.ArgErr()
 		}
@@ -297,6 +298,11 @@ func parseTLS(h Helper) ([]ConfigValue, error) {
 		})
 	}
 
+	// custom certificate selection
+	if len(certSelector.AnyTag) > 0 {
+		cp.CertSelection = &certSelector
+	}
+
 	// connection policy -- always add one, to ensure that TLS
 	// is enabled, because this directive was used (this is
 	// needed, for instance, when a site block has a key of
diff --git a/caddyconfig/httpcaddyfile/httptype.go b/caddyconfig/httpcaddyfile/httptype.go
index f1109581a..4df5421af 100644
--- a/caddyconfig/httpcaddyfile/httptype.go
+++ b/caddyconfig/httpcaddyfile/httptype.go
@@ -527,7 +527,11 @@ func (st *ServerType) serversFromPairings(
 		}
 
 		// tidy things up a bit
-		srv.TLSConnPolicies = consolidateConnPolicies(srv.TLSConnPolicies)
+		var err error
+		srv.TLSConnPolicies, err = consolidateConnPolicies(srv.TLSConnPolicies)
+		if err != nil {
+			return nil, fmt.Errorf("consolidating TLS connection policies for server %d: %v", i, err)
+		}
 		srv.Routes = consolidateRoutes(srv.Routes)
 
 		servers[fmt.Sprintf("srv%d", i)] = srv
@@ -538,7 +542,7 @@ func (st *ServerType) serversFromPairings(
 
 // consolidateConnPolicies combines TLS connection policies that are the same,
 // for a cleaner overall output.
-func consolidateConnPolicies(cps caddytls.ConnectionPolicies) caddytls.ConnectionPolicies {
+func consolidateConnPolicies(cps caddytls.ConnectionPolicies) (caddytls.ConnectionPolicies, error) {
 	for i := 0; i < len(cps); i++ {
 		for j := 0; j < len(cps); j++ {
 			if j == i {
@@ -551,9 +555,106 @@ func consolidateConnPolicies(cps caddytls.ConnectionPolicies) caddytls.Connectio
 				i--
 				break
 			}
+
+			// if they have the same matcher, try to reconcile each field: either they must
+			// be identical, or we have to be able to combine them safely
+			if reflect.DeepEqual(cps[i].MatchersRaw, cps[j].MatchersRaw) {
+				if len(cps[i].ALPN) > 0 &&
+					len(cps[j].ALPN) > 0 &&
+					!reflect.DeepEqual(cps[i].ALPN, cps[j].ALPN) {
+					return nil, fmt.Errorf("two policies with same match criteria have conflicting ALPN: %v vs. %v",
+						cps[i].ALPN, cps[j].ALPN)
+				}
+				if len(cps[i].CipherSuites) > 0 &&
+					len(cps[j].CipherSuites) > 0 &&
+					!reflect.DeepEqual(cps[i].CipherSuites, cps[j].CipherSuites) {
+					return nil, fmt.Errorf("two policies with same match criteria have conflicting cipher suites: %v vs. %v",
+						cps[i].CipherSuites, cps[j].CipherSuites)
+				}
+				if cps[i].ClientAuthentication == nil &&
+					cps[j].ClientAuthentication != nil &&
+					!reflect.DeepEqual(cps[i].ClientAuthentication, cps[j].ClientAuthentication) {
+					return nil, fmt.Errorf("two policies with same match criteria have conflicting client auth configuration: %+v vs. %+v",
+						cps[i].ClientAuthentication, cps[j].ClientAuthentication)
+				}
+				if len(cps[i].Curves) > 0 &&
+					len(cps[j].Curves) > 0 &&
+					!reflect.DeepEqual(cps[i].Curves, cps[j].Curves) {
+					return nil, fmt.Errorf("two policies with same match criteria have conflicting curves: %v vs. %v",
+						cps[i].Curves, cps[j].Curves)
+				}
+				if cps[i].DefaultSNI != "" &&
+					cps[j].DefaultSNI != "" &&
+					cps[i].DefaultSNI != cps[j].DefaultSNI {
+					return nil, fmt.Errorf("two policies with same match criteria have conflicting default SNI: %s vs. %s",
+						cps[i].DefaultSNI, cps[j].DefaultSNI)
+				}
+				if cps[i].ProtocolMin != "" &&
+					cps[j].ProtocolMin != "" &&
+					cps[i].ProtocolMin != cps[j].ProtocolMin {
+					return nil, fmt.Errorf("two policies with same match criteria have conflicting min protocol: %s vs. %s",
+						cps[i].ProtocolMin, cps[j].ProtocolMin)
+				}
+				if cps[i].ProtocolMax != "" &&
+					cps[j].ProtocolMax != "" &&
+					cps[i].ProtocolMax != cps[j].ProtocolMax {
+					return nil, fmt.Errorf("two policies with same match criteria have conflicting max protocol: %s vs. %s",
+						cps[i].ProtocolMax, cps[j].ProtocolMax)
+				}
+				if cps[i].CertSelection != nil && cps[j].CertSelection != nil {
+					// merging fields other than AnyTag is not implemented
+					if !reflect.DeepEqual(cps[i].CertSelection.SerialNumber, cps[j].CertSelection.SerialNumber) ||
+						!reflect.DeepEqual(cps[i].CertSelection.SubjectOrganization, cps[j].CertSelection.SubjectOrganization) ||
+						cps[i].CertSelection.PublicKeyAlgorithm != cps[j].CertSelection.PublicKeyAlgorithm ||
+						!reflect.DeepEqual(cps[i].CertSelection.AllTags, cps[j].CertSelection.AllTags) {
+						return nil, fmt.Errorf("two policies with same match criteria have conflicting cert selections: %+v vs. %+v",
+							cps[i].CertSelection, cps[j].CertSelection)
+					}
+				}
+
+				// by now we've decided that we can merge the two -- we'll keep i and drop j
+
+				if len(cps[i].ALPN) == 0 && len(cps[j].ALPN) > 0 {
+					cps[i].ALPN = cps[j].ALPN
+				}
+				if len(cps[i].CipherSuites) == 0 && len(cps[j].CipherSuites) > 0 {
+					cps[i].CipherSuites = cps[j].CipherSuites
+				}
+				if cps[i].ClientAuthentication == nil && cps[j].ClientAuthentication != nil {
+					cps[i].ClientAuthentication = cps[j].ClientAuthentication
+				}
+				if len(cps[i].Curves) == 0 && len(cps[j].Curves) > 0 {
+					cps[i].Curves = cps[j].Curves
+				}
+				if cps[i].DefaultSNI == "" && cps[j].DefaultSNI != "" {
+					cps[i].DefaultSNI = cps[j].DefaultSNI
+				}
+				if cps[i].ProtocolMin == "" && cps[j].ProtocolMin != "" {
+					cps[i].ProtocolMin = cps[j].ProtocolMin
+				}
+				if cps[i].ProtocolMax == "" && cps[j].ProtocolMax != "" {
+					cps[i].ProtocolMax = cps[j].ProtocolMax
+				}
+
+				if cps[i].CertSelection == nil && cps[j].CertSelection != nil {
+					// if j is the only one with a policy, move it over to i
+					cps[i].CertSelection = cps[j].CertSelection
+				} else if cps[i].CertSelection != nil && cps[j].CertSelection != nil {
+					// if both have one, then combine AnyTag
+					for _, tag := range cps[j].CertSelection.AnyTag {
+						if !sliceContains(cps[i].CertSelection.AnyTag, tag) {
+							cps[i].CertSelection.AnyTag = append(cps[i].CertSelection.AnyTag, tag)
+						}
+					}
+				}
+
+				cps = append(cps[:j], cps[j+1:]...)
+				i--
+				break
+			}
 		}
 	}
-	return cps
+	return cps, nil
 }
 
 // appendSubrouteToRouteList appends the routes in subroute
diff --git a/caddytest/integration/sni_test.go b/caddytest/integration/sni_test.go
index d32978248..e48346df2 100644
--- a/caddytest/integration/sni_test.go
+++ b/caddytest/integration/sni_test.go
@@ -57,8 +57,7 @@ func TestDefaultSNI(t *testing.T) {
             "tls_connection_policies": [
               {
                 "certificate_selection": {
-                  "policy": "custom",
-                  "tag": "cert0"
+                  "any_tag": ["cert0"]
                 },
                 "match": {
                   "sni": [
@@ -155,8 +154,7 @@ func TestDefaultSNIWithNamedHostAndExplicitIP(t *testing.T) {
             "tls_connection_policies": [
               {
                 "certificate_selection": {
-                  "policy": "custom",
-                  "tag": "cert0"
+                  "any_tag": ["cert0"]
                 },
                 "default_sni": "a.caddy.localhost",
                 "match": {
@@ -238,8 +236,7 @@ func TestDefaultSNIWithPortMappingOnly(t *testing.T) {
             "tls_connection_policies": [
               {
                 "certificate_selection": {
-                  "policy": "custom",
-                  "tag": "cert0"
+                  "any_tag": ["cert0"]
                 },
                 "default_sni": "a.caddy.localhost"
               }
diff --git a/go.mod b/go.mod
index c8279c412..82eda3ab3 100644
--- a/go.mod
+++ b/go.mod
@@ -5,7 +5,7 @@ go 1.14
 require (
 	github.com/Masterminds/sprig/v3 v3.0.2
 	github.com/alecthomas/chroma v0.7.2-0.20200305040604-4f3623dce67a
-	github.com/caddyserver/certmagic v0.10.6
+	github.com/caddyserver/certmagic v0.10.7
 	github.com/dustin/go-humanize v1.0.1-0.20200219035652-afde56e7acac
 	github.com/go-acme/lego/v3 v3.5.0
 	github.com/google/cel-go v0.4.1
diff --git a/go.sum b/go.sum
index 8d0e925f3..606edcaa0 100644
--- a/go.sum
+++ b/go.sum
@@ -120,8 +120,8 @@ github.com/bombsimon/wsl/v2 v2.0.0/go.mod h1:mf25kr/SqFEPhhcxW1+7pxzGlW+hIl/hYTK
 github.com/boombuler/barcode v1.0.0/go.mod h1:paBWMcWSl3LHKBqUq+rly7CNSldXjb2rDl3JlRe0mD8=
 github.com/bradfitz/go-smtpd v0.0.0-20170404230938-deb6d6237625/go.mod h1:HYsPBTaaSFSlLx/70C2HPIMNZpVV8+vt/A+FMnYP11g=
 github.com/buger/jsonparser v0.0.0-20181115193947-bf1c66bbce23/go.mod h1:bbYlZJ7hK1yFx9hf58LP0zeX7UjIGs20ufpu3evjr+s=
-github.com/caddyserver/certmagic v0.10.6 h1:sCya6FmfaN74oZE46kqfaFOVoROD/mF36rTQfjN7TZc=
-github.com/caddyserver/certmagic v0.10.6/go.mod h1:Y8jcUBctgk/IhpAzlHKfimZNyXCkfGgRTC0orl8gROQ=
+github.com/caddyserver/certmagic v0.10.7 h1:OwT4Delj91ee7vumu+CnFJdWLsYBYj6kJ2PwsoqI7LA=
+github.com/caddyserver/certmagic v0.10.7/go.mod h1:Y8jcUBctgk/IhpAzlHKfimZNyXCkfGgRTC0orl8gROQ=
 github.com/cenkalti/backoff/v4 v4.0.0 h1:6VeaLF9aI+MAUQ95106HwWzYZgJJpZ4stumjj6RFYAU=
 github.com/cenkalti/backoff/v4 v4.0.0/go.mod h1:eEew/i+1Q6OrCDZh3WiXYv3+nJwBASZ8Bog/87DQnVg=
 github.com/census-instrumentation/opencensus-proto v0.2.0/go.mod h1:f6KPmirojxKA12rnyqOA5BBL4O983OfeGPqjHWSTneU=
diff --git a/modules/caddytls/certselection.go b/modules/caddytls/certselection.go
index 88b7e0be8..8a24034c4 100644
--- a/modules/caddytls/certselection.go
+++ b/modules/caddytls/certselection.go
@@ -20,65 +20,104 @@ import (
 	"fmt"
 	"math/big"
 
-	"github.com/caddyserver/caddy/v2"
 	"github.com/caddyserver/certmagic"
 )
 
-func init() {
-	caddy.RegisterModule(CustomCertSelectionPolicy{})
-}
-
 // CustomCertSelectionPolicy represents a policy for selecting the certificate
 // used to complete a handshake when there may be multiple options. All fields
 // specified must match the candidate certificate for it to be chosen.
 // This was needed to solve https://github.com/caddyserver/caddy/issues/2588.
 type CustomCertSelectionPolicy struct {
-	SerialNumber        *big.Int           `json:"serial_number,omitempty"`
-	SubjectOrganization string             `json:"subject_organization,omitempty"`
-	PublicKeyAlgorithm  PublicKeyAlgorithm `json:"public_key_algorithm,omitempty"`
-	Tag                 string             `json:"tag,omitempty"`
-}
+	// The certificate must have one of these serial numbers.
+	SerialNumber []*big.Int `json:"serial_number,omitempty"`
 
-// CaddyModule returns the Caddy module information.
-func (CustomCertSelectionPolicy) CaddyModule() caddy.ModuleInfo {
-	return caddy.ModuleInfo{
-		ID:  "tls.certificate_selection.custom",
-		New: func() caddy.Module { return new(CustomCertSelectionPolicy) },
-	}
+	// The certificate must have one of these organization names.
+	SubjectOrganization []string `json:"subject_organization,omitempty"`
+
+	// The certificate must use this public key algorithm.
+	PublicKeyAlgorithm PublicKeyAlgorithm `json:"public_key_algorithm,omitempty"`
+
+	// The certificate must have at least one of the tags in the list.
+	AnyTag []string `json:"any_tag,omitempty"`
+
+	// The certificate must have all of the tags in the list.
+	AllTags []string `json:"all_tags,omitempty"`
 }
 
-// SelectCertificate implements certmagic.CertificateSelector.
-func (p CustomCertSelectionPolicy) SelectCertificate(_ *tls.ClientHelloInfo, choices []certmagic.Certificate) (certmagic.Certificate, error) {
+// SelectCertificate implements certmagic.CertificateSelector. It
+// only chooses a certificate that at least meets the criteria in
+// p. It then chooses the first non-expired certificate that is
+// compatible with the client. If none are valid, it chooses the
+// first viable candidate anyway.
+func (p CustomCertSelectionPolicy) SelectCertificate(hello *tls.ClientHelloInfo, choices []certmagic.Certificate) (certmagic.Certificate, error) {
+	viable := make([]certmagic.Certificate, 0, len(choices))
+
+nextChoice:
 	for _, cert := range choices {
-		if p.SerialNumber != nil && cert.SerialNumber.Cmp(p.SerialNumber) != 0 {
-			continue
+		if len(p.SerialNumber) > 0 {
+			var found bool
+			for _, sn := range p.SerialNumber {
+				if cert.Leaf.SerialNumber.Cmp(sn) == 0 {
+					found = true
+					break
+				}
+			}
+			if !found {
+				continue
+			}
+		}
+
+		if len(p.SubjectOrganization) > 0 {
+			var found bool
+			for _, subjOrg := range p.SubjectOrganization {
+				for _, org := range cert.Leaf.Subject.Organization {
+					if subjOrg == org {
+						found = true
+						break
+					}
+				}
+			}
+			if !found {
+				continue
+			}
 		}
 
 		if p.PublicKeyAlgorithm != PublicKeyAlgorithm(x509.UnknownPublicKeyAlgorithm) &&
-			PublicKeyAlgorithm(cert.PublicKeyAlgorithm) != p.PublicKeyAlgorithm {
+			PublicKeyAlgorithm(cert.Leaf.PublicKeyAlgorithm) != p.PublicKeyAlgorithm {
 			continue
 		}
 
-		if p.SubjectOrganization != "" {
-			var matchOrg bool
-			for _, org := range cert.Subject.Organization {
-				if p.SubjectOrganization == org {
-					matchOrg = true
+		if len(p.AnyTag) > 0 {
+			var found bool
+			for _, tag := range p.AnyTag {
+				if cert.HasTag(tag) {
+					found = true
 					break
 				}
 			}
-			if !matchOrg {
+			if !found {
 				continue
 			}
 		}
 
-		if p.Tag != "" && !cert.HasTag(p.Tag) {
-			continue
+		if len(p.AllTags) > 0 {
+			for _, tag := range p.AllTags {
+				if !cert.HasTag(tag) {
+					continue nextChoice
+				}
+			}
 		}
 
-		return cert, nil
+		// this certificate at least meets the policy's requirements,
+		// but we still have to check expiration and compatibility
+		viable = append(viable, cert)
 	}
-	return certmagic.Certificate{}, fmt.Errorf("no certificates matched custom selection policy")
+
+	if len(viable) == 0 {
+		return certmagic.Certificate{}, fmt.Errorf("no certificates matched custom selection policy")
+	}
+
+	return certmagic.DefaultCertificateSelector(hello, viable)
 }
 
 // Interface guard
diff --git a/modules/caddytls/connpolicy.go b/modules/caddytls/connpolicy.go
index 52ccdd96d..4fd811237 100644
--- a/modules/caddytls/connpolicy.go
+++ b/modules/caddytls/connpolicy.go
@@ -18,12 +18,10 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"encoding/base64"
-	"encoding/json"
 	"fmt"
 	"strings"
 
 	"github.com/caddyserver/caddy/v2"
-	"github.com/caddyserver/certmagic"
 	"github.com/go-acme/lego/v3/challenge/tlsalpn01"
 )
 
@@ -46,15 +44,6 @@ func (cp ConnectionPolicies) Provision(ctx caddy.Context) error {
 			cp[i].matchers = append(cp[i].matchers, modIface.(ConnectionMatcher))
 		}
 
-		// certificate selector
-		if pol.CertSelection != nil {
-			val, err := ctx.LoadModule(pol, "CertSelection")
-			if err != nil {
-				return fmt.Errorf("loading certificate selection module: %s", err)
-			}
-			cp[i].certSelector = val.(certmagic.CertificateSelector)
-		}
-
 		// enable HTTP/2 by default
 		if len(pol.ALPN) == 0 {
 			pol.ALPN = append(pol.ALPN, defaultALPN...)
@@ -123,7 +112,7 @@ type ConnectionPolicy struct {
 
 	// How to choose a certificate if more than one matched
 	// the given ServerName (SNI) value.
-	CertSelection json.RawMessage `json:"certificate_selection,omitempty" caddy:"namespace=tls.certificate_selection inline_key=policy"`
+	CertSelection *CustomCertSelectionPolicy `json:"certificate_selection,omitempty"`
 
 	// The list of cipher suites to support. Caddy's
 	// defaults are modern and secure.
@@ -151,8 +140,6 @@ type ConnectionPolicy struct {
 	DefaultSNI string `json:"default_sni,omitempty"`
 
 	matchers     []ConnectionMatcher
-	certSelector certmagic.CertificateSelector
-
 	stdTLSConfig *tls.Config
 }
 
@@ -184,9 +171,7 @@ func (p *ConnectionPolicy) buildStandardTLSConfig(ctx caddy.Context) error {
 			// more at handshake-time, but I don't know how to practically pre-build
 			// a certmagic config for each combination of conn policy + automation policy...
 			cfg := *tlsApp.getConfigForName(hello.ServerName)
-			if p.certSelector != nil {
-				cfg.CertSelection = p.certSelector
-			}
+			cfg.CertSelection = p.CertSelection
 			cfg.DefaultServerName = p.DefaultSNI
 			return cfg.GetCertificate(hello)
 		},