nixos/cfssl: Add new module for cfssl #44127

johanot · 2018-07-26T14:38:46Z

based on module originally written by @srhb
complies with available options in cfssl v1.3.2
uid and gid 299 reserved in ids.nix
added simple nixos test case

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

johanot · 2018-07-26T16:03:11Z

@GrahamcOfBorg test cfssl

srhb · 2018-07-26T16:38:10Z

@johanot You need to have privileges in OfBorg for that.

@GrahamcOfBorg test cfssl

GrahamcOfBorg · 2018-07-26T16:38:36Z

Success on aarch64-linux

Attempted: tests.cfssl

No partial log is available.

GrahamcOfBorg · 2018-07-26T16:43:02Z

Success on x86_64-linux (full log)

Attempted: tests.cfssl

Partial log (click to expand)

machine: exit status 1
syncing
machine: running command: sync
machine: exit status 0
test script finished in 12.46s
cleaning up
killing machine (pid 597)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/8gklc39jal58cfjhzqb7qx6v0xgg1hxn-vm-test-run-cfssl

infinisil · 2018-07-28T03:04:21Z

nixos/modules/services/security/cfssl.nix

+
+    dataDir = mkOption {
+      default = "/var/lib/cfssl";
+      type = types.str;


Use types.path instead

infinisil · 2018-07-28T03:05:22Z

nixos/modules/services/security/cfssl.nix

+in {
+  options.services.cfssl = {
+    enable = mkEnableOption ''
+      Whether to enable the CFSSL CA api-server.


Only use mkEnableOption "the CFSSL CA api-server" here, mkEnableOption adds the "Whether to enable" thing.

infinisil · 2018-07-28T03:05:59Z

nixos/modules/services/security/cfssl.nix

+
+    port = mkOption {
+      default = 8888;
+      type = types.int;


You could use the newish types.ints.u16 to prevent users trying to assign an invalid port (have seen people do this)

infinisil · 2018-07-28T03:06:12Z

nixos/modules/services/security/cfssl.nix

+    dataDir = mkOption {
+      default = "/var/lib/cfssl";
+      type = types.str;
+      description = "cfssl work directory";


All descriptions should end with a colon.

infinisil · 2018-07-28T03:09:25Z

nixos/modules/services/security/cfssl.nix

+    };
+
+    ca = mkOption {
+      default = "file:${cfg.dataDir}/ca.pem";


I think doing it like this would make the module system rebuild the man pages and stuff when somebody changes cfg.dataDir. Using defaultText = "file:\${cfg.dataDir}/ca.pem", removing the default = , and setting services.cfssl.ca = "file:${cfg.dataDir}/ca.pem"` in the config section at the bottom should fix that.

Same with the caKey option.

infinisil · 2018-07-28T03:12:18Z

nixos/modules/services/security/cfssl.nix

+
+    configFile = mkOption {
+      default = null;
+      type = types.nullOr types.str; # FIXME: types.nonStorePath, may contain secrets


I think it's the user's responsibility to know when secrets would end up in the nix store. There are also a lot of users for which that isn't much of a problem anyways because they encrypt their nix store and are the only users.

A warning in the description would be appropriate though. Same for the other options that have a FIXME.

infinisil · 2018-07-28T03:14:25Z

nixos/modules/services/security/cfssl.nix

+
+    responderKey = mkOption {
+      default = null;
+      type = types.nullOr types.str; # FIXME: types.nonStorePath, secret


types.path, same for tlsKey and dbConfig.

infinisil · 2018-07-28T03:16:33Z

nixos/modules/services/security/cfssl.nix

+  };
+
+  config = {
+    users.extraGroups."cfssl" = {


Nit: cfssl doesn't need to be quoted, same with the one in users.extraUsers below.

infinisil · 2018-07-28T03:17:39Z

nixos/modules/services/security/cfssl.nix

+    systemd.services.cfssl = mkIf cfg.enable {
+      description = "CFSSL CA API server";
+      wantedBy = [ "multi-user.target" ];
+


Add after = [ "network.target" ]; here to prevent it from starting before the basic network stuff is working.

johanot · 2018-07-28T07:36:39Z

@infinisil Thanks for feedback! Will do the necessary changes Monday.

johanot · 2018-07-30T09:14:19Z

@infinisil please review: 16a3ed4 .. Keeping the commit separate for your convenience for now. Will squash if you approve the changes.

infinisil · 2018-07-30T18:47:24Z

nixos/modules/services/security/cfssl.nix

+            (opt "int-dir" intDir)
+            (opt "metadata" metadata)
+            (opt "remote" remote)
+            (opt "config" configFile)


I only just saw this, how does this config option interact with the other args? Because if it can provide all these values, then it would make sense to have a configFile option allowing you to pass your own config file (which would be passed via --config if it's set), and fall back to a generated config file from all the other options. Only the --config arg would need to be passed in both cases.

Edit: This also allows the user to not store secrets in the store, as they can provide their own config somewhere out of /nix/store.

@infinisil I hear what you say. But the config file does not have the same options as the cmdline options. Ref: https://github.com/cloudflare/cfssl/blob/80d5f5b4cfe73f5a918b851f03d468eb28499131/config/config.go#L74

Unfortunately it's not well documented, but the config file contains an optional list of different profiles for CA-signing. While some flags overlap - like AuthConfig (remotes) and CN-constraints - others do not. Like Bind Address (only cmdline) and Cert content (expirty, constraints etc.) (only configfile).

infinisil · 2018-08-02T20:42:08Z

@GrahamcOfBorg test cfssl

GrahamcOfBorg · 2018-08-02T20:42:41Z

Success on aarch64-linux

Attempted: tests.cfssl

No partial log is available.

GrahamcOfBorg · 2018-08-02T20:45:21Z

Success on x86_64-linux (full log)

Attempted: tests.cfssl

Partial log (click to expand)

machine: exit status 1
syncing
machine: running command: sync
machine: exit status 0
test script finished in 59.58s
cleaning up
killing machine (pid 597)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/3ai0zc5074np28x6gbhnb61kaklkj931-vm-test-run-cfssl

infinisil

LGTM thanks, feel free to squash!

johanot · 2018-08-02T22:25:47Z

@infinisil Thanks! Squash done.

infinisil · 2018-08-02T23:00:36Z

Oh wait, one minor thing: can you change the commit message to "nixos/cfssl: init"?

@srhb

- based on module originally written by @srhb - complies with available options in cfssl v1.3.2 - uid and gid 299 reserved in ids.nix - added simple nixos test case

johanot · 2018-08-03T07:41:43Z

Done :)

infinisil · 2018-08-03T14:39:19Z

Awesome, thanks :)

lopsided98 · 2018-08-21T20:18:55Z

The user creation is not guarded by the mkIf cfg.enable which is causing a cfssl user to be created on every NixOS user's system, as well as the /var/lib/cfssl directory.

kalbasit · 2018-08-21T20:21:12Z

@lopsided98 I confirm seen them on my system. Can you open a separate issue so we can track it and fix it in a separate PR? Thx!

srhb · 2018-08-21T20:21:37Z

On it.

lopsided98 · 2018-08-21T20:21:57Z

I'm writing a PR right now, the fix is pretty simple.

srhb · 2018-08-21T20:24:56Z

@lopsided98 Oh, okay.

lopsided98 · 2018-08-21T20:28:26Z

See #45441

infinisil · 2018-08-22T02:52:40Z

Whoops, totally missed that!

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jul 26, 2018

infinisil requested changes Jul 28, 2018

View reviewed changes

johanot force-pushed the nixos-cfssl branch from 16a3ed4 to df1f337 Compare July 30, 2018 18:41

infinisil reviewed Jul 30, 2018

View reviewed changes

johanot mentioned this pull request Aug 1, 2018

Kubernetes 1.10.5 is unusable, auth is not working #43395

Closed

infinisil approved these changes Aug 2, 2018

View reviewed changes

johanot force-pushed the nixos-cfssl branch from df1f337 to 90472d4 Compare August 2, 2018 22:15

nixos/cfssl: init

7d7c36f

- based on module originally written by @srhb - complies with available options in cfssl v1.3.2 - uid and gid 299 reserved in ids.nix - added simple nixos test case

johanot force-pushed the nixos-cfssl branch from 90472d4 to 7d7c36f Compare August 3, 2018 07:41

infinisil merged commit d31f89d into NixOS:master Aug 3, 2018

johanot deleted the nixos-cfssl branch August 3, 2018 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/cfssl: Add new module for cfssl #44127

nixos/cfssl: Add new module for cfssl #44127

johanot commented Jul 26, 2018

johanot commented Jul 26, 2018

srhb commented Jul 26, 2018

GrahamcOfBorg commented Jul 26, 2018

GrahamcOfBorg commented Jul 26, 2018

infinisil Jul 28, 2018

infinisil Jul 28, 2018

infinisil Jul 28, 2018

infinisil Jul 28, 2018

infinisil Jul 28, 2018

infinisil Jul 28, 2018

infinisil Jul 28, 2018

infinisil Jul 28, 2018

infinisil Jul 28, 2018

johanot commented Jul 28, 2018

johanot commented Jul 30, 2018

infinisil Jul 30, 2018 •

edited

Loading

johanot Jul 30, 2018

infinisil commented Aug 2, 2018

GrahamcOfBorg commented Aug 2, 2018

GrahamcOfBorg commented Aug 2, 2018

infinisil left a comment

johanot commented Aug 2, 2018

infinisil commented Aug 2, 2018

johanot commented Aug 3, 2018

infinisil commented Aug 3, 2018

lopsided98 commented Aug 21, 2018

kalbasit commented Aug 21, 2018

srhb commented Aug 21, 2018

lopsided98 commented Aug 21, 2018

srhb commented Aug 21, 2018

lopsided98 commented Aug 21, 2018

infinisil commented Aug 22, 2018

nixos/cfssl: Add new module for cfssl #44127

nixos/cfssl: Add new module for cfssl #44127

Conversation

johanot commented Jul 26, 2018

Things done

johanot commented Jul 26, 2018

srhb commented Jul 26, 2018

GrahamcOfBorg commented Jul 26, 2018

GrahamcOfBorg commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanot commented Jul 28, 2018

johanot commented Jul 30, 2018

infinisil Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented Aug 2, 2018

GrahamcOfBorg commented Aug 2, 2018

GrahamcOfBorg commented Aug 2, 2018

infinisil left a comment

Choose a reason for hiding this comment

johanot commented Aug 2, 2018

infinisil commented Aug 2, 2018

johanot commented Aug 3, 2018

infinisil commented Aug 3, 2018

lopsided98 commented Aug 21, 2018

kalbasit commented Aug 21, 2018

srhb commented Aug 21, 2018

lopsided98 commented Aug 21, 2018

srhb commented Aug 21, 2018

lopsided98 commented Aug 21, 2018

infinisil commented Aug 22, 2018

infinisil Jul 30, 2018 •

edited

Loading