remove secrets from config yaml and terraform configuration files

[leej3]

Resolves #13.

Proposal for managing secrets... this approach:

• Makes use of the fact that terraform variables can be set by setting an environment variable with the same name but prefixed with TF_VAR_
• Could be used for all secrets/tokens.
• Makes it harder for secrets to leak as they are never stored as plain text.
• Does not expose secrets during a render (I need to confirm this, I may have to define the variable as sensitive in variables.tf).
• Can serve as an interim prior to more thorough managment of secrets using something like vault/keycloak.
• In some cases will require the removal of some values from the schema. e.g. client_id and client_key for google oauth.

[leej3]

Further thoughts on this with respect to breakage of schema compatibility. Currently the schema sets client_id/client_key for a number of different auth use-cases.

With the current approach we would have to remove these from the schema (breaking backwards compatibility) and in each case document the env variable that needs to be set in order to define the appropriate terraform variable.

We could alternatively use a convention whereby the current fields are filled with a placeholder value instead of the current key. For example:

security:
  authentication:
    type: custom
    authentication_class: "oauthenticator.google.GoogleOAuthenticator"
    config:
      oauth_callback_url: 'https://qhub.ouraes.com/hub/oauth_callback'
      client_id: QHUB_SECRET_google_oauth_client_id
      client_secret: QHUB_SECRET_google_oauth_client_secret
...

The above convention would trigger a search for the env variables google_oauth_client_id and google_oath_client_secret and would use them to set TF_VAR_google_oauth_client_id etc which then populates the corresponding terraform variables. This approach prevents schema breakage and could likely be backwards compatible with the current implementation of storing secrets (if we wish to continue to support that). It seems a hacky though...

For now I will continue to implement this with schema modification...

[leej3]

So @costrouc pointed out that it is not just .tf files that the secrets are exposed in during the render. Sometimes it is put in other files that won't work as cleanly as we had envisaged. So for example, the auth client_id and client_key are leaked via the jupyterhub config:

data$ grep -r oops_secret .
./qhub-config.yaml:      client_id: oops_secret
./qhub-config.yaml:      client_secret: oops_secret
./infrastructure/jupyterhub.yaml:      c.GitHubOAuthenticator.client_id = "oops_secret"
./infrastructure/jupyterhub.yaml:      c.GitHubOAuthenticator.client_secret = "oops_secret"

Until we implement something more systematic for secrets management it seems like avoiding rendering these values is too challenging and if security is the imperative on a CI provider based deployment the best option is to simply not render.

[aktech]

So @costrouc pointed out that it is not just .tf files that the secrets are exposed in during the render. Sometimes it is put in other files that won't work as cleanly as we had envisaged. So for example, the auth client_id and client_key are leaked via the jupyterhub config:

That's true, that those SECRETS are engrained in the files. My thought was to have placeholders in those files and and replace them in terraform during deployment via terraform variables, in here:
https://github.com/Quansight/qhub/blob/08c94b91428f68d47270ba003a7461c45ef64f48/qhub/template/%7B%7B%20cookiecutter.repo_directory%20%7D%7D/infrastructure/modules/kubernetes/services/meta/qhub/main.tf#L12-L13

[leej3]

This implements both approaches based on the two use-cases:

• The user wishes to remove secrets from the config yaml. This can be done for any field using the QHUB_SECRET_ pattern. These values will often continue to be rendered but if the user turns off rendering it goes a long way to protecting the values.
• Due to the way the Prefect integration is implemented the required secret can be provided to terraform through the TF_VAR_prefect_token environment variable. This variable is now explicitly checked for. Overall this is a nice pattern and should be mimicked for future integrations.

@aktech's suggested is a nice addition to this PR.. .namely, we implement a extra bit of logic to replace the QHUB_SECRET placeholder after rendering (so we don't leak secrets there) but before the relevant file is provided to terraform. This may have to be implemented on a case by case basis because the relevant values might be in any file... or perhaps we could search every file for the placeholder patterns...

[aktech]

I have mostly pointed out some nitpicks, overall looks good to me. I will take another look if you have time to address the comment here

I can then review get_secret_config_entries and it's helper functions more closely.

[leej3]

Sorry about the style issues. Fixed now. Tests for the get_secret_config_entries function are in tests/test_render.py or did you mean that I should add more?

[aktech]

Sorry about the style issues. Fixed now.

Awesome, thanks!

Tests for the get_secret_config_entries function are in tests/test_render.py or did you mean that I should add more?

Yep, I missed it apologies. Taking a look now.

[aktech]

This looks great now. One last thing, can you add some documentation, maybe here.

[aktech]

Looks great, thanks for working on it!