Second create-tenant request fails with 401 Forbidden

Bug #916386 reported by Ewan Mellor
12
This bug affects 2 people
Affects Status Importance Assigned to Milestone
OpenStack Identity (keystone)
Fix Released
Medium
Dolph Mathews

Bug Description

I am using the SQLalchemy backend, MySQL 5.0.77, SQLAlchemy 0.6.5, Keystone latest. Note that MySQL and SQLAlchemy may be a bit old (I am using CentOS 5.5 as the base platform, with some backports) so if this turns out not to be a problem on a more modern platform then we may want to just ignore this bug.

I am making create-tenant requests (POST to /v2.0/tenants) as a global admin (i.e. no tenant, admin rights). The first one succeeds as expected. The second one fails with 401 Forbidden and the log message 'User <UUID> failed check - did not have role 1'.

What's happening is this:
1. the first create-tenant request is causing a new tenants row to be created in the database, with an autoincrementing integer in the tenants.id column.
2. When the second request comes in, the access check is calling keystone.backends.sqlalchemy.api.list_tenant_roles_for_user (via keystone.managers.grant.list_tenant_roles_for_user). That in turn is resolving user_roles.tenant_id (legitimately NULL) by calling api.TENANT.id_to_uid(None).
3. TENANT.id_to_uid has no shortcut for id=None. This leads to it making a database query equivalent to "SELECT uid FROM tenant WHERE id IS NULL". This is where things go wrong. This query _should_ return zero rows, because id IS NULL should always be false on an autoincremented integer. However, the query response includes the new tenants row created in step 1. In the database, id has been populated correctly, and is not NULL. SQLalchemy doesn't see that though. The session is clearly caching a row which has the new data submitted to the database, but _not_ the value in the auto-populated id column.
4. This results in TENANT.id_to_uid(None) returning the UID of the most recently created tenant. Which isn't good.
5. This state disappears immediately it seems -- subsequent calls to id_to_uuid(None) return None as expected.

I can see a couple of fixes for this:
1. Make id_to_uid(None) shortcut to return None, and therefore never hit the database in that case. This is a good idea for performance reasons anyway (queries of the form 'id IS NULL' should always return zero rows anyway). The reason I'm not proposing that straight away is that it feels like papering over a dangerous crack. There may be other situations that I haven't found yet where the database layer returns the wrong thing.
2. Change sqlalchemy.Driver._init_session_maker to set autocommit=False, not True. This fixes the problem. I have no idea what the impact would be on the general transactional correctness of the rest of the system though, so I'd need someone who knows how this layer is supposed to work to have a look at that one.

Ewan Mellor (ewanmellor)
description: updated
description: updated
Revision history for this message
Dolph Mathews (dolph) wrote :

First of all, I agree that a shortcut for None would be an improvement, but I'd still like to reproduce this issue if possible.

I'm in the process of setting up an environment similar to yours, but in the mean time, should a client-side test that amounts to the following be sufficient reproduce?

    self.create_tenant(assert_status=201)
    self.create_tenant(assert_status=201)

Changed in keystone:
assignee: nobody → Dolph Mathews (dolph)
milestone: none → essex-4
Revision history for this message
Dolph Mathews (dolph) wrote :

Additionally, what version of Python are you on?

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

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

Changed in keystone:
status: New → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to keystone (master)

Reviewed: https://review.openstack.org/3484
Committed: http://github.com/openstack/keystone/commit/d2e6f63fe107c17e344337b8db030733ed9bbb2e
Submitter: Jenkins
Branch: master

commit d2e6f63fe107c17e344337b8db030733ed9bbb2e
Author: Dolph Mathews <email address hidden>
Date: Thu Jan 26 15:29:34 2012 -0600

    Added shortcut for id=NULL queries (bug 916386)

    Change-Id: I9d7ae05f860d4a8f56bb9b7ab77221e201478393

Changed in keystone:
status: In Progress → Fix Committed
Joseph Heck (heckj)
Changed in keystone:
importance: Undecided → Medium
Thierry Carrez (ttx)
Changed in keystone:
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in keystone:
milestone: essex-4 → 2012.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.