[Nix-dev] [PATCH] LDAP non-anonymous bind

[Nicolas Pierron]

Hi,

On Thu, Sep 29, 2011 at 23:15, Rickard Nilsson
<rickard.nilsson at telia.com> 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 <nicolas.b.pierron at gmail.com>:
>
>> Hi Richard,
>>
>> On Sat, Aug 27, 2011 at 12:41, Rickard Nilsson
>> <rickard.nilsson at telia.com> 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/