crl-storer: check number before uploading

crl/storer/storer.go

@@ -4,6 +4,8 @@ import (
 	"bytes"
 	"context"
 	"crypto/sha256"
+	"crypto/x509/pkix"
+	"encoding/asn1"
 	"encoding/base64"
 	"errors"
 	"fmt"
@@ -24,15 +26,16 @@ import (
 	blog "github.com/letsencrypt/boulder/log"
 )
 
-// s3Putter matches the subset of the s3.Client interface which we use, to allow
+// simpleS3 matches the subset of the s3.Client interface which we use, to allow
 // simpler mocking in tests.
-type s3Putter interface {
+type simpleS3 interface {
 	PutObject(ctx context.Context, params *s3.PutObjectInput, optFns ...func(*s3.Options)) (*s3.PutObjectOutput, error)
+	GetObject(ctx context.Context, params *s3.GetObjectInput, optFns ...func(*s3.Options)) (*s3.GetObjectOutput, error)
 }
 
 type crlStorer struct {
 	cspb.UnimplementedCRLStorerServer
-	s3Client         s3Putter
+	s3Client         simpleS3
 	s3Bucket         string
 	issuers          map[issuance.IssuerNameID]*issuance.Certificate
 	uploadCount      *prometheus.CounterVec
@@ -44,7 +47,7 @@ type crlStorer struct {
 
 func New(
 	issuers []*issuance.Certificate,
-	s3Client s3Putter,
+	s3Client simpleS3,
 	s3Bucket string,
 	stats prometheus.Registerer,
 	log blog.Logger,
@@ -90,12 +93,16 @@ func New(
 // TODO(#6261): Unify all error messages to identify the shard they're working
 // on as a JSON object including issuer, crl number, and shard number.
 
+// UploadCRL implements the gRPC method of the same name. It takes a stream of
+// bytes as its input, parses and runs some sanity checks on the CRL, and then
+// uploads it to S3.
 func (cs *crlStorer) UploadCRL(stream cspb.CRLStorer_UploadCRLServer) error {
 	var issuer *issuance.Certificate
 	var shardIdx int64
 	var crlNumber *big.Int
 	crlBytes := make([]byte, 0)
 
+	// Read all of the messages from the input stream.
 	for {
 		in, err := stream.Recv()
 		if err != nil {
@@ -126,9 +133,9 @@ func (cs *crlStorer) UploadCRL(stream cspb.CRLStorer_UploadCRLServer) error {
 		case *cspb.UploadCRLRequest_CrlChunk:
 			crlBytes = append(crlBytes, payload.CrlChunk...)
 		}
-
 	}
 
+	// Do some basic sanity checks on the received metadata and CRL.
 	if issuer == nil || crlNumber == nil {
 		return errors.New("got no metadata message")
 	}
@@ -151,9 +158,47 @@ func (cs *crlStorer) UploadCRL(stream cspb.CRLStorer_UploadCRLServer) error {
 		return fmt.Errorf("validating signature for %s: %w", crlId, err)
 	}
 
+	// Before uploading this CRL, we want to compare it against the previous CRL
+	// to ensure that the CRL Number field is not going backwards. This is an
+	// additional safety check against clock skew and potential races, if multiple
+	// crl-updaters are working on the same shard at the same time. We only run
+	// these checks if we found a CRL, so we don't block uploading brand new CRLs.
+	filename := fmt.Sprintf("%d/%d.crl", issuer.NameID(), shardIdx)
+	prevObj, err := cs.s3Client.GetObject(stream.Context(), &s3.GetObjectInput{
+		Bucket: &cs.s3Bucket,
+		Key:    &filename,
+	})
+	if err != nil {
+		noSuchKey := &types.NoSuchKey{}
+		if !errors.As(err, &noSuchKey) {
+			return fmt.Errorf("getting previous CRL for %s: %w", crlId, err)
+		}
+		cs.log.Infof("no previous CRL found for %s", crlId)
+	} else {
+		prevBytes, err := io.ReadAll(prevObj.Body)
+		if err != nil {
+			return fmt.Errorf("downloading previous CRL for %s: %w", crlId, err)
+		}
+
+		prevCRL, err := crl_x509.ParseRevocationList(prevBytes)
+		if err != nil {
+			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)
+		}
+	}
+
+	// Finally actually upload the new CRL.
 	start := cs.clk.Now()
 
-	filename := fmt.Sprintf("%d/%d.crl", issuer.NameID(), shardIdx)
 	checksum := sha256.Sum256(crlBytes)
 	checksumb64 := base64.StdEncoding.EncodeToString(checksum[:])
 	crlContentType := "application/pkix-crl"
@@ -184,3 +229,13 @@ 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
+}

crl/storer/storer_test.go

@@ -13,6 +13,7 @@ import (
 	"time"
 
 	"github.com/aws/aws-sdk-go-v2/service/s3"
+	"github.com/aws/aws-sdk-go-v2/service/s3/types"
 	"github.com/jmhodges/clock"
 	"google.golang.org/grpc"
 	"google.golang.org/protobuf/types/known/emptypb"
@@ -269,11 +270,14 @@ func TestUploadCRLMismatchedNumbers(t *testing.T) {
 	test.AssertContains(t, err.Error(), "mismatched")
 }
 
-type fakeS3Putter struct {
+// fakeSimpleS3 implements the simpleS3 interface, provides prevBytes for
+// downloads, and checks that uploads match the expectBytes.
+type fakeSimpleS3 struct {
+	prevBytes   []byte
 	expectBytes []byte
 }
 
-func (p *fakeS3Putter) PutObject(ctx context.Context, params *s3.PutObjectInput, optFns ...func(*s3.Options)) (*s3.PutObjectOutput, error) {
+func (p *fakeSimpleS3) PutObject(ctx context.Context, params *s3.PutObjectInput, optFns ...func(*s3.Options)) (*s3.PutObjectOutput, error) {
 	recvBytes, err := io.ReadAll(params.Body)
 	if err != nil {
 		return nil, err
@@ -284,11 +288,79 @@ func (p *fakeS3Putter) PutObject(ctx context.Context, params *s3.PutObjectInput,
 	return &s3.PutObjectOutput{}, nil
 }
 
+func (p *fakeSimpleS3) GetObject(ctx context.Context, params *s3.GetObjectInput, optFns ...func(*s3.Options)) (*s3.GetObjectOutput, error) {
+	if p.prevBytes != nil {
+		return &s3.GetObjectOutput{Body: io.NopCloser(bytes.NewReader(p.prevBytes))}, nil
+	}
+	return nil, &types.NoSuchKey{}
+}
+
 // Test that the correct bytes get propagated to S3.
 func TestUploadCRLSuccess(t *testing.T) {
 	storer, iss := setupTestUploadCRL(t)
 	errs := make(chan error, 1)
 
+	ins := make(chan *cspb.UploadCRLRequest)
+	go func() {
+		errs <- storer.UploadCRL(&fakeUploadCRLServerStream{input: ins})
+	}()
+	ins <- &cspb.UploadCRLRequest{
+		Payload: &cspb.UploadCRLRequest_Metadata{
+			Metadata: &cspb.CRLMetadata{
+				IssuerNameID: int64(iss.Cert.NameID()),
+				Number:       2,
+			},
+		},
+	}
+
+	prevCRLBytes, err := crl_x509.CreateRevocationList(
+		rand.Reader,
+		&crl_x509.RevocationList{
+			ThisUpdate: storer.clk.Now(),
+			NextUpdate: storer.clk.Now().Add(time.Hour),
+			Number:     big.NewInt(1),
+			RevokedCertificates: []crl_x509.RevokedCertificate{
+				{SerialNumber: big.NewInt(123), RevocationTime: time.Now().Add(-time.Hour)},
+			},
+		},
+		iss.Cert.Certificate,
+		iss.Signer,
+	)
+	test.AssertNotError(t, err, "creating test CRL")
+
+	storer.clk.Sleep(time.Minute)
+
+	crlBytes, err := crl_x509.CreateRevocationList(
+		rand.Reader,
+		&crl_x509.RevocationList{
+			ThisUpdate: storer.clk.Now(),
+			NextUpdate: storer.clk.Now().Add(time.Hour),
+			Number:     big.NewInt(2),
+			RevokedCertificates: []crl_x509.RevokedCertificate{
+				{SerialNumber: big.NewInt(123), RevocationTime: time.Now().Add(-time.Hour)},
+			},
+		},
+		iss.Cert.Certificate,
+		iss.Signer,
+	)
+	test.AssertNotError(t, err, "creating test CRL")
+
+	storer.s3Client = &fakeSimpleS3{prevBytes: prevCRLBytes, expectBytes: crlBytes}
+	ins <- &cspb.UploadCRLRequest{
+		Payload: &cspb.UploadCRLRequest_CrlChunk{
+			CrlChunk: crlBytes,
+		},
+	}
+	close(ins)
+	err = <-errs
+	test.AssertNotError(t, err, "uploading valid CRL should work")
+}
+
+// Test that the correct bytes get propagated to S3 for a CRL with to predecessor.
+func TestUploadNewCRLSuccess(t *testing.T) {
+	storer, iss := setupTestUploadCRL(t)
+	errs := make(chan error, 1)
+
 	ins := make(chan *cspb.UploadCRLRequest)
 	go func() {
 		errs <- storer.UploadCRL(&fakeUploadCRLServerStream{input: ins})
@@ -301,6 +373,7 @@ func TestUploadCRLSuccess(t *testing.T) {
 			},
 		},
 	}
+
 	crlBytes, err := crl_x509.CreateRevocationList(
 		rand.Reader,
 		&crl_x509.RevocationList{
@@ -315,7 +388,8 @@ func TestUploadCRLSuccess(t *testing.T) {
 		iss.Signer,
 	)
 	test.AssertNotError(t, err, "creating test CRL")
-	storer.s3Client = &fakeS3Putter{expectBytes: crlBytes}
+
+	storer.s3Client = &fakeSimpleS3{expectBytes: crlBytes}
 	ins <- &cspb.UploadCRLRequest{
 		Payload: &cspb.UploadCRLRequest_CrlChunk{
 			CrlChunk: crlBytes,
@@ -326,12 +400,80 @@ func TestUploadCRLSuccess(t *testing.T) {
 	test.AssertNotError(t, err, "uploading valid CRL should work")
 }
 
-type brokenS3Putter struct{}
+// Test that we get an error when the previous CRL has a higher CRL number.
+func TestUploadCRLBackwardsNumber(t *testing.T) {
+	storer, iss := setupTestUploadCRL(t)
+	errs := make(chan error, 1)
+
+	ins := make(chan *cspb.UploadCRLRequest)
+	go func() {
+		errs <- storer.UploadCRL(&fakeUploadCRLServerStream{input: ins})
+	}()
+	ins <- &cspb.UploadCRLRequest{
+		Payload: &cspb.UploadCRLRequest_Metadata{
+			Metadata: &cspb.CRLMetadata{
+				IssuerNameID: int64(iss.Cert.NameID()),
+				Number:       1,
+			},
+		},
+	}
+
+	prevCRLBytes, err := crl_x509.CreateRevocationList(
+		rand.Reader,
+		&crl_x509.RevocationList{
+			ThisUpdate: storer.clk.Now(),
+			NextUpdate: storer.clk.Now().Add(time.Hour),
+			Number:     big.NewInt(2),
+			RevokedCertificates: []crl_x509.RevokedCertificate{
+				{SerialNumber: big.NewInt(123), RevocationTime: time.Now().Add(-time.Hour)},
+			},
+		},
+		iss.Cert.Certificate,
+		iss.Signer,
+	)
+	test.AssertNotError(t, err, "creating test CRL")
+
+	storer.clk.Sleep(time.Minute)
+
+	crlBytes, err := crl_x509.CreateRevocationList(
+		rand.Reader,
+		&crl_x509.RevocationList{
+			ThisUpdate: storer.clk.Now(),
+			NextUpdate: storer.clk.Now().Add(time.Hour),
+			Number:     big.NewInt(1),
+			RevokedCertificates: []crl_x509.RevokedCertificate{
+				{SerialNumber: big.NewInt(123), RevocationTime: time.Now().Add(-time.Hour)},
+			},
+		},
+		iss.Cert.Certificate,
+		iss.Signer,
+	)
+	test.AssertNotError(t, err, "creating test CRL")
+
+	storer.s3Client = &fakeSimpleS3{prevBytes: prevCRLBytes, expectBytes: crlBytes}
+	ins <- &cspb.UploadCRLRequest{
+		Payload: &cspb.UploadCRLRequest_CrlChunk{
+			CrlChunk: crlBytes,
+		},
+	}
+	close(ins)
+	err = <-errs
+	test.AssertError(t, err, "uploading out-of-order numbers should fail")
+	test.AssertContains(t, err.Error(), "crlNumber not strictly increasing")
+}
+
+// brokenSimpleS3 implements the simpleS3 interface. It returns errors for all
+// uploads and downloads.
+type brokenSimpleS3 struct{}
 
-func (p *brokenS3Putter) PutObject(ctx context.Context, params *s3.PutObjectInput, optFns ...func(*s3.Options)) (*s3.PutObjectOutput, error) {
+func (p *brokenSimpleS3) PutObject(ctx context.Context, params *s3.PutObjectInput, optFns ...func(*s3.Options)) (*s3.PutObjectOutput, error) {
 	return nil, errors.New("sorry")
 }
 
+func (p *brokenSimpleS3) GetObject(ctx context.Context, params *s3.GetObjectInput, optFns ...func(*s3.Options)) (*s3.GetObjectOutput, error) {
+	return nil, errors.New("oops")
+}
+
 // Test that we get an error when S3 falls over.
 func TestUploadCRLBrokenS3(t *testing.T) {
 	storer, iss := setupTestUploadCRL(t)
@@ -363,7 +505,7 @@ func TestUploadCRLBrokenS3(t *testing.T) {
 		iss.Signer,
 	)
 	test.AssertNotError(t, err, "creating test CRL")
-	storer.s3Client = &brokenS3Putter{}
+	storer.s3Client = &brokenSimpleS3{}
 	ins <- &cspb.UploadCRLRequest{
 		Payload: &cspb.UploadCRLRequest_CrlChunk{
 			CrlChunk: crlBytes,
@@ -372,5 +514,5 @@ func TestUploadCRLBrokenS3(t *testing.T) {
 	close(ins)
 	err = <-errs
 	test.AssertError(t, err, "uploading to broken S3 should fail")
-	test.AssertContains(t, err.Error(), "uploading to S3")
+	test.AssertContains(t, err.Error(), "getting previous CRL")
 }

test/integration/crl_test.go

@@ -73,11 +73,6 @@ func TestCRLPipeline(t *testing.T) {
 	err = client.RevokeCertificate(client.Account, cert, client.PrivateKey, 5)
 	test.AssertNotError(t, err, "failed to revoke test certificate")
 
-	// Clear the s3-test-srv to prepare for another round of CRLs.
-	resp, err = http.Post("http://localhost:7890/clear", "text/plain", nil)
-	test.AssertNotError(t, err, "s3-test-srv GET /clear failed")
-	test.AssertEquals(t, resp.StatusCode, 200)
-
 	// Reset the "leasedUntil" column to prepare for another round of CRLs.
 	_, err = db.Exec(`UPDATE crlShards SET leasedUntil = ?`, fc.Now().Add(-time.Minute))
 	test.AssertNotError(t, err, "resetting leasedUntil column")