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/ldap: unify secrets handling #64951

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jameysharp
Copy link
Contributor

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 the users.ldap configuration 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.enable is true, or changing secrets. Details are in the commit messages.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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.

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
@flokli
Copy link
Contributor

flokli commented Apr 28, 2020

Oops, sorry to leave this idling around for so long. I just stumbled over it while trying to fix the ldap test.

I really like this - could you rebase to master?

@stale
Copy link

stale bot commented Jun 3, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 3, 2021
@wegank wegank marked this pull request as draft March 20, 2024 15:12
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants