Gracefully handle CSRs without CNs (hashicorp#20982)

* Allow not specifying CN on CSR Signed-off-by: Alexander Scheel <[email protected]> * Add test case validating behavior Signed-off-by: Alexander Scheel <[email protected]> * Add notice about failure to validate Signed-off-by: Alexander Scheel <[email protected]> --------- Signed-off-by: Alexander Scheel <[email protected]>
cipherboy · Jun 7, 2023 · 08c1efa · 08c1efa
1 parent 9f87bcf
commit 08c1efa
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 3 deletions.
diff --git a/builtin/logical/pki/acme_challenge_engine.go b/builtin/logical/pki/acme_challenge_engine.go
@@ -14,6 +14,8 @@ var MaxChallengeTimeout = 1 * time.Minute
 
 const MaxRetryAttempts = 5
 
+const ChallengeAttemptFailedMsg = "this may occur if the validation target was misconfigured: check that challenge responses are available at the required locations and retry."
+
 type ChallengeValidation struct {
 	// Account KID that this validation attempt is recorded under.
 	Account string `json:"account"`
@@ -401,7 +403,7 @@ func (ace *ACMEChallengeEngine) _verifyChallenge(sc *storageContext, id string,
 
 		valid, err = ValidateHTTP01Challenge(authz.Identifier.Value, cv.Token, cv.Thumbprint, config)
 		if err != nil {
-			err = fmt.Errorf("%w: error validating http-01 challenge %v: %s", ErrIncorrectResponse, id, err.Error())
+			err = fmt.Errorf("%w: error validating http-01 challenge %v: %v; %v", ErrIncorrectResponse, id, err, ChallengeAttemptFailedMsg)
 			return ace._verifyChallengeRetry(sc, cv, authzPath, authz, challenge, err, id)
 		}
 	case ACMEDNSChallenge:
@@ -412,7 +414,7 @@ func (ace *ACMEChallengeEngine) _verifyChallenge(sc *storageContext, id string,
 
 		valid, err = ValidateDNS01Challenge(authz.Identifier.Value, cv.Token, cv.Thumbprint, config)
 		if err != nil {
-			err = fmt.Errorf("%w: error validating dns-01 challenge %v: %s", ErrIncorrectResponse, id, err.Error())
+			err = fmt.Errorf("%w: error validating dns-01 challenge %v: %v; %v", ErrIncorrectResponse, id, err, ChallengeAttemptFailedMsg)
 			return ace._verifyChallengeRetry(sc, cv, authzPath, authz, challenge, err, id)
 		}
 	default:

diff --git a/builtin/logical/pki/path_acme_order.go b/builtin/logical/pki/path_acme_order.go
@@ -11,6 +11,7 @@ import (
 	"net"
 	"net/http"
 	"sort"
+	"strings"
 	"time"
 
 	"github.com/hashicorp/vault/sdk/helper/strutil"
@@ -480,6 +481,34 @@ func storeCertificate(sc *storageContext, signedCertBundle *certutil.ParsedCertB
 	return nil
 }
 
+func maybeAugmentReqDataWithSuitableCN(ac *acmeContext, csr *x509.CertificateRequest, data *framework.FieldData) {
+	// Role doesn't require a CN, so we don't care.
+	if !ac.role.RequireCN {
+		return
+	}
+
+	// CSR contains a CN, so use that one.
+	if csr.Subject.CommonName != "" {
+		return
+	}
+
+	// Choose a CN in the order wildcard -> DNS -> IP -> fail.
+	for _, name := range csr.DNSNames {
+		if strings.Contains(name, "*") {
+			data.Raw["common_name"] = name
+			return
+		}
+	}
+	if len(csr.DNSNames) > 0 {
+		data.Raw["common_name"] = csr.DNSNames[0]
+		return
+	}
+	if len(csr.IPAddresses) > 0 {
+		data.Raw["common_name"] = csr.IPAddresses[0].String()
+		return
+	}
+}
+
 func issueCertFromCsr(ac *acmeContext, csr *x509.CertificateRequest) (*certutil.ParsedCertBundle, issuerID, error) {
 	pemBlock := &pem.Block{
 		Type:    "CERTIFICATE REQUEST",
@@ -495,6 +524,11 @@ func issueCertFromCsr(ac *acmeContext, csr *x509.CertificateRequest) (*certutil.
 		Schema: getCsrSignVerbatimSchemaFields(),
 	}
 
+	// XXX: Usability hack: by default, minimalist roles have require_cn=true,
+	// but some ACME clients do not provision one in the certificate as modern
+	// (TLS) clients are mostly verifying against server's DNS SANs.
+	maybeAugmentReqDataWithSuitableCN(ac, csr, data)
+
 	signingBundle, issuerId, err := ac.sc.fetchCAInfoWithIssuer(ac.issuer.ID.String(), IssuanceUsage)
 	if err != nil {
 		return nil, "", fmt.Errorf("failed loading CA %s: %w", ac.issuer.ID.String(), err)

diff --git a/builtin/logical/pki/path_acme_test.go b/builtin/logical/pki/path_acme_test.go
@@ -784,7 +784,7 @@ func TestAcmeIgnoresRoleExtKeyUsage(t *testing.T) {
 		"allowed_domains":             "localdomain",
 		"allow_subdomains":            "true",
 		"allow_wildcard_certificates": "true",
-		"require_cn":                  "false",
+		"require_cn":                  "true", /* explicit default */
 		"server_flag":                 "true",
 		"client_flag":                 "true",
 		"code_signing_flag":           "true",