From 9f99176b3c8dd2d7c9a6ebf9c619d9c7fea2b64b Mon Sep 17 00:00:00 2001
From: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date: Thu, 26 Mar 2015 19:34:28 +0100
Subject: [PATCH] SAML-2.0: rework on commit 05fe802b8d, improve handling of
 ProtocolBinding and AssertionConsumerServiceURL

When the same URL was used for many bindings, the current code did not
work. Now we use
lasso_saml20_provider_check_assertion_consumer_service_url() to validate
url and binding are matching, if no binding is suggested we take the
first one defined for this URL.

Using AssertionConsumerServiceIndex and any of the other assertion
consumer designator attributes is still forbidden.
---
 lasso/saml-2.0/login.c            | 39 +++++++++++++++----------------
 tests/data/sp5-saml2/metadata.xml |  6 +++++
 tests/login_tests_saml2.c         | 24 +++++++++++++++++++
 3 files changed, 49 insertions(+), 20 deletions(-)

diff --git a/lasso/saml-2.0/login.c b/lasso/saml-2.0/login.c
index cd754242..62fcc406 100644
--- a/lasso/saml-2.0/login.c
+++ b/lasso/saml-2.0/login.c
@@ -303,9 +303,6 @@ lasso_saml20_login_process_authn_request_msg(LassoLogin *login, const char *auth
 	remote_provider->role = LASSO_PROVIDER_ROLE_SP;
 	server->parent.role = LASSO_PROVIDER_ROLE_IDP;
 
-	/* Normally those three attributes are mutually exclusive, but Google Apps send
-	 * ProtocolBinding and AssertionConsumerServiceURL at the same time, so we support this case
-	 * by validating that it matches the same endpoint */
 	if (((authn_request->ProtocolBinding != NULL) ||
 			(authn_request->AssertionConsumerServiceURL != NULL)) &&
 			(authn_request->AssertionConsumerServiceIndex != -1))
@@ -318,19 +315,26 @@ lasso_saml20_login_process_authn_request_msg(LassoLogin *login, const char *auth
 	protocol_binding = authn_request->ProtocolBinding;
 	if (protocol_binding || authn_request->AssertionConsumerServiceURL)
 	{
-		const gchar *acs_url_binding = NULL;
-
 		if (authn_request->AssertionConsumerServiceURL) {
-			acs_url_binding = lasso_saml20_provider_get_assertion_consumer_service_binding_by_url(
-					remote_provider, authn_request->AssertionConsumerServiceURL);
-			if (! acs_url_binding) {
-				// Sent ACS URL is unknown
-				rc = LASSO_PROFILE_ERROR_INVALID_PROTOCOLPROFILE;
-				goto cleanup;
-			}
-			if (! protocol_binding) {
-				// Only ACS URL sent
-				protocol_binding = acs_url_binding;
+			if (protocol_binding) {
+				if (! lasso_saml20_provider_check_assertion_consumer_service_url(
+							remote_provider, 
+							authn_request->AssertionConsumerServiceURL,
+							authn_request->ProtocolBinding)) {
+					// Sent ACS URL is unknown
+					rc = LASSO_PROFILE_ERROR_INVALID_PROTOCOLPROFILE;
+					goto cleanup;
+				}
+			} else {
+				// Only ACS URL sent, choose the first associated binding
+				protocol_binding = lasso_saml20_provider_get_assertion_consumer_service_binding_by_url(
+						remote_provider, authn_request->AssertionConsumerServiceURL);
+				if (! protocol_binding) {
+					rc = LASSO_PROFILE_ERROR_INVALID_PROTOCOLPROFILE;
+					goto cleanup;
+				}
+				lasso_assign_string(authn_request->ProtocolBinding,
+						protocol_binding);
 			}
 		}
 
@@ -349,11 +353,6 @@ lasso_saml20_login_process_authn_request_msg(LassoLogin *login, const char *auth
 			rc = LASSO_PROFILE_ERROR_INVALID_PROTOCOLPROFILE;
 			goto cleanup;
 		}
-		// We received both a protocolbinding and an acs url, check both matches
-		if (acs_url_binding && g_strcmp0(protocol_binding, acs_url_binding) != 0) {
-			rc = LASSO_PROFILE_ERROR_INVALID_PROTOCOLPROFILE;
-			goto cleanup;
-		}
 	} else {
 		/* protocol binding not set; so it will look into
 		 * AssertionConsumerServiceIndex
diff --git a/tests/data/sp5-saml2/metadata.xml b/tests/data/sp5-saml2/metadata.xml
index da2d693c..6ad4a2eb 100644
--- a/tests/data/sp5-saml2/metadata.xml
+++ b/tests/data/sp5-saml2/metadata.xml
@@ -90,6 +90,12 @@ LlTxKnCrWAXftSm1rNtewTsF
     <AssertionConsumerService index="2"
       Binding="urn:oasis:names:tc:SAML:2.0:bindings:PAOS"
       Location="http://sp5/singleSignOnSOAP" />
+    <AssertionConsumerService index="3"
+      Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST"
+      Location="http://sp5/singleSignOnPostAndArtifact" />
+    <AssertionConsumerService index="4"
+      Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Artifact"
+      Location="http://sp5/singleSignOnPostAndArtifact" />
     <NameIDFormat>urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress</NameIDFormat>
   </SPSSODescriptor>
   <Organization>
diff --git a/tests/login_tests_saml2.c b/tests/login_tests_saml2.c
index 719917a6..a83beb24 100644
--- a/tests/login_tests_saml2.c
+++ b/tests/login_tests_saml2.c
@@ -1099,6 +1099,30 @@ START_TEST(test08_test_authnrequest_flags)
 				.protocol_binding = LASSO_SAML2_METADATA_BINDING_ARTIFACT,
 				.stop_after_build_assertion = 1,
 			});
+	sso_initiated_by_sp2(idp_context, sp_context, 
+			(SsoSettings) { 
+				.assertion_consumer_service_url = "http://sp5/singleSignOnPost",
+				.protocol_binding = LASSO_SAML2_METADATA_BINDING_POST,
+				.stop_after_build_assertion = 1,
+			});
+	sso_initiated_by_sp2(idp_context, sp_context, 
+			(SsoSettings) { 
+				.assertion_consumer_service_url = "http://sp5/singleSignOnArtifact",
+				.protocol_binding = LASSO_SAML2_METADATA_BINDING_ARTIFACT,
+				.stop_after_build_assertion = 1,
+			});
+	sso_initiated_by_sp2(idp_context, sp_context, 
+			(SsoSettings) { 
+				.assertion_consumer_service_url = "http://sp5/singleSignOnPostAndArtifact",
+				.protocol_binding = LASSO_SAML2_METADATA_BINDING_ARTIFACT,
+				.stop_after_build_assertion = 1,
+			});
+	sso_initiated_by_sp2(idp_context, sp_context, 
+			(SsoSettings) { 
+				.assertion_consumer_service_url = "http://sp5/singleSignOnPostAndArtifact",
+				.protocol_binding = LASSO_SAML2_METADATA_BINDING_POST,
+				.stop_after_build_assertion = 1,
+			});
 	unblock_lasso_logs;
 
 	/* Cleanup */