From ceb902329cbabdf344dc89fc661abc105b8638bc Mon Sep 17 00:00:00 2001 From: leej3 Date: Mon, 5 Jul 2021 18:08:41 +0100 Subject: [PATCH 01/12] use env var for prefect secret --- qhub/deploy.py | 33 +++++++++++++++++++ .../kubernetes/services/prefect/variables.tf | 1 - 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/qhub/deploy.py b/qhub/deploy.py index 3394c488af..05daeccd03 100644 --- a/qhub/deploy.py +++ b/qhub/deploy.py @@ -1,4 +1,5 @@ import logging +import os import re from subprocess import CalledProcessError @@ -42,6 +43,9 @@ def guided_install( ): # 01 Check Environment Variables check_cloud_credentials(config) + # Check that secrets required for terraform + # variables are set as required + check_secrets(config) # 02 Create terraform backend remote state bucket # backwards compatible with `qhub-config.yaml` which @@ -127,3 +131,32 @@ def add_clearml_dns(zone_name, record_name, record_type, ip_or_hostname): for dns_record in dns_records: update_record(zone_name, dns_record, record_type, ip_or_hostname) + + +def check_secrets(config): + """ + Checks that the appropriate variables are set based on the current config. + These variables are prefixed with TF_VAR_ and are used to populate the + corresponding variables in the terraform deployment. e.g. + TF_VAR_prefect_token sets the prefect_token variable in Terraform. These + values are set in the terraform state but are not leaked when the + terraform render occurs. + """ + + missing_env_vars = [] + + # Check prefect integration set up. + if "prefect" in config: + var = "TF_VAR_prefect_token" + if not var in os.environ: + missing_env_vars.append(var) + + + + if missing_env_vars: + raise EnvironmentError( + "Some environment variables used to propagate secrets to the " + "terraform deployment were not set. Please set these before " + f"continuing: {', '.join(missing_env_vars)}" + ) + diff --git a/qhub/template/{{ cookiecutter.repo_directory }}/infrastructure/modules/kubernetes/services/prefect/variables.tf b/qhub/template/{{ cookiecutter.repo_directory }}/infrastructure/modules/kubernetes/services/prefect/variables.tf index 8b59ecef69..5ae97eadae 100644 --- a/qhub/template/{{ cookiecutter.repo_directory }}/infrastructure/modules/kubernetes/services/prefect/variables.tf +++ b/qhub/template/{{ cookiecutter.repo_directory }}/infrastructure/modules/kubernetes/services/prefect/variables.tf @@ -10,7 +10,6 @@ variable "namespace" { variable "prefect_token" { type = string - default = "" } variable "image" { From e0ef68b2023758f0c3b05749b64035fc88d250fe Mon Sep 17 00:00:00 2001 From: leej3 Date: Thu, 8 Jul 2021 14:29:27 +0100 Subject: [PATCH 02/12] add support for using env vars for secrets --- qhub/render/__init__.py | 72 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/qhub/render/__init__.py b/qhub/render/__init__.py index ea4fc23e3a..2d6a09ba06 100644 --- a/qhub/render/__init__.py +++ b/qhub/render/__init__.py @@ -1,5 +1,6 @@ import pathlib import collections +import functools import json import os from shutil import copyfile @@ -116,6 +117,12 @@ def render_template(output_directory, config_filename, force=False): with filename.open() as f: config = yaml.safe_load(f) + + # For any config values that start with + # QHUB_SECRET_, set the values using the + # corresponding env var. + set_env_vars_in_config(config) + config["repo_directory"] = repo_directory patch_dask_gateway_extra_config(config) @@ -191,3 +198,68 @@ def qhubignore_matches(_): os.rmdir(root_path / dir) except OSError: pass # Silently fail if 'saved' files are present so dir not empty + + +def set_env_vars_in_config(config): + """ + + For values in the config starting with 'QHUB_SECRET_XXX' the environment + variables are searched for the pattern XXX and the config value is + modified. This enables setting secret values that should not be directly + stored in the config file. + + NOTE: variables are most likely written to a file somewhere upon render. In + order to further reduce risk of exposure of any of these variables you might + consider preventing storage of the terraform render output. + """ + private_entries = get_secret_config_entries(config) + for idx in private_entries: + set_qhub_secret(config,idx) + + +def get_secret_config_entries(config,config_idx=None,private_entries=None): + output = private_entries or [] + if config_idx is None: + sub_dict = config + config_idx = [] + else: + sub_dict = get_sub_config(config,config_idx) + + for key, value in sub_dict.items(): + if type(value) is dict: + sub_dict_outputs = get_secret_config_entries(config,[*config_idx,key],private_entries) + output = [*output,*sub_dict_outputs] + else: + if "QHUB_SECRET_" in str(value): + output = [*output,[*config_idx, key]] + return output + + +def get_sub_config(conf, conf_idx): + sub_config = functools.reduce(dict.__getitem__,conf_idx,conf) + return sub_config + + +def set_sub_config(conf, conf_idx,value): + + get_sub_config(conf,conf_idx[:-1])[conf_idx[-1]] = value + + +def set_qhub_secret(config,idx): + placeholder = get_sub_config(config,idx) + secret_var = get_qhub_secret(placeholder) + set_sub_config(config,idx,secret_var) + + +def get_qhub_secret(secret_var): + env_var = secret_var.lstrip("QHUB_SECRET_") + val = os.environ.get(env_var) + if not val: + raise EnvironmentError( + f"Since '{secret_var}' was found in the" + " QHub config, the environment variable" + f" '{env_var}' must be set." + ) + return val + + From 89385acf73883b948adad3e7cfc0e1beefcfdb38 Mon Sep 17 00:00:00 2001 From: leej3 Date: Thu, 8 Jul 2021 14:30:15 +0100 Subject: [PATCH 03/12] add some tests --- tests/test_deploy.py | 17 +++++++++++++++++ tests/test_render.py | 34 +++++++++++++++++++++++++++++++++- 2 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 tests/test_deploy.py diff --git a/tests/test_deploy.py b/tests/test_deploy.py new file mode 100644 index 0000000000..5658e43408 --- /dev/null +++ b/tests/test_deploy.py @@ -0,0 +1,17 @@ +import pytest +from qhub.deploy import check_secrets + +def test_check_secrets(monkeypatch): + + # Should do nothing without prefect key + check_secrets({"key":"value"}) + + # Should do nothing if appropriate var is set + monkeypatch.setenv("TF_VAR_prefect_token","secret_token") + check_secrets({"prefect":"value"}) + + # Should raise error if var is not set + monkeypatch.delenv("TF_VAR_prefect_token") + with pytest.raises(EnvironmentError): + check_secrets({"prefect":"value"}) + diff --git a/tests/test_render.py b/tests/test_render.py index deed67a4e3..b52afa2ec6 100644 --- a/tests/test_render.py +++ b/tests/test_render.py @@ -2,7 +2,7 @@ from ruamel import yaml -from qhub.render import render_template +from qhub.render import render_template, set_env_vars_in_config from qhub.initialize import render_config @@ -37,3 +37,35 @@ def test_render(project, namespace, domain, cloud_provider, ci_provider, auth_pr output_directory = tmp_path / "test" render_template(str(output_directory), config_filename, force=True) + + +def test_get_secret_config_entries(monkeypatch): + sec1 = "secret1" + sec2 = "nestedsecret1" + config_orig = { + "key1":"value1", + "key2":"QHUB_SECRET_secret_val", + "key3":{ + "nested_key1":"nested_value1", + "nested_key2":"QHUB_SECRET_nested_secret_val", + }, + } + expected = { + "key1":"value1", + "key2":sec1, + "key3":{ + "nested_key1":"nested_value1", + "nested_key2":sec2, + }, + } + + # should raise error if implied env var is not set + with pytest.raises(EnvironmentError): + config = config_orig.copy() + set_env_vars_in_config(config) + + monkeypatch.setenv("secret_val", sec1, prepend=False) + monkeypatch.setenv("nested_secret_val", sec2, prepend=False) + config = config_orig.copy() + set_env_vars_in_config(config) + assert config == expected From 5815be914c099f563fa13cdf1b9b30424ea0a7e3 Mon Sep 17 00:00:00 2001 From: leej3 Date: Fri, 9 Jul 2021 11:21:49 +0100 Subject: [PATCH 04/12] blacken for style fixes --- qhub/deploy.py | 8 +------- qhub/render/__init__.py | 28 ++++++++++++++-------------- tests/test_deploy.py | 10 +++++----- 3 files changed, 20 insertions(+), 26 deletions(-) diff --git a/qhub/deploy.py b/qhub/deploy.py index 05daeccd03..2c74981e0a 100644 --- a/qhub/deploy.py +++ b/qhub/deploy.py @@ -62,10 +62,7 @@ def guided_install( terraform.init(directory="infrastructure") terraform.apply( directory="infrastructure", - targets=[ - "module.kubernetes", - "module.kubernetes-initialization", - ], + targets=["module.kubernetes", "module.kubernetes-initialization",], ) # 04 Create qhub initial state (up to nginx-ingress) @@ -151,12 +148,9 @@ def check_secrets(config): if not var in os.environ: missing_env_vars.append(var) - - if missing_env_vars: raise EnvironmentError( "Some environment variables used to propagate secrets to the " "terraform deployment were not set. Please set these before " f"continuing: {', '.join(missing_env_vars)}" ) - diff --git a/qhub/render/__init__.py b/qhub/render/__init__.py index 2d6a09ba06..e09752e7ae 100644 --- a/qhub/render/__init__.py +++ b/qhub/render/__init__.py @@ -214,41 +214,43 @@ def set_env_vars_in_config(config): """ private_entries = get_secret_config_entries(config) for idx in private_entries: - set_qhub_secret(config,idx) + set_qhub_secret(config, idx) -def get_secret_config_entries(config,config_idx=None,private_entries=None): +def get_secret_config_entries(config, config_idx=None, private_entries=None): output = private_entries or [] if config_idx is None: sub_dict = config config_idx = [] else: - sub_dict = get_sub_config(config,config_idx) + sub_dict = get_sub_config(config, config_idx) for key, value in sub_dict.items(): if type(value) is dict: - sub_dict_outputs = get_secret_config_entries(config,[*config_idx,key],private_entries) - output = [*output,*sub_dict_outputs] + sub_dict_outputs = get_secret_config_entries( + config, [*config_idx, key], private_entries + ) + output = [*output, *sub_dict_outputs] else: if "QHUB_SECRET_" in str(value): - output = [*output,[*config_idx, key]] + output = [*output, [*config_idx, key]] return output def get_sub_config(conf, conf_idx): - sub_config = functools.reduce(dict.__getitem__,conf_idx,conf) + sub_config = functools.reduce(dict.__getitem__, conf_idx, conf) return sub_config -def set_sub_config(conf, conf_idx,value): +def set_sub_config(conf, conf_idx, value): - get_sub_config(conf,conf_idx[:-1])[conf_idx[-1]] = value + get_sub_config(conf, conf_idx[:-1])[conf_idx[-1]] = value -def set_qhub_secret(config,idx): - placeholder = get_sub_config(config,idx) +def set_qhub_secret(config, idx): + placeholder = get_sub_config(config, idx) secret_var = get_qhub_secret(placeholder) - set_sub_config(config,idx,secret_var) + set_sub_config(config, idx, secret_var) def get_qhub_secret(secret_var): @@ -261,5 +263,3 @@ def get_qhub_secret(secret_var): f" '{env_var}' must be set." ) return val - - diff --git a/tests/test_deploy.py b/tests/test_deploy.py index 5658e43408..274e3edc63 100644 --- a/tests/test_deploy.py +++ b/tests/test_deploy.py @@ -1,17 +1,17 @@ import pytest from qhub.deploy import check_secrets + def test_check_secrets(monkeypatch): # Should do nothing without prefect key - check_secrets({"key":"value"}) + check_secrets({"key": "value"}) # Should do nothing if appropriate var is set - monkeypatch.setenv("TF_VAR_prefect_token","secret_token") - check_secrets({"prefect":"value"}) + monkeypatch.setenv("TF_VAR_prefect_token", "secret_token") + check_secrets({"prefect": "value"}) # Should raise error if var is not set monkeypatch.delenv("TF_VAR_prefect_token") with pytest.raises(EnvironmentError): - check_secrets({"prefect":"value"}) - + check_secrets({"prefect": "value"}) From 6ef84658d9ff0a39fe214a9774165cc9d986a5cd Mon Sep 17 00:00:00 2001 From: leej3 Date: Fri, 9 Jul 2021 11:28:18 +0100 Subject: [PATCH 05/12] fix for when prefect is not enabled --- qhub/deploy.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qhub/deploy.py b/qhub/deploy.py index 2c74981e0a..49d6968093 100644 --- a/qhub/deploy.py +++ b/qhub/deploy.py @@ -143,9 +143,9 @@ def check_secrets(config): missing_env_vars = [] # Check prefect integration set up. - if "prefect" in config: + if "prefect" in config and config["prefect"]["enabled"]: var = "TF_VAR_prefect_token" - if not var in os.environ: + if var not in os.environ: missing_env_vars.append(var) if missing_env_vars: From 7bfb632180f43a8fcdd9bc234518d341058b93dd Mon Sep 17 00:00:00 2001 From: leej3 Date: Fri, 9 Jul 2021 12:13:26 +0100 Subject: [PATCH 06/12] oops --- tests/test_render.py | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/tests/test_render.py b/tests/test_render.py index b52afa2ec6..fe8105fdae 100644 --- a/tests/test_render.py +++ b/tests/test_render.py @@ -15,7 +15,9 @@ ("azure-pytest", "dev", "azure.qhub.dev", "azure", "github-actions", "github"), ], ) -def test_render(project, namespace, domain, cloud_provider, ci_provider, auth_provider, tmp_path): +def test_render( + project, namespace, domain, cloud_provider, ci_provider, auth_provider, tmp_path +): config = render_config( project_name=project, namespace=namespace, @@ -43,29 +45,29 @@ def test_get_secret_config_entries(monkeypatch): sec1 = "secret1" sec2 = "nestedsecret1" config_orig = { - "key1":"value1", - "key2":"QHUB_SECRET_secret_val", - "key3":{ - "nested_key1":"nested_value1", - "nested_key2":"QHUB_SECRET_nested_secret_val", + "key1": "value1", + "key2": "QHUB_SECRET_secret_val", + "key3": { + "nested_key1": "nested_value1", + "nested_key2": "QHUB_SECRET_nested_secret_val", }, } expected = { - "key1":"value1", - "key2":sec1, - "key3":{ - "nested_key1":"nested_value1", - "nested_key2":sec2, + "key1": "value1", + "key2": sec1, + "key3": { + "nested_key1": "nested_value1", + "nested_key2": sec2, }, } # should raise error if implied env var is not set with pytest.raises(EnvironmentError): - config = config_orig.copy() + config = config_orig.copy() set_env_vars_in_config(config) monkeypatch.setenv("secret_val", sec1, prepend=False) monkeypatch.setenv("nested_secret_val", sec2, prepend=False) - config = config_orig.copy() + config = config_orig.copy() set_env_vars_in_config(config) assert config == expected From 94bf880c641a93de5cf9e3ed42447723dbc14fd9 Mon Sep 17 00:00:00 2001 From: leej3 Date: Fri, 9 Jul 2021 12:22:27 +0100 Subject: [PATCH 07/12] oops2 --- qhub/deploy.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/qhub/deploy.py b/qhub/deploy.py index 49d6968093..b1685e9c5b 100644 --- a/qhub/deploy.py +++ b/qhub/deploy.py @@ -62,7 +62,10 @@ def guided_install( terraform.init(directory="infrastructure") terraform.apply( directory="infrastructure", - targets=["module.kubernetes", "module.kubernetes-initialization",], + targets=[ + "module.kubernetes", + "module.kubernetes-initialization", + ], ) # 04 Create qhub initial state (up to nginx-ingress) From 9a56ad99af34fc6e2a447117527bd49298a0ad30 Mon Sep 17 00:00:00 2001 From: leej3 Date: Fri, 9 Jul 2021 12:33:33 +0100 Subject: [PATCH 08/12] fix test for check_secrets --- tests/test_deploy.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/test_deploy.py b/tests/test_deploy.py index 274e3edc63..853055f7b3 100644 --- a/tests/test_deploy.py +++ b/tests/test_deploy.py @@ -4,14 +4,15 @@ def test_check_secrets(monkeypatch): - # Should do nothing without prefect key + # Should do nothing without prefect key or not enabled check_secrets({"key": "value"}) + check_secrets({"prefect": {"enabled": False}}) # Should do nothing if appropriate var is set monkeypatch.setenv("TF_VAR_prefect_token", "secret_token") - check_secrets({"prefect": "value"}) + check_secrets({"prefect": {"enabled": True}}) # Should raise error if var is not set monkeypatch.delenv("TF_VAR_prefect_token") with pytest.raises(EnvironmentError): - check_secrets({"prefect": "value"}) + check_secrets({"prefect": {"enabled": True}}) From a70ae36f2d5dc7090077b3a41fe8fdd39b50effd Mon Sep 17 00:00:00 2001 From: leej3 Date: Fri, 9 Jul 2021 14:43:28 +0100 Subject: [PATCH 09/12] update terraform version for test --- .github/workflows/test.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index c45b97d43d..926c5b7aa7 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -76,13 +76,13 @@ jobs: if: ${{ matrix.provider != 'local' }} uses: hashicorp/terraform-github-actions@master with: - tf_actions_version: 0.14.9 + tf_actions_version: 1.0.0 tf_actions_subcommand: 'fmt' tf_actions_working_dir: 'qhub-${{ matrix.provider }}-deployment/terraform-state' - name: 'Terraform Format' uses: hashicorp/terraform-github-actions@master with: - tf_actions_version: 0.14.9 + tf_actions_version: 1.0.0 tf_actions_subcommand: 'fmt' tf_actions_working_dir: 'qhub-${{ matrix.provider }}-deployment/infrastructure' - name: QHub Render Artifact From ae080b5547203651da8d78b7c007b91a5951548b Mon Sep 17 00:00:00 2001 From: leej3 Date: Fri, 9 Jul 2021 14:48:20 +0100 Subject: [PATCH 10/12] reformat tf module --- .../modules/kubernetes/services/prefect/variables.tf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qhub/template/{{ cookiecutter.repo_directory }}/infrastructure/modules/kubernetes/services/prefect/variables.tf b/qhub/template/{{ cookiecutter.repo_directory }}/infrastructure/modules/kubernetes/services/prefect/variables.tf index 5ae97eadae..e680017395 100644 --- a/qhub/template/{{ cookiecutter.repo_directory }}/infrastructure/modules/kubernetes/services/prefect/variables.tf +++ b/qhub/template/{{ cookiecutter.repo_directory }}/infrastructure/modules/kubernetes/services/prefect/variables.tf @@ -9,7 +9,7 @@ variable "namespace" { } variable "prefect_token" { - type = string + type = string } variable "image" { From 3dbff812fe37e2e01df2c311a1fb01beca0a428f Mon Sep 17 00:00:00 2001 From: leej3 Date: Fri, 9 Jul 2021 17:05:05 +0100 Subject: [PATCH 11/12] document secret hiding functionality --- .../source/02_get_started/04_configuration.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/docs/source/02_get_started/04_configuration.md b/docs/source/02_get_started/04_configuration.md index 0e375af90e..8c224bfa74 100644 --- a/docs/source/02_get_started/04_configuration.md +++ b/docs/source/02_get_started/04_configuration.md @@ -144,6 +144,25 @@ security: gid: 102 ``` +### Omitting sensitive values + +If you wish to avoid storing secrets etc. directly in the config yaml file you +can instead set the values in environment variables. This substitution is +triggered by setting config values to "QHUB_SECRET_" followed by the +environment variable name. For example, you could set the environment variables +github_client_id and github_client_key and write the following in your config +file: + +```yaml +security: + authentication: + type: GitHub + config: + client_id: QHUB_SECRET_github_client_id + client_secret: QHUB_SECRET_github_client_key + oauth_callback_url: https://do.qhub.dev/hub/oauth_callback +``` + ### Authentication `security.authentication` is for configuring the OAuth and GitHub From e7a5058cdb487d9460cfa5a943ab9420eab5f7c7 Mon Sep 17 00:00:00 2001 From: leej3 Date: Fri, 9 Jul 2021 17:09:08 +0100 Subject: [PATCH 12/12] quote var names --- docs/source/02_get_started/04_configuration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/02_get_started/04_configuration.md b/docs/source/02_get_started/04_configuration.md index 8c224bfa74..5b344fddf1 100644 --- a/docs/source/02_get_started/04_configuration.md +++ b/docs/source/02_get_started/04_configuration.md @@ -150,7 +150,7 @@ If you wish to avoid storing secrets etc. directly in the config yaml file you can instead set the values in environment variables. This substitution is triggered by setting config values to "QHUB_SECRET_" followed by the environment variable name. For example, you could set the environment variables -github_client_id and github_client_key and write the following in your config +"github_client_id" and "github_client_key" and write the following in your config file: ```yaml