From 64f90d58fcb634c02560d6ab6b09951d6a0cc2ef Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 26 Jun 2024 20:49:19 +0000 Subject: [PATCH] Add high-level validations for invalid image commands Signed-off-by: Brian Goff --- load.go | 10 ++++++-- load_test.go | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++ source.go | 39 ++++++++++++++++++++++++++----- source_test.go | 40 ++++++++++++++++++++++++++++++++ 4 files changed, 143 insertions(+), 8 deletions(-) diff --git a/load.go b/load.go index 75d19feb..fd90ea11 100644 --- a/load.go +++ b/load.go @@ -142,9 +142,15 @@ func (s *Source) validate(failContext ...string) (retErr error) { } if s.DockerImage.Cmd != nil { + // If someone *really* wants to extract the entire rootfs, they need to say so explicitly. + // We won't fill this in for them, particularly because this is almost certainly not the user's intent. + if s.Path == "" { + retErr = goerrors.Join(retErr, errors.Errorf("source path cannot be empty")) + } + for _, mnt := range s.DockerImage.Cmd.Mounts { - if mnt.Dest == s.Path { - return errors.Wrap(errInvalidMountConfig, "") + if err := mnt.validate(s.Path); err != nil { + retErr = goerrors.Join(retErr, err) } if err := mnt.Spec.validate("docker image source with ref", "'"+s.DockerImage.Ref+"'"); err != nil { retErr = goerrors.Join(retErr, err) diff --git a/load_test.go b/load_test.go index 445b5efa..6c5c5821 100644 --- a/load_test.go +++ b/load_test.go @@ -236,6 +236,68 @@ func TestSourceValidation(t *testing.T) { Generate: []*SourceGenerator{{Gomod: &GeneratorGomod{}}}, }, }, + { + title: "docker images with cmd source must specify a path to extract", + expectErr: true, + src: Source{ + Path: "", + DockerImage: &SourceDockerImage{ + Ref: "notexists:latest", + Cmd: &Command{ + Steps: []*BuildStep{ + {Command: ":"}, + }, + }, + }, + }, + }, + { + title: "cmd souce mount dest must not be /", + expectErr: true, + src: Source{ + Path: "/foo", + DockerImage: &SourceDockerImage{ + Ref: "notexists:latest", + Cmd: &Command{ + Steps: []*BuildStep{ + {Command: ":"}, + }, + Mounts: []SourceMount{ + { + Dest: "/", + Spec: Source{ + Inline: &SourceInline{ + File: &SourceInlineFile{}, + }, + }, + }, + }, + }, + }, + }, + }, + { + title: "cmd source mount dest must not be a descendent of the extracted source path", + expectErr: true, + src: Source{ + Path: "/foo", + DockerImage: &SourceDockerImage{ + Ref: "notexists:latest", + Cmd: &Command{ + Mounts: []SourceMount{ + { + Dest: "/foo", + Spec: Source{ + Inline: &SourceInline{ + File: &SourceInlineFile{}, + }, + }, + }, + }, + }, + }, + }, + }, } for _, tc := range cases { diff --git a/source.go b/source.go index 2f75593f..6c25f9e9 100644 --- a/source.go +++ b/source.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "path/filepath" + "strings" "github.com/moby/buildkit/client/llb" "github.com/moby/buildkit/identity" @@ -30,6 +31,7 @@ func getFilter(src Source, forMount bool, opts ...llb.ConstraintsOpt) llb.StateO // if we're using a mount for these sources, the mount will handle path extraction path = "/" } + switch { case src.HTTP != nil, src.Git != nil, @@ -196,6 +198,35 @@ func (s *Source) AsMount(name string, sOpt SourceOpts, opts ...llb.ConstraintsOp var errInvalidMountConfig = errors.New("invalid mount config") +func pathHasPrefix(s string, prefix string) bool { + if s == prefix { + return true + } + + s = filepath.Clean(s) + prefix = filepath.Clean(prefix) + + if s == prefix { + return true + } + + if strings.HasPrefix(s, prefix+"/") { + return true + } + return false +} + +func (m *SourceMount) validate(root string) error { + if m.Dest == "/" { + return errors.Wrap(errInvalidMountConfig, "mount destination must not be \"/\"") + } + if root != "/" && pathHasPrefix(m.Dest, root) { + // We cannot support this as the base mount for subPath will shadow the mount being done here. + return errors.Wrapf(errInvalidMountConfig, "mount destination (%s) must not be a descendent of the target source path (%s)", m.Dest, root) + } + return nil +} + // must not be called with a nil cmd pointer // subPath must be a valid non-empty path func generateSourceFromImage(st llb.State, cmd *Command, sOpts SourceOpts, subPath string, opts ...llb.ConstraintsOpt) (llb.State, error) { @@ -220,12 +251,8 @@ func generateSourceFromImage(st llb.State, cmd *Command, sOpts SourceOpts, subPa baseRunOpts := []llb.RunOption{CacheDirsToRunOpt(cmd.CacheDirs, "", "")} for _, src := range cmd.Mounts { - if src.Dest == "/" { - return llb.Scratch(), errors.Wrap(errInvalidMountConfig, "mount destination must not be \"/\"") - } - if subPath != "/" && filepath.HasPrefix(src.Dest, subPath) { - // We cannot support this as the base mount for subPath will shadow the mount being done here. - return llb.Scratch(), errors.Wrapf(errInvalidMountConfig, "mount destination (%s) must not be a descendent of the target source path (%s)", src.Dest, subPath) + if err := src.validate(subPath); err != nil { + return llb.Scratch(), err } srcSt, err := src.Spec.AsMount(src.Dest, sOpts, opts...) diff --git a/source_test.go b/source_test.go index edbd49ab..5933b973 100644 --- a/source_test.go +++ b/source_test.go @@ -1041,3 +1041,43 @@ func (stubMetaResolver) ResolveImageConfig(ctx context.Context, ref string, opt } return ref, "", dt, nil } + +func Test_pathHasPrefix(t *testing.T) { + type testCase struct { + path string + prefix string + expect bool + } + cases := []testCase{ + {"/foo", "/foobar", false}, + {"/foo", "/foo", true}, + {"/foo/", "/foo", true}, + {"/foo/", "/foo/", true}, + {"//foo", "/foo", true}, + {"//foo/", "/foo", true}, + {"/foo/bar", "/foo", true}, + {"/foo/bar/", "/foo", true}, + {"/foo/bar/", "/foo/", true}, + {"/foo/bar", "/foo/", true}, + {"/foo/bar", "/bar", false}, + {"/foo/bar", "/foo/bar/baz", false}, + {"/foo/bar/baz", "/foo/bar", true}, + {"/foo//bar", "/foo", true}, + {"/foo//bar/", "/foo", true}, + {"/foo//bar/", "/foo/", true}, + {"/foo//bar/", "/foo//", true}, + } + + // Replace / char which is special for go tests with something less special. + forTestName := func(s string) string { + return strings.Replace(s, "/", "__", -1) + } + + for _, tc := range cases { + name := fmt.Sprintf("path=%s,prefix=%s", forTestName(tc.path), forTestName(tc.prefix)) + t.Run(name, func(t *testing.T) { + hasPrefix := pathHasPrefix(tc.path, tc.prefix) + assert.Equal(t, hasPrefix, tc.expect) + }) + } +}