Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Follow-up to #5334 #5382

Merged
merged 3 commits into from
Jun 28, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .changes/unreleased/Under the Hood-20220606-230353.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
kind: Under the Hood
body: Update context readme + clean up context code"
kind: Security
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This won't change how the changelog markdown gets generated but for consistency can we change the file name to be Security-20220606-230353.yaml? For context, the filenames are just like this for uniqueness, they don't actually get any fields out of the name.

body: Move string interpolation of "secret" env vars outside of Jinja context. Update "contexts" README
time: 2022-06-06T23:03:53.022568+02:00
custom:
Author: jtcohen6
Expand Down
7 changes: 7 additions & 0 deletions .changes/unreleased/Under the Hood-20220616-120516.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
kind: Under the Hood
body: 'Add annotation to render_value method reimplemented in #5334'
time: 2022-06-16T12:05:16.474078+02:00
custom:
Author: jtcohen6
Issue: "4796"
PR: "5382"
1 change: 1 addition & 0 deletions .changie.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ kinds:
- label: Docs
- label: Under the Hood
- label: Dependencies
- label: Security
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

custom:
- key: Author
label: GitHub Username(s) (separated by a single space if multiple)
Expand Down
5 changes: 5 additions & 0 deletions core/dbt/config/renderer.py
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,12 @@ def name(self):
return "Secret"

def render_value(self, value: Any, keypath: Optional[Keypath] = None) -> Any:
# First, standard Jinja rendering, with special handling for 'secret' environment variables
# "{{ env_var('DBT_SECRET_ENV_VAR') }}" -> "$$$DBT_SECRET_START$$$DBT_SECRET_ENV_{VARIABLE_NAME}$$$DBT_SECRET_END$$$"
# This prevents Jinja manipulation of secrets via macros/filters that might leak partial/modified values in logs
rendered = super().render_value(value, keypath)
# Now, detect instances of the placeholder value ($$$DBT_SECRET_START...DBT_SECRET_END$$$)
# and replace them with the actual secret value
if SECRET_ENV_PREFIX in str(rendered):
search_group = f"({SECRET_ENV_PREFIX}(.*))"
pattern = SECRET_PLACEHOLDER.format(search_group).replace("$", r"\$")
Expand Down
22 changes: 22 additions & 0 deletions tests/functional/context_methods/test_secret_env_vars.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,3 +134,25 @@ def test_fail_clone_with_scrubbing(self, project):
_, log_output = run_dbt_and_capture(["deps"])

assert "abc123" not in str(excinfo.value)


class TestCloneFailSecretNotRendered(TestCloneFailSecretScrubbed):
# as above, with some Jinja manipulation
@pytest.fixture(scope="class")
def packages(self):
return {
"packages": [
{
"git": "https://fakeuser:{{ env_var('DBT_ENV_SECRET_GIT_TOKEN') | join(' ') }}@github.com/dbt-labs/fake-repo.git"
},
]
}

def test_fail_clone_with_scrubbing(self, project):
with pytest.raises(InternalException) as excinfo:
_, log_output = run_dbt_and_capture(["deps"])

# we should not see any manipulated form of the secret value (abc123) here
# we should see a manipulated form of the placeholder instead
assert "a b c 1 2 3" not in str(excinfo.value)
assert "D B T _ E N V _ S E C R E T _ G I T _ T O K E N" in str(excinfo.value)