diff --git a/ca/ca.go b/ca/ca.go index 22593e80b61..6f8d48f82c0 100644 --- a/ca/ca.go +++ b/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) diff --git a/ca/ca_test.go b/ca/ca_test.go index a63fd373e57..666d7706aee 100644 --- a/ca/ca_test.go +++ b/ca/ca_test.go @@ -588,6 +588,7 @@ 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. // @@ -595,7 +596,7 @@ func TestInvalidCSRs(t *testing.T) { // * 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. // @@ -603,7 +604,7 @@ func TestInvalidCSRs(t *testing.T) { // * 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. // @@ -611,23 +612,23 @@ func TestInvalidCSRs(t *testing.T) { // * 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) diff --git a/csr/csr.go b/csr/csr.go index 3bb97b1124b..ce61a0b80e5 100644 --- a/csr/csr.go +++ b/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 { diff --git a/csr/csr_test.go b/csr/csr_test.go index 77e894a7407..41104b94a13 100644 --- a/csr/csr_test.go +++ b/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,7 +121,7 @@ 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, @@ -128,7 +129,7 @@ func TestVerifyCSR(t *testing.T) { testingPolicy, &mockPA{}, 0, - errors.New("CSR contains more than 1 DNS names"), + berrors.BadCSRError("CSR contains more than 1 DNS names"), }, { signedReqWithBadNames, diff --git a/errors/errors.go b/errors/errors.go index 33908ac8bdf..652088dba00 100644 --- a/errors/errors.go +++ b/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...) +} diff --git a/probs/probs.go b/probs/probs.go index 94fb414e0e4..f9307aec550 100644 --- a/probs/probs.go +++ b/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, + } +} diff --git a/ra/ra.go b/ra/ra.go index 57f8d2c8f17..b0d97e77bd1 100644 --- a/ra/ra.go +++ b/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 diff --git a/test/v2_integration.py b/test/v2_integration.py index 93b618a085a..414ca0fd931 100644 --- a/test/v2_integration.py +++ b/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") diff --git a/web/probs.go b/web/probs.go index 70329fc4b53..d3374b5a8a0 100644 --- a/web/probs.go +++ b/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.