From fe7cec886a8497660deae2efa3c3b3a585df24f8 Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Tue, 12 Dec 2023 12:58:17 +0100 Subject: [PATCH 01/15] Fix supported characters in SM --- .../ingestion/models/custom_pydantic.py | 6 +- .../service/secrets/SecretsManager.java | 98 +++++++++++-------- .../service/secrets/SecretsUtil.java | 2 +- .../secrets/SecretsManagerLifecycleTest.java | 13 +++ 4 files changed, 76 insertions(+), 43 deletions(-) diff --git a/ingestion/src/metadata/ingestion/models/custom_pydantic.py b/ingestion/src/metadata/ingestion/models/custom_pydantic.py index 7c69b286c54b..a75f5a0e92df 100644 --- a/ingestion/src/metadata/ingestion/models/custom_pydantic.py +++ b/ingestion/src/metadata/ingestion/models/custom_pydantic.py @@ -25,6 +25,8 @@ logger = logging.getLogger("metadata") +SECRET = "secret:" + class CustomSecretStr(SecretStr): """ @@ -91,10 +93,10 @@ def get_secret_value(self, skip_secret_manager: bool = False) -> str: if ( not skip_secret_manager - and self._secret_value.startswith("secret:") + and self._secret_value.startswith(SECRET) and SecretsManagerFactory().get_secrets_manager() ): - secret_id = self._secret_value.replace("secret:", "") + secret_id = self._secret_value.replace(SECRET, "") try: return ( SecretsManagerFactory() diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java index ba7029c54b9a..31ec4f79c292 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java @@ -20,6 +20,7 @@ import java.util.Arrays; import java.util.Locale; import java.util.Set; +import java.util.regex.Pattern; import javax.ws.rs.core.Response; import lombok.Getter; import org.openmetadata.annotations.PasswordField; @@ -44,6 +45,7 @@ public abstract class SecretsManager { @Getter private final String clusterPrefix; @Getter private final SecretsManagerProvider secretsManagerProvider; private Fernet fernet; + private static final Pattern SECRET_ID_PATTERN = Pattern.compile("[^A-Za-z0-9/_\\-]"); private static final Set> DO_NOT_ENCRYPT_CLASSES = Set.of(OpenMetadataJWTClientConfig.class, BasicAuthMechanism.class); @@ -210,60 +212,72 @@ public OpenMetadataConnection decryptOpenMetadataConnection(OpenMetadataConnecti } private Object encryptPasswordFields(Object toEncryptObject, String secretId, boolean store) { - if (!DO_NOT_ENCRYPT_CLASSES.contains(toEncryptObject.getClass())) { + try { + if (!DO_NOT_ENCRYPT_CLASSES.contains(toEncryptObject.getClass())) { + // for each get method + Arrays.stream(toEncryptObject.getClass().getMethods()) + .filter(ReflectionUtil::isGetMethodOfObject) + .forEach( + method -> { + Object obj = ReflectionUtil.getObjectFromMethod(method, toEncryptObject); + String fieldName = method.getName().replaceFirst("get", ""); + // if the object matches the package of openmetadata + if (obj != null && obj.getClass().getPackageName().startsWith("org.openmetadata")) { + // encryptPasswordFields + encryptPasswordFields( + obj, buildSecretId(false, secretId, fieldName.toLowerCase(Locale.ROOT)), store); + // check if it has annotation + } else if (obj != null && method.getAnnotation(PasswordField.class) != null) { + // store value if proceed + String newFieldValue = + storeValue(fieldName, fernet.decryptIfApplies((String) obj), secretId, store); + // get setMethod + Method toSet = ReflectionUtil.getToSetMethod(toEncryptObject, obj, fieldName); + // set new value + ReflectionUtil.setValueInMethod( + toEncryptObject, + Fernet.isTokenized(newFieldValue) + ? newFieldValue + : store ? fernet.encrypt(newFieldValue) : newFieldValue, + toSet); + } + }); + } + return toEncryptObject; + } catch (Exception e) { + throw new SecretsManagerException( + String.format("Error trying to encrypt object with secret ID [%s] due to [%s]", secretId, e.getMessage())); + } + } + + private Object decryptPasswordFields(Object toDecryptObject) { + try { // for each get method - Arrays.stream(toEncryptObject.getClass().getMethods()) + Arrays.stream(toDecryptObject.getClass().getMethods()) .filter(ReflectionUtil::isGetMethodOfObject) .forEach( method -> { - Object obj = ReflectionUtil.getObjectFromMethod(method, toEncryptObject); + Object obj = ReflectionUtil.getObjectFromMethod(method, toDecryptObject); String fieldName = method.getName().replaceFirst("get", ""); // if the object matches the package of openmetadata if (obj != null && obj.getClass().getPackageName().startsWith("org.openmetadata")) { // encryptPasswordFields - encryptPasswordFields(obj, buildSecretId(false, secretId, fieldName.toLowerCase(Locale.ROOT)), store); + decryptPasswordFields(obj); // check if it has annotation } else if (obj != null && method.getAnnotation(PasswordField.class) != null) { - // store value if proceed - String newFieldValue = storeValue(fieldName, fernet.decryptIfApplies((String) obj), secretId, store); + String fieldValue = (String) obj; // get setMethod - Method toSet = ReflectionUtil.getToSetMethod(toEncryptObject, obj, fieldName); + Method toSet = ReflectionUtil.getToSetMethod(toDecryptObject, obj, fieldName); // set new value ReflectionUtil.setValueInMethod( - toEncryptObject, - Fernet.isTokenized(newFieldValue) - ? newFieldValue - : store ? fernet.encrypt(newFieldValue) : newFieldValue, - toSet); + toDecryptObject, Fernet.isTokenized(fieldValue) ? fernet.decrypt(fieldValue) : fieldValue, toSet); } }); + return toDecryptObject; + } catch (Exception e) { + throw new SecretsManagerException( + String.format("Error trying to decrypt object [%s] due to [%s]", toDecryptObject.toString(), e.getMessage())); } - return toEncryptObject; - } - - private Object decryptPasswordFields(Object toDecryptObject) { - // for each get method - Arrays.stream(toDecryptObject.getClass().getMethods()) - .filter(ReflectionUtil::isGetMethodOfObject) - .forEach( - method -> { - Object obj = ReflectionUtil.getObjectFromMethod(method, toDecryptObject); - String fieldName = method.getName().replaceFirst("get", ""); - // if the object matches the package of openmetadata - if (obj != null && obj.getClass().getPackageName().startsWith("org.openmetadata")) { - // encryptPasswordFields - decryptPasswordFields(obj); - // check if it has annotation - } else if (obj != null && method.getAnnotation(PasswordField.class) != null) { - String fieldValue = (String) obj; - // get setMethod - Method toSet = ReflectionUtil.getToSetMethod(toDecryptObject, obj, fieldName); - // set new value - ReflectionUtil.setValueInMethod( - toDecryptObject, Fernet.isTokenized(fieldValue) ? fernet.decrypt(fieldValue) : fieldValue, toSet); - } - }); - return toDecryptObject; } protected abstract String storeValue(String fieldName, String value, String secretId, boolean store); @@ -276,8 +290,12 @@ protected String buildSecretId(boolean addClusterPrefix, String... secretIdValue } else { format.append("%s"); } + + // keep only alphanumeric characters and /, since we use / to create the FQN in the secrets manager + Object[] cleanIdValues = + Arrays.stream(secretIdValues).map(str -> SECRET_ID_PATTERN.matcher(str).replaceAll("_")).toArray(); // skip first one in case of addClusterPrefix is false to avoid adding extra separator at the beginning - Arrays.stream(secretIdValues) + Arrays.stream(cleanIdValues) .skip(addClusterPrefix ? 0 : 1) .forEach( secretIdValue -> { @@ -287,7 +305,7 @@ protected String buildSecretId(boolean addClusterPrefix, String... secretIdValue format.append("/"); format.append("%s"); }); - return String.format(format.toString(), (Object[]) secretIdValues).toLowerCase(); + return String.format(format.toString(), cleanIdValues).toLowerCase(); } @VisibleForTesting diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsUtil.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsUtil.java index d82d98e6009b..8e888592c42d 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsUtil.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsUtil.java @@ -34,7 +34,7 @@ public static String buildExceptionMessageUnrecognizedField( } return String.format(defaultMessage, type); } - return null; + return exceptionMessage; } public static String buildExceptionMessageConnection( diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java index b381bc32ccc9..a19721f3de90 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java @@ -125,4 +125,17 @@ void testWorkflowLifecycle() { assertEquals(exception.getMessage(), String.format("Key [%s] not found in in-memory secrets manager", secretName)); } + + @Test + void test_buildSecretId() { + // cluster prefix adds the initial / + assertEquals("/openmetadata/database/test_name", secretsManager.buildSecretId(true, "Database", "test_name")); + // non cluster prefix appends whatever it receives + assertEquals("database/test_name", secretsManager.buildSecretId(false, "Database", "test_name")); + assertEquals("/something/new/test_name", secretsManager.buildSecretId(false, "/something/new", "test_name")); + + // keep only alphanumeric characters and /, since we use / to create the FQN in the secrets manager + assertEquals("/openmetadata/database/test_name", secretsManager.buildSecretId(true, "Database", "test name")); + assertEquals("/something/new/test_name", secretsManager.buildSecretId(false, "/something/new", "test name")); + } } From 54454c4c919f697fc14364e4a3ca2e7a92788fa5 Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Tue, 12 Dec 2023 16:51:06 +0100 Subject: [PATCH 02/15] Update SM --- conf/openmetadata.yaml | 2 ++ .../secrets/AWSBasedSecretsManager.java | 4 +-- .../service/secrets/AWSSSMSecretsManager.java | 13 +++++++--- .../service/secrets/AWSSecretsManager.java | 15 ++++++++--- .../secrets/ExternalSecretsManager.java | 4 +-- .../secrets/InMemorySecretsManager.java | 8 +++--- .../service/secrets/NoopSecretsManager.java | 8 +++--- .../service/secrets/SecretsManager.java | 26 ++++++++++++++++--- .../secrets/SecretsManagerFactory.java | 12 +++++---- .../secrets/AWSSSMSecretsManagerTest.java | 4 ++- .../secrets/AWSSecretsManagerTest.java | 4 ++- .../secrets/NoopSecretsManagerTest.java | 4 ++- .../secrets/SecretsManagerLifecycleTest.java | 25 ++++++++++++++---- .../secrets/secretsManagerConfiguration.json | 16 +++++++++++- 14 files changed, 107 insertions(+), 38 deletions(-) diff --git a/conf/openmetadata.yaml b/conf/openmetadata.yaml index 839d362906e2..e0c06a81c4d9 100644 --- a/conf/openmetadata.yaml +++ b/conf/openmetadata.yaml @@ -279,6 +279,8 @@ fernetConfiguration: secretsManagerConfiguration: secretsManager: ${SECRET_MANAGER:-noop} # Possible values are "noop", "aws", "aws-ssm" + prefix: ${SECRET_MANAGER_PREFIX:-""} # Define the secret key ID as /// + tags: ${SECRET_MANAGER_TAGS-[]} # Add tags to the created resource, e.g., in AWS. Format is "[key1:value1,key2:value2,...]" # it will use the default auth provider for the secrets' manager service if parameters are not set parameters: region: ${OM_SM_REGION:-""} diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSBasedSecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSBasedSecretsManager.java index 01af42deb9bf..f5238be1cc87 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSBasedSecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSBasedSecretsManager.java @@ -28,8 +28,8 @@ public abstract class AWSBasedSecretsManager extends ExternalSecretsManager { public static final String REGION = "region"; protected AWSBasedSecretsManager( - SecretsManagerProvider awsProvider, SecretsManagerConfiguration config, String clusterPrefix) { - super(awsProvider, clusterPrefix, 100); + SecretsManagerProvider awsProvider, SecretsManagerConfiguration config, SecretsConfig secretsConfig) { + super(awsProvider, secretsConfig, 100); // initialize the secret client depending on the SecretsManagerConfiguration passed if (config != null && config.getParameters() != null diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSSMSecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSSMSecretsManager.java index 7612ca9c3eab..f7b97f7bd72f 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSSMSecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSSMSecretsManager.java @@ -23,13 +23,14 @@ import software.amazon.awssdk.services.ssm.model.GetParameterRequest; import software.amazon.awssdk.services.ssm.model.ParameterType; import software.amazon.awssdk.services.ssm.model.PutParameterRequest; +import software.amazon.awssdk.services.ssm.model.Tag; public class AWSSSMSecretsManager extends AWSBasedSecretsManager { private static AWSSSMSecretsManager instance = null; private SsmClient ssmClient; - private AWSSSMSecretsManager(SecretsManagerConfiguration config, String clusterPrefix) { - super(MANAGED_AWS_SSM, config, clusterPrefix); + private AWSSSMSecretsManager(SecretsManagerConfiguration config, SecretsConfig secretsConfig) { + super(MANAGED_AWS_SSM, config, secretsConfig); } @Override @@ -61,6 +62,10 @@ private void putSecretParameter(String parameterName, String parameterValue, boo .value(parameterValue) .overwrite(overwrite) .type(ParameterType.SECURE_STRING) + .tags( + this.getSecretsConfig().getTags().entrySet().stream() + .map(entry -> Tag.builder().key(entry.getKey()).value(entry.getValue()).build()) + .toList()) .build(); this.ssmClient.putParameter(putParameterRequest); } @@ -77,8 +82,8 @@ protected void deleteSecretInternal(String secretName) { this.ssmClient.deleteParameter(deleteParameterRequest); } - public static AWSSSMSecretsManager getInstance(SecretsManagerConfiguration config, String clusterPrefix) { - if (instance == null) instance = new AWSSSMSecretsManager(config, clusterPrefix); + public static AWSSSMSecretsManager getInstance(SecretsManagerConfiguration config, SecretsConfig secretsConfig) { + if (instance == null) instance = new AWSSSMSecretsManager(config, secretsConfig); return instance; } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSecretsManager.java index 019041e6137c..4d982801fbf7 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSecretsManager.java @@ -17,6 +17,7 @@ import com.google.common.annotations.VisibleForTesting; import java.util.Objects; +import lombok.extern.slf4j.Slf4j; import org.openmetadata.schema.security.secrets.SecretsManagerConfiguration; import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; import software.amazon.awssdk.regions.Region; @@ -24,14 +25,16 @@ import software.amazon.awssdk.services.secretsmanager.model.CreateSecretRequest; import software.amazon.awssdk.services.secretsmanager.model.DeleteSecretRequest; import software.amazon.awssdk.services.secretsmanager.model.GetSecretValueRequest; +import software.amazon.awssdk.services.secretsmanager.model.Tag; import software.amazon.awssdk.services.secretsmanager.model.UpdateSecretRequest; +@Slf4j public class AWSSecretsManager extends AWSBasedSecretsManager { private static AWSSecretsManager instance = null; private SecretsManagerClient secretsClient; - private AWSSecretsManager(SecretsManagerConfiguration config, String clusterPrefix) { - super(MANAGED_AWS, config, clusterPrefix); + private AWSSecretsManager(SecretsManagerConfiguration config, SecretsConfig secretsConfig) { + super(MANAGED_AWS, config, secretsConfig); } @Override @@ -52,6 +55,10 @@ public void storeSecret(String secretName, String secretValue) { .name(secretName) .description("This secret was created by OpenMetadata") .secretString(Objects.isNull(secretValue) ? NULL_SECRET_STRING : secretValue) + .tags( + this.getSecretsConfig().getTags().entrySet().stream() + .map(entry -> Tag.builder().key(entry.getKey()).value(entry.getValue()).build()) + .toList()) .build(); this.secretsClient.createSecret(createSecretRequest); } @@ -79,9 +86,9 @@ protected void deleteSecretInternal(String secretName) { this.secretsClient.deleteSecret(deleteSecretRequest); } - public static AWSSecretsManager getInstance(SecretsManagerConfiguration config, String clusterPrefix) { + public static AWSSecretsManager getInstance(SecretsManagerConfiguration config, SecretsConfig secretsConfig) { if (instance == null) { - instance = new AWSSecretsManager(config, clusterPrefix); + instance = new AWSSecretsManager(config, secretsConfig); } return instance; } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/ExternalSecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/ExternalSecretsManager.java index 76662f3611e7..f3c6d8813597 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/ExternalSecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/ExternalSecretsManager.java @@ -22,8 +22,8 @@ public abstract class ExternalSecretsManager extends SecretsManager { private final long waitTimeBetweenStoreCalls; protected ExternalSecretsManager( - SecretsManagerProvider secretsManagerProvider, String clusterPrefix, long waitTimeBetweenCalls) { - super(secretsManagerProvider, clusterPrefix); + SecretsManagerProvider secretsManagerProvider, SecretsConfig secretsConfig, long waitTimeBetweenCalls) { + super(secretsManagerProvider, secretsConfig); waitTimeBetweenStoreCalls = waitTimeBetweenCalls; } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/InMemorySecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/InMemorySecretsManager.java index 3727b4d0cb7f..ea0306a691d4 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/InMemorySecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/InMemorySecretsManager.java @@ -24,13 +24,13 @@ public class InMemorySecretsManager extends ExternalSecretsManager { private static InMemorySecretsManager instance; @Getter private final Map secretsMap = new HashMap<>(); - protected InMemorySecretsManager(String clusterPrefix) { - super(SecretsManagerProvider.IN_MEMORY, clusterPrefix, 0); + protected InMemorySecretsManager(SecretsConfig secretsConfig) { + super(SecretsManagerProvider.IN_MEMORY, secretsConfig, 0); } - public static InMemorySecretsManager getInstance(String clusterPrefix) { + public static InMemorySecretsManager getInstance(SecretsConfig secretsConfig) { if (instance == null) { - instance = new InMemorySecretsManager(clusterPrefix); + instance = new InMemorySecretsManager(secretsConfig); } return instance; } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/NoopSecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/NoopSecretsManager.java index 75aed6b9f90e..73e0e6408d03 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/NoopSecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/NoopSecretsManager.java @@ -18,13 +18,13 @@ public class NoopSecretsManager extends SecretsManager { private static NoopSecretsManager instance; - private NoopSecretsManager(String clusterPrefix, SecretsManagerProvider secretsManagerProvider) { - super(secretsManagerProvider, clusterPrefix); + private NoopSecretsManager(SecretsManagerProvider secretsManagerProvider, SecretsConfig secretsConfig) { + super(secretsManagerProvider, secretsConfig); } - public static NoopSecretsManager getInstance(String clusterPrefix, SecretsManagerProvider secretsManagerProvider) { + public static NoopSecretsManager getInstance(SecretsManagerProvider secretsManagerProvider, SecretsConfig secretsConfig) { if (instance == null) { - instance = new NoopSecretsManager(clusterPrefix, secretsManagerProvider); + instance = new NoopSecretsManager(secretsManagerProvider, secretsConfig); } return instance; } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java index 31ec4f79c292..bcaddd2e0d5f 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java @@ -18,11 +18,15 @@ import com.google.common.annotations.VisibleForTesting; import java.lang.reflect.Method; import java.util.Arrays; +import java.util.List; import java.util.Locale; +import java.util.Map; import java.util.Set; import java.util.regex.Pattern; +import java.util.stream.Collectors; import javax.ws.rs.core.Response; import lombok.Getter; +import lombok.extern.slf4j.Slf4j; import org.openmetadata.annotations.PasswordField; import org.openmetadata.schema.auth.BasicAuthMechanism; import org.openmetadata.schema.entity.automations.Workflow; @@ -41,8 +45,20 @@ import org.openmetadata.service.util.IngestionPipelineBuilder; import org.openmetadata.service.util.ReflectionUtil; +@Slf4j public abstract class SecretsManager { - @Getter private final String clusterPrefix; + public record SecretsConfig(String clusterName, String prefix, List tags) { + public Map getTags() { + try { + return tags.stream().collect(Collectors.toMap(s -> s.split(":")[0], s -> s.split(":")[1])); + } catch (Exception e) { + LOG.error(String.format("The SecretsConfig could not extract tags from [%s]", tags)); + return Map.of(); + } + } + } + + @Getter private final SecretsConfig secretsConfig; @Getter private final SecretsManagerProvider secretsManagerProvider; private Fernet fernet; private static final Pattern SECRET_ID_PATTERN = Pattern.compile("[^A-Za-z0-9/_\\-]"); @@ -50,9 +66,9 @@ public abstract class SecretsManager { private static final Set> DO_NOT_ENCRYPT_CLASSES = Set.of(OpenMetadataJWTClientConfig.class, BasicAuthMechanism.class); - protected SecretsManager(SecretsManagerProvider secretsManagerProvider, String clusterPrefix) { + protected SecretsManager(SecretsManagerProvider secretsManagerProvider, SecretsConfig secretsConfig) { this.secretsManagerProvider = secretsManagerProvider; - this.clusterPrefix = clusterPrefix; + this.secretsConfig = secretsConfig; this.fernet = Fernet.getInstance(); } @@ -286,7 +302,9 @@ protected String buildSecretId(boolean addClusterPrefix, String... secretIdValue StringBuilder format = new StringBuilder(); if (addClusterPrefix) { format.append("/"); - format.append(clusterPrefix); + format.append(secretsConfig.prefix); + format.append("/"); + format.append(secretsConfig.clusterName); } else { format.append("%s"); } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManagerFactory.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManagerFactory.java index 1a9caf6bee8d..3560b55a565f 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManagerFactory.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManagerFactory.java @@ -28,6 +28,8 @@ public static SecretsManager createSecretsManager(SecretsManagerConfiguration co if (secretsManager != null) { return secretsManager; } + SecretsManager.SecretsConfig secretsConfig = + new SecretsManager.SecretsConfig(clusterName, config.getPrefix(), config.getTags()); SecretsManagerProvider secretsManagerProvider = config != null && config.getSecretsManager() != null ? config.getSecretsManager() : SecretsManagerProvider.NOOP; switch (secretsManagerProvider) { @@ -43,18 +45,18 @@ public static SecretsManager createSecretsManager(SecretsManagerConfiguration co If for example we want to set the AWS SSM (non-managed) we configure the server as `secretsManager: aws-ssm` and set the Airflow env vars to connect to AWS SSM as specified in the docs: - https://docs.open-metadata.org/v1.0.0/deployment/secrets-manager/supported-implementations/aws-ssm-parameter-store + https://docs.open-metadata.org/deployment/secrets-manager/supported-implementations/aws-ssm-parameter-store */ - secretsManager = NoopSecretsManager.getInstance(clusterName, secretsManagerProvider); + secretsManager = NoopSecretsManager.getInstance(secretsManagerProvider, secretsConfig); break; case MANAGED_AWS: - secretsManager = AWSSecretsManager.getInstance(config, clusterName); + secretsManager = AWSSecretsManager.getInstance(config, secretsConfig); break; case MANAGED_AWS_SSM: - secretsManager = AWSSSMSecretsManager.getInstance(config, clusterName); + secretsManager = AWSSSMSecretsManager.getInstance(config, secretsConfig); break; case IN_MEMORY: - secretsManager = InMemorySecretsManager.getInstance(clusterName); + secretsManager = InMemorySecretsManager.getInstance(secretsConfig); break; default: throw new IllegalArgumentException("Not implemented secret manager store: " + secretsManagerProvider); diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSSMSecretsManagerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSSMSecretsManagerTest.java index 78d660aaf041..8bb5262f385b 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSSMSecretsManagerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSSMSecretsManagerTest.java @@ -19,13 +19,15 @@ import org.openmetadata.schema.security.secrets.SecretsManagerProvider; import software.amazon.awssdk.services.ssm.SsmClient; +import java.util.List; + public class AWSSSMSecretsManagerTest extends ExternalSecretsManagerTest { @Mock private SsmClient ssmClient; @Override void setUpSpecific(SecretsManagerConfiguration config) { - secretsManager = AWSSSMSecretsManager.getInstance(config, "openmetadata"); + secretsManager = AWSSSMSecretsManager.getInstance(config, new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"))); ((AWSSSMSecretsManager) secretsManager).setSsmClient(ssmClient); reset(ssmClient); } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSecretsManagerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSecretsManagerTest.java index ceb3d323179f..2a497b50a2b8 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSecretsManagerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSecretsManagerTest.java @@ -19,13 +19,15 @@ import org.openmetadata.schema.security.secrets.SecretsManagerProvider; import software.amazon.awssdk.services.secretsmanager.SecretsManagerClient; +import java.util.List; + public class AWSSecretsManagerTest extends ExternalSecretsManagerTest { @Mock private SecretsManagerClient secretsManagerClient; @Override void setUpSpecific(SecretsManagerConfiguration config) { - secretsManager = AWSSecretsManager.getInstance(config, "openmetadata"); + secretsManager = AWSSecretsManager.getInstance(config, new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"))); ((AWSSecretsManager) secretsManager).setSecretsClient(secretsManagerClient); reset(secretsManagerClient); } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/NoopSecretsManagerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/NoopSecretsManagerTest.java index 63d0658c1c6e..ee0822ced9bf 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/NoopSecretsManagerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/NoopSecretsManagerTest.java @@ -34,6 +34,8 @@ import org.openmetadata.service.fernet.Fernet; import org.openmetadata.service.util.JsonUtils; +import java.util.List; + @ExtendWith(MockitoExtension.class) public class NoopSecretsManagerTest { @@ -43,7 +45,7 @@ public class NoopSecretsManagerTest { @BeforeAll static void setUp() { - secretsManager = NoopSecretsManager.getInstance("openmetadata", SecretsManagerProvider.NOOP); + secretsManager = NoopSecretsManager.getInstance(SecretsManagerProvider.NOOP, new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"))); Fernet fernet = Mockito.mock(Fernet.class); lenient().when(fernet.decrypt(anyString())).thenReturn(DECRYPTED_VALUE); lenient().when(fernet.decryptIfApplies(anyString())).thenReturn(DECRYPTED_VALUE); diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java index a19721f3de90..56257d6d4d96 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java @@ -7,6 +7,7 @@ import static org.mockito.Mockito.lenient; import static org.openmetadata.schema.api.services.CreateDatabaseService.DatabaseServiceType.Mysql; +import java.util.List; import java.util.Map; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -35,7 +36,9 @@ public class SecretsManagerLifecycleTest { @BeforeAll static void setUp() { - secretsManager = InMemorySecretsManager.getInstance("openmetadata"); + secretsManager = + InMemorySecretsManager.getInstance( + new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key1:value1", "key2:value2"))); Fernet fernet = Mockito.mock(Fernet.class); lenient().when(fernet.decrypt(anyString())).thenReturn(DECRYPTED_VALUE); lenient().when(fernet.decryptIfApplies(anyString())).thenReturn(DECRYPTED_VALUE); @@ -46,7 +49,7 @@ static void setUp() { @Test void testDatabaseServiceConnectionConfigLifecycle() { String password = "openmetadata-test"; - String secretName = "/openmetadata/database/test/authtype/password"; + String secretName = "/prefix/openmetadata/database/test/authtype/password"; String connectionName = "test"; Map> mysqlConnection = Map.of("authType", Map.of("password", password)); @@ -82,7 +85,7 @@ void testDatabaseServiceConnectionConfigLifecycle() { @Test void testWorkflowLifecycle() { String password = "openmetadata_password"; - String secretName = "/openmetadata/workflow/test-connection/request/connection/config/authtype/password"; + String secretName = "/prefix/openmetadata/workflow/test-connection/request/connection/config/authtype/password"; Workflow workflow = new Workflow() @@ -129,13 +132,25 @@ void testWorkflowLifecycle() { @Test void test_buildSecretId() { // cluster prefix adds the initial / - assertEquals("/openmetadata/database/test_name", secretsManager.buildSecretId(true, "Database", "test_name")); + assertEquals( + "/prefix/openmetadata/database/test_name", secretsManager.buildSecretId(true, "Database", "test_name")); // non cluster prefix appends whatever it receives assertEquals("database/test_name", secretsManager.buildSecretId(false, "Database", "test_name")); assertEquals("/something/new/test_name", secretsManager.buildSecretId(false, "/something/new", "test_name")); // keep only alphanumeric characters and /, since we use / to create the FQN in the secrets manager - assertEquals("/openmetadata/database/test_name", secretsManager.buildSecretId(true, "Database", "test name")); + assertEquals( + "/prefix/openmetadata/database/test_name", secretsManager.buildSecretId(true, "Database", "test name")); assertEquals("/something/new/test_name", secretsManager.buildSecretId(false, "/something/new", "test name")); } + + @Test + void test_getTags() { + assertEquals(Map.of("key1", "value1", "key2", "value2"), secretsManager.getSecretsConfig().getTags()); + + // if the tags are not well formatted, we don't return anything + SecretsManager.SecretsConfig secretsConfig = + new SecretsManager.SecretsConfig(null, null, List.of("key:value", "random")); + assertEquals(Map.of(), secretsConfig.getTags()); + } } diff --git a/openmetadata-spec/src/main/resources/json/schema/security/secrets/secretsManagerConfiguration.json b/openmetadata-spec/src/main/resources/json/schema/security/secrets/secretsManagerConfiguration.json index 330f39be990d..691df5a10a79 100644 --- a/openmetadata-spec/src/main/resources/json/schema/security/secrets/secretsManagerConfiguration.json +++ b/openmetadata-spec/src/main/resources/json/schema/security/secrets/secretsManagerConfiguration.json @@ -1,7 +1,7 @@ { "$id": "https://open-metadata.org/schema/security/secrets/secretsManagerConfiguration.json", "$schema": "http://json-schema.org/draft-07/schema#", - "title": "ValidateSSLClientConfig", + "title": "SecretsManagerConfiguration", "description": "OpenMetadata server configuration for the Secrets Manager feature.", "type": "object", "javaType": "org.openmetadata.schema.security.secrets.SecretsManagerConfiguration", @@ -12,6 +12,20 @@ "$ref": "secretsManagerProvider.json", "default": "noop" }, + "prefix": { + "title": "Secret ID Prefix", + "description": "Prefix to be added to the secret key ID: `///`", + "type": "string" + }, + "tags": { + "title": "Secret Resource Tags", + "description": "Add tags to the created resource, e.g., in AWS. Format is `[key1:value1,key2:value2,...]`", + "type": "array", + "items": { + "type": "string" + }, + "default": null + }, "parameters": { "title": "Parameters", "description": "Extra parameters used by the Secrets Manager implementation.", From 35d8345bbdf3be994876cf32568457e0b5774f91 Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Tue, 12 Dec 2023 17:54:52 +0100 Subject: [PATCH 03/15] Fixes --- conf/openmetadata.yaml | 2 +- .../service/secrets/AWSSSMSecretsManager.java | 2 +- .../service/secrets/AWSSecretsManager.java | 2 +- .../service/secrets/NoopSecretsManager.java | 3 ++- .../service/secrets/SecretsManager.java | 20 +++++++++---------- .../secrets/AWSSSMSecretsManagerTest.java | 7 ++++--- .../secrets/AWSSecretsManagerTest.java | 7 ++++--- .../secrets/NoopSecretsManagerTest.java | 8 +++++--- .../secrets/SecretsManagerLifecycleTest.java | 4 ++-- 9 files changed, 30 insertions(+), 25 deletions(-) diff --git a/conf/openmetadata.yaml b/conf/openmetadata.yaml index e0c06a81c4d9..59f5d0e27057 100644 --- a/conf/openmetadata.yaml +++ b/conf/openmetadata.yaml @@ -280,7 +280,7 @@ fernetConfiguration: secretsManagerConfiguration: secretsManager: ${SECRET_MANAGER:-noop} # Possible values are "noop", "aws", "aws-ssm" prefix: ${SECRET_MANAGER_PREFIX:-""} # Define the secret key ID as /// - tags: ${SECRET_MANAGER_TAGS-[]} # Add tags to the created resource, e.g., in AWS. Format is "[key1:value1,key2:value2,...]" + tags: ${SECRET_MANAGER_TAGS:-[]} # Add tags to the created resource, e.g., in AWS. Format is `[key1:value1,key2:value2,...]` # it will use the default auth provider for the secrets' manager service if parameters are not set parameters: region: ${OM_SM_REGION:-""} diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSSMSecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSSMSecretsManager.java index f7b97f7bd72f..76b6ce7dea0d 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSSMSecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSSMSecretsManager.java @@ -63,7 +63,7 @@ private void putSecretParameter(String parameterName, String parameterValue, boo .overwrite(overwrite) .type(ParameterType.SECURE_STRING) .tags( - this.getSecretsConfig().getTags().entrySet().stream() + SecretsManager.getTags(getSecretsConfig()).entrySet().stream() .map(entry -> Tag.builder().key(entry.getKey()).value(entry.getValue()).build()) .toList()) .build(); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSecretsManager.java index 4d982801fbf7..288d340786f9 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSecretsManager.java @@ -56,7 +56,7 @@ public void storeSecret(String secretName, String secretValue) { .description("This secret was created by OpenMetadata") .secretString(Objects.isNull(secretValue) ? NULL_SECRET_STRING : secretValue) .tags( - this.getSecretsConfig().getTags().entrySet().stream() + SecretsManager.getTags(getSecretsConfig()).entrySet().stream() .map(entry -> Tag.builder().key(entry.getKey()).value(entry.getValue()).build()) .toList()) .build(); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/NoopSecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/NoopSecretsManager.java index 73e0e6408d03..75177803f7c5 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/NoopSecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/NoopSecretsManager.java @@ -22,7 +22,8 @@ private NoopSecretsManager(SecretsManagerProvider secretsManagerProvider, Secret super(secretsManagerProvider, secretsConfig); } - public static NoopSecretsManager getInstance(SecretsManagerProvider secretsManagerProvider, SecretsConfig secretsConfig) { + public static NoopSecretsManager getInstance( + SecretsManagerProvider secretsManagerProvider, SecretsConfig secretsConfig) { if (instance == null) { instance = new NoopSecretsManager(secretsManagerProvider, secretsConfig); } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java index bcaddd2e0d5f..0ffc7b889191 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java @@ -47,16 +47,7 @@ @Slf4j public abstract class SecretsManager { - public record SecretsConfig(String clusterName, String prefix, List tags) { - public Map getTags() { - try { - return tags.stream().collect(Collectors.toMap(s -> s.split(":")[0], s -> s.split(":")[1])); - } catch (Exception e) { - LOG.error(String.format("The SecretsConfig could not extract tags from [%s]", tags)); - return Map.of(); - } - } - } + public record SecretsConfig(String clusterName, String prefix, List tags) {} @Getter private final SecretsConfig secretsConfig; @Getter private final SecretsManagerProvider secretsManagerProvider; @@ -383,4 +374,13 @@ private void deleteSecrets(Object toDeleteSecretsFrom, String secretId) { }); } } + + public static Map getTags(SecretsConfig secretsConfig) { + try { + return secretsConfig.tags.stream().collect(Collectors.toMap(s -> s.split(":")[0], s -> s.split(":")[1])); + } catch (Exception e) { + LOG.error(String.format("The SecretsConfig could not extract tags from [%s]", secretsConfig.tags)); + return Map.of(); + } + } } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSSMSecretsManagerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSSMSecretsManagerTest.java index 8bb5262f385b..ec540a1e6e96 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSSMSecretsManagerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSSMSecretsManagerTest.java @@ -14,20 +14,21 @@ import static org.mockito.Mockito.reset; +import java.util.List; import org.mockito.Mock; import org.openmetadata.schema.security.secrets.SecretsManagerConfiguration; import org.openmetadata.schema.security.secrets.SecretsManagerProvider; import software.amazon.awssdk.services.ssm.SsmClient; -import java.util.List; - public class AWSSSMSecretsManagerTest extends ExternalSecretsManagerTest { @Mock private SsmClient ssmClient; @Override void setUpSpecific(SecretsManagerConfiguration config) { - secretsManager = AWSSSMSecretsManager.getInstance(config, new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"))); + secretsManager = + AWSSSMSecretsManager.getInstance( + config, new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"))); ((AWSSSMSecretsManager) secretsManager).setSsmClient(ssmClient); reset(ssmClient); } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSecretsManagerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSecretsManagerTest.java index 2a497b50a2b8..62453463dfc9 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSecretsManagerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSecretsManagerTest.java @@ -14,20 +14,21 @@ import static org.mockito.Mockito.reset; +import java.util.List; import org.mockito.Mock; import org.openmetadata.schema.security.secrets.SecretsManagerConfiguration; import org.openmetadata.schema.security.secrets.SecretsManagerProvider; import software.amazon.awssdk.services.secretsmanager.SecretsManagerClient; -import java.util.List; - public class AWSSecretsManagerTest extends ExternalSecretsManagerTest { @Mock private SecretsManagerClient secretsManagerClient; @Override void setUpSpecific(SecretsManagerConfiguration config) { - secretsManager = AWSSecretsManager.getInstance(config, new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"))); + secretsManager = + AWSSecretsManager.getInstance( + config, new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"))); ((AWSSecretsManager) secretsManager).setSecretsClient(secretsManagerClient); reset(secretsManagerClient); } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/NoopSecretsManagerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/NoopSecretsManagerTest.java index ee0822ced9bf..6d7b51ec5e06 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/NoopSecretsManagerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/NoopSecretsManagerTest.java @@ -20,6 +20,7 @@ import static org.openmetadata.schema.api.services.CreateMlModelService.MlModelServiceType.Sklearn; import static org.openmetadata.schema.entity.services.ServiceType.ML_MODEL; +import java.util.List; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -34,8 +35,6 @@ import org.openmetadata.service.fernet.Fernet; import org.openmetadata.service.util.JsonUtils; -import java.util.List; - @ExtendWith(MockitoExtension.class) public class NoopSecretsManagerTest { @@ -45,7 +44,10 @@ public class NoopSecretsManagerTest { @BeforeAll static void setUp() { - secretsManager = NoopSecretsManager.getInstance(SecretsManagerProvider.NOOP, new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"))); + secretsManager = + NoopSecretsManager.getInstance( + SecretsManagerProvider.NOOP, + new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"))); Fernet fernet = Mockito.mock(Fernet.class); lenient().when(fernet.decrypt(anyString())).thenReturn(DECRYPTED_VALUE); lenient().when(fernet.decryptIfApplies(anyString())).thenReturn(DECRYPTED_VALUE); diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java index 56257d6d4d96..63e58bc93fb6 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java @@ -146,11 +146,11 @@ void test_buildSecretId() { @Test void test_getTags() { - assertEquals(Map.of("key1", "value1", "key2", "value2"), secretsManager.getSecretsConfig().getTags()); + assertEquals(Map.of("key1", "value1", "key2", "value2"), SecretsManager.getTags(secretsManager.getSecretsConfig())); // if the tags are not well formatted, we don't return anything SecretsManager.SecretsConfig secretsConfig = new SecretsManager.SecretsConfig(null, null, List.of("key:value", "random")); - assertEquals(Map.of(), secretsConfig.getTags()); + assertEquals(Map.of(), SecretsManager.getTags(secretsConfig)); } } From e90a6482b36310a911ca992358d6614210fb1cb1 Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Tue, 12 Dec 2023 18:04:45 +0100 Subject: [PATCH 04/15] Fixes --- .../org/openmetadata/service/secrets/SecretsManager.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java index 0ffc7b889191..ae433e42e265 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java @@ -292,8 +292,10 @@ private Object decryptPasswordFields(Object toDecryptObject) { protected String buildSecretId(boolean addClusterPrefix, String... secretIdValues) { StringBuilder format = new StringBuilder(); if (addClusterPrefix) { - format.append("/"); - format.append(secretsConfig.prefix); + if (secretsConfig.prefix != null && !secretsConfig.prefix.isEmpty()) { + format.append("/"); + format.append(secretsConfig.prefix); + } format.append("/"); format.append(secretsConfig.clusterName); } else { From ef0c8aaf7219623df77b58ead652c0eab01751a8 Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Wed, 13 Dec 2023 07:55:49 +0100 Subject: [PATCH 05/15] Improve class conversion exceptions --- .../service/secrets/SecretsManager.java | 18 ++++++--------- .../service/secrets/SecretsUtil.java | 22 +++++++++++++++++++ .../secrets/masker/PasswordEntityMasker.java | 8 +++---- .../services/DatabaseServiceResourceTest.java | 4 ++-- .../services/PipelineServiceResourceTest.java | 2 +- 5 files changed, 35 insertions(+), 19 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java index ae433e42e265..79e82f8b2fe1 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java @@ -66,8 +66,7 @@ protected SecretsManager(SecretsManagerProvider secretsManagerProvider, SecretsC public Object encryptServiceConnectionConfig( Object connectionConfig, String connectionType, String connectionName, ServiceType serviceType) { try { - Class clazz = ReflectionUtil.createConnectionConfigClass(connectionType, serviceType); - Object newConnectionConfig = ClassConverterFactory.getConverter(clazz).convert(connectionConfig); + Object newConnectionConfig = SecretsUtil.convert(connectionConfig, connectionType, connectionName, serviceType); return encryptPasswordFields(newConnectionConfig, buildSecretId(true, serviceType.value(), connectionName), true); } catch (Exception e) { String message = SecretsUtil.buildExceptionMessageConnection(e.getMessage(), connectionType, true); @@ -83,8 +82,7 @@ public Object encryptServiceConnectionConfig( public Object decryptServiceConnectionConfig( Object connectionConfig, String connectionType, ServiceType serviceType) { try { - Class clazz = ReflectionUtil.createConnectionConfigClass(connectionType, serviceType); - Object newConnectionConfig = ClassConverterFactory.getConverter(clazz).convert(connectionConfig); + Object newConnectionConfig = SecretsUtil.convert(connectionConfig, connectionType, null, serviceType); return decryptPasswordFields(newConnectionConfig); } catch (Exception e) { String message = SecretsUtil.buildExceptionMessageConnection(e.getMessage(), connectionType, false); @@ -116,7 +114,7 @@ public void decryptAuthenticationMechanism(String name, AuthenticationMechanism decryptPasswordFields(authenticationMechanism); } catch (Exception e) { throw new CustomExceptionMessage( - Response.Status.BAD_REQUEST, String.format("Failed to encrypt user bot instance [%s]", name)); + Response.Status.BAD_REQUEST, String.format("Failed to decrypt user bot instance [%s]", name)); } } } @@ -148,7 +146,7 @@ public void decryptIngestionPipeline(IngestionPipeline ingestionPipeline) { } catch (Exception e) { throw new CustomExceptionMessage( Response.Status.BAD_REQUEST, - String.format("Failed to encrypt ingestion pipeline instance [%s]", ingestionPipeline.getName())); + String.format("Failed to decrypt ingestion pipeline instance [%s]", ingestionPipeline.getName())); } ingestionPipeline.setOpenMetadataServerConnection(openMetadataConnection); } @@ -179,7 +177,7 @@ public Workflow decryptWorkflow(Workflow workflow) { decryptPasswordFields(workflowConverted); } catch (Exception e) { throw new CustomExceptionMessage( - Response.Status.BAD_REQUEST, String.format("Failed to encrypt workflow instance [%s]", workflow.getName())); + Response.Status.BAD_REQUEST, String.format("Failed to decrypt workflow instance [%s]", workflow.getName())); } workflowConverted.setOpenMetadataServerConnection(openMetadataConnection); return workflowConverted; @@ -211,7 +209,7 @@ public OpenMetadataConnection decryptOpenMetadataConnection(OpenMetadataConnecti decryptPasswordFields(openMetadataConnectionConverted); } catch (Exception e) { throw new CustomExceptionMessage( - Response.Status.BAD_REQUEST, "Failed to encrypt OpenMetadataConnection instance."); + Response.Status.BAD_REQUEST, "Failed to decrypt OpenMetadataConnection instance."); } return openMetadataConnectionConverted; } @@ -330,10 +328,8 @@ public void deleteSecretsFromServiceConnectionConfig( Object connectionConfig, String connectionType, String connectionName, ServiceType serviceType) { try { - Class clazz = ReflectionUtil.createConnectionConfigClass(connectionType, serviceType); - Object newConnectionConfig = ClassConverterFactory.getConverter(clazz).convert(connectionConfig); + Object newConnectionConfig = SecretsUtil.convert(connectionConfig, connectionType, connectionName, serviceType); deleteSecrets(newConnectionConfig, buildSecretId(true, serviceType.value(), connectionName)); - } catch (Exception e) { String message = SecretsUtil.buildExceptionMessageConnection(e.getMessage(), connectionType, true); if (message != null) { diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsUtil.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsUtil.java index 8e888592c42d..46bdfbaf715f 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsUtil.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsUtil.java @@ -15,6 +15,10 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; +import org.openmetadata.schema.entity.services.ServiceType; +import org.openmetadata.service.exception.InvalidServiceConnectionException; +import org.openmetadata.service.secrets.converter.ClassConverterFactory; +import org.openmetadata.service.util.ReflectionUtil; public final class SecretsUtil { @@ -57,4 +61,22 @@ public static String buildExceptionMessageConnection(String exceptionMessage, St public static String buildExceptionMessageConnectionMask(String exceptionMessage, String type, boolean mask) { return buildExceptionMessageConnection(exceptionMessage, type, "mask", "unmask", mask); } + + public static Object convert( + Object connectionConfig, String connectionType, String connectionName, ServiceType serviceType) { + try { + Class clazz = ReflectionUtil.createConnectionConfigClass(connectionType, serviceType); + return ClassConverterFactory.getConverter(clazz).convert(connectionConfig); + } catch (Exception e) { + // If we have the name we are trying to encrypt a connection + if (connectionName != null) { + throw new InvalidServiceConnectionException( + String.format( + "Failed to convert [%s] to type [%s]. Review the connection.", connectionName, connectionType)); + } + // If we don't have the name, we are decrypting from the db + throw new InvalidServiceConnectionException( + String.format("Failed to load the connection of type [%s]. Did migrations run properly?", connectionType)); + } + } } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/masker/PasswordEntityMasker.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/masker/PasswordEntityMasker.java index 8bf1bd59896a..888757a59af9 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/masker/PasswordEntityMasker.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/masker/PasswordEntityMasker.java @@ -39,8 +39,7 @@ protected PasswordEntityMasker() {} public Object maskServiceConnectionConfig(Object connectionConfig, String connectionType, ServiceType serviceType) { if (connectionConfig != null) { try { - Class clazz = ReflectionUtil.createConnectionConfigClass(connectionType, serviceType); - Object convertedConnectionConfig = ClassConverterFactory.getConverter(clazz).convert(connectionConfig); + Object convertedConnectionConfig = SecretsUtil.convert(connectionConfig, connectionType, null, serviceType); maskPasswordFields(convertedConnectionConfig); return convertedConnectionConfig; } catch (Exception e) { @@ -95,9 +94,8 @@ public Object unmaskServiceConnectionConfig( Object connectionConfig, Object originalConnectionConfig, String connectionType, ServiceType serviceType) { if (originalConnectionConfig != null && connectionConfig != null) { try { - Class clazz = ReflectionUtil.createConnectionConfigClass(connectionType, serviceType); - Object toUnmaskConfig = ClassConverterFactory.getConverter(clazz).convert(connectionConfig); - Object originalConvertedConfig = ClassConverterFactory.getConverter(clazz).convert(originalConnectionConfig); + Object toUnmaskConfig = SecretsUtil.convert(connectionConfig, connectionType, null, serviceType); + Object originalConvertedConfig = SecretsUtil.convert(connectionConfig, connectionType, null, serviceType); Map passwordsMap = new HashMap<>(); buildPasswordsMap(originalConvertedConfig, NEW_KEY, passwordsMap); unmaskPasswordFields(toUnmaskConfig, NEW_KEY, passwordsMap); diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/services/DatabaseServiceResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/services/DatabaseServiceResourceTest.java index 14749305220a..da1160a14e4b 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/services/DatabaseServiceResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/services/DatabaseServiceResourceTest.java @@ -174,7 +174,7 @@ void post_put_invalidConnection_as_admin_4xx(TestInfo test) throws IOException { assertResponseContains( () -> createEntity(createRequest(test).withDescription(null).withConnection(dbConn), ADMIN_AUTH_HEADERS), BAD_REQUEST, - "InvalidServiceConnectionException for service [Snowflake] due to [Failed to encrypt connection instance of Snowflake. Did the Fernet Key change?]"); + String.format("Failed to convert [%s] to type [Snowflake]. Review the connection.", getEntityName(test))); DatabaseService service = createAndCheckEntity(createRequest(test).withDescription(null), ADMIN_AUTH_HEADERS); // Update database description and ingestion service that are null CreateDatabaseService update = createRequest(test).withDescription("description1"); @@ -188,7 +188,7 @@ void post_put_invalidConnection_as_admin_4xx(TestInfo test) throws IOException { assertResponseContains( () -> updateEntity(update, OK, ADMIN_AUTH_HEADERS), BAD_REQUEST, - "InvalidServiceConnectionException for service [Snowflake] due to [Failed to unmask connection instance of Snowflake]."); + "Failed to load the connection of type [Snowflake]. Did migrations run properly?"); } @Test diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/services/PipelineServiceResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/services/PipelineServiceResourceTest.java index bc5d0f54dfc9..1ff585f6f2bb 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/services/PipelineServiceResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/services/PipelineServiceResourceTest.java @@ -155,7 +155,7 @@ void post_put_invalidConnection_as_admin_4xx(TestInfo test) { createEntity( createRequest(test).withDescription(null).withConnection(pipelineConnection), ADMIN_AUTH_HEADERS), BAD_REQUEST, - "InvalidServiceConnectionException for service [Airflow] due to [Failed to encrypt connection instance of Airflow. Did the Fernet Key change?]"); + String.format("Failed to convert [%s] to type [Airflow]. Review the connection.", getEntityName(test))); } @Test From 5f2007ac3c13248f7aced479bf5d73d097a4710e Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Thu, 14 Dec 2023 09:01:00 +0100 Subject: [PATCH 06/15] Comments --- .../openmetadata/common/utils/CommonUtil.java | 11 ++++- .../exception/SecretsManagerException.java | 4 ++ .../secrets/AWSBasedSecretsManager.java | 17 +++---- .../service/secrets/AWSSSMSecretsManager.java | 9 ++-- .../service/secrets/AWSSecretsManager.java | 9 ++-- .../service/secrets/NoopSecretsManager.java | 5 +- .../service/secrets/SecretsManager.java | 49 +++++++++++-------- .../secrets/SecretsManagerFactory.java | 10 ++-- .../secrets/SecretsManagerLifecycleTest.java | 6 +-- 9 files changed, 67 insertions(+), 53 deletions(-) diff --git a/common/src/main/java/org/openmetadata/common/utils/CommonUtil.java b/common/src/main/java/org/openmetadata/common/utils/CommonUtil.java index 822d79631d54..3d56ad395a19 100644 --- a/common/src/main/java/org/openmetadata/common/utils/CommonUtil.java +++ b/common/src/main/java/org/openmetadata/common/utils/CommonUtil.java @@ -50,7 +50,7 @@ @Slf4j public final class CommonUtil { - private static final List jarNameFilter = List.of("openmetadata", "collate"); + private static final List JAR_NAME_FILTER = List.of("openmetadata", "collate"); private CommonUtil() {} @@ -60,7 +60,7 @@ public static List getResources(Pattern pattern) throws IOException { String classPath = System.getProperty("java.class.path", "."); List classPathElements = Arrays.stream(classPath.split(File.pathSeparator)) - .filter(jarName -> jarNameFilter.stream().anyMatch(jarName.toLowerCase()::contains)) + .filter(jarName -> JAR_NAME_FILTER.stream().anyMatch(jarName.toLowerCase()::contains)) .toList(); for (String element : classPathElements) { @@ -71,6 +71,13 @@ public static List getResources(Pattern pattern) throws IOException { return resources; } + /** Check if any given object falls under OM, or Collate packages */ + public static Boolean isOpenMetadataObject(Object obj) { + return obj != null + && JAR_NAME_FILTER.stream() + .anyMatch(Arrays.stream(obj.getClass().getPackageName().split("\\.")).toList()::contains); + } + private static Collection getResourcesFromJarFile(File file, Pattern pattern) { ArrayList retval = new ArrayList<>(); try (ZipFile zf = new ZipFile(file)) { diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/exception/SecretsManagerException.java b/openmetadata-service/src/main/java/org/openmetadata/service/exception/SecretsManagerException.java index 22f3cd9e8aae..fd805c213d6a 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/exception/SecretsManagerException.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/exception/SecretsManagerException.java @@ -24,6 +24,10 @@ public SecretsManagerException(String message) { super(Response.Status.INTERNAL_SERVER_ERROR, message); } + public SecretsManagerException(Response.Status status, String message) { + super(status.getStatusCode(), message); + } + public static SecretsManagerException byMessage(String secretManager, String connectionType, String errorMessage) { return new SecretsManagerException(buildMessageByName(secretManager, connectionType, errorMessage)); } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSBasedSecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSBasedSecretsManager.java index f5238be1cc87..46633f63e420 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSBasedSecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSBasedSecretsManager.java @@ -14,7 +14,6 @@ package org.openmetadata.service.secrets; import org.apache.logging.log4j.util.Strings; -import org.openmetadata.schema.security.secrets.SecretsManagerConfiguration; import org.openmetadata.schema.security.secrets.SecretsManagerProvider; import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; @@ -27,17 +26,17 @@ public abstract class AWSBasedSecretsManager extends ExternalSecretsManager { public static final String SECRET_ACCESS_KEY = "secretAccessKey"; public static final String REGION = "region"; - protected AWSBasedSecretsManager( - SecretsManagerProvider awsProvider, SecretsManagerConfiguration config, SecretsConfig secretsConfig) { + protected AWSBasedSecretsManager(SecretsManagerProvider awsProvider, SecretsConfig secretsConfig) { super(awsProvider, secretsConfig, 100); // initialize the secret client depending on the SecretsManagerConfiguration passed - if (config != null - && config.getParameters() != null - && !Strings.isBlank((String) config.getParameters().getAdditionalProperties().getOrDefault(REGION, ""))) { - String region = (String) config.getParameters().getAdditionalProperties().getOrDefault(REGION, ""); - String accessKeyId = (String) config.getParameters().getAdditionalProperties().getOrDefault(ACCESS_KEY_ID, ""); + if (secretsConfig != null + && secretsConfig.parameters() != null + && !Strings.isBlank((String) secretsConfig.parameters().getAdditionalProperties().getOrDefault(REGION, ""))) { + String region = (String) secretsConfig.parameters().getAdditionalProperties().getOrDefault(REGION, ""); + String accessKeyId = + (String) secretsConfig.parameters().getAdditionalProperties().getOrDefault(ACCESS_KEY_ID, ""); String secretAccessKey = - (String) config.getParameters().getAdditionalProperties().getOrDefault(SECRET_ACCESS_KEY, ""); + (String) secretsConfig.parameters().getAdditionalProperties().getOrDefault(SECRET_ACCESS_KEY, ""); AwsCredentialsProvider credentialsProvider; if (Strings.isBlank(accessKeyId) && Strings.isBlank(secretAccessKey)) { credentialsProvider = DefaultCredentialsProvider.create(); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSSMSecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSSMSecretsManager.java index 76b6ce7dea0d..b03f88e8d8c4 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSSMSecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSSMSecretsManager.java @@ -15,7 +15,6 @@ import static org.openmetadata.schema.security.secrets.SecretsManagerProvider.MANAGED_AWS_SSM; import com.google.common.annotations.VisibleForTesting; -import org.openmetadata.schema.security.secrets.SecretsManagerConfiguration; import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.ssm.SsmClient; @@ -29,8 +28,8 @@ public class AWSSSMSecretsManager extends AWSBasedSecretsManager { private static AWSSSMSecretsManager instance = null; private SsmClient ssmClient; - private AWSSSMSecretsManager(SecretsManagerConfiguration config, SecretsConfig secretsConfig) { - super(MANAGED_AWS_SSM, config, secretsConfig); + private AWSSSMSecretsManager(SecretsConfig secretsConfig) { + super(MANAGED_AWS_SSM, secretsConfig); } @Override @@ -82,8 +81,8 @@ protected void deleteSecretInternal(String secretName) { this.ssmClient.deleteParameter(deleteParameterRequest); } - public static AWSSSMSecretsManager getInstance(SecretsManagerConfiguration config, SecretsConfig secretsConfig) { - if (instance == null) instance = new AWSSSMSecretsManager(config, secretsConfig); + public static AWSSSMSecretsManager getInstance(SecretsConfig secretsConfig) { + if (instance == null) instance = new AWSSSMSecretsManager(secretsConfig); return instance; } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSecretsManager.java index 288d340786f9..fbac45579ea7 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/AWSSecretsManager.java @@ -18,7 +18,6 @@ import com.google.common.annotations.VisibleForTesting; import java.util.Objects; import lombok.extern.slf4j.Slf4j; -import org.openmetadata.schema.security.secrets.SecretsManagerConfiguration; import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.secretsmanager.SecretsManagerClient; @@ -33,8 +32,8 @@ public class AWSSecretsManager extends AWSBasedSecretsManager { private static AWSSecretsManager instance = null; private SecretsManagerClient secretsClient; - private AWSSecretsManager(SecretsManagerConfiguration config, SecretsConfig secretsConfig) { - super(MANAGED_AWS, config, secretsConfig); + private AWSSecretsManager(SecretsConfig secretsConfig) { + super(MANAGED_AWS, secretsConfig); } @Override @@ -86,9 +85,9 @@ protected void deleteSecretInternal(String secretName) { this.secretsClient.deleteSecret(deleteSecretRequest); } - public static AWSSecretsManager getInstance(SecretsManagerConfiguration config, SecretsConfig secretsConfig) { + public static AWSSecretsManager getInstance(SecretsConfig secretsConfig) { if (instance == null) { - instance = new AWSSecretsManager(config, secretsConfig); + instance = new AWSSecretsManager(secretsConfig); } return instance; } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/NoopSecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/NoopSecretsManager.java index 75177803f7c5..72c67c71aa6d 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/NoopSecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/NoopSecretsManager.java @@ -22,10 +22,9 @@ private NoopSecretsManager(SecretsManagerProvider secretsManagerProvider, Secret super(secretsManagerProvider, secretsConfig); } - public static NoopSecretsManager getInstance( - SecretsManagerProvider secretsManagerProvider, SecretsConfig secretsConfig) { + public static NoopSecretsManager getInstance(SecretsConfig secretsConfig) { if (instance == null) { - instance = new NoopSecretsManager(secretsManagerProvider, secretsConfig); + instance = new NoopSecretsManager(SecretsManagerProvider.NOOP, secretsConfig); } return instance; } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java index 79e82f8b2fe1..28298074d7af 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java @@ -18,25 +18,26 @@ import com.google.common.annotations.VisibleForTesting; import java.lang.reflect.Method; import java.util.Arrays; +import java.util.HashMap; import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Set; import java.util.regex.Pattern; -import java.util.stream.Collectors; import javax.ws.rs.core.Response; import lombok.Getter; import lombok.extern.slf4j.Slf4j; import org.openmetadata.annotations.PasswordField; +import org.openmetadata.common.utils.CommonUtil; import org.openmetadata.schema.auth.BasicAuthMechanism; import org.openmetadata.schema.entity.automations.Workflow; import org.openmetadata.schema.entity.services.ServiceType; import org.openmetadata.schema.entity.services.ingestionPipelines.IngestionPipeline; import org.openmetadata.schema.entity.teams.AuthenticationMechanism; import org.openmetadata.schema.security.client.OpenMetadataJWTClientConfig; +import org.openmetadata.schema.security.secrets.Parameters; import org.openmetadata.schema.security.secrets.SecretsManagerProvider; import org.openmetadata.schema.services.connections.metadata.OpenMetadataConnection; -import org.openmetadata.service.exception.CustomExceptionMessage; import org.openmetadata.service.exception.InvalidServiceConnectionException; import org.openmetadata.service.exception.SecretsManagerException; import org.openmetadata.service.fernet.Fernet; @@ -47,7 +48,7 @@ @Slf4j public abstract class SecretsManager { - public record SecretsConfig(String clusterName, String prefix, List tags) {} + public record SecretsConfig(String clusterName, String prefix, List tags, Parameters parameters) {} @Getter private final SecretsConfig secretsConfig; @Getter private final SecretsManagerProvider secretsManagerProvider; @@ -101,7 +102,7 @@ public void encryptAuthenticationMechanism(String name, AuthenticationMechanism try { encryptPasswordFields(authenticationMechanism, buildSecretId(true, "bot", name), true); } catch (Exception e) { - throw new CustomExceptionMessage( + throw new SecretsManagerException( Response.Status.BAD_REQUEST, String.format("Failed to encrypt user bot instance [%s]", name)); } } @@ -113,7 +114,7 @@ public void decryptAuthenticationMechanism(String name, AuthenticationMechanism try { decryptPasswordFields(authenticationMechanism); } catch (Exception e) { - throw new CustomExceptionMessage( + throw new SecretsManagerException( Response.Status.BAD_REQUEST, String.format("Failed to decrypt user bot instance [%s]", name)); } } @@ -128,7 +129,7 @@ public void encryptIngestionPipeline(IngestionPipeline ingestionPipeline) { try { encryptPasswordFields(ingestionPipeline, buildSecretId(true, "pipeline", ingestionPipeline.getName()), true); } catch (Exception e) { - throw new CustomExceptionMessage( + throw new SecretsManagerException( Response.Status.BAD_REQUEST, String.format("Failed to encrypt ingestion pipeline instance [%s]", ingestionPipeline.getName())); } @@ -144,7 +145,7 @@ public void decryptIngestionPipeline(IngestionPipeline ingestionPipeline) { try { decryptPasswordFields(ingestionPipeline); } catch (Exception e) { - throw new CustomExceptionMessage( + throw new SecretsManagerException( Response.Status.BAD_REQUEST, String.format("Failed to decrypt ingestion pipeline instance [%s]", ingestionPipeline.getName())); } @@ -160,7 +161,7 @@ public Workflow encryptWorkflow(Workflow workflow) { try { encryptPasswordFields(workflowConverted, buildSecretId(true, "workflow", workflow.getName()), true); } catch (Exception e) { - throw new CustomExceptionMessage( + throw new SecretsManagerException( Response.Status.BAD_REQUEST, String.format("Failed to encrypt workflow instance [%s]", workflow.getName())); } workflowConverted.setOpenMetadataServerConnection(openMetadataConnection); @@ -176,7 +177,7 @@ public Workflow decryptWorkflow(Workflow workflow) { try { decryptPasswordFields(workflowConverted); } catch (Exception e) { - throw new CustomExceptionMessage( + throw new SecretsManagerException( Response.Status.BAD_REQUEST, String.format("Failed to decrypt workflow instance [%s]", workflow.getName())); } workflowConverted.setOpenMetadataServerConnection(openMetadataConnection); @@ -192,7 +193,7 @@ public OpenMetadataConnection encryptOpenMetadataConnection( try { encryptPasswordFields(openMetadataConnectionConverted, buildSecretId(true, "serverconnection"), store); } catch (Exception e) { - throw new CustomExceptionMessage( + throw new SecretsManagerException( Response.Status.BAD_REQUEST, "Failed to encrypt OpenMetadataConnection instance."); } return openMetadataConnectionConverted; @@ -208,7 +209,7 @@ public OpenMetadataConnection decryptOpenMetadataConnection(OpenMetadataConnecti try { decryptPasswordFields(openMetadataConnectionConverted); } catch (Exception e) { - throw new CustomExceptionMessage( + throw new SecretsManagerException( Response.Status.BAD_REQUEST, "Failed to decrypt OpenMetadataConnection instance."); } return openMetadataConnectionConverted; @@ -227,7 +228,7 @@ private Object encryptPasswordFields(Object toEncryptObject, String secretId, bo Object obj = ReflectionUtil.getObjectFromMethod(method, toEncryptObject); String fieldName = method.getName().replaceFirst("get", ""); // if the object matches the package of openmetadata - if (obj != null && obj.getClass().getPackageName().startsWith("org.openmetadata")) { + if (Boolean.TRUE.equals(CommonUtil.isOpenMetadataObject(obj))) { // encryptPasswordFields encryptPasswordFields( obj, buildSecretId(false, secretId, fieldName.toLowerCase(Locale.ROOT)), store); @@ -265,7 +266,7 @@ private Object decryptPasswordFields(Object toDecryptObject) { Object obj = ReflectionUtil.getObjectFromMethod(method, toDecryptObject); String fieldName = method.getName().replaceFirst("get", ""); // if the object matches the package of openmetadata - if (obj != null && obj.getClass().getPackageName().startsWith("org.openmetadata")) { + if (Boolean.TRUE.equals(CommonUtil.isOpenMetadataObject(obj))) { // encryptPasswordFields decryptPasswordFields(obj); // check if it has annotation @@ -347,7 +348,7 @@ public void deleteSecretsFromWorkflow(Workflow workflow) { try { deleteSecrets(workflowConverted, buildSecretId(true, "workflow", workflow.getName())); } catch (Exception e) { - throw new CustomExceptionMessage( + throw new SecretsManagerException( Response.Status.BAD_REQUEST, String.format("Failed to delete secrets from workflow instance [%s]", workflow.getName())); } @@ -364,7 +365,7 @@ private void deleteSecrets(Object toDeleteSecretsFrom, String secretId) { // check if it has annotation: // We are replicating the logic that we use for storing the fields we need to encrypt // at encryptPasswordFields - if (obj != null && obj.getClass().getPackageName().startsWith("org.openmetadata")) { + if (Boolean.TRUE.equals(CommonUtil.isOpenMetadataObject(obj))) { deleteSecrets(obj, buildSecretId(false, secretId, fieldName.toLowerCase(Locale.ROOT))); } else if (obj != null && method.getAnnotation(PasswordField.class) != null) { deleteSecretInternal(buildSecretId(false, secretId, fieldName.toLowerCase(Locale.ROOT))); @@ -374,11 +375,17 @@ private void deleteSecrets(Object toDeleteSecretsFrom, String secretId) { } public static Map getTags(SecretsConfig secretsConfig) { - try { - return secretsConfig.tags.stream().collect(Collectors.toMap(s -> s.split(":")[0], s -> s.split(":")[1])); - } catch (Exception e) { - LOG.error(String.format("The SecretsConfig could not extract tags from [%s]", secretsConfig.tags)); - return Map.of(); - } + Map tags = new HashMap<>(); + secretsConfig.tags.forEach( + keyValue -> { + try { + tags.put(keyValue.split(":")[0], keyValue.split(":")[1]); + } catch (Exception e) { + LOG.error( + String.format( + "The SecretsConfig could not extract tag from [%s] due to [%s]", keyValue, e.getMessage())); + } + }); + return tags; } } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManagerFactory.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManagerFactory.java index 3560b55a565f..1d0179944d8e 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManagerFactory.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManagerFactory.java @@ -29,9 +29,10 @@ public static SecretsManager createSecretsManager(SecretsManagerConfiguration co return secretsManager; } SecretsManager.SecretsConfig secretsConfig = - new SecretsManager.SecretsConfig(clusterName, config.getPrefix(), config.getTags()); + new SecretsManager.SecretsConfig(clusterName, config.getPrefix(), config.getTags(), config.getParameters()); SecretsManagerProvider secretsManagerProvider = config != null && config.getSecretsManager() != null ? config.getSecretsManager() : SecretsManagerProvider.NOOP; + switch (secretsManagerProvider) { case NOOP: case AWS_SSM: @@ -47,19 +48,18 @@ If for example we want to set the AWS SSM (non-managed) we configure to connect to AWS SSM as specified in the docs: https://docs.open-metadata.org/deployment/secrets-manager/supported-implementations/aws-ssm-parameter-store */ - secretsManager = NoopSecretsManager.getInstance(secretsManagerProvider, secretsConfig); + secretsManager = NoopSecretsManager.getInstance(secretsConfig); break; case MANAGED_AWS: - secretsManager = AWSSecretsManager.getInstance(config, secretsConfig); + secretsManager = AWSSecretsManager.getInstance(secretsConfig); break; case MANAGED_AWS_SSM: - secretsManager = AWSSSMSecretsManager.getInstance(config, secretsConfig); + secretsManager = AWSSSMSecretsManager.getInstance(secretsConfig); break; case IN_MEMORY: secretsManager = InMemorySecretsManager.getInstance(secretsConfig); break; default: - throw new IllegalArgumentException("Not implemented secret manager store: " + secretsManagerProvider); } return secretsManager; } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java index 63e58bc93fb6..fbb5f262131d 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java @@ -38,7 +38,7 @@ public class SecretsManagerLifecycleTest { static void setUp() { secretsManager = InMemorySecretsManager.getInstance( - new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key1:value1", "key2:value2"))); + new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key1:value1", "key2:value2"), null)); Fernet fernet = Mockito.mock(Fernet.class); lenient().when(fernet.decrypt(anyString())).thenReturn(DECRYPTED_VALUE); lenient().when(fernet.decryptIfApplies(anyString())).thenReturn(DECRYPTED_VALUE); @@ -150,7 +150,7 @@ void test_getTags() { // if the tags are not well formatted, we don't return anything SecretsManager.SecretsConfig secretsConfig = - new SecretsManager.SecretsConfig(null, null, List.of("key:value", "random")); - assertEquals(Map.of(), SecretsManager.getTags(secretsConfig)); + new SecretsManager.SecretsConfig(null, null, List.of("random", "key:value", "random"), null); + assertEquals(Map.of("key", "value"), SecretsManager.getTags(secretsConfig)); } } From a7ea9990e1507dab61a23de5e546d5dd9cc7da78 Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Thu, 14 Dec 2023 10:02:20 +0100 Subject: [PATCH 07/15] Rename noop to db secrets manager providee --- .../migrations/native/1.3.0/mysql/schemaChanges.sql | 6 ++++++ .../native/1.3.0/postgres/schemaChanges.sql | 11 +++++++++++ conf/openmetadata.yaml | 2 +- ...NoopSecretsManager.java => DBSecretsManager.java} | 10 +++++----- .../service/secrets/SecretsManagerFactory.java | 6 +++--- ...etsManagerTest.java => DBSecretsManagerTest.java} | 11 +++++------ .../service/secrets/SecretsManagerFactoryTest.java | 12 ++++++------ .../connections/metadata/openMetadataConnection.json | 2 +- .../security/secrets/secretsManagerProvider.json | 4 ++-- 9 files changed, 40 insertions(+), 24 deletions(-) rename openmetadata-service/src/main/java/org/openmetadata/service/secrets/{NoopSecretsManager.java => DBSecretsManager.java} (74%) rename openmetadata-service/src/test/java/org/openmetadata/service/secrets/{NoopSecretsManagerTest.java => DBSecretsManagerTest.java} (93%) diff --git a/bootstrap/sql/migrations/native/1.3.0/mysql/schemaChanges.sql b/bootstrap/sql/migrations/native/1.3.0/mysql/schemaChanges.sql index 386f705ecafa..0b6e2c6dea76 100644 --- a/bootstrap/sql/migrations/native/1.3.0/mysql/schemaChanges.sql +++ b/bootstrap/sql/migrations/native/1.3.0/mysql/schemaChanges.sql @@ -31,3 +31,9 @@ set json = JSON_INSERT( false ) where name = 'DataInsightsApplication'; + +-- Rename NOOP Secret Manager to DB +update metadata_service_entity +set json = JSON_REPLACE(json, '$.connection.config.secretsManagerProvider', 'db') +where name = 'OpenMetadata' + and JSON_EXTRACT(json, '$.connection.config.secretsManagerProvider') = 'noop'; \ No newline at end of file diff --git a/bootstrap/sql/migrations/native/1.3.0/postgres/schemaChanges.sql b/bootstrap/sql/migrations/native/1.3.0/postgres/schemaChanges.sql index dd9d8d2c3ab1..f8b25af25d42 100644 --- a/bootstrap/sql/migrations/native/1.3.0/postgres/schemaChanges.sql +++ b/bootstrap/sql/migrations/native/1.3.0/postgres/schemaChanges.sql @@ -38,3 +38,14 @@ SET json = jsonb_set( to_jsonb(false) ) where name = 'DataInsightsApplication'; + +-- Rename NOOP Secret Manager to DB +update metadata_service_entity +set json = jsonb_set( + json #- '{connection,config,secretsManagerProvider}', + '{connection,config,secretsManagerProvider}', + '"db"', + true +) +where name = 'OpenMetadata' + and json #>> '{connection,config,secretsManagerProvider}' = 'noop'; diff --git a/conf/openmetadata.yaml b/conf/openmetadata.yaml index 59f5d0e27057..921bc880f426 100644 --- a/conf/openmetadata.yaml +++ b/conf/openmetadata.yaml @@ -278,7 +278,7 @@ fernetConfiguration: fernetKey: ${FERNET_KEY:-jJ/9sz0g0OHxsfxOoSfdFdmk3ysNmPRnH3TUAbz3IHA=} secretsManagerConfiguration: - secretsManager: ${SECRET_MANAGER:-noop} # Possible values are "noop", "aws", "aws-ssm" + secretsManager: ${SECRET_MANAGER:-db} # Possible values are "db", "managed-aws", "managed-aws-ssm" prefix: ${SECRET_MANAGER_PREFIX:-""} # Define the secret key ID as /// tags: ${SECRET_MANAGER_TAGS:-[]} # Add tags to the created resource, e.g., in AWS. Format is `[key1:value1,key2:value2,...]` # it will use the default auth provider for the secrets' manager service if parameters are not set diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/NoopSecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/DBSecretsManager.java similarity index 74% rename from openmetadata-service/src/main/java/org/openmetadata/service/secrets/NoopSecretsManager.java rename to openmetadata-service/src/main/java/org/openmetadata/service/secrets/DBSecretsManager.java index 72c67c71aa6d..265508c7741d 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/NoopSecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/DBSecretsManager.java @@ -15,16 +15,16 @@ import org.openmetadata.schema.security.secrets.SecretsManagerProvider; -public class NoopSecretsManager extends SecretsManager { - private static NoopSecretsManager instance; +public class DBSecretsManager extends SecretsManager { + private static DBSecretsManager instance; - private NoopSecretsManager(SecretsManagerProvider secretsManagerProvider, SecretsConfig secretsConfig) { + private DBSecretsManager(SecretsManagerProvider secretsManagerProvider, SecretsConfig secretsConfig) { super(secretsManagerProvider, secretsConfig); } - public static NoopSecretsManager getInstance(SecretsConfig secretsConfig) { + public static DBSecretsManager getInstance(SecretsConfig secretsConfig) { if (instance == null) { - instance = new NoopSecretsManager(SecretsManagerProvider.NOOP, secretsConfig); + instance = new DBSecretsManager(SecretsManagerProvider.DB, secretsConfig); } return instance; } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManagerFactory.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManagerFactory.java index 1d0179944d8e..3553d11dba6d 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManagerFactory.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManagerFactory.java @@ -31,10 +31,10 @@ public static SecretsManager createSecretsManager(SecretsManagerConfiguration co SecretsManager.SecretsConfig secretsConfig = new SecretsManager.SecretsConfig(clusterName, config.getPrefix(), config.getTags(), config.getParameters()); SecretsManagerProvider secretsManagerProvider = - config != null && config.getSecretsManager() != null ? config.getSecretsManager() : SecretsManagerProvider.NOOP; + config != null && config.getSecretsManager() != null ? config.getSecretsManager() : SecretsManagerProvider.DB; switch (secretsManagerProvider) { - case NOOP: + case DB: case AWS_SSM: case AWS: /* @@ -48,7 +48,7 @@ If for example we want to set the AWS SSM (non-managed) we configure to connect to AWS SSM as specified in the docs: https://docs.open-metadata.org/deployment/secrets-manager/supported-implementations/aws-ssm-parameter-store */ - secretsManager = NoopSecretsManager.getInstance(secretsConfig); + secretsManager = DBSecretsManager.getInstance(secretsConfig); break; case MANAGED_AWS: secretsManager = AWSSecretsManager.getInstance(secretsConfig); diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/NoopSecretsManagerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/DBSecretsManagerTest.java similarity index 93% rename from openmetadata-service/src/test/java/org/openmetadata/service/secrets/NoopSecretsManagerTest.java rename to openmetadata-service/src/test/java/org/openmetadata/service/secrets/DBSecretsManagerTest.java index 6d7b51ec5e06..814fde24b657 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/NoopSecretsManagerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/DBSecretsManagerTest.java @@ -36,18 +36,17 @@ import org.openmetadata.service.util.JsonUtils; @ExtendWith(MockitoExtension.class) -public class NoopSecretsManagerTest { +public class DBSecretsManagerTest { private static final String ENCRYPTED_VALUE = "fernet:abcdef"; private static final String DECRYPTED_VALUE = "123456"; - private static NoopSecretsManager secretsManager; + private static DBSecretsManager secretsManager; @BeforeAll static void setUp() { secretsManager = - NoopSecretsManager.getInstance( - SecretsManagerProvider.NOOP, - new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"))); + DBSecretsManager.getInstance( + new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"), null)); Fernet fernet = Mockito.mock(Fernet.class); lenient().when(fernet.decrypt(anyString())).thenReturn(DECRYPTED_VALUE); lenient().when(fernet.decryptIfApplies(anyString())).thenReturn(DECRYPTED_VALUE); @@ -87,7 +86,7 @@ void testDecryptServiceConnectionWithoutPassword() { @Test void testReturnsExpectedSecretManagerProvider() { - assertEquals(SecretsManagerProvider.NOOP, secretsManager.getSecretsManagerProvider()); + assertEquals(SecretsManagerProvider.DB, secretsManager.getSecretsManagerProvider()); } private void testEncryptServiceConnection() { diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerFactoryTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerFactoryTest.java index d4673ea070a6..1d6f6512ddb1 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerFactoryTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerFactoryTest.java @@ -34,24 +34,24 @@ void setUp() { @Test void testDefaultIsCreatedIfNullConfig() { - assertTrue(SecretsManagerFactory.createSecretsManager(config, CLUSTER_NAME) instanceof NoopSecretsManager); + assertTrue(SecretsManagerFactory.createSecretsManager(config, CLUSTER_NAME) instanceof DBSecretsManager); } @Test void testDefaultIsCreatedIfMissingSecretManager() { - assertTrue(SecretsManagerFactory.createSecretsManager(config, CLUSTER_NAME) instanceof NoopSecretsManager); + assertTrue(SecretsManagerFactory.createSecretsManager(config, CLUSTER_NAME) instanceof DBSecretsManager); } @Test void testIsCreatedIfLocalSecretsManager() { - config.setSecretsManager(SecretsManagerProvider.NOOP); - assertTrue(SecretsManagerFactory.createSecretsManager(config, CLUSTER_NAME) instanceof NoopSecretsManager); + config.setSecretsManager(SecretsManagerProvider.DB); + assertTrue(SecretsManagerFactory.createSecretsManager(config, CLUSTER_NAME) instanceof DBSecretsManager); } @Test void testIsCreatedIfAWSSecretsManager() { initConfigForAWSBasedSecretManager(SecretsManagerProvider.AWS); - assertTrue(SecretsManagerFactory.createSecretsManager(config, CLUSTER_NAME) instanceof NoopSecretsManager); + assertTrue(SecretsManagerFactory.createSecretsManager(config, CLUSTER_NAME) instanceof DBSecretsManager); } @Test @@ -63,7 +63,7 @@ void testIsCreatedIfManagedAWSSecretsManager() { @Test void testIsCreatedIfAWSSSMSecretsManager() { initConfigForAWSBasedSecretManager(SecretsManagerProvider.AWS_SSM); - assertTrue(SecretsManagerFactory.createSecretsManager(config, CLUSTER_NAME) instanceof NoopSecretsManager); + assertTrue(SecretsManagerFactory.createSecretsManager(config, CLUSTER_NAME) instanceof DBSecretsManager); } @Test diff --git a/openmetadata-spec/src/main/resources/json/schema/entity/services/connections/metadata/openMetadataConnection.json b/openmetadata-spec/src/main/resources/json/schema/entity/services/connections/metadata/openMetadataConnection.json index ba095f5c2773..81105859969e 100644 --- a/openmetadata-spec/src/main/resources/json/schema/entity/services/connections/metadata/openMetadataConnection.json +++ b/openmetadata-spec/src/main/resources/json/schema/entity/services/connections/metadata/openMetadataConnection.json @@ -91,7 +91,7 @@ }, "secretsManagerProvider": { "$ref": "./../../../../security/secrets/secretsManagerProvider.json", - "default": "noop" + "default": "db" }, "secretsManagerLoader": { "$ref": "./../../../../security/secrets/secretsManagerClientLoader.json", diff --git a/openmetadata-spec/src/main/resources/json/schema/security/secrets/secretsManagerProvider.json b/openmetadata-spec/src/main/resources/json/schema/security/secrets/secretsManagerProvider.json index a323a665615b..6180c4fbab7f 100644 --- a/openmetadata-spec/src/main/resources/json/schema/security/secrets/secretsManagerProvider.json +++ b/openmetadata-spec/src/main/resources/json/schema/security/secrets/secretsManagerProvider.json @@ -5,7 +5,7 @@ "description": "OpenMetadata Secrets Manager Provider. Make sure to configure the same secrets manager providers as the ones configured on the OpenMetadata server.", "type": "string", "javaType": "org.openmetadata.schema.security.secrets.SecretsManagerProvider", - "enum": ["noop", "managed-aws","aws", "managed-aws-ssm", "aws-ssm", "in-memory"], - "default": "noop", + "enum": ["db", "managed-aws","aws", "managed-aws-ssm", "aws-ssm", "in-memory"], + "default": "db", "additionalProperties": false } From 0bc7bc4dd5afb69abec516d7879d9630c7fea1d9 Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Thu, 14 Dec 2023 14:11:57 +0100 Subject: [PATCH 08/15] Update sm --- .../src/metadata/utils/secrets/noop_secrets_manager.py | 7 ++++--- .../src/metadata/utils/secrets/secrets_manager_factory.py | 6 +++--- .../tests/integration/ometa/test_ometa_secrets_manager.py | 6 +++--- .../metadata/utils/secrets/test_secrets_manager_factory.py | 4 ++-- .../service/secrets/AWSSSMSecretsManagerTest.java | 3 ++- .../service/secrets/AWSSecretsManagerTest.java | 3 ++- 6 files changed, 16 insertions(+), 13 deletions(-) diff --git a/ingestion/src/metadata/utils/secrets/noop_secrets_manager.py b/ingestion/src/metadata/utils/secrets/noop_secrets_manager.py index 0d9f708edf87..8ee38acd66ae 100644 --- a/ingestion/src/metadata/utils/secrets/noop_secrets_manager.py +++ b/ingestion/src/metadata/utils/secrets/noop_secrets_manager.py @@ -18,12 +18,13 @@ from metadata.utils.secrets.secrets_manager import SecretsManager -class NoopSecretsManager(SecretsManager): +class DBSecretsManager(SecretsManager): """ - LocalSecretsManager is used when there is not a secrets' manager configured. + Will return the string as received. The client does not need to pick + it up from any external resource. """ - provider: str = SecretsManagerProvider.noop.name + provider: str = SecretsManagerProvider.db.name def get_string_value(self, secret_id: str) -> str: return secret_id diff --git a/ingestion/src/metadata/utils/secrets/secrets_manager_factory.py b/ingestion/src/metadata/utils/secrets/secrets_manager_factory.py index ab18b2fd69f2..643e99594bcd 100644 --- a/ingestion/src/metadata/utils/secrets/secrets_manager_factory.py +++ b/ingestion/src/metadata/utils/secrets/secrets_manager_factory.py @@ -23,7 +23,7 @@ from metadata.utils.secrets.aws_secrets_manager import AWSSecretsManager from metadata.utils.secrets.aws_ssm_secrets_manager import AWSSSMSecretsManager from metadata.utils.secrets.client.loader import secrets_manager_client_loader -from metadata.utils.secrets.noop_secrets_manager import NoopSecretsManager +from metadata.utils.secrets.noop_secrets_manager import DBSecretsManager from metadata.utils.secrets.secrets_manager import SecretsManager from metadata.utils.singleton import Singleton @@ -85,9 +85,9 @@ def _get_secrets_manager( """ if ( secrets_manager_provider is None - or secrets_manager_provider == SecretsManagerProvider.noop + or secrets_manager_provider == SecretsManagerProvider.db ): - return NoopSecretsManager() + return DBSecretsManager() if secrets_manager_provider in ( SecretsManagerProvider.aws, SecretsManagerProvider.managed_aws, diff --git a/ingestion/tests/integration/ometa/test_ometa_secrets_manager.py b/ingestion/tests/integration/ometa/test_ometa_secrets_manager.py index 5f3086198a94..c093920bb637 100644 --- a/ingestion/tests/integration/ometa/test_ometa_secrets_manager.py +++ b/ingestion/tests/integration/ometa/test_ometa_secrets_manager.py @@ -30,7 +30,7 @@ ) from metadata.ingestion.ometa.ometa_api import OpenMetadata from metadata.utils.secrets.aws_secrets_manager import AWSSecretsManager -from metadata.utils.secrets.noop_secrets_manager import NoopSecretsManager +from metadata.utils.secrets.noop_secrets_manager import DBSecretsManager from metadata.utils.singleton import Singleton @@ -55,7 +55,7 @@ def setUp(cls) -> None: def test_ometa_with_local_secret_manager(self): self._init_local_secret_manager() - assert type(self.metadata.secrets_manager_client) is NoopSecretsManager + assert type(self.metadata.secrets_manager_client) is DBSecretsManager assert type(self.metadata._auth_provider) is NoOpAuthenticationProvider def test_ometa_with_local_secret_manager_with_google_auth(self): @@ -64,7 +64,7 @@ def test_ometa_with_local_secret_manager_with_google_auth(self): secretKey="/fake/path" ) self._init_local_secret_manager() - assert type(self.metadata.secrets_manager_client) is NoopSecretsManager + assert type(self.metadata.secrets_manager_client) is DBSecretsManager assert type(self.metadata._auth_provider) is GoogleAuthenticationProvider @mock.patch.dict(os.environ, {"AWS_DEFAULT_REGION": "us-east-2"}, clear=True) diff --git a/ingestion/tests/unit/metadata/utils/secrets/test_secrets_manager_factory.py b/ingestion/tests/unit/metadata/utils/secrets/test_secrets_manager_factory.py index fb57551807b6..7a85887a7954 100644 --- a/ingestion/tests/unit/metadata/utils/secrets/test_secrets_manager_factory.py +++ b/ingestion/tests/unit/metadata/utils/secrets/test_secrets_manager_factory.py @@ -24,7 +24,7 @@ from metadata.generated.schema.security.secrets.secretsManagerProvider import ( SecretsManagerProvider, ) -from metadata.utils.secrets.noop_secrets_manager import NoopSecretsManager +from metadata.utils.secrets.noop_secrets_manager import DBSecretsManager from metadata.utils.secrets.secrets_manager_factory import ( SecretsManagerConfigException, SecretsManagerFactory, @@ -77,7 +77,7 @@ def test_get_none_secret_manager(self): ) assert secrets_manager_factory.get_secrets_manager() is not None assert isinstance( - secrets_manager_factory.get_secrets_manager(), NoopSecretsManager + secrets_manager_factory.get_secrets_manager(), DBSecretsManager ) @patch("metadata.clients.aws_client.boto3") diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSSMSecretsManagerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSSMSecretsManagerTest.java index ec540a1e6e96..d21269c361d2 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSSMSecretsManagerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSSMSecretsManagerTest.java @@ -28,7 +28,8 @@ public class AWSSSMSecretsManagerTest extends ExternalSecretsManagerTest { void setUpSpecific(SecretsManagerConfiguration config) { secretsManager = AWSSSMSecretsManager.getInstance( - config, new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"))); + config, + new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"), null)); ((AWSSSMSecretsManager) secretsManager).setSsmClient(ssmClient); reset(ssmClient); } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSecretsManagerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSecretsManagerTest.java index 62453463dfc9..127df5429fe5 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSecretsManagerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSecretsManagerTest.java @@ -28,7 +28,8 @@ public class AWSSecretsManagerTest extends ExternalSecretsManagerTest { void setUpSpecific(SecretsManagerConfiguration config) { secretsManager = AWSSecretsManager.getInstance( - config, new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"))); + config, + new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"), null)); ((AWSSecretsManager) secretsManager).setSecretsClient(secretsManagerClient); reset(secretsManagerClient); } From b48234eee3399c1c1da9c4fa318e5ecf0ae0dec0 Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Thu, 14 Dec 2023 14:18:37 +0100 Subject: [PATCH 09/15] Fix --- .../openmetadata/service/secrets/AWSSSMSecretsManagerTest.java | 1 - .../org/openmetadata/service/secrets/AWSSecretsManagerTest.java | 1 - 2 files changed, 2 deletions(-) diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSSMSecretsManagerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSSMSecretsManagerTest.java index d21269c361d2..f3fcac3ef665 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSSMSecretsManagerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSSMSecretsManagerTest.java @@ -28,7 +28,6 @@ public class AWSSSMSecretsManagerTest extends ExternalSecretsManagerTest { void setUpSpecific(SecretsManagerConfiguration config) { secretsManager = AWSSSMSecretsManager.getInstance( - config, new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"), null)); ((AWSSSMSecretsManager) secretsManager).setSsmClient(ssmClient); reset(ssmClient); diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSecretsManagerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSecretsManagerTest.java index 127df5429fe5..d9acdc155c09 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSecretsManagerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/AWSSecretsManagerTest.java @@ -28,7 +28,6 @@ public class AWSSecretsManagerTest extends ExternalSecretsManagerTest { void setUpSpecific(SecretsManagerConfiguration config) { secretsManager = AWSSecretsManager.getInstance( - config, new SecretsManager.SecretsConfig("openmetadata", "prefix", List.of("key:value", "key2:value2"), null)); ((AWSSecretsManager) secretsManager).setSecretsClient(secretsManagerClient); reset(secretsManagerClient); From 6914420444b3d4c850889ab8cc2d4a86726e7f87 Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Thu, 14 Dec 2023 14:43:32 +0100 Subject: [PATCH 10/15] db SM --- .../src/test/resources/openmetadata-secure-test.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openmetadata-service/src/test/resources/openmetadata-secure-test.yaml b/openmetadata-service/src/test/resources/openmetadata-secure-test.yaml index f1ebff67803e..d8a9cb81d984 100644 --- a/openmetadata-service/src/test/resources/openmetadata-secure-test.yaml +++ b/openmetadata-service/src/test/resources/openmetadata-secure-test.yaml @@ -112,7 +112,7 @@ migrationConfiguration: # port: 0 secretsManagerConfiguration: - secretsManager: noop + secretsManager: db health: From 0b59463eb6a59bfe5edd16d32c92374f544661a0 Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Thu, 14 Dec 2023 15:24:02 +0100 Subject: [PATCH 11/15] db SM --- docker/development/docker-compose-postgres.yml | 4 ++-- docker/development/docker-compose.yml | 4 ++-- .../docker-compose-openmetadata.yml | 4 ++-- docker/docker-compose-quickstart/docker-compose-postgres.yml | 4 ++-- docker/docker-compose-quickstart/docker-compose.yml | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/docker/development/docker-compose-postgres.yml b/docker/development/docker-compose-postgres.yml index 4aa7d14e9b9f..fb16769e5b2f 100644 --- a/docker/development/docker-compose-postgres.yml +++ b/docker/development/docker-compose-postgres.yml @@ -149,7 +149,7 @@ services: FERNET_KEY: ${FERNET_KEY:-jJ/9sz0g0OHxsfxOoSfdFdmk3ysNmPRnH3TUAbz3IHA=} #secretsManagerConfiguration - SECRET_MANAGER: ${SECRET_MANAGER:-noop} + SECRET_MANAGER: ${SECRET_MANAGER:-db} #parameters: OM_SM_REGION: ${OM_SM_REGION:-""} OM_SM_ACCESS_KEY_ID: ${OM_SM_ACCESS_KEY_ID:-""} @@ -295,7 +295,7 @@ services: FERNET_KEY: ${FERNET_KEY:-jJ/9sz0g0OHxsfxOoSfdFdmk3ysNmPRnH3TUAbz3IHA=} #secretsManagerConfiguration - SECRET_MANAGER: ${SECRET_MANAGER:-noop} + SECRET_MANAGER: ${SECRET_MANAGER:-db} #parameters: OM_SM_REGION: ${OM_SM_REGION:-""} OM_SM_ACCESS_KEY_ID: ${OM_SM_ACCESS_KEY_ID:-""} diff --git a/docker/development/docker-compose.yml b/docker/development/docker-compose.yml index 4cc1e7e98752..0011fa2b841e 100644 --- a/docker/development/docker-compose.yml +++ b/docker/development/docker-compose.yml @@ -148,7 +148,7 @@ services: FERNET_KEY: ${FERNET_KEY:-jJ/9sz0g0OHxsfxOoSfdFdmk3ysNmPRnH3TUAbz3IHA=} #secretsManagerConfiguration - SECRET_MANAGER: ${SECRET_MANAGER:-noop} + SECRET_MANAGER: ${SECRET_MANAGER:-db} #parameters: OM_SM_REGION: ${OM_SM_REGION:-""} OM_SM_ACCESS_KEY_ID: ${OM_SM_ACCESS_KEY_ID:-""} @@ -296,7 +296,7 @@ services: FERNET_KEY: ${FERNET_KEY:-jJ/9sz0g0OHxsfxOoSfdFdmk3ysNmPRnH3TUAbz3IHA=} #secretsManagerConfiguration - SECRET_MANAGER: ${SECRET_MANAGER:-noop} + SECRET_MANAGER: ${SECRET_MANAGER:-db} #parameters: OM_SM_REGION: ${OM_SM_REGION:-""} OM_SM_ACCESS_KEY_ID: ${OM_SM_ACCESS_KEY_ID:-""} diff --git a/docker/docker-compose-openmetadata/docker-compose-openmetadata.yml b/docker/docker-compose-openmetadata/docker-compose-openmetadata.yml index 2b6858cc126a..905fa21aba50 100644 --- a/docker/docker-compose-openmetadata/docker-compose-openmetadata.yml +++ b/docker/docker-compose-openmetadata/docker-compose-openmetadata.yml @@ -98,7 +98,7 @@ services: FERNET_KEY: ${FERNET_KEY:-jJ/9sz0g0OHxsfxOoSfdFdmk3ysNmPRnH3TUAbz3IHA=} #secretsManagerConfiguration - SECRET_MANAGER: ${SECRET_MANAGER:-noop} + SECRET_MANAGER: ${SECRET_MANAGER:-db} #parameters: OM_SM_REGION: ${OM_SM_REGION:-""} OM_SM_ACCESS_KEY_ID: ${OM_SM_ACCESS_KEY_ID:-""} @@ -240,7 +240,7 @@ services: FERNET_KEY: ${FERNET_KEY:-jJ/9sz0g0OHxsfxOoSfdFdmk3ysNmPRnH3TUAbz3IHA=} #secretsManagerConfiguration - SECRET_MANAGER: ${SECRET_MANAGER:-noop} + SECRET_MANAGER: ${SECRET_MANAGER:-db} #parameters: OM_SM_REGION: ${OM_SM_REGION:-""} OM_SM_ACCESS_KEY_ID: ${OM_SM_ACCESS_KEY_ID:-""} diff --git a/docker/docker-compose-quickstart/docker-compose-postgres.yml b/docker/docker-compose-quickstart/docker-compose-postgres.yml index aea3ada072b6..51146a7b2659 100644 --- a/docker/docker-compose-quickstart/docker-compose-postgres.yml +++ b/docker/docker-compose-quickstart/docker-compose-postgres.yml @@ -142,7 +142,7 @@ services: FERNET_KEY: ${FERNET_KEY:-jJ/9sz0g0OHxsfxOoSfdFdmk3ysNmPRnH3TUAbz3IHA=} #secretsManagerConfiguration - SECRET_MANAGER: ${SECRET_MANAGER:-noop} + SECRET_MANAGER: ${SECRET_MANAGER:-db} #parameters: OM_SM_REGION: ${OM_SM_REGION:-""} OM_SM_ACCESS_KEY_ID: ${OM_SM_ACCESS_KEY_ID:-""} @@ -285,7 +285,7 @@ services: FERNET_KEY: ${FERNET_KEY:-jJ/9sz0g0OHxsfxOoSfdFdmk3ysNmPRnH3TUAbz3IHA=} #secretsManagerConfiguration - SECRET_MANAGER: ${SECRET_MANAGER:-noop} + SECRET_MANAGER: ${SECRET_MANAGER:-db} #parameters: OM_SM_REGION: ${OM_SM_REGION:-""} OM_SM_ACCESS_KEY_ID: ${OM_SM_ACCESS_KEY_ID:-""} diff --git a/docker/docker-compose-quickstart/docker-compose.yml b/docker/docker-compose-quickstart/docker-compose.yml index fbb4237b32a5..8f223d383071 100644 --- a/docker/docker-compose-quickstart/docker-compose.yml +++ b/docker/docker-compose-quickstart/docker-compose.yml @@ -140,7 +140,7 @@ services: FERNET_KEY: ${FERNET_KEY:-jJ/9sz0g0OHxsfxOoSfdFdmk3ysNmPRnH3TUAbz3IHA=} #secretsManagerConfiguration - SECRET_MANAGER: ${SECRET_MANAGER:-noop} + SECRET_MANAGER: ${SECRET_MANAGER:-db} #parameters: OM_SM_REGION: ${OM_SM_REGION:-""} OM_SM_ACCESS_KEY_ID: ${OM_SM_ACCESS_KEY_ID:-""} @@ -283,7 +283,7 @@ services: FERNET_KEY: ${FERNET_KEY:-jJ/9sz0g0OHxsfxOoSfdFdmk3ysNmPRnH3TUAbz3IHA=} #secretsManagerConfiguration - SECRET_MANAGER: ${SECRET_MANAGER:-noop} + SECRET_MANAGER: ${SECRET_MANAGER:-db} #parameters: OM_SM_REGION: ${OM_SM_REGION:-""} OM_SM_ACCESS_KEY_ID: ${OM_SM_ACCESS_KEY_ID:-""} From b7530a5fb73096ff61b3eb7fba53fd74d956a824 Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Thu, 14 Dec 2023 17:21:37 +0100 Subject: [PATCH 12/15] Fix test --- .../metadata/utils/secrets/test_secrets_manager_factory.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ingestion/tests/unit/metadata/utils/secrets/test_secrets_manager_factory.py b/ingestion/tests/unit/metadata/utils/secrets/test_secrets_manager_factory.py index 7a85887a7954..4d44fa294dcf 100644 --- a/ingestion/tests/unit/metadata/utils/secrets/test_secrets_manager_factory.py +++ b/ingestion/tests/unit/metadata/utils/secrets/test_secrets_manager_factory.py @@ -40,7 +40,7 @@ def setUp(cls) -> None: def test_get_not_implemented_secret_manager(self): with self.assertRaises(NotImplementedError) as not_implemented_error: om_connection: OpenMetadataConnection = self.build_open_metadata_connection( - SecretsManagerProvider.noop, + SecretsManagerProvider.db, SecretsManagerClientLoader.noop, ) om_connection.secretsManagerProvider = "aws" @@ -54,7 +54,7 @@ def test_get_not_implemented_secret_manager(self): def test_invalid_config_secret_manager(self): om_connection: OpenMetadataConnection = self.build_open_metadata_connection( - SecretsManagerProvider.noop, + SecretsManagerProvider.db, SecretsManagerClientLoader.noop, ) om_connection.secretsManagerLoader = "random" @@ -66,7 +66,7 @@ def test_invalid_config_secret_manager(self): def test_get_none_secret_manager(self): om_connection: OpenMetadataConnection = self.build_open_metadata_connection( - SecretsManagerProvider.noop, + SecretsManagerProvider.db, SecretsManagerClientLoader.noop, ) om_connection.secretsManagerProvider = None From 2a399e59e84618d6b28d9d1833a943c4d128eae4 Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Fri, 15 Dec 2023 09:13:39 +0100 Subject: [PATCH 13/15] UI --- .../src/main/resources/ui/src/mocks/IngestionListTable.mock.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openmetadata-ui/src/main/resources/ui/src/mocks/IngestionListTable.mock.ts b/openmetadata-ui/src/main/resources/ui/src/mocks/IngestionListTable.mock.ts index 9e16f1d200c9..1d49e5c0b175 100644 --- a/openmetadata-ui/src/main/resources/ui/src/mocks/IngestionListTable.mock.ts +++ b/openmetadata-ui/src/main/resources/ui/src/mocks/IngestionListTable.mock.ts @@ -65,7 +65,7 @@ const mockESIngestionData: IngestionPipeline[] = [ securityConfig: { jwtToken: 'eyJraWQiOiJHYjM4OWEtOWY3Ni1nZGpzLWE5MmotMDI0MmJrO', }, - secretsManagerProvider: SecretsManagerProvider.Noop, + secretsManagerProvider: SecretsManagerProvider.Db, secretsManagerLoader: SecretsManagerClientLoader.Noop, apiVersion: 'v1', includeTopics: true, From 02298a7648a9c569146a7d06c42cbd1a66471d03 Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Fri, 15 Dec 2023 11:24:17 +0100 Subject: [PATCH 14/15] Update openmetadata-ui/src/main/resources/ui/src/mocks/IngestionListTable.mock.ts --- .../src/main/resources/ui/src/mocks/IngestionListTable.mock.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openmetadata-ui/src/main/resources/ui/src/mocks/IngestionListTable.mock.ts b/openmetadata-ui/src/main/resources/ui/src/mocks/IngestionListTable.mock.ts index 1d49e5c0b175..4324a3f34bbb 100644 --- a/openmetadata-ui/src/main/resources/ui/src/mocks/IngestionListTable.mock.ts +++ b/openmetadata-ui/src/main/resources/ui/src/mocks/IngestionListTable.mock.ts @@ -65,7 +65,7 @@ const mockESIngestionData: IngestionPipeline[] = [ securityConfig: { jwtToken: 'eyJraWQiOiJHYjM4OWEtOWY3Ni1nZGpzLWE5MmotMDI0MmJrO', }, - secretsManagerProvider: SecretsManagerProvider.Db, + secretsManagerProvider: SecretsManagerProvider.DB, secretsManagerLoader: SecretsManagerClientLoader.Noop, apiVersion: 'v1', includeTopics: true, From 2190687fe8c03e1effa02654cfca02dd146043b0 Mon Sep 17 00:00:00 2001 From: Pere Miquel Brull Date: Fri, 15 Dec 2023 14:49:33 +0100 Subject: [PATCH 15/15] update default --- .../json/schema/entity/automations/testServiceConnection.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openmetadata-spec/src/main/resources/json/schema/entity/automations/testServiceConnection.json b/openmetadata-spec/src/main/resources/json/schema/entity/automations/testServiceConnection.json index 1adcff0e1e70..aca0502095f2 100644 --- a/openmetadata-spec/src/main/resources/json/schema/entity/automations/testServiceConnection.json +++ b/openmetadata-spec/src/main/resources/json/schema/entity/automations/testServiceConnection.json @@ -50,7 +50,7 @@ }, "secretsManagerProvider": { "$ref": "../../security/secrets/secretsManagerProvider.json", - "default": "noop" + "default": "db" } }, "additionalProperties": false