Nice idea, but for now certainly an RFC On Mon, 3 Nov 2008, Alexander Gavrilov wrote: > Some environments may require selective limiting of read access to > repositories. While even dumb http transport supports it through .htaccess > files, gitweb currently does not implement discretionary access control. > > This patch adds a configuration-contolled check that matches simple > 'Reguire user'/'Reguire group' lines in the .htaccess files with the Typo: Reguire -> Require > authenticated user name. Using group authentication requires specifying > a path to the Apache group file in the configuration. > > Using .htaccess has an additional bonus that the same authentication > data can be used both for gitweb and the dumb http transport. I'm not sure if it wouldn't be a better solution to try to ask web server to do authentication, for example in MOD_PERL case via $r object (if I remember correctly)... > > Signed-off-by: Alexander Gavrilov <angavrilov@xxxxxxxxx> > --- > > I also created a gitosis fork that can generate the necessary files: > > http://repo.or.cz/w/gitosis/httpauth.git > > -- Alexander > > gitweb/INSTALL | 14 ++++++++++ > gitweb/gitweb.perl | 68 +++++++++++++++++++++++++++++++++++++++++++++++++-- > 2 files changed, 79 insertions(+), 3 deletions(-) > > diff --git a/gitweb/INSTALL b/gitweb/INSTALL > index 26967e2..0841db6 100644 > --- a/gitweb/INSTALL > +++ b/gitweb/INSTALL > @@ -166,6 +166,20 @@ Gitweb repositories > shows repositories only if this file exists in its object database > (if directory has the magic file named $export_ok). > > +- Finally, it is possible to use primitive .htaccess authentication by > + enabling the $check_htaccess variable in the config file. Gitweb > + recognizes the following htaccess commands: > + > + Require user name1 name2 ... # grant access to the listed users > + Require group group1 group2 ... # grant access to the listed groups > + Deny from all # deny unless overridden by a Require > + > + Access is granted if the currently authenticated user matches one > + of the Require lines, or if the file does not contain any of the listed > + commands, or if .htaccess does not exist. If the file exists but cannot > + be opened, access is denied. To use group authentication you have to > + point $auth_group_file to the group list in Apache format. > + > Generating projects list using gitweb > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index 63c793e..4b962c3 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -98,6 +98,12 @@ our $export_ok = "++GITWEB_EXPORT_OK++"; > # only allow viewing of repositories also shown on the overview page > our $strict_export = "++GITWEB_STRICT_EXPORT++"; > > +# check basic authentication rules in .htaccess > +our $check_htaccess = 0; > + > +# name of the file that lists groups for htaccess check > +our $auth_group_file = ""; > + > # list of git base URLs used for URL to where fetch project from, > # i.e. full URL is "$git_base_url/$project" > our @git_base_url_list = grep { $_ ne '' } ("++GITWEB_BASE_URL++"); > @@ -397,10 +403,64 @@ sub check_head_link { > (-l $headfile && readlink($headfile) =~ /^refs\/heads\//)); > } > > +# set of htaccess groups for the current user > +our %cur_auth_groups = (); Why it is hash, and not list? What are the keys, and what are values? > + > +sub find_current_groups($$) { Style: I think we prefer not using function prototypes. > + my ($gfile, $user) = @_; > + return () unless $gfile && $user; Style: you can use simple "return" which means '()' in list context, and 'undef' in scalar context. So it could have been written as: + ($gfile && $user) or return; But I'd rather someone better in Perl decided... > + > + my @groups; > + open my $gf, $gfile or return (); > + > + while(<$gf>) { > + next unless /^\s*(\S+)\s*:\s*(\S.*\S)\s*$/; > + my ($grp, $usrs) = ($1, $2); > + push @groups, $grp if grep { $_ eq $user } split (' ', $usrs); Wouldn't it be better to use regexp match instead of this split+grep pipeline? > + } > + > + close $gf; > + return @groups; > +} > + > +sub check_htaccess_files($) { Style: I think we prefer not using function prototypes. This function lack description: where it tries to find '.htaccess' files, what does it return, etc. > + my ($dir) = @_; > + my $user = $cgi->remote_user() || ' '; Why "|| ' '" here? > + > + while (length $dir >= length $projectroot) { > + my $file = "$dir/.htaccess"; > + next unless -e $file; > + open my $htf, $file or return 0; > + > + my $ok = 0; > + my $need_ok = 0; > + while (<$htf>) { > + if (/^\s*Require\s+user\s+(\S.*\S)\s*$/i) { > + $ok++ if grep { $_ eq $user; } split (' ', $1); > + $need_ok++; > + } elsif (/^\s*Require\s+group\s+(\S.*\S)\s*$/i) { > + $ok++ if grep { $cur_auth_groups{$_}; } split(' ', $1); > + $need_ok++; > + } elsif (/^\s*Deny\s+from\s+all\s*$/ix) { > + $need_ok++; > + } > + } > + close $htf; > + > + return $ok if $need_ok; > + last; > + } continue { > + $dir =~ s/\/[^\/]*$// or last; > + } First, this loop is IMHO very hacky and unclean, using 'continue' block (which is not very visible on first glance) to advance through loop. Second, while this loop might be good for _single_ repository, it is cleanly suboptimal in the case of 'projects_list' action... unless you want to list projects which are not accessible (I think it is the case for accessing static pages / static files). Third, again there is split+grep (well, this is at least consistent). > + > + return 1; > +} > + > sub check_export_ok { > my ($dir) = @_; > return (check_head_link($dir) && > - (!$export_ok || -e "$dir/$export_ok")); > + (!$export_ok || -e "$dir/$export_ok") && > + (!$check_htaccess || check_htaccess_files($dir))); > } O.K. Nice and clean. > > # process alternate names for backward compatibility > @@ -626,6 +686,9 @@ if (defined $action) { > } > } > > +# compute authenticated groups > +$cur_auth_groups{$_}++ for find_current_groups($auth_group_file, $cgi->remote_user()); > + This is a bit hacky. And I am not sure if its place should be here... > # parameters which are pathnames > our $project = $input_params{'project'}; > if (defined $project) { > @@ -853,8 +916,7 @@ sub validate_project { > my $input = shift || return undef; > if (!validate_pathname($input) || > !(-d "$projectroot/$input") || > - !check_head_link("$projectroot/$input") || > - ($export_ok && !(-e "$projectroot/$input/$export_ok")) || > + !check_export_ok("$projectroot/$input") || > ($strict_export && !project_in_list($input))) { > return undef; > } else { And this is independent change, and should be in separate patch... ...or should be dropped (I'd have to examine this code better). > -- > 1.6.0.3.15.gb8d36 > -- Jakub Narebski Poland -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
