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

[Dmitrii Shcherbakov]

[Christian Ehrhardt  (paelzer)]

Hi Dmitri,
I can't do a full review yet, but I have skimmed over things and have a few questions upfront that would block others as well I think.

Changing the source directly will not work but we could convert that for you if needed.
You'll need to use changes in debian/patches/ using (d)quilt.

More importantly this seems to become a maintenance dept forever.
Could you add references that one would add on patch headers
Especially the source of the changes is of interest, to know when we can drop having Delta.

(same applies to the other branches)

To help here my template - pick what applies from below and add it when creating a proper patch in debian/patches/..

--- for own changes ---
Description: <todo-desc>
 long-text
Forwarded: yes/no/not-needed(explain in the desc why)
Author: Christian Ehrhardt <email address hidden>
Origin: <todo-URL-to-git or similar>
Bug-Ubuntu: https://bugs.launchpad.net/bugs/<todo-bug>
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=<todo-bug>
Last-Update: 2019-03-17

--- for backports ---
<Carry description from source>

# IF MODIFIED
Author: Christian Ehrhardt <email address hidden>
Original-Author: <todo-author>
Origin: backport, <todo-URL-to-git>
# OTHERWISE
Origin: upstream, <todo-URL-to-git>

Bug-Ubuntu: https://bugs.launchpad.net/bugs/<todo-bug>
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=<todo-bug>
Last-Update: 2019-05-19

--- for external fixes that are brough upstream by 3rd party ---
<Carry description from source>
Forwarded: yes/no/not-needed(explain in the desc why)
Author: TODO
Origin: url (explain in desc why this is uncommon e.g. no upstream repo)
Bug-Ubuntu: https://bugs.launchpad.net/bugs/<todo-bug>
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=<todo-bug>
Last-Update: 2019-05-19

review: Needs Fixing

[Dmitrii Shcherbakov (dmitriis)]

> Especially the source of the changes is of interest, to know when we can drop having Delta.

Yes, the source of changes is this patch:
https://dev.entrouvert.org/projects/lasso/repository/revisions/1e85f1b2bd30c0d93b4a2ef37b35abeae3d15b56/diff
https://dev.entrouvert.org/issues/34409

The fix should appear in releases >2.6.0 upstream.

I can convert this to a patch and place it under debian/patches/ and add patch headers.

376cd4b... 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)

[Dmitrii Shcherbakov (dmitriis)]

Christian,

Updated the change with the patch moved under debian/patches.

[Christian Ehrhardt  (paelzer)]

Thanks for updating this and the other branches.
To further polish you might mention the name of the patch in the changelog.
Like:
  * d/p/PAOS-Do-not-populate-Destination-attribute.patch: Do not populate "Destination" attribute (LP: #1833299)

Further you don't need the [ Name ] on top of the changes, a sponsor would create that if he replaces your name at the bottom.

But both of these are "style" and no show stopper.
It LGTM now.

I'll +1 on all MPs but you'll also need a SRU template on the bug before this can be sponsored for pre-Eoan.

review: Approve

[Christian Ehrhardt  (paelzer)]

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

[Christian Ehrhardt  (paelzer)]

Same FYI as on Eoan, changelog and functional commit should be split and target release not be "UNRELEASED".

ANd in this case versioning will not increment the first number after ubuntu to 2 but become 1.1 in this SRU case.

I recommend reading https://wiki.ubuntu.com/SecurityTeam/UpdatePreparation
For now on this MP I have all fixed up prior to the upload.

tag: upload/2.5.1-0ubuntu1.1

Tagged and sponsored into SRU queue.

diff --git a/debian/changelog b/debian/changelog
index dc3e645..85960e6 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,10 @@
+lasso (2.5.1-0ubuntu2) 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.5.1-0ubuntu1) bionic; urgency=medium
 
   * New upstream stable point release.
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..459603d
--- /dev/null
+++ b/debian/patches/PAOS-Do-not-populate-Destination-attribute.patch
@@ -0,0 +1,89 @@
+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/
+--- a/lasso/saml-2.0/login.c
++++ b/lasso/saml-2.0/login.c
+@@ -222,7 +222,7 @@
+ 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 @@
+ 	}
+ 
+ 	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;
+--- a/lasso/saml-2.0/profile.c
++++ b/lasso/saml-2.0/profile.c
+@@ -968,7 +968,15 @@
+ 		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 90e7b19..da56810 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1 +1,2 @@
+PAOS-Do-not-populate-Destination-attribute.patch
 Use-INSTALLDIRS-vendor-for-the-Perl-bindings-as-per-.patch