Enhancements to make ZeroSSL issuer more usable in Caddy

Author: mholt

README.md

@@ -522,7 +522,7 @@ To use ZeroSSL's API instead, use the [`ZeroSSLIssuer`](https://pkg.go.dev/githu
 magic := certmagic.NewDefault()
 
 magic.Issuers = []certmagic.Issuer{
-	certmagic.NewZeroSSLIssuer(magic, certmagic.ZeroSSLIssuer{
+	certmagic.ZeroSSLIssuer{
 		APIKey: "<your ZeroSSL API key>",
 	}),
 }

acmeclient.go

@@ -161,8 +161,8 @@ func (iss *ACMEIssuer) newACMEClient(useTestCA bool) (*acmez.Client, error) {
 	if err != nil {
 		return nil, err
 	}
-	if u.Scheme != "https" && !isLoopback(u.Host) && !isInternal(u.Host) {
-		return nil, fmt.Errorf("%s: insecure CA URL (HTTPS required)", caURL)
+	if u.Scheme != "https" && !SubjectIsInternal(u.Host) {
+		return nil, fmt.Errorf("%s: insecure CA URL (HTTPS required for non-internal CA)", caURL)
 	}
 
 	client := &acmez.Client{

acmeissuer.go

@@ -323,14 +323,32 @@ func (iss *ACMEIssuer) isAgreed() bool {
 
 // PreCheck performs a few simple checks before obtaining or
 // renewing a certificate with ACME, and returns whether this
-// batch is eligible for certificates if using Let's Encrypt.
-// It also ensures that an email address is available.
+// batch is eligible for certificates. It also ensures that an
+// email address is available if possible.
+//
+// IP certificates via ACME are defined in RFC 8738.
 func (am *ACMEIssuer) PreCheck(ctx context.Context, names []string, interactive bool) error {
-	publicCA := strings.Contains(am.CA, "api.letsencrypt.org") || strings.Contains(am.CA, "acme.zerossl.com") || strings.Contains(am.CA, "api.pki.goog")
+	publicCAsAndIPCerts := map[string]bool{ // map of public CAs to whether they support IP certificates (last updated: Q1 2024)
+		"api.letsencrypt.org": false, // https://community.letsencrypt.org/t/certificate-for-static-ip/84/2?u=mholt
+		"acme.zerossl.com":    false, // only supported via their API, not ACME endpoint
+		"api.pki.goog":        true,  // https://pki.goog/faq/#faq-IPCerts
+		"api.buypass.com":     false, // https://community.buypass.com/t/h7hm76w/buypass-support-for-rfc-8738
+		"acme.ssl.com":        false,
+	}
+	var publicCA, ipCertAllowed bool
+	for caSubstr, ipCert := range publicCAsAndIPCerts {
+		if strings.Contains(am.CA, caSubstr) {
+			publicCA, ipCertAllowed = true, ipCert
+			break
+		}
+	}
 	if publicCA {
 		for _, name := range names {
 			if !SubjectQualifiesForPublicCert(name) {
-				return fmt.Errorf("subject does not qualify for a public certificate: %s", name)
+				return fmt.Errorf("subject '%s' does not qualify for a public certificate", name)
+			}
+			if !ipCertAllowed && SubjectIsIP(name) {
+				return fmt.Errorf("subject '%s' cannot have public IP certificate from %s (if CA's policy has changed, please notify the developers in an issue)", name, am.CA)
 			}
 		}
 	}

cache.go

@@ -16,7 +16,7 @@ package certmagic
 
 import (
 	"fmt"
-	weakrand "math/rand" // seeded elsewhere
+	weakrand "math/rand"
 	"strings"
 	"sync"
 	"time"

certificates.go

@@ -386,22 +386,18 @@ func SubjectQualifiesForCert(subj string) bool {
 
 // SubjectQualifiesForPublicCert returns true if the subject
 // name appears eligible for automagic TLS with a public
-// CA such as Let's Encrypt. For example: localhost and IP
-// addresses are not eligible because we cannot obtain certs
+// CA such as Let's Encrypt. For example: internal IP addresses
+// and localhost are not eligible because we cannot obtain certs
 // for those names with a public CA. Wildcard names are
 // allowed, as long as they conform to CABF requirements (only
 // one wildcard label, and it must be the left-most label).
 func SubjectQualifiesForPublicCert(subj string) bool {
 	// must at least qualify for a certificate
 	return SubjectQualifiesForCert(subj) &&
 
-		// localhost, .localhost TLD, and .local TLD are ineligible
+		// loopback hosts and internal IPs are ineligible
 		!SubjectIsInternal(subj) &&
 
-		// cannot be an IP address (as of yet), see
-		// https://community.letsencrypt.org/t/certificate-for-static-ip/84/2?u=mholt
-		!SubjectIsIP(subj) &&
-
 		// only one wildcard label allowed, and it must be left-most, with 3+ labels
 		(!strings.Contains(subj, "*") ||
 			(strings.Count(subj, "*") == 1 &&
@@ -416,12 +412,55 @@ func SubjectIsIP(subj string) bool {
 }
 
 // SubjectIsInternal returns true if subj is an internal-facing
-// hostname or address.
+// hostname or address, including localhost/loopback hosts.
+// Ports are ignored, if present.
 func SubjectIsInternal(subj string) bool {
+	subj = strings.ToLower(strings.TrimSuffix(hostOnly(subj), "."))
 	return subj == "localhost" ||
 		strings.HasSuffix(subj, ".localhost") ||
 		strings.HasSuffix(subj, ".local") ||
-		strings.HasSuffix(subj, ".home.arpa")
+		strings.HasSuffix(subj, ".home.arpa") ||
+		isInternalIP(subj)
+}
+
+// isInternalIP returns true if the IP of addr
+// belongs to a private network IP range. addr
+// must only be an IP or an IP:port combination.
+func isInternalIP(addr string) bool {
+	privateNetworks := []string{
+		"127.0.0.0/8", // IPv4 loopback
+		"0.0.0.0/16",
+		"10.0.0.0/8",     // RFC1918
+		"172.16.0.0/12",  // RFC1918
+		"192.168.0.0/16", // RFC1918
+		"169.254.0.0/16", // RFC3927 link-local
+		"::1/7",          // IPv6 loopback
+		"fe80::/10",      // IPv6 link-local
+		"fc00::/7",       // IPv6 unique local addr
+	}
+	host := hostOnly(addr)
+	ip := net.ParseIP(host)
+	if ip == nil {
+		return false
+	}
+	for _, privateNetwork := range privateNetworks {
+		_, ipnet, _ := net.ParseCIDR(privateNetwork)
+		if ipnet.Contains(ip) {
+			return true
+		}
+	}
+	return false
+}
+
+// hostOnly returns only the host portion of hostport.
+// If there is no port or if there is an error splitting
+// the port off, the whole input string is returned.
+func hostOnly(hostport string) string {
+	host, _, err := net.SplitHostPort(hostport)
+	if err != nil {
+		return hostport // OK; probably had no port to begin with
+	}
+	return host
 }
 
 // MatchWildcard returns true if subject (a candidate DNS name)

certificates_test.go

@@ -143,7 +143,8 @@ func TestSubjectQualifiesForPublicCert(t *testing.T) {
 		{"Sub.Example.COM", true},
 		{"127.0.0.1", false},
 		{"127.0.1.5", false},
-		{"69.123.43.94", false},
+		{"1.2.3.4", true},
+		{"69.123.43.94", true},
 		{"::1", false},
 		{"::", false},
 		{"0.0.0.0", false},
@@ -166,7 +167,7 @@ func TestSubjectQualifiesForPublicCert(t *testing.T) {
 		{"foo.bar.home.arpa", false},
 		{"192.168.1.3", false},
 		{"10.0.2.1", false},
-		{"169.112.53.4", false},
+		{"169.112.53.4", true},
 		{"$hostname", false},
 		{"%HOSTNAME%", false},
 		{"{hostname}", false},

certmagic.go

@@ -302,52 +302,6 @@ type OnDemandConfig struct {
 	hostAllowlist map[string]struct{}
 }
 
-// isLoopback returns true if the hostname of addr looks
-// explicitly like a common local hostname. addr must only
-// be a host or a host:port combination.
-func isLoopback(addr string) bool {
-	host := hostOnly(addr)
-	return host == "localhost" ||
-		strings.Trim(host, "[]") == "::1" ||
-		strings.HasPrefix(host, "127.")
-}
-
-// isInternal returns true if the IP of addr
-// belongs to a private network IP range. addr
-// must only be an IP or an IP:port combination.
-// Loopback addresses are considered false.
-func isInternal(addr string) bool {
-	privateNetworks := []string{
-		"10.0.0.0/8",
-		"172.16.0.0/12",
-		"192.168.0.0/16",
-		"fc00::/7",
-	}
-	host := hostOnly(addr)
-	ip := net.ParseIP(host)
-	if ip == nil {
-		return false
-	}
-	for _, privateNetwork := range privateNetworks {
-		_, ipnet, _ := net.ParseCIDR(privateNetwork)
-		if ipnet.Contains(ip) {
-			return true
-		}
-	}
-	return false
-}
-
-// hostOnly returns only the host portion of hostport.
-// If there is no port or if there is an error splitting
-// the port off, the whole input string is returned.
-func hostOnly(hostport string) string {
-	host, _, err := net.SplitHostPort(hostport)
-	if err != nil {
-		return hostport // OK; probably had no port to begin with
-	}
-	return host
-}
-
 // PreChecker is an interface that can be optionally implemented by
 // Issuers. Pre-checks are performed before each call (or batch of
 // identical calls) to Issue(), giving the issuer the option to ensure

config.go

@@ -599,7 +599,7 @@ func (cfg *Config) obtainCert(ctx context.Context, name string, interactive bool
 			// are compliant, so their CSR requirements just needlessly add friction, complexity,
 			// and inefficiency for clients. CommonName has been deprecated for 25+ years.
 			useCSR := csr
-			if _, ok := issuer.(*ZeroSSLIssuer); ok {
+			if issuer.IssuerKey() == zerosslIssuerKey {
 				useCSR, err = cfg.generateCSR(privKey, []string{name}, true)
 				if err != nil {
 					return err

crypto.go

@@ -281,7 +281,7 @@ func hashCertificateChain(certChain [][]byte) string {
 func namesFromCSR(csr *x509.CertificateRequest) []string {
 	var nameSet []string
 	// TODO: CommonName should not be used (it has been deprecated for 25+ years,
-	// but Sectigo CA still requires it to be filled out and not overlap SANs...)
+	// but ZeroSSL CA still requires it to be filled out and not overlap SANs...)
 	if csr.Subject.CommonName != "" {
 		nameSet = append(nameSet, csr.Subject.CommonName)
 	}

go.mod

@@ -2,8 +2,10 @@ module github.com/caddyserver/certmagic
 
 go 1.22.0
 
+toolchain go1.22.2
+
 require (
-	github.com/caddyserver/zerossl v0.1.1
+	github.com/caddyserver/zerossl v0.1.2
 	github.com/klauspost/cpuid/v2 v2.2.7
 	github.com/libdns/libdns v0.2.2
 	github.com/mholt/acmez/v2 v2.0.0-beta.2

go.sum

@@ -1,5 +1,5 @@
-github.com/caddyserver/zerossl v0.1.1 h1:yQL7QXZnEb/ddH6JsNPGBANETUMHPFlAV5+a+Epxgbo=
-github.com/caddyserver/zerossl v0.1.1/go.mod h1:wtiJEHbdvunr40ZzhXlnIkOB8Xj4eKtBKizCcZitJiQ=
+github.com/caddyserver/zerossl v0.1.2 h1:tlEu1VzWGoqcCpivs9liKAKhfpJWYJkHEMmlxRbVAxE=
+github.com/caddyserver/zerossl v0.1.2/go.mod h1:wtiJEHbdvunr40ZzhXlnIkOB8Xj4eKtBKizCcZitJiQ=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/klauspost/cpuid/v2 v2.0.12/go.mod h1:g2LTdtYhdyuGPqyWyv7qRAmj1WBqxuObKfj5c0PQa7c=

zerosslissuer.go

@@ -26,26 +26,17 @@ import (
 	"time"
 
 	"github.com/caddyserver/zerossl"
+	"github.com/mholt/acmez/v2"
 	"github.com/mholt/acmez/v2/acme"
 	"go.uber.org/zap"
 )
 
-// NewZeroSSLIssuer returns a ZeroSSL issuer with default values filled in
-// for empty fields in the template.
-func NewZeroSSLIssuer(cfg *Config, template ZeroSSLIssuer) *ZeroSSLIssuer {
-	if cfg == nil {
-		panic("cannot make valid ZeroSSLIssuer without an associated CertMagic config")
-	}
-	template.config = cfg
-	template.logger = defaultLogger.Named("zerossl")
-	return &template
-}
-
 // ZeroSSLIssuer can get certificates from ZeroSSL's API. (To use ZeroSSL's ACME
 // endpoint, use the ACMEIssuer instead.) Note that use of the API is restricted
 // by payment tier.
 type ZeroSSLIssuer struct {
 	// The API key (or "access key") for using the ZeroSSL API.
+	// REQUIRED.
 	APIKey string
 
 	// How many days the certificate should be valid for.
@@ -63,16 +54,26 @@ type ZeroSSLIssuer struct {
 	// validation, set this field.
 	CNAMEValidation *DNSManager
 
-	config *Config
-	logger *zap.Logger
+	// Where to store verification material temporarily.
+	// Set this on all instances in a cluster to the same
+	// value to enable distributed verification.
+	Storage Storage
+
+	// An optional (but highly recommended) logger.
+	Logger *zap.Logger
 }
 
 // Issue obtains a certificate for the given csr.
 func (iss *ZeroSSLIssuer) Issue(ctx context.Context, csr *x509.CertificateRequest) (*IssuedCertificate, error) {
 	client := iss.getClient()
 
 	identifiers := namesFromCSR(csr)
-	logger := iss.logger.With(zap.Strings("identifiers", identifiers))
+
+	logger := iss.Logger
+	if logger == nil {
+		logger = zap.NewNop()
+	}
+	logger = logger.With(zap.Strings("identifiers", identifiers))
 
 	logger.Info("creating certificate")
 
@@ -134,16 +135,19 @@ func (iss *ZeroSSLIssuer) Issue(ctx context.Context, csr *x509.CertificateReques
 			}),
 		}
 
-		distSolver := distributedSolver{
-			storage:                iss.config.Storage,
-			storageKeyIssuerPrefix: "zerossl",
-			solver:                 httpVerifier,
+		var solver acmez.Solver = httpVerifier
+		if iss.Storage != nil {
+			solver = distributedSolver{
+				storage:                iss.Storage,
+				storageKeyIssuerPrefix: iss.IssuerKey(),
+				solver:                 httpVerifier,
+			}
 		}
 
-		if err = distSolver.Present(ctx, acme.Challenge{}); err != nil {
+		if err = solver.Present(ctx, acme.Challenge{}); err != nil {
 			return nil, fmt.Errorf("presenting token for verification: %v", err)
 		}
-		defer distSolver.CleanUp(ctx, acme.Challenge{})
+		defer solver.CleanUp(ctx, acme.Challenge{})
 	} else {
 		verificationMethod = zerossl.CNAMEVerification
 		logger = logger.With(zap.String("verification_method", string(verificationMethod)))
@@ -248,9 +252,7 @@ func (iss *ZeroSSLIssuer) getHTTPPort() int {
 }
 
 // IssuerKey returns the unique issuer key for ZeroSSL.
-func (iss *ZeroSSLIssuer) IssuerKey() string {
-	return "zerossl"
-}
+func (iss *ZeroSSLIssuer) IssuerKey() string { return zerosslIssuerKey }
 
 // Revoke revokes the given certificate. Only do this if there is a security or trust
 // concern with the certificate.
@@ -274,6 +276,7 @@ func (iss *ZeroSSLIssuer) Revoke(ctx context.Context, cert CertificateResource,
 const (
 	zerosslAPIBase              = "https://" + zerossl.BaseURL + "/acme"
 	zerosslValidationPathPrefix = "/.well-known/pki-validation/"
+	zerosslIssuerKey            = "zerossl"
 )
 
 // Interface guards