[Ovirt-devel] [PATCH node] Password configuration script for the root password and sasl users

[Jim Meyering]

Bryan Kearney <bkearney at redhat.com> wrote:
> diff --git a/scripts/ovirt-config-password b/scripts/ovirt-config-password
> index 8b13789..af99915 100755
> --- a/scripts/ovirt-config-password
> +++ b/scripts/ovirt-config-password
> @@ -1 +1,93 @@
> +#!/bin/bash
> +#
> +# Set the root password and others
> +
> +ROOT_PASSWORD=""
> +
> +function sasl_password {
> +	printf "adding user $1 to the sasl list for libvirt\n"
> +	echo $2 | saslpasswd2 -a libvirt -p $1

You need quotes here.
Otherwise, a password with e.g., "foo   bar" would be
shortened (tokenized) to "foo bar".  Also, you can't use
echo, because that would honor (i.e., ignore) a leading -n, -e, or -E
option in the password string.

   	printf '%s\n' "$2" | saslpasswd2 -a libvirt -p "$1"

Even with a tiny function like this, it's more readable and slightly
more maintainable (less risk of confusing $1 and $2) to give names
to the parameters, e.g.,

    function sasl_password {
        user=$1
        passwd=$2
        printf "adding user $user to the sasl list for libvirt\n"
        printf '%s\n' "$passwd" | saslpasswd2 -a libvirt -p "$user"
    }

> +}
> +
> +function set_root_password {
> +	while true; do
> +		printf "\nPlease enter the new root password (hit return to skip) "
> +		read -s
> +		if [[ $REPLY == "" ]]; then
> +			return 1
> +		fi
> +		ROOT_PASSWORD=$REPLY
> +		printf "\nPlease enter again to confirm "
> +		read -s
> +		ROOT_PASSWORD_CONFIRM=$REPLY
> +		if [[ $ROOT_PASSWORD == $ROOT_PASSWORD_CONFIRM ]]; then
> +			echo $ROOT_PASSWORD | passwd --stdin root
> +			sasl_password root $ROOT_PASSWORD

Both of the above require quotes, too:

   			printf '%s\n' "$ROOT_PASSWORD" | passwd --stdin root
   			sasl_password root "$ROOT_PASSWORD"

> +			break
> +		else
> +			printf "\nPaswords did not match. Please try again"

Don't you want a "\n" at the end, here?
Probably instead of the one at the beginning.

> +		fi
> +	done
> +	return 0
> +}

These two functions are similar enough that they should use the
same code.

> +# Prompts the user for a single username, password combo
> +function prompt_sasl_user {
> +	while true; do
> +		printf "\nPlease enter a new user (hit return to skip) "
> +		read
> +		if [[ $REPLY == "" ]]; then
> +			break
> +		fi
> +		SASL_USER=$REPLY
> +		printf "\nPlease enter the password for $SASL_USER (hit return to skip) "
> +		read -s
> +		if [[ $REPLY == "" ]]; then
> +			return 1
> +		fi
> +		SASL_PASSWORD=$REPLY
> +		printf "\nPlease enter again to confirm "
> +		read -s
> +		SASL_PASSWORD_CONFIRM=$REPLY
> +		if [[ $SASL_PASSWORD == $SASL_PASSWORD_CONFIRM ]]; then
> +			sasl_password $SASL_USER $SASL_PASSWORD
> +			break
> +		else
> +			printf "\nPaswords did not match. Please try again"
> +		fi
> +	done
> +
> +}

How about this (untested):

# Usage: set_SASL_password USER
# Prompt(twice) for a password for the specified USER.
# If they match, set that user's system password,
# and add USER to the SASL list for libvirt.
set_SASL_password()
{
    user=$1
    while : ; do
	printf "\nPlease enter the new $user password (hit return to skip) "
	read -s
	test -z "$REPLY" && return 1
	local passwd=$REPLY
	printf "\nPlease enter again to confirm "
	read -s
	local confirm=$REPLY
	if test "$passwd" = "$confirm"; then
	    printf '%s\n' "$passwd" | passwd --stdin "$user"
	    sasl_password "$user" "$passwd"
	    return 0
	fi

	printf "Paswords did not match. Please try again\n"
    done
}

Then you'd use
  set_SASL_passwd root
Prompt for SASL_USER separately, then
  set_SASL_passwd $SASL_USER


> +#Check for the root user first
> +while true ; do
> +	printf "\nWould you like to set the root password (Y|N) "
> +	read
> +	case $REPLY in
> +	Y|y)
> +		set_root_password
> +		if [[ $? == 0 ]] ; then
> +			break ;
> +		fi

The more maintainable/readable idiom for the above 4 lines is like this:

    		set_root_password && break

> +		;;
> +	N|n)
> +		break ;
> +		;;

This makes the code treat any non-Y/y response like "N".
If you want to treat a response like "yes" differently,
you can add a catch-all case:

        *)
             printf "invalid response: %s\n" "$REPLY"
             ;;

> +	esac
> +done
> +
> +#Check for any sasl users
> +while true ; do
> +	printf "\nWould you like to add a new sasl user for libvirt (Y|N) "
> +	read
> +	case $REPLY in
> +	Y|y)
> +		prompt_sasl_user
> +		;;
> +	N|n)
> +		break ;
> +		;;
> +	esac
> +done
> +