-
Notifications
You must be signed in to change notification settings - Fork 35
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
Changes from 8 commits
94f34a1
716b7ea
e41db50
b24b310
d8bdabc
a13616e
4423c5c
45b8f41
84e19ee
ba1a924
8ab4795
1caa17f
8d9c72b
c8eeac1
9bf5e4c
a03f3e4
ea8a6a4
cef38e4
1609b5f
41c48db
d808230
ee1d73a
a0ca08d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
||
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." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -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(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 = { | ||
|
@@ -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]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please you make this a record type There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
@@ -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) | ||
|
@@ -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 " | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same question as for metadata There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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