GUACAMOLE-1629: Hook KSM vault code into base vault code and clean up.

extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/secret/VaultSecretService.java

@@ -22,6 +22,8 @@ package org.apache.guacamole.vault.secret;
 import java.util.Map;
 import java.util.concurrent.Future;
 import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.net.auth.Connectable;
+import org.apache.guacamole.net.auth.UserContext;
 import org.apache.guacamole.protocol.GuacamoleConfiguration;
 import org.apache.guacamole.token.TokenFilter;
 
@@ -55,7 +57,9 @@ public interface VaultSecretService {
     /**
      * Returns a Future which eventually completes with the value of the secret
      * having the given name. If no such secret exists, the Future will be
-     * completed with null.
+     * completed with null. The secrets retrieved from this method are independent
+     * of the context of the particular connection being established, or any
+     * associated user context.
      *
      * @param name
      *     The name of the secret to retrieve.
@@ -72,6 +76,35 @@ public interface VaultSecretService {
      */
     Future<String> getValue(String name) throws GuacamoleException;
 
+    /**
+     * Returns a Future which eventually completes with the value of the secret
+     * having the given name. If no such secret exists, the Future will be
+     * completed with null. The connection or connection group, as well as the
+     * user context associated with the request are provided for additional context.
+     *
+     * @param userContext
+     *     The user context associated with the connection or connection group for
+     *     which the secret is being retrieved.
+     *
+     * @param connectable
+     *     The connection or connection group for which the secret is being retrieved.
+     *
+     * @param name
+     *     The name of the secret to retrieve.
+     *
+     * @return
+     *     A Future which completes with value of the secret having the given
+     *     name. If no such secret exists, the Future will be completed with
+     *     null. If an error occurs asynchronously which prevents retrieval of
+     *     the secret, that error will be exposed through an ExecutionException
+     *     when an attempt is made to retrieve the value from the Future.
+     *
+     * @throws GuacamoleException
+     *     If the secret cannot be retrieved due to an error.
+     */
+    Future<String> getValue(UserContext userContext, Connectable connectable,
+            String name) throws GuacamoleException;
+
     /**
      * Returns a map of token names to corresponding Futures which eventually
      * complete with the value of that token, where each token is dynamically
@@ -80,6 +113,12 @@ public interface VaultSecretService {
      * function should be implemented to provide automatic tokens for those
      * secrets and remove the need for manual mapping via YAML.
      *
+     * @param userContext
+     *     The user context from which the connectable originated.
+     *
+     * @param connectable
+     *     The connection or connection group for which the tokens are being replaced.
+     *
      * @param config
      *     The configuration of the Guacamole connection for which tokens are
      *     being generated. This configuration may be empty or partial,
@@ -99,7 +138,7 @@ public interface VaultSecretService {
      *     If an error occurs producing the tokens and values required for the
      *     given configuration.
      */
-    Map<String, Future<String>> getTokens(GuacamoleConfiguration config,
+    Map<String, Future<String>> getTokens(UserContext userContext, Connectable connectable,
-            TokenFilter filter) throws GuacamoleException;
+            GuacamoleConfiguration config, TokenFilter filter) throws GuacamoleException;
 
 }

extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/user/VaultUserContext.java

@@ -31,6 +31,7 @@ import java.util.concurrent.Future;
 import org.apache.guacamole.GuacamoleException;
 import org.apache.guacamole.GuacamoleServerException;
 import org.apache.guacamole.form.Form;
+import org.apache.guacamole.net.auth.Connectable;
 import org.apache.guacamole.net.auth.Connection;
 import org.apache.guacamole.net.auth.ConnectionGroup;
 import org.apache.guacamole.net.auth.TokenInjectingUserContext;
@@ -193,6 +194,10 @@ public class VaultUserContext extends TokenInjectingUserContext {
      * corresponding values from the vault, using the given TokenFilter to
      * filter tokens within the secret names prior to retrieving those secrets.
      *
+     * @param connectable
+     *     The connection or connection group to which the connection is being
+     *     established.
+     *
      * @param tokenMapping
      *     The mapping dictating the name of the secret which maps to each
      *     parameter token, where the key is the name of the parameter token
@@ -222,7 +227,8 @@ public class VaultUserContext extends TokenInjectingUserContext {
      *     If the value for any applicable secret cannot be retrieved from the
      *     vault due to an error.
      */
-    private Map<String, Future<String>> getTokens(Map<String, String> tokenMapping,
+    private Map<String, Future<String>> getTokens(
+            Connectable connectable, Map<String, String> tokenMapping,
             TokenFilter secretNameFilter, GuacamoleConfiguration config,
             TokenFilter configFilter) throws GuacamoleException {
 
@@ -247,14 +253,16 @@ public class VaultUserContext extends TokenInjectingUserContext {
 
             // Initiate asynchronous retrieval of the token value
             String tokenName = entry.getKey();
-            Future<String> secret = secretService.getValue(secretName);
+            Future<String> secret = secretService.getValue(
+                    this, connectable, secretName);
             pendingTokens.put(tokenName, secret);
 
         }
 
         // Additionally include any dynamic, parameter-based tokens
-        pendingTokens.putAll(secretService.getTokens(config, configFilter));
+        pendingTokens.putAll(secretService.getTokens(
-        
+                this, connectable, config, configFilter));
+
         return pendingTokens;
 
     }
@@ -329,7 +337,8 @@ public class VaultUserContext extends TokenInjectingUserContext {
 
         // Substitute tokens producing secret names, retrieving and storing
         // those secrets as parameter tokens
-        tokens.putAll(resolve(getTokens(confService.getTokenMapping(), filter,
+        tokens.putAll(resolve(getTokens(
+                connectionGroup, confService.getTokenMapping(), filter,
                 null, new TokenFilter(tokens))));
 
     }
@@ -409,8 +418,8 @@ public class VaultUserContext extends TokenInjectingUserContext {
 
         // Substitute tokens producing secret names, retrieving and storing
         // those secrets as parameter tokens
-        tokens.putAll(resolve(getTokens(confService.getTokenMapping(), filter,
+        tokens.putAll(resolve(getTokens(connection, confService.getTokenMapping(),
-                config, new TokenFilter(tokens))));
+                filter, config, new TokenFilter(tokens))));
 
     }
 

extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/KsmAuthenticationProviderModule.java

@@ -26,9 +26,8 @@ import org.apache.guacamole.vault.ksm.conf.KsmConfigurationService;
 import org.apache.guacamole.vault.ksm.secret.KsmSecretService;
 import org.apache.guacamole.vault.conf.VaultAttributeService;
 import org.apache.guacamole.vault.conf.VaultConfigurationService;
-import org.apache.guacamole.vault.ksm.secret.KsmCache;
-import org.apache.guacamole.vault.ksm.secret.KsmCacheFactory;
 import org.apache.guacamole.vault.ksm.secret.KsmClient;
+import org.apache.guacamole.vault.ksm.secret.KsmClientFactory;
 import org.apache.guacamole.vault.ksm.secret.KsmRecordService;
 import org.apache.guacamole.vault.secret.VaultSecretService;
 
@@ -55,16 +54,15 @@ public class KsmAuthenticationProviderModule
     protected void configureVault() {
 
         // Bind services specific to Keeper Secrets Manager
-        bind(KsmClient.class);
         bind(KsmRecordService.class);
         bind(VaultAttributeService.class).to(KsmAttributeService.class);
         bind(VaultConfigurationService.class).to(KsmConfigurationService.class);
         bind(VaultSecretService.class).to(KsmSecretService.class);
 
-        // Bind factory for creating KSM Caches
+        // Bind factory for creating KSM Clients
         install(new FactoryModuleBuilder()
-                .implement(KsmCache.class, KsmCache.class)
+                .implement(KsmClient.class, KsmClient.class)
-                .build(KsmCacheFactory.class));
+                .build(KsmClientFactory.class));
     }
 
 }

extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfigProperty.java

@@ -1,44 +0,0 @@
-/*
- * 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.vault.ksm.conf;
-
-import com.keepersecurity.secretsManager.core.KeyValueStorage;
-import org.apache.guacamole.GuacamoleException;
-import org.apache.guacamole.properties.GuacamoleProperty;
-
-/**
- * A GuacamoleProperty whose value is Keeper Secrets Manager {@link KeyValueStorage}
- * object. The value of this property must be base64-encoded JSON, as output by
- * the Keeper Commander CLI tool via the "sm client add" command.
- */
-public abstract class KsmConfigProperty implements GuacamoleProperty<KeyValueStorage> {
-
-    @Override
-    public KeyValueStorage parseValue(String value) throws GuacamoleException {
-
-        // If no property provided, return null.
-        if (value == null)
-            return null;
-
-        // Parse the base-64 encoded JSON into a KeyValueStorage object
-        return KsmConfig.parseKsmConfig(value);
-    }
-
-}

extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfigurationService.java

@@ -22,13 +22,16 @@ package org.apache.guacamole.vault.ksm.conf;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 
-import javax.annotation.Nullable;
+import javax.annotation.Nonnull;
 
 import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleServerException;
 import org.apache.guacamole.environment.Environment;
 import org.apache.guacamole.properties.BooleanGuacamoleProperty;
+import org.apache.guacamole.properties.StringGuacamoleProperty;
 import org.apache.guacamole.vault.conf.VaultConfigurationService;
 
+import com.keepersecurity.secretsManager.core.InMemoryStorage;
 import com.keepersecurity.secretsManager.core.KeyValueStorage;
 import com.keepersecurity.secretsManager.core.SecretsManagerOptions;
 
@@ -62,7 +65,7 @@ public class KsmConfigurationService extends VaultConfigurationService {
      * The base64-encoded configuration information generated by the Keeper
      * Commander CLI tool.
      */
-    private static final KsmConfigProperty KSM_CONFIG = new KsmConfigProperty() {
+    private static final StringGuacamoleProperty KSM_CONFIG = new StringGuacamoleProperty() {
 
         @Override
         public String getName() {
@@ -127,35 +130,68 @@ public class KsmConfigurationService extends VaultConfigurationService {
         return environment.getProperty(STRIP_WINDOWS_DOMAINS, false);
     }
 
+
+    /**
+     * Return the globally-defined base-64-encoded JSON KSM configuration blob
+     * as a string.
+     *
+     * @return
+     *     The globally-defined base-64-encoded JSON KSM configuration blob
+     *     as a string.
+     *
+     * @throws GuacamoleException
+     *     If the value specified within guacamole.properties cannot be
+     *     parsed or does not exist.
+     */
+    public String getKsmConfig() throws GuacamoleException {
+        return environment.getRequiredProperty(KSM_CONFIG);
+    }
+
+    /**
+     * Given a base64-encoded JSON KSM configuration, parse and return a
+     * KeyValueStorage object.
+     *
+     * @param value
+     *     The base64-encoded JSON KSM configuration to parse.
+     *
+     * @return
+     *     The KeyValueStorage that is a result of the parsing operation
+     *
+     * @throws GuacamoleException
+     *     If the provided value is not valid base-64 encoded JSON KSM configuration.
+     */
+    private static KeyValueStorage parseKsmConfig(String value) throws GuacamoleException {
+
+        // Parse base64 value as KSM config storage
+        try {
+            return new InMemoryStorage(value);
+        }
+        catch (IllegalArgumentException e) {
+            throw new GuacamoleServerException("Invalid base64 configuration "
+                    + "for Keeper Secrets Manager.", e);
+        }
+
+    }
+
     /**
      * Returns the options required to authenticate with Keeper Secrets Manager
      * when retrieving secrets. These options are read from the contents of
      * base64-encoded JSON configuration data generated by the Keeper Commander
-     * CLI tool. This configuration data may be passed directly as an argument.
+     * CLI tool. This configuration data must be passed directly as an argument.
-     * If not provided as an argument, it must be present in the config file.
      *
      * @param ksmConfig
-     *     Optional KSM config data. If provided, it will be used instead of any
+     *     The KSM configuration blob to parse.
-     *     KSM config data present in the config file. If not provided, the value
-     *     in the config file will be used.
      *
      * @return
      *     The options that should be used when connecting to Keeper Secrets
      *     Manager when retrieving secrets.
      *
      * @throws GuacamoleException
-     *     If an invalid ksmConfig parameter is provided, or required properties
+     *     If an invalid ksmConfig parameter is provided.
-     *     are not specified within guacamole.properties or cannot be parsed, or
-     *     the KSM configuration cannot be parsed.
      */
-    public SecretsManagerOptions getSecretsManagerOptions(@Nullable String ksmConfig) throws GuacamoleException {
+    public SecretsManagerOptions getSecretsManagerOptions(@Nonnull String ksmConfig) throws GuacamoleException {
 
-        // Attempt to parse the KSM config provided as an argument if available.
+        return new SecretsManagerOptions(
-        // If not provided as an argument, it must be present in the config file.
+                parseKsmConfig(ksmConfig), null, getAllowUnverifiedCertificate());
-        KeyValueStorage parsedKsmConfig = ksmConfig != null
-                ? KsmConfig.parseKsmConfig(ksmConfig)
-                : environment.getRequiredProperty(KSM_CONFIG);
-
-        return new SecretsManagerOptions(parsedKsmConfig, null, getAllowUnverifiedCertificate());
     }
 }

extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmCache.java

@@ -1,484 +0,0 @@
-/*
- * 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.vault.ksm.secret;
-
-import com.google.inject.Inject;
-import com.google.inject.assistedinject.Assisted;
-import com.google.inject.assistedinject.AssistedInject;
-import com.keepersecurity.secretsManager.core.Hosts;
-import com.keepersecurity.secretsManager.core.KeeperRecord;
-import com.keepersecurity.secretsManager.core.KeeperSecrets;
-import com.keepersecurity.secretsManager.core.Login;
-import com.keepersecurity.secretsManager.core.Notation;
-import com.keepersecurity.secretsManager.core.SecretsManager;
-import com.keepersecurity.secretsManager.core.SecretsManagerOptions;
-
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-import java.util.concurrent.CompletableFuture;
-import java.util.concurrent.Future;
-import java.util.concurrent.locks.ReadWriteLock;
-import java.util.concurrent.locks.ReentrantReadWriteLock;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
-
-import org.apache.guacamole.GuacamoleException;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
-/**
- * A cache of Keeper Secrets Manager records, specific to a single KSM account. This class
- * includes internal functionality necessary to fetch records from KSM as needed.
- */
-public class KsmCache {
-    /**
-     * Logger for this class.
-     */
-    private static final Logger logger = LoggerFactory.getLogger(KsmCache.class);
-
-    /**
-     * Service for retrieving data from records.
-     */
-    @Inject
-    private KsmRecordService recordService;
-
-    /**
-     * The publicly-accessible URL for Keeper's documentation covering Keeper
-     * notation.
-     */
-    private static final String KEEPER_NOTATION_DOC_URL =
-            "https://docs.keeper.io/secrets-manager/secrets-manager/about/keeper-notation";
-
-    /**
-     * The regular expression that Keeper notation must match to be related to
-     * file retrieval. As the Keeper SDK provides mutually-exclusive for
-     * retrieving secret values and files via notation, the notation must first
-     * be tested to determine whether it refers to a file.
-     */
-    private static final Pattern KEEPER_FILE_NOTATION = Pattern.compile("^(keeper://)?[^/]*/file/.+");
-
-    /**
-     * The maximum amount of time that an entry will be stored in the cache
-     * before being refreshed, in milliseconds.
-     */
-    private static final long CACHE_INTERVAL = 5000;
-
-    /**
-     * The KSM configuration associated with this cache instance.
-     */
-    private final SecretsManagerOptions ksmConfig;
-
-    /**
-     * Create a new KSM cache instance based around the provided KSM configuration.
-     *
-     * @param ksmConfig
-     *     The KSM configuration to use when retrieving properties from KSM.
-     */
-    @AssistedInject
-    public KsmCache(@Assisted SecretsManagerOptions ksmConfig) {
-        this.ksmConfig = ksmConfig;
-    }
-
-    /**
-     * Read/write lock which guards access to all cached data, including the
-     * timestamp recording the last time the cache was refreshed. Readers of
-     * the cache must first acquire (and eventually release) the read lock, and
-     * writers of the cache must first acquire (and eventually release) the
-     * write lock.
-     */
-    private final ReadWriteLock cacheLock = new ReentrantReadWriteLock();
-
-    /**
-     * The timestamp that the cache was last refreshed, in milliseconds, as
-     * returned by System.currentTimeMillis(). This value is automatically
-     * updated if {@link #validateCache()} refreshes the cache. This value must
-     * not be accessed without {@link #cacheLock} acquired appropriately.
-     */
-    private volatile long cacheTimestamp = 0;
-
-    /**
-     * The full cached set of secrets last retrieved from Keeper Secrets
-     * Manager. This value is automatically updated if {@link #validateCache()}
-     * refreshes the cache. This value must not be accessed without
-     * {@link #cacheLock} acquired appropriately.
-     */
-    private KeeperSecrets cachedSecrets = null;
-
-    /**
-     * All records retrieved from Keeper Secrets Manager, where each key is the
-     * UID of the corresponding record. The contents of this Map are
-     * automatically updated if {@link #validateCache()} refreshes the cache.
-     * This Map must not be accessed without {@link #cacheLock} acquired
-     * appropriately.
-     */
-    private final Map<String, KeeperRecord> cachedRecordsByUid = new HashMap<>();
-
-    /**
-     * All records retrieved from Keeper Secrets Manager, where each key is the
-     * hostname or IP address of the corresponding record. The hostname or IP
-     * address of a record is determined by {@link Hosts} fields, thus a record
-     * may be associated with multiple hosts. If a record is associated with
-     * multiple hosts, there will be multiple references to that record within
-     * this Map. The contents of this Map are automatically updated if
-     * {@link #validateCache()} refreshes the cache. This Map must not be
-     * accessed without {@link #cacheLock} acquired appropriately. Before using
-     * a value from this Map, {@link #cachedAmbiguousHosts} must first be
-     * checked to verify that there is indeed only one record associated with
-     * that host.
-     */
-    private final Map<String, KeeperRecord> cachedRecordsByHost = new HashMap<>();
-
-    /**
-     * The set of all hostnames or IP addresses that are associated with
-     * multiple records, and thus cannot uniquely identify a record. The
-     * contents of this Set are automatically updated if
-     * {@link #validateCache()} refreshes the cache. This Set must not be
-     * accessed without {@link #cacheLock} acquired appropriately.This Set
-     * must be checked before using a value retrieved from
-     * {@link #cachedRecordsByHost}.
-     */
-    private final Set<String> cachedAmbiguousHosts = new HashSet<>();
-
-    /**
-     * All records retrieved from Keeper Secrets Manager, where each key is the
-     * username of the corresponding record. The username of a record is
-     * determined by {@link Login} fields, thus a record may be associated with
-     * multiple users. If a record is associated with multiple users, there
-     * will be multiple references to that record within this Map. The contents
-     * of this Map are automatically updated if {@link #validateCache()}
-     * refreshes the cache. This Map must not be accessed without
-     * {@link #cacheLock} acquired appropriately. Before using a value from
-     * this Map, {@link #cachedAmbiguousUsernames} must first be checked to
-     * verify that there is indeed only one record associated with that user.
-     */
-    private final Map<String, KeeperRecord> cachedRecordsByUsername = new HashMap<>();
-
-    /**
-     * The set of all usernames that are associated with multiple records, and
-     * thus cannot uniquely identify a record. The contents of this Set are
-     * automatically updated if {@link #validateCache()} refreshes the cache.
-     * This Set must not be accessed without {@link #cacheLock} acquired
-     * appropriately.This Set must be checked before using a value retrieved
-     * from {@link #cachedRecordsByUsername}.
-     */
-    private final Set<String> cachedAmbiguousUsernames = new HashSet<>();
-
-    /**
-     * Validates that all cached data is current with respect to
-     * {@link #CACHE_INTERVAL}, refreshing data from the server as needed.
-     *
-     * @throws GuacamoleException
-     *     If an error occurs preventing the cached data from being refreshed.
-     */
-    private void validateCache() throws GuacamoleException {
-
-        long currentTime = System.currentTimeMillis();
-
-        // Perform a read-only check that the cache has actually expired before
-        // continuing
-        cacheLock.readLock().lock();
-        try {
-            if (currentTime - cacheTimestamp < CACHE_INTERVAL)
-                return;
-        }
-        finally {
-            cacheLock.readLock().unlock();
-        }
-
-        cacheLock.writeLock().lock();
-        try {
-
-            // Cache may have been updated since the read-only check. Re-verify
-            // that the cache has expired before continuing with a full refresh
-            if (currentTime - cacheTimestamp < CACHE_INTERVAL)
-                return;
-
-            // Attempt to pull all records first, allowing that operation to
-            // succeed/fail BEFORE we clear out the last cached success
-            KeeperSecrets secrets = SecretsManager.getSecrets(ksmConfig);
-            List<KeeperRecord> records = secrets.getRecords();
-
-            // Store all secrets within cache
-            cachedSecrets = secrets;
-
-            // Clear unambiguous cache of all records by UID
-            cachedRecordsByUid.clear();
-
-            // Clear cache of host-based records
-            cachedAmbiguousHosts.clear();
-            cachedRecordsByHost.clear();
-
-            // Clear cache of login-based records
-            cachedAmbiguousUsernames.clear();
-            cachedRecordsByUsername.clear();
-
-            // Store all records, sorting each into host-based and login-based
-            // buckets
-            records.forEach(record -> {
-
-                // Store based on UID ...
-                cachedRecordsByUid.put(record.getRecordUid(), record);
-
-                // ... and hostname/address
-                String hostname = recordService.getHostname(record);
-                addRecordForHost(record, hostname);
-
-                // Store based on username ONLY if no hostname (will otherwise
-                // result in ambiguous entries for servers tied to identical
-                // accounts)
-                if (hostname == null)
-                    addRecordForLogin(record, recordService.getUsername(record));
-
-            });
-
-            // Cache has been refreshed
-            this.cacheTimestamp = System.currentTimeMillis();
-
-        }
-        finally {
-            cacheLock.writeLock().unlock();
-        }
-
-    }
-
-    /**
-     * Associates the given record with the given hostname. The hostname may be
-     * null. Both {@link #cachedRecordsByHost} and {@link #cachedAmbiguousHosts}
-     * are updated appropriately. The write lock of {@link #cacheLock} must
-     * already be acquired before invoking this function.
-     *
-     * @param record
-     *     The record to associate with the hosts in the given field.
-     *
-     * @param hostname
-     *     The hostname/address that the given record should be associated
-     *     with. This may be null.
-     */
-    private void addRecordForHost(KeeperRecord record, String hostname) {
-
-        if (hostname == null)
-            return;
-
-        KeeperRecord existing = cachedRecordsByHost.putIfAbsent(hostname, record);
-        if (existing != null && record != existing)
-            cachedAmbiguousHosts.add(hostname);
-
-    }
-
-    /**
-     * Associates the given record with the given username. The given username
-     * may be null. Both {@link #cachedRecordsByUsername} and
-     * {@link #cachedAmbiguousUsernames} are updated appropriately. The write
-     * lock of {@link #cacheLock} must already be acquired before invoking this
-     * function.
-     *
-     * @param record
-     *     The record to associate with the given username.
-     *
-     * @param username
-     *     The username that the given record should be associated with. This
-     *     may be null.
-     */
-    private void addRecordForLogin(KeeperRecord record, String username) {
-
-        if (username == null)
-            return;
-
-        KeeperRecord existing = cachedRecordsByUsername.putIfAbsent(username, record);
-        if (existing != null && record != existing)
-            cachedAmbiguousUsernames.add(username);
-
-    }
-
-    /**
-     * Returns all records accessible via Keeper Secrets Manager. The records
-     * returned are arbitrarily ordered.
-     *
-     * @return
-     *     An unmodifiable Collection of all records accessible via Keeper
-     *     Secrets Manager,  in no particular order.
-     *
-     * @throws GuacamoleException
-     *     If an error occurs that prevents records from being retrieved.
-     */
-    public Collection<KeeperRecord> getRecords() throws GuacamoleException {
-        validateCache();
-        cacheLock.readLock().lock();
-        try {
-            return Collections.unmodifiableCollection(cachedRecordsByUid.values());
-        }
-        finally {
-            cacheLock.readLock().unlock();
-        }
-    }
-
-    /**
-     * Returns the record having the given UID. If no such record exists, null
-     * is returned.
-     *
-     * @param uid
-     *     The UID of the record to return.
-     *
-     * @return
-     *     The record having the given UID, or null if there is no such record.
-     *
-     * @throws GuacamoleException
-     *     If an error occurs that prevents the record from being retrieved.
-     */
-    public KeeperRecord getRecord(String uid) throws GuacamoleException {
-        validateCache();
-        cacheLock.readLock().lock();
-        try {
-            return cachedRecordsByUid.get(uid);
-        }
-        finally {
-            cacheLock.readLock().unlock();
-        }
-    }
-
-    /**
-     * Returns the record associated with the given hostname or IP address. If
-     * no such record exists, or there are multiple such records, null is
-     * returned.
-     *
-     * @param hostname
-     *     The hostname of the record to return.
-     *
-     * @return
-     *     The record associated with the given hostname, or null if there is
-     *     no such record or multiple such records.
-     *
-     * @throws GuacamoleException
-     *     If an error occurs that prevents the record from being retrieved.
-     */
-    public KeeperRecord getRecordByHost(String hostname) throws GuacamoleException {
-        validateCache();
-        cacheLock.readLock().lock();
-        try {
-
-            if (cachedAmbiguousHosts.contains(hostname)) {
-                logger.debug("The hostname/address \"{}\" is referenced by "
-                        + "multiple Keeper records and cannot be used to "
-                        + "locate individual secrets.", hostname);
-                return null;
-            }
-
-            return cachedRecordsByHost.get(hostname);
-
-        }
-        finally {
-            cacheLock.readLock().unlock();
-        }
-    }
-
-    /**
-     * Returns the record associated with the given username. If no such record
-     * exists, or there are multiple such records, null is returned.
-     *
-     * @param username
-     *     The username of the record to return.
-     *
-     * @return
-     *     The record associated with the given username, or null if there is
-     *     no such record or multiple such records.
-     *
-     * @throws GuacamoleException
-     *     If an error occurs that prevents the record from being retrieved.
-     */
-    public KeeperRecord getRecordByLogin(String username) throws GuacamoleException {
-        validateCache();
-        cacheLock.readLock().lock();
-        try {
-
-            if (cachedAmbiguousUsernames.contains(username)) {
-                logger.debug("The username \"{}\" is referenced by multiple "
-                        + "Keeper records and cannot be used to locate "
-                        + "individual secrets.", username);
-                return null;
-            }
-
-            return cachedRecordsByUsername.get(username);
-
-        }
-        finally {
-            cacheLock.readLock().unlock();
-        }
-    }
-
-    /**
-     * Returns the value of the secret stored within Keeper Secrets Manager and
-     * represented by the given Keeper notation. Keeper notation locates the
-     * value of a specific field, custom field, or file associated with a
-     * specific record. See: https://docs.keeper.io/secrets-manager/secrets-manager/about/keeper-notation
-     *
-     * @param notation
-     *     The Keeper notation of the secret to retrieve.
-     *
-     * @return
-     *     A Future which completes with the value of the secret represented by
-     *     the given Keeper notation, or null if there is no such secret.
-     *
-     * @throws GuacamoleException
-     *     If the requested secret cannot be retrieved or the Keeper notation
-     *     is invalid.
-     */
-    public Future<String> getSecret(String notation) throws GuacamoleException {
-        validateCache();
-        cacheLock.readLock().lock();
-        try {
-
-            // Retrieve any relevant file asynchronously
-            Matcher fileNotationMatcher = KEEPER_FILE_NOTATION.matcher(notation);
-            if (fileNotationMatcher.matches())
-                return recordService.download(Notation.getFile(cachedSecrets, notation));
-
-            // Retrieve string values synchronously
-            return CompletableFuture.completedFuture(Notation.getValue(cachedSecrets, notation));
-
-        }
-
-        // Unfortunately, the notation parser within the Keeper SDK throws
-        // plain Errors for retrieval failures ...
-        catch (Error e) {
-            logger.warn("Record \"{}\" does not exist.", notation);
-            logger.debug("Retrieval of record by Keeper notation failed.", e);
-            return CompletableFuture.completedFuture(null);
-        }
-
-        // ... and plain Exceptions for parse failures (no subclasses)
-        catch (Exception e) {
-            logger.warn("\"{}\" is not valid Keeper notation. Please check "
-                    + "the documentation at {} for valid formatting.",
-                    notation, KEEPER_NOTATION_DOC_URL);
-            logger.debug("Provided Keeper notation could not be parsed.", e);
-            return CompletableFuture.completedFuture(null);
-        }
-        finally {
-            cacheLock.readLock().unlock();
-        }
-
-    }
-
-}

extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java

@@ -20,19 +20,33 @@
 package org.apache.guacamole.vault.ksm.secret;
 
 import com.google.inject.Inject;
-import com.google.inject.Singleton;
+import com.google.inject.assistedinject.Assisted;
+import com.google.inject.assistedinject.AssistedInject;
+import com.keepersecurity.secretsManager.core.Hosts;
 import com.keepersecurity.secretsManager.core.KeeperRecord;
+import com.keepersecurity.secretsManager.core.KeeperSecrets;
+import com.keepersecurity.secretsManager.core.Login;
+import com.keepersecurity.secretsManager.core.Notation;
+import com.keepersecurity.secretsManager.core.SecretsManager;
 import com.keepersecurity.secretsManager.core.SecretsManagerOptions;
 
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.Future;
-
+import java.util.concurrent.locks.ReadWriteLock;
-import javax.annotation.Nullable;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 import org.apache.guacamole.GuacamoleException;
-import org.apache.guacamole.vault.ksm.conf.KsmConfigurationService;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Client which retrieves records from Keeper Secrets Manager, allowing
@@ -42,88 +56,292 @@ import org.apache.guacamole.vault.ksm.conf.KsmConfigurationService;
  * information), it's not possible for the server to perform a search of
  * content on the client's behalf. The client has to perform its own search.
  */
-@Singleton
 public class KsmClient {
 
     /**
-     * Service for retrieving configuration information.
+     * Logger for this class.
+     */
+    private static final Logger logger = LoggerFactory.getLogger(KsmClient.class);
+
+    /**
+     * Service for retrieving data from records.
      */
     @Inject
-    private KsmConfigurationService confService;
+    private KsmRecordService recordService;
 
     /**
-     * Factory for creating KSM cache instances for particular KSM configs.
+     * The publicly-accessible URL for Keeper's documentation covering Keeper
+     * notation.
      */
-    @Inject
+    private static final String KEEPER_NOTATION_DOC_URL =
-    private KsmCacheFactory ksmCacheFactory;
+            "https://docs.keeper.io/secrets-manager/secrets-manager/about/keeper-notation";
 
     /**
-     * A map of base-64 encoded JSON KSM config blobs to associated KSM cache instances.
+     * The regular expression that Keeper notation must match to be related to
-     * The `null` entry in this Map is associated with the KSM configuration parsed
+     * file retrieval. As the Keeper SDK provides mutually-exclusive for
-     * from the guacamole.properties config file.
+     * retrieving secret values and files via notation, the notation must first
+     * be tested to determine whether it refers to a file.
      */
-    private final Map<String, KsmCache> ksmCacheMap = new HashMap<>();
+    private static final Pattern KEEPER_FILE_NOTATION = Pattern.compile("^(keeper://)?[^/]*/file/.+");
 
     /**
-     * Create and return a KSM cache for the provided KSM config if not already
+     * The maximum amount of time that an entry will be stored in the cache
-     * present in the cache map, the existing cache entry.
+     * before being refreshed, in milliseconds.
+     */
+    private static final long CACHE_INTERVAL = 5000;
+
+    /**
+     * The KSM configuration associated with this client instance.
+     */
+    private final SecretsManagerOptions ksmConfig;
+
+    /**
+     * Read/write lock which guards access to all cached data, including the
+     * timestamp recording the last time the cache was refreshed. Readers of
+     * the cache must first acquire (and eventually release) the read lock, and
+     * writers of the cache must first acquire (and eventually release) the
+     * write lock.
+     */
+    private final ReadWriteLock cacheLock = new ReentrantReadWriteLock();
+
+    /**
+     * The timestamp that the cache was last refreshed, in milliseconds, as
+     * returned by System.currentTimeMillis(). This value is automatically
+     * updated if {@link #validateCache()} refreshes the cache. This value must
+     * not be accessed without {@link #cacheLock} acquired appropriately.
+     */
+    private volatile long cacheTimestamp = 0;
+
+    /**
+     * The full cached set of secrets last retrieved from Keeper Secrets
+     * Manager. This value is automatically updated if {@link #validateCache()}
+     * refreshes the cache. This value must not be accessed without
+     * {@link #cacheLock} acquired appropriately.
+     */
+    private KeeperSecrets cachedSecrets = null;
+
+    /**
+     * All records retrieved from Keeper Secrets Manager, where each key is the
+     * UID of the corresponding record. The contents of this Map are
+     * automatically updated if {@link #validateCache()} refreshes the cache.
+     * This Map must not be accessed without {@link #cacheLock} acquired
+     * appropriately.
+     */
+    private final Map<String, KeeperRecord> cachedRecordsByUid = new HashMap<>();
+
+    /**
+     * All records retrieved from Keeper Secrets Manager, where each key is the
+     * hostname or IP address of the corresponding record. The hostname or IP
+     * address of a record is determined by {@link Hosts} fields, thus a record
+     * may be associated with multiple hosts. If a record is associated with
+     * multiple hosts, there will be multiple references to that record within
+     * this Map. The contents of this Map are automatically updated if
+     * {@link #validateCache()} refreshes the cache. This Map must not be
+     * accessed without {@link #cacheLock} acquired appropriately. Before using
+     * a value from this Map, {@link #cachedAmbiguousHosts} must first be
+     * checked to verify that there is indeed only one record associated with
+     * that host.
+     */
+    private final Map<String, KeeperRecord> cachedRecordsByHost = new HashMap<>();
+
+    /**
+     * The set of all hostnames or IP addresses that are associated with
+     * multiple records, and thus cannot uniquely identify a record. The
+     * contents of this Set are automatically updated if
+     * {@link #validateCache()} refreshes the cache. This Set must not be
+     * accessed without {@link #cacheLock} acquired appropriately.This Set
+     * must be checked before using a value retrieved from
+     * {@link #cachedRecordsByHost}.
+     */
+    private final Set<String> cachedAmbiguousHosts = new HashSet<>();
+
+    /**
+     * All records retrieved from Keeper Secrets Manager, where each key is the
+     * username of the corresponding record. The username of a record is
+     * determined by {@link Login} fields, thus a record may be associated with
+     * multiple users. If a record is associated with multiple users, there
+     * will be multiple references to that record within this Map. The contents
+     * of this Map are automatically updated if {@link #validateCache()}
+     * refreshes the cache. This Map must not be accessed without
+     * {@link #cacheLock} acquired appropriately. Before using a value from
+     * this Map, {@link #cachedAmbiguousUsernames} must first be checked to
+     * verify that there is indeed only one record associated with that user.
+     */
+    private final Map<String, KeeperRecord> cachedRecordsByUsername = new HashMap<>();
+
+    /**
+     * The set of all usernames that are associated with multiple records, and
+     * thus cannot uniquely identify a record. The contents of this Set are
+     * automatically updated if {@link #validateCache()} refreshes the cache.
+     * This Set must not be accessed without {@link #cacheLock} acquired
+     * appropriately.This Set must be checked before using a value retrieved
+     * from {@link #cachedRecordsByUsername}.
+     */
+    private final Set<String> cachedAmbiguousUsernames = new HashSet<>();
+
+    /**
+     * Create a new KSM client based around the provided KSM configuration.
      *
      * @param ksmConfig
-     *     The base-64 encoded JSON KSM config blob associated with the cache entry.
+     *     The KSM configuration to use when retrieving properties from KSM.
-     *     If an associated entry does not already exist, it will be created using
-     *     this configuration.
-     *
-     * @return
-     *     A KSM cache for the provided KSM config if not already present in the
-     *     cache map, otherwise the existing cache entry.
-     *
-     * @throws GuacamoleException
-     *     If an error occurs while creating the KSM cache.
      */
-    private KsmCache createCacheIfNeeded(@Nullable String ksmConfig)
+    @AssistedInject
-            throws GuacamoleException {
+    public KsmClient(@Assisted SecretsManagerOptions ksmConfig) {
-
+        this.ksmConfig = ksmConfig;
-        // If a cache already exists for the provided config, use it
-        KsmCache ksmCache = ksmCacheMap.get(ksmConfig);
-        if (ksmCache != null)
-            return ksmCache;
-
-        // Create and store a new KSM cache instance for the provided KSM config blob
-        SecretsManagerOptions options = confService.getSecretsManagerOptions(ksmConfig);
-        return ksmCacheMap.put(ksmConfig, ksmCacheFactory.create(options));
     }
 
     /**
-     * Returns all records accessible via Keeper Secrets Manager, associated
+     * Validates that all cached data is current with respect to
-     * with the provided KSM config. If no KSM config is provided, records
+     * {@link #CACHE_INTERVAL}, refreshing data from the server as needed.
-     * associated with the default configuration as retrieved from the config
-     * file will be used. The records returned are arbitrarily ordered.
      *
-     * @param ksmConfig
+     * @throws GuacamoleException
-     *     The base-64 encoded JSON KSM config blob associated associated with
+     *     If an error occurs preventing the cached data from being refreshed.
-     *     the KSM vault that should be used to fetch the records.
+     */
+    private void validateCache() throws GuacamoleException {
+
+        long currentTime = System.currentTimeMillis();
+
+        // Perform a read-only check that the cache has actually expired before
+        // continuing
+        cacheLock.readLock().lock();
+        try {
+            if (currentTime - cacheTimestamp < CACHE_INTERVAL)
+                return;
+        }
+        finally {
+            cacheLock.readLock().unlock();
+        }
+
+        cacheLock.writeLock().lock();
+        try {
+
+            // Client may have been updated since the read-only check. Re-verify
+            // that the cache has expired before continuing with a full refresh
+            if (currentTime - cacheTimestamp < CACHE_INTERVAL)
+                return;
+
+            // Attempt to pull all records first, allowing that operation to
+            // succeed/fail BEFORE we clear out the last cached success
+            KeeperSecrets secrets = SecretsManager.getSecrets(ksmConfig);
+            List<KeeperRecord> records = secrets.getRecords();
+
+            // Store all secrets within cache
+            cachedSecrets = secrets;
+
+            // Clear unambiguous cache of all records by UID
+            cachedRecordsByUid.clear();
+
+            // Clear cache of host-based records
+            cachedAmbiguousHosts.clear();
+            cachedRecordsByHost.clear();
+
+            // Clear cache of login-based records
+            cachedAmbiguousUsernames.clear();
+            cachedRecordsByUsername.clear();
+
+            // Store all records, sorting each into host-based and login-based
+            // buckets
+            records.forEach(record -> {
+
+                // Store based on UID ...
+                cachedRecordsByUid.put(record.getRecordUid(), record);
+
+                // ... and hostname/address
+                String hostname = recordService.getHostname(record);
+                addRecordForHost(record, hostname);
+
+                // Store based on username ONLY if no hostname (will otherwise
+                // result in ambiguous entries for servers tied to identical
+                // accounts)
+                if (hostname == null)
+                    addRecordForLogin(record, recordService.getUsername(record));
+
+            });
+
+            // Client has been refreshed
+            this.cacheTimestamp = System.currentTimeMillis();
+
+        }
+        finally {
+            cacheLock.writeLock().unlock();
+        }
+
+    }
+
+    /**
+     * Associates the given record with the given hostname. The hostname may be
+     * null. Both {@link #cachedRecordsByHost} and {@link #cachedAmbiguousHosts}
+     * are updated appropriately. The write lock of {@link #cacheLock} must
+     * already be acquired before invoking this function.
+     *
+     * @param record
+     *     The record to associate with the hosts in the given field.
+     *
+     * @param hostname
+     *     The hostname/address that the given record should be associated
+     *     with. This may be null.
+     */
+    private void addRecordForHost(KeeperRecord record, String hostname) {
+
+        if (hostname == null)
+            return;
+
+        KeeperRecord existing = cachedRecordsByHost.putIfAbsent(hostname, record);
+        if (existing != null && record != existing)
+            cachedAmbiguousHosts.add(hostname);
+
+    }
+
+    /**
+     * Associates the given record with the given username. The given username
+     * may be null. Both {@link #cachedRecordsByUsername} and
+     * {@link #cachedAmbiguousUsernames} are updated appropriately. The write
+     * lock of {@link #cacheLock} must already be acquired before invoking this
+     * function.
+     *
+     * @param record
+     *     The record to associate with the given username.
+     *
+     * @param username
+     *     The username that the given record should be associated with. This
+     *     may be null.
+     */
+    private void addRecordForLogin(KeeperRecord record, String username) {
+
+        if (username == null)
+            return;
+
+        KeeperRecord existing = cachedRecordsByUsername.putIfAbsent(username, record);
+        if (existing != null && record != existing)
+            cachedAmbiguousUsernames.add(username);
+
+    }
+
+    /**
+     * Returns all records accessible via Keeper Secrets Manager. The records
+     * returned are arbitrarily ordered.
      *
      * @return
      *     An unmodifiable Collection of all records accessible via Keeper
-     *     Secrets Manager, in no particular order.
+     *     Secrets Manager,  in no particular order.
      *
      * @throws GuacamoleException
      *     If an error occurs that prevents records from being retrieved.
      */
-    public Collection<KeeperRecord> getRecords(
+    public Collection<KeeperRecord> getRecords() throws GuacamoleException {
-                @Nullable String ksmConfig) throws GuacamoleException {
+        validateCache();
-
+        cacheLock.readLock().lock();
-        // Call through to the associated KSM cache instance
+        try {
-        return createCacheIfNeeded(ksmConfig).getRecords();
+            return Collections.unmodifiableCollection(cachedRecordsByUid.values());
+        }
+        finally {
+            cacheLock.readLock().unlock();
+        }
     }
 
     /**
-     * Returns the record having the given KSM config, and the given UID.
+     * Returns the record having the given UID. If no such record exists, null
-     * If no such record exists, null is returned.
+     * is returned.
-     *
-     * @param ksmConfig
-     *     The base-64 encoded JSON KSM config blob associated associated with
-     *     the KSM vault that should be used to fetch the record.
      *
      * @param uid
      *     The UID of the record to return.
@@ -134,21 +352,21 @@ public class KsmClient {
      * @throws GuacamoleException
      *     If an error occurs that prevents the record from being retrieved.
      */
-    public KeeperRecord getRecord(
+    public KeeperRecord getRecord(String uid) throws GuacamoleException {
-                @Nullable String ksmConfig, String uid) throws GuacamoleException {
+        validateCache();
-
+        cacheLock.readLock().lock();
-        // Call through to the associated KSM cache instance
+        try {
-        return createCacheIfNeeded(ksmConfig).getRecord(uid);
+            return cachedRecordsByUid.get(uid);
+        }
+        finally {
+            cacheLock.readLock().unlock();
+        }
     }
 
     /**
-     * Returns the record associated with the given hostname or IP address
+     * Returns the record associated with the given hostname or IP address. If
-     * and the given KSM config. If no such record exists, or there are multiple
+     * no such record exists, or there are multiple such records, null is
-     * such records, null is returned.
+     * returned.
-     *
-     * @param ksmConfig
-     *     The base-64 encoded JSON KSM config blob associated associated with
-     *     the KSM vault that should be used to fetch the record.
      *
      * @param hostname
      *     The hostname of the record to return.
@@ -160,22 +378,30 @@ public class KsmClient {
      * @throws GuacamoleException
      *     If an error occurs that prevents the record from being retrieved.
      */
-    public KeeperRecord getRecordByHost(
+    public KeeperRecord getRecordByHost(String hostname) throws GuacamoleException {
-                @Nullable String ksmConfig, String hostname) throws GuacamoleException {
+        validateCache();
+        cacheLock.readLock().lock();
+        try {
 
-        // Call through to the associated KSM cache instance
+            if (cachedAmbiguousHosts.contains(hostname)) {
-        return createCacheIfNeeded(ksmConfig).getRecordByHost(hostname);
+                logger.debug("The hostname/address \"{}\" is referenced by "
+                        + "multiple Keeper records and cannot be used to "
+                        + "locate individual secrets.", hostname);
+                return null;
+            }
 
+            return cachedRecordsByHost.get(hostname);
+
+        }
+        finally {
+            cacheLock.readLock().unlock();
+        }
     }
 
     /**
      * Returns the record associated with the given username. If no such record
      * exists, or there are multiple such records, null is returned.
      *
-     * @param ksmConfig
-     *     The base-64 encoded JSON KSM config blob associated associated with
-     *     the KSM vault that should be used to fetch the record.
-     *
      * @param username
      *     The username of the record to return.
      *
@@ -186,24 +412,31 @@ public class KsmClient {
      * @throws GuacamoleException
      *     If an error occurs that prevents the record from being retrieved.
      */
-    public KeeperRecord getRecordByLogin(
+    public KeeperRecord getRecordByLogin(String username) throws GuacamoleException {
-                @Nullable String ksmConfig, String username) throws GuacamoleException {
+        validateCache();
+        cacheLock.readLock().lock();
+        try {
 
-        // Call through to the associated KSM cache instance
+            if (cachedAmbiguousUsernames.contains(username)) {
-        return createCacheIfNeeded(ksmConfig).getRecordByLogin(username);
+                logger.debug("The username \"{}\" is referenced by multiple "
+                        + "Keeper records and cannot be used to locate "
+                        + "individual secrets.", username);
+                return null;
+            }
 
+            return cachedRecordsByUsername.get(username);
+
+        }
+        finally {
+            cacheLock.readLock().unlock();
+        }
     }
 
     /**
-     * Returns the value of the secret stored within the Keeper Secrets Manager vault
+     * Returns the value of the secret stored within Keeper Secrets Manager and
-     * associated with the provided KSM config, and represented by the given Keeper
+     * represented by the given Keeper notation. Keeper notation locates the
-     * notation. Keeper notation locates the value of a specific field, custom field,
+     * value of a specific field, custom field, or file associated with a
-     * or file associated with a specific record.
+     * specific record. See: https://docs.keeper.io/secrets-manager/secrets-manager/about/keeper-notation
-     * See: https://docs.keeper.io/secrets-manager/secrets-manager/about/keeper-notation
-     *
-     * @param ksmConfig
-     *     The base-64 encoded JSON KSM config blob associated associated with
-     *     the KSM vault that should be used to fetch the record.
      *
      * @param notation
      *     The Keeper notation of the secret to retrieve.
@@ -216,11 +449,40 @@ public class KsmClient {
      *     If the requested secret cannot be retrieved or the Keeper notation
      *     is invalid.
      */
-    public Future<String> getSecret(
+    public Future<String> getSecret(String notation) throws GuacamoleException {
-        @Nullable String ksmConfig, String notation) throws GuacamoleException {
+        validateCache();
+        cacheLock.readLock().lock();
+        try {
 
-        // Call through to the associated KSM cache instance
+            // Retrieve any relevant file asynchronously
-        return createCacheIfNeeded(ksmConfig).getSecret(notation);
+            Matcher fileNotationMatcher = KEEPER_FILE_NOTATION.matcher(notation);
+            if (fileNotationMatcher.matches())
+                return recordService.download(Notation.getFile(cachedSecrets, notation));
+
+            // Retrieve string values synchronously
+            return CompletableFuture.completedFuture(Notation.getValue(cachedSecrets, notation));
+
+        }
+
+        // Unfortunately, the notation parser within the Keeper SDK throws
+        // plain Errors for retrieval failures ...
+        catch (Error e) {
+            logger.warn("Record \"{}\" does not exist.", notation);
+            logger.debug("Retrieval of record by Keeper notation failed.", e);
+            return CompletableFuture.completedFuture(null);
+        }
+
+        // ... and plain Exceptions for parse failures (no subclasses)
+        catch (Exception e) {
+            logger.warn("\"{}\" is not valid Keeper notation. Please check "
+                    + "the documentation at {} for valid formatting.",
+                    notation, KEEPER_NOTATION_DOC_URL);
+            logger.debug("Provided Keeper notation could not be parsed.", e);
+            return CompletableFuture.completedFuture(null);
+        }
+        finally {
+            cacheLock.readLock().unlock();
+        }
 
     }
 

extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClientFactory.java

@@ -24,22 +24,22 @@ import javax.annotation.Nonnull;
 import com.keepersecurity.secretsManager.core.SecretsManagerOptions;
 
 /**
- * Factory for creating KsmCache instances.
+ * Factory for creating KsmClient instances.
  */
-public interface KsmCacheFactory {
+public interface KsmClientFactory {
 
     /**
-     * Returns a new instance of a KsmCache instance associated with
+     * Returns a new instance of a KsmClient instance associated with
      * the provided KSM configuration options.
      *
      * @param ksmConfigOptions
-     *     The KSM config options to use when constructing the KsmCache
+     *     The KSM config options to use when constructing the KsmClient
      *     object.
      *
      * @return
-     *     A new KsmCache instance associated with the provided KSM config
+     *     A new KsmClient instance associated with the provided KSM config
      *     options.
      */
-    KsmCache create(@Nonnull SecretsManagerOptions ksmConfigOptions);
+    KsmClient create(@Nonnull SecretsManagerOptions ksmConfigOptions);
 
 }

extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java

@@ -22,31 +22,47 @@ package org.apache.guacamole.vault.ksm.secret;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 import com.keepersecurity.secretsManager.core.KeeperRecord;
+import com.keepersecurity.secretsManager.core.SecretsManagerOptions;
+
 import java.io.UnsupportedEncodingException;
 import java.net.URLEncoder;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.Future;
 
+import javax.annotation.Nonnull;
+
 import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.net.auth.Connectable;
+import org.apache.guacamole.net.auth.Connection;
+import org.apache.guacamole.net.auth.ConnectionGroup;
+import org.apache.guacamole.net.auth.Directory;
+import org.apache.guacamole.net.auth.UserContext;
 import org.apache.guacamole.protocol.GuacamoleConfiguration;
 import org.apache.guacamole.token.TokenFilter;
+import org.apache.guacamole.vault.ksm.conf.KsmAttributeService;
 import org.apache.guacamole.vault.ksm.conf.KsmConfigurationService;
 import org.apache.guacamole.vault.secret.VaultSecretService;
 import org.apache.guacamole.vault.secret.WindowsUsername;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Service which retrieves secrets from Keeper Secrets Manager.
+ * The configuration used to connect to KSM can be set at a global
+ * level using guacamole.properties, or using a connection group
+ * attribute.
  */
 @Singleton
 public class KsmSecretService implements VaultSecretService {
 
     /**
-     * Client for retrieving records and secrets from Keeper Secrets Manager.
+     * Logger for this class.
      */
-    @Inject
+    private static final Logger logger = LoggerFactory.getLogger(VaultSecretService.class);
-    private KsmClient ksm;
 
     /**
      * Service for retrieving data from records.
@@ -60,6 +76,53 @@ public class KsmSecretService implements VaultSecretService {
     @Inject
     private KsmConfigurationService confService;
 
+    /**
+     * Factory for creating KSM client instances.
+     */
+    @Inject
+    private KsmClientFactory ksmClientFactory;
+
+    /**
+     * A map of base-64 encoded JSON KSM config blobs to associated KSM client instances.
+     * The `null` entry in this Map is associated with the KSM configuration parsed
+     * from the guacamole.properties config file. A distinct KSM client will exist for
+     * every KSM config.
+     */
+    private final ConcurrentMap<String, KsmClient> ksmClientMap = new ConcurrentHashMap<>();
+
+    /**
+     * Create and return a KSM cache for the provided KSM config if not already
+     * present in the cache map, otherwise return the existing cache entry.
+     *
+     * @param ksmConfig
+     *     The base-64 encoded JSON KSM config blob associated with the cache entry.
+     *     If an associated entry does not already exist, it will be created using
+     *     this configuration.
+     *
+     * @return
+     *     A KSM cache for the provided KSM config if not already present in the
+     *     cache map, otherwise the existing cache entry.
+     *
+     * @throws GuacamoleException
+     *     If an error occurs while creating the KSM cache.
+     */
+    private KsmClient getClient(@Nonnull String ksmConfig)
+            throws GuacamoleException {
+
+        // If a cache already exists for the provided config, use it
+        KsmClient ksmClient = ksmClientMap.get(ksmConfig);
+        if (ksmClient != null)
+            return ksmClient;
+
+        // Create and store a new KSM cache instance for the provided KSM config blob
+        SecretsManagerOptions options = confService.getSecretsManagerOptions(ksmConfig);
+        ksmClient = ksmClientFactory.create(options);
+        KsmClient prevClient = ksmClientMap.putIfAbsent(ksmConfig, ksmClient);
+
+        // If the cache was already set before this thread got there, use the existing one
+        return prevClient != null ? prevClient : ksmClient;
+    }
+
     @Override
     public String canonicalize(String nameComponent) {
         try {
@@ -74,9 +137,21 @@ public class KsmSecretService implements VaultSecretService {
         }
     }
 
+    @Override
+    public Future<String> getValue(UserContext userContext, Connectable connectable,
+            String name) throws GuacamoleException {
+
+        // Attempt to find a KSM config for this connection or group
+        String ksmConfig = getConnectionGroupKsmConfig(userContext, connectable);
+
+        return getClient(ksmConfig).getSecret(name);
+    }
+
     @Override
     public Future<String> getValue(String name) throws GuacamoleException {
-        return ksm.getSecret(name);
+
+        // Use the default KSM configuration from guacamole.properties
+        return getClient(confService.getKsmConfig()).getSecret(name);
     }
 
     /**
@@ -153,13 +228,82 @@ public class KsmSecretService implements VaultSecretService {
 
     }
 
+    /**
+     * Search for a KSM configuration attribute, recursing up the connection group tree
+     * until a connection group with the appropriate attribute is found. If the KSM config
+     * is found, it will be returned. If not, the default value from the config file will
+     * be returned.
+     *
+     * @param userContext
+     *     The userContext associated with the connection or connection group.
+     *
+     * @param connectable
+     *     A connection or connection group for which the tokens are being replaced.
+     *
+     * @return
+     *     The value of the KSM configuration attribute if found in the tree, the default
+     *     KSM config blob defined in guacamole.properties otherwise.
+     *
+     * @throws GuacamoleException
+     *     If an error occurs while attempting to retrieve the KSM config attribute, or if
+     *     no KSM config is found in the connection group tree, and the value is also not
+     *     defined in the config file.
+     */
+    private String getConnectionGroupKsmConfig(
+            UserContext userContext, Connectable connectable) throws GuacamoleException {
+
+        // Check to make sure it's a usable type before proceeding
+        if (
+                !(connectable instanceof Connection)
+                && !(connectable instanceof ConnectionGroup)) {
+            logger.warn(
+                    "Unsupported Connectable type: {}; skipping KSM config lookup.",
+                    connectable.getClass());
+
+            // Use the default value if searching is impossible
+            return confService.getKsmConfig();
+        }
+
+        // For connections, start searching the parent group for the KSM config
+        // For connection groups, start searching the group directly
+        String parentIdentifier = (connectable instanceof Connection)
+                ? ((Connection) connectable).getParentIdentifier()
+                : ((ConnectionGroup) connectable).getIdentifier();
+
+        Directory<ConnectionGroup> connectionGroupDirectory = userContext.getConnectionGroupDirectory();
+        while (true) {
+
+            // Fetch the parent group, if one exists
+            ConnectionGroup group = connectionGroupDirectory.get(parentIdentifier);
+            if (group == null)
+                break;
+
+            // If the current connection group has the KSM configuration attribute, return immediately
+            String ksmConfig = group.getAttributes().get(KsmAttributeService.KSM_CONFIGURATION_ATTRIBUTE);
+            if (ksmConfig != null)
+                return ksmConfig;
+
+            // Otherwise, keep searching up the tree until an appropriate configuration is found
+            parentIdentifier = group.getParentIdentifier();
+        }
+
+        // If no KSM configuration was ever found, use the default value
+        return confService.getKsmConfig();
+    }
+
     @Override
-    public Map<String, Future<String>> getTokens(GuacamoleConfiguration config,
+    public Map<String, Future<String>> getTokens(UserContext userContext, Connectable connectable,
-            TokenFilter filter) throws GuacamoleException {
+            GuacamoleConfiguration config, TokenFilter filter) throws GuacamoleException {
 
         Map<String, Future<String>> tokens = new HashMap<>();
         Map<String, String> parameters = config.getParameters();
 
+        // Attempt to find a KSM config for this connection or group
+        String ksmConfig = getConnectionGroupKsmConfig(userContext, connectable);
+
+        // Get a client instance for this KSM config
+        KsmClient ksm = getClient(ksmConfig);
+
         // Retrieve and define server-specific tokens, if any
         String hostname = parameters.get("hostname");
         if (hostname != null && !hostname.isEmpty())