Set workable permissions on OCI -> sandbox rootless builds

[dtrudg]

Description of the Pull Request (PR):

We previously used a patched version of image-tools to extract OCI layers into a rootfs. This was patched to ensure all files and dirs extracted from a layer had minimum permissions such that when performing a rootless sandbox build:

1. overwrites from subsequent layers succeeded even over files with 000 or similar permissions
2. the rootfs could be moved and deleted by the non-root user/owner

In 3.4.0 we switched to umoci for extractions, as it is more faithful to the content of the layers than image-tools. umoci handles (1) above. However, we need to fix (2) ourself, as umoci will faithfully create a rootfs with 000 files in it, which cannot be moved or trivially rm'd by the non-root user.

This fixes or addresses the following GitHub issues:

• Fixes Restrictive permissions on non-root sandbox - build failures / cannot delete #4524 (which was split from sandbox directory cannot be removed if original container has non-writeable directories #4517)

Before submitting a PR, make sure you have done the following:

• Read the Guidelines for Contributing, and this PR conforms to the stated requirements.
• Added changes to the CHANGELOG if necessary according to the Contribution Guidelines
• Added tests to validate this PR and tested this PR locally with a make testall
• Based this PR against the appropriate branch according to the Contribution Guidelines
• Added myself as a contributor to the Contributors File

Attn: @singularity-maintainers

[jmstover]

LGTM

[ikaneshiro]

looks good. Fixes the issue when testing locally for me

[DrDaveD]

I did a build with this patch to test it, and it works with setuid-root singularity, but I still get this error when I run with -u:

INFO:    Starting build...
INFO:    Building into existing container: /cloud/login/dwd/scratch/centos7-sandbox
FATAL:   While performing build: failed to retrieve path for /cloud/login/dwd/scratch/centos7-sandbox: lstat /cloud/login/dwd/scratch/centos7-sandbox: no such file or directory

[dtrudg]

@DrDaveD - thanks... I think that is another somewhat separate issue. I think I need to ask @cclerget to look at that . I'll try to split the different issues out into separate things tomorrow morning.

[dtrudg]

Noting here that I chatted with @mem out of band on this, with particular reference to the comment above.

The current state of this work is that:

• This code does fix the Restrictive permissions on non-root sandbox - build failures / cannot delete #4524 and ERROR: build: failed to make environment files: open /tmp/sbuild-468665265/fs/etc/resolv.conf: permission denied #4532 issues, and could be applied by someone with an urgent need (or they can roll back to 3.3).
• Due to the way that master incorporates Do not move rootfs around for sandbox #4400 we can build a centos7 sandbox as non-root on master without the build failing due to permission issues regardless of the location of TMPDIR. This will be much more faithful to the original docker image than if we mangle permissions - which is probably a very good thing.
• Doing the above does mean you may end up with a sandbox you have to manually chmod before you delete, but perhaps that is acceptable - or we could put in a more specific permissions fix for certain non-root sandbox circumstances, rather than all non-root OCI rootfs assemblies.
• Whichever way we go for OCI, let's think about how we end up with perm differences for OCI vs other routes (@mem's def file example) and consider if we should tackle anything there.

Upshot is that I don't think this should be rushed for a 3.4.2. It is probably wise there is some thought on it over the weekend, and that it is picked up in the working group meeting Sylabs developers have, and we look to comment from other developers here too.

[dtrudg]

There has been some general discussion about the permissions issue, thinking about the context of changes for 3.4.2 and a future 3.5.0. To summarize, general consensus is that:

1. Not mangling permissions would be the 'right' thing to do, to ensure that a sandbox is faithful to the upstream layer content. Switching to umoci was partly to pursue this aim (ref Importing docker image results in incorrect file permissions #3880). A faithful sandbox should be an ultimate goal.
2. Incorporating the non-relocation of sandboxes from master into a 3.4.2 would prevent build failures, create faithful sandboxes, but leave them unable to be removed without changing permissions. This is against user expectations since 2.x and we have many users who are likely unfamiliar with the consequences of owned, but 000 files, leaving them to believe that faithful permissions are an 'error' in our extraction.

Note that with regard to the above, as @mem has pointed out above, you can create true 000 files in a sandbox via build from definition file etc. which would lead to the 'surprise' when a user tries to copy/move/delete a sandbox. Therefore we have inconsistency here, and the 'correct' way to resolve it agrees with (1)... that we should be faithful to permissions specified in any source.

With these points in mind, we plan to move forward to:

1. Bring back permission mangling to 3.4.2 to restore the previous behavior so that there is no surprise for users in the 3.4 series. This will likely be accomplished via a (revised?) version of this PR... to the 3.4 branch only.
2. Do not mangle permissions in master. Raise the topic with the community via all routes. Explore if we can e.g. warn users about 000 perms and consequences when building a sandbox, and/or provide a command to remove such a sandbox. Take the temperature of the community for the balance of 'faithful sandbox permissions' vs 'no surprises'.

[dtrudg]

Setting WIP on this as need to ensure the perm change only comes in on sandbox destinations, and not in oci->sandbox->sif intermediate step to avoid regressing on #3880 in sif (via a different mechanism)

[dtrudg]

Well - turns out we cant do the perm change on only sandbox destinations as we will hit #4532 then. Will revert to the full <=3.3. behavior of modifying perms directly on OCI extraction. This means that with regard to #3880 permissions changes on subsequent layers are honored properly during umoci extraction, but then mangled by us... so may not match 100% docker perms, even in a SIF.

#4532 is going to need to be addressed by different means on master if we are not going to mangle permissions - as the faithful container permissions after extraction prevent insertion of some files we put / modify in the container.

[mem]

It looks good for what we need right now.

Note that because walk is doing a depth-first traversal, and because readDirNames is reading all the directory entries at once, this is going to consume quite a bit of memory for large and deep directory structures (it's going to allocate memory for all the entries in a directory, recurse into one directory, allocate memory for all the entries there and keep going until it finds a directory without any further subdirectories; only when going back up it's going to start releasing the slices it allocated along the way). No need to fix this right now, but it's worth keeping it in mind.

[DrDaveD]

Note for those looking at this in the future: this fix is only in the 3.4 branch. At this time the master (3.5) branch does not always make directories user-writable (and therefore deletable) and the current plan is to not change the permissions there, even though it is not backward compatible, in order to make it more compatible with other OCI compliant tools.

[dtrudg]

With regard to the above comment, we have discussion sought on the mailing list here: https://groups.google.com/a/lbl.gov/forum/#!topic/singularity/wScrzv7_fxc