Decouple secret keys from each other

At the moment, the gitlab-rails secret_token is also used for encrypting OTP secrets in the DB. We can't fix this automatically without making people re-encrypt everything, or disable 2FA, but we can make this easier in future by making this explicit.

See https://gitlab.com/gitlab-org/gitlab-ce/issues/3963. This should not be merged until after https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5274.

@jacobvosmaer-gitlab can you take a look please?

What is going on here? :)

OK, an updated download link.

As discussed on slack, this Gitlab['gitlab_shell']['secret_token'] would be easier to distinguish if it were in a separate paragraph.

Considering that we are also cleaning this up on the gitlab side, I would propose we also:

• get rid of Gitlab['gitlab_ci']['secret_key_base'] => I don't see this being used anywhere
• put db_key_base under gitlab_rails, not under gitlab_ci
• rename Gitlab['gitlab_rails']['secret_token'] to secret_key_base (while still supporting secret_token).

In other words, I think we should do the same sort of cleanup here we did over in gitlab-ce.

See below how we only generate Gitlab['gitlab_ci']['secret_key_base'] but never use it on master.

% git grep secret_key_base
files/gitlab-cookbooks/gitlab/libraries/gitlab.rb:      Gitlab['gitlab_ci']['secret_key_base'] ||= if Gitlab['gitlab_ci']['secret_token']
files/gitlab-cookbooks/gitlab/libraries/helper.rb:                        'secret_key_base' => Gitlab['gitlab_ci']['secret_key_base'],
files/gitlab-cookbooks/gitlab/templates/default/secrets.yml.erb:  secret_key_base: <%= @db_key_base %>
% git log -1
0b4b7ca Merge branch 'update-openshift-gitlab-version' into 'master'

Maybe these changes should go into a doc/build/README.md. The above section actually just shows how to run/test the changes in the cookbook, not how to build a package.

secret_token.erb is being used in gitlab-shell recipe still so we cannot remove it.

Oops, good catch.

@jacobvosmaer-gitlab @smcgivern Please do not cleanup anything gitlab_ci related. This is scheduled for 9.0.

@marin @jacobvosmaer-gitlab I've made the changes above, with the caveat that I didn't touch gitlab-ci stuff, just gitlab-rails. Can you take another look please?

Added 3 commits:

• 71a3007d - Keep secret_token.erb for gitlab-shell
• e33bc914 - secret_token -> secret_key_base for gitlab-rails
• b19b7359 - Move local build instructions to build doc

@marin and I talked for about an hour just now just to try and make sense of all this. One thing I did not know is that only last week we lost all encrypted DB data on dev.gitlab.org after a restore because we misplaced the db_key_base secret.

Our conclusions are:

• this to risky for 8.10, should go in 8.11
• we need tests (probably https://github.com/sethvargo/chefspec)
• gitlab_rails['db_key_base'], which is set on dev.gitlab.org, is currently IGNORED by the internal cookbook
• gitlab_ci['secret_key_base'] is useless, we can remove it
• in current Omnibus installations, the following happens:
  • .secret contains 'a'
  • config/secrets.yml contains 'secret_key_base: b'
  • Gitlab::Application.secrets.secret_key_base == 'b'
  • Gitlab::Application.config.secret_key_base == 'a'
  • WAT

This demonstrates how gitlab_rails['db_key_base'] is ignored:

jacobvosmaer@debian8-test:~$ sudo grep db_key_base /etc/gitlab/gitlab.rb
gitlab_rails['db_key_base'] = "gitlab_rails['db_key_base']"
jacobvosmaer@debian8-test:~$ sudo grep -r db_key_base /var/opt/gitlab/gitlab-rails/etc
/var/opt/gitlab/gitlab-rails/etc/secrets.yml:  db_key_base: 004dcaef29647f0791f677dcddfa79e7f954159d62f064587e3a256c1ec8275d4afb2d2b050075f5a8d092db18999402180904a492eecfafb968db0030925dcd

cc @jnijhof

this to risky for 8.10, should go in 8.11

in current Omnibus installations, the following happens:

Isn't that because it sets secret_key_base: <%= @db_key_base %> in config/secrets.yml? We ignore this at the moment, because of the whole .secret dance in the initializer, so I don't believe that's going to be our biggest problem.

@smcgivern you may be right but I do know that last week's data loss was about db_key_base on omnibus. And it finely illustrates the madness we are dealing with here.

@jacobvosmaer-gitlab yes, sorry, that was just meant to be to narrowly focused to the point about secret_key_base. I take the point about not using db_key_base.

@smcgivern being narrowly focused on the problem is the reason we have this mess now Thanks for being brave enough to untangle this mess!

Touché @marin

For now, should I just start working through https://gitlab.com/gitlab-org/omnibus-gitlab/merge_requests/874#note_13173789? I'm not sure if it makes sense to add specs in this MR, or add specs in a separate MR to master and then pick this up again after.

@smcgivern Maybe finish up first the MR in CE you have been working on. After that it doesn't really matter if you do a separate MR for specs that gets merged first, your call.

@marin I think the CE MR also depends on this one, in that we don't want to be writing things in the Rails app in an omnibus installation. (Maybe I'm wrong about this?) With the current omnibus + my CE MR, the Rails app will try to write to config/secrets.yml and delete .secret from the Rails root.

Rails app won't be able to delete .secret nor write to config/secrets.yml in omnibus-gitlab because those files are read only for the git user and are owned by root. So you are not wrong about that. If CE MR is waiting for this then it makes sense to continue working here :)

We cannot merge the CE MR without doing something in omnibus. Recall, for instance, that omnibus sets secret_key_base in secrets.yml to a wrong value, and the only reason this works is that .secret is set to the right value by Omnibus, and the old initializer (that @smcgivern is now changing) prefers .secret to secrets.yml.

Issue for the db_key_base insanity: https://gitlab.com/gitlab-org/omnibus-gitlab/issues/1429

mentioned in issue #1429 (closed)

mentioned in merge request !905 (merged)

Closing in favour of !905 (merged) and !910 (merged).

Status changed to closed

mentioned in commit 6321963d