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

nixos/nfsd: run rpc-statd as a normal user #96844

Merged
merged 1 commit into from Sep 9, 2020
Merged

Conversation

@peterhoeg
Copy link
Member

@peterhoeg peterhoeg commented Aug 31, 2020

Motivation for this change

Instead of running it as root, use a dedicated user.

Been running with it here for a while without any issues.

Closes #63756

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@aanderse
Copy link
Contributor

@aanderse aanderse commented Aug 31, 2020

Out of curiosity what user does this run on other distros like Arch, Debian, Gentoo, RedHat, etc...?

@peterhoeg
Copy link
Member Author

@peterhoeg peterhoeg commented Aug 31, 2020

@peterhoeg peterhoeg force-pushed the peterhoeg:m/nfs branch from 23cd945 to 6a0a4ab Aug 31, 2020
@aanderse
Copy link
Contributor

@aanderse aanderse commented Sep 1, 2020

This is somewhat confusing if you aren't familiar with the internals of nfs. Since you aren't running the service as the statd user the module simply appears to be creating a user for no apparent reason (if you don't know that rpc.statd will drop privileges based on the ownership of a directory, which I did not know). A comment would go a long way.

If we look at the mkdir usage in the preStart this starts to look like a mild security concern. If /var/lib/nfs is blown away for some reason and tmpfiles doesn't have a chance to run then the mkdir command will create directories owned by root and we're back to the (bad) situation the module is currently in. I would recommend removing all (or at least as much) usage of mkdir as possible here in favour of another solution (like tmpfiles.rules). It would become even easier if we made an attribute set to replace the exports option... but now I'm starting to make massive scope creep on this PR 😆

If we merge this PR now as is would you have capacity to either work on or review a PR which replaced the exports option with an attribute set?

@peterhoeg peterhoeg force-pushed the peterhoeg:m/nfs branch 2 times, most recently from aea3ef7 to b3596db Sep 6, 2020
@peterhoeg
Copy link
Member Author

@peterhoeg peterhoeg commented Sep 6, 2020

Very good points - I totally missed those mkdirs @aanderse. It's been cleaned up now and some docs added too.

# /var/lib/nfs
systemd.tmpfiles.rules = [
"d /var/lib/nfs 0700 ${rpcUser} ${rpcUser} - -"
"d /var/lib/nfs/recovery 0755 root root - -"

This comment has been minimized.

@aanderse

aanderse Sep 6, 2020
Contributor

Does this throw a silent runtime error with systemd-tmpfiles stating unsafe transition? I'm thinking it will... 🤔

This comment has been minimized.

@peterhoeg

peterhoeg Sep 7, 2020
Author Member

Why would it do that?

This comment has been minimized.

@peterhoeg peterhoeg force-pushed the peterhoeg:m/nfs branch from b3596db to d626441 Sep 7, 2020
@peterhoeg
Copy link
Member Author

@peterhoeg peterhoeg commented Sep 7, 2020

Found another mkdir invocation which is now gone.

@peterhoeg peterhoeg closed this Sep 7, 2020
@peterhoeg peterhoeg deleted the peterhoeg:m/nfs branch Sep 7, 2020
@peterhoeg peterhoeg restored the peterhoeg:m/nfs branch Sep 7, 2020
@peterhoeg
Copy link
Member Author

@peterhoeg peterhoeg commented Sep 7, 2020

It works here though. I just tried shutting down the nfs service, remove the directory and reboot the nas. This is what is created:

# ls -la nfs
total 8
drwx------  1 statd statd 132 Sep  7 20:22 .
drwxr-xr-x  1 root  root  256 Sep  7 20:22 ..
-rw-r--r--  1 root  root  207 Sep  7 20:22 etab
-rw-------  1 root  root    0 Sep  7 20:22 .etab.lock
-rw-r--r--  1 root  root    0 Sep  7 20:22 export-lock
drwxr-xr-x  1 root  root    0 Sep  7 20:22 recovery
dr-xr-xr-x 11 root  root    0 Sep  7 20:22 rpc_pipefs
drwxr-xr-x  1 root  root    0 Sep  7 20:22 sm
drwxr-xr-x  1 root  root    0 Sep  7 20:22 sm.bak
-rw-r--r--  1 root  root    4 Sep  7 20:22 state
drwxr-xr-x  1 root  root    0 Sep  7 20:22 v4recovery
@peterhoeg peterhoeg reopened this Sep 7, 2020
@peterhoeg
Copy link
Member Author

@peterhoeg peterhoeg commented Sep 7, 2020

This is admittedly with this PR applied to 20.03 which uses systemd 243. I don't know if anything changed since then.

Copy link
Contributor

@aanderse aanderse left a comment

Great, glad to hear this is fine. I can't recall the details of what systemd classifies as an unsafe transition but this is working so... LGTM 👍

@peterhoeg
Copy link
Member Author

@peterhoeg peterhoeg commented Sep 9, 2020

@peterhoeg peterhoeg merged commit 42eebd7 into NixOS:master Sep 9, 2020
17 of 18 checks passed
17 of 18 checks passed
tests tests
Details
tests tests
Details
action
Details
action
Details
Wait for ofborg This failed status will be cleared when ofborg finishes eval.
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d626441"; rev="d6264419f5c2ea3601f65f607f5ea8b187548bc7"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d626441"; rev="d6264419f5c2ea3601f65f607f5ea8b187548bc7"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d626441"; rev="d6264419f5c2ea3601f65f607f5ea8b187548bc7"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d626441"; rev="d6264419f5c2ea3601f65f607f5ea8b187548bc7"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d626441"; rev="d6264419f5c2ea3601f65f607f5ea8b187548bc7"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d626441"; rev="d6264419f5c2ea3601f65f607f5ea8b187548bc7"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="d626441"; rev="d6264419f5c2ea3601f65f607f5ea8b187548bc7"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@peterhoeg peterhoeg deleted the peterhoeg:m/nfs branch Sep 9, 2020
@vcunat
Copy link
Member

@vcunat vcunat commented Sep 10, 2020

This merge breaks a test #97582 (I checked a local revert). Unless we have a fix very soon (say, today), I suggest to revert it until the issue is resolved.

@peterhoeg peterhoeg mentioned this pull request Sep 10, 2020
4 of 10 tasks complete
vcunat added a commit that referenced this pull request Sep 10, 2020
This reverts commit 42eebd7, reversing
changes made to b169bfc.

This breaks nfs3.simple test and even current PR #97656 wouldn't fix it.
Therefore let's revert for now to unblock the channels.
@peterhoeg
Copy link
Member Author

@peterhoeg peterhoeg commented Sep 11, 2020

Thanks for handling this - things were working perfectly fine here. I'll get this fixed later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.