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

build: disallow root without build users #3415

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LnL7
Copy link
Member

@LnL7 LnL7 commented Mar 14, 2020

Currently nix enforces build user separation for root by setting build-users-group = nixbld by default in this case. However there's no validation allowing the user to disable it. For nix-daemon specifically there used to be a check that ensured builds would not run as root, however that was also removed in 98968fb.

A bunch of issues mention this bug as a "workaround" for root installations which is pretty
bad IMHO. And it looks like there are also things in nix that don't account for this behaviour, making it only partially functional.

suspicious ownership or permission on '...'; rejecting this build output

@zimbatm
Copy link
Member

zimbatm commented Mar 16, 2020

related: nix-community#1 which makes root also use the daemon protocol when the nix-daemon is started

@edolstra
Copy link
Member

However there's no validation allowing the user to disable it.

What do you mean with validation?

Allowing the user to disable build isolation is a legitimate use case, especially in containers.

@LnL7
Copy link
Member Author

LnL7 commented Mar 16, 2020

@edolstra I disagree with the premise that user separation is not important for containers. I would argue the opposite, being able to setup an environment needed to run a project including global files, users, etc. is exactly what containers are intended for.

Another example of a project that completely refuses to run as root or setuid is postgresql, preventing users from shooting themselves in the foot with a setup that has horrible security implications. Regardless of whether it might be slightly less bad in certain contexts.

@edolstra
Copy link
Member

@LnL7 The issues you linked to suggest that people really need this (or think they do...). I'm not saying user separation cannot be useful for containers. But generally a container should run a single service (and shouldn't have a writable Nix store).

BTW, we also allow user separation to be disabled for non-root users (e.g. when you do --store ~/my-nix). For most people, that's just as bad for security as disabling user separation for root. (I mean, my user account contains all my actual data - I don't really care about root.) So should we disallow that as well?

@LnL7
Copy link
Member Author

LnL7 commented Mar 16, 2020

@edolstra Yeah, I think this mainly stems from confusion about build user separation and the nix-daemon are separate things that don't depend on eachother.

As for a regular single user install, yes it would be good to make that more clear. However unlike with root there are cases where not enough permissions are available. Sandboxing also plays a role in this ofcourse, if you feel strongly about it I could also include that in the condition.

@domenkozar
Copy link
Member

Maybe we should disable single-user installation for root and advise how to use multi-user installation correctly even the case when there's no need to run the daemon.

@stale
Copy link

stale bot commented Feb 12, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Feb 12, 2021
@InLaw
Copy link

InLaw commented Jul 23, 2021

any update?

@stale stale bot removed the stale label Jul 23, 2021
@stale
Copy link

stale bot commented Apr 16, 2022

I marked this as stale due to inactivity. → More info

@Ericson2314
Copy link
Member

CC @yorickvP because #8017, after/during which is a good chance to document what exactly are the semantics of the build environment. Even if we don't want to make this an error, we could make it a warning.

@stale stale bot removed the stale label Mar 10, 2023
@Ericson2314 Ericson2314 added build-problem Nix fails to compile or test; also improvements to build process stale security Security-related issues labels Mar 10, 2023
@stale stale bot removed stale labels Mar 10, 2023
@Ericson2314
Copy link
Member

In any case, I think a good preperatory step would be making build-users-group std::optional<std::string>, not std::string. @LnL7 would you be interested in doing that?

@Ericson2314
Copy link
Member

Marking as draft because it's old and there comments.

@Ericson2314 Ericson2314 marked this pull request as draft May 13, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-problem Nix fails to compile or test; also improvements to build process security Security-related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants