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/nsswitch cleanup nss modules #87016

Merged
merged 11 commits into from May 14, 2020
Merged

nixos/nsswitch cleanup nss modules #87016

merged 11 commits into from May 14, 2020

Conversation

@flokli
Copy link
Contributor

flokli commented May 5, 2020

This moves the remaining NSS modules to their corresponding modules, and contains some follow-up fixes, where we missed adding to the group database (as the passwdArray was used both for system.nssDatabases.passed and system.nssDatabases.group).

It also removes the conditional loading of systemd nss modules, as requested in #86940 (comment).

closes #86350.

Motivation for this change

#86350

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.
@flokli flokli requested review from arianvp, florianjacob and dasJ May 5, 2020
@flokli flokli changed the title nsswitch cleanup nixos/nsswitch cleanup nss modules May 5, 2020
@flokli flokli mentioned this pull request May 5, 2020
5 of 5 tasks complete
@flokli flokli requested a review from andir May 5, 2020
@dasJ
Copy link
Member

dasJ commented May 5, 2020

@GrahamcOfBorg test google-oslogin avahi

Samba test doesn't seem to be testing the right stuff.
LDAP test is broken as in #86305.

@dasJ
Copy link
Member

dasJ commented May 5, 2020

The code looks good to me, but I haven't tested it (as I don't use most of the stuff), which is why I'm commenting instead of approving.

Thanks for spotting my mistake with the groups, looks good now.

@flokli
Copy link
Contributor Author

flokli commented May 5, 2020

Maybe ping @xastor and @ajs124 on the samba changes, as there are no VM tests testing this currently.

@ajs124
Copy link
Member

ajs124 commented May 5, 2020

I say what @dasJ says.

@dasJ
Copy link
Member

dasJ commented May 6, 2020

@ajs124 As a general rule of thumb or only in this particular case?

@flokli flokli force-pushed the flokli:nsswitch-cleanup branch from 2cbec66 to 33b0343 May 6, 2020
@flokli
Copy link
Contributor Author

flokli commented May 6, 2020

squashed the fixup commit and rebased on master.

Copy link
Member

andir left a comment

Does look reasonable to me.

Do we have some tests or can you write some integration tests with beaker? ;)

nixos/modules/config/nsswitch.nix Outdated Show resolved Hide resolved
nixos/modules/system/boot/systemd.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/sssd.nix Show resolved Hide resolved
@flokli flokli requested a review from andir May 9, 2020
@flokli flokli force-pushed the flokli:nsswitch-cleanup branch from 33b0343 to 6330aec May 9, 2020
@flokli flokli requested a review from doronbehar May 9, 2020
flokli added 9 commits May 5, 2020
nixos/modules/config/nsswitch.nix uses `passwdArray` for both `passwd`
and `group`, but when moving this into the sss module in
edddc7c, it didn't get split
appropriately.
nixos/modules/config/nsswitch.nix uses `passwdArray` for both `passwd`
and `group`, but when moving this into the google-oslogin module in
4b71b6f, it didn't get split
appropriately.
nixos/modules/config/nsswitch.nix uses `passwdArray` for both `passwd`
and `group`, but when moving this into the systemd module in
c0995d2, it didn't get split
appropriately.
now that passwdArray and shadowArray aren't used anymore, these can be
folded.
This is now already triggered by the nsswitch module, as we set
system.nssModules.
A disabled nscd breaks nss module loading on NixOS, and systemd without
its nss modules doesn't really work either - instead of silently
disabling its nss modules if nscd is disabled, let the assertion in
nsswitch handle this.
flokli added 2 commits May 9, 2020
This is all inside a global cfg.enable conditional, so we don't need to
check here again.
Show the config option triggering the assertion, so people don't
necessary lookup the nixpkgs source code.
@flokli flokli force-pushed the flokli:nsswitch-cleanup branch from 6330aec to 23ba506 May 11, 2020
@flokli
Copy link
Contributor Author

flokli commented May 11, 2020

Improved the error message as described, and rebased on top of master another time. PTAL.

@flokli
Copy link
Contributor Author

flokli commented May 14, 2020

Okay, let's cook this in unstable.

@flokli flokli merged commit 4a85559 into NixOS:master May 14, 2020
14 checks passed
14 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="23ba506"; rev="23ba50611321035e02f5bfc2f5f809600ccc2f7b"; } ./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="23ba506"; rev="23ba50611321035e02f5bfc2f5f809600ccc2f7b"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="23ba506"; rev="23ba50611321035e02f5bfc2f5f809600ccc2f7b"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="23ba506"; rev="23ba50611321035e02f5bfc2f5f809600ccc2f7b"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="23ba506"; rev="23ba50611321035e02f5bfc2f5f809600ccc2f7b"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="23ba506"; rev="23ba50611321035e02f5bfc2f5f809600ccc2f7b"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="23ba506"; rev="23ba50611321035e02f5bfc2f5f809600ccc2f7b"; } ./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
@flokli flokli deleted the flokli:nsswitch-cleanup branch May 14, 2020
@orivej
Copy link
Contributor

orivej commented May 20, 2020

The current error message is not very helpful, I still had to look at the source code, look up the commit and this PR to understand what is going on:

Loading NSS modules from system.nssModules (/nix/store/…), requires services.nscd.enable being set to true.

I'd suggest a more actionable error message:

NSS modules from system.nssModules (/nix/store/…) will not be loaded without nscd, please set system.nssModules = lib.mkForce [ ]; or services.nscd.enable = true;

(I guess that those who have already disabled nscd would rather keep it disabled and do not need this assertion at all. However, the new users might like to be warned when they disable nscd for the first time…)

@flokli
Copy link
Contributor Author

flokli commented May 20, 2020

@orivej the problem was that people disabling nscd didn't really notice they had their system with a dysfunctional NSS system.

Most code adding to nss modules only did this "optionally" when nscd was enabled (and by doing this, never triggering the assertion). However, there's a lot of places in NixOS where we rely on a functional NSS system, so alerting in these cases by actually triggering the assertion is preferrable.

The suggested error message isn't that clear either on what's preferred now. If we want to avoid people having to read the source, we probably need to be a bit more verbose. What about this?

NSS modules from system.nssModules (/nix/store/…) will not be loaded unless nscd is enabled. Disabling nscd is stongly discouraged, as a dysfunctional NSS system breaks features already used in a base NixOS system, such as systemd dynamic user lookups or container hostname resolution. If you really know what you're doing, you can override this by setting system.nssModules = lib.mkForce [ ];.

Can you elaborate on usecases to disable nscd? When would one explicitly want to opt in to have broken NSS lookups?

@orivej
Copy link
Contributor

orivej commented May 20, 2020

I have disabled nscd because I do not want to erase the boundary between Linux network namespaces when it comes to DNS — otherwise the requests from nscd-aware applications originate from the nscd namespace rather than the application namespace. (Here are the relevant bits of my config: #52411 (comment).) I know that this disables some systemd-based name resolution and I'm OK with that since I never encounter such names.

I'd suggest this edit:

NSS modules from system.nssModules (/nix/store/…) will not be loaded unless nscd is enabled. Disabling nscd breaks systemd dynamic user lookup and container hostname resolution. To enable nscd, do not set services.nscd.enable = false; to proceed without nscd, set system.nssModules = lib.mkForce [ ].

@flokli
Copy link
Contributor Author

flokli commented May 21, 2020

@orivej thanks for the pointer! That's an interesting usecase. I wonder if you could just make nscd unavailable from these namespaced units, via BindPaths, or whether there's a way to restrict this to tell nscd-aware applications to not resolve hosts via nscd from that namespace.

I don't remember exactly how the breakages looked (need to check), but "systemd dynamic user lookup and container hostname resolution" are only examples on a basic installation - with disabled nscd, all custom nss modules will break.

@flokli flokli mentioned this pull request May 23, 2020
7 of 14 tasks complete
@delroth

This comment has been minimized.

Copy link
Contributor

delroth commented on 90bc3ec Jun 24, 2020

This should have been [ pkgs.sssd ] instead.

This comment has been minimized.

Copy link
Contributor Author

flokli replied Jun 24, 2020

You're right - can you file a PR fixing this (and possibly adding a test)?

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.

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