Skip to content

Add specs for secrets handling

username-removed-443319 requested to merge chefspec into master

Once !910 (merged) is merged into this, and this is merged into master, this closes #1387 (closed) and closes #1429 (closed). This goes with https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5274 and should be merged as close as possible to that.

What does this MR do?

Add (failing) specs to describe what we want to do when we load the secrets, shuffle them, generate missing ones, and save them to disk.

The problem

What we want to end up with

Key Used for Legacy names
gitlab_rails['db_key_base'] CI variables; import credentials gitlab_ci['db_key_base']; gitlab_rails['db_key_base'] (NOT USED)
gitlab_rails['otp_key_base'] OTP secrets gitlab_rails['secret_token']
gitlab_rails['secret_key_base'] Devise tokens gitlab_ci['db_key_base']; gitlab_rails['secret_token'] (NOT USED); gitlab_rails['db_key_base'] (NOT USED)

Previously, gitlab_rails['secret_token'] was stored in .secret, and other secrets in config/secrets.yml. We want to store all of these secrets in config/secrets.yml, under matching names in Omnibus and the Rails app.

Why aren't those names used?

The value of gitlab_rails['db_key_base'] will be set in config/secrets.yml if gitlab_ci['db_key_base'] is falsey, which is impossible as we generate a new one in that case.

Similarly, we write the value of gitlab_ci['db_key_base'] to config/secrets.yml under secret_key_base, and the value of gitlab_rails['secret_token] to .secret. The former value is used for Devise tokens, and the latter value is used for encrypting OTP secrets in the database - which is almost exactly the opposite of what you'd expect based on their names! (This is my understanding of trying to untangle this mess. Devise prefers the value in config/secrets.yml, and we explicitly use the value from .secret for OTP secrets.)

The solution

Changes to secrets file

The following secrets don't need to be written to /etc/gitlab/gitlab-secrets.json any more. We will continue to read and use them from these keys if needed, but will only write under the new names.

  • gitlab_rails['secret_token']
  • gitlab_ci['secret_token']
  • gitlab_ci['secret_key_base']
  • gitlab_ci['db_key_base']

We will add these keys:

  • gitlab_rails['db_key_base']
  • gitlab_rails['otp_key_base']
  • gitlab_rails['secret_key_base']

Changes to /etc/gitlab/gitlab.rb

None needed - these aren't in the template file, and we can't rewrite this file.

Changes to files written in Rails app directory

  • Don't write .secret, and delete it if it exists (?)
  • Make the key names in config/secrets.yml match everywhere

New configuration failure modes

  • gitlab_rails['db_key_base'] && gitlab_ci['db_key_base'] && gitlab_rails['db_key_base'] != gitlab_ci['db_key_base'] - the former isn't used now, and the latter won't be used after this refactor, so if they're both set, they should have the same value
  • gitlab_rails['otp_key_base'] && !gitlab_rails['secret_token] - if the legacy key isn't set, but the new one is, then it's possible the new key doesn't contain the right value (we still need a value under the old name for Devise anyway)
  • At some future release, we'll probably want to fail if values only exist under the old keys in /etc/gitlab/gitlab.rb, (for example, gitlab_ci['db_key_base'] but no gitlab_rails['db_key_base']) but that doesn't have to be any time soon

Doc updates

To avoid losing secrets, you must keep /etc/gitlab/gitlab.rb and /etc/gitlab/gitlab-secrets.json. We need to be very clear about this. (For source installations, only config/secrets.yml is needed.)

Precedence

Key Sources (highest priority first)
gitlab_rails['db_key_base'] /etc/gitlab/gitlab.rb; /etc/gitlab/gitlab-secrets.json
gitlab_rails['otp_key_base'] $SECRET_KEY_BASE (handled in Rails app); /etc/gitlab/gitlab.rb; /etc/gitlab/gitlab-secrets.json
gitlab_rails['secret_token'] $SECRET_KEY_BASE (handled in Rails app); /etc/gitlab/gitlab.rb; /etc/gitlab/gitlab-secrets.json

What do I need from you?

  1. Is the above wall of text correct?
  2. Are these the kind of specs we want?
  3. How do I fail in the failure cases listed above? The reason there aren't specs is that I don't know exactly what the failure would look like.
  4. Do we want to delete the Rails .secret file if it exists already? It shouldn't be needed after https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5274, but that will also ignore it if it doesn't need it.
  5. Do we need specs for the writing of config/secrets.yml? I'd prefer to remove the logic that's already there and have it be a straightforward serialisation of the gitlab_rails secrets hash, in which case a spec seems overkill.

Merge request reports