No validation between client's IdP and Keystone IdP

[Marek Denis]

With today's configuration there is no strict link between federated assertion issued by a trusted IdP and a IdP configured inside Keystone. Hence, user has ability to choose a mapping and possibly get unauthorized access.

Proposed solution: setup a IdP identified included in an assertion issued by a IdP and validate whether that both values are equal.

[Jeremy Stanley (fungi)]

I've added an incomplete security advisory task and subscribed the Keystone core security reviewers to evaluate this report for validity and determination on whether it needs to be kept embargoed while a fix is under development.

[Marek Denis (marek-denis)]

So looks like there are two fixes for that:

- Leave the code and fix documentation indicating the probper way AND warning that inappropriate (not strict enough) configuration may lead to serious security risk

- Extend v3/OS-FEDERATION/identity_providers entities with parameter like "idp-identified" that must match the idp id included in every assertion issued by a certain identity provider. Next, while authenticating - compare the values from the assertion and the keystone's backend.

[Nathan Kinder (nkinder)]

I think that this all pertains to the way that the httpd module that handles the SP side of things works. In the SAML case, the httpd config should define what certs/keys are used for the signing and verification of the assertion. If the 'Location' directive is explicitly tied to the IdP, then it would not be possible to take the assertion from one IdP and use the OS-FEDERATION URL for another IdP to choose an unauthorized mapping.

For example, consider an IdP named 'ipsilon'. The following mod_auth_mellon configuration in the httpd instance running Keystone ties the certs/keys to the IdP specific URL:

----------------------------------------------------------------------------------------------
  <Location /v3/OS-FEDERATION/identity_providers/ipsilon/protocols/saml2/auth>
    AuthType "Mellon"
    MellonEnable "auth"
    MellonSPPrivateKeyFile /etc/httpd/mellon/http_keystone.key
    MellonSPCertFile /etc/httpd/mellon/http_keystone.cert
    MellonSPMetadataFile /etc/httpd/mellon/http_keystone.xml
    MellonIdPMetadataFile /etc/httpd/mellon/idp-metadata.xml
    MellonEndpointPath /v3/OS-FEDERATION/identity_providers/ipsilon/protocols/saml2/auth/mellon
  </Location>
----------------------------------------------------------------------------------------------

If a wildcard was used for the 'Location', then OS-FEDERATION requests for all IdPs could resolve to different mappings. This configuration doesn't really make sense as the same certs/keys/metadata would be used for all IdPs.

[Nathan Kinder (nkinder)]

I'd also like to add that I like Marek's second recommendation of adding the ability to check an assertion value against the IdP id in Keystone. This seems like a nice thing to do from a hardening perspective.

[Morgan Fainberg (mdrnstm)]

I am going to mark this as confirmed and say we definitely need to get this fixed sooner (even if it's only a doc change) so will mark it as high-priority.

[Thierry Carrez (ttx)]

IIUC this feels like OSSN territory (class B2 or D depending on how likely it is to get it wrong) - would benefit from an OSSN to explain the potential weakness in past versions, and strengthening to avoid unnecessary exposure in the future ?

[Marco Fargetta (fmarco76)]

To solve the problem in the documentation this patch should be OK. It describes a different configuration for shibboleth avoiding the problem. Actually, the configuration considers the different URLs for authentication as different shibboleth applications each accepting only one IdP. Using this configuration a URL can be accessed only by the IdP specified in the shibboleth application.

Nevertheless, the use of the IdP entityID inside the code is better for many reasons and should be developed in the future.

[Marek Denis (marek-denis)]

I'd say it should be implemented *now* and this should be the proper fix for the bug.

[Marco Fargetta (fmarco76)]

I am working on a proper implementation not requiring this work around in the documentation

[Jeremy Stanley (fungi)]

However this doesn't sound like something suitable for a stable backport, so we likely still need the workaround documented for Icehouse/Juno and the fix in master for Kilo. If we go this route, there wouldn't be a security advisory and we could likely lift the embargo as soon as there is consensus for the workaround recommendations.

[Marco Fargetta (fmarco76)]

+1
IMHO,I think it would be nice to have some small re-design of the mapping system for Kilo. There were many requests during the summit so I would include this change in that context and leave the documentation change for previous releases.

[Marek Denis (marek-denis)]

Morgan, Dolph - any chances for reviewing Marco's documentation patch he proposed last week?

[Dolph Mathews (dolph)]

The documentation fix looks sufficient to me, along with an OSSN for anyone who based a deployment on the previous documentation.

My only nit on the patch itself is that there could be stronger wording for anyone that misses the OSSN. Namely, the existing documentation for the protocol portion of the URL includes a warning that reads:

  Otherwise *every* federated protocol will be handled by Shibboleth.

Something similar would be nice to include for the IDP identifier as well (effectively warning against the mistake in the current documentation).

+1 for the patch as-is, however.

[Dolph Mathews (dolph)]

I'm also skeptical that a functional backport is necessary for this (especially anything introducing new API attributes, which certainly wouldn't be an automatic fix to affected stable/* users). It's a configuration issue first and foremost, and a documentation fix + advisory would sufficiently address that.

[Morgan Fainberg (mdrnstm)]

I agree with Dolph. The documentation fix looks sufficient and likely is all that is needed in the back port. The functional change is something that likely can be carried in master forward.

+1 as is, and agreed with the Nit to use stronger verbiage.

[Marco Fargetta (fmarco76)]

Is there any news for the proposed fix?

[Thierry Carrez (ttx)]

Proposed B1 class.

Proposed plan is to make this bug public, fix in master, document issue in past releases (OSSN?). Will proceed next Monday unless someone disagrees (or before if we can get an early ack from the OSSG)

[Nathan Kinder (nkinder)]

@ttx
The plan to make this public on Monday is fine with me. I will work on drafting the OSSN for this in private in the meantime.

[Morgan Fainberg (mdrnstm)]

I am fine with that plan.

[Dolph Mathews (dolph)]

+1 for plan in comment #17

[Thierry Carrez (ttx)]

Confirmed Class B1

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

commit 7cd8640e649e8f17cf5e77eab29060407a4f0cae
Author: Marco Fargetta <email address hidden>
Date: Wed Nov 19 16:59:10 2014 +0100

    Multiple IdPs problem

    The documentation is modified in order to allow mulitple
    IdPs using the OS-Federation avoiding conflicts among them.

    The previous proposed configuration allows user from one IdP
    to get mapped as user from a different IdP. With the chenge
    proposed this is not anymore possible.

    Change-Id: I9d62a840c122fb36c02c56a84c4f2ef8c30303c4
    Closes-Bug: 1390124

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: feature/hierarchical-multitenancy
Review: https://review.openstack.org/138182

[OpenStack Infra (hudson-openstack)]

Change abandoned by Morgan Fainberg (<email address hidden>) on branch: feature/hierarchical-multitenancy
Review: https://review.openstack.org/138182

[OpenStack Infra (hudson-openstack)]

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

[Nathan Kinder (nkinder)]

This has been published as OSSN-0047:

  https://wiki.openstack.org/wiki/OSSN/OSSN-0047