For these 2 options (interval and before), the acme module already has options that probably do the same: security.acme.{renewInterval,validMin}. So I suggest using the acme names for them.

Fixed in: a94946e

Can be just

type = with types; attrsOf (submodule {

And therefore remove 2 parens at the end of this section.

And it might work well to use attrsOf (either path (submodule { here instead, this would be able to replace impSpecs (so this should probably be renamed to just specs then).

Removed parens as part of: a94946e .. Will look into the possibility (and potential problems) of merging impSpecs and declSpecs now.

Combined impSpecs and declSpecs in e77155c

While I understand the intention behind combining the specs, I think it'll lead to a poorer understanding of the intended separation. It's important that impSpecs are never imported into the store, since they'll most likely (always?) contain secrets. The either machinery necessary to facilitate a joined specs introduces a lot of opportunities for future maintainers to accidentally import the paths -- and even if it's safe (which it is in the current state) the error messages if you muck up the value are not great.

In contrast, impSpecs might look non-dry, but it requires very trivial machinery and it'll be easier to ensure that none of its values are accidentally leaked into the store.

The > should also be escaped.

Fixed in a94946e

Is there a problem with always setting this to true? Why is it false by default? I would like to have this handled automatically, an option for this doesn't seem justified.

Removed option mkTargetDirs as part of e77155c

Is this description correct? The linked page says "this specifies the service manager to use for restarting or reloading services."

Also, can this ever be anything other than systemd? We are on NixOS after all, which doesn't support anything else.

Whoops.. c/p error. as for the description. It is changed as part of a94946e.

Regarding other init-systems, yes. We probably won't need them in NixOS. But the command service manager is useful, when you just want to execute a verbatim command or script instead of invoking systemd, and dummy can be useful for debugging.

I'm personally ok with leaving the list as-is (vendor provided). But if you think so, I'm also happy to remove circus openrc and sysv. As long as default remains to be systemd.

Unnecessary parens

Fixed in a94946e

This is just the same as the one string itself, no need for concatStringsSep

Fixed in a94946e

I think it would be nicer without with pkgs; here, it's only needed 3 times and it's rather short.

Done. a94946e

Same here, pkgs is only needed once.

Done. a94946e

This whole file could need a big nice let in at the start to declare everything and make the code much more readable, so many brackets right now!

Done. e77155c

The { ... }: here is still unnecessary, can drop another pair of () then.

Is there a problem with having the path type in front of the submodule as originally recommended? I'd rather not go scrolling down many lines to find the other part of the either. So I suggest using either path (submodule { ... as previously mentioned.

Will remove the { ... }:, no worries :)

I'm so happy you ask about the order of the types, because yes there is a problem actually. IMO it's a bug in the module system. You see, the type check for types.path is defined as builtins.substring 0 1 (toString x) == "/"; . This means that if x is of type attrs, then (toString x) will fail with cannot coerce set to string.

types.either is defined as t1.check || t2.check. This means that if the submodule is t1, then the t1 type check will guard the failing path check, which thus will never be executed for sets. If, on the other hand, the submodule becomes t2 in the either-clause, then the path type-check will fail.

I think this should be reported as a bug. The path type-check should somehow check if the input is coercible to a string (and if not return false) before treating it as a string.

{ ... } removed in 9082a61

Should the check for path really be (builtins.tryEval (builtins.substring 0 1 (toString x) == "/")).value? It feels icky to use tryEval here, but I certainly do expect either to not care about the order of its arguments, and path does break this as described.

Disregard my comment here, it was pointed out to me that it's complete nonsense, since string coercion errors aren't catchable with tryEval.

This seems to be the same as just allSpecs = pkgs.linkFarm "certmgr.d" specs;

Of course. I just forgot to reduce it after all my testing. Fixed in 9082a61

Could also use this which looks a bit cleaner imo:

specs = mapAttrsToList (n: v: rec {
  name = n + ".json";
  path = if isAttrs v then pkgs.writeText name (builtins.toJSON v) else v;
}) cfg.specs;

Changed to match your suggestion in: 9082a61

Couple things: n isn't used -> concatMap/map instead. More descriptive name instead of v (can be confusing with later v). Named this variable properly, and removed the flexibility of set and using cfg.specs always instead, that's all we need. I don't think using unique is worth it, it's an expensive operation (O(n^2)) and mkdir handles duplicates just fine. And some more things. All in all I think it should look like this:

specPaths = map dirOf (concatMap (spec:
  if isAttrs spec then
    collect isString (filterAttrsRecursive (n: v: isAttrs v || n == "path") spec)
  else
    [ spec ]
) (attrValues cfg.specs)));

preStart = ''
  ${concatStringsSep " \\\n" (["mkdir -p"] ++ map escapeShellArg specPaths)}
  ...
''

Changed to match your suggestion in: 9082a61

Fixed in 9082a61 .. And I added an extra assertion to check if the list of specs is empty.

Why this weird check? Maybe a $machine->sleep(3) will work instead if the waitForUnit really isn't enough.

The check is there to confirm that nginx is in fact restarted by certmgr after certificate issue. Starting nginx ... would be present in the journal more than once if nginx was restarted. waitForUnit will terminate just fine even if certmgr restart action is set to no-op/dummy.

Ah I see

Almost missed this, but very important:

Like this, this service is running as root, which should be avoided (also see #41092). Check out the DynamicUser and RuntimeDirectory systemd options, they should probably suffice.

Unfortunately it is not as simple as DynamicUser.. Ok.. I looked at the acme module, curious to see how it handles private key file-permissions, when running as non-root. Certmgr has to be able to issue certificates, and generate private keys for daemon A, B and C - each running with different users - and still private key files must be protected and readable for the exact daemon it is intended to only.

It looks like the acme module creates a systemd service for each cert, allowing for different users per cert. @infinisil .. Do you think I should rewrite the certmgr module along the same lines, or do you have other suggestions?

lib is already with;'d above, can be removed.

any (e: e == x) [ ... ] = elem x [ ... ]

the parens around that can also be removed, function application binds very strongly