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

Mounted SSH Store #7912

Merged
merged 6 commits into from
Nov 21, 2023
Merged

Mounted SSH Store #7912

merged 6 commits into from
Nov 21, 2023

Conversation

mupdt
Copy link
Contributor

@mupdt mupdt commented Feb 27, 2023

Motivation

To implement the issue #7890, a feature request to implement support for out-links (i.e. the addPermRoot feature) in a new remote store called mounted-ssh-ng://. This way users will be able to create out-links on remote stores that share the filesystem with the local host (on which the nix client runs).

Context

It turns out that the concepts of indirect and permanent GC roots were placed in a slightly inconvenient place in the Store abstractions. This change suggests a slight change in the abstraction for indirect and permanent GC roots that makes them more amenable for implementation in more diverse types of stores (such as remote stores).

This change is backwards compatible (old features will work with older nix daemons). It should be noted that the new mounted-ssh-ng:// store requires a newer nix daemon, but this is an opt-in feature whose adoption can be managed with a non-disruptive "daemon first, clients later" upgrade path.

cc @rickynils @edolstra

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

@edolstra
Copy link
Member

We need to consider the security implications since this allows you to create arbitrary symlinks as the remote user, even if the remote user has a forced SSH command that wouldn't normally allow that kind of access to the remote filesystem.

@mupdt
Copy link
Contributor Author

mupdt commented Feb 27, 2023

@edolstra: Ah, that makes sense. I guess this means we would have to make sure the nix daemon doesn't allow addPermRoot but the remote store does. I guess some sort of flag would do here (to enable addPermRoot but keep it disabled by default).

Perhaps a flag on the ssh-ng remote store: ssh-ng://hostname?allow-perm-roots=true? While the nix-daemon service would never have this option enabled. Does that sound reasonable?

Update: The way allow-perm-roots would work is that the remote store will handle addPermRoots command. The remote store not forward the addPermRoots command to the daemon. Instead, the remote store itself will create a symlink (as the SSH user on the remote side) and will ask the daemon to create an indirect root to that symlink. There should be no privilege escalation since nix is reusing the SSH user on the remote side. I hope that makes sense.

@Ericson2314
Copy link
Member

Can we make an addPermRoot that doesn't have an arbitrary path? (Seems related to --indirect or whatever the opposite is?)

@mupdt
Copy link
Contributor Author

mupdt commented Mar 1, 2023

@Ericson2314: You mean the daemon should check if the user who's requesting addPermRoot has the permission to create the given symlink?

We're currently working on a proposal that doesn't need this. In this proposal the daemon will not create symlinks (it will be the client that creates the symlinks). The daemon will only point indirect GC roots to the given symlinks. If all goes well I'll update this PR soon.

@mupdt
Copy link
Contributor Author

mupdt commented Mar 1, 2023

Update: It looks like the solution we're proposing doesn't work. That's because the nix-daemon that is spawned by the client on the remote end is a very thin forwarder of streams (it does not perform any of the operations). I assumed that the client-spawned nix-daemon could perform the addPermRoot command (to create the symlinks) before forwarding the addIndirectRoot further to the daemon. So, this won't work.

Next attempt is to let the SSHStore create the symlinks via ssh (by issuing ln -s commands) and then issue addIndirectRoot request. @edolstra @Ericson2314: Does that sounds reasonable? If not, may I ask for any other suggestions? 🙏

Update 2: Ah, now I discovered that nix-daemon can be convinced quite easily to process ops rather than just forward streams. 🤔 This might do for us.

@Ericson2314
Copy link
Member

Ericson2314 commented Mar 1, 2023

@mupdt A few things

  1. Nevermind what I said above :). It was a half-baked thought, sorry for the noise.

  2. Regardless of this PR, I think we do want to gate access to addPermRoot in LocalFSStore. Right now, we rely on there not being a way to do this in a privileged daemon, by virtue of it not being exposed, and that is poor security / not depth in defense. We should assume a failure mode like (a) your PR is used without the uds:// override or (b) your PR is used with the forwarding ssh-ng:// nix daemon and make sure the privileged daemon will still not make any symlinks.

    Again to be clear, this is me observing something that is in no way your fault :). To me is is an existing weakness in the code base I am just now noticing. (And probably the ultimate solution is so separate a lot of these interfaces into read-only vs read+write halves.)

  3. While your solution is good for you, wouldn't it start trying to do these out links for everyone, even those that don't have the file system mounted? I think would might be better would be to create a mounted-ssh:// store that basically has the same overrides that unix:// does:

    • The ssh client (not middle nix daemon) would create the direct root
    • The remote side would create the indirect root
    • The filesystems have to be the same such that these line up.

Besides cleanly separating out this use-case, you would also get the performance benefits of the other LocalFSStore overrides --- basically leveraging NFS for more things and the ssh connection for fewer things.

How do these sound?

@mupdt
Copy link
Contributor Author

mupdt commented Mar 1, 2023

@Ericson2314: I still have to wrap my head around your suggestion, sorry about that. Just a quick question regarding 3. to check if I understand that part correctly: The indirect root would requires a symlink somewhere in client's directories. However, if the client creates the symlink on the local host, then there is a race condition when the remote side creates an indirect root to the symlink (the symlink might not be there yet). This means the symlink has to be created on the same host where the indirect root is created (to avoid the race). Would you say that this must be done by the middle nix daemon?

@Ericson2314
Copy link
Member

@mupdt Interesting point with the race. Looking at LocalStore::addIndirectRoot it will happily make a dangling symlink, but yes there is a race condition where it could be garbage collected before the new one shows up.

Is there any source of fsync that works reliably with NFS? I do sort of like the client writing the symlink to "prove" it is looking at the same filesystem, but this doesn't matter that much, and there are in principle issues from exposing a way to force the remote side to spam fsyncs just as there is forcing the remote side to write symlinks.

The bottom line is a mounted-ssh:// could still override this method (after making it virtual) and do whatever it likes, so I think we are OK :).

@mupdt
Copy link
Contributor Author

mupdt commented Mar 1, 2023

@Ericson2314: Re NFS: I believe fsync semantics do not extend beyond the host and won't be useful in this respect, unfortunately (unless we spin-wait; which I think we can avoid if we can make the "symlink on remote" approach). 😢

But yes, Ithe goal is definitely to do something here that is useful for everyone. I'll try to wrap my head around the mounted-ssh:// idea and see what I can do. 😅 I'll keep you posted.

@mupdt
Copy link
Contributor Author

mupdt commented Mar 6, 2023

@Ericson2314: After a bit more diving and attempts at implementing the mounted-ssh:// idea, I think I came up with something relatively generic and extensible. I discovered that we can generalize the "perm GC roots" idea without security concerns and still leave it up to the stores how they implement it. However, we need to hide it behind a new boolean setting.

In short, I think this implements the mounted-ssh:// idea through an option, rather than creating a new protocol. I hope this makes sense.

{
if (stdio) {
if (auto store = openUncachedStore().dynamic_pointer_cast<RemoteStore>()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I extracted this into a function (without changing it), that's why it appears as if a lot has changed here.

Copy link
Member

Choose a reason for hiding this comment

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

@mupdt Maybe lets go PR this separately like you did before?

@mupdt
Copy link
Contributor Author

mupdt commented Mar 13, 2023

@Ericson2314 @edolstra: Could I ask you for another look? 🙏

Btw, we ran a suite of integration tests with this patch and are also using it interactively now. It's looking good so far, but we'll keep using it in the long term too from now on.

@Ericson2314
Copy link
Member

@mupdt Sorry you had to give me multiple pings. Looking now.

@Ericson2314
Copy link
Member

@mupdt So the basic idea here is good, but do note that now casts to GcStore without the setting will lead to failing addPermRoot calls, whereas before it just wouldn't try.

@Ericson2314 Ericson2314 added protocol Things involving the daemon protocol & compatibility issues store Issues and pull requests concerning the Nix store labels Mar 14, 2023
@mupdt
Copy link
Contributor Author

mupdt commented Mar 14, 2023

@Ericson2314: Got it, yes. Thankfully the addPermRoot call will not fail, it's ignored as before by this code. In other words, the addPermRoot call will be dropped by the RemoteStore before it reaches the nix daemon. Does that sounds reasonable?

@Ericson2314
Copy link
Member

@mupdt Sorry agian for some delay.

So you are right, that has the right behavior. However, I am not super comfortable making an operation in isolation allowed to fail, merely on the assumption that we always use that operation in an optional/best effort way. Nothing about "add perm root'' says "optional outlink" after all, and arguably one explicitly passing --outlink and it being ignored is a bad thing anyways.

This leads me, surprise surprise, back to the mounted-ssh:// thing so we have two separate classes. If it helps, I might be able to look at the refactor to do that myself, but I can't promise I'd get to it before next week.

@mupdt
Copy link
Contributor Author

mupdt commented Mar 16, 2023

@Ericson2314: Ah, I think I misunderstood, sorry about that! I'll go ahead an try to refactor.

A quick question before I dive into code: What difference in implementation do you suggest for ssh-ng:// compared to mounted-ssh://? The reason I ask is that every RemoteStore (including ssh-ng) inherits GcStore (where the addPermRoot API now lives). If I understand correctly you're saying that I should move the addPermRoot API out of GcStore and into its own class (unfortunately, I think there's no other fitting class for this at the moment)? And then only mounted-ssh:// should implement this new class?

Btw, what do you think should I add a warning log message that notifies the user that --out-link was ignored?

P.s.: No worries at all about the delay, really! Thank you so much for the feedback. 🙏 I do want to make sure this feature is useful for everyone and as great as possible so everyone is comfortable with it. So it's totally okay if it takes a bunch of time.

@Ericson2314
Copy link
Member

Ericson2314 commented Mar 17, 2023

@mupdt Yeah I think both root methods (addIndirectRoot and addPermRoot) can just go in LocalFSStore. RemoteStore would not implement either. but UDSRemoteStore would implement both and MountedSshStore would also implement both.

BTW, you may notice this means addIndirectRoot is no longer usable from plain ssh://, but I think this is a feature not a bug. Today one can add indirect roots willy-nilly and possibly get a remote-side Nix daemon to traverse arbitrary directories. That can't be good.

Btw, what do you think should I add a warning log message that notifies the user that --out-link was ignored?

If you like, but I would personally couple that with the more aggressive deduplicating of that code you held off on before, so I am fine if it is again held off and not part of this PR either.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-managed-datalake/26541/1

@mupdt
Copy link
Contributor Author

mupdt commented Mar 23, 2023

@Ericson2314: I just pushed the implementation of the mounted-ssh:// store. Indeed it looks much cleaner. It works like a charm. I hope it looks reasonable. 🤞 Could you give it another look?

src/libstore/ssh-store.cc Outdated Show resolved Hide resolved
src/libstore/globals.hh Outdated Show resolved Hide resolved
@mupdt mupdt marked this pull request as ready for review August 30, 2023 10:13
@Ericson2314
Copy link
Member

@mupdt Thanks!

A new test is totally fine --- I had never meant to suggest you literally reuse the same same test, so sorry if you (as it seems perhaps) spent a long while trying to get that to work. It totally makes sense that

  1. Using the mounted ssh store as remote builder
  2. both stores having store dir == real store dir so symlinks are mounted
  3. not stepping on each others' toes with things like file locks

is an impossible combination.

src/nix/daemon.cc Outdated Show resolved Hide resolved
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Other then the CLI flag stuff, looks good to me!

@Ericson2314 Ericson2314 marked this pull request as draft August 30, 2023 22:11
@Ericson2314
Copy link
Member

@mupdt Are you able to make the last few small changes for this?

@mupdt
Copy link
Contributor Author

mupdt commented Nov 15, 2023

@Ericson2314: Oh my, I completely dropped the ball on this one. Apologies! Of course, I'll do it asap.

@Ericson2314
Copy link
Member

All good! Looking forward to doing the final review ;)

@mupdt mupdt marked this pull request as ready for review November 18, 2023 19:34
src/nix/daemon.cc Outdated Show resolved Hide resolved
@mupdt
Copy link
Contributor Author

mupdt commented Nov 18, 2023

@Ericson2314: Suggestions added and rebased on latest master. Please take another look and sorry for the delay.

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Excited to have this!

@Ericson2314 Ericson2314 merged commit a6b315a into NixOS:master Nov 21, 2023
8 checks passed
@mupdt mupdt deleted the gcstore-add-perm-root branch November 22, 2023 09:05
@gbpdt
Copy link
Contributor

gbpdt commented Nov 22, 2023

Excited to have this!

Thanks everyone for the hard work getting this over the line!

@SuperSandro2000
Copy link
Member

Is there a blog post or config on how to experiment with this somewhere?

@mupdt
Copy link
Contributor Author

mupdt commented Nov 23, 2023

Unfortunately, no blog post, but the tests demonstrate a bit how to use it and what to expect.

For example, say you have two machines, let's call them client and server. The server is the one where the nix daemon runs, where /nix and the home folder are exported via NFS. The client doesn't run a nix daemon, but /nix and the home folder are both mounted from the server via NFS. Now on the client you can use nix build from your home folder with the --store mounted-ssh-ng://server argument. This will run the build on the server as well as create an out-link on the server (inside the home folder). This makes sure that out-links are actually generated (server-side) and gc roots correctly registered with the server's nix daemon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request or proposal idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. new-cli Relating to the "nix" command protocol Things involving the daemon protocol & compatibility issues store Issues and pull requests concerning the Nix store with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants