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

[Dmitrii Shcherbakov]

09eb392... by Dmitrii Shcherbakov on 2019-07-07

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)

[Christian Ehrhardt  (paelzer)]

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

[Christian Ehrhardt  (paelzer)]

Thanks for adding the requested details, e.g. SRU template.

[Christian Ehrhardt  (paelzer)]

Note a few things for further improvements of such MPs in the future:
- The target release should obviously not be "UNRELEASED"
- One (of many) reasons for "git ubuntu" is to ease rebases for new merges.
  To do that changelog and other commits should be separate.

I fixed those up for you on sponsoring for now ...
Tagged and sponsored into Eoan

Test built all (fixed up) builds in PPA https://launchpad.net/~paelzer/+archive/ubuntu/bug-1833299-testbuilds-before-sponsor-lasso

tag: upload/2.6.0-2ubuntu1

diff --git a/debian/changelog b/debian/changelog
index 7770a57..9067135 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+lasso (2.6.0-2ubuntu1) UNRELEASED; urgency=high
+
+  * d/p/PAOS-Do-not-populate-Destination-attribute.patch: Do not populate
+    "Destination" attribute (LP: #1833299)
+
+ -- Dmitrii Shcherbakov <[e-mail]>  Thu, 04 Jul 2019 18:42:56 -0500
+
 lasso (2.6.0-2build1) disco; urgency=medium
 
   * No-change rebuild to build without python3.6 support.
diff --git a/debian/patches/PAOS-Do-not-populate-Destination-attribute.patch b/debian/patches/PAOS-Do-not-populate-Destination-attribute.patch
new file mode 100644
index 0000000..3594f81
--- /dev/null
+++ b/debian/patches/PAOS-Do-not-populate-Destination-attribute.patch
@@ -0,0 +1,93 @@
+Description: 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.
+Author: Dmitrii Shcherbakov <[e-mail]>
+Origin: upstream, https://repos.entrouvert.org/lasso.git/commit/?id=1e85f1b2bd30c0d93b4a2ef37b35abeae3d15b56
+Bug: https://dev.entrouvert.org/issues/34409
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/lasso/+bug/1833299
+Applied-Upstream: https://repos.entrouvert.org/lasso.git/commit/?id=1e85f1b2bd30c0d93b4a2ef37b35abeae3d15b56
+Reviewed-by: Benjamin Dauvergne <[e-mail]>
+Reviewed-by: John Dennis <[e-mail]>
+Last-Update: 2019-07-06
+---
+This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
+Index: lasso/lasso/saml-2.0/login.c
+===================================================================
+--- lasso.orig/lasso/saml-2.0/login.c
++++ lasso/lasso/saml-2.0/login.c
+@@ -222,7 +222,7 @@ _lasso_login_must_verify_signature(Lasso
+ gint
+ lasso_saml20_login_build_authn_request_msg(LassoLogin *login)
+ {
+-	char *url = NULL;
++	char *assertionConsumerServiceURL = NULL;
+ 	gboolean must_sign = TRUE;
+ 	LassoProfile *profile;
+ 	LassoSamlp2AuthnRequest *authn_request;
+@@ -247,29 +247,29 @@ lasso_saml20_login_build_authn_request_m
+ 	}
+ 
+ 	if (login->http_method == LASSO_HTTP_METHOD_PAOS) {
+-
+ 		/*
+ 		 * PAOS is special, the url passed to build_request is the
+ 		 * AssertionConsumerServiceURL of this SP, not the
+-		 * destination.
++		 * destination IdP URL. This is done to fill paos:responseConsumerURL
++		 * appropriately down the line in build_request_msg.
++		 * See https://dev.entrouvert.org/issues/34409 for more information.
+ 		 */
+ 		if (authn_request->AssertionConsumerServiceURL) {
+-			url = authn_request->AssertionConsumerServiceURL;
++			assertionConsumerServiceURL = authn_request->AssertionConsumerServiceURL;
+ 			if (!lasso_saml20_provider_check_assertion_consumer_service_url(
+-					LASSO_PROVIDER(profile->server), url, LASSO_SAML2_METADATA_BINDING_PAOS)) {
++					LASSO_PROVIDER(profile->server), assertionConsumerServiceURL, LASSO_SAML2_METADATA_BINDING_PAOS)) {
+ 				rc = LASSO_PROFILE_ERROR_INVALID_REQUEST;
+ 				goto cleanup;
+ 			}
+ 		} else {
+-			url = lasso_saml20_provider_get_assertion_consumer_service_url_by_binding(
++			assertionConsumerServiceURL = lasso_saml20_provider_get_assertion_consumer_service_url_by_binding(
+ 					LASSO_PROVIDER(profile->server), LASSO_SAML2_METADATA_BINDING_PAOS);
+-			lasso_assign_new_string(authn_request->AssertionConsumerServiceURL, url);
++			lasso_assign_new_string(authn_request->AssertionConsumerServiceURL, assertionConsumerServiceURL);
+ 		}
+ 	}
+ 
+-
+ 	lasso_check_good_rc(lasso_saml20_profile_build_request_msg(profile, "SingleSignOnService",
+-				login->http_method, url));
++				login->http_method, assertionConsumerServiceURL));
+ 
+ cleanup:
+ 	return rc;
+Index: lasso/lasso/saml-2.0/profile.c
+===================================================================
+--- lasso.orig/lasso/saml-2.0/profile.c
++++ lasso/lasso/saml-2.0/profile.c
+@@ -967,7 +967,15 @@ lasso_saml20_profile_build_request_msg(L
+ 		made_url = url = get_url(provider, service, http_method_to_binding(method));
+ 	}
+ 
+-	if (url) {
++
++	// Usage of the Destination attribute on a request is mandated only
++	// in "3.4.5.2" and "3.5.5.2" in saml-bindings-2.0-os for signed requests
++	// and is marked as optional in the XSD schema otherwise.
++	// PAOS is a special case because an SP does not select an IdP - ECP does
++	// it instead. Therefore, this attribute needs to be left unpopulated.
++	if (method == LASSO_HTTP_METHOD_PAOS) {
++		lasso_release_string(((LassoSamlp2RequestAbstract*)profile->request)->Destination);
++	} else if (url) {
+ 		lasso_assign_string(((LassoSamlp2RequestAbstract*)profile->request)->Destination,
+ 				url);
+ 	} else {
diff --git a/debian/patches/series b/debian/patches/series
index e69de29..fface8c 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -0,0 +1 @@
+PAOS-Do-not-populate-Destination-attribute.patch