Add Store::isTrustedClient() by MatthewCroughan · Pull Request #7515 · NixOS/nix

MatthewCroughan · 2022-12-26T20:46:53Z

This PR extends the worker protocol in worker-protocol-hh by adding a new operation (47) which permits a client to check if they are trusted. I have needed this for some scripts I'm trying to write which use Nix, that need to check.

In addition, it also adds a new command nix store check-trusted which will make use of this addition. It will return exit code 0 if trusted, 1 if not trusted or 2 if an unexpected error occurs.

I have also added a new checkInfo function to doctor.cc which will print an inconsequential blue [INFO] telling the user whether they are trusted. This does not change the exit status of the nix doctor command.

The following Bash function is an example of a fallback mechanism I've implemented in Bash but do not have the skills to implement in C++.

function isTrustedUser {
  function writeDummyBuildLog {
    {
      echo 0x6378696e 0x00000000; # WORKER_MAGIC_1
      echo 0x0a010000 0x00000000; # PROTOCOL_VERSION
      echo 0x2d000000 0x00000000; # wopAddBuildlog (45)
      echo 0x26000000 0x00000000; # Length of Store Path (38)
      echo -n "$(echo -n aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-a.drv | xxd -p -c0)00000000000000000000"; # Store Path
    } \
    | xxd -p -r \
    | socat - UNIX-CONNECT:/nix/var/nix/daemon-socket/socket \
    | tr -d '\0'
  }
  x=$(writeDummyBuildLog)
  if (grep -aq "error:" <<< "$x") && (grep -aq "not privileged" <<< "$x")
  then
    echo "You are not a trusted user"
    return 1
  elif (grep -aq "error:" <<< "$x")
    echo "An unknown error occurred whilst checking if you are a trusted user"
    return 2
  else
    :
    return 0
  fi
}
isTrustedUser

Thanks a lot to @roberth who introduced me to the C++ codebase and answering a lot of questions. And thanks to @tomberek + @pkharvey + @helicalbytes for helping me understand the daemon protocol.

NobbZ · 2022-12-27T18:44:10Z

This is a good first step into a solution for #6752/#6672.

Can't say much codewise though.

MatthewCroughan · 2022-12-27T18:48:22Z

This PR will also improve #7461

src/libstore/dummy-store.cc

src/libstore/remote-store.cc

src/nix/doctor.cc

MatthewCroughan · 2022-12-27T19:50:31Z

I somehow accidentally removed the review request for @edolstra @fricklerhandwerk and @thufschmitt just by requesting a re-review from @max-privatevoid, this seems to be a bug in the way reviews are handled on GitHub. I am unable to add new reviewers, or add these reviewers back.

MatthewCroughan · 2022-12-27T20:10:27Z

I marked the PR as a draft, then unmarked it as a draft. It seems that the GitHub review bug is still in effect.

MatthewCroughan · 2022-12-29T22:14:30Z

Just pushed to resolve conflicts with the release notes.

src/libstore/daemon.cc

src/libstore/build/local-derivation-goal.cc

thufschmitt

Thanks, that's a useful command to have I think.

Left some suggestions on the code and docs, but that looks good overall.

(That also makes me realize with horror that we don't have any test for the "non-trusted-user" case 😬 )

thufschmitt · 2023-01-02T09:19:24Z

src/libstore/store-api.hh

    };

+    /* Check if the user making the connection is a trusted-user. */
+    virtual bool isTrustedUser() = 0;


I think it's fine to define this to unsupported by default like we do for a bunch of other methods and remove all the custom implementations for the stores that don't support it:

Suggested change

virtual bool isTrustedUser() = 0;

virtual bool isTrustedUser()

{ unsupported("isTrustedUser"); }

That definitely makes it a smaller diff too.

Done. I've left the implementation for legacy ssh though, since it's more helpful to print it.

@thufschmitt This pattern is counterproductive for discovering such functionality as recursive nix store is not trusted early. Perhaps not a great example, but who knows what else we might have missed because of the bad default?

@roberth since the default behaviour is to fail loudly, I think it's OK because it's a conservative approach, and we can always refine the implementation after the fact if we realize that we've been missing one

src/libstore/remote-store.cc

src/nix/doctor.cc

src/nix/store-check-trusted.cc

MatthewCroughan · 2023-01-19T00:48:17Z

I've rebased this PR on 2.14 after the Nix 2.13 release yesterday.

tests/remote-store.sh

tomberek · 2023-01-23T12:49:44Z

From Nix meeting:

instead of a new subcommand, implement nix store ping to return these results explicitly when available
In a new PR add --json to nix store ping to allow for easier scripting.
include the trusted status into the daemon handshake, this is mediated in the area of the below.

if (GET_PROTOCOL_MINOR(conn.daemonVersion) >= 33) {
            conn.to.flush();
            conn.daemonNixVersion = readString(conn.from);
        }

Ericson2314 · 2023-02-02T15:37:48Z

@MatthewCroughan Do note with #7723 merged the nix store ping changes will be less work!

nixos-discourse · 2023-02-13T14:51:07Z

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

https://discourse.nixos.org/t/2023-01-20-nix-team-meeting-minutes-25/25432/1

MatthewCroughan · 2023-02-13T14:55:01Z

Have dropped the ball on this temporarily, I'm a bit ill after FOSDEM, will pick this up when I am better, anyone else feel free to contribute by making a PR to this PR!

nixos-discourse · 2023-02-13T14:58:59Z

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

https://discourse.nixos.org/t/2023-01-23-nix-team-meeting-minutes-26/25433/1

fricklerhandwerk · 2023-04-05T10:40:43Z

Discussed in the Nix team meeting 2023-04-03:

@Ericson2314 rewrote the PR based on the team's requested changes, needs a different reviewer
assigned to @thufschmitt

nixos-discourse · 2023-04-05T11:06:58Z

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

https://discourse.nixos.org/t/2023-04-03-nix-team-meeting-minutes-46/27008/1

domenkozar · 2023-04-06T10:39:16Z

Would be great to have this in for 2.15.

thufschmitt

The change is good overall, but the wire encoding/decoding logic is a bit too smart imho. Let's keep things simple as much as we can.

doc/manual/src/release-notes/rl-next.md

src/libstore/remote-store.cc

src/libstore/daemon.cc

This function returns true or false depending on whether the Nix client is trusted or not. Mostly relevant when speaking to a remote store with a daemon. We include this information in `nix ping store` and `nix doctor` Co-Authored-By: John Ericson <John.Ericson@Obsidian.Systems>

Ericson2314 · 2023-04-07T00:03:13Z

Good point, I did sort of forget about that mechanism! 😳

thufschmitt

Yay!

Just a small suggestion, but fairly minor and not blocking. So let's merge

thufschmitt · 2023-04-07T07:37:34Z

src/libstore/remote-store.cc

+        case 2:
+            return { NotTrusted };
+        default:
+            throw Error("Invalid trusted status from remote");


Saving it for a potential follow-up (and fairly low-prio), but it could be nicer to just warn and return std::nullopt in that case. Just to make potential future evolutions of the protocol nicer.

I had thought about that, but I think it's just better to bunl the protocol version on that case? Maybe I'm forgetting, but I couldn't think of another time we tried to do this sort of week forward compat before.

We currently have more or less one forward compat mechanism, which is the protocol version handshake.
If we're going to improve the forward compat mechanism, it would make more sense to do it structurally, with a self-describing or schema based serialization where we can add fields as we please.

MatthewCroughan requested review from edolstra and thufschmitt as code owners December 26, 2022 20:46

MatthewCroughan force-pushed the mc/check-trusted-user branch 4 times, most recently from b03b43b to 8421857 Compare December 27, 2022 18:04

MatthewCroughan requested a review from fricklerhandwerk as a code owner December 27, 2022 18:04

max-privatevoid suggested changes Dec 27, 2022

View reviewed changes

src/libstore/dummy-store.cc Outdated Show resolved Hide resolved

src/libstore/remote-store.cc Outdated Show resolved Hide resolved

src/nix/doctor.cc Outdated Show resolved Hide resolved

MatthewCroughan force-pushed the mc/check-trusted-user branch from 8421857 to d1481b0 Compare December 27, 2022 19:44

MatthewCroughan requested review from max-privatevoid and removed request for edolstra, fricklerhandwerk and thufschmitt December 27, 2022 19:47

MatthewCroughan force-pushed the mc/check-trusted-user branch from d1481b0 to 2a0406e Compare December 27, 2022 20:06

MatthewCroughan marked this pull request as draft December 27, 2022 20:09

MatthewCroughan marked this pull request as ready for review December 27, 2022 20:09

MatthewCroughan force-pushed the mc/check-trusted-user branch from c93ae9c to 2a0406e Compare December 27, 2022 20:18

roberth mentioned this pull request Dec 28, 2022

Improve protocol major macro #7523

Open

max-privatevoid approved these changes Dec 28, 2022

View reviewed changes

tomberek requested a review from edolstra December 28, 2022 18:32

MatthewCroughan force-pushed the mc/check-trusted-user branch from 2a0406e to 01c67e4 Compare December 29, 2022 22:12

roberth reviewed Dec 29, 2022

View reviewed changes

src/libstore/daemon.cc Outdated Show resolved Hide resolved

src/libstore/build/local-derivation-goal.cc Outdated Show resolved Hide resolved

MatthewCroughan changed the title ~~Add Store::checkTrustedUser() and nix store check-trusted~~ Add Store::isTrustedUser() and nix store check-trusted Dec 30, 2022

MatthewCroughan force-pushed the mc/check-trusted-user branch from e1a65b2 to 0d28385 Compare December 30, 2022 04:26

thufschmitt suggested changes Jan 2, 2023

View reviewed changes

Ericson2314 reviewed Jan 20, 2023

View reviewed changes

tests/remote-store.sh Outdated Show resolved Hide resolved

thufschmitt assigned Ericson2314 Jan 23, 2023

yorickvP mentioned this pull request Jan 31, 2023

nix store ping: add --json flag #7723

Merged

7 tasks

fricklerhandwerk added feature Feature request or proposal UX The way in which users interact with Nix. Higher level than UI. store Issues and pull requests concerning the Nix store labels Mar 3, 2023

Ericson2314 mentioned this pull request Mar 13, 2023

Triage and break up the global settings #5638

Open

Ericson2314 force-pushed the mc/check-trusted-user branch from a13a7c3 to 7968371 Compare April 3, 2023 02:34

github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Apr 3, 2023

MatthewCroughan changed the title ~~Add Store::isTrustedUser() and nix store check-trusted~~ Add Store::isTrustedClient() Apr 3, 2023

Ericson2314 added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Apr 3, 2023

thufschmitt assigned thufschmitt and unassigned Ericson2314 Apr 3, 2023

thufschmitt suggested changes Apr 6, 2023

View reviewed changes

doc/manual/src/release-notes/rl-next.md Outdated Show resolved Hide resolved

src/libstore/remote-store.cc Outdated Show resolved Hide resolved

src/libstore/daemon.cc Outdated Show resolved Hide resolved

Ericson2314 force-pushed the mc/check-trusted-user branch from 7968371 to 9207f94 Compare April 7, 2023 00:02

thufschmitt approved these changes Apr 7, 2023

View reviewed changes

thufschmitt enabled auto-merge April 7, 2023 07:39

thufschmitt merged commit 81dfc2b into NixOS:master Apr 7, 2023

MatthewCroughan deleted the mc/check-trusted-user branch April 7, 2023 14:27

	virtual bool isTrustedUser() = 0;
	virtual bool isTrustedUser()
	{ unsupported("isTrustedUser"); }

Uh oh!

Conversation

MatthewCroughan commented Dec 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NobbZ commented Dec 27, 2022

Uh oh!

MatthewCroughan commented Dec 27, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MatthewCroughan commented Dec 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MatthewCroughan commented Dec 27, 2022

Uh oh!

MatthewCroughan commented Dec 29, 2022

Uh oh!

Uh oh!

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MatthewCroughan commented Jan 19, 2023

Uh oh!

Uh oh!

tomberek commented Jan 23, 2023

Uh oh!

Ericson2314 commented Feb 2, 2023

Uh oh!

nixos-discourse commented Feb 13, 2023

Uh oh!

MatthewCroughan commented Feb 13, 2023

Uh oh!

nixos-discourse commented Feb 13, 2023

Uh oh!

fricklerhandwerk commented Apr 5, 2023

Uh oh!

nixos-discourse commented Apr 5, 2023

Uh oh!

domenkozar commented Apr 6, 2023

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ericson2314 commented Apr 7, 2023

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

MatthewCroughan commented Dec 26, 2022 •

edited

Loading

MatthewCroughan commented Dec 27, 2022 •

edited

Loading