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

Secret fixture to hide in the logs #8613

Closed
Marcdsv opened this issue Apr 30, 2021 · 13 comments
Closed

Secret fixture to hide in the logs #8613

Marcdsv opened this issue Apr 30, 2021 · 13 comments
Labels
topic: fixtures anything involving fixtures directly or indirectly type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature

Comments

@Marcdsv
Copy link

Marcdsv commented Apr 30, 2021

I have a fixture that gets credentials from a vault, and if the test fails the credentials are displayed in the log, which is not great regarding security...

Currently is seems the only ways to avoid that is to either set pytest argument --tb='short' (but it's a general parameter and I'd like to keep other values displayed, only hide the secret ones) or to redefine the logging function to hide specific values.

I suggest adding a parameter to the decorator to indicate that the fixture value should not be displayed in the log (or replaced with *******), for example:
@fixture(secret=True)

@The-Compiler
Copy link
Member

How exactly are they displayed? Could you show an example log (with the actual values redacted, of course)?

@The-Compiler The-Compiler added the status: needs information reporter needs to provide more information; can be closed after 2 or more weeks of inactivity label Apr 30, 2021
@Marcdsv
Copy link
Author

Marcdsv commented Apr 30, 2021

So for example my test function is:

def test_stuff(get_credentials):
    ...
    assert val != 0

with the fixture

@fixture(autouse=True, scope='session')
def get_credentials():
    """Return credentials as a dict with 'user' and 'password' keys."""
    ...
    return credentials

If the test fails, the display in the logs will be:

___________________________ test_stuff ___________________________

get_credentials = {'user': 'the_username', 'password': 'the_password_in_plain_sight'}

    def test_stuff(get_credentials):
        ...
>       assert val != 0
E       assert 0 != 0

test_module.py:28: AssertionError

@The-Compiler The-Compiler added topic: fixtures anything involving fixtures directly or indirectly type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature and removed status: needs information reporter needs to provide more information; can be closed after 2 or more weeks of inactivity labels Apr 30, 2021
@The-Compiler
Copy link
Member

Here's a complete example based on yours:

import pytest

@pytest.fixture(autouse=True, scope='session')
def get_credentials():
    """Return credentials as a dict with 'user' and 'password' keys."""
    return {'user': 'the_username', 'password': 'the_password_in_plain_sight'}

def test_stuff(get_credentials):
    val = 0
    assert val != 0

I'm kind of sceptical whether that kind of functionality should be built into pytest, since it's only a very narrow scenario where printing gets prevented. There are countless other ways the value could be printed (e.g. by it being part of an assert), and it's easy to convey a false promise with an option like this.

Then again, looks like you're not the only one: python - How can I prevent the value of a fixture to be printed? - Stack Overflow.

But I think it'd be a better solution to replace the password string with a custom wrapper object with an obscured __repr__ - that would make sure it never is visible when printed, no matter where it's printed from.

@Marcdsv
Copy link
Author

Marcdsv commented Apr 30, 2021

Maybe the behaviour of the secret fixture parameter could be as you said to redefine the __repr__ function of the fixture returned object so that it's hidden wherever it's printed, no false promise.
I understand it's a narrow scenario, and I can implement this workaround myself for my specific case, I just thought it could be a nice feature to propose.

@Marcdsv
Copy link
Author

Marcdsv commented Apr 30, 2021

Actually after some investigation I'm not sure it's possible to override the __repr__ function of an instance, you'd have to override the class function (I think we don't want that) or implement a custom class, and I don't know if it would be easy to implement a dynamic custom class that inherits from the class of the input object.

@The-Compiler
Copy link
Member

I'd still argue doing this kind of thing is the responsibility of your code and not pytest, since the problem isn't pytest-specific - but let's see what others think.

@nicoddemus
Copy link
Member

I also don't think this is pytest's responsibility, and there's the good point that it would be tricky to get this correctly given this value can be shown in so many places already, besides plugins might inadvertently show it in the end (for example pytest-sugar, or a plugin which logs things somewhere else).

As suggested, better wrap those credentials under a new object:

class Secret:
    def __init__(self, value):
        self.value = value

    def __repr__(self):
        return "Secret(********)"

    def __str___(self):
        return "*******"
     

@pytest.fixture(autouse=True, scope='session')
def get_credentials():
    """Return credentials as a dict with 'user' and 'password' keys."""
    return {'user': 'the_username', 'password': Secret('the_password_in_plain_sight')}

So 👎 on having this on the core somehow, but definitely a good example to put on the docs. 😁

@symonk
Copy link
Member

symonk commented Apr 30, 2021

I'm a -1 on the idea just on what has been mentioned already, I don't think we can reliably secure such values as it depends on what else they are doing/using plugin-wise etc, a custom wrapper with a obfuscated str/repr is (while a bit meh) a semi viable workaround, It does however raise a very important issue that we should mention in the documentation

@Zac-HD
Copy link
Member

Zac-HD commented May 1, 2021

Also 👎 on adding this to pytest, and -0 on adding it to the docs - this just doesn't seem like the test runner's problem.

As prior art, Pydantic has a SecretStr class which behaves much like Niccodemus' example code.

@kumiDa
Copy link
Contributor

kumiDa commented May 3, 2021

With the documentation taking the style suggested here: https://documentation.divio.com./
Won't @nicoddemus's suggestion be suited to be featured under the How-To section of the documentation?

@asottile
Copy link
Member

asottile commented May 4, 2021

@rahul-kumi I don't think even that is in the domain of pytest's responsibility -- it'd be more of a "how to secrets with python" which is orthogonal to testing

I also agree this is -1 so I'm going to close since we seem to have consensus

an aside: having live credentials in tests seems like a recipe for disaster, I usually prefer placeholder credentials

@asottile asottile closed this as completed May 4, 2021
@AldenPeterson
Copy link

One use case for live credentials is if Pytest is being used for integration testing and not unittesting.

We have usernames/passwords we use and retrieve from a credential store during runtime.

I suspect we will end up going down this approach and recommending everyone name their fixtures accordingly - https://stackoverflow.com/a/64555208/1048539

The alternative of this might work, too (I took that example and made it more like how the OP and myself would use it):

import pytest

class Secret:
    def __init__(self, value):
        self.value = value

    def __repr__(self):
        return "Secret(********)"

    def __str___(self):
        return "*******"

def get_from_vault(key):
    return "something looked up in vault"


@pytest.fixture(scope='session')
def password():
    return Secret(get_from_vault("key_in_value"))

def login(username, password):
    pass

def test_using_password(password):
    # reference the value directly in a function
    login("username", password.value)
    # If you use the value directly in an assert, it'll still log if it fails
    assert "something looked up in vault" == password.value

    # but won't be printed here
    assert False

@nicoddemus
Copy link
Member

nicoddemus commented May 4, 2021

# If you use the value directly in an assert, it'll still log if it fails
assert "something looked up in vault" == password.value

Indeed, but that can be improved by making value private and implementing __eq__ which compares the argument with _value:

class Secret:
    def __init__(self, value):
        self._value = value

    def __repr__(self):
        return "Secret(********)"

    def __str___(self):
        return "*******"
    
    def __eq__(self, other):
        if not isinstance(other, str):
            return NotImplemented
        return self._value == other  

Then you can safely use:

assert password == "something looked up in vault"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: fixtures anything involving fixtures directly or indirectly type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature
Projects
None yet
Development

No branches or pull requests

8 participants