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

fix: serialize secrets to json. #465

Merged
merged 7 commits into from
Apr 11, 2019

Conversation

Atheuz
Copy link
Contributor

@Atheuz Atheuz commented Apr 6, 2019

Change Summary

This change adds JSON serialization of secret types, SecretStr and SecretBytes. Also modifies the str functions of the same.
It does not support serialization of the actual serialized values, which it maybe should.

Related issue number

#462

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable

@codecov
Copy link

codecov bot commented Apr 6, 2019

Codecov Report

Merging #465 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #465   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          14     14           
  Lines        2225   2230    +5     
  Branches      437    437           
=====================================
+ Hits         2225   2230    +5

@Atheuz
Copy link
Contributor Author

Atheuz commented Apr 6, 2019

I don't know if this is a good idea or not. I think @samuelcolvin would prefer not to do something like this.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

I think the change required here can be much simpler.

pydantic/types.py Show resolved Hide resolved
docs/examples/ex_secret_types.py Outdated Show resolved Hide resolved
pydantic/json.py Outdated Show resolved Hide resolved
@Atheuz
Copy link
Contributor Author

Atheuz commented Apr 8, 2019

@samuelcolvin

The reason I made changes to the str function for both is that if we keep it as is, then we end up with these kinds of things as JSON output:

>>> import pydantic
>>> class Settings(pydantic.BaseModel):
...     passw: pydantic.SecretStr
...
>>> conf = Settings(passw="1234")
>>> conf
<Settings passw=SecretStr('**********')>
>>> conf.dict()
{'passw': SecretStr('**********')}
>>> conf.json()
'{"passw": "SecretStr(\'**********\')"}'

Where as with the modified str we would get something like this instead:

>>> import pydantic
>>> class Settings(pydantic.BaseModel):
...     passw: pydantic.SecretStr
...
>>> conf = Settings(passw="1234")
>>> conf
<Settings passw=SecretStr('**********')>
>>> conf.dict()
{'passw': SecretStr('**********')}
>>> conf.json()
'{"passw": "**********"}'

Please let me know what you think.

@samuelcolvin
Copy link
Member

Ok, let's add a display() method to both with returns either None or ********** and use that in JSON, then keep str and repr the same.

@samuelcolvin
Copy link
Member

That will then need one line the docs/example to explain/show it.

@Atheuz
Copy link
Contributor Author

Atheuz commented Apr 8, 2019

@samuelcolvin is that something like what you were thinking?

@samuelcolvin
Copy link
Member

Yes exactly.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise LGTM.

pydantic/types.py Outdated Show resolved Hide resolved
docs/examples/ex_secret_types.py Outdated Show resolved Hide resolved
@Atheuz
Copy link
Contributor Author

Atheuz commented Apr 11, 2019

@samuelcolvin I've been thinking about how to deal with a situation such as this:

from datetime import datetime
from typing import List, Any, Union, Dict
from pydantic import BaseSettings, BaseModel, SecretStr, validator
import os

class Test:
    def __init__(self, config):
        self.config = config
        self.uri = f"http://{config['USERNAME']}:{config['PASSWORD']}@{config['HOST']}"  # Won't work!
    

class NestedSettings(BaseModel):
    HOST: str
    PASSWORD: SecretStr
    USERNAME: SecretStr
    

class Settings(BaseModel):
    USERNAME: SecretStr = SecretStr("")
    PASSWORD: SecretStr = SecretStr("")
    HOST: str = "postgres.github.io"
    PORT: int = 5432
    DB_NAME: str = "testdb"
    VARS: NestedSettings
    
    DB_URI: SecretStr = SecretStr("")
    
    
    @validator("DB_URI", pre=True, whole=True, always=True)
    def set_db_uri(cls, v, values, **kwargs):
        """Set DB_URI."""
        username = values["USERNAME"].get_secret_value()
        password = values["PASSWORD"].get_secret_value()
        host = values["HOST"]
        port = values["PORT"]
        db_name = values["DB_NAME"]
        result = f"postgresql://{username}:{password}@{host}:{port}/{db_name}"
        return result
        
    
def main():
    external_data = {"USERNAME": "abc", "PASSWORD": "def", "VARS": {"HOST": "www.google.com", "USERNAME": "user1", "PASSWORD":"passw"}}
    conf = Settings(**external_data)
    print(conf.dict())
    testobj = Test(conf.dict()["VARS"])
    print(testobj.uri)
    
if __name__ == '__main__':
    main()

Which would result in:

{'VARS': {'HOST': 'www.google.com', 'PASSWORD': SecretStr('**********'), 'USERNAME': SecretStr('**********')}, 'USERNAME': SecretStr('**********'), 'PASSWORD': SecretStr('**********'), 'HOST': 'postgres.github.io', 'PORT': 5432, 'DB_NAME': 'testdb', 'DB_URI': SecretStr('**********')}
http://SecretStr('**********'):SecretStr('**********')@www.google.com

If you need the inner 'object' to be provided for the configuration of another item, you have to kind of force it to fit by modifying the values in-place to be what you want. Like for instance in the above example, where you'd have to unsecret the secret items(manually turn everything into str) before you can proceed.

Which is not especially useful, ideally we would be able to unsecret the entire dict recursively and then pass it on to classes or functions that need those items to be plain-text.

Do you have any thoughts on this?

@samuelcolvin
Copy link
Member

samuelcolvin commented Apr 11, 2019

See dangerouslySetInnerHTML in react.

See hazmat in cryptography.

The whole point is to make it somewhat involved to get the raw value.

If there's any point whatsoever in include SecretStr in pydantic, then we have to make accessing the raw value only possible with get_secret_value() or an internal value.

If people don't need it to be secret, they don't have to use it.

In fact in your example above, it's still not exactly complicated to generate the uri:

f'postgresql://{username.get_secret_value()}:{password.get_secret_value()}@{host}:{port}/{db_name}'

@Atheuz
Copy link
Contributor Author

Atheuz commented Apr 11, 2019

@samuelcolvin In the postgresql example, that would expose secrets if the conf object is printed out.
I think I just need to adjust how I would be using these and use the raw values ocne the config has been loaded, in places where those values will not be exposed.

Anyway, that's not really related to this PR, just a couple of thoughts I was having.
Let me know if you want me to make further changes.

@samuelcolvin samuelcolvin merged commit 449661b into pydantic:master Apr 11, 2019
@samuelcolvin
Copy link
Member

that would expose secrets if the conf object is printed out.

Yes, obviously!

If you use the secret values then they may be visible in the context in which you use them, there's no way around that.

The only point in SecretStr is to avoid it be printed in exceptions or logging (facebook!) until they're used.

samuelcolvin added a commit that referenced this pull request Apr 11, 2019
alexdrydew pushed a commit to alexdrydew/pydantic that referenced this pull request Dec 23, 2023
* use pytest-examples

* fix docstring tests on emscripten

* working on linting examples

* fixing more examples

* update docstring

* updating examples

* all examples formatted

* reformat examples

* uprev deps

* revert coverage

* revert other uprevs

* remove "HYPOTHESIS_PROFILE: slow"

* fix ci

* linting and add tmate

* move tmate

* add -g to RUSTFLAGS

* allow coverage to fail
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants