From dda426b61a18590a81c5b3af281eb0c410756692 Mon Sep 17 00:00:00 2001 From: Vishakha Agarwal Date: Thu, 2 Aug 2018 16:31:54 +0530 Subject: [PATCH] Add openstack_groups to assertion Currently, a keystone IdP does not provide the groups to which user belong when generating SAML assertions.This patch adds an additional attribute called "openstack_groups" in the assertion. Change-Id: I205e8bbf9a4579b16177f57e29e363f4205a2b48 Closes-Bug: #1641625 --- devstack/files/federation/attribute-map.xml | 1 + .../admin/federation/mapping_combinations.rst | 57 ++++++++++++++++- doc/source/admin/federation/shibboleth.inc | 11 ++-- keystone/api/_shared/saml.py | 24 ++++++- keystone/federation/idp.py | 26 +++++++- keystone/federation/utils.py | 62 +++++++++++++------ .../unit/contrib/federation/test_utils.py | 25 ++++++++ keystone/tests/unit/mapping_fixtures.py | 37 +++++++++++ .../unit/saml2/signed_saml2_assertion.xml | 4 ++ keystone/tests/unit/test_v3_federation.py | 37 ++++++++--- .../notes/bug-1641625-fe463874dc5edb10.yaml | 7 +++ 11 files changed, 254 insertions(+), 37 deletions(-) create mode 100644 releasenotes/notes/bug-1641625-fe463874dc5edb10.yaml diff --git a/devstack/files/federation/attribute-map.xml b/devstack/files/federation/attribute-map.xml index 4094caad02..6e1b3c7da1 100644 --- a/devstack/files/federation/attribute-map.xml +++ b/devstack/files/federation/attribute-map.xml @@ -12,6 +12,7 @@ + diff --git a/doc/source/admin/federation/mapping_combinations.rst b/doc/source/admin/federation/mapping_combinations.rst index 60869d7e75..7af5a2e7ba 100644 --- a/doc/source/admin/federation/mapping_combinations.rst +++ b/doc/source/admin/federation/mapping_combinations.rst @@ -892,7 +892,7 @@ look as follows: .. code-block:: xml - + @@ -933,6 +933,57 @@ names we have in the Identity Provider. It will map any user with the name ] } +A keystone user's groups can also be mapped to groups in the service provider. +For example, with the following attributes declared in Shibboleth's attributes file: + +.. code-block:: xml + + + + + +Then the following mapping can be used to map the user's group membership from the keystone +IdP to groups in the keystone SP: + +.. code-block:: json + + { + "rules": [ + { + "local": + [ + { + "user": + { + "name": "{0}" + } + }, + { + "groups": "{1}" + } + ], + "remote": + [ + { + "type": "openstack_user" + }, + { + "type": "openstack_groups" + } + ] + } + ] + } + + +``openstack_user``, and ``openstack_groups`` will be matched by service +provider to the attribute names we have in the Identity Provider. It will +take the ``openstack_user`` attribute and finds in the assertion then inserts +it directly in the mapping. The identity provider will set the value of +``openstack_groups`` by group name and domain name to which the user belongs +in the Idp. Suppose the user belongs to 'group1' in domain 'Default' in the IdP +then it will map to a group with the same name and same domain's name in the SP. + The possible attributes that can be used in a mapping are `openstack_user`, -`openstack_user_domain`, `openstack_roles`, `openstack_project`, and -`openstack_project_domain`. +`openstack_user_domain`, `openstack_roles`, `openstack_project`, +`openstack_project_domain` and `openstack_groups`. diff --git a/doc/source/admin/federation/shibboleth.inc b/doc/source/admin/federation/shibboleth.inc index 9415659adc..95710c8340 100644 --- a/doc/source/admin/federation/shibboleth.inc +++ b/doc/source/admin/federation/shibboleth.inc @@ -236,11 +236,12 @@ attributes will be needed in ``/etc/shibboleth/attribute-map.xml``: .. code-block:: xml - - - - - + + + + + + And update the ``REMOTE_USER`` variable in ``/etc/shibboleth/shibboleth2.xml`` if desired: diff --git a/keystone/api/_shared/saml.py b/keystone/api/_shared/saml.py index 2e338957c6..956acb8c9b 100644 --- a/keystone/api/_shared/saml.py +++ b/keystone/api/_shared/saml.py @@ -10,6 +10,8 @@ # License for the specific language governing permissions and limitations # under the License. +from oslo_serialization import jsonutils + from keystone.common import provider_api import keystone.conf from keystone import exception @@ -47,8 +49,28 @@ def create_base_saml_assertion(auth): project_domain_name = token.project_domain['name'] subject_domain_name = token.user_domain['name'] + def group_membership(): + """Return a list of dictionaries serialized as strings. + + The expected return structure is:: + + ['JSON:{"name":"group1","domain":{"name":"Default"}}', + 'JSON:{"name":"group2","domain":{"name":"Default"}}'] + """ + user_groups = [] + groups = PROVIDERS.identity_api.list_groups_for_user( + token.user_id) + for group in groups: + user_group = {} + group_domain_name = PROVIDERS.resource_api.get_domain( + group['domain_id'])['name'] + user_group["name"] = group['name'] + user_group["domain"] = {'name': group_domain_name} + user_groups.append('JSON:' + jsonutils.dumps(user_group)) + return user_groups + groups = group_membership() generator = keystone_idp.SAMLGenerator() response = generator.samlize_token( issuer, sp_url, subject, subject_domain_name, - role_names, project, project_domain_name) + role_names, project, project_domain_name, groups) return response, service_provider diff --git a/keystone/federation/idp.py b/keystone/federation/idp.py index e0c983e7b4..fd464f5c20 100644 --- a/keystone/federation/idp.py +++ b/keystone/federation/idp.py @@ -48,7 +48,8 @@ class SAMLGenerator(object): self.assertion_id = uuid.uuid4().hex def samlize_token(self, issuer, recipient, user, user_domain_name, roles, - project, project_domain_name, expires_in=None): + project, project_domain_name, groups, + expires_in=None): """Convert Keystone attributes to a SAML assertion. :param issuer: URL of the issuing party @@ -65,6 +66,9 @@ class SAMLGenerator(object): :type project: string :param project_domain_name: Project Domain name :type project_domain_name: string + :param groups: List of strings of user groups and domain name, where + strings are serialized dictionaries. + :type groups: list :param expires_in: Sets how long the assertion is valid for, in seconds :type expires_in: int @@ -76,7 +80,8 @@ class SAMLGenerator(object): saml_issuer = self._create_issuer(issuer) subject = self._create_subject(user, expiration_time, recipient) attribute_statement = self._create_attribute_statement( - user, user_domain_name, roles, project, project_domain_name) + user, user_domain_name, roles, project, project_domain_name, + groups) authn_statement = self._create_authn_statement(issuer, expiration_time) signature = self._create_signature() @@ -162,7 +167,8 @@ class SAMLGenerator(object): return subject def _create_attribute_statement(self, user, user_domain_name, roles, - project, project_domain_name): + project, project_domain_name, + groups): """Create an object that represents a SAML AttributeStatement. @@ -188,6 +194,15 @@ class SAMLGenerator(object): Default + + JSON:{"name":"group1","domain":{"name":"Default"}} + + JSON:{"name":"group2","domain":{"name":"Default"}} + + + :returns: XML object @@ -218,6 +233,11 @@ class SAMLGenerator(object): attribute_statement.attribute.append(project_attribute) attribute_statement.attribute.append(project_domain_attribute) attribute_statement.attribute.append(user_domain_attribute) + + if groups: + groups_attribute = _build_attribute( + 'openstack_groups', groups) + attribute_statement.attribute.append(groups_attribute) return attribute_statement def _create_authn_statement(self, issuer, expiration_time): diff --git a/keystone/federation/utils.py b/keystone/federation/utils.py index 7dea74f08c..aab9b35242 100644 --- a/keystone/federation/utils.py +++ b/keystone/federation/utils.py @@ -19,6 +19,7 @@ import flask import jsonschema from oslo_config import cfg from oslo_log import log +from oslo_serialization import jsonutils from oslo_utils import timeutils import six @@ -555,6 +556,48 @@ class RuleProcessor(object): LOG.debug('mapped_properties: %s', mapped_properties) return mapped_properties + def _normalize_groups(self, identity_value): + # In this case, identity_value['groups'] is a string + # representation of a list, and we want a real list. This is + # due to the way we do direct mapping substitutions today (see + # function _update_local_mapping() ) + if 'name' in identity_value['groups']: + try: + group_names_list = ast.literal_eval( + identity_value['groups']) + except (ValueError, SyntaxError): + group_names_list = [identity_value['groups']] + + def convert_json(group): + if group.startswith('JSON:'): + return jsonutils.loads(group.lstrip('JSON:')) + return group + + group_dicts = [convert_json(g) for g in group_names_list] + for g in group_dicts: + if 'domain' not in g: + msg = _("Invalid rule: %(identity_value)s. Both " + "'groups' and 'domain' keywords must be " + "specified.") + msg = msg % {'identity_value': identity_value} + raise exception.ValidationError(msg) + else: + if 'domain' not in identity_value: + msg = _("Invalid rule: %(identity_value)s. Both " + "'groups' and 'domain' keywords must be " + "specified.") + msg = msg % {'identity_value': identity_value} + raise exception.ValidationError(msg) + try: + group_names_list = ast.literal_eval( + identity_value['groups']) + except (ValueError, SyntaxError): + group_names_list = [identity_value['groups']] + domain = identity_value['domain'] + group_dicts = [{'name': name, 'domain': domain} for name in + group_names_list] + return group_dicts + def _transform(self, identity_values): """Transform local mappings, to an easier to understand format. @@ -642,24 +685,7 @@ class RuleProcessor(object): groups_by_domain.setdefault(domain, list()).append(group) group_names.extend(extract_groups(groups_by_domain)) if 'groups' in identity_value: - if 'domain' not in identity_value: - msg = _("Invalid rule: %(identity_value)s. Both 'groups' " - "and 'domain' keywords must be specified.") - msg = msg % {'identity_value': identity_value} - raise exception.ValidationError(msg) - # In this case, identity_value['groups'] is a string - # representation of a list, and we want a real list. This is - # due to the way we do direct mapping substitutions today (see - # function _update_local_mapping() ) - try: - group_names_list = ast.literal_eval( - identity_value['groups']) - except (ValueError, SyntaxError): - group_names_list = [identity_value['groups']] - domain = identity_value['domain'] - group_dicts = [{'name': name, 'domain': domain} for name in - group_names_list] - + group_dicts = self._normalize_groups(identity_value) group_names.extend(group_dicts) if 'group_ids' in identity_value: # If identity_values['group_ids'] is a string representation diff --git a/keystone/tests/unit/contrib/federation/test_utils.py b/keystone/tests/unit/contrib/federation/test_utils.py index b182cb10fd..29b7f11a01 100644 --- a/keystone/tests/unit/contrib/federation/test_utils.py +++ b/keystone/tests/unit/contrib/federation/test_utils.py @@ -801,6 +801,31 @@ class MappingRuleEngineTests(unit.BaseTestCase): ] self.assertEqual(expected_projects, values['projects']) + def test_rule_engine_for_groups_and_domain(self): + """Should return user's groups and group domain. + + The GROUP_DOMAIN_ASSERTION should successfully have a match in + MAPPING_GROUPS_DOMAIN_OF_USER. This will test the case where a groups + with its domain will exist`, and return user's groups and group domain. + + """ + mapping = mapping_fixtures.MAPPING_GROUPS_DOMAIN_OF_USER + assertion = mapping_fixtures.GROUPS_DOMAIN_ASSERTION + rp = mapping_utils.RuleProcessor(FAKE_MAPPING_ID, mapping['rules']) + values = rp.process(assertion) + + self.assertValidMappedUserObject(values) + user_name = assertion.get('openstack_user') + user_groups = ['group1', 'group2'] # since we know the input assertion + groups = values.get('group_names', {}) + group_list = [g.get('name') for g in groups] + group_ids = values.get('group_ids') + name = values.get('user', {}).get('name') + + self.assertEqual(user_name, name) + self.assertEqual(user_groups, group_list) + self.assertEqual([], group_ids, ) + class TestUnicodeAssertionData(unit.BaseTestCase): """Ensure that unicode data in the assertion headers works. diff --git a/keystone/tests/unit/mapping_fixtures.py b/keystone/tests/unit/mapping_fixtures.py index 0a9d194e28..341ba897ae 100644 --- a/keystone/tests/unit/mapping_fixtures.py +++ b/keystone/tests/unit/mapping_fixtures.py @@ -1494,6 +1494,35 @@ MAPPING_GROUPS_WITH_EMAIL = { ] } + +MAPPING_GROUPS_DOMAIN_OF_USER = { + "rules": [ + { + "local": + [ + { + "user": + { + "name": "{0}" + } + }, + { + "groups": "{1}" + } + ], + "remote": + [ + { + "type": "openstack_user" + }, + { + "type": "openstack_groups" + } + ] + } + ] +} + EMPLOYEE_ASSERTION = { 'Email': 'tim@example.com', 'UserName': 'tbo', @@ -1652,6 +1681,14 @@ GROUPS_ASSERTION_ONLY_ONE_GROUP = { 'groups': 'ALL USERS' } +GROUPS_DOMAIN_ASSERTION = { + 'openstack_user': 'bwilliams', + 'openstack_user_domain': 'default', + 'openstack_roles': 'Admin', + 'openstack_groups': 'JSON:{"name":"group1","domain":{"name":"xxx"}};' + 'JSON:{"name":"group2","domain":{"name":"yyy"}}' +} + MAPPING_UNICODE = { "rules": [ { diff --git a/keystone/tests/unit/saml2/signed_saml2_assertion.xml b/keystone/tests/unit/saml2/signed_saml2_assertion.xml index 414ff9cf77..1e7bd0db25 100644 --- a/keystone/tests/unit/saml2/signed_saml2_assertion.xml +++ b/keystone/tests/unit/saml2/signed_saml2_assertion.xml @@ -65,5 +65,9 @@ UHeBXxQq/GmfBv3l+V5ObQ+EHKnyDodLHCk= project_domain + + JSON:{"name":"group1","domain":{"name":"Default"}} + JSON:{"name":"group2","domain":{"name":"Default"}} + diff --git a/keystone/tests/unit/test_v3_federation.py b/keystone/tests/unit/test_v3_federation.py index a24ddfc464..61175a17ed 100644 --- a/keystone/tests/unit/test_v3_federation.py +++ b/keystone/tests/unit/test_v3_federation.py @@ -3818,6 +3818,8 @@ class SAMLGenerationTests(test_v3.RestfulTestCase): ROLES = ['admin', 'member'] PROJECT = 'development' PROJECT_DOMAIN = 'project_domain' + GROUPS = ['JSON:{"name":"group1","domain":{"name":"Default"}}', + 'JSON:{"name":"group2","domain":{"name":"Default"}}'] SAML_GENERATION_ROUTE = '/auth/OS-FEDERATION/saml2' ECP_GENERATION_ROUTE = '/auth/OS-FEDERATION/saml2/ecp' ASSERTION_VERSION = "2.0" @@ -3849,7 +3851,8 @@ class SAMLGenerationTests(test_v3.RestfulTestCase): self.SUBJECT, self.SUBJECT_DOMAIN, self.ROLES, self.PROJECT, - self.PROJECT_DOMAIN) + self.PROJECT_DOMAIN, + self.GROUPS) assertion = response.assertion self.assertIsNotNone(assertion) @@ -3879,6 +3882,10 @@ class SAMLGenerationTests(test_v3.RestfulTestCase): self.assertEqual(self.PROJECT_DOMAIN, project_domain_attribute.attribute_value[0].text) + group_attribute = assertion.attribute_statement[0].attribute[5] + for attribute_value in group_attribute.attribute_value: + self.assertIn(attribute_value.text, self.GROUPS) + def test_comma_in_certfile_path(self): self.config_fixture.config( group='saml', @@ -3893,7 +3900,8 @@ class SAMLGenerationTests(test_v3.RestfulTestCase): self.SUBJECT_DOMAIN, self.ROLES, self.PROJECT, - self.PROJECT_DOMAIN) + self.PROJECT_DOMAIN, + self.GROUPS) def test_comma_in_keyfile_path(self): self.config_fixture.config( @@ -3909,7 +3917,8 @@ class SAMLGenerationTests(test_v3.RestfulTestCase): self.SUBJECT_DOMAIN, self.ROLES, self.PROJECT, - self.PROJECT_DOMAIN) + self.PROJECT_DOMAIN, + self.GROUPS) def test_verify_assertion_object(self): """Test that the Assertion object is built properly. @@ -3925,7 +3934,8 @@ class SAMLGenerationTests(test_v3.RestfulTestCase): self.SUBJECT, self.SUBJECT_DOMAIN, self.ROLES, self.PROJECT, - self.PROJECT_DOMAIN) + self.PROJECT_DOMAIN, + self.GROUPS) assertion = response.assertion self.assertEqual(self.ASSERTION_VERSION, assertion.version) @@ -3944,7 +3954,8 @@ class SAMLGenerationTests(test_v3.RestfulTestCase): self.SUBJECT, self.SUBJECT_DOMAIN, self.ROLES, self.PROJECT, - self.PROJECT_DOMAIN) + self.PROJECT_DOMAIN, + self.GROUPS) saml_str = response.to_string() response = etree.fromstring(saml_str) @@ -3970,6 +3981,10 @@ class SAMLGenerationTests(test_v3.RestfulTestCase): project_domain_attribute = assertion[4][4] self.assertEqual(self.PROJECT_DOMAIN, project_domain_attribute[0].text) + group_attribute = assertion[4][5] + for attribute_value in group_attribute: + self.assertIn(attribute_value.text, self.GROUPS) + def test_assertion_using_explicit_namespace_prefixes(self): def mocked_subprocess_check_output(*popenargs, **kwargs): # the last option is the assertion file to be signed @@ -3988,7 +4003,8 @@ class SAMLGenerationTests(test_v3.RestfulTestCase): self.SUBJECT, self.SUBJECT_DOMAIN, self.ROLES, self.PROJECT, - self.PROJECT_DOMAIN) + self.PROJECT_DOMAIN, + self.GROUPS) assertion_xml = response.assertion.to_string() # The expected values in the assertions bellow need to be 'str' in # Python 2 and 'bytes' in Python 3 @@ -4017,7 +4033,8 @@ class SAMLGenerationTests(test_v3.RestfulTestCase): response = generator.samlize_token(self.ISSUER, self.RECIPIENT, self.SUBJECT, self.SUBJECT_DOMAIN, self.ROLES, self.PROJECT, - self.PROJECT_DOMAIN) + self.PROJECT_DOMAIN, + self.GROUPS) signature = response.assertion.signature self.assertIsNotNone(signature) @@ -4132,6 +4149,9 @@ class SAMLGenerationTests(test_v3.RestfulTestCase): project_domain_attribute = assertion[4][4] self.assertIsInstance(project_domain_attribute[0].text, str) + group_attribute = assertion[4][5] + self.assertIsInstance(group_attribute[0].text, str) + def test_invalid_scope_body(self): """Test that missing the scope in request body raises an exception. @@ -4247,6 +4267,9 @@ class SAMLGenerationTests(test_v3.RestfulTestCase): project_domain_attribute = assertion[4][4] self.assertIsInstance(project_domain_attribute[0].text, str) + group_attribute = assertion[4][5] + self.assertIsInstance(group_attribute[0].text, str) + @mock.patch('saml2.create_class_from_xml_string') @mock.patch('oslo_utils.fileutils.write_to_tempfile') @mock.patch.object(subprocess, 'check_output') diff --git a/releasenotes/notes/bug-1641625-fe463874dc5edb10.yaml b/releasenotes/notes/bug-1641625-fe463874dc5edb10.yaml new file mode 100644 index 0000000000..035338ee00 --- /dev/null +++ b/releasenotes/notes/bug-1641625-fe463874dc5edb10.yaml @@ -0,0 +1,7 @@ +--- +features: + - | + [`bug 1641625 `_] + The keystone configured as an identity provider now includes an additional + attribute called `openstack_groups` in the assertion when generating SAML + assertions.