Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add CRL capabilities to issuance package #7300

Merged
merged 9 commits into from
Feb 13, 2024
Merged

Add CRL capabilities to issuance package #7300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5dc9bb2
Separate issuance.Profile out from issuance.Issuer
aarongable Jan 26, 2024
09f6c91
Fix CI
aarongable Jan 30, 2024
0c6fcfc
Fix Prepare comment
aarongable Jan 30, 2024
c4a0a97
Improve CRL IssuingDistributionPoint lint
aarongable Feb 2, 2024
955dff9
Add CRL capabilities to issuance package
aarongable Feb 2, 2024
78ffdc4
Review comments
aarongable Feb 7, 2024
3c6acad
Merge branch main into issuance-crls
aarongable Feb 8, 2024
89861bd
Merge branch 'main' into issuance-crls
aarongable Feb 9, 2024
f688015
Improve CRLURLBase validation
aarongable Feb 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions ca/ca_test.go
View file
Open in desktop
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},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was @beautifulentropy who provided a nice distinction:

  • period: a span of time, unanchored from any particular instance (i.e. a duration)
  • interval: a span of time anchored to a specific beginning and end time

So ValidityPeriod would be nice here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, except that "Validity Period" is defined by RFC 5280 as being a thing that applies to certificates only (i.e. between NotBefore and NotAfter), while "Validity Interval" is the term used by the BRs to speak about CRL and OCSP responses (i.e. between ThisUpdate and NextUpdate): https://github.com/cabforum/servercert/blob/main/docs/BR.md#4910-on-line-revocation-checking-requirements

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
Open in desktop
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
Open in desktop
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