#2912 Handle openid_connect_signing_key variable to enable backup/restore & restarts without errors

[ebarped]

This PR is based on #2631, solves #2912, and is related to #2356 and #2420

[ebarped]

@sachilles I had tested it both in docker-compose and in gitlab-rc.yml, and its working properly.

Also, the question about GITLAB_SECRETS_ENCRYPTED_SETTINGS_KEY_BASE that you asked in #2631 (comment)

Is it problematic if GITLAB_SECRETS_ENCRYPTED_SETTINGS_KEY_BASE is not set?

Has the same relevance here, but with the env var GITLAB_SECRETS_OPENID_CONNECT_SIGNING_KEY, and the answer is no.
If you set it, every time you restart/upgrade, you will have the same value set, and you will avoid errors. But if you dont set it, the behaviour of gitlab is the actual one.

(Nonetheless, i would recommend to set it to avoid problems...)

[ebarped]

Can this PR get some feedback? Thanks! @kkimurak @sameersbn

[kkimurak]

Thank you for your mention me. I don't have merge privileges or anything, but I wrote a comment. I hope you'll take a look and have a discuss with maintainer.

P.S.: @sameersbn is away from maintainer for a few years.

[kkimurak]

I think it is a good idea to give an example in documentation since the format is unique compared to other parameters.
However, I don't think you should give the actual values in the sample docker-compose.yml and so on. From some of the discussions, it seems that many people are using the configuration files in this repository as is (literally as is - without changing old postgresql images that are no longer supported to newer releases, without reconfiguring strange secrets strings long-and-random-alpha-numeric-string).

I feel that commonly known private keys are an easy target for cracking (I am just an electrical/electronic board designer, with no knowledge of OpenID, and not a security expert, so I may be writing strange things).

[ebarped]

Yes, i just googled rsa private key example and used the first one that i have found: https://phpseclib.com/docs/rsa-keys

I used a real key so a newcomer can try and test the project easily without making changes.

You are proposing that in the docker-compose.yml, instead of an actual key, we should use something like:

    - |-
      GITLAB_SECRETS_OPENID_CONNECT_SIGNING_KEY=|
          -----BEGIN RSA PRIVATE KEY-----
          AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
          AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
          AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
          AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
          AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
          AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
          AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
          -----END RSA PRIVATE KEY-----

in order to force people to change the value for a real key?

[kkimurak]

I'm not sure what is better way. I may have needlessly complicated things. Please ignore me.

---

If we really want to force the user to set the correct value, we could just make an error if the value is empty and prevent the container from starting. We do that for some other secrets. In addition, we may want to validate that the key is correct as an RSA key, for example by executing openssl rsa -in <openid signing key file> -check (Just saw a gist that says so, I have not checked it works.)

I personally don't like the idea - i.e., container won't start if no secrets are set.
It makes some sense that the container would not start if GITLAB_SECRETS_DB_KEY_BASE is not set since no user would use GitLab without a database, surely. However, it is possible to use GitLab without OpenID (and I don't actually use OpenID). Am I mistaken in this?
I believe that the PR of adding a feature should have as little impact as possible on users who don't use that feature.

On the other hand, I am a bit confused by my lack of knowledge when I look at the GitLab source code. It says "encrypted_settings_key_base is optional for now" and it generates secure token if empty, only when environment variable GITLAB_GENERATE_ENCRYPTED_SETTINGS_KEY_BASE is set.
What is optional? If something that allows GitLab to start without setting is optional, then openid_connect_signing_key should be too. However, looking at the code, it's not. I'm really not sure what is openid_connect_signing_key and how is it used for.

I am also wary that my personal assertions as written above will create many scenarios that we cannot cover. I haven't checked yet, but from your comment, changing the OpenID signing key after the fact would cause serious problems.
Therefore, we must avoid having this key randomly generated by not setting a value and then reset by restarting the container. An approach to facilitate this might be to cache the key generated by GitLab and restore. But there will be some complex condition like "only when the user does not set the variable" and so on.
So, it is much easier to force the user to set the value.

[ebarped]

I dont know if you are affected or not, we dont use openID either...

What we are experiencing is: when trying to restore a backup in our current version (16.4.4), we find that a lot of things are not working (errors 500 in webhooks, settings pages... etc)

The thing that is changing from our prod gitlab and the restored one is this openid_connect_signing_key key, that is generated by gitlab on startup.
If we set the production value of this key into the restored one, everything goes well, and the errors dissappear.

It seems that openid_connect_signing_key is a required value in a default gitlab installation, i dont know if other people are not suffering our problem, or they just dont do backups.

More info:

• https://docs.gitlab.com/charts/installation/secrets.html#gitlab-rails-secret
• Missing Rails.application.secrets.openid_connect_signing_key for production environment. The secret will be generated and stored in config/secrets.yml. #1627
• https://docs.gitlab.com/omnibus/maintenance/#rotate-the-secrets-file

It seems that its a new secret that is set by default in gitlab, so we should handle it.

[kkimurak]

@ebarped
Sorry for late.
I have tested to restore on my test environment (running sameersbn/gitlab:16.9.2 without this PR) but I could not reproduce the issue. I was able to access to admin page, repository page, admin settings page, repository settings page, webhook page without 500 errors. Of course I could clone the code or run a new CI.
There may be conditions that cause this problem to occur (features in use or not, for example).
I'd also like to see how backup/restore works on 16.4.4 (the version you're running).

Anyway, if your symptoms were cured by the contents of this PR, I have no reason to block this to be merged.
Some my frustration talking above can be ignored as they are not to be discussed here but on upstream (with more research).

[ebarped]

Hi again @kkimurak!

I managed to find what is causing us the trouble doing the restore: Gitlab for slack app (info).

If i remove all the config about this integration, i can sucessfully restore gitlab, regardless of the content of openid_connect_signing_key field.

It seems that, internally, gitlab uses openid_connect_signing_key to encrypt the information about this integration.

So, i dont know how to proceed from here...

1. Should we merge this PR and force everyone to set the openid_connect_signing_key field?
   • the config is a little bit more complicated (a new field to handle), but we avoid this (and probably other) errors
2. Should we let the people handle their particular config and use cases?
   • the config is easier and generic, but people may suffer from this error and not find the root cause

[ebarped]

So @kkimurak, what do you think that will be the correct approach here? Try to merge the PR or close it?

[kkimurak]

@ebarped Sorry for late. As I already stated, I have no major reason to block this PR to be merged.
If I had to comment, it would be whether to make the parameter "required". If there is any opinion from the maintainer, I will follow it.

[ebarped]

Thanks! What will be the following steps then? Just wait for @sameersbn to merge?

[kkimurak]

@ebarped Yes, but @sachilles has been the only active maintainer in recent years. Based on the usual trends, we should see a response around the release of gitlab (22th of each month) at the latest.

@sachilles
Could you be so kind to review it when you have time?
There may be room for improvement (e.g., to validate GITLAB_SECRETS_OPENID_CONNECT_SIGNING_KEY as an rsa key - this is more of an enhancement), but the current code works fine and is useful to those who need it.

We are at a loss to decide if this parameter should be mandatory or not.

[kkimurak]

@ebarped
P.S. CI seems to be failing because it reached to timeout (up to 1hour). It can be retriggered by re-push.

[ebarped]

CI is 🆗 now

[ebarped]

@sachilles If we dont merge this (or a similar PR), people wont be able to migrate from slack webhooks to gitlab for slack app (https://docs.gitlab.com/ee/user/project/integrations/slack.html)

[ebarped]

Hi! I have encountered some errors on our configuration, so maybe this PR is not necessary at the moment.

I will further investigate and bring back more info, but in the meantime stop the process to merge this PR please.

Thanks!