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

Projects
None yet
3 participants
@Atheuz
Copy link
Contributor

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor Author

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.

@samuelcolvin
Copy link
Owner

left a comment

I think the change required here can be much simpler.

Show resolved Hide resolved pydantic/types.py
Show resolved Hide resolved docs/examples/ex_secret_types.py Outdated
Show resolved Hide resolved pydantic/json.py Outdated
@Atheuz

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Owner

commented Apr 8, 2019

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

This comment has been minimized.

Copy link
Owner

commented Apr 8, 2019

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

@Atheuz

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

@samuelcolvin is that something like what you were thinking?

@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Apr 8, 2019

Yes exactly.

@samuelcolvin
Copy link
Owner

left a comment

otherwise LGTM.

Show resolved Hide resolved pydantic/types.py Outdated
Show resolved Hide resolved docs/examples/ex_secret_types.py Outdated

@Atheuz Atheuz force-pushed the Atheuz:fix-secrets-json branch from 2727be7 to 801bcf8 Apr 11, 2019

@Atheuz

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Owner

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

This comment has been minimized.

Copy link
Contributor Author

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 samuelcolvin:master Apr 11, 2019

4 of 7 checks passed

Header rules No header rules processed
Details
Pages changed All files already uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
codecov/project 100% (+0%) compared to 63afb45
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Apr 11, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.