mike-jumper commented on code in PR #751:
URL: https://github.com/apache/guacamole-client/pull/751#discussion_r979105408
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java:
##########
@@ -144,7 +149,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.equals(ksmConfig))
Review Comment:
This will throw an NPE if `getUserKSMConfig()` returned `null`.
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/user/KsmConnection.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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.user;
+
+import java.util.Map;
+
+import org.apache.guacamole.net.auth.DelegatingConnection;
+import org.apache.guacamole.vault.ksm.conf.KsmAttributeService;
+import org.apache.guacamole.net.auth.Connection;
+
+import com.google.common.collect.Maps;
+
+/**
+ * A Connection that explicitly adds a blank entry for any defined KSM
+ * connection attributes. This ensures that any such field will always
+ * be displayed to the user when editing a connection through the UI.
+ */
+public class KsmConnection extends DelegatingConnection {
+
+ /**
+ * Create a new Vault connection wrapping the provided Connection record.
Any
+ * attributes defined in the provided connection attribute forms will have
empty
+ * values automatically populated when getAttributes() is called.
+ *
+ * @param connection
+ * The connection record to wrap.
+ */
+ KsmConnection(Connection connection) {
+ super(connection);
+ }
+
+ /**
+ * Return the underlying wrapped connection record.
+ *
+ * @return
+ * The wrapped connection record.
+ */
+ Connection getUnderlyingConnection() {
+ return getDelegateConnection();
+ }
+
+ @Override
+ public Map<String, String> getAttributes() {
+
+ // Make a copy of the existing map
+ Map<String, String> attributes =
Maps.newHashMap(super.getAttributes());
+
+ // Add the configuration attribute
+
attributes.putIfAbsent(KsmAttributeService.KSM_CONFIGURATION_ATTRIBUTE, null);
+ return attributes;
Review Comment:
Unlike connection parameters, object attributes are not automatically
filtered from visibility to the user. They won't be displayed in the UI, but
they'll be present in the bodies of relevant REST API requests.
Given the nature of these attributes, we should hide them unless the user
has appropriate permissions.
If the same issue is present with connection groups from that previous
change, we should hide them there, as well.
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java:
##########
@@ -304,18 +326,97 @@ 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();
+ // User-level config is enabled IFF the appropriate attribute is set
to true
+ if (connectable instanceof Attributes)
+ return KsmAttributeService.TRUTH_VALUE.equals(((Attributes)
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);
+ // If there's no attributes to check, the use config cannot be enabled
Review Comment:
Do you mean user-level config?
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/conf/KsmAttributeService.java:
##########
@@ -36,28 +41,94 @@
@Singleton
public class KsmAttributeService implements VaultAttributeService {
+ /**
+ * Logger for this class.
+ */
+ private static final Logger logger =
LoggerFactory.getLogger(KsmAttributeService.class);
+
+ /**
+ * 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, connections, 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) {
+
+ logger.warn(
+ "Unable to determine if user preference attributes "
+ + "should be exposed due to config parsing error.", e);
Review Comment:
We should only log the full stack trace at the debug level. For example:
https://github.com/apache/guacamole-client/blob/754e9649f1fa0ba225ee42b56ded64bc283d17df/extensions/guacamole-auth-json/src/main/java/org/apache/guacamole/auth/json/RequestValidationService.java#L69-L70
The idea is that only the details needed by an administrator perusing the
logs should be presented at levels above debug, while noisier details needed
for development (like stack traces) should be kept at the debug level.
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/secret/KsmSecretService.java:
##########
@@ -341,6 +436,12 @@ public Map<String, Future<String>> getTokens(UserContext
userContext, Connectabl
ksm.getRecordByDomain(filteredDomain));
}
+ // 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 did this get moved?
##########
extensions/guacamole-vault/modules/guacamole-vault-ksm/src/main/java/org/apache/guacamole/vault/ksm/user/KsmUser.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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.user;
+
+import java.util.Map;
+
+import org.apache.guacamole.net.auth.DelegatingUser;
+import org.apache.guacamole.net.auth.User;
+import org.apache.guacamole.vault.ksm.conf.KsmAttributeService;
+
+import com.google.common.collect.Maps;
+
+/**
+ * A User that explicitly adds a blank entry for any defined
+ * KSM user attributes.
+ */
+public class KsmUser extends DelegatingUser {
+
+ /**
+ * Create a new Vault user wrapping the provided User record. Any
+ * KSM-specific attribute forms will have empty values automatically
+ * populated when getAttributes() is called.
+ *
+ * @param user
+ * The user record to wrap.
+ */
+ KsmUser(User user) {
+ super(user);
+ }
+
+ /**
+ * Return the underlying wrapped user record.
+ *
+ * @return
+ * The wrapped user record.
+ */
+ User getUnderlyingUser() {
+ return getDelegateUser();
+ }
+
+ @Override
+ public Map<String, String> getAttributes() {
+
+ // Make a copy of the existing map
+ Map<String, String> attributes =
Maps.newHashMap(super.getAttributes());
+
+ // Add the configuration attribute
+
attributes.putIfAbsent(KsmAttributeService.KSM_CONFIGURATION_ATTRIBUTE, null);
+ return attributes;
Review Comment:
Here, as with connections and connection groups, we should not expose this
attribute unless the user has appropriate permissions. I suggest hiding it from
every user (even administrators) except the very user it applies to.
Another approach that might satisfy the requirements of all these objects
could be to _never_ expose the value of the attribute. If it were strictly
write-only, with only some sort of indicator for whether a value is set and a
mechanism to clear it, then there would be no concern.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
