diff --git a/crl/storer/storer.go b/crl/storer/storer.go index d1470c7a46a..cd0bf86c0f0 100644 --- a/crl/storer/storer.go +++ b/crl/storer/storer.go @@ -5,6 +5,8 @@ import ( "context" "crypto/sha256" "crypto/x509" + "crypto/x509/pkix" + "encoding/asn1" "encoding/base64" "errors" "fmt" @@ -14,6 +16,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/aws/aws-sdk-go-v2/service/s3/types" + smithyhttp "github.com/aws/smithy-go/transport/http" "github.com/jmhodges/clock" "github.com/prometheus/client_golang/prometheus" "google.golang.org/protobuf/types/known/emptypb" @@ -24,15 +27,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 +48,7 @@ type crlStorer struct { func New( issuers []*issuance.Certificate, - s3Client s3Putter, + s3Client simpleS3, s3Bucket string, stats prometheus.Registerer, log blog.Logger, @@ -90,12 +94,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 +134,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 +159,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 { + var smithyErr *smithyhttp.ResponseError + if !errors.As(err, &smithyErr) || smithyErr.HTTPStatusCode() != 404 { + return fmt.Errorf("getting previous CRL for %s: %w", crlId, err) + } + cs.log.Infof("No previous CRL found for %s, proceeding", crlId) + } else { + prevBytes, err := io.ReadAll(prevObj.Body) + if err != nil { + return fmt.Errorf("downloading previous CRL for %s: %w", crlId, err) + } + + prevCRL, err := 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 +230,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 +} diff --git a/crl/storer/storer_test.go b/crl/storer/storer_test.go index 28c6b79f888..5df24f37f42 100644 --- a/crl/storer/storer_test.go +++ b/crl/storer/storer_test.go @@ -10,10 +10,12 @@ import ( "errors" "io" "math/big" + "net/http" "testing" "time" "github.com/aws/aws-sdk-go-v2/service/s3" + smithyhttp "github.com/aws/smithy-go/transport/http" "github.com/jmhodges/clock" "google.golang.org/grpc" "google.golang.org/protobuf/types/known/emptypb" @@ -269,11 +271,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 +289,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, &smithyhttp.ResponseError{Response: &smithyhttp.Response{Response: &http.Response{StatusCode: 404}}} +} + // 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 := x509.CreateRevocationList( + rand.Reader, + &x509.RevocationList{ + ThisUpdate: storer.clk.Now(), + NextUpdate: storer.clk.Now().Add(time.Hour), + Number: big.NewInt(1), + RevokedCertificateEntries: []x509.RevocationListEntry{ + {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 := x509.CreateRevocationList( + rand.Reader, + &x509.RevocationList{ + ThisUpdate: storer.clk.Now(), + NextUpdate: storer.clk.Now().Add(time.Hour), + Number: big.NewInt(2), + RevokedCertificateEntries: []x509.RevocationListEntry{ + {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 +374,7 @@ func TestUploadCRLSuccess(t *testing.T) { }, }, } + crlBytes, err := x509.CreateRevocationList( rand.Reader, &x509.RevocationList{ @@ -315,7 +389,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 +401,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 := x509.CreateRevocationList( + rand.Reader, + &x509.RevocationList{ + ThisUpdate: storer.clk.Now(), + NextUpdate: storer.clk.Now().Add(time.Hour), + Number: big.NewInt(2), + RevokedCertificateEntries: []x509.RevocationListEntry{ + {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 := x509.CreateRevocationList( + rand.Reader, + &x509.RevocationList{ + ThisUpdate: storer.clk.Now(), + NextUpdate: storer.clk.Now().Add(time.Hour), + Number: big.NewInt(1), + RevokedCertificateEntries: []x509.RevocationListEntry{ + {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 +506,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 +515,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") } diff --git a/test/integration/crl_test.go b/test/integration/crl_test.go index 1bfa6329ba0..b96aad5b4ae 100644 --- a/test/integration/crl_test.go +++ b/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") diff --git a/test/s3-test-srv/main.go b/test/s3-test-srv/main.go index 5f0142c05dd..10a3dce2b82 100644 --- a/test/s3-test-srv/main.go +++ b/test/s3-test-srv/main.go @@ -18,6 +18,17 @@ import ( type s3TestSrv struct { sync.RWMutex allSerials map[string]revocation.Reason + allShards map[string][]byte +} + +func (srv *s3TestSrv) handleS3(w http.ResponseWriter, r *http.Request) { + if r.Method == "PUT" { + srv.handleUpload(w, r) + } else if r.Method == "GET" { + srv.handleDownload(w, r) + } else { + w.WriteHeader(405) + } } func (srv *s3TestSrv) handleUpload(w http.ResponseWriter, r *http.Request) { @@ -37,6 +48,7 @@ func (srv *s3TestSrv) handleUpload(w http.ResponseWriter, r *http.Request) { srv.Lock() defer srv.Unlock() + srv.allShards[r.URL.Path] = body for _, rc := range crl.RevokedCertificateEntries { srv.allSerials[core.SerialToString(rc.SerialNumber)] = revocation.Reason(rc.ReasonCode) } @@ -45,15 +57,14 @@ func (srv *s3TestSrv) handleUpload(w http.ResponseWriter, r *http.Request) { w.Write([]byte("{}")) } -func (srv *s3TestSrv) handleClear(w http.ResponseWriter, r *http.Request) { - if r.Method != "POST" { - w.WriteHeader(405) +func (srv *s3TestSrv) handleDownload(w http.ResponseWriter, r *http.Request) { + body, ok := srv.allShards[r.URL.Path] + if !ok { + w.WriteHeader(404) return } - - srv.Lock() - defer srv.Unlock() - srv.allSerials = make(map[string]revocation.Reason) + w.WriteHeader(200) + w.Write(body) } func (srv *s3TestSrv) handleQuery(w http.ResponseWriter, r *http.Request) { @@ -84,10 +95,12 @@ func main() { listenAddr := flag.String("listen", "0.0.0.0:7890", "Address to listen on") flag.Parse() - srv := s3TestSrv{allSerials: make(map[string]revocation.Reason)} + srv := s3TestSrv{ + allSerials: make(map[string]revocation.Reason), + allShards: make(map[string][]byte), + } - http.HandleFunc("/", srv.handleUpload) - http.HandleFunc("/clear", srv.handleClear) + http.HandleFunc("/", srv.handleS3) http.HandleFunc("/query", srv.handleQuery) s := http.Server{