Merge lp:~roadmr/canonical-identity-provider/support-saml-persistent-nameid-policy-format into lp:canonical-identity-provider

Proposed by Daniel Manrique on 2019-01-18
Status: Merged
Approved by: Daniel Manrique on 2019-01-24
Approved revision: 1681
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: lp:~roadmr/canonical-identity-provider/support-saml-persistent-nameid-policy-format
Merge into: lp:canonical-identity-provider
Prerequisite: lp:~roadmr/canonical-identity-provider/honor-authnrequest-nameid-policy-format-field
Diff against target: 140 lines (+88/-5)
2 files modified
src/ubuntu_sso_saml/processors.py (+15/-0)
src/ubuntu_sso_saml/tests/test_processors.py (+73/-5)
To merge this branch: bzr merge lp:~roadmr/canonical-identity-provider/support-saml-persistent-nameid-policy-format
Reviewer Review Type Date Requested Status
Maximiliano Bertacchini 2019-01-18 Approve on 2019-01-23
Review via email: mp+361982@code.launchpad.net

Commit message

Actually honor SAML AuthnRequests' NameIDPolicy format.

This was done to accommodate SPs (Bomgar) which request persistent names,
indicating so in their AuthnRequest. SSO usually ignores this and always
sends "email"-policy names, which most SPs handle fine but Bomgar fails with.

The fully-correct thing to do would be to always honor the AuthnRequest and
support most/all of the SAML-specified policies, but that'd be a large effort
and risks changing semantics for other SPs, which would prevent people from
logging in, which would be bad.

Another quirk is that, while we respond saying we're giving a persistent
identifier, our identifier (the e-mail address) does NOT actually conform
to persistent semantics per SAML spec's section 8.3; we are sending the
same value (the e-mail address), just saying it's "persistent" as requested
by the SP. We do have a persistent identifier (the OpenID) but we can't send that
because then it gets sent as the username, identifier, and email address. Again,
support for this can be added to our django-saml2-idp fork but it's more work
for something that at the moment is required only by one SP.

Due do the above, in order to support Bomgar (and possibly other SPs) in a more
selective way, we only honor a non-email NameIDPolicy if:
    - the SP is configured to honor this (a boolean in the SPConfig)
    - the requested NameID policy is "persistent".

This allows us to switch this on only for very specific SPs for which we
have more control and fully understand the consequences of "lying" with our
"persistent" support.

In all other cases, we continue ignoring/overriding this and always sending
our response with "email" policy.

(As a rant, the best thing to do would be to trash our hacky SAML library
and integrate OneLogin's SAML library which is fully standards-compliant,
which is however a huge undertaking we would have to consider and prioritize)

Description of the change

This change was requested by the support folks (Mark Thomas) to allow them to use SAML authentication on their Bomgar box (for providing remote screensharing for support purposes), avoiding the need to add users manually.

Actually honor SAML AuthnRequests' NameIDPolicy format.

This was done to accommodate SPs (Bomgar) which request persistent names,
indicating so in their AuthnRequest. SSO usually ignores this and always
sends "email"-policy names, which most SPs handle fine but Bomgar fails with.

The fully-correct thing to do would be to always honor the AuthnRequest and
support most/all of the SAML-specified policies, but that'd be a large effort
and risks changing semantics for other SPs, which would prevent people from
logging in, which would be bad.

Another quirk is that, while we respond saying we're giving a persistent
identifier, our identifier (the e-mail address) does NOT actually conform
to persistent semantics per SAML spec's section 8.3; we are sending the
same value (the e-mail address), just saying it's "persistent" as requested
by the SP. We do have a persistent identifier (the OpenID) but we can't send that
because then it gets sent as the username, identifier, and email address. Again,
support for this can be added to our django-saml2-idp fork but it's more work
for something that at the moment is required only by one SP.

Due do the above, in order to support Bomgar (and possibly other SPs) in a more
selective way, we only honor a non-email NameIDPolicy if:
    - the SP is configured to honor this (a boolean in the SPConfig)
    - the requested NameID policy is "persistent".

This allows us to switch this on only for very specific SPs for which we
have more control and fully understand the consequences of "lying" with our
"persistent" support.

In all other cases, we continue ignoring/overriding this and always sending
our response with "email" policy.

(As a rant, the best thing to do would be to trash our hacky SAML library
and integrate OneLogin's SAML library which is fully standards-compliant,
which is however a huge undertaking we would have to consider and prioritize)

To post a comment you must log in.
1681. By Daniel Manrique on 2019-01-18

Merged honor-authnrequest-nameid-policy-format-field into support-saml-persistent-nameid-policy-format.

Maximiliano Bertacchini (maxiberta) wrote :

I'm not too familiar with this, but looks good to me.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/ubuntu_sso_saml/processors.py'
2--- src/ubuntu_sso_saml/processors.py 2018-04-10 16:02:31 +0000
3+++ src/ubuntu_sso_saml/processors.py 2019-01-18 21:02:45 +0000
4@@ -338,6 +338,21 @@
5 params['PROVIDER_NAME'] = request.get('providername', '')
6 self._request_params = params
7
8+ # Use a different nameid format if the spconfig allows honoring it
9+ # and the requested format is supported (currently only "email" and
10+ # "persistent").
11+ sp_config = get_config_from_processor(self)
12+ if (sp_config is not None and
13+ sp_config.honor_authnrequest_nameidpolicy_format):
14+ nameidpolicy = request.findChild('samlp:nameidpolicy')
15+ if nameidpolicy is not None:
16+ requested_format = nameidpolicy.get('format', 'email')
17+ requested_format = requested_format.split(":")[-1]
18+ if requested_format not in ("email", "persistent"):
19+ requested_format = "email"
20+ template = 'urn:oasis:names:tc:SAML:2.0:nameid-format:{fmt}'
21+ self._subject_format = template.format(fmt=requested_format)
22+
23 def _determine_audience(self):
24 self._audience = super(CanonicalProcessor, self)._determine_audience()
25
26
27=== modified file 'src/ubuntu_sso_saml/tests/test_processors.py'
28--- src/ubuntu_sso_saml/tests/test_processors.py 2018-05-28 19:00:39 +0000
29+++ src/ubuntu_sso_saml/tests/test_processors.py 2019-01-18 21:02:45 +0000
30@@ -1154,7 +1154,7 @@
31 >
32 <saml:Issuer xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">{issuer}</saml:Issuer>
33 <samlp:NameIDPolicy xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
34- Format="urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress"
35+ Format="urn:oasis:names:tc:SAML:1.1:nameid-format:{nameid_format}"
36 AllowCreate="true"
37 />
38 <samlp:RequestedAuthnContext xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
39@@ -1178,7 +1178,7 @@
40 >
41 <saml:Issuer xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">{issuer}</saml:Issuer>
42 <samlp:NameIDPolicy xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
43- Format="urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress"
44+ Format="urn:oasis:names:tc:SAML:1.1:nameid-format:{nameid_format}"
45 AllowCreate="true"
46 />
47 <samlp:RequestedAuthnContext xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
48@@ -1220,14 +1220,28 @@
49 self.assertNotIn('<saml:AudienceRestriction>', samlresponse)
50
51 def assert_saml_response_email(self, samlresponse, email):
52- self.assertIn(email, samlresponse)
53-
54- def get_request_data(self, compressed=True, has_acs_url=True):
55+ self.assert_nameid(samlresponse, email, "email")
56+
57+ def assert_nameid(self, samlresponse, email, the_format):
58+ # The_format can be "email" or "persistent" currently.
59+ full_format = "urn:oasis:names:tc:SAML:2.0:nameid-format:" + the_format
60+ samlsoup = PyQuery(samlresponse.replace('xmlns:', 'xmlnamespace:'))
61+ nameids = samlsoup.find('NameID')
62+ self.assertEqual(1, len(nameids))
63+ self.assertEqual(full_format, nameids[0].attrib['format'])
64+ self.assertEqual(email, nameids[0].text)
65+
66+ def get_request_data(
67+ self,
68+ compressed=True,
69+ has_acs_url=True,
70+ nameid_format="emailAddress"):
71 data = dict(**self.REQUEST_DATA)
72 params = {
73 'audience': self.AUDIENCE,
74 'issuer': self.REQUEST_ISSUER,
75 'relay_state': self.RELAY_STATE,
76+ 'nameid_format': nameid_format,
77 }
78 if has_acs_url:
79 params['acs_url'] = self.ACS_URL
80@@ -1271,6 +1285,60 @@
81 self.assert_successful_saml_response(samlresponse)
82 self.assert_unrestricted_saml_response(samlresponse)
83
84+ def test_nameid_honor_custom_format_persistent_requested(self):
85+ # make sure there is a saml config which honors nameid format from
86+ # authnrequest
87+ self.setup_saml_sp(honor_authnrequest_nameidpolicy_format=True)
88+
89+ data = self.get_request_data(nameid_format="persistent")
90+ samlresponse = self.do_saml_request(data=data)
91+ self.assert_successful_saml_response(samlresponse)
92+ self.assert_nameid(samlresponse, self.login_email, "persistent")
93+
94+ def test_nameid_dont_honor_custom_format_persistent_requested(self):
95+ self.setup_saml_sp(honor_authnrequest_nameidpolicy_format=False)
96+
97+ data = self.get_request_data(nameid_format="persistent")
98+ samlresponse = self.do_saml_request(data=data)
99+ self.assert_successful_saml_response(samlresponse)
100+ self.assert_nameid(samlresponse, self.login_email, "email")
101+
102+ def test_nameid_honor_custom_format_email_requested(self):
103+ # make sure there is a saml config which honors nameid format from
104+ # authnrequest
105+ self.setup_saml_sp(honor_authnrequest_nameidpolicy_format=True)
106+
107+ data = self.get_request_data(nameid_format="email")
108+ samlresponse = self.do_saml_request(data=data)
109+ self.assert_successful_saml_response(samlresponse)
110+ self.assert_nameid(samlresponse, self.login_email, "email")
111+
112+ def test_nameid_dont_honor_custom_format_email_requested(self):
113+ self.setup_saml_sp(honor_authnrequest_nameidpolicy_format=False)
114+
115+ data = self.get_request_data(nameid_format="email")
116+ samlresponse = self.do_saml_request(data=data)
117+ self.assert_successful_saml_response(samlresponse)
118+ self.assert_nameid(samlresponse, self.login_email, "email")
119+
120+ def test_nameid_honor_custom_format_bogus_requested(self):
121+ # make sure there is a saml config which honors nameid format from
122+ # authnrequest
123+ self.setup_saml_sp(honor_authnrequest_nameidpolicy_format=True)
124+
125+ data = self.get_request_data(nameid_format="nonexistentandbogus")
126+ samlresponse = self.do_saml_request(data=data)
127+ self.assert_successful_saml_response(samlresponse)
128+ self.assert_nameid(samlresponse, self.login_email, "email")
129+
130+ def test_nameid_dont_honor_custom_format_bogus_requested(self):
131+ self.setup_saml_sp(honor_authnrequest_nameidpolicy_format=False)
132+
133+ data = self.get_request_data(nameid_format="nonexistentandbogus")
134+ samlresponse = self.do_saml_request(data=data)
135+ self.assert_successful_saml_response(samlresponse)
136+ self.assert_nameid(samlresponse, self.login_email, "email")
137+
138 def test_authnrequest_unknown_sp(self):
139 data = self.get_request_data()
140 response = self.client.get('/+saml', data=data, follow=True)