get_identity_providers policy should be singular

[Matthew Edmonds]

identity:get_identity_providers should be identity:get_identity_provider (singular) since a GET is targeted on a single provider and the code is setup to check for identity:get_identity_provider (singular). See https://github.com/openstack/keystone/blob/c7e29560b7bf7a44e44722eea0645bf18ad56af3/keystone/federation/controllers.py#L112

found in master (pike)

The ocata default policy.json also has this problem. Unless someone manually overrode policy to specify identity:get_identity_provider (singular), the result would be that the default rule was actually used for that check instead of identity:get_identity_providers. We could go back and fix the default policy.json for past releases, but the default actually has the same value as identity:get_identity_providers, and if nobody has complained it's probably safer to just leave it. It is, after all, just defaults there and anyone can override by specifying the correct value.

But we must fix in pike to go along with the shift of policy into code. Policy defaults in code definitely need to match up with what the code actually checks. There should no longer be any reliance on the default rule.

[OpenStack Infra (hudson-openstack)]

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

[Boris Bobrov (bbobrov)]

This might be serious for someone. Before this change, get_identity_provider method was protected only by "default" rule, and "get_identity_providers" rule in policy json was protecting nothing (because no such method exist in keystone. If some operator relied on changing the "get_identity_providers" rule, or on changing the "default" rule, they might be affected.

Luckily, it is hard to find out id of an identity provider. Getting an identity provider by an unprivileged user probably doesn't give out any useful info.

Fixing this bug will probably not break anybody; if it will, they probably have security vulnerability in their system.

[Jeremy Stanley (fungi)]

Just to confirm, this is only a risk for the upcoming Pike release, not a defect in current supported stable branches? If so, we would not issue a security advisory unless it remains unfixed at release time per report class Y ("Vulnerability only found in development release") in our taxonomy: https://security.openstack.org/vmt-process.html#incident-report-taxonomy

[Matthew Edmonds (edmondsw)]

I don't know that I'd consider this a vulnerability in any release, but I guess you could say that since we ignore "identity:get_identity_providers" in keystone policy.json, someone who changes that value will think they've restricted that API when they have not actually done so. But the default that would have taken effect was admin (for any release), so the only way you could be trying to restrict more than that is to disable it entirely. And someone customizing policy should test that their changes are working, so I don't expect anybody is in this boat today.

[Boris Bobrov (bbobrov)]

> Just to confirm, this is only a risk for the upcoming Pike release, not a defect in current supported stable branches?

No, there is risk for all supported branches.

My guess is that it is C1 issue.

[Jeremy Stanley (fungi)]

Is there any intent to backport fixes for this issue to newton and ocata, or is the plan to only fix it in pike?

[Matthew Edmonds (edmondsw)]

Boris, exactly what do you think the vulnerability is here? Is it what I said in comment 4? If so, does the rest of my comment there not assuage your concerns? Please also check the description, which I updated with reasoning including why I don't think we should backport anything to ocata.

[Boris Bobrov (bbobrov)]

> Unless someone manually overrode policy to specify identity:get_identity_provider (singular), the result would be that the default rule was actually used for that check instead of identity:get_identity_providers.

Changing policy.json and default rules is a normal operation for the cloud. We were going to change it in our public cloud, for example.

> We could go back and fix the default policy.json for past releases, but the default actually has the same value as identity:get_identity_providers, and if nobody has complained it's probably safer to just leave it. It is, after all, just defaults there and anyone can override by specifying the correct value.

Or we could rename the controller to get_identity_providers. But i think the risk here would be bigger than if we just ignore it.

We probably will not backport it to prior releases.

[Lance Bragstad (lbragstad)]

To recap the conversation and summarize what was discuss in IRC [0].

There is a security issue if a deployer modifies the default policy role required for an operation but wishes to keep the identity:get_identity_providers protected at the "admin-level". This was deemed as unlikely since the default and get_identity_provider were protected with the same admin_required rule.

For the sake of process, we can merge the proposed fix [1] with a detailed release note explaining the case. After that we can propose the patch to stable/ocata as well as stable/newton. Even though a deployer can technically issue this fix without a new release, the process of issuing a release note seems valuable at least for the sake of process.

[0] http://eavesdrop.openstack.org/irclogs/%23openstack-keystone/%23openstack-keystone.2017-07-11.log.html#t2017-07-11T21:26:46
[1] https://review.openstack.org/#/c/482142/

[Tristan Cacqueray (tristan-cacqueray)]

I've added an OSSN task to see if a Security Note would make more sense here since this is kind of an insecure default config value (class B2).

[OpenStack Infra (hudson-openstack)]

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

commit b7119637a04d0a07fa6419a407f433c01bbd1db2
Author: Matthew Edmonds <email address hidden>
Date: Mon Jul 10 09:20:18 2017 -0400

    fix identity:get_identity_providers typo

    Changes identity:get_identity_providers policy rule to
    identity:get_identity_provider to match what is checked by the code.

    Change-Id: I0841abd30fd15c034b5836e42a18938634b509b1
    Closes-Bug: #1703369

[Jeremy Stanley (fungi)]

Since Luke is running with the OSSN task confirmed, I'm going to take that as agreement that this is class B2 and set our OSSA task to won't fix. Thanks!

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/ocata
Review: https://review.openstack.org/485694

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/newton
Review: https://review.openstack.org/485695

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/485694
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=6c96675d63257d7ef2c27c5598c642efa7a25d08
Submitter: Jenkins
Branch: stable/ocata

commit 6c96675d63257d7ef2c27c5598c642efa7a25d08
Author: Matthew Edmonds <email address hidden>
Date: Mon Jul 10 09:20:18 2017 -0400

    fix identity:get_identity_providers typo

    Changes identity:get_identity_providers policy rule to
    identity:get_identity_provider to match what is checked by the code.

    Conflicts:
      keystone/common/policies/identity_provider.py

    There was a conflict backporting this change since the policy-in-code
    work in new in Pike. The conflict was resolved by removing the
    policy-in-code change and making it manually against the old
    etc/policy.json file.

    Change-Id: I0841abd30fd15c034b5836e42a18938634b509b1
    Closes-Bug: #1703369
    (cherry picked from commit b7119637a04d0a07fa6419a407f433c01bbd1db2)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/485695
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=bd49c3ef6daa474e9c84c0d8721c0f6812ee3d2c
Submitter: Jenkins
Branch: stable/newton

commit bd49c3ef6daa474e9c84c0d8721c0f6812ee3d2c
Author: Matthew Edmonds <email address hidden>
Date: Mon Jul 10 09:20:18 2017 -0400

    fix identity:get_identity_providers typo

    Changes identity:get_identity_providers policy rule to
    identity:get_identity_provider to match what is checked by the code.

    Conflicts:
      keystone/common/policies/identity_provider.py

    There was a conflict backporting this change since the policy-in-code
    work in new in Pike. The conflict was resolved by removing the
    policy-in-code change and making it manually against the old
    etc/policy.json file.

    Change-Id: I0841abd30fd15c034b5836e42a18938634b509b1
    Closes-Bug: #1703369
    (cherry picked from commit b7119637a04d0a07fa6419a407f433c01bbd1db2)
    (cherry picked from commit 8038f545daa31354e08a4063209295439005c0b8)

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/keystone 11.0.3 release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/keystone 10.0.3 release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/keystone 12.0.0.0b3 development milestone.

[Nick Tait (nickthetait)]

I am starting to work on this task.

[Nick Tait (nickthetait)]

Have published a draft of this OSSN. Seeking reviews here https://review.openstack.org/#/c/559440/

Is this a comprehensive list of affected releases: Pike, Ocata, and Newton? What about Mitaka?

[Nick Tait (nickthetait)]

OSSN has been merged. This bug can be closed now.

[Mircea Vutcovici (mirceavutcovici)]

The same typo is in Horizon too:

$ git remote -v
origin git://git.openstack.org/openstack/horizon (fetch)
origin git://git.openstack.org/openstack/horizon (push)
$ grep -rl get_identity_providers
doc/source/contributor/topics/policy.rst
openstack_auth/tests/conf/keystone_policy.json
openstack_auth/tests/conf/policy.v3cloudsample.json

Should I submit a new bug?

[Luke Hinds (lhinds)]

Sounds right Mircea, but it won't be a security issue this time, as its in docs / unit tests, rather than code that could be used in production. Still needs a bug raised in horizon though, and well spotted.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/564150
Committed: https://git.openstack.org/cgit/openstack/horizon/commit/?id=93bb571888a1bff4fa1e110356dbf2cb9fb4ee52
Submitter: Zuul
Branch: master

commit 93bb571888a1bff4fa1e110356dbf2cb9fb4ee52
Author: Radomir Dopieralski <email address hidden>
Date: Wed Apr 25 11:37:05 2018 +0200

    Replace all mentions of get_identity_providers with get_identity_provider

    There was a typo in keystone's policy files, and it has been fixed in
    Keystone already, we should also fix it to match.

    Change-Id: I41e4381765f3bfc5988ca235e6cbeb6d1ba62fc2
    Closes-bug: #1703369

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/queens
Review: https://review.openstack.org/564219

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/564219
Committed: https://git.openstack.org/cgit/openstack/horizon/commit/?id=0253c8f1221594335afd9a2582a310e3e1d2c0a3
Submitter: Zuul
Branch: stable/queens

commit 0253c8f1221594335afd9a2582a310e3e1d2c0a3
Author: Radomir Dopieralski <email address hidden>
Date: Wed Apr 25 11:37:05 2018 +0200

    Replace all mentions of get_identity_providers with get_identity_provider

    There was a typo in keystone's policy files, and it has been fixed in
    Keystone already, we should also fix it to match.

    Change-Id: I41e4381765f3bfc5988ca235e6cbeb6d1ba62fc2
    Closes-bug: #1703369
    (cherry picked from commit 93bb571888a1bff4fa1e110356dbf2cb9fb4ee52)

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/horizon 13.0.1 release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/horizon 14.0.0.0b2 development milestone.