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

prune: Use sudo for repo-build #165

Merged
merged 1 commit into from
Oct 11, 2018

Conversation

cgwalters
Copy link
Member

Since it's owned by root.

Since it's owned by root.
@jlebon jlebon merged commit 293080d into coreos:master Oct 11, 2018
@dustymabe
Copy link
Member

this is actually going to cause problems in the future because we won't be able to run sudo in openshift at all. There's no way to make the bare-user files not root owned ?

The bare-user is a later addition that is like bare in that files are unpacked, but it can (and should generally) be created as non-root. In this mode

seems like that should be possible?

@cgwalters
Copy link
Member Author

There's no way to make the bare-user files not root owned ?

See coreos/rpm-ostree#1045

because we won't be able to run sudo in openshift at all.

Sure you can - just try it! It just won't gain the full set of capabilities, but the one we need right now is cap_dac_override.

@cgwalters
Copy link
Member Author

(Now if you're referring to the fact that OpenShift requires dynamic uid allocation by default and does I think suppress the ability to change uid, it's easy for a cluster admin to oc adm policy add-scc-to-user anyuid -z default or so, and this is really pretty safe)

@dustymabe
Copy link
Member

yeah this is what I'm seeing now:

sh-4.4$ sudo su -
sudo: PERM_SUDOERS: setresuid(-1, 1, -1): Operation not permitted
sudo: no valid sudoers sources found, quitting
sudo: setresuid() [0, 0, 0] -> [1000240000, -1, -1]: Operation not permitted
sudo: unable to initialize policy plugin

so if we fix coreos/rpm-ostree#1045 then this need goes away?

@cgwalters
Copy link
Member Author

so if we fix coreos/rpm-ostree#1045 then this need goes away?

No, that's just one of the problems. Running rpm-ostree compose as non-root is a distinct thing from the no --privileged Docker issue.

Now you're intersecting them note - one can run rpm-ostree as non-root outside of a container (or in a container but with the outer one --privileged but dropping privs to invoke it).

Doing both is going to be really quite hard - I am doubtful we can do that soon. I don't think it's unreasonable to get the anyuid granted for this.

@cgwalters
Copy link
Member Author

Though I would say actually you're right in pointing out that the repo can be operated on by non-root. I started on this:

diff --git a/src/prune_builds b/src/prune_builds
index 895e2f7..19866df 100755
--- a/src/prune_builds
+++ b/src/prune_builds
@@ -74,15 +74,18 @@ for build_dir, _ in builds_to_delete:
     shutil.rmtree(os.path.join(builds_dir, build_dir))
 
 # and finally prune OSTree repos
-def ostree_prune(repo_name, sudo=False):
+def ostree_prune(repo_name):
     repo = os.path.join(args.workdir, repo_name)
     argv = []
-    if sudo:
-        argv.extend(['sudo', '--'])
     print(f"Pruning {repo_name}")
+    s = os.lstat(repo)
+    # Currently the build repo can be owned by uid 0.  See
+    # https://github.com/coreos/coreos-assembler/pull/165#issuecomment-429105037
+    if s.st_uid == 0 and os.getuid() != 0:
+        argv.extend(['sudo', '--'])
     argv.extend(["ostree", "prune", "--repo", repo, "--refs-only",
                     f"--depth={args.keep_last_n-1}"])
     subprocess.run(argv, check=True)
 
 ostree_prune('repo')
-ostree_prune('repo-build', sudo=True)
+ostree_prune('repo-build')

But then I realized we're not actually creating the repo owned by root either.

But yeah we can make pruning the repo etc. work as non-root just fine.

@jlebon
Copy link
Member

jlebon commented Oct 12, 2018

Was this patch originally motivated by an error you hit while running prune? I've never actually hit any issues, but of course we have files owned as root so it seemed plausible. But checking now, all we need is for dirs to not be owned by root, and we have:

$ find repo-build -type d -uid 0
repo-build/refs/heads/fedora
repo-build/refs/heads/fedora/29
repo-build/refs/heads/fedora/29/x86_64

which should be fine for prune since we're not setting refs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants