Skip to content

Commit

Permalink
Unwrap SA Add/Revoke Certificate methods (letsencrypt#5598)
Browse files Browse the repository at this point in the history 
Make the gRPC wrappers for the SA's `AddCertificate`,
`AddPrecertificate`, `AddSerial`, and `RevokeCertificate`
methods simple pass-throughs.

Fixup a couple tests that were passing only because their
requests to in-memory SA objects were not passing through
the wrapper's consistency checks.

Part of letsencrypt#5532
  • Loading branch information
@aarongable
aarongable authored Aug 25, 2021
1 parent 443862b commit 2fe12cd
Show file tree
Hide file tree
Showing 15 changed files with 185 additions and 168 deletions.
18 changes: 12 additions & 6 deletions ca/ca.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ const (
)

type certificateStorage interface {
AddCertificate(context.Context, []byte, int64, []byte, *time.Time) (string, error)
AddCertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*sapb.AddCertificateResponse, error)
GetCertificate(ctx context.Context, req *sapb.Serial) (*corepb.Certificate, error)
AddPrecertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*emptypb.Empty, error)
AddSerial(ctx context.Context, req *sapb.AddSerialRequest) (*emptypb.Empty, error)
Expand Down Expand Up @@ -483,8 +483,11 @@ func (ca *certificateAuthorityImpl) storeCertificate(
certDER []byte,
issuerID int64) error {
var err error
now := ca.clk.Now()
_, err = ca.sa.AddCertificate(ctx, certDER, regID, nil, &now)
_, err = ca.sa.AddCertificate(ctx, &sapb.AddCertificateRequest{
Der: certDER,
RegID: regID,
Issued: ca.clk.Now().UnixNano(),
})
if err != nil {
ca.orphanCount.With(prometheus.Labels{"type": "cert"}).Inc()
err = berrors.InternalServerError(err.Error())
Expand Down Expand Up @@ -560,19 +563,22 @@ func (ca *certificateAuthorityImpl) integrateOrphan() error {
// we reverse the process and add ca.backdate.
issued := cert.NotBefore.Add(ca.backdate)
if orphan.Precert {
issuedNanos := issued.UnixNano()
_, err = ca.sa.AddPrecertificate(context.Background(), &sapb.AddCertificateRequest{
Der: orphan.DER,
RegID: orphan.RegID,
Ocsp: orphan.OCSPResp,
Issued: issuedNanos,
Issued: issued.UnixNano(),
IssuerID: orphan.IssuerID,
})
if err != nil && !errors.Is(err, berrors.Duplicate) {
return fmt.Errorf("failed to store orphaned precertificate: %s", err)
}
} else {
_, err = ca.sa.AddCertificate(context.Background(), orphan.DER, orphan.RegID, nil, &issued)
_, err = ca.sa.AddCertificate(context.Background(), &sapb.AddCertificateRequest{
Der: orphan.DER,
RegID: orphan.RegID,
Issued: issued.UnixNano(),
})
if err != nil && !errors.Is(err, berrors.Duplicate) {
return fmt.Errorf("failed to store orphaned certificate: %s", err)
}
Expand Down
33 changes: 16 additions & 17 deletions ca/ca_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,9 @@ type mockSA struct {
certificate core.Certificate
}

func (m *mockSA) AddCertificate(ctx context.Context, der []byte, _ int64, _ []byte, _ *time.Time) (string, error) {
m.certificate.DER = der
return "", nil
func (m *mockSA) AddCertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*sapb.AddCertificateResponse, error) {
m.certificate.DER = req.Der
return nil, nil
}

func (m *mockSA) AddPrecertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*emptypb.Empty, error) {
Expand Down Expand Up @@ -908,18 +908,18 @@ type queueSA struct {
fail bool
duplicate bool

issued *time.Time
issuedPrecert *time.Time
issued time.Time
issuedPrecert time.Time
}

func (qsa *queueSA) AddCertificate(_ context.Context, _ []byte, _ int64, _ []byte, issued *time.Time) (string, error) {
func (qsa *queueSA) AddCertificate(_ context.Context, req *sapb.AddCertificateRequest) (*sapb.AddCertificateResponse, error) {
if qsa.fail {
return "", errors.New("bad")
return nil, errors.New("bad")
} else if qsa.duplicate {
return "", berrors.DuplicateError("is a dupe")
return nil, berrors.DuplicateError("is a dupe")
}
qsa.issued = issued
return "", nil
qsa.issued = time.Unix(0, req.Issued).UTC()
return nil, nil
}

func (qsa *queueSA) AddPrecertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*emptypb.Empty, error) {
Expand All @@ -928,8 +928,7 @@ func (qsa *queueSA) AddPrecertificate(ctx context.Context, req *sapb.AddCertific
} else if qsa.duplicate {
return nil, berrors.DuplicateError("is a dupe")
}
issued := time.Unix(0, req.Issued)
qsa.issuedPrecert = &issued
qsa.issuedPrecert = time.Unix(0, req.Issued).UTC()
return nil, nil
}

Expand Down Expand Up @@ -978,7 +977,7 @@ func TestPrecertOrphanQueue(t *testing.T) {
OrderID: 1,
Csr: CNandSANCSR,
})
test.AssertError(t, err, "Expected IssuePrecertificate to fail with `failSA`")
test.AssertError(t, err, "Expected IssuePrecertificate to fail with `qsa.fail = true`")

matches := testCtx.logger.GetAllMatching(`orphaning precertificate.* regID=\[1\], orderID=\[1\]`)
if len(matches) != 1 {
Expand All @@ -993,7 +992,7 @@ func TestPrecertOrphanQueue(t *testing.T) {
err = ca.integrateOrphan()
test.AssertNotError(t, err, "integrateOrphan failed")
if !qsa.issuedPrecert.Equal(fakeNow) {
t.Errorf("expected issued time to be %s, got %s", fakeNow, *qsa.issued)
t.Errorf("expected issued time to be %s, got %s", fakeNow, qsa.issuedPrecert)
}
err = ca.integrateOrphan()
if err != goque.ErrEmpty {
Expand Down Expand Up @@ -1066,7 +1065,7 @@ func TestOrphanQueue(t *testing.T) {
err = ca.integrateOrphan()
test.AssertNotError(t, err, "integrateOrphan failed")
if !qsa.issued.Equal(fakeNow) {
t.Errorf("expected issued time to be %s, got %s", fakeNow, *qsa.issued)
t.Errorf("expected issued time to be %s, got %s", fakeNow, qsa.issued)
}
err = ca.integrateOrphan()
if err != goque.ErrEmpty {
Expand All @@ -1084,7 +1083,7 @@ func TestOrphanQueue(t *testing.T) {
err = ca.integrateOrphan()
test.AssertNotError(t, err, "integrateOrphan failed with duplicate cert")
if !qsa.issued.Equal(fakeNow) {
t.Errorf("expected issued time to be %s, got %s", fakeNow, *qsa.issued)
t.Errorf("expected issued time to be %s, got %s", fakeNow, qsa.issued)
}
err = ca.integrateOrphan()
if err != goque.ErrEmpty {
Expand Down Expand Up @@ -1114,7 +1113,7 @@ func TestOrphanQueue(t *testing.T) {
err = ca.integrateOrphan()
test.AssertNotError(t, err, "integrateOrphan failed")
if !qsa.issued.Equal(fakeNow) {
t.Errorf("expected issued time to be %s, got %s", fakeNow, *qsa.issued)
t.Errorf("expected issued time to be %s, got %s", fakeNow, qsa.issued)
}
err = ca.integrateOrphan()
if err != goque.ErrEmpty {
Expand Down
9 changes: 6 additions & 3 deletions cmd/admin-revoker/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ type mockCA struct {
}

func (ca *mockCA) GenerateOCSP(context.Context, *capb.GenerateOCSPRequest, ...grpc.CallOption) (*capb.OCSPResponse, error) {
return &capb.OCSPResponse{}, nil
return &capb.OCSPResponse{Response: []byte("fakeocspbytes")}, nil
}

type mockPurger struct{}
Expand Down Expand Up @@ -103,8 +103,11 @@ func TestRevokeBatch(t *testing.T) {
IssuerID: 1,
})
test.AssertNotError(t, err, "failed to add test cert")
now := time.Now()
_, err = ssa.AddCertificate(context.Background(), der, reg.Id, nil, &now)
_, err = ssa.AddCertificate(context.Background(), &sapb.AddCertificateRequest{
Der: der,
RegID: reg.Id,
Issued: time.Now().UnixNano(),
})
test.AssertNotError(t, err, "failed to add test cert")
_, err = serialFile.WriteString(fmt.Sprintf("%s\n", core.SerialToString(serial)))
test.AssertNotError(t, err, "failed to write serial to temp file")
Expand Down
8 changes: 6 additions & 2 deletions cmd/cert-checker/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/letsencrypt/boulder/metrics"
"github.com/letsencrypt/boulder/policy"
"github.com/letsencrypt/boulder/sa"
sapb "github.com/letsencrypt/boulder/sa/proto"
"github.com/letsencrypt/boulder/sa/satest"
"github.com/letsencrypt/boulder/test"
"github.com/letsencrypt/boulder/test/vars"
Expand Down Expand Up @@ -282,8 +283,11 @@ func TestGetAndProcessCerts(t *testing.T) {
rawCert.SerialNumber = big.NewInt(mrand.Int63())
certDER, err := x509.CreateCertificate(rand.Reader, &rawCert, &rawCert, &testKey.PublicKey, testKey)
test.AssertNotError(t, err, "Couldn't create certificate")
issued := fc.Now()
_, err = sa.AddCertificate(context.Background(), certDER, reg.Id, nil, &issued)
_, err = sa.AddCertificate(context.Background(), &sapb.AddCertificateRequest{
Der: certDER,
RegID: reg.Id,
Issued: fc.Now().Add(time.Hour).UnixNano(),
})
test.AssertNotError(t, err, "Couldn't add certificate")
}

Expand Down
15 changes: 8 additions & 7 deletions cmd/ocsp-updater/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -380,23 +380,24 @@ func TestStoreResponseGuard(t *testing.T) {
serialStr := core.SerialToString(parsedCert.SerialNumber)
reason := int64(0)
revokedDate := fc.Now().UnixNano()
err = sa.RevokeCertificate(context.Background(), &sapb.RevokeCertificateRequest{
Serial: serialStr,
Reason: reason,
Date: revokedDate,
_, err = sa.RevokeCertificate(context.Background(), &sapb.RevokeCertificateRequest{
Serial: serialStr,
Reason: reason,
Date: revokedDate,
Response: []byte("fakeocspbytes"),
})
test.AssertNotError(t, err, "Failed to revoked certificate")

// Attempt to update OCSP response where status.Status is good but stored status
// is revoked, this should fail silently
status.OCSPResponse = []byte{0, 1, 1}
status.OCSPResponse = []byte("newfakeocspbytes")
err = updater.storeResponse(&status)
test.AssertNotError(t, err, "Failed to update certificate status")

// Make sure the OCSP response hasn't actually changed
unchangedStatus, err := sa.GetCertificateStatus(ctx, core.SerialToString(parsedCert.SerialNumber))
test.AssertNotError(t, err, "Failed to get certificate status")
test.AssertEquals(t, len(unchangedStatus.OCSPResponse), 0)
test.AssertEquals(t, string(unchangedStatus.OCSPResponse), "fakeocspbytes")

// Changing the status to the stored status should allow the update to occur
status.Status = core.OCSPStatusRevoked
Expand All @@ -406,7 +407,7 @@ func TestStoreResponseGuard(t *testing.T) {
// Make sure the OCSP response has been updated
changedStatus, err := sa.GetCertificateStatus(ctx, core.SerialToString(parsedCert.SerialNumber))
test.AssertNotError(t, err, "Failed to get certificate status")
test.AssertEquals(t, len(changedStatus.OCSPResponse), 3)
test.AssertEquals(t, string(changedStatus.OCSPResponse), "newfakeocspbytes")
}

func TestGenerateOCSPResponsePrecert(t *testing.T) {
Expand Down
19 changes: 14 additions & 5 deletions cmd/orphan-finder/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ type config struct {
}

type certificateStorage interface {
AddCertificate(context.Context, []byte, int64, []byte, *time.Time) (string, error)
AddCertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*sapb.AddCertificateResponse, error)
AddPrecertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*emptypb.Empty, error)
GetCertificate(ctx context.Context, req *sapb.Serial) (*corepb.Certificate, error)
GetPrecertificate(ctx context.Context, req *sapb.Serial) (*corepb.Certificate, error)
Expand Down Expand Up @@ -328,7 +328,12 @@ func (opf *orphanFinder) storeLogLine(ctx context.Context, line string) (found b
issuedDate := cert.NotBefore.Add(opf.backdate)
switch typ {
case certOrphan:
_, err = opf.sa.AddCertificate(ctx, parsed.certDER, parsed.regID, response, &issuedDate)
_, err = opf.sa.AddCertificate(ctx, &sapb.AddCertificateRequest{
Der: parsed.certDER,
RegID: parsed.regID,
Ocsp: response,
Issued: issuedDate.UnixNano(),
})
case precertOrphan:
_, err = opf.sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
Der: parsed.certDER,
Expand Down Expand Up @@ -363,14 +368,18 @@ func (opf *orphanFinder) parseDER(derPath string, regID int64) {

switch typ {
case certOrphan:
_, err = opf.sa.AddCertificate(ctx, der, regID, response, &issuedDate)
_, err = opf.sa.AddCertificate(ctx, &sapb.AddCertificateRequest{
Der: der,
RegID: regID,
Ocsp: response,
Issued: issuedDate.UnixNano(),
})
case precertOrphan:
issued := issuedDate.UnixNano()
_, err = opf.sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
Der: der,
RegID: regID,
Ocsp: response,
Issued: issued,
Issued: issuedDate.UnixNano(),
})
default:
err = errors.New("unknown orphan type")
Expand Down
18 changes: 7 additions & 11 deletions cmd/orphan-finder/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,23 +33,19 @@ type mockSA struct {
clk clock.FakeClock
}

func (m *mockSA) AddCertificate(ctx context.Context, der []byte, regID int64, _ []byte, issued *time.Time) (string, error) {
parsed, err := x509.ParseCertificate(der)
func (m *mockSA) AddCertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*sapb.AddCertificateResponse, error) {
parsed, err := x509.ParseCertificate(req.Der)
if err != nil {
return "", err
return nil, err
}
cert := &corepb.Certificate{
Der: der,
RegistrationID: regID,
Der: req.Der,
RegistrationID: req.RegID,
Serial: core.SerialToString(parsed.SerialNumber),
}
if issued == nil {
cert.Issued = m.clk.Now().UnixNano()
} else {
cert.Issued = issued.UnixNano()
Issued: req.Issued,
}
m.certificates = append(m.certificates, cert)
return "", nil
return nil, nil
}

func (m *mockSA) GetCertificate(ctx context.Context, req *sapb.Serial) (*corepb.Certificate, error) {
Expand Down
4 changes: 2 additions & 2 deletions core/interfaces.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,15 +126,15 @@ type StorageGetter interface {
type StorageAdder interface {
NewRegistration(ctx context.Context, req *corepb.Registration) (*corepb.Registration, error)
UpdateRegistration(ctx context.Context, req *corepb.Registration) (*emptypb.Empty, error)
AddCertificate(ctx context.Context, der []byte, regID int64, ocsp []byte, issued *time.Time) (digest string, err error)
AddCertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*sapb.AddCertificateResponse, error)
AddPrecertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*emptypb.Empty, error)
AddSerial(ctx context.Context, req *sapb.AddSerialRequest) (*emptypb.Empty, error)
DeactivateRegistration(ctx context.Context, req *sapb.RegistrationID) (*emptypb.Empty, error)
NewOrder(ctx context.Context, order *corepb.Order) (*corepb.Order, error)
SetOrderProcessing(ctx context.Context, order *corepb.Order) error
FinalizeOrder(ctx context.Context, order *corepb.Order) error
SetOrderError(ctx context.Context, order *corepb.Order) error
RevokeCertificate(ctx context.Context, req *sapb.RevokeCertificateRequest) error
RevokeCertificate(ctx context.Context, req *sapb.RevokeCertificateRequest) (*emptypb.Empty, error)
// New authz2 methods
NewAuthorizations2(ctx context.Context, req *sapb.AddPendingAuthorizationsRequest) (*sapb.Authorization2IDs, error)
FinalizeAuthorization2(ctx context.Context, req *sapb.FinalizeAuthorizationRequest) error
Expand Down
Loading

0 comments on commit 2fe12cd

Please sign in to comment.