Re: [Linux-ha-dev] Patch for anything RA
Hi,
On Tue, Jul 06, 2010 at 08:52:12PM +0200, Lars Ellenberg wrote:
On Tue, Jul 06, 2010 at 06:36:14PM +0200, Dejan Muhamedagic wrote:
Hi Yuusuke-san,
On Wed, Jun 30, 2010 at 08:00:18PM +0900, Yuusuke IIDA wrote:
Hi,
For anything RA, I revised it with some function addition.
The list of the change is as follows.
* I added the option which could choose whether you used a login shell
to want
to let a command inherit an environment variable of Resource Agent.
OK, I assume that this may be useful at times. Though I'm not
very happy with the new parameter name, I couldn't come up with
another one. The big difference is, I guess, that the .profile
files are sourced. Perhaps to name it just login_shell?
the difference is that su - user clears the environment first
(and then re-populates it from where that user usually gets his environment),
su user (no dash) does not clear, but inherit the current environment.
OK.
* I revised it to handle an escape character in character string set by
cmdline_options such as follows adequately.
--- for example: ---
primitive A ocf:heartbeat:anything \
params \
binfile=X \
cmdline_options=-V -c \openssl des-ede3 -d -base64 -k 'yy y'\
-i \
--- ---
Uh, this escaping gives me headache.
should this not be much easier by doing
- cmd=su -c \$variables\
+ cmd=su -c '$variables'
?
no escaping by sed necessary,
except maybe (if you are paranoid)
escaping of ' itself:
sed -e s/'/'''/
Good, I'd really rather avoid trying to escape stuff in the user
data if possible. Yuusuke-san, can you please test and see if
this works for you. Then we can perhaps advise accordingly in the
meta-data.
As long as we do cmd=su -c \$variable\, it is not sufficient to
escape \ (as the proposed patch by Yuusuke-san does), actually you'd
need to escape ` and $ and various other things as well.
Unless you consider it a feature that these would be
expanded already in the context of the eval running as root,
not in the context of the su $user nohup.
Which is (as it is now) a potential root exploit,
once you start taking cib admin != cluster root serious.
which is not really sensible to do IMO, anyways. But I digres.
Hm. Maybe we should move the eval into that context, anyways?
sort of
cmd=eval '${supposedly_properly_escaped_variable}'
su ... -c $cmd ?
Hmm, I hope that the users could skip the acrobatics and do the
processing elsewhere if absolutely needed.
But, for the record:
The line says:
+cmdline_options=`... | sed 's/\\\/\\\/g' | ...`
How does the left side expand? Shouldn't that be an even number
of backslashes? The right side also has 7 backslashes.
the first stripping of \ is done by the shell,
before feeding the whole thing to the `` subshell.
And the \ quoting within `` is subtle:
backslash retains its literal meaning except when followed by $, `, or \
so those \/ combinations could have been written as \\/ as well
(if only to reduce the headache of the reader, slightly)
but need not be. BTW, that is one of the differences between `` and $() ...
yep, its not pretty, but correct, though not necessarily consistent
between various shells and versions :(
god, I hate it when I know these useless facts from the top of my head,
I wish I had done less shell coding ;-)
:)
Cheers,
Dejan
* Strip off the trailing clone marker.
- quotations from the following.
http://hg.clusterlabs.org/pacemaker/stable-1.0/file/94515b3503b5/extra/resources/Dummy#l143
OK.
Can you please split the patch in three parts, so that we have
unrelated changes in signel patches.
Yes, please ;-)
--
: 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/
___
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/
Re: [Linux-ha-dev] Patch for anything RA
Hi,
I divided a patch.
(2010/07/07 17:59), Dejan Muhamedagic wrote:
Hi,
On Tue, Jul 06, 2010 at 08:52:12PM +0200, Lars Ellenberg wrote:
On Tue, Jul 06, 2010 at 06:36:14PM +0200, Dejan Muhamedagic wrote:
Hi Yuusuke-san,
On Wed, Jun 30, 2010 at 08:00:18PM +0900, Yuusuke IIDA wrote:
Hi,
For anything RA, I revised it with some function addition.
The list of the change is as follows.
* I added the option which could choose whether you used a login shell to want
to let a command inherit an environment variable of Resource Agent.
OK, I assume that this may be useful at times. Though I'm not
very happy with the new parameter name, I couldn't come up with
another one. The big difference is, I guess, that the .profile
files are sourced. Perhaps to name it just login_shell?
the difference is that su - user clears the environment first
(and then re-populates it from where that user usually gets his environment),
su user (no dash) does not clear, but inherit the current environment.
OK.
* I revised it to handle an escape character in character string set by
cmdline_options such as follows adequately.
--- for example: ---
primitive A ocf:heartbeat:anything \
params \
binfile=X \
cmdline_options=-V -c \openssl des-ede3 -d -base64 -k 'yy y'\ -i \
--- ---
Uh, this escaping gives me headache.
should this not be much easier by doing
- cmd=su -c \$variables\
+ cmd=su -c '$variables'
?
no escaping by sed necessary,
except maybe (if you are paranoid)
escaping of ' itself:
sed -e s/'/'''/
Good, I'd really rather avoid trying to escape stuff in the user
data if possible. Yuusuke-san, can you please test and see if
this works for you. Then we can perhaps advise accordingly in the
meta-data.
I tested this processing.
There was not a problem.
Many thanks,
Yuusuke.
As long as we do cmd=su -c \$variable\, it is not sufficient to
escape \ (as the proposed patch by Yuusuke-san does), actually you'd
need to escape ` and $ and various other things as well.
Unless you consider it a feature that these would be
expanded already in the context of the eval running as root,
not in the context of the su $user nohup.
Which is (as it is now) a potential root exploit,
once you start taking cib admin != cluster root serious.
which is not really sensible to do IMO, anyways. But I digres.
Hm. Maybe we should move the eval into that context, anyways?
sort of
cmd=eval '${supposedly_properly_escaped_variable}'
su ... -c $cmd ?
Hmm, I hope that the users could skip the acrobatics and do the
processing elsewhere if absolutely needed.
But, for the record:
The line says:
+cmdline_options=`... | sed 's/\\\/\\\/g' | ...`
How does the left side expand? Shouldn't that be an even number
of backslashes? The right side also has 7 backslashes.
the first stripping of \ is done by the shell,
before feeding the whole thing to the `` subshell.
And the \ quoting within `` is subtle:
backslash retains its literal meaning except when followed by $, `, or \
so those \/ combinations could have been written as \\/ as well
(if only to reduce the headache of the reader, slightly)
but need not be. BTW, that is one of the differences between `` and $() ...
yep, its not pretty, but correct, though not necessarily consistent
between various shells and versions :(
god, I hate it when I know these useless facts from the top of my head,
I wish I had done less shell coding ;-)
:)
Cheers,
Dejan
* Strip off the trailing clone marker.
- quotations from the following.
http://hg.clusterlabs.org/pacemaker/stable-1.0/file/94515b3503b5/extra/resources/Dummy#l143
OK.
Can you please split the patch in three parts, so that we have
unrelated changes in signel patches.
Yes, please ;-)
--
: 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/
___
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/
--
METRO SYSTEMS CO., LTD
YuusukeIida
Mail: iiday...@intellilink.co.jp
diff -r 8cb5ba3e1d97 heartbeat/anything
--- a/heartbeat/anything Fri Jun 25 19:54:24 2010 +0200
+++ b/heartbeat/anything Wed Jul 07 14:42:44 2010 +0900
@@ -46,6 +46,7 @@
# does something and then exits.
# Initialization:
+: ${OCF_RESKEY_login_shell:=true}
: ${OCF_FUNCTIONS_DIR=${OCF_ROOT}/resource.d/heartbeat}
. ${OCF_FUNCTIONS_DIR}/.ocf-shellfuncs
@@ -74,14 +75,14 @@
if [ -n $logfile -a -n $errlogfile ]
then
# We have logfile and errlogfile, so redirect STDOUT und
Re: [Linux-ha-dev] Patch for anything RA
On Wed, Jul 07, 2010 at 07:19:55PM +0900, Yuusuke IIDA wrote:
should this not be much easier by doing
- cmd=su -c \$variables\
+ cmd=su -c '$variables'
?
no escaping by sed necessary,
except maybe (if you are paranoid)
escaping of ' itself:
sed -e s/'/'''/
Good, I'd really rather avoid trying to escape stuff in the user
data if possible.
Then maybe we should enforce that the RESKEYs do not contain
shell meta characters at all.
Which would require the user to write a proper wrapper script
for anything more fancy than a few command line options,
and not try to inline shellscripts as oneliners into the cib.
I'd very much prefer that option.
Yuusuke-san, can you please test and see if
this works for you. Then we can perhaps advise accordingly in the
meta-data.
I tested this processing.
There was not a problem.
No, you did something else. See below.
diff -r 8cb5ba3e1d97 heartbeat/anything
--- a/heartbeat/anything Fri Jun 25 19:54:24 2010 +0200
+++ b/heartbeat/anything Wed Jul 07 14:42:44 2010 +0900
@@ -46,6 +46,7 @@
# does something and then exits.
# Initialization:
+: ${OCF_RESKEY_login_shell:=true}
would not preserve environment better match the intended use case?
also there is a dedicated option to su with that name (and function).
so we would still do su - $user -c ..., but optionally as
su - --preserve-environment $user -c ...
(or whatever the most compatible short options are accros platforms)
You do:
- cmd=su - $user -c \nohup $binfile $cmdline_options
$logfile 2 $errlogfile \'echo \$!'
+ cmd=su $login_shell $user -c \nohup $binfile
$cmdline_options $logfile 2 $errlogfile \'echo \$!'
-cmdline_options=$OCF_RESKEY_cmdline_options
+cmdline_options='$OCF_RESKEY_cmdline_options'
which effectively places the literal, unexpanded
$OCF_RESKEY_cmdline_options into cmd, and only the eval later will
actually expand it. That is different from what I suggested.
This would be more obvious if you did
+ cmd=su $login_shell $user -c \nohup $binfile \$OCF_RESKEY_cmdline_options
$logfile 2 $errlogfile \'echo \$!'
But now the debug echo $cmd will not actually show what command is going to
be executed,
only print the variable name...
And you still need to escape, think of
cmdline_options=; echo thanks::0:0 /etc/passwd; true
does not need to be evil on purpose, maybe just accidentally contains
shell meta characters, or imbalanced quoting while figuring out how to
best write mini shellscripts in cib parameters...
leading to ugly side effects that will go unnoticed for a long time.
What I suggested was:
cmdline_options=$(echo $OCF_RESKEY_cmdline_options | sed -e s/'/'''/)
(not ``; if you insist on ``, you need to
double the backslashes in sed yet again; I think)
# btw, bash would do: t=';
cmdline_options=${OCF_RESKEY_cmdline_options//$t/$t\\$t$t}
# or use printf -v cmdline_options %q $OCF_RESKEY_cmdline_options,
# and leave off the enclosing '' later...
# so many ways to do get it wrong ;-)
(actually, we would also need to escape $binfile in the same fashion)
and then put the eval into the su command, and only to word-split
cmdline_options.
something like:
su $login_shell $user -c eval set -- '$cmdline_options'; nohup '$binfile'
\\...@\ \$logfile\ 2 \$errlogfile\ echo \$!
(pending typos...)
which still allows for embeded subshells, but they are much less likely
done by accident. It does no longer allow for concatenation (; more;
commands; added;) or redirection.
I could throw together an actual patch implementing these suggestions,
but as I said above, I'd much prefer to simply not do any escaping,
but require the user to write wrapper scripts if he needs something more
fancy than cmdline_options=--future --type=uncertain.
Basically
case $OCF_RESKEY_cmdline_options in
# possibly I forgot some harmless chars, add those to the whitelist.
# but rather whitelist and forget some than blacklist and forget some.
# most definetely do not whitelist: '*?#`$()[]
*[!\ a-zA-Z0-9/_.%,=+-]*)
exit $OCF_ERROR_ARGS ;;
esac
and the same for binfile.
Less potential to get it wrong in the cib.
Less confusiong quoting and escaping levels,
both in the cib and in the resource agent.
--
: 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/
Re: [Linux-ha-dev] Patch for anything RA
Hi Yuusuke-san,
On Wed, Jun 30, 2010 at 08:00:18PM +0900, Yuusuke IIDA wrote:
Hi,
For anything RA, I revised it with some function addition.
The list of the change is as follows.
* I added the option which could choose whether you used a login shell to
want
to let a command inherit an environment variable of Resource Agent.
OK, I assume that this may be useful at times. Though I'm not
very happy with the new parameter name, I couldn't come up with
another one. The big difference is, I guess, that the .profile
files are sourced. Perhaps to name it just login_shell?
* I revised it to handle an escape character in character string set by
cmdline_options such as follows adequately.
--- for example: ---
primitive A ocf:heartbeat:anything \
params \
binfile=X \
cmdline_options=-V -c \openssl des-ede3 -d -base64 -k 'yy y'\ -i \
--- ---
Uh, this escaping gives me headache. The line says:
+cmdline_options=`... | sed 's/\\\/\\\/g' | ...`
How does the left side expand? Shouldn't that be an even number
of backslashes? The right side also has 7 backslashes.
* Strip off the trailing clone marker.
- quotations from the following.
http://hg.clusterlabs.org/pacemaker/stable-1.0/file/94515b3503b5/extra/resources/Dummy#l143
OK.
Can you please split the patch in three parts, so that we have
unrelated changes in signel patches.
Cheers,
Dejan
Best Regards,
Yuusuke IIDA
--
METRO SYSTEMS CO., LTD
YuusukeIida
Mail: iiday...@intellilink.co.jp
--- anything 2010-06-30 19:53:06.0 +0900
+++ ../extra/anything 2010-06-30 19:55:42.0 +0900
@@ -74,14 +74,14 @@
if [ -n $logfile -a -n $errlogfile ]
then
# We have logfile and errlogfile, so redirect STDOUT
und STDERR to different files
- cmd=su - $user -c \nohup $binfile $cmdline_options
$logfile 2 $errlogfile \'echo \$!'
+ cmd=su $use_login_shell $user -c \nohup $binfile
$cmdline_options $logfile 2 $errlogfile \'echo \$!'
else if [ -n $logfile ]
then
# We only have logfile so redirect STDOUT and
STDERR to the same file
- cmd=su - $user -c \nohup $binfile
$cmdline_options $logfile 21 \'echo \$!'
+ cmd=su $use_login_shell $user -c \nohup
$binfile $cmdline_options $logfile 21 \'echo \$!'
else
# We have neither logfile nor errlogfile, so
we're not going to redirect anything
- cmd=su - $user -c \nohup $binfile
$cmdline_options \'echo \$!'
+ cmd=su $use_login_shell $user -c \nohup
$binfile $cmdline_options \'echo \$!'
fi
fi
ocf_log debug Starting $process: $cmd
@@ -165,15 +165,24 @@
fi
}
-# FIXME: Attributes special meaning to the resource id
-process=$OCF_RESOURCE_INSTANCE
+if [ ${OCF_RESKEY_CRM_meta_globally_unique} = false ]; then
+ # Strip off the trailing clone marker
+ process=`echo ${OCF_RESOURCE_INSTANCE} | sed s/:[0-9][0-9]//`
+else
+ process=$OCF_RESOURCE_INSTANCE
+fi
binfile=$OCF_RESKEY_binfile
-cmdline_options=$OCF_RESKEY_cmdline_options
+cmdline_options=`echo $OCF_RESKEY_cmdline_options | sed 's/\\\/\\\/g'
| sed 's/\/\\\/g'`
pidfile=$OCF_RESKEY_pidfile
[ -z $pidfile ] pidfile=${HA_VARRUN}/anything_${process}.pid
logfile=$OCF_RESKEY_logfile
errlogfile=$OCF_RESKEY_errlogfile
user=$OCF_RESKEY_user
+if [ ocf_is_true $OCF_RESKEY_use_login_shell ]; then
+ use_login_shell=-
+else
+ use_login_shell=
+fi
[ -z $user ] user=root
anything_validate() {
@@ -268,6 +277,13 @@
shortdesc lang=enSeconds to wait after having sent SIGTERM before
sending SIGKILL in stop operation/shortdesc
content type=string default=/
/parameter
+parameter name=use_login_shell
+longdesc lang=en
+It is setting to decide whether you use a login shell in a user carrying out
a command.
+/longdesc
+shortdesc lang=enSetting whether or not I use a login shell/shortdesc
+content type=string default=true/
+/parameter
/parameters
actions
action name=start timeout=20s /
___
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/
___
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/
Re: [Linux-ha-dev] Patch for anything RA
On Tue, Jul 06, 2010 at 06:36:14PM +0200, Dejan Muhamedagic wrote:
Hi Yuusuke-san,
On Wed, Jun 30, 2010 at 08:00:18PM +0900, Yuusuke IIDA wrote:
Hi,
For anything RA, I revised it with some function addition.
The list of the change is as follows.
* I added the option which could choose whether you used a login shell to
want
to let a command inherit an environment variable of Resource Agent.
OK, I assume that this may be useful at times. Though I'm not
very happy with the new parameter name, I couldn't come up with
another one. The big difference is, I guess, that the .profile
files are sourced. Perhaps to name it just login_shell?
the difference is that su - user clears the environment first
(and then re-populates it from where that user usually gets his environment),
su user (no dash) does not clear, but inherit the current environment.
* I revised it to handle an escape character in character string set by
cmdline_options such as follows adequately.
--- for example: ---
primitive A ocf:heartbeat:anything \
params \
binfile=X \
cmdline_options=-V -c \openssl des-ede3 -d -base64 -k 'yy y'\
-i \
--- ---
Uh, this escaping gives me headache.
should this not be much easier by doing
- cmd=su -c \$variables\
+ cmd=su -c '$variables'
?
no escaping by sed necessary,
except maybe (if you are paranoid)
escaping of ' itself:
sed -e s/'/'''/
As long as we do cmd=su -c \$variable\, it is not sufficient to
escape \ (as the proposed patch by Yuusuke-san does), actually you'd
need to escape ` and $ and various other things as well.
Unless you consider it a feature that these would be
expanded already in the context of the eval running as root,
not in the context of the su $user nohup.
Which is (as it is now) a potential root exploit,
once you start taking cib admin != cluster root serious.
which is not really sensible to do IMO, anyways. But I digres.
Hm. Maybe we should move the eval into that context, anyways?
sort of
cmd=eval '${supposedly_properly_escaped_variable}'
su ... -c $cmd ?
But, for the record:
The line says:
+cmdline_options=`... | sed 's/\\\/\\\/g' | ...`
How does the left side expand? Shouldn't that be an even number
of backslashes? The right side also has 7 backslashes.
the first stripping of \ is done by the shell,
before feeding the whole thing to the `` subshell.
And the \ quoting within `` is subtle:
backslash retains its literal meaning except when followed by $, `, or \
so those \/ combinations could have been written as \\/ as well
(if only to reduce the headache of the reader, slightly)
but need not be. BTW, that is one of the differences between `` and $() ...
yep, its not pretty, but correct, though not necessarily consistent
between various shells and versions :(
god, I hate it when I know these useless facts from the top of my head,
I wish I had done less shell coding ;-)
* Strip off the trailing clone marker.
- quotations from the following.
http://hg.clusterlabs.org/pacemaker/stable-1.0/file/94515b3503b5/extra/resources/Dummy#l143
OK.
Can you please split the patch in three parts, so that we have
unrelated changes in signel patches.
Yes, please ;-)
--
: 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/
