Add ability to read secrets from files #1820

mdgilene · 2020-08-12T01:14:58Z

Change Summary

Added ability for BaseSettings to load SecretStr and SecretBytes from files. I opted to drop the ability to load complex types from secrets as there is no existing secret type for complex objects.

Related issue number

#1279

Checklist

Unit tests for the changes exist
Tests pass on CI and coverage remains at 100%
Documentation reflects the changes where applicable
changes/<pull request or issue id>-<github username>.md file added describing change
(see changes/README.md for details)

codecov · 2020-08-12T01:18:04Z

Codecov Report

Merging #1820 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1820   +/-   ##
=======================================
  Coverage   99.90%   99.90%           
=======================================
  Files          21       21           
  Lines        4051     4078   +27     
  Branches      807      817   +10     
=======================================
+ Hits         4047     4074   +27     
  Misses          3        3           
  Partials        1        1

Impacted Files	Coverage Δ
pydantic/env_settings.py	`100.00% <100.00%> (ø)`
pydantic/utils.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5288920...f1c17ca. Read the comment docs.

FichteFoll

Looking good from my side. Just a few things, notably the SecretStr documentation issue.

pydantic/env_settings.py

samuelcolvin

this is looking great, a few things to change.

samuelcolvin · 2020-10-09T10:01:27Z

pydantic/env_settings.py

 from .typing import display_as_type
 from .utils import deep_update, sequence_like

 env_file_sentinel = str(object())
+secrets_dir_sentinel = str(object())


this doesn't seem to be used.

Yep, leftover from copying the environment code. Will remove

samuelcolvin · 2020-10-09T10:02:52Z

pydantic/env_settings.py

+
+        secrets_path = Path(secrets_dir)
+
+        if not secrets_path.is_dir():


surely to avoid a lot of confusion we should raise an error if secrets_path.is_file()?

perhaps we should also raise an error/warning if the directory is missing? i'm not so sure on that.

Perhaps only if it's provided via an argument, not config?

Good point, I think that since the default value for secrets_dir is None, that providing any value pointing to a directory that is missing can be an error.

samuelcolvin · 2020-10-09T10:14:36Z

pydantic/env_settings.py

+                warnings.warn(
+                    f'found secret file matching field "{field.name}" but the field was not '
+                    'of type "SecretStr" or "SecretBytes". This secret file was not loaded.',
+                    Warning,


perhaps we should remove this and use UserWarning so the warning is printed? (From memory only UserWarning is printed out by default which would be useful I imagine.

samuelcolvin · 2020-10-09T10:16:14Z

pydantic/env_settings.py

+        for field in self.__fields__.values():
+            if field.type_ not in (SecretStr, SecretBytes):
+                warnings.warn(
+                    f'found secret file matching field "{field.name}" but the field was not '


wait, this is in the wrong place I think. No file has been found yet, we're just iterating through fields.

samuelcolvin · 2020-10-09T10:19:21Z

pydantic/env_settings.py

+        if not secrets_path.is_dir():
+            return d
+
+        secret_paths = {


remove this see comment below

samuelcolvin · 2020-10-09T10:22:26Z

pydantic/env_settings.py

+
+            value: Optional[str] = None
+            for env_name in field.field_info.extra['env_names']:
+                path = secret_paths.get(env_name, None)


Better to remove the `` dict above and just do

path = secrets_path / env_name if path.is_file() and secret type: add to the main dict elif path.is_dir() and secret type: warning that it's a directory elif path.is_file() and not secret type: warning that the file exist but type is not secret

samuelcolvin · 2020-10-09T10:23:07Z

pydantic/env_settings.py

+        """
+        Build secrets suitable for passing to the Model.
+        """
+        d: Dict[str, Optional[str]] = {}


more descriptive name please.

Ok, this is the same name used by the environment loading code so may want to fix that at another time as well.

samuelcolvin · 2020-10-09T10:23:57Z

pydantic/utils.py

@@ -122,13 +122,14 @@ def in_ipython() -> bool:
 KeyType = TypeVar('KeyType')


-def deep_update(mapping: Dict[KeyType, Any], updating_mapping: Dict[KeyType, Any]) -> Dict[KeyType, Any]:
+def deep_update(mapping: Dict[KeyType, Any], *updating_mappings: Dict[KeyType, Any]) -> Dict[KeyType, Any]:


👍 looks good

samuelcolvin · 2020-10-09T10:26:49Z

docs/usage/settings.md

+**a dotenv file and environment variables will always take priority over values loaded from the secrets directory**.
+
+Passing a file path via the `_secrets_dir` keyword argument on instantiation (method 2) will override
+the value (if any) set on the `Config` class.


Does this work with docker secrets and AWS Parameter Store, if so, please can we add a section to the docs saying that and giving an example of how you'd use them.

I don't have any experience with AWS so I can't speak to it. However, Docker secrets as far as I'm aware simply mount the secrets as volumes to /run/secrets so yes this should handle that use case.

samuelcolvin · 2020-10-25T16:32:25Z

I've made a few tweaks to the docs, this looks great and I'm ready to merge it except one thing.

I think these values from secrets files should apply to all fields, not just SecretStr and SecretBytes. This is because:

it would make it more like other variable substitutions from arguments, environment variables and dot-env files, and thus less confusing
I often want field types to be something like HttpUrl or even int, restricting to SecretStr and SecretBytes would prevent that

Sorry, I should have thought about this before, but it didn't occur to me until now.

@mdgilene and @FichteFoll what do you think? Unless you have strong objections, I can either wait for you to make the change, or make it myself.

FichteFoll · 2020-10-25T16:47:50Z

I think these values from secrets files should apply to all fields, not just SecretStr and SecretBytes.

I definitely agree with your points there, as they reflect my own concerns regarding gating this behind special classes. While the explicit declaration of loading from secrets makes it easier to see what values are different at a glance, it also limits the functionality compared to loading values from environment variable, where nothing needs to be configured.

(I'm also sorry for not raising this before despite thinking about it, as it didn't affect my particular use case.)

mdgilene · 2020-10-25T17:37:11Z

I don't have a strong preference one way or the other. So opening it up to all types works for me. I don't actually have much time at the moment, so if you are able to make the change that would be awesome. Otherwise it might take a few days till I get around to it.

samuelcolvin · 2020-10-25T17:53:22Z

okay, thanks. I'll try and do it.

FichteFoll · 2020-10-25T22:05:33Z

tests/test_settings.py

        Settings()


+@pytest.mark.skipif(sys.platform.startswith('win'), reason='windows paths break regex')


~~You could .replace('/', r'[\\/]').~~

Or alternatively just use os.sep instead of a / literal?

I tried escaping, too boring without a Windows install.

FichteFoll · 2020-10-25T22:06:04Z

pydantic/env_settings.py

        """
        secrets: Dict[str, Optional[str]] = {}

        secrets_dir = _secrets_dir or self.__config__.secrets_dir
        if secrets_dir is None:
            return secrets

-        secrets_path = Path(secrets_dir)
+        secrets_path = Path(secrets_dir).expanduser()


Is this documented?

No, but it's pretty obvious

Related #1804

Add ability to read secrets from files

c9543ec

mdgilene force-pushed the settings-from-secret-files branch from 4d5192a to c9543ec Compare August 12, 2020 02:17

Added docs and updated tests to handle new _build_values argument

ea6e031

mdgilene marked this pull request as ready for review August 12, 2020 17:56

FichteFoll reviewed Aug 14, 2020

View reviewed changes

pydantic/env_settings.py Outdated Show resolved Hide resolved

pydantic/env_settings.py Outdated Show resolved Hide resolved

mdgilene added 3 commits August 14, 2020 11:33

Updated docs to note which types are loaded

94d29b5

Updated deep_update to take a list of update mappings

0dba50b

Added warning when fields are not defined as a secret type

8ae4fec

mdgilene force-pushed the settings-from-secret-files branch from 708e380 to 8ae4fec Compare August 14, 2020 21:18

FichteFoll reviewed Aug 15, 2020

View reviewed changes

pydantic/env_settings.py Outdated Show resolved Hide resolved

Merge branch 'master' into settings-from-secret-files

7938dd4

samuelcolvin reviewed Oct 9, 2020

View reviewed changes

Address additional PR feedback

88d2547

mdgilene requested a review from samuelcolvin October 9, 2020 15:36

improve documentation

2194cfa

remove redundant UserWarning lines

ea87983

samuelcolvin added 4 commits October 25, 2020 19:11

allow any field type, not just SecretStr

b98798e

Merge branch 'master' into mdgilene-settings-from-secret-files

071179c

regex escape paths in windows 😴

8efa093

skip tests on windows 😧 🔫

033703d

FichteFoll reviewed Oct 25, 2020

View reviewed changes

Merge branch 'master' into mdgilene-settings-from-secret-files

f1c17ca

samuelcolvin merged commit 30cd121 into pydantic:master Oct 26, 2020

PrettyWood mentioned this pull request Jan 3, 2021

Read values from secrets files when using BaseSettings #1279

Closed


		secrets_path = Path(secrets_dir)

		if not secrets_path.is_dir():

		Settings()


		@pytest.mark.skipif(sys.platform.startswith('win'), reason='windows paths break regex')

Uh oh!

Add ability to read secrets from files #1820

Add ability to read secrets from files #1820

Uh oh!

Conversation

mdgilene commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Summary

Related issue number

Checklist

Uh oh!

codecov bot commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

FichteFoll left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samuelcolvin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samuelcolvin commented Oct 25, 2020

Uh oh!

FichteFoll commented Oct 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdgilene commented Oct 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samuelcolvin commented Oct 25, 2020

Uh oh!

FichteFoll Oct 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdgilene commented Aug 12, 2020 •

edited

Loading

codecov bot commented Aug 12, 2020 •

edited

Loading

FichteFoll left a comment •

edited

Loading

FichteFoll commented Oct 25, 2020 •

edited

Loading

mdgilene commented Oct 25, 2020 •

edited

Loading

FichteFoll Oct 25, 2020 •

edited

Loading