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/
