diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ServiceEntityRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ServiceEntityRepository.java index 913dd3ec127..d72728d7df6 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ServiceEntityRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ServiceEntityRepository.java @@ -100,6 +100,14 @@ public abstract class ServiceEntityRepository< return service; } + /** Remove the secrets from the secret manager */ + @Override + protected void postDelete(T service) { + SecretsManagerFactory.getSecretsManager() + .deleteSecretsFromServiceConnectionConfig( + service.getConnection().getConfig(), service.getServiceType().value(), service.getName(), serviceType); + } + @Override public ServiceUpdater getUpdater(T original, T updated, Operation operation) { return new ServiceUpdater(original, updated, operation); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/WorkflowRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/WorkflowRepository.java index 28c21326921..04e73419fa9 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/WorkflowRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/WorkflowRepository.java @@ -59,6 +59,12 @@ public class WorkflowRepository extends EntityRepository { entity.withOwner(owner).withOpenMetadataServerConnection(openmetadataConnection); } + /** Remove the secrets from the secret manager */ + @Override + protected void postDelete(Workflow workflow) { + SecretsManagerFactory.getSecretsManager().deleteSecretsFromWorkflow(workflow); + } + @Override public void storeRelationships(Workflow entity) { storeOwner(entity, entity.getOwner()); 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 749f1a22d2b..4d632f11c36 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 @@ -19,6 +19,7 @@ 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; +import software.amazon.awssdk.services.ssm.model.DeleteParameterRequest; import software.amazon.awssdk.services.ssm.model.GetParameterRequest; import software.amazon.awssdk.services.ssm.model.ParameterType; import software.amazon.awssdk.services.ssm.model.PutParameterRequest; @@ -72,6 +73,12 @@ public class AWSSSMSecretsManager extends AWSBasedSecretsManager { return ssmClient.getParameter(parameterRequest).parameter().value(); } + @Override + protected void deleteSecretInternal(String secretName) { + DeleteParameterRequest deleteParameterRequest = DeleteParameterRequest.builder().name(secretName).build(); + this.ssmClient.deleteParameter(deleteParameterRequest); + } + public static AWSSSMSecretsManager getInstance(SecretsManagerConfiguration config, String clusterPrefix) { if (INSTANCE == null) INSTANCE = new AWSSSMSecretsManager(config, clusterPrefix); 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 079a010d13d..c2d6f16bf0e 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 @@ -22,6 +22,7 @@ import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider; import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.secretsmanager.SecretsManagerClient; 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.UpdateSecretRequest; @@ -74,6 +75,12 @@ public class AWSSecretsManager extends AWSBasedSecretsManager { return this.secretsClient.getSecretValue(getSecretValueRequest).secretString(); } + @Override + protected void deleteSecretInternal(String secretName) { + DeleteSecretRequest deleteSecretRequest = DeleteSecretRequest.builder().secretId(secretName).build(); + this.secretsClient.deleteSecret(deleteSecretRequest); + } + public static AWSSecretsManager getInstance(SecretsManagerConfiguration config, String clusterPrefix) { if (INSTANCE == null) INSTANCE = new AWSSecretsManager(config, clusterPrefix); return INSTANCE; 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 650de61e5ef..0096aaffc64 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 @@ -45,6 +45,11 @@ public class InMemorySecretsManager extends ExternalSecretsManager { storeSecret(secretName, secretValue); } + @Override + protected void deleteSecretInternal(String secretName) { + secretsMap.remove(secretName); + } + @Override String getSecret(String secretName) { String value = secretsMap.getOrDefault(secretName, null); 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 8cc1776e863..ecd2d35145c 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 @@ -32,4 +32,8 @@ public class NoopSecretsManager extends SecretsManager { protected String storeValue(String fieldName, String value, String secretId, boolean store) { return value; } + + // Nothing to delete on the Noop SM. We only delete on External SM + @Override + protected void deleteSecretInternal(String secretName) {} } 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 52f93d9337c..b218e3eddc0 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 @@ -301,4 +301,57 @@ public abstract class SecretsManager { void setFernet(Fernet fernet) { this.fernet = fernet; } + + protected abstract void deleteSecretInternal(String secretName); + + 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); + deleteSecrets(newConnectionConfig, buildSecretId(true, serviceType.value(), connectionName)); + + } catch (Exception e) { + String message = SecretsUtil.buildExceptionMessageConnection(e.getMessage(), connectionType, true); + if (message != null) { + throw new InvalidServiceConnectionException(message); + } + throw InvalidServiceConnectionException.byMessage( + connectionType, String.format("Failed to delete secrets from connection instance of %s", connectionType)); + } + } + + public void deleteSecretsFromWorkflow(Workflow workflow) { + Workflow workflowConverted = (Workflow) ClassConverterFactory.getConverter(Workflow.class).convert(workflow); + // we don't store OM conn sensitive data + workflowConverted.setOpenMetadataServerConnection(null); + try { + deleteSecrets(workflowConverted, buildSecretId(true, "workflow", workflow.getName())); + } catch (Exception e) { + throw new CustomExceptionMessage( + Response.Status.BAD_REQUEST, + String.format("Failed to delete secrets from workflow instance [%s]", workflow.getName())); + } + } + + private void deleteSecrets(Object toDeleteSecretsFrom, String secretId) { + if (!DO_NOT_ENCRYPT_CLASSES.contains(toDeleteSecretsFrom.getClass())) { + Arrays.stream(toDeleteSecretsFrom.getClass().getMethods()) + .filter(ReflectionUtil::isGetMethodOfObject) + .forEach( + method -> { + Object obj = ReflectionUtil.getObjectFromMethod(method, toDeleteSecretsFrom); + String fieldName = method.getName().replaceFirst("get", ""); + // 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")) { + 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))); + } + }); + } + } } 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 1c063314659..1a9caf6bee8 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 @@ -34,6 +34,17 @@ public class SecretsManagerFactory { case NOOP: case AWS_SSM: case AWS: + /* + We handle AWS and AWS_SSM as a NoopSecretsManager since we don't + need to WRITE any secrets. We will be just reading them out of the + AWS instance on the INGESTION side, but the server does not need + to do anything here. + + 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 + */ secretsManager = NoopSecretsManager.getInstance(clusterName, secretsManagerProvider); break; case MANAGED_AWS: diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/util/IngestionPipelineBuilder.java b/openmetadata-service/src/main/java/org/openmetadata/service/util/IngestionPipelineBuilder.java index 4861ac4066a..94f0658dcc4 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/util/IngestionPipelineBuilder.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/util/IngestionPipelineBuilder.java @@ -27,7 +27,7 @@ public final class IngestionPipelineBuilder { } /** Build `IngestionPipeline` object with concrete class for the config which by definition it is a `Object`. */ - public static IngestionPipeline addDefinedConfig(IngestionPipeline ingestionPipeline) { + public static void addDefinedConfig(IngestionPipeline ingestionPipeline) { if (DBT.equals(ingestionPipeline.getPipelineType()) && ingestionPipeline.getSourceConfig() != null) { ingestionPipeline .getSourceConfig() @@ -41,6 +41,5 @@ public final class IngestionPipelineBuilder { ClassConverterFactory.getConverter(OpenMetadataConnection.class) .convert(ingestionPipeline.getOpenMetadataServerConnection())); } - return ingestionPipeline; } } 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 new file mode 100644 index 00000000000..d32b31a2dc0 --- /dev/null +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/SecretsManagerLifecycleTest.java @@ -0,0 +1,125 @@ +package org.openmetadata.service.secrets; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.lenient; +import static org.openmetadata.schema.api.services.CreateDatabaseService.DatabaseServiceType.Mysql; + +import java.util.Map; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoExtension; +import org.openmetadata.schema.api.services.DatabaseConnection; +import org.openmetadata.schema.entity.automations.TestServiceConnectionRequest; +import org.openmetadata.schema.entity.automations.Workflow; +import org.openmetadata.schema.entity.automations.WorkflowType; +import org.openmetadata.schema.entity.services.ServiceType; +import org.openmetadata.schema.services.connections.database.MysqlConnection; +import org.openmetadata.service.exception.SecretsManagerException; +import org.openmetadata.service.fernet.Fernet; + +@ExtendWith(MockitoExtension.class) +public class SecretsManagerLifecycleTest { + + private static final String ENCRYPTED_VALUE = "fernet:abcdef"; + private static final String DECRYPTED_VALUE = "123456"; + + // We'll test the secret creation and deletion using the In Memory SM + private static InMemorySecretsManager secretsManager; + + @BeforeAll + static void setUp() { + secretsManager = InMemorySecretsManager.getInstance("openmetadata"); + Fernet fernet = Mockito.mock(Fernet.class); + lenient().when(fernet.decrypt(anyString())).thenReturn(DECRYPTED_VALUE); + lenient().when(fernet.decryptIfApplies(anyString())).thenReturn(DECRYPTED_VALUE); + lenient().when(fernet.encrypt(anyString())).thenReturn(ENCRYPTED_VALUE); + secretsManager.setFernet(fernet); + } + + @Test + void testDatabaseServiceConnectionConfigLifecycle() { + String password = "openmetadata-test"; + String secretName = "/openmetadata/database/test/password"; + String connectionName = "test"; + Map mysqlConnection = Map.of("password", password); + + // Ensure encrypted service connection config encrypts the password + MysqlConnection actualConnection = + (MysqlConnection) + secretsManager.encryptServiceConnectionConfig( + mysqlConnection, Mysql.value(), connectionName, ServiceType.DATABASE); + assertNotEquals(password, actualConnection.getPassword()); + + // Decrypt the encrypted password and validate + actualConnection = + (MysqlConnection) + secretsManager.decryptServiceConnectionConfig(actualConnection, Mysql.value(), ServiceType.DATABASE); + assertEquals(DECRYPTED_VALUE, actualConnection.getPassword()); + + // SM will have the key stored + String secretValue = secretsManager.getSecret(secretName); + assertEquals(secretValue, DECRYPTED_VALUE); + + // Now we delete the service + secretsManager.deleteSecretsFromServiceConnectionConfig( + mysqlConnection, "Mysql", connectionName, ServiceType.DATABASE); + + // We won't be able to get the key again + SecretsManagerException exception = + assertThrows(SecretsManagerException.class, () -> secretsManager.getSecret(secretName)); + + assertEquals(exception.getMessage(), String.format("Key [%s] not found in in-memory secrets manager", secretName)); + } + + @Test + void testWorkflowLifecycle() { + String password = "openmetadata_password"; + String secretName = "/openmetadata/workflow/test-connection/request/connection/config/password"; + + Workflow workflow = + new Workflow() + .withName("test-connection") + .withWorkflowType(WorkflowType.TEST_CONNECTION) + .withRequest( + new TestServiceConnectionRequest() + .withServiceType(ServiceType.DATABASE) + .withConnectionType("Mysql") + .withConnection( + new DatabaseConnection() + .withConfig( + new MysqlConnection() + .withHostPort("mysql:3306") + .withUsername("openmetadata_user") + .withPassword(password)))); + + Workflow encrypted = secretsManager.encryptWorkflow(workflow); + TestServiceConnectionRequest encryptedRequest = (TestServiceConnectionRequest) encrypted.getRequest(); + DatabaseConnection encryptedConnection = (DatabaseConnection) encryptedRequest.getConnection(); + MysqlConnection encryptedConfig = (MysqlConnection) encryptedConnection.getConfig(); + assertNotEquals(password, encryptedConfig.getPassword()); + + Workflow decrypted = secretsManager.decryptWorkflow(encrypted); + TestServiceConnectionRequest decryptedRequest = (TestServiceConnectionRequest) decrypted.getRequest(); + DatabaseConnection decryptedConnection = (DatabaseConnection) decryptedRequest.getConnection(); + MysqlConnection decryptedConfig = (MysqlConnection) decryptedConnection.getConfig(); + assertEquals(DECRYPTED_VALUE, decryptedConfig.getPassword()); + + // SM will have the key stored + String secretValue = secretsManager.getSecret(secretName); + assertEquals(secretValue, DECRYPTED_VALUE); + + // Now we delete the service + secretsManager.deleteSecretsFromWorkflow(workflow); + + // We won't be able to get the key again + SecretsManagerException exception = + assertThrows(SecretsManagerException.class, () -> secretsManager.getSecret(secretName)); + + assertEquals(exception.getMessage(), String.format("Key [%s] not found in in-memory secrets manager", secretName)); + } +}