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.

Sign up for GitHub

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

Exclude some profile fields from Jinja rendering when they are not valid Jinja. #7630

Merged
merged 5 commits into from
May 17, 2023
Merged

Exclude some profile fields from Jinja rendering when they are not valid Jinja. #7630

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
91b50ab
CT-2583: Exclude some profile fields from Jinja rendering.
peterallenwebb May 15, 2023
cd483ee
CT-2583: Add functional test.
peterallenwebb May 15, 2023
1a6afcc
CT-2583: Change approach to password jinja detection
peterallenwebb May 15, 2023
9b745e0
CT-2583: Extract string constant and add additional checks
peterallenwebb May 16, 2023
4ab9240
CT-2583: Improve unit test coverage
peterallenwebb May 16, 2023
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
6 changes: 6 additions & 0 deletions .changes/unreleased/Fixes-20230515-142851.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Fixes
body: Exclude password fields from Jinja rendering.
Copy link
Contributor

Choose a reason for hiding this comment

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

One idea for updating the changelog entry to reflect the latest approach:

Suggested change
body: Exclude password fields from Jinja rendering.
body: Fall back if rendering the password field fails.

time: 2023-05-15T14:28:51.400321-04:00
custom:
Author: peterallenwebb
Issue: "7629"
12 changes: 11 additions & 1 deletion core/dbt/config/renderer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,17 @@ 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)

try:
rendered = super().render_value(value, keypath)
except Exception as ex:
Copy link
Member

Choose a reason for hiding this comment

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

Catch a more specific exception here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would except I do not know what will be in the exception message here, and I'm concerned that the risk of it containing sensitive information that would be logged could outweigh the value of a narrower catch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the issue that we don't have any idea of the Exception? Or is it that we expect a few specific ones (let's say ExceptionA and ExceptionB for simplicity) and also want to make sure that we catch anything? If it's the latter, then would this be a compromise:

try:
    rendered = super().render_value(value, keypath)
except ExceptionA:
    # do some ExceptionA stuff
except ExceptionB:
    # do some ExceptionB stuff
except Exception as ex:
    # this shouldn't happen so throw a dbt exception, such as DbtCompilerException

Copy link
Member

Choose a reason for hiding this comment

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

I like @mikealfare's suggestion here

if keypath and "password" in keypath:
# Passwords sometimes contain jinja-esque characters, but we
# don't want to render them if they aren't valid jinja.
rendered = value
else:
raise ex

# 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):
Expand Down
25 changes: 25 additions & 0 deletions tests/functional/profiles/test_profiles_yml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import pathlib

from dbt.cli.main import dbtRunner


class TestProfileParsing:
def test_no_jinja_for_password(self, project, profiles_root):
with open(pathlib.Path(profiles_root, "profiles.yml"), "w") as profiles_yml:
profiles_yml.write(
"""test:
Copy link
Member

Choose a reason for hiding this comment

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

Can you store this string as a constant at the top to make it easier to read?

outputs:
default:
dbname: my_db
host: localhost
password: no{{jinja{%re{#ndering
port: 12345
schema: dummy
threads: 4
type: postgres
user: peter.webb
target: default
"""
)
result = dbtRunner().invoke(["parse"])
assert result.success # for now, at least, just ensure success
Copy link
Member

Choose a reason for hiding this comment

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

Is there a failure case we can test here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add an additional check that scans the log messages for the password string.