Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unpacked images should add back file permissions for group/other. #349

Closed
GrahamDumpleton opened this issue Mar 11, 2022 · 7 comments · Fixed by #366
Closed

Unpacked images should add back file permissions for group/other. #349

GrahamDumpleton opened this issue Mar 11, 2022 · 7 comments · Fixed by #366
Labels
carvel accepted This issue should be considered for future work and that the triage process has been completed carvel triage This issue has not yet been reviewed for validity enhancement This issue is a feature request good first issue An issue that will be a good candidate for a new contributor priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome.

Comments

@GrahamDumpleton
Copy link

Describe the problem/challenge you have

The imgpkg push -i option supports being able to create OCI artefact images containing arbitrary files. Thus it can serve as the equivalent of hosting a tarball on a website, but where things are stored as an image on an image registry.

As much as this appeals as a general solution for packaging up arbitrary files for distribution it suffers a problem.

The problem is that when you run imgpkg push the directories/files copied into the image only preserve user permissions and group/other permissions are discarded. As such, when using imgpkg pull to download and unpack the image, directories/files get unpacked with only user permission set. For example:

0 drwx------  4 graham  wheel  128  8 Mar 12:44 .
0 drwx------  4 graham  wheel  128  8 Mar 12:44 ..
8 -rw-------  1 graham  wheel   50  1 Jan  1970 Dockerfile
8 -rw-------  1 graham  wheel   13  1 Jan  1970 index.html
8 -rwx------  1 graham  wheel  128  1 Jan  1970 script.sh

This is always the case, and is nothing to do with a user using a restrictive umask. In other words this happens even if the user umask is 022, which should preserve group/other permissions for r-x.

An example of where this is problematic is where the image artefact is used to hold source code files that a user downloads and then needs to run a docker build in to create a runnable container image.

The problem in this case is that if files are copied into the container image using COPY in the Dockerfile they are done so with only user file permissions, but in the container image the files would be owned by root. If the container image is then run as non root it can't access the files copied into the container image.

Describe the solution you'd like

Best solution may be to continue only storing the file permissions in the image, but when unpacking copy the user permissions to group/other, but where whether those permissions persist will depend on the users umask.

Using this approach, where a change is only made in imgpkg pull behaviour, any images created with an older imgpkg version would also work the same. So not dependent on what imgpkg push did.

So instead of the above result, if the umask had been 022, you would get:

0 drwxr-xr-x  4 graham  wheel  128  8 Mar 12:44 .
0 drwxr-xr-x  4 graham  wheel  128  8 Mar 12:44 ..
8 -rw-r--r--  1 graham  wheel   50  1 Jan  1970 Dockerfile
8 -rw-r--r--  1 graham  wheel   13  1 Jan  1970 index.html
8 -rwxr-xr-x  1 graham  wheel  128  1 Jan  1970 script.sh

This is how a Git server effectively works when a fresh copy is checked out.

Anything else you would like to add:

If working in a controlled environment where you know what the umask would be, you can work around the issue if for example you know the umask would be 022, by running:

imgpkg pull -i ... -o ...
chmod -R go=u-w ...

This is okay if being done in a script, but if the user is directly running the imgpkg pull command, it is inconvenient to have to tell them to fix up permissions.


Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help work on this issue.

@GrahamDumpleton GrahamDumpleton added carvel triage This issue has not yet been reviewed for validity enhancement This issue is a feature request labels Mar 11, 2022
@joaopapereira
Copy link
Member

Thanks for the issue.

I think the solution looks good. Going to move this to carvel accepted, if someone thinks this might cause any problem with their setup, just let us know.

@joaopapereira joaopapereira added good first issue An issue that will be a good candidate for a new contributor carvel accepted This issue should be considered for future work and that the triage process has been completed priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome. and removed carvel triage This issue has not yet been reviewed for validity labels Mar 17, 2022
@joaopapereira
Copy link
Member

@GrahamDumpleton if this is blocking you in some way let us know and we can move this to a higher priority

@GrahamDumpleton
Copy link
Author

For my primary use cases where permissions matter, the execution of imgpkg is wrapped up in a script so I am able to fix up permissions myself for now.

@GrahamDumpleton
Copy link
Author

Also asked in Slack, but will this change end up getting inherited by vendir? In other words, does vendir just incorporate the same code go code from imgpkg, or does it perhaps exec imgpkg on command line?

@joaopapereira
Copy link
Member

Has I replied in Slack, vendir consumes imgpkg as a binary, so as soon as we have a new version of imgpkg and you download it to your machine, it should just work with vendir

@joaopapereira joaopapereira removed the carvel triage This issue has not yet been reviewed for validity label Apr 13, 2022
@github-actions github-actions bot added the carvel triage This issue has not yet been reviewed for validity label Apr 13, 2022
@GrahamDumpleton
Copy link
Author

The merged changed doesn't seem to entirely do what is required, but is very close. The problem now is that the root directory permissions aren't set right. Everything else underneath is okay.

$ imgpkg version
imgpkg version 0.28.0
$ imgpkg pull -i registry.default.svc.cluster.local:5001/lab-k8s-fundamentals-files@sha256:4000ba14e21ededec5253c72d393554c3f4549dca8f73965f295cfd1b8d22e14 -o /tmp/xxx
Pulling image 'registry.default.svc.cluster.local:5001/lab-k8s-fundamentals-files@sha256:4000ba14e21ededec5253c72d393554c3f4549dca8f73965f295cfd1b8d22e14'
Extracting layer 'sha256:96600e8a306976ae5368cc15d8809dce296457afcf9de346ffc9120a996228ed' (1/1)

Succeeded

$ ls -las /tmp/xxx
total 64
 4 drwx------  6 eduk8s root  4096 Apr 15 05:17 .
 4 drwxrwxrwt  1 root   root  4096 Apr 15 05:17 ..
 4 -rw-r--r--  1 eduk8s root   779 Jan  1  1970 Dockerfile
...

When run under vendir the permissions were even stranger, having:

4 drwx--S---  6 eduk8s root  4096 Apr 15 05:17 .

@GrahamDumpleton
Copy link
Author

GrahamDumpleton commented Apr 15, 2022

Under vendir that sticky bit for group even gets applied on sub directories.

 4 drwx--S---  4 eduk8s root  4096 Apr 15 05:38 .
 4 drwxrwsrwx  3 root   root  4096 Apr 15 05:38 ..
 4 drwxr-sr-x 10 eduk8s root  4096 Apr 15 05:38 exercises
12 -rw-r--r--  1 eduk8s root 11357 Jan  1  1970 LICENSE
 4 -rw-r--r--  1 eduk8s root   139 Jan  1  1970 README.md
 4 drwxr-sr-x  5 eduk8s root  4096 Apr 15 05:38 workshop
$ vendir version
vendir version 0.26.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel accepted This issue should be considered for future work and that the triage process has been completed carvel triage This issue has not yet been reviewed for validity enhancement This issue is a feature request good first issue An issue that will be a good candidate for a new contributor priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants