Support running dist compilations in unprivileged scenarios #128

Xanewok · 2021-12-15T21:43:35Z

Closes #114
Closes #10
Closes #9

Xanewok · 2021-12-15T22:50:18Z

I'll mark this as ready for review to trigger gitlab runs, sorry for the noise.

Xanewok · 2021-12-15T23:38:52Z

So far so good, the dist test suite on gitlab is passing 2/4 and we need bwrap available to progress further, so that's blocked on paritytech/scripts#368 and updated image deployment for now IIUC.

drahnr · 2021-12-16T06:32:08Z

src/bin/cachepot-dist/build.rs

-        if !nix::unistd::getuid().is_root() || !nix::unistd::geteuid().is_root() {
-            // Not root, or a setuid binary - haven't put enough thought into supporting this, bail
-            bail!("not running as root")
-        }


https://stackoverflow.com/questions/39215025/how-to-check-if-linux-user-namespaces-are-supported-by-current-os-kernel#39722086

I decided to drop that for now, since it wildly depends on the running distro and whatnot

Could you create a separate issue and paste the link next to this TODO?

src/bin/cachepot-dist/build.rs

tests/harness/mod.rs

montekki · 2022-02-01T18:09:42Z

Merged with latest master and fixed CI (almost). I believe that remaining failing targets will be fixed when the logging targets are changed back to info.

The rootless feature seems to work, at least locally for me.

The TODOs and FIXMEs have to be fixed before merging.

So what else has to be done here?

montekki

See above comment.

Xanewok · 2022-02-03T00:45:11Z

I've fixed some of the outstanding issues and leftover debugging bits that I've sprinkled around.

So what else has to be done here?

One key point that I wanted to address is that the current approach was a proof of concept. Because we need to unshare the user namespace in the main thread, I opted for a fork here, which is technically not safe (see the note about it not being async-signal-safe), so that I can conveniently call it from a forked process (the forked thread is turned into the child's main thread). I tried to move out some unsafe functions (e.g. anything that allocates) out of the fork-child block but this approach not bullet-proof and more of a best effort.

To solve that, I wanted to separate another entrypoint for our compilation (think cachepot-dist sandbox) and call that instead. Additionally, we should consider introducing some kind of a run-time switch for the sandboxing implementation (like CACHEPOT_SANDBOX={bubblewrap, userns}), defaulting to the old bubblewrap logic, to err on the safe side.

However, if the test suite agrees, I'd prefer to merge this now and work incrementally from there and possibly do a point release (or an RC, cc @drahnr) to relieve the shipping pressure, which would help unblock me mentally.

Xanewok · 2022-02-03T01:31:58Z

I'm getting this error on our GitLab run:

2022-02-03T01:17:22.121 TRACE [PID 5079] dist worker Waiting on child bubblewrap build with PID 5456
2022-02-03T01:17:22.123 WARN  [PID 5456] dist worker Couldn't set up a build environment for bubblewrap: Failed to mount overlay FS: overlayfs "/tmp/cachepot_dist_testbXfSR7/distsystem/server-builddir/toolchains/12553bee8311c582858a47c1e186c21a69f1c77fe26bbe6628549c4eca67803f",upperdir="/tmp/cachepot_dist_testbXfSR7/distsystem/server-builddir/builds/12553bee8311c582858a47c1e186c21a69f1c77fe26bbe6628549c4eca67803f-1/upper",workdir="/tmp/cachepot_dist_testbXfSR7/distsystem/server-builddir/builds/12553bee8311c582858a47c1e186c21a69f1c77fe26bbe6628549c4eca67803f-1/work" -> "/tmp/cachepot_dist_testbXfSR7/distsystem/server-builddir/builds/12553bee8311c582858a47c1e186c21a69f1c77fe26bbe6628549c4eca67803f-1/target": Operation not permitted (os error 1) ("/tmp/cachepot_dist_testbXfSR7/distsystem/server-builddir/toolchains/12553bee8311c582858a47c1e186c21a69f1c77fe26bbe6628549c4eca67803f": exists, upperdir: exists, workdir: exists, same-fs, target: exists, regular-user)

which can be parsed to

Failed to mount overlay FS: overlayfs (...) Operation not permitted (os error 1)

What comes to mind is that we use 5.10 kernel but native overlayfs support in the user namespace was allowed in 5.11 (and 5.13 if we count the SELinux bug fix).

@rcny do you think it would be feasible to run these using the slightly newer kernel or should we look towards fuse-overlayfs for the time being?

Xanewok · 2022-02-03T01:45:13Z

And it seems that for a change the GHA job does not like us messing with gid_map:

Couldn't set up a build environment for bubblewrap: Failed writing to gid_map

but it has no problems if we write to setgroups and uid_map?

rcny · 2022-02-03T13:41:31Z

@rcny do you think it would be feasible to run these using the slightly newer kernel or should we look towards fuse-overlayfs for the time being?

Opened the issue about updating kernel to 5.15.x LTS.

Can you go with fuse-overlayfs for now? I'm not quite sure when someone from the team would be able to pick the task.

Xanewok · 2022-02-04T23:33:50Z

Can you go with fuse-overlayfs for now?

We talked on Element and decided to move with fuse-overlayfs for now until someone updates the kernel to 5.15 in the meantime.

Xanewok · 2022-02-05T16:54:31Z

Squashed and rebased on top of now merged #140 to clean up the history; I'll work on the fuse-overlayfs and the run-time switch for the sandbox mentioned above.

Xanewok · 2022-02-09T22:59:55Z

I implemented a feature gate behind the CACHEPOT_SANDBOX=userns environment variable - if it's unset or set to anything else, we fall back to using the regular, non-namespaced approach used before. If it's set, however, it uses the fork-then-unshare approach. I tried to simplify the implementation by using only a single UDS and by serializing the entire process output to help me abstract over a simple fn(CompileCommand) -> Result<BuildResult> build function.

I tried to minimize the diff - it might be helpful to run git diff -w to ignore the whitespace changes.

Xanewok · 2022-02-10T00:26:35Z

Okay, this is now really ready to review - I noticed a bug has slipped in and since fixing it in 67bfbee I have managed to get GHA working using user namespaces, so at least we'll have some CI coverage for that feature 🎉

~~GitLab test is marked as allow_failure but marks the GitHub suite incorrectly as failing - should I just remove GitLab test for now until we update the kernel/implement fuse-overlayfs?~~

EDIT: Removed GitLab for now, will bring it back once I close #143

Don't run relevant test in CI for now; we don't want to mark the test suite as red in GH for the time being

montekki · 2022-02-10T13:34:02Z

src/bin/cachepot-dist/build.rs

+            }
+
+            trace!("performing compile");
+            // FIXME: Adapt the notes for the user namespaces


has to be fixed fixme

montekki · 2022-02-10T13:34:57Z

src/bin/cachepot-dist/build.rs

-        if !nix::unistd::getuid().is_root() || !nix::unistd::geteuid().is_root() {
-            // Not root, or a setuid binary - haven't put enough thought into supporting this, bail
-            bail!("not running as root")
-        }


Could you create a separate issue and paste the link next to this TODO?

montekki · 2022-02-10T13:36:56Z

src/bin/cachepot-dist/build.rs

+    // The reason why we need to fork in the first place is that creating
+    // a new user namespace with `CLONE_NEWUSER` is required to be called
+    // from a main thread, which fork() separates the calling thread as one.
+    // FIXME: Redesign this binary to be re-executable like


Also an issue and a link would be appreciated.

montekki

Generally approve, if this is not merged until I recover I will take a closer look

drahnr · 2022-02-10T14:36:38Z

src/bin/cachepot-dist/build.rs

+                Ok(..) => std::process::exit(0),
+                Err(..) => std::process::exit(1),


Printing the error here might be a good idea.

Xanewok · 2022-02-10T18:38:00Z

Added links and error log in fd3e3bd

Xanewok requested a review from drahnr December 15, 2021 21:43

Xanewok force-pushed the igor-integration-tests-in-docker-take2 branch from fa7e58d to 7f64b3d Compare December 15, 2021 22:49

Xanewok marked this pull request as ready for review December 15, 2021 22:50

Xanewok requested review from montekki and a team as code owners December 15, 2021 22:50

Xanewok mentioned this pull request Dec 15, 2021

cachepot-ci: Add bubblewrap for dist compiles/tests paritytech/scripts#368

Merged

drahnr reviewed Dec 16, 2021

View reviewed changes

src/bin/cachepot-dist/build.rs Outdated Show resolved Hide resolved

drahnr mentioned this pull request Dec 17, 2021

Cachepot fails to start server unless run as root #130

Closed

drahnr reviewed Feb 1, 2022

View reviewed changes

tests/harness/mod.rs Outdated Show resolved Hide resolved

montekki reviewed Feb 1, 2022

View reviewed changes

Xanewok changed the title ~~WIP: Support running dist compilations in unprivileged scenarios~~ Support running dist compilations in unprivileged scenarios Feb 3, 2022

Xanewok mentioned this pull request Feb 5, 2022

Prepare for running the dist suite as an unprivileged user #140

Merged

WIP: Implement build sandboxing using unshared user namespaces

79f4862

Xanewok force-pushed the igor-integration-tests-in-docker-take2 branch from 1a1dbff to 79f4862 Compare February 5, 2022 16:52

Xanewok added 4 commits February 9, 2022 21:18

Don't derive Debug when not needed

83f0348

Gate the new unshared user namespace behind CACHEPOT_SANDBOX env var

ced8631

Minimize diff

48b8ab8

Remove a trailing comma

fed82b4

Xanewok mentioned this pull request Feb 9, 2022

Enforce running dist test suite in CI when unprivileged #143

Closed

Clean up CI and temp. allow unprivileged dist tests to fail

0f97b7a

Xanewok requested review from drahnr and montekki February 9, 2022 22:53

Xanewok added 6 commits February 10, 2022 00:11

Simulate allow-failure in GHA

45d7bf9

ci: oops, GHA uses ! for negation

26631cc

Fetch gid correctly and use effective IDs to mimic unshare's behavior

67bfbee

WIP: See if GHA will be fixed now

a95d6d8

Warn if overlay build failed

913662f

Actually, GHA using unprivileged user namespaces works!

af4f4fc

Revert .gitlab-ci.yml

b5386b6

Don't run relevant test in CI for now; we don't want to mark the test suite as red in GH for the time being

montekki reviewed Feb 10, 2022

View reviewed changes

montekki approved these changes Feb 10, 2022

View reviewed changes

drahnr reviewed Feb 10, 2022

View reviewed changes

drahnr approved these changes Feb 10, 2022

View reviewed changes

This was referenced Feb 10, 2022

dist: Improve checking prerequisites for overlayfs builders #144

Open

dist: Refactor user namespace overlayfs build mode to not use fork() #145

Open

Address review feedback

fd3e3bd

Xanewok enabled auto-merge (squash) February 10, 2022 18:38

Xanewok merged commit 2252ea1 into master Feb 10, 2022

Xanewok deleted the igor-integration-tests-in-docker-take2 branch February 10, 2022 18:43

drahnr mentioned this pull request Feb 27, 2023

Port remaining cachepot changesets mozilla/sccache#1620

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support running dist compilations in unprivileged scenarios #128

Support running dist compilations in unprivileged scenarios #128

Xanewok commented Dec 15, 2021 •

edited

Loading

Xanewok commented Dec 15, 2021

Xanewok commented Dec 15, 2021

drahnr Dec 16, 2021

Xanewok Feb 9, 2022

montekki Feb 10, 2022

montekki commented Feb 1, 2022

montekki left a comment

Xanewok commented Feb 3, 2022

Xanewok commented Feb 3, 2022

Xanewok commented Feb 3, 2022

rcny commented Feb 3, 2022

Xanewok commented Feb 4, 2022

Xanewok commented Feb 5, 2022 •

edited

Loading

Xanewok commented Feb 9, 2022

Xanewok commented Feb 10, 2022 •

edited

Loading

montekki Feb 10, 2022

montekki Feb 10, 2022

montekki Feb 10, 2022

montekki left a comment

drahnr Feb 10, 2022

Xanewok commented Feb 10, 2022

		Ok(..) => std::process::exit(0),
		Err(..) => std::process::exit(1),

Support running dist compilations in unprivileged scenarios #128

Support running dist compilations in unprivileged scenarios #128

Conversation

Xanewok commented Dec 15, 2021 • edited Loading

Xanewok commented Dec 15, 2021

Xanewok commented Dec 15, 2021

drahnr Dec 16, 2021

Choose a reason for hiding this comment

Xanewok Feb 9, 2022

Choose a reason for hiding this comment

montekki Feb 10, 2022

Choose a reason for hiding this comment

montekki commented Feb 1, 2022

montekki left a comment

Choose a reason for hiding this comment

Xanewok commented Feb 3, 2022

Xanewok commented Feb 3, 2022

Xanewok commented Feb 3, 2022

rcny commented Feb 3, 2022

Xanewok commented Feb 4, 2022

Xanewok commented Feb 5, 2022 • edited Loading

Xanewok commented Feb 9, 2022

Xanewok commented Feb 10, 2022 • edited Loading

montekki Feb 10, 2022

Choose a reason for hiding this comment

montekki Feb 10, 2022

Choose a reason for hiding this comment

montekki Feb 10, 2022

Choose a reason for hiding this comment

montekki left a comment

Choose a reason for hiding this comment

drahnr Feb 10, 2022

Choose a reason for hiding this comment

Xanewok commented Feb 10, 2022

Xanewok commented Dec 15, 2021 •

edited

Loading

Xanewok commented Feb 5, 2022 •

edited

Loading

Xanewok commented Feb 10, 2022 •

edited

Loading