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

talyz · 2019-08-07T14:41:59Z

Motivation for this change

Improve the gitlab module in various ways:

Add support for storing secrets in files (even arbitrary secrets in extraConfig)
Split preStart into a privileged and unprivileged part (fixing issue systemd's PermissionsStartOnly is deprecated #53852 for the gitlab module)
Make initialization more robust
Fix missing ca_file for SMTP (upstream issue 790)

This does, as far as I can tell, what the unmaintained #31358 set out to do, and more. I takes a slightly different approach to the problem by mostly using jq to put secrets in place.

To split preStart I implemented support for this in the systemd module rather than hack it into the gitlab module so that other modules can benefit from it. This should solve issues where PermissionsStartOnly cannot be replaced with only tmpfiles.

Needs testing

Upgrading from an existing gitlab setup on 19.03 to this

Things done

Notify maintainers

cc @globin @etu @adisbladis @infinisil @lheckemann @nh2 @aanderse

adamtulinius · 2019-08-07T14:48:38Z

All the work related to systemd should probably be a separate pull request.

talyz · 2019-08-07T15:12:01Z

Hm, I was advised that it probably shouldn't, since the gitlab work depends on it, but maybe that's incorrect? I can of course split it into two separate PRs, but the gitlab PR would be broken until the systemd PR is merged, but maybe that's okay?

adamtulinius · 2019-08-07T15:28:43Z

Hm, I was advised that it probably shouldn't, since the gitlab work depends on it, but maybe that's incorrect?

I just figured it would be easier to review as two separate things, since it's probably not the same people responsible for reviewing the gitlab and systemd parts. Hopefully somebody more knowledgeable can chip in.

talyz · 2019-08-07T16:03:36Z

Yeah, I'm good with either, so if anyone feels strongly that it should be split, I'll split it.

aanderse

First - thank you for tackling this! The module could really use some love. I'm really glad you have taken this on as the end goal is fantastic. I have some opinions on how you're trying to achieve that goal. I hope we can have some productive discussion on the topic.

nixos/modules/services/misc/gitlab.nix

nixos/modules/system/boot/systemd-unit-options.nix

nixos/modules/services/misc/gitlab.nix

flokli · 2019-08-14T13:52:54Z

Thanks for all the great work done here :-)

Can you rebase test and rebase this on top of #66621?

talyz · 2019-08-20T06:59:29Z

@flokli Thanks! Sorry for the late reply - I've been on vacationing in Berlin for the last week, but I'll be going to cccamp now, so I'll probably have an opportunity to finish this up. I can see that the pr is merged, so I'll just rebase on master. In the meantime I'll push what I have at the moment and anyone interested can have a look. :)

talyz · 2019-08-20T11:38:56Z

Hm, just noticed that services.postgresql.ensureDatabases isn't in stable, so the backwards compatibility stuff I implemented is probably not necessary. I'll get rid of it.

nixos/modules/services/databases/postgresql.nix

nixos/modules/services/misc/gitlab.nix

talyz · 2019-08-26T12:42:46Z

@flokli I rebased on master yesterday and it seems to work well. I've tried just switching to it, and resetting by deleting the db and state, and can't find any obvious issues. :)

nixos/modules/services/misc/gitlab.nix

globin · 2019-09-01T10:21:03Z

nixos/modules/services/misc/gitlab.nix

+        message = "When services.gitlab.databaseHost is customized, services.gitlab.databasePasswordFile must be set!";
+      }
+      {
+        assertion = cfg.initialRootPasswordFile != null;


Why don't you just remove the default = null? Then you don't need to create the assertions, same for the assertions below.

I did this because I find the generic message you get when you use an undefined parameter less helpful, but if you want them gone, I can remove them.

aanderse

@talyz thanks for your work based on my comments. I'm approving the database parts of this change. I don't know much about gitlab on NixOS as I have never run the module (or looked at it much, before this PR) so I'll leave final approval to those more aware than myself.

Given the freeze coming up very quickly I would suggest @globin or @fpletz as the escalation point. @globin are you able to approve and merge this, or specify what changes are required to meet approval?

flokli · 2019-09-05T22:16:53Z

nixos/modules/services/misc/gitlab.nix

+         ];
+       } "_secret" -> { ".example[1].relevant.secret" = "/path/to/secret"; }
+  */
+  recursiveGetAttrWithJqPrefix = item: attr:


I think these two functions, plus the pattern to insert secrets are much more useful for nixos secret management in general, to be hidden in the gitlab module only. Thanks a lot for writing it ❤️

Could this be moved to nixos/lib/utils.nix, with maybe a line of docs in nixos/doc? I think other services can make use of that, too…

That being said, this should not block the PR before the feature freeze. If you don't find the time to do it in this PR, it can still be moved there at a later point.

No problem! :) I'll push the changes in a bit, but where in the docs would this fit?

@flokli I simplified the usage a bit, so that the pattern of using the functions together and outputting the JSON to a file for input is no longer necessary. Does it look okay?

Sorry, somehow I missed the notification. I guess it's fine for now to have it as multi-line comments. Thanks!

No worries. :) Okay, great! Yeah, I think it should be in the docs, but I couldn't find a good spot. Maybe there should be a Utility Functions subsection to the Functions reference in the nixpkgs manual?

Work around upstream issue NixOS#790 by explicitly referencing the ca-certificates.crt file.

Add support for storing secrets in files outside the nix store, since files in the nix store are world-readable and secrets therefore can't be stored safely there. The old string options are kept, since they can potentially be handy for testing purposes, but their descriptions now state that they shouldn't be used in production. The manual section is updated to use the file options rather than the string options and the tests now test both.

Use the postgresql module to provision a local db (if databaseCreateLocally is true) instead of doing this locally. Switch to using the local unix socket for db connections by default; this is needed since dbs created by the postgresql module only support peer authentication. Instead of running the rake tasks db:schema:load, db:migrate and db:seed_fu, run gitlab:db:configure, which in turn runs these tasks when needed. Solves issue NixOS#53852 for gitlab.

Introduce new functions which allows modules to define options where, if the input is an attrset and the output is JSON, the user can define arbitrary secrets.

Adds the ability to make any parameter specified in extraConfig secret by defining it an attrset containing the attr _secret, which in turn is a path to a file containing the actual secret.

srhb · 2019-09-08T11:29:00Z

This broke master eval; error: undefined variable 'genJqSecretsReplacementSnippet'' at /home/sarah/src/nixpkgs/nixos/lib/utils.nix:112:36

The set in which it is defined is not recursive.

srhb · 2019-09-08T11:30:17Z

Hmm, maybe it got lost in the merge?

talyz · 2019-09-08T13:23:14Z

@srhb Oops, yeah, that's my bad - I didn't rebase recently enough. Thanks for fixing it!

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Aug 7, 2019

aanderse requested changes Aug 7, 2019

View reviewed changes

flokli changed the title ~~nixos/gitlab nixos/systemd: Add support for fully priviliged scripts in systemd services, secure secrets in gitlab, and more~~ nixos/gitlab nixos/systemd: Add support for fully privileged scripts in systemd services, secure secrets in gitlab, and more Aug 14, 2019

talyz force-pushed the gitlab branch from 8838460 to 2709231 Compare August 20, 2019 07:01

talyz requested a review from thoughtpolice as a code owner August 20, 2019 07:01

talyz changed the title ~~nixos/gitlab nixos/systemd: Add support for fully privileged scripts in systemd services, secure secrets in gitlab, and more~~ nixos/gitlab nixos/postgresql: Add support for database extensions and owners in postgresql, secure secrets in gitlab, and more Aug 20, 2019

aanderse reviewed Aug 20, 2019

View reviewed changes

nixos/modules/services/databases/postgresql.nix Outdated Show resolved Hide resolved

talyz force-pushed the gitlab branch from 2709231 to f812e8f Compare August 25, 2019 16:42

aanderse reviewed Aug 25, 2019

View reviewed changes

nixos/modules/services/misc/gitlab.nix Outdated Show resolved Hide resolved

talyz force-pushed the gitlab branch from f812e8f to 35b22e4 Compare August 26, 2019 11:35

talyz force-pushed the gitlab branch 2 times, most recently from 0eef678 to 445f054 Compare August 31, 2019 16:59

talyz changed the title ~~nixos/gitlab nixos/postgresql: Add support for database extensions and owners in postgresql, secure secrets in gitlab, and more~~ nixos/gitlab: Add support for secure secrets and more Aug 31, 2019

aanderse requested changes Aug 31, 2019

View reviewed changes

nixos/modules/services/misc/gitlab.nix Outdated Show resolved Hide resolved

talyz force-pushed the gitlab branch from 445f054 to a718bb5 Compare September 1, 2019 09:28

globin requested changes Sep 1, 2019

View reviewed changes

aanderse approved these changes Sep 5, 2019

View reviewed changes

flokli reviewed Sep 5, 2019

View reviewed changes

nixos/gitlab: Fix missing ca_file for SMTP

7648b4f

Work around upstream issue NixOS#790 by explicitly referencing the ca-certificates.crt file.

talyz added 4 commits September 6, 2019 16:54

nixos/utils: Handle arbitrary secrets in JSON output files

64358cb

Introduce new functions which allows modules to define options where, if the input is an attrset and the output is JSON, the user can define arbitrary secrets.

nixos/gitlab: Extract arbitrary secrets from extraConfig

240649a

Adds the ability to make any parameter specified in extraConfig secret by defining it an attrset containing the attr _secret, which in turn is a path to a file containing the actual secret.

talyz force-pushed the gitlab branch from a718bb5 to 240649a Compare September 6, 2019 15:00

etu approved these changes Sep 7, 2019

View reviewed changes

flokli merged commit 2f3b9cd into NixOS:master Sep 7, 2019

talyz deleted the gitlab branch September 8, 2019 10:53

srhb mentioned this pull request Sep 8, 2019

nixos/lib/utils: Make the set recursive again, unbreak eval #68308

Merged

aanderse mentioned this pull request Sep 10, 2019

nixos/mysql: add authentication option #68353

Closed

10 tasks

schneefux mentioned this pull request Sep 13, 2019

gitlab module permission denied after upgrade #68696

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

talyz commented Aug 7, 2019

adamtulinius commented Aug 7, 2019

talyz commented Aug 7, 2019

adamtulinius commented Aug 7, 2019

talyz commented Aug 7, 2019

aanderse left a comment

flokli commented Aug 14, 2019

talyz commented Aug 20, 2019

talyz commented Aug 20, 2019 •

edited

Loading

talyz commented Aug 26, 2019

globin Sep 1, 2019

talyz Sep 1, 2019 •

edited

Loading

aanderse left a comment

flokli Sep 5, 2019

flokli Sep 5, 2019

talyz Sep 6, 2019

talyz Sep 6, 2019

flokli Sep 7, 2019

talyz Sep 8, 2019

srhb commented Sep 8, 2019 •

edited

Loading

srhb commented Sep 8, 2019

talyz commented Sep 8, 2019

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

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

Conversation

talyz commented Aug 7, 2019

Motivation for this change

Needs testing

Things done

Notify maintainers

adamtulinius commented Aug 7, 2019

talyz commented Aug 7, 2019

adamtulinius commented Aug 7, 2019

talyz commented Aug 7, 2019

aanderse left a comment

Choose a reason for hiding this comment

flokli commented Aug 14, 2019

talyz commented Aug 20, 2019

talyz commented Aug 20, 2019 • edited Loading

talyz commented Aug 26, 2019

globin Sep 1, 2019

Choose a reason for hiding this comment

talyz Sep 1, 2019 • edited Loading

Choose a reason for hiding this comment

aanderse left a comment

Choose a reason for hiding this comment

flokli Sep 5, 2019

Choose a reason for hiding this comment

flokli Sep 5, 2019

Choose a reason for hiding this comment

talyz Sep 6, 2019

Choose a reason for hiding this comment

talyz Sep 6, 2019

Choose a reason for hiding this comment

flokli Sep 7, 2019

Choose a reason for hiding this comment

talyz Sep 8, 2019

Choose a reason for hiding this comment

srhb commented Sep 8, 2019 • edited Loading

srhb commented Sep 8, 2019

talyz commented Sep 8, 2019

talyz commented Aug 20, 2019 •

edited

Loading

talyz Sep 1, 2019 •

edited

Loading

srhb commented Sep 8, 2019 •

edited

Loading