From 8dcd7a0ac7dcd60c7bf7ff6dd53b2c945bfc186f Mon Sep 17 00:00:00 2001 From: Esteban Date: Thu, 13 Jun 2024 23:36:32 +0000 Subject: [PATCH] Avoid potential connectivity related layer corruption in userspace convertor Add additional checks to prevent deduplicated layer commit files from having partial downloads leading to corrupted images. Signed-off-by: Esteban --- cmd/convertor/builder/builder_utils.go | 16 +- cmd/convertor/builder/overlaybd_builder.go | 57 +++- .../builder/overlaybd_builder_test.go | 313 +++++++++++++++++- 3 files changed, 372 insertions(+), 14 deletions(-) diff --git a/cmd/convertor/builder/builder_utils.go b/cmd/convertor/builder/builder_utils.go index 4a2d9541..a0c33711 100644 --- a/cmd/convertor/builder/builder_utils.go +++ b/cmd/convertor/builder/builder_utils.go @@ -119,10 +119,18 @@ func fetchManifestAndConfig(ctx context.Context, fetcher remotes.Fetcher, desc s } func downloadLayer(ctx context.Context, fetcher remotes.Fetcher, targetFile string, desc specs.Descriptor, decompress bool) error { - rc, err := fetcher.Fetch(ctx, desc) + rcoriginal, err := fetcher.Fetch(ctx, desc) if err != nil { return err } + + verifier := desc.Digest.Verifier() + // tee the reader to verify the digest + // this is because the decompression result + // will be different from the original for which + // the digest is calculated. + rc := io.TeeReader(rcoriginal, verifier) + dir := path.Dir(targetFile) if err := os.MkdirAll(dir, 0755); err != nil { return err @@ -131,6 +139,7 @@ func downloadLayer(ctx context.Context, fetcher remotes.Fetcher, targetFile stri if err != nil { return err } + if decompress { rc, err = compression.DecompressStream(rc) if err != nil { @@ -140,6 +149,11 @@ func downloadLayer(ctx context.Context, fetcher remotes.Fetcher, targetFile stri if _, err = io.Copy(ftar, rc); err != nil { return err } + + if !verifier.Verified() { + return fmt.Errorf("failed to verify digest %v", desc.Digest) + } + return nil } diff --git a/cmd/convertor/builder/overlaybd_builder.go b/cmd/convertor/builder/overlaybd_builder.go index 09a8c4f1..313f1570 100644 --- a/cmd/convertor/builder/overlaybd_builder.go +++ b/cmd/convertor/builder/overlaybd_builder.go @@ -45,8 +45,9 @@ const ( ) type overlaybdConvertResult struct { - desc specs.Descriptor - chainID string + desc specs.Descriptor + chainID string + fromDedup bool } type overlaybdBuilderEngine struct { @@ -94,12 +95,31 @@ func (e *overlaybdBuilderEngine) DownloadLayer(ctx context.Context, idx int) err func (e *overlaybdBuilderEngine) BuildLayer(ctx context.Context, idx int) error { layerDir := e.getLayerDir(idx) - alreadyConverted := false - // check if we used a previously converted layer to skip conversion - if _, err := os.Stat(path.Join(layerDir, commitFile)); err == nil { - alreadyConverted = true - } - if !alreadyConverted { + // If the layer is from dedup we should have a downloaded commit file + commitFilePresent := false + if _, err := os.Stat(path.Join(layerDir, commitFile)); err != nil { + if !os.IsNotExist(err) { + return fmt.Errorf("failed to check if layer %d is already present abort conversion: %w", idx, err) + } + // commit file is not present + } else { + commitFilePresent = true + logrus.Debugf("layer %d commit file detected", idx) + } + if e.overlaybdLayers[idx].fromDedup { + // check if the previously converted layer is present + if commitFilePresent { + logrus.Debugf("layer %d is from dedup", idx) + } else { + return fmt.Errorf("layer %d is from dedup but commit file is missing", idx) + } + } else { + // This should not happen, but if it does, we should fail the conversion or + // risk corrupting the image. + if commitFilePresent { + return fmt.Errorf("layer %d is not from dedup but commit file is present", idx) + } + mkfs := e.mkfs && (idx == 0) vsizeGB := 0 if idx == 0 { @@ -143,6 +163,12 @@ func (e *overlaybdBuilderEngine) UploadLayer(ctx context.Context, idx int) error if err != nil { return errors.Wrapf(err, "failed to get descriptor for layer %d", idx) } + if e.overlaybdLayers[idx].fromDedup { + // validate that the layer digests match if the layer is from dedup + if desc.Digest != e.overlaybdLayers[idx].desc.Digest { + return fmt.Errorf("layer %d digest mismatch, expected %s, got %s", idx, e.overlaybdLayers[idx].desc.Digest, desc.Digest) + } + } desc.MediaType = e.mediaTypeImageLayer() desc.Annotations = map[string]string{ label.OverlayBDVersion: version.OverlayBDVersionNumber, @@ -354,12 +380,25 @@ func (e *overlaybdBuilderEngine) StoreConvertedLayerDetails(ctx context.Context, if e.db == nil { return nil } + if e.overlaybdLayers[idx].fromDedup { + logrus.Infof("layer %d skip storing conversion details", idx) + return nil + } return e.db.CreateLayerEntry(ctx, e.host, e.repository, e.overlaybdLayers[idx].desc.Digest, e.overlaybdLayers[idx].chainID, e.overlaybdLayers[idx].desc.Size) } func (e *overlaybdBuilderEngine) DownloadConvertedLayer(ctx context.Context, idx int, desc specs.Descriptor) error { targetFile := path.Join(e.getLayerDir(idx), commitFile) - return downloadLayer(ctx, e.fetcher, targetFile, desc, true) + err := downloadLayer(ctx, e.fetcher, targetFile, desc, true) + if err != nil { + // We should remove the commit file if the download failed to allow for fallback conversion + os.Remove(targetFile) // Remove any file that may have failed to download + return errors.Wrapf(err, "failed to download layer %d", idx) + } + // Mark that this layer is from dedup + e.overlaybdLayers[idx].fromDedup = true + e.overlaybdLayers[idx].desc = desc // If we are deduping store the dedup descriptor for later validation + return nil } func (e *overlaybdBuilderEngine) Cleanup() { diff --git a/cmd/convertor/builder/overlaybd_builder_test.go b/cmd/convertor/builder/overlaybd_builder_test.go index 4259e7d4..7a27aab2 100644 --- a/cmd/convertor/builder/overlaybd_builder_test.go +++ b/cmd/convertor/builder/overlaybd_builder_test.go @@ -19,9 +19,13 @@ package builder import ( "context" "fmt" + "os" + "path" + "path/filepath" "testing" testingresources "github.com/containerd/accelerated-container-image/cmd/convertor/testingresources" + sn "github.com/containerd/accelerated-container-image/pkg/types" "github.com/containerd/containerd/errdefs" "github.com/containerd/containerd/images" _ "github.com/containerd/containerd/pkg/testutil" // Handle custom root flag @@ -225,12 +229,313 @@ func Test_overlaybd_builder_CheckForConvertedManifest(t *testing.T) { func Test_overlaybd_builder_StoreConvertedLayerDetails(t *testing.T) { ctx := context.Background() base := &builderEngineBase{ - db: nil, + db: nil, + repository: "hello-world", + host: "sample.localstore.io", } + e := &overlaybdBuilderEngine{ builderEngineBase: base, + overlaybdLayers: []overlaybdConvertResult{ + { + fromDedup: true, + // TODO: Maybe change this for an actually converted layer in the future + desc: v1.Descriptor{ + Digest: testingresources.DockerV2_Manifest_Simple_Layer_0_Digest, + Size: testingresources.DockerV2_Manifest_Simple_Layer_0_Size, + MediaType: v1.MediaTypeImageLayerGzip, + }, + chainID: "fake-chain-id", + }, + }, + } + e.manifest = v1.Manifest{ + Layers: []v1.Descriptor{ + { + Digest: testingresources.DockerV2_Manifest_Simple_Layer_0_Digest, + Size: testingresources.DockerV2_Manifest_Simple_Layer_0_Size, + MediaType: v1.MediaTypeImageLayerGzip, + }, + }, + } + t.Run("No DB Present", func(t *testing.T) { + err := e.StoreConvertedLayerDetails(ctx, 0) + testingresources.Assert(t, err == nil, "StoreConvertedLayerDetails() returned an unexpected Error") + }) + + t.Run("Layer is marked as deduplicated, avoid storing", func(t *testing.T) { + base.db = testingresources.NewLocalDB() + + err := e.StoreConvertedLayerDetails(ctx, 0) + testingresources.Assert(t, err == nil, "StoreConvertedLayerDetails() returned an unexpected Error") + base.db.GetLayerEntryForRepo(ctx, e.host, e.repository, "fake-chain-id") + }) +} + +func Test_overlaybd_builder_BuildLayer_HandlesPreviouslyConvertedLayers(t *testing.T) { + ctx := context.Background() + resolver := testingresources.GetTestResolver(t, ctx) + fetcher := testingresources.GetTestFetcherFromResolver(t, ctx, resolver, testingresources.DockerV2_Manifest_Simple_Ref) + base := &builderEngineBase{ + fetcher: fetcher, + host: "sample.localstore.io", + repository: "hello-world", + } + // TODO: Maybe change this for an actually converted layer in the future + targetDesc := v1.Descriptor{ + Digest: testingresources.DockerV2_Manifest_Simple_Layer_0_Digest, + Size: testingresources.DockerV2_Manifest_Simple_Layer_0_Size, + MediaType: v1.MediaTypeImageLayerGzip, + } + config := &sn.OverlayBDBSConfig{ + Lowers: []sn.OverlayBDBSConfigLower{}, + ResultFile: "", + } + config.Lowers = append(config.Lowers, sn.OverlayBDBSConfigLower{ + File: overlaybdBaseLayer, + }) + + t.Run("Dedup working as expected", func(t *testing.T) { + e := &overlaybdBuilderEngine{ + builderEngineBase: base, + overlaybdConfig: config, + } + e.manifest.Layers = []v1.Descriptor{targetDesc} + tmpDir := t.TempDir() + e.workDir = tmpDir + e.overlaybdLayers = []overlaybdConvertResult{ + { + fromDedup: true, + }, + } + + // Try before setting the commit file. This will fail because create tool + // is not present but helps verify the fallback. Error type here depends + // on if the tool is present or not. + if err := e.BuildLayer(ctx, 0); err == nil { + t.Fatal("Expected an error but got none") + } + + // Simulate a commit file + if err := os.MkdirAll(e.getLayerDir(0), 0777); err != nil { + t.Fatal(err) + } + + // Try again with parent directory present + if err := e.BuildLayer(ctx, 0); err == nil { + t.Fatal("Expected an error but got none") + } + + file, err := os.Create(filepath.Join(e.getLayerDir(0), commitFile)) + if err != nil { + t.Fatal(err) + } + file.Close() + if err = e.BuildLayer(ctx, 0); err != nil { + t.Error(err) + } + }) + + // We attempt to clean any leftover files when we fail to download a dedup + // layer so this scenario is unlikely to happen but it's a good sanity check. + // In this case, we assume the commit file is present but invalid. + t.Run("Dedup left partial commit file", func(t *testing.T) { + e := &overlaybdBuilderEngine{ + builderEngineBase: base, + overlaybdConfig: config, + } + e.manifest.Layers = []v1.Descriptor{targetDesc} + tmpDir := t.TempDir() + e.workDir = tmpDir + e.overlaybdLayers = []overlaybdConvertResult{ + { + fromDedup: false, + }, + } + // Simulate a commit file + if err := os.MkdirAll(e.getLayerDir(0), 0777); err != nil { + t.Fatal(err) + } + file, err := os.Create(filepath.Join(e.getLayerDir(0), commitFile)) + if err != nil { + t.Fatal(err) + } + file.Close() + if err = e.BuildLayer(ctx, 0); err == nil { + t.Fatal("Expected an error but got none") + } else { + if err.Error() != "layer 0 is not from dedup but commit file is present" { + t.Fatalf("Unexpected error: %v", err) + } + } + }) + + // This is a scenario that should not happen but it's a good sanity check. + t.Run("Dedup but somehow failed to download commit file", func(t *testing.T) { + e := &overlaybdBuilderEngine{ + builderEngineBase: base, + overlaybdConfig: config, + } + e.manifest.Layers = []v1.Descriptor{targetDesc} + tmpDir := t.TempDir() + e.workDir = tmpDir + e.overlaybdLayers = []overlaybdConvertResult{ + { + fromDedup: true, + }, + } + // Simulate a commit file + if err := os.MkdirAll(e.getLayerDir(0), 0777); err != nil { + t.Fatal(err) + } + + if err := e.BuildLayer(ctx, 0); err == nil { + t.Fatal("Expected an error but got none") + } else { + if err.Error() != "layer 0 is from dedup but commit file is missing" { + t.Fatalf("Unexpected error: %v", err) + } + } + }) +} + +func Test_overlaybd_builder_DownloadConvertedLayer(t *testing.T) { + ctx := context.Background() + resolver := testingresources.GetTestResolver(t, ctx) + fetcher := testingresources.GetTestFetcherFromResolver(t, ctx, resolver, testingresources.DockerV2_Manifest_Simple_Ref) + base := &builderEngineBase{ + fetcher: fetcher, + host: "sample.localstore.io", + repository: "hello-world", } - // No DB Case - err := e.StoreConvertedLayerDetails(ctx, 0) - testingresources.Assert(t, err == nil, "StoreConvertedLayerDetails() returned an unexpected Error") + // TODO: Maybe change this for an actually converted layer in the future + targetDesc := v1.Descriptor{ + Digest: testingresources.DockerV2_Manifest_Simple_Layer_0_Digest, + Size: testingresources.DockerV2_Manifest_Simple_Layer_0_Size, + MediaType: v1.MediaTypeImageLayerGzip, + } + config := &sn.OverlayBDBSConfig{ + Lowers: []sn.OverlayBDBSConfigLower{}, + ResultFile: "", + } + config.Lowers = append(config.Lowers, sn.OverlayBDBSConfigLower{ + File: overlaybdBaseLayer, + }) + + t.Run("DownloadConvertedLayer Succeeds", func(t *testing.T) { + e := &overlaybdBuilderEngine{ + builderEngineBase: base, + overlaybdConfig: config, + } + e.manifest.Layers = []v1.Descriptor{targetDesc} + tmpDir := t.TempDir() + e.workDir = tmpDir + e.overlaybdLayers = []overlaybdConvertResult{ + { + fromDedup: false, + }, + } + + if err := e.DownloadConvertedLayer(ctx, 0, targetDesc); err != nil { + t.Fatalf("DownloadConvertedLayer() failed with error: %v", err) + } + + // Check if the commit file is present + if _, err := os.Stat(filepath.Join(e.getLayerDir(0), commitFile)); err != nil { + t.Fatalf("Expected commit file but got: %v", err) + } + + testingresources.Assert(t, e.overlaybdLayers[0].fromDedup, "DownloadConvertedLayer() did not mark layer as dedup") + testingresources.Assert(t, e.overlaybdLayers[0].desc.Digest != "", "DownloadConvertedLayer() did not set the digest") + }) +} + +func Test_overlaybd_builder_UploadLayer(t *testing.T) { + ctx := context.Background() + targetManifest := "sample.localstore.io/hello-world:another" + resolver := testingresources.GetTestResolver(t, ctx) + fetcher := testingresources.GetTestFetcherFromResolver(t, ctx, resolver, testingresources.DockerV2_Manifest_Simple_Ref) + pusher := testingresources.GetTestPusherFromResolver(t, ctx, resolver, targetManifest) + base := &builderEngineBase{ + fetcher: fetcher, + host: "sample.localstore.io", + repository: "hello-world", + pusher: pusher, + } + // TODO: Maybe change this for an actually converted layer in the future + targetDesc := v1.Descriptor{ + Digest: testingresources.DockerV2_Manifest_Simple_Layer_0_Digest, + Size: testingresources.DockerV2_Manifest_Simple_Layer_0_Size, + MediaType: v1.MediaTypeImageLayerGzip, + } + config := &sn.OverlayBDBSConfig{ + Lowers: []sn.OverlayBDBSConfigLower{}, + ResultFile: "", + } + config.Lowers = append(config.Lowers, sn.OverlayBDBSConfigLower{ + File: overlaybdBaseLayer, + }) + + t.Run("UploadLayer Succeeds for non dedup layer", func(t *testing.T) { + e := &overlaybdBuilderEngine{ + builderEngineBase: base, + overlaybdConfig: config, + } + e.manifest.Layers = []v1.Descriptor{targetDesc} + tmpDir := t.TempDir() + e.workDir = tmpDir + e.overlaybdLayers = []overlaybdConvertResult{ + { + fromDedup: false, + }, + } + // Get a commit file (We are using the downloadConvertedLayer here just to get the file) + if err := e.DownloadConvertedLayer(ctx, 0, targetDesc); err != nil { + t.Fatalf("DownloadConvertedLayer() failed with error: %v", err) + } + e.overlaybdLayers[0].fromDedup = false // Reset the flag to simulate a non dedup layer + + if err := e.UploadLayer(ctx, 0); err != nil { + t.Fatalf("UploadLayer() failed with error: %v", err) + } + }) + + t.Run("UploadLayer Fails for non matching dedup layer", func(t *testing.T) { + e := &overlaybdBuilderEngine{ + builderEngineBase: base, + overlaybdConfig: config, + } + e.manifest.Layers = []v1.Descriptor{targetDesc} + tmpDir := t.TempDir() + e.workDir = tmpDir + e.overlaybdLayers = []overlaybdConvertResult{ + { + desc: targetDesc, // Set the desc to the targetDesc to simulate a mismatch + fromDedup: true, + }, + } + // Simulate a corrupted commit file + if err := os.MkdirAll(e.getLayerDir(0), 0777); err != nil { + t.Fatal(err) + } + file, err := os.Create(filepath.Join(e.getLayerDir(0), commitFile)) + if err != nil { + t.Fatal(err) + } + file.WriteString("corrupted overlaybdfile simulation") + file.Close() + + layerDir := e.getLayerDir(0) + corruptedDesc, err := getFileDesc(path.Join(layerDir, commitFile), false) + if err != nil { + t.Fatalf("getFileDesc() failed with error: %v", err) + } + + // Upload should fail because the converted layer is not as expected + if err = e.UploadLayer(ctx, 0); err != nil { + if err.Error() != fmt.Sprintf("layer %d digest mismatch, expected %s, got %s", 0, targetDesc.Digest, corruptedDesc.Digest) { + t.Fatalf("UploadLayer() failed with unexpected error: %v", err) + } + } + }) }