From 402876bc429ffacb1e1dd109117a1f7972c86754 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Fri, 1 Sep 2023 09:02:02 -0700 Subject: [PATCH 1/4] crl-storer: check number before uploading --- crl/storer/storer.go | 67 +++++++++++++-- crl/storer/storer_test.go | 156 +++++++++++++++++++++++++++++++++-- test/integration/crl_test.go | 5 -- test/s3-test-srv/main.go | 31 ++++--- 4 files changed, 231 insertions(+), 28 deletions(-) diff --git a/crl/storer/storer.go b/crl/storer/storer.go index 055c0f0280b..ded8e1a0ada 100644 --- a/crl/storer/storer.go +++ b/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 +} diff --git a/crl/storer/storer_test.go b/crl/storer/storer_test.go index ecdf0c7e903..14e3f6963d3 100644 --- a/crl/storer/storer_test.go +++ b/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") } 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 4708f0e896c..abb1edfeb74 100644 --- a/test/s3-test-srv/main.go +++ b/test/s3-test-srv/main.go @@ -18,6 +18,15 @@ 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) + } } func (srv *s3TestSrv) handleUpload(w http.ResponseWriter, r *http.Request) { @@ -37,6 +46,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.RevokedCertificates { reason := 0 if rc.ReasonCode != nil { @@ -49,15 +59,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) { @@ -88,10 +97,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{ From 51be9f9484cb82fadd59206afbff167c833669d8 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Thu, 7 Sep 2023 13:26:04 -0700 Subject: [PATCH 2/4] Fix integration tests --- crl/storer/storer.go | 7 ++++--- test/s3-test-srv/main.go | 2 ++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/crl/storer/storer.go b/crl/storer/storer.go index ded8e1a0ada..a06dbf7fc82 100644 --- a/crl/storer/storer.go +++ b/crl/storer/storer.go @@ -15,6 +15,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" @@ -169,11 +170,11 @@ func (cs *crlStorer) UploadCRL(stream cspb.CRLStorer_UploadCRLServer) error { Key: &filename, }) if err != nil { - noSuchKey := &types.NoSuchKey{} - if !errors.As(err, &noSuchKey) { + 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", crlId) + cs.log.Infof("No previous CRL found for %s, proceeding", crlId) } else { prevBytes, err := io.ReadAll(prevObj.Body) if err != nil { diff --git a/test/s3-test-srv/main.go b/test/s3-test-srv/main.go index abb1edfeb74..56b64542749 100644 --- a/test/s3-test-srv/main.go +++ b/test/s3-test-srv/main.go @@ -26,6 +26,8 @@ func (srv *s3TestSrv) handleS3(w http.ResponseWriter, r *http.Request) { srv.handleUpload(w, r) } else if r.Method == "GET" { srv.handleDownload(w, r) + } else { + w.WriteHeader(405) } } From a58f5fb31ec83b31a36a13f5989a0ecd0172d99e Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Thu, 7 Sep 2023 13:50:09 -0700 Subject: [PATCH 3/4] And unit test too --- crl/storer/storer_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crl/storer/storer_test.go b/crl/storer/storer_test.go index 14e3f6963d3..ddc02f18f70 100644 --- a/crl/storer/storer_test.go +++ b/crl/storer/storer_test.go @@ -9,11 +9,12 @@ import ( "errors" "io" "math/big" + "net/http" "testing" "time" "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" "google.golang.org/grpc" "google.golang.org/protobuf/types/known/emptypb" @@ -292,7 +293,7 @@ func (p *fakeSimpleS3) GetObject(ctx context.Context, params *s3.GetObjectInput, if p.prevBytes != nil { return &s3.GetObjectOutput{Body: io.NopCloser(bytes.NewReader(p.prevBytes))}, nil } - return nil, &types.NoSuchKey{} + return nil, &smithyhttp.ResponseError{Response: &smithyhttp.Response{Response: &http.Response{StatusCode: 404}}} } // Test that the correct bytes get propagated to S3. From 2521236c8557066fc1a2c5c235c434ed395fc803 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Mon, 25 Sep 2023 16:08:52 -0700 Subject: [PATCH 4/4] comment --- crl/storer/storer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crl/storer/storer.go b/crl/storer/storer.go index ace57c5c600..cd0bf86c0f0 100644 --- a/crl/storer/storer.go +++ b/crl/storer/storer.go @@ -170,7 +170,7 @@ func (cs *crlStorer) UploadCRL(stream cspb.CRLStorer_UploadCRLServer) error { Key: &filename, }) if err != nil { - smithyErr := &smithyhttp.ResponseError{} + var smithyErr *smithyhttp.ResponseError if !errors.As(err, &smithyErr) || smithyErr.HTTPStatusCode() != 404 { return fmt.Errorf("getting previous CRL for %s: %w", crlId, err) }