Skip to content

Commit

Permalink
Add CRL capabilities to issuance package (#7300)
Browse files Browse the repository at this point in the history 
Move the CRL issuance logic -- building an x509.RevocationList template,
populating it with correctly-built extensions, linting it, and actually
signing it -- out of the //ca package and into the //issuance package.
This means that the CA's CRL code no longer needs to be able to reach
inside the issuance package to access its issuers and certificates (and
those fields will be able to be made private after the same is done for
OCSP issuance).

Additionally, improve the configuration of CRL issuance, create
additional checks on CRL's ThisUpdate and NextUpdate fields, and make it
possible for a CRL to contain two IssuingDistributionPoint URIs so that
we can migrate to shorter addresses.

IN-10045 tracks the corresponding production changes.

Fixes #7159
Part of #7296
Part of #7294
Part of #7094
Part of #7100
  • Loading branch information
@aarongable
aarongable authored Feb 13, 2024
1 parent 3865b46 commit ad699af
Show file tree
Hide file tree
Showing 13 changed files with 474 additions and 171 deletions.
14 changes: 8 additions & 6 deletions ca/ca_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ func setup(t *testing.T) *testCtx {
UseForECDSALeaves: true,
IssuerURL: "http://not-example.com/issuer-url",
OCSPURL: "http://not-example.com/ocsp",
CRLURL: "http://not-example.com/crl",
CRLURLBase: "http://not-example.com/crl/",
Location: issuance.IssuerLoc{File: ecdsaIntKey, CertFile: ecdsaIntCert},
}, fc)
test.AssertNotError(t, err, "Couldn't load test issuer")
Expand All @@ -195,7 +195,7 @@ func setup(t *testing.T) *testCtx {
UseForECDSALeaves: true,
IssuerURL: "http://not-example.com/issuer-url",
OCSPURL: "http://not-example.com/ocsp",
CRLURL: "http://not-example.com/crl",
CRLURLBase: "http://not-example.com/crl/",
Location: issuance.IssuerLoc{File: rsaIntKey, CertFile: rsaIntCert},
}, fc)
test.AssertNotError(t, err, "Couldn't load test issuer")
Expand Down Expand Up @@ -234,8 +234,10 @@ func setup(t *testing.T) *testCtx {

crl, err := NewCRLImpl(
boulderIssuers,
boulderProfile.Lints,
time.Hour,
issuance.CRLProfileConfig{
ValidityInterval: config.Duration{Duration: 216 * time.Hour},
MaxBackdate: config.Duration{Duration: time.Hour},
},
"http://c.boulder.test",
100,
blog.NewMock(),
Expand Down Expand Up @@ -693,8 +695,8 @@ func issueCertificateSubTestUnknownExtension(t *testing.T, i *TestCertificateIss
test.AssertMetricWithLabelsEquals(t, i.ca.signatureCount, prometheus.Labels{"purpose": "precertificate"}, 1)

// NOTE: The hard-coded value here will have to change over time as Boulder
// adds new (unrequested) extensions to certificates.
expectedExtensionCount := 10
// adds or removes (unrequested/default) extensions in certificates.
expectedExtensionCount := 9
test.AssertEquals(t, len(i.cert.Extensions), expectedExtensionCount)
}

Expand Down
129 changes: 31 additions & 98 deletions ca/crl.go
View file
Original file line number Diff line number Diff line change
@@ -1,18 +1,12 @@
package ca

import (
"crypto/rand"
"crypto/sha256"
"crypto/x509"
"crypto/x509/pkix"
"encoding/asn1"
"errors"
"fmt"
"io"
"strings"
"time"

"github.com/zmap/zlint/v3/lint"

capb "github.com/letsencrypt/boulder/ca/proto"
"github.com/letsencrypt/boulder/core"
Expand All @@ -25,9 +19,8 @@ import (
type crlImpl struct {
capb.UnimplementedCRLGeneratorServer
issuers map[issuance.NameID]*issuance.Issuer
// TODO(#7159): Move this into the CRL profile
lints lint.Registry
lifetime time.Duration
profile *issuance.CRLProfile
// TODO(#7094): Remove this once all CRLs have IDPs built from issuer.crlURLBase.
idpBase string
maxLogLen int
log blog.Logger
Expand All @@ -38,21 +31,24 @@ type crlImpl struct {
// issue CRLs from. lifetime sets the validity period (inclusive) of the
// resulting CRLs. idpBase is the base URL from which IssuingDistributionPoint
// URIs will constructed; it must use the http:// scheme.
func NewCRLImpl(issuers []*issuance.Issuer, lints lint.Registry, lifetime time.Duration, idpBase string, maxLogLen int, logger blog.Logger) (*crlImpl, error) {
func NewCRLImpl(
issuers []*issuance.Issuer,
profileConfig issuance.CRLProfileConfig,
idpBase string,
maxLogLen int,
logger blog.Logger) (*crlImpl, error) {
issuersByNameID := make(map[issuance.NameID]*issuance.Issuer, len(issuers))
for _, issuer := range issuers {
issuersByNameID[issuer.NameID()] = issuer
}

if lifetime == 0 {
logger.Warningf("got zero for crl lifetime; setting to default 9 days")
lifetime = 9 * 24 * time.Hour
} else if lifetime >= 10*24*time.Hour {
return nil, fmt.Errorf("crl lifetime cannot be more than 10 days, got %q", lifetime)
} else if lifetime <= 0*time.Hour {
return nil, fmt.Errorf("crl lifetime must be positive, got %q", lifetime)
profile, err := issuance.NewCRLProfile(profileConfig)
if err != nil {
return nil, fmt.Errorf("loading CRL profile: %w", err)
}

// TODO(#7094): Remove this once all CRLs have IDPs built from their
// issuer.crlURLBase instead.
if !strings.HasPrefix(idpBase, "http://") {
return nil, fmt.Errorf("issuingDistributionPoint base URI must use http:// scheme, got %q", idpBase)
}
Expand All @@ -62,8 +58,7 @@ func NewCRLImpl(issuers []*issuance.Issuer, lints lint.Registry, lifetime time.D

return &crlImpl{
issuers: issuersByNameID,
lints: lints,
lifetime: lifetime,
profile: profile,
idpBase: idpBase,
maxLogLen: maxLogLen,
log: logger,
Expand All @@ -72,8 +67,7 @@ func NewCRLImpl(issuers []*issuance.Issuer, lints lint.Registry, lifetime time.D

func (ci *crlImpl) GenerateCRL(stream capb.CRLGenerator_GenerateCRLServer) error {
var issuer *issuance.Issuer
var template *x509.RevocationList
var shard int64
var req *issuance.CRLRequest
rcs := make([]x509.RevocationListEntry, 0)

for {
Expand All @@ -87,11 +81,11 @@ func (ci *crlImpl) GenerateCRL(stream capb.CRLGenerator_GenerateCRLServer) error

switch payload := in.Payload.(type) {
case *capb.GenerateCRLRequest_Metadata:
if template != nil {
if req != nil {
return errors.New("got more than one metadata message")
}

template, err = ci.metadataToTemplate(payload.Metadata)
req, err = ci.metadataToRequest(payload.Metadata)
if err != nil {
return err
}
Expand All @@ -102,8 +96,6 @@ func (ci *crlImpl) GenerateCRL(stream capb.CRLGenerator_GenerateCRLServer) error
return fmt.Errorf("got unrecognized IssuerNameID: %d", payload.Metadata.IssuerNameID)
}

shard = payload.Metadata.ShardIdx

case *capb.GenerateCRLRequest_Entry:
rc, err := ci.entryToRevokedCertificate(payload.Entry)
if err != nil {
Expand All @@ -117,23 +109,23 @@ func (ci *crlImpl) GenerateCRL(stream capb.CRLGenerator_GenerateCRLServer) error
}
}

if template == nil {
if req == nil {
return errors.New("no crl metadata received")
}

// Add the Issuing Distribution Point extension.
idp, err := makeIDPExt(ci.idpBase, issuer.NameID(), shard)
if err != nil {
return fmt.Errorf("creating IDP extension: %w", err)
// TODO(#7296): Remove this fallback once all configs have issuer.CRLBaseURL
// set and our CRL URLs disclosed in CCADB have been updated.
if ci.idpBase != "" {
req.DeprecatedIDPBaseURL = ci.idpBase
}
template.ExtraExtensions = append(template.ExtraExtensions, *idp)

// Compute a unique ID for this issuer-number-shard combo, to tie together all
// the audit log lines related to its issuance.
logID := blog.LogLineChecksum(fmt.Sprintf("%d", issuer.NameID()) + template.Number.String() + fmt.Sprintf("%d", shard))
logID := blog.LogLineChecksum(fmt.Sprintf("%d", issuer.NameID()) + req.Number.String() + fmt.Sprintf("%d", req.Shard))
ci.log.AuditInfof(
"Signing CRL: logID=[%s] issuer=[%s] number=[%s] shard=[%d] thisUpdate=[%s] nextUpdate=[%s] numEntries=[%d]",
logID, issuer.Cert.Subject.CommonName, template.Number.String(), shard, template.ThisUpdate, template.NextUpdate, len(rcs),
"Signing CRL: logID=[%s] issuer=[%s] number=[%s] shard=[%d] thisUpdate=[%s] numEntries=[%d]",
logID, issuer.Cert.Subject.CommonName, req.Number.String(), req.Shard, req.ThisUpdate, len(rcs),
)

if len(rcs) > 0 {
Expand All @@ -155,19 +147,9 @@ func (ci *crlImpl) GenerateCRL(stream capb.CRLGenerator_GenerateCRLServer) error
ci.log.AuditInfo(builder.String())
}

template.RevokedCertificateEntries = rcs
req.Entries = rcs

err = issuer.Linter.CheckCRL(template, ci.lints)
if err != nil {
return err
}

crlBytes, err := x509.CreateRevocationList(
rand.Reader,
template,
issuer.Cert.Certificate,
issuer.Signer,
)
crlBytes, err := issuer.IssueCRL(ci.profile, req)
if err != nil {
return fmt.Errorf("signing crl: %w", err)
}
Expand Down Expand Up @@ -197,18 +179,17 @@ func (ci *crlImpl) GenerateCRL(stream capb.CRLGenerator_GenerateCRLServer) error
return nil
}

func (ci *crlImpl) metadataToTemplate(meta *capb.CRLMetadata) (*x509.RevocationList, error) {
// TODO(#7153): Check each value via core.IsAnyNilOrZero
if meta.IssuerNameID == 0 || core.IsAnyNilOrZero(meta.ThisUpdate) {
func (ci *crlImpl) metadataToRequest(meta *capb.CRLMetadata) (*issuance.CRLRequest, error) {
if core.IsAnyNilOrZero(meta.IssuerNameID, meta.ThisUpdate, meta.ShardIdx) {
return nil, errors.New("got incomplete metadata message")
}
thisUpdate := meta.ThisUpdate.AsTime()
number := bcrl.Number(thisUpdate)

return &x509.RevocationList{
return &issuance.CRLRequest{
Number: number,
Shard: meta.ShardIdx,
ThisUpdate: thisUpdate,
NextUpdate: thisUpdate.Add(-time.Second).Add(ci.lifetime),
}, nil
}

Expand All @@ -229,51 +210,3 @@ func (ci *crlImpl) entryToRevokedCertificate(entry *corepb.CRLEntry) (*x509.Revo
ReasonCode: int(entry.Reason),
}, nil
}

// distributionPointName represents the ASN.1 DistributionPointName CHOICE as
// defined in RFC 5280 Section 4.2.1.13. We only use one of the fields, so the
// others are omitted.
type distributionPointName struct {
// Technically, FullName is of type GeneralNames, which is of type SEQUENCE OF
// GeneralName. But GeneralName itself is of type CHOICE, and the ans1.Marhsal
// function doesn't support marshalling structs to CHOICEs, so we have to use
// asn1.RawValue and encode the GeneralName ourselves.
FullName []asn1.RawValue `asn1:"optional,tag:0"`
}

// issuingDistributionPoint represents the ASN.1 IssuingDistributionPoint
// SEQUENCE as defined in RFC 5280 Section 5.2.5. We only use two of the fields,
// so the others are omitted.
type issuingDistributionPoint struct {
DistributionPoint distributionPointName `asn1:"optional,tag:0"`
OnlyContainsUserCerts bool `asn1:"optional,tag:1"`
}

// makeIDPExt returns a critical IssuingDistributionPoint extension containing a
// URI built from the base url, the issuer's NameID, and the shard number. It
// also sets the OnlyContainsUserCerts boolean to true.
func makeIDPExt(base string, issuer issuance.NameID, shardIdx int64) (*pkix.Extension, error) {
val := issuingDistributionPoint{
DistributionPoint: distributionPointName{
[]asn1.RawValue{ // GeneralNames
{ // GeneralName
Class: 2, // context-specific
Tag: 6, // uniformResourceIdentifier, IA5String
Bytes: []byte(fmt.Sprintf("%s/%d/%d.crl", base, issuer, shardIdx)),
},
},
},
OnlyContainsUserCerts: true,
}

valBytes, err := asn1.Marshal(val)
if err != nil {
return nil, err
}

return &pkix.Extension{
Id: asn1.ObjectIdentifier{2, 5, 29, 28}, // id-ce-issuingDistributionPoint
Value: valBytes,
Critical: true,
}, nil
}
15 changes: 8 additions & 7 deletions ca/crl_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,10 @@ package ca

import (
"crypto/x509"
"fmt"
"io"
"testing"
"time"

"github.com/jmhodges/clock"
"google.golang.org/grpc"
"google.golang.org/protobuf/types/known/timestamppb"

Expand Down Expand Up @@ -73,13 +72,13 @@ func TestGenerateCRL(t *testing.T) {
go func() {
errs <- crli.GenerateCRL(mockGenerateCRLBidiStream{input: ins, output: nil})
}()
// TODO(#7100) Change usage of time.Now() to fakeclock.Now()
now := time.Now()
now := testCtx.fc.Now()
ins <- &capb.GenerateCRLRequest{
Payload: &capb.GenerateCRLRequest_Metadata{
Metadata: &capb.CRLMetadata{
IssuerNameID: 1,
ThisUpdate: timestamppb.New(now),
ShardIdx: 1,
},
},
}
Expand All @@ -98,6 +97,7 @@ func TestGenerateCRL(t *testing.T) {
Metadata: &capb.CRLMetadata{
IssuerNameID: int64(testCtx.boulderIssuers[0].NameID()),
ThisUpdate: timestamppb.New(now),
ShardIdx: 1,
},
},
}
Expand All @@ -106,11 +106,13 @@ func TestGenerateCRL(t *testing.T) {
Metadata: &capb.CRLMetadata{
IssuerNameID: int64(testCtx.boulderIssuers[0].NameID()),
ThisUpdate: timestamppb.New(now),
ShardIdx: 1,
},
},
}
close(ins)
err = <-errs
fmt.Println("done waiting for error")
test.AssertError(t, err, "can't generate CRL with duplicate metadata")
test.AssertContains(t, err.Error(), "got more than one metadata message")

Expand Down Expand Up @@ -168,14 +170,12 @@ func TestGenerateCRL(t *testing.T) {
}
close(done)
}()
fc := clock.NewFake()
fc.Set(time.Date(2020, time.October, 10, 23, 17, 0, 0, time.UTC))
now = fc.Now()
ins <- &capb.GenerateCRLRequest{
Payload: &capb.GenerateCRLRequest_Metadata{
Metadata: &capb.CRLMetadata{
IssuerNameID: int64(testCtx.boulderIssuers[0].NameID()),
ThisUpdate: timestamppb.New(now),
ShardIdx: 1,
},
},
}
Expand Down Expand Up @@ -212,6 +212,7 @@ func TestGenerateCRL(t *testing.T) {
Metadata: &capb.CRLMetadata{
IssuerNameID: int64(testCtx.boulderIssuers[0].NameID()),
ThisUpdate: timestamppb.New(now),
ShardIdx: 1,
},
},
}
Expand Down
Loading

0 comments on commit ad699af

Please sign in to comment.