nova.exception._cleanse_dict should use oslo_utils.strutils._SANITIZE_KEYS

[Matt Riedemann]

The wrap_exception decorator in nova.exception uses the _cleanse_dict helper method to remove any keys from the args/kwargs list of the method that was called, but only checks those keys of the form *_pass:

http://git.openstack.org/cgit/openstack/nova/tree/nova/exception.py?id=12.0.0.0b2#n57

def _cleanse_dict(original):
    """Strip all admin_password, new_pass, rescue_pass keys from a dict."""
    return {k: v for k, v in six.iteritems(original) if "_pass" not in k}

The oslo_utils.strutils module has it's own list of keys to sanitized used in it's mask_password method:

http://git.openstack.org/cgit/openstack/oslo.utils/tree/oslo_utils/strutils.py#n54

_SANITIZE_KEYS = ['adminPass', 'admin_pass', 'password', 'admin_password',
                  'auth_token', 'new_pass', 'auth_password', 'secret_uuid',
                  'sys_pswd']

The nova code should probably be using some form of the same thing that strutils is using for mask_password, which uses a regex to find hits. For example, if the arg was 'auth_token' or simply 'password', _cleanse_dict would fail to filter it out.

You could also argue that the oslo.messaging log notifier should be using oslo_utils.strutils.mask_password before it logs the message - which isn't happening in that library today.

[Ryan Rossiter (rlrossit)]

Do we want to maintain the current functionality of _cleanse_dict() that removes the keys, or do we want to move closer to strutils and have it leave the keys, but mask the values?

[Matt Riedemann (mriedem)]

Per comment 1, I assume the _cleanse_dict method would continue to remove the keys.

[Matt Riedemann (mriedem)]

This is the part of oslo.messaging that it seems we should call strutils.mask_password on the json string:

http://git.openstack.org/cgit/openstack/oslo.messaging/tree/oslo_messaging/notify/_impl_log.py#n41

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/215308

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/215349

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/215349
Committed: https://git.openstack.org/cgit/openstack/oslo.messaging/commit/?id=c990ee02faa9f88e07a27af12eae397e7e8097dc
Submitter: Jenkins
Branch: master

commit c990ee02faa9f88e07a27af12eae397e7e8097dc
Author: Ryan Rossiter <email address hidden>
Date: Thu Aug 20 20:47:42 2015 +0000

    Mask passwords when logging messages

    When logging a message, any secrets and passwords should be masked. This
    uses oslo_utils.strutils to mask any passwords that are to be logged.

    Change-Id: I263d44c0f2e900c5f6e210cbd7ec56e48d0d5bb2
    Closes-Bug: #1487038

[Sivasathurappan Radhakrishnan (siva-radhakrishnan)]

@Ryan Rossiter Can you please let us know if you still working on the patch you proposed ?

[OpenStack Infra (hudson-openstack)]

Change abandoned by Michael Still (<email address hidden>) on branch: master
Review: https://review.openstack.org/215308
Reason: This code hasn't been updated in a long time, and is in merge conflict. I am going to abandon this review, but feel free to restore it if you're still working on this.

[Markus Zoeller (markus_z) (mzoeller)]

Cleanup
=======

There are no open reviews for this bug report since more than 2 weeks.
To signal that to other contributors which might provide patches for
this bug, I switch the status from "In Progress" to "Confirmed" and
remove the assignee.
Feel free to add yourself as assignee and to push a review for it.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/388345

[OpenStack Infra (hudson-openstack)]

Change abandoned by Sean Dague (<email address hidden>) on branch: master
Review: https://review.openstack.org/388345
Reason: This review is > 4 weeks without comment, and is not mergable in it's current state. We are abandoning this for now. Feel free to reactivate the review by pressing the restore button and leaving a 'recheck' comment to get fresh test results.

[Sean Dague (sdague)]

There are no currently open reviews on this bug, changing the status back to the previous state and unassigning. If there are active reviews related to this bug, please include links in comments.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/629769

[OpenStack Infra (hudson-openstack)]

Change abandoned by "Stephen Finucane <email address hidden>" on branch: master
Review: https://review.opendev.org/c/openstack/nova/+/629769