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 5 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
7 changes: 5 additions & 2 deletions ca/ca.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ type certificateAuthorityImpl struct {
sa sapb.StorageAuthorityCertificateClient
pa core.PolicyAuthority
issuers issuerMaps
profile *issuance.Profile

// This is temporary, and will be used for testing and slow roll-out
// of ECDSA issuance, but will then be removed.
Expand Down Expand Up @@ -103,6 +104,7 @@ func NewCertificateAuthorityImpl(
sa sapb.StorageAuthorityCertificateClient,
pa core.PolicyAuthority,
boulderIssuers []*issuance.Issuer,
certificateProfile *issuance.Profile,
ecdsaAllowList *ECDSAAllowList,
certExpiry time.Duration,
certBackdate time.Duration,
Expand Down Expand Up @@ -147,6 +149,7 @@ func NewCertificateAuthorityImpl(
sa: sa,
pa: pa,
issuers: issuers,
profile: certificateProfile,
validityPeriod: certExpiry,
backdate: certBackdate,
prefix: serialPrefix,
Expand Down Expand Up @@ -281,7 +284,7 @@ func (ca *certificateAuthorityImpl) IssueCertificateForPrecertificate(ctx contex
ca.log.AuditInfof("Signing cert: serial=[%s] regID=[%d] names=[%s] precert=[%s]",
serialHex, req.RegistrationID, names, hex.EncodeToString(precert.Raw))

_, issuanceToken, err := issuer.Prepare(issuanceReq)
_, issuanceToken, err := issuer.Prepare(ca.profile, issuanceReq)
if err != nil {
ca.log.AuditErrf("Preparing cert failed: serial=[%s] regID=[%d] names=[%s] err=[%v]",
serialHex, req.RegistrationID, names, err)
Expand Down Expand Up @@ -439,7 +442,7 @@ func (ca *certificateAuthorityImpl) issuePrecertificateInner(ctx context.Context
NotAfter: validity.NotAfter,
}

lintCertBytes, issuanceToken, err := issuer.Prepare(req)
lintCertBytes, issuanceToken, err := issuer.Prepare(ca.profile, req)
if err != nil {
ca.log.AuditErrf("Preparing precert failed: serial=[%s] regID=[%d] names=[%s] err=[%v]",
serialHex, issueReq.RegistrationID, strings.Join(csr.DNSNames, ", "), err)
Expand Down
133 changes: 61 additions & 72 deletions ca/ca_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package ca

import (
"context"
"crypto"
"crypto/ecdsa"
"crypto/elliptic"
"crypto/rand"
Expand Down Expand Up @@ -32,7 +31,6 @@ import (
"github.com/letsencrypt/boulder/features"
"github.com/letsencrypt/boulder/goodkey"
"github.com/letsencrypt/boulder/issuance"
"github.com/letsencrypt/boulder/linter"
blog "github.com/letsencrypt/boulder/log"
"github.com/letsencrypt/boulder/metrics"
"github.com/letsencrypt/boulder/must"
Expand Down Expand Up @@ -110,6 +108,7 @@ type testCtx struct {
pa core.PolicyAuthority
ocsp *ocspImpl
crl *crlImpl
profile *issuance.Profile
certExpiry time.Duration
certBackdate time.Duration
serialPrefix int
Expand Down Expand Up @@ -148,30 +147,8 @@ func (m *mockSA) SetCertificateStatusReady(ctx context.Context, req *sapb.Serial
return &emptypb.Empty{}, nil
}

var caKey crypto.Signer
var caCert *issuance.Certificate
var caCert2 *issuance.Certificate
var caLinter *linter.Linter
var caLinter2 *linter.Linter
var ctx = context.Background()

func init() {
var err error
caCert, caKey, err = issuance.LoadIssuer(issuance.IssuerLoc{
File: caKeyFile,
CertFile: caCertFile,
})
if err != nil {
panic(fmt.Sprintf("Unable to load %q and %q: %s", caKeyFile, caCertFile, err))
}
caCert2, err = issuance.LoadCertificate(caCertFile2)
if err != nil {
panic(fmt.Sprintf("Unable to parse %q: %s", caCertFile2, err))
}
caLinter, _ = linter.New(caCert.Certificate, caKey, []string{"w_subject_common_name_included"})
caLinter2, _ = linter.New(caCert2.Certificate, caKey, []string{"w_subject_common_name_included"})
}

func setup(t *testing.T) *testCtx {
features.Reset()
fc := clock.NewFake()
Expand All @@ -182,46 +159,44 @@ func setup(t *testing.T) *testCtx {
err = pa.LoadHostnamePolicyFile("../test/hostname-policy.yaml")
test.AssertNotError(t, err, "Couldn't set hostname policy")

boulderProfile := func(rsa, ecdsa bool) *issuance.Profile {
res, _ := issuance.NewProfile(
issuance.ProfileConfig{
AllowMustStaple: true,
AllowCTPoison: true,
AllowSCTList: true,
AllowCommonName: true,
Policies: []issuance.PolicyConfig{
{OID: "2.23.140.1.2.1"},
},
MaxValidityPeriod: config.Duration{Duration: time.Hour * 8760},
MaxValidityBackdate: config.Duration{Duration: time.Hour},
},
issuance.IssuerConfig{
UseForECDSALeaves: ecdsa,
UseForRSALeaves: rsa,
IssuerURL: "http://not-example.com/issuer-url",
OCSPURL: "http://not-example.com/ocsp",
CRLURL: "http://not-example.com/crl",
boulderProfile, err := issuance.NewProfile(
issuance.ProfileConfig{
AllowMustStaple: true,
AllowCTPoison: true,
AllowSCTList: true,
AllowCommonName: true,
Policies: []issuance.PolicyConfig{
{OID: "2.23.140.1.2.1"},
},
)
return res
}
boulderIssuers := []*issuance.Issuer{
// Must list ECDSA-only issuer first, so it is the default for ECDSA.
{
Cert: caCert2,
Signer: caKey,
Profile: boulderProfile(false, true),
Linter: caLinter2,
Clk: fc,
},
{
Cert: caCert,
Signer: caKey,
Profile: boulderProfile(true, true),
Linter: caLinter,
Clk: fc,
MaxValidityPeriod: config.Duration{Duration: time.Hour * 8760},
MaxValidityBackdate: config.Duration{Duration: time.Hour},
},
}
[]string{"w_subject_common_name_included"},
)
test.AssertNotError(t, err, "Couldn't create test profile")

ecdsaOnlyIssuer, err := issuance.LoadIssuer(issuance.IssuerConfig{
UseForRSALeaves: false,
UseForECDSALeaves: true,
IssuerURL: "http://not-example.com/issuer-url",
OCSPURL: "http://not-example.com/ocsp",
CRLURLBase: "http://not-example.com/crl/",
Location: issuance.IssuerLoc{File: caKeyFile, CertFile: caCertFile2},
}, fc)
test.AssertNotError(t, err, "Couldn't load test issuer")

ecdsaAndRSAIssuer, err := issuance.LoadIssuer(issuance.IssuerConfig{
UseForRSALeaves: true,
UseForECDSALeaves: true,
IssuerURL: "http://not-example.com/issuer-url",
OCSPURL: "http://not-example.com/ocsp",
CRLURLBase: "http://not-example.com/crl/",
Location: issuance.IssuerLoc{File: caKeyFile, CertFile: caCertFile},
}, fc)
test.AssertNotError(t, err, "Couldn't load test issuer")

// Must list ECDSA-only issuer first, so it is the default for ECDSA.
boulderIssuers := []*issuance.Issuer{ecdsaOnlyIssuer, ecdsaAndRSAIssuer}

keyPolicy := goodkey.KeyPolicy{
AllowRSA: true,
Expand Down Expand Up @@ -254,7 +229,10 @@ func setup(t *testing.T) *testCtx {

crl, err := NewCRLImpl(
boulderIssuers,
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 All @@ -265,6 +243,7 @@ func setup(t *testing.T) *testCtx {
pa: pa,
ocsp: ocsp,
crl: crl,
profile: boulderProfile,
certExpiry: 8760 * time.Hour,
certBackdate: time.Hour,
serialPrefix: 17,
Expand All @@ -287,6 +266,7 @@ func TestSerialPrefix(t *testing.T) {
nil,
nil,
nil,
nil,
testCtx.certExpiry,
testCtx.certBackdate,
0,
Expand All @@ -304,6 +284,7 @@ func TestSerialPrefix(t *testing.T) {
nil,
nil,
nil,
nil,
testCtx.certExpiry,
testCtx.certBackdate,
128,
Expand Down Expand Up @@ -396,6 +377,7 @@ func issueCertificateSubTestSetup(t *testing.T, e *ECDSAAllowList) (*certificate
sa,
testCtx.pa,
testCtx.boulderIssuers,
testCtx.profile,
e,
testCtx.certExpiry,
testCtx.certBackdate,
Expand Down Expand Up @@ -441,6 +423,7 @@ func TestNoIssuers(t *testing.T) {
sa,
testCtx.pa,
nil, // No issuers
testCtx.profile,
nil,
testCtx.certExpiry,
testCtx.certBackdate,
Expand All @@ -464,6 +447,7 @@ func TestMultipleIssuers(t *testing.T) {
sa,
testCtx.pa,
testCtx.boulderIssuers,
testCtx.profile,
nil,
testCtx.certExpiry,
testCtx.certBackdate,
Expand All @@ -477,20 +461,20 @@ func TestMultipleIssuers(t *testing.T) {
testCtx.fc)
test.AssertNotError(t, err, "Failed to remake CA")

// Test that an RSA CSR gets issuance from the RSA issuer, caCert.
// Test that an RSA CSR gets issuance from the RSA issuer.
issuedCert, err := ca.IssuePrecertificate(ctx, &capb.IssueCertificateRequest{Csr: CNandSANCSR, RegistrationID: arbitraryRegID})
test.AssertNotError(t, err, "Failed to issue certificate")
cert, err := x509.ParseCertificate(issuedCert.DER)
test.AssertNotError(t, err, "Certificate failed to parse")
err = cert.CheckSignatureFrom(caCert2.Certificate)
err = cert.CheckSignatureFrom(testCtx.boulderIssuers[1].Cert.Certificate)
test.AssertNotError(t, err, "Certificate failed signature validation")

// Test that an ECDSA CSR gets issuance from the ECDSA issuer, caCert2.
// Test that an ECDSA CSR gets issuance from the ECDSA issuer.
issuedCert, err = ca.IssuePrecertificate(ctx, &capb.IssueCertificateRequest{Csr: ECDSACSR, RegistrationID: arbitraryRegID})
test.AssertNotError(t, err, "Failed to issue certificate")
cert, err = x509.ParseCertificate(issuedCert.DER)
test.AssertNotError(t, err, "Certificate failed to parse")
err = cert.CheckSignatureFrom(caCert2.Certificate)
err = cert.CheckSignatureFrom(testCtx.boulderIssuers[0].Cert.Certificate)
test.AssertNotError(t, err, "Certificate failed signature validation")
}

Expand All @@ -504,7 +488,7 @@ func TestECDSAAllowList(t *testing.T) {
test.AssertNotError(t, err, "Failed to issue certificate")
cert, err := x509.ParseCertificate(result.DER)
test.AssertNotError(t, err, "Certificate failed to parse")
test.AssertByteEquals(t, cert.RawIssuer, caCert2.RawSubject)
test.AssertByteEquals(t, cert.RawIssuer, ca.issuers.byAlg[x509.ECDSA].Cert.RawSubject)

// With allowlist not containing arbitraryRegID, issuance should fall back to RSA issuer.
regIDMap = makeRegIDsMap([]int64{2002})
Expand All @@ -513,7 +497,7 @@ func TestECDSAAllowList(t *testing.T) {
test.AssertNotError(t, err, "Failed to issue certificate")
cert, err = x509.ParseCertificate(result.DER)
test.AssertNotError(t, err, "Certificate failed to parse")
test.AssertByteEquals(t, cert.RawIssuer, caCert.RawSubject)
test.AssertByteEquals(t, cert.RawIssuer, ca.issuers.byAlg[x509.RSA].Cert.RawSubject)

// With empty allowlist but ECDSAForAll enabled, issuance should come from ECDSA issuer.
ca, _ = issueCertificateSubTestSetup(t, nil)
Expand All @@ -523,7 +507,7 @@ func TestECDSAAllowList(t *testing.T) {
test.AssertNotError(t, err, "Failed to issue certificate")
cert, err = x509.ParseCertificate(result.DER)
test.AssertNotError(t, err, "Certificate failed to parse")
test.AssertByteEquals(t, cert.RawIssuer, caCert2.RawSubject)
test.AssertByteEquals(t, cert.RawIssuer, ca.issuers.byAlg[x509.ECDSA].Cert.RawSubject)
}

func TestInvalidCSRs(t *testing.T) {
Expand Down Expand Up @@ -585,6 +569,7 @@ func TestInvalidCSRs(t *testing.T) {
sa,
testCtx.pa,
testCtx.boulderIssuers,
testCtx.profile,
nil,
testCtx.certExpiry,
testCtx.certBackdate,
Expand Down Expand Up @@ -621,6 +606,7 @@ func TestRejectValidityTooLong(t *testing.T) {
sa,
testCtx.pa,
testCtx.boulderIssuers,
testCtx.profile,
nil,
testCtx.certExpiry,
testCtx.certBackdate,
Expand Down Expand Up @@ -681,8 +667,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 Expand Up @@ -721,6 +707,7 @@ func TestIssueCertificateForPrecertificate(t *testing.T) {
sa,
testCtx.pa,
testCtx.boulderIssuers,
testCtx.profile,
nil,
testCtx.certExpiry,
testCtx.certBackdate,
Expand Down Expand Up @@ -826,6 +813,7 @@ func TestIssueCertificateForPrecertificateDuplicateSerial(t *testing.T) {
sa,
testCtx.pa,
testCtx.boulderIssuers,
testCtx.profile,
nil,
testCtx.certExpiry,
testCtx.certBackdate,
Expand Down Expand Up @@ -867,6 +855,7 @@ func TestIssueCertificateForPrecertificateDuplicateSerial(t *testing.T) {
errorsa,
testCtx.pa,
testCtx.boulderIssuers,
testCtx.profile,
nil,
testCtx.certExpiry,
testCtx.certBackdate,
Expand Down
Loading