[Pkg-openldap-devel] Bug#723957: slapd: commented olcDbDirectory config line causes unusable system and potential data loss on upgrade

[Ryan Tandy]

tags 723957 + patch
thanks

Hi Matt,

Thanks for reporting and triaging this bug. I'm sorry no one has
answered it until now.

On 21/09/13 09:13 AM, Matt Brown wrote:
> The get_directory method used in several maint scripts contains a
> bug that causes it to return multiple lines of output if a commented
> olcDbDirectory line also exists in the configuration file.

First, I should mention that manually editing the files belonging to
cn=config is explicitly not supported by upstream, and newer openldap
versions have notices to that effect in those files as well as logging
warnings if manual edits are detected (by a CRC check). Of course, that
doesn't make this upgrade failure any less of a bug.

> The callers of get_directory use filesystem existence checks on the
> output of get_directory to determine whether to actually backup the
> database, and silently continue without backing up when multiple
> lines of output are returned.

> The following patch (anchoring the match to start of line) would be a
> minimal fix for this critical issue, but a more proper fix would be
> for the preinst to bail out if it is unable to actually backup a
> database that it knows to exist from the config!

Agreed. I want to avoid re-introducing #584712, but I think checking
whether the olcDbDirectory attribute is present (the [ -n ] test) is
sufficient to decide whether a database has local files that need to be
backed up. The [ -d ] test lets cases like this go by silently, and
there's already code shortly after for catching the slapcat failure if
something actually does go wrong. So I'd propose this change in addition
to your suggestion:

--- slapd.preinst
+++ slapd.preinst
@@ -173,7 +173,7 @@
  	echo >&2 "  Dumping to $dir: "
  	(get_suffix | while read suffix; do
  		dbdir=`get_directory "$suffix"`
-		if [ -n "$dbdir" ] && [ -d "$dbdir" ]; then
+		if [ -n "$dbdir" ]; then
  			file="$dir/$suffix.ldif"
  			echo -n "  - directory $suffix... " >&2
  			# Need to support slapd.d migration from preinst

The anchoring bug you noticed is present in a few other spots too,
affecting for example olcSuffix.

Attached is a patch combining all the mentioned changes. It lets me
complete an upgrade when cn=config's files contain commented olcSuffix and
olcDbDirectory lines as well as a second database with no
olcDbDirectory, and falls through to the slapcat-failure case if the DB
directory is inaccessible.

IMO in general the maintainer scripts should be using slapcat to query
the config DB and not just looking into the files directly, which could
avoid this sort of issue, but there are probably implications I
haven't thought of.
-------------- next part --------------
diff -Nru openldap-2.4.39/debian/slapd.scripts-common openldap-2.4.39/debian/slapd.scripts-common
--- openldap-2.4.39/debian/slapd.scripts-common	2014-04-05 16:35:04.000000000 -0700
+++ openldap-2.4.39/debian/slapd.scripts-common	2014-04-05 16:33:38.000000000 -0700
@@ -165,7 +165,7 @@
 	echo >&2 "  Dumping to $dir: "
 	(get_suffix | while read suffix; do
 		dbdir=`get_directory "$suffix"`
-		if [ -n "$dbdir" ] && [ -d "$dbdir" ]; then
+		if [ -n "$dbdir" ]; then
 			file="$dir/$suffix.ldif"
 			echo -n "  - directory $suffix... " >&2
 			# Need to support slapd.d migration from preinst
@@ -275,14 +275,14 @@
 			sed -n -e's/^suffix[[:space:]]\+"*\([^"]\+\)"*/\1/p' $f
 		done
 	else
-		grep -h olcSuffix ${SLAPD_CONF}/cn\=config/olcDatabase*.ldif | cut -d: -f 2
+		grep -h ^olcSuffix ${SLAPD_CONF}/cn\=config/olcDatabase*.ldif | cut -d: -f 2
 	fi
 }
 # }}}
 get_directory() {							# {{{
 # Returns the db directory for a given suffix
 	if [ -d "${SLAPD_CONF}" ] && get_suffix | grep -q "$1" ; then
-		grep "olcDbDirectory:" `grep -l "olcSuffix: $1" ${SLAPD_CONF}/cn\=config/olcDatabase*.ldif` | cut -d: -f 2 | sed 's/^  *//g'
+		grep "^olcDbDirectory:" `grep -l "^olcSuffix: $1" ${SLAPD_CONF}/cn\=config/olcDatabase*.ldif` | cut -d: -f 2 | sed 's/^  *//g'
 	elif [ -f "${SLAPD_CONF}" ]; then
 		# Extract the directory for the given suffix ($1)
 		for f in `get_all_slapd_conf_files`; do