Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid potential connectivity related layer corruption in userspace convertor #289

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion cmd/convertor/builder/builder_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -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
}

Expand Down
57 changes: 48 additions & 9 deletions cmd/convertor/builder/overlaybd_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,9 @@ const (
)

type overlaybdConvertResult struct {
desc specs.Descriptor
chainID string
desc specs.Descriptor
chainID string
fromDedup bool
}

type overlaybdBuilderEngine struct {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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() {
Expand Down
Loading
Loading