security.acme: added reload and restart lists for sytemd targets #34388

qknight · 2018-01-29T14:42:24Z

Motivation for this change

added a list for systemd reload and restart targets
using the nixos module system to set the default at the domain option (instead of using code in the config = section which duplicates code and makes it not accessible from outside the acme.nix scope)

Things done

Fits CONTRIBUTING.md.

zimbatm · 2018-01-29T14:49:09Z

nixos/modules/security/acme.nix

+            reload = mkOption {
+              type = types.listOf types.str;
+              default = [];
+              example = [ "postfix.service" ];


postfix.service is used in both examples. Which one is needed for postfix and can we find a different common example for the other-one?

my main goal is postfix.service but maybe there are other use-cases as well.

we could use dovecot.service for instance

zimbatm · 2018-01-29T14:51:38Z

nixos/modules/security/acme.nix

@@ -193,10 +225,11 @@ in
          servicesLists = mapAttrsToList certToServices cfg.certs;
          certToServices = cert: data:
              let
-                domain = if data.domain != null then data.domain else cert;
+                restartJobs = lib.unique data.systemd.restart;
+                reloadJobs = lib.subtractLists restartJobs (lib.unique data.systemd.reload);


What is the case where a restart jobs is promoted to reload only? It seems like a configuration error.

Reversely, if one wants to change a job from reloading to restarting, how would you do that?

Using a { jobName = "restart" | "reload" } data-structure would guarantee the uniqueness and allow that kind of override.

@zimbatm i 'feel' that there are use-cases but can't think of one atm. so maybe it is over-engineered.

many services as postfix.service and dovecot.service can load new certificates using reload so one does not have to cut connections, reset state and such, which would be the case with a full reload.

but there could be a plugin for dovecot, a plugin like https://pigeonhole.dovecot.org/, which hypotetically would require postfix to restart. i made this up of course, but the code using lib.unique and restart over reload is already there and simple so why not just use it.

qknight · 2018-01-29T14:52:27Z

i have to fix a issue with the overall design first and then i will reopen the PR

zimbatm · 2018-01-29T14:54:08Z

sure no problem

qknight · 2018-01-29T16:01:23Z

ATM i wonder why there is: cert.domain at all. what is the motivation to not just use the attrName as 'domain' key? if no such motivation is there i'd like to rewrite the service and force the attrName to be the domain and get rid of the domain mkOption completely.

fpletz · 2018-01-29T16:05:11Z

The domain option was introduced to make it possible to have different certificates with the same common name (i.e. with different SubjAltNames for different services).

qknight · 2018-01-29T20:10:12Z

now that @fpletz explained why domain needs to be there i think that this patch is working for both of our use-cases.

zimbatm · 2018-01-30T10:53:18Z

@qknight I pushed a simplified proposal to your branch that takes into account what we where saying previously. It still needs a bit of testing. Feel free to revert or change it however you like.

qknight · 2018-01-30T14:14:18Z

@zimbatm happy to use your modified version. i have to find out what has to be done now, do i merge your stuff now and then sqash-merge it?

zimbatm · 2018-01-30T16:32:38Z

yes or keep adding commits to the branch and I'll squash them during merge

qknight · 2018-01-31T12:50:20Z

another thing i don't understand is security.acme.certs.<name>.webroot. in nixcloud-webservices i'm using a constant here, which points to /var/lib/acme/acme-challenges for all my security.acme.certs.

i assume this value is closely coupled with the vhost abstraction in nginx you guys wrote, is that correct? i'd like to add a example value and a better description to it, any ideas what that could be?

zimbatm · 2018-01-31T17:32:34Z

yeah, I didn't build it but I'm pretty sure it's tied to the nginx enableTLS feature. It should work with Apache or any other webserver that can map the .well-known folder to that path. I believe that ACME has other validation methods, in which case this folder would be pretty useless.

qknight · 2018-02-01T16:29:51Z

@zimbatm if you like it, please merge it. i didn't test 'our' work in production yet, so merge with care. and thanks for your effort, very much appreciated!

qknight · 2018-02-02T13:54:11Z

@zimbatm @fpletz

regarding default values

at the moment this is the most important patch so that domain is always assigned to a resonable value (not null but a string) by default. this is an important patch, since i want to read the certificates directly and don't have to check if they are null and then "otherwise" use the attrName.

regarding restart/reload

creating the new reload and restart targets was motivated by the idea that one can merge the different configs for a single domain so nixcloud.email could use the same TLS certificates as nixcloud.webservices but since merging does not work our current solution is to use seperate certs for these two use-cases.

this usage still does not exceed the quota of weekly certs of 4.

we could basically drop this from the PR and maybe create a new abstraction: security.TLS which uses security.acme as a backend.

qknight · 2018-02-02T21:08:46Z

webroot might have to be a unique directory but i'm not sure.

zimbatm · 2018-02-04T12:48:33Z

How about creating a new PR with just the default values change?

The rest is less obvious to me and I don't have a setup to test it out right now; I think overall the idea is good, it just needs to be fleshed out a bit more.

qknight · 2018-02-04T14:26:20Z

@zimbatm yes, this seems to be a good idea. let's skip the restart/reload feature completely for the time being.

qknight · 2018-02-04T14:36:45Z

@zimbatm

please check this latest patch and squash merge it.
after your merge into master i would like to cherry-pick this into 17.09 later on, agreed?

zimbatm · 2018-02-06T19:16:40Z

@GrahamcOfBorg test acme

GrahamcOfBorg · 2018-02-06T19:17:01Z

Failure on aarch64-linux (full log)

Partial log (click to expand)

Package ‘botan-1.10.17’ in /var/lib/gc-of-borg/nix-test-rs-3/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-aarch64-community-3/pkgs/development/libraries/botan/generic.nix:46 is not supported on ‘aarch64-linux’, refusing to evaluate.

a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowBroken = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowBroken = true; }
to ~/.config/nixpkgs/config.nix.

GrahamcOfBorg · 2018-02-06T19:30:16Z

Success on x86_64-linux (full log)

Partial log (click to expand)

webserver: running command: sync
webserver: exit status 0
test script finished in 105.64s
cleaning up
killing client (pid 612)
killing letsencrypt (pid 593)
killing webserver (pid 623)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/tmp/nix-build-vm-test-run-acme.drv-0/vde1.ctl': Directory not empty
/nix/store/7f6jj93lgz7klhdsl6kc2gsqn9cfk4rk-vm-test-run-acme

zimbatm · 2018-02-06T19:42:16Z

@qknight looks good to me. It should be safe to be cherry-picked as well.

Any idea on what to name the commit?

qknight · 2018-02-06T19:50:01Z

something like acme_default_value_via_module_system

dotlambda

This breaks the build of the manual:

options-db.xml:6594: parser error : Opening and ending tag mismatch: name line 6594 and option
(<option>security.acme.cert.<name>.group</option>) to read SSL private certifica

dotlambda · 2018-02-06T21:36:02Z

Fixed XML in #34683

qknight requested review from zimbatm, fpletz and abbradar January 29, 2018 14:42

qknight force-pushed the acme_systemd_extension branch from 1a3c65f to e1abea1 Compare January 29, 2018 14:43

zimbatm reviewed Jan 29, 2018

View reviewed changes

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jan 29, 2018

qknight closed this Jan 29, 2018

qknight reopened this Jan 29, 2018

security.acme: added reload and restart lists for sytemd targets

32b1f93

qknight force-pushed the acme_systemd_extension branch from e1abea1 to 32b1f93 Compare January 29, 2018 20:43

qknight force-pushed the acme_systemd_extension branch from b566713 to 32b1f93 Compare February 1, 2018 16:28

zimbatm and others added 2 commits February 4, 2018 15:28

simplified proposal

cbdc86d

fixed missing "types" string and added example

299d871

reverted services reload/restart

8a4ba4f

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) labels Feb 4, 2018

tiny fix

2e6775e

zimbatm merged commit edeacd0 into NixOS:master Feb 6, 2018

dotlambda reviewed Feb 6, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

security.acme: added reload and restart lists for sytemd targets #34388

security.acme: added reload and restart lists for sytemd targets #34388

qknight commented Jan 29, 2018

zimbatm Jan 29, 2018

qknight Jan 29, 2018

qknight Jan 29, 2018

zimbatm Jan 29, 2018 •

edited

qknight Jan 29, 2018

qknight Jan 29, 2018 •

edited

qknight commented Jan 29, 2018

zimbatm commented Jan 29, 2018

qknight commented Jan 29, 2018

fpletz commented Jan 29, 2018

qknight commented Jan 29, 2018

zimbatm commented Jan 30, 2018

qknight commented Jan 30, 2018

zimbatm commented Jan 30, 2018

qknight commented Jan 31, 2018

zimbatm commented Jan 31, 2018

qknight commented Feb 1, 2018

qknight commented Feb 2, 2018 •

edited

qknight commented Feb 2, 2018

zimbatm commented Feb 4, 2018

qknight commented Feb 4, 2018

qknight commented Feb 4, 2018

zimbatm commented Feb 6, 2018

GrahamcOfBorg commented Feb 6, 2018

GrahamcOfBorg commented Feb 6, 2018

zimbatm commented Feb 6, 2018

qknight commented Feb 6, 2018

dotlambda left a comment

dotlambda commented Feb 6, 2018

security.acme: added reload and restart lists for sytemd targets #34388

security.acme: added reload and restart lists for sytemd targets #34388

Conversation

qknight commented Jan 29, 2018

Motivation for this change

Things done

zimbatm Jan 29, 2018

Choose a reason for hiding this comment

qknight Jan 29, 2018

Choose a reason for hiding this comment

qknight Jan 29, 2018

Choose a reason for hiding this comment

zimbatm Jan 29, 2018 • edited

Choose a reason for hiding this comment

qknight Jan 29, 2018

Choose a reason for hiding this comment

qknight Jan 29, 2018 • edited

Choose a reason for hiding this comment

qknight commented Jan 29, 2018

zimbatm commented Jan 29, 2018

qknight commented Jan 29, 2018

fpletz commented Jan 29, 2018

qknight commented Jan 29, 2018

zimbatm commented Jan 30, 2018

qknight commented Jan 30, 2018

zimbatm commented Jan 30, 2018

qknight commented Jan 31, 2018

zimbatm commented Jan 31, 2018

qknight commented Feb 1, 2018

qknight commented Feb 2, 2018 • edited

regarding default values

regarding restart/reload

qknight commented Feb 2, 2018

zimbatm commented Feb 4, 2018

qknight commented Feb 4, 2018

qknight commented Feb 4, 2018

zimbatm commented Feb 6, 2018

GrahamcOfBorg commented Feb 6, 2018

GrahamcOfBorg commented Feb 6, 2018

zimbatm commented Feb 6, 2018

qknight commented Feb 6, 2018

dotlambda left a comment

Choose a reason for hiding this comment

dotlambda commented Feb 6, 2018

zimbatm Jan 29, 2018 •

edited

qknight Jan 29, 2018 •

edited

qknight commented Feb 2, 2018 •

edited