[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Patch for fixing keystone rc bug #687311 : permission to upload in SID?



On Wed, Sep 26, 2012 at 02:22:30 +0800, Thomas Goirand wrote:

> Hi,
> 
> I've tried to fix #687311. To the best of my knowledge, a review of
> at least one person, and some practical tests, the package
> configuration works as expected, respecting the policy, after
> applying the patch.
> 
> Since the patch isn't small (6 files changed, 95 insertions(+), 36
> deletions), I'm sending it as attachment before upload into SID, so
> the release team can object it if it's wrong.
> 
> Please let me know,
> 
> Thomas (zigo)

> diff --git a/debian/changelog b/debian/changelog
> index cbc8543..37519fa 100644
> --- a/debian/changelog
> +++ b/debian/changelog
> @@ -1,3 +1,10 @@
> +keystone (2012.1.1-7) unstable; urgency=low
> +
> +  * Fixes band handling (eg: policy violation) of keystone.conf which was
> +  conffiles, but changed in the posinst (Closes: #687311).
> +
> + -- Thomas Goirand <zigo@debian.org>  Wed, 12 Sep 2012 17:09:47 +0000
> +
>  keystone (2012.1.1-6) unstable; urgency=high
>  
>    * CVE-2012-4413: Revoking a role does not affect existing tokens
> diff --git a/debian/keystone.config b/debian/keystone.config
> index 84aad01..e1236d5 100644
> --- a/debian/keystone.config
> +++ b/debian/keystone.config
> @@ -1,19 +1,84 @@
>  #!/bin/sh
> +
>  set -e
>  
>  . /usr/share/debconf/confmodule
>  
> +### Reading of values in the keystone config file       ###
> +### and setting default for dbconfig-common accordingly ###
> +KEY_CONF=/etc/keystone/keystone.conf
> +
> +# Create config files if they don't exist
> +if ! [ -e /etc/keystone ] ; then
> +	mkdir /etc/keystone
> +fi
> +if ! [ -e /etc/keystone/keystone.conf ] ; then
> +	cp /usr/share/doc/keystone/keystone.conf.sample ${KEY_CONF}
> +fi
> +

Normally the config script doesn't touch configuration files, it reads
the current configuration if it exists, updates the values in debconf,
then asks the user if necessary.  The postinst then writes the actual
config.

> +if [ -e "${KEY_CONF}" ] ; then
> +	KEY_CONF_AUTH_TOKEN=`grep -E "^([ \t])*admin_token([ \t])*=([ \t])*" ${KEY_CONF} | awk '{print $3}'`
> +	if [ -n "${KEY_CONF_AUTH_TOKEN}" ] ; then
> +		db_set keystone/auth-token ${KEY_CONF_AUTH_TOKEN}
> +	fi
> +fi
>  db_input low keystone/auth-token || true
>  db_input low keystone/configure_db || true
>  db_go
> +
>  db_get keystone/configure_db
> -if [ "$RET" = "true" ]; then
> -    if [ -f /usr/share/dbconfig-common/dpkg/config ];
> -    then
> -	dbc_dbtypes="sqlite3, mysql, pgsql"
> -	db_authmethod_user="password"
> -	dbc_basepath="/var/lib/keystone"
> +if [ "$RET" = "true" ] && [ -e "${KEY_CONF}" ] && [ -f /usr/share/dbconfig-common/dpkg/config ] ; then
>  	. /usr/share/dbconfig-common/dpkg/config
> +	KEY_CONF_DB_CON_INFO=`grep -E "^([ \t])*connection([ \t])*=([ \t])*" ${KEY_CONF} | awk '{print $3}'`
> +	KEY_CONF_DB_TYPE=`echo ${KEY_CONF_DB_CON_INFO} | cut -d":" -f1`
> +	# If we have an undefined SQL type, we go back to a more sane default (eg: SQLite)
> +	if [ "${KEY_CONF_DB_TYPE}" != "sqlite" ] && [ "${KEY_CONF_DB_TYPE}" != "mysql" ] && [ "${KEY_CONF_DB_TYPE}" != "pgsql" ] ; then
> +		KEY_CONF_DB_CON_INFO="sqlite:///var/lib/keystone/keystone.sqlite"
> +		KEY_CONF_DB_TYPE="sqlite"
> +	fi
> +	if [ "${KEY_CONF_DB_TYPE}" = "sqlite" ] ; then
> +		# This is the invalid default in the etc/keystone.conf in the source package
> +		if [ "${KEY_CONF_DB_CON_INFO}" = "sqlite:///keystone.db" ] ; then
> +			KEY_CONF_DB_CON_INFO="sqlite:///var/lib/keystone/keystone.sqlite"
> +		fi
> +
> +		KEY_CONF_DB_PATH=`echo "${KEY_CONF_DB_CON_INFO}" | awk '{print substr($0,11)}'`
> +		if [ -z "${KEY_CONF_DB_PATH}" ] ; then
> +			KEY_CONF_DB_PATH=/var/lib/keystone/keystone.sqlite
> +		fi
> +		dbc_basepath=`dirname "${KEY_CONF_DB_PATH}"`
> +		dbc_dbname=`basename "${KEY_CONF_DB_PATH}"`
> +		dbc_dbtypes="sqlite3, mysql, pgsql"
> +	else
> +		# Later, the postinst does: mysql://$dbc_dbuser:$dbc_dbpass@${dbc_dbserver:-localhost}$dbport/$dbc_dbname
> +		# so we are supposed to parse that if it exists
> +		KEY_CONF_ADDR=`echo "${KEY_CONF_DB_CON_INFO}" | awk '{print substr($0,9)}'`
> +		KEY_CONF_BEFORE_AT=`echo "${KEY_CONF_ADDR}" | cut -d"@" -f1`
> +		KEY_CONF_AFTER_AT=`echo "${KEY_CONF_ADDR}" | cut -d"@" -f1`
> +
> +		KEY_CONF_USER=`echo "${KEY_CONF_BEFORE_AT}" | cut -d":" -f1`
> +		KEY_CONF_PASS=`echo "${KEY_CONF_BEFORE_AT}" | cut -d":" -f2`
> +		KEY_CONF_SERVER_PORT=`echo "${KEY_CONF_AFTER_AT}" | cut -d"/" -f1`
> +		KEY_CONF_DB_NAME=`echo "${KEY_CONF_AFTER_AT}" | cut -d"/" -f2`
> +
> +		KEY_CONF_SERVER=`echo "${KEY_CONF_SERVER_PORT}" | cut -d":" -f1`
> +		KEY_CONF_PORT=`echo "${KEY_CONF_SERVER_PORT}" | cut -d":" -f2`
> +		if [ -n "${KEY_CONF_PORT}" ] ; then
> +			dbc_dbport=${KEY_CONF_PORT}
> +		fi
> +
> +		if [ -n "${KEY_CONF_USER}" ] && [ -n "${KEY_CONF_PASS}" ] && [ -n "${KEY_CONF_SERVER_PORT}" ] && [ -n "${KEY_CONF_DB_NAME}" ] ; then
> +			dbc_dbuser=${KEY_CONF_USER}
> +			dbc_dbpass=${KEY_CONF_PASS}
> +			dbc_dbserver=${KEY_CONF_SERVER}
> +			dbc_dbname=${KEY_CONF_DB_NAME}
> +		fi
> +		if [ "${KEY_CONF_DB_TYPE}" = "mysql" ] ; then
> +			dbc_dbtypes="mysql, pgsql, sqlite3"
> +		else
> +			dbc_dbtypes="pgsql, mysql, sqlite3"
> +		fi
> +		db_authmethod_user="password"
> +	fi
>  	dbc_go keystone $@
> -    fi
>  fi
> diff --git a/debian/keystone.install b/debian/keystone.install
> index 9dfb505..26d1053 100644
> --- a/debian/keystone.install
> +++ b/debian/keystone.install
> @@ -1,2 +1,4 @@
>  usr/bin/*
> -etc/* etc/keystone
> \ No newline at end of file
> +etc/default_catalog.templates	/etc/keystone
> +etc/logging.conf.sample	/usr/share/doc/keystone
> +etc/policy.json		/etc/keystone
> diff --git a/debian/keystone.postinst b/debian/keystone.postinst
> index 9692a90..883b0a7 100755
> --- a/debian/keystone.postinst
> +++ b/debian/keystone.postinst
> @@ -1,5 +1,7 @@
>  #!/bin/sh
> +
>  set -e
> +set -x
>  

That should be removed before upload.

>  if [ "$1" = "configure" ]
>  then
> @@ -13,37 +15,21 @@ then
>              --disabled-password \
>              --group keystone
>  
> -
>      db_get keystone/configure_db
>      if [ "$RET" = "true" ]; then
>  	db_get keystone/database-type
> -	if [ $RET = "sqlite3" ]
> -	then
> -	    dbc_name="keystone.sqlite"
> -	    db_set keystone/db/dbname $dbc_name
> -	fi
>  	dbc_dbfile_owner="keystone:keystone"
>  
>  	dbc_go keystone $@
>  
>  	if [ "$dbc_install" = "true" ]
>  	then
> -            case "$dbc_dbtype" in
> -		sqlite3)
> -                    SQL_CONNECTION="sqlite:///$dbc_basepath/$dbc_dbname"
> -                    ;;
> -		mysql)
> -                    [ -n "$dbc_dbport" ] && dbport=:$dbc_dbport
> -                    SQL_CONNECTION="mysql://$dbc_dbuser:$dbc_dbpass@${dbc_dbserver:-localhost}$dbport/$dbc_dbname"
> -                    ;;
> -		pgsql)
> -                    [ -n "$dbc_dbport" ] && dbport=:$dbc_dbport
> -                    SQL_CONNECTION="pgsql://$dbc_dbuser:$dbc_dbpass@${dbc_dbserver:-localhost}$dbport/$dbc_dbname"
> -                    ;;
> -		*)
> -                    SQL_CONNECTION="sqlite:////var/lib/keystone/$dbc_dbname"
> -                    ;;
> -            esac
> +	    if [ "$dbc_dbtype" = "mysql" ] || [ "$dbc_dbtype" = "pgsql" ] ; then
> +	        [ -n "$dbc_dbport" ] && dbport=:$dbc_dbport
> +	        SQL_CONNECTION="$dbc_dbtype://$dbc_dbuser:$dbc_dbpass@${dbc_dbserver:-localhost}$dbport/$dbc_dbname"
> +	    else
> +	        SQL_CONNECTION="sqlite:///$dbc_basepath/$dbc_dbname"
> +	    fi
>  
>              sed -e "s,^connection\s*=\s*.\+$,connection = $SQL_CONNECTION," -i /etc/keystone/keystone.conf
>  
> @@ -54,12 +40,9 @@ then
>  	fi
>      fi
>  
> -    if [ -z "$2" ]
> -    then
> -	db_get keystone/auth-token
> -	AUTH_TOKEN=${RET:-ADMIN}
> -	sed -s "s,^admin_token = ADMIN,admin_token = $AUTH_TOKEN," -i /etc/keystone/keystone.conf
> -    fi
> +    db_get keystone/auth-token
> +    AUTH_TOKEN=${RET:-ADMIN}
> +    sed -ie 's|^[ \t]*admin_token[ \t]*=.*|admin_token = '${AUTH_TOKEN}'|' /etc/keystone/keystone.conf
>  
>      chown keystone:keystone -R /var/lib/keystone /var/log/keystone /etc/keystone
>      chmod 0750 /etc/keystone
> diff --git a/debian/keystone.postrm b/debian/keystone.postrm
> index ca5d17f..402b5e3 100644
> --- a/debian/keystone.postrm
> +++ b/debian/keystone.postrm
> @@ -20,6 +20,7 @@ case "$1" in
>      purge)
>          rm -rf /var/log/keystone
>  	rm -rf /var/lib/keystone
> +	rm -rf /etc/keystone
>  esac
>  
>  #DEBHELPER#
> diff --git a/debian/rules b/debian/rules
> index 2f33685..fffc403 100755
> --- a/debian/rules
> +++ b/debian/rules
> @@ -42,6 +42,7 @@ override_dh_install:
>  	rm -rf debian/python-keystone/usr/lib/python*/*/doc
>  	rm -rf debian/python-keystone/usr/lib/python*/*/tools
>  	rm -rf debian/python-keystone/usr/lib/python*/*/examples
> +	cp -f etc/keystone.conf debian/keystone/usr/share/doc/keystone/keystone.conf.sample
>  
>  override_dh_clean:
>  	rm -rf $(CURDIR)/build $(CURDIR)/keystone.egg-info $(CURDIR)/.cache

I didn't look in detail, but the rest seems reasonable AFAICT.

Cheers,
Julien

Attachment: signature.asc
Description: Digital signature


Reply to: