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.

Sign up for GitHub

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

Jump to bottom

Trustless remote building for input-addressed drvs #3921

Merged
merged 35 commits into from May 8, 2023
Merged

Trustless remote building for input-addressed drvs #3921

merged 35 commits into from May 8, 2023

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Aug 12, 2020 •
edited

Motivation

Fixes #2789.

Input-addressed outputs cannot be safely pushed to a remote builder without trusting the client. This makes trustless remote building without experimental features completely useless in practice. Making them work is widely requested.

The solution is simple: when not trusted, instead of using buildDerivation (which sends a mere BasicDerivation and requires trust, use buildPaths and send the entire Derivation and its closure).

Context

Known failure mode

We still cannot copy input-addressed paths to a remote builder safely. In that case, this still fails (see the should-fail test). The manual work around is to tell the remote builder to build that path first, so it already has it. This can be done with nix-build --store <them> --eval-store <us>.

That is manual and tedious, but in may real world cases can be avoided in practice. (e.g. when remote builders are always used, and the remote builders trust one another, this won't arise.)

Seems good enough to start with.

Depends on #8230 for testing with untrusted remote store.

Depends on #7515 for knowing whether the remote store trusts us

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
  • documentation in the internal API docs
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

@Ericson2314 Ericson2314 force-pushed the trustless-remote-builder-simple branch from b5f5779 to 6fa196c Compare August 13, 2020 21:00
@Ericson2314 Ericson2314 force-pushed the trustless-remote-builder-simple branch from aa6e71b to 46bb107 Compare August 13, 2020 21:45
@Ericson2314 @matthewbauer
Trustless remote building
cbc4344 
Co-authored-by: Matthew Bauer <mjbauer95@gmail.com>
@Ericson2314 Ericson2314 force-pushed the trustless-remote-builder-simple branch from 46bb107 to cbc4344 Compare August 14, 2020 04:54
@Ericson2314 Ericson2314 changed the title WIP: Trustless remote builder, simple version -- contains #3918 Trustless remote builder, simple version -- contains #3918 Aug 14, 2020
@Ericson2314 Ericson2314 changed the title Trustless remote builder, simple version -- contains #3918 WIP: Trustless remote builder, simple version -- contains #3918 Aug 14, 2020
@Ericson2314 Ericson2314 changed the title WIP: Trustless remote builder, simple version -- contains #3918 WIP: Trustless remote builder, simple version -- contains #3918 & #3930 Aug 14, 2020
@Ericson2314 Ericson2314 mentioned this pull request Aug 16, 2020
Open
@Ericson2314 Ericson2314 force-pushed the trustless-remote-builder-simple branch from c91c7d1 to 9dd28a6 Compare August 16, 2020 16:08
@Ericson2314 Ericson2314 changed the title WIP: Trustless remote builder, simple version -- contains #3918 & #3930 WIP: Trustless remote builder, simple version -- contains #3930 & #3940 Aug 17, 2020
meditans and others added 2 commits August 17, 2020 13:15
@meditans
Disable failing tests
1bf31ba
@Ericson2314
But back check.sh
36758a1 
Whether it fails or not, it is no a new test so we have to leave it.
@Ericson2314 Ericson2314 changed the title WIP: Trustless remote builder, simple version -- contains #3930 & #3940 Trustless remote builder, simple version -- contains #3930 & #3940 Aug 17, 2020
@Ericson2314 Ericson2314 changed the title Trustless remote builder, simple version -- contains #3930 & #3940 Trustless remote builder, simple version Aug 20, 2020
edolstra
edolstra reviewed Sep 4, 2020
View reviewed changes
src/libstore/store-api.cc Outdated Show resolved Hide resolved
src/libstore/store-api.hh Outdated Show resolved Hide resolved
src/nix-daemon/nix-daemon.cc Outdated
@@ -278,14 +279,26 @@ static int _main(int argc, char * * argv)
printVersion("nix-daemon");
else if (*arg == "--stdio")
stdio = true;
else return false;
else if (*arg == "--trust") {
Copy link
Member

Choose a reason for hiding this comment

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

What does this flag do? What are we trusting?

Copy link
Member Author

Choose a reason for hiding this comment

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

stdin. When we have a Unix domain socket we can do getPeerInfo, but we can't do that with stdin so currently we just always trust stdin (that is, if we aren't just proxying stdin another daemon, as is the common case with ssh-ng).

This provides a way to not trust stdin, which is useful for testing. I could make these flag --trust-stdin / --no-trust-stdin for clarity.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have generalized the flag so it also works in the Unix domain socket case; it forces the trust one way or the other instead of checking the user viaauthPeer.

@github-actions github-actions bot added new-cli Relating to the "nix" command and removed store Issues and pull requests concerning the Nix store labels Apr 12, 2023
Ericson2314
Ericson2314 commented Apr 17, 2023
View reviewed changes
src/build-remote/build-remote.cc Outdated
// If we don't know whether we are trusted (e.g. `ssh://`
// stores), we assume we are. This is neccessary for backwards
// compat.
if (std::optional trust = sshStore->isTrustedClient(); (!trust || *trust) || drv.type().isCA()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

C.f. The huuuuuge comment by me in wopBuildDerivation in src/libstore/daemon.cc. All of this follows from that pretty simply.

@Ericson2314
Merge remote-tracking branch 'upstream/master' into trustless-remote-…
b1343e8 
…builder-simple
@Ericson2314
nix-testing -> daemon-trust-override
e95db8f 
And only enable in the tests that need it. This makes it less of a
sledgehammer.
Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Apr 17, 2023
@Ericson2314
Experimentally allow forcing nix-daemon trust; use this to test
91ad0b7 
We finally test the status quo of remote build trust in a number of
ways. We create a new experimental feature on `nix-daemon` to do so.

PR NixOS#3921, which improves the situation with trustless remote building,
will build upon these changes. This code / tests was pull out of there
to make this, so everything is easier to review, and in particular we
test before and after so the new behavior in that PR is readily apparent
from the testsuite diff alone.
@Ericson2314 Ericson2314 mentioned this pull request Apr 17, 2023
Merged
8 tasks
@Ericson2314 Ericson2314 marked this pull request as draft April 17, 2023 14:01
@Ericson2314
Copy link
Member Author

Making this draft not because it is not ready, but because I pulled out #8230 as preparatory PR we should merge first for better reviewing.

Ericson2314 added a commit to obsidiansystems/nix that referenced this pull request Apr 17, 2023
@Ericson2314
Experimentally allow forcing nix-daemon trust; use this to test
d41e1be 
We finally test the status quo of remote build trust in a number of
ways. We create a new experimental feature on `nix-daemon` to do so.

PR NixOS#3921, which improves the situation with trustless remote building,
will build upon these changes. This code / tests was pull out of there
to make this, so everything is easier to review, and in particular we
test before and after so the new behavior in that PR is readily apparent
from the testsuite diff alone.
@Ericson2314 Ericson2314 force-pushed the trustless-remote-builder-simple branch from 695befa to ab5ca60 Compare April 17, 2023 17:54
@Ericson2314 Ericson2314 marked this pull request as ready for review April 17, 2023 17:55
@github-actions github-actions bot removed the new-cli Relating to the "nix" command label Apr 17, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-04-17-nix-team-meeting-minutes-49/27379/1

@Ericson2314
Copy link
Member Author

OK, this is now quite small and easy to review!

fricklerhandwerk
fricklerhandwerk approved these changes May 8, 2023
View reviewed changes
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Reviewed in Nix team meeting.

src/build-remote/build-remote.cc Outdated Show resolved Hide resolved
src/build-remote/build-remote.cc Outdated Show resolved Hide resolved
src/build-remote/build-remote.cc Outdated Show resolved Hide resolved
src/build-remote/build-remote.cc Outdated Show resolved Hide resolved
Ericson2314 and others added 2 commits May 8, 2023 09:57
@Ericson2314 @fricklerhandwerk
Apply suggestions from code review
cd0d8e0 
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
@Ericson2314
Split comment, match with each variable
df53a7d
@Ericson2314 Ericson2314 enabled auto-merge May 8, 2023 14:08
@Ericson2314 Ericson2314 merged commit b5d9ef0 into NixOS:master May 8, 2023
8 checks passed
@arcnmx
Copy link
Member

arcnmx commented May 8, 2023

🎉 🎉 🎉

@Ericson2314 Ericson2314 deleted the trustless-remote-builder-simple branch May 8, 2023 16:54
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-05-08-nix-team-meeting-minutes-53/27967/1

@zowoq zowoq mentioned this pull request Aug 16, 2023
Merged
@GaetanLepage GaetanLepage mentioned this pull request Nov 14, 2023
Merged
@superherointj superherointj mentioned this pull request Jan 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbauer matthewbauer matthewbauer left review comments

@edolstra edolstra edolstra left review comments

@fricklerhandwerk fricklerhandwerk fricklerhandwerk approved these changes

@thufschmitt thufschmitt Awaiting requested review from thufschmitt

Assignees
No one assigned
Labels
feature Feature request or proposal with-tests Issues related to testing. PRs with tests have some priority
Projects
Nix team
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

distributed builds require a trusted remote user