[Pkg-openldap-devel] RFR: Preliminary patch for cn=config support: new installs
Steve Langasek
vorlon at debian.org
Wed Jul 30 02:11:05 UTC 2008
On Mon, Jul 28, 2008 at 04:53:34PM -0400, Mathias Gug wrote:
> On Fri, Jul 18, 2008 at 03:25:21PM -0400, Mathias Gug wrote:
> >
> I've attached a new version of the patch: it mainly adds support to ask
> the end user to enter a password for the cn=config tree.
> Let me know what you think about it.
Looks like progress.
I've attached an incremental patch including some fixes; detailed comments
are available in-line below.
I've also published my patch as a bzr branch at
<lp:~vorlon/openldap/debian-cnconfig/>. (Note to
the team: Mathias's work is available at
<lp:~mathiaz/openldap/debian-cnconfig>).
And I've finished going through my backlog of email on this subject,
noticing for the first time that Matthijs had expressed a preference for
keeping around the support for slapd.conf. Matthijs, what are your present
thoughts? Do you think Mathias's implementation is robust enough that we
can forego the slapd.conf fallback, and move down the path of simplifying
the maintainer scripts? I feel that keeping slapd.conf around would be
"options for the sake of options", and that if we're going to support
cn=config we should get rid of our slapd.conf support ASAP...
=== modified file 'debian/slapd.postinst'
--- debian/slapd.postinst 2008-07-20 02:30:18 +0000
+++ debian/slapd.postinst 2008-07-28 20:37:35 +0000
<snip>
@@ -30,14 +30,48 @@
backup_config_once
echo done. >&2
- configure_v2_protocol_support
Defers the slapd config changes until after the cn=config migration; good.
if previous_version_older 2.4.7-4; then
if ! migrate_checkpoint_and_slurpd; then
db_input critical slapd/slurpd_obsolete || true
db_go || true
fi
- config_obsolete_schemacheck
+ if [ -f "${SLAPD_CONF}" ]; then
+ sed -i "s|^(schemacheck\s+)|#\$1|" "$SLAPD_CONF"
+ fi
+ fi
This looks like it might be a regression? slapd.conf can have includes, and
the schemacheck option could be buried in one of those, and if it is the
config import will fail. So unfortunately, I think we need to keep that
functionality around until after the lenny release.
+ # Migrate from slapd.conf to slapd.d/.
+ if previous_version_older 2.4.10-3 && [ -f "${SLAPD_CONF}" ]; then
^^^^^^^^
This will need incrementing now, since I've just uploaded 2.4.10-3 without
this patch.
+ mkdir /etc/ldap/slapd.d/
+ # Need to create an entry for the config database
+ # to set the rootdn and rootpw
+ # so that cn=config can be accessed.
+ db_get slapd/internal/cfgadminpw
Well, it's unfortunate that we don't seem to be able to reliably reuse the
existing value from the config, because that means more debconf templates to
be translated. :/ Should it be possible in some cases to query this
password from the slapd.conf+slapcat output, or is that too painful to
consider?
BTW, it looks like the only place the cfgadmin password is being wiped is in
wipe_admin_pass - and the cfgpassword{1,2} questions are never wiped - so
the config password is always going to be stored in the debconf database on
upgrades. I think we want to wipe this from the database once it's been
written out, don't we?
+ capture_diagnostics slaptest -f ${upgrade_slapd_conf} -F /etc/ldap/slapd.d/ || failed=1
+ if [ "$failed" ]; then
+ echo failed. >&2
+ echo >&2
+ cat <<-EOF
+Migrating slapd.conf file (${upgrade_slapd_conf}) to slapd.d failed with the following error while running slaptest:
+EOF
+ release_diagnostics " "
+ rm -rf /etc/ldap/slapd.d/
+ exit 1
+ fi
Would be nice if this were done as a debconf note, but let's concentrate on
making sure it just never fails instead. ;)
+ rm ${upgrade_slapd_conf}
+ mv ${SLAPD_CONF} ${SLAPD_CONF}.old
+ SLAPD_CONF=/etc/ldap/slapd.d
+ sed -i "s|^SLAPD_CONF=.\+|SLAPD_CONF=${SLAPD_CONF}|" /etc/default/slapd
fi
Ah, you've thought of everything here - I was just starting to wonder about
idempotent handling of /etc/default/slapd, and here I see you've done it. :)
@@ -59,7 +95,8 @@
# Versions prior to 2.4.7-1 could create a slapd.conf that wasn't
# readable by the openldap user.
- update_slapd_conf_permissions
+ update_permissions "${SLAPD_CONF}"
+
}
# }}}
Ah yes, very elegant that the database and slapd.conf now (have to) have the
same permissions :-)
=== modified file 'debian/slapd.config'
--- debian/slapd.config 2008-07-20 02:30:18 +0000
+++ debian/slapd.config 2008-07-28 20:37:35 +0000
Looks good to me.
@@ -161,6 +176,33 @@
if [ "$1" = configure ] && [ -n "$2" ]; then
configure_dumping
configure_allow_v2_binds
+ # Ask the cfg admin password when migrating to a cn=config environment.
+ if previous_version_older 2.4.10-3 && [ -f "${SLAPD_CONF}" ]; then
Another version bump needed, of course.
+ while true; do
+ db_input high slapd/cfgpassword1 || true
+ db_input high slapd/cfgpassword2 || true
+ db_go || true
+
+ # Make sure the passwords match
+ local pass1 pass2
+ db_get slapd/cfgpassword1
+ pass1="$RET"
+ db_get slapd/cfgpassword2
+ pass2="$RET"
+
+ if [ "$pass1" = "$pass2" ]; then
+ break;
+ fi
+ db_fset slapd/cfgpassword1 seen false
+ db_fset slapd/cfgpassword2 seen false
+ db_input critical slapd/password_mismatch
Should this clear out the values that were set in the previous round, the
way get_admin_password does? (Pathological scenario: the user ends up with
mismatched passwords set, aborts debconf, and reruns the configuration with
DEBIAN_FRONTEND=noninteractive, sending us into an infinite loop of debconf
error messages...)
+ db_go
+ done
+ db_get slapd/cfgpassword1
+ if [ ! -z "$RET" ]; then
+ db_set slapd/internal/cfgadminpw {crypt}`create_password_hash "$RET"`
+ fi
Technically, we could save ourselves a round trip to debconf by expanding
the scope of the pass1 variable and reusing it here.
=== added file 'debian/slapd.init.ldif'
--- debian/slapd.init.ldif 1970-01-01 00:00:00 +0000
+++ debian/slapd.init.ldif 2008-07-18 17:49:37 +0000
Note to self: run a comparison between a fresh install, and an upgrade from
a stock config, to see how the resulting config dirs differ.
=== modified file 'debian/slapd.postrm'
--- debian/slapd.postrm 2007-12-21 21:05:19 +0000
+++ debian/slapd.postrm 2008-07-18 17:49:37 +0000
@@ -22,6 +22,7 @@
if [ "$1" = "purge" ]; then
echo -n "Removing slapd configuration... "
rm -f /etc/ldap/slapd.conf 2>/dev/null || true
+ rm -rf /etc/ldap/slapd.d/ 2>/dev/null || true
rmdir /etc/ldap/schema || true
echo done
We should also take care of removing the /etc/ldap/slapd.conf.old file, if
any, that we created in the migration.
=== modified file 'debian/slapd.scripts-common'
--- debian/slapd.scripts-common 2008-07-20 02:30:18 +0000
+++ debian/slapd.scripts-common 2008-07-28 20:39:55 +0000
@@ -46,7 +46,7 @@
# Return success if yes.
# Usage: if database_format_changed; then
- if dpkg --compare-versions "$OLD_VERSION" lt-nl 2.4.7; then
+ if dpkg --compare-versions "$OLD_VERSION" le-nl 2.3.30-6; then
return 0
else
return 1
Should be reverted, as discussed on IRC.
update_databases_permissions() { # {{{
- parse_configuration_file
- for db in `get_database_list`; do
- dbdir=`get_directory $db`
+ for suffix in `get_suffix`; do
+ dbdir=`get_directory $suffix`
update_permissions "$dbdir"
chmod 0700 "$dbdir"
done
}
A good improvement. update_databases_permissions() is only called from the
postinst, so is not a case where we need to support slapd.conf in
get_suffix.
@@ -129,18 +113,22 @@
# If the user wants us to dump the databases they are dumped to the
# configured directory.
- local db suffix file dir failed
+ local db suffix file dir failed slapcat_opts
database_dumping_enabled || return 0
dir=`database_dumping_destdir`
echo >&2 " Dumping to $dir: "
- parse_configuration_file
- for db in `get_database_list`; do
- suffix=`get_suffix $db`
+ for suffix in `get_suffix`; do
As you rightly point out in your comments on get_suffix, dump_databases() is
called from the preinst and therefore needs to understand slapd.conf.
However, get_suffix doesn't understand slapd.conf include files; so I think
this is another case where we need to keep around the nasty inline perl
parser. :/
On the bright side, I think it means that get_suffix probably doesn't have
to support slapd.conf.
@@ -645,8 +478,8 @@
echo >&2 " Moving old database directory to /var/backups:"
move_old_database_away /var/lib/ldap
fi
+ create_ldap_directories
create_new_slapd_conf "$basedn" "$backend"
- create_ldap_directories
create_new_directory "$basedn" "$dc"
# Put the right permissions on this directory.
What's the reason for this change?
@@ -658,62 +491,52 @@
}
# }}}
create_new_slapd_conf() { # {{{
-# Creates a new slapd.conf for the suffix given
+# Creates a new slapd.d configurtion for the suffix given
# Usage: create_new_slapd_conf <basedn> <backend>
s/configurtion/configuration/ :)
+ # Need to have a version of the backend name whith the first
+ # letter capitalize (ex: olcBdbConfig or olcHdbConfig) to set
+ # the correct objectClass attribute in the db configuration.
s/whith/with/; s/capitalize/&d/
+ capture_diagnostics slapadd -F "${SLAPD_CONF}" \
+ -b "cn=config" -l ${init_ldif} || failed=1
+ if [ "$failed" ]; then
+ echo failed. >&2
echo >&2
<snip>
+ cat <<-EOF
+Loading the initial configuration from the ldif file (${init_ldif}) failed with the following
+error while running slapadd:
EOF
+ release_diagnostics " "
exit 1
fi
Isn't the "failed" line redundant?
Why does this take out the warning about a missing
/etc/ldap/schema/core.schema? Should it be replaced with a warning about
core.ldif...?
@@ -735,49 +558,37 @@
adminpass="$RET"
echo -n " Creating initial LDAP directory... " >&2
-
- cat <<-EOF | noisy_slapadd
- dn: $basedn
- objectClass: top
- objectClass: dcObject
- objectClass: organization
- o: $organization
- dc: $dc
-
- dn: cn=admin,$basedn
- objectClass: simpleSecurityObject
- objectClass: organizationalRole
- cn: admin
- description: LDAP administrator
- userPassword: {crypt}$adminpass
- EOF
-
+ init_ldif=`mktemp /tmp/slapd_init_dir.ldif.XXXXXXXXXX`
+ echo "dn: $basedn
+objectClass: top
+objectClass: dcObject
+objectClass: organization
+o: $organization
+dc: $dc
+
+dn: cn=admin,$basedn
+objectClass: simpleSecurityObject
+objectClass: organizationalRole
+cn: admin
+description: LDAP administrator
+userPassword: $adminpass
+" > ${init_ldif}
+ capture_diagnostics slapadd -F "${SLAPD_CONF}" \
+ -b "${basedn}" -l ${init_ldif} || failed=1
+ if [ "$failed" ]; then
+ echo failed. >&2
+ echo >&2
+ cat <<-EOF
+Loading the initial directory structure from the ldif file (${init_ldif}) failed with the following
+error while running slapadd:
+EOF
+ release_diagnostics " "
+ exit 1
+ fi
+ rm "${init_ldif}"
echo "done." >&2
}
# }}}
This particular change removes the need for noisy_slapadd to exist at all,
but it also leaves behind tmp files on failure (as does the changed
create_new_slapd_conf() function). I think that in the case of
create_new_directory where the ldif is small enough to fit on a screen, the
original behavior might have been better, so we don't have to leave tmp
files around for the user to inspect if they want to understand what
happened.
@@ -787,88 +598,20 @@
db_get slapd/allow_ldap_v2
if [ "$RET" != "true" ]; then return 0; fi
Hrm; not your bug, but apparently we don't have a way to /disable/ v2
support via debconf... we ought to fix that.
@@ -910,7 +655,7 @@
db_get slapd/password1
if [ ! -z "$RET" ]; then
- db_set slapd/internal/adminpw `create_password_hash "$RET"`
+ db_set slapd/internal/adminpw {crypt}`create_password_hash "$RET"`
fi
}
Well, c.f. Debian bug #490930. :)
=== modified file 'debian/slapd.templates'
--- debian/slapd.templates 2008-07-20 02:30:18 +0000
+++ debian/slapd.templates 2008-07-28 20:37:35 +0000
@@ -73,6 +73,17 @@
Please enter the admin password for your LDAP directory again to verify
that you have typed it correctly.
+Template: slapd/cfgpassword1
+Type: password
+_Description: Config tree administrator password:
+ Please enter the password for the administrator of the configuration tree.
+
+Template: slapd/cfgpassword2
+Type: password
+_Description: Confirm password:
+ Please enter the admin password for the administrator of the configuration
+ tree again to verify that you have typed it correctly.
+
Redundancy here; we should say either:
Please enter the admin password for the configuration tree again to verify
that you have typed it correctly.
or:
Please enter the password for the administrator fo the configuration tree
again to verify that you have typed it correctly.
Type: note
_Description: Password mismatch
@@ -88,6 +99,11 @@
Description: Encrypted admin password:
Internal template, should never be displayed to users.
+Template: slapd/internal/cfgadminpw
+Type: password
+Description: Encrypted config admin password:
+ Internal template, should never be displayed to users.
+
Do we actually need a separate internal question for this, or could it be
stored in the existing slapd/internal/adminpw question? AFAICS, with the
current implementation they're never different, and I don't see anywhere
that adminpw would be cleared before we're done with it.
In fact, it looks like create_new_slapd_conf doesn't even use the
cfgadminpw, it uses slapd/internal/adminpw... :)
Otherwise, this looks good to me.
Thanks!
--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
slangasek at ubuntu.com vorlon at debian.org
More information about the Pkg-openldap-devel
mailing list