Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/step-ca: Adding module for step-ca #97225

Closed
wants to merge 1 commit into from
Closed

nixos/step-ca: Adding module for step-ca #97225

wants to merge 1 commit into from

Conversation

Church-
Copy link

@Church- Church- commented Sep 5, 2020

nixos: Adding module for step-ca, set up by default to allow for both regular x.509 certs and SSH certs as well.

Motivation for this change

There wasn't already a module, and having an easily set up root CA is something I've found to be fairly useful.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Church-
Copy link
Author

Church- commented Sep 5, 2020

Planning to add documentation later today.

Copy link
Member

@midchildan midchildan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I'd love to see this merged, and have made some suggestions to help improve this module.

description = "Port to listen on.";
};
passwordFile = lib.mkOption {
type = lib.types.str;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lib.types.path is preferred for filesystem paths. https://nixos.org/manual/nixos/stable/index.html#sec-option-types-basic

address = lib.mkOption {
type = lib.types.str;
default = "";
description = "Address to listen at, defaults to all interfaces.";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example should be added here to indicate the expected format:

example = "localhost";

};
name = lib.mkOption {
type = lib.types.str;
default = "SmallStep CA";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nitpick, but I think the "CA" part should be left out of the name because the resulting certificates would be named "Smallstep CA Root CA" and "Smallstep CA intermediate CA" otherwise.

description =
"Path to file containing password of your first provisioner.";
};
user = lib.mkOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be named provisioner as it would confuse people in two ways:

  1. The official docs refers to the option as "provisioner"
  2. The user option is commonly used throughout NixOS to refer to UNIX users

user = lib.mkOption {
type = lib.types.str;
default = "admin";
description = "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description should be non-empty.

Suggested change
description = "";
description = "The name of the first provisioner.";

'';
serviceConfig = {
Type = "simple";
ExecReload = "/run/current-system/sw/bin/kill -HUP $MAINPID";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifying the exact package is preferred instead of relying on a global path.

Suggested change
ExecReload = "/run/current-system/sw/bin/kill -HUP $MAINPID";
ExecReload = "${pkgs.coreutils}/bin/kill -HUP $MAINPID";

description = "Address to listen at, defaults to all interfaces.";
};
port = lib.mkOption {
type = lib.types.int;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

};
user = lib.mkOption {
type = lib.types.str;
default = "admin";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nit, but I think we should stick to the official example as the default. https://smallstep.com/docs/step-ca/getting-started

Suggested change
default = "admin";
default = "you@smallstep.com";

};
port = lib.mkOption {
type = lib.types.int;
default = 9075;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should stick to the official example here too. https://smallstep.com/docs/step-ca/getting-started

Suggested change
default = 9075;
default = 8443;

serviceConfig = {
Type = "simple";
ExecReload = "/run/current-system/sw/bin/kill -HUP $MAINPID";
PIDFile = "step-ca.pid";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another nit, but this should be unnecessary for Type = simple services. Users can explicitly enable this by manually specifying systemd.services.step-ca.serviceConfig.PIDFile in their NixOS configuration instead.

@Church-
Copy link
Author

Church- commented Nov 21, 2020

@midchildan All good points thanks! Planning to spend a bit of time fixing these nits up tomorrow, just been swamped at work.

@mohe2015
Copy link
Contributor

mohe2015 commented Jan 24, 2021

@Church- don't you also need to add:

diff --git a/nixos/modules/module-list.nix b/nixos/modules/module-list.nix
index 1ccfba68453..ff8798c133e 100644
--- a/nixos/modules/module-list.nix
+++ b/nixos/modules/module-list.nix
@@ -836,6 +836,7 @@
   ./services/security/shibboleth-sp.nix
   ./services/security/sks.nix
   ./services/security/sshguard.nix
+  ./services/security/step-ca.nix
   ./services/security/tor.nix
   ./services/security/torify.nix
   ./services/security/torsocks.nix

(Also adressing the feedback would probably be nice).
If you find the time it would be great if you could add options for predefined CA keys. (See https://smallstep.com/docs/step-cli/reference/ca/init)

@jlesquembre
Copy link
Member

Since #112322 was merged, can we close this one?

@midchildan
Copy link
Member

I believe so.

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

Successfully merging this pull request may close these issues.

None yet

5 participants