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

Bug #1487038 reported by Matt Riedemann
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Compute (nova)
In Progress
Low
Unassigned
oslo.messaging
Fix Released
Medium
Ryan Rossiter

Bug Description

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.

Matt Riedemann (mriedem)
Changed in nova:
status: New → Confirmed
importance: Undecided → Low
Revision history for this message
Ryan Rossiter (rlrossit) wrote :

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?

Revision history for this message
Matt Riedemann (mriedem) wrote :

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

Revision history for this message
Matt Riedemann (mriedem) wrote :

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

Changed in oslo.messaging:
status: New → Confirmed
importance: Undecided → Medium
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to nova (master)

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

Changed in nova:
assignee: nobody → Ryan Rossiter (rlrossit)
status: Confirmed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to oslo.messaging (master)

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

Changed in oslo.messaging:
assignee: nobody → Ryan Rossiter (rlrossit)
status: Confirmed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to oslo.messaging (master)

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

Changed in oslo.messaging:
status: In Progress → Fix Committed
Changed in oslo.messaging:
milestone: none → 2.4.0
status: Fix Committed → Fix Released
Revision history for this message
Sivasathurappan Radhakrishnan (siva-radhakrishnan) wrote :

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on nova (master)

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.

Revision history for this message
Markus Zoeller (markus_z) (mzoeller) wrote :

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.

Changed in nova:
status: In Progress → Confirmed
assignee: Ryan Rossiter (rlrossit) → nobody
Changed in nova:
assignee: nobody → Sivasathurappan Radhakrishnan (siva-radhakrishnan)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to nova (master)

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

Changed in nova:
status: Confirmed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on nova (master)

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.

Revision history for this message
Sean Dague (sdague) wrote :

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.

Changed in nova:
status: In Progress → Confirmed
assignee: Sivasathurappan Radhakrishnan (siva-radhakrishnan) → nobody
tags: added: notifications
tags: added: low-hanging-fruit
Changed in nova:
assignee: nobody → Hesam Chobanlou (hesamchobanlou)
Changed in nova:
status: Confirmed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to nova (master)

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

Changed in nova:
assignee: Hesam Chobanlou (hesamchobanlou) → nobody
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on nova (master)

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

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers