You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@perl.apache.org by Fred Moyer <fr...@redhotpenguin.com> on 2010/04/20 23:51:40 UTC
Another A:T patch for review
This is a more extensive patch than the one I submitted a couple days
ago. This makes a move to keep any Apache::Test tests (or tests in
modules that use Apache::Test) from running as root. Most tests are
likely being run as non-privileged users currently; if you ever ran
them as root you likely had to change the permissions of the build
files, so that seems to be an exception.
There's one more addition here where the prompt to skip the test suite
is skipped if the tests are running under $ENV{AUTOMATED_TESTING}
Index: Makefile.PL
===================================================================
--- Makefile.PL (revision 935715)
+++ Makefile.PL (working copy)
@@ -223,3 +223,16 @@
$string;
};
}
+
+sub MY::test {
+ my $self = shift;
+
+ # root is not allowed to run tests
+ if (my $no_root_tests = Apache::TestMM::is_root()) {
+ return $no_root_tests;
+ }
+
+ # run tests normally for non root users
+ return $self->Apache::TestMM::test(@_);
+}
+
Index: lib/Apache/TestRun.pm
===================================================================
--- lib/Apache/TestRun.pm (revision 935715)
+++ lib/Apache/TestRun.pm (working copy)
@@ -46,7 +46,6 @@
my $orig_conf_opts;
my %core_files = ();
-my %original_t_perms = ();
my @std_run = qw(start-httpd run-tests stop-httpd);
my @others = qw(verbose configure clean help ssl http11 bugreport
@@ -563,8 +562,6 @@
}
}
- $self->adjust_t_perms();
-
if ($opts->{'start-httpd'}) {
exit_perl 0 unless $server->start;
}
@@ -603,8 +600,6 @@
sub stop {
my $self = shift;
- $self->restore_t_perms;
-
return $self->{server}->stop if $self->{opts}->{'stop-httpd'};
}
@@ -950,174 +945,6 @@
}, $vars->{top_dir});
}
-# this function handles the cases when the test suite is run under
-# 'root':
-#
-# 1. When user 'bar' is chosen to run Apache with, files and dirs
-# created by 'root' might be not writable/readable by 'bar'
-#
-# 2. when the source is extracted as user 'foo', and the chosen user
-# to run Apache under is 'bar', in which case normally 'bar' won't
-# have the right permissions to write into the fs created by 'foo'.
-#
-# We solve that by 'chown -R bar.bar t/' in a portable way.
-#
-# 3. If the parent directory is not rwx for the chosen user, that user
-# won't be able to read/write the DocumentRoot. In which case we
-# have nothing else to do, but to tell the user to fix the situation.
-#
-sub adjust_t_perms {
- my $self = shift;
-
- return if Apache::TestConfig::WINFU;
-
- %original_t_perms = (); # reset global
-
- my $user = getpwuid($>) || '';
- if ($user eq 'root') {
- my $vars = $self->{test_config}->{vars};
- my $user = $vars->{user};
- my($uid, $gid) = (getpwnam($user))[2..3]
- or die "Can't find out uid/gid of '$user'";
-
- warning "root mode: ".
- "changing the files ownership to '$user' ($uid:$gid)";
- finddepth(sub {
- $original_t_perms{$File::Find::name} = [(stat $_)[4..5]];
- chown $uid, $gid, $_;
- }, $vars->{t_dir});
-
- $self->check_perms($user, $uid, $gid);
-
- $self->become_nonroot($user, $uid, $gid);
- }
-}
-
-sub restore_t_perms {
- my $self = shift;
-
- return if Apache::TestConfig::WINFU;
-
- if (%original_t_perms) {
- warning "root mode: restoring the original files ownership";
- my $vars = $self->{test_config}->{vars};
- while (my($file, $ids) = each %original_t_perms) {
- next unless -e $file; # files could be deleted
- chown @$ids, $file;
- }
- }
-}
-
-# this sub is executed from an external process only, since it
-# "sudo"'s into a uid/gid of choice
-sub run_root_fs_test {
- my($uid, $gid, $dir) = @_;
-
- # first must change gid and egid ("$gid $gid" for an empty
- # setgroups() call as explained in perlvar.pod)
- my $groups = "$gid $gid";
- $( = $) = $groups;
- die "failed to change gid to $gid"
- unless $( eq $groups && $) eq $groups;
-
- # only now can change uid and euid
- $< = $> = $uid+0;
- die "failed to change uid to $uid" unless $< == $uid && $> == $uid;
-
- my $file = catfile $dir, ".apache-test-file-$$-".time.int(rand);
- eval "END { unlink q[$file] }";
-
- # unfortunately we can't run the what seems to be an obvious test:
- # -r $dir && -w _ && -x _
- # since not all perl implementations do it right (e.g. sometimes
- # acls are ignored, at other times setid/gid change is ignored)
- # therefore we test by trying to attempt to read/write/execute
-
- # -w
- open TEST, ">$file" or die "failed to open $file: $!";
-
- # -x
- -f $file or die "$file cannot be looked up";
- close TEST;
-
- # -r
- opendir DIR, $dir or die "failed to open dir $dir: $!";
- defined readdir DIR or die "failed to read dir $dir: $!";
- close DIR;
-
- # all tests passed
- print "OK";
-}
-
-sub check_perms {
- my ($self, $user, $uid, $gid) = @_;
-
- # test that the base dir is rwx by the selected non-root user
- my $vars = $self->{test_config}->{vars};
- my $dir = $vars->{t_dir};
- my $perl = Apache::TestConfig::shell_ready($vars->{perl});
-
- # find where Apache::TestRun was loaded from, so we load this
- # exact package from the external process
- my $inc = dirname dirname $INC{"Apache/TestRun.pm"};
- my $sub = "Apache::TestRun::run_root_fs_test";
- my $check = <<"EOI";
-$perl -Mlib=$inc -MApache::TestRun -e 'eval { $sub($uid, $gid, q[$dir]) }';
-EOI
- warning "testing whether '$user' is able to -rwx $dir\n$check\n";
-
- my $res = qx[$check] || '';
- warning "result: $res";
- unless ($res eq 'OK') {
- $self->user_error(1);
- #$self->restore_t_perms;
- error <<"EOI";
-You are running the test suite under user 'root'.
-Apache cannot spawn child processes as 'root', therefore
-we attempt to run the test suite with user '$user' ($uid:$gid).
-The problem is that the path (including all parent directories):
- $dir
-must be 'rwx' by user '$user', so Apache can read and write under that
-path.
-
-There are several ways to resolve this issue. One is to move and
-rebuild the distribution to '/tmp/' and repeat the 'make test'
-phase. The other is not to run 'make test' as root (i.e. building
-under your /home/user directory).
-
-You can test whether some directory is suitable for 'make test' under
-'root', by running a simple test. For example to test a directory
-'$dir', run:
-
- % $check
-Only if the test prints 'OK', the directory is suitable to be used for
-testing.
-EOI
- skip_test_suite();
- exit_perl 0;
- }
-}
-
-# in case the client side creates any files after the initial chown
-# adjustments we want the server side to be able to read/write them, so
-# they better be with the same permissions. dropping root permissions
-# and becoming the same user as the server side solves this problem.
-sub become_nonroot {
- my ($self, $user, $uid, $gid) = @_;
-
- warning "the client side drops 'root' permissions and becomes '$user'";
-
- # first must change gid and egid ("$gid $gid" for an empty
- # setgroups() call as explained in perlvar.pod)
- my $groups = "$gid $gid";
- $( = $) = $groups;
- die "failed to change gid to $gid" unless $( eq $groups && $) eq $groups;
-
- # only now can change uid and euid
- $< = $> = $uid+0;
- die "failed to change uid to $uid" unless $< == $uid && $> == $uid;
-}
-
sub run_request {
my($test_config, $opts) = @_;
@@ -1312,7 +1139,8 @@
# we can't prompt when STDIN is not attached to tty, unless we
# were told that's it OK via env var (in which case some program
# will feed the interactive prompts
- unless (-t STDIN || $ENV{APACHE_TEST_INTERACTIVE_PROMPT_OK}) {
+ unless (-t STDIN || $ENV{APACHE_TEST_INTERACTIVE_PROMPT_OK}
+ || !$ENV{AUTOMATED_TESTING} ) {
$no_doubt = 1;
}
Index: lib/Apache/TestMM.pm
===================================================================
--- lib/Apache/TestMM.pm (revision 935715)
+++ lib/Apache/TestMM.pm (working copy)
@@ -22,6 +22,29 @@
use Apache::TestConfig ();
use Apache::TestTrace;
+
+sub is_root {
+ my $user = getpwuid($>) || '';
+
+ return unless $user eq 'root';
+
+ return <<EOM;
+test::
+\t\@echo
+\t\@echo Apache::Test tests must be run as a non-privileged user on *nix
+\t\@echo based filesystems. There are several reasons, but stemming from
+\t\@echo the fact that the httpd process is owned by a non privileged user
+\t\@echo and thus cannot access the test files if they are owned by root.
+\t\@echo
+\t\@echo Please rebuild Apache::Test as a non-privileged user and run the
+\t\@echo tests as that user, then use sudo to install the module. Or build
+\t\@echo against your own locally installed version of Perl.
+\t\@echo
+EOM
+}
+
+
+
sub import {
my $class = shift;
@@ -52,6 +75,12 @@
sub test {
my $self = shift;
+
+ # root is not allowed to run tests
+ if (my $no_root_tests = Apache::TestMM::is_root()) {
+ return $no_root_tests;
+ }
+
my $env = Apache::TestConfig->passenv_makestr();
my $tests = "TEST_FILES =\n";
Index: Changes
===================================================================
--- Changes (revision 935715)
+++ Changes (working copy)
@@ -8,11 +8,18 @@
=item 1.33-dev
+Add check for automated testing environment variable before prompting
+with EU::MM to quit the test suite.
+[Adam Prime, Fred Moyer]
+
+Prevent tests from running as root user. Partly for security, partly
+to keep test failures down which require questionable workarounds to fix.
+[Fred Moyer]
+
https://rt.cpan.org/Public/Bug/Display.html?id=32993
use TAP::Harness for Apache::TestHarnessPHP
[Mark A. Hershberger]
-
https://rt.cpan.org/Public/Bug/Display.html?id=54476
Fix error where non root user gets test failure with httpd suexec and mod_fcgid
[Peter (Stig) Edwards]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: Another A:T patch for review
Posted by Fred Moyer <fr...@redhotpenguin.com>.
2010/4/21 Torsten Förtsch <to...@gmx.net>:
> But now, ServerRoot, DocumentRoot etc might not be read/writable by the user.
> Stas tried to solve that by chowning or chmoding our files. Still the tests
> can fail if the current directory is not accessible by the unprivileged user
> because some parent directory is forbidden.
That's correct, in most cases cpan unpacks the distribution in the
/root directory which is not readable by unprivileged users.
> Why not create a temp dir as the unprivileged user (using File::Temp, so the
> base path can be adjusted via $ENV{TMPDIR}) and copy all the stuff needed by
> Apache there.
>
> Assuming that the stuff that needs to be copied is all in t and perhaps blib
> these directories can even be symlinked (or "mount --bind"ed on linux) back to
> the current directory.
My concern here is that the code to help users run the tests as root
is already quite complicated. And it requires manual intervention by
the user in some cases, and if the process fails partway through, the
operations are not atomic. So if the user or some other process
cancels the test partway through, you have files accessible by any
user in root directory space.
I checked in a fix for the failing tests that spurred this effort (one
that is quite simple), so the original goal of this patch has been
achieved. But after spending time in the code dealing with the issues
that arise from running the tests as root, it seems that from a
security, simplicity, and maintainability angle that just not allowing
root to run the tests is the cleanest solution.
Bear in mind that when Stas wrote that code, he was funded by
Ticketmaster full time for two years. So he had the time and
resources to develop a solution for this edge case, but it still
requires more work to be done to get it working hands free. The edge
case of root running the tests is small enough that I think it just
needs to go away so we can focus on other issues which are more
critical and widely used.
>> +sub is_root {
>> + my $user = getpwuid($>) || '';
>> +
>> + return unless $user eq 'root';
>
> why not C<< $>==0 >>?
>
> This is what apache checks, not the name.
I took this style from the existing Apache::Test code. So there are
several ways to do it, but this way is already established in the
existing code.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: Another A:T patch for review
Posted by Torsten Förtsch <to...@gmx.net>.
On Tuesday 20 April 2010 23:51:40 Fred Moyer wrote:
> This is a more extensive patch than the one I submitted a couple days
> ago. This makes a move to keep any Apache::Test tests (or tests in
> modules that use Apache::Test) from running as root. Most tests are
> likely being run as non-privileged users currently; if you ever ran
> them as root you likely had to change the permissions of the build
> files, so that seems to be an exception.
>
> There's one more addition here where the prompt to skip the test suite
> is skipped if the tests are running under $ENV{AUTOMATED_TESTING}
The root of the problem if I understand it correctly is that Apache doesn't
run as
User root
unless compiled with a special BIG_SECURITY_HOLE flag or so.
So, we have to start it as an unprivileged user.
But now, ServerRoot, DocumentRoot etc might not be read/writable by the user.
Stas tried to solve that by chowning or chmoding our files. Still the tests
can fail if the current directory is not accessible by the unprivileged user
because some parent directory is forbidden.
Why not create a temp dir as the unprivileged user (using File::Temp, so the
base path can be adjusted via $ENV{TMPDIR}) and copy all the stuff needed by
Apache there.
Assuming that the stuff that needs to be copied is all in t and perhaps blib
these directories can even be symlinked (or "mount --bind"ed on linux) back to
the current directory.
Another way to deal with it may be:
PREREQ_PM => {
($<==0 ? ('Please retry as non-privilged user'=>0) : ()),
...
},
or see "How can I stop getting FAIL reports for missing libraries or other
non-Perl dependencies?" in http://wiki.cpantesters.org/wiki/CPANAuthorNotes
> Index: lib/Apache/TestMM.pm
> ===================================================================
> --- lib/Apache/TestMM.pm (revision 935715)
> +++ lib/Apache/TestMM.pm (working copy)
> @@ -22,6 +22,29 @@
> use Apache::TestConfig ();
> use Apache::TestTrace;
>
> +
> +sub is_root {
> + my $user = getpwuid($>) || '';
> +
> + return unless $user eq 'root';
why not C<< $>==0 >>?
This is what apache checks, not the name.
Torsten Förtsch
--
Need professional modperl support? Hire me! (http://foertsch.name)
Like fantasy? http://kabatinte.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org
Re: Another A:T patch for review
Posted by Adam Prime <ad...@utoronto.ca>.
I said this stuff in IRC, but I'll put it here too in case anyone else
wants to weigh in.
From what I understand, the primary goal of this patch is to fix a
bunch of false failures that are coming back for Apache::Test from cpan
testers for Apache::Test (see links below).
The bulk of this patch rips out a bunch of code that tried to help make
your tests pass, if for some reason you were running them as root.
I'm not really sure what the benefit of ripping all that code out is.
Sure, it's a bad idea to run your tests as root, but someone (stas by
the looks of things) thought handling that condition was worth writing a
few hundred lines of code back when it got written.
I think it would be less risky to make a release that only the
$ENV{AUTOMATED_TESTING} hunk, and see if that fixed the issue with CPAN
testers.
Thoughts?
Adam
cpan testers failure reports:
http://www.cpantesters.org/cpan/report/07134427-b19f-3f77-b713-d32bba55d77f
http://www.cpantesters.org/cpan/report/07129036-b19f-3f77-b713-d32bba55d77f
http://www.cpantesters.org/cpan/report/07122549-b19f-3f77-b713-d32bba55d77f
Fred Moyer wrote:
> This is a more extensive patch than the one I submitted a couple days
> ago. This makes a move to keep any Apache::Test tests (or tests in
> modules that use Apache::Test) from running as root. Most tests are
> likely being run as non-privileged users currently; if you ever ran
> them as root you likely had to change the permissions of the build
> files, so that seems to be an exception.
>
> There's one more addition here where the prompt to skip the test suite
> is skipped if the tests are running under $ENV{AUTOMATED_TESTING}
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@perl.apache.org
For additional commands, e-mail: dev-help@perl.apache.org