Skip to content

Commit fca702d

Browse files
thaJeztahthaJeztah
authored
Merge pull request from GHSA-xw73-rw38-6vjc
[24.0 backport] image/cache: Restrict cache candidates to locally built images
2 parents f78a772 + 44e6f3d commit fca702d

12 files changed

Lines changed: 266 additions & 57 deletions

File tree

‎builder/builder.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"github.com/docker/docker/image"
1616
"github.com/docker/docker/layer"
1717
"github.com/opencontainers/go-digest"
18+
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
1819
)
1920

2021
const (
@@ -89,7 +90,7 @@ type ImageCacheBuilder interface {
8990
type ImageCache interface {
9091
// GetCache returns a reference to a cached image whose parent equals `parent`
9192
// and runconfig equals `cfg`. A cache miss is expected to return an empty ID and a nil error.
92-
GetCache(parentID string, cfg *container.Config) (imageID string, err error)
93+
GetCache(parentID string, cfg *container.Config, platform ocispec.Platform) (imageID string, err error)
9394
}
9495

9596
// Image represents a Docker image used by the builder.

‎builder/dockerfile/copy.go‎

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"net/url"
99
"os"
1010
"path/filepath"
11-
"runtime"
1211
"sort"
1312
"strings"
1413
"time"
@@ -74,7 +73,7 @@ type copier struct {
7473
source builder.Source
7574
pathCache pathCache
7675
download sourceDownloader
77-
platform *ocispec.Platform
76+
platform ocispec.Platform
7877
// for cleanup. TODO: having copier.cleanup() is error prone and hard to
7978
// follow. Code calling performCopy should manage the lifecycle of its params.
8079
// Copier should take override source as input, not imageMount.
@@ -83,19 +82,7 @@ type copier struct {
8382
}
8483

8584
func copierFromDispatchRequest(req dispatchRequest, download sourceDownloader, imageSource *imageMount) copier {
86-
platform := req.builder.platform
87-
if platform == nil {
88-
// May be nil if not explicitly set in API/dockerfile
89-
platform = &ocispec.Platform{}
90-
}
91-
if platform.OS == "" {
92-
// Default to the dispatch requests operating system if not explicit in API/dockerfile
93-
platform.OS = req.state.operatingSystem
94-
}
95-
if platform.OS == "" {
96-
// This is a failsafe just in case. Shouldn't be hit.
97-
platform.OS = runtime.GOOS
98-
}
85+
platform := req.builder.getPlatform(req.state)
9986

10087
return copier{
10188
source: req.source,

‎builder/dockerfile/dispatchers.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -349,9 +349,16 @@ func dispatchRun(ctx context.Context, d dispatchRequest, c *instructions.RunComm
349349
saveCmd = prependEnvOnCmd(d.state.buildArgs, buildArgs, cmdFromArgs)
350350
}
351351

352+
cacheArgsEscaped := argsEscaped
353+
// ArgsEscaped is not persisted in the committed image on Windows.
354+
// Use the original from previous build steps for cache probing.
355+
if d.state.operatingSystem == "windows" {
356+
cacheArgsEscaped = stateRunConfig.ArgsEscaped
357+
}
358+
352359
runConfigForCacheProbe := copyRunConfig(stateRunConfig,
353360
withCmd(saveCmd),
354-
withArgsEscaped(argsEscaped),
361+
withArgsEscaped(cacheArgsEscaped),
355362
withEntrypointOverride(saveCmd, nil))
356363
if hit, err := d.builder.probeCache(d.state, runConfigForCacheProbe); err != nil || hit {
357364
return err

‎builder/dockerfile/imageprobe.go‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,15 @@ import (
55

66
"github.com/docker/docker/api/types/container"
77
"github.com/docker/docker/builder"
8+
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
89
"github.com/sirupsen/logrus"
910
)
1011

1112
// ImageProber exposes an Image cache to the Builder. It supports resetting a
1213
// cache.
1314
type ImageProber interface {
1415
Reset(ctx context.Context) error
15-
Probe(parentID string, runConfig *container.Config) (string, error)
16+
Probe(parentID string, runConfig *container.Config, platform ocispec.Platform) (string, error)
1617
}
1718

1819
type resetFunc func(context.Context) (builder.ImageCache, error)
@@ -51,11 +52,11 @@ func (c *imageProber) Reset(ctx context.Context) error {
5152

5253
// Probe checks if cache match can be found for current build instruction.
5354
// It returns the cachedID if there is a hit, and the empty string on miss
54-
func (c *imageProber) Probe(parentID string, runConfig *container.Config) (string, error) {
55+
func (c *imageProber) Probe(parentID string, runConfig *container.Config, platform ocispec.Platform) (string, error) {
5556
if c.cacheBusted {
5657
return "", nil
5758
}
58-
cacheID, err := c.cache.GetCache(parentID, runConfig)
59+
cacheID, err := c.cache.GetCache(parentID, runConfig, platform)
5960
if err != nil {
6061
return "", err
6162
}
@@ -74,6 +75,6 @@ func (c *nopProber) Reset(ctx context.Context) error {
7475
return nil
7576
}
7677

77-
func (c *nopProber) Probe(_ string, _ *container.Config) (string, error) {
78+
func (c *nopProber) Probe(_ string, _ *container.Config, _ ocispec.Platform) (string, error) {
7879
return "", nil
7980
}

‎builder/dockerfile/internals.go‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"fmt"
1111
"strings"
1212

13+
"github.com/containerd/containerd/platforms"
1314
"github.com/docker/docker/api/types"
1415
"github.com/docker/docker/api/types/backend"
1516
"github.com/docker/docker/api/types/container"
@@ -328,7 +329,7 @@ func getShell(c *container.Config, os string) []string {
328329
}
329330

330331
func (b *Builder) probeCache(dispatchState *dispatchState, runConfig *container.Config) (bool, error) {
331-
cachedID, err := b.imageProber.Probe(dispatchState.imageID, runConfig)
332+
cachedID, err := b.imageProber.Probe(dispatchState.imageID, runConfig, b.getPlatform(dispatchState))
332333
if cachedID == "" || err != nil {
333334
return false, err
334335
}
@@ -388,3 +389,17 @@ func hostConfigFromOptions(options *types.ImageBuildOptions) *container.HostConf
388389
}
389390
return hc
390391
}
392+
393+
func (b *Builder) getPlatform(state *dispatchState) ocispec.Platform {
394+
// May be nil if not explicitly set in API/dockerfile
395+
out := platforms.DefaultSpec()
396+
if b.platform != nil {
397+
out = *b.platform
398+
}
399+
400+
if state.operatingSystem != "" {
401+
out.OS = state.operatingSystem
402+
}
403+
404+
return out
405+
}

‎builder/dockerfile/mockbackend_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/docker/docker/image"
1515
"github.com/docker/docker/layer"
1616
"github.com/opencontainers/go-digest"
17+
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
1718
)
1819

1920
// MockBackend implements the builder.Backend interface for unit testing
@@ -111,7 +112,7 @@ type mockImageCache struct {
111112
getCacheFunc func(parentID string, cfg *container.Config) (string, error)
112113
}
113114

114-
func (mic *mockImageCache) GetCache(parentID string, cfg *container.Config) (string, error) {
115+
func (mic *mockImageCache) GetCache(parentID string, cfg *container.Config, _ ocispec.Platform) (string, error) {
115116
if mic.getCacheFunc != nil {
116117
return mic.getCacheFunc(parentID, cfg)
117118
}

‎daemon/containerd/cache.go‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,9 @@ import (
88
"github.com/docker/docker/api/types/container"
99
imagetype "github.com/docker/docker/api/types/image"
1010
"github.com/docker/docker/builder"
11+
"github.com/docker/docker/errdefs"
1112
"github.com/docker/docker/image"
13+
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
1214
)
1315

1416
// MakeImageCache creates a stateful image cache.
@@ -29,16 +31,19 @@ type imageCache struct {
2931
c *ImageService
3032
}
3133

32-
func (ic *imageCache) GetCache(parentID string, cfg *container.Config) (imageID string, err error) {
34+
func (ic *imageCache) GetCache(parentID string, cfg *container.Config, platform ocispec.Platform) (imageID string, err error) {
3335
ctx := context.TODO()
3436

3537
if parentID == "" {
3638
// TODO handle "parentless" image cache lookups ("FROM scratch")
3739
return "", nil
3840
}
3941

40-
parent, err := ic.c.GetImage(ctx, parentID, imagetype.GetImageOpts{})
42+
parent, err := ic.c.GetImage(ctx, parentID, imagetype.GetImageOpts{Platform: &platform})
4143
if err != nil {
44+
if errdefs.IsNotFound(err) {
45+
return "", nil
46+
}
4247
return "", err
4348
}
4449

@@ -54,8 +59,11 @@ func (ic *imageCache) GetCache(parentID string, cfg *container.Config) (imageID
5459
}
5560

5661
for _, children := range children {
57-
childImage, err := ic.c.GetImage(ctx, children.String(), imagetype.GetImageOpts{})
62+
childImage, err := ic.c.GetImage(ctx, children.String(), imagetype.GetImageOpts{Platform: &platform})
5863
if err != nil {
64+
if errdefs.IsNotFound(err) {
65+
continue
66+
}
5967
return "", err
6068
}
6169

‎daemon/images/image_builder.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,9 @@ func (i *ImageService) CreateImage(ctx context.Context, config []byte, parent st
257257
return nil, errors.Wrapf(err, "failed to set parent %s", parent)
258258
}
259259
}
260+
if err := i.imageStore.SetBuiltLocally(id); err != nil {
261+
return nil, errors.Wrapf(err, "failed to mark image %s as built locally", id)
262+
}
260263

261264
return i.imageStore.Get(id)
262265
}

‎daemon/images/image_commit.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,9 @@ func (i *ImageService) CommitImage(ctx context.Context, c backend.CommitConfig)
6262
if err != nil {
6363
return "", err
6464
}
65+
if err := i.imageStore.SetBuiltLocally(id); err != nil {
66+
return "", err
67+
}
6568

6669
if c.ParentImageID != "" {
6770
if err := i.imageStore.SetParent(id, image.ID(c.ParentImageID)); err != nil {

‎image/cache/cache.go‎

Lines changed: 75 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,14 @@ import (
66
"reflect"
77
"strings"
88

9+
"github.com/containerd/containerd/platforms"
910
containertypes "github.com/docker/docker/api/types/container"
1011
"github.com/docker/docker/dockerversion"
1112
"github.com/docker/docker/image"
1213
"github.com/docker/docker/layer"
14+
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
1315
"github.com/pkg/errors"
16+
"github.com/sirupsen/logrus"
1417
)
1518

1619
// NewLocal returns a local image cache, based on parent chain
@@ -26,8 +29,8 @@ type LocalImageCache struct {
2629
}
2730

2831
// GetCache returns the image id found in the cache
29-
func (lic *LocalImageCache) GetCache(imgID string, config *containertypes.Config) (string, error) {
30-
return getImageIDAndError(getLocalCachedImage(lic.store, image.ID(imgID), config))
32+
func (lic *LocalImageCache) GetCache(imgID string, config *containertypes.Config, platform ocispec.Platform) (string, error) {
33+
return getImageIDAndError(getLocalCachedImage(lic.store, image.ID(imgID), config, platform))
3134
}
3235

3336
// New returns an image cache, based on history objects
@@ -51,8 +54,8 @@ func (ic *ImageCache) Populate(image *image.Image) {
5154
}
5255

5356
// GetCache returns the image id found in the cache
54-
func (ic *ImageCache) GetCache(parentID string, cfg *containertypes.Config) (string, error) {
55-
imgID, err := ic.localImageCache.GetCache(parentID, cfg)
57+
func (ic *ImageCache) GetCache(parentID string, cfg *containertypes.Config, platform ocispec.Platform) (string, error) {
58+
imgID, err := ic.localImageCache.GetCache(parentID, cfg, platform)
5659
if err != nil {
5760
return "", err
5861
}
@@ -215,7 +218,23 @@ func getImageIDAndError(img *image.Image, err error) (string, error) {
215218
// of the image with imgID, that had the same config when it was
216219
// created. nil is returned if a child cannot be found. An error is
217220
// returned if the parent image cannot be found.
218-
func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *containertypes.Config) (*image.Image, error) {
221+
func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *containertypes.Config, platform ocispec.Platform) (*image.Image, error) {
222+
if config == nil {
223+
return nil, nil
224+
}
225+
226+
isBuiltLocally := func(id image.ID) bool {
227+
builtLocally, err := imageStore.IsBuiltLocally(id)
228+
if err != nil {
229+
logrus.WithFields(logrus.Fields{
230+
"error": err,
231+
"id": id,
232+
}).Warn("failed to check if image was built locally")
233+
return false
234+
}
235+
return builtLocally
236+
}
237+
219238
// Loop on the children of the given image and check the config
220239
getMatch := func(siblings []image.ID) (*image.Image, error) {
221240
var match *image.Image
@@ -225,6 +244,25 @@ func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *contain
225244
return nil, fmt.Errorf("unable to find image %q", id)
226245
}
227246

247+
if !isBuiltLocally(id) {
248+
continue
249+
}
250+
251+
imgPlatform := ocispec.Platform{
252+
Architecture: img.Architecture,
253+
OS: img.OS,
254+
OSVersion: img.OSVersion,
255+
OSFeatures: img.OSFeatures,
256+
Variant: img.Variant,
257+
}
258+
// Discard old linux/amd64 images with empty platform.
259+
if imgPlatform.OS == "" && imgPlatform.Architecture == "" {
260+
continue
261+
}
262+
if !platforms.OnlyStrict(platform).Match(imgPlatform) {
263+
continue
264+
}
265+
228266
if compare(&img.ContainerConfig, config) {
229267
// check for the most up to date match
230268
if match == nil || match.Created.Before(img.Created) {
@@ -238,11 +276,29 @@ func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *contain
238276
// In this case, this is `FROM scratch`, which isn't an actual image.
239277
if imgID == "" {
240278
images := imageStore.Map()
279+
241280
var siblings []image.ID
242281
for id, img := range images {
243-
if img.Parent == imgID {
244-
siblings = append(siblings, id)
282+
if img.Parent != "" {
283+
continue
245284
}
285+
286+
if !isBuiltLocally(id) {
287+
continue
288+
}
289+
290+
// Do a quick initial filter on the Cmd to avoid adding all
291+
// non-local images with empty parent to the siblings slice and
292+
// performing a full config compare.
293+
//
294+
// config.Cmd is set to the current Dockerfile instruction so we
295+
// check it against the img.ContainerConfig.Cmd which is the
296+
// command of the last layer.
297+
if !strSliceEqual(img.ContainerConfig.Cmd, config.Cmd) {
298+
continue
299+
}
300+
301+
siblings = append(siblings, id)
246302
}
247303
return getMatch(siblings)
248304
}
@@ -251,3 +307,15 @@ func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *contain
251307
siblings := imageStore.Children(imgID)
252308
return getMatch(siblings)
253309
}
310+
311+
func strSliceEqual(a, b []string) bool {
312+
if len(a) != len(b) {
313+
return false
314+
}
315+
for i := 0; i < len(a); i++ {
316+
if a[i] != b[i] {
317+
return false
318+
}
319+
}
320+
return true
321+
}

0 commit comments

Comments
 (0)