Apply ownership in unpacked entries #3

glestaris · 2016-09-15T17:24:52Z

This patch will apply the user/group, found in the tar entry, to the unpacked
files, directories, and symlinks. It will only do so if the user running the
oci-unpack tool is root.

To test the patch, I used latest Ubuntu from Dockerhub, converted to OCI
format with skopeo:

skopeo copy docker://ubuntu oci://$PWD/ubuntu

With the current oci-unpack, all files and directories are owned by the user
that does the unpacking:

root@vagrant:/tmp# ls -la ubuntu-unpacked-old/etc/shadow
-rw-r----- 1 root root 652 Sep 15 17:17 ubuntu-unpacked-old/etc/shadow

With the proposed patch, the correct ownership is applied:

root@vagrant:/tmp# ls -la ubuntu-unpacked/etc/shadow
-rw-r----- 1 root shadow 652 Sep 15 17:14 ubuntu-unpacked/etc/shadow

runcom · 2016-09-15T17:26:34Z

Not super sure about this, ownership should be preserved regardless of whether the tool is run by root or now. (not sure how docker engine handles this also)

wking · 2016-09-15T17:35:24Z

On Thu, Sep 15, 2016 at 10:26:35AM -0700, Antonio Murdaca wrote:

Not super sure about this, ownership should be preserved
irregardless of whether the tool is run by root or now. (not sure
how docker engine handles this also)

It is usually not possible to preserve ownership when an unprivileged
user is unpacking. From GNU's tar [1,2]:
—
--no-same-owner
Extract files as yourself (default for ordinary users).
--no-same-permissions
Apply the user's umask when extracting permissions from the
archive (default for ordinary users).

ea4e2ea just hard-codes that default, which is fine with me.

runcom · 2016-09-15T17:37:09Z

It is usually not possible to preserve ownership when an unprivileged
user is unpacking. From GNU's tar [1,2]:

then we should warn about running oci-unpack only with root (or any privileged user) instead of patching the code to check for uid 0. Just fail when trying to chown root:root it's sufficient for me.

wking · 2016-09-15T17:40:11Z

But we may need to reroll or add helper functions to avoid 1:

$ make lint
checking lint
image/manifest.go:123::warning: cyclomatic complexity 38 of function unpackLayer() is high (> 35) (gocyclo)

wking · 2016-09-15T17:41:50Z

On Thu, Sep 15, 2016 at 10:37:10AM -0700, Antonio Murdaca wrote:

It is usually not possible to preserve ownership when an unprivileged
user is unpacking. From GNU's tar [1,2]:

then we should warn about running oci-unpack only with root (or any
privileged user) instead of patching the code to check for uid 0.

Sometimes you cannot become root, and it would be nice to support
image-tools for unprivileged users ;). If there is a lot of
resistance to baking in the default, I'm fine adding --(no-)same-owner
and --(no-)same-group to follow tar and make these optional.

wking · 2016-09-15T17:43:06Z

On Thu, Sep 15, 2016 at 10:37:10AM -0700, Antonio Murdaca wrote:

It is usually not possible to preserve ownership when an unprivileged
user is unpacking. From GNU's tar [1,2]:

then we should warn about running oci-unpack only with root (or any
privileged user) instead of patching the code to check for uid 0.

And if you take this perfect-unpack rule (which I want to relax,
possibly with an option) to its logical conculsion, you'd also be
dying if the unpacker's umask wasn't 0777 ;).

runcom · 2016-09-15T17:43:06Z

Sometimes you cannot become root, and it would be nice to support
image-tools for unprivileged users ;). If there is a lot of
resistance to baking in the default, I'm fine adding --(no-)same-owner
and --(no-)same-group to follow tar and make these optional.

do you have any real example of a use case where you don't need to be root to unpack a rootfs with the correct ownership? I may be missing something...

again, the uid == 0 is an ugly hack to me.

runcom · 2016-09-15T17:46:16Z

Sometimes you cannot become root, and it would be nice to support
image-tools for unprivileged users ;).

you don't support "image-tools" - validate works w/o privileges for instance. if any of the tool needs privileges to run properly let's just document it instead of them running "unpack" with a normal user and having ownership broken on the image files.

wking · 2016-09-15T17:47:31Z

On Thu, Sep 15, 2016 at 10:43:07AM -0700, Antonio Murdaca wrote:

Sometimes you cannot become root, and it would be nice to support
image-tools for unprivileged users ;). If there is a lot of
resistance to baking in the default, I'm fine adding --(no-)same-owner
and --(no-)same-group to follow tar and make these optional.

do you have any real example of a use case where you don't need to
be root to unpack a rootfs? I may be missing something...

You're about to launch a container as an unprivileged user, and will
be using user namespaces to map your 1000 UID to zero.

runcom · 2016-09-15T17:48:35Z

You're about to launch a container as an unprivileged user, and will
be using user namespaces to map your 1000 UID to zero.

but isn't files ownership broken because you did not chmod correctly when unpacking with an unpriv user?

# this is wrong no?
root@vagrant:/tmp# ls -la ubuntu-unpacked-old/etc/shadow
-rw-r----- 1 root root 652 Sep 15 17:17 ubuntu-unpacked-old/etc/shadow

wking · 2016-09-15T17:54:22Z

On Thu, Sep 15, 2016 at 10:48:36AM -0700, Antonio Murdaca wrote:

You're about to launch a container as an unprivileged user, and will
be using user namespaces to map your 1000 UID to zero.

but isn't files ownership broken…

Nope, the ownership (1000:1000, because I didn't have permission to
chown) is fine.

… because you did not chmod correctly when running with an unpriv
user?

The unpacker should be applying the entry mode regardless of ownership
on the unpacked file. I'm not sure if it does this yet, but it's
orthogonal to this PR.

glestaris · 2016-09-15T21:21:22Z

Not super sure about this, ownership should be preserved regardless of whether the tool is run by root or now. (not sure how docker engine handles this also)

@runcom I understand your concern. I chose this route because tar offers the same behaviour (as @wking pointed out already). I will make it so that there is a flag to disable preserving the ownership.

You're about to launch a container as an unprivileged user, and will be using user namespaces to map your 1000 UID to zero.

@wking, GrootFS is already dealing with image translation for unprivileged users. It's doing so by making a user namespace and setting a UID map (using the newuidmap tool) for the translation. The unprivileged user has CAP_SYS_ADMIN in the namespace and is able to chown all files within a range of UIDs.

@runcom is right. The file that should have been owned by the group shadow inside the container will now be owned by root if all files are blindly chowned to 1000:1000.

That said, and given the complexity of translating images as a non-root user, I think this is not part of this PR. If oci-unpack needs to support such a use case I am happy to migrate some code from GrootFS that deals with that. I am not sure though if oci-unpack (and the rest of the image-tools) is meant to be a complete tool or just a reference implementation of the specs.

glestaris · 2016-09-15T21:23:51Z

wrong button :)

wking · 2016-09-15T21:35:52Z

On Thu, Sep 15, 2016 at 02:21:23PM -0700, George Lestaris wrote:

@wking, GrootFS is already
dealing with image translation for unprivileged users. It's doing so
by making a user namespace and setting a UID map (using the
newuidmap tool) for the translation. The unprivileged user has
CAP_SYS_ADMIN in the namespace and is able to chown all files
within a range of UIDs.

Is that a range of one? From user_namespaces(7):

Either the writing process has the CAP_SETUID (CAP_SETGID)
capability in the parent user namespace.

…
Or otherwise all of the following restrictions apply:
- The data written to uid_map (gid_map) must consist of a single
  line that maps the writing process's effective user ID (group
  ID) in the parent user namespace to a user ID (group ID) in the
  user namespace.
…

If so, I don't see the point (you'll end up with everything owned by
the unpacking user anyway). Here's an unprivileged user namespace
failing to chown a directory to an unmapped user:group:

$ unshare -Ur chown 5:5 /
chown: changing ownership of ‘/’: Invalid argument

@runcom is right. The file that should have been owned by the group
shadow inside the container will now be owned by root if all files
are blindly chowned to the 1000:1000.

This doesn't matter, because unprivileged user namespaces will only
have one effective user/group in them anyway. With no way (that I
know of) to have multiple users inside such a container, owner/group
flattening poses no increased security risk.

wking · 2016-09-15T21:45:43Z

On Thu, Sep 15, 2016 at 02:33:46PM -0700, W. Trevor King wrote:

Thu, Sep 15, 2016 at 02:21:23PM -0700, George Lestaris:

@wking, GrootFS is already
dealing with image translation for unprivileged users. It's doing so
by making a user namespace and setting a UID map (using the
newuidmap tool) for the translation. The unprivileged user has
CAP_SYS_ADMIN in the namespace and is able to chown all files
within a range of UIDs.

Is that a range of one? From user_namespaces(7):

Either the writing process has the CAP_SETUID (CAP_SETGID)
capability in the parent user namespace.

…

Or otherwise all of the following restrictions apply:

The data written to uid_map (gid_map) must consist of a single
line that maps the writing process's effective user ID (group
ID) in the parent user namespace to a user ID (group ID) in the
user namespace.

Ah, you could get around this with /etc/subuid and newuidmap [1](which is presumably setuid root), assuming your sysadmin was kind
enough to install it and allocate you sufficient UIDs. But I still
think we want to support the “I'm just a poor undergrad that nobody
cares about, but I still want to unpack and run this OCI image” use
case ;). More on that in opencontainers/runc#38 and
opencontainers/runc#774, with a number of folks in places that block
setuid software 2.

glestaris · 2016-09-15T22:14:05Z

For GrootFS doing it through newuidmap/newgidmap is an important use case. Otherwise, with the default behaviour of the user namespace, where you can only map your own UID and GID, chown is a noop. As I said before, this looks like a complete different PR/issue to me.

Can someone explain to me what this means:

$ make lint
checking lint
WARNING: deadline exceeded by linter errcheck on cmd/oci-create-runtime-bundle (try increasing --deadline)
WARNING: deadline exceeded by linter structcheck on cmd/oci-create-runtime-bundle (try increasing --deadline)
WARNING: deadline exceeded by linter varcheck on cmd/oci-create-runtime-bundle (try increasing --deadline)

It's apparently the reason why the second commit didn't pass CI. Can someone restart the job or should I re-push something? Is it worth fixing this deadline?

glestaris · 2016-09-15T22:15:58Z

Also, it makes me a bit sad that the preserveOwnership parameter has to be added to so many functions. Does anyone have a better idea to avoid this?

wking · 2016-09-15T22:21:53Z

On Thu, Sep 15, 2016 at 03:15:58PM -0700, George Lestaris wrote:

Also, it makes me a bit sad that the preserveOwnership parameter
has to be added to so many functions. Does anyone have a better idea
to avoid this?

type Unpacker {
bool preserveOwnership
}

func (unpacker *Unpacker) Unpack(…) error {
…
}

wking · 2016-09-15T22:23:51Z

On Thu, Sep 15, 2016 at 03:14:06PM -0700, George Lestaris wrote:

WARNING: deadline exceeded by linter errcheck on cmd/oci-create-runtime-bundle (try increasing --deadline)

I imagine Travis got stuck for too long, and we shoudn't worry about
this until the PR settles back down and collects some LGTMs ;).

wking · 2016-09-15T22:54:30Z

cmd/oci-create-runtime-bundle/main.go

+	typ         string // the type to bundle, can be empty string
+	ref         string
+	root        string
+	noSameOwner bool


To avoid double-negatives, I'd prefer sameOwner.

wking · 2016-09-15T22:58:52Z

cmd/oci-create-runtime-bundle/main.go

@@ -81,6 +82,11 @@ func newBundleCmd(stdout, stderr *log.Logger) *cobra.Command {
 It is strongly recommended to keep the default value.`,
 	)

+	cmd.Flags().BoolVar(
+		&v.noSameOwner, "no-same-owner", false,
+		`Extract files as yourself instead of preserving the owner and group of each entry.`,


This would have to get turned around with a positive --same-owner. Do cobra's boolean flags provide a way to set on/off?

The problem with having it --same-owner is that the default behaviour will be ignoring the ownership. The user will need to explicitly provide the flag, which sounds error-prone.

What do you mean by setting the flag on/off?

wking · 2016-09-15T23:01:06Z

image/manifest.go

+
+	case tar.TypeDir:
+		if err := os.Chown(path, hdr.Uid, hdr.Gid); err != nil {
+			return errors.Wrap(err, "error chowing directory")


Probably not worth splitting cases just for file/directory in the error message. Can we roll this in with tar.TypeReg? If the Chown error doesn't already include it (I haven't checked), it would be nice to include the path.

Agreed, I am not a big fun of it either. I would even go one step further into dropping the switch altogether§ and calling Lchown for every entry (file, directory or symlink).

wking · 2016-09-15T23:02:50Z

image/manifest_test.go

@@ -112,7 +112,7 @@ func TestUnpackLayer(t *testing.T) {
 			Digest:    fmt.Sprintf("sha256:%s", fmt.Sprintf("%x", h.Sum(nil))),
 		}},
 	}
-	err = testManifest.unpack(newPathWalker(tmp1), filepath.Join(tmp1, "rootfs"))
+	err = testManifest.unpack(newPathWalker(tmp1), filepath.Join(tmp1, "rootfs"), false)


Can we get a new test for preserveOwnership == true? I'm not sure what Go's test framework includes for skipping tests, but it would be nice to run that if the tests were run as root.

runcom · 2016-09-16T08:12:50Z

But I still
think we want to support the “I'm just a poor undergrad that nobody
cares about, but I still want to unpack and run this OCI image” use
case ;).

That is just misleading, as @glestaris pointed out above, you'll end up having wrong ownership on files and I don't think that's wanted - well, you're undergrad could probably go and learn how to do things right maybe.
I can see people opening issues because "my rootfs is wrong because I did run the unpack w/o root and my file has group "root" instead of "shadow".

However, the inverted logic with a flag is working for me /cc @vbatts

wking · 2016-09-16T09:06:56Z

On Thu, Sep 15, 2016 at 11:56:28PM -0700, George Lestaris wrote:

+func applyOwnership(path string, hdr *tar.Header) error {
switch hdr.Typeflag {

case tar.TypeSymlink:
if err := os.Lchown(path, hdr.Uid, hdr.Gid); err != nil {
    return errors.Wrap(err, "error chowing symlink")
}
case tar.TypeReg, tar.TypeRegA:
if err := os.Chown(path, hdr.Uid, hdr.Gid); err != nil {
    return errors.Wrap(err, "error chowing file")
}
case tar.TypeDir:
if err := os.Chown(path, hdr.Uid, hdr.Gid); err != nil {
    return errors.Wrap(err, "error chowing directory")
Agreed, I am not a big fun of it either. I would even go one step
further into dropping the switch altogether§ and calling Lchown
for every entry (file, directory or symlink).

Always calling Lchown sounds good to me.

wking · 2016-09-16T09:09:46Z

On Thu, Sep 15, 2016 at 11:54:01PM -0700, George Lestaris wrote:

cmd.Flags().BoolVar(
&v.noSameOwner, "no-same-owner", false,
`Extract files as yourself instead of preserving the owner and group of each entry.`,
The problem with having it --same-owner is that the default
behaviour will be ignoring the ownership. The user will need to
explicitly provide the flag, which sounds error-prone.

You could have the default behavior switch on root-ness, and the flag
would let you override it either way (just like tar). Tar works
reasonably well, so I think that's a good pattern to follow ;).

What do you mean by setting the flag on/off?

If you add a ‘same-owner’ boolean, do you get support for --same-owner
/ --no-same-owner? Or --same-owner=y / --same-owner=n? Or some such?
That lets you explicitly flip the switch in both directions. Or are
cobra's boolean flags only one-way?

runcom · 2016-09-16T09:13:52Z

If you add a ‘same-owner’ boolean, do you get support for --same-owner
/ --no-same-owner? Or --same-owner=y / --same-owner=n? Or some such?
That lets you explicitly flip the switch in both directions. Or are
cobra's boolean flags only one-way?

BoolVarT is what you're looking for I think

wking · 2016-09-16T09:19:34Z

On Fri, Sep 16, 2016 at 01:12:51AM -0700, Antonio Murdaca wrote:

But I still
think we want to support the “I'm just a poor undergrad that nobody
cares about, but I still want to unpack and run this OCI image” use
case ;).

That is just misleading, as @glestaris pointed out above, you'll end
up having wrong ownership on files and I don't think that's wanted -
well, you're undergrad could probably go and learn how to do things
right maybe.

What do you want them to do, kidnap the sysadmin and force them to
install a setuid binary on the department cluster (note: not
recommended)? Sometimes users are on systems where they don't have
root access or the ear of anyone who does. I think we still want to
support them.

I can see people opening issues because "my rootfs is wrong because
I did run the unpack w/o root and my file has group "root" instead
of "shadow".

It's not wrong if you have only one GID to use. Say you had the power
to chown the file on unpack (setuid root oci-unpack?) and unpacked an
image that used two different groups for it's files. Now you launch
your container with a user namespace, mapping yourself to root. In
that case, at least one of the files you unpack is going to have the
unmapped ‘nobody’ group, which is probably not what the image author
indended. And if you happen to be user 1234 on your system and the
image-author set up things owned by root, everything is going to be
nobody:nobody inside your user namespace.

If unprivileged users have the image content automatically translated
to belong to them, then all of the single-mapping user-namespace cases
work except the ones that require multiple users/groups. There was no
way for unprivileged users to support them anyway, so that's not a big
loss. Folks who have newuidmap installed setuid can also install a
setuid unpacking script (in which case they're effectively root as far
as this issue is concerned).

wking · 2016-09-16T09:21:57Z

On Fri, Sep 16, 2016 at 02:13:53AM -0700, Antonio Murdaca wrote:

If you add a ‘same-owner’ boolean, do you get support for --same-owner
/ --no-same-owner? Or --same-owner=y / --same-owner=n? Or some such?
That lets you explicitly flip the switch in both directions. Or are
cobra's boolean flags only one-way?

BoolVarT is what you're looking for I think

Maybe? Can you link to docs? I can't turn any up…

runcom · 2016-09-16T09:28:31Z

Maybe? Can you link to docs? I can't turn any up…

it's BoolTFlag but it's only in https://github.com/urfave/cli, myb

glestaris · 2016-09-16T18:01:23Z

I pushed a new set of commits.

@wking it does not include the test for the ownership preservation case. I will try to add it in the next few days if you still think it's important.

@runcom is correct. The BoolTFlag type of behaviour is not supported by cobra CLI. Instead, I use the --same-owner as a flag only when the command is run by a non-root user. Hence the behaviour now is:

When run by root: entries will be chowned
When run by a non-root user: entries will not be chowned
When run by a non-root user with the --same-owner flag: entries will be chowned

glestaris · 2016-10-03T22:06:01Z

Interesting point @cyphar! I feel awkward hijacking the PR for this discussion so I made an issue where we can continue: #33.

wking · 2016-10-03T22:11:10Z

On Mon, Oct 03, 2016 at 02:50:44PM -0700, Aleksa Sarai wrote:

Case 2 actually can also involve when you have a mapping which is
smaller than the number of UIDs used within the image -- which is
sort of a mix between case 1 and case 2…

So for a given UID there are three cases:

It's mapped in the current user namespace, and the caller has
permission to chown files to that UID.
It's mapped in the current user namespace, and the caller does not
have permission to chown files to that UID.
It's not mapped in the current user namespace.

But I don't think we need much special handling beyond 7d9350e for
this. When sameOwner is set, we'll fail with a chown permission error
on either 2 or 3. When sameOwner is not set, you're fine because
you're never calling chown.

The only thing I think we're missing is a --no-same-owner, which you
could use to avoid erroring if the tarball contained instances of 2.
If you don't have enough local UIDs to cover the tarball's needs, I
don't see a point in gracefully degrading (how would you know which
IDs to flatten?). You could provide switches to explicitly control
the mapping, but I'd rather avoid that complication and jump straight
to a single user filesystem. In practice, I expect it will be rare
that a user has a multiple available UIDs but not enough to cover the
container.

cyphar · 2016-10-03T22:20:48Z

@wking

When sameOwner is set, we'll fail with a chown permission error on either 2 or 3.

You get EINVAL in case 3.

wking · 2016-10-03T22:25:57Z

On Mon, Oct 03, 2016 at 03:20:49PM -0700, Aleksa Sarai wrote:

@wking

When sameOwner is set, we'll fail with a chown permission error on either 2 or 3.

You get EINVAL in case 3.

Ah, right. That may make the cases easier to distinguish, but I think
the sameOwner invocations should die on chown errors regardless of
what the error is.

vbatts · 2016-10-06T16:37:42Z

besides a rebase, what is needed here?
@glestaris @runcom

glestaris · 2016-10-06T20:40:09Z

@vbatts I did the rebasing Pending the CI to run I think it should be alright. The latest side discussion on UID translation was redirected to #33. @runcom are you okay with the current interface/behaviour?

This PR can be summed up to the following logic:

When run by root: entries will be chowned to the defined UID/GID
When run by a non-root user: entries will not be chowned
When run by a non-root user with the --same-owner flag: entries will be chowned to the defined UID/GID

glestaris · 2016-10-06T20:47:26Z

I don't understand the CI failures :P

It fails in .gitvalidation in either case, but, when run locally:

❯ git-validation -v -run DCO,short-subject,dangling-whitespace -range HEAD~3..HEAD -v
 * d25c148 "Add flag to configure ownership preservation" ... PASS
  - PASS - commit does not have any whitespace errors
  - PASS - has a valid DCO
  - PASS - commit subject is 72 characters or less! *yay*
 * c79195f "Preserve ownership when unpacking image layers" ... PASS
  - PASS - commit does not have any whitespace errors
  - PASS - has a valid DCO
  - PASS - commit subject is 72 characters or less! *yay*
 * 0ff99eb "Extract unpacking functionality to a struct" ... PASS
  - PASS - commit does not have any whitespace errors
  - PASS - has a valid DCO
  - PASS - commit subject is 72 characters or less! *yay*

glestaris · 2016-10-25T20:40:34Z

I rebased this again :) Unfortunately, I still can't make make .gitvalidation happy in CI. It works fine on my machine :D Does anyone know what's going wrong?

Ping @vbatts @jonboulle @stevvooe PTAL

wking · 2016-10-25T21:55:18Z

On Tue, Oct 25, 2016 at 01:40:34PM -0700, George Lestaris wrote:

Does anyone know what's going wrong?

I bet your error 1 is another instance of Travis not setting
$TRAVIS_COMMIT_RANGE 2. I can generate a 128 exit code with:

$ git log ''
fatal: ambiguous argument '': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git [...] -- [...]'
$ echo $?
128

Ping @vbatts.

cyphar · 2016-11-05T16:04:15Z

I would like to eventually see image.Unpacker take a set of ID mappings for user and group. But that's just me. Overall this LGTM (though I feel that we should make the default zero value of image.Unpacker preserve the ownership -- which is the "expected" thing to do).

@runcom @vbatts Quickly regarding whether or not it is ever correct to extract a rootfs without changing the owner properly (as I mentioned in #33): I think you're missing the point. What matters to someone who is creating these images is that I can set the correct owners inside the diff layers (whether or not I had to cheat to do it is a separate issue -- and outside the concern of the extraction API).

In particular, if you look at rpmbuild you'll notice that part of the rpm spec file lets you specify what the translation should be for ownership. So even though the build process is run by an unprivileged user, you still get an rpm which has files owned by root. That's something that we need our extraction tooling to be able to handle (so that I can make umoci work within OBS and KIWI -- build systems where you don't have root). Not to mention the rootless container case (which I must admit is quite close to my heart).

glestaris · 2016-11-07T22:45:24Z

@cyphar that could be an easy next step. Given how stalled this PR is I don't want to add more context. I am willing to PR adding UID/GID translation once this is merged.

At this point, though, approaching the two-month mark, I start to wonder if it would be better to close this and create a fresh one without the 67 comments which may or may not help maintainers get to the bottom of it faster :)

Signed-off-by: George Lestaris <glestaris@gmail.com>

The `--same-owner` flag can be used to force ownership preservation when `oci-create-runtime-bundle` or `oci-unpack` are run by a non-root user. When run by root, both commands with preserve ownership by default. Signed-off-by: George Lestaris <glestaris@gmail.com>

cyphar · 2016-11-08T10:56:57Z

@glestaris Fair enough. For what it's worth I'm going to implement it my way inside umoci and then I'll work on merging the components here. Personally I feel like some of the side-tracking on these threads is getting tiresome -- especially when it comes to us focusing on over-engineering a solution rather than just implementing the damn thing.

stevvooe · 2016-11-18T20:19:09Z

image/unpacker.go

+type Unpacker struct {
+	// If set, the ownership of files, directories and symblic links defined in
+	// the layers, will be preserved in the unpacked root file system.
+	PreserveOwnership bool


Might it be better that this takes a function that translates the entries, rather than just a bool? There are a lot of different ways to do it and make this work with this bool may be a little awkward.

PreserveOwnership, the default (I guess) would this:

func PreserveOwnship(uid, gid string) (string, string, error) { u, err := os/user.Current() if err { return "", "", err } return u.Uid, u.Gid, nil }

Result of this function can be memoized to minimize the number of calls during the unpacking process.

@cyphar has something closer to this in cyphar/umoci@9f88dd9a, see here. I'm fine with that as an end goal, but this PR is certainly better than what we have now. I'm fine with it landing while someone works up a PR for an explicit ID-mapping approach.

Introducing the --same-owner tag introduces an oversimplification to this problem that may be limiting. This model is getting enshrined into the depths of this package, which is the path to permanence.

This needs to be fixed before this PR lands.

I'd like to note that actually the implementation of a --same-owner flag is harder than you might think, and it's not just a matter of chowning all of the files to the same owner. I don't really mind how the mapping of users is done (though I feel like the functional approach will end up being overkill when what you actually want is to have explicit rspec.LinuxIDMapping with an option to ignore errors [meaning to default to root] during the mapping).

If you're an unprivileged user (which is the goal of this PR) you have a bunch of restrictions that mean you have to hack around things like files which give you no read permission even though you are their owner.

On Fri, Nov 18, 2016 at 06:41:55PM -0800, Aleksa Sarai wrote:

I'd like to note that actually the implementation of a --same-owner flag is harder than you might think, and it's not just a matter of chowning all of the files to the same owner.

What's wrong with this PR's current --same-owner implementation? It's not chowning files to the same owner, it's just not chowning them at all. Can you propose a test exposing flaws in that approach?

On Fri, Nov 18, 2016 at 08:27:16PM -0800, Aleksa Sarai wrote:

% chmod 0000 some

That does looks awkward, but this PR is just about ownership. Mode bits seem orthogonal, although both are mentioned in #17. Since this PR is already two months old, I'd rather limit the scope creep. We can always address mode bits in follow-up work.

That does looks awkward, but this PR is just about ownership.

Sure. But my point was that unprivileged unpacking has problems that are beyond just coming up with a nice mapping function. It was an aside, not a complaint about the current PR.

I'd rather limit the scope creep.

Pot, meet kettle.

@cyphar While the IDMapping approach is sufficient, a callback function seems like a first step in getting this right without propagating the data structure with a bool too widely. There may be lots of ways to map these ids in the future but this is orthogonal to how they're applied.

Indeed, there is more complexity to applying uid/gid, but I don't see how this affects where we get them from.

@stevvoe I don't mind making PreserveOwnership a callback. What changes would you like in the CLI? The --same-owner is mostly inspired by the tar CLI.

@glestaris Making this a callback/interface is sufficient.

I was just worried about having policy embedded deep in the unpacking layer.

@cyphar

Bump to v0.1.0. Vote was +7 -0 #3. Closes #116 LGTMs: @cyphar @coolljt0725 @vbatts

sumitsanghrajka · 2018-09-13T21:04:38Z

@glestaris wondering what happened to this patch?

cyphar · 2018-09-13T23:00:34Z

It hasn't been worked on in a while. In the meantime you now have tools like umoci which handle this and other things much better. I would recommend using those for general purpose image manipulation. There was a plan to merge most of these changes back into image-tools but those discussions stalled very quickly. (Disclaimer: I wrote umoci.)

caniszczyk · 2018-09-14T16:03:39Z

Aleksa, have you thought about just moving umoci to oci?

…

On Thu, Sep 13, 2018 at 6:00 PM Aleksa Sarai ***@***.***> wrote: It hasn't been worked on in a while. In the meantime you now have tools like umoci <https://github.com/openSUSE/umoci> which handle this and other things much better. I would recommend using those for general purpose image manipulation. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD5IeXtudbols_BEaZq3UShwPyYtwfRks5uauOSgaJpZM4J-H6j> .

-- Cheers, Chris Aniszczyk http://aniszczyk.org +1 512 961 6719

vbatts · 2018-09-14T16:06:19Z

I feel like he has proposed that and some folks weren't in favor. On Fri, Sep 14, 2018, 12:03 Chris Aniszczyk <notifications@github.com> wrote:

…

Aleksa, have you thought about just moving umoci to oci? On Thu, Sep 13, 2018 at 6:00 PM Aleksa Sarai ***@***.***> wrote: > It hasn't been worked on in a while. In the meantime you now have tools > like umoci <https://github.com/openSUSE/umoci> which handle this and > other things much better. I would recommend using those for general purpose > image manipulation. > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > < #3 (comment) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AAD5IeXtudbols_BEaZq3UShwPyYtwfRks5uauOSgaJpZM4J-H6j > > . > -- Cheers, Chris Aniszczyk http://aniszczyk.org +1 512 961 6719 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEF6RXD6rY76KgZ73sUsX02ccVC6hdXks5ua9NdgaJpZM4J-H6j> .

caniszczyk · 2018-09-14T16:10:47Z

times have changed though, wonder if it would warrant another discussion On Fri, Sep 14, 2018 at 11:06 AM Vincent Batts <notifications@github.com> wrote:

…

I feel like he has proposed that and some folks weren't in favor. On Fri, Sep 14, 2018, 12:03 Chris Aniszczyk ***@***.***> wrote: > Aleksa, have you thought about just moving umoci to oci? > > On Thu, Sep 13, 2018 at 6:00 PM Aleksa Sarai ***@***.***> > wrote: > > > It hasn't been worked on in a while. In the meantime you now have tools > > like umoci <https://github.com/openSUSE/umoci> which handle this and > > other things much better. I would recommend using those for general > purpose > > image manipulation. > > > > — > > You are receiving this because you are subscribed to this thread. > > Reply to this email directly, view it on GitHub > > < > #3 (comment) > >, > > or mute the thread > > < > https://github.com/notifications/unsubscribe-auth/AAD5IeXtudbols_BEaZq3UShwPyYtwfRks5uauOSgaJpZM4J-H6j > > > > . > > > > > -- > Cheers, > > Chris Aniszczyk > http://aniszczyk.org > +1 512 961 6719 > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > < #3 (comment) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AAEF6RXD6rY76KgZ73sUsX02ccVC6hdXks5ua9NdgaJpZM4J-H6j > > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD5ITAMe_tgunHFkkQgg9gLMfZcGzWVks5ua9P8gaJpZM4J-H6j> .

-- Cheers, Chris Aniszczyk http://aniszczyk.org +1 512 961 6719

jonboulle · 2018-09-14T16:14:50Z

https://groups.google.com/a/opencontainers.org/forum/#!topic/dev/GZ2q3jGlI0w

Honestly we should just deprecate image-tools and if Aleksa is still open to it then umoci should be the official replacement

cyphar · 2018-09-15T11:22:34Z

I wouldn't have a problem with re-opening the discussion of merging umoci into OCI again. In the past two years, umoci has progressed quite a bit and I'm still happily working on some new stuff within it. My main two questions would be:

Currently umoci only has one maintainer (me). How should we pick the rest of the maintainers? Is it entirely up to me, or is there a process for picking other maintainers (I'd suggest it be based on contributions and who is willing to be a maintainer)?
Would the project still be free to experiment with more experimental things (like the OCIv2 discussions that I've started having -- where we can build a better underlying format and use umoci as the testbench for it before pushing a proposal for standardisation)?

cyphar · 2018-09-17T23:13:50Z

/ping @caniszczyk ^^

Did you want to discuss this on the ML again?

caniszczyk · 2018-09-17T23:20:23Z

@cyphar I'm happy to kickstart the discussion, I see two approaches:

new subproject of OCI (will require @opencontainers/tob approval)
merge into image-tools (wouldn't require TOB approval necessary)

cyphar · 2018-09-18T08:03:55Z

@caniszczyk I would prefer (1), but the key question about (2) is how would that work -- would the idea be that we just copy over the entire project and rename it? Is this not sort of abusing the rules for project inclusion 😉?

glestaris closed this Sep 15, 2016

glestaris reopened this Sep 15, 2016

wking suggested changes Sep 15, 2016

View reviewed changes

wking mentioned this pull request Oct 18, 2016

image/manifest: Recursively remove pre-existing entries when unpacking #42

Merged

glestaris added 3 commits November 7, 2016 23:02

Extract unpacking functionality to a struct

008617f

Signed-off-by: George Lestaris <glestaris@gmail.com>

Preserve ownership when unpacking image layers

f2ef884

Signed-off-by: George Lestaris <glestaris@gmail.com>

stevvooe reviewed Nov 18, 2016

View reviewed changes

xiekeyang mentioned this pull request Feb 16, 2017

Bump version for v0.1.0 #116

Merged

cyphar added a commit that referenced this pull request Mar 2, 2017

merge branch 'pr-116'

126c1bc

Bump to v0.1.0. Vote was +7 -0 #3. Closes #116 LGTMs: @cyphar @coolljt0725 @vbatts

glestaris closed this Oct 4, 2020

Apply ownership in unpacked entries #3

Apply ownership in unpacked entries #3

Conversation

glestaris commented Sep 15, 2016

runcom commented Sep 15, 2016 • edited Loading

wking commented Sep 15, 2016 • edited Loading

runcom commented Sep 15, 2016 • edited Loading

wking commented Sep 15, 2016

wking commented Sep 15, 2016

wking commented Sep 15, 2016

runcom commented Sep 15, 2016 • edited Loading

runcom commented Sep 15, 2016

wking commented Sep 15, 2016

runcom commented Sep 15, 2016 • edited Loading

wking commented Sep 15, 2016

glestaris commented Sep 15, 2016 • edited Loading

glestaris commented Sep 15, 2016

wking commented Sep 15, 2016

wking commented Sep 15, 2016

glestaris commented Sep 15, 2016 • edited Loading

glestaris commented Sep 15, 2016

wking commented Sep 15, 2016

wking commented Sep 15, 2016

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

runcom commented Sep 16, 2016 • edited Loading

wking commented Sep 16, 2016

wking commented Sep 16, 2016

runcom commented Sep 16, 2016

wking commented Sep 16, 2016

wking commented Sep 16, 2016

runcom commented Sep 16, 2016

glestaris commented Sep 16, 2016

glestaris commented Oct 3, 2016

wking commented Oct 3, 2016

cyphar commented Oct 3, 2016

wking commented Oct 3, 2016

vbatts commented Oct 6, 2016

glestaris commented Oct 6, 2016 • edited Loading

glestaris commented Oct 6, 2016

glestaris commented Oct 25, 2016

wking commented Oct 25, 2016

cyphar commented Nov 5, 2016 • edited Loading

glestaris commented Nov 7, 2016

cyphar commented Nov 8, 2016

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

sumitsanghrajka commented Sep 13, 2018

cyphar commented Sep 13, 2018 • edited Loading

caniszczyk commented Sep 14, 2018 via email

vbatts commented Sep 14, 2018 via email

caniszczyk commented Sep 14, 2018 via email

jonboulle commented Sep 14, 2018

cyphar commented Sep 15, 2018

cyphar commented Sep 17, 2018 • edited Loading

caniszczyk commented Sep 17, 2018

cyphar commented Sep 18, 2018

runcom commented Sep 15, 2016 •

edited

Loading

wking commented Sep 15, 2016 •

edited

Loading

runcom commented Sep 15, 2016 •

edited

Loading

runcom commented Sep 15, 2016 •

edited

Loading

runcom commented Sep 15, 2016 •

edited

Loading

glestaris commented Sep 15, 2016 •

edited

Loading

glestaris commented Sep 15, 2016 •

edited

Loading

runcom commented Sep 16, 2016 •

edited

Loading

glestaris commented Oct 6, 2016 •

edited

Loading

cyphar commented Nov 5, 2016 •

edited

Loading

cyphar commented Sep 13, 2018 •

edited

Loading

cyphar commented Sep 17, 2018 •

edited

Loading