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

[RFC 0008] Readonly recursive Nix #8

Closed
wants to merge 2 commits into from

Conversation

shlevy
Copy link
Member

@shlevy shlevy commented Apr 2, 2017

@shlevy shlevy force-pushed the readonly-recursive-nix branch 3 times, most recently from aa4c91f to bb53272 Compare April 2, 2017 14:36
@shlevy shlevy force-pushed the readonly-recursive-nix branch from bb53272 to 781648e Compare April 2, 2017 14:37
@shlevy
Copy link
Member Author

shlevy commented Apr 2, 2017

@edolstra

Multiple processes in the build may be trying to open connections in
parallel, but as long as each only consumes one response it doesn't
matter if the response is the exact one corresponding to the specific
request made.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what this means. It seems to say there is some sort of race with multiple connections but I don't see how.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All processes in the build share the same socket FD they use to request a new connection, those requests could beinterleaved.

@edolstra
Copy link
Member

edolstra commented Apr 3, 2017

The idea seems mostly fine. A few comments:

  • The main objection is that it significantly increases the attack surface from Nix builds, since they can now talk to a root process outside of the sandbox.

  • Regarding daemon availability: A simpler approach would be to move all the daemon code from src/nix-daemon to libstore as a Daemon class that can be instantiated by DerivationGoal. So we just fire off a thread to handle incoming connections from the build. This also makes it easy to pass the StoreViewStore to the daemon (Daemon would just take a Store as a constructor argument).

  • I don't see why FdDaemonStore is needed. Wouldn't the regular RemoteStore work? The only thing needed is a way to set the socket path (at least for non-sandbox builds where we can't create /nix/var/nix/daemon-socket/socket).

  • I don't understand the name "StoreViewStore". Maybe RestrictedStore?

@shlevy
Copy link
Member Author

shlevy commented Apr 3, 2017

@edolstra thanks for taking a look!

The main objection is that it significantly increases the attack surface from Nix builds, since they can now talk to a root process outside of the sandbox.

True, I'll include that with the next draft (and the counterpoint that the daemon is already meant to be accessible to everyone in a normal setup)

Regarding daemon availability: A simpler approach would be to move all the daemon code from src/nix-daemon to libstore as a Daemon class that can be instantiated by DerivationGoal. So we just fire off a thread to handle incoming connections from the build. This also makes it easy to pass the StoreViewStore to the daemon (Daemon would just take a Store as a constructor argument).

Sure, makes sense.

I don't see why FdDaemonStore is needed. Wouldn't the regular RemoteStore work? The only thing needed is a way to set the socket path (at least for non-sandbox builds where we can't create /nix/var/nix/daemon-socket/socket).

Inheriting a file descriptor is just an alternative to specifying the socket path.I slightly prefer the former, but not strongly, I'll change this to do it that way instead if you prefer.

I don't understand the name "StoreViewStore". Maybe RestrictedStore?

Yeah, that's btter. Though the more I think about it the more it seems like this is more a parameter of the daemon than an actual standalone store implementation anyway.

@joachifm
Copy link

joachifm commented Apr 3, 2017

and the counterpoint that the daemon is already meant to be accessible to everyone in a normal setup

If I understand correctly, the concern is not necessarily other local users but that the proposal increases the feasibility of attacking the host by injecting malicious code into the build process.

@edolstra
Copy link
Member

edolstra commented Apr 3, 2017

Right. Currently sandboxed builds don't have access to the daemon socket.

@shlevy
Copy link
Member Author

shlevy commented Apr 3, 2017

Right, it's not a complete counterpoint, just that the daemon already should be pretty secure.

@shlevy
Copy link
Member Author

shlevy commented Apr 3, 2017

We could put this behind an off-by-default option.

@Ericson2314
Copy link
Member

I'd like to see some of the comparison with import-from-derivation fleshed out. For example, I'm missing why this is more expressive.

@zimbatm zimbatm changed the title Add readonly-recursive-nix RFC [RFC 008] Add readonly-recursive-nix RFC Apr 4, 2017
@edolstra edolstra changed the title [RFC 008] Add readonly-recursive-nix RFC [RFC 008] Readonly recursive Nix Apr 10, 2017
@zimbatm zimbatm changed the title [RFC 008] Readonly recursive Nix [RFC 0008] Readonly recursive Nix Apr 13, 2017
@shlevy
Copy link
Member Author

shlevy commented Apr 13, 2017

@edolstra @Ericson2314 Updated in response to your comments.

@Ericson2314
Copy link
Member

Ericson2314 commented Apr 16, 2017

@shlevy I'll respond in detail later when I'm not on my phone, but I think all those IFD drawbacks are very surmountable.

@shlevy
Copy link
Member Author

shlevy commented Apr 27, 2017

@edolstra can we expect a response either way from you here?

@edolstra
Copy link
Member

edolstra commented May 1, 2017

This looks good to me, thanks!

@shlevy
Copy link
Member Author

shlevy commented May 1, 2017

@edolstra Awesome! 🎊

@zimbatm So what's the next step from the perspective of the RFC process?

@Ericson2314
Copy link
Member

@edolstra, would you also be OK in an RFC for improving import-from-derivation? They are in many cases competing ways to achieve the same thing, but implementation-wise there is no conflict.

@0xABAB
Copy link

0xABAB commented Jun 30, 2017

I think the RFC document (the one in the first message) is so vague that I have no idea what problems it intends to solve. Just based on that, I am already against.

@zimbatm
Copy link
Member

zimbatm commented Jul 2, 2017

@shlevy the RFC is very dry right now. @edolstra and you can understand it because you have a lot of shared context on the nix implementation.

To help the user's perspective, would you mind adding an example of nix code that would use the feature and then describe the various steps that the runtime would take when invoking the code?

@shlevy
Copy link
Member Author

shlevy commented Jul 2, 2017

@zimbatm good call, will do when I get a chance

@shlevy shlevy closed this Jan 19, 2018
d-goldin added a commit to d-goldin/rfcs that referenced this pull request Nov 20, 2019
infinisil referenced this pull request in nixpkgs-architecture/rfc-140 Jan 30, 2023
Shorten the paths, rename to package.nix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants