-
Notifications
You must be signed in to change notification settings - Fork 513
[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
base: main
Are you sure you want to change the base?
[cloud-sql-proxy] Remove env var assignment of db passwords #1314
Conversation
/gcbrun |
cda43bb
to
78da4b1
Compare
/gcbrun |
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.
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 |
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.
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='' |
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.
remove DB_ADMIN_PASSWORD environment variable entirely. Do not store sensitive data in environment variables.
cloud-sql-proxy/cloud-sql-proxy.sh
Outdated
if [[ -n "${DB_ADMIN_SECRET}" ]] ; then | ||
gcloud secrets versions access "${DB_ADMIN_SECRET#*:}" \ | ||
--project="${METASTORE_INSTANCE%%:*}" \ | ||
--secret="${DB_ADMIN_SECRET%:*}" > /dev/shm/db-pw |
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.
Instead of storing classified material in environment variables, store to ephemeral storage, and ensure it is purged on exit of script.
cloud-sql-proxy/cloud-sql-proxy.sh
Outdated
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 |
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 test could instead be done with wc -c
to eliminate the final instance of the variable being stored in bash strings
cloud-sql-proxy/cloud-sql-proxy.sh
Outdated
# Decrypt password | ||
DB_HIVE_PASSWORD="$(gsutil cat "${DB_HIVE_PASSWORD_URI}" | | ||
gsutil cat "${DB_HIVE_PASSWORD_URI}" | | ||
gcloud kms decrypt \ | ||
--ciphertext-file - \ | ||
--plaintext-file - \ |
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.
--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. |
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.
these lines are duplicated in the single perl line below
</property> | ||
</configuration> | ||
EOF | ||
|
||
bdconfig merge_configurations \ | ||
/usr/local/bin/bdconfig merge_configurations \ |
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.
rocky doesn't have /usr/local/bin
in its PATH
cloud-sql-proxy/cloud-sql-proxy.sh
Outdated
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}"/ |
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.
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}
b41174f
to
680dd55
Compare
Add secret manager as a database password specification source
680dd55
to
0f660fc
Compare
/gcbrun |
/gcbrun |
/gcbrun |
cloud-sql-proxy/cloud-sql-proxy.sh
Outdated
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 |
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.
What does the perl validation do in this context?
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.
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.
Add secret manager as a password specification source