Skip to content

[cloud-sql-proxy] Remove env var assignment of db passwords #1314

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

cjac
Copy link
Contributor

@cjac cjac commented Apr 3, 2025

Add secret manager as a password specification source

@cjac
Copy link
Contributor Author

cjac commented Apr 3, 2025

/gcbrun

@cjac cjac changed the title Removing passwords from environment variable assignment [cloud-sql-proxy] Removing passwords from environment variable assignment Apr 3, 2025
@cjac cjac changed the title [cloud-sql-proxy] Removing passwords from environment variable assignment [cloud-sql-proxy] Remove passwords from environment variable assignment Apr 3, 2025
@cjac cjac force-pushed the hive-secret-manager-20250402 branch 2 times, most recently from cda43bb to 78da4b1 Compare April 3, 2025 02:43
@cjac
Copy link
Contributor Author

cjac commented Apr 3, 2025

/gcbrun

@cjac cjac changed the title [cloud-sql-proxy] Remove passwords from environment variable assignment [cloud-sql-proxy] Remove env var assignment of mysql passwords Apr 3, 2025
Copy link
Contributor Author

@cjac cjac left a comment

Choose a reason for hiding this comment

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

Some notes about why the changes are being made, and thoughts about how to improve.

METASTORE_PROXY_PORT="${METASTORE_INSTANCE##*:}"
else
METASTORE_PROXY_PORT=${DEFAULT_DB_PORT["${CLOUDSQL_INSTANCE_TYPE}"]}
if [[ -z "${METASTORE_PROXY_PORT}" ]] ; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't overwrite the value that was collected from the metadata attribute ; only attempt to set this value if the attrbiute was not set.

@@ -233,39 +239,46 @@ readonly KMS_KEY_URI
DB_ADMIN_PASSWORD_URI="$(/usr/share/google/get_metadata_value attributes/db-admin-password-uri || echo '')"
readonly DB_ADMIN_PASSWORD_URI

DB_ADMIN_PASSWORD=''
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove DB_ADMIN_PASSWORD environment variable entirely. Do not store sensitive data in environment variables.

if [[ -n "${DB_ADMIN_SECRET}" ]] ; then
gcloud secrets versions access "${DB_ADMIN_SECRET#*:}" \
--project="${METASTORE_INSTANCE%%:*}" \
--secret="${DB_ADMIN_SECRET%:*}" > /dev/shm/db-pw
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of storing classified material in environment variables, store to ephemeral storage, and ensure it is purged on exit of script.

fi
if [[ "${CLOUDSQL_INSTANCE_TYPE}" == "POSTGRES" && -z "${DB_ADMIN_PASSWORD}" ]]; then
if [[ "${CLOUDSQL_INSTANCE_TYPE}" == "POSTGRES" && -z "$(perl -pe 'chomp' < /dev/shm/db-pw)" ]]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test could instead be done with wc -c to eliminate the final instance of the variable being stored in bash strings

# Decrypt password
DB_HIVE_PASSWORD="$(gsutil cat "${DB_HIVE_PASSWORD_URI}" |
gsutil cat "${DB_HIVE_PASSWORD_URI}" |
gcloud kms decrypt \
--ciphertext-file - \
--plaintext-file - \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

--plaintext-file /dev/shm/hive-pw

@@ -394,13 +407,6 @@ function install_cloud_sql_proxy() {
local proxy_flags
proxy_flags="$(get_proxy_flags)"

# Validate db_hive_password and escape invalid xml characters if found.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these lines are duplicated in the single perl line below

</property>
</configuration>
EOF

bdconfig merge_configurations \
/usr/local/bin/bdconfig merge_configurations \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rocky doesn't have /usr/local/bin in its PATH

function initialize_postgres_metastore_db() {
log 'Initialzing POSTGRES DB for Hive metastore ...'
local admin_connection=postgresql://"${DB_ADMIN_USER}":"${DB_ADMIN_PASSWORD}"@127.0.0.1:"${METASTORE_PROXY_PORT}"/
local hive_connection=postgresql://"${DB_HIVE_USER}":"${DB_HIVE_PASSWORD}"@127.0.0.1:"${METASTORE_PROXY_PORT}"/postgres
local admin_connection=postgresql://"${DB_ADMIN_USER}":"$(perl -pe 'chomp' < /dev/shm/db-pw)"@127.0.0.1:"${METASTORE_PROXY_PORT}"/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was mistaken, after the wc change above, these two lines will be the final instance of assignment of classified material to bash environment variable. I'm not certain how best to address this problem, but I will stand up a PG instance to see if there's a way to specify DSN components from a config file as is done for mysql with --defaults-file=${admin_defaults_file}

@cjac cjac force-pushed the hive-secret-manager-20250402 branch 2 times, most recently from b41174f to 680dd55 Compare April 4, 2025 21:42
Add secret manager as a database password specification source
@cjac cjac force-pushed the hive-secret-manager-20250402 branch from 680dd55 to 0f660fc Compare April 4, 2025 21:53
@cjac
Copy link
Contributor Author

cjac commented Apr 4, 2025

/gcbrun

@cjac
Copy link
Contributor Author

cjac commented Apr 5, 2025

/gcbrun

@cjac cjac requested a review from dilipgodhia April 7, 2025 16:59
@cjac
Copy link
Contributor Author

cjac commented Apr 7, 2025

/gcbrun

fi
if [[ "${CLOUDSQL_INSTANCE_TYPE}" == "POSTGRES" && -z "${DB_ADMIN_PASSWORD}" ]]; then
if [[ "${CLOUDSQL_INSTANCE_TYPE}" == "POSTGRES" ]] && [[ "$(perl -pe 'chomp' < /dev/shm/db-pw | wc -c)" != "0" ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does the perl validation do in this context?

Copy link
Contributor Author

@cjac cjac Apr 8, 2025

Choose a reason for hiding this comment

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

Thanks for asking, Axel!

[ETA: Composed on phone en route to/from stadium]

The < directs to perl's STDIN file handle the contents of the named file

perl -e instructs Perl to evaluate the value of the -e argument instead of loading and evaluating a named file. For instance, the following instructs Perl to print hello world (followed by a newline character [$/]) to STDOUT: perl -e 'print "hello world$/"'

perl -pe instructs Perl to act as a filter, printing to STDOUT, one line at a time, anything that it receives on STDIN, after evaluating the code in the argument to -e. A string containing the line being processed is assigned to the default variable, $_ before the -e code is evaluated, and $_ is printed to STDOUT after the code is evaluated.

The Perl chomp function strips a trailing newline ($/) from a string variable named as chomp's first argument. If no argument is passed, strips trailing newline from the default variable, $_. If no trailing newline exists, chomp is a null operation.

So in toto, this command reads the file, strips the newline, and compares the result to an empty string. The functionality of the Perl code is the same as the bash code, but does not expose the secret to the log when bash is executed with the -x option.

I have made minor alterations to this password length check to be pushed anon. Must have proper keyboard to use emacs.

@cjac cjac changed the title [cloud-sql-proxy] Remove env var assignment of mysql passwords [cloud-sql-proxy] Remove env var assignment of db passwords Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants