nixos/ldap: unify secrets handling #64951
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Most of the module was written with the shorter style, but ldapConfig was not. This patch just makes the module internally consistent.
Without nscd, most programs won't have the LDAP NSS modules on their LD_LIBRARY_PATH and so will fail all LDAP lookups.
Instead of having a mkIf on pretty much every setting, divide the settings into those which apply when nslcd is enabled, versus those which apply when nslcd is disabled. I find this easier to understand.
Most of the generated config file is the same whether you're using the daemon or not.
Whether users.ldap.daemon.enable is true or false, there's a config file which may need to have secrets added to it. So let's pick one way of doing that and use it for both. Any NixOS system which uses LDAP for user lookups needs to have nscd enabled. That means that whether we're using the LDAP client daemon or not, there's a single systemd service which depends on our LDAP config file: either nslcd or nscd, respectively. Previously this module used an activation snippet in the no-daemon case, but we can extend the nscd service definition with an extra ExecStartPre instead, much like the one which the daemon case already used. So changing the secrets now only requires restarting the appropriate daemon, rather than re-running the activation script. The LDAP client daemon previously had restartTriggers set to a fixed path, which meant it had no effect. restartTriggers only work if the specified filename changes when the configuration has changed in a way which requires a restart. This patch ensures that the ExecStartPre directive changes if any part of the configuration changes, which is sufficient to ensure that switch-to-configuration will restart the daemon if and only if that's necessary. The two cases ran with different privileges. With the daemon, the secrets needed to be readable by the unprivileged nslcd user. Without it, the secrets were read by root. This patch makes both cases run as root (using systemd's "!" prefix on Exec* commands), then change the generated file's ownership as necessary. So now the administrator can have the secrets be owned by any user they want. Both cases have hard-coded paths for their config files, in /etc, but we can simply symlink those into /run if we have to attach secrets. Previously this module either LD_PRELOADed a library to rewrite the config file path, or overwrote the symlink which had been constructed by system/etc/etc.nix. Neither option is necessary. In both configurations, `mktemp` was used without arguments, which means it placed the temporary config files in /tmp. In some configurations that's a different filesystem than /etc, and it's almost certainly a different filesystem than /run, so the subsequent `mv` commands involved copying the file an extra time. Worse than being mildly inefficient, it doesn't replace the destination atomically, so I can't easily convince myself that there are no correctness or security bugs there. This patch puts the temporary file in the same directory as the final config file, so the final rename is atomic. Tested with: nix-build nixos/release.nix -A tests.ldap.x86_64-linux
ofborg
bot
added
6.topic: nixos
|
Oops, sorry to leave this idling around for so long. I just stumbled over it while trying to fix the I really like this - could you rebase to master? |
ryantm
added
2.status: merge conflict
|
I marked this as stale due to inactivity. → More info |
stale
bot
added
the
2.status: stale
stale
bot
removed
the
2.status: stale
wegank
added
the
2.status: stale
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: This pull request only works if both #64268 and #64387 are merged first, which it's looking like they will be. I'm opening this PR even though those haven't landed yet so that these changes can get some review ahead of time.
Motivation for this change
I just wanted to get rid of some activation scripts, but this module demanded a bit more attention.
These patches all affect only
nixos/modules/config/ldap.nix, which implements theusers.ldapconfiguration options for systems which connect to an LDAP server for authentication.Each patch cleans up some aspect of the module implementation, and the last patch additionally fixes several corner-case bugs around switching configurations while
users.ldap.enableis true, or changing secrets. Details are in the commit messages.Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)