Greg Padgett has posted comments on this change.
Change subject: core: Encrypt CHAP credentials in the database
......................................................................
Patch Set 3: No score; I would prefer that you didn't submit this
(7 inline comments)
Thanks for the comments, a new revision will be on its way... (Also,
un-verifying due to my accidental 'set -p'.)
....................................................
File backend/manager/dbscripts/upgrade/03_01_1440_encrypt_chap_password.sh
Line 1: #!/bin/bash
My hesitation would be that we can't use 'set -o pipefail' so there would be
more temp variables, quoting to watch out for, etc. But I can look into it and
test in posix mode.
Line 2:
Line 3: #include db general functions
Line 4: source ./dbfunctions.sh
Line 5:
Line 3: #include db general functions
Line 4: source ./dbfunctions.sh
Line 5:
Line 6: # detect failure of commands within pipelines
Line 7: set -p
This should be 'set -o pipefail'
Line 8:
Line 9: # get configuration values needed for password encryption from DB
Line 10: certificate=$(get_config_value "CertificateFileName" "general")
Line 11:
Line 6: # detect failure of commands within pipelines
Line 7: set -p
Line 8:
Line 9: # get configuration values needed for password encryption from DB
Line 10: certificate=$(get_config_value "CertificateFileName" "general")
Do you mean to remove them because they aren't necessary?
Line 11:
Line 12: # change password column to text to fit the encrypted password.
Line 13: CMD="select
fn_db_change_column_type('storage_server_connections','password','VARCHAR','text');"
Line 14: execute_command "${CMD}" "${DATABASE}" ${SERVERNAME} ${PORT} >
/dev/null
Line 19: execute_command "${CMD}" "${DATABASE}" ${SERVERNAME} ${PORT} >
${filename}
Line 20: while read line
Line 21: do
Line 22: # extracting the relevant fields values from each record.
Line 23: if [ $(echo $line | grep "|" |wc -l) -eq 0 ]; then
Right, the typical pipe delimiters here are decent but not foolproof. This
query is simple enough to avoid the formatting and parsing altogether (which
I'd prefer over xml, csv, etc) by querying only for ids, then querying each id
for each attribute. More queries, less post-processing. Less efficient
probably, but we are using the shell so that's already not the highest
priority. We still would need a blank line check, but that could be a simple
regex match.
Line 24: continue
Line 25: fi
Line 26: connId=$(echo "${line}" | cut -d "|" -f1 | sed 's/^ *//g; s/
*$//g')
Line 27: connName=$(echo "${line}" | cut -d "|" -f2 | sed 's/^ *//g; s/
*$//g')
Line 22: # extracting the relevant fields values from each record.
Line 23: if [ $(echo $line | grep "|" |wc -l) -eq 0 ]; then
Line 24: continue
Line 25: fi
Line 26: connId=$(echo "${line}" | cut -d "|" -f1 | sed 's/^ *//g; s/
*$//g')
I find it more idiomatic to extract fields with cut and trim with sed. The
original I based this on was similar but used 'tr' to remove spaces. I wanted
to preserve spaces in the password so used 'sed' instead (and did it on all 3
so they'd match).
Line 27: connName=$(echo "${line}" | cut -d "|" -f2 | sed 's/^ *//g; s/
*$//g')
Line 28: connPasswd=$(echo "${line}" | cut -d "|" -f3 | sed 's/^ *//g; s/
*$//g')
Line 29: if [ "$connId" != "" -a "$connPasswd" != "" ]; then
Line 30: # encrypt the password
Line 25: fi
Line 26: connId=$(echo "${line}" | cut -d "|" -f1 | sed 's/^ *//g; s/
*$//g')
Line 27: connName=$(echo "${line}" | cut -d "|" -f2 | sed 's/^ *//g; s/
*$//g')
Line 28: connPasswd=$(echo "${line}" | cut -d "|" -f3 | sed 's/^ *//g; s/
*$//g')
Line 29: if [ "$connId" != "" -a "$connPasswd" != "" ]; then
... and speaking of idiomatic ;)
Line 30: # encrypt the password
Line 31: encryptedPasswd=$(echo -n "$connPasswd" | /usr/bin/openssl
rsautl -certin -inkey $certificate -encrypt -pkcs | /usr/bin/openssl enc -a)
Line 32: if [ $? -ne 0 ]; then
Line 33: echo "Failed to encrypt connection ${connName} password.
The password will remain unencrypted in the database until this is complete."
Line 27: connName=$(echo "${line}" | cut -d "|" -f2 | sed 's/^ *//g; s/
*$//g')
Line 28: connPasswd=$(echo "${line}" | cut -d "|" -f3 | sed 's/^ *//g; s/
*$//g')
Line 29: if [ "$connId" != "" -a "$connPasswd" != "" ]; then
Line 30: # encrypt the password
Line 31: encryptedPasswd=$(echo -n "$connPasswd" | /usr/bin/openssl
rsautl -certin -inkey $certificate -encrypt -pkcs | /usr/bin/openssl enc -a)
I left it out so that the input would match the encrypted base64 passwords
stored by the engine (it also breaks them into multiple lines). I think we
should change both or neither, thoughts?
Line 32: if [ $? -ne 0 ]; then
Line 33: echo "Failed to encrypt connection ${connName} password.
The password will remain unencrypted in the database until this is complete."
Line 34: else
Line 35: # update the password field for the given connection
--
To view, visit http://gerrit.ovirt.org/8344
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I15b4cba7418d9d818fb2fd69c708fdeb20942f9c
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Greg Padgett <gpadg...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Eduardo <ewars...@redhat.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Greg Padgett <gpadg...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
