Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#14340 & #13849 - Clean secret ID and improve encrypt/decrypt exception management #14356

Merged
merged 17 commits into from
Dec 18, 2023
Merged
6 changes: 6 additions & 0 deletions bootstrap/sql/migrations/native/1.3.0/mysql/schemaChanges.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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';
11 changes: 11 additions & 0 deletions bootstrap/sql/migrations/native/1.3.0/postgres/schemaChanges.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
@Slf4j
public final class CommonUtil {

private static final List<String> jarNameFilter = List.of("openmetadata", "collate");
private static final List<String> JAR_NAME_FILTER = List.of("openmetadata", "collate");

private CommonUtil() {}

Expand All @@ -60,7 +60,7 @@ public static List<String> getResources(Pattern pattern) throws IOException {
String classPath = System.getProperty("java.class.path", ".");
List<String> 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) {
Expand All @@ -71,6 +71,13 @@ public static List<String> 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<String> getResourcesFromJarFile(File file, Pattern pattern) {
ArrayList<String> retval = new ArrayList<>();
try (ZipFile zf = new ZipFile(file)) {
Expand Down
4 changes: 3 additions & 1 deletion conf/openmetadata.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,9 @@ 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 /<prefix>/<clusterName>/<key>
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:-""}
Expand Down
4 changes: 2 additions & 2 deletions docker/development/docker-compose-postgres.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:-""}
Expand Down Expand Up @@ -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:-""}
Expand Down
4 changes: 2 additions & 2 deletions docker/development/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:-""}
Expand Down Expand Up @@ -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:-""}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:-""}
Expand Down Expand Up @@ -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:-""}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:-""}
Expand Down Expand Up @@ -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:-""}
Expand Down
4 changes: 2 additions & 2 deletions docker/docker-compose-quickstart/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ services:
FERNET_KEY: ${FERNET_KEY:-jJ/9sz0g0OHxsfxOoSfdFdmk3ysNmPRnH3TUAbz3IHA=}

#secretsManagerConfiguration
SECRET_MANAGER: ${SECRET_MANAGER:-noop}
SECRET_MANAGER: ${SECRET_MANAGER:-db}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if someone exported SECRET_MANAGER as noop they will run into issues, worth noting this as backward incompatible change and capture it in our upgrade notes

#parameters:
OM_SM_REGION: ${OM_SM_REGION:-""}
OM_SM_ACCESS_KEY_ID: ${OM_SM_ACCESS_KEY_ID:-""}
Expand Down Expand Up @@ -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:-""}
Expand Down
6 changes: 4 additions & 2 deletions ingestion/src/metadata/ingestion/models/custom_pydantic.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@

logger = logging.getLogger("metadata")

SECRET = "secret:"


class CustomSecretStr(SecretStr):
"""
Expand Down Expand Up @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand All @@ -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):
Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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"
Expand All @@ -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"
Expand All @@ -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
Expand All @@ -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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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, String clusterPrefix) {
super(awsProvider, clusterPrefix, 100);
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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,21 @@
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;
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;
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(SecretsConfig secretsConfig) {
super(MANAGED_AWS_SSM, secretsConfig);
}

@Override
Expand Down Expand Up @@ -61,6 +61,10 @@ private void putSecretParameter(String parameterName, String parameterValue, boo
.value(parameterValue)
.overwrite(overwrite)
.type(ParameterType.SECURE_STRING)
.tags(
SecretsManager.getTags(getSecretsConfig()).entrySet().stream()
.map(entry -> Tag.builder().key(entry.getKey()).value(entry.getValue()).build())
.toList())
.build();
this.ssmClient.putParameter(putParameterRequest);
}
Expand All @@ -77,8 +81,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(SecretsConfig secretsConfig) {
if (instance == null) instance = new AWSSSMSecretsManager(secretsConfig);
return instance;
}

Expand Down
Loading
Loading