Skip to content

Commit 2fe12cd

Browse files
aarongableaarongable
authored
Unwrap SA Add/Revoke Certificate methods (letsencrypt#5598)
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
1 parent 443862b commit 2fe12cd

File tree

15 files changed

+185
-168
lines changed

15 files changed

+185
-168
lines changed

‎ca/ca.go‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ const (
4242
)
4343

4444
type certificateStorage interface {
45-
AddCertificate(context.Context, []byte, int64, []byte, *time.Time) (string, error)
45+
AddCertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*sapb.AddCertificateResponse, error)
4646
GetCertificate(ctx context.Context, req *sapb.Serial) (*corepb.Certificate, error)
4747
AddPrecertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*emptypb.Empty, error)
4848
AddSerial(ctx context.Context, req *sapb.AddSerialRequest) (*emptypb.Empty, error)
@@ -483,8 +483,11 @@ func (ca *certificateAuthorityImpl) storeCertificate(
483483
certDER []byte,
484484
issuerID int64) error {
485485
var err error
486-
now := ca.clk.Now()
487-
_, err = ca.sa.AddCertificate(ctx, certDER, regID, nil, &now)
486+
_, err = ca.sa.AddCertificate(ctx, &sapb.AddCertificateRequest{
487+
Der: certDER,
488+
RegID: regID,
489+
Issued: ca.clk.Now().UnixNano(),
490+
})
488491
if err != nil {
489492
ca.orphanCount.With(prometheus.Labels{"type": "cert"}).Inc()
490493
err = berrors.InternalServerError(err.Error())
@@ -560,19 +563,22 @@ func (ca *certificateAuthorityImpl) integrateOrphan() error {
560563
// we reverse the process and add ca.backdate.
561564
issued := cert.NotBefore.Add(ca.backdate)
562565
if orphan.Precert {
563-
issuedNanos := issued.UnixNano()
564566
_, err = ca.sa.AddPrecertificate(context.Background(), &sapb.AddCertificateRequest{
565567
Der: orphan.DER,
566568
RegID: orphan.RegID,
567569
Ocsp: orphan.OCSPResp,
568-
Issued: issuedNanos,
570+
Issued: issued.UnixNano(),
569571
IssuerID: orphan.IssuerID,
570572
})
571573
if err != nil && !errors.Is(err, berrors.Duplicate) {
572574
return fmt.Errorf("failed to store orphaned precertificate: %s", err)
573575
}
574576
} else {
575-
_, err = ca.sa.AddCertificate(context.Background(), orphan.DER, orphan.RegID, nil, &issued)
577+
_, err = ca.sa.AddCertificate(context.Background(), &sapb.AddCertificateRequest{
578+
Der: orphan.DER,
579+
RegID: orphan.RegID,
580+
Issued: issued.UnixNano(),
581+
})
576582
if err != nil && !errors.Is(err, berrors.Duplicate) {
577583
return fmt.Errorf("failed to store orphaned certificate: %s", err)
578584
}

‎ca/ca_test.go‎

Lines changed: 16 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -150,9 +150,9 @@ type mockSA struct {
150150
certificate core.Certificate
151151
}
152152

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

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

911-
issued *time.Time
912-
issuedPrecert *time.Time
911+
issued time.Time
912+
issuedPrecert time.Time
913913
}
914914

915-
func (qsa *queueSA) AddCertificate(_ context.Context, _ []byte, _ int64, _ []byte, issued *time.Time) (string, error) {
915+
func (qsa *queueSA) AddCertificate(_ context.Context, req *sapb.AddCertificateRequest) (*sapb.AddCertificateResponse, error) {
916916
if qsa.fail {
917-
return "", errors.New("bad")
917+
return nil, errors.New("bad")
918918
} else if qsa.duplicate {
919-
return "", berrors.DuplicateError("is a dupe")
919+
return nil, berrors.DuplicateError("is a dupe")
920920
}
921-
qsa.issued = issued
922-
return "", nil
921+
qsa.issued = time.Unix(0, req.Issued).UTC()
922+
return nil, nil
923923
}
924924

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

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

983982
matches := testCtx.logger.GetAllMatching(`orphaning precertificate.* regID=\[1\], orderID=\[1\]`)
984983
if len(matches) != 1 {
@@ -993,7 +992,7 @@ func TestPrecertOrphanQueue(t *testing.T) {
993992
err = ca.integrateOrphan()
994993
test.AssertNotError(t, err, "integrateOrphan failed")
995994
if !qsa.issuedPrecert.Equal(fakeNow) {
996-
t.Errorf("expected issued time to be %s, got %s", fakeNow, *qsa.issued)
995+
t.Errorf("expected issued time to be %s, got %s", fakeNow, qsa.issuedPrecert)
997996
}
998997
err = ca.integrateOrphan()
999998
if err != goque.ErrEmpty {
@@ -1066,7 +1065,7 @@ func TestOrphanQueue(t *testing.T) {
10661065
err = ca.integrateOrphan()
10671066
test.AssertNotError(t, err, "integrateOrphan failed")
10681067
if !qsa.issued.Equal(fakeNow) {
1069-
t.Errorf("expected issued time to be %s, got %s", fakeNow, *qsa.issued)
1068+
t.Errorf("expected issued time to be %s, got %s", fakeNow, qsa.issued)
10701069
}
10711070
err = ca.integrateOrphan()
10721071
if err != goque.ErrEmpty {
@@ -1084,7 +1083,7 @@ func TestOrphanQueue(t *testing.T) {
10841083
err = ca.integrateOrphan()
10851084
test.AssertNotError(t, err, "integrateOrphan failed with duplicate cert")
10861085
if !qsa.issued.Equal(fakeNow) {
1087-
t.Errorf("expected issued time to be %s, got %s", fakeNow, *qsa.issued)
1086+
t.Errorf("expected issued time to be %s, got %s", fakeNow, qsa.issued)
10881087
}
10891088
err = ca.integrateOrphan()
10901089
if err != goque.ErrEmpty {
@@ -1114,7 +1113,7 @@ func TestOrphanQueue(t *testing.T) {
11141113
err = ca.integrateOrphan()
11151114
test.AssertNotError(t, err, "integrateOrphan failed")
11161115
if !qsa.issued.Equal(fakeNow) {
1117-
t.Errorf("expected issued time to be %s, got %s", fakeNow, *qsa.issued)
1116+
t.Errorf("expected issued time to be %s, got %s", fakeNow, qsa.issued)
11181117
}
11191118
err = ca.integrateOrphan()
11201119
if err != goque.ErrEmpty {

‎cmd/admin-revoker/main_test.go‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ type mockCA struct {
3535
}
3636

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

4141
type mockPurger struct{}
@@ -103,8 +103,11 @@ func TestRevokeBatch(t *testing.T) {
103103
IssuerID: 1,
104104
})
105105
test.AssertNotError(t, err, "failed to add test cert")
106-
now := time.Now()
107-
_, err = ssa.AddCertificate(context.Background(), der, reg.Id, nil, &now)
106+
_, err = ssa.AddCertificate(context.Background(), &sapb.AddCertificateRequest{
107+
Der: der,
108+
RegID: reg.Id,
109+
Issued: time.Now().UnixNano(),
110+
})
108111
test.AssertNotError(t, err, "failed to add test cert")
109112
_, err = serialFile.WriteString(fmt.Sprintf("%s\n", core.SerialToString(serial)))
110113
test.AssertNotError(t, err, "failed to write serial to temp file")

‎cmd/cert-checker/main_test.go‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"github.com/letsencrypt/boulder/metrics"
2525
"github.com/letsencrypt/boulder/policy"
2626
"github.com/letsencrypt/boulder/sa"
27+
sapb "github.com/letsencrypt/boulder/sa/proto"
2728
"github.com/letsencrypt/boulder/sa/satest"
2829
"github.com/letsencrypt/boulder/test"
2930
"github.com/letsencrypt/boulder/test/vars"
@@ -282,8 +283,11 @@ func TestGetAndProcessCerts(t *testing.T) {
282283
rawCert.SerialNumber = big.NewInt(mrand.Int63())
283284
certDER, err := x509.CreateCertificate(rand.Reader, &rawCert, &rawCert, &testKey.PublicKey, testKey)
284285
test.AssertNotError(t, err, "Couldn't create certificate")
285-
issued := fc.Now()
286-
_, err = sa.AddCertificate(context.Background(), certDER, reg.Id, nil, &issued)
286+
_, err = sa.AddCertificate(context.Background(), &sapb.AddCertificateRequest{
287+
Der: certDER,
288+
RegID: reg.Id,
289+
Issued: fc.Now().Add(time.Hour).UnixNano(),
290+
})
287291
test.AssertNotError(t, err, "Couldn't add certificate")
288292
}
289293

‎cmd/ocsp-updater/main_test.go‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -380,23 +380,24 @@ func TestStoreResponseGuard(t *testing.T) {
380380
serialStr := core.SerialToString(parsedCert.SerialNumber)
381381
reason := int64(0)
382382
revokedDate := fc.Now().UnixNano()
383-
err = sa.RevokeCertificate(context.Background(), &sapb.RevokeCertificateRequest{
384-
Serial: serialStr,
385-
Reason: reason,
386-
Date: revokedDate,
383+
_, err = sa.RevokeCertificate(context.Background(), &sapb.RevokeCertificateRequest{
384+
Serial: serialStr,
385+
Reason: reason,
386+
Date: revokedDate,
387+
Response: []byte("fakeocspbytes"),
387388
})
388389
test.AssertNotError(t, err, "Failed to revoked certificate")
389390

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

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

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

412413
func TestGenerateOCSPResponsePrecert(t *testing.T) {

‎cmd/orphan-finder/main.go‎

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ type config struct {
6161
}
6262

6363
type certificateStorage interface {
64-
AddCertificate(context.Context, []byte, int64, []byte, *time.Time) (string, error)
64+
AddCertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*sapb.AddCertificateResponse, error)
6565
AddPrecertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*emptypb.Empty, error)
6666
GetCertificate(ctx context.Context, req *sapb.Serial) (*corepb.Certificate, error)
6767
GetPrecertificate(ctx context.Context, req *sapb.Serial) (*corepb.Certificate, error)
@@ -328,7 +328,12 @@ func (opf *orphanFinder) storeLogLine(ctx context.Context, line string) (found b
328328
issuedDate := cert.NotBefore.Add(opf.backdate)
329329
switch typ {
330330
case certOrphan:
331-
_, err = opf.sa.AddCertificate(ctx, parsed.certDER, parsed.regID, response, &issuedDate)
331+
_, err = opf.sa.AddCertificate(ctx, &sapb.AddCertificateRequest{
332+
Der: parsed.certDER,
333+
RegID: parsed.regID,
334+
Ocsp: response,
335+
Issued: issuedDate.UnixNano(),
336+
})
332337
case precertOrphan:
333338
_, err = opf.sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
334339
Der: parsed.certDER,
@@ -363,14 +368,18 @@ func (opf *orphanFinder) parseDER(derPath string, regID int64) {
363368

364369
switch typ {
365370
case certOrphan:
366-
_, err = opf.sa.AddCertificate(ctx, der, regID, response, &issuedDate)
371+
_, err = opf.sa.AddCertificate(ctx, &sapb.AddCertificateRequest{
372+
Der: der,
373+
RegID: regID,
374+
Ocsp: response,
375+
Issued: issuedDate.UnixNano(),
376+
})
367377
case precertOrphan:
368-
issued := issuedDate.UnixNano()
369378
_, err = opf.sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
370379
Der: der,
371380
RegID: regID,
372381
Ocsp: response,
373-
Issued: issued,
382+
Issued: issuedDate.UnixNano(),
374383
})
375384
default:
376385
err = errors.New("unknown orphan type")

‎cmd/orphan-finder/main_test.go‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -33,23 +33,19 @@ type mockSA struct {
3333
clk clock.FakeClock
3434
}
3535

36-
func (m *mockSA) AddCertificate(ctx context.Context, der []byte, regID int64, _ []byte, issued *time.Time) (string, error) {
37-
parsed, err := x509.ParseCertificate(der)
36+
func (m *mockSA) AddCertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*sapb.AddCertificateResponse, error) {
37+
parsed, err := x509.ParseCertificate(req.Der)
3838
if err != nil {
39-
return "", err
39+
return nil, err
4040
}
4141
cert := &corepb.Certificate{
42-
Der: der,
43-
RegistrationID: regID,
42+
Der: req.Der,
43+
RegistrationID: req.RegID,
4444
Serial: core.SerialToString(parsed.SerialNumber),
45-
}
46-
if issued == nil {
47-
cert.Issued = m.clk.Now().UnixNano()
48-
} else {
49-
cert.Issued = issued.UnixNano()
45+
Issued: req.Issued,
5046
}
5147
m.certificates = append(m.certificates, cert)
52-
return "", nil
48+
return nil, nil
5349
}
5450

5551
func (m *mockSA) GetCertificate(ctx context.Context, req *sapb.Serial) (*corepb.Certificate, error) {

‎core/interfaces.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,15 +126,15 @@ type StorageGetter interface {
126126
type StorageAdder interface {
127127
NewRegistration(ctx context.Context, req *corepb.Registration) (*corepb.Registration, error)
128128
UpdateRegistration(ctx context.Context, req *corepb.Registration) (*emptypb.Empty, error)
129-
AddCertificate(ctx context.Context, der []byte, regID int64, ocsp []byte, issued *time.Time) (digest string, err error)
129+
AddCertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*sapb.AddCertificateResponse, error)
130130
AddPrecertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*emptypb.Empty, error)
131131
AddSerial(ctx context.Context, req *sapb.AddSerialRequest) (*emptypb.Empty, error)
132132
DeactivateRegistration(ctx context.Context, req *sapb.RegistrationID) (*emptypb.Empty, error)
133133
NewOrder(ctx context.Context, order *corepb.Order) (*corepb.Order, error)
134134
SetOrderProcessing(ctx context.Context, order *corepb.Order) error
135135
FinalizeOrder(ctx context.Context, order *corepb.Order) error
136136
SetOrderError(ctx context.Context, order *corepb.Order) error
137-
RevokeCertificate(ctx context.Context, req *sapb.RevokeCertificateRequest) error
137+
RevokeCertificate(ctx context.Context, req *sapb.RevokeCertificateRequest) (*emptypb.Empty, error)
138138
// New authz2 methods
139139
NewAuthorizations2(ctx context.Context, req *sapb.AddPendingAuthorizationsRequest) (*sapb.Authorization2IDs, error)
140140
FinalizeAuthorization2(ctx context.Context, req *sapb.FinalizeAuthorizationRequest) error

0 commit comments

Comments
 (0)