On Fri, Feb 04, 2011 at 04:16:23PM +0100, jer...@intuxicated.org wrote: > Hi, > > I've updated the resource agent so that it tries to establish an LDAP > connection. Most of the issues where resolved, but I haven't had the time > yet to test with other OSes so I changed /bin/sh to /bin/bash. I'll try to > remove bashisms as soon as possible. > > Jeroen
Thanks. This is no actual review, but only a suggestion to cut down on the "unwieldy for loop" Dejan complained about. > # > # slapd_validate_dirs: retrieves all slapd database directories from the > # configuration file(s) and verifies existence and > # permissions. > # > slapd_validate_dirs() > { > local dir > local dirs > local groups=`groups $user | sed -ne 's/^[^:]\+: \(.*\)$/\1/p' | sed 's/ > \+/\\\|/' 2>/dev/null` > local perms > local result > local state > > > [ "$user" = "root" ] && return $OCF_SUCCESS Why would a world writeable data directory not be a problem if the service runs as root? > IFS=$NEWLINE > > if [ -d "$config_file" ]; then > for dir in `find "$config_file"/'cn=config' -type f -name olcDatabase* > -exec \ Careful, unescaped *, don't do that. Will break in completely non-obvious ways if for some obscure reason in the current workingdirectory there are files named olcDatabase-something. > sed -ne 's/^olcDbDirectory:[[:space:]]\+\(.\+\)/\1/p' {} \;` find | xargs sed ? Sure that there is no leading space allowed? /me is outing himself has ignorant to ldap configuration syntax. > do > dir=${dir#\"*} no * necessary, actually the * is misleading here. > dir=${dir%\"*} the same > dirs="$dirs$NEWLINE$dir" what do you $NEWLINE for, there? do you want to allow for spaces in directories, but no newline? > done why not strip the quotes in sed as well, so you can say "dirs=$(whatever)" ? this should be equivalent to this for loop: dirs=$(find "$config_file/cn=config" -type f -name "olcDatabase*" -print0 | xargs -0 sed -n -e '/^olcDbDirectory:[[:space:]]/!d;' \ -e 's/^[^:]*:[[:space:]]*//;s/^"//;s/"$//;p') > elif [ -f "$config_file" ]; then > for dir in `sed -ne 's/^directory[[:space:]]\+\(.\+\)/\1\n/p' > "$config_file"` > do > dir=${dir#\"*} > dir=${dir%\"*} > dirs="$dirs$NEWLINE$dir" > done the same, dirs=$(sed -n -e '/^directory[[:space:]]/!d;' \ -e 's/^[a-z]*[[:space:]]*//;s/^"//;s/"$//;p' \ < "$config_file") what about the else ? no -d, and no -f either, probably not existing (ignore specials for a second). That's not good either? > fi > > state=$OCF_SUCCESS > > for dir in $dirs; do > IFS=$ORIG_IFS In case you actually wanted to allow spaces (but no newlines) in directory names, as I believe you attempted, with bash you'd do ORIG_IFS=$IFS IFS=$'\n' dirs=($( .... sed ... )) IFS=$ORIG_IFS and then later again for dir in "${dirs[@]}"; do ... But see below. > perms=`stat -c "%U:%G:%a" "$dir"`; result=$? > > > if [ $result -eq 0 ]; then > > echo "$perms" | grep "$user:[^:]\+:7.." >/dev/null 2>&1; result=$? > > if [ $result -ne 0 ]; then > > if [ -n "$groups" ]; then > echo "$perms" | grep "[^:]\+:\($group\|$groups\):.7." >/dev/null > 2>&1; result=$? > else > echo "$perms" | grep "[^:]\+:$group:.7." >/dev/null 2>&1; result=$? > fi > > > if [ $result -ne 0 ]; then > > echo "$perms" | grep ":..7" >/dev/null 2>&1; result=$? > > if [ $result -ne 0 ]; then > state=$OCF_ERR_GENERIC > ocf_log err "slapd data directory '$dir' is not writable." > else > ocf_log warn "slapd data directory '$dir' is world writable. Mode > 0700 recommended." > fi > else > ocf_log warn "slapd data directory '$dir' is group writable. Mode > 0700 recommended." > fi > > else > ocf_log warn "slapd data directory '$dir' is accessible by others. > Mode 0700 recommended." > fi > fi > > IFS=$NEWLINE > done > > IFS=$ORIG_IFS > > return $state > } If I read that correctly, it checks if some directories are * in some way writeable by $user * complain, if dir is not mode 0700 and owned by $user * does not care if it exists (was that intentional?) dirs=$( find $dirs -maxdepth 0 -not \( -type d -user $user -perm 0700 \) ) for dir in $dirs; do if ! su $user -s /bin/sh -c "test -w '$dir'" ; then state=$OCF_ERR_GENERIC ocf_log err "... $dir not writable by $user ..." else ocf_log warn "... consider to 'chmod 0700 $dir; chown $user $dir;'" fi done Note about spaces in path names... gets more ugly, but doable. You need to decide wether you want to allow something that no-one is actually doing, ever ;-) and simply refuse it, in step 1, when gathering the list of directories to check. -- : Lars Ellenberg : LINBIT | Your Way to High Availability : DRBD/HA support and consulting http://www.linbit.com DRBD® and LINBIT® are registered trademarks of LINBIT, Austria. _______________________________________________________ Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev Home Page: http://linux-ha.org/