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/openldap: fix assertion #64387

Merged
merged 1 commit into from Jul 17, 2019

Conversation

@jameysharp
Copy link
Contributor

commented Jul 6, 2019

Motivation for this change

In commit d43dc68, @Mic92 split the rootpw option to allow specifying it in a file kept outside the Nix store, as an alternative to specifying the password directly in the config.

Prior to that, rootpw's type was str, but in order to allow both alternatives, it had to become nullOr str with a default of null. So I can see why this assertion, that either rootpw or rootpwFile are specified, makes sense to add here.

However, these options aren't used if the configDir option is set, so as written this assertion breaks valid configurations, including the configuration used by nixos/tests/ldap.nix.

So this patch fixes the assertion so that it doesn't fire if configDir is set.

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.

nixos/openldap: fix assertion
In commit d43dc68, @Mic92 split the
rootpw option to allow specifying it in a file kept outside the Nix
store, as an alternative to specifying the password directly in the
config.

Prior to that, rootpw's type was `str`, but in order to allow both
alternatives, it had to become `nullOr str` with a default of `null`. So
I can see why this assertion, that either rootpw or rootpwFile are
specified, makes sense to add here.

However, these options aren't used if the configDir option is set, so as
written this assertion breaks valid configurations, including the
configuration used by nixos/tests/ldap.nix.

So this patch fixes the assertion so that it doesn't fire if configDir
is set.

@jameysharp jameysharp referenced this pull request Jul 6, 2019

Merged

nixos/nscd: DynamicUser and other cleanups #64268

4 of 10 tasks complete

jameysharp added a commit to jameysharp/nixpkgs that referenced this pull request Jul 14, 2019

nixos/ldap: unify secrets handling
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.

This commit needs both NixOS#64268 (nscd fixes) and NixOS#64387 (ldap test fixes)
merged before I'm willing to stand behind its quality.

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. One
advantage is that changing the secrets now only requires restarting the
appropriate daemon, rather than re-running the activation script.

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.

Tested with:
  nix-build nixos/release.nix -A tests.ldap.x86_64-linux
(but only with the above-mentioned pull requests merged as well)

jameysharp added a commit to jameysharp/nixpkgs that referenced this pull request Jul 14, 2019

nixos/ldap: unify secrets handling
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.

This commit needs both NixOS#64268 (nscd fixes) and NixOS#64387 (ldap test fixes)
merged before I'm willing to stand behind its quality.

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. One
advantage is that changing the secrets now only requires restarting the
appropriate daemon, rather than re-running the activation script.

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.

Tested with:
  nix-build nixos/release.nix -A tests.ldap.x86_64-linux
(but only with the above-mentioned pull requests merged as well)

@jameysharp jameysharp referenced this pull request Jul 15, 2019

Merged

resolvconf service: init #62955

3 of 10 tasks complete
@abbradar
Copy link
Member

left a comment

Seems a simple fix; I'll merge this in several days provided noone else who actually uses OpenLDAP jumps in.

@@ -237,8 +237,8 @@ in
config = mkIf cfg.enable {
assertions = [
{
assertion = cfg.rootpwFile != null || cfg.rootpw != null;
message = "Either services.openldap.rootpw or services.openldap.rootpwFile must be set";
assertion = cfg.configDir != null || cfg.rootpwFile != null || cfg.rootpw != null;

This comment has been minimized.

Copy link
@abbradar

abbradar Jul 15, 2019

Member

Can be written as cfg.configDir == null -> cfg.rootpwFile != null || cfg.rootpw != null for more clarity, but that's completely not important.

This comment has been minimized.

Copy link
@jameysharp

jameysharp Jul 15, 2019

Author Contributor

Thanks for reviewing! I think that was how I wrote it originally and I found it more confusing that way. Maybe it's just confusing no matter how it's written. 😓

@abbradar

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

BTW what was the test that you mentioned failed before this PR?

@jameysharp

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

The LDAP tests failed during nix-instantiate when I ran:

nix-build nixos/release.nix -A tests.ldap.x86_64-linux

The configuration there trips the assertion as it's currently written.

@abbradar

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

@GrahamcOfBorg test ldap

@jameysharp jameysharp referenced this pull request Jul 16, 2019

Open

nixos/ldap: unify secrets handling #64951

4 of 10 tasks complete

@abbradar abbradar merged commit d4e5748 into NixOS:master Jul 17, 2019

14 checks passed

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="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
tests.ldap on aarch64-linux Success
Details
tests.ldap on x86_64-linux Success
Details

@jameysharp jameysharp deleted the jameysharp:fix-ldap-tests branch Jul 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.