From 818471ac8489f9b75452c99e1a356412dad2425b Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Mon, 3 Oct 2022 13:41:54 -0700 Subject: [PATCH] GUACAMOLE-1224: Migrate existing auth-related logging to global event listener. --- .../InMemoryAuthenticationFailureTracker.java | 37 +------- .../guacamole/net/auth/Credentials.java | 32 +++++++ .../net/event/AuthenticationSuccessEvent.java | 49 +++++++++- .../guacamole/event/EventLoggingListener.java | 56 ++++++++++- .../apache/guacamole/event/RemoteAddress.java | 95 +++++++++++++++++++ .../rest/auth/AuthenticationService.java | 78 +-------------- 6 files changed, 233 insertions(+), 114 deletions(-) create mode 100644 guacamole/src/main/java/org/apache/guacamole/event/RemoteAddress.java diff --git a/extensions/guacamole-auth-ban/src/main/java/org/apache/guacamole/auth/ban/status/InMemoryAuthenticationFailureTracker.java b/extensions/guacamole-auth-ban/src/main/java/org/apache/guacamole/auth/ban/status/InMemoryAuthenticationFailureTracker.java index b655168e1..58ed4e014 100644 --- a/extensions/guacamole-auth-ban/src/main/java/org/apache/guacamole/auth/ban/status/InMemoryAuthenticationFailureTracker.java +++ b/extensions/guacamole-auth-ban/src/main/java/org/apache/guacamole/auth/ban/status/InMemoryAuthenticationFailureTracker.java @@ -21,7 +21,6 @@ package org.apache.guacamole.auth.ban.status; import com.github.benmanes.caffeine.cache.Cache; import com.github.benmanes.caffeine.cache.Caffeine; -import javax.servlet.http.HttpServletRequest; import org.apache.guacamole.GuacamoleException; import org.apache.guacamole.GuacamoleServerException; import org.apache.guacamole.language.TranslatableGuacamoleClientTooManyException; @@ -88,40 +87,6 @@ public class InMemoryAuthenticationFailureTracker implements AuthenticationFailu } - /** - * Returns whether the given Credentials do not contain any specific - * authentication parameters, including HTTP parameters. An authentication - * request that contains no parameters whatsoever will tend to be the - * first, anonymous, credential-less authentication attempt that results in - * the initial login screen rendering. - * - * @param credentials - * The Credentials object to test. - * - * @return - * true if the given Credentials contain no authentication parameters - * whatsoever, false otherwise. - */ - private boolean isEmpty(Credentials credentials) { - - // An authentication request that contains an explicit username or - // password (even if blank) is non-empty, regardless of how the values - // were passed - if (credentials.getUsername() != null || credentials.getPassword() != null) - return false; - - // All further tests depend on HTTP request details - HttpServletRequest request = credentials.getRequest(); - if (request == null) - return true; - - // An authentication request is non-empty if it contains any HTTP - // parameters at all or contains an authentication token - return !request.getParameterNames().hasMoreElements() - && request.getHeader("Guacamole-Token") == null; - - } - /** * Reports that the given address has just failed to authenticate and * returns the AuthenticationFailureStatus that represents that failure. If @@ -168,7 +133,7 @@ public class InMemoryAuthenticationFailureTracker implements AuthenticationFailu boolean failed) throws GuacamoleException { // Ignore requests that do not contain explicit parameters of any kind - if (isEmpty(credentials)) + if (credentials.isEmpty()) return; // Determine originating address of the authentication request diff --git a/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Credentials.java b/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Credentials.java index 40f991244..6ad0e240b 100644 --- a/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Credentials.java +++ b/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Credentials.java @@ -236,4 +236,36 @@ public class Credentials implements Serializable { this.remoteHostname = remoteHostname; } + /** + * Returns whether this Credentials object does not contain any specific + * authentication parameters, including HTTP parameters and the HTTP header + * used for the authentication token. An authentication request that + * contains no parameters whatsoever will tend to be the first, anonymous, + * credential-less authentication attempt that results in the initial login + * screen rendering. + * + * @return + * true if this Credentials object contains no authentication + * parameters whatsoever, false otherwise. + */ + public boolean isEmpty() { + + // An authentication request that contains an explicit username or + // password (even if blank) is non-empty, regardless of how the values + // were passed + if (getUsername() != null || getPassword() != null) + return false; + + // All further tests depend on HTTP request details + HttpServletRequest httpRequest = getRequest(); + if (httpRequest == null) + return true; + + // An authentication request is non-empty if it contains any HTTP + // parameters at all or contains an authentication token + return !httpRequest.getParameterNames().hasMoreElements() + && httpRequest.getHeader("Guacamole-Token") == null; + + } + } diff --git a/guacamole-ext/src/main/java/org/apache/guacamole/net/event/AuthenticationSuccessEvent.java b/guacamole-ext/src/main/java/org/apache/guacamole/net/event/AuthenticationSuccessEvent.java index a9b21dce8..a7270968e 100644 --- a/guacamole-ext/src/main/java/org/apache/guacamole/net/event/AuthenticationSuccessEvent.java +++ b/guacamole-ext/src/main/java/org/apache/guacamole/net/event/AuthenticationSuccessEvent.java @@ -42,17 +42,47 @@ public class AuthenticationSuccessEvent implements UserEvent, CredentialEvent, */ private final AuthenticatedUser authenticatedUser; + /** + * Whether the successful authentication attempt represented by this event + * is related to an established Guacamole session. + */ + private final boolean existingSession; + /** * Creates a new AuthenticationSuccessEvent which represents a successful * authentication attempt by the user identified by the given - * AuthenticatedUser object. + * AuthenticatedUser object. The authentication attempt is presumed to be + * a fresh authentication attempt unrelated to an established session (a + * login attempt). * * @param authenticatedUser * The AuthenticatedUser identifying the user that successfully * authenticated. */ public AuthenticationSuccessEvent(AuthenticatedUser authenticatedUser) { + this(authenticatedUser, false); + } + + /** + * Creates a new AuthenticationSuccessEvent which represents a successful + * authentication attempt by the user identified by the given + * AuthenticatedUser object. Whether the authentication attempt is + * related to an established session (a periodic re-authentication attempt + * that updates session status) or not (a fresh login attempt) is + * determined by the value of the provided flag. + * + * @param authenticatedUser + * The AuthenticatedUser identifying the user that successfully + * authenticated. + * + * @param existingSession + * Whether this AuthenticationSuccessEvent represents an + * re-authentication attempt that updates the status of an established + * Guacamole session. + */ + public AuthenticationSuccessEvent(AuthenticatedUser authenticatedUser, boolean existingSession) { this.authenticatedUser = authenticatedUser; + this.existingSession = existingSession; } @Override @@ -70,4 +100,21 @@ public class AuthenticationSuccessEvent implements UserEvent, CredentialEvent, return getAuthenticatedUser().getAuthenticationProvider(); } + /** + * Returns whether the successful authentication attempt represented by + * this event is related to an established Guacamole session. During normal + * operation, the Guacamole web application will periodically + * re-authenticate with the server to verify its authentication token and + * update the session state, in which case the value returned by this + * function will be true. If the user was not already authenticated and has + * just initially logged in, false is returned. + * + * @return + * true if this AuthenticationSuccessEvent is related to a Guacamole + * session that was already established, false otherwise. + */ + public boolean isExistingSession() { + return existingSession; + } + } diff --git a/guacamole/src/main/java/org/apache/guacamole/event/EventLoggingListener.java b/guacamole/src/main/java/org/apache/guacamole/event/EventLoggingListener.java index 85d616cbb..2cf3a70d6 100644 --- a/guacamole/src/main/java/org/apache/guacamole/event/EventLoggingListener.java +++ b/guacamole/src/main/java/org/apache/guacamole/event/EventLoggingListener.java @@ -22,8 +22,12 @@ package org.apache.guacamole.event; import javax.annotation.Nonnull; import org.apache.guacamole.GuacamoleException; import org.apache.guacamole.GuacamoleResourceNotFoundException; +import org.apache.guacamole.net.auth.Credentials; +import org.apache.guacamole.net.auth.credentials.GuacamoleInsufficientCredentialsException; import org.apache.guacamole.net.event.ApplicationShutdownEvent; import org.apache.guacamole.net.event.ApplicationStartedEvent; +import org.apache.guacamole.net.event.AuthenticationFailureEvent; +import org.apache.guacamole.net.event.AuthenticationSuccessEvent; import org.apache.guacamole.net.event.DirectoryEvent; import org.apache.guacamole.net.event.DirectoryFailureEvent; import org.apache.guacamole.net.event.DirectorySuccessEvent; @@ -115,6 +119,52 @@ public class EventLoggingListener implements Listener { } } + /** + * Logs that authentication succeeded for a user. + * + * @param event + * The event describing the authentication attempt that succeeded. + */ + private void logSuccess(AuthenticationSuccessEvent event) { + if (!event.isExistingSession()) + logger.info("{} successfully authenticated from {}", + new RequestingUser(event), + new RemoteAddress(event.getCredentials())); + else + logger.debug("{} successfully re-authenticated their existing " + + "session from {}", new RequestingUser(event), + new RemoteAddress(event.getCredentials())); + } + + /** + * Logs that authentication failed for a user. + * + * @param event + * The event describing the authentication attempt that failed. + */ + private void logFailure(AuthenticationFailureEvent event) { + + Credentials creds = event.getCredentials(); + String username = creds.getUsername(); + + if (creds.isEmpty()) + logger.debug("Empty authentication attempt (login screen " + + "initialization) from {} failed: {}", + new RemoteAddress(creds), new Failure(event)); + else if (username == null || username.isEmpty()) + logger.debug("Anonymous authentication attempt from {} failed: {}", + new RemoteAddress(creds), new Failure(event)); + else if (event.getFailure() instanceof GuacamoleInsufficientCredentialsException) + logger.debug("Authentication attempt from {} for user \"{}\" " + + "requires additional credentials to continue: {}", + new RemoteAddress(creds), username, new Failure(event)); + else + logger.warn("Authentication attempt from {} for user \"{}\" " + + "failed: {}", new RemoteAddress(creds), username, + new Failure(event)); + + } + @Override public void handleEvent(@Nonnull Object event) throws GuacamoleException { @@ -124,7 +174,11 @@ public class EventLoggingListener implements Listener { else if (event instanceof DirectoryFailureEvent) logFailure((DirectoryFailureEvent) event); - // Logout / session expiration + // Login / logout / session expiration + else if (event instanceof AuthenticationSuccessEvent) + logSuccess((AuthenticationSuccessEvent) event); + else if (event instanceof AuthenticationFailureEvent) + logFailure((AuthenticationFailureEvent) event); else if (event instanceof UserSessionInvalidatedEvent) logger.info("{} has logged out, or their session has expired or " + "been terminated.", new RequestingUser((UserSessionInvalidatedEvent) event)); diff --git a/guacamole/src/main/java/org/apache/guacamole/event/RemoteAddress.java b/guacamole/src/main/java/org/apache/guacamole/event/RemoteAddress.java new file mode 100644 index 000000000..f55602f59 --- /dev/null +++ b/guacamole/src/main/java/org/apache/guacamole/event/RemoteAddress.java @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.guacamole.event; + +import java.util.regex.Pattern; +import javax.servlet.http.HttpServletRequest; +import org.apache.guacamole.net.auth.Credentials; + +/** + * Loggable representation of the remote address of a user, including any + * intervening proxies noted by "X-Forwarded-For". This representation takes + * into account the fact that "X-Forwarded-For" may come from an untrusted + * source, logging such addresses within square brackets alongside the trusted + * source IP. + */ +public class RemoteAddress implements LoggableDetail { + + /** + * Regular expression which matches any IPv4 address. + */ + private static final String IPV4_ADDRESS_REGEX = "([0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3})"; + + /** + * Regular expression which matches any IPv6 address. + */ + private static final String IPV6_ADDRESS_REGEX = "([0-9a-fA-F]*(:[0-9a-fA-F]*){0,7})"; + + /** + * Regular expression which matches any IP address, regardless of version. + */ + private static final String IP_ADDRESS_REGEX = "(" + IPV4_ADDRESS_REGEX + "|" + IPV6_ADDRESS_REGEX + ")"; + + /** + * Regular expression which matches any Port Number. + */ + private static final String PORT_NUMBER_REGEX = "(:[0-9]{1,5})?"; + + /** + * Pattern which matches valid values of the de-facto standard + * "X-Forwarded-For" header. + */ + private static final Pattern X_FORWARDED_FOR = Pattern.compile("^" + IP_ADDRESS_REGEX + PORT_NUMBER_REGEX + "(, " + IP_ADDRESS_REGEX + PORT_NUMBER_REGEX + ")*$"); + + /** + * The credentials supplied by the user when they authenticated. + */ + private final Credentials creds; + + /** + * Creates a new RemoteAddress representing the source address of the HTTP + * request that provided the given Credentials. + * + * @param creds + * The Credentials associated with the request whose source address + * should be represented by this RemoteAddress. + */ + public RemoteAddress(Credentials creds) { + this.creds = creds; + } + + @Override + public String toString() { + + HttpServletRequest request = creds.getRequest(); + if (request == null) + return creds.getRemoteAddress(); + + // Log X-Forwarded-For, if present and valid + String header = request.getHeader("X-Forwarded-For"); + if (header != null && X_FORWARDED_FOR.matcher(header).matches()) + return "[" + header + ", " + request.getRemoteAddr() + "]"; + + // If header absent or invalid, just use source IP + return request.getRemoteAddr(); + + } + +} diff --git a/guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java b/guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java index 48a2d5a36..305073de3 100644 --- a/guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java +++ b/guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java @@ -21,9 +21,7 @@ package org.apache.guacamole.rest.auth; import java.util.ArrayList; import java.util.List; -import java.util.regex.Pattern; import javax.inject.Inject; -import javax.servlet.http.HttpServletRequest; import org.apache.guacamole.GuacamoleException; import org.apache.guacamole.GuacamoleSecurityException; @@ -98,57 +96,6 @@ public class AuthenticationService { */ public static final String TOKEN_PARAMETER_NAME = "token"; - /** - * Regular expression which matches any IPv4 address. - */ - private static final String IPV4_ADDRESS_REGEX = "([0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3})"; - - /** - * Regular expression which matches any IPv6 address. - */ - private static final String IPV6_ADDRESS_REGEX = "([0-9a-fA-F]*(:[0-9a-fA-F]*){0,7})"; - - /** - * Regular expression which matches any IP address, regardless of version. - */ - private static final String IP_ADDRESS_REGEX = "(" + IPV4_ADDRESS_REGEX + "|" + IPV6_ADDRESS_REGEX + ")"; - - /** - * Regular expression which matches any Port Number. - */ - private static final String PORT_NUMBER_REGEX = "(:[0-9]{1,5})?"; - - /** - * Pattern which matches valid values of the de-facto standard - * "X-Forwarded-For" header. - */ - private static final Pattern X_FORWARDED_FOR = Pattern.compile("^" + IP_ADDRESS_REGEX + PORT_NUMBER_REGEX + "(, " + IP_ADDRESS_REGEX + PORT_NUMBER_REGEX + ")*$"); - - /** - * Returns a formatted string containing an IP address, or list of IP - * addresses, which represent the HTTP client and any involved proxies. As - * the headers used to determine proxies can easily be forged, this data is - * superficially validated to ensure that it at least looks like a list of - * IPs. - * - * @param request - * The HTTP request to format. - * - * @return - * A formatted string containing one or more IP addresses. - */ - private String getLoggableAddress(HttpServletRequest request) { - - // Log X-Forwarded-For, if present and valid - String header = request.getHeader("X-Forwarded-For"); - if (header != null && X_FORWARDED_FOR.matcher(header).matches()) - return "[" + header + ", " + request.getRemoteAddr() + "]"; - - // If header absent or invalid, just use source IP - return request.getRemoteAddr(); - - } - /** * Attempts authentication against all AuthenticationProviders, in order, * using the provided credentials. The first authentication failure takes @@ -437,12 +384,12 @@ public class AuthenticationService { else { authToken = authTokenGenerator.getToken(); tokenSessionMap.put(authToken, new GuacamoleSession(listenerService, authenticatedUser, userContexts)); - logger.debug("Login was successful for user \"{}\".", authenticatedUser.getIdentifier()); } // Report authentication success try { - listenerService.handleEvent(new AuthenticationSuccessEvent(authenticatedUser)); + listenerService.handleEvent(new AuthenticationSuccessEvent(authenticatedUser, + existingSession != null)); } catch (GuacamoleException e) { throw new GuacamoleAuthenticationProcessException("User " @@ -454,25 +401,9 @@ public class AuthenticationService { // Log and rethrow any authentication errors catch (GuacamoleAuthenticationProcessException e) { - // Get request and username for sake of logging - HttpServletRequest request = credentials.getRequest(); - String username = credentials.getUsername(); - listenerService.handleEvent(new AuthenticationFailureEvent(credentials, e.getAuthenticationProvider(), e.getCause())); - // Log authentication failures with associated usernames - if (username != null) { - if (logger.isWarnEnabled()) - logger.warn("Authentication attempt from {} for user \"{}\" failed.", - getLoggableAddress(request), username); - } - - // Log anonymous authentication failures - else if (logger.isDebugEnabled()) - logger.debug("Anonymous authentication attempt from {} failed.", - getLoggableAddress(request)); - // Rethrow exception e.rethrowCause(); @@ -490,11 +421,6 @@ public class AuthenticationService { } - if (logger.isInfoEnabled()) - logger.info("User \"{}\" successfully authenticated from {}.", - authenticatedUser.getIdentifier(), - getLoggableAddress(credentials.getRequest())); - return authToken; }