Add new attribute to the federation protocol API

Modify the FederationProtocolModel class and add the
remote_id_atributte to the federation_protocol table.
Add the respective migration and tests files. And
also modify the schema to expect a remote_id_attribute
property.

Closes-bug: #1724645

Co-authored-by: Colleen Murphy<colleen@gazlene.net>

Change-Id: I9802c8a5c187bae16de89893ca8639b01cd7cb1b

api-ref/source/v3-ext/federation.inc

@@ -8,6 +8,13 @@ Provide the ability for users to manage Identity Providers (IdPs) and establish
 a set of rules to map federation protocol attributes to Identity API
 attributes. Requires v3.0+ of the Identity API.
 
+What's New in Version 1.4
+=========================
+
+Corresponding to Identity API v3.12 release.
+
+- Added `remote_id_attribute` as an attribute of a Protocol.
+
 What's New in Version 1.3
 =========================
 
@@ -117,6 +124,16 @@ Required attributes:
   Indicates which mapping should be used to process federated authentication
   requests.
 
+Optional attributes:
+
+- ``remote_id_attribute`` (string)
+
+  Key to obtain the entity ID of the Identity Provider from the HTTPD
+  environment. For `mod_shib`, this would be `Shib-Identity-Provider`. For
+  `mod_auth_openidc`, this could be `HTTP_OIDC_ISS`. For `mod_auth_mellon`,
+  this could be `MELLON_IDP`. This overrides the default value provided in
+  keystone.conf.
+
 Mappings
 --------
 

keystone/api/auth.py

@@ -334,11 +334,14 @@ class AuthTokenResource(_AuthFederationWebSSOBase):
 class AuthFederationWebSSOResource(_AuthFederationWebSSOBase):
     @classmethod
     def _perform_auth(cls, protocol_id):
-        try:
+        idps = PROVIDERS.federation_api.list_idps()
+        for idp in idps:
             remote_id_name = federation_utils.get_remote_id_parameter(
-                protocol_id)
-            remote_id = flask.request.environ[remote_id_name]
-        except KeyError:
+                idp, protocol_id)
+            remote_id = flask.request.environ.get(remote_id_name)
+            if remote_id:
+                break
+        if not remote_id:
             msg = 'Missing entity ID from environment'
             tr_msg = _('Missing entity ID from environment')
             LOG.error(msg)

keystone/common/sql/contract_repo/versions/064_contract_add_remote_id_attribute_to_federation_protocol_table.py

@@ -0,0 +1,15 @@
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+
+def upgrade(migrate_engine):
+    pass

keystone/common/sql/data_migration_repo/versions/064_migrate_add_remote_id_attribute_to_federation_protocol_table.py

@@ -0,0 +1,15 @@
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+
+def upgrade(migrate_engine):
+    pass

keystone/common/sql/expand_repo/versions/064_expand_add_remote_id_attribute_to_federation_protocol_table.py

@@ -0,0 +1,22 @@
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+import sqlalchemy as sql
+
+
+def upgrade(migrate_engine):
+    meta = sql.MetaData()
+    meta.bind = migrate_engine
+
+    federation_protocol_table = sql.Table('federation_protocol', meta, autoload=True)
+    remote_id_attribute = sql.Column('remote_id_attribute', sql.String(64))
+    federation_protocol_table.create_column(remote_id_attribute)

keystone/conf/federation.py

@@ -35,10 +35,12 @@ assertions. Matched variables are passed into the federated mapping engine.
 remote_id_attribute = cfg.StrOpt(
     'remote_id_attribute',
     help=utils.fmt("""
-Value to be used to obtain the entity ID of the Identity Provider from the
-environment. For `mod_shib`, this would be `Shib-Identity-Provider`. For
-`mod_auth_openidc`, this could be `HTTP_OIDC_ISS`. For `mod_auth_mellon`, this
-could be `MELLON_IDP`.
+Default value for all protocols to be used to obtain the entity ID of the
+Identity Provider from the environment. For `mod_shib`, this would be
+`Shib-Identity-Provider`. For `mod_auth_openidc`, this could be
+`HTTP_OIDC_ISS`. For `mod_auth_mellon`, this could be `MELLON_IDP`. This can be
+overridden on a per-protocol basis by providing a `remote_id_attribute` to the
+federation protocol using the API.
 """))
 
 federated_domain_name = cfg.StrOpt(

keystone/federation/backends/sql.py

@@ -28,13 +28,14 @@ LOG = log.getLogger(__name__)
 
 class FederationProtocolModel(sql.ModelBase, sql.ModelDictMixin):
     __tablename__ = 'federation_protocol'
-    attributes = ['id', 'idp_id', 'mapping_id']
-    mutable_attributes = frozenset(['mapping_id'])
+    attributes = ['id', 'idp_id', 'mapping_id', 'remote_id_attribute']
+    mutable_attributes = frozenset(['mapping_id', 'remote_id_attribute'])
 
     id = sql.Column(sql.String(64), primary_key=True)
     idp_id = sql.Column(sql.String(64), sql.ForeignKey('identity_provider.id',
                         ondelete='CASCADE'), primary_key=True)
     mapping_id = sql.Column(sql.String(64), nullable=False)
+    remote_id_attribute = sql.Column(sql.String(64))
 
     @classmethod
     def from_dict(cls, dictionary):

keystone/federation/schema.py

@@ -117,8 +117,14 @@ identity_provider_update = {
     'additionalProperties': False
 }
 
+_remote_id_attribute_properties = {
+    'type': 'string',
+    'maxLength': 64,
+}
+
 _protocol_properties = {
-    'mapping_id': parameter_types.mapping_id_string
+    'mapping_id': parameter_types.mapping_id_string,
+    'remote_id_attribute': _remote_id_attribute_properties
 }
 
 protocol_create = {

keystone/federation/utils.py

@@ -22,6 +22,7 @@ from oslo_log import log
 from oslo_utils import timeutils
 import six
 
+from keystone.common import provider_api
 import keystone.conf
 from keystone import exception
 from keystone.i18n import _
@@ -29,6 +30,7 @@ from keystone.i18n import _
 
 CONF = keystone.conf.CONF
 LOG = log.getLogger(__name__)
+PROVIDERS = provider_api.ProviderAPIs
 
 
 class UserType(object):
@@ -277,23 +279,28 @@ def validate_expiration(token):
         raise exception.Unauthorized(_('Federation token is expired'))
 
 
-def get_remote_id_parameter(protocol):
+def get_remote_id_parameter(idp, protocol):
     # NOTE(marco-fargetta): Since we support any protocol ID, we attempt to
-    # retrieve the remote_id_attribute of the protocol ID. If it's not
-    # registered in the config, then register the option and try again.
-    # This allows the user to register protocols other than oidc and saml2.
-    remote_id_parameter = None
-    try:
-        remote_id_parameter = CONF[protocol]['remote_id_attribute']
-    except AttributeError:
-        # TODO(dolph): Move configuration registration to keystone.conf
-        CONF.register_opt(cfg.StrOpt('remote_id_attribute'),
-                          group=protocol)
+    # retrieve the remote_id_attribute of the protocol ID. It will look up first
+    # if the remote_id_attribute exists.
+    protocol_ref = PROVIDERS.federation_api.get_protocol(idp['id'], protocol)
+    remote_id_parameter = protocol_ref.get('remote_id_attribute')
+    if remote_id_parameter:
+        return remote_id_parameter
+    else:
+        # If it's not registered in the config, then register the option and try again.
+        # This allows the user to register protocols other than oidc and saml2.
         try:
             remote_id_parameter = CONF[protocol]['remote_id_attribute']
-        except AttributeError:  # nosec
-            # No remote ID attr, will be logged and use the default instead.
-            pass
+        except AttributeError:
+            # TODO(dolph): Move configuration registration to keystone.conf
+            CONF.register_opt(cfg.StrOpt('remote_id_attribute'),
+                              group=protocol)
+            try:
+                remote_id_parameter = CONF[protocol]['remote_id_attribute']
+            except AttributeError:  # nosec
+                # No remote ID attr, will be logged and use the default instead.
+                pass
     if not remote_id_parameter:
         LOG.debug('Cannot find "remote_id_attribute" in configuration '
                   'group %s. Trying default location in '
@@ -305,7 +312,7 @@ def get_remote_id_parameter(protocol):
 
 def validate_idp(idp, protocol, assertion):
     """The IdP providing the assertion should be registered for the mapping."""
-    remote_id_parameter = get_remote_id_parameter(protocol)
+    remote_id_parameter = get_remote_id_parameter(idp, protocol)
     if not remote_id_parameter or not idp['remote_ids']:
         LOG.debug('Impossible to identify the IdP %s ', idp['id'])
         # If nothing is defined, the administrator may want to

keystone/tests/unit/federation/test_core.py

@@ -63,6 +63,18 @@ class TestFederationProtocol(unit.TestCase):
                           protocol['id'],
                           protocol)
 
+    def test_create_protocol_with_remote_id_attribute(self):
+        protocol = {
+            'id': uuid.uuid4().hex,
+            'mapping_id': self.mapping['id'],
+            'remote_id_attribute': uuid.uuid4().hex
+        }
+        protocol_ret = PROVIDERS.federation_api.create_protocol(
+            self.idp['id'], protocol['id'], protocol
+        )
+        self.assertEqual(protocol['remote_id_attribute'],
+                         protocol_ret['remote_id_attribute'])
+
     def test_update_protocol(self):
         protocol = {
             'id': uuid.uuid4().hex,
@@ -97,3 +109,19 @@ class TestFederationProtocol(unit.TestCase):
                           self.idp['id'],
                           protocol['id'],
                           protocol)
+
+    def test_update_protocol_with_remote_id_attribute(self):
+        protocol = {
+            'id': uuid.uuid4().hex,
+            'mapping_id': self.mapping['id']
+        }
+        protocol_ret = PROVIDERS.federation_api.create_protocol(
+            self.idp['id'], protocol['id'], protocol
+        )
+        new_remote_id_attribute = uuid.uuid4().hex
+        protocol['remote_id_attribute'] = new_remote_id_attribute
+        protocol_ret = PROVIDERS.federation_api.update_protocol(
+            self.idp['id'], protocol['id'], protocol
+        )
+        self.assertEqual(protocol['remote_id_attribute'],
+                         protocol_ret['remote_id_attribute'])

keystone/tests/unit/test_backend_federation_sql.py

@@ -34,7 +34,8 @@ class SqlFederation(test_backend_sql.SqlModels):
     def test_federated_protocol(self):
         cols = (('id', sql.String, 64),
                 ('idp_id', sql.String, 64),
-                ('mapping_id', sql.String, 64))
+                ('mapping_id', sql.String, 64),
+                ('remote_id_attribute', sql.String, 64))
         self.assertExpectedSchema('federation_protocol', cols)
 
     def test_mapping(self):

keystone/tests/unit/test_sql_upgrade.py

@@ -3387,6 +3387,26 @@ class FullMigration(SqlMigrateBase, unit.TestCase):
             ['id', 'project_id', 'resource_limit', 'description',
              'internal_id', 'registered_limit_id', 'domain_id'])
 
+    def test_migration_064_add_remote_id_attribute_to_federation_protocol(self):
+        self.expand(63)
+        self.migrate(63)
+        self.contract(63)
+
+        federation_protocol_table_name = 'federation_protocol'
+        self.assertTableColumns(
+            federation_protocol_table_name,
+            ['id', 'idp_id', 'mapping_id']
+        )
+
+        self.expand(64)
+        self.migrate(64)
+        self.contract(64)
+
+        self.assertTableColumns(
+            federation_protocol_table_name,
+            ['id', 'idp_id', 'mapping_id', 'remote_id_attribute']
+        )
+
 
 class MySQLOpportunisticFullMigration(FullMigration):
     FIXTURE = db_fixtures.MySQLOpportunisticFixture

keystone/tests/unit/test_v3_federation.py

@@ -2968,6 +2968,27 @@ class FederatedTokenTests(test_v3.RestfulTestCase, FederatedSetupMixin):
             token['user']['name'])
         self.assertNotEqual(token['user']['name'], token['user']['id'])
 
+    def test_issue_unscoped_token_with_remote_different_from_protocol(self):
+        protocol = PROVIDERS.federation_api.get_protocol(
+            self.IDP_WITH_REMOTE, self.PROTOCOL
+        )
+        protocol['remote_id_attribute'] = uuid.uuid4().hex
+        PROVIDERS.federation_api.update_protocol(
+            self.IDP_WITH_REMOTE, protocol['id'], protocol
+        )
+        self._issue_unscoped_token(
+            idp=self.IDP_WITH_REMOTE,
+            environment={
+                protocol['remote_id_attribute']: self.REMOTE_IDS[0]
+            }
+        )
+        self.assertRaises(
+            exception.Unauthorized,
+            self._issue_unscoped_token,
+            idp=self.IDP_WITH_REMOTE,
+            environment={uuid.uuid4().hex: self.REMOTE_IDS[0]}
+        )
+
 
 class FernetFederatedTokenTests(test_v3.RestfulTestCase, FederatedSetupMixin):
     AUTH_METHOD = 'token'
@@ -4896,6 +4917,28 @@ class WebSSOTests(FederatedTokenTests):
         # needs to be encoded
         self.assertIn(self.TRUSTED_DASHBOARD.encode('utf-8'), resp.data)
 
+    def test_issue_unscoped_token_with_remote_from_protocol(self):
+        self.config_fixture.config(
+            group='federation', remote_id_attribute=None
+        )
+        self.config_fixture.config(
+            group=self.PROTOCOL, remote_id_attribute=None
+        )
+        protocol = PROVIDERS.federation_api.get_protocol(
+            self.IDP_WITH_REMOTE, self.PROTOCOL
+        )
+        protocol['remote_id_attribute'] = self.PROTOCOL_REMOTE_ID_ATTR
+        PROVIDERS.federation_api.update_protocol(
+            self.IDP_WITH_REMOTE, protocol['id'], protocol
+        )
+        environment = {self.PROTOCOL_REMOTE_ID_ATTR: self.REMOTE_IDS[0],
+                       'QUERY_STRING': 'origin=%s' % self.ORIGIN}
+        environment.update(mapping_fixtures.EMPLOYEE_ASSERTION)
+        with self.make_request(environ=environment):
+            resp = auth_api.AuthFederationWebSSOResource._perform_auth(
+                self.PROTOCOL)
+        self.assertIn(self.TRUSTED_DASHBOARD.encode('utf-8'), resp.data)
+
 
 class K2KServiceCatalogTests(test_v3.RestfulTestCase):
     SP1 = 'SP1'

releasenotes/notes/bug-1724645-a94659dfd0f45b9a.yaml

@@ -0,0 +1,16 @@
+---
+features:
+  - |
+    [`bug 1724645 <https://bugs.launchpad.net/keystone/+bug/1724645>`_]
+    Adds a new attribute, ``remote_id_attribute``, to the federation protocol
+    object, which allows WebSSO authentication to forward authentication
+    requests through the right implementation for a federated protocol based on
+    the remote ID attribute in the authentication headers.
+fixes:
+  - |
+    [`bug 1724645 <https://bugs.launchpad.net/keystone/+bug/1724645>`_]
+    Fixes an issue where multiple implementations of a federation protocol,
+    such as Shibboleth and Mellon for the SAML2.0 protocol, could not be
+    differentiated from one another because they had to share the same globally
+    configured remote ID attribute. Now the remote ID attribute can be set on
+    the protocol object itself.