I've removed the static UIDs/GIDs as suggested in NixOS/rfcs#52, I think this is backwards compatible.

DynamicUser should be possible with ExecStart = !slapd ... (we need root initially to open sockets) and continuing to rely on slapd to drop permissions. This would not be compatible with user-defined data directories, if people are OK with that I can work on implementing it.

@GrahamcOfBorg eval

@GrahamcOfBorg build openldap

@GrahamcOfBorg test openldap

Thanks for the review, I've committed the suggestions.

I'm a bit confused about the grahamcofborg-eval failure (This PR does not cleanly list package outputs after merging.), with the error message:

nix-env failed:
warning: ignoring the user-specified setting 'restrict-eval', because it is a restricted setting and you are not a trusted user
error: stack overflow (possible infinite recursion)

I'm not sure what in this change triggers this, or what I can do to resolve it so advice is appreciated.

I can reproduce this by building the manual:

$ nix-build nixos/release.nix -A manual
error: stack overflow (possible infinite recursion)

Ah, the reason seems to be here:

ldapAttrsType = types.submodule {
  children = mkOption {
    # If I replace this with just types.attrs, the error does not occur
    type = types.attrsOf ldapAttrsType;
  };
};

Is there a way to override the documentation type for this case, so it doesn't go into the infinite recursion here?

Ah, the reason seems to be here:

ldapAttrsType = types.submodule {
  children = mkOption {
    # If I replace this with just types.attrs, the error does not occur
    type = types.attrsOf ldapAttrsType;
  };
};

Is there a way to override the documentation type for this case, so it doesn't go into the infinite recursion here?

Maybe lazyAttrsOf? https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/misc/home-assistant.nix#L106
Don't ask me what it does but it seems to support recursion. @infinisil might have better insights on this.

I think this is fixed now, by simply marking all options below the top-level of openldap.settings as visible = false. It's not super clean, but could be worse.

I'm not sure why CI hasn't triggered yet... Nevermind, there it goes.

Does it not include the schemas?

╭─[~/result]─joerg@eve
╰─ % sudo /nix/store/r5zff1swk044mjcx69k141i7f28rkn4s-openldap-2.4.50/bin/slaptest -f /nix/store/v1q6gl85rim8s9f1xbgna5h4yjkr35x3-slapd.conf -F /etc/openldap/slapd.d

config file testing succeeded

╭─[~/result]─joerg@eve
╰─ % sudo /nix/store/r5zff1swk044mjcx69k141i7f28rkn4s-openldap-2.4.50/bin/slaptest -u -F /etc/openldap/slapd.d
5f39668c olcObjectClasses: value #0 olcObjectClasses: ObjectClass not found: "uidObject"
5f39668c config error processing cn=config: olcObjectClasses: ObjectClass not found: "uidObject"
slaptest: bad configuration directory!

I think the schemas are correctly included by openldap.
It is just my own schemas that need to be move first into the schema domain or they cannot pick up the included schemas.

config.ldif.txt

Hm, I'm surprised that this PR caused changes here. Could you post the shell commands to reproduce after deployment? I'm assuming something like:

slapadd -F /etc/openldap/slapd.d -n0 -l ./config.ldif.txt
slaptest -u -F /etc/openldap/slapd.d

Anything else? Are you using a declarativeContents?

Could you try loading your schema with extraConfig (i.e. include /path/to/my/mySchema.schema)? The conversion using slaptest should handle this.

OK, I think all comments are addressed. Overall, I think it might be nice to keep the top-level defaultSchemas option, specifying "cn=schema.includes" = [ ... ] is tedious, but on the other hand only needs to be done once per config. Yes/no?

I'd also like some way of defaulting olcDbDirectory so it doesn't have to be explicitly specified, but can't think of a sensible way of doing this. As pointed out earlier, some database types don't persist to disk at all, and we'd have to find a way to avoid conflicts when multiple databases are present. Any suggestions?

Apart from those two points, I think this is ready to go now.

@Mic92 and @infinisil, thanks for the review. I think I've addressed all of the comments here and resolved the merge conflict. Is there anything else here that requires more discussion/work?

Thanks. This looks a lot better now: Mic92/dotfiles@d1eb62a