From f7d90a641ee4d0d715c4b0f53f7ed1368cb850cc Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Tue, 28 Jun 2022 20:55:19 +0000 Subject: [PATCH 1/7] GUACAMOLE-1629: Add configuration properties and associated translations. --- .../vault/conf/VaultAttributeService.java | 40 ++++++++++++ .../vault/user/VaultUserContext.java | 16 +++++ .../ksm/KsmAuthenticationProviderModule.java | 3 + .../vault/ksm/conf/KsmAttributeService.java | 63 +++++++++++++++++++ .../src/main/resources/translations/en.json | 12 ++++ 5 files changed, 134 insertions(+) create mode 100644 extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/conf/VaultAttributeService.java create mode 100644 extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmAttributeService.java create mode 100644 extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/resources/translations/en.json diff --git a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/conf/VaultAttributeService.java b/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/conf/VaultAttributeService.java new file mode 100644 index 000000000..b77601621 --- /dev/null +++ b/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/conf/VaultAttributeService.java @@ -0,0 +1,40 @@ +/* + * 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.conf; + +import java.util.Collection; + +import org.apache.guacamole.form.Form; + +/** + * A service that exposes attributes for the admin UI, specific to the vault + * implementation. Any vault implementation will need to expose the attributes + * necessary for that implementation. + */ +public interface VaultAttributeService { + + /** + * Return all connection group attributes to be exposed through the admin UI. + * + * @return + * All connection group attributes to be exposed through the admin UI. + */ + public Collection
getConnectionGroupAttributes(); +} diff --git a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/user/VaultUserContext.java b/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/user/VaultUserContext.java index 53901483e..58056842b 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/user/VaultUserContext.java +++ b/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/user/VaultUserContext.java @@ -22,12 +22,15 @@ package org.apache.guacamole.vault.user; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.AssistedInject; + +import java.util.Collection; import java.util.HashMap; import java.util.Map; import java.util.concurrent.ExecutionException; 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.Connection; import org.apache.guacamole.net.auth.ConnectionGroup; import org.apache.guacamole.net.auth.TokenInjectingUserContext; @@ -35,6 +38,7 @@ import org.apache.guacamole.net.auth.UserContext; import org.apache.guacamole.protocol.GuacamoleConfiguration; import org.apache.guacamole.token.GuacamoleTokenUndefinedException; import org.apache.guacamole.token.TokenFilter; +import org.apache.guacamole.vault.conf.VaultAttributeService; import org.apache.guacamole.vault.conf.VaultConfigurationService; import org.apache.guacamole.vault.secret.VaultSecretService; import org.slf4j.Logger; @@ -121,6 +125,13 @@ public class VaultUserContext extends TokenInjectingUserContext { @Inject private VaultSecretService secretService; + /** + * Service for retrieving any custom attributes defined for the + * current vault implementation. + */ + @Inject + private VaultAttributeService attributeService; + /** * Creates a new VaultUserContext which automatically injects tokens * containing values of secrets retrieved from a vault. The given @@ -403,4 +414,9 @@ public class VaultUserContext extends TokenInjectingUserContext { } + @Override + public Collection getConnectionGroupAttributes() { + return attributeService.getConnectionGroupAttributes(); + } + } diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/KsmAuthenticationProviderModule.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/KsmAuthenticationProviderModule.java index bcc5a784e..17580b8a0 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/KsmAuthenticationProviderModule.java +++ b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/KsmAuthenticationProviderModule.java @@ -21,8 +21,10 @@ package org.apache.guacamole.vault.ksm; import org.apache.guacamole.GuacamoleException; import org.apache.guacamole.vault.VaultAuthenticationProviderModule; +import org.apache.guacamole.vault.ksm.conf.KsmAttributeService; 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.KsmClient; import org.apache.guacamole.vault.ksm.secret.KsmRecordService; @@ -51,6 +53,7 @@ public class KsmAuthenticationProviderModule // 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); } diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmAttributeService.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmAttributeService.java new file mode 100644 index 000000000..ade0f97e9 --- /dev/null +++ b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmAttributeService.java @@ -0,0 +1,63 @@ +/* + * 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 java.util.Arrays; +import java.util.Collection; +import java.util.Collections; + +import org.apache.guacamole.form.Form; +import org.apache.guacamole.form.MultilineField; +import org.apache.guacamole.vault.conf.VaultAttributeService; + +import com.google.inject.Singleton; + +/** + * A service that exposes KSM-specific attributes, allowing setting KSM + * configuration through the admin interface. + */ +@Singleton +public class KsmAttributeService implements VaultAttributeService { + + /** + * The name of the attribute which can contain a KSM configuration blob + * associated with a connection group. + */ + public static final String KSM_CONFIGURATION_ATTRIBUTE = "ksm-config"; + + /** + * All attributes related to configuring the KSM vault on a + * per-connection-group basis. + */ + public static final Form KSM_CONFIGURATION_FORM = new Form("ksm-config", + Arrays.asList(new MultilineField(KSM_CONFIGURATION_ATTRIBUTE))); + + /** + * All KSM-specific connection group attributes, organized by form. + */ + public static final Collection KSM_CONNECTION_GROUP_ATTRIBUTES = + Collections.unmodifiableCollection(Arrays.asList(KSM_CONFIGURATION_FORM)); + + @Override + public Collection getConnectionGroupAttributes() { + return KSM_CONNECTION_GROUP_ATTRIBUTES; + } + +} diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/resources/translations/en.json b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/resources/translations/en.json new file mode 100644 index 000000000..abda16cef --- /dev/null +++ b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/resources/translations/en.json @@ -0,0 +1,12 @@ +{ + + "DATA_SOURCE_KEEPER_SECRETS_MANAGER" : { + "NAME" : "Keeper Secrets Manager" + }, + + "CONNECTION_GROUP_ATTRIBUTES" : { + "SECTION_HEADER_KSM_CONFIG" : "Keeper Secrets Manager", + "FIELD_HEADER_KSM_CONFIG" : "KSM Service Configuration " + } + +} From 16efc0cdc1836617933c701505ed53b877f1a121 Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Tue, 28 Jun 2022 23:19:15 +0000 Subject: [PATCH 2/7] GUACAMOLE-1629: Implement multiple-vault support for KSM codebase. --- .../ksm/KsmAuthenticationProviderModule.java | 9 + .../guacamole/vault/ksm/conf/KsmConfig.java | 60 +++ .../vault/ksm/conf/KsmConfigProperty.java | 13 +- .../ksm/conf/KsmConfigurationService.java | 30 +- .../guacamole/vault/ksm/secret/KsmCache.java | 484 ++++++++++++++++++ .../vault/ksm/secret/KsmCacheFactory.java | 45 ++ .../guacamole/vault/ksm/secret/KsmClient.java | 426 ++++----------- 7 files changed, 712 insertions(+), 355 deletions(-) create mode 100644 extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfig.java create mode 100644 extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmCache.java create mode 100644 extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmCacheFactory.java diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/KsmAuthenticationProviderModule.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/KsmAuthenticationProviderModule.java index 17580b8a0..9d54439ea 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/KsmAuthenticationProviderModule.java +++ b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/KsmAuthenticationProviderModule.java @@ -26,10 +26,14 @@ 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.KsmRecordService; import org.apache.guacamole.vault.secret.VaultSecretService; +import com.google.inject.assistedinject.FactoryModuleBuilder; + /** * Guice module which configures injections specific to Keeper Secrets * Manager support. @@ -56,6 +60,11 @@ public class KsmAuthenticationProviderModule bind(VaultAttributeService.class).to(KsmAttributeService.class); bind(VaultConfigurationService.class).to(KsmConfigurationService.class); bind(VaultSecretService.class).to(KsmSecretService.class); + + // Bind factory for creating KSM Caches + install(new FactoryModuleBuilder() + .implement(KsmCache.class, KsmCache.class) + .build(KsmCacheFactory.class)); } } diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfig.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfig.java new file mode 100644 index 000000000..54aaec753 --- /dev/null +++ b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfig.java @@ -0,0 +1,60 @@ +/* + * 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.InMemoryStorage; +import com.keepersecurity.secretsManager.core.KeyValueStorage; +import org.apache.guacamole.GuacamoleException; +import org.apache.guacamole.GuacamoleServerException; + +/** + * A utility for parsing base64-encoded JSON, as output by the Keeper Commander + * CLI tool via the "sm client add" command into a Keeper Secrets Manager + * {@link KeyValueStorage} object. + */ +public class KsmConfig { + + /** + * 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. + */ + public 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); + } + + } + +} diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfigProperty.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfigProperty.java index aaddb0de0..afd8c921a 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfigProperty.java +++ b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfigProperty.java @@ -19,10 +19,8 @@ package org.apache.guacamole.vault.ksm.conf; -import com.keepersecurity.secretsManager.core.InMemoryStorage; import com.keepersecurity.secretsManager.core.KeyValueStorage; import org.apache.guacamole.GuacamoleException; -import org.apache.guacamole.GuacamoleServerException; import org.apache.guacamole.properties.GuacamoleProperty; /** @@ -39,15 +37,8 @@ public abstract class KsmConfigProperty implements GuacamoleProperty 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 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 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 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 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 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 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 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(); + } + + } + +} diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmCacheFactory.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmCacheFactory.java new file mode 100644 index 000000000..4da9ee6bd --- /dev/null +++ b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmCacheFactory.java @@ -0,0 +1,45 @@ +/* + * 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 javax.annotation.Nonnull; + +import com.keepersecurity.secretsManager.core.SecretsManagerOptions; + +/** + * Factory for creating KsmCache instances. + */ +public interface KsmCacheFactory { + + /** + * Returns a new instance of a KsmCache instance associated with + * the provided KSM configuration options. + * + * @param ksmConfigOptions + * The KSM config options to use when constructing the KsmCache + * object. + * + * @return + * A new KsmCache instance associated with the provided KSM config + * options. + */ + KsmCache create(@Nonnull SecretsManagerOptions ksmConfigOptions); + +} diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java index fa177014b..ae80e68e2 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java +++ b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java @@ -21,29 +21,18 @@ package org.apache.guacamole.vault.ksm.secret; import com.google.inject.Inject; import com.google.inject.Singleton; -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 javax.annotation.Nullable; + 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 @@ -56,11 +45,6 @@ import org.slf4j.LoggerFactory; @Singleton public class KsmClient { - /** - * Logger for this class. - */ - private static final Logger logger = LoggerFactory.getLogger(KsmClient.class); - /** * Service for retrieving configuration information. */ @@ -68,268 +52,78 @@ public class KsmClient { private KsmConfigurationService confService; /** - * Service for retrieving data from records. + * Factory for creating KSM cache instances for particular KSM configs. */ @Inject - private KsmRecordService recordService; + private KsmCacheFactory ksmCacheFactory; /** - * The publicly-accessible URL for Keeper's documentation covering Keeper - * notation. + * 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 static final String KEEPER_NOTATION_DOC_URL = - "https://docs.keeper.io/secrets-manager/secrets-manager/about/keeper-notation"; + private final Map ksmCacheMap = new HashMap<>(); /** - * 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; - - /** - * 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 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 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 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 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 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. + * + * @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 preventing the cached data from being refreshed. + * If an error occurs while creating the KSM cache. */ - private void validateCache() throws GuacamoleException { + private KsmCache createCacheIfNeeded(@Nullable String ksmConfig) + 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 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(); - } + // 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)); } /** - * 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. + * Returns all records accessible via Keeper Secrets Manager, associated + * with the provided KSM config. If no KSM config is provided, records + * associated with the default configuration as retrieved from the config + * file will be used. The records returned are arbitrarily ordered. * - * @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. + * @param ksmConfig + * The base-64 encoded JSON KSM config blob associated associated with + * the KSM vault that should be used to fetch the records. * * @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 getRecords() throws GuacamoleException { - validateCache(); - cacheLock.readLock().lock(); - try { - return Collections.unmodifiableCollection(cachedRecordsByUid.values()); - } - finally { - cacheLock.readLock().unlock(); - } + public Collection getRecords( + @Nullable String ksmConfig) throws GuacamoleException { + + // Call through to the associated KSM cache instance + return createCacheIfNeeded(ksmConfig).getRecords(); } /** - * Returns the record having the given UID. If no such record exists, null - * is returned. + * Returns the record having the given KSM config, and the given UID. + * If no such record exists, 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 uid * The UID of the record to return. @@ -340,21 +134,21 @@ public class KsmClient { * @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(); - } + public KeeperRecord getRecord( + @Nullable String ksmConfig, String uid) throws GuacamoleException { + + // Call through to the associated KSM cache instance + return createCacheIfNeeded(ksmConfig).getRecord(uid); } /** - * 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. + * Returns the record associated with the given hostname or IP address + * and the given KSM config. 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 hostname * The hostname of the record to return. @@ -366,30 +160,22 @@ public class KsmClient { * @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 { + public KeeperRecord getRecordByHost( + @Nullable String ksmConfig, String hostname) throws GuacamoleException { - 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; - } + // Call through to the associated KSM cache instance + return createCacheIfNeeded(ksmConfig).getRecordByHost(hostname); - 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. * @@ -400,31 +186,24 @@ public class KsmClient { * @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 { + public KeeperRecord getRecordByLogin( + @Nullable String ksmConfig, String username) throws GuacamoleException { - 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; - } + // Call through to the associated KSM cache instance + return createCacheIfNeeded(ksmConfig).getRecordByLogin(username); - 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 + * Returns the value of the secret stored within the Keeper Secrets Manager vault + * associated with the provided KSM config, 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 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. @@ -437,40 +216,11 @@ public class KsmClient { * If the requested secret cannot be retrieved or the Keeper notation * is invalid. */ - public Future getSecret(String notation) throws GuacamoleException { - validateCache(); - cacheLock.readLock().lock(); - try { + public Future getSecret( + @Nullable String ksmConfig, String notation) throws GuacamoleException { - // 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(); - } + // Call through to the associated KSM cache instance + return createCacheIfNeeded(ksmConfig).getSecret(notation); } From fee2f8b416afdaf8d574d8b9a3bb2f98addaa133 Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Wed, 29 Jun 2022 20:29:58 +0000 Subject: [PATCH 3/7] GUACAMOLE-1629: Hook KSM vault code into base vault code and clean up. --- .../vault/secret/VaultSecretService.java | 45 +- .../vault/user/VaultUserContext.java | 23 +- .../ksm/KsmAuthenticationProviderModule.java | 10 +- .../vault/ksm/conf/KsmConfigProperty.java | 44 -- .../ksm/conf/KsmConfigurationService.java | 72 ++- .../guacamole/vault/ksm/secret/KsmCache.java | 484 ------------------ .../guacamole/vault/ksm/secret/KsmClient.java | 448 ++++++++++++---- ...acheFactory.java => KsmClientFactory.java} | 12 +- .../vault/ksm/secret/KsmSecretService.java | 156 +++++- 9 files changed, 627 insertions(+), 667 deletions(-) delete mode 100644 extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfigProperty.java delete mode 100644 extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmCache.java rename extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/{KsmCacheFactory.java => KsmClientFactory.java} (79%) diff --git a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/secret/VaultSecretService.java b/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/secret/VaultSecretService.java index 76349bad9..81204beb5 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/secret/VaultSecretService.java +++ b/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 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 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> getTokens(GuacamoleConfiguration config, - TokenFilter filter) throws GuacamoleException; + Map> getTokens(UserContext userContext, Connectable connectable, + GuacamoleConfiguration config, TokenFilter filter) throws GuacamoleException; } diff --git a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/user/VaultUserContext.java b/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/user/VaultUserContext.java index 58056842b..876d7d186 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/user/VaultUserContext.java +++ b/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> getTokens(Map tokenMapping, + private Map> getTokens( + Connectable connectable, Map 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 secret = secretService.getValue(secretName); + Future 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, - config, new TokenFilter(tokens)))); + tokens.putAll(resolve(getTokens(connection, confService.getTokenMapping(), + filter, config, new TokenFilter(tokens)))); } diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/KsmAuthenticationProviderModule.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/KsmAuthenticationProviderModule.java index 9d54439ea..6a7a70c83 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/KsmAuthenticationProviderModule.java +++ b/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) - .build(KsmCacheFactory.class)); + .implement(KsmClient.class, KsmClient.class) + .build(KsmClientFactory.class)); } } diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfigProperty.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfigProperty.java deleted file mode 100644 index afd8c921a..000000000 --- a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfigProperty.java +++ /dev/null @@ -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 { - - @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); - } - -} diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfigurationService.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfigurationService.java index c50662cf5..3ef02b8b9 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmConfigurationService.java +++ b/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. - * If not provided as an argument, it must be present in the config file. + * CLI tool. This configuration data must be passed directly as an argument. * * @param ksmConfig - * Optional KSM config data. If provided, it will be used instead of any - * KSM config data present in the config file. If not provided, the value - * in the config file will be used. + * The KSM configuration blob to parse. * * @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 - * are not specified within guacamole.properties or cannot be parsed, or - * the KSM configuration cannot be parsed. + * If an invalid ksmConfig parameter is provided. */ - 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. - // If not provided as an argument, it must be present in the config file. - KeyValueStorage parsedKsmConfig = ksmConfig != null - ? KsmConfig.parseKsmConfig(ksmConfig) - : environment.getRequiredProperty(KSM_CONFIG); - - return new SecretsManagerOptions(parsedKsmConfig, null, getAllowUnverifiedCertificate()); + return new SecretsManagerOptions( + parseKsmConfig(ksmConfig), null, getAllowUnverifiedCertificate()); } } diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmCache.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmCache.java deleted file mode 100644 index 178493871..000000000 --- a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmCache.java +++ /dev/null @@ -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 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 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 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 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 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 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 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 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(); - } - - } - -} diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java index ae80e68e2..be514ddfe 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java +++ b/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 javax.annotation.Nullable; +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.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 KsmCacheFactory ksmCacheFactory; + private static final String KEEPER_NOTATION_DOC_URL = + "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 `null` entry in this Map is associated with the KSM configuration parsed - * from the guacamole.properties config file. + * 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 final Map 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 - * present in the cache map, the existing cache entry. + * 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 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 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 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 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 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 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. - * 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. + * The KSM configuration to use when retrieving properties from KSM. */ - private KsmCache createCacheIfNeeded(@Nullable String ksmConfig) - throws GuacamoleException { - - // 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)); + @AssistedInject + public KsmClient(@Assisted SecretsManagerOptions ksmConfig) { + this.ksmConfig = ksmConfig; } /** - * Returns all records accessible via Keeper Secrets Manager, associated - * with the provided KSM config. If no KSM config is provided, records - * associated with the default configuration as retrieved from the config - * file will be used. The records returned are arbitrarily ordered. + * Validates that all cached data is current with respect to + * {@link #CACHE_INTERVAL}, refreshing data from the server as needed. * - * @param ksmConfig - * The base-64 encoded JSON KSM config blob associated associated with - * the KSM vault that should be used to fetch the records. + * @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 { + + // 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 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 getRecords( - @Nullable String ksmConfig) throws GuacamoleException { - - // Call through to the associated KSM cache instance - return createCacheIfNeeded(ksmConfig).getRecords(); + public Collection getRecords() throws GuacamoleException { + validateCache(); + cacheLock.readLock().lock(); + try { + return Collections.unmodifiableCollection(cachedRecordsByUid.values()); + } + finally { + cacheLock.readLock().unlock(); + } } /** - * Returns the record having the given KSM config, and the given UID. - * If no such record exists, 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. + * Returns the record having the given UID. If no such record exists, null + * is returned. * * @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( - @Nullable String ksmConfig, String uid) throws GuacamoleException { - - // Call through to the associated KSM cache instance - return createCacheIfNeeded(ksmConfig).getRecord(uid); + 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 - * and the given KSM config. 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. + * 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. @@ -160,22 +378,30 @@ public class KsmClient { * @throws GuacamoleException * If an error occurs that prevents the record from being retrieved. */ - public KeeperRecord getRecordByHost( - @Nullable String ksmConfig, String hostname) throws GuacamoleException { + public KeeperRecord getRecordByHost(String hostname) throws GuacamoleException { + validateCache(); + cacheLock.readLock().lock(); + try { - // Call through to the associated KSM cache instance - return createCacheIfNeeded(ksmConfig).getRecordByHost(hostname); + 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 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( - @Nullable String ksmConfig, String username) throws GuacamoleException { + public KeeperRecord getRecordByLogin(String username) throws GuacamoleException { + validateCache(); + cacheLock.readLock().lock(); + try { - // Call through to the associated KSM cache instance - return createCacheIfNeeded(ksmConfig).getRecordByLogin(username); + 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 the Keeper Secrets Manager vault - * associated with the provided KSM config, 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 ksmConfig - * The base-64 encoded JSON KSM config blob associated associated with - * the KSM vault that should be used to fetch the record. + * 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. @@ -216,11 +449,40 @@ public class KsmClient { * If the requested secret cannot be retrieved or the Keeper notation * is invalid. */ - public Future getSecret( - @Nullable String ksmConfig, String notation) throws GuacamoleException { + public Future getSecret(String notation) throws GuacamoleException { + validateCache(); + cacheLock.readLock().lock(); + try { - // Call through to the associated KSM cache instance - return createCacheIfNeeded(ksmConfig).getSecret(notation); + // 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(); + } } diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmCacheFactory.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClientFactory.java similarity index 79% rename from extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmCacheFactory.java rename to extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClientFactory.java index 4da9ee6bd..f8220c16f 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmCacheFactory.java +++ b/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); } diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java index 21d7c91ac..6e2c8f201 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java +++ b/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 KsmClient ksm; + private static final Logger logger = LoggerFactory.getLogger(VaultSecretService.class); /** * 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 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 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 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 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> getTokens(GuacamoleConfiguration config, - TokenFilter filter) throws GuacamoleException { + public Map> getTokens(UserContext userContext, Connectable connectable, + GuacamoleConfiguration config, TokenFilter filter) throws GuacamoleException { Map> tokens = new HashMap<>(); Map 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()) From 5b69bf405d034258e1dd8ce34819add0dc2992ae Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Tue, 5 Jul 2022 17:59:44 +0000 Subject: [PATCH 4/7] GUACAMOLE-1629: Use TextField for KSM configuration since it's always one line. --- .../guacamole/vault/ksm/conf/KsmAttributeService.java | 4 ++-- .../apache/guacamole/vault/ksm/secret/KsmClient.java | 4 ++-- .../guacamole/vault/ksm/secret/KsmSecretService.java | 10 ++++------ 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmAttributeService.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmAttributeService.java index ade0f97e9..83ab9c4a3 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmAttributeService.java +++ b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmAttributeService.java @@ -24,7 +24,7 @@ import java.util.Collection; import java.util.Collections; import org.apache.guacamole.form.Form; -import org.apache.guacamole.form.MultilineField; +import org.apache.guacamole.form.TextField; import org.apache.guacamole.vault.conf.VaultAttributeService; import com.google.inject.Singleton; @@ -47,7 +47,7 @@ public class KsmAttributeService implements VaultAttributeService { * per-connection-group basis. */ public static final Form KSM_CONFIGURATION_FORM = new Form("ksm-config", - Arrays.asList(new MultilineField(KSM_CONFIGURATION_ATTRIBUTE))); + Arrays.asList(new TextField(KSM_CONFIGURATION_ATTRIBUTE))); /** * All KSM-specific connection group attributes, organized by form. diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java index be514ddfe..5572ef71d 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java +++ b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmClient.java @@ -215,7 +215,7 @@ public class KsmClient { cacheLock.writeLock().lock(); try { - // Client may have been updated since the read-only check. Re-verify + // 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; @@ -258,7 +258,7 @@ public class KsmClient { }); - // Client has been refreshed + // Cache has been refreshed this.cacheTimestamp = System.currentTimeMillis(); } diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java index 6e2c8f201..bea98d934 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java +++ b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java @@ -84,14 +84,12 @@ public class KsmSecretService implements VaultSecretService { /** * 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. + * A distinct KSM client will exist for every KSM config. */ private final ConcurrentMap ksmClientMap = new ConcurrentHashMap<>(); /** - * Create and return a KSM cache for the provided KSM config if not already + * Create and return a KSM client for the provided KSM config if not already * present in the cache map, otherwise return the existing cache entry. * * @param ksmConfig @@ -100,11 +98,11 @@ public class KsmSecretService implements VaultSecretService { * this configuration. * * @return - * A KSM cache for the provided KSM config if not already present in the + * A KSM client 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. + * If an error occurs while creating the KSM client. */ private KsmClient getClient(@Nonnull String ksmConfig) throws GuacamoleException { From 374f1b5e494c968166fe3eb9a66b6f7769981400 Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Wed, 6 Jul 2022 17:55:28 +0000 Subject: [PATCH 5/7] GUACAMOLE-1629: Always include any pre-existing connection group attributes when exposing new ones. --- .../guacamole/vault/conf/VaultAttributeService.java | 7 +++++-- .../apache/guacamole/vault/user/VaultUserContext.java | 10 +++++++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/conf/VaultAttributeService.java b/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/conf/VaultAttributeService.java index b77601621..f4129bb95 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/conf/VaultAttributeService.java +++ b/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/conf/VaultAttributeService.java @@ -31,10 +31,13 @@ import org.apache.guacamole.form.Form; public interface VaultAttributeService { /** - * Return all connection group attributes to be exposed through the admin UI. + * Return all custom connection group attributes to be exposed through the + * admin UI for the current vault implementation. * * @return - * All connection group attributes to be exposed through the admin UI. + * All custom connection group attributes to be exposed through the + * admin UI for the current vault implementation. + * */ public Collection getConnectionGroupAttributes(); } diff --git a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/user/VaultUserContext.java b/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/user/VaultUserContext.java index 876d7d186..06ee8aaac 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/user/VaultUserContext.java +++ b/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/user/VaultUserContext.java @@ -28,6 +28,9 @@ import java.util.HashMap; import java.util.Map; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; +import java.util.stream.Collectors; +import java.util.stream.Stream; + import org.apache.guacamole.GuacamoleException; import org.apache.guacamole.GuacamoleServerException; import org.apache.guacamole.form.Form; @@ -425,7 +428,12 @@ public class VaultUserContext extends TokenInjectingUserContext { @Override public Collection getConnectionGroupAttributes() { - return attributeService.getConnectionGroupAttributes(); + + // Add any custom attributes to any previously defined attributes + return Stream.concat( + super.getConnectionGroupAttributes().stream(), + attributeService.getConnectionGroupAttributes().stream() + ).collect(Collectors.toUnmodifiableList()); } } From 0585ab5e5bc505d1338a419a19ea1b5484792738 Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Wed, 6 Jul 2022 19:01:33 +0000 Subject: [PATCH 6/7] GUACAMOLE-1629: Fix client/cache confusion in comments. --- .../guacamole/vault/conf/VaultAttributeService.java | 1 - .../guacamole/vault/ksm/secret/KsmSecretService.java | 12 ++++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/conf/VaultAttributeService.java b/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/conf/VaultAttributeService.java index f4129bb95..2bd08cde6 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/conf/VaultAttributeService.java +++ b/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/conf/VaultAttributeService.java @@ -37,7 +37,6 @@ public interface VaultAttributeService { * @return * All custom connection group attributes to be exposed through the * admin UI for the current vault implementation. - * */ public Collection getConnectionGroupAttributes(); } diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java index bea98d934..2436a4e62 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java +++ b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java @@ -90,16 +90,16 @@ public class KsmSecretService implements VaultSecretService { /** * Create and return a KSM client for the provided KSM config if not already - * present in the cache map, otherwise return the existing cache entry. + * present in the client map, otherwise return the existing client entry. * * @param ksmConfig - * The base-64 encoded JSON KSM config blob associated with the cache entry. + * The base-64 encoded JSON KSM config blob associated with the client entry. * If an associated entry does not already exist, it will be created using * this configuration. * * @return * A KSM client for the provided KSM config if not already present in the - * cache map, otherwise the existing cache entry. + * client map, otherwise the existing client entry. * * @throws GuacamoleException * If an error occurs while creating the KSM client. @@ -107,17 +107,17 @@ public class KsmSecretService implements VaultSecretService { private KsmClient getClient(@Nonnull String ksmConfig) throws GuacamoleException { - // If a cache already exists for the provided config, use it + // If a client 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 + // Create and store a new KSM client 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 + // If the client was already set before this thread got there, use the existing one return prevClient != null ? prevClient : ksmClient; } From 12832bed88caff888e1193d109d58ca1d7e9905e Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Wed, 6 Jul 2022 19:06:42 +0000 Subject: [PATCH 7/7] GUACAMOLE-1629: Use Java 8 compatible collector to append connection group attributes. --- .../org/apache/guacamole/vault/user/VaultUserContext.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/user/VaultUserContext.java b/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/user/VaultUserContext.java index 06ee8aaac..dbfbbb9cb 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/user/VaultUserContext.java +++ b/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/user/VaultUserContext.java @@ -24,6 +24,7 @@ import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.AssistedInject; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.concurrent.ExecutionException; @@ -430,10 +431,11 @@ public class VaultUserContext extends TokenInjectingUserContext { public Collection getConnectionGroupAttributes() { // Add any custom attributes to any previously defined attributes - return Stream.concat( + return Collections.unmodifiableCollection(Stream.concat( super.getConnectionGroupAttributes().stream(), attributeService.getConnectionGroupAttributes().stream() - ).collect(Collectors.toUnmodifiableList()); + ).collect(Collectors.toList())); + } }