keystoneauth middleware not domain aware (keystone v3 issue)

I kind of felt it was going to cause trouble when reviewing the patch that was adding it https://review.openstack.org/#/c/22820/ oh well past is past.

I think we should just disable it in a way, I am still not sure if there is a use case for *:username for even in v2, did you guys had some in HP ?

If there is use cases, I think updating Swift middleware to be domain aware when running v3 may be the way to go.

Submitted as bug 1299153, further complications for Swift:

--------------
There is a problem with the Swift ACL code -- it is not domain-aware. It assumes that user names are unique. I've filled that as bug 1299146.

However, even if Swift's ACL code becomes domain-aware, a deployer might enable multiple domains, but use the v2 protocols with auth_token in the Swift pipeline.

A possible solution is to add the ability for Keystone to enforce unique user names. If Swift knows this it is enabled, it can process ACLs safely as-is without worrying about mapping users do domains.
--------------

Merging the two issues (creating Keystone and Swift tasks) since those are two facets of the same problem.

Point of clarification: the vulnerability is only exposed when authtoken middleware is configured to use keystone v3 API.

When authtoken uses keystone V2 API, users in domains other than the default are not authenticated.

When authtoken is configured to use keystone V3 API, users in other domains may be authenticated, but keystoneauth in swift is unaware of the multiple domains when matching users to ACL entries.

Here is a patch that remove completely the ACL, there is as well the *:* ACL affecting us which this patch removes as well.

The advisory would need to be expanded to say that we are removing those *:*

Spoke with Alistair on the phone this morning and this patch doesn't fix it as I am missing the fact that tenant as well are not unique between domains. Alistair is working on it and will keep us posted.

This probably will need to be documented as an unsupported combination in icehouse, so this bug will probably soon be made public. Yell if you oppose.

I'd like to add this warning in the install guide. Let me know if that location is sufficient and if you accept this wording:

You must configure and run Identity API v 2.0 with Object Storage so that access control lists protect access to objects.

Maybe add:

> You must configure and run Identity API v 2.0 with Object Storage so that access control lists protect access to objects.
As a consequence you cannot use domain based tenants created via the V3.0 identity API with Object Storage. Note that
Current python clients for most services also do not yet support authentication for domain based tenants.

> -----Original Message-----
> From: <email address hidden> [mailto:<email address hidden>] On Behalf
> Of Anne Gentle
> Sent: 08 April 2014 17:07
> To: Day, Phil
> Subject: [Bug 1299146] Re: keystoneauth middleware not domain aware
> (keystone v3 issue)
>
> I'd like to add this warning in the install guide. Let me know if that location is
> sufficient and if you accept this wording:
>
> You must configure and run Identity API v 2.0 with Object Storage so that
> access control lists protect access to objects.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1299146
>
> Title:
> keystoneauth middleware not domain aware (keystone v3 issue)
>
> Status in OpenStack Identity (Keystone):
> Incomplete
> Status in OpenStack Security Advisories:
> Incomplete
> Status in OpenStack Object Storage (Swift):
> New
>
> Bug description:
>
> With Keystone V3, user's in two different domains can have the same
> name. Swift ACLs contian the "bare" username. Example:
>
> X-Container-Read: *:sally
>
> This is ok with Keystone V2, where all users have unique names.
> However, with Keystone V3, there could be a "sally" in domain "A" and
> a different "sally" in domain "B". As-is, this allows information to
> leak to unauthorized/unintended users. Hence this bug is marked as a
> security vulnerability.
>
> Note: the solution is not to simply to continue use the v2 API in
> auth_token. The problem is at the Keystone server side; specifically
> if you allow multiple domains, user name clashes can occur. With the
> v2 API, auth_token does not know what the domain name is. With the V3
> API auth_token, it adds X-DOMAIN-NAME and X-DOMAIN-ID to the
> environment. However, since keystoneauth does not look at these
> variables, using V3 in auth_token does not add anything.
>
> We could extend the ACL syntax to assoicate the domain with the
> intended user. For example: sally@A. However, exisitng ACLs must be
> supported. However, swift does not know the domain associated with an
> account...making it difficult to map usernames in the ACL to a domain.
>
> Even if the Swift middleware is made domain-aware, we need to warn
> deployers that they must not combine V3 multiple domains with V2
> auth_token in the pipeline. A solution that would allow that is to
> have an option in Keystone to enforse unique usernames across domains.
> With that option, there would be no need to worry about domains when
> processing Swift ACLs. I'll file a Keystone bug for this suggestion.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/keystone/+bug/1299146/+subscriptions

+1 To Phi's comment!

In terms of fixing this, I think the following is needed in order to enable keystone v3 with swift. Note that the proposed changes would be confined to the keystoneauth middleware and apply only when operating in a keystone v3 regime.

1. Prohibit the creation of new swift container ACLs using the 'legacy' keystone cross-tenant ACL syntax (i.e. <tenant>:<user>). Usernames are no longer unique across v3 keystone domains and this syntax is not capable of expressing domain qualified user names because there are no reserved characters that can be used as a delimiter between the domain and user parts e.g. <user>@<domain> is not suitable.

2. Legacy ACLs that *already exist* on containers created under the v2 keystone regime can still be applied IFF the requesting user and the account are in the default v3 domain. The account can be assumed to be in the default domain because the legacy ACL must pre-date v3 migration, and the user's domain is added to the request context by authtoken middleware when validating against a v3 API.

3. All new ACLs, and modifications to existing ACLs, must be made using a new ACL syntax that supports domain-qualified user names. Any container POST having an X-Container-[Read|Write] that does not conform to the new ACL syntax should be rejected. (With resellerAdmin requests it may not be possible for swift to determine that the target of a POST is in the default domain and so we must prevent creation of any further legacy ACLs in order to enforce the assumption under item 2).

4. A new ACL format should be provided using JSON to express explicit user, project and domain name/id parameters. The syntax could be similar to swift's account ACLs.

For existing operators that have a need to adopt v3 keystone and need to map *existing* accounts to multiple v3 domains, a keystoneauth config option could be provided to override the above prohibitions and continue to apply legacy ACLs regardless of user/tenant domain. In doing so the operator should be sure that usernames are guaranteed to be globally unique in their system otherwise they remain vulnerable to the original issue.

We are working on a patch.

Any modifications to ACL syntax will have to apply only to Keystone-managed accounts. Swift can use multiple simultaneous auth systems, and it would be a major inconvenience for folks not using Keystone to be forced into some new ACL syntax in order to solve a problem they don't even have.

I think the problem here is not to do with how to add domains but a mis-understanding of the scoping of username.

Yes username is scoped to a domain, user_id is a global unique identifier (across domains). Swift (and others) shouldn't need to do anything about becoming domain aware but it should be using the user_id to identify it's users not username. The same thing applies to projects (was tenants). The project name is only unique within a domain, the project id should be globally unique.

Documented gap, not a vulnerability.

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

Much of the thinking I outlined above (#10) is motivated by attempting to maintain the name based ACLs that Swift has supported to date, and providing backwards compatibility with those when an existing system migrates to v3 keystone.

As Jamie points out (#12) , restricting identification to id's (not name's) in the swift ACLs will fix the immediate issue. I've taken that approach in this patch https://review.openstack.org/86430. The change in behavior will only occur when tokens are validated using a v3 keystone API.

This obviously reduces usability w.r.t. v2 for swift clients accustomed to citing tenant/user names - that could be addressed by introducing some new ACL syntax (keystone specific) in further patches. And there is the issue of how we deal with existing ACLs using names when migrating a system from v2 to v3.

So reading #12 it seems that id is the way to go - usability be damned. But I'm confused on the syntax with the double ids?

If we wanted to try and keep names, something like domain:project or domain:user would probably uniquely identify the tenant_id or user_id today - but it'll break down once Keystone adds super-conglomerates or universes concepts above domains.

But as long as you get in one id - that should be enough right?

Does an identifier like tenant_id:user_id even make sense if the user_id matches by the tenant_id doesn't? Shouldn't tenant_id:name be good enough to identify a unique user or even just user_id?

@Clay (#16) Just user_id is fine to identify a unique user, and that is supported in the ACL with the wildcard syntax *:userID.

The syntax ACL 'tenantX:userY' does not mean 'allow userY who is contained within tenantX'. Users and tenants are independent of each other (its possibly easier to comprehend in terms of users and projects rather than 'tenants'). So tenant_id:name is not sufficient as an ACL because the tenant_id does not disambiguate the user name.

Rather, the tenantX:userY syntax specifies two conditions for container access: 1) the user is userY and 2) the user's token is scoped for tenantX. (Either condition can be forced true with a wildcard.) Since we can have more than one tenant *named* 'acme' we need to tighten up that part of the syntax to require an tenant ID. And since we can have more than one user named 'bob' we need to tighten up the other part of the syntax to require a user ID.

HTH.

Also, users does not actually contained within a tenant. Users are contained within a domain. User linked to a tenant/project via role assignment. For example, userX is granted the Operator role for tenantY. However, the way the ACL is structured today, role is not part of the consideration when creating a cross-tenant ACL. For example, grant read access to containerZ for anyone who as Reseller role for tenantY.

k, got it - thanks.

Reviewed: https://review.openstack.org/86430
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=a4f634bd898603225d2218eec220b61a8fd9865c
Submitter: Jenkins
Branch: master

commit a4f634bd898603225d2218eec220b61a8fd9865c
Author: anc <email address hidden>
Date: Fri Mar 28 02:46:08 2014 +0000

    Restrict keystone cross-tenant ACLs to IDs

    The keystoneauth middleware supports cross-tenant access
    control using the syntax <tenant>:<user> in container ACLs,
    where <tenant> and <user> may currently be either a unique
    id or a name. As a result of the keystone v3 API introducing
    domains, names are no longer globally unique and are only
    unique within a domain. The use of unqualified tenant and
    user names in this ACL syntax is therefore not 'safe' in a
    keystone v3 environment.

    This patch modifies keystoneauth to restrict cross-tenant
    ACL matching to use only ids for accounts that are not in
    the default domain. For backwards compatibility,
    names will still be matched in ACLs when both the requesting
    user and tenant are known to be in the default domain AND the
    account's tenant is also in the default domain (the default
    domain being the domain to which existing tenants are
    migrated).

    Accounts existing prior to this patch are assumed to be for
    tenants in the default domain. New accounts created using a
    v2 token scoped on the tenant are also assumed to be in the
    default domain. New accounts created using a v3 token scoped
    on the tenant will learn their domain membership from the
    token info. New accounts created using any unscoped token,
    (i.e. with a reselleradmin role) will have unknown domain
    membership and therefore be assumed to NOT be in the default
    domain.

    Despite this provision for backwards compatibility, names
    must no longer be used when setting new ACLs in any account,
    including new accounts in the default domain.

    This change obviously impacts users accustomed to specifying
    cross-tenant ACLs in terms of names, and further work will be
    necessary to restore those use cases. Some ideas are
    discussed under the bug report. With that caveat, this patch
    removes the reported vulnerability when using
    swift/keystoneauth with a keystone v3 API.

    Note: to observe the new 'restricted' behaviour you will need
    to setup keystone user(s) and tenant(s) in a non-default domain
    and set auth_version = v3.0 in the auth_token middleware config
    section of proxy-server.conf. You may also benefit from the
    keystone v3 enabled swiftclient patch under review here:
    https://review.openstack.org/#/c/91788/

    DocImpact

    blueprint keystone-v3-support

    Closes-Bug: #1299146

    Change-Id: Ib32df093f7450f704127da77ff06b595f57615cb

Related fix proposed to branch: master
Review: https://review.openstack.org/121493

Reviewed: https://review.openstack.org/121493
Committed: https://git.openstack.org/cgit/openstack/openstack-manuals/commit/?id=6781e6b5b3036f353d6c1c9f8c07f32e824fc4cc
Submitter: Jenkins
Branch: master

commit 6781e6b5b3036f353d6c1c9f8c07f32e824fc4cc
Author: Alistair Coles <email address hidden>
Date: Mon Sep 15 10:54:16 2014 +0100

    Remove warning to only use swift with keystone v2

    The warning to only use swift with the keystone v2 API
    should be removed now that a fix has been merged to make
    swift aware of v3 domains.

    Fix in swift is https://review.openstack.org/#/c/86430

    Change-Id: Iac172a48513dee25aa2d0830257d46f25f0d5648
    Related-bug: #1299146

Fix proposed to branch: feature/ec
Review: https://review.openstack.org/122541

Reviewed: https://review.openstack.org/122541
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=18901494e9b4b47fb181a5ead8ed82738db8563f
Submitter: Jenkins
Branch: feature/ec

commit fc5cee5f05692f7e6dd5ad5a6d0ae682dd4bf3e0
Author: Christian Schwede <email address hidden>
Date: Mon Sep 15 17:22:54 2014 +0000

    Allow filtering by region in swift-recon

    The option "-r" is already used, thus only "--region" is used to specify
    filter by region.

    Change-Id: If769f2f3191c202933b03b48fe0f22b7c94a4dd6
    Closes-Bug: 1369583

commit 423ac74e888dcd693129100e0b37a51428bb62e1
Author: Christian Schwede <email address hidden>
Date: Sun Sep 14 23:41:19 2014 +0200

    Fix internal link to keystoneauth in documentation

    This patch fixes a broken link at the end of the table in
    http://docs.openstack.org/developer/swift/logs.html#swift-source

    Change-Id: I989173ac93e0f840997333be0d5cec07eb77b304

commit 64548420c87f3163ed543c9e9a02a4f1abec69e0
Author: Andreas Jaeger <email address hidden>
Date: Sat Sep 13 09:48:14 2014 +0200

    Stop using intersphinx

    Remove intersphinx from the docs build as it triggers network calls that
    occasionally fail, and we don't really use intersphinx (links other
    sphinx documents out on the internet)

    This also removes the requirement for internet access during docs build.

    This can cause docs jobs to fail if the project errors out on
    warnings.

    Change-Id: I71e941e2a639641a662a163c682eb86d51de42fb
    Related-Bug: #1368910

commit 5c9835125802c51e2eb2823f5208d53c358a5e84
Author: Christian Schwede <email address hidden>
Date: Fri Sep 12 14:37:04 2014 +0000

    Fix RingBuilder._build_max_replicas_by_tier docstring

    The current docstring doesn't include zones, and the order of the
    entries is not up to date with the current code. Let's fix this.

    Change-Id: Ibabd79427b83d9e8c86b2caeb93dee219c8274c0

commit a03732e142540e5a7d6cb11de5232f0642beb20d
Author: Alistair Coles <email address hidden>
Date: Fri Sep 12 10:20:19 2014 +0100

    Add comments to clarify change to www-authenticate test

    Trivial patch to tidy-up change to the functional test for
    www-authenticate header and add a comment to explain
    that multiple header values might be returned.

    Change-Id: If62cb3fd9e11450a2be0cec71e80ecb74a959d04
    Related-bug: 1368048

commit ab96796dc8d1da9037330da0822c8b8d2264d192
Author: Alistair Coles <email address hidden>
Date: Thu Sep 11 10:23:32 2014 +0100

    Fix broken www-authenticate functional test

    testQuotedWWWAuthenticateHeader functional test started failing
    due to a change to keystonemiddleware.auth_token, which now adds
    its own www-authenticate header in addition to the one that swift
    keystoneauth adds.

    This patch changes the functional test to check expected
    swift generated header value is in the concatenation of
    www-authenticate values.

    Verified that functional tests still pass using tempauth.

    Closes-Bug: 1368048
    Change-Id: I913af077df800a559d259c1622f286ad10eae9df

commit f4d3facdf...

Based on conversation with John Dickinson and the bug, I am removing KEystone from this bug as there is nothing to do within Keystone.

This issue was fixed in the openstack/openstack-manuals 15.0.0 release.