From f879da6ee57486daaa894e4c5843aafc14934cdf Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 14 May 2015 22:20:38 +0000 Subject: [PATCH 1/4] GUAC-587: Move loading of extensions into own function. Do not allow lack of extensions directory to stop loading of app.js and app.css. --- .../net/basic/extension/ExtensionModule.java | 56 +++++++++++++------ 1 file changed, 40 insertions(+), 16 deletions(-) diff --git a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/ExtensionModule.java b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/ExtensionModule.java index e2bae9bdf..99c20e377 100644 --- a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/ExtensionModule.java +++ b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/ExtensionModule.java @@ -216,13 +216,20 @@ public class ExtensionModule extends ServletModule { return ALLOWED_GUACAMOLE_VERSIONS.contains(guacamoleVersion); } - @Override - protected void configureServlets() { - - // Load authentication provider from guacamole.properties for sake of backwards compatibility - Class authProviderProperty = getAuthProviderProperty(); - if (authProviderProperty != null) - bindAuthenticationProvider(authProviderProperty); + /** + * Loads all extensions within the GUACAMOLE_HOME/extensions directory, if + * any, adding their static resource to the given resoure collections. + * + * @param javaScriptResources + * A modifiable collection of static JavaScript resources which may + * receive new JavaScript resources from extensions. + * + * @param cssResources + * A modifiable collection of static CSS resources which may receive + * new CSS resources from extensions. + */ + private void loadExtensions(Collection javaScriptResources, + Collection cssResources) { // Retrieve and validate extensions directory File extensionsDir = new File(environment.getGuacamoleHome(), EXTENSIONS_DIRECTORY); @@ -239,14 +246,10 @@ public class ExtensionModule extends ServletModule { }); - // Init JavaScript resources with base guacamole.min.js - Collection javaScriptResources = new ArrayList(); - javaScriptResources.add(new WebApplicationResource(getServletContext(), "/guacamole.min.js")); - - // Init CSS resources with base guacamole.min.css - Collection cssResources = new ArrayList(); - cssResources.add(new WebApplicationResource(getServletContext(), "/guacamole.min.css")); - + // Verify contents are accessible + if (extensionFiles == null) + logger.warn("Although GUACAMOLE_HOME/" + EXTENSIONS_DIRECTORY + " exists, its contents cannot be read."); + // Load each extension within the extension directory for (File extensionFile : extensionFiles) { @@ -285,7 +288,28 @@ public class ExtensionModule extends ServletModule { } - // Default to basic auth if nothing else chosen/provided + } + + @Override + protected void configureServlets() { + + // Load authentication provider from guacamole.properties for sake of backwards compatibility + Class authProviderProperty = getAuthProviderProperty(); + if (authProviderProperty != null) + bindAuthenticationProvider(authProviderProperty); + + // Init JavaScript resources with base guacamole.min.js + Collection javaScriptResources = new ArrayList(); + javaScriptResources.add(new WebApplicationResource(getServletContext(), "/guacamole.min.js")); + + // Init CSS resources with base guacamole.min.css + Collection cssResources = new ArrayList(); + cssResources.add(new WebApplicationResource(getServletContext(), "/guacamole.min.css")); + + // Load all extensions + loadExtensions(javaScriptResources, cssResources); + + // Bind basic auth if nothing else chosen/provided if (boundAuthenticationProvider == null) { logger.info("Using default, \"basic\", XML-driven authentication."); bindAuthenticationProvider(BasicFileAuthenticationProvider.class); From 60b10296535def7fe3ac77bbb114c0e780643355 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 14 May 2015 22:22:22 +0000 Subject: [PATCH 2/4] GUAC-587: The extension manifest must not be null. --- .../org/glyptodon/guacamole/net/basic/extension/Extension.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/Extension.java b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/Extension.java index 2f3aa17c4..a7891cb4d 100644 --- a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/Extension.java +++ b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/Extension.java @@ -226,6 +226,8 @@ public class Extension { // Parse manifest manifest = mapper.readValue(extension.getInputStream(manifestEntry), ExtensionManifest.class); + if (manifest == null) + throw new GuacamoleServerException("Contents of " + MANIFEST_NAME + " must be a valid JSON object."); } From 62b3028cf0e33b8e113761450c7dfb1da3672fdb Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 14 May 2015 23:10:19 +0000 Subject: [PATCH 3/4] GUAC-587: Ensure an AuthenticationProvider is always bound. --- .../AuthenticationProviderFacade.java | 150 ++++++++++++++++++ .../net/basic/extension/ExtensionModule.java | 3 +- 2 files changed, 151 insertions(+), 2 deletions(-) create mode 100644 guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/AuthenticationProviderFacade.java diff --git a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/AuthenticationProviderFacade.java b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/AuthenticationProviderFacade.java new file mode 100644 index 000000000..4316fe1c0 --- /dev/null +++ b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/AuthenticationProviderFacade.java @@ -0,0 +1,150 @@ +/* + * Copyright (C) 2015 Glyptodon LLC + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package org.glyptodon.guacamole.net.basic.extension; + +import java.lang.reflect.InvocationTargetException; +import org.glyptodon.guacamole.GuacamoleException; +import org.glyptodon.guacamole.net.auth.AuthenticationProvider; +import org.glyptodon.guacamole.net.auth.Credentials; +import org.glyptodon.guacamole.net.auth.UserContext; +import org.glyptodon.guacamole.net.auth.credentials.CredentialsInfo; +import org.glyptodon.guacamole.net.auth.credentials.GuacamoleInsufficientCredentialsException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Provides a safe wrapper around an AuthenticationProvider subclass, such that + * authentication attempts can cleanly fail, and errors can be properly logged, + * even if the AuthenticationProvider cannot be instantiated. + * + * @author Michael Jumper + */ +public class AuthenticationProviderFacade implements AuthenticationProvider { + + /** + * Logger for this class. + */ + private Logger logger = LoggerFactory.getLogger(AuthenticationProviderFacade.class); + + /** + * The underlying authentication provider, or null if the authentication + * provider could not be instantiated. + */ + private final AuthenticationProvider authProvider; + + /** + * Creates a new AuthenticationProviderFacade which delegates all function + * calls to an instance of the given AuthenticationProvider subclass. If + * an instance of the given class cannot be created, creation of this + * facade will still succeed, but its use will result in errors being + * logged, and all authentication attempts will fail. + * + * @param authProviderClass + * The AuthenticationProvider subclass to instantiate. + */ + public AuthenticationProviderFacade(Class authProviderClass) { + + AuthenticationProvider instance = null; + + try { + // Attempt to instantiate the authentication provider + instance = authProviderClass.getConstructor().newInstance(); + } + catch (NoSuchMethodException e) { + logger.error("The authentication extension in use is not properly defined. " + + "Please contact the developers of the extension or, if you " + + "are the developer, turn on debug-level logging."); + logger.debug("AuthenticationProvider is missing a default constructor.", e); + } + catch (SecurityException e) { + logger.error("The Java security mananager is preventing authentication extensions " + + "from being loaded. Please check the configuration of Java or your " + + "servlet container."); + logger.debug("Creation of AuthenticationProvider disallowed by security manager.", e); + } + catch (InstantiationException e) { + logger.error("The authentication extension in use is not properly defined. " + + "Please contact the developers of the extension or, if you " + + "are the developer, turn on debug-level logging."); + logger.debug("AuthenticationProvider cannot be instantiated.", e); + } + catch (IllegalAccessException e) { + logger.error("The authentication extension in use is not properly defined. " + + "Please contact the developers of the extension or, if you " + + "are the developer, turn on debug-level logging."); + logger.debug("Default constructor of AuthenticationProvider is not public.", e); + } + catch (IllegalArgumentException e) { + logger.error("The authentication extension in use is not properly defined. " + + "Please contact the developers of the extension or, if you " + + "are the developer, turn on debug-level logging."); + logger.debug("Default constructor of AuthenticationProvider cannot accept zero arguments.", e); + } + catch (InvocationTargetException e) { + + // Obtain causing error - create relatively-informative stub error if cause is unknown + Throwable cause = e.getCause(); + if (cause == null) + cause = new GuacamoleException("Error encountered during initialization."); + + logger.error("Authentication extension failed to start: {}", cause.getMessage()); + logger.debug("AuthenticationProvider instantiation failed.", e); + + } + + // Associate instance, if any + authProvider = instance; + + } + + @Override + public UserContext getUserContext(Credentials credentials) + throws GuacamoleException { + + // Ignore auth attempts if no auth provider could be loaded + if (authProvider == null) { + logger.warn("Authentication attempt denied because the authentication system could not be loaded. Please for errors earlier in the logs."); + throw new GuacamoleInsufficientCredentialsException("Permission denied.", CredentialsInfo.USERNAME_PASSWORD); + } + + // Delegate to underlying auth provider + return authProvider.getUserContext(credentials); + + } + + @Override + public UserContext updateUserContext(UserContext context, Credentials credentials) + throws GuacamoleException { + + // Ignore auth attempts if no auth provider could be loaded + if (authProvider == null) { + logger.warn("Reauthentication attempt denied because the authentication system could not be loaded. Please for errors earlier in the logs."); + throw new GuacamoleInsufficientCredentialsException("Permission denied.", CredentialsInfo.USERNAME_PASSWORD); + } + + // Delegate to underlying auth provider + return authProvider.updateUserContext(context, credentials); + + } + +} diff --git a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/ExtensionModule.java b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/ExtensionModule.java index 99c20e377..aabc06a62 100644 --- a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/ExtensionModule.java +++ b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/ExtensionModule.java @@ -22,7 +22,6 @@ package org.glyptodon.guacamole.net.basic.extension; -import com.google.inject.Singleton; import com.google.inject.servlet.ServletModule; import java.io.File; import java.io.FileFilter; @@ -197,7 +196,7 @@ public class ExtensionModule extends ServletModule { // Bind authentication provider logger.debug("Binding AuthenticationProvider \"{}\".", authenticationProvider); - bind(AuthenticationProvider.class).to(authenticationProvider).in(Singleton.class); + bind(AuthenticationProvider.class).toInstance(new AuthenticationProviderFacade(authenticationProvider)); } From 51d3fd20302d7f94cfb4552f4c7f41761b6650f5 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 14 May 2015 23:51:02 +0000 Subject: [PATCH 4/4] GUAC-587: Failed providers should throw "invalid" credential exceptions, not "insufficient". --- .../net/basic/extension/AuthenticationProviderFacade.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/AuthenticationProviderFacade.java b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/AuthenticationProviderFacade.java index 4316fe1c0..b5dd36d4a 100644 --- a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/AuthenticationProviderFacade.java +++ b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/extension/AuthenticationProviderFacade.java @@ -28,7 +28,7 @@ import org.glyptodon.guacamole.net.auth.AuthenticationProvider; import org.glyptodon.guacamole.net.auth.Credentials; import org.glyptodon.guacamole.net.auth.UserContext; import org.glyptodon.guacamole.net.auth.credentials.CredentialsInfo; -import org.glyptodon.guacamole.net.auth.credentials.GuacamoleInsufficientCredentialsException; +import org.glyptodon.guacamole.net.auth.credentials.GuacamoleInvalidCredentialsException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -124,7 +124,7 @@ public class AuthenticationProviderFacade implements AuthenticationProvider { // Ignore auth attempts if no auth provider could be loaded if (authProvider == null) { logger.warn("Authentication attempt denied because the authentication system could not be loaded. Please for errors earlier in the logs."); - throw new GuacamoleInsufficientCredentialsException("Permission denied.", CredentialsInfo.USERNAME_PASSWORD); + throw new GuacamoleInvalidCredentialsException("Permission denied.", CredentialsInfo.USERNAME_PASSWORD); } // Delegate to underlying auth provider @@ -139,7 +139,7 @@ public class AuthenticationProviderFacade implements AuthenticationProvider { // Ignore auth attempts if no auth provider could be loaded if (authProvider == null) { logger.warn("Reauthentication attempt denied because the authentication system could not be loaded. Please for errors earlier in the logs."); - throw new GuacamoleInsufficientCredentialsException("Permission denied.", CredentialsInfo.USERNAME_PASSWORD); + throw new GuacamoleInvalidCredentialsException("Permission denied.", CredentialsInfo.USERNAME_PASSWORD); } // Delegate to underlying auth provider