Skip to content

Synchronize Secret Format and Usage #1620

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

Merged
merged 23 commits into from
Jul 11, 2025
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
94f34a1
Checkpoint secret alignment in CDK
lewijacn Apr 28, 2025
716b7ea
Further refactoring for auth secret
lewijacn Apr 30, 2025
e41db50
Checkpoint
lewijacn Apr 30, 2025
b24b310
Merge remote-tracking branch 'origin/main' into sane-secret
lewijacn Jun 26, 2025
d8bdabc
Checkpoint tests working for CDK, Console Lib, and Replayer
lewijacn Jul 1, 2025
a13616e
Minor cleanup
lewijacn Jul 1, 2025
4423c5c
Minor cleanup and removal of unused terms
lewijacn Jul 2, 2025
45b8f41
Remove backup directory
lewijacn Jul 2, 2025
84e19ee
Merge remote-tracking branch 'origin/main' into sane-secret
lewijacn Jul 2, 2025
ba1a924
Minor bug fixes
lewijacn Jul 2, 2025
8ab4795
Fix migration console task role
lewijacn Jul 2, 2025
1caa17f
Update context.json and logging of RFS_COMMAND
lewijacn Jul 3, 2025
8d9c72b
Merge remote-tracking branch 'origin/main' into sane-secret
lewijacn Jul 3, 2025
c8eeac1
Remove indirection with managed snapshot auto setting
lewijacn Jul 3, 2025
9bf5e4c
Consistent censoring when printing program args
lewijacn Jul 3, 2025
a03f3e4
Address PR comments, and general enhancements
lewijacn Jul 7, 2025
ea8a6a4
Merge remote-tracking branch 'origin/main' into sane-secret
lewijacn Jul 9, 2025
cef38e4
Add tests and better logging of improper secrets in console lib
lewijacn Jul 9, 2025
1609b5f
Provide cleaner output on error for cat-indices and cluster curl comm…
lewijacn Jul 9, 2025
41c48db
Flake8 fix
lewijacn Jul 9, 2025
d808230
Merge remote-tracking branch 'origin/main' into sane-secret
lewijacn Jul 11, 2025
ee1d73a
Add additional debug logging for call_api failures
lewijacn Jul 11, 2025
a0ca08d
Increase log level for integ tests call_api errors
lewijacn Jul 11, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions DocumentsFromSnapshotMigration/docker/Dockerfile
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
# Using same base image as other Java containers in this repo
FROM amazoncorretto:17-al2023-headless

# Install the AWS CLI in the container
RUN dnf install -y aws-cli --setopt=install_weak_deps=False && \
dnf install -y procps && \
dnf clean all && \
rm -rf /var/cache/dnf
# Install procps to use pgrep in entrypoint.sh
RUN dnf install -y procps && \
dnf clean all && \
rm -rf /var/cache/dnf

# Requires Gradle to genearte runtime jars initially
COPY ./build/runtimeJars /rfs-app/jars
Expand Down
39 changes: 5 additions & 34 deletions DocumentsFromSnapshotMigration/docker/entrypoint.sh
Original file line number Diff line number Diff line change
Expand Up @@ -36,43 +36,14 @@ trap cleanup SIGTERM SIGINT SIGQUIT


# Print our ENV variables
if [[ $RFS_COMMAND != *"--target-password"* && $RFS_COMMAND != *"--targetPassword"* ]]; then
echo "RFS_COMMAND: $RFS_COMMAND"
else
echo "RFS Target Cluster password found in RFS_COMMAND; skipping logging of the value"
fi
echo "RFS_COMMAND: $RFS_COMMAND"
Copy link
Member

Choose a reason for hiding this comment

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

This will print the password if set by --target-password right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes if passed explicitly as a command argument (say with extraArgs). We already do this though correct?

[ -z "$RFS_COMMAND" ] && \
{ echo "Warning: RFS_COMMAND is empty! Exiting."; exit 1; } || \
while true; do
    echo "Running command $RFS_COMMAND"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like this was an existing bug, have cleaned up now so this script will redact in both places


echo "RFS_TARGET_USER: $RFS_TARGET_USER"
echo "RFS_TARGET_PASSWORD: <redacted>"
echo "RFS_TARGET_PASSWORD_ARN: $RFS_TARGET_PASSWORD_ARN"

# Check if the RFS Command already contains a username; only do special work if it does not
if [[ $RFS_COMMAND != *"--target-username"* && $RFS_COMMAND != *"--targetUsername"* ]]; then
if [[ -n "$RFS_TARGET_USER" ]]; then
echo "Using username from ENV variable RFS_TARGET_USER. Updating RFS Command with username."
RFS_COMMAND="$RFS_COMMAND --target-username \"$RFS_TARGET_USER\""
fi
if [ -n "$TARGET_USERNAME" ]; then
echo "TARGET_USERNAME is set by the environment."
fi

# Check if the RFS Command already contains a password; only do special work if it does not
if [[ $RFS_COMMAND != *"--target-password"* && $RFS_COMMAND != *"--targetPassword"* ]]; then
PASSWORD_TO_USE=""

# Check if the password is available in plaintext; if, use it. Otherwise, retrieve it from AWS Secrets Manager
if [[ -n "$RFS_TARGET_PASSWORD" ]]; then
echo "Using plaintext password from ENV variable RFS_TARGET_PASSWORD"
PASSWORD_TO_USE="$RFS_TARGET_PASSWORD"
elif [[ -n "$RFS_TARGET_PASSWORD_ARN" ]]; then
# Retrieve password from AWS Secrets Manager if ARN is provided
echo "Using password from AWS Secrets Manager ARN in ENV variable RFS_TARGET_PASSWORD_ARN"
PASSWORD_TO_USE=$(aws secretsmanager get-secret-value --secret-id "$RFS_TARGET_PASSWORD_ARN" --query SecretString --output text)
fi

# Append the username/password to the RFS Command if have an updated password
if [[ -n "$PASSWORD_TO_USE" ]]; then
echo "Updating RFS Command with password."
RFS_COMMAND="$RFS_COMMAND --target-password \"$PASSWORD_TO_USE\""
fi
if [ -n "$TARGET_PASSWORD" ]; then
echo "TARGET_PASSWORD is set by the environment."
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't worry about adding these lines here since you're handling it within main for the java process.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes let's do our logging there, removed this

fi

# Extract the value passed after --s3-local-dir or --s3LocalDir
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ public class RfsMigrateDocuments {
public static final int NO_WORK_LEFT_EXIT_CODE = 3;
public static final int TOLERABLE_CLIENT_SERVER_CLOCK_DIFFERENCE_SECONDS = 5;
public static final String LOGGING_MDC_WORKER_ID = "workerId";
public static final String TARGET_USERNAME_ENV_VAR = "TARGET_USERNAME";
public static final String TARGET_PASSWORD_ENV_VAR = "TARGET_PASSWORD";

// Decrease successor nextAcquisitionLeaseExponent if shard setup takes less than 2.5% of total lease time
// Increase successor nextAcquisitionLeaseExponent if shard setup takes more than 10% of lease total time
Expand Down Expand Up @@ -254,6 +256,26 @@ public String getTransformerConfigParameterArgPrefix() {
private String transformerConfigFile;
}

public static class EnvParameters {

private EnvParameters() {
throw new IllegalStateException("EnvParameters utility class should not instantiated");
}

public static void injectFromEnv(Args args) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can wait, but this could be generalized into the solution for https://opensearch.atlassian.net/browse/MIGRATIONS-2634.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes that is my intention for the future of this, and had to stop myself a couple times from implementing it, as I'd like this work to focus on using secrets. I'd like to see us create a more generic class that we can use reuse for this work. Ideally it has type checking for converting str to whatever data type and allows us to specify some list of parameters to check in the environment for

List<String> addedEnvParams = new ArrayList<>();
if (args.targetArgs.username == null && System.getenv(TARGET_USERNAME_ENV_VAR) != null) {
args.targetArgs.username = System.getenv(TARGET_USERNAME_ENV_VAR);
addedEnvParams.add(TARGET_USERNAME_ENV_VAR);
}
if (args.targetArgs.password == null && System.getenv(TARGET_PASSWORD_ENV_VAR) != null) {
args.targetArgs.password = System.getenv(TARGET_PASSWORD_ENV_VAR);
addedEnvParams.add(TARGET_PASSWORD_ENV_VAR);
}
log.info("Adding parameters from the following expected environment variables: {}", addedEnvParams);
}
}

public static class NoWorkLeftException extends Exception {
public NoWorkLeftException(String message) {
super(message);
Expand Down Expand Up @@ -297,6 +319,7 @@ public static void main(String[] args) throws Exception {
Args arguments = new Args();
JCommander jCommander = JCommander.newBuilder().addObject(arguments).build();
jCommander.parse(args);
EnvParameters.injectFromEnv(arguments);

if (arguments.help) {
jCommander.usage();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ def generate_formatted_cluster_dict(default_dict, override_dict):
formatted_dict[auth_type] = auth_dict
add_to_dict(auth_dict, "username", cluster_dict, "basicAuthUsername")
add_to_dict(auth_dict, "password", cluster_dict, "basicAuthPassword")
add_to_dict(auth_dict, "password_from_secret_arn", cluster_dict, "basicAuthPasswordFromSecretArn")
add_to_dict(auth_dict, "user_secret_arn", cluster_dict, "basicAuthUserSecretArn")
add_to_dict(auth_dict, "region", cluster_dict, "region")
add_to_dict(auth_dict, "service", cluster_dict, "service")
formatted_dict[auth_type] = None if not auth_dict else auth_dict
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ Exactly one of the following blocks must be present:
- `no_auth`: may be empty, no authorization to use.
- `basic_auth`:
- `username`
- `password` OR `password_from_secret_arn`
- `password` \
OR
- `user_from_secret_arn`: A secrets manager secret containing both a `username` and `password` key within the secret
- `sigv4`:
- `region`: Optional, specify a region for the sigv4 signing, the default is the current region.
- `service`: Optional, specify a service signing name for the cluster, e.g `es` for Amazon OpenSearch Service and `aoss` for Amazon OpenSearch Serverless. Defaults to `es`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,23 +25,40 @@
"nullable": True,
}


def validate_basic_auth_options(field, value, error):
username = value.get("username")
password = value.get("password")
user_secret_arn = value.get("user_secret_arn")

has_user_pass = username is not None and password is not None
has_user_secret = user_secret_arn is not None

if has_user_pass and has_user_secret:
error(field, "Cannot provide both (username + password) and user_secret_arn")
elif has_user_pass and (username == "" or password == ""):
error(field, "Both username and password must be non-empty")
elif not has_user_pass and not has_user_secret:
error(field, "Must provide either (username + password) or user_secret_arn")


BASIC_AUTH_SCHEMA = {
"type": "dict",
"schema": {
"username": {
"type": "string",
"required": True,
"required": False,
},
"password": {
"type": "string",
"required": False,
},
"password_from_secret_arn": {
"user_secret_arn": {
"type": "string",
"required": False,
}
},
"check_with": contains_one_of({"password", "password_from_secret_arn"})
"check_with": validate_basic_auth_options
}

SIGV4_SCHEMA = {
Expand Down Expand Up @@ -102,19 +119,31 @@ def __init__(self, config: Dict, client_options: Optional[ClientOptions] = None)
self.auth_details = config["sigv4"] if config["sigv4"] is not None else {}
self.client_options = client_options

def get_basic_auth_password(self) -> str:
"""This method will return the basic auth password, if basic auth is enabled.
It will pull a password from the secrets manager if necessary.
def get_basic_auth_details(self) -> tuple[str, str]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

please you make this a record type { username: str, password: str } since both types are str

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It sounds like you are saying to have something like a NamedTuple defined here that has username and password fields to give more specificity here and be less error prone. Have added this in

"""Return a tuple of (username, password) for basic auth. Will use username/password if provided in plaintext,
otherwise will pull both username/password as keys in the specified secrets manager secret.
"""
assert self.auth_type == AuthMethod.BASIC_AUTH
assert self.auth_details is not None # for mypy's sake
if "password" in self.auth_details:
return self.auth_details["password"]
if "username" in self.auth_details and "password" in self.auth_details:
return (self.auth_details["username"], self.auth_details["password"])
# Pull password from AWS Secrets Manager
assert "password_from_secret_arn" in self.auth_details # for mypy's sake
assert "user_secret_arn" in self.auth_details # for mypy's sake
client = create_boto3_client(aws_service_name="secretsmanager", client_options=self.client_options)
password = client.get_secret_value(SecretId=self.auth_details["password_from_secret_arn"])
return password["SecretString"]
secret_response = client.get_secret_value(SecretId=self.auth_details["user_secret_arn"])
secret_dict = json.loads(secret_response["SecretString"])

if not isinstance(secret_dict, dict):
raise ValueError(f"Expected secret {self.auth_details['user_secret_arn']} to be a JSON object with username"
f" and password fields")

missing_keys = [k for k in ("username", "password") if k not in secret_dict]
if missing_keys:
raise ValueError(
f"Secret is missing required key(s): {', '.join(missing_keys)}"
)

return (secret_dict["username"], secret_dict["password"])

def _get_sigv4_details(self, force_region=False) -> tuple[str, Optional[str]]:
"""Return the service signing name and region name. If force_region is true,
Expand All @@ -130,11 +159,8 @@ def _get_sigv4_details(self, force_region=False) -> tuple[str, Optional[str]]:
def _generate_auth_object(self) -> requests.auth.AuthBase | None:
if self.auth_type == AuthMethod.BASIC_AUTH:
assert self.auth_details is not None # for mypy's sake
password = self.get_basic_auth_password()
return HTTPBasicAuth(
self.auth_details.get("username", None),
password
)
username, password = self.get_basic_auth_details()
return HTTPBasicAuth(username, password)
elif self.auth_type == AuthMethod.SIGV4:
service_name, region_name = self._get_sigv4_details(force_region=True)
return SigV4AuthPlugin(service_name, region_name)
Expand Down Expand Up @@ -183,8 +209,8 @@ def execute_benchmark_workload(self, workload: str,
client_options += ",use_ssl:true"
password_to_censor = ""
if self.auth_type == AuthMethod.BASIC_AUTH:
password_to_censor = self.get_basic_auth_password()
client_options += (f",basic_auth_user:{self.auth_details['username']},"
username, password_to_censor = self.get_basic_auth_details()
client_options += (f",basic_auth_user:{username},"
f"basic_auth_password:{password_to_censor}")
elif self.auth_type == AuthMethod.SIGV4:
raise NotImplementedError(f"Auth type {self.auth_type} is not currently support for executing "
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,9 +201,10 @@ def migrate_or_evaluate(self, command: str, extra_args=None) -> CommandResult:

if self._target_cluster.auth_type == AuthMethod.BASIC_AUTH:
try:
username, password = self._target_cluster.get_basic_auth_details()
command_args.update({
"--target-username": self._target_cluster.auth_details.get("username"),
"--target-password": self._target_cluster.get_basic_auth_password()
"--target-username": username,
"--target-password": password
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the creds were in a secret, will we pass them through the command line here? I'd like to see us propagate them as an env variable to be consistent w/ the container apps (which this will likely someday be too)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have updated this to follow same pattern for RFS and Replayer. I'd like to see us make this more robust in this task: https://opensearch.atlassian.net/browse/MIGRATIONS-2634

})
logger.info("Using basic auth for target cluster")
except KeyError as e:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -285,8 +285,8 @@ def create_pipeline_from_env(osi_client,
# Target endpoints for OSI are not currently allowed a port
target_endpoint_clean = sanitize_endpoint(target_cluster.endpoint, True)
source_auth_secret = None
if source_cluster.auth_details and "password_from_secret_arn" in source_cluster.auth_details:
source_auth_secret = source_cluster.auth_details["password_from_secret_arn"]
if source_cluster.auth_details and "user_from_secret_arn" in source_cluster.auth_details:
source_auth_secret = source_cluster.auth_details["user_from_secret_arn"]
pipeline_config_string = construct_pipeline_config(pipeline_config_file_path=pipeline_template_path,
source_endpoint=source_endpoint_clean,
source_auth_type=source_cluster.auth_type,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,10 @@ def _collect_universal_command_args(self) -> Dict:

if self.source_cluster.auth_type == AuthMethod.BASIC_AUTH:
try:
username, password = self.source_cluster.get_basic_auth_details()
command_args.update({
"--source-username": self.source_cluster.auth_details.get("username"),
"--source-password": self.source_cluster.get_basic_auth_password()
"--source-username": username,
"--source-password": password
Copy link
Collaborator

Choose a reason for hiding this comment

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

same question as for metadata

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have updated this to follow same pattern for RFS and Replayer. I'd like to see us make this more robust in this task: https://opensearch.atlassian.net/browse/MIGRATIONS-2634

})
logger.info("Using basic auth for source cluster")
except KeyError as e:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,7 @@ def test_valid_cluster_with_secrets_auth_created():
"endpoint": "https://opensearchtarget:9200",
"allow_insecure": True,
"basic_auth": {
"username": "admin",
"password_from_secret_arn": "arn:aws:secretsmanager:us-east-1:12345678912:secret:master-user-os-pass"
"user_secret_arn": "arn:aws:secretsmanager:us-east-1:12345678912:secret:master-user-os-pass"
},
}
cluster = Cluster(valid_with_secrets)
Expand All @@ -107,14 +106,14 @@ def test_invalid_cluster_with_two_passwords_refused():
"basic_auth": {
"username": "XXXXX",
"password": "XXXXXXXXXXXXXX",
"password_from_secret_arn": "arn:aws:secretsmanager:us-east-1:12345678912:secret:master-user-os-pass"
"user_secret_arn": "arn:aws:secretsmanager:us-east-1:12345678912:secret:master-user-os-pass"
},
}
with pytest.raises(ValueError) as excinfo:
Cluster(two_passwords)
assert "Invalid config file for cluster" in excinfo.value.args[0]
assert excinfo.value.args[1]["cluster"][0]['basic_auth'] == [
"More than one value is present: ['password', 'password_from_secret_arn']"
"Cannot provide both (username + password) and user_secret_arn"
]


Expand All @@ -130,7 +129,7 @@ def test_invalid_cluster_with_zero_passwords_refused():
Cluster(two_passwords)
assert "Invalid config file for cluster" in excinfo.value.args[0]
assert excinfo.value.args[1]["cluster"][0]['basic_auth'] == [
"No values are present from set: ['password', 'password_from_secret_arn']"
"Must provide either (username + password) or user_secret_arn"
]


Expand Down Expand Up @@ -304,13 +303,13 @@ def test_valid_cluster_api_call_with_secrets_auth(requests_mock, aws_credentials
"endpoint": "https://opensearchtarget:9200",
"allow_insecure": True,
"basic_auth": {
"username": "admin",
"password_from_secret_arn": None # Will be set later
"user_secret_arn": None # Will be set later
},
}
secret_value = "myfakepassword"
username = valid_with_secrets["basic_auth"]["username"]
b64encoded_token = b64encode(f"{username}:{secret_value}".encode('utf-8')).decode("ascii")
username = 'unit_test_user'
password = 'unit_test_pass'
secret_value = f"{{\"username\": \"{username}\", \"password\": \"{password}\"}}"
b64encoded_token = b64encode(f"{username}:{password}".encode('utf-8')).decode("ascii")
auth_header_should_be = f"Basic {b64encoded_token}"

requests_mock.get(f"{valid_with_secrets['endpoint']}/test_api", json={'test': True})
Expand All @@ -321,7 +320,7 @@ def test_valid_cluster_api_call_with_secrets_auth(requests_mock, aws_credentials
Name="test-cluster-password",
SecretString=secret_value,
)
valid_with_secrets["basic_auth"]["password_from_secret_arn"] = secret['ARN']
valid_with_secrets["basic_auth"]["user_secret_arn"] = secret['ARN']
cluster = Cluster(valid_with_secrets)
assert isinstance(cluster, Cluster)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,7 @@ def test_metadata_with_target_config_auth_makes_correct_subprocess_call(mocker):
mocker.patch("sys.stderr.write")
metadata.migrate()

username, password = target.get_basic_auth_details()
mock.assert_called_once_with([
"/root/metadataMigration/bin/MetadataMigration",
"migrate",
Expand All @@ -328,8 +329,8 @@ def test_metadata_with_target_config_auth_makes_correct_subprocess_call(mocker):
"--s3-local-dir", config["from_snapshot"]["local_dir"],
"--s3-repo-uri", config["from_snapshot"]["s3"]["repo_uri"],
"--s3-region", config["from_snapshot"]["s3"]["aws_region"],
"--target-username", target.auth_details.get("username"),
"--target-password", target.get_basic_auth_password(),
"--target-username", username,
"--target-password", password,
"--target-insecure",
], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, check=True
)
Expand Down
Loading