From ffdae2d338f0a48b41eb5e915e606cbe914e766a Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Thu, 23 Jul 2020 18:39:10 -0700 Subject: [PATCH] Return proto from ca.IssueCertificateFromPrecertificate (#4982) This is the only method on the ca which uses a non-proto type as its request or response value. Changing this to use a proto removes the last logic from the wrappers, allowing them to be removed in a future CL. It also makes the interface more uniform and easier to reason about. Issue: #4940 --- ca/ca.go | 42 +++++++++++++++++++++++++++--------------- ca/ca_test.go | 6 +++--- core/interfaces.go | 2 +- grpc/ca-wrappers.go | 14 +++----------- mocks/ca.go | 30 ++++++++++++------------------ ra/ra.go | 10 +++++++--- ra/ra_test.go | 4 ++-- 7 files changed, 55 insertions(+), 53 deletions(-) diff --git a/ca/ca.go b/ca/ca.go index 69b92910754..8d5868b46d4 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -595,52 +595,65 @@ func (ca *CertificateAuthorityImpl) IssuePrecertificate(ctx context.Context, iss // final certificate, but this is just a belt-and-suspenders measure, since // there could be race conditions where two goroutines are issuing for the same // serial number at the same time. -func (ca *CertificateAuthorityImpl) IssueCertificateForPrecertificate(ctx context.Context, req *capb.IssueCertificateForPrecertificateRequest) (core.Certificate, error) { - emptyCert := core.Certificate{} - +func (ca *CertificateAuthorityImpl) IssueCertificateForPrecertificate(ctx context.Context, req *capb.IssueCertificateForPrecertificateRequest) (*corepb.Certificate, error) { // issueReq.orderID may be zero, for ACMEv1 requests. if core.IsAnyNilOrZero(req, req.DER, req.SCTs, req.RegistrationID) { - return emptyCert, berrors.InternalServerError("Incomplete cert for precertificate request") + return nil, berrors.InternalServerError("Incomplete cert for precertificate request") } precert, err := x509.ParseCertificate(req.DER) if err != nil { - return emptyCert, err + return nil, err } serialHex := core.SerialToString(precert.SerialNumber) if _, err = ca.sa.GetCertificate(ctx, serialHex); err == nil { err = berrors.InternalServerError("issuance of duplicate final certificate requested: %s", serialHex) ca.log.AuditErr(err.Error()) - return emptyCert, err + return nil, err } else if !berrors.Is(err, berrors.NotFound) { - return emptyCert, fmt.Errorf("error checking for duplicate issuance of %s: %s", serialHex, err) + return nil, fmt.Errorf("error checking for duplicate issuance of %s: %s", serialHex, err) } var scts []ct.SignedCertificateTimestamp for _, sctBytes := range req.SCTs { var sct ct.SignedCertificateTimestamp _, err = cttls.Unmarshal(sctBytes, &sct) if err != nil { - return emptyCert, err + return nil, err } scts = append(scts, sct) } certPEM, err := ca.defaultIssuer.eeSigner.SignFromPrecert(precert, scts) if err != nil { - return emptyCert, err + return nil, err } ca.signatureCount.WithLabelValues(string(certType)).Inc() block, _ := pem.Decode(certPEM) if block == nil || block.Type != "CERTIFICATE" { err = berrors.InternalServerError("invalid certificate value returned") ca.log.AuditErrf("PEM decode error, aborting: serial=[%s] pem=[%s] err=[%v]", serialHex, certPEM, err) - return emptyCert, err + return nil, err } certDER := block.Bytes ca.log.AuditInfof("Signing success: serial=[%s] names=[%s] certificate=[%s]", serialHex, strings.Join(precert.DNSNames, ", "), hex.EncodeToString(req.DER), hex.EncodeToString(certDER)) - return ca.storeCertificate(ctx, *req.RegistrationID, *req.OrderID, precert.SerialNumber, certDER) + err = ca.storeCertificate(ctx, *req.RegistrationID, *req.OrderID, precert.SerialNumber, certDER) + if err != nil { + return nil, err + } + serialString := core.SerialToString(precert.SerialNumber) + digest := core.Fingerprint256(certDER) + issued := precert.NotBefore.UnixNano() + expires := precert.NotAfter.UnixNano() + return &corepb.Certificate{ + RegistrationID: req.RegistrationID, + Serial: &serialString, + Der: certDER, + Digest: &digest, + Issued: &issued, + Expires: &expires, + }, nil } type validity struct { @@ -794,7 +807,7 @@ func (ca *CertificateAuthorityImpl) storeCertificate( regID int64, orderID int64, serialBigInt *big.Int, - certDER []byte) (core.Certificate, error) { + certDER []byte) error { var err error now := ca.clk.Now() _, err = ca.sa.AddCertificate(ctx, certDER, regID, nil, &now) @@ -811,10 +824,9 @@ func (ca *CertificateAuthorityImpl) storeCertificate( RegID: regID, }) } - return core.Certificate{}, err + return err } - - return core.Certificate{DER: certDER}, nil + return nil } type orphanedCert struct { diff --git a/ca/ca_test.go b/ca/ca_test.go index 942cf710aba..c24982fff46 100644 --- a/ca/ca_test.go +++ b/ca/ca_test.go @@ -870,7 +870,7 @@ func TestIssueCertificateForPrecertificate(t *testing.T) { OrderID: new(int64), }) test.AssertNotError(t, err, "Failed to issue cert from precert") - parsedCert, err := x509.ParseCertificate(cert.DER) + parsedCert, err := x509.ParseCertificate(cert.Der) test.AssertNotError(t, err, "Failed to parse cert") // Check for SCT list extension @@ -1111,7 +1111,7 @@ 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.storeCertificate( + err = ca.storeCertificate( context.Background(), 1, 1, @@ -1152,7 +1152,7 @@ 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.storeCertificate( + err = ca.storeCertificate( context.Background(), 1, 1, diff --git a/core/interfaces.go b/core/interfaces.go index 70ab209aee2..9bd576e29cc 100644 --- a/core/interfaces.go +++ b/core/interfaces.go @@ -97,7 +97,7 @@ type CertificateAuthority interface { IssuePrecertificate(ctx context.Context, issueReq *capb.IssueCertificateRequest) (*capb.IssuePrecertificateResponse, error) // [RegistrationAuthority] - IssueCertificateForPrecertificate(ctx context.Context, req *capb.IssueCertificateForPrecertificateRequest) (Certificate, error) + IssueCertificateForPrecertificate(ctx context.Context, req *capb.IssueCertificateForPrecertificateRequest) (*corepb.Certificate, error) GenerateOCSP(ctx context.Context, ocspReq *capb.GenerateOCSPRequest) (*capb.OCSPResponse, error) } diff --git a/grpc/ca-wrappers.go b/grpc/ca-wrappers.go index 1b543def187..2a950f4e3a6 100644 --- a/grpc/ca-wrappers.go +++ b/grpc/ca-wrappers.go @@ -27,12 +27,8 @@ func (cac CertificateAuthorityClientWrapper) IssuePrecertificate(ctx context.Con return cac.inner.IssuePrecertificate(ctx, issueReq) } -func (cac CertificateAuthorityClientWrapper) IssueCertificateForPrecertificate(ctx context.Context, req *capb.IssueCertificateForPrecertificateRequest) (core.Certificate, error) { - res, err := cac.inner.IssueCertificateForPrecertificate(ctx, req) - if err != nil { - return core.Certificate{}, err - } - return PBToCert(res) +func (cac CertificateAuthorityClientWrapper) IssueCertificateForPrecertificate(ctx context.Context, req *capb.IssueCertificateForPrecertificateRequest) (*corepb.Certificate, error) { + return cac.inner.IssueCertificateForPrecertificate(ctx, req) } func (cac CertificateAuthorityClientWrapper) GenerateOCSP(ctx context.Context, req *capb.GenerateOCSPRequest) (*capb.OCSPResponse, error) { @@ -65,11 +61,7 @@ func (cas *CertificateAuthorityServerWrapper) IssuePrecertificate(ctx context.Co } func (cas *CertificateAuthorityServerWrapper) IssueCertificateForPrecertificate(ctx context.Context, req *capb.IssueCertificateForPrecertificateRequest) (*corepb.Certificate, error) { - cert, err := cas.inner.IssueCertificateForPrecertificate(ctx, req) - if err != nil { - return nil, err - } - return CertToPB(cert), nil + return cas.inner.IssueCertificateForPrecertificate(ctx, req) } func (cas *CertificateAuthorityServerWrapper) GenerateOCSP(ctx context.Context, req *capb.GenerateOCSPRequest) (*capb.OCSPResponse, error) { diff --git a/mocks/ca.go b/mocks/ca.go index 063165d7d83..f31431dd145 100644 --- a/mocks/ca.go +++ b/mocks/ca.go @@ -7,7 +7,7 @@ import ( "fmt" capb "github.com/letsencrypt/boulder/ca/proto" - "github.com/letsencrypt/boulder/core" + corepb "github.com/letsencrypt/boulder/core/proto" "github.com/letsencrypt/boulder/revocation" ) @@ -17,21 +17,6 @@ type MockCA struct { PEM []byte } -// IssueCertificate is a mock -func (ca *MockCA) IssueCertificate(ctx context.Context, _ *capb.IssueCertificateRequest) (core.Certificate, error) { - if ca.PEM == nil { - return core.Certificate{}, fmt.Errorf("MockCA's PEM field must be set before calling IssueCertificate") - } - block, _ := pem.Decode(ca.PEM) - cert, err := x509.ParseCertificate(block.Bytes) - if err != nil { - return core.Certificate{}, err - } - return core.Certificate{ - DER: cert.Raw, - }, nil -} - // IssuePrecertificate is a mock func (ca *MockCA) IssuePrecertificate(ctx context.Context, _ *capb.IssueCertificateRequest) (*capb.IssuePrecertificateResponse, error) { if ca.PEM == nil { @@ -48,8 +33,17 @@ func (ca *MockCA) IssuePrecertificate(ctx context.Context, _ *capb.IssueCertific } // IssueCertificateForPrecertificate is a mock -func (ca *MockCA) IssueCertificateForPrecertificate(ctx context.Context, req *capb.IssueCertificateForPrecertificateRequest) (core.Certificate, error) { - return core.Certificate{DER: req.DER}, nil +func (ca *MockCA) IssueCertificateForPrecertificate(ctx context.Context, req *capb.IssueCertificateForPrecertificateRequest) (*corepb.Certificate, error) { + var mockInt = int64(1) + var mockString = "mock" + return &corepb.Certificate{ + Der: req.DER, + RegistrationID: &mockInt, + Serial: &mockString, + Digest: &mockString, + Issued: &mockInt, + Expires: &mockInt, + }, nil } // GenerateOCSP is a mock diff --git a/ra/ra.go b/ra/ra.go index 450578c7955..e766951da15 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -1225,7 +1225,7 @@ func (ra *RegistrationAuthorityImpl) issueCertificateInner( return emptyCert, wrapError(err, "issuing certificate for precertificate") } - parsedCertificate, err := x509.ParseCertificate([]byte(cert.DER)) + parsedCertificate, err := x509.ParseCertificate([]byte(cert.Der)) if err != nil { // berrors.InternalServerError because the certificate from the CA should be // parseable. @@ -1233,7 +1233,7 @@ func (ra *RegistrationAuthorityImpl) issueCertificateInner( } // Asynchronously submit the final certificate to any configured logs - go ra.ctpolicy.SubmitFinalCert(cert.DER, parsedCertificate.NotAfter) + go ra.ctpolicy.SubmitFinalCert(cert.Der, parsedCertificate.NotAfter) err = ra.MatchesCSR(parsedCertificate, csr) if err != nil { @@ -1246,7 +1246,11 @@ func (ra *RegistrationAuthorityImpl) issueCertificateInner( logEvent.NotAfter = parsedCertificate.NotAfter ra.newCertCounter.Inc() - return cert, nil + res, err := bgrpc.PBToCert(cert) + if err != nil { + return emptyCert, nil + } + return res, nil } func (ra *RegistrationAuthorityImpl) getSCTs(ctx context.Context, cert []byte, expiration time.Time) (core.SCTDERs, error) { diff --git a/ra/ra_test.go b/ra/ra_test.go index dc0eb98a142..1e0284a8fd9 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -3524,8 +3524,8 @@ func (ca *mockCAFailCertForPrecert) IssuePrecertificate(_ context.Context, _ *ca func (ca *mockCAFailCertForPrecert) IssueCertificateForPrecertificate( _ context.Context, - _ *capb.IssueCertificateForPrecertificateRequest) (core.Certificate, error) { - return core.Certificate{}, ca.err + _ *capb.IssueCertificateForPrecertificateRequest) (*corepb.Certificate, error) { + return &corepb.Certificate{}, ca.err } // TestIssueCertificateInnerErrs tests that errors from the CA caught during