From 786430612eb45b8812ae5727fdc7a07889feae92 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Fri, 21 Jan 2022 16:34:23 -0800 Subject: [PATCH] GUACAMOLE-641: Canonicalize individual, tokenized components of secret names rather than the whole name. In the event that a secret name is structured, such as the URL-like notation used by Keeper Secrets Manager, canonicalizing/encoding the entire name could result in the name itself becoming invalid. Only the portions that come from tokens should be canonicalized. --- .../secret/AzureKeyVaultSecretService.java | 4 ++-- .../vault/secret/VaultSecretService.java | 18 +++++++-------- .../vault/user/VaultUserContext.java | 23 ++++++++++++++++--- 3 files changed, 31 insertions(+), 14 deletions(-) diff --git a/extensions/guacamole-vault/modules/guacamole-vault-azure/src/main/java/org/apache/guacamole/vault/azure/secret/AzureKeyVaultSecretService.java b/extensions/guacamole-vault/modules/guacamole-vault-azure/src/main/java/org/apache/guacamole/vault/azure/secret/AzureKeyVaultSecretService.java index cd5ed3537..cf9faa4c3 100644 --- a/extensions/guacamole-vault/modules/guacamole-vault-azure/src/main/java/org/apache/guacamole/vault/azure/secret/AzureKeyVaultSecretService.java +++ b/extensions/guacamole-vault/modules/guacamole-vault-azure/src/main/java/org/apache/guacamole/vault/azure/secret/AzureKeyVaultSecretService.java @@ -66,8 +66,8 @@ public class AzureKeyVaultSecretService extends CachedVaultSecretService { * not allowed by Azure Key Vault, replacing them with a single dash. */ @Override - public String canonicalize(String name) { - Matcher disallowed = DISALLOWED_CHARACTERS.matcher(name); + public String canonicalize(String nameComponent) { + Matcher disallowed = DISALLOWED_CHARACTERS.matcher(nameComponent); return disallowed.replaceAll("-"); } 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 04f3783df..1ff230ca5 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 @@ -29,25 +29,25 @@ public interface VaultSecretService { /** * Translates an arbitrary string, which may contain characters not allowed - * by the vault implementation, into a string which is a valid secret name. - * The type of transformation performed on the string, if any, will depend - * on the specific requirements of the vault provider. + * by the vault implementation, into a string which is valid within a + * secret name. The type of transformation performed on the string, if any, + * will depend on the specific requirements of the vault provider. * * NOTE: It is critical that this transformation is deterministic and * reasonably predictable for users. If an implementation must apply a * transformation to secret names, that transformation needs to be * documented. * - * @param name - * An arbitrary string intended for use as a secret name, but which may - * contain characters not allowed by the vault implementation. + * @param nameComponent + * An arbitrary string intended for use within a secret name, but which + * may contain characters not allowed by the vault implementation. * * @return - * A name containing essentially the same content as the provided + * A string containing essentially the same content as the provided * string, but transformed deterministically such that it is acceptable - * as a secret name by the vault provider. + * as a component of a secret name by the vault provider. */ - String canonicalize(String name); + String canonicalize(String nameComponent); /** * Returns a Future which eventually completes with the value of the secret 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 a9541aca2..1367ac438 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 @@ -147,14 +147,31 @@ public class VaultUserContext extends TokenInjectingUserContext { /** * Creates a new TokenFilter instance with token values set for all tokens * which are not specific to connections or connection groups. Currently, - * this is only the vault-specific username token ("USERNAME"). + * this is only the vault-specific username token ("USERNAME"). Each token + * stored within the returned TokenFilter via setToken() will be + * automatically canonicalized for use within secret names. * * @return * A new TokenFilter instance with token values set for all tokens * which are not specific to connections or connection groups. */ private TokenFilter createFilter() { - TokenFilter filter = new TokenFilter(); + + // Create filter that automatically canonicalizes all token values + TokenFilter filter = new TokenFilter() { + + @Override + public void setToken(String name, String value) { + super.setToken(name, secretService.canonicalize(value)); + } + + @Override + public void setTokens(Map tokens) { + tokens.entrySet().forEach((entry) -> setToken(entry.getKey(), entry.getValue())); + } + + }; + filter.setToken(USERNAME_TOKEN, self().getIdentifier()); return filter; } @@ -196,7 +213,7 @@ public class VaultUserContext extends TokenInjectingUserContext { // secrets which cannot be translated String secretName; try { - secretName = secretService.canonicalize(filter.filterStrict(entry.getValue())); + secretName = filter.filterStrict(entry.getValue()); } catch (GuacamoleTokenUndefinedException e) { logger.debug("Secret for token \"{}\" will not be retrieved. "