projectatomic / atomicapp

[UNMAINTAINED] This is the reference implementation of the Nulecule container application Specification: Atomic App
102 stars 71 forks source link

When fetching or extracting, set the correct uid + guid #772

Closed cdrage closed 8 years ago

cdrage commented 8 years ago

Issue: Whenever we fetch a container and extract it to a location, it will set the permission as "root".

Solution: By checking if we are in a container as well as running via SUDO we are able to determine what user we are running as. This commit sets the files which are extracting to the corresponding user permission that ran the initial atomicapp whether through the atomicapp CLI or atomic CLI.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.4%) to 57.341% when pulling 086019df0529d6ff9ac975d9952957c502912258 on cdrage:make-sure-we-own-the-dir into d85321322d5d747d7feda8d9edab8d1eefaf810f on projectatomic:master.

cdrage commented 8 years ago

Closes #745

cdrage commented 8 years ago

dotests

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.3%) to 57.245% when pulling 28d6c5819834403842a5c6675d19b293abdaff86 on cdrage:make-sure-we-own-the-dir into d85321322d5d747d7feda8d9edab8d1eefaf810f on projectatomic:master.

cdrage commented 8 years ago

dotests

dustymabe commented 8 years ago

ok I think we are close. there might be a few more places that we need to add the code that does the owner fixups. here is what I get on my system:

[vagrant@f24 projectatomic-guestbookgo-atomicapp-204b4b9f525a]$ ls -lh ./
total 24K
-rw-r--r--. 1 vagrant vagrant  298 Jul 18 20:47 answers.conf.gen
drwxr-xr-x. 4 vagrant vagrant 4.0K Jul 18 20:47 artifacts
-rw-r--r--. 1 vagrant vagrant  313 Jul 18 19:03 Dockerfile
drwxr-xr-x. 3 root    root    4.0K Jul 18 20:47 external
-rw-r--r--. 1 vagrant vagrant  801 Jul 18 19:03 Nulecule
-rw-r--r--. 1 vagrant vagrant 2.1K Jul 18 19:03 README.md

so there is that one root root in there.

cdrage commented 8 years ago

@dustymabe boo. i'll work on that

cdrage commented 8 years ago

Updated, but fixing test error atm with TestNuleculeComponentLoadExternalApplication.test_loading_app_by_unpacking

EDIT: updated the PR based on @dustymabe 's comments. Going to fix the test later (for some reason can't patch a patch, may have to change test completely)

dustymabe commented 8 years ago

LGTM

dustymabe commented 8 years ago

I found a corner case with the getUserName() function. I think we should still merge this PR. This case fails though:

If I am a normal user that has access to the docker daemon (i.e. it is unsecured like on the adb/cdk) I can execute docker run commands without sudo. If I execute docker run without sudo then my egid will be 0 but SUDO_USER env variable will not be set so we hit this case in the if statement: https://github.com/projectatomic/atomicapp/blob/b2bf5caec27e32298009f154253499e515a22de1/atomicapp/utils.py#L391

So we improperly get 'root' ownership on the files.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.3%) to 56.906% when pulling 74ce69cc21abf8e901841f372d3f520b186d3aff on cdrage:make-sure-we-own-the-dir into b2bf5caec27e32298009f154253499e515a22de1 on projectatomic:master.

cdrage commented 8 years ago

@dustymabe added one simple test as well as updated the code. Good for mergin'

cdrage commented 8 years ago

openshift failing = timing issue. gunna run again.

cdrage commented 8 years ago

dotests

dustymabe commented 8 years ago

Feel free to merge when you are happy.