Skip to content

Commit 3b0e57e

Browse files
aarongableaarongable
authored
Move //revocation/reasons.go into the post-OCSP world (#8355)
Define the acceptable revocation reason codes directly in our `revocation` package. Make the int-to-string and string-to-int conversion capabilities into immutable functions, rather than ad-hoc dictionary lookups. Do the same for the user- or admin-allowed reasons. These changes make the revocation package wholly standalone, with no dependencies (except fmt). Update all of our logic and tests to use revocation.Reasons instead of constants from the /x/crypto/ocsp package. This removes all reliance on the /x/crypto/ocsp package for revocation reasons, which felt awkward in a CRLs-only world. Finally, take advantage of the improvements above to give the ceremony CRL tool the ability to take revocation reasons as strings, rather than integers. This behavior matches the capabilities already present in the admin revoke-cert tool Fixes #8328
1 parent a2d4e70 commit 3b0e57e

File tree

16 files changed

+222
-211
lines changed

16 files changed

+222
-211
lines changed

‎cmd/admin/cert.go‎

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,6 @@ import (
1414
"sync/atomic"
1515
"unicode"
1616

17-
"golang.org/x/crypto/ocsp"
18-
1917
core "github.com/letsencrypt/boulder/core"
2018
berrors "github.com/letsencrypt/boulder/errors"
2119
rapb "github.com/letsencrypt/boulder/ra/proto"
@@ -76,24 +74,18 @@ func (s *subcommandRevokeCert) Run(ctx context.Context, a *admin) error {
7674
return fmt.Errorf("got unacceptable parallelism %d", s.parallelism)
7775
}
7876

79-
reasonCode := revocation.Reason(-1)
80-
for code := range revocation.AdminAllowedReasons {
81-
if s.reasonStr == revocation.ReasonToString[code] {
82-
reasonCode = code
83-
break
84-
}
85-
}
86-
if reasonCode == revocation.Reason(-1) {
87-
return fmt.Errorf("got unacceptable revocation reason %q", s.reasonStr)
77+
reasonCode, err := revocation.StringToReason(s.reasonStr)
78+
if err != nil {
79+
return fmt.Errorf("looking up revocation reason: %w", err)
8880
}
8981

90-
if s.skipBlock && reasonCode == ocsp.KeyCompromise {
82+
if s.skipBlock && reasonCode == revocation.KeyCompromise {
9183
// We would only add the SPKI hash of the pubkey to the blockedKeys table if
9284
// the revocation reason is keyCompromise.
9385
return errors.New("-skip-block-key only makes sense with -reason=1")
9486
}
9587

96-
if s.malformed && reasonCode == ocsp.KeyCompromise {
88+
if s.malformed && reasonCode == revocation.KeyCompromise {
9789
// This is because we can't extract and block the pubkey if we can't
9890
// parse the certificate.
9991
return errors.New("cannot revoke malformed certs for reason keyCompromise")

‎cmd/bad-key-revoker/main.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99

1010
"github.com/jmhodges/clock"
1111
"github.com/prometheus/client_golang/prometheus"
12-
"golang.org/x/crypto/ocsp"
1312
"google.golang.org/grpc"
1413
"google.golang.org/protobuf/types/known/emptypb"
1514

@@ -20,6 +19,7 @@ import (
2019
bgrpc "github.com/letsencrypt/boulder/grpc"
2120
blog "github.com/letsencrypt/boulder/log"
2221
rapb "github.com/letsencrypt/boulder/ra/proto"
22+
"github.com/letsencrypt/boulder/revocation"
2323
"github.com/letsencrypt/boulder/sa"
2424
)
2525

@@ -190,7 +190,7 @@ func (bkr *badKeyRevoker) revokeCerts(certs []unrevokedCertificate) error {
190190
_, err := bkr.raClient.AdministrativelyRevokeCertificate(context.Background(), &rapb.AdministrativelyRevokeCertificateRequest{
191191
Cert: cert.DER,
192192
Serial: cert.Serial,
193-
Code: int64(ocsp.KeyCompromise),
193+
Code: int64(revocation.KeyCompromise),
194194
AdminName: "bad-key-revoker",
195195
})
196196
if err != nil {

‎cmd/ceremony/main.go‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
"github.com/letsencrypt/boulder/goodkey"
2828
"github.com/letsencrypt/boulder/linter"
2929
"github.com/letsencrypt/boulder/pkcs11helpers"
30+
"github.com/letsencrypt/boulder/revocation"
3031
"github.com/letsencrypt/boulder/strictyaml"
3132
)
3233

@@ -447,7 +448,7 @@ type crlConfig struct {
447448
RevokedCertificates []struct {
448449
CertificatePath string `yaml:"certificate-path"`
449450
RevocationDate string `yaml:"revocation-date"`
450-
RevocationReason int `yaml:"revocation-reason"`
451+
RevocationReason string `yaml:"revocation-reason"`
451452
} `yaml:"revoked-certificates"`
452453
} `yaml:"crl-profile"`
453454
SkipLints []string `yaml:"skip-lints"`
@@ -487,7 +488,7 @@ func (cc crlConfig) validate() error {
487488
if rc.RevocationDate == "" {
488489
return errors.New("crl-profile.revoked-certificates.revocation-date is required")
489490
}
490-
if rc.RevocationReason == 0 {
491+
if rc.RevocationReason == "" {
491492
return errors.New("crl-profile.revoked-certificates.revocation-reason is required")
492493
}
493494
}
@@ -994,9 +995,13 @@ func crlCeremony(configBytes []byte) error {
994995
SerialNumber: cert.SerialNumber,
995996
RevocationTime: revokedAt,
996997
}
997-
encReason, err := asn1.Marshal(rc.RevocationReason)
998+
reasonCode, err := revocation.StringToReason(rc.RevocationReason)
998999
if err != nil {
999-
return fmt.Errorf("failed to marshal revocation reason %q: %s", rc.RevocationReason, err)
1000+
return fmt.Errorf("looking up revocation reason: %w", err)
1001+
}
1002+
encReason, err := asn1.Marshal(reasonCode)
1003+
if err != nil {
1004+
return fmt.Errorf("failed to marshal revocation reason %d (%q): %s", reasonCode, rc.RevocationReason, err)
10001005
}
10011006
revokedCert.Extensions = []pkix.Extension{{
10021007
Id: asn1.ObjectIdentifier{2, 5, 29, 21}, // id-ce-reasonCode

‎cmd/ceremony/main_test.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1187,7 +1187,7 @@ func TestCRLConfig(t *testing.T) {
11871187
RevokedCertificates []struct {
11881188
CertificatePath string `yaml:"certificate-path"`
11891189
RevocationDate string `yaml:"revocation-date"`
1190-
RevocationReason int `yaml:"revocation-reason"`
1190+
RevocationReason string `yaml:"revocation-reason"`
11911191
} `yaml:"revoked-certificates"`
11921192
}{
11931193
ThisUpdate: "this-update",
@@ -1219,7 +1219,7 @@ func TestCRLConfig(t *testing.T) {
12191219
RevokedCertificates []struct {
12201220
CertificatePath string `yaml:"certificate-path"`
12211221
RevocationDate string `yaml:"revocation-date"`
1222-
RevocationReason int `yaml:"revocation-reason"`
1222+
RevocationReason string `yaml:"revocation-reason"`
12231223
} `yaml:"revoked-certificates"`
12241224
}{
12251225
ThisUpdate: "this-update",
@@ -1252,7 +1252,7 @@ func TestCRLConfig(t *testing.T) {
12521252
RevokedCertificates []struct {
12531253
CertificatePath string `yaml:"certificate-path"`
12541254
RevocationDate string `yaml:"revocation-date"`
1255-
RevocationReason int `yaml:"revocation-reason"`
1255+
RevocationReason string `yaml:"revocation-reason"`
12561256
} `yaml:"revoked-certificates"`
12571257
}{
12581258
ThisUpdate: "this-update",
@@ -1261,7 +1261,7 @@ func TestCRLConfig(t *testing.T) {
12611261
RevokedCertificates: []struct {
12621262
CertificatePath string `yaml:"certificate-path"`
12631263
RevocationDate string `yaml:"revocation-date"`
1264-
RevocationReason int `yaml:"revocation-reason"`
1264+
RevocationReason string `yaml:"revocation-reason"`
12651265
}{{}},
12661266
},
12671267
},
@@ -1291,7 +1291,7 @@ func TestCRLConfig(t *testing.T) {
12911291
RevokedCertificates []struct {
12921292
CertificatePath string `yaml:"certificate-path"`
12931293
RevocationDate string `yaml:"revocation-date"`
1294-
RevocationReason int `yaml:"revocation-reason"`
1294+
RevocationReason string `yaml:"revocation-reason"`
12951295
} `yaml:"revoked-certificates"`
12961296
}{
12971297
ThisUpdate: "this-update",
@@ -1300,7 +1300,7 @@ func TestCRLConfig(t *testing.T) {
13001300
RevokedCertificates: []struct {
13011301
CertificatePath string `yaml:"certificate-path"`
13021302
RevocationDate string `yaml:"revocation-date"`
1303-
RevocationReason int `yaml:"revocation-reason"`
1303+
RevocationReason string `yaml:"revocation-reason"`
13041304
}{{
13051305
CertificatePath: "path",
13061306
}},
@@ -1332,7 +1332,7 @@ func TestCRLConfig(t *testing.T) {
13321332
RevokedCertificates []struct {
13331333
CertificatePath string `yaml:"certificate-path"`
13341334
RevocationDate string `yaml:"revocation-date"`
1335-
RevocationReason int `yaml:"revocation-reason"`
1335+
RevocationReason string `yaml:"revocation-reason"`
13361336
} `yaml:"revoked-certificates"`
13371337
}{
13381338
ThisUpdate: "this-update",
@@ -1341,7 +1341,7 @@ func TestCRLConfig(t *testing.T) {
13411341
RevokedCertificates: []struct {
13421342
CertificatePath string `yaml:"certificate-path"`
13431343
RevocationDate string `yaml:"revocation-date"`
1344-
RevocationReason int `yaml:"revocation-reason"`
1344+
RevocationReason string `yaml:"revocation-reason"`
13451345
}{{
13461346
CertificatePath: "path",
13471347
RevocationDate: "date",
@@ -1374,7 +1374,7 @@ func TestCRLConfig(t *testing.T) {
13741374
RevokedCertificates []struct {
13751375
CertificatePath string `yaml:"certificate-path"`
13761376
RevocationDate string `yaml:"revocation-date"`
1377-
RevocationReason int `yaml:"revocation-reason"`
1377+
RevocationReason string `yaml:"revocation-reason"`
13781378
} `yaml:"revoked-certificates"`
13791379
}{
13801380
ThisUpdate: "this-update",
@@ -1383,11 +1383,11 @@ func TestCRLConfig(t *testing.T) {
13831383
RevokedCertificates: []struct {
13841384
CertificatePath string `yaml:"certificate-path"`
13851385
RevocationDate string `yaml:"revocation-date"`
1386-
RevocationReason int `yaml:"revocation-reason"`
1386+
RevocationReason string `yaml:"revocation-reason"`
13871387
}{{
13881388
CertificatePath: "path",
13891389
RevocationDate: "date",
1390-
RevocationReason: 1,
1390+
RevocationReason: "keyCompromise",
13911391
}},
13921392
},
13931393
},

‎crl/updater/updater.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import (
1212

1313
"github.com/jmhodges/clock"
1414
"github.com/prometheus/client_golang/prometheus"
15-
"golang.org/x/crypto/ocsp"
1615
"google.golang.org/protobuf/types/known/emptypb"
1716
"google.golang.org/protobuf/types/known/timestamppb"
1817

@@ -23,6 +22,7 @@ import (
2322
cspb "github.com/letsencrypt/boulder/crl/storer/proto"
2423
"github.com/letsencrypt/boulder/issuance"
2524
blog "github.com/letsencrypt/boulder/log"
25+
"github.com/letsencrypt/boulder/revocation"
2626
sapb "github.com/letsencrypt/boulder/sa/proto"
2727
)
2828

@@ -207,7 +207,7 @@ func reRevoked(a *proto.CRLEntry, b *proto.CRLEntry) (*proto.CRLEntry, error) {
207207
if b.RevokedAt.AsTime().Before(a.RevokedAt.AsTime()) {
208208
first, second = b, a
209209
}
210-
if first.Reason != ocsp.KeyCompromise && second.Reason == ocsp.KeyCompromise {
210+
if revocation.Reason(first.Reason) != revocation.KeyCompromise && revocation.Reason(second.Reason) == revocation.KeyCompromise {
211211
return second, nil
212212
}
213213
// The RA has logic to prevent re-revocation for any reason other than KeyCompromise,

‎crl/updater/updater_test.go‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
"testing"
1212
"time"
1313

14-
"golang.org/x/crypto/ocsp"
1514
"google.golang.org/grpc"
1615
"google.golang.org/protobuf/types/known/emptypb"
1716
"google.golang.org/protobuf/types/known/timestamppb"
@@ -25,6 +24,7 @@ import (
2524
"github.com/letsencrypt/boulder/issuance"
2625
blog "github.com/letsencrypt/boulder/log"
2726
"github.com/letsencrypt/boulder/metrics"
27+
"github.com/letsencrypt/boulder/revocation"
2828
sapb "github.com/letsencrypt/boulder/sa/proto"
2929
"github.com/letsencrypt/boulder/test"
3030
)
@@ -278,7 +278,7 @@ func TestUpdateShard(t *testing.T) {
278278
entries: []*corepb.CRLEntry{
279279
{
280280
Serial: "0311b5d430823cfa25b0fc85d14c54ee35",
281-
Reason: int32(ocsp.KeyCompromise),
281+
Reason: int32(revocation.KeyCompromise),
282282
RevokedAt: now,
283283
},
284284
},
@@ -287,17 +287,17 @@ func TestUpdateShard(t *testing.T) {
287287
entries: []*corepb.CRLEntry{
288288
{
289289
Serial: "0311b5d430823cfa25b0fc85d14c54ee35",
290-
Reason: int32(ocsp.KeyCompromise),
290+
Reason: int32(revocation.KeyCompromise),
291291
RevokedAt: now,
292292
},
293293
{
294294
Serial: "037d6a05a0f6a975380456ae605cee9889",
295-
Reason: int32(ocsp.AffiliationChanged),
295+
Reason: int32(revocation.AffiliationChanged),
296296
RevokedAt: now,
297297
},
298298
{
299299
Serial: "03aa617ab8ee58896ba082bfa25199c884",
300-
Reason: int32(ocsp.Unspecified),
300+
Reason: int32(revocation.Unspecified),
301301
RevokedAt: now,
302302
},
303303
},
@@ -310,9 +310,9 @@ func TestUpdateShard(t *testing.T) {
310310
test.AssertNotError(t, err, "updateShard")
311311

312312
expectedEntries := map[string]int32{
313-
"0311b5d430823cfa25b0fc85d14c54ee35": int32(ocsp.KeyCompromise),
314-
"037d6a05a0f6a975380456ae605cee9889": int32(ocsp.AffiliationChanged),
315-
"03aa617ab8ee58896ba082bfa25199c884": int32(ocsp.Unspecified),
313+
"0311b5d430823cfa25b0fc85d14c54ee35": int32(revocation.KeyCompromise),
314+
"037d6a05a0f6a975380456ae605cee9889": int32(revocation.AffiliationChanged),
315+
"03aa617ab8ee58896ba082bfa25199c884": int32(revocation.Unspecified),
316316
}
317317
for r := range bytes.SplitSeq(recordingUploader.crlBody, []byte("\n")) {
318318
if len(r) == 0 {
@@ -580,25 +580,25 @@ func TestAddFromStream(t *testing.T) {
580580
yesterday := now.Add(-24 * time.Hour)
581581
simpleEntry := &corepb.CRLEntry{
582582
Serial: "abcdefg",
583-
Reason: ocsp.CessationOfOperation,
583+
Reason: int32(revocation.CessationOfOperation),
584584
RevokedAt: timestamppb.New(yesterday),
585585
}
586586

587587
reRevokedEntry := &corepb.CRLEntry{
588588
Serial: "abcdefg",
589-
Reason: ocsp.KeyCompromise,
589+
Reason: int32(revocation.KeyCompromise),
590590
RevokedAt: timestamppb.New(now),
591591
}
592592

593593
reRevokedEntryOld := &corepb.CRLEntry{
594594
Serial: "abcdefg",
595-
Reason: ocsp.KeyCompromise,
595+
Reason: int32(revocation.KeyCompromise),
596596
RevokedAt: timestamppb.New(now.Add(-48 * time.Hour)),
597597
}
598598

599599
reRevokedEntryBadReason := &corepb.CRLEntry{
600600
Serial: "abcdefg",
601-
Reason: ocsp.AffiliationChanged,
601+
Reason: int32(revocation.AffiliationChanged),
602602
RevokedAt: timestamppb.New(now),
603603
}
604604

@@ -691,12 +691,12 @@ func TestAddFromStreamDisallowedSerialPrefix(t *testing.T) {
691691
input := []*corepb.CRLEntry{
692692
{
693693
Serial: "abcdefg",
694-
Reason: ocsp.CessationOfOperation,
694+
Reason: int32(revocation.CessationOfOperation),
695695
RevokedAt: timestamppb.New(yesterday),
696696
},
697697
{
698698
Serial: "01020304",
699-
Reason: ocsp.CessationOfOperation,
699+
Reason: int32(revocation.CessationOfOperation),
700700
RevokedAt: timestamppb.New(yesterday),
701701
},
702702
}

0 commit comments

Comments
 (0)