nixos/gitlab: Store secrets in files rather than the store #31358

srhb · 2017-11-07T13:34:54Z

Motivation for this change

The current gitlab module stores all secrets in the store, rendering them world readable. This PR converts databasePassword and secrets.* to types.path and ensures that preStart substitutes them into place. The NixOS test has also been altered to write dummy secrets to the store (but it still times out in qemu, which I've not been able to fix.)

There are some things I would like to improve:

Better error on the change. Right now, I'm relying on types.path solely, which could conceivably fail.
The database password is still emitted in the journal upon creation. This should be safe since normal users cannot read it, though
The substitution feels brittle. I am not a sed wizard.

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
- 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/)
Fits CONTRIBUTING.md.

globin · 2017-11-07T14:10:56Z

This at least needs an entry in the release-notes, also cc @fpletz

lheckemann · 2017-11-07T21:03:15Z

nixos/modules/services/misc/gitlab.nix

+        SECRETKEY=$(cat ${cfg.secrets.secret})
+        OTPKEY=$(cat ${cfg.secrets.otp})
+        DBKEY=$(cat ${cfg.secrets.db})
+        JWSKEY=$(${pkgs.jq}/bin/jq -Rs . ${cfg.secrets.jws} | sed -e 's,\\n,\\\\n,g')


What about other escape codes, should they exist in the string? I don't really know if they will, but it's really not obvious, at least to me. To be fair I'm not really sure what the JWS key is :)

The JWS key is actually the only one that's reasonably safe to assume doesn't contain a comma ( :( ), because it's base64 encoded...

lheckemann · 2017-11-07T21:05:07Z

nixos/modules/services/misc/gitlab.nix

        ln -fs ${pkgs.writeText "unicorn.rb" unicornConfig} ${cfg.statePath}/config/unicorn.rb

        chown -R ${cfg.user}:${cfg.group} ${cfg.statePath}/
        chmod -R ug+rwX,o-rwx+X ${cfg.statePath}/

+        DBPASSWORD=$(cat ${cfg.databasePassword})


Should probably be quoted to prevent whitespace in the password from breaking.

Actually never mind, this seems to be fine.

This quoting is needed for databasePassword to tolerate whitespace: DBPASSWORD=$(cat "${cfg.databasePassword}"). The "outer" quoting is unneeded, as mentioned, although it looks a bit scary :-)

Although while putting spaces in passwords is a sensible thing to do IMHO, anyone who's putting a space in the path to their password should reevaluate their life choices. Again IMHO ;)

This at least can be trivially fixed, and I'll do so. Thanks!

lheckemann · 2017-11-07T21:09:51Z

nixos/modules/services/misc/gitlab.nix

+        sed -e "s,#secretkeyplaceholder#,$SECRETKEY,g"   -i ${cfg.statePath}/config/secrets.yml
+        sed -e "s,#otpkeyplaceholder#,$OTPKEY,g"         -i ${cfg.statePath}/config/secrets.yml
+        sed -e "s,#dbkeyplaceholder#,$DBKEY,g"           -i ${cfg.statePath}/config/secrets.yml
+        sed -e "s,#jwskeyplaceholder#,$JWSKEY,g"         -i ${cfg.statePath}/config/secrets.yml


All these substitutions will break if any of the keys/passwords contain a comma, right? Perhaps use ${VAR//,/\\,/} rather than $VAR in each case.

If this seems like an acceptable fix to the substitution issue, I'll do it. I feel like the entire sed approach is wrong, but I don't know of any other reasonable alternatives. :/

lheckemann · 2017-11-07T21:16:59Z

nixos/modules/services/misc/gitlab.nix

        # Install the shell required to push repositories
        ln -fs ${pkgs.writeText "config.yml" gitlabShellYml} "$GITLAB_SHELL_CONFIG_PATH"
        ln -fs ${cfg.packages.gitlab-shell}/hooks "$GITLAB_SHELL_HOOKS_PATH"
        ${cfg.packages.gitlab-shell}/bin/install

        if [ "${cfg.databaseHost}" = "127.0.0.1" ]; then
          if ! test -e "${cfg.statePath}/db-created"; then
-            ${pkgs.sudo}/bin/sudo -u ${pgSuperUser} psql postgres -c "CREATE ROLE ${cfg.databaseUsername} WITH LOGIN NOCREATEDB NOCREATEROLE ENCRYPTED PASSWORD '${cfg.databasePassword}'"
+            ${pkgs.sudo}/bin/sudo -u ${pgSuperUser} psql postgres -c "CREATE ROLE ${cfg.databaseUsername} WITH LOGIN NOCREATEDB NOCREATEROLE ENCRYPTED PASSWORD '$DBPASSWORD'"


This would break with a password containing a quote, right? Similarly to before, I'd suggest ${DBPASSWORD//'/\\'/}… Although I'm guessing that would still go wrong in other ways 😢 (I wouldn't be surprised if postgres interprets other escape sequences too.) Oh, the joys of substituting text into other languages.

Also, shouldn't bash refrain from variable substitutions in single quotes anyway?

Uh, yes. I must have been fooled by the peer authentication.

Oh, no, this is fine.

$ foo=test; echo "This is fine: '$foo'" This is fine: 'test'

Hm, weird. Without the outer quotes:

foo=test; echo This is fine: '$foo' This is fine: $foo

Ah, I guess that has the same problem that @dhess pointed out.

Yes, because it would appear in ps, just for echo rather than psql. You could also remove the -, since psql won't care about the leading whitespace AFAIK.

Another alternative is psql postgres <<< "CREATE ROLE ${cfg.databaseUsername} WITH LOGIN NOCREATEDB NOCREATEROLE ENCRYPTED PASSWORD '$DBPASSWORD'"

How about 4c9744b ?

Ma27 · 2017-11-08T12:00:45Z

nixos/modules/services/misc/gitlab.nix

@@ -247,8 +247,7 @@ in {
      };

      databasePassword = mkOption {
-        type = types.str;
-        default = "";
+        type = types.path;


aren't paths copied into the nix store as well? I stumbled upon this in #29298

As far as I understand, neither the value of type str or path is copied directly to the store. The problem is that the secretsYml file was. Instead, that file (which is still copied to the store) is merely a template which we copy out substitute into at runtime from files whose path are specified here.

Actually, I suppose the configuration is stored, but it doesn't matter, since the path, not the contents is what's made world-readable. At best you can now see where the secret is stored, but it should still be secure (if permissions are correct.)

@srhb good work on making NixOS more secure! With regards to password options having type path see this comment by @domenkozar: #24288 (comment).

srhb · 2017-11-08T18:23:11Z

@lheckemann pointed out that bash is pretty good at substitutions, so let's try that instead. >_>

The only outstanding issue is really the createrole where single quotes will cause issues.

domenkozar · 2017-11-15T17:08:34Z

nixos/modules/services/misc/gitlab.nix

        ln -fs ${pkgs.writeText "unicorn.rb" unicornConfig} ${cfg.statePath}/config/unicorn.rb

        chown -R ${cfg.user}:${cfg.group} ${cfg.statePath}/
        chmod -R ug+rwX,o-rwx+X ${cfg.statePath}/

+        cat > ${cfg.statePath}/config/secrets.yml << EOF
+        production:
+          secret_key_base: $(cat "${cfg.secrets.secret}")


Does this really prevent store path being created? I think you should do:

nix-repl> "${toString ./timeout.txt}" "/home/ielectric/timeout.txt"

instead of

nix-repl> "${./timeout.txt}" "/nix/store/qvggx3asvcz26r9wpclpy67f0k5zryg6-timeout.txt"

See https://stackoverflow.com/a/43850372/133235 as a reference

I'm sorry, I don't understand exactly where you think it's going wrong. Can you point me at the exact spot? Here's an example of the rendered preStart-script:

cat > /var/gitlab/state/config/secrets.yml << EOF production: secret_key_base: $(cat "/run/keys/secret") otp_key_base: $(cat "/run/keys/otp") db_key_base: $(cat "/run/keys/db") jws_private_key: $(/nix/store/dc4f9mfgbwb0wygwk7qw5b0s0mv7kcml-jq-1.5/bin/jq -Rs . "/run/keys/jws") EOF

This is with services.gitlab.secrets.db set to "/run/keys/db" and so on.

The problem is, someone can specify it as /run/keys/db instead of "/run/keys/db" and it would get copied to the store if you don't call toString first.

Good point! That's actually a fair argument against using path at all...

0ba5f49 incorporates your suggestion, thank you. :)

We could come up with a type like nonStorePath meaning it would validate the value is not in store but is a path.

I think that would actually be pretty handy!

@domenkozar Do you still want nonStorePath for this PR?

dhess · 2017-12-02T02:47:31Z

I have one other request for this patch (and thank you for the work!) -- can you add after and wants dependencies on "keys.target", so that those of us who use NixOps can safely deploy these secrets via deployment.keys?

It's rare for a NixOS service to do this, but there is precedent: c.f.,

nixpkgs/nixos/modules/services/mail/dovecot.nix

Line 340 in 1741042

after = [ "keys.target" "network.target" ];

srhb · 2017-12-05T08:14:55Z

@dhess Like so?

dhess · 2017-12-05T09:07:22Z

Perfect, thank you!

srhb · 2018-01-05T21:58:58Z

I believe this looks good after I've fix the merge conflicts.

@domenkozar Are you satisfied with the changes?

lheckemann · 2018-04-19T12:02:14Z

Bump? Would be nice to have this merged.

arianvp · 2018-08-06T09:01:21Z

Can we get this merged? :)

andir · 2018-10-06T00:27:09Z

@srhb @domenkozar What has to be done to get this (rebased) and merged? It looks pretty good.

Is the lack of a nonStorePath a blocker?

srhb · 2018-10-06T06:35:12Z

I no longer use gitlab, so I'm not in a position to test, but I can do a blind rebase and someone else can take over from there.

infinisil · 2018-10-13T18:03:27Z

nixos/modules/services/misc/gitlab.nix

@@ -368,7 +350,7 @@ in {
      };

      secrets.secret = mkOption {
-        type = types.str;
+        type = types.path;


One thing I don't like is how this breaks everybody's config with a mediocre error message (type mismatch). Alternatives are:

Introduce {secret,db,...}File options for path based passwords. I don't like this because it introduces more options.

Introduce a type types.changed types.str types.path for such situations, maybe even types.changed "<reason, and remedy>" types.str types.path. I quite like this one.

I don't consider this a blocker for this PR though.

@infinisil, in nextcloud you have the adminPass and adminPassFile options next to each other. I think that's quite an ok solution.

As per infinisil's suggestion

Suggested change

type = types.path;

type = types.changed "The secret is now stored in a file, rather than plain text, to avoid it being world-visible." types.str types.path;

flokli · 2018-11-03T11:34:07Z

@globin @fpletz Could you take over?

globin · 2018-11-03T16:18:21Z

Could you make this work with either a file or the previous in-store-secrets to not break gitlab for anyone currently using it?

turion · 2018-11-11T11:02:57Z

Could you make this work with either a file or the previous in-store-secrets to not break gitlab for anyone currently using it?

What currently breaks?

turion · 2018-11-11T11:07:51Z

I'd like to work on this as I want to host my own gitlab instance, but I'm not very good at nix still.

One other problem with having the secret in the config is that you can't upload your config anywhere without "cleaning" it first. It would be great if there was a mkSecretOption or so that would automatically correspond to a path with the correct permissions where the secret is generated and stored.

globin · 2019-10-22T11:07:21Z

This has been implemented

lheckemann reviewed Nov 7, 2017

View reviewed changes

Ma27 reviewed Nov 8, 2017

View reviewed changes

fpletz added this to the 18.03 milestone Nov 9, 2017

fpletz added 1.severity: security 8.has: module (update) 9.needs: changelog labels Nov 9, 2017

domenkozar requested changes Nov 15, 2017

View reviewed changes

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Nov 15, 2017

basvandijk mentioned this pull request Nov 28, 2017

Provide options for storing secrets outside the Nix store #24288

Open

75 tasks

srhb force-pushed the gitlab-secure-wip branch 2 times, most recently from a9f9e26 to 2c79f06 Compare January 8, 2018 14:39

matthewbauer modified the milestones: 18.03, 18.09 Oct 1, 2018

samueldr modified the milestones: 18.09, 19.03 Oct 6, 2018

nixos/gitlab: Store secrets in files rather than the store

f2fb706

srhb force-pushed the gitlab-secure-wip branch from 2c79f06 to f2fb706 Compare October 6, 2018 06:43

GrahamcOfBorg added 6.topic: nixos 8.has: changelog 8.has: documentation labels Oct 6, 2018

infinisil reviewed Oct 13, 2018

View reviewed changes

FRidh modified the milestones: 19.03, 19.09 Mar 3, 2019

talyz mentioned this pull request Aug 7, 2019

nixos/gitlab: Add support for secure secrets and more #66274

Merged

10 tasks

globin closed this Oct 22, 2019

	type = types.path;
	type = types.changed "The secret is now stored in a file, rather than plain text, to avoid it being world-visible." types.str types.path;

nixos/gitlab: Store secrets in files rather than the store #31358

nixos/gitlab: Store secrets in files rather than the store #31358

Conversation

srhb commented Nov 7, 2017 • edited

Motivation for this change

Things done

globin commented Nov 7, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srhb commented Nov 8, 2017

domenkozar Nov 15, 2017 • edited

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

dhess commented Dec 2, 2017

srhb commented Dec 5, 2017

dhess commented Dec 5, 2017

srhb commented Jan 5, 2018

lheckemann commented Apr 19, 2018

arianvp commented Aug 6, 2018

andir commented Oct 6, 2018

srhb commented Oct 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flokli commented Nov 3, 2018

globin commented Nov 3, 2018

turion commented Nov 11, 2018

turion commented Nov 11, 2018

globin commented Oct 22, 2019

srhb commented Nov 7, 2017 •

edited

domenkozar Nov 15, 2017 •

edited