GUACAMOLE-96: Ensure valid codes cannot be reused. Project: http://git-wip-us.apache.org/repos/asf/guacamole-client/repo Commit: http://git-wip-us.apache.org/repos/asf/guacamole-client/commit/b1c23f20 Tree: http://git-wip-us.apache.org/repos/asf/guacamole-client/tree/b1c23f20 Diff: http://git-wip-us.apache.org/repos/asf/guacamole-client/diff/b1c23f20
Branch: refs/heads/master Commit: b1c23f20d00b030cb8a8691f8aad1d53a341f8ff Parents: 456b8a0 Author: Michael Jumper <mjum...@apache.org> Authored: Wed Nov 22 18:53:29 2017 -0800 Committer: Michael Jumper <mjum...@apache.org> Committed: Sun Feb 4 19:45:18 2018 -0800 ---------------------------------------------------------------------- .../auth/totp/TOTPAuthenticationProvider.java | 3 +- .../totp/TOTPAuthenticationProviderModule.java | 2 + .../totp/user/CodeUsageTrackingService.java | 264 +++++++++++++++++++ .../auth/totp/user/UserVerificationService.java | 9 +- 4 files changed, 276 insertions(+), 2 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/guacamole-client/blob/b1c23f20/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/TOTPAuthenticationProvider.java ---------------------------------------------------------------------- diff --git a/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/TOTPAuthenticationProvider.java b/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/TOTPAuthenticationProvider.java index 28e2380..4f18304 100644 --- a/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/TOTPAuthenticationProvider.java +++ b/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/TOTPAuthenticationProvider.java @@ -23,6 +23,7 @@ import org.apache.guacamole.auth.totp.user.UserVerificationService; import com.google.inject.Guice; import com.google.inject.Injector; import org.apache.guacamole.GuacamoleException; +import org.apache.guacamole.auth.totp.user.CodeUsageTrackingService; import org.apache.guacamole.auth.totp.user.TOTPUserContext; import org.apache.guacamole.net.auth.AuthenticatedUser; import org.apache.guacamole.net.auth.AuthenticationProvider; @@ -119,7 +120,7 @@ public class TOTPAuthenticationProvider implements AuthenticationProvider { @Override public void shutdown() { - // Do nothing + injector.getInstance(CodeUsageTrackingService.class).shutdown(); } } http://git-wip-us.apache.org/repos/asf/guacamole-client/blob/b1c23f20/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/TOTPAuthenticationProviderModule.java ---------------------------------------------------------------------- diff --git a/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/TOTPAuthenticationProviderModule.java b/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/TOTPAuthenticationProviderModule.java index 94b7232..d1f7f96 100644 --- a/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/TOTPAuthenticationProviderModule.java +++ b/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/TOTPAuthenticationProviderModule.java @@ -23,6 +23,7 @@ import org.apache.guacamole.auth.totp.user.UserVerificationService; import com.google.inject.AbstractModule; import org.apache.guacamole.GuacamoleException; import org.apache.guacamole.auth.totp.conf.ConfigurationService; +import org.apache.guacamole.auth.totp.user.CodeUsageTrackingService; import org.apache.guacamole.environment.Environment; import org.apache.guacamole.environment.LocalEnvironment; import org.apache.guacamole.net.auth.AuthenticationProvider; @@ -73,6 +74,7 @@ public class TOTPAuthenticationProviderModule extends AbstractModule { bind(Environment.class).toInstance(environment); // Bind TOTP-specific services + bind(CodeUsageTrackingService.class); bind(ConfigurationService.class); bind(UserVerificationService.class); http://git-wip-us.apache.org/repos/asf/guacamole-client/blob/b1c23f20/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/CodeUsageTrackingService.java ---------------------------------------------------------------------- diff --git a/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/CodeUsageTrackingService.java b/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/CodeUsageTrackingService.java new file mode 100644 index 0000000..c9a94b4 --- /dev/null +++ b/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/CodeUsageTrackingService.java @@ -0,0 +1,264 @@ +/* + * 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.auth.totp.user; + +import com.google.inject.Inject; +import com.google.inject.Singleton; +import java.util.Iterator; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import org.apache.guacamole.GuacamoleException; +import org.apache.guacamole.auth.totp.conf.ConfigurationService; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Service for tracking past valid uses of TOTP codes. An internal thread + * periodically walks through records of past codes, removing records which + * should be invalid by their own nature (no longer matching codes generated by + * the secret key). + */ +@Singleton +public class CodeUsageTrackingService { + + /** + * The number of periods during which a previously-used code should remain + * unusable. Once this period has elapsed, the code can be reused again if + * it is otherwise valid. + */ + private static final int INVALID_INTERVAL = 2; + + /** + * Logger for this class. + */ + private final Logger logger = LoggerFactory.getLogger(CodeUsageTrackingService.class); + + /** + * Executor service which runs the cleanup task. + */ + private final ScheduledExecutorService executor = Executors.newScheduledThreadPool(1); + + /** + * Service for retrieving configuration information. + */ + @Inject + private ConfigurationService confService; + + /** + * Map of previously-used codes to the timestamp after which the code can + * be used again, providing the TOTP key legitimately generates that code. + */ + private final ConcurrentMap<UsedCode, Long> invalidCodes = + new ConcurrentHashMap<UsedCode, Long>(); + + /** + * Creates a new CodeUsageTrackingService which tracks past valid uses of + * TOTP codes on a per-user basis. + */ + public CodeUsageTrackingService() { + executor.scheduleAtFixedRate(new CodeEvictionTask(), 1, 1, TimeUnit.MINUTES); + } + + /** + * Task which iterates through all explicitly-invalidated codes, evicting + * those codes which are old enough that they would fail validation against + * the secret key anyway. + */ + private class CodeEvictionTask implements Runnable { + + @Override + public void run() { + + // Get start time of cleanup check + long checkStart = System.currentTimeMillis(); + + // For each code still being tracked, remove those which are old + // enough that they would fail validation against the secret key + Iterator<Map.Entry<UsedCode, Long>> entries = invalidCodes.entrySet().iterator(); + while (entries.hasNext()) { + + Map.Entry<UsedCode, Long> entry = entries.next(); + long invalidUntil = entry.getValue(); + + // If code is sufficiently old, evict it and check the next one + if (checkStart >= invalidUntil) + entries.remove(); + + } + + // Log completion and duration + logger.debug("TOTP tracking cleanup check completed in {} ms.", + System.currentTimeMillis() - checkStart); + + } + + } + + /** + * A valid TOTP code which was previously used by a particular user. + */ + private class UsedCode { + + /** + * The username of the user which previously used this code. + */ + private final String username; + + /** + * The valid code given by the user. + */ + private final String code; + + /** + * Creates a new UsedCode which records the given code as having been + * used by the given user. + * + * @param username + * The username of the user which previously used the given code. + * + * @param code + * The valid code given by the user. + */ + public UsedCode(String username, String code) { + this.username = username; + this.code = code; + } + + /** + * Returns the username of the user which previously used the code + * associated with this UsedCode. + * + * @return + * The username of the user which previously used this code. + */ + public String getUsername() { + return username; + } + + /** + * Returns the valid code given by the user when this UsedCode was + * created. + * + * @return + * The valid code given by the user. + */ + public String getCode() { + return code; + } + + @Override + public int hashCode() { + int hash = 7; + hash = 79 * hash + this.username.hashCode(); + hash = 79 * hash + this.code.hashCode(); + return hash; + } + + @Override + public boolean equals(Object obj) { + + if (this == obj) + return true; + + if (obj == null) + return false; + + if (getClass() != obj.getClass()) + return false; + + final UsedCode other = (UsedCode) obj; + return username.equals(other.username) && code.equals(other.code); + + } + + } + + /** + * Attempts to mark the given code as used. The code MUST have already been + * validated against the user's secret key, as this function only verifies + * whether the code has been previously used, not whether it is actually + * valid. If the code has not previously been used, the code is stored as + * having been used by the given user at the current time. + * + * @param username + * The username of the user who has attempted to use the given valid + * code. + * + * @param code + * The otherwise-valid code given by the user. + * + * @return + * true if the code has not previously been used by the given user and + * has now been marked as previously used, false otherwise. + * + * @throws GuacamoleException + * If configuration information necessary to determine the length of + * time a code should be marked as invalid cannot be read from + * guacamole.properties. + */ + public boolean useCode(String username, String code) + throws GuacamoleException { + + // Repeatedly attempt to use the given code until an explicit success + // or failure has occurred + UsedCode usedCode = new UsedCode(username, code); + for (;;) { + + // Explicitly invalidate each used code for two periods after its + // first successful use + long current = System.currentTimeMillis(); + long invalidUntil = current + confService.getPeriod() * 1000 * INVALID_INTERVAL; + + // Try to use the given code, marking it as used within the map of + // now-invalidated codes + Long expires = invalidCodes.putIfAbsent(usedCode, invalidUntil); + if (expires == null) + return true; + + // If the code was already used, fail to use the code if + // insufficient time has elapsed since it was last used + // successfully + if (expires > current) + return false; + + + // Otherwise, the code is actually valid - remove the invalidated + // code only if it still has the expected expiration time, and + // retry using the code + invalidCodes.remove(usedCode, expires); + + } + + } + + /** + * Cleans up resources which may be in use by this service in the + * background, such as other threads. This function MUST be invoked during + * webapp shutdown to avoid leaking these resources. + */ + public void shutdown() { + executor.shutdownNow(); + } + +} http://git-wip-us.apache.org/repos/asf/guacamole-client/blob/b1c23f20/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/UserVerificationService.java ---------------------------------------------------------------------- diff --git a/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/UserVerificationService.java b/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/UserVerificationService.java index 8264efd..30108e1 100644 --- a/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/UserVerificationService.java +++ b/extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/UserVerificationService.java @@ -65,6 +65,12 @@ public class UserVerificationService { private ConfigurationService confService; /** + * Service for tracking whether TOTP codes have been used. + */ + @Inject + private CodeUsageTrackingService codeService; + + /** * Provider for AuthenticationCodeField instances. */ @Inject @@ -254,7 +260,8 @@ public class UserVerificationService { confService.getMode(), confService.getDigits()); // Verify provided TOTP against value produced by generator - if (code.equals(totp.generate()) || code.equals(totp.previous())) { + if ((code.equals(totp.generate()) || code.equals(totp.previous())) + && codeService.useCode(username, code)) { // Record key as confirmed, if it hasn't already been so recorded if (!key.isConfirmed()) {
