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

nslcd: restart when nslcd.conf changes and let pwdutils tools use libnss_ldap.so #46792

Closed
wants to merge 3 commits into from

Conversation

@ju1m
Copy link

commented Sep 17, 2018

Motivation for this change

Restart nslcd service when its configuration is modified by Nix. And set a global LD_LIBRARY_PATH to libnss_ldap.so so that getent and passwd can contact nslcd. Not sure what is missing to have passwd actually change the password as ldappasswd can. I was able to do it on Debian, but unable to find what config is missing to have passwd fully work on Nix.

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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

nixos/modules/config/ldap.nix Outdated
@@ -214,6 +214,9 @@ in
if cfg.daemon.enable then nss_pam_ldapd else nss_ldap
);

# Let pwdutils tools query nslcd through lbnss_ldap.so.
environment.sessionVariables.LD_LIBRARY_PATH = [ "${pkgs.nss_pam_ldapd}/lib" ];

This comment has been minimized.

Copy link
@Mic92

Mic92 Sep 18, 2018

Contributor

For nss modules we usually add those nscd's library path instead.

This comment has been minimized.

Copy link
@ju1m

ju1m Sep 18, 2018

Author

oh right, thank you Mic92! I've amended the pull request (as suggested by https://nixos.org/nixpkgs/manual/#idm140737315696608 )
The result seems ok.

# echo $LD_LIBRARY_PATH
/nix/store/rx76y57vl69ynnig325pid8jb0466298-systemd-239/lib:/nix/store/lm4vib2nl74z5hpsn11wfw2jxvmsgrwb-nss-pam-ldapd-0.9.7/lib

I still don't know though how to let passwd actually change a password through LDAP.

This comment has been minimized.

Copy link
@flokli

flokli Dec 14, 2018

Contributor

enabling changing the ldap password simply via passwd seems to be possible, it's described here: https://serverfault.com/a/806504

This comment has been minimized.

Copy link
@ju1m

ju1m Dec 25, 2018

Author

@flokli
Thanks. So, I am now able to change the LDAP password with passwd.
In nixpkgs/nixos/modules/security/pam.nix I had to:

           # Password management.
-          password requisite pam_unix.so nullok sha512
+          password sufficient pam_unix.so nullok sha512

I don't know if this fix is "secure", nor how to apply it simply without modifying Nixpkgs directly. In the same file, one can read:

# !!! TODO: move the LDAP stuff to the LDAP module, and the
# Samba stuff to the Samba module.  This requires that the PAM
# module provides the right hooks.

FYI, in the LDAP database, not only had I to give write and auth access to userPassword, but also read access to self on the ou=posix,… subtree, because when a user $USER runs passwd $USER, nslcd binds to the LDAP server as $USER with the current password and then do some searches on that subtree:

olcAccess: to attrs=userPassword
  by self write
  by anonymous auth
  by dn="gidNumber=0+uidNumber=0,cn=peercred,cn=external,cn=auth" write
  by * none
olcAccess: to attrs=shadowLastChange
  by self write
  by dn="gidNumber=0+uidNumber=0,cn=peercred,cn=external,cn=auth" write
  by * none
olcAccess: to dn.sub="ou=posix,dc=example,dc=coop"
  by self read
  by dn="gidNumber=${toString groups.nslcd.gid}+uidNumber=${toString users.nslcd.uid},cn=peercred,cn=external,cn=auth" read

Concerning the NixOS configuration of nslcd, here is my current one:

config = { 
  users.ldap = { 
    enable = true;
    server = "ldapi:///";
    base = "ou=posix,dc=example,dc=coop";
    daemon = { 
      enable = true;
      extraConfig = ''
        sasl_mech EXTERNAL
          # NOTE: nslcd cannot use SASL to bind to rootpwmoddn
          # which is the DN used by nslcd when passwd is run by root
          # to change the userPassword of an LDAP user.
          # SEE: https://www.reddit.com/r/linuxadmin/comments/53sxpl/how_do_i_configure_nslcd_to_use_a_sasl_external/d7w9awd/
          # Thus, use: ldappasswd -H ldapi:// -Y EXTERNAL uid=$user,ou=accounts,ou=posix,dc=example,dc=coop
      '';
    };
  };
};

Note also that the Reddit comment mentioned above recommends sssd as a better alternative to nslcd.

@@ -214,6 +214,9 @@ in
if cfg.daemon.enable then nss_pam_ldapd else nss_ldap
);

# Let pwdutils tools query nslcd through lbnss_ldap.so.
environment.sessionVariables.LD_LIBRARY_PATH = config.system.nssModules.path;

This comment has been minimized.

Copy link
@Mic92

Mic92 Sep 19, 2018

Contributor

What I meant to say is, that your pam module should be in nssModules so it gets loaded by the nscd daemon rather then exporting nssModules.path to LD_LIBRARY_PATH: https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/system/nscd.nix#L52

This comment has been minimized.

Copy link
@ju1m

ju1m Sep 20, 2018

Author

But here it's not nslcd which is missing libnss_ldap.so, it's glibc's utilities like getent or passwd, which, as far as I can understand, need to load libnss_ldap.so to reach nslcd.
Like in this 2012 bug on Ubuntu: https://bugs.launchpad.net/ubuntu/+source/libnss-ldap/+bug/1022903
Maybe those utilities can be listed and wrapped to set the correct LD_LIBRARY_PATH, maybe there is another mechanism for that kind of situation. I don't know. Meanwhile, a system wide LD_LIBRARY_PATH does the job :s

This comment has been minimized.

Copy link
@flokli

flokli Dec 14, 2018

Contributor

@ju1m NixOS uses nscd and modifies its LD_LIBRARY_PATH, so we don't have to do it globally - see commit message of e712417:

Preferably, we would not be using nscd at all. But we need to
use it because glibc reads nss modules from /etc/nsswitch.conf
by looking relative to the global LD_LIBRARY_PATH. Because LD_LIBRARY_PATH
is not set globally (as that would lead to impurities and ABI issues),
glibc will fail to find any nss modules.
Instead, as a hack, we start up nscd with LD_LIBRARY_PATH set
for only that service. Glibc will forward all nss syscalls to
nscd, which will then respect the LD_LIBRARY_PATH and only
read from locations specified in the NixOS config.
we can load nss modules in a pure fashion.

You might have been affected by negative caching, fixed in #50316 .

Can you try on master and check if that's still an issue? It might probably also make sense to add/extend a nixos test with your testcase.

This comment has been minimized.

Copy link
@ju1m

ju1m Dec 25, 2018

Author

@flokli Thanks again, I don't know for the cachin, but the LD_LIBRARY_PATH set only for nscd seems to work well.

This comment has been minimized.

Copy link
@flokli

flokli Dec 29, 2018

Contributor

So is setting LD_LIBRARY_PATH still needed for nslcd? If no, please drop that line.
If yes, please do in systemd.services.nslcd, but not system-wide.

@@ -214,6 +214,9 @@ in
if cfg.daemon.enable then nss_pam_ldapd else nss_ldap
);

# Let pwdutils tools query nslcd through lbnss_ldap.so.
environment.sessionVariables.LD_LIBRARY_PATH = config.system.nssModules.path;

This comment has been minimized.

Copy link
@flokli

flokli Dec 29, 2018

Contributor

So is setting LD_LIBRARY_PATH still needed for nslcd? If no, please drop that line.
If yes, please do in systemd.services.nslcd, but not system-wide.

@@ -242,6 +245,7 @@ in
''}
'';

restartTriggers = [ nslcdConfig.source ];

This comment has been minimized.

Copy link
@flokli

flokli Dec 29, 2018

Contributor

We should add a comment here that we need to put the config file in /etc/ above and "watch for it" here, as it's not possible to specify another nslcd config file path.

This comment has been minimized.

Copy link
@flokli

flokli Dec 29, 2018

Contributor

While at it, we can replace the RuntimeDirectory setup phase from above with the native systemd implementation.

This comment has been minimized.

Copy link
@ju1m

ju1m Dec 30, 2018

Author

So is setting LD_LIBRARY_PATH still needed for nslcd? If no, please drop that line.
If yes, please do in systemd.services.nslcd, but not system-wide.

Line dropped. My setting of LD_LIBRARY_PATH was for getent/passwd not for nslcd per se, but as far as I can test it's no longer needed, at least now than nscd caching is no longer in the way (#50316). Moreover both nscd and nslcd already have a custom LD_LIBRARY_PATH loading libnss_ldap.so:

I've force-pushed two commits, one to restart nslcd on config change, and one to use systemd's RuntimeDirectory.

@flokli

This comment has been minimized.

Copy link
Contributor

commented Dec 29, 2018

About the part of being able to change the password through passwd:

I think changing these settings directly is posisble, but really depends on other pam modules in there too, and we should extend nixos/tests/ldap.nix, to make sure it keeps working :-)

@ju1m ju1m force-pushed the ju1m:nslcd branch from cfa6d56 to d2e59e8 Dec 30, 2018
@ju1m ju1m requested a review from Infinisil as a code owner Dec 30, 2018
@ju1m

This comment has been minimized.

Copy link
Author

commented Dec 30, 2018

About the part of being able to change the password through passwd:

I think changing these settings directly is posisble, but really depends on other pam modules in there too, and we should extend nixos/tests/ldap.nix, to make sure it keeps working :-)

With the currently hardcoded password requisite pam_unix.so and the pam_ldap.so line put after it and not before, I don't know how to make it work.

Concerning the testing, I am too afraid to spend too much time on learning the testing machinery so far :(

@flokli

This comment has been minimized.

Copy link
Contributor

commented Dec 30, 2018

@ju1m I wanted to say changing the hardcoded values to the different settings is ok, in case the configuration then resembles more how it's done in other distros - see my configuration change in account management group.

About the ldap case,

password    sufficient    pam_unix.so […]
[…]
password sufficient pam_ldap.so  use_first_pass 
[…]
password    required      pam_deny.so

seems to be what distros normally configure, that's pretty much what we have in NixOS (except flipping password requisite to password sufficient as suggested)

Personally, I fear adding hooks to configure the pam configuration line outside of the pam module itself as suggested in pam.nix since 2013 might become a huge mess of configuration lines with priorities outside the pam module itself and complicate things a lot, so I kinda prefer to just change this in-place, documenting in the release notes as done in #52488.

Still, having some tests ensuring things work as expected (and making sure they don't break in the future) should be the goal, too.

For this PR, you probably only need to do the changes in the pam configuration, extend the ldap server permission configuration, and extend the testScript by running passwd on both clients, verifying the password was changed on the other one, or something similar.

NOTE: slapd.conf is deprecated, hence use cn=config.
@ju1m

This comment has been minimized.

Copy link
Author

commented Jan 8, 2019

Still, having some tests ensuring things work as expected (and making sure they don't break in the future) should be the goal, too.

For this PR, you probably only need to do the changes in the pam configuration, extend the ldap server permission configuration, and extend the testScript by running passwd on both clients, verifying the password was changed on the other one, or something similar.

So.. using the nixos/tests/ machinery was not that hard :)

nix build -f nixos/tests/ldap.nix

Except I've hit a Firefox 63 bug which is unable to load result/jquery.min.js because it fails to follow multiple symbolic links correctly, hence I couldn't expand the test results. As a quick and dirty workaround I did:

sudo ln -sf $(readlink -e result/jquery.min.js) result/jquery.js

I've pushed the tests I was able to write.
I had to rework a bit openldap's preStart to use cn=config instead of the deprecated slapd.conf. Maybe this code could be moved into nixos/services/ at some point.
Also, since chpasswd is only usable by root and NixOS's passwd does not have --stdin, I'm using expect to test password changing as a user. I've tried to make those tests robust, but I am no expect expert.

@Mic92

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2019

@flokli will continue to review this?

Copy link
Contributor

left a comment

Thanks a lot for wrapping this up!

Added some small nitpicks, but this looks very good in general!

@Mic92 I'm not very familiar with the ldap-specifics (ldif files, nixos-specific server startup), so would appreciate if you could take a look at these - also in what could/should be moved to the openldap module directly.

'';
systemd.services.openldap = {
preStart = ''
set -e

This comment has been minimized.

Copy link
@flokli

flokli Jan 9, 2019

Contributor

This might be a bit verbose, and parts of it could / should possibly be offloaded to using systemd's StateDirectory / RuntimeDirectory / tmpfiles from the NixOS openldap module itself, but shouldn't block this PR.

This comment has been minimized.

Copy link
@flokli

flokli Jan 9, 2019

Contributor

cc @Mic92

This comment has been minimized.

Copy link
@ju1m

ju1m Jan 9, 2019

Author

I've not changed to using systemd's StateDirectory= and StateDirectoryMode= because openldap.dataDir and openldap.configDir are absolute paths, whereas systemd's are relative to /var/lib.

rootpwmoddn = "cn=admin,${dbSuffix}";
rootpwmodpw = "/etc/nslcd.rootpwmodpw";
};
# XXX: password stored in clear in Nix's store, but this is a test.

This comment has been minimized.

Copy link
@flokli

flokli Jan 9, 2019

Contributor
Suggested change
# XXX: password stored in clear in Nix's store, but this is a test.
# NOTE: password stored in clear in Nix's store, but this is a test.

to match above comment style

distinguishedName = "cn=admin,${dbSuffix}";
password = "/etc/ldap/bind.password";
};
# XXX: password stored in clear in Nix's store, but this is a test.

This comment has been minimized.

Copy link
@flokli

flokli Jan 9, 2019

Contributor
Suggested change
# XXX: password stored in clear in Nix's store, but this is a test.
# NOTE: password stored in clear in Nix's store, but this is a test.

to match above comment style

dn: olcDatabase=config,cn=config
objectClass: olcDatabaseConfig
olcRootDN: cn=admin,cn=config
#olcRootPW:

This comment has been minimized.

Copy link
@flokli

flokli Jan 9, 2019

Contributor
Suggested change
#olcRootPW:
#olcRootPW:

trailing whitespace

@@ -338,7 +338,7 @@ let
auth required pam_deny.so
# Password management.
password requisite pam_unix.so nullok sha512
password sufficient pam_unix.so nullok sha512

This comment has been minimized.

Copy link
@flokli

flokli Jan 9, 2019

Contributor

can you add a line or two below my release notes about pam_unix in nixos/doc/manual/release-notes/rl-1903.xml, explaining the change?

@ju1m ju1m closed this Jan 9, 2019
@ju1m ju1m deleted the ju1m:nslcd branch Jan 9, 2019
@ju1m

This comment has been minimized.

Copy link
Author

commented Jan 9, 2019

Oops, while trying to rebase m nslcd branch with upstream nixpkgs (to get rl-1903.xml) while keeping a shallow repository locally, I've deleted my nslcd branch hoping to make Github accept my push, but this only made Github close this PR :s The working way seems to be through Compare and PR to myself: https://github.com/KirstieJane/STEMMRoleModels/wiki/Syncing-your-fork-to-the-original-repository-via-the-browser
I'll see if I can reopen this PR and reattach it to the new nslcd branch.

@ju1m

This comment has been minimized.

Copy link
Author

commented Jan 9, 2019

I see a Reopen button, but grayed out. I can only comment.

@ju1m

This comment has been minimized.

Copy link
Author

commented Jan 9, 2019

Updated commit is here https://github.com/ju1m/nixpkgs/commits/nslcd I don't know if I should wait a reopen of this PR or open a new one?

@flokli

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

@ju1m I also can't reopen this PR. Can you create a new one?

@ju1m

This comment has been minimized.

Copy link
Author

commented Jan 10, 2019

@ju1m I also can't reopen this PR. Can you create a new one?

Here we go: #53762
Sorry for breaking things :\

@flokli flokli referenced this pull request Jan 12, 2019
3 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.