Hi, On Thu, Sep 29, 2011 at 23:15, Rickard Nilsson <[email protected]> wrote: > I finally got around fixing the LDAP patch according to your suggestions.
Great. > The password is now stored in a separate file, which is read from the > activation script. I also cleaned up the options definitions a bit. Would > you care to look at it again? No problem. I look at your solution, storing the password in a file is the good way to proceed. 1/ I checked that sed copy the content of the symbolic link before creating a new file at the same location. This is important otherwise you could have modified the content of the nix-store which is a good thing to do. I still have a doubt about updates, could you check that the ldap configuration is well updated when changing any option? Because your configuration file is no longer a symlink to /etc/static. 2/ your activation script has no dependencies. Have a look at /var/run/current-system/activate to check which one is important, I guess you may need "etc" and may be "users" such as modules/services/misc/nix-daemon.nix: system.activationScripts.nix = stringAfter [ "etc" "users" ] modules/services/misc/nix-daemon.nix- '' modules/services/misc/nix-daemon.nix- # Set up Nix. Setting this is to get snippet order correctly. The activation is run just after the stage1. Thus almost nothing is setup yet. These should not be hard modification, and I would be please to merge your patch after that ;) > Best regards, > Rickard Nilsson > > > Den 2011-08-28 00:43:53 skrev Nicolas Pierron <[email protected]>: > >> Hi Richard, >> >> On Sat, Aug 27, 2011 at 12:41, Rickard Nilsson >> <[email protected]> wrote: >>> >>> I need to bind to my LDAP server with credentials when looking up users, >>> so >>> I added the options "bindAnonymously", "binddn" and "bindpw" to >>> modules/config/ldap.nix. >> >> Thanks for contributing. >> >>> I think the patch should be rather uncontroversial, >>> but I'm happy to make any adjustments required to get it in. >> >> I have some remarks about your patch before accepting it into the >> mainline. >> >> 1/ Based on the context I can't blame you but the current way to go is >> to use type for option declarations such as >> >> type = with pkgs.lib.types; bool; >> type = with pkgs.lib.types; string; >> >> This help users by reporting errors early as well as providing >> specialized merge rules. >> >> 2/ Your patch has a security issue. All users have access to the >> /nix/store, especially the ldap.conf file produced by the function >> pkgs.writeText. Thus, "bindpw" field would appear as readable by all >> users of your machine. Today, we have no mean to prevent storage of >> some files in a public (to all users of the computer) nix store. To >> use password safely in NixOS you must declare a file containing the >> password, and use the activation script to substitute a pattern by the >> content of the file. >> >> 3/ All your options are starting by "bind", could you make an >> attribute of it and use clear name for the fields, such as: >> >> bind = { >> Identified = mkOption { >> default = false; >> type = with pkgs.lib.types; bool; >> description = " ... "; >> }; >> >> domainName = mkOption { >> ... >> }; >> >> password = mkOption { >> default = "/etc/ldap/bind.password"; >> type = with pkgs.lib.types; string; >> description = " ... "; >> }; >> }; >> >> >> I have additional question which are not related to your patch, but to >> the difficulty you encounter to get your hands dirty by patching >> NixOS. Your answers to these questions interest me to improve the >> overall user experience. Did you use the documentation wiki/manual ? >> Is it readable ? Did you found ldap.nix easily ? How many attempts >> did you had before getting a working configuration ? How much did >> that took between the need and your first working patch ? >> >> Sincerely, -- Nicolas Pierron http://www.linkedin.com/in/nicolasbpierron - http://nbp.name/ _______________________________________________ nix-dev mailing list [email protected] http://lists.science.uu.nl/mailman/listinfo/nix-dev
