Skip to content

Comments

Add mutable config option#1

Open
kwohlfahrt wants to merge 13 commits intoposcat0x04:openldap-modernizefrom
kwohlfahrt:openldap-modernize
Open

Add mutable config option#1
kwohlfahrt wants to merge 13 commits intoposcat0x04:openldap-modernizefrom
kwohlfahrt:openldap-modernize

Conversation

@kwohlfahrt
Copy link

This mainly adds the option mutableConfig, as discussed in NixOS#141240. I got carried away and did a few other things too:

  1. Refactor the tests. They now only use one VM (with specializations) instance, instead of two, speeding things up a bit.
  2. Add more test cases (to cover the new mutableConfigoption).
  3. Minor fixes to escape args correctly (necessary for ldapi:/// sockets with custom path)
  4. Split the declarative content scripts into separate ExecStartPre lines. This makes it obvious from systemctl status which part has failed.

I added one additional assertion, that means declarative database contents are not usable with the configDir option (which essentially bypasses all of this module). I don't think this check is technically 100% necessary, since if the custom configDir contains the right olcDbDirectory it should work, but it's easy to get wrong and I don't see much of a use case for using NixOS for database contents, but not config.

I'm also a little cautious about the other assertion (assertion = dataDirs ? "${dn}";) - when we have mutableConfig = true, this assertion might pass, even though the actual configuration has diverged from what is in settings. This kind of thing is why I'm not a huge fan of mutableConfig. Any suggestions for things to change here, or should we leave this as-is?

This is messy, because it means the config construction script has to
have root access to create the config dir and set permissions.
Additionally, the intention with systemd seems to be that
ConfigurationDirectory is world-readable, which is not suitable for us
because it contains secrets.

Look into LoadCredential for alternatives.
TODO: add mutable/immutable content, for symmetry.
@kwohlfahrt
Copy link
Author

As an aside, I took a brief look at implementing DynamicUser = true for this unit. The difficulty here is providing access to secrets - currently this is done using included attributes (like foobar:< file:///path/to/secret) but that doesn't work if we don't know what user needs access to the file.

I'm planning to use LoadCredentials, but this is not available in ExecStartPre (see systemd/systemd#19604). I don't think this is a deliberate decision on their part, more of an oversight, so we can probably get it changed. Do you think it's worth putting some time into enabling DynamicUser?

@poscat0x04
Copy link
Owner

Sorry I'm very busy right now, I'll review this PR asap.

@kwohlfahrt
Copy link
Author

Hi @poscat0x04 - any chance you'll have some free time to review this in the near future?

If not, do you mind if I open a new PR against NixOS/nixpkgs to replace yours with these changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants