Merge branch 'master' of github.com:letsencrypt/boulder into ocsp-fai…

…l-stops-issuances

ca/ca.go

@@ -584,7 +584,9 @@ func (ca *CertificateAuthorityImpl) issuePrecertificateInner(ctx context.Context
 		*issueReq.RegistrationID,
 	); err != nil {
 		ca.log.AuditErr(err.Error())
-		return nil, berrors.MalformedError(err.Error())
+		// VerifyCSR returns berror instances that can be passed through as-is
+		// without wrapping.
+		return nil, err
 	}
 
 	extensions, err := ca.extensionsFromCSR(csr)

ca/ca_test.go

@@ -588,46 +588,47 @@ func TestInvalidCSRs(t *testing.T) {
 		csrPath      string
 		check        func(t *testing.T, ca *CertificateAuthorityImpl, sa *mockSA)
 		errorMessage string
+		errorType    berrors.ErrorType
 	}{
 		// Test that the CA rejects CSRs that have no names.
 		//
 		// CSR generated by Go:
 		// * Random RSA public key.
 		// * CN = [none]
 		// * DNSNames = [none]
-		{"RejectNoHostnames", "./testdata/no_names.der.csr", nil, "Issued certificate with no names"},
+		{"RejectNoHostnames", "./testdata/no_names.der.csr", nil, "Issued certificate with no names", berrors.BadCSR},
 
 		// Test that the CA rejects CSRs that have too many names.
 		//
 		// CSR generated by Go:
 		// * Random public key
 		// * CN = [none]
 		// * DNSNames = not-example.com, www.not-example.com, mail.example.com
-		{"RejectTooManyHostnames", "./testdata/too_many_names.der.csr", nil, "Issued certificate with too many names"},
+		{"RejectTooManyHostnames", "./testdata/too_many_names.der.csr", nil, "Issued certificate with too many names", berrors.BadCSR},
 
 		// Test that the CA rejects CSRs that have public keys that are too short.
 		//
 		// CSR generated by Go:
 		// * Random public key -- 512 bits long
 		// * CN = (none)
 		// * DNSNames = not-example.com, www.not-example.com, mail.not-example.com
-		{"RejectShortKey", "./testdata/short_key.der.csr", nil, "Issued a certificate with too short a key."},
+		{"RejectShortKey", "./testdata/short_key.der.csr", nil, "Issued a certificate with too short a key.", berrors.BadPublicKey},
 
 		// CSR generated by Go:
 		// * Random RSA public key.
 		// * CN = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.com
 		// * DNSNames = [none]
-		{"RejectLongCommonName", "./testdata/long_cn.der.csr", nil, "Issued a certificate with a CN over 64 bytes."},
+		{"RejectLongCommonName", "./testdata/long_cn.der.csr", nil, "Issued a certificate with a CN over 64 bytes.", berrors.BadCSR},
 
 		// CSR generated by OpenSSL:
 		// Edited signature to become invalid.
-		{"RejectWrongSignature", "./testdata/invalid_signature.der.csr", nil, "Issued a certificate based on a CSR with an invalid signature."},
+		{"RejectWrongSignature", "./testdata/invalid_signature.der.csr", nil, "Issued a certificate based on a CSR with an invalid signature.", berrors.BadCSR},
 
 		// CSR generated by Go:
 		// * Random public key
 		// * CN = not-example.com
 		// * Includes an extensionRequest attribute for an empty TLS Feature extension
-		{"TLSFeatureUnknown", "./testdata/tls_feature_unknown.der.csr", issueCertificateSubTestTLSFeatureUnknown, "Issued a certificate based on a CSR with an empty TLS feature extension."},
+		{"TLSFeatureUnknown", "./testdata/tls_feature_unknown.der.csr", issueCertificateSubTestTLSFeatureUnknown, "Issued a certificate based on a CSR with an empty TLS feature extension.", berrors.Malformed},
 	}
 
 	for _, testCase := range testCases {
@@ -650,7 +651,7 @@ func TestInvalidCSRs(t *testing.T) {
 			issueReq := &caPB.IssueCertificateRequest{Csr: serializedCSR, RegistrationID: &arbitraryRegID}
 			_, err = ca.IssuePrecertificate(ctx, issueReq)
 
-			test.Assert(t, berrors.Is(err, berrors.Malformed), "Incorrect error type returned")
+			test.Assert(t, berrors.Is(err, testCase.errorType), "Incorrect error type returned")
 			test.AssertEquals(t, signatureCountByPurpose("cert", ca.signatureCount), 0)
 
 			test.AssertError(t, err, testCase.errorMessage)

csr/csr.go

@@ -3,11 +3,10 @@ package csr
 import (
 	"crypto"
 	"crypto/x509"
-	"errors"
-	"fmt"
 	"strings"
 
 	"github.com/letsencrypt/boulder/core"
+	berrors "github.com/letsencrypt/boulder/errors"
 	"github.com/letsencrypt/boulder/goodkey"
 	"github.com/letsencrypt/boulder/identifier"
 )
@@ -33,13 +32,13 @@ var goodSignatureAlgorithms = map[x509.SignatureAlgorithm]bool{
 }
 
 var (
-	invalidPubKey        = errors.New("invalid public key in CSR")
-	unsupportedSigAlg    = errors.New("signature algorithm not supported")
-	invalidSig           = errors.New("invalid signature on CSR")
-	invalidEmailPresent  = errors.New("CSR contains one or more email address fields")
-	invalidIPPresent     = errors.New("CSR contains one or more IP address fields")
-	invalidNoDNS         = errors.New("at least one DNS name is required")
-	invalidAllSANTooLong = errors.New("CSR doesn't contain a SAN short enough to fit in CN")
+	invalidPubKey        = berrors.BadPublicKeyError("invalid public key in CSR")
+	unsupportedSigAlg    = berrors.BadCSRError("signature algorithm not supported")
+	invalidSig           = berrors.BadCSRError("invalid signature on CSR")
+	invalidEmailPresent  = berrors.BadCSRError("CSR contains one or more email address fields")
+	invalidIPPresent     = berrors.BadCSRError("CSR contains one or more IP address fields")
+	invalidNoDNS         = berrors.BadCSRError("at least one DNS name is required")
+	invalidAllSANTooLong = berrors.BadCSRError("CSR doesn't contain a SAN short enough to fit in CN")
 )
 
 // VerifyCSR checks the validity of a x509.CertificateRequest. Before doing checks it normalizes
@@ -52,7 +51,7 @@ func VerifyCSR(csr *x509.CertificateRequest, maxNames int, keyPolicy *goodkey.Ke
 		return invalidPubKey
 	}
 	if err := keyPolicy.GoodKey(key); err != nil {
-		return fmt.Errorf("invalid public key in CSR: %s", err)
+		return berrors.BadPublicKeyError("invalid public key in CSR: %s", err)
 	}
 	if !goodSignatureAlgorithms[csr.SignatureAlgorithm] {
 		return unsupportedSigAlg
@@ -73,10 +72,10 @@ func VerifyCSR(csr *x509.CertificateRequest, maxNames int, keyPolicy *goodkey.Ke
 		return invalidAllSANTooLong
 	}
 	if len(csr.Subject.CommonName) > maxCNLength {
-		return fmt.Errorf("CN was longer than %d bytes", maxCNLength)
+		return berrors.BadCSRError("CN was longer than %d bytes", maxCNLength)
 	}
 	if len(csr.DNSNames) > maxNames {
-		return fmt.Errorf("CSR contains more than %d DNS names", maxNames)
+		return berrors.BadCSRError("CSR contains more than %d DNS names", maxNames)
 	}
 	idents := make([]identifier.ACMEIdentifier, len(csr.DNSNames))
 	for i, dnsName := range csr.DNSNames {

csr/csr_test.go

@@ -11,6 +11,7 @@ import (
 	"testing"
 
 	"github.com/letsencrypt/boulder/core"
+	berrors "github.com/letsencrypt/boulder/errors"
 	"github.com/letsencrypt/boulder/goodkey"
 	"github.com/letsencrypt/boulder/identifier"
 	"github.com/letsencrypt/boulder/test"
@@ -120,15 +121,15 @@ func TestVerifyCSR(t *testing.T) {
 			testingPolicy,
 			&mockPA{},
 			0,
-			errors.New("CN was longer than 64 bytes"),
+			berrors.BadCSRError("CN was longer than %d bytes", maxCNLength),
 		},
 		{
 			signedReqWithHosts,
 			1,
 			testingPolicy,
 			&mockPA{},
 			0,
-			errors.New("CSR contains more than 1 DNS names"),
+			berrors.BadCSRError("CSR contains more than 1 DNS names"),
 		},
 		{
 			signedReqWithBadNames,

errors/errors.go

@@ -26,6 +26,7 @@ const (
 	OrderNotReady
 	DNS
 	BadPublicKey
+	BadCSR
 )
 
 // BoulderError represents internal Boulder errors
@@ -135,3 +136,7 @@ func DNSError(msg string, args ...interface{}) error {
 func BadPublicKeyError(msg string, args ...interface{}) error {
 	return New(BadPublicKey, msg, args...)
 }
+
+func BadCSRError(msg string, args ...interface{}) error {
+	return New(BadCSR, msg, args...)
+}

probs/probs.go

@@ -26,6 +26,7 @@ const (
 	BadSignatureAlgorithmProblem = ProblemType("badSignatureAlgorithm")
 	BadPublicKeyProblem          = ProblemType("badPublicKey")
 	BadRevocationReasonProblem   = ProblemType("badRevocationReason")
+	BadCSRProblem                = ProblemType("badCSR")
 
 	V1ErrorNS = "urn:acme:error:"
 	V2ErrorNS = "urn:ietf:params:acme:error:"
@@ -321,3 +322,12 @@ func BadRevocationReason(detail string, a ...interface{}) *ProblemDetails {
 		HTTPStatus: http.StatusBadRequest,
 	}
 }
+
+// BadCSR returns a ProblemDetails representing a BadCSRProblem.
+func BadCSR(detail string, a ...interface{}) *ProblemDetails {
+	return &ProblemDetails{
+		Type:       BadCSRProblem,
+		Detail:     fmt.Sprintf(detail, a...),
+		HTTPStatus: http.StatusBadRequest,
+	}
+}

ra/ra.go

@@ -1035,7 +1035,9 @@ func (ra *RegistrationAuthorityImpl) FinalizeOrder(ctx context.Context, req *rap
 	}
 
 	if err := csrlib.VerifyCSR(csrOb, ra.maxNames, &ra.keyPolicy, ra.PA, ra.forceCNFromSAN, *req.Order.RegistrationID); err != nil {
-		return nil, berrors.MalformedError(err.Error())
+		// VerifyCSR returns berror instances that can be passed through as-is
+		// without wrapping.
+		return nil, err
 	}
 
 	// Dedupe, lowercase and sort both the names from the CSR and the names in the

test/v2_integration.py

@@ -1129,7 +1129,7 @@ def test_long_san_no_cn():
         # if we get to this raise the auth_and_issue call didn't fail, so fail the test
         raise Exception("Issuance didn't fail when the only SAN in a certificate was longer than the max CN length")
     except messages.Error as e:
-        if e.typ != "urn:ietf:params:acme:error:malformed":
+        if e.typ != "urn:ietf:params:acme:error:badCSR":
             raise Exception('Expected malformed type problem, got {0}'.format(e.typ))
         if e.detail != 'Error finalizing order :: issuing precertificate: CSR doesn\'t contain a SAN short enough to fit in CN':
             raise Exception('Problem detail did not match expected')
@@ -1252,17 +1252,12 @@ def test_blocked_key_cert():
     try:
         order = client.poll_and_finalize(order)
     except acme_errors.Error as e:
-        # TODO(@cpu): this _should_ be type
-        # urn:ietf:params:acme:error:badPublicKey but this will require
-        # refactoring the `csr` package error handling.
-        # See https://github.com/letsencrypt/boulder/issues/4418
-        if e.typ != "urn:ietf:params:acme:error:malformed":
+        if e.typ != "urn:ietf:params:acme:error:badPublicKey":
             raise Exception("problem did not have correct error type, had {0}".format(e.typ))
         if e.detail != "Error finalizing order :: invalid public key in CSR: public key is forbidden":
             raise Exception("problem did not have correct error detail, had {0}".format(e.detail))
         testPass = True
 
-
     if testPass is False:
         raise Exception("expected cert creation to fail with Error when using blocked key")
 

web/probs.go

@@ -37,6 +37,8 @@ func problemDetailsForBoulderError(err *berrors.BoulderError, msg string) *probs
 		outProb = probs.OrderNotReady("%s :: %s", msg, err)
 	case berrors.BadPublicKey:
 		outProb = probs.BadPublicKey("%s :: %s", msg, err)
+	case berrors.BadCSR:
+		outProb = probs.BadCSR("%s :: %s", msg, err)
 	default:
 		// Internal server error messages may include sensitive data, so we do
 		// not include it.