From d027fb542757b7d242650c99cbfeeea7b00f4306 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Thu, 6 Jul 2017 05:48:46 +1000 Subject: [PATCH 1/4] pkg: add mtreefilter package This makes it much simpler to create filters for []mtree.InodeDelta given an arbitrary filter function. This is necessary to implement volume masking (as well as arbitrary path masking and possibly glob masking in the future). Signed-off-by: Aleksa Sarai --- pkg/mtreefilter/mask.go | 76 ++++++++++++++++++ pkg/mtreefilter/mask_test.go | 144 +++++++++++++++++++++++++++++++++++ 2 files changed, 220 insertions(+) create mode 100644 pkg/mtreefilter/mask.go create mode 100644 pkg/mtreefilter/mask_test.go diff --git a/pkg/mtreefilter/mask.go b/pkg/mtreefilter/mask.go new file mode 100644 index 000000000..7f878b801 --- /dev/null +++ b/pkg/mtreefilter/mask.go @@ -0,0 +1,76 @@ +/* + * umoci: Umoci Modifies Open Containers' Images + * Copyright (C) 2017 SUSE LLC. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package mtreefilter + +import ( + "path/filepath" + + "github.com/apex/log" + "github.com/vbatts/go-mtree" +) + +// FilterFunc is a function used when filtering deltas with FilterDeltas. +type FilterFunc func(path string) bool + +// isParent returns whether the path a is lexically an ancestor of the path b. +func isParent(a, b string) bool { + a = filepath.Clean(a) + b = filepath.Clean(b) + + for a != b && b != filepath.Dir(b) { + b = filepath.Dir(b) + } + return a == b +} + +// MaskFilter is a factory for FilterFuncs that will mask all InodeDelta paths +// that are lexical children of any path in the mask slice. All paths are +// considered to be relative to '/'. +func MaskFilter(masks []string) FilterFunc { + return func(path string) bool { + // Convert the path to be cleaned and relative-to-root. + path = filepath.Join("/", path) + + // Check that no masks are matched. + for _, mask := range masks { + // Mask also needs to be cleaned and relative-to-root. + mask = filepath.Join("/", mask) + + // Is it a parent? + if isParent(mask, path) { + log.Debugf("maskfilter: ignoring path %q matched by mask %q", path, mask) + return false + } + } + + return true + } +} + +// FilterDeltas is a helper function to easily filter []mtree.InodeDelta with a +// filter function. Only entries which have `filter(delta.Path()) == true` will +// be included in the returned slice. +func FilterDeltas(deltas []mtree.InodeDelta, filter FilterFunc) []mtree.InodeDelta { + var filtered []mtree.InodeDelta + for _, delta := range deltas { + if filter(delta.Path()) { + filtered = append(filtered, delta) + } + } + return filtered +} diff --git a/pkg/mtreefilter/mask_test.go b/pkg/mtreefilter/mask_test.go new file mode 100644 index 000000000..e05a6a017 --- /dev/null +++ b/pkg/mtreefilter/mask_test.go @@ -0,0 +1,144 @@ +/* + * umoci: Umoci Modifies Open Containers' Images + * Copyright (C) 2017 SUSE LLC. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package mtreefilter + +import ( + "io/ioutil" + "os" + "path/filepath" + "testing" + + "github.com/vbatts/go-mtree" +) + +func TestIsParent(t *testing.T) { + for _, test := range []struct { + parent, path string + expected bool + }{ + {"/", "/a", true}, + {"/", "/a/b/c", true}, + {"/", "/", true}, + {"/a path/", "/a path", true}, + {"/a nother path", "/a nother path/test", true}, + {"/a nother path", "/a nother path/test/1 2/ 33 /", true}, + {"/path1", "/path2", false}, + {"/pathA", "/PATHA", false}, + {"/pathC", "/path", false}, + {"/path9", "/", false}, + // Make sure it's not the same as filepath.HasPrefix. + {"/a/b/c", "/a/b/c/d", true}, + {"/a/b/c", "/a/b/cd", false}, + {"/a/b/c", "/a/bcd", false}, + {"/a/bc", "/a/bcd", false}, + {"/abc", "/abcd", false}, + } { + got := isParent(test.parent, test.path) + if got != test.expected { + t.Errorf("isParent(%q, %q) got %v expected %v", test.parent, test.path, got, test.expected) + } + } +} + +func TestMaskDeltas(t *testing.T) { + dir, err := ioutil.TempDir("", "TestMaskDeltas-") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + var mtreeKeywords []mtree.Keyword = append(mtree.DefaultKeywords, "sha256digest") + + // Create some files. + if err != ioutil.WriteFile(filepath.Join(dir, "file1"), []byte("contents"), 0644) { + t.Fatal(err) + } + if err != ioutil.WriteFile(filepath.Join(dir, "file2"), []byte("another content"), 0644) { + t.Fatal(err) + } + if err != os.MkdirAll(filepath.Join(dir, "dir", "child"), 0755) { + t.Fatal(err) + } + if err != os.MkdirAll(filepath.Join(dir, "dir", "child2"), 0755) { + t.Fatal(err) + } + if err != ioutil.WriteFile(filepath.Join(dir, "dir", "file 3"), []byte("more content"), 0644) { + t.Fatal(err) + } + if err != ioutil.WriteFile(filepath.Join(dir, "dir", "child2", "4 files"), []byte("very content"), 0644) { + t.Fatal(err) + } + + // Generate a diff. + originalDh, err := mtree.Walk(dir, nil, mtreeKeywords, nil) + if err != nil { + t.Fatal(err) + } + + // Modify the root. + if err := os.RemoveAll(filepath.Join(dir, "file2")); err != nil { + t.Fatal(err) + } + if err := ioutil.WriteFile(filepath.Join(dir, "dir", "new"), []byte("more content"), 0755); err != nil { + t.Fatal(err) + } + if err := ioutil.WriteFile(filepath.Join(dir, "file1"), []byte("different contents"), 0666); err != nil { + t.Fatal(err) + } + + // Generate the set of diffs. + newDh, err := mtree.Walk(dir, nil, mtreeKeywords, nil) + if err != nil { + t.Fatal(err) + } + diff, err := mtree.Compare(originalDh, newDh, mtreeKeywords) + if err != nil { + t.Fatal(err) + } + + for _, test := range []struct { + paths []string + }{ + {nil}, + {[]string{"/"}}, + {[]string{"dir"}}, + {[]string{filepath.Join("dir", "child2")}}, + {[]string{"file2"}}, + {[]string{"/", "file2"}}, + {[]string{"file2", filepath.Join("dir", "child2")}}, + } { + newDiff := FilterDeltas(diff, MaskFilter(test.paths)) + for _, delta := range newDiff { + if len(test.paths) == 0 { + if len(newDiff) != len(diff) { + t.Errorf("expected diff={} to give %d got %d", len(diff), len(newDiff)) + } + } else { + found := false + for _, path := range test.paths { + if !isParent(path, delta.Path()) { + found = true + } + } + if !found { + t.Errorf("expected one of %v to not be a parent of %q", test.paths, delta.Path()) + } + } + } + } +} From e8098c40d518ef314a0a14d632d1c06db21e07f9 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Thu, 6 Jul 2017 05:50:35 +1000 Subject: [PATCH 2/4] repack: add --no-mask-volumes and --mask-path The reason for the split is that some users may wish to mask arbitrary paths that have not been set with --config.volumes (which therefore requires --mask-path). We make --no-mask-volumes an opt-out option (despite the backwards compatibility problem) because it's a security feature that really shouldn't be opt-in. I don't anticipate this will break current users of KIWI (and future users will hit this and realise it's a feature). However, this needs to be toggle-able because: 1. It is a backwards-incompatible change to the command-line API. Since KIWI has its own opinions about configuration and its relation to file unpacking, so older users of umoci need a way to switch to the old semantics. 2. Some users don't want to follow the suggestion of the spec that volumes should not be included in layers. While it's not clear when this would be a good idea (--no-mask-volumes is a security feature against credential exposure), we must retain it as an option. If nobody ends up using it we can remove the ability to disable it. Signed-off-by: Aleksa Sarai --- cmd/umoci/repack.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/cmd/umoci/repack.go b/cmd/umoci/repack.go index b6681d010..a23aff6f1 100644 --- a/cmd/umoci/repack.go +++ b/cmd/umoci/repack.go @@ -31,6 +31,7 @@ import ( "github.com/openSUSE/umoci/oci/casext" igen "github.com/openSUSE/umoci/oci/config/generate" "github.com/openSUSE/umoci/oci/layer" + "github.com/openSUSE/umoci/pkg/mtreefilter" ispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" "github.com/urfave/cli" @@ -65,6 +66,17 @@ manifest and configuration information uses the new diff atop the old manifest.` // repack creates a new image, with a given tag. Category: "image", + Flags: []cli.Flag{ + cli.StringSliceFlag{ + Name: "mask-path", + Usage: "set of path prefixes in which deltas will be ignored when generating new layers", + }, + cli.BoolFlag{ + Name: "no-mask-volumes", + Usage: "do not add the Config.Volumes of the image to the set of masked paths", + }, + }, + Action: repack, Before: func(ctx *cli.Context) error { @@ -156,6 +168,19 @@ func repack(ctx *cli.Context) error { "ndiff": len(diffs), }).Debugf("umoci: checked mtree spec") + // We need to mask config.Volumes. + config, err := mutator.Config(context.Background()) + if err != nil { + return errors.Wrap(err, "get config") + } + maskedPaths := ctx.StringSlice("mask-path") + if !ctx.Bool("no-mask-volumes") { + for v := range config.Volumes { + maskedPaths = append(maskedPaths, v) + } + } + diffs = mtreefilter.FilterDeltas(diffs, mtreefilter.MaskFilter(maskedPaths)) + reader, err := layer.GenerateLayer(fullRootfsPath, diffs, &meta.MapOptions) if err != nil { return errors.Wrap(err, "generate diff layer") From 519c84c6dcf4df294c371696bd42dc30f72f0348 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Thu, 6 Jul 2017 05:54:43 +1000 Subject: [PATCH 3/4] test: add repack tests for masked paths Signed-off-by: Aleksa Sarai --- test/repack.bats | 106 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/test/repack.bats b/test/repack.bats index 638d20281..540860e8f 100644 --- a/test/repack.bats +++ b/test/repack.bats @@ -499,3 +499,109 @@ function teardown() { image-verify "${IMAGE}" } + +@test "umoci repack [--config.volumes]" { + BUNDLE_A="$(setup_tmpdir)" + BUNDLE_B="$(setup_tmpdir)" + BUNDLE_C="$(setup_tmpdir)" + BUNDLE_D="$(setup_tmpdir)" + + # Set some paths to be volumes. + umoci config --image "${IMAGE}:${TAG}" --config.volume /volume --config.volume "/some nutty/path name/ here" + [ "$status" -eq 0 ] + image-verify "${IMAGE}" + + # Unpack the image. + umoci unpack --image "${IMAGE}:${TAG}" "$BUNDLE_A" + [ "$status" -eq 0 ] + bundle-verify "$BUNDLE_A" + + # Create files in those volumes. + mkdir -p "$BUNDLE_A/rootfs/some nutty/path name/" + echo "this is a test" > "$BUNDLE_A/rootfs/some nutty/path name/ here" + mkdir -p "$BUNDLE_A/rootfs/volume" + echo "another test" > "$BUNDLE_A/rootfs/volume/test" + # ... and some outside. + echo "more tests" > "$BUNDLE_A/rootfs/some nutty/path " + mkdir -p "$BUNDLE_A/rootfs/some/volume" + echo "in a mirror directory" > "$BUNDLE_A/rootfs/some/volume/test" + echo "checking mirror" > "$BUNDLE_A/rootfs/volumetest" + + # Repack the image under a new tag. + umoci repack --image "${IMAGE}:${TAG}-new" "$BUNDLE_A" + [ "$status" -eq 0 ] + image-verify "${IMAGE}" + + # Re-extract to verify the volume changes weren't included. + umoci unpack --image "${IMAGE}:${TAG}-new" "$BUNDLE_B" + [ "$status" -eq 0 ] + bundle-verify "$BUNDLE_B" + + # Check the files. + [ -f "$BUNDLE_B/rootfs/some nutty/path " ] + [[ "$(cat "$BUNDLE_B/rootfs/some nutty/path ")" == "more tests" ]] + [ -d "$BUNDLE_B/rootfs/some/volume" ] + [ -f "$BUNDLE_B/rootfs/some/volume/test" ] + [[ "$(cat "$BUNDLE_B/rootfs/some/volume/test")" == "in a mirror directory" ]] + [ -f "$BUNDLE_B/rootfs/volumetest" ] + [[ "$(cat "$BUNDLE_B/rootfs/volumetest")" == "checking mirror" ]] + + # Volume paths must not be included. + ! [ -e "$BUNDLE_B/rootfs/volume" ] + ! [ -e "$BUNDLE_B/rootfs/volume/test" ] + ! [ -e "$BUNDLE_B/rootfs/some nutty/path name" ] + ! [ -e "$BUNDLE_B/rootfs/some nutty/path name/ here" ] + + # Repack a copy with volumes not masked. + umoci repack --image "${IMAGE}:${TAG}-nomask" --no-mask-volumes "$BUNDLE_A" + [ "$status" -eq 0 ] + image-verify "${IMAGE}" + + # Extract the no-mask variant to make sure that masked changes *were* included. + umoci unpack --image "${IMAGE}:${TAG}-nomask" "$BUNDLE_C" + [ "$status" -eq 0 ] + bundle-verify "$BUNDLE_C" + + # Check the files. + [ -f "$BUNDLE_C/rootfs/some nutty/path " ] + [[ "$(cat "$BUNDLE_C/rootfs/some nutty/path ")" == "more tests" ]] + [ -d "$BUNDLE_C/rootfs/some/volume" ] + [ -f "$BUNDLE_C/rootfs/some/volume/test" ] + [[ "$(cat "$BUNDLE_C/rootfs/some/volume/test")" == "in a mirror directory" ]] + [ -f "$BUNDLE_C/rootfs/volumetest" ] + [[ "$(cat "$BUNDLE_C/rootfs/volumetest")" == "checking mirror" ]] + + # Volume paths must be included, as well as their contents. + [ -e "$BUNDLE_C/rootfs/volume" ] + [ -f "$BUNDLE_C/rootfs/volume/test" ] + [[ "$(cat "$BUNDLE_C/rootfs/volume/test")" == "another test" ]] + [ -d "$BUNDLE_C/rootfs/some nutty/path name" ] + [ -f "$BUNDLE_C/rootfs/some nutty/path name/ here" ] + [[ "$(cat "$BUNDLE_C/rootfs/some nutty/path name/ here")" == "this is a test" ]] + + # Re-do everything but this time with --mask-path. + umoci repack --image "${IMAGE}:${TAG}-new" --mask-path /volume "$BUNDLE_A" + [ "$status" -eq 0 ] + image-verify "${IMAGE}" + + # Re-extract to verify the masked path changes weren't included. + umoci unpack --image "${IMAGE}:${TAG}-new" "$BUNDLE_D" + [ "$status" -eq 0 ] + bundle-verify "$BUNDLE_D" + + # Check the files. + [ -f "$BUNDLE_D/rootfs/some nutty/path " ] + [[ "$(cat "$BUNDLE_D/rootfs/some nutty/path ")" == "more tests" ]] + [ -d "$BUNDLE_D/rootfs/some/volume" ] + [ -f "$BUNDLE_D/rootfs/some/volume/test" ] + [[ "$(cat "$BUNDLE_D/rootfs/some/volume/test")" == "in a mirror directory" ]] + [ -f "$BUNDLE_D/rootfs/volumetest" ] + [[ "$(cat "$BUNDLE_D/rootfs/volumetest")" == "checking mirror" ]] + + # Masked paths must not be included. + ! [ -e "$BUNDLE_D/rootfs/volume" ] + ! [ -e "$BUNDLE_D/rootfs/volume/test" ] + # And volumes will also not be included. + ! [ -e "$BUNDLE_D/rootfs/some nutty/path name" ] + ! [ -e "$BUNDLE_D/rootfs/some nutty/path name/ here" ] +} From 1e52a943c0678e8488319544529d79896ec32e18 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Thu, 6 Jul 2017 05:47:06 +1000 Subject: [PATCH 4/4] CHANGELOG: update to include volume features Signed-off-by: Aleksa Sarai --- CHANGELOG.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 048a26a4d..e1618b5dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/). functionality has not been used "in the wild" and `umoci` doesn't have the UX to create such structures (yet) but these will be implemented in future versions. openSUSE/umoci#145 +- `umoci repack` now supports `--mask-path` to ignore changes in the rootfs + that are in a child of at least one of the provided masks when generating new + layers. openSUSE/umoci#127 ### Changed - Error messages from `github.com/openSUSE/umoci/oci/cas/drivers/dir` actually @@ -32,11 +35,19 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - `umoci unpack` now generates `config.json` blobs according to the [still proposed][ispec-pr492] OCI image specification conversion document. openSUSE/umoci#120 +- `umoci repack` also now automatically adding `Config.Volumes` from the image + configuration to the set of masked paths. This matches recently added + [recommendations by the spec][ispec-pr694], but is a backwards-incompatible + change because the new default is that `Config.Volumes` **will** be masked. + If you wish to retain the old semantics, use `--no-mask-volumes` (though make + sure to be aware of the reasoning behind `Config.Volume` masking). + openSUSE/umoci#127 [cii]: https://bestpractices.coreinfrastructure.org/projects/1084 [rspec-v1.0.0-rc6]: https://github.com/opencontainers/runtime-spec/releases/tag/v1.0.0-rc6 [ispec-v1.0.0-rc7]: https://github.com/opencontainers/image-spec/releases/tag/v1.0.0-rc7 [ispec-pr492]: https://github.com/opencontainers/image-spec/pull/492 +[ispec-pr694]: https://github.com/opencontainers/image-spec/pull/694 ## [0.2.1] - 2017-04-12 ### Added