Merge pull request from GHSA-xw73-rw38-6vjc

[25.0 backport] image/cache: Restrict cache candidates to locally built images

builder/builder.go

@@ -14,6 +14,7 @@ import (
 	"github.com/docker/docker/image"
 	"github.com/docker/docker/layer"
 	"github.com/opencontainers/go-digest"
+	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 )
 
 const (
@@ -85,7 +86,7 @@ type ImageCacheBuilder interface {
 type ImageCache interface {
 	// GetCache returns a reference to a cached image whose parent equals `parent`
 	// and runconfig equals `cfg`. A cache miss is expected to return an empty ID and a nil error.
-	GetCache(parentID string, cfg *container.Config) (imageID string, err error)
+	GetCache(parentID string, cfg *container.Config, platform ocispec.Platform) (imageID string, err error)
 }
 
 // Image represents a Docker image used by the builder.

builder/dockerfile/copy.go

@@ -8,7 +8,6 @@ import (
 	"net/url"
 	"os"
 	"path/filepath"
-	"runtime"
 	"sort"
 	"strings"
 	"time"
@@ -74,7 +73,7 @@ type copier struct {
 	source      builder.Source
 	pathCache   pathCache
 	download    sourceDownloader
-	platform    *ocispec.Platform
+	platform    ocispec.Platform
 	// for cleanup. TODO: having copier.cleanup() is error prone and hard to
 	// follow. Code calling performCopy should manage the lifecycle of its params.
 	// Copier should take override source as input, not imageMount.
@@ -83,19 +82,7 @@ type copier struct {
 }
 
 func copierFromDispatchRequest(req dispatchRequest, download sourceDownloader, imageSource *imageMount) copier {
-	platform := req.builder.platform
-	if platform == nil {
-		// May be nil if not explicitly set in API/dockerfile
-		platform = &ocispec.Platform{}
-	}
-	if platform.OS == "" {
-		// Default to the dispatch requests operating system if not explicit in API/dockerfile
-		platform.OS = req.state.operatingSystem
-	}
-	if platform.OS == "" {
-		// This is a failsafe just in case. Shouldn't be hit.
-		platform.OS = runtime.GOOS
-	}
+	platform := req.builder.getPlatform(req.state)
 
 	return copier{
 		source:      req.source,

builder/dockerfile/dispatchers.go

@@ -348,9 +348,16 @@ func dispatchRun(ctx context.Context, d dispatchRequest, c *instructions.RunComm
 		saveCmd = prependEnvOnCmd(d.state.buildArgs, buildArgs, cmdFromArgs)
 	}
 
+	cacheArgsEscaped := argsEscaped
+	// ArgsEscaped is not persisted in the committed image on Windows.
+	// Use the original from previous build steps for cache probing.
+	if d.state.operatingSystem == "windows" {
+		cacheArgsEscaped = stateRunConfig.ArgsEscaped
+	}
+
 	runConfigForCacheProbe := copyRunConfig(stateRunConfig,
 		withCmd(saveCmd),
-		withArgsEscaped(argsEscaped),
+		withArgsEscaped(cacheArgsEscaped),
 		withEntrypointOverride(saveCmd, nil))
 	if hit, err := d.builder.probeCache(d.state, runConfigForCacheProbe); err != nil || hit {
 		return err

builder/dockerfile/imageprobe.go

@@ -6,13 +6,14 @@ import (
 	"github.com/containerd/log"
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/builder"
+	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 )
 
 // ImageProber exposes an Image cache to the Builder. It supports resetting a
 // cache.
 type ImageProber interface {
 	Reset(ctx context.Context) error
-	Probe(parentID string, runConfig *container.Config) (string, error)
+	Probe(parentID string, runConfig *container.Config, platform ocispec.Platform) (string, error)
 }
 
 type resetFunc func(context.Context) (builder.ImageCache, error)
@@ -51,11 +52,11 @@ func (c *imageProber) Reset(ctx context.Context) error {
 
 // Probe checks if cache match can be found for current build instruction.
 // It returns the cachedID if there is a hit, and the empty string on miss
-func (c *imageProber) Probe(parentID string, runConfig *container.Config) (string, error) {
+func (c *imageProber) Probe(parentID string, runConfig *container.Config, platform ocispec.Platform) (string, error) {
 	if c.cacheBusted {
 		return "", nil
 	}
-	cacheID, err := c.cache.GetCache(parentID, runConfig)
+	cacheID, err := c.cache.GetCache(parentID, runConfig, platform)
 	if err != nil {
 		return "", err
 	}
@@ -74,6 +75,6 @@ func (c *nopProber) Reset(ctx context.Context) error {
 	return nil
 }
 
-func (c *nopProber) Probe(_ string, _ *container.Config) (string, error) {
+func (c *nopProber) Probe(_ string, _ *container.Config, _ ocispec.Platform) (string, error) {
 	return "", nil
 }

builder/dockerfile/internals.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/containerd/containerd/platforms"
 	"github.com/containerd/log"
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/backend"
@@ -328,7 +329,7 @@ func getShell(c *container.Config, os string) []string {
 }
 
 func (b *Builder) probeCache(dispatchState *dispatchState, runConfig *container.Config) (bool, error) {
-	cachedID, err := b.imageProber.Probe(dispatchState.imageID, runConfig)
+	cachedID, err := b.imageProber.Probe(dispatchState.imageID, runConfig, b.getPlatform(dispatchState))
 	if cachedID == "" || err != nil {
 		return false, err
 	}
@@ -388,3 +389,17 @@ func hostConfigFromOptions(options *types.ImageBuildOptions) *container.HostConf
 	}
 	return hc
 }
+
+func (b *Builder) getPlatform(state *dispatchState) ocispec.Platform {
+	// May be nil if not explicitly set in API/dockerfile
+	out := platforms.DefaultSpec()
+	if b.platform != nil {
+		out = *b.platform
+	}
+
+	if state.operatingSystem != "" {
+		out.OS = state.operatingSystem
+	}
+
+	return out
+}

builder/dockerfile/mockbackend_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/docker/docker/image"
 	"github.com/docker/docker/layer"
 	"github.com/opencontainers/go-digest"
+	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 )
 
 // MockBackend implements the builder.Backend interface for unit testing
@@ -106,7 +107,7 @@ type mockImageCache struct {
 	getCacheFunc func(parentID string, cfg *container.Config) (string, error)
 }
 
-func (mic *mockImageCache) GetCache(parentID string, cfg *container.Config) (string, error) {
+func (mic *mockImageCache) GetCache(parentID string, cfg *container.Config, _ ocispec.Platform) (string, error) {
 	if mic.getCacheFunc != nil {
 		return mic.getCacheFunc(parentID, cfg)
 	}

daemon/containerd/cache.go

@@ -11,6 +11,7 @@ import (
 	"github.com/docker/docker/builder"
 	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/image"
+	"github.com/docker/docker/image/cache"
 	"github.com/opencontainers/go-digest"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 )
@@ -53,7 +54,7 @@ type localCache struct {
 	imageService *ImageService
 }
 
-func (ic *localCache) GetCache(parentID string, cfg *container.Config) (imageID string, err error) {
+func (ic *localCache) GetCache(parentID string, cfg *container.Config, platform ocispec.Platform) (imageID string, err error) {
 	ctx := context.TODO()
 
 	var children []image.ID
@@ -98,9 +99,12 @@ func (ic *localCache) GetCache(parentID string, cfg *container.Config) (imageID
 			return "", err
 		}
 
-		if isMatch(&cc, cfg) {
-			childImage, err := ic.imageService.GetImage(ctx, child.String(), imagetype.GetImageOpts{})
+		if cache.CompareConfig(&cc, cfg) {
+			childImage, err := ic.imageService.GetImage(ctx, child.String(), imagetype.GetImageOpts{Platform: &platform})
 			if err != nil {
+				if errdefs.IsNotFound(err) {
+					continue
+				}
 				return "", err
 			}
 
@@ -123,10 +127,10 @@ type imageCache struct {
 	lc           *localCache
 }
 
-func (ic *imageCache) GetCache(parentID string, cfg *container.Config) (imageID string, err error) {
+func (ic *imageCache) GetCache(parentID string, cfg *container.Config, platform ocispec.Platform) (imageID string, err error) {
 	ctx := context.TODO()
 
-	imgID, err := ic.lc.GetCache(parentID, cfg)
+	imgID, err := ic.lc.GetCache(parentID, cfg, platform)
 	if err != nil {
 		return "", err
 	}
@@ -142,7 +146,7 @@ func (ic *imageCache) GetCache(parentID string, cfg *container.Config) (imageID
 	lenHistory := 0
 
 	if parentID != "" {
-		parent, err = ic.imageService.GetImage(ctx, parentID, imagetype.GetImageOpts{})
+		parent, err = ic.imageService.GetImage(ctx, parentID, imagetype.GetImageOpts{Platform: &platform})
 		if err != nil {
 			return "", err
 		}
@@ -206,61 +210,3 @@ func (ic *imageCache) isParent(ctx context.Context, img *image.Image, parentID i
 	}
 	return ic.isParent(ctx, p, parentID)
 }
-
-// compare two Config struct. Do not compare the "Image" nor "Hostname" fields
-// If OpenStdin is set, then it differs
-func isMatch(a, b *container.Config) bool {
-	if a == nil || b == nil ||
-		a.OpenStdin || b.OpenStdin {
-		return false
-	}
-	if a.AttachStdout != b.AttachStdout ||
-		a.AttachStderr != b.AttachStderr ||
-		a.User != b.User ||
-		a.OpenStdin != b.OpenStdin ||
-		a.Tty != b.Tty {
-		return false
-	}
-
-	if len(a.Cmd) != len(b.Cmd) ||
-		len(a.Env) != len(b.Env) ||
-		len(a.Labels) != len(b.Labels) ||
-		len(a.ExposedPorts) != len(b.ExposedPorts) ||
-		len(a.Entrypoint) != len(b.Entrypoint) ||
-		len(a.Volumes) != len(b.Volumes) {
-		return false
-	}
-
-	for i := 0; i < len(a.Cmd); i++ {
-		if a.Cmd[i] != b.Cmd[i] {
-			return false
-		}
-	}
-	for i := 0; i < len(a.Env); i++ {
-		if a.Env[i] != b.Env[i] {
-			return false
-		}
-	}
-	for k, v := range a.Labels {
-		if v != b.Labels[k] {
-			return false
-		}
-	}
-	for k := range a.ExposedPorts {
-		if _, exists := b.ExposedPorts[k]; !exists {
-			return false
-		}
-	}
-
-	for i := 0; i < len(a.Entrypoint); i++ {
-		if a.Entrypoint[i] != b.Entrypoint[i] {
-			return false
-		}
-	}
-	for key := range a.Volumes {
-		if _, exists := b.Volumes[key]; !exists {
-			return false
-		}
-	}
-	return true
-}

daemon/images/image_builder.go

@@ -255,6 +255,9 @@ func (i *ImageService) CreateImage(ctx context.Context, config []byte, parent st
 			return nil, errors.Wrapf(err, "failed to set parent %s", parent)
 		}
 	}
+	if err := i.imageStore.SetBuiltLocally(id); err != nil {
+		return nil, errors.Wrapf(err, "failed to mark image %s as built locally", id)
+	}
 
 	return i.imageStore.Get(id)
 }

daemon/images/image_commit.go

@@ -62,6 +62,9 @@ func (i *ImageService) CommitImage(ctx context.Context, c backend.CommitConfig)
 	if err != nil {
 		return "", err
 	}
+	if err := i.imageStore.SetBuiltLocally(id); err != nil {
+		return "", err
+	}
 
 	if c.ParentImageID != "" {
 		if err := i.imageStore.SetParent(id, image.ID(c.ParentImageID)); err != nil {