From 3dbb821baf9c29078ccf49290c4d15e2262da6fa Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Fri, 21 Jan 2022 15:23:40 -0800 Subject: [PATCH] GUACAMOLE-641: Retrieve tokens asynchronously and in parallel. --- .../secret/AzureKeyVaultSecretService.java | 48 +++++++++--- .../secret/CachedVaultSecretService.java | 24 ++++-- .../auth/vault/secret/VaultSecretService.java | 15 ++-- .../auth/vault/user/VaultUserContext.java | 76 +++++++++++++++---- 4 files changed, 122 insertions(+), 41 deletions(-) diff --git a/extensions/guacamole-auth-vault/modules/guacamole-auth-vault-azure/src/main/java/org/apache/guacamole/auth/vault/azure/secret/AzureKeyVaultSecretService.java b/extensions/guacamole-auth-vault/modules/guacamole-auth-vault-azure/src/main/java/org/apache/guacamole/auth/vault/azure/secret/AzureKeyVaultSecretService.java index 8498f81ea..65d75dc9b 100644 --- a/extensions/guacamole-auth-vault/modules/guacamole-auth-vault-azure/src/main/java/org/apache/guacamole/auth/vault/azure/secret/AzureKeyVaultSecretService.java +++ b/extensions/guacamole-auth-vault/modules/guacamole-auth-vault-azure/src/main/java/org/apache/guacamole/auth/vault/azure/secret/AzureKeyVaultSecretService.java @@ -25,10 +25,11 @@ import com.google.inject.Singleton; import com.microsoft.azure.keyvault.KeyVaultClient; import com.microsoft.azure.keyvault.authentication.KeyVaultCredentials; import com.microsoft.azure.keyvault.models.SecretBundle; +import com.microsoft.rest.ServiceCallback; +import java.util.concurrent.CompletableFuture; import java.util.regex.Matcher; import java.util.regex.Pattern; import org.apache.guacamole.GuacamoleException; -import org.apache.guacamole.GuacamoleServerException; import org.apache.guacamole.auth.vault.azure.conf.AzureKeyVaultAuthenticationException; import org.apache.guacamole.auth.vault.azure.conf.AzureKeyVaultConfigurationService; import org.apache.guacamole.auth.vault.secret.CachedVaultSecretService; @@ -77,20 +78,43 @@ public class AzureKeyVaultSecretService extends CachedVaultSecretService { int ttl = confService.getSecretTTL(); String url = confService.getVaultURL(); - try { + CompletableFuture retrievedValue = new CompletableFuture<>(); - // Retrieve requested secret from Azure Key Vault - KeyVaultClient client = new KeyVaultClient(credentialProvider.get()); - SecretBundle secret = client.getSecret(url, name); + // getSecretAsync() still blocks for around half a second, despite + // technically being asynchronous + (new Thread() { - // Cache retrieved value - String value = (secret != null) ? secret.value() : null; - return new CachedSecret(value, ttl); + @Override + public void run() { + try { - } - catch (AzureKeyVaultAuthenticationException e) { - throw new GuacamoleServerException("Unable to authenticate with Azure.", e); - } + // Retrieve requested secret from Azure Key Vault + KeyVaultClient client = new KeyVaultClient(credentialProvider.get()); + client.getSecretAsync(url, name, new ServiceCallback() { + + @Override + public void failure(Throwable t) { + retrievedValue.completeExceptionally(t); + } + + @Override + public void success(SecretBundle secret) { + String value = (secret != null) ? secret.value() : null; + retrievedValue.complete(value); + } + + }); + + } + catch (AzureKeyVaultAuthenticationException e) { + retrievedValue.completeExceptionally(e); + } + } + + }).start(); + + // Cache retrieved value + return new CachedSecret(retrievedValue, ttl); } diff --git a/extensions/guacamole-auth-vault/modules/guacamole-auth-vault-base/src/main/java/org/apache/guacamole/auth/vault/secret/CachedVaultSecretService.java b/extensions/guacamole-auth-vault/modules/guacamole-auth-vault-base/src/main/java/org/apache/guacamole/auth/vault/secret/CachedVaultSecretService.java index 838449857..63e95ce6d 100644 --- a/extensions/guacamole-auth-vault/modules/guacamole-auth-vault-base/src/main/java/org/apache/guacamole/auth/vault/secret/CachedVaultSecretService.java +++ b/extensions/guacamole-auth-vault/modules/guacamole-auth-vault-base/src/main/java/org/apache/guacamole/auth/vault/secret/CachedVaultSecretService.java @@ -48,9 +48,10 @@ public abstract class CachedVaultSecretService implements VaultSecretService { protected class CachedSecret { /** - * The value of the secret at the time it was last retrieved. + * A Future which contains or will contain the value of the secret at + * the time it was last retrieved. */ - private final String value; + private final Future value; /** * The time the value should be considered out-of-date, in milliseconds @@ -64,13 +65,15 @@ public abstract class CachedVaultSecretService implements VaultSecretService { * which it should be considered out-of-date. * * @param value - * The current value of the secret. + * A Future which contains or will contain the current value of the + * secret. If no such secret exists, the given Future should + * complete with null. * * @param ttl * The maximum number of milliseconds that this value should be * cached. */ - public CachedSecret(String value, int ttl) { + public CachedSecret(Future value, int ttl) { this.value = value; this.expires = System.currentTimeMillis() + ttl; } @@ -80,9 +83,14 @@ public abstract class CachedVaultSecretService implements VaultSecretService { * The actual value of the secret may have changed. * * @return - * The value of the secret at the time it was last retrieved. + * A Future which will eventually complete with the value of the + * secret at the time it was last retrieved. If no such secret + * exists, the Future will be completed with null. If an error + * occurs 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. */ - public String getValue() { + public Future getValue() { return value; } @@ -129,7 +137,7 @@ public abstract class CachedVaultSecretService implements VaultSecretService { throws GuacamoleException; @Override - public String getValue(String name) throws GuacamoleException { + public Future getValue(String name) throws GuacamoleException { CompletableFuture refreshEntry; @@ -175,7 +183,7 @@ public abstract class CachedVaultSecretService implements VaultSecretService { try { CachedSecret secret = refreshCachedSecret(name); refreshEntry.complete(secret); - logger.debug("Cached secret for \"{}\" has been refreshed.", name); + logger.debug("Cached secret for \"{}\" will be refreshed.", name); return secret.getValue(); } diff --git a/extensions/guacamole-auth-vault/modules/guacamole-auth-vault-base/src/main/java/org/apache/guacamole/auth/vault/secret/VaultSecretService.java b/extensions/guacamole-auth-vault/modules/guacamole-auth-vault-base/src/main/java/org/apache/guacamole/auth/vault/secret/VaultSecretService.java index 49d0d9ce3..a78826eb5 100644 --- a/extensions/guacamole-auth-vault/modules/guacamole-auth-vault-base/src/main/java/org/apache/guacamole/auth/vault/secret/VaultSecretService.java +++ b/extensions/guacamole-auth-vault/modules/guacamole-auth-vault-base/src/main/java/org/apache/guacamole/auth/vault/secret/VaultSecretService.java @@ -19,6 +19,7 @@ package org.apache.guacamole.auth.vault.secret; +import java.util.concurrent.Future; import org.apache.guacamole.GuacamoleException; /** @@ -49,19 +50,23 @@ public interface VaultSecretService { String canonicalize(String name); /** - * Returns the value of the secret having the given name. If no such - * secret exists, null is returned. + * 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. * * @param name * The name of the secret to retrieve. * * @return - * The value of the secret having the given name, or null if no such - * secret exists. + * 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. */ - String getValue(String name) throws GuacamoleException; + Future getValue(String name) throws GuacamoleException; } diff --git a/extensions/guacamole-auth-vault/modules/guacamole-auth-vault-base/src/main/java/org/apache/guacamole/auth/vault/user/VaultUserContext.java b/extensions/guacamole-auth-vault/modules/guacamole-auth-vault-base/src/main/java/org/apache/guacamole/auth/vault/user/VaultUserContext.java index 13cb6d515..5e8e6276d 100644 --- a/extensions/guacamole-auth-vault/modules/guacamole-auth-vault-base/src/main/java/org/apache/guacamole/auth/vault/user/VaultUserContext.java +++ b/extensions/guacamole-auth-vault/modules/guacamole-auth-vault-base/src/main/java/org/apache/guacamole/auth/vault/user/VaultUserContext.java @@ -24,7 +24,10 @@ import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.AssistedInject; 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.auth.vault.conf.VaultConfigurationService; import org.apache.guacamole.net.auth.Connection; import org.apache.guacamole.net.auth.ConnectionGroup; @@ -149,9 +152,9 @@ public class VaultUserContext extends TokenInjectingUserContext { } /** - * Retrieve all applicable tokens and corresponding values from the vault, - * using the given TokenFilter to filter tokens within the secret names - * prior to retrieving those secrets. + * Initiates asynchronous retrieval of all applicable tokens and + * corresponding values from the vault, using the given TokenFilter to + * filter tokens within the secret names prior to retrieving those secrets. * * @param tokenMapping * The mapping dictating the name of the secret which maps to each @@ -165,20 +168,20 @@ public class VaultUserContext extends TokenInjectingUserContext { * secrets to be retrieved from the vault. * * @return - * The tokens which should be added to the in-progress call to - * connect(). + * A Map of token name to Future, where each Future represents the + * pending retrieval operation which will ultimately be completed with + * the value of all secrets mapped to that token. * * @throws GuacamoleException * 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(Map tokenMapping, TokenFilter filter) throws GuacamoleException { - Map tokens = new HashMap<>(); - - // Populate map with tokens containing the values of all secrets - // indicated in the token mapping + // Populate map with pending secret retrieval operations corresponding + // to each mapped token + Map> pendingTokens = new HashMap<>(tokenMapping.size()); for (Map.Entry entry : tokenMapping.entrySet()) { // Translate secret pattern into secret name, ignoring any @@ -195,19 +198,60 @@ public class VaultUserContext extends TokenInjectingUserContext { continue; } + // Initiate asynchronous retrieval of the token value + String tokenName = entry.getKey(); + Future secret = secretService.getValue(secretName); + pendingTokens.put(tokenName, secret); + + } + + return pendingTokens; + + } + + /** + * Waits for all pending secret retrieval operations to complete, + * transforming each Future within the given Map into its contained String + * value. + * + * @param pendingTokens + * A Map of token name to Future, where each Future represents the + * pending retrieval operation which will ultimately be completed with + * the value of all secrets mapped to that token. + * + * @throws GuacamoleException + * If the value for any applicable secret cannot be retrieved from the + * vault due to an error. + */ + private Map resolve(Map> pendingTokens) throws GuacamoleException { + + // Populate map with tokens containing the values of their + // corresponding secrets + Map tokens = new HashMap<>(pendingTokens.size()); + for (Map.Entry> entry : pendingTokens.entrySet()) { + + // Complete secret retrieval operation, blocking if necessary + String secretValue; + try { + secretValue = entry.getValue().get(); + } + catch (InterruptedException | ExecutionException e) { + throw new GuacamoleServerException("Retrieval of secret value " + + "failed.", e); + } + // If a value is defined for the secret in question, store that // value under the mapped token String tokenName = entry.getKey(); - String secretValue = secretService.getValue(secretName); if (secretValue != null) { tokens.put(tokenName, secretValue); logger.debug("Token \"{}\" populated with value from " - + "secret \"{}\".", tokenName, secretName); + + "secret.", tokenName); } else logger.debug("Token \"{}\" not populated. Mapped " - + "secret \"{}\" has no value.", - tokenName, secretName); + + "secret has no value.", tokenName); } @@ -231,7 +275,7 @@ public class VaultUserContext extends TokenInjectingUserContext { // Substitute tokens producing secret names, retrieving and storing // those secrets as parameter tokens - return getTokens(confService.getTokenMapping(), filter); + return resolve(getTokens(confService.getTokenMapping(), filter)); } @@ -274,7 +318,7 @@ public class VaultUserContext extends TokenInjectingUserContext { // Substitute tokens producing secret names, retrieving and storing // those secrets as parameter tokens - return getTokens(confService.getTokenMapping(), filter); + return resolve(getTokens(confService.getTokenMapping(), filter)); }