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

2012-09-30 Thread Thomas Goirand 

On 09/29/2012 09:37 PM, Julien Cristau wrote:

On Thu, Sep 27, 2012 at 15:16:06 +0800, Thomas Goirand wrote:


On 09/26/2012 02:59 AM, Julien Cristau wrote:

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.


Right, what was I thinking. Of course. (I knew something was wrong
there. shame on me :/)

I uploaded the fixed package to SID and will ask for an unblock in 10 days.


In the postinst, this:

+   if ! [ -e ${KEY_CONF} ] ; then
+   cp /usr/share/doc/keystone/keystone.conf.sample ${KEY_CONF}
 fi

violates policy 12.3:

  Packages must not require the existence of any files in
  `/usr/share/doc/' in order to function [1].  Any files that are
  referenced by programs but are also useful as stand alone
  documentation should be installed under `/usr/share/package/' with
  symbolic links from `/usr/share/doc/package'.

see also 10.7.3, which suggests to put sample config files used this way
in /usr/share/package.

Cheers,
Julien


Hi,

Thanks for the advice. I will do the necessary changes soon.

I have already fixed the scripting in the config script as per your 
other email (I did test it, but not enough it seems, I believe it's now ok).


Cheers,

Thomas


--
To UNSUBSCRIBE, email to debian-release-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/506816cc.7030...@debian.org

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

2012-09-29 Thread Julien Cristau 
On Thu, Sep 27, 2012 at 15:16:06 +0800, Thomas Goirand wrote:

 On 09/26/2012 02:59 AM, Julien Cristau wrote:
 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.
 
 Right, what was I thinking. Of course. (I knew something was wrong
 there. shame on me :/)
 
 I uploaded the fixed package to SID and will ask for an unblock in 10 days.
 
At least this:

+   KEY_CONF_BEFORE_AT=`echo ${KEY_CONF_ADDR} | cut -d@ -f1`
+   KEY_CONF_AFTER_AT=`echo ${KEY_CONF_ADDR} | cut -d@ -f1`

looks broken.  Did you actually test the script?

Cheers,
Julien


signature.asc
Description: Digital signature

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

2012-09-29 Thread Julien Cristau 
On Thu, Sep 27, 2012 at 15:16:06 +0800, Thomas Goirand wrote:

 On 09/26/2012 02:59 AM, Julien Cristau wrote:
 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.
 
 Right, what was I thinking. Of course. (I knew something was wrong
 there. shame on me :/)
 
 I uploaded the fixed package to SID and will ask for an unblock in 10 days.
 
In the postinst, this:

+   if ! [ -e ${KEY_CONF} ] ; then
+   cp /usr/share/doc/keystone/keystone.conf.sample ${KEY_CONF}
fi

violates policy 12.3:

 Packages must not require the existence of any files in
 `/usr/share/doc/' in order to function [1].  Any files that are
 referenced by programs but are also useful as stand alone
 documentation should be installed under `/usr/share/package/' with
 symbolic links from `/usr/share/doc/package'.

see also 10.7.3, which suggests to put sample config files used this way
in /usr/share/package.

Cheers,
Julien


signature.asc
Description: Digital signature

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

2012-09-27 Thread Thomas Goirand 

On 09/26/2012 02:59 AM, Julien Cristau wrote:

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.


Right, what was I thinking. Of course. (I knew something was wrong 
there. shame on me :/)


I uploaded the fixed package to SID and will ask for an unblock in 10 days.

Thomas


--
To UNSUBSCRIBE, email to debian-release-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/5063fd36.8010...@debian.org

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

2012-09-26 Thread Russ Allbery 
Thomas Goirand z...@debian.org writes:

 Thanks for reviewing the patch, and spotting the leftover set -x in the
 postinst which I missed switching off after my tests.

 On 09/26/2012 02:59 AM, Julien Cristau wrote:
 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.

 Is this a must, a should, or your own packaging habits? Is it
 completely forbidden to do how I did?

config can run during preinst, so you generally should not do anything
that manipulates the file system during config since you can't be
guaranteed that the package is unpacked or that any of its dependencies
are available.  See debconf-devel(7):

The config script should not need to modify the filesystem at all. It
just examines the state of the system, and asks questions, and debconf
stores the answers to be acted on later by the postinst script.
Conversely, the postinst script should almost never use debconf to ask
questions, but should instead act on the answers to questions asked by
the config script.

I'm not sure that I would say completely forbidden, but it's something
to avoid unless there's some overriding requirement for config to work
that way.

-- 
Russ Allbery (r...@debian.org)   http://www.eyrie.org/~eagle/


-- 
To UNSUBSCRIBE, email to debian-release-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/87fw65i9il@windlord.stanford.edu

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

2012-09-26 Thread Julien Cristau 
On Wed, Sep 26, 2012 at 13:47:21 +0800, Thomas Goirand wrote:

 On 09/26/2012 02:59 AM, Julien Cristau wrote:
 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.
 
 Is this a must, a should, or your own packaging habits? Is it
 completely forbidden to do how I did?
 
at least the cp /usr/share/doc/keystone/keystone.conf.sample
${KEY_CONF} can't be done in config because the package is not yet
unpacked at that stage, you only have the Essential set.

Cheers,
Julien


signature.asc
Description: Digital signature

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

2012-09-25 Thread Thomas Goirand 

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 z...@debian.org  Wed, 12 Sep 2012 17:09:47 +
+
 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
+
+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

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

2012-09-25 Thread Julien Cristau 
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 z...@debian.org  Wed, 12 Sep 2012 17:09:47 +
 +
  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

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

2012-09-25 Thread Thomas Goirand 

Hi,

Thanks for reviewing the patch, and spotting the leftover set -x in the 
postinst which I missed switching off after my tests.


On 09/26/2012 02:59 AM, Julien Cristau wrote:

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.


Is this a must, a should, or your own packaging habits? Is it 
completely forbidden to do how I did?


Thomas


--
To UNSUBSCRIBE, email to debian-release-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Archive: http://lists.debian.org/506296e9.9060...@debian.org