Skip to content

Commit

Permalink
Centralize IDP construction and make IDP comparison smarter (#7341)
Browse files Browse the repository at this point in the history
Change crl-storer to only require that 1 of the IssuingDistributionPoint
URIs remain consistent between consecutive CRLs in the same sequence.
This allows us to add and remove IDP URIs, so we can change our IDP
scheme over time.

To facilitate this, also move all code which builds or parses IDP
extensions into a single place, so that we don't have to have multiple
definitions of the same types and similar code in many places.

Fixes #7340
Part of #7296
  • Loading branch information
aarongable authored Mar 7, 2024
1 parent c110a3e commit 7432833
Show file tree
Hide file tree
Showing 7 changed files with 176 additions and 133 deletions.
32 changes: 2 additions & 30 deletions cmd/ceremony/crl.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,13 @@ package main
import (
"crypto"
"crypto/x509"
"crypto/x509/pkix"
"encoding/asn1"
"encoding/pem"
"errors"
"fmt"
"math/big"
"time"

"github.com/letsencrypt/boulder/crl/idp"
"github.com/letsencrypt/boulder/linter"
)

Expand All @@ -37,7 +36,7 @@ func generateCRL(signer crypto.Signer, issuer *x509.Certificate, thisUpdate, nex
return nil, errors.New("nextUpdate must be less than 12 months after thisUpdate")
}
// Add the Issuing Distribution Point extension.
idp, err := makeIDPExt()
idp, err := idp.MakeCACertsExt()
if err != nil {
return nil, fmt.Errorf("creating IDP extension: %w", err)
}
Expand All @@ -60,30 +59,3 @@ func generateCRL(signer crypto.Signer, issuer *x509.Certificate, thisUpdate, nex

return pem.EncodeToMemory(&pem.Block{Type: "X509 CRL", Bytes: crlBytes}), nil
}

// issuingDistributionPoint represents the ASN.1 IssuingDistributionPoint
// SEQUENCE as defined in RFC 5280 Section 5.2.5. We only use one of the fields,
// all others are omitted.
// https://datatracker.ietf.org/doc/html/rfc5280#page-66
type issuingDistributionPoint struct {
OnlyContainsCACerts bool `asn1:"optional,tag:2"`
}

// makeIDPExt returns a critical IssuingDistributionPoint extension enabling the
// OnlyContainsCACerts boolean.
func makeIDPExt() (*pkix.Extension, error) {
val := issuingDistributionPoint{
OnlyContainsCACerts: 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
}
102 changes: 102 additions & 0 deletions crl/idp/idp.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
package idp

import (
"crypto/x509/pkix"
"encoding/asn1"
"errors"
"fmt"
)

var idpOID = asn1.ObjectIdentifier{2, 5, 29, 28} // id-ce-issuingDistributionPoint

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

// 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"`
}

// MakeUserCertsExt returns a critical IssuingDistributionPoint extension
// containing the given URLs and with the OnlyContainsUserCerts boolean set to
// true.
func MakeUserCertsExt(urls []string) (pkix.Extension, error) {
var gns []asn1.RawValue
for _, url := range urls {
gns = append(gns, asn1.RawValue{ // GeneralName
Class: 2, // context-specific
Tag: 6, // uniformResourceIdentifier, IA5String
Bytes: []byte(url),
})
}

val := issuingDistributionPoint{
DistributionPoint: distributionPointName{FullName: gns},
OnlyContainsUserCerts: true,
}

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

return pkix.Extension{
Id: idpOID,
Value: valBytes,
Critical: true,
}, nil
}

// MakeCACertsExt returns a critical IssuingDistributionPoint extension
// asserting the OnlyContainsCACerts boolean.
func MakeCACertsExt() (*pkix.Extension, error) {
val := issuingDistributionPoint{
OnlyContainsCACerts: true,
}

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

return &pkix.Extension{
Id: idpOID,
Value: valBytes,
Critical: true,
}, nil
}

// GetIDPURIs returns the URIs contained within the issuingDistributionPoint
// extension, if present, or an error otherwise.
func GetIDPURIs(exts []pkix.Extension) ([]string, error) {
for _, ext := range exts {
if ext.Id.Equal(idpOID) {
val := issuingDistributionPoint{}
rest, err := asn1.Unmarshal(ext.Value, &val)
if err != nil {
return nil, fmt.Errorf("parsing IssuingDistributionPoint extension: %w", err)
}
if len(rest) != 0 {
return nil, fmt.Errorf("parsing IssuingDistributionPoint extension: got %d unexpected trailing bytes", len(rest))
}
var uris []string
for _, generalName := range val.DistributionPoint.FullName {
uris = append(uris, string(generalName.Bytes))
}
return uris, nil
}
}
return nil, errors.New("no IssuingDistributionPoint extension found")
}
40 changes: 40 additions & 0 deletions crl/idp/idp_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package idp

import (
"encoding/hex"
"testing"

"github.com/letsencrypt/boulder/test"
)

func TestMakeUserCertsExt(t *testing.T) {
t.Parallel()
dehex := func(s string) []byte { r, _ := hex.DecodeString(s); return r }
tests := []struct {
name string
urls []string
want []byte
}{
{
name: "one (real) url",
urls: []string{"http://prod.c.lencr.org/20506757847264211/126.crl"},
want: dehex("303AA035A0338631687474703A2F2F70726F642E632E6C656E63722E6F72672F32303530363735373834373236343231312F3132362E63726C8101FF"),
},
{
name: "two urls",
urls: []string{"http://old.style/12345678/90.crl", "http://new.style/90.crl"},
want: dehex("3042A03DA03B8620687474703A2F2F6F6C642E7374796C652F31323334353637382F39302E63726C8617687474703A2F2F6E65772E7374796C652F39302E63726C8101FF"),
},
}
for _, tc := range tests {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
got, err := MakeUserCertsExt(tc.urls)
test.AssertNotError(t, err, "should never fail to marshal asn1 to bytes")
test.AssertDeepEquals(t, got.Id, idpOID)
test.AssertEquals(t, got.Critical, true)
test.AssertDeepEquals(t, got.Value, tc.want)
})
}
}
41 changes: 23 additions & 18 deletions crl/storer/storer.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,12 @@ import (
"context"
"crypto/sha256"
"crypto/x509"
"crypto/x509/pkix"
"encoding/asn1"
"encoding/base64"
"errors"
"fmt"
"io"
"math/big"
"slices"
"time"

"github.com/aws/aws-sdk-go-v2/service/s3"
Expand All @@ -22,6 +21,7 @@ import (
"google.golang.org/protobuf/types/known/emptypb"

"github.com/letsencrypt/boulder/crl"
"github.com/letsencrypt/boulder/crl/idp"
cspb "github.com/letsencrypt/boulder/crl/storer/proto"
"github.com/letsencrypt/boulder/issuance"
blog "github.com/letsencrypt/boulder/log"
Expand Down Expand Up @@ -186,15 +186,30 @@ func (cs *crlStorer) UploadCRL(stream cspb.CRLStorer_UploadCRLServer) error {
return fmt.Errorf("parsing previous CRL for %s: %w", crlId, err)
}

idp := getIDPExt(crl.Extensions)
prevIdp := getIDPExt(prevCRL.Extensions)
if !bytes.Equal(idp, prevIdp) {
return fmt.Errorf("IDP does not match previous: %x != %x", idp, prevIdp)
}

if crl.Number.Cmp(prevCRL.Number) <= 0 {
return fmt.Errorf("crlNumber not strictly increasing: %d <= %d", crl.Number, prevCRL.Number)
}

idpURIs, err := idp.GetIDPURIs(crl.Extensions)
if err != nil {
return fmt.Errorf("getting IDP for %s: %w", crlId, err)
}

prevURIs, err := idp.GetIDPURIs(prevCRL.Extensions)
if err != nil {
return fmt.Errorf("getting previous IDP for %s: %w", crlId, err)
}

uriMatch := false
for _, uri := range idpURIs {
if slices.Contains(prevURIs, uri) {
uriMatch = true
break
}
}
if !uriMatch {
return fmt.Errorf("IDP does not match previous: %v !∩ %v", idpURIs, prevURIs)
}
}

// Finally actually upload the new CRL.
Expand Down Expand Up @@ -230,13 +245,3 @@ func (cs *crlStorer) UploadCRL(stream cspb.CRLStorer_UploadCRLServer) error {

return stream.SendAndClose(&emptypb.Empty{})
}

// getIDPExt returns the contents of the issuingDistributionPoint extension, if present.
func getIDPExt(exts []pkix.Extension) []byte {
for _, ext := range exts {
if ext.Id.Equal(asn1.ObjectIdentifier{2, 5, 29, 28}) { // id-ce-issuingDistributionPoint
return ext.Value
}
}
return nil
}
7 changes: 7 additions & 0 deletions crl/storer/storer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"crypto/elliptic"
"crypto/rand"
"crypto/x509"
"crypto/x509/pkix"
"errors"
"io"
"math/big"
Expand All @@ -20,6 +21,7 @@ import (
"google.golang.org/grpc"
"google.golang.org/protobuf/types/known/emptypb"

"github.com/letsencrypt/boulder/crl/idp"
cspb "github.com/letsencrypt/boulder/crl/storer/proto"
"github.com/letsencrypt/boulder/issuance"
blog "github.com/letsencrypt/boulder/log"
Expand Down Expand Up @@ -307,6 +309,9 @@ func TestUploadCRLSuccess(t *testing.T) {
storer, iss := setupTestUploadCRL(t)
errs := make(chan error, 1)

idpExt, err := idp.MakeUserCertsExt([]string{"http://c.ex.org"})
test.AssertNotError(t, err, "creating test IDP extension")

ins := make(chan *cspb.UploadCRLRequest)
go func() {
errs <- storer.UploadCRL(&fakeUploadCRLServerStream{input: ins})
Expand All @@ -329,6 +334,7 @@ func TestUploadCRLSuccess(t *testing.T) {
RevokedCertificateEntries: []x509.RevocationListEntry{
{SerialNumber: big.NewInt(123), RevocationTime: time.Now().Add(-time.Hour)},
},
ExtraExtensions: []pkix.Extension{idpExt},
},
iss.Cert.Certificate,
iss.Signer,
Expand All @@ -346,6 +352,7 @@ func TestUploadCRLSuccess(t *testing.T) {
RevokedCertificateEntries: []x509.RevocationListEntry{
{SerialNumber: big.NewInt(123), RevocationTime: time.Now().Add(-time.Hour)},
},
ExtraExtensions: []pkix.Extension{idpExt},
},
iss.Cert.Certificate,
iss.Signer,
Expand Down
53 changes: 2 additions & 51 deletions issuance/crl.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,14 @@ package issuance
import (
"crypto/rand"
"crypto/x509"
"crypto/x509/pkix"
"encoding/asn1"
"fmt"
"math/big"
"time"

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

"github.com/letsencrypt/boulder/config"
"github.com/letsencrypt/boulder/crl/idp"
"github.com/letsencrypt/boulder/linter"
)

Expand Down Expand Up @@ -92,7 +91,7 @@ func (i *Issuer) IssueCRL(prof *CRLProfile, req *CRLRequest) ([]byte, error) {
// contain the new-style CRLDP URL instead.
idps = append(idps, fmt.Sprintf("%s/%d/%d.crl", req.DeprecatedIDPBaseURL, i.NameID(), req.Shard))
}
idp, err := makeIDPExt(idps)
idp, err := idp.MakeUserCertsExt(idps)
if err != nil {
return nil, fmt.Errorf("creating IDP extension: %w", err)
}
Expand All @@ -115,51 +114,3 @@ func (i *Issuer) IssueCRL(prof *CRLProfile, req *CRLRequest) ([]byte, error) {

return crlBytes, 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
// the given URLs and with the OnlyContainsUserCerts boolean set to true.
func makeIDPExt(urls []string) (pkix.Extension, error) {
var gns []asn1.RawValue
for _, url := range urls {
gns = append(gns, asn1.RawValue{ // GeneralName
Class: 2, // context-specific
Tag: 6, // uniformResourceIdentifier, IA5String
Bytes: []byte(url),
})
}

val := issuingDistributionPoint{
DistributionPoint: distributionPointName{FullName: gns},
OnlyContainsUserCerts: true,
}

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

return pkix.Extension{
Id: asn1.ObjectIdentifier{2, 5, 29, 28}, // id-ce-issuingDistributionPoint
Value: valBytes,
Critical: true,
}, nil
}
Loading

0 comments on commit 7432833

Please sign in to comment.