| Message ID | 20180321023515.11356-1-dan.rue@linaro.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Petr Vorel |
| Headers | show |
| Series | testcases: fix file path to control access to cron | expand |
Hi Dan, thanks for your patch. Cron scripts would deserver rewriting to use new shell API (+ cleanup). > From: Lucas Magasweran <lucas.magasweran@ieee.org> > crontab uses /etc/cron.{allow,deny} to control access on most distributions, as > well as RHEL. To maintain backwards compatibility with older still supported > distributions, the test setup will search the man page for the correct paths > (e.g. /var/spool/cron/allow). If the man page is not available (e.g. on an Do you know which distros and cron versions are really affected? Is it RHEL 4? SLES 10 SP4 uses vixie cron 4.1 with files in /etc. I wonder if we need still to support old path. > embedded device), it will default to the /etc paths. This method supports > vixie-cron, cronie, etc... BTW (looking at busybox source) busybox crond does not support allow/deny, but that's another issue. > The repeating code has been moved into a common shell script library to > simplify maintenance. > Reported-by: Myungho Jung <mhjungk@gmail.com> > Signed-off-by: Lucas Magasweran <lucas.magasweran@ieee.org> > Tested-by: Dan Rue <drue@therub.org> > --- > testcases/commands/cron/cron_allow01 | 12 +----------- > testcases/commands/cron/cron_common.sh | 15 +++++++++++++++ > testcases/commands/cron/cron_deny01 | 14 +------------- > testcases/commands/cron/cron_neg_tests.sh | 2 +- > testcases/commands/cron/cron_pos_tests.sh | 13 +------------ > 5 files changed, 19 insertions(+), 37 deletions(-) > create mode 100755 testcases/commands/cron/cron_common.sh > diff --git a/testcases/commands/cron/cron_allow01 b/testcases/commands/cron/cron_allow01 > index 9a5e4d2406..7e05f7b7dd 100755 > --- a/testcases/commands/cron/cron_allow01 > +++ b/testcases/commands/cron/cron_allow01 > @@ -26,17 +26,7 @@ > echo "This script contains bashism that needs to be fixed!" > -iam=`whoami` > - > -tvar=${MACHTYPE%-*} > -tvar=${tvar#*-} > - > -if [ "$tvar" = "redhat" -o "$tvar" = "redhat-linux" ] > -then > -CRON_ALLOW="/etc/cron.allow" > -else > -CRON_ALLOW="/var/spool/cron/allow" > -fi > +. cron_common.sh > TEST_USER1="ca_user1" > TEST_USER1_HOME="/home/$TEST_USER1" > diff --git a/testcases/commands/cron/cron_common.sh b/testcases/commands/cron/cron_common.sh > new file mode 100755 > index 0000000000..5ed90c4039 > --- /dev/null > +++ b/testcases/commands/cron/cron_common.sh > @@ -0,0 +1,15 @@ > +#!/bin/sh > + > +iam=`whoami` > + > +tvar=${MACHTYPE%-*} > +tvar=${tvar#*-} Can you please removed tvar? It's not needed any more and MACHTYPE is a bashism. This will remove most of bashisms (the only left will be function keyword in cron_pos_tests.sh). > + > +setup_path() { > + # Support paths used by older distributions of RHEL or SLES. > + export CRON_DENY="$(man crontab 2>/dev/null | grep -m 1 -o '/[\/a-z.]*deny$' || echo "/etc/cron.deny")" > + export CRON_ALLOW="$(man crontab 2>/dev/null | grep -m 1 -o '/[\/a-z.]*allow$' || echo "/etc/cron.allow")" This has wrong quotation marks (although it's working. Better would be: export CRON_DENY="$(man crontab 2>/dev/null | grep -m 1 -o '/[\/a-z.]*deny$' || echo '/etc/cron.deny')" export CRON_ALLOW="$(man crontab 2>/dev/null | grep -m 1 -o '/[\/a-z.]*allow$' || echo '/etc/cron.allow')" > +} > + > +setup_path > + > diff --git a/testcases/commands/cron/cron_deny01 b/testcases/commands/cron/cron_deny01 > index 9d32039251..263a7829b5 100755 > --- a/testcases/commands/cron/cron_deny01 > +++ b/testcases/commands/cron/cron_deny01 > @@ -26,19 +26,7 @@ > echo "This script contains bashism that needs to be fixed!" > -iam=`whoami` > - > -tvar=${MACHTYPE%-*} > -tvar=${tvar#*-} > - > -if [ "$tvar" = "redhat" -o "$tvar" = "redhat-linux" ] > -then > -CRON_DENY="/etc/cron.deny" > -CRON_ALLOW="/etc/cron.allow" > -else > -CRON_DENY="/var/spool/cron/deny" > -CRON_ALLOW="/var/spool/cron/allow" > -fi > +. cron_common.sh > TEST_USER1="cd_user1" > TEST_USER1_HOME="/home/$TEST_USER1" > diff --git a/testcases/commands/cron/cron_neg_tests.sh b/testcases/commands/cron/cron_neg_tests.sh > index 9c3d6f6c7c..49c5c4e5d0 100755 > --- a/testcases/commands/cron/cron_neg_tests.sh > +++ b/testcases/commands/cron/cron_neg_tests.sh > @@ -9,7 +9,7 @@ > # 12/03/04 Marty Ridgeway Pull RHEl4 tests out from script > -iam=`whoami` > +. cron_common.sh > if [ $iam = "root" ]; then > if [ $# -lt 1 ] ; then > diff --git a/testcases/commands/cron/cron_pos_tests.sh b/testcases/commands/cron/cron_pos_tests.sh > index ece114c84c..a0ddaea57b 100755 > --- a/testcases/commands/cron/cron_pos_tests.sh > +++ b/testcases/commands/cron/cron_pos_tests.sh > @@ -2,18 +2,7 @@ > # Positive tests for cron, that means these tests have to pass > -iam=`whoami` > - > -tvar=${MACHTYPE%-*} > -tvar=${tvar#*-} > - > -if [ "$tvar" = "redhat" -o "$tvar" = "redhat-linux" ] > -then > - CRON_ALLOW="/etc/cron.allow" > -else > - CRON_ALLOW="/var/spool/cron/allow" > -fi > - > +. cron_common.sh > if [ $iam = "root" ]; then > if [ $# -lt 1 ] ; then Kind regards, Petr
Hi! > TEST_USER1="ca_user1" > TEST_USER1_HOME="/home/$TEST_USER1" > diff --git a/testcases/commands/cron/cron_common.sh b/testcases/commands/cron/cron_common.sh > new file mode 100755 > index 0000000000..5ed90c4039 > --- /dev/null > +++ b/testcases/commands/cron/cron_common.sh > @@ -0,0 +1,15 @@ > +#!/bin/sh > + > +iam=`whoami` > + > +tvar=${MACHTYPE%-*} > +tvar=${tvar#*-} > + > +setup_path() { > + # Support paths used by older distributions of RHEL or SLES. > + export CRON_DENY="$(man crontab 2>/dev/null | grep -m 1 -o '/[\/a-z.]*deny$' || echo "/etc/cron.deny")" > + export CRON_ALLOW="$(man crontab 2>/dev/null | grep -m 1 -o '/[\/a-z.]*allow$' || echo "/etc/cron.allow")" > +} FYI I do have three crontab manual pages installed on my system, two by the cron package and one by man-pages-posix package. The 'man 1 crontab' seems to be the one we want to parse though, it will be picked by default since the predefined order starts with the section 1, but we may as well specify it explicitly. Also I will check my collection of virtual machines, with a bit of luck all of the SLES versions that used /var/spool/cron/ are out of support now.
> From: Lucas Magasweran <lucas.magasweran@ieee.org> > crontab uses /etc/cron.{allow,deny} to control access on most distributions, as > well as RHEL. To maintain backwards compatibility with older still supported > distributions, the test setup will search the man page for the correct paths > (e.g. /var/spool/cron/allow). If the man page is not available (e.g. on an > embedded device), it will default to the /etc paths. This method supports > vixie-cron, cronie, etc... > The repeating code has been moved into a common shell script library to > simplify maintenance. > Reported-by: Myungho Jung <mhjungk@gmail.com> > Signed-off-by: Lucas Magasweran <lucas.magasweran@ieee.org> > Tested-by: Dan Rue <drue@therub.org> > --- <snip> > -if [ "$tvar" = "redhat" -o "$tvar" = "redhat-linux" ] > -then > -CRON_ALLOW="/etc/cron.allow" > -else > -CRON_ALLOW="/var/spool/cron/allow" > -fi > +. cron_common.sh We've decided to drop support for /var/spool/cron/{allow,deny} entirely. Please change your patch. If you wonder why we drop the support: This old path used original Vixie-cron / ISC, cronie which is a fork of Vixie-cron version 4.1 used changed the path in 2007 [1], [2]. Original patch used for RHEL /etc/cron.{allow,deny} anyway, even SLE 9.3 used vixie-cron with patched to the new path. Debian changed the path in it's vixie-cron in 1999 [3] Kind regards, Petr [1] https://github.com/cronie-crond/cronie/commit/7f6c24bc5aaddcc95556764fa3f801f2166e86b9 [2] https://github.com/cronie-crond/cronie/commit/9ff26cfe84d14af46dcf8af39fba9c25534df6f7 [3] https://anonscm.debian.org/cgit/pkg-cron/pkg-cron.git/commit/?id=546d93985f662362005109e2f069ebe15c0112aa
diff --git a/testcases/commands/cron/cron_allow01 b/testcases/commands/cron/cron_allow01 index 9a5e4d2406..7e05f7b7dd 100755 --- a/testcases/commands/cron/cron_allow01 +++ b/testcases/commands/cron/cron_allow01 @@ -26,17 +26,7 @@ echo "This script contains bashism that needs to be fixed!" -iam=`whoami` - -tvar=${MACHTYPE%-*} -tvar=${tvar#*-} - -if [ "$tvar" = "redhat" -o "$tvar" = "redhat-linux" ] -then -CRON_ALLOW="/etc/cron.allow" -else -CRON_ALLOW="/var/spool/cron/allow" -fi +. cron_common.sh TEST_USER1="ca_user1" TEST_USER1_HOME="/home/$TEST_USER1" diff --git a/testcases/commands/cron/cron_common.sh b/testcases/commands/cron/cron_common.sh new file mode 100755 index 0000000000..5ed90c4039 --- /dev/null +++ b/testcases/commands/cron/cron_common.sh @@ -0,0 +1,15 @@ +#!/bin/sh + +iam=`whoami` + +tvar=${MACHTYPE%-*} +tvar=${tvar#*-} + +setup_path() { + # Support paths used by older distributions of RHEL or SLES. + export CRON_DENY="$(man crontab 2>/dev/null | grep -m 1 -o '/[\/a-z.]*deny$' || echo "/etc/cron.deny")" + export CRON_ALLOW="$(man crontab 2>/dev/null | grep -m 1 -o '/[\/a-z.]*allow$' || echo "/etc/cron.allow")" +} + +setup_path + diff --git a/testcases/commands/cron/cron_deny01 b/testcases/commands/cron/cron_deny01 index 9d32039251..263a7829b5 100755 --- a/testcases/commands/cron/cron_deny01 +++ b/testcases/commands/cron/cron_deny01 @@ -26,19 +26,7 @@ echo "This script contains bashism that needs to be fixed!" -iam=`whoami` - -tvar=${MACHTYPE%-*} -tvar=${tvar#*-} - -if [ "$tvar" = "redhat" -o "$tvar" = "redhat-linux" ] -then -CRON_DENY="/etc/cron.deny" -CRON_ALLOW="/etc/cron.allow" -else -CRON_DENY="/var/spool/cron/deny" -CRON_ALLOW="/var/spool/cron/allow" -fi +. cron_common.sh TEST_USER1="cd_user1" TEST_USER1_HOME="/home/$TEST_USER1" diff --git a/testcases/commands/cron/cron_neg_tests.sh b/testcases/commands/cron/cron_neg_tests.sh index 9c3d6f6c7c..49c5c4e5d0 100755 --- a/testcases/commands/cron/cron_neg_tests.sh +++ b/testcases/commands/cron/cron_neg_tests.sh @@ -9,7 +9,7 @@ # 12/03/04 Marty Ridgeway Pull RHEl4 tests out from script ######################################################## -iam=`whoami` +. cron_common.sh if [ $iam = "root" ]; then if [ $# -lt 1 ] ; then diff --git a/testcases/commands/cron/cron_pos_tests.sh b/testcases/commands/cron/cron_pos_tests.sh index ece114c84c..a0ddaea57b 100755 --- a/testcases/commands/cron/cron_pos_tests.sh +++ b/testcases/commands/cron/cron_pos_tests.sh @@ -2,18 +2,7 @@ # Positive tests for cron, that means these tests have to pass -iam=`whoami` - -tvar=${MACHTYPE%-*} -tvar=${tvar#*-} - -if [ "$tvar" = "redhat" -o "$tvar" = "redhat-linux" ] -then - CRON_ALLOW="/etc/cron.allow" -else - CRON_ALLOW="/var/spool/cron/allow" -fi - +. cron_common.sh if [ $iam = "root" ]; then if [ $# -lt 1 ] ; then