repack: do not include Volume paths in new layers #127

cyphar · 2017-06-03T19:50:17Z

This is (unfortunately) not mandated in the specification, but in
order to avoid accidentally spilling private information into published
layers (which is one use of Volumes) we must ignore all layers included
in Config.Volumes.

In future we should also add some flags or alernative ways of masking
paths.

Signed-off-by: Aleksa Sarai asarai@suse.de

cyphar · 2017-06-03T19:52:51Z

@vrothberg PTAL

vrothberg · 2017-06-08T06:33:38Z

Sorry, I did not forget about this. Hopefully I find time to check this PR next week.

cyphar · 2017-06-25T12:04:49Z

This is now actually recommended by opencontainers/image-spec#694 (yay, finally).

vrothberg · 2017-06-25T14:58:04Z

Awesome! I put checking the code on tomorrow's to-do.

vrothberg

Somehow my guts tell me to invert the functionality and add an --unmask-volumes flag and mask volumes by default. Especially because "If a new image is created from a container based on the image described by this configuration, data in these paths SHOULD NOT be included in the new image."

vrothberg · 2017-06-26T07:27:07Z

pkg/mtreefilter/mask.go

+	return func(path string) bool {
+		// Convert path to relative-to-root.
+		path = filepath.Clean(path)
+		path = filepath.Join("/", path)


Do you mean an absolute path? If so, we should use filpath.Abs(path).

filepath.Abs can fail, and it also tries to do things we don't want from memory. Effectively what this does in this context is that it makes the path underneath the root. However, I've just realised that it shouldn't be necessary to have a Clean(path) beforehand because the prefix path is / (which is safe against Clean issues).

vrothberg · 2017-06-26T07:29:02Z

pkg/mtreefilter/mask.go

+		for _, mask := range masks {
+			// Mask also needs to be relative-to-root.
+			mask = filepath.Clean(mask)
+			mask = filepath.Join("/", mask)


Absolute path?

vrothberg · 2017-06-26T07:46:30Z

pkg/mtreefilter/mask.go

+// 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.


That's a very elegant function 👍

Thanks. 😉

vrothberg · 2017-06-26T08:10:25Z

test/repack.bats

+	bundle-verify "$BUNDLE_A"
+
+	# Create files in those volumes.
+	mkdir -p "$BUNDLE_A/rootfs/some nutty/path name/"


I would expect unpack to create volume paths. Is there a specific reason not to? Run-times should be able to deal with both cases?

Hmmm. I'm not sure. VOLUME in a Dockerfile creates the directory in the image (from memory) so I'm not sure if it makes sense for us to do that -- what should we do if a non-directory already exists there (for example)? I don't like the idea of erroring out in that case, but ignoring it also seems fishy.

I'll have to think about it some more, but can you open an issue for us to review at a later date?

Good idea, let's move to an issue 👍

vrothberg · 2017-06-26T08:47:46Z

test/repack.bats

+	! [ -e "$BUNDLE_B/rootfs/some nutty/path name" ]
+	! [ -e "$BUNDLE_B/rootfs/some nutty/path name/ here" ]
+
+	# TODO: We need to add some flags to repack to allow users to ignore VOLUME.


I am not sure I understand this comment correctly. What do you mean by "ignore"? In the sense that content will be copied regardless of the path being a volume?

There's one hiding at the bottom :)

vrothberg · 2017-06-26T08:59:02Z

CHANGELOG.md

@@ -5,6 +5,12 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).

 ## [Unreleased]
+### Added
+- `umoci repack` now supports `--mask-path` to ignore certain changes in the
+  rootfs when generating new layers.


I prefer it to be more explicit: [...] to ignore changes in specified path of the rootfs when generating new layers.

vrothberg · 2017-06-26T09:05:40Z

cmd/umoci/repack.go

+	if ctx.Bool("mask-volumes") {
+		for v := range config.Volumes {
+			volumes = append(volumes, v)
+		}
 	}


I suggest to rename volumes to maskedPaths.

vrothberg · 2017-06-26T09:13:07Z

test/repack.bats

+	[ "$status" -eq 0 ]
+	image-verify "${IMAGE}"
+
+	# Re-extract to verify the volume changes weren't included.


...the masked path changes...

vrothberg · 2017-06-26T09:14:08Z

test/repack.bats

+	# Masked paths must not be included.
+	! [ -e "$BUNDLE_C/rootfs/volume" ]
+	! [ -e "$BUNDLE_C/rootfs/volume/test" ]
+	# But volumes might not be included.


might BE included?

cyphar

Some responses enclosed.

As for the opt-in security (we already discussed this, I'm just putting it here for posterity):

I agree that opt-in security is bad (and my main argument for proposing adding this language to the spec was security). My main concern was that users like KIWI don't actually want to have this option be the default, and I'm not sure how we should approach changing our API (specifically adding flags that opt-out of new features) for such users.

We need to talk to the KIWI folks about what they want the default to be and come to an agreement on how they would like to be able to interface with us if we change our ABI. This will additionally be very important with the rc5 bump because in future I'm planning on changing how you reference tags (in somewhat minor ways, but I really want --layout to die).

cyphar · 2017-06-26T09:49:05Z

pkg/mtreefilter/mask.go

+	return func(path string) bool {
+		// Convert path to relative-to-root.
+		path = filepath.Clean(path)
+		path = filepath.Join("/", path)


filepath.Abs can fail, and it also tries to do things we don't want from memory. Effectively what this does in this context is that it makes the path underneath the root. However, I've just realised that it shouldn't be necessary to have a Clean(path) beforehand because the prefix path is / (which is safe against Clean issues).

cyphar · 2017-06-27T16:25:27Z

pkg/mtreefilter/mask.go

+// 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.


Thanks. 😉

cyphar · 2017-06-27T16:25:39Z

pkg/mtreefilter/mask.go

+		for _, mask := range masks {
+			// Mask also needs to be relative-to-root.
+			mask = filepath.Clean(mask)
+			mask = filepath.Join("/", mask)


cyphar · 2017-06-27T16:27:56Z

test/repack.bats

+	bundle-verify "$BUNDLE_A"
+
+	# Create files in those volumes.
+	mkdir -p "$BUNDLE_A/rootfs/some nutty/path name/"


Hmmm. I'm not sure. VOLUME in a Dockerfile creates the directory in the image (from memory) so I'm not sure if it makes sense for us to do that -- what should we do if a non-directory already exists there (for example)? I don't like the idea of erroring out in that case, but ignoring it also seems fishy.

I'll have to think about it some more, but can you open an issue for us to review at a later date?

cyphar · 2017-07-05T20:17:44Z

I'm going to wait on merging this until I speak to @davidcassany about how he feels about --no-mask-volumes and how we're going to handle CLI changes in the future.

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 <asarai@suse.de>

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 <asarai@suse.de>

cyphar · 2017-07-19T14:46:44Z

I've rebased this and made it opt-out after discussions with @davidcassany. I agree with him that I severely doubt that any current users will be broken by this and in the future KIWI will have strict requirements on umoci versions.

Signed-off-by: Aleksa Sarai <asarai@suse.de>

cyphar · 2017-07-19T17:03:33Z

LGTM.

@cyphar

LGTMs: @cyphar Closes #127

cyphar added cmd/repack oci/layer labels Jun 3, 2017

cyphar added this to the 0.3.0 milestone Jun 3, 2017

cyphar force-pushed the ignore-volumes-in-snapshot branch 6 times, most recently from f29c820 to b150f72 Compare June 5, 2017 12:46

vrothberg reviewed Jun 26, 2017

View reviewed changes

cyphar commented Jun 27, 2017

View reviewed changes

vrothberg mentioned this pull request Jun 28, 2017

unpack and volumes: How could / should / must umoci behave? #130

Open

cyphar force-pushed the ignore-volumes-in-snapshot branch from b150f72 to 1ee0585 Compare July 5, 2017 20:02

cyphar added 2 commits July 20, 2017 00:22

cyphar force-pushed the ignore-volumes-in-snapshot branch from 1ee0585 to f4153fe Compare July 19, 2017 14:46

cyphar added 2 commits July 20, 2017 01:19

test: add repack tests for masked paths

519c84c

Signed-off-by: Aleksa Sarai <asarai@suse.de>

CHANGELOG: update to include volume features

1e52a94

Signed-off-by: Aleksa Sarai <asarai@suse.de>

cyphar force-pushed the ignore-volumes-in-snapshot branch from f4153fe to 1e52a94 Compare July 19, 2017 15:20

cyphar merged commit 1e52a94 into master Jul 19, 2017

cyphar added a commit that referenced this pull request Jul 19, 2017

merge branch 'pr-127'

6982793

LGTMs: @cyphar Closes #127

cyphar deleted the ignore-volumes-in-snapshot branch July 19, 2017 17:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

repack: do not include Volume paths in new layers #127

repack: do not include Volume paths in new layers #127

cyphar commented Jun 3, 2017 •

edited

cyphar commented Jun 3, 2017

vrothberg commented Jun 8, 2017

cyphar commented Jun 25, 2017

vrothberg commented Jun 25, 2017

vrothberg left a comment

vrothberg Jun 26, 2017

cyphar Jun 26, 2017

vrothberg Jun 26, 2017

cyphar Jun 27, 2017

vrothberg Jun 26, 2017

cyphar Jun 27, 2017

vrothberg Jun 26, 2017

cyphar Jun 27, 2017

vrothberg Jun 28, 2017

vrothberg Jun 26, 2017

vrothberg Jun 28, 2017

vrothberg Jun 26, 2017

vrothberg Jun 26, 2017

vrothberg Jun 26, 2017

vrothberg Jun 26, 2017

cyphar left a comment

cyphar Jun 26, 2017

cyphar Jun 27, 2017

cyphar Jun 27, 2017

cyphar Jun 27, 2017

cyphar commented Jul 5, 2017

cyphar commented Jul 19, 2017

cyphar commented Jul 19, 2017

repack: do not include Volume paths in new layers #127

repack: do not include Volume paths in new layers #127

Conversation

cyphar commented Jun 3, 2017 • edited

cyphar commented Jun 3, 2017

vrothberg commented Jun 8, 2017

cyphar commented Jun 25, 2017

vrothberg commented Jun 25, 2017

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar commented Jul 5, 2017

cyphar commented Jul 19, 2017

cyphar commented Jul 19, 2017

cyphar commented Jun 3, 2017 •

edited