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

Improving integration of `nslcd`, PAM and `openldap`. #53762

Merged
merged 3 commits into from Jan 30, 2019

Conversation

@ju1m
Copy link

commented Jan 10, 2019

Motivation for this change

Restart nslcd service when its configuration is modified by Nix.
And change PAM config to enable LDAP password changing through nslcd.

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.

Previously, for example, changing an LDAP account's password through PAM
was not possible: the whole password module verification
was exited prematurely by <literal>pam_unix</literal>,
preventing <literal>pam_ldap</literal> to manage the password as it should.

This comment has been minimized.

Copy link
@flokli

flokli Jan 12, 2019

Contributor

We should also mention that (and why) we load password required pam_deny.so at the end.

This comment has been minimized.

Copy link
@ju1m

ju1m Jan 18, 2019

Author

I've put that pam_deny by precaution, since I've seen it a few line above in auth required pam_deny.so and in Debian's /etc/pam.d/common-password with the comment: "here's the fallback if no module succeeds". But Debian seems to need that pam_deny because it is using some jumping over modules with [success=N, …] generated controls, cf. pam.conf(5):

N (an unsigned integer)
    equivalent to ok with the side effect of jumping over the next N modules in the stack.

As far as I can test, appending password required pam_deny.so is not needed in current NixOS PAM config: both with or without it, giving a wrong LDAP password gives an authentication failure as expected; but I am no PAM expert.

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

This comment has been minimized.

Copy link
@flokli

flokli Jan 12, 2019

Contributor

trailing whitespace.

@flokli

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2019

@Mic92 can take a look at the ldap-specific bits itself, as written in #46792 (review) ?

test -z "${cfg.bind.distinguishedName}" -o ! -f "${cfg.bind.password}" ||
printf 'bindpw %s\n' "$(cat ${cfg.bind.password})"
test -z "${cfg.daemon.rootpwmoddn}" -o ! -f "${cfg.daemon.rootpwmodpw}" ||
printf 'rootpwmodpw %s\n' "$(cat ${cfg.daemon.rootpwmodpw})"

This comment has been minimized.

Copy link
@Mic92

Mic92 Jan 17, 2019

Contributor

Does this need any escaping?

This comment has been minimized.

Copy link
@ju1m

ju1m Jan 18, 2019

Author

I've pushed a change quoting ${cfg.bind.password} and ${cfg.daemon.rootpwmodpw} in the cat since here someone could be using (very unwise) paths with spaces. What kind of escaping do you have in mind?

Julien Moutinho
nixos/tests: test LDAP password changing through nslcd
NOTE: slapd.conf is deprecated, hence use cn=config.

@ju1m ju1m force-pushed the ju1m:nslcd branch from 3762ec9 to 65cfba2 Jan 18, 2019

@flokli

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

@GrahamcOfBorg build nixosTests.ldap

@flokli

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

built and verified locally.

I guess improving some escaping can be done in a followup PR if needed, but no need t delay this further.

@flokli flokli merged commit d3c2ed2 into NixOS:master Jan 30, 2019

10 of 13 checks passed

nixosTests.ldap on aarch64-linux Failure
Details
nixosTests.ldap on x86_64-darwin No attempt
Details
nixosTests.ldap on x86_64-linux Failure
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-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
@srhb

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

This appears to have broken the ldap test. Was that intentional?

@srhb

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

eb90d97 looks like the culprit

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