OpenStack Identity (keystone)

user_enabled_invert does not properly handle string values

Bug #1376053 reported by Nathan Kinder on 2014-09-30

This bug affects 1 person

Affects		Status	Importance	Assigned to	Milestone
	OpenStack Identity (keystone)	Fix Released	Medium	Nathan Kinder	OpenStack Identity (keystone) 2014.2 "juno"

Bug Description

The user_enabled_invert setting is supposed to invert the meaning of True/False for the user enabled attribute. This makes "lock" attributes useful, where "False" indicates that an account is not locked.

The invert logic expects that we have a bool type that we then invert using 'not' in this snippet of code from UserApi._ldap_res_to_model:

--------------------------------------------------------------------
        elif self.enabled_invert and not self.enabled_emulation:
            enabled = obj.get('enabled', self.enabled_default)
            obj['enabled'] = not enabled
--------------------------------------------------------------------

The problem is that we get a string type from the default value, and a bool type from LDAP. Evaluating a string with 'not' will be False for any non-empty string. This means that we will fail to invert a string of "False" that is inherited from the default setting if no value is returned from LDAP, leading to accounts being inadvertently disabled. This code needs to handle converting a str type to bool before inverting the value.

See original description

Nathan Kinder (nkinder) on 2014-09-30

tags:	added: juno-rc-candidate
Changed in keystone:
assignee:	nobody → Nathan Kinder (nkinder)
status:	New → In Progress
summary:	- user_enabled_invert does notproperly handle string values + user_enabled_invert does not properly handle string values

Morgan Fainberg (mdrnstm) on 2014-09-30

tags:	added: juno-rc-potential removed: juno-rc-candidate
Changed in keystone:
importance:	Undecided → Medium

Revision history for this message

OpenStack Infra (hudson-openstack) wrote on 2014-10-01: Fix proposed to keystone (master)

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

OpenStack Infra (hudson-openstack) on 2014-10-01

Changed in keystone:
assignee:	Nathan Kinder (nkinder) → Morgan Fainberg (mdrnstm)
assignee:	Morgan Fainberg (mdrnstm) → Nathan Kinder (nkinder)
assignee:	Nathan Kinder (nkinder) → Morgan Fainberg (mdrnstm)

Revision history for this message

OpenStack Infra (hudson-openstack) wrote on 2014-10-01: Fix proposed to keystone (proposed/juno)

Fix proposed to branch: proposed/juno
Review: https://review.openstack.org/125257

Nathan Kinder (nkinder) on 2014-10-01

Changed in keystone:
assignee:	Morgan Fainberg (mdrnstm) → Nathan Kinder (nkinder)

Nathan Kinder (nkinder) on 2014-10-01

description:

updated

Revision history for this message

OpenStack Infra (hudson-openstack) wrote on 2014-10-01: Fix merged to keystone (master)

Reviewed: https://review.openstack.org/125243
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=50a6312ffa1f636bd74c98d9938ea4083bed2768
Submitter: Jenkins
Branch: master

commit 50a6312ffa1f636bd74c98d9938ea4083bed2768
Author: Nathan Kinder <email address hidden>
Date: Tue Sep 30 17:36:22 2014 -0700

Handle default string values when using user_enabled_invert

    When the user_enabled_invert setting is being used, values returned
    from LDAP are ultimately converted to a bool type when we reach the
    inversion logic. If the user_enabled_default value is used due to
    no value being returned from LDAP, the type is a string. This causes
    the inversion logic to be evaluated incorrectly, as 'not' will return
    False for any non-empty string. This results in disabled accounts
    that should be enabled.

Change-Id: Id7b024c12815748305458ca05fc8f8a6324c1908
Closes-bug: #1376053

Changed in keystone:
status:	In Progress → Fix Committed

Revision history for this message

OpenStack Infra (hudson-openstack) wrote on 2014-10-02: Fix merged to keystone (proposed/juno)

Reviewed: https://review.openstack.org/125257
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=92dffc1aff66b428de20c3087e974c90bbc9f244
Submitter: Jenkins
Branch: proposed/juno

commit 92dffc1aff66b428de20c3087e974c90bbc9f244
Author: Nathan Kinder <email address hidden>
Date: Tue Sep 30 17:36:22 2014 -0700

Handle default string values when using user_enabled_invert

    Change-Id: Id7b024c12815748305458ca05fc8f8a6324c1908
    Closes-bug: #1376053
    (cherry picked from commit 50a6312ffa1f636bd74c98d9938ea4083bed2768)

Thierry Carrez (ttx) on 2014-10-06

Changed in keystone:
milestone:	none → juno-rc2
status:	Fix Committed → Fix Released
tags:	removed: juno-rc-potential

Thierry Carrez (ttx) on 2014-10-16

Changed in keystone:
milestone:	juno-rc2 → 2014.2

Revision history for this message

OpenStack Infra (hudson-openstack) wrote on 2014-10-16: Fix proposed to keystone (master)

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

Revision history for this message

OpenStack Infra (hudson-openstack) wrote on 2015-06-10: Fix merged to keystone (master)

Download full text (5.5 KiB)

Reviewed: https://review.openstack.org/128930
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=b788121927abe04a1bbbd1d47eb04cd8d9311904
Submitter: Jenkins
Branch: master

commit ef8d9aa4e1ca3b5465a5bba0cdb6dcb9be1fe9ca
Author: Dolph Mathews <email address hidden>
Date: Tue Oct 7 14:35:28 2014 +0000

updated translations

this boils down to:

    $ python setup.py extract_messages
    $ python setup.py update_catalog --no-fuzzy-matching \
      --ignore-obsolete=true
    $ source \
      ../../openstack-infra/project-config/jenkins/scripts/common_translation_update.sh
    $ setup_loglevel_vars
    $ cleanup_po_files keystone

Change-Id: I2a03f3d7eebe0be0250d4834256dfa3c634dbb48

commit 079c6ad6c911226251fa2a601a27296cfe15e0b3
Author: Henry Nash <email address hidden>
Date: Sun Sep 28 11:16:26 2014 +0100

Ensure sql upgrade tests can run with non-sqlite databases.

    This patch fixes the issues that were preventing the running of
    live sql upgrade tests (either by running test_sql_upgrade directly
    or via test_sql_livetest), namely:

    - Dropping the tables that were in existence before the current
      scope of migration in an order that is FK friendly
    - Fixing an issue where the tables were being dropped in the
      wrong order in the downgrade of federation
    - Ensuring we don't hold sessions open over upgrade/downgrade
      steps in our test methods

Limitations:

- This patch has not been tested with DB2

    Closes-Bug: 1363047
    Closes-Bug: 1375937
    Change-Id: Ied4741a9646b57bc6f2ddcdc8a380ea55b2a9634

commit 4ae1879a79e338e7323935fd17896ba8a4e84fb9
Author: David Stanek <email address hidden>
Date: Fri Oct 3 18:52:54 2014 +0000

Validates controller methods exist when specified

    It was possible to specify an invalid controller method in a router.
    This will not cause an error until runtime. This change catches the
    error much earlier in the application lifecycle. In fact with this
    change errors should not be able to pass unit tests even if there is
    no specific test for the behavior.

Related-bug: #1377304
Change-Id: Icc5646c143a234127a8b4ac8a74342ef3dca7e80

commit 5caf29ad5d90a65d3b10dc55bb101c96b543e4f8
Author: David Stanek <email address hidden>
Date: Fri Oct 3 20:00:30 2014 +0000

Fixes an error deleting an endpoint group project

    Deleting a endpoint group project fails because the router specifies
    a controller method that doesn't exist. This returns a 500 error to
    the user for what should be a successful operation.

Change-Id: I3b91d8023d31555893fb944da73633a69d8e286f
Closes-bug: #1377304

commit c64eae8678327067ef22099e846d927bccb4a804
Author: Brant Knudson <email address hidden>
Date: Wed Oct 1 11:11:21 2014 -0500

Fix tests comparing tokens

    There were tests that verified that the PKI token body could be
    encrypted with CMS and compared to the token ID in the response.
    This test isn't safe because the token body may be different than
    the token encrypted with CMS since the order of items in the dict
    can change.
    ...

Reviewed:  https://review.openstack.org/128930
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=b788121927abe04a1bbbd1d47eb04cd8d9311904
Submitter: Jenkins
Branch:    master

commit ef8d9aa4e1ca3b5465a5bba0cdb6dcb9be1fe9ca
Author: Dolph Mathews <dolph.mathews@gmail.com>
Date:   Tue Oct 7 14:35:28 2014 +0000

updated translations
    
    this boils down to:
    
    $ python setup.py extract_messages
    $ python setup.py update_catalog --no-fuzzy-matching \
      --ignore-obsolete=true
    $ source \
      ../../openstack-infra/project-config/jenkins/scripts/common_translation_update.sh
    $ setup_loglevel_vars
    $ cleanup_po_files keystone
    
    Change-Id: I2a03f3d7eebe0be0250d4834256dfa3c634dbb48

commit 079c6ad6c911226251fa2a601a27296cfe15e0b3
Author: Henry Nash <henryn@linux.vnet.ibm.com>
Date:   Sun Sep 28 11:16:26 2014 +0100

Ensure sql upgrade tests can run with non-sqlite databases.
    
    This patch fixes the issues that were preventing the running of
    live sql upgrade tests (either by running test_sql_upgrade directly
    or via test_sql_livetest), namely:
    
    - Dropping the tables that were in existence before the current
      scope of migration in an order that is FK friendly
    - Fixing an issue where the tables were being dropped in the
      wrong order in the downgrade of federation
    - Ensuring we don't hold sessions open over upgrade/downgrade
      steps in our test methods
    
    Limitations:
    
    - This patch has not been tested with DB2
    
    Closes-Bug: 1363047
    Closes-Bug: 1375937
    Change-Id: Ied4741a9646b57bc6f2ddcdc8a380ea55b2a9634

commit 4ae1879a79e338e7323935fd17896ba8a4e84fb9
Author: David Stanek <dstanek@dstanek.com>
Date:   Fri Oct 3 18:52:54 2014 +0000

Validates controller methods exist when specified
    
    It was possible to specify an invalid controller method in a router.
    This will not cause an error until runtime. This change catches the
    error much earlier in the application lifecycle. In fact with this
    change errors should not be able to pass unit tests even if there is
    no specific test for the behavior.
    
    Related-bug: #1377304
    Change-Id: Icc5646c143a234127a8b4ac8a74342ef3dca7e80

commit 5caf29ad5d90a65d3b10dc55bb101c96b543e4f8
Author: David Stanek <dstanek@dstanek.com>
Date:   Fri Oct 3 20:00:30 2014 +0000

Fixes an error deleting an endpoint group project
    
    Deleting a endpoint group project fails because the router specifies
    a controller method that doesn't exist. This returns a 500 error to
    the user for what should be a successful operation.
    
    Change-Id: I3b91d8023d31555893fb944da73633a69d8e286f
    Closes-bug: #1377304

commit c64eae8678327067ef22099e846d927bccb4a804
Author: Brant Knudson <bknudson@us.ibm.com>
Date:   Wed Oct 1 11:11:21 2014 -0500

Fix tests comparing tokens
    
    There were tests that verified that the PKI token body could be
    encrypted with CMS and compared to the token ID in the response.
    This test isn't safe because the token body may be different than
    the token encrypted with CMS since the order of items in the dict
    can change.
    
    The fix is to change the test to decode the PKI token ID and
    compare that to the response body JSON instead.
    
    Change-Id: Icc649b96071ff084d5c76f2ea2bcf3ecb08a0351
    (cherry picked from commit 83394ba71ba06e9899aaa62f211919075f2a3d91)

commit 508e904c7410de841b54e4744e43c7470e1f13d0
Author: Nathan Kinder <nkinder@redhat.com>
Date:   Tue Sep 30 08:26:35 2014 -0700

Convert unicode to UTF8 when calling ldap.str2dn()
    
    The string passed to ldap.str2dn() needs to be UTF8 encoded, otherwise
    we encounter UnicodeEncodeError exceptions.
    
    This patch adds UTF8 encoding where it was missing for ldap.str2dn()
    calls as well as some unit tests to cover the functions that were
    lacking this encoding step.
    
    Change-Id: I96e718f1d43f4aad272ca4990d3905a5adf1f07d
    Closes-bug: #1375139
    (cherry picked from commit 09d38008aa0e7f8c6802088c807dd0ad6ba80e10)

commit eefa0feaf5aa933fb9cb6313eb58ad3b0cb501fc
Author: Nathan Kinder <nkinder@redhat.com>
Date:   Tue Sep 30 07:39:33 2014 -0700

Fix parsing of emulated enabled DN
    
    If a non-default emulated enabled DN is specified in configuration,
    the DN structure returned from python-ldap is incorrectly converted
    to a string.  This leads to an index error when we attempt to extract
    the RDN attribute and value.
    
    This patch removes the incorrect string conversion and instead does
    the proper conversion on the RDN strings that we extract.
    
    Change-Id: I8f0c4594cfa9a41e1875870c3eb63fae32c8c041
    Resolves-bug: #1375772
    (cherry picked from commit 5380ddaadb0ce34b866b097cb6ac1396d2b30100)

commit 92dffc1aff66b428de20c3087e974c90bbc9f244
Author: Nathan Kinder <nkinder@redhat.com>
Date:   Tue Sep 30 17:36:22 2014 -0700

Handle default string values when using user_enabled_invert
    
    When the user_enabled_invert setting is being used, values returned
    from LDAP are ultimately converted to a bool type when we reach the
    inversion logic.  If the user_enabled_default value is used due to
    no value being returned from LDAP, the type is a string.  This causes
    the inversion logic to be evaluated incorrectly, as 'not' will return
    False for any non-empty string.  This results in disabled accounts
    that should be enabled.
    
    Change-Id: Id7b024c12815748305458ca05fc8f8a6324c1908
    Closes-bug: #1376053
    (cherry picked from commit 50a6312ffa1f636bd74c98d9938ea4083bed2768)

Report a bug

This report contains Public information

Everyone can see this information.

You are

Subscribing...

Edit bug mail

Other bug subscribers

Remote bug watches

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