Skip to content

Commit

Permalink
Return proto from ca.IssueCertificateFromPrecertificate (#4982)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@aarongable
aarongable committed Jul 24, 2020
1 parent 62eae60 commit ffdae2d
Show file tree
Hide file tree
Showing 7 changed files with 55 additions and 53 deletions.
42 changes: 27 additions & 15 deletions ca/ca.go
View file
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions ca/ca_test.go
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion core/interfaces.go
View file
Expand Up @@ -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)
}
Expand Down
14 changes: 3 additions & 11 deletions grpc/ca-wrappers.go
View file
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
30 changes: 12 additions & 18 deletions mocks/ca.go
View file
Expand Up @@ -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"
)

Expand All @@ -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 {
Expand All @@ -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
Expand Down
10 changes: 7 additions & 3 deletions ra/ra.go
View file
Expand Up @@ -1225,15 +1225,15 @@ 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.
return emptyCert, berrors.InternalServerError("failed to parse certificate: %s", err.Error())
}

// 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 {
Expand All @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions ra/ra_test.go
View file
Expand Up @@ -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
Expand Down

0 comments on commit ffdae2d

Please sign in to comment.