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/nfs: Allow Kerberized NFS #73989

Merged
merged 7 commits into from Dec 13, 2019
Merged

nixos/nfs: Allow Kerberized NFS #73989

merged 7 commits into from Dec 13, 2019

Conversation

@kwohlfahrt
Copy link
Contributor

@kwohlfahrt kwohlfahrt commented Nov 23, 2019

Motivation for this change

Currently, NFS mounts secured with Kerberos do not work (see #72722).

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 nix-review --run "nix-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.
Notify maintainers

cc @tfc - regarding the Perl -> Python test port
cc @edolstra as maintainer of the previous nfs test

Copy link
Contributor

@rnhmjoj rnhmjoj left a comment

You have to update nixos/tests/all-tests.nix to build the nfs/ directory, instead of nfs.nix.

nixos/tests/nfs/kerberos.nix Outdated Show resolved Hide resolved
nixos/tests/nfs/kerberos.nix Outdated Show resolved Hide resolved
client.fail("echo bla >> /data/foo")
client.succeed("echo root_pw | kinit root", "echo bla >> /data/foo")
server.succeed("test -e /data/foo")

This comment has been minimized.

@rnhmjoj

rnhmjoj Nov 24, 2019
Contributor

Maybe use wait_for_file here

This comment has been minimized.

@kwohlfahrt

kwohlfahrt Nov 24, 2019
Author Contributor

test -e is what the plain NFS test uses, I assume because NFS should be synchronous here (i.e. by the time the file is visible on the client, it has been created on the server).

@kwohlfahrt kwohlfahrt force-pushed the kwohlfahrt:nfs-tests-python branch 2 times, most recently from db4977f to 52f6a9e Nov 24, 2019
nixos/tests/all-tests.nix Outdated Show resolved Hide resolved
@kwohlfahrt kwohlfahrt force-pushed the kwohlfahrt:nfs-tests-python branch 3 times, most recently from dd33065 to 913a10b Nov 24, 2019
@kwohlfahrt
Copy link
Contributor Author

@kwohlfahrt kwohlfahrt commented Nov 25, 2019

As noted in the issue, there are two remaining problems:

  1. file uid/gids are all messed up. This can be fixed by running nfs-idmapd on the client, but docs suggest that this is no longer necessary as it is replaced by the nfsidmap binary. Indeed, the service is masked if services.nfs.server.enable = false. EDIT: This is #68106
  2. It is not possible to write to files as root, even if the corresponding kerberos principal exists. This may be intended behavior.
@kwohlfahrt kwohlfahrt changed the title [WIP] Allow Kerberized NFS nixos/nfs: Allow Kerberized NFS Nov 25, 2019
@kwohlfahrt kwohlfahrt marked this pull request as ready for review Nov 25, 2019
nixos/tests/nfs.nix Outdated Show resolved Hide resolved
@kwohlfahrt
Copy link
Contributor Author

@kwohlfahrt kwohlfahrt commented Nov 25, 2019

I appear to have CI errors, but the message in details is not very clear to me: attribute 'x86_64-linux' missing, at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/eval-0-gleber.ewr1.nix.ci/nixos/release-combined.nix:50:43. How do I fix this?

@kwohlfahrt kwohlfahrt force-pushed the kwohlfahrt:nfs-tests-python branch from 913a10b to 62856e8 Nov 26, 2019
@kwohlfahrt
Copy link
Contributor Author

@kwohlfahrt kwohlfahrt commented Nov 26, 2019

This PR has expanded in scope a bit - there is now a kernel patch to fix the hard-coded ID request-key path. This touches a few other issues, namely #68106 and #34638, though those still need a bit of module work to ensure keyutils is present for example.

server.succeed("mkdir -p /data/alice")
server.succeed("chown alice:users /data/alice")
# set up kerberos database

This comment has been minimized.

@tfc

tfc Nov 27, 2019
Contributor

the comments here that tell what happens on a higher level could all be subtests, so this information even appears in the log.

This comment has been minimized.

@tfc

tfc Nov 27, 2019
Contributor

Oh sorry i already stated this in another comment in the same PR and didn't remember.

This comment has been minimized.

@kwohlfahrt

kwohlfahrt Nov 27, 2019
Author Contributor

I've changed most of the other ones. Here, this is really set-up code that is not related to the functionality under test. Is subtest intended for this use as well?

@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Nov 27, 2019

This PR has expanded in scope a bit - there is now a kernel patch to fix the hard-coded ID request-key path. This touches a few other issues, namely #68106 and #34638, though those still need a bit of module work to ensure keyutils is present for example.

I had no idea this could get this complicated. Thanks for all the work you've done, keep it up!

@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Nov 27, 2019

I appear to have CI errors, but the message in details is not very clear to me: attribute 'x86_64-linux' missing, at /var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/eval-0-gleber.ewr1.nix.ci/nixos/release-combined.nix:50:43. How do I fix this?

I can't try right now because I don't have nearly ram to run an evaluation but you could try to rebase your branch with current master. Maybe at the point you did a checkout master itself was broken.

ofBorg runs something like this

$ nix-instantiate nixos/release-combined.nix -A tested
@kwohlfahrt
Copy link
Contributor Author

@kwohlfahrt kwohlfahrt commented Nov 27, 2019

I figured out the build failure - I copied the test structure from the kerberos tests, which I wrote last year. It turns out they were never enabled in release-combined.nix, because I didn't know this was a necessary step, and the way I'd been calling them doesn't work.

@kwohlfahrt kwohlfahrt force-pushed the kwohlfahrt:nfs-tests-python branch from e7b8e45 to 7a6ec9e Nov 27, 2019
@kwohlfahrt
Copy link
Contributor Author

@kwohlfahrt kwohlfahrt commented Nov 27, 2019

All of the tests here are OK now. Remaining question are:

  • How should /etc/request-key.conf be generated? The issues linked above require different lines in this file.
  • should keyutils be part of the default systemPackages?
@rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Nov 27, 2019

should keyutils be part of the default systemPackages?

If it's only needed for kerberos I would say to install it behind a guard like krb5.enable.

@kwohlfahrt
Copy link
Contributor Author

@kwohlfahrt kwohlfahrt commented Nov 27, 2019

@rnhmjoj - no, its needed for id-mapping in general. This is not turned on by default for sec=sys (i.e. not kerberos), but such a configuration is possible, see here. I assume this is the reason for #68106 as it doesn't mention kerberos anywhere.

It is also needed for CIFS, at least with kerberos, see #34638. I'm not familiar enough with it to know if this can also be necessary without kerberos - the man page doesn't mention it so it seems likely.

We can test for the presence of a NFS/CIFS filesystem before installing it, which seems sensible.

@kwohlfahrt kwohlfahrt force-pushed the kwohlfahrt:nfs-tests-python branch from 8f8e04d to 6ea57ed Nov 27, 2019
@kwohlfahrt kwohlfahrt requested a review from alyssais as a code owner Nov 27, 2019
"systemctl restart kadmind.service kdc.service",
)
for unit in ["kadmind", "kdc"]:
server.wait_for_unit(f"{unit}.service")

This comment has been minimized.

@Mic92

Mic92 Nov 28, 2019
Contributor

nitpick: same lines of code / easier to read:

Suggested change
server.wait_for_unit(f"{unit}.service")
server.wait_for_unit("kadmind.service")
server.wait_for_unit("kdc.service")

This comment has been minimized.

@kwohlfahrt

kwohlfahrt Dec 12, 2019
Author Contributor

Nits picked. Anything left blocking merging?

@FRidh FRidh added this to Needs review in Staging Nov 30, 2019
kwohlfahrt added 3 commits Nov 23, 2019
Necessary for nfs server with kerberos auth
This is necessary for id mapping to work with NFS + Kerberos, and also
touches #68106 and 634638.
@kwohlfahrt kwohlfahrt force-pushed the kwohlfahrt:nfs-tests-python branch from 6ea57ed to d17c834 Dec 12, 2019
kwohlfahrt added 3 commits Nov 27, 2019
A patch is necessary upstream to support multiple configs via symlinks
in /etc/request-key.d

Once that is done, we can add support for CIFS as well
@kwohlfahrt kwohlfahrt force-pushed the kwohlfahrt:nfs-tests-python branch from d17c834 to e0b9b5f Dec 12, 2019
@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Dec 13, 2019

@GrahamcOfBorg test nfs

@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Dec 13, 2019

@GrahamcOfBorg test nfs.simple nfs.kerberos

@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Dec 13, 2019

@GrahamcOfBorg test nfs3.simple nfs4.simple nfs4.kerberos

@Mic92 Mic92 merged commit 1c329c9 into NixOS:staging Dec 13, 2019
15 checks passed
15 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
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
keyutils, linux, nfs-utils on aarch64-linux Success
Details
keyutils, linux, nfs-utils on x86_64-linux Success
Details
Staging automation moved this from Needs review to Done Dec 13, 2019
@kwohlfahrt kwohlfahrt deleted the kwohlfahrt:nfs-tests-python branch Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.