You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2022/06/30 19:51:59 UTC
[GitHub] [guacamole-client] mike-jumper commented on a diff in pull request #739: GUACAMOLE-1629: Allow vault configurations per connection group
mike-jumper commented on code in PR #739:
URL: https://github.com/apache/guacamole-client/pull/739#discussion_r911393943
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java:
##########
@@ -56,280 +45,87 @@
@Singleton
public class KsmClient {
- /**
- * Logger for this class.
- */
- private static final Logger logger = LoggerFactory.getLogger(KsmClient.class);
-
/**
* Service for retrieving configuration information.
*/
@Inject
private KsmConfigurationService confService;
/**
- * Service for retrieving data from records.
+ * Factory for creating KSM cache instances for particular KSM configs.
*/
@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;
-
- /**
- * 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;
+ private KsmCacheFactory ksmCacheFactory;
/**
- * 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.
+ * A map of base-64 encoded JSON KSM config blobs to associated KSM cache instances.
+ * The `null` entry in this Map is associated with the KSM configuration parsed
+ * from the guacamole.properties config file.
*/
- private KeeperSecrets cachedSecrets = null;
+ private final Map<String, KsmCache> ksmCacheMap = new HashMap<>();
/**
- * 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.
+ * Create and return a KSM cache for the provided KSM config if not already
+ * present in the cache map, the existing cache entry.
*
- * @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(confService.getSecretsManagerOptions());
- 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 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.
*
- * @param record
- * The record to associate with the hosts in the given field.
+ * @return
+ * A KSM cache for the provided KSM config if not already present in the
+ * cache map, otherwise the existing cache entry.
*
- * @param hostname
- * The hostname/address that the given record should be associated
- * with. This may be null.
+ * @throws GuacamoleException
+ * If an error occurs while creating the KSM cache.
*/
- private void addRecordForHost(KeeperRecord record, String hostname) {
+ private KsmCache createCacheIfNeeded(@Nullable String ksmConfig)
+ throws GuacamoleException {
- if (hostname == null)
- return;
-
- KeeperRecord existing = cachedRecordsByHost.putIfAbsent(hostname, record);
- if (existing != null && record != existing)
- cachedAmbiguousHosts.add(hostname);
+ // 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);
+ ksmCache = ksmCacheFactory.create(options);
+ ksmCacheMap.put(ksmConfig, ksmCache);
+ return ksmCache;
Review Comment:
Is this threadsafe?
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java:
##########
@@ -56,280 +45,87 @@
@Singleton
public class KsmClient {
- /**
- * Logger for this class.
- */
- private static final Logger logger = LoggerFactory.getLogger(KsmClient.class);
-
/**
* Service for retrieving configuration information.
*/
@Inject
private KsmConfigurationService confService;
/**
- * Service for retrieving data from records.
+ * Factory for creating KSM cache instances for particular KSM configs.
*/
@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;
-
- /**
- * 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;
+ private KsmCacheFactory ksmCacheFactory;
/**
- * 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.
+ * A map of base-64 encoded JSON KSM config blobs to associated KSM cache instances.
+ * The `null` entry in this Map is associated with the KSM configuration parsed
+ * from the guacamole.properties config file.
*/
- private KeeperSecrets cachedSecrets = null;
+ private final Map<String, KsmCache> ksmCacheMap = new HashMap<>();
/**
- * 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.
+ * Create and return a KSM cache for the provided KSM config if not already
+ * present in the cache map, the existing cache entry.
*
- * @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(confService.getSecretsManagerOptions());
- 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 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.
*
- * @param record
- * The record to associate with the hosts in the given field.
+ * @return
+ * A KSM cache for the provided KSM config if not already present in the
+ * cache map, otherwise the existing cache entry.
*
- * @param hostname
- * The hostname/address that the given record should be associated
- * with. This may be null.
+ * @throws GuacamoleException
+ * If an error occurs while creating the KSM cache.
*/
- private void addRecordForHost(KeeperRecord record, String hostname) {
+ private KsmCache createCacheIfNeeded(@Nullable String ksmConfig)
Review Comment:
If abstracting things as a cache: I suggest renaming to `getCache()` and letting the creation aspect of this be an internal concern.
That said, I think perhaps this should instead be abstracted at the `KsmClient` level. If the idea is that a particular connection might retrieve secrets using a different KSM config, I think it would make more sense for each config produce a dedicated, separate `KsmClient` singleton, rather than swapping out the internal cache of that client based on which config is in use.
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java:
##########
@@ -153,24 +180,82 @@ private void addRecordTokens(Map<String, Future<String>> tokens, String prefix,
}
+ /**
+ * 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, null 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, null otherwise.
+ *
+ * @throws GuacamoleException
+ * If an error occurs while attempting to retrieve the KSM config attribute.
+ */
+ 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());
+ return null;
+ }
+
+ // 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 (connectionGroupDirectory.getIdentifiers().contains(parentIdentifier)) {
+
+ // Fetch the parent group
+ ConnectionGroup group = connectionGroupDirectory.get(parentIdentifier);
Review Comment:
`getIdentifiers()` can be a heavy function, as it lists everything. Instead, since `get()` will return `null` if there is no such object, we should just call `get()` directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org