vault: 0.6.5 -> 0.7.2 with service #26130

ghost · 2017-05-26T16:14:25Z

Motivation for this change

Updating vault and a service to run it !

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
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/)
Fits CONTRIBUTING.md.

mention-bot · 2017-05-26T16:14:29Z

@katyucha, thanks for your PR! By analyzing the history of the files in this pull request, we identified @pradeepchhetri, @edolstra and @offlinehacker to be potential reviewers.

lheckemann · 2017-05-26T16:30:57Z

nixos/modules/services/security/vault.nix

+
+    ${if cfg.telemetry.extraConfig != "" then "
+      telemetry { 
+        ${if cfg.telemetry.disable_hostname then "disable_hostname = \"true\"" else ""}


telemetry.disable_hostname only takes effect if telemetry.extraConfig is set?

No need of this parameter if you doesn't set telemetry destination in extraConfig like a statsd, circonus or statsite...

I see, so this is self-hosted telemetry and not external. Thanks for the clarification!

cstrahan

Overall, this looks great! Just pointed out some changes that I think would improve things a bit.

cstrahan · 2017-05-26T20:15:03Z

nixos/modules/services/security/vault.nix

+        };
+
+        tls_disable = mkOption {
+          type = types.str;


This should use types.bool.

cstrahan · 2017-05-26T20:16:21Z

nixos/modules/services/security/vault.nix

+        };
+
+        tls_min_version = mkOption {
+          type = types.str;


This should use something like types.enum [ "tls10" "tls11" "tls12" ]

cstrahan · 2017-05-26T20:19:04Z

nixos/modules/services/security/vault.nix

+        };
+
+        cluster_address = mkOption {
+          type = types.str;


Rather than using "" to denote an unset cluster_address, I'd recommend making this types.nullOr types.str, with default = null.

all right, I'll write that for others string else " ''

cstrahan · 2017-05-26T20:21:22Z

nixos/modules/services/security/vault.nix

+        };
+
+        tls_prefer_server_cipher_suites = mkOption {
+          type = types.str;


This could be something like types.nullOr (types.enum [ "tls10" "tls11" "tls12" ]).

Hi. I note for TLS but for Cipher, it's a big list you can see here: https://golang.org/src/crypto/tls/cipher_suites.go . Is types.enum a little bit crazy for this case ?

cstrahan · 2017-05-26T20:22:21Z

nixos/modules/services/security/vault.nix

+        };
+
+        tls_require_and_verify_client_cert = mkOption {
+          type = types.str;


This should be types.bool.

ghost

updated

joelthompson · 2017-05-30T15:11:46Z

Hi @katyucha -- this looks cool!

Is there a reason you're bumping Vault to 0.7.1 and not 0.7.2, which fixes a bug with the audit logging?

fpletz · 2017-05-31T00:19:19Z

nixos/modules/services/security/vault.nix

+
+      ${if cfg.listener.tls_prefer_server_cipher_suites  then "tls_prefer_server_cipher_suites = \"true\"" else "tls_prefer_server_cipher_suites = \"false\"" } 
+
+      ${if cfg.listener.tls_require_and_verify_client_cert then "tls_require_and_verify_client_cert = \"true\"" else "tls_require_and_verify_client_cert = \"false\"" } 


You could use:

tls_require_and_verify_client_cert = "${boolToString cfg.listener.tls_require_and_verify_client_cert}";

or even abstract it some more and use it on more places:

let boolOption = name: opt: "${name} = \"${boolToString opt}\""; in '' ${boolOption "tls_prefer_server_cipher_suites" cfg.listener.tls_require_and_verify_client_cert} '';

So you don't have to repeat yourself. Same with the string options. :)

ghost

Updated !

joachifm · 2017-06-17T14:33:21Z

Is this ready to integrate?

joelthompson · 2017-06-27T13:48:37Z

A question to security experts: should the daemon run as root (as now) or a vault user should be created for it ?

HashiCorp recommends NOT running as root:

Don't run as root. Vault is designed to run as an unprivileged user, and there is no reason to run Vault with root or Administrator privileges. Running Vault as a regular user reduces its privilege. Configuration files for Vault should have permissions set to restrict access to only the Vault user.

However, if you do run as non-root, you either need to disable_mlock in the config or set the ipc_lock capability. See the disable_mlock documentation

joachifm · 2017-06-27T14:18:44Z

Some variation of serviceConfig.AmbientCapabilities = cap_ipc_lock perhaps.

ghost · 2017-06-29T20:00:03Z

Volt fork my MR ... no need of this now

lheckemann reviewed May 26, 2017

View reviewed changes

cstrahan suggested changes May 26, 2017

View reviewed changes

ghost changed the title ~~vault: 0.6.5 -> 0.7.1 with service~~ WIP: vault: 0.6.5 -> 0.7.1 with service May 27, 2017

ghost changed the title ~~WIP: vault: 0.6.5 -> 0.7.1 with service~~ vault: 0.6.5 -> 0.7.1 with service May 28, 2017

pSub added 8.has: package (update) 6.topic: nixos labels May 28, 2017

ghost commented May 29, 2017

View reviewed changes

fpletz reviewed May 31, 2017

View reviewed changes

ghost changed the title ~~vault: 0.6.5 -> 0.7.1 with service~~ WIP : vault: 0.6.5 -> 0.7.1 with service May 31, 2017

ghost changed the title ~~WIP : vault: 0.6.5 -> 0.7.1 with service~~ vault: 0.6.5 -> 0.7.2 with service Jun 1, 2017

ghost commented Jun 1, 2017

View reviewed changes

ghost changed the title ~~vault: 0.6.5 -> 0.7.2 with service~~ WIP: vault: 0.6.5 -> 0.7.2 with service Jun 6, 2017

Vault: 0.6.5 -> 0.7.2 with services

cbe36da

ghost changed the title ~~WIP: vault: 0.6.5 -> 0.7.2 with service~~ vault: 0.6.5 -> 0.7.2 with service Jun 7, 2017

delete lines

Verified

This commit was signed with the committer’s verified signature.

globin Robin Gloster

GPG key ID: 5E4C836C632C2882

Verified
Learn about vigilant mode

3f693e5

ghost closed this Jun 29, 2017

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vault: 0.6.5 -> 0.7.2 with service #26130

vault: 0.6.5 -> 0.7.2 with service #26130

ghost commented May 26, 2017

mention-bot commented May 26, 2017

lheckemann May 26, 2017

ghost May 26, 2017

lheckemann May 26, 2017

cstrahan left a comment

cstrahan May 26, 2017

ghost May 27, 2017

cstrahan May 26, 2017

ghost May 27, 2017

cstrahan May 26, 2017

ghost May 27, 2017

cstrahan May 26, 2017

ghost May 27, 2017

cstrahan May 26, 2017

ghost May 27, 2017

ghost left a comment

joelthompson commented May 30, 2017

fpletz May 31, 2017

ghost left a comment

joachifm commented Jun 17, 2017

joelthompson commented Jun 27, 2017

joachifm commented Jun 27, 2017

ghost commented Jun 29, 2017


		${if cfg.listener.tls_prefer_server_cipher_suites then "tls_prefer_server_cipher_suites = \"true\"" else "tls_prefer_server_cipher_suites = \"false\"" }

		${if cfg.listener.tls_require_and_verify_client_cert then "tls_require_and_verify_client_cert = \"true\"" else "tls_require_and_verify_client_cert = \"false\"" }

vault: 0.6.5 -> 0.7.2 with service #26130

vault: 0.6.5 -> 0.7.2 with service #26130

Conversation

ghost commented May 26, 2017

Motivation for this change

Things done

mention-bot commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstrahan left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

joelthompson commented May 30, 2017

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

joachifm commented Jun 17, 2017

joelthompson commented Jun 27, 2017

joachifm commented Jun 27, 2017

ghost commented Jun 29, 2017