You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2022/09/21 17:27:27 UTC
[GitHub] [guacamole-client] mike-jumper commented on a diff in pull request #751: GUACAMOLE-1656: Add per-user KSM vault functionality.
mike-jumper commented on code in PR #751:
URL: https://github.com/apache/guacamole-client/pull/751#discussion_r976754422
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmAttributeService.java:
##########
@@ -36,28 +39,85 @@
@Singleton
public class KsmAttributeService implements VaultAttributeService {
+ /**
+ * Service for retrieving KSM configuration details.
+ */
+ @Inject
+ private KsmConfigurationService configurationService;
+
/**
* The name of the attribute which can contain a KSM configuration blob
- * associated with a connection group.
+ * associated with either a connection group or user.
*/
public static final String KSM_CONFIGURATION_ATTRIBUTE = "ksm-config";
/**
* All attributes related to configuring the KSM vault on a
- * per-connection-group basis.
+ * per-connection-group or per-user basis.
*/
public static final Form KSM_CONFIGURATION_FORM = new Form("ksm-config",
Arrays.asList(new TextField(KSM_CONFIGURATION_ATTRIBUTE)));
/**
- * All KSM-specific connection group attributes, organized by form.
+ * All KSM-specific attributes for users or connection groups, organized by form.
*/
- public static final Collection<Form> KSM_CONNECTION_GROUP_ATTRIBUTES =
+ public static final Collection<Form> KSM_ATTRIBUTES =
Collections.unmodifiableCollection(Arrays.asList(KSM_CONFIGURATION_FORM));
+ /**
+ * The name of the attribute which can controls whether a KSM user configuration
+ * is enabled on a connection-by-connection basis.
+ */
+ public static final String KSM_USER_CONFIG_ENABLED_ATTRIBUTE = "ksm-user-config-enabled";
+
+ /**
+ * The string value used by KSM attributes to represent the boolean value "true".
+ */
+ public static final String TRUTH_VALUE = "true";
+
+ /**
+ * All attributes related to configuring the KSM vault on a per-connection basis.
+ */
+ public static final Form KSM_CONNECTION_FORM = new Form("ksm-config",
+ Arrays.asList(new BooleanField(KSM_USER_CONFIG_ENABLED_ATTRIBUTE, TRUTH_VALUE)));
+
+ /**
+ * All KSM-specific attributes for connections, organized by form.
+ */
+ public static final Collection<Form> KSM_CONNECTION_ATTRIBUTES =
+ Collections.unmodifiableCollection(Arrays.asList(KSM_CONNECTION_FORM));
+
+ @Override
+ public Collection<Form> getConnectionAttributes() {
+ return KSM_CONNECTION_ATTRIBUTES;
+ }
+
@Override
public Collection<Form> getConnectionGroupAttributes() {
- return KSM_CONNECTION_GROUP_ATTRIBUTES;
+ return KSM_ATTRIBUTES;
+ }
+
+ @Override
+ public Collection<Form> getUserAttributes() {
+ return KSM_ATTRIBUTES;
}
+ @Override
+ public Collection<Form> getUserPreferenceAttributes() {
+
+ try {
+
+ // Expose the user attributes IFF user-level KSM configuration is enabled
+ return configurationService.getAllowUserConfig() ? KSM_ATTRIBUTES : Collections.emptyList();
+
+ }
+
+ catch (GuacamoleException e) {
+
+ // If the configuration can't be parsed, default to not exposing the attribute
+ return Collections.emptyList();
+ }
Review Comment:
The admin-readable details exception and the fact that it is preventing per-user vaults from being configured should be logged.
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java:
##########
@@ -144,7 +148,24 @@ public Future<String> getValue(UserContext userContext, Connectable connectable,
// Attempt to find a KSM config for this connection or group
String ksmConfig = getConnectionGroupKsmConfig(userContext, connectable);
- return getClient(ksmConfig).getSecret(name);
+ return getClient(ksmConfig).getSecret(name, new GuacamoleExceptionSupplier<Future<String>>() {
+
+ @Override
+ public Future<String> get() throws GuacamoleException {
+
+ // Get the user-supplied KSM config, if allowed by config and
+ // set by the user
+ String userKsmConfig = getUserKSMConfig(userContext, connectable);
+
+ // If the user config happens to be the same as admin-defined one,
+ // don't bother trying again
+ if (userKsmConfig != ksmConfig)
Review Comment:
Is this memory address comparison intentional?
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java:
##########
@@ -386,6 +482,38 @@ public Map<String, Future<String>> getTokens(UserContext userContext, Connectabl
addRecordTokens(tokens, "KEEPER_USER_",
ksm.getRecordByLogin(filter.filter(username), null));
}
+ }
+
+ @Override
+ public Map<String, Future<String>> getTokens(UserContext userContext, Connectable connectable,
+ GuacamoleConfiguration config, TokenFilter filter) throws GuacamoleException {
+
+ Map<String, Future<String>> tokens = new HashMap<>();
+ Map<String, String> parameters = config.getParameters();
+
+ // Attempt to find a KSM config for this connection or group
+ String ksmConfig = getConnectionGroupKsmConfig(userContext, connectable);
+
+ // Create a list containing just the global / connection group config
+ List<KsmClient> ksmClients = new ArrayList<>(2);
+ ksmClients.add(getClient(ksmConfig));
+
+ // Only use the user-specific KSM config if explicitly enabled in the global
+ // configuration, AND for the specific connectable being connected to
+ String userKsmConfig = getUserKSMConfig(userContext, connectable);
+ if (userKsmConfig != null && !userKsmConfig.trim().isEmpty())
+ ksmClients.add(0, getClient(userKsmConfig));
+
+ // Iterate through the KSM clients, processing using the user-specific
+ // config first (if it exists), to ensure that any admin-defined values
+ // will override the user-specific values
+ Iterator<KsmClient> ksmIterator = ksmClients.iterator();
+ while (ksmIterator.hasNext()) {
+
+ // Add tokens to the Connectable for each KSM client
+ addConnectableTokens(
+ config, ksmIterator.next(), tokens, parameters, filter);
+ }
Review Comment:
Why build up a temporary list of `KsmClient` vs. call `addConnectableTokens()` twice?
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/user/KsmDirectoryService.java:
##########
@@ -222,6 +231,37 @@ public void processAttributes(Attributes entity) throws GuacamoleException {
}
+ @Override
+ public Directory<Connection> getConnectionDirectory(
+ Directory<Connection> underlyingDirectory) throws GuacamoleException {
+
+ // A Connection directory that will decorate all connections with a
+ // KsmConnection wrapper to ensure that all defined KSM fields will
+ // be exposed in the connection group attributes.
+ return new DecoratingDirectory<Connection>(underlyingDirectory) {
+
+ @Override
+ protected Connection decorate(Connection connection) throws GuacamoleException {
+
+ // Wrap in a KsmConnection class to ensure that all defined KSM fields will be
+ // present
+ return new KsmConnection(
+ connection,
+ ksmAttributeService.getConnectionAttributes().stream().flatMap(
+ form -> form.getFields().stream().map(field -> field.getName())
+ ).collect(Collectors.toList()));
Review Comment:
Feels somewhat wonky to pull this dynamically when we have `KSM_CONNECTION_ATTRIBUTES` available statically and the stated purpose of `KsmConnection` is to ensure those attributes are defined. Why not have `KsmConnection` leverage `KSM_CONNECTION_ATTRIBUTES` internally such that it need not be fed each attribute name manually?
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java:
##########
@@ -304,18 +325,101 @@ private String getConnectionGroupKsmConfig(
}
- @Override
- public Map<String, Future<String>> getTokens(UserContext userContext, Connectable connectable,
- GuacamoleConfiguration config, TokenFilter filter) throws GuacamoleException {
+ /**
+ * Returns true if user-level KSM configuration is enabled for the given
+ * Connectable, false otherwise.
+ *
+ * @param connectable
+ * The connectable to check for whether user-level KSM configs are
+ * enabled.
+ *
+ * @return
+ * True if user-level KSM configuration is enabled for the given
+ * Connectable, false otherwise.
+ */
+ private boolean isKsmUserConfigEnabled(Connectable connectable) {
- Map<String, Future<String>> tokens = new HashMap<>();
- Map<String, String> parameters = config.getParameters();
+ // If it's a connection, user-level config is enabled IFF the appropriate
+ // attribute is set to true
+ if (connectable instanceof Connection)
+ return KsmAttributeService.TRUTH_VALUE.equals(((Connection) connectable).getAttributes().get(
+ KsmAttributeService.KSM_USER_CONFIG_ENABLED_ATTRIBUTE));
- // Attempt to find a KSM config for this connection or group
- String ksmConfig = getConnectionGroupKsmConfig(userContext, connectable);
+ // KSM token replacement is not enabled for balancing groups, so for
+ // now, user-level KSM configs will be explicitly disabled.
+ // TODO: If token replacement for connection parameters is implemented
+ // for balancing groups, implement this functionality for them as well.
+ return false;
Review Comment:
Token replacement for balancing groups is available, just not based on the contents of underlying connection parameters. Tokens defined via the YAML will work.
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java:
##########
@@ -325,13 +429,6 @@ public Map<String, Future<String>> getTokens(UserContext userContext, Connectabl
// Tokens specific to RDP
if ("rdp".equals(config.getProtocol())) {
-
- // Retrieve and define gateway server-specific tokens, if any
- String gatewayHostname = parameters.get("gateway-hostname");
- if (gatewayHostname != null && !gatewayHostname.isEmpty())
- addRecordTokens(tokens, "KEEPER_GATEWAY_",
- ksm.getRecordByHost(filter.filter(gatewayHostname)));
Review Comment:
Why is this being removed?
##########
guacamole/src/main/java/org/apache/guacamole/rest/user/UserObjectTranslator.java:
##########
@@ -59,9 +59,20 @@ public void applyExternalChanges(User existingObject,
public void filterExternalObject(UserContext userContext, APIUser object)
throws GuacamoleException {
- // Filter object attributes by defined schema
- object.setAttributes(filterAttributes(userContext.getUserAttributes(),
- object.getAttributes()));
+ // If a user is editing themselves ...
+ if (object.getUsername().equals(userContext.self().getIdentifier())) {
+
+ // ... they may only edit preference attributes
+ object.setAttributes(filterAttributes(userContext.getUserPreferenceAttributes(),
+ object.getAttributes()));
+
+ } else {
Review Comment:
Please don't cuddle the `else`: https://guacamole.apache.org/guac-style/#braces
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/user/KsmDirectoryService.java:
##########
@@ -269,4 +309,55 @@ protected ConnectionGroup undecorate(ConnectionGroup connectionGroup) throws Gua
};
}
+
+ @Override
+ public Directory<User> getUserDirectory(
+ Directory<User> underlyingDirectory) throws GuacamoleException {
+
+ // A User directory that will intercept add and update calls to
+ // validate KSM configurations, and translate one-time-tokens, if possible
+ // Additionally, this directory will will decorate all users with a
+ // KsmUser wrapper to ensure that all defined KSM fields will be exposed
+ // in the user attributes.
+ return new DecoratingDirectory<User>(underlyingDirectory) {
+
+ @Override
+ public void add(User user) throws GuacamoleException {
+
+ // Check for the KSM config attribute and translate the one-time token
+ // if possible before adding
+ processAttributes(user);
+ super.add(user);
+ }
+
+ @Override
+ public void update(User user) throws GuacamoleException {
+
+ // Check for the KSM config attribute and translate the one-time token
+ // if possible before updating
+ processAttributes(user);
+ super.update(user);
+ }
+
+ @Override
+ protected User decorate(User user) throws GuacamoleException {
+
+ // Wrap in a KsmUser class to ensure that all defined KSM fields will be
+ // present
+ return new KsmUser(
+ user,
+ ksmAttributeService.getUserAttributes().stream().flatMap(
+ form -> form.getFields().stream().map(field -> field.getName())
+ ).collect(Collectors.toList()));
Review Comment:
Same here - would it make more sense to let `KsmUser` do this itself internally with `KSM_ATTRIBUTES`?
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java:
##########
@@ -304,18 +325,101 @@ private String getConnectionGroupKsmConfig(
}
- @Override
- public Map<String, Future<String>> getTokens(UserContext userContext, Connectable connectable,
- GuacamoleConfiguration config, TokenFilter filter) throws GuacamoleException {
+ /**
+ * Returns true if user-level KSM configuration is enabled for the given
+ * Connectable, false otherwise.
+ *
+ * @param connectable
+ * The connectable to check for whether user-level KSM configs are
+ * enabled.
+ *
+ * @return
+ * True if user-level KSM configuration is enabled for the given
+ * Connectable, false otherwise.
+ */
+ private boolean isKsmUserConfigEnabled(Connectable connectable) {
- Map<String, Future<String>> tokens = new HashMap<>();
- Map<String, String> parameters = config.getParameters();
+ // If it's a connection, user-level config is enabled IFF the appropriate
+ // attribute is set to true
+ if (connectable instanceof Connection)
+ return KsmAttributeService.TRUTH_VALUE.equals(((Connection) connectable).getAttributes().get(
Review Comment:
The `Attributes` interface might be a better choice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org