From bf2727ede5ade81fdd7e59d82d8f4811032ac8b1 Mon Sep 17 00:00:00 2001
From: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com>
Date: Fri, 28 Jun 2019 02:36:19 +0300
Subject: [PATCH] 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.

Fixes Bug: 34409
Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com>
---
 lasso/saml-2.0/login.c   | 27 +++++++++++++++++----------
 lasso/saml-2.0/profile.c | 10 +++++++++-
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/lasso/saml-2.0/login.c b/lasso/saml-2.0/login.c
index 6e8f7553..0e60974a 100644
--- a/lasso/saml-2.0/login.c
+++ b/lasso/saml-2.0/login.c
@@ -222,7 +222,7 @@ _lasso_login_must_verify_signature(LassoProfile *profile) {
 gint
 lasso_saml20_login_build_authn_request_msg(LassoLogin *login)
 {
-	char *url = NULL;
+	char *consumer_url = NULL;
 	gboolean must_sign = TRUE;
 	LassoProfile *profile;
 	LassoSamlp2AuthnRequest *authn_request;
@@ -247,29 +247,36 @@ lasso_saml20_login_build_authn_request_msg(LassoLogin *login)
 	}
 
 	if (login->http_method == LASSO_HTTP_METHOD_PAOS) {
-
 		/*
-		 * PAOS is special, the url passed to build_request is the
+		 * PAOS is special, as the destination is determined
+		 * by a client (ECP), not an SP. The saml-bindings-2.0-os
+		 * specification only describes the use of the destination
+		 * attribute for HTTP redirect binding and HTTP POST binding,
+		 * not PAOS. So the destination attribute must be left unset to
+		 * avoid IdP-side errors.
+		 *
+		 * However, the url passed to build_request is the
 		 * AssertionConsumerServiceURL of this SP, not the
-		 * destination.
+-                * destination IdP URL. The build_request function handles
+                 * PAOS as a special case in terms of populating the
+		 * destination attribute.
 		 */
 		if (authn_request->AssertionConsumerServiceURL) {
-			url = authn_request->AssertionConsumerServiceURL;
+			consumer_url = 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), consumer_url, 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(
+			consumer_url = 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, consumer_url);
 		}
 	}
 
-
 	lasso_check_good_rc(lasso_saml20_profile_build_request_msg(profile, "SingleSignOnService",
-				login->http_method, url));
+				login->http_method, consumer_url));
 
 cleanup:
 	return rc;
diff --git a/lasso/saml-2.0/profile.c b/lasso/saml-2.0/profile.c
index 22a4e08c..85f535ae 100644
--- a/lasso/saml-2.0/profile.c
+++ b/lasso/saml-2.0/profile.c
@@ -968,7 +968,15 @@ lasso_saml20_profile_build_request_msg(LassoProfile *profile, const char *servic
 		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 {
-- 
2.20.1

