X Tutup
Skip to content

Commit 0b0917c

Browse files
jshajsha
authored
Revert "Remove StoreIssuerInfo flag in CA (letsencrypt#4850)" (letsencrypt#4868)
This reverts commit 6454513. We actually need to wait 90 days to ensure the issuerID field of the certificateStatus table is non-nil for all extant certificates.
1 parent 325bba3 commit 0b0917c

File tree

17 files changed

+200
-559
lines changed

17 files changed

+200
-559
lines changed

‎ca/ca.go‎

Lines changed: 72 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"crypto/ecdsa"
88
"crypto/rand"
99
"crypto/rsa"
10+
"crypto/sha256"
1011
"crypto/x509"
1112
"encoding/asn1"
1213
"encoding/hex"
@@ -36,8 +37,8 @@ import (
3637
corepb "github.com/letsencrypt/boulder/core/proto"
3738
csrlib "github.com/letsencrypt/boulder/csr"
3839
berrors "github.com/letsencrypt/boulder/errors"
40+
"github.com/letsencrypt/boulder/features"
3941
"github.com/letsencrypt/boulder/goodkey"
40-
"github.com/letsencrypt/boulder/issuercerts"
4142
blog "github.com/letsencrypt/boulder/log"
4243
sapb "github.com/letsencrypt/boulder/sa/proto"
4344
)
@@ -115,9 +116,11 @@ const (
115116
type CertificateAuthorityImpl struct {
116117
rsaProfile string
117118
ecdsaProfile string
119+
// A map from issuer cert common name to an internalIssuer struct
120+
issuers map[string]*internalIssuer
118121
// A map from issuer ID to internalIssuer
119-
idToIssuer map[issuercerts.ID]*internalIssuer
120-
// The issuer that will be used for issuance (as opposed to OCSP signing)
122+
idToIssuer map[int64]*internalIssuer
123+
// The common name of the default issuer cert
121124
defaultIssuer *internalIssuer
122125
sa certificateStorage
123126
pa core.PolicyAuthority
@@ -163,12 +166,11 @@ func makeInternalIssuers(
163166
issuers []Issuer,
164167
policy *cfsslConfig.Signing,
165168
lifespanOCSP time.Duration,
166-
) (map[issuercerts.ID]*internalIssuer, error) {
169+
) (map[string]*internalIssuer, error) {
167170
if len(issuers) == 0 {
168171
return nil, errors.New("No issuers specified.")
169172
}
170-
internalIssuers := make(map[issuercerts.ID]*internalIssuer)
171-
cns := make(map[string]bool)
173+
internalIssuers := make(map[string]*internalIssuer)
172174
for _, iss := range issuers {
173175
if iss.Cert == nil || iss.Signer == nil {
174176
return nil, errors.New("Issuer with nil cert or signer specified.")
@@ -179,11 +181,10 @@ func makeInternalIssuers(
179181
}
180182

181183
cn := iss.Cert.Subject.CommonName
182-
if cns[cn] {
184+
if internalIssuers[cn] != nil {
183185
return nil, errors.New("Multiple issuer certs with the same CommonName are not supported")
184186
}
185-
id := issuercerts.FromCert(iss.Cert).ID()
186-
internalIssuers[id] = &internalIssuer{
187+
internalIssuers[cn] = &internalIssuer{
187188
cert: iss.Cert,
188189
eeSigner: eeSigner,
189190
ocspSigner: iss.Signer,
@@ -192,8 +193,16 @@ func makeInternalIssuers(
192193
return internalIssuers, nil
193194
}
194195

196+
// idForIssuer generates a stable ID for an issuer certificate. This
197+
// is used for identifying which issuer issued a certificate in the
198+
// certificateStatus table.
199+
func idForIssuer(cert *x509.Certificate) int64 {
200+
h := sha256.Sum256(cert.Raw)
201+
return big.NewInt(0).SetBytes(h[:4]).Int64()
202+
}
203+
195204
// NewCertificateAuthorityImpl creates a CA instance that can sign certificates
196-
// from a single issuer (the first in the issuers slice), and can sign OCSP
205+
// from a single issuer (the first first in the issuers slice), and can sign OCSP
197206
// for any of the issuer certificates provided.
198207
func NewCertificateAuthorityImpl(
199208
config ca_config.CAConfig,
@@ -242,7 +251,7 @@ func NewCertificateAuthorityImpl(
242251
if err != nil {
243252
return nil, err
244253
}
245-
defaultIssuer := internalIssuers[issuercerts.FromCert(issuers[0].Cert).ID()]
254+
defaultIssuer := internalIssuers[issuers[0].Cert.Subject.CommonName]
246255

247256
rsaProfile := config.RSAProfile
248257
ecdsaProfile := config.ECDSAProfile
@@ -292,6 +301,7 @@ func NewCertificateAuthorityImpl(
292301
ca = &CertificateAuthorityImpl{
293302
sa: sa,
294303
pa: pa,
304+
issuers: internalIssuers,
295305
defaultIssuer: defaultIssuer,
296306
rsaProfile: rsaProfile,
297307
ecdsaProfile: ecdsaProfile,
@@ -309,9 +319,9 @@ func NewCertificateAuthorityImpl(
309319
signErrorCounter: signErrorCounter,
310320
}
311321

312-
ca.idToIssuer = make(map[issuercerts.ID]*internalIssuer)
313-
for _, ii := range internalIssuers {
314-
id := issuercerts.FromCert(ii.cert).ID()
322+
ca.idToIssuer = make(map[int64]*internalIssuer)
323+
for _, ii := range ca.issuers {
324+
id := idForIssuer(ii.cert)
315325
ca.idToIssuer[id] = ii
316326
}
317327

@@ -423,33 +433,48 @@ var ocspStatusToCode = map[string]int{
423433
func (ca *CertificateAuthorityImpl) GenerateOCSP(ctx context.Context, req *caPB.GenerateOCSPRequest) (*caPB.OCSPResponse, error) {
424434
var issuer *internalIssuer
425435
var serial *big.Int
426-
if req.IssuerID == nil {
427-
return nil, fmt.Errorf("no issuerID provided")
428-
}
429-
if req.Serial == nil {
430-
return nil, fmt.Errorf("no serial provided")
431-
}
432436
// Once the feature is enabled we need to support both RPCs that include
433437
// IssuerID and those that don't as we still need to be able to update rows
434438
// that didn't have an IssuerID set when they were created. Once this feature
435439
// has been enabled for a full OCSP lifetime cycle we can remove this
436440
// functionality.
437-
serialInt, err := core.StringToSerial(*req.Serial)
438-
if err != nil {
439-
return nil, err
440-
}
441-
serial = serialInt
442-
var ok bool
443-
issuer, ok = ca.idToIssuer[issuercerts.ID(*req.IssuerID)]
444-
if !ok {
445-
return nil, fmt.Errorf("This CA doesn't have an issuer cert with ID %d", *req.IssuerID)
446-
}
447-
exists, err := ca.sa.SerialExists(ctx, &sapb.Serial{Serial: req.Serial})
448-
if err != nil {
449-
return nil, err
450-
}
451-
if !*exists.Exists {
452-
return nil, fmt.Errorf("GenerateOCSP was asked to sign OCSP for certification with unknown serial %q", *req.Serial)
441+
if features.Enabled(features.StoreIssuerInfo) && req.IssuerID != nil {
442+
serialInt, err := core.StringToSerial(*req.Serial)
443+
if err != nil {
444+
return nil, err
445+
}
446+
serial = serialInt
447+
var ok bool
448+
issuer, ok = ca.idToIssuer[*req.IssuerID]
449+
if !ok {
450+
return nil, fmt.Errorf("This CA doesn't have an issuer cert with ID %d", *req.IssuerID)
451+
}
452+
exists, err := ca.sa.SerialExists(ctx, &sapb.Serial{Serial: req.Serial})
453+
if err != nil {
454+
return nil, err
455+
}
456+
if !*exists.Exists {
457+
return nil, fmt.Errorf("GenerateOCSP was asked to sign OCSP for certification with unknown serial %q", *req.Serial)
458+
}
459+
} else {
460+
cert, err := x509.ParseCertificate(req.CertDER)
461+
if err != nil {
462+
ca.log.AuditErr(err.Error())
463+
return nil, err
464+
}
465+
466+
serial = cert.SerialNumber
467+
cn := cert.Issuer.CommonName
468+
issuer = ca.issuers[cn]
469+
if issuer == nil {
470+
return nil, fmt.Errorf("This CA doesn't have an issuer cert with CommonName %q", cn)
471+
}
472+
err = cert.CheckSignatureFrom(issuer.cert)
473+
if err != nil {
474+
return nil, fmt.Errorf("GenerateOCSP was asked to sign OCSP for cert "+
475+
"%s from %q, but the cert's signature was not valid: %s.",
476+
core.SerialToString(cert.SerialNumber), cn, err)
477+
}
453478
}
454479

455480
now := ca.clk.Now().Truncate(time.Hour)
@@ -498,16 +523,10 @@ func (ca *CertificateAuthorityImpl) IssuePrecertificate(ctx context.Context, iss
498523
return nil, err
499524
}
500525

501-
// we currently only use one issuer, in the future when we support multiple
502-
// the issuer will need to be derived from issueReq
503-
issuerID := int64(issuercerts.FromCert(ca.defaultIssuer.cert).ID())
504-
505526
status := string(core.OCSPStatusGood)
506527
ocspResp, err := ca.GenerateOCSP(ctx, &caPB.GenerateOCSPRequest{
507-
Serial: &serialHex,
508-
CertDER: precertDER,
509-
Status: &status,
510-
IssuerID: &issuerID,
528+
CertDER: precertDER,
529+
Status: &status,
511530
})
512531
if err != nil {
513532
err = berrors.InternalServerError(err.Error())
@@ -516,13 +535,17 @@ func (ca *CertificateAuthorityImpl) IssuePrecertificate(ctx context.Context, iss
516535
}
517536

518537
req := &sapb.AddCertificateRequest{
519-
Der: precertDER,
520-
RegID: &regID,
521-
Ocsp: ocspResp.Response,
522-
Issued: &nowNanos,
523-
IssuerID: &issuerID,
538+
Der: precertDER,
539+
RegID: &regID,
540+
Ocsp: ocspResp.Response,
541+
Issued: &nowNanos,
524542
}
525543

544+
// we currently only use one issuer, in the future when we support multiple
545+
// the issuer will need to be derived from issueReq
546+
issuerID := idForIssuer(ca.defaultIssuer.cert)
547+
req.IssuerID = &issuerID
548+
526549
_, err = ca.sa.AddPrecertificate(ctx, req)
527550
if err != nil {
528551
ca.orphanCount.With(prometheus.Labels{"type": "precert"}).Inc()

‎ca/ca_test.go‎

Lines changed: 21 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ import (
3939
berrors "github.com/letsencrypt/boulder/errors"
4040
"github.com/letsencrypt/boulder/features"
4141
"github.com/letsencrypt/boulder/goodkey"
42-
"github.com/letsencrypt/boulder/issuercerts"
4342
blog "github.com/letsencrypt/boulder/log"
4443
"github.com/letsencrypt/boulder/metrics"
4544
"github.com/letsencrypt/boulder/policy"
@@ -493,16 +492,10 @@ func TestOCSP(t *testing.T) {
493492
test.AssertNotError(t, err, "Failed to issue")
494493
parsedCert, err := x509.ParseCertificate(cert.DER)
495494
test.AssertNotError(t, err, "Failed to parse cert")
496-
497-
caCertIssuerID := int64(issuercerts.FromCert(caCert).ID())
498-
serial := core.SerialToString(parsedCert.SerialNumber)
499495
status := string(core.OCSPStatusGood)
500-
501496
ocspResp, err := ca.GenerateOCSP(ctx, &caPB.GenerateOCSPRequest{
502-
IssuerID: &caCertIssuerID,
503-
Serial: &serial,
504-
CertDER: cert.DER,
505-
Status: &status,
497+
CertDER: cert.DER,
498+
Status: &status,
506499
})
507500
test.AssertNotError(t, err, "Failed to generate OCSP")
508501
parsed, err := ocsp.ParseResponse(ocspResp.Response, caCert)
@@ -511,6 +504,13 @@ func TestOCSP(t *testing.T) {
511504
test.AssertEquals(t, parsed.RevocationReason, 0)
512505
test.AssertEquals(t, parsed.SerialNumber.Cmp(parsedCert.SerialNumber), 0)
513506

507+
// Test that signatures are checked.
508+
_, err = ca.GenerateOCSP(ctx, &caPB.GenerateOCSPRequest{
509+
CertDER: append(cert.DER, byte(0)),
510+
Status: &status,
511+
})
512+
test.AssertError(t, err, "Generated OCSP for cert with bad signature")
513+
514514
// Load multiple issuers, including the old issuer, and ensure OCSP is still
515515
// signed correctly.
516516
newIssuerCert, err := core.LoadCert("../test/test-ca2.pem")
@@ -543,16 +543,14 @@ func TestOCSP(t *testing.T) {
543543
parsedNewCert, err := x509.ParseCertificate(newCert.DER)
544544
test.AssertNotError(t, err, "Failed to parse newCert")
545545

546-
newIssuerID := int64(issuercerts.FromCert(newIssuers[0].Cert).ID())
547-
newSerial := core.SerialToString(parsedNewCert.SerialNumber)
546+
err = parsedNewCert.CheckSignatureFrom(newIssuerCert)
547+
t.Logf("check sig: %s", err)
548548

549549
// ocspResp2 is a second OCSP response for `cert` (issued by caCert), and
550550
// should be signed by caCert.
551551
ocspResp2, err := ca.GenerateOCSP(ctx, &caPB.GenerateOCSPRequest{
552-
IssuerID: &newIssuerID,
553-
Serial: &newSerial,
554-
CertDER: append([]byte(nil), cert.DER...),
555-
Status: &status,
552+
CertDER: append([]byte(nil), cert.DER...),
553+
Status: &status,
556554
})
557555
test.AssertNotError(t, err, "Failed to sign second OCSP response")
558556
_, err = ocsp.ParseResponse(ocspResp2.Response, caCert)
@@ -561,10 +559,8 @@ func TestOCSP(t *testing.T) {
561559
// newCertOcspResp is an OCSP response for `newCert` (issued by newIssuer),
562560
// and should be signed by newIssuer.
563561
newCertOcspResp, err := ca.GenerateOCSP(ctx, &caPB.GenerateOCSPRequest{
564-
IssuerID: &newIssuerID,
565-
Serial: &newSerial,
566-
CertDER: newCert.DER,
567-
Status: &status,
562+
CertDER: newCert.DER,
563+
Status: &status,
568564
})
569565
test.AssertNotError(t, err, "Failed to generate OCSP")
570566
parsedNewCertOcspResp, err := ocsp.ParseResponse(newCertOcspResp.Response, newIssuerCert)
@@ -1257,6 +1253,7 @@ func TestIssuePrecertificateLinting(t *testing.T) {
12571253
func TestGenerateOCSPWithIssuerID(t *testing.T) {
12581254
testCtx := setup(t)
12591255
sa := &mockSA{}
1256+
_ = features.Set(map[string]bool{"StoreIssuerInfo": true})
12601257
ca, err := NewCertificateAuthorityImpl(
12611258
testCtx.caConfig,
12621259
sa,
@@ -1269,7 +1266,7 @@ func TestGenerateOCSPWithIssuerID(t *testing.T) {
12691266
nil)
12701267
test.AssertNotError(t, err, "Failed to create CA")
12711268

1272-
// req contains bad IssuerID
1269+
// GenerateOCSP with feature enabled + req contains bad IssuerID
12731270
issuerID := int64(666)
12741271
serial := "DEADDEADDEADDEADDEADDEADDEADDEADDEAD"
12751272
status := string(core.OCSPStatusGood)
@@ -1280,22 +1277,22 @@ func TestGenerateOCSPWithIssuerID(t *testing.T) {
12801277
})
12811278
test.AssertError(t, err, "GenerateOCSP didn't fail with invalid IssuerID")
12821279

1283-
// req contains good IssuerID
1284-
issuerID = int64(issuercerts.FromCert(ca.defaultIssuer.cert).ID())
1280+
// GenerateOCSP with feature enabled + req contains good IssuerID
1281+
issuerID = idForIssuer(ca.defaultIssuer.cert)
12851282
_, err = ca.GenerateOCSP(context.Background(), &caPB.GenerateOCSPRequest{
12861283
IssuerID: &issuerID,
12871284
Serial: &serial,
12881285
Status: &status,
12891286
})
12901287
test.AssertNotError(t, err, "GenerateOCSP failed")
12911288

1292-
// req doesn't contain IssuerID or Serial
1289+
// GenerateOCSP with feature enabled + req doesn't contain IssuerID
12931290
issueReq := caPB.IssueCertificateRequest{Csr: CNandSANCSR, RegistrationID: &arbitraryRegID}
12941291
cert, err := ca.IssuePrecertificate(ctx, &issueReq)
12951292
test.AssertNotError(t, err, "Failed to issue")
12961293
_, err = ca.GenerateOCSP(context.Background(), &caPB.GenerateOCSPRequest{
12971294
CertDER: cert.DER,
12981295
Status: &status,
12991296
})
1300-
test.AssertError(t, err, "Expected error from GenerateOCSP")
1297+
test.AssertNotError(t, err, "GenerateOCSP failed")
13011298
}

0 commit comments

Comments
 (0)
X Tutup