From d1090b00b0c6626e6ee4577a270edf9efa59ecc8 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Wed, 24 Jun 2020 15:41:21 -0700 Subject: [PATCH 1/2] GUACAMOLE-103: Use UriBuilder for syntax-aware URL construction, rather than simple String concatenation. --- .../auth/saml/AuthenticationProviderService.java | 7 ++++--- .../saml/SAMLAuthenticationProviderResource.java | 13 +++++-------- .../auth/saml/conf/ConfigurationService.java | 3 ++- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java b/extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java index ddc6dbde5..063f0dc8b 100644 --- a/extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java +++ b/extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/AuthenticationProviderService.java @@ -26,7 +26,6 @@ import com.onelogin.saml2.authn.SamlResponse; import com.onelogin.saml2.exception.SettingsException; import com.onelogin.saml2.exception.ValidationError; import com.onelogin.saml2.settings.Saml2Settings; -import com.onelogin.saml2.util.Util; import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; @@ -39,6 +38,7 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Set; import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.core.UriBuilder; import javax.xml.parsers.ParserConfigurationException; import javax.xml.xpath.XPathExpressionException; import org.apache.guacamole.auth.saml.conf.ConfigurationService; @@ -196,8 +196,9 @@ public class AuthenticationProviderService { AuthnRequest samlReq = new AuthnRequest(samlSettings); URI authUri; try { - authUri = new URI(samlSettings.getIdpSingleSignOnServiceUrl() + "?SAMLRequest=" + - Util.urlEncoder(samlReq.getEncodedAuthnRequest())); + authUri = UriBuilder.fromUri(samlSettings.getIdpSingleSignOnServiceUrl().toURI()) + .queryParam("SAMLRequest", samlReq.getEncodedAuthnRequest()) + .build(); } catch (IOException e) { logger.error("Error encoding authentication request to string: {}", e.getMessage()); diff --git a/extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/SAMLAuthenticationProviderResource.java b/extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/SAMLAuthenticationProviderResource.java index 4a1e52199..1b4049456 100644 --- a/extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/SAMLAuthenticationProviderResource.java +++ b/extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/SAMLAuthenticationProviderResource.java @@ -26,7 +26,6 @@ import com.onelogin.saml2.exception.ValidationError; import com.onelogin.saml2.http.HttpRequest; import com.onelogin.saml2.servlet.ServletUtils; import com.onelogin.saml2.settings.Saml2Settings; -import com.onelogin.saml2.util.Util; import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; @@ -39,6 +38,7 @@ import javax.ws.rs.FormParam; import javax.ws.rs.Path; import javax.ws.rs.POST; import javax.ws.rs.core.Context; +import javax.ws.rs.core.UriBuilder; import javax.xml.bind.DatatypeConverter; import javax.xml.parsers.ParserConfigurationException; import javax.xml.xpath.XPathExpressionException; @@ -101,7 +101,7 @@ public class SAMLAuthenticationProviderResource { @Context HttpServletRequest consumedRequest) throws GuacamoleException { - String guacBase = confService.getCallbackUrl().toString(); + URI guacBase = confService.getCallbackUrl(); Saml2Settings samlSettings = confService.getSamlSettings(); try { HttpRequest request = ServletUtils @@ -111,9 +111,9 @@ public class SAMLAuthenticationProviderResource { String responseHash = hashSamlResponse(samlResponseString); samlResponseMap.putSamlResponse(responseHash, samlResponse); - return Response.seeOther(new URI(guacBase - + "?responseHash=" - + Util.urlEncoder(responseHash)) + return Response.seeOther(UriBuilder.fromUri(guacBase) + .queryParam("responseHash", responseHash) + .build() ).build(); } @@ -132,9 +132,6 @@ public class SAMLAuthenticationProviderResource { catch (SettingsException e) { throw new GuacamoleServerException("Settings exception processing SAML response.", e); } - catch (URISyntaxException e) { - throw new GuacamoleServerException("URI exception process SAML response.", e); - } catch (ValidationError e) { throw new GuacamoleServerException("Exception validating SAML response.", e); } diff --git a/extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java b/extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java index ca830cfd1..50627e32b 100644 --- a/extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java +++ b/extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java @@ -27,6 +27,7 @@ import com.onelogin.saml2.util.Constants; import java.net.URI; import java.util.HashMap; import java.util.Map; +import javax.ws.rs.core.UriBuilder; import org.apache.guacamole.GuacamoleException; import org.apache.guacamole.GuacamoleServerException; import org.apache.guacamole.environment.Environment; @@ -335,7 +336,7 @@ public class ConfigurationService { samlMap.put(SettingsBuilder.SP_ENTITYID_PROPERTY_KEY, getEntityId().toString()); samlMap.put(SettingsBuilder.SP_ASSERTION_CONSUMER_SERVICE_URL_PROPERTY_KEY, - getCallbackUrl().toString() + "/api/ext/saml/callback"); + UriBuilder.fromUri(getCallbackUrl()).path("api/ext/saml/callback").build().toString()); SettingsBuilder samlBuilder = new SettingsBuilder(); Saml2Settings samlSettings = samlBuilder.fromValues(samlMap).build(); From feba7c6da06f2da32c71e5ea6d812c55ea74cf0e Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Wed, 24 Jun 2020 16:00:07 -0700 Subject: [PATCH 2/2] GUACAMOLE-103: Read entity ID and callback URL from properties only if needed. --- .../auth/saml/conf/ConfigurationService.java | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java b/extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java index 50627e32b..508060c3a 100644 --- a/extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java +++ b/extensions/guacamole-auth-saml/src/main/java/org/apache/guacamole/auth/saml/conf/ConfigurationService.java @@ -331,13 +331,18 @@ public class ConfigurationService { samlMap.put(SettingsBuilder.IDP_SINGLE_SIGN_ON_SERVICE_BINDING_PROPERTY_KEY, Constants.BINDING_HTTP_REDIRECT); } - - // Common settings, required with or without metadata file. - samlMap.put(SettingsBuilder.SP_ENTITYID_PROPERTY_KEY, - getEntityId().toString()); - samlMap.put(SettingsBuilder.SP_ASSERTION_CONSUMER_SERVICE_URL_PROPERTY_KEY, - UriBuilder.fromUri(getCallbackUrl()).path("api/ext/saml/callback").build().toString()); - + + // Read entity ID from properties if not provided within metadata XML + if (!samlMap.containsKey(SettingsBuilder.SP_ENTITYID_PROPERTY_KEY)) { + samlMap.put(SettingsBuilder.SP_ENTITYID_PROPERTY_KEY, getEntityId().toString()); + } + + // Derive ACS URL from properties if not provided within metadata XML + if (!samlMap.containsKey(SettingsBuilder.SP_ASSERTION_CONSUMER_SERVICE_URL_PROPERTY_KEY)) { + samlMap.put(SettingsBuilder.SP_ASSERTION_CONSUMER_SERVICE_URL_PROPERTY_KEY, + UriBuilder.fromUri(getCallbackUrl()).path("api/ext/saml/callback").build().toString()); + } + SettingsBuilder samlBuilder = new SettingsBuilder(); Saml2Settings samlSettings = samlBuilder.fromValues(samlMap).build(); samlSettings.setStrict(getStrict());