From 6e44cac05bad44ec868d9f6a57d65412d65e4c1c Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Thu, 27 Jul 2023 22:54:53 +0900 Subject: [PATCH 01/12] host builder nonregular files --- pkg/oci/containerize.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/pkg/oci/containerize.go b/pkg/oci/containerize.go index 6e5f43c77f..8f1041c127 100644 --- a/pkg/oci/containerize.go +++ b/pkg/oci/containerize.go @@ -7,7 +7,6 @@ import ( "fmt" "io" "os" - slashpath "path" "path/filepath" "strings" @@ -136,6 +135,8 @@ func newDataTarball(source, target string, ignored []string, verbose bool) error tw := tar.NewWriter(gw) defer tw.Close() + baseDir := "/func" + return filepath.Walk(source, func(path string, info os.FileInfo, err error) error { if err != nil { return err @@ -150,27 +151,26 @@ func newDataTarball(source, target string, ignored []string, verbose bool) error } } - header, err := tar.FileInfoHeader(info, info.Name()) - if err != nil { - return err + var link string + if info.Mode()&os.ModeSymlink == os.ModeSymlink { + if link, err = os.Readlink(path); err != nil { + return err + } } - relPath, err := filepath.Rel(source, path) + header, err := tar.FileInfoHeader(info, link) if err != nil { return err } - header.Name = slashpath.Join("/func", filepath.ToSlash(relPath)) - // TODO: should we set file timestamps to the build start time of cfg.t? - // header.ModTime = timestampArgument - - if err := tw.WriteHeader(header); err != nil { + header.Name = filepath.Join(baseDir, strings.TrimPrefix(path, source)) + if err = tw.WriteHeader(header); err != nil { return err } if verbose { fmt.Printf("→ %v \n", header.Name) } - if info.IsDir() { + if !info.Mode().IsRegular() { //nothing more to do for non-regular return nil } From be43ac3fcf11b3a3e09f1d69d7f94ee7e2995c14 Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Tue, 13 Feb 2024 15:51:26 +0900 Subject: [PATCH 02/12] disallow links outside root --- pkg/oci/containerize.go | 28 ++++++++++++++++++++++++++++ pkg/oci/containerize_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) create mode 100644 pkg/oci/containerize_test.go diff --git a/pkg/oci/containerize.go b/pkg/oci/containerize.go index 8f1041c127..21e73528b4 100644 --- a/pkg/oci/containerize.go +++ b/pkg/oci/containerize.go @@ -13,6 +13,7 @@ import ( v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/google/go-containerregistry/pkg/v1/tarball" "github.com/google/go-containerregistry/pkg/v1/types" + "github.com/pkg/errors" ) // languageLayerBuilder builds the layer for the given language whuch may @@ -156,6 +157,12 @@ func newDataTarball(source, target string, ignored []string, verbose bool) error if link, err = os.Readlink(path); err != nil { return err } + ok, err := isWithin(source, link) + if err != nil { + return errors.Wrap(err, "error checking if link is outside function root") + } else if !ok { + return fmt.Errorf("links outside Function root disallowed. Link %v references %v", path, link) + } } header, err := tar.FileInfoHeader(info, link) @@ -185,6 +192,27 @@ func newDataTarball(source, target string, ignored []string, verbose bool) error }) } +// isWithin returns true if the parent path has the child path within. +// If parent and child are the same target, this is considered within. +func isWithin(parent, child string) (bool, error) { + var err error + // Convert the paths to clean absolute paths. + if parent, err = filepath.Abs(filepath.Clean(parent)); err != nil { + return false, err + } + if child, err = filepath.Abs(filepath.Clean(child)); err != nil { + return false, err + } + // Calculate the relative path between them + relPath, err := filepath.Rel(parent, child) + if err != nil { + return false, err + } + // Child is not within parent if the relative path from parent to child + // requires .. + return !strings.HasPrefix(relPath, ".."), nil +} + // newCertLayer creates the shared data layer in the container file hierarchy and // returns both its descriptor and layer metadata. func newCertsLayer(cfg *buildConfig) (desc v1.Descriptor, layer v1.Layer, err error) { diff --git a/pkg/oci/containerize_test.go b/pkg/oci/containerize_test.go new file mode 100644 index 0000000000..c6298c3e2c --- /dev/null +++ b/pkg/oci/containerize_test.go @@ -0,0 +1,34 @@ +package oci + +import "testing" + +// Test_isWithin ensures that the isWithin method checks +// that various combinations of parent and child paths. +func Test_isWithin(t *testing.T) { + tests := []struct { + parent string + child string + want bool + name string + }{ + {"/", "/b", true, "Base case, a subdir of an absolute path"}, + {"/a", "/ab", false, "Ensure simple substring does not match"}, + {"./", ".", true, "Ensure links are both made absolute"}, + {"/a/b/../c", "/a/c/d/../", true, "Ensure the links are both sanitized"}, + {"/a", "/a/b/../../", false, "Ensure escaping the parent is a mismatch"}, + {"./", "../", false, "Ensure simple relative mismatch"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := isWithin(tt.parent, tt.child) + if err != nil { + t.Fatal(err) + } + if got != tt.want { + t.Log(tt.name) + t.Errorf("isWithin() = %v, want %v", got, tt.want) + } + }) + } + +} From ed15d98e5a9077757625f17a5a5b8377b5e07fc9 Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Wed, 14 Feb 2024 13:22:20 +0900 Subject: [PATCH 03/12] add back in correct path support for windows --- pkg/oci/containerize.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/pkg/oci/containerize.go b/pkg/oci/containerize.go index 21e73528b4..0d812c42e6 100644 --- a/pkg/oci/containerize.go +++ b/pkg/oci/containerize.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "os" + slashpath "path" "path/filepath" "strings" @@ -136,13 +137,12 @@ func newDataTarball(source, target string, ignored []string, verbose bool) error tw := tar.NewWriter(gw) defer tw.Close() - baseDir := "/func" - return filepath.Walk(source, func(path string, info os.FileInfo, err error) error { if err != nil { return err } + // Skip files explicitly ignored for _, v := range ignored { if info.Name() == v { if info.IsDir() { @@ -152,9 +152,10 @@ func newDataTarball(source, target string, ignored []string, verbose bool) error } } - var link string + // Error if links exist which reach outside of the function root if info.Mode()&os.ModeSymlink == os.ModeSymlink { - if link, err = os.Readlink(path); err != nil { + link, err := os.Readlink(path) + if err != nil { return err } ok, err := isWithin(source, link) @@ -165,13 +166,17 @@ func newDataTarball(source, target string, ignored []string, verbose bool) error } } - header, err := tar.FileInfoHeader(info, link) + header, err := tar.FileInfoHeader(info, info.Name()) if err != nil { return err } - header.Name = filepath.Join(baseDir, strings.TrimPrefix(path, source)) - if err = tw.WriteHeader(header); err != nil { + relPath, err := filepath.Rel(source, path) + if err != nil { + return err + } + header.Name = slashpath.Join("/func", filepath.ToSlash(relPath)) + if err := tw.WriteHeader(header); err != nil { return err } if verbose { From 5c42bbdbe19516208506bb149d7e6476975e9cb1 Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Fri, 16 Feb 2024 12:29:36 +0900 Subject: [PATCH 04/12] code review updates --- pkg/oci/containerize.go | 70 +++++++++++-------- pkg/oci/containerize_test.go | 47 ++++++++----- pkg/oci/testdata/test-links/...validName.lnk | 1 + pkg/oci/testdata/test-links/...validName.txt | 1 + pkg/oci/testdata/test-links/a.lnk | 1 + pkg/oci/testdata/test-links/a.txt | 1 + pkg/oci/testdata/test-links/absoluteLink | 1 + pkg/oci/testdata/test-links/b/c/linkToParent | 1 + .../test-links/b/linkOutsideRootsParent | 1 + .../testdata/test-links/b/linkToCurrentDir | 1 + pkg/oci/testdata/test-links/b/linkToRoot | 1 + .../testdata/test-links/b/linkToRootsParent | 1 + pkg/oci/testdata/test-links/linkToRoot | 1 + 13 files changed, 79 insertions(+), 49 deletions(-) create mode 120000 pkg/oci/testdata/test-links/...validName.lnk create mode 100644 pkg/oci/testdata/test-links/...validName.txt create mode 120000 pkg/oci/testdata/test-links/a.lnk create mode 100644 pkg/oci/testdata/test-links/a.txt create mode 120000 pkg/oci/testdata/test-links/absoluteLink create mode 120000 pkg/oci/testdata/test-links/b/c/linkToParent create mode 120000 pkg/oci/testdata/test-links/b/linkOutsideRootsParent create mode 120000 pkg/oci/testdata/test-links/b/linkToCurrentDir create mode 120000 pkg/oci/testdata/test-links/b/linkToRoot create mode 120000 pkg/oci/testdata/test-links/b/linkToRootsParent create mode 120000 pkg/oci/testdata/test-links/linkToRoot diff --git a/pkg/oci/containerize.go b/pkg/oci/containerize.go index 0d812c42e6..feb9cf2fcf 100644 --- a/pkg/oci/containerize.go +++ b/pkg/oci/containerize.go @@ -124,7 +124,7 @@ func newDataLayer(cfg *buildConfig) (desc v1.Descriptor, layer v1.Layer, err err return } -func newDataTarball(source, target string, ignored []string, verbose bool) error { +func newDataTarball(root, target string, ignored []string, verbose bool) error { targetFile, err := os.Create(target) if err != nil { return err @@ -137,7 +137,7 @@ func newDataTarball(source, target string, ignored []string, verbose bool) error tw := tar.NewWriter(gw) defer tw.Close() - return filepath.Walk(source, func(path string, info os.FileInfo, err error) error { + return filepath.Walk(root, func(path string, info os.FileInfo, err error) error { if err != nil { return err } @@ -152,18 +152,9 @@ func newDataTarball(source, target string, ignored []string, verbose bool) error } } - // Error if links exist which reach outside of the function root - if info.Mode()&os.ModeSymlink == os.ModeSymlink { - link, err := os.Readlink(path) - if err != nil { - return err - } - ok, err := isWithin(source, link) - if err != nil { - return errors.Wrap(err, "error checking if link is outside function root") - } else if !ok { - return fmt.Errorf("links outside Function root disallowed. Link %v references %v", path, link) - } + // Check for invalid links (absolute, outside of function root, etc) + if err := validateLink(root, path, info); err != nil { + return err } header, err := tar.FileInfoHeader(info, info.Name()) @@ -171,7 +162,7 @@ func newDataTarball(source, target string, ignored []string, verbose bool) error return err } - relPath, err := filepath.Rel(source, path) + relPath, err := filepath.Rel(root, path) if err != nil { return err } @@ -197,25 +188,44 @@ func newDataTarball(source, target string, ignored []string, verbose bool) error }) } -// isWithin returns true if the parent path has the child path within. -// If parent and child are the same target, this is considered within. -func isWithin(parent, child string) (bool, error) { - var err error - // Convert the paths to clean absolute paths. - if parent, err = filepath.Abs(filepath.Clean(parent)); err != nil { - return false, err +// validateLink returns an error if the given file is allowed given the +// - Is an absoute link +// - Is a link to something outside of the given function root +// - Errors obtaining this information +func validateLink(root, path string, info os.FileInfo) error { + if info.Mode()&os.ModeSymlink != os.ModeSymlink { + return nil // not a symlink + } + + // tgt is the raw target of the link. + // This path is either absolute or relative to the link's location. + tgt, err := os.Readlink(path) + if err != nil { + return err } - if child, err = filepath.Abs(filepath.Clean(child)); err != nil { - return false, err + + // Absolute links will not be correct when copied into the runtime + // container, because they are placed into path into '/func', + if filepath.IsAbs(tgt) { + return errors.New("project may not contain absolute links") } - // Calculate the relative path between them - relPath, err := filepath.Rel(parent, child) + + // Calculate the actual target of the link + // (relative to our current working directory) + lnkTgt := filepath.Join(filepath.Dir(path), tgt) + + // Calculate the relative path from the function's root to + // this actual target location + relLnkTgt, err := filepath.Rel(root, lnkTgt) if err != nil { - return false, err + return err } - // Child is not within parent if the relative path from parent to child - // requires .. - return !strings.HasPrefix(relPath, ".."), nil + + // Fail if this path is outside the function's root. + if strings.HasPrefix(relLnkTgt, ".."+string(filepath.Separator)) || relLnkTgt == ".." { + return errors.New("links must stay within project root") + } + return nil } // newCertLayer creates the shared data layer in the container file hierarchy and diff --git a/pkg/oci/containerize_test.go b/pkg/oci/containerize_test.go index c6298c3e2c..369d45fdd2 100644 --- a/pkg/oci/containerize_test.go +++ b/pkg/oci/containerize_test.go @@ -1,34 +1,43 @@ package oci -import "testing" +import ( + "os" + "path/filepath" + "testing" +) + +func Test_validateLink(t *testing.T) { + root := "testdata/test-links" -// Test_isWithin ensures that the isWithin method checks -// that various combinations of parent and child paths. -func Test_isWithin(t *testing.T) { tests := []struct { - parent string - child string - want bool - name string + path string // path of the file within test project root + valid bool // If it should be considered valid + name string // descriptive name of the test }{ - {"/", "/b", true, "Base case, a subdir of an absolute path"}, - {"/a", "/ab", false, "Ensure simple substring does not match"}, - {"./", ".", true, "Ensure links are both made absolute"}, - {"/a/b/../c", "/a/c/d/../", true, "Ensure the links are both sanitized"}, - {"/a", "/a/b/../../", false, "Ensure escaping the parent is a mismatch"}, - {"./", "../", false, "Ensure simple relative mismatch"}, + {"a.txt", true, "do not evaluate regular files"}, + {"a.lnk", true, "do not evaluate directories"}, + {"absoluteLink", false, "disallow absolute-path links"}, + {"a.lnk", true, "links to files within the root are allowed"}, + {"...validName.txt", true, "allow files with dot prefixes"}, + {"...validName.lnk", true, "allow links with target of dot prefixed names"}, + {"linkToRoot", true, "allow links to the project root"}, + {"b/linkToRoot", true, "allow links to the project root from within subdir"}, + {"b/linkToCurrentDir", true, "allow links to a subdirectory within the project"}, + {"b/linkToRootsParent", false, "disallow links to the project's immediate parent"}, + {"b/linkOutsideRootsParent", false, "disallow links outside project root and its parent"}, + {"b/c/linkToParent", true, "allow links up, but within project"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := isWithin(tt.parent, tt.child) + path := filepath.Join(root, tt.path) + info, err := os.Lstat(path) // filepath.Walk does not follow symlinks if err != nil { t.Fatal(err) } - if got != tt.want { - t.Log(tt.name) - t.Errorf("isWithin() = %v, want %v", got, tt.want) + err = validateLink(root, path, info) + if err == nil != tt.valid { + t.Fatalf("expected %v, got %v", tt.valid, err) } }) } - } diff --git a/pkg/oci/testdata/test-links/...validName.lnk b/pkg/oci/testdata/test-links/...validName.lnk new file mode 120000 index 0000000000..444926c99e --- /dev/null +++ b/pkg/oci/testdata/test-links/...validName.lnk @@ -0,0 +1 @@ +...validName.txt \ No newline at end of file diff --git a/pkg/oci/testdata/test-links/...validName.txt b/pkg/oci/testdata/test-links/...validName.txt new file mode 100644 index 0000000000..fa6198b097 --- /dev/null +++ b/pkg/oci/testdata/test-links/...validName.txt @@ -0,0 +1 @@ +validName.txt diff --git a/pkg/oci/testdata/test-links/a.lnk b/pkg/oci/testdata/test-links/a.lnk new file mode 120000 index 0000000000..1891a26c08 --- /dev/null +++ b/pkg/oci/testdata/test-links/a.lnk @@ -0,0 +1 @@ +./a.txt \ No newline at end of file diff --git a/pkg/oci/testdata/test-links/a.txt b/pkg/oci/testdata/test-links/a.txt new file mode 100644 index 0000000000..4ef30bbfe2 --- /dev/null +++ b/pkg/oci/testdata/test-links/a.txt @@ -0,0 +1 @@ +file a diff --git a/pkg/oci/testdata/test-links/absoluteLink b/pkg/oci/testdata/test-links/absoluteLink new file mode 120000 index 0000000000..28434a9033 --- /dev/null +++ b/pkg/oci/testdata/test-links/absoluteLink @@ -0,0 +1 @@ +/var/example/absolute/link \ No newline at end of file diff --git a/pkg/oci/testdata/test-links/b/c/linkToParent b/pkg/oci/testdata/test-links/b/c/linkToParent new file mode 120000 index 0000000000..a96aa0ea9d --- /dev/null +++ b/pkg/oci/testdata/test-links/b/c/linkToParent @@ -0,0 +1 @@ +.. \ No newline at end of file diff --git a/pkg/oci/testdata/test-links/b/linkOutsideRootsParent b/pkg/oci/testdata/test-links/b/linkOutsideRootsParent new file mode 120000 index 0000000000..a8a4f8c212 --- /dev/null +++ b/pkg/oci/testdata/test-links/b/linkOutsideRootsParent @@ -0,0 +1 @@ +../../.. \ No newline at end of file diff --git a/pkg/oci/testdata/test-links/b/linkToCurrentDir b/pkg/oci/testdata/test-links/b/linkToCurrentDir new file mode 120000 index 0000000000..945c9b46d6 --- /dev/null +++ b/pkg/oci/testdata/test-links/b/linkToCurrentDir @@ -0,0 +1 @@ +. \ No newline at end of file diff --git a/pkg/oci/testdata/test-links/b/linkToRoot b/pkg/oci/testdata/test-links/b/linkToRoot new file mode 120000 index 0000000000..a96aa0ea9d --- /dev/null +++ b/pkg/oci/testdata/test-links/b/linkToRoot @@ -0,0 +1 @@ +.. \ No newline at end of file diff --git a/pkg/oci/testdata/test-links/b/linkToRootsParent b/pkg/oci/testdata/test-links/b/linkToRootsParent new file mode 120000 index 0000000000..c25bddb6dd --- /dev/null +++ b/pkg/oci/testdata/test-links/b/linkToRootsParent @@ -0,0 +1 @@ +../.. \ No newline at end of file diff --git a/pkg/oci/testdata/test-links/linkToRoot b/pkg/oci/testdata/test-links/linkToRoot new file mode 120000 index 0000000000..945c9b46d6 --- /dev/null +++ b/pkg/oci/testdata/test-links/linkToRoot @@ -0,0 +1 @@ +. \ No newline at end of file From bd663526d1f97866ab0abbe1a0d012ede80c6575 Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Fri, 16 Feb 2024 13:40:04 +0900 Subject: [PATCH 05/12] host builder file test --- pkg/oci/builder_test.go | 106 +++++++++++++++++++++++++++++++--------- 1 file changed, 83 insertions(+), 23 deletions(-) diff --git a/pkg/oci/builder_test.go b/pkg/oci/builder_test.go index eb4086fc9b..c4a8da1b47 100644 --- a/pkg/oci/builder_test.go +++ b/pkg/oci/builder_test.go @@ -46,7 +46,64 @@ func TestBuilder_Build(t *testing.T) { last := path(f.Root, fn.RunDataDir, "builds", "last", "oci") - validateOCI(last, t) + validateOCIStructure(last, t) // validate it adheres to the basics of the OCI spec +} + +// TestBuilder_Files ensures that static files are added to the container +// image as expected. This includes template files, regular files and links. +func TestBuilder_Files(t *testing.T) { + root, done := Mktemp(t) + defer done() + + // Create a function with the default template + f, err := fn.New().Init(fn.Function{Root: root, Runtime: "go"}) + if err != nil { + t.Fatal(err) + } + + // Add a regular file + if err := os.WriteFile("a.txt", []byte("file a"), 0644); err != nil { + t.Fatal(err) + } + + // Links + var linkMode fs.FileMode + var linkExecutable bool + if runtime.GOOS != "windows" { + // Default: create a symlink + linkMode = fs.ModeSymlink + linkExecutable = true + if err := os.Symlink("a.txt", "a.lnk"); err != nil { + t.Fatal(err) + } + } else { + // Windows: create a copy + if err := os.WriteFile("a.lnk", []byte("file a"), 0644); err != nil { + t.Fatal(err) + } + } + + if err := NewBuilder("", true).Build(context.Background(), f, TestPlatforms); err != nil { + t.Fatal(err) + } + + expected := []fileInfo{ + {Path: "/etc/pki/tls/certs/ca-certificates.crt"}, + {Path: "/etc/ssl/certs/ca-certificates.crt"}, + {Path: "/func", Type: fs.ModeDir}, + {Path: "/func/README.md"}, + {Path: "/func/a.lnk", Executable: linkExecutable, Type: linkMode}, + {Path: "/func/a.txt"}, + {Path: "/func/f", Executable: true}, + {Path: "/func/func.yaml"}, + {Path: "/func/go.mod"}, + {Path: "/func/handle.go"}, + {Path: "/func/handle_test.go"}, + } + + last := path(f.Root, fn.RunDataDir, "builds", "last", "oci") + + validateOCIFiles(last, expected, t) } // TestBuilder_Concurrency @@ -145,9 +202,9 @@ type ImageIndex struct { } `json:"manifests"` } -// validateOCI performs a cursory check that the given path exists and +// validateOCIStructue performs a cursory check that the given path exists and // has the basics of an OCI compliant structure. -func validateOCI(path string, t *testing.T) { +func validateOCIStructure(path string, t *testing.T) { if _, err := os.Stat(path); err != nil { t.Fatalf("unable to stat output path. %v", path) return @@ -185,9 +242,24 @@ func validateOCI(path string, t *testing.T) { } if len(imageIndex.Manifests) < 1 { - t.Fatal("fewer manifests") + t.Fatal("no manifests") + } +} + +// validateOCIFiles ensures that the OCI image at path contains files with +// the given attributes. +func validateOCIFiles(path string, expected []fileInfo, t *testing.T) { + // Load the Image Index + bb, err := os.ReadFile(filepath.Join(path, "index.json")) + if err != nil { + t.Fatalf("failed to read index.json: %v", err) + } + var imageIndex ImageIndex + if err = json.Unmarshal(bb, &imageIndex); err != nil { + t.Fatalf("failed to parse index.json: %v", err) } + // Load the first manifest digest := strings.TrimPrefix(imageIndex.Manifests[0].Digest, "sha256:") manifestFile := filepath.Join(path, "blobs", "sha256", digest) manifestFileData, err := os.ReadFile(manifestFile) @@ -203,12 +275,6 @@ func validateOCI(path string, t *testing.T) { if err != nil { t.Fatal(err) } - - type fileInfo struct { - Path string - Type fs.FileMode - Executable bool - } var files []fileInfo for _, layer := range mf.Layers { @@ -247,19 +313,13 @@ func validateOCI(path string, t *testing.T) { return files[i].Path < files[j].Path }) - expectedFiles := []fileInfo{ - {Path: "/etc/pki/tls/certs/ca-certificates.crt"}, - {Path: "/etc/ssl/certs/ca-certificates.crt"}, - {Path: "/func", Type: fs.ModeDir}, - {Path: "/func/README.md"}, - {Path: "/func/f", Executable: true}, - {Path: "/func/func.yaml"}, - {Path: "/func/go.mod"}, - {Path: "/func/handle.go"}, - {Path: "/func/handle_test.go"}, - } - - if diff := cmp.Diff(expectedFiles, files); diff != "" { + if diff := cmp.Diff(expected, files); diff != "" { t.Error("files in oci differ from expectation (-want, +got):", diff) } } + +type fileInfo struct { + Path string + Type fs.FileMode + Executable bool +} From a7c8d1f00418d6aff786acd2803900df33abfe8b Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Fri, 16 Feb 2024 13:54:03 +0900 Subject: [PATCH 06/12] windows-specific absolute link check --- pkg/oci/containerize.go | 2 +- pkg/oci/containerize_test.go | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/pkg/oci/containerize.go b/pkg/oci/containerize.go index feb9cf2fcf..25edfd08d3 100644 --- a/pkg/oci/containerize.go +++ b/pkg/oci/containerize.go @@ -211,7 +211,7 @@ func validateLink(root, path string, info os.FileInfo) error { } // Calculate the actual target of the link - // (relative to our current working directory) + // (relative to the parent of the symlink) lnkTgt := filepath.Join(filepath.Dir(path), tgt) // Calculate the relative path from the function's root to diff --git a/pkg/oci/containerize_test.go b/pkg/oci/containerize_test.go index 369d45fdd2..cd587b06d4 100644 --- a/pkg/oci/containerize_test.go +++ b/pkg/oci/containerize_test.go @@ -3,6 +3,7 @@ package oci import ( "os" "path/filepath" + "runtime" "testing" ) @@ -40,4 +41,18 @@ func Test_validateLink(t *testing.T) { } }) } + // Run a windows-specific absolute path test + // Note this technically succeeds on unix systems, but wrapping in + // an runtime check seems like a good idea to make it more clear. + if runtime.GOOS != "windows" { + path := "c://some/absolute/path" + info, err := os.Lstat(path) + if err != nil { + t.Fatal(err) + } + err = validateLink(root, path, info) + if err == nil { + t.Fatal("absolute path should be invalid on windows") + } + } } From da57b3205c8952283c6dba7c1345b513686a7611 Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Fri, 16 Feb 2024 14:16:21 +0900 Subject: [PATCH 07/12] windows-specific test --- pkg/oci/containerize_test.go | 7 +++---- pkg/oci/testdata/test-links/absoluteLinkWindows | 1 + 2 files changed, 4 insertions(+), 4 deletions(-) create mode 120000 pkg/oci/testdata/test-links/absoluteLinkWindows diff --git a/pkg/oci/containerize_test.go b/pkg/oci/containerize_test.go index cd587b06d4..6817b32749 100644 --- a/pkg/oci/containerize_test.go +++ b/pkg/oci/containerize_test.go @@ -41,11 +41,10 @@ func Test_validateLink(t *testing.T) { } }) } + // Run a windows-specific absolute path test - // Note this technically succeeds on unix systems, but wrapping in - // an runtime check seems like a good idea to make it more clear. - if runtime.GOOS != "windows" { - path := "c://some/absolute/path" + if runtime.GOOS == "windows" { + path := "testdata/test-links/absoluteLinkWindows" info, err := os.Lstat(path) if err != nil { t.Fatal(err) diff --git a/pkg/oci/testdata/test-links/absoluteLinkWindows b/pkg/oci/testdata/test-links/absoluteLinkWindows new file mode 120000 index 0000000000..c3aaf3f5c3 --- /dev/null +++ b/pkg/oci/testdata/test-links/absoluteLinkWindows @@ -0,0 +1 @@ +c://some/absolute/path \ No newline at end of file From a0022b245bc5ad7b28b4851248a9bd558fc11a32 Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Fri, 16 Feb 2024 14:53:33 +0900 Subject: [PATCH 08/12] refactor Resolve the link target one level higher such that it can be used when creating the tar header without repetition. --- pkg/oci/builder_test.go | 3 ++- pkg/oci/containerize.go | 37 ++++++++++++++++++------------------ pkg/oci/containerize_test.go | 9 ++++----- 3 files changed, 24 insertions(+), 25 deletions(-) diff --git a/pkg/oci/builder_test.go b/pkg/oci/builder_test.go index c4a8da1b47..2c61732cc2 100644 --- a/pkg/oci/builder_test.go +++ b/pkg/oci/builder_test.go @@ -70,9 +70,10 @@ func TestBuilder_Files(t *testing.T) { var linkMode fs.FileMode var linkExecutable bool if runtime.GOOS != "windows" { - // Default: create a symlink + // Default case: use symlinks linkMode = fs.ModeSymlink linkExecutable = true + if err := os.Symlink("a.txt", "a.lnk"); err != nil { t.Fatal(err) } diff --git a/pkg/oci/containerize.go b/pkg/oci/containerize.go index 25edfd08d3..4a5f2896c3 100644 --- a/pkg/oci/containerize.go +++ b/pkg/oci/containerize.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "io" + "io/fs" "os" slashpath "path" "path/filepath" @@ -152,12 +153,14 @@ func newDataTarball(root, target string, ignored []string, verbose bool) error { } } - // Check for invalid links (absolute, outside of function root, etc) - if err := validateLink(root, path, info); err != nil { - return err + lnk := "" // if link, this will be used as the target + if info.Mode()&fs.ModeSymlink != 0 { + if lnk, err = validateLink(root, path, info); err != nil { + return err + } } - header, err := tar.FileInfoHeader(info, info.Name()) + header, err := tar.FileInfoHeader(info, lnk) if err != nil { return err } @@ -188,26 +191,22 @@ func newDataTarball(root, target string, ignored []string, verbose bool) error { }) } -// validateLink returns an error if the given file is allowed given the -// - Is an absoute link -// - Is a link to something outside of the given function root -// - Errors obtaining this information -func validateLink(root, path string, info os.FileInfo) error { - if info.Mode()&os.ModeSymlink != os.ModeSymlink { - return nil // not a symlink - } - +// validateLink returns the target of a given link and an error if +// that target is either absolute or outside the given project root. +// or referrs to a target outside of the given root. +// For conven +func validateLink(root, path string, info os.FileInfo) (tgt string, err error) { // tgt is the raw target of the link. // This path is either absolute or relative to the link's location. - tgt, err := os.Readlink(path) + tgt, err = os.Readlink(path) if err != nil { - return err + return tgt, fmt.Errorf("cannot read link: %w", err) } // Absolute links will not be correct when copied into the runtime // container, because they are placed into path into '/func', if filepath.IsAbs(tgt) { - return errors.New("project may not contain absolute links") + return tgt, errors.New("project may not contain absolute links") } // Calculate the actual target of the link @@ -218,14 +217,14 @@ func validateLink(root, path string, info os.FileInfo) error { // this actual target location relLnkTgt, err := filepath.Rel(root, lnkTgt) if err != nil { - return err + return tgt, err } // Fail if this path is outside the function's root. if strings.HasPrefix(relLnkTgt, ".."+string(filepath.Separator)) || relLnkTgt == ".." { - return errors.New("links must stay within project root") + return tgt, errors.New("links must stay within project root") } - return nil + return } // newCertLayer creates the shared data layer in the container file hierarchy and diff --git a/pkg/oci/containerize_test.go b/pkg/oci/containerize_test.go index 6817b32749..322cd93f94 100644 --- a/pkg/oci/containerize_test.go +++ b/pkg/oci/containerize_test.go @@ -7,6 +7,8 @@ import ( "testing" ) +// Test_validateLink ensures that the validateLink function disallows +// links which are absolute or refer to targets outside the given root. func Test_validateLink(t *testing.T) { root := "testdata/test-links" @@ -15,11 +17,8 @@ func Test_validateLink(t *testing.T) { valid bool // If it should be considered valid name string // descriptive name of the test }{ - {"a.txt", true, "do not evaluate regular files"}, - {"a.lnk", true, "do not evaluate directories"}, {"absoluteLink", false, "disallow absolute-path links"}, {"a.lnk", true, "links to files within the root are allowed"}, - {"...validName.txt", true, "allow files with dot prefixes"}, {"...validName.lnk", true, "allow links with target of dot prefixed names"}, {"linkToRoot", true, "allow links to the project root"}, {"b/linkToRoot", true, "allow links to the project root from within subdir"}, @@ -35,7 +34,7 @@ func Test_validateLink(t *testing.T) { if err != nil { t.Fatal(err) } - err = validateLink(root, path, info) + _, err = validateLink(root, path, info) if err == nil != tt.valid { t.Fatalf("expected %v, got %v", tt.valid, err) } @@ -49,7 +48,7 @@ func Test_validateLink(t *testing.T) { if err != nil { t.Fatal(err) } - err = validateLink(root, path, info) + _, err = validateLink(root, path, info) if err == nil { t.Fatal("absolute path should be invalid on windows") } From 71b588747ff483e44b55928cb414cb8403861f0b Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Fri, 16 Feb 2024 16:01:13 +0900 Subject: [PATCH 09/12] validate link header targets --- pkg/oci/builder_test.go | 16 +++++++++++----- pkg/oci/containerize.go | 2 +- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/pkg/oci/builder_test.go b/pkg/oci/builder_test.go index 2c61732cc2..b824a6e691 100644 --- a/pkg/oci/builder_test.go +++ b/pkg/oci/builder_test.go @@ -67,12 +67,16 @@ func TestBuilder_Files(t *testing.T) { } // Links - var linkMode fs.FileMode - var linkExecutable bool + var link struct { + Target string + Mode fs.FileMode + Executable bool + } if runtime.GOOS != "windows" { // Default case: use symlinks - linkMode = fs.ModeSymlink - linkExecutable = true + link.Target = "a.txt" + link.Mode = fs.ModeSymlink + link.Executable = true if err := os.Symlink("a.txt", "a.lnk"); err != nil { t.Fatal(err) @@ -93,7 +97,7 @@ func TestBuilder_Files(t *testing.T) { {Path: "/etc/ssl/certs/ca-certificates.crt"}, {Path: "/func", Type: fs.ModeDir}, {Path: "/func/README.md"}, - {Path: "/func/a.lnk", Executable: linkExecutable, Type: linkMode}, + {Path: "/func/a.lnk", Linkname: link.Target, Type: link.Mode, Executable: link.Executable}, {Path: "/func/a.txt"}, {Path: "/func/f", Executable: true}, {Path: "/func/func.yaml"}, @@ -306,6 +310,7 @@ func validateOCIFiles(path string, expected []fileInfo, t *testing.T) { Path: hdr.Name, Type: hdr.FileInfo().Mode() & fs.ModeType, Executable: (hdr.FileInfo().Mode()&0111 == 0111) && !hdr.FileInfo().IsDir(), + Linkname: hdr.Linkname, }) } }() @@ -323,4 +328,5 @@ type fileInfo struct { Path string Type fs.FileMode Executable bool + Linkname string } diff --git a/pkg/oci/containerize.go b/pkg/oci/containerize.go index 4a5f2896c3..4a89f22689 100644 --- a/pkg/oci/containerize.go +++ b/pkg/oci/containerize.go @@ -217,7 +217,7 @@ func validateLink(root, path string, info os.FileInfo) (tgt string, err error) { // this actual target location relLnkTgt, err := filepath.Rel(root, lnkTgt) if err != nil { - return tgt, err + return } // Fail if this path is outside the function's root. From 1bdd09b01bbdd908012746c13e14ff229839a23f Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Fri, 16 Feb 2024 16:15:55 +0900 Subject: [PATCH 10/12] expand link test --- pkg/oci/containerize.go | 8 +++----- pkg/oci/containerize_test.go | 25 ++++++++++++++++++++----- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/pkg/oci/containerize.go b/pkg/oci/containerize.go index 4a89f22689..43d07f30f5 100644 --- a/pkg/oci/containerize.go +++ b/pkg/oci/containerize.go @@ -155,7 +155,7 @@ func newDataTarball(root, target string, ignored []string, verbose bool) error { lnk := "" // if link, this will be used as the target if info.Mode()&fs.ModeSymlink != 0 { - if lnk, err = validateLink(root, path, info); err != nil { + if lnk, err = validatedLinkTarget(root, path, info); err != nil { return err } } @@ -191,11 +191,9 @@ func newDataTarball(root, target string, ignored []string, verbose bool) error { }) } -// validateLink returns the target of a given link and an error if +// validatedLinkTarget returns the target of a given link or an error if // that target is either absolute or outside the given project root. -// or referrs to a target outside of the given root. -// For conven -func validateLink(root, path string, info os.FileInfo) (tgt string, err error) { +func validatedLinkTarget(root, path string, info os.FileInfo) (tgt string, err error) { // tgt is the raw target of the link. // This path is either absolute or relative to the link's location. tgt, err = os.Readlink(path) diff --git a/pkg/oci/containerize_test.go b/pkg/oci/containerize_test.go index 322cd93f94..e6d9b8de08 100644 --- a/pkg/oci/containerize_test.go +++ b/pkg/oci/containerize_test.go @@ -7,9 +7,10 @@ import ( "testing" ) -// Test_validateLink ensures that the validateLink function disallows -// links which are absolute or refer to targets outside the given root. -func Test_validateLink(t *testing.T) { +// Test_validatedLinkTaarget ensures that the function disallows +// links which are absolute or refer to targets outside the given root, in +// addition to the basic job of returning the value of reading the link. +func Test_validatedLinkTarget(t *testing.T) { root := "testdata/test-links" tests := []struct { @@ -34,7 +35,7 @@ func Test_validateLink(t *testing.T) { if err != nil { t.Fatal(err) } - _, err = validateLink(root, path, info) + _, err = validatedLinkTarget(root, path, info) if err == nil != tt.valid { t.Fatalf("expected %v, got %v", tt.valid, err) } @@ -48,9 +49,23 @@ func Test_validateLink(t *testing.T) { if err != nil { t.Fatal(err) } - _, err = validateLink(root, path, info) + _, err = validatedLinkTarget(root, path, info) if err == nil { t.Fatal("absolute path should be invalid on windows") } } + + // Spot-check the base case of being a decorator for os.ReadLink + path := "testdata/test-links/a.lnk" + info, err := os.Lstat(path) + if err != nil { + t.Fatal(err) + } + tgt, err := validatedLinkTarget(root, path, info) + if err != nil { + t.Fatal(err) + } + if tgt != "./a.txt" { + t.Fatalf("expected target of 'a/lnk' to be 'a.txt', got '%v'", tgt) + } } From d09d5beff3395faccf07990c1556f1ca6ac40f14 Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Mon, 19 Feb 2024 13:16:25 +0900 Subject: [PATCH 11/12] merge os-specific test cases --- pkg/oci/containerize_test.go | 82 ++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 40 deletions(-) diff --git a/pkg/oci/containerize_test.go b/pkg/oci/containerize_test.go index e6d9b8de08..00ba57ef0d 100644 --- a/pkg/oci/containerize_test.go +++ b/pkg/oci/containerize_test.go @@ -13,59 +13,61 @@ import ( func Test_validatedLinkTarget(t *testing.T) { root := "testdata/test-links" + allRuntimes := []string{} // optional list of runtimes a test applies to + + testAppliesToCurrentRuntime := func(testRuntimes []string) bool { + if len(testRuntimes) == 0 { + return true // no filter defined. + } + for _, r := range testRuntimes { + if runtime.GOOS == r { + return true // filter defined, and current is defined. + } + } + return false // filter defined; current not in set. + } + tests := []struct { - path string // path of the file within test project root - valid bool // If it should be considered valid - name string // descriptive name of the test + path string // path of the file within test project root + valid bool // If it should be considered valid + target string // optional test of the returned value (target) + name string // descriptive name of the test + runtimes []string // only apply this test to the given runtime(s) }{ - {"absoluteLink", false, "disallow absolute-path links"}, - {"a.lnk", true, "links to files within the root are allowed"}, - {"...validName.lnk", true, "allow links with target of dot prefixed names"}, - {"linkToRoot", true, "allow links to the project root"}, - {"b/linkToRoot", true, "allow links to the project root from within subdir"}, - {"b/linkToCurrentDir", true, "allow links to a subdirectory within the project"}, - {"b/linkToRootsParent", false, "disallow links to the project's immediate parent"}, - {"b/linkOutsideRootsParent", false, "disallow links outside project root and its parent"}, - {"b/c/linkToParent", true, "allow links up, but within project"}, + {"absoluteLink", false, "", "disallow absolute-path links on linux", []string{"linux"}}, + {"absoluteLinkWindows", false, "", "disallow absolute-path links on windows", []string{"windows"}}, + {"a.lnk", true, "", "links to files within the root are allowed", allRuntimes}, + {"...validName.lnk", true, "", "allow links with target of dot prefixed names", allRuntimes}, + {"linkToRoot", true, "", "allow links to the project root", allRuntimes}, + {"b/linkToRoot", true, "", "allow links to the project root from within subdir", allRuntimes}, + {"b/linkToCurrentDir", true, "", "allow links to a subdirectory within the project", allRuntimes}, + {"b/linkToRootsParent", false, "", "disallow links to the project's immediate parent", allRuntimes}, + {"b/linkOutsideRootsParent", false, "", "disallow links outside project root and its parent", allRuntimes}, + {"b/c/linkToParent", true, "", " allow links up, but within project", allRuntimes}, + {"a.lnk", true, "./a.txt", "spot-check link target on linux", []string{"linux"}}, + {"a.lnk", true, ".\\a.txt", "spot-check link target on windows ", []string{"windows"}}, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + if !testAppliesToCurrentRuntime(tt.runtimes) { + return // The test has a runtime filter defined + } + path := filepath.Join(root, tt.path) info, err := os.Lstat(path) // filepath.Walk does not follow symlinks if err != nil { t.Fatal(err) } - _, err = validatedLinkTarget(root, path, info) + target, err := validatedLinkTarget(root, path, info) + if err == nil != tt.valid { - t.Fatalf("expected %v, got %v", tt.valid, err) + t.Fatalf("expected validity '%v', got '%v'", tt.valid, err) + } + if tt.target != "" && target != tt.target { + t.Fatalf("expected target %q, got %q", tt.target, target) } }) } - // Run a windows-specific absolute path test - if runtime.GOOS == "windows" { - path := "testdata/test-links/absoluteLinkWindows" - info, err := os.Lstat(path) - if err != nil { - t.Fatal(err) - } - _, err = validatedLinkTarget(root, path, info) - if err == nil { - t.Fatal("absolute path should be invalid on windows") - } - } - - // Spot-check the base case of being a decorator for os.ReadLink - path := "testdata/test-links/a.lnk" - info, err := os.Lstat(path) - if err != nil { - t.Fatal(err) - } - tgt, err := validatedLinkTarget(root, path, info) - if err != nil { - t.Fatal(err) - } - if tgt != "./a.txt" { - t.Fatalf("expected target of 'a/lnk' to be 'a.txt', got '%v'", tgt) - } } From 17c097d2048a36b0be2d5ea29d710757607dc75f Mon Sep 17 00:00:00 2001 From: Luke Kingland Date: Tue, 20 Feb 2024 11:32:57 +0900 Subject: [PATCH 12/12] remove os filter and unused arg --- pkg/oci/containerize.go | 4 +-- pkg/oci/containerize_test.go | 60 +++++++++++++----------------------- 2 files changed, 23 insertions(+), 41 deletions(-) diff --git a/pkg/oci/containerize.go b/pkg/oci/containerize.go index 43d07f30f5..011ecc5e1f 100644 --- a/pkg/oci/containerize.go +++ b/pkg/oci/containerize.go @@ -155,7 +155,7 @@ func newDataTarball(root, target string, ignored []string, verbose bool) error { lnk := "" // if link, this will be used as the target if info.Mode()&fs.ModeSymlink != 0 { - if lnk, err = validatedLinkTarget(root, path, info); err != nil { + if lnk, err = validatedLinkTarget(root, path); err != nil { return err } } @@ -193,7 +193,7 @@ func newDataTarball(root, target string, ignored []string, verbose bool) error { // validatedLinkTarget returns the target of a given link or an error if // that target is either absolute or outside the given project root. -func validatedLinkTarget(root, path string, info os.FileInfo) (tgt string, err error) { +func validatedLinkTarget(root, path string) (tgt string, err error) { // tgt is the raw target of the link. // This path is either absolute or relative to the link's location. tgt, err = os.Readlink(path) diff --git a/pkg/oci/containerize_test.go b/pkg/oci/containerize_test.go index 00ba57ef0d..33acffb461 100644 --- a/pkg/oci/containerize_test.go +++ b/pkg/oci/containerize_test.go @@ -1,7 +1,6 @@ package oci import ( - "os" "path/filepath" "runtime" "testing" @@ -13,53 +12,36 @@ import ( func Test_validatedLinkTarget(t *testing.T) { root := "testdata/test-links" - allRuntimes := []string{} // optional list of runtimes a test applies to - - testAppliesToCurrentRuntime := func(testRuntimes []string) bool { - if len(testRuntimes) == 0 { - return true // no filter defined. - } - for _, r := range testRuntimes { - if runtime.GOOS == r { - return true // filter defined, and current is defined. - } - } - return false // filter defined; current not in set. + // Windows-specific absolute link and link target values: + absoluteLink := "absoluteLink" + linkTarget := "./a.txt" + if runtime.GOOS == "windows" { + absoluteLink = "absoluteLinkWindows" + linkTarget = ".\\a.txt" } tests := []struct { - path string // path of the file within test project root - valid bool // If it should be considered valid - target string // optional test of the returned value (target) - name string // descriptive name of the test - runtimes []string // only apply this test to the given runtime(s) + path string // path of the file within test project root + valid bool // If it should be considered valid + target string // optional test of the returned value (target) + name string // descriptive name of the test }{ - {"absoluteLink", false, "", "disallow absolute-path links on linux", []string{"linux"}}, - {"absoluteLinkWindows", false, "", "disallow absolute-path links on windows", []string{"windows"}}, - {"a.lnk", true, "", "links to files within the root are allowed", allRuntimes}, - {"...validName.lnk", true, "", "allow links with target of dot prefixed names", allRuntimes}, - {"linkToRoot", true, "", "allow links to the project root", allRuntimes}, - {"b/linkToRoot", true, "", "allow links to the project root from within subdir", allRuntimes}, - {"b/linkToCurrentDir", true, "", "allow links to a subdirectory within the project", allRuntimes}, - {"b/linkToRootsParent", false, "", "disallow links to the project's immediate parent", allRuntimes}, - {"b/linkOutsideRootsParent", false, "", "disallow links outside project root and its parent", allRuntimes}, - {"b/c/linkToParent", true, "", " allow links up, but within project", allRuntimes}, - {"a.lnk", true, "./a.txt", "spot-check link target on linux", []string{"linux"}}, - {"a.lnk", true, ".\\a.txt", "spot-check link target on windows ", []string{"windows"}}, + {absoluteLink, false, "", "disallow absolute-path links on linux"}, + {"a.lnk", true, linkTarget, "spot-check link target"}, + {"a.lnk", true, "", "links to files within the root are allowed"}, + {"...validName.lnk", true, "", "allow links with target of dot prefixed names"}, + {"linkToRoot", true, "", "allow links to the project root"}, + {"b/linkToRoot", true, "", "allow links to the project root from within subdir"}, + {"b/linkToCurrentDir", true, "", "allow links to a subdirectory within the project"}, + {"b/linkToRootsParent", false, "", "disallow links to the project's immediate parent"}, + {"b/linkOutsideRootsParent", false, "", "disallow links outside project root and its parent"}, + {"b/c/linkToParent", true, "", " allow links up, but within project"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if !testAppliesToCurrentRuntime(tt.runtimes) { - return // The test has a runtime filter defined - } - path := filepath.Join(root, tt.path) - info, err := os.Lstat(path) // filepath.Walk does not follow symlinks - if err != nil { - t.Fatal(err) - } - target, err := validatedLinkTarget(root, path, info) + target, err := validatedLinkTarget(root, path) if err == nil != tt.valid { t.Fatalf("expected validity '%v', got '%v'", tt.valid, err)