Sanitize passwords when logging payload in wsgi for API Extensions

Bug #1247217 reported by Brent Tang
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Compute (nova)
Fix Released
High
Lance Bragstad
oslo-incubator
Fix Released
High
Christopher Yeoh

Bug Description

The fix for bug 1231263 ( https://bugs.launchpad.net/nova/+bug/1231263 ) addressed not logging the clear-text password in the nova wsgi.py module for the adminPass attribute for the Server Change Password REST API, but this only addressed that specific attribute. Since Nova has support for the ability to add REST API Extensions (in the contrib directory), there could any number of other password-related attributes in the request/response body for those additional extensions.

Although it would not be possible to know all of the various sensitive attributes that these API's would pass in the request/response (the only way to totally eliminate the exposure would be to not log the request/response which is useful for debugging), I would like to propose a change similar to the one that was made in keystone (under https://bugs.launchpad.net/keystone/+bug/1166697) to mask the password in the log statement for any attribute that contains the "password" sub-string in it.

The change would in essence be to update the _SANITIZE_KEYS / _SANITIZE_PATTERNS lists in the nova/api/openstack/wsgi.py module to include a pattern for the "password" sub-string.

Also, for a slight performance benefit, it may be useful to put a check in to see if debug logging level is enabled around the debug statement that does the sanitize call (since the request/response bodies could be fairly large and wouldn't want to take the hit to do the pattern matches if debug isn't on).

Tags: api security
Brent Tang (btang-d)
tags: added: security
Matt Riedemann (mriedem)
tags: added: api
Changed in nova:
status: New → Confirmed
Revision history for this message
Davanum Srinivas (DIMS) (dims-v) wrote :

Go for it. +1 :)

Revision history for this message
Lance Bragstad (lbragstad) wrote :

I linked the bug # in my commit message but it hasn't shown up yet?

Just FYI for everyone following:

https://review.openstack.org/#/c/55193/

Matt Riedemann (mriedem)
Changed in oslo:
status: New → In Progress
Changed in oslo:
assignee: nobody → Lance Bragstad (ldbragst)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to oslo-incubator (master)

Reviewed: https://review.openstack.org/55193
Committed: http://github.com/openstack/oslo-incubator/commit/76b0cd10c414ae71e6f041adb431460b3337c63d
Submitter: Jenkins
Branch: master

commit 76b0cd10c414ae71e6f041adb431460b3337c63d
Author: Lance Bragstad <email address hidden>
Date: Mon Nov 4 22:35:36 2013 +0000

    Add mask password impl from other projects

    A couple of different projects have an implementation for 'masking'
    passwords in logs and wsgi output.

    This seems like something that should go into Oslo-incubator as a common
    implementation. This implementation is based off the following two
    implementations in Keystone and Nova:

      https://review.openstack.org/#/c/26487/12
      https://review.openstack.org/#/c/49664/

    Partial-Bug: #1247217

    Change-Id: I2b1bbb4d3ddaa163573b685b3e3a5e8a3977e390

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

Nove oslo sync patch is here: https://review.openstack.org/#/c/56059/

Changed in nova:
status: Confirmed → In Progress
assignee: nobody → Lance Bragstad (ldbragst)
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/56069

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to oslo-incubator (master)

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

Changed in oslo:
assignee: Lance Bragstad (ldbragst) → Christopher Yeoh (cyeoh-0)
Mark McLoughlin (markmc)
Changed in nova:
importance: Undecided → High
Changed in oslo:
importance: Undecided → High
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to oslo-incubator (master)

Reviewed: https://review.openstack.org/56154
Committed: http://github.com/openstack/oslo-incubator/commit/25c5854d67547ee177cc84341eaf9c8eb7c10834
Submitter: Jenkins
Branch: master

commit 25c5854d67547ee177cc84341eaf9c8eb7c10834
Author: Chris Yeoh <email address hidden>
Date: Wed Nov 13 16:35:36 2013 +1030

    Adds admin_password as key to be sanitized when logging

    The Nova V3 API is moving to use admin_password instead of
    admin_pass (both are currently in use). So adding admin_password
    as a key that the value should be sanitized for when
    logging.

    Change-Id: I0eb66c948516624db8b770498ae541025545d981
    Partial-Bug: #1247217

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to nova (master)

Reviewed: https://review.openstack.org/56059
Committed: http://github.com/openstack/nova/commit/138f24dea3b1fa34a39eaa0e937b19657b287ccc
Submitter: Jenkins
Branch: master

commit 138f24dea3b1fa34a39eaa0e937b19657b287ccc
Author: Lance Bragstad <email address hidden>
Date: Thu Nov 14 15:40:05 2013 +0000

    Sync log.py from Oslo-incubator

    This change syncs log.py and it's dependencies from Oslo-incubator to
    include the password sanitation fix that landed in Oslo with commit
    76b0cd10c414ae71e6f041adb431460b3337c63d.

    The following is a list of the change IDs that this
    commit is bringing in from the last sync of Oslo-incubator with
    respect to the files we are touching.

    2b40a10 python3: Fix UserString import
    c331e74 Enable multiple translation domains for gettextutils
    89369c3 gettextutils: port to Python 3
    6d49bca Translate all substitution elements of a Message object
    3970d46 Fix typos in oslo
    88db9c8 When translating if no locale is given use default locale
    53ebd30 python3: use six.text_types for unicode()
    25c5854 Adds admin_password as key to be sanitized when logging
    2251cb5 Do not name variables as builtins
    04c1b5a Type check for Message param to avoid AttributeError

    Change-Id: Ie4d5929604e3fc057ea09edbbf3adf16f8839a4e
    Partial-Bug: #1247217

Changed in nova:
status: In Progress → Fix Committed
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/56069
Committed: http://github.com/openstack/nova/commit/187140c2fcafb56752f0160832607e374a4a94ff
Submitter: Jenkins
Branch: master

commit 187140c2fcafb56752f0160832607e374a4a94ff
Author: Lance Bragstad <email address hidden>
Date: Thu Nov 14 15:44:15 2013 +0000

    Use password masking utility provided in Oslo

    Implement the mask_password function provided in Oslo-incubator's
    log.py. Instead of having a specific version in Nova different from
    other projects that are essentially doing the same thing.

    Change-Id: I7e04b7d31d4d6959b17b1da9654553042eec70f1
    Closes-Bug: #1247217

Changed in nova:
milestone: none → icehouse-2
Revision history for this message
Doug Hellmann (doug-hellmann) wrote :

It looks like the change landed in oslo, so I marked this as committed there.

Changed in oslo:
status: In Progress → Fix Committed
milestone: none → icehouse-2
Thierry Carrez (ttx)
Changed in nova:
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in oslo:
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in oslo:
milestone: icehouse-2 → 2014.1
Thierry Carrez (ttx)
Changed in nova:
milestone: icehouse-2 → 2014.1
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.