From ecca3492e990b3f6b20831d94d372fb7e6f80581 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 9 Oct 2019 20:11:11 -0400 Subject: [PATCH 1/2] csr: return berrors in VerifyCSR. (#4473) This also adds the badCSR error type specified by RFC 8555. It is a natural fit for the errors in VerifyCSR that aren't covered by badPublicKey. The web package function for converting a berror to a problem is updated for the new badCSR error type. The callers (RA and CA) are updated to return the berrors from VerifyCSR as is instead of unconditionally wrapping them as a berrors.MalformedError instance. Unit/integration tests are updated accordingly. Resolves #4418 --- ca/ca.go | 4 +++- ca/ca_test.go | 15 ++++++++------- csr/csr.go | 23 +++++++++++------------ csr/csr_test.go | 5 +++-- errors/errors.go | 5 +++++ probs/probs.go | 10 ++++++++++ ra/ra.go | 4 +++- test/v2_integration.py | 9 ++------- web/probs.go | 2 ++ 9 files changed, 47 insertions(+), 30 deletions(-) diff --git a/ca/ca.go b/ca/ca.go index ffa16d3fecb..396b7f38ec1 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -595,7 +595,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 7df637dc489..39aa098a9fa 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. From fead807c7cb3f6b3ad1fb910a66847d4c2dbe315 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 9 Oct 2019 17:11:58 -0700 Subject: [PATCH 2/2] Make PrecertificateOCSP the default behavior. (#4465) In the process, rename generateOCSPAndStoreCertificate to just storeCertificate, because that function doesn't generate OCSP anymore; instead the OCSP is generated (and stored) at precertificate issuance time. --- ca/ca.go | 122 +++++++----------- ca/ca_test.go | 8 +- ...20190902000000_AddPrecertificatesTable.sql | 0 3 files changed, 52 insertions(+), 78 deletions(-) rename sa/{_db-next => _db}/migrations/20190902000000_AddPrecertificatesTable.sql (100%) diff --git a/ca/ca.go b/ca/ca.go index 396b7f38ec1..8b1a31cbf72 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -36,7 +36,6 @@ import ( corepb "github.com/letsencrypt/boulder/core/proto" csrlib "github.com/letsencrypt/boulder/csr" berrors "github.com/letsencrypt/boulder/errors" - "github.com/letsencrypt/boulder/features" "github.com/letsencrypt/boulder/goodkey" blog "github.com/letsencrypt/boulder/log" "github.com/letsencrypt/boulder/metrics" @@ -450,68 +449,57 @@ func (ca *CertificateAuthorityImpl) IssuePrecertificate(ctx context.Context, iss regID := *issueReq.RegistrationID - if features.Enabled(features.PrecertificateOCSP) { - serialHex := core.SerialToString(serialBigInt) - nowNanos := ca.clk.Now().UnixNano() - expiresNanos := validity.NotAfter.UnixNano() - _, err = ca.sa.AddSerial(ctx, &sapb.AddSerialRequest{ - Serial: &serialHex, - RegID: ®ID, - Created: &nowNanos, - Expires: &expiresNanos, - }) - if err != nil { - return nil, err - } - - precertDER, err := ca.issuePrecertificateInner(ctx, issueReq, serialBigInt, validity, precertType) - if err != nil { - return nil, err - } - - ocspResp, err := ca.GenerateOCSP(ctx, core.OCSPSigningRequest{ - CertDER: precertDER, - Status: string(core.OCSPStatusGood), - }) - if err != nil { - err = berrors.InternalServerError(err.Error()) - ca.log.AuditInfof("OCSP Signing failure: serial=[%s] err=[%s]", serialHex, err) - } + serialHex := core.SerialToString(serialBigInt) + nowNanos := ca.clk.Now().UnixNano() + expiresNanos := validity.NotAfter.UnixNano() + _, err = ca.sa.AddSerial(ctx, &sapb.AddSerialRequest{ + Serial: &serialHex, + RegID: ®ID, + Created: &nowNanos, + Expires: &expiresNanos, + }) + if err != nil { + return nil, err + } - _, err = ca.sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: precertDER, - RegID: ®ID, - Ocsp: ocspResp, - Issued: &nowNanos, - }) - if err != nil { - err = berrors.InternalServerError(err.Error()) - ca.log.AuditErrf("Failed RPC to store at SA, orphaning precertificate: serial=[%s] cert=[%s] err=[%v], regID=[%d], orderID=[%d]", - serialHex, hex.EncodeToString(precertDER), err, *issueReq.RegistrationID, *issueReq.OrderID) - if ca.orphanQueue != nil { - ca.queueOrphan(&orphanedCert{ - DER: precertDER, - RegID: regID, - OCSPResp: ocspResp, - Precert: true, - }) - } - return nil, err - } + precertDER, err := ca.issuePrecertificateInner(ctx, issueReq, serialBigInt, validity, precertType) + if err != nil { + return nil, err + } - return &caPB.IssuePrecertificateResponse{ - DER: precertDER, - }, nil + ocspResp, err := ca.GenerateOCSP(ctx, core.OCSPSigningRequest{ + CertDER: precertDER, + Status: string(core.OCSPStatusGood), + }) + if err != nil { + err = berrors.InternalServerError(err.Error()) + ca.log.AuditInfof("OCSP Signing failure: serial=[%s] err=[%s]", serialHex, err) + } - } else { - precertDER, err := ca.issuePrecertificateInner(ctx, issueReq, serialBigInt, validity, precertType) - if err != nil { - return nil, err + _, err = ca.sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ + Der: precertDER, + RegID: ®ID, + Ocsp: ocspResp, + Issued: &nowNanos, + }) + if err != nil { + err = berrors.InternalServerError(err.Error()) + ca.log.AuditErrf("Failed RPC to store at SA, orphaning precertificate: serial=[%s] cert=[%s] err=[%v], regID=[%d], orderID=[%d]", + serialHex, hex.EncodeToString(precertDER), err, *issueReq.RegistrationID, *issueReq.OrderID) + if ca.orphanQueue != nil { + ca.queueOrphan(&orphanedCert{ + DER: precertDER, + RegID: regID, + OCSPResp: ocspResp, + Precert: true, + }) } - return &caPB.IssuePrecertificateResponse{ - DER: precertDER, - }, nil + return nil, err } + + return &caPB.IssuePrecertificateResponse{ + DER: precertDER, + }, nil } // IssueCertificateForPrecertificate takes a precertificate and a set of SCTs for that precertificate @@ -549,7 +537,7 @@ func (ca *CertificateAuthorityImpl) IssueCertificateForPrecertificate(ctx contex ca.log.AuditInfof("Signing success: serial=[%s] names=[%s] precertificate=[%s] certificate=[%s]", serialHex, strings.Join(precert.DNSNames, ", "), hex.EncodeToString(req.DER), hex.EncodeToString(certDER)) - return ca.generateOCSPAndStoreCertificate(ctx, *req.RegistrationID, *req.OrderID, precert.SerialNumber, certDER) + return ca.storeCertificate(ctx, *req.RegistrationID, *req.OrderID, precert.SerialNumber, certDER) } type validity struct { @@ -700,7 +688,7 @@ func (ca *CertificateAuthorityImpl) issuePrecertificateInner(ctx context.Context return certDER, nil } -func (ca *CertificateAuthorityImpl) generateOCSPAndStoreCertificate( +func (ca *CertificateAuthorityImpl) storeCertificate( ctx context.Context, regID int64, orderID int64, @@ -708,20 +696,6 @@ func (ca *CertificateAuthorityImpl) generateOCSPAndStoreCertificate( certDER []byte) (core.Certificate, error) { var err error var ocspResp []byte - if !features.Enabled(features.PrecertificateOCSP) { - ocspResp, err = ca.GenerateOCSP(ctx, core.OCSPSigningRequest{ - CertDER: certDER, - Status: string(core.OCSPStatusGood), - }) - if err != nil { - err = berrors.InternalServerError(err.Error()) - ca.log.AuditInfof("OCSP Signing failure: serial=[%s] err=[%s]", core.SerialToString(serialBigInt), err) - // Ignore errors here to avoid orphaning the certificate. The - // ocsp-updater will look for certs with a zero ocspLastUpdated - // and generate the initial response in this case. - } - } - now := ca.clk.Now() _, err = ca.sa.AddCertificate(ctx, certDER, regID, ocspResp, &now) if err != nil { diff --git a/ca/ca_test.go b/ca/ca_test.go index 39aa098a9fa..666d7706aee 100644 --- a/ca/ca_test.go +++ b/ca/ca_test.go @@ -1033,14 +1033,14 @@ func TestOrphanQueue(t *testing.T) { } certDER, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, k.Public(), k) test.AssertNotError(t, err, "Failed to generate test cert") - _, err = ca.generateOCSPAndStoreCertificate( + _, err = ca.storeCertificate( context.Background(), 1, 1, tmpl.SerialNumber, certDER, ) - test.AssertError(t, err, "generateOCSPAndStoreCertificate didn't fail when AddCertificate failed") + test.AssertError(t, err, "storeCertificate didn't fail when AddCertificate failed") qsa.fail = false err = ca.integrateOrphan() @@ -1074,14 +1074,14 @@ func TestOrphanQueue(t *testing.T) { // add cert to queue, and recreate queue to make sure it still has the cert qsa.fail = true qsa.duplicate = false - _, err = ca.generateOCSPAndStoreCertificate( + _, err = ca.storeCertificate( context.Background(), 1, 1, tmpl.SerialNumber, certDER, ) - test.AssertError(t, err, "generateOCSPAndStoreCertificate didn't fail when AddCertificate failed") + test.AssertError(t, err, "storeCertificate didn't fail when AddCertificate failed") err = orphanQueue.Close() test.AssertNotError(t, err, "Failed to close the queue cleanly") orphanQueue, err = goque.OpenQueue(tmpDir) diff --git a/sa/_db-next/migrations/20190902000000_AddPrecertificatesTable.sql b/sa/_db/migrations/20190902000000_AddPrecertificatesTable.sql similarity index 100% rename from sa/_db-next/migrations/20190902000000_AddPrecertificatesTable.sql rename to sa/_db/migrations/20190902000000_AddPrecertificatesTable.sql