Ubuntu
lasso package

Merge ~dmitriis/ubuntu/+source/lasso:1833299-cosmic-devel-paos-ecp-destination into ubuntu/+source/lasso:ubuntu/cosmic-devel

  1. Git
  2. lp:~dmitriis/ubuntu/+source/lasso
  3. 1833299-cosmic-devel-paos-ecp-destination
  4. Merge into ubuntu/cosmic-devel
Proposed by Dmitrii Shcherbakov
Status: Rejected
Rejected by: Christian Ehrhardt 
Proposed branch: ~dmitriis/ubuntu/+source/lasso:1833299-cosmic-devel-paos-ecp-destination
Merge into: ubuntu/+source/lasso:ubuntu/cosmic-devel
Diff against target: 117 lines (+97/-0)
3 files modified
debian/changelog (+7/-0)
debian/patches/PAOS-Do-not-populate-Destination-attribute.patch (+89/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Christian Ehrhardt  (community) Approve
Canonical Server Pending
Review via email: mp+369737@code.launchpad.net
To post a comment you must log in.
~dmitriis/ubuntu/+source/lasso:1833299-cosmic-devel-paos-ecp-destination updated
d8dca33... by Dmitrii Shcherbakov

PAOS: Do not populate "Destination" attribute

When ECP profile (saml-ecp-v2.0-cs01) is used with PAOS binding Lasso
populates an AuthnRequest with the "Destination" attribute set to
AssertionConsumerURL of an SP - this leads to IdP-side errors because
the destination attribute in the request does not match the IdP URL.

The "Destination" attribute is mandatory only for HTTP Redirect and HTTP
Post bindings when AuthRequests are signed per saml-bindings-2.0-os
(sections 3.4.5.2 and 3.5.5.2). Specifically for PAOS it makes sense to
avoid setting that optional attribute because an ECP decides which IdP
to use, not the SP.

This patch was merged upstream: https://dev.entrouvert.org/issues/34409

New changelog entries:

* d/p/PAOS-Do-not-populate-Destination-attribute.patch: Do not populate
  "Destination" attribute (LP: #1833299)

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

ack, see details in Bionic MP https://code.launchpad.net/~dmitriis/ubuntu/+source/lasso/+git/lasso/+merge/369738

review: Approve
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

I think we are so close to Cosmic EOL that we don't need to care about it anymore.
Let me know if you think otherwise.

Revision history for this message
Dmitrii Shcherbakov (dmitriis) wrote :

I'm good with skipping Cosmic.

Unmerged commits

d8dca33... by Dmitrii Shcherbakov

PAOS: Do not populate "Destination" attribute

When ECP profile (saml-ecp-v2.0-cs01) is used with PAOS binding Lasso
populates an AuthnRequest with the "Destination" attribute set to
AssertionConsumerURL of an SP - this leads to IdP-side errors because
the destination attribute in the request does not match the IdP URL.

The "Destination" attribute is mandatory only for HTTP Redirect and HTTP
Post bindings when AuthRequests are signed per saml-bindings-2.0-os
(sections 3.4.5.2 and 3.5.5.2). Specifically for PAOS it makes sense to
avoid setting that optional attribute because an ECP decides which IdP
to use, not the SP.

This patch was merged upstream: https://dev.entrouvert.org/issues/34409

New changelog entries:

* d/p/PAOS-Do-not-populate-Destination-attribute.patch: Do not populate
  "Destination" attribute (LP: #1833299)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/debian/changelog b/debian/changelog
index c3dff7b..1511d90 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
1lasso (2.5.1-0ubuntu3) UNRELEASED; urgency=medium
2
3 * d/p/PAOS-Do-not-populate-Destination-attribute.patch: Do not populate
4 "Destination" attribute (LP: #1833299)
5
6 -- Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com> Thu, 04 Jul 2019 18:42:56 -0500
7
1lasso (2.5.1-0ubuntu2) cosmic; urgency=medium8lasso (2.5.1-0ubuntu2) cosmic; urgency=medium
29
3 * No-change rebuild to build for python3.7.10 * No-change rebuild to build for python3.7.
diff --git a/debian/patches/PAOS-Do-not-populate-Destination-attribute.patch b/debian/patches/PAOS-Do-not-populate-Destination-attribute.patch
4new file mode 10064411new file mode 100644
index 0000000..459603d
--- /dev/null
+++ b/debian/patches/PAOS-Do-not-populate-Destination-attribute.patch
@@ -0,0 +1,89 @@
1Description: PAOS: Do not populate "Destination" attribute
2 When ECP profile (saml-ecp-v2.0-cs01) is used with PAOS binding Lasso
3 populates an AuthnRequest with the "Destination" attribute set to
4 AssertionConsumerURL of an SP - this leads to IdP-side errors because
5 the destination attribute in the request does not match the IdP URL.
6
7 The "Destination" attribute is mandatory only for HTTP Redirect and HTTP
8 Post bindings when AuthRequests are signed per saml-bindings-2.0-os
9 (sections 3.4.5.2 and 3.5.5.2). Specifically for PAOS it makes sense to
10 avoid setting that optional attribute because an ECP decides which IdP
11 to use, not the SP.
12Author: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com>
13Origin: upstream, https://repos.entrouvert.org/lasso.git/commit/?id=1e85f1b2bd30c0d93b4a2ef37b35abeae3d15b56
14Bug: https://dev.entrouvert.org/issues/34409
15Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/lasso/+bug/1833299
16Applied-Upstream: https://repos.entrouvert.org/lasso.git/commit/?id=1e85f1b2bd30c0d93b4a2ef37b35abeae3d15b56
17Reviewed-by: Benjamin Dauvergne <bdauvergne@entrouvert.com>
18Reviewed-by: John Dennis <jdennis@redhat.com>
19Last-Update: 2019-07-06
20---
21This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
22--- a/lasso/saml-2.0/login.c
23+++ b/lasso/saml-2.0/login.c
24@@ -222,7 +222,7 @@
25 gint
26 lasso_saml20_login_build_authn_request_msg(LassoLogin *login)
27 {
28- char *url = NULL;
29+ char *assertionConsumerServiceURL = NULL;
30 gboolean must_sign = TRUE;
31 LassoProfile *profile;
32 LassoSamlp2AuthnRequest *authn_request;
33@@ -247,29 +247,29 @@
34 }
35
36 if (login->http_method == LASSO_HTTP_METHOD_PAOS) {
37-
38 /*
39 * PAOS is special, the url passed to build_request is the
40 * AssertionConsumerServiceURL of this SP, not the
41- * destination.
42+ * destination IdP URL. This is done to fill paos:responseConsumerURL
43+ * appropriately down the line in build_request_msg.
44+ * See https://dev.entrouvert.org/issues/34409 for more information.
45 */
46 if (authn_request->AssertionConsumerServiceURL) {
47- url = authn_request->AssertionConsumerServiceURL;
48+ assertionConsumerServiceURL = authn_request->AssertionConsumerServiceURL;
49 if (!lasso_saml20_provider_check_assertion_consumer_service_url(
50- LASSO_PROVIDER(profile->server), url, LASSO_SAML2_METADATA_BINDING_PAOS)) {
51+ LASSO_PROVIDER(profile->server), assertionConsumerServiceURL, LASSO_SAML2_METADATA_BINDING_PAOS)) {
52 rc = LASSO_PROFILE_ERROR_INVALID_REQUEST;
53 goto cleanup;
54 }
55 } else {
56- url = lasso_saml20_provider_get_assertion_consumer_service_url_by_binding(
57+ assertionConsumerServiceURL = lasso_saml20_provider_get_assertion_consumer_service_url_by_binding(
58 LASSO_PROVIDER(profile->server), LASSO_SAML2_METADATA_BINDING_PAOS);
59- lasso_assign_new_string(authn_request->AssertionConsumerServiceURL, url);
60+ lasso_assign_new_string(authn_request->AssertionConsumerServiceURL, assertionConsumerServiceURL);
61 }
62 }
63
64-
65 lasso_check_good_rc(lasso_saml20_profile_build_request_msg(profile, "SingleSignOnService",
66- login->http_method, url));
67+ login->http_method, assertionConsumerServiceURL));
68
69 cleanup:
70 return rc;
71--- a/lasso/saml-2.0/profile.c
72+++ b/lasso/saml-2.0/profile.c
73@@ -968,7 +968,15 @@
74 made_url = url = get_url(provider, service, http_method_to_binding(method));
75 }
76
77- if (url) {
78+
79+ // Usage of the Destination attribute on a request is mandated only
80+ // in "3.4.5.2" and "3.5.5.2" in saml-bindings-2.0-os for signed requests
81+ // and is marked as optional in the XSD schema otherwise.
82+ // PAOS is a special case because an SP does not select an IdP - ECP does
83+ // it instead. Therefore, this attribute needs to be left unpopulated.
84+ if (method == LASSO_HTTP_METHOD_PAOS) {
85+ lasso_release_string(((LassoSamlp2RequestAbstract*)profile->request)->Destination);
86+ } else if (url) {
87 lasso_assign_string(((LassoSamlp2RequestAbstract*)profile->request)->Destination,
88 url);
89 } else {
diff --git a/debian/patches/series b/debian/patches/series
index 90e7b19..da56810 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1 +1,2 @@
1PAOS-Do-not-populate-Destination-attribute.patch
1Use-INSTALLDIRS-vendor-for-the-Perl-bindings-as-per-.patch2Use-INSTALLDIRS-vendor-for-the-Perl-bindings-as-per-.patch

Subscribers

People subscribed via source and target branches