From aa06c81f2985a387c80ffda19991cf6fb8ea27ac Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Mon, 8 Aug 2022 23:10:41 +0000 Subject: [PATCH 1/5] GUACAMOLE-1661: Add domain search support for KSM vault extension. --- .../guacamole/vault/ksm/secret/KsmClient.java | 134 +++++++++++++++++- .../vault/ksm/secret/KsmSecretService.java | 12 ++ 2 files changed, 141 insertions(+), 5 deletions(-) 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 5572ef71d..c854fbd3b 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 @@ -34,6 +34,7 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; @@ -45,6 +46,8 @@ 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.apache.guacamole.vault.secret.WindowsUsername; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -63,6 +66,12 @@ public class KsmClient { */ private static final Logger logger = LoggerFactory.getLogger(KsmClient.class); + /** + * Service for retrieving configuration information. + */ + @Inject + private KsmConfigurationService confService; + /** * Service for retrieving data from records. */ @@ -179,6 +188,30 @@ public class KsmClient { */ private final Set cachedAmbiguousUsernames = new HashSet<>(); + /** + * All records retrieved from Keeper Secrets Manager, where each key is the + * domain of the corresponding record. The domain of a record is + * determined by {@link Login} fields, thus a record may be associated with + * multiple domains. If a record is associated with multiple domains, 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 #cachedAmbiguousDomains} must first be checked to + * verify that there is indeed only one record associated with that domain. + */ + private final Map cachedRecordsByDomain = new HashMap<>(); + + /** + * The set of all domains 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 #cachedRecordsByDomain}. + */ + private final Set cachedAmbiguousDomains = new HashSet<>(); + /** * Create a new KSM client based around the provided KSM configuration. * @@ -239,9 +272,17 @@ public class KsmClient { cachedAmbiguousUsernames.clear(); cachedRecordsByUsername.clear(); - // Store all records, sorting each into host-based and login-based - // buckets - records.forEach(record -> { + // Clear cache of domain-based records + cachedAmbiguousDomains.clear(); + cachedRecordsByDomain.clear(); + + // Store all records, sorting each into host-based, login-based, + // and domain-based buckets + Iterator recordIterator = records.iterator(); + while(recordIterator.hasNext()) { + + // Go through records one at a time + KeeperRecord record = recordIterator.next(); // Store based on UID ... cachedRecordsByUid.put(record.getRecordUid(), record); @@ -250,13 +291,38 @@ public class KsmClient { String hostname = recordService.getHostname(record); addRecordForHost(record, hostname); + // ... and domain + String domain = recordService.getDomain(record); + addRecordForDomain(record, domain); + + // Fetch the username + String username = recordService.getUsername(record); + + // If domains should be split out from usernames + if (username != null && confService.getSplitWindowsUsernames()) { + + // Attempt to split the domain of the username + WindowsUsername usernameAndDomain = ( + WindowsUsername.splitWindowsUsernameFromDomain(username)); + + if (usernameAndDomain.hasDomain()) { + + // Update the username if a domain has been stripped off + username = usernameAndDomain.getUsername(); + + // Use the username-split domain if not already set explicitly + if (domain == null) + addRecordForDomain(record, usernameAndDomain.getDomain()); + } + } + // 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)); + addRecordForLogin(record, username); - }); + } // Cache has been refreshed this.cacheTimestamp = System.currentTimeMillis(); @@ -268,6 +334,30 @@ public class KsmClient { } + /** + * Associates the given record with the given domain. The domain may be + * null. Both {@link #cachedRecordsByDomain} and {@link #cachedAmbiguousDomains} + * 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 domains in the given field. + * + * @param domain + * The domain that the given record should be associated with. + * This may be null. + */ + private void addRecordForDomain(KeeperRecord record, String domain) { + + if (domain == null) + return; + + KeeperRecord existing = cachedRecordsByDomain.putIfAbsent(domain, record); + if (existing != null && record != existing) + cachedAmbiguousDomains.add(domain); + + } + /** * Associates the given record with the given hostname. The hostname may be * null. Both {@link #cachedRecordsByHost} and {@link #cachedAmbiguousHosts} @@ -432,6 +522,40 @@ public class KsmClient { } } + /** + * Returns the record associated with the given domain. If no such record + * exists, or there are multiple such records, null is returned. + * + * @param domain + * The domain of the record to return. + * + * @return + * The record associated with the given domain, 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 getRecordByDomain(String domain) throws GuacamoleException { + validateCache(); + cacheLock.readLock().lock(); + try { + + if (cachedAmbiguousDomains.contains(domain)) { + logger.debug("The domain \"{}\" is referenced by multiple " + + "Keeper records and cannot be used to locate " + + "individual secrets.", domain); + return null; + } + + return cachedRecordsByDomain.get(domain); + + } + 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 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 d8168dc8f..604ae2865 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 @@ -344,6 +344,18 @@ public class KsmSecretService implements VaultSecretService { addRecordTokens(tokens, "KEEPER_GATEWAY_USER_", ksm.getRecordByLogin(filter.filter(gatewayUsername))); + // Retrieve and define domain tokens, if any + String domain = parameters.get("domain"); + if (domain != null && !domain.isEmpty()) + addRecordTokens(tokens, "KEEPER_DOMAIN_", + ksm.getRecordByDomain(filter.filter(domain))); + + // Retrieve and define gateway domain tokens, if any + String gatewayDomain = parameters.get("gateway-domain"); + if (gatewayDomain != null && !gatewayDomain.isEmpty()) + addRecordTokens(tokens, "KEEPER_GATEWAY_DOMAIN_", + ksm.getRecordByDomain(filter.filter(gatewayDomain))); + } return tokens; From 593cfaaffede48f09f72f5b72f9455ef3e56d477 Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Wed, 10 Aug 2022 23:49:59 +0000 Subject: [PATCH 2/5] GUACAMOLE-1661: Match by both user and domain when using KEEPER_USER_ tokens. --- .../vault/conf/VaultConfigurationService.java | 17 +++ .../ksm/conf/KsmConfigurationService.java | 18 +++ .../guacamole/vault/ksm/secret/KsmClient.java | 121 +++++++++------- .../vault/ksm/secret/KsmSecretService.java | 63 ++++++--- .../vault/ksm/secret/UserDomain.java | 129 ++++++++++++++++++ 5 files changed, 278 insertions(+), 70 deletions(-) create mode 100644 extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/UserDomain.java diff --git a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/conf/VaultConfigurationService.java b/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/conf/VaultConfigurationService.java index 490da6816..3a3189ba5 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/conf/VaultConfigurationService.java +++ b/extensions/guacamole-vault/modules/guacamole-vault-base/src/main/java/org/apache/guacamole/vault/conf/VaultConfigurationService.java @@ -206,4 +206,21 @@ public abstract class VaultConfigurationService { */ public abstract boolean getSplitWindowsUsernames() throws GuacamoleException; + /** + * Return whether domains should be considered when matching user records + * that are fetched from the vault. + * + * If set to true, the username and domain must both match when matching + * records from the vault. If false, only the username will be considered. + * + * @return + * true if both the username and domain should be considered when + * matching user records from the vault. + * + * @throws GuacamoleException + * If the value specified within guacamole.properties cannot be + * parsed. + */ + public abstract boolean getMatchUserRecordsByDomain() throws GuacamoleException; + } 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 3ef02b8b9..c9a9536ea 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 @@ -96,6 +96,19 @@ public class KsmConfigurationService extends VaultConfigurationService { } }; + /** + * Whether domains should be considered when matching login records in the KSM vault. + * If true, both the domain and username must match for a record to match when using + * tokens like "KEEPER_USER_*". If false, only the username must match. + */ + private static final BooleanGuacamoleProperty MATCH_USER_DOMAINS = new BooleanGuacamoleProperty() { + + @Override + public String getName() { + return "ksm-match-domains-for-users"; + } + }; + /** * Creates a new KsmConfigurationService which reads the configuration * from "ksm-token-mapping.yml" and properties from @@ -130,6 +143,11 @@ public class KsmConfigurationService extends VaultConfigurationService { return environment.getProperty(STRIP_WINDOWS_DOMAINS, false); } + @Override + public boolean getMatchUserRecordsByDomain() throws GuacamoleException { + return environment.getProperty(MATCH_USER_DOMAINS, false); + } + /** * Return the globally-defined base-64-encoded JSON KSM configuration blob 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 c854fbd3b..34f350234 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 @@ -46,6 +46,7 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import org.apache.guacamole.GuacamoleException; +import org.apache.guacamole.net.auth.User; import org.apache.guacamole.vault.ksm.conf.KsmConfigurationService; import org.apache.guacamole.vault.secret.WindowsUsername; import org.slf4j.Logger; @@ -166,27 +167,27 @@ public class KsmClient { /** * 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 + * username/domain of the corresponding record. The username of a record is + * determined by {@link Login} and "domain" 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 #cachedAmbiguousUsers} must first be checked to * verify that there is indeed only one record associated with that user. */ - private final Map cachedRecordsByUsername = new HashMap<>(); + private final Map cachedRecordsByUser = 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}. + * The set of all username/domain combos 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 #cachedRecordsByUser}. */ - private final Set cachedAmbiguousUsernames = new HashSet<>(); + private final Set cachedAmbiguousUsers = new HashSet<>(); /** * All records retrieved from Keeper Secrets Manager, where each key is the @@ -269,8 +270,8 @@ public class KsmClient { cachedRecordsByHost.clear(); // Clear cache of login-based records - cachedAmbiguousUsernames.clear(); - cachedRecordsByUsername.clear(); + cachedAmbiguousUsers.clear(); + cachedRecordsByUser.clear(); // Clear cache of domain-based records cachedAmbiguousDomains.clear(); @@ -295,32 +296,32 @@ public class KsmClient { String domain = recordService.getDomain(record); addRecordForDomain(record, domain); - // Fetch the username + // Get the username off of the record String username = recordService.getUsername(record); - // If domains should be split out from usernames - if (username != null && confService.getSplitWindowsUsernames()) { + // If we have a username, and there isn't already a domain explicitly defined + if (username != null && domain == null + && confService.getSplitWindowsUsernames()) { - // Attempt to split the domain of the username + // Attempt to split out the domain of the username WindowsUsername usernameAndDomain = ( WindowsUsername.splitWindowsUsernameFromDomain(username)); - if (usernameAndDomain.hasDomain()) { + // Use the username-split domain if not already set explicitly + if (usernameAndDomain.hasDomain()) + domain = usernameAndDomain.getDomain(); + addRecordForDomain(record, domain); - // Update the username if a domain has been stripped off - username = usernameAndDomain.getUsername(); - - // Use the username-split domain if not already set explicitly - if (domain == null) - addRecordForDomain(record, usernameAndDomain.getDomain()); - } } - // 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, username); + // If domain matching is not enabled for user records, + // explicitly set all domains to null to allow matching + // on username only + if (!confService.getMatchUserRecordsByDomain()) + domain = null; + + // Store the login by username and domain + addRecordForLogin(record, username, domain); } @@ -383,27 +384,34 @@ public class KsmClient { } /** - * 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 + * Associates the given record with the given user, and optional domain. + * The given username or domain may be null. Both {@link #cachedRecordsByUser} + * and {@link #cachedAmbiguousUsers} 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. + * The record to associate with the given user. * * @param username * The username that the given record should be associated with. This * may be null. + * + * @param domain + * The domain that the given record should be associated with. This + * may be null. */ - private void addRecordForLogin(KeeperRecord record, String username) { + private void addRecordForLogin( + KeeperRecord record, String username, String domain) { if (username == null) return; - KeeperRecord existing = cachedRecordsByUsername.putIfAbsent(username, record); + UserDomain userDomain = new UserDomain(username, domain); + KeeperRecord existing = cachedRecordsByUser.putIfAbsent( + userDomain, record); if (existing != null && record != existing) - cachedAmbiguousUsernames.add(username); + cachedAmbiguousUsers.add(userDomain); } @@ -489,32 +497,41 @@ public class KsmClient { } /** - * Returns the record associated with the given username. If no such record - * exists, or there are multiple such records, null is returned. + * Returns the record associated with the given username and domain. If no + * such record exists, or there are multiple such records, null is returned. * * @param username * The username of the record to return. * + * @param domain + * The domain 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. + * The record associated with the given username and domain, 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 { + public KeeperRecord getRecordByLogin( + String username, String domain) throws GuacamoleException { + validateCache(); cacheLock.readLock().lock(); + + UserDomain userDomain = new UserDomain(username, domain); + try { - if (cachedAmbiguousUsernames.contains(username)) { - logger.debug("The username \"{}\" is referenced by multiple " - + "Keeper records and cannot be used to locate " - + "individual secrets.", username); + if (cachedAmbiguousUsers.contains(userDomain)) { + logger.debug("The username \"{}\" with domain \"{}\" is " + + "referenced by multiple Keeper records and " + + "cannot be used to locate individual secrets.", + username, domain); return null; } - return cachedRecordsByUsername.get(username); + return cachedRecordsByUser.get(userDomain); } finally { 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 604ae2865..3b8a6dc0d 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 @@ -323,12 +323,6 @@ public class KsmSecretService implements VaultSecretService { addRecordTokens(tokens, "KEEPER_SERVER_", ksm.getRecordByHost(filter.filter(hostname))); - // Retrieve and define user-specific tokens, if any - String username = parameters.get("username"); - if (username != null && !username.isEmpty()) - addRecordTokens(tokens, "KEEPER_USER_", - ksm.getRecordByLogin(filter.filter(username))); - // Tokens specific to RDP if ("rdp".equals(config.getProtocol())) { @@ -338,24 +332,57 @@ public class KsmSecretService implements VaultSecretService { addRecordTokens(tokens, "KEEPER_GATEWAY_", ksm.getRecordByHost(filter.filter(gatewayHostname))); + // Retrieve and define domain tokens, if any + String domain = parameters.get("domain"); + String filteredDomain = null; + if (domain != null && !domain.isEmpty()) { + filteredDomain = filter.filter(domain); + addRecordTokens(tokens, "KEEPER_DOMAIN_", + ksm.getRecordByDomain(filteredDomain)); + } + + // Retrieve and define gateway domain tokens, if any + String gatewayDomain = parameters.get("gateway-domain"); + String filteredGatewayDomain = null; + if (gatewayDomain != null && !gatewayDomain.isEmpty()) { + filteredGatewayDomain = filter.filter(gatewayDomain); + addRecordTokens(tokens, "KEEPER_GATEWAY_DOMAIN_", + ksm.getRecordByDomain(filteredGatewayDomain)); + } + + // If domain matching is disabled for user records, + // explicitly set the domains to null when storing + // user records to enable username-only matching + if (!confService.getMatchUserRecordsByDomain()) { + filteredDomain = null; + filteredGatewayDomain = null; + } + + // Retrieve and define user-specific tokens, if any + String username = parameters.get("username"); + if (username != null && !username.isEmpty()) + addRecordTokens(tokens, "KEEPER_USER_", + ksm.getRecordByLogin(filter.filter(username), + filteredDomain)); + // Retrieve and define gateway user-specific tokens, if any String gatewayUsername = parameters.get("gateway-username"); if (gatewayUsername != null && !gatewayUsername.isEmpty()) addRecordTokens(tokens, "KEEPER_GATEWAY_USER_", - ksm.getRecordByLogin(filter.filter(gatewayUsername))); + ksm.getRecordByLogin( + filter.filter(gatewayUsername), + filteredGatewayDomain)); - // Retrieve and define domain tokens, if any - String domain = parameters.get("domain"); - if (domain != null && !domain.isEmpty()) - addRecordTokens(tokens, "KEEPER_DOMAIN_", - ksm.getRecordByDomain(filter.filter(domain))); - - // Retrieve and define gateway domain tokens, if any - String gatewayDomain = parameters.get("gateway-domain"); - if (gatewayDomain != null && !gatewayDomain.isEmpty()) - addRecordTokens(tokens, "KEEPER_GATEWAY_DOMAIN_", - ksm.getRecordByDomain(filter.filter(gatewayDomain))); + } else { + // Retrieve and define user-specific tokens, if any + // NOTE that non-RDP connections do not have a domain + // field in the connection parameters, so the domain + // will always be null + String username = parameters.get("username"); + if (username != null && !username.isEmpty()) + addRecordTokens(tokens, "KEEPER_USER_", + ksm.getRecordByLogin(filter.filter(username), null)); } return tokens; diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/UserDomain.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/UserDomain.java new file mode 100644 index 000000000..bb2c2309d --- /dev/null +++ b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/UserDomain.java @@ -0,0 +1,129 @@ +/* + * 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 javax.annotation.Nullable; + +/** + * A class intended for use as a key in KSM user record client cache. This + * class contains both a username and a password. When identifying a KSM + * record using token syntax like "KEEPER_USER_*", the user record will + * actually be identified by both the user and domain, if the appropriate + * settings are enabled. + */ +class UserDomain { + + /** + * The username associated with the user record. + * This field should never be null. + */ + private final String username; + + /** + * The domain associated with the user record. + * This field can be null. + */ + private final String domain; + + /** + * Create a new UserDomain instance with the provided username and + * domain. The domain may be null, but the username should never be. + * + * @param username + * The username to create the UserDomain instance with. This should + * never be null. + * + * @param domain + * The domain to create the UserDomain instance with. This can be null. + */ + UserDomain(@Nonnull String username, @Nullable String domain) { + this.username = username; + this.domain = domain; + } + + @Override + public int hashCode() { + + final int prime = 31; + + int result = 1; + result = prime * result + ((domain == null) ? 0 : domain.hashCode()); + result = prime * result + ((username == null) ? 0 : username.hashCode()); + + return result; + + } + + @Override + public boolean equals(Object obj) { + + // Check if the other object is this exact object + if (this == obj) + return true; + + // Check if the other object is null + if (obj == null) + return false; + + // Check if the other object is also a UserDomain + if (getClass() != obj.getClass()) + return false; + + // If it is a UserDomain, it must have the same username... + UserDomain other = (UserDomain) obj; + if (username == null) { + if (other.username != null) + return false; + } else if (!username.equals(other.username)) + return false; + + // .. and the same domain + if (domain == null) { + if (other.domain != null) + return false; + } else if (!domain.equals(other.domain)) + return false; + + return true; + } + + /** + * Get the username associated with this UserDomain. + * + * @return + * The username associated with this UserDomain. + */ + public String getUsername() { + return username; + } + + + /** + * Get the domain associated with this UserDomain. + * + * @return + * The domain associated with this UserDomain. + */ + public String getDomain() { + return domain; + } + +} \ No newline at end of file From e0a9364dde890bff48481ad9e95affa38a77abb4 Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Wed, 17 Aug 2022 18:05:41 +0000 Subject: [PATCH 3/5] GUACAMOLE-1661: Simplify and clarify KSM domain search code. --- .../guacamole/vault/ksm/secret/KsmClient.java | 16 +++--- .../vault/ksm/secret/KsmSecretService.java | 4 +- .../{UserDomain.java => UserLogin.java} | 50 +++++++------------ 3 files changed, 30 insertions(+), 40 deletions(-) rename extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/{UserDomain.java => UserLogin.java} (63%) 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 34f350234..a573259b5 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 @@ -177,7 +177,7 @@ public class KsmClient { * this Map, {@link #cachedAmbiguousUsers} must first be checked to * verify that there is indeed only one record associated with that user. */ - private final Map cachedRecordsByUser = new HashMap<>(); + private final Map cachedRecordsByUser = new HashMap<>(); /** * The set of all username/domain combos that are associated with multiple @@ -187,7 +187,7 @@ public class KsmClient { * acquired appropriately. This Set must be checked before using a value * retrieved from {@link #cachedRecordsByUser}. */ - private final Set cachedAmbiguousUsers = new HashSet<>(); + private final Set cachedAmbiguousUsers = new HashSet<>(); /** * All records retrieved from Keeper Secrets Manager, where each key is the @@ -307,10 +307,12 @@ public class KsmClient { WindowsUsername usernameAndDomain = ( WindowsUsername.splitWindowsUsernameFromDomain(username)); - // Use the username-split domain if not already set explicitly - if (usernameAndDomain.hasDomain()) + // Use the username-split domain if available + if (usernameAndDomain.hasDomain()) { domain = usernameAndDomain.getDomain(); + username = usernameAndDomain.getUsername(); addRecordForDomain(record, domain); + } } @@ -407,7 +409,7 @@ public class KsmClient { if (username == null) return; - UserDomain userDomain = new UserDomain(username, domain); + UserLogin userDomain = new UserLogin(username, domain); KeeperRecord existing = cachedRecordsByUser.putIfAbsent( userDomain, record); if (existing != null && record != existing) @@ -504,7 +506,7 @@ public class KsmClient { * The username of the record to return. * * @param domain - * The domain of the record to return. + * The domain of the record to return, or null if no domain exists. * * @return * The record associated with the given username and domain, or null @@ -519,7 +521,7 @@ public class KsmClient { validateCache(); cacheLock.readLock().lock(); - UserDomain userDomain = new UserDomain(username, domain); + UserLogin userDomain = new UserLogin(username, domain); try { 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 3b8a6dc0d..cb5868cfa 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 @@ -373,7 +373,9 @@ public class KsmSecretService implements VaultSecretService { filter.filter(gatewayUsername), filteredGatewayDomain)); - } else { + } + + else { // Retrieve and define user-specific tokens, if any // NOTE that non-RDP connections do not have a domain diff --git a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/UserDomain.java b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/UserLogin.java similarity index 63% rename from extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/UserDomain.java rename to extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/UserLogin.java index bb2c2309d..5cd50ee6c 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/UserDomain.java +++ b/extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/UserLogin.java @@ -19,6 +19,8 @@ package org.apache.guacamole.vault.ksm.secret; +import java.util.Objects; + import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -29,7 +31,7 @@ import javax.annotation.Nullable; * actually be identified by both the user and domain, if the appropriate * settings are enabled. */ -class UserDomain { +class UserLogin { /** * The username associated with the user record. @@ -44,17 +46,17 @@ class UserDomain { private final String domain; /** - * Create a new UserDomain instance with the provided username and + * Create a new UserLogin instance with the provided username and * domain. The domain may be null, but the username should never be. * * @param username - * The username to create the UserDomain instance with. This should + * The username to create the UserLogin instance with. This should * never be null. * * @param domain - * The domain to create the UserDomain instance with. This can be null. + * The domain to create the UserLogin instance with. This can be null. */ - UserDomain(@Nonnull String username, @Nullable String domain) { + UserLogin(@Nonnull String username, @Nullable String domain) { this.username = username; this.domain = domain; } @@ -62,13 +64,7 @@ class UserDomain { @Override public int hashCode() { - final int prime = 31; - - int result = 1; - result = prime * result + ((domain == null) ? 0 : domain.hashCode()); - result = prime * result + ((username == null) ? 0 : username.hashCode()); - - return result; + return Objects.hash(domain, username); } @@ -83,33 +79,23 @@ class UserDomain { if (obj == null) return false; - // Check if the other object is also a UserDomain + // Check if the other object is also a UserLogin if (getClass() != obj.getClass()) return false; - // If it is a UserDomain, it must have the same username... - UserDomain other = (UserDomain) obj; - if (username == null) { - if (other.username != null) - return false; - } else if (!username.equals(other.username)) - return false; + // If the other object is also a UserLogin, it must + // have the same username and domain + UserLogin other = (UserLogin) obj; + return Objects.equals(username, other.username) + && Objects.equals(domain, other.domain); - // .. and the same domain - if (domain == null) { - if (other.domain != null) - return false; - } else if (!domain.equals(other.domain)) - return false; - - return true; } /** - * Get the username associated with this UserDomain. + * Get the username associated with this UserLogin. * * @return - * The username associated with this UserDomain. + * The username associated with this UserLogin. */ public String getUsername() { return username; @@ -117,10 +103,10 @@ class UserDomain { /** - * Get the domain associated with this UserDomain. + * Get the domain associated with this UserLogin. * * @return - * The domain associated with this UserDomain. + * The domain associated with this UserLogin. */ public String getDomain() { return domain; From 2b997a9992cb246ae9e6aecbb52e9daed2680d91 Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Wed, 24 Aug 2022 19:03:16 +0000 Subject: [PATCH 4/5] GUACAMOLE-1661: Restore logic to not index records by login if hostname is already defined. --- .../org/apache/guacamole/vault/ksm/secret/KsmClient.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) 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 a573259b5..fd339dac5 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 @@ -322,8 +322,11 @@ public class KsmClient { if (!confService.getMatchUserRecordsByDomain()) domain = null; - // Store the login by username and domain - addRecordForLogin(record, username, domain); + // Store based on login ONLY if no hostname (will otherwise + // result in ambiguous entries for servers tied to identical + // accounts) + if (hostname == null) + addRecordForLogin(record, username, domain); } From c7bb1cb50c6dae7efd8f85f39ef5320d787a6696 Mon Sep 17 00:00:00 2001 From: James Muehlner Date: Thu, 25 Aug 2022 00:03:18 +0000 Subject: [PATCH 5/5] GUACAMOLE-1661: Parse config only once when iterating records. --- .../guacamole/vault/ksm/secret/KsmClient.java | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) 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 fd339dac5..6e0bdf177 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 @@ -277,13 +277,13 @@ public class KsmClient { cachedAmbiguousDomains.clear(); cachedRecordsByDomain.clear(); + // Parse configuration + final boolean shouldSplitUsernames = confService.getSplitWindowsUsernames(); + final boolean shouldMatchByDomain = confService.getMatchUserRecordsByDomain(); + // Store all records, sorting each into host-based, login-based, // and domain-based buckets - Iterator recordIterator = records.iterator(); - while(recordIterator.hasNext()) { - - // Go through records one at a time - KeeperRecord record = recordIterator.next(); + records.forEach(record -> { // Store based on UID ... cachedRecordsByUid.put(record.getRecordUid(), record); @@ -300,8 +300,7 @@ public class KsmClient { String username = recordService.getUsername(record); // If we have a username, and there isn't already a domain explicitly defined - if (username != null && domain == null - && confService.getSplitWindowsUsernames()) { + if (username != null && domain == null && shouldSplitUsernames) { // Attempt to split out the domain of the username WindowsUsername usernameAndDomain = ( @@ -319,7 +318,7 @@ public class KsmClient { // If domain matching is not enabled for user records, // explicitly set all domains to null to allow matching // on username only - if (!confService.getMatchUserRecordsByDomain()) + if (!shouldMatchByDomain) domain = null; // Store based on login ONLY if no hostname (will otherwise @@ -328,7 +327,7 @@ public class KsmClient { if (hostname == null) addRecordForLogin(record, username, domain); - } + }); // Cache has been refreshed this.cacheTimestamp = System.currentTimeMillis();