Tenant role conflicts/overlaps can be a security issue

[Jason Rouault]

During the validate token call all the tenant roles (associated with the tenant scoped token) are returned to the middle-ware component and then passed along in the X_ROLES header to the OS service for consumption. In the case were more than one OS service are bound to the same tenant (e.g. Swift and Nova, or Nova 1 and Nova 2), a user with particular role for one service, lets just say the 'Admin' role will now also have the 'Admin' role in the second service. This is because roles are currently only scoped to the tenant level. The middle-ware just takes all returned tenant roles and stuffs them into the X_ROLES header regardless of the actual service the middle-ware is protecting. A quick fix to this problem would be to change the validate token interfaces (GET/HEAD /tokens/{tokenId}) to require a {serviceId} filter... so something like GET /tokens/{tokenId}?serviceId={serviceId}. The Keystone service would then only return roles in the response that are tied to that specific serviceId. If the serviceId was not provided, or was invalid, or no roles where found for that serviceId, then a 401 would be returned. Future Keystone work could consider allowing to filter down to the {endpointId}, but for such a change it would require a data model change to allow serviceIds to be defined on endpoint references.... Not to mention more API changes.

[Thierry Carrez (ttx)]

Subscribed Ziad, as PTL for Keystone.

If I understand correctly, this is more an unfortunate design choice (roles being scoped to tenants) than a security flaw ? If you confirm that the current behavior is by design, I agree that this design should be improved to allow roles scoped to tenant + service, but I see no reason to keep this private.

If this is not by design and roles should actually be scoped to tenant + service, then this is a vulnerability and we should keep the bug private until this is fixed. And if this affect stable/diablo as well, I suggest we delay 2011.3.1 to include the fix...

[Jason Rouault (jason.rouault)]

Designs can induce security defects... I think this should stay private until HP introduces the changes

[Thierry Carrez (ttx)]

Jason: oh, I agree with you -- my question stems from my ignorance of Keystone stuff. If Keystone roles are supposed to be universal rather than service-specific, then this is not a security issue. The admin of one is supposed to be the admin of the other. If roles are supposed to be service-specific, then this is definitely a serious security issue.

[Jason Rouault (jason.rouault)]

Thierry: roles are supposed to be service specific

[Thierry Carrez (ttx)]

Dolph, Ziad, please ack/comment

[Dolph Mathews (dolph)]

While this *was* by design, we definitely recognize that the design should be re-thought (especially with the introduction of Capabilities on the horizon).

Other than additional query parameters for ?service/endpoint, I don't see this as a change of spec, but rather just implementation behavior.

[Thierry Carrez (ttx)]

So Dolph, if I understand correctly, this was by design but could definitely evolve in the future towards service-specific rights ? That means it will probably be improved in Essex but should not be fixed in stable/diablo ?

If that interpretation is correct, I tend to not consider this a security hole but rather an architectural limitation that should be lifted in future releases... and would advocate for this bug being made public (but not before the original reporter is ok with it).

@Jason: let me know if you agree with Dolph's position, and when you're comfortable opening this up (which can be after HP evolves its implementation).

[Jason Rouault (jason.rouault)]

I think this needs to be addressed in diablo/stable for the servidId filter. HP is looking at committing the fix

[Thierry Carrez (ttx)]

Adding members of the stable maintenance team.

All: Should we delay 2011.3.1 to include that fix ?

[Dolph Mathews (dolph)]

As long as it's backwards compatible (i.e. optional) in stable/diablo, I don't see why we couldn't/shouldn't include it.

[Ziad Sawalha (ziad-sawalha)]

We did not consider the use case of having multiple instances of services or services that don't trust each other sharing one instance of Keystone. That was not an oversight, but a conscious decision to focus on exiting incubation and integrating with existing OpenStack services in known, common deployments.

That is not to negate the concern Jason brings up, which is valid.

We did, however, blueprint a mechanism to separate services so they can own their own endpoints and roles. That mechanism was not fully implemented until today (https://review.openstack.org/1864). Using the enforced ownership over service names, you can have services own their role prefixes (ex. nova:admin is owned by nova and glance cannot edit it. Similarly, glance:admin is created and owned by glance).

While all roles are still returned to the middleware, the service can identify the roles it owns by the prefix.

But I still look forward to the patch from HP. Meanwhile, I would consider this workaround a mitigation for the above risk. I will work on documenting it.

[Jason Rouault (jason.rouault)]

Ziad, the change to add a service prefix will only work if the service (e.g. Nova) knows how to consume the concatenated servicename:rolename. Today services do not know how to consume this (e.g. they just look for 'admin'). I assume this change is for Essex, and in the Essex work stream all the other services will change to understand the prefix?

[Guang Yee (guang-yee)]

Proposed Solution:

We'll be changing the validate token APIs to look for the mandatory request
parameter "serviceId" for scoped token validation. Specifically,

GET/HEAD /tokens/{tokenId}?[belongsTo=<tenantID>&][serviceId=<comma-separated service IDs>]

"serviceId" is a comma-separated list of service IDs.

The Keystone service will filter the roles with the given service IDs. If the serviceId
was not provided, or was invalid, or no roles were found for that serviceId, then a
401 would be returned.

Global roles (roles without any tenant association) will only be returned if
the global service ID is included in "serviceId". For example,

serviceId=<service ID>,<global service ID>

Keep in mind that the global service ID is just an special string denoting the need to return
the global roles. It is configurable in keystone.conf. For example,

global_service_id = global

The global service ID in middleware must match the one specified in keystone.conf inorder for
the global roles to return. Any mismatch will result in 401 since the global service ID does
not match any valid service in the backend.

MAKE NO MISTAKE, THIS CHANGE WILL NOT BE BACKWARD COMPATIBLE.

As mentioned above, If the serviceId was not provided, or was invalid, or no roles were found
for that serviceId, then a 401 would be returned.

Furthermore, we will be modifying the middleware and keystone-manage CLI to facilitate
service IDs.

Affected Middleware Components: auth_token.py, swift_auth.py, and quantum_auth_token.py.

auth_token.py and quantum_auth_token.py:

Users are required to specify the "service_ids" property in the auth_token section of the conf file.
"service_ids" is a comma-separated list if service IDs.

For example, if this is a Nova service instance and its service ID is 888, user will need to set it
in api-paste.ini.

[filter:authtoken]
...
service_ids = 888
...

To specify multiple service IDs.

[filter:authtoken]
...
service_ids = 888,999
...

By default, global roles (roles without tenant association) will not be return on validate token call.
To ask Keystone to return the global roles, user must specify the global service ID. For example,

[filter:authtoken]
...
service_ids = 888,999,global
...

As mentioned above, the global service ID is also configurable in keystone.conf. The global service ID
in middleware must match the one specified in keystone.conf inorder for the global roles to return.

swift_auth.py:

Same as auth_token.py exception it is using the "keystone_service_ids" property instead of "service_ids".

Affected CLI: keystone-manage

"keystone-manage role add" will not accept an optional service name parameter. For example,

keystone-manage role add nova_role nova_service

However, service name can be part of the role name. For example,

keystone-manage role add nove_service:nova_role

Though redundant, but user may also specify service name and prefix the role name with the service name.
For example,

keystone-manage role add nova_service:nova_role nova_service

However, if both service name and serive_name prefix are specified, they must match. Otherwise, ValueError
will be raised.

[Thierry Carrez (ttx)]

Reopening since there is no consensus on the fix yet

[Guang Yee (guang-yee)]

Please let me know if there are problems with the proposed solution. Otherwise, I'll be pushing the changes for review shortly.

[Jason Rouault (jason.rouault)]

yep, I like

[Guang Yee (guang-yee)]

After much consideration, we've decided to make the serviceId parameter "optional" in order to fulfill backward compatible requirement.

GET/HEAD /tokens/{tokenId}?[belongsTo=<tenantID>&][serviceId=<comma-separated service IDs>]

Keep in mind that the absence of serviceId, if one chooses to, means we still have tenant role conflicts/overlaps security problems as described in the bug.

Please let me know if there are objections. Otherwise, I'll start implementing the changes.

[Thierry Carrez (ttx)]

Ziad, Dolph: please comment !

[Dolph Mathews (dolph)]

The discussion has continued in the code review: https://review.openstack.org/#change,publish,2018

Recommend we mark this bug public now, as it is referenced by the commit message under review. The optional/required debate could use community feedback.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

commit fafc25b3b1d6f0610d31819d55c6c480cfd5bb5a
Author: Guang Yee <email address hidden>
Date: Mon Dec 26 15:44:56 2011 -0800

    Add HP-IDM extension to fix Bug 890411

    See

    https://github.com/openstack/keystone/raw/master/keystone/content/admin/HP-IDM-admin-devguide.pdf
    https://github.com/openstack/keystone/raw/master/keystone/content/admin/HP-IDM-admin.wadl

    and

    https://bugs.launchpad.net/keystone/+bug/890411

    for more details.

    incorporated feedbacks from https://review.openstack.org/#change,2624

    Change-Id: Ied659b3c010ac4efa04c073388bdf3f3a67a51f0

[OpenStack Infra (hudson-openstack)]

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

[Thierry Carrez (ttx)]

Obviously not being worked on

[Kurt Seifried (kseifried)]

This sounds like it might need a CVE id. Can anyone confirm yes/no if this needs a CVE?

[Dolph Mathews (dolph)]

The high level ask here was basically the subject of this Havana summit session: https://etherpad.openstack.org/havana-endpoint-filtering

It's being worked on, but not as a "bug."

[Kurt Seifried (kseifried)]

Ping, I assume this is not being treated as security flaw, can anyone confirm that? thanks.

[Morgan Fainberg (mdrnstm)]

This stems from a design decision, and isn't really a bug. This is more of a lack of a feature. This should be written up as a specification: https://git.openstack.org/cgit/openstack/keystone-specs and treated like a new feature. Marking this bug as "wont fix"

[Morgan Fainberg (mdrnstm)]

Correction, marked as Invalid.