From: Ævar Arnfjörð Bjarmason Date: Sun, 04 Jul 2010 12:22:05 GMT Subject: Re: [PATCH 1/4] git-cvsserver: implement script based pserver authentication Message-ID: In-Reply-To: On Sun, Jul 4, 2010 at 08:51, Áshin László wrote: > Hi, > > On Sun, Jul 4, 2010 at 00:14, Ævar Arnfjörð Bjarmason wrote: >> On Sat, Jul 3, 2010 at 21:21, Áshin László wrote: >>>  Documentation/git-cvsserver.txt |   46 +++++++++++++++++++++++++++++++++++--- >>>  git-cvsserver.perl              |   28 +++++++++++++++++++++++ >>>  t/t9400-git-cvsserver-server.sh |   45 ++++++++++++++++++++++++++++++++++++++ >>>  3 files changed, 115 insertions(+), 4 deletions(-) >>> >>> diff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt >>> index 7004dd2..b3c3122 100644 >>> --- a/Documentation/git-cvsserver.txt >>> +++ b/Documentation/git-cvsserver.txt >>> @@ -100,10 +100,48 @@ looks like >>>  ------ >>> >>>  Only anonymous access is provided by pserve by default. To commit you >> >> I think it's always called "pserver" with an -r. > > Yes but it is not part of my changeset. Sorry, misread the diff. >>> -will have to create pserver accounts, simply add a gitcvs.authdb >>> -setting in the config file of the repositories you want the cvsserver >>> -to allow writes to, for example: >>> +will have to specify an authentication option in the config file. >>> +Currently there are two options are available for authentication through >>> +pserver in git-cvsserver: one through an authenticator script and an other >>> +through a textual authentication database. If both are specified in the config >>> +file then the script based solution will be used. >> >> If both are specified shouldn't we throw an error? > > Actually a check can be added to the code. OK, I will do that. Nice. >>> +  a. To use the authentication script based method, simply add a >>> +     gitcvs.authscript setting in the config file of the repositories you want >>> +     the cvsserver to allow writes to, for example: >>> ++ >>> +-- >>> +------ >>> + >>> +   [gitcvs] >>> +    authscript = /usr/local/bin/cvsserver-auth.sh >>> + >>> +------ >>> +The file specified here must be executable by the user the git-cvsserver runs >>> +under the name of. This script has to read exactly two lines from its standard >> >> "the name of" is redundant here. >> >>> +input as long as git-cvsserver passes the username and the password on separate >>> +lines. After the script did its task by checking the username and password >>> +given it has to return zero if the authentication was successful and return >>> +other value if it was not. >> >> Better as: "The script will receive two lines on standard input, the >> first is the username and the second is the password. It should return >> 0 if the user was successfully authenticated, and a non-zero value if >> not". > > OK, a little bit I overcomplicated it :) > > >>> + >>> +Here is an example for an authentication script which checks the users against >>> +active directory: >>> +------ >>> +#!/bin/sh >>> +# /usr/local/bin/cvsserver-auth.sh >>> >>> +read username >>> +read password >>> + >>> +wbinfo -a "${username}%${password}" >> >> Do all POSIX shells implicitly exit with the return value of the last >> statement they evaluate. I don't know. > > "The exit status of a sequential list *shall* be the exit status of > the last command in the list." > ( After registration, > http://www.opengroup.org/onlinepubs/000095399/utilities/xcu_chap02.html#tag_02_08 > ) > Generally, all kind of list constructions use this principle according > to this page. > > Terminology of "shall": > "For an implementation that conforms to IEEE Std 1003.1-2001, > describes a feature or behavior that is mandatory. *An application can > rely on the existence of the feature or behavior*. > > For an application or user, describes a behavior that is mandatory." Nice, good to have that cleared up in general :) >>> +------ >>> +-- >>> + >>> +  b. To use the authentication database based method, simply add a >>> +     gitcvs.authdb setting in the config file of the repositories you want the >>> +     cvsserver to allow writes to, for example: >>> ++ >>> +-- >>>  ------ >>> >>>    [gitcvs] >>> @@ -125,7 +163,7 @@ Alternatively you can produce the password with >>> perl's crypt() operator: >>>  ----- >>>    perl -e 'my ($user, $pass) = @ARGV; printf "%s:%s\n", $user, >>> crypt($user, $pass)' $USER password >>>  ----- >>> - >>> +-- >>>  Then provide your password via the pserver method, for example: >>>  ------ >>>    cvs -d:pserver:someuser:somepassword server/path/repo.git co >>> diff --git a/git-cvsserver.perl b/git-cvsserver.perl >>> index e9f3037..9664e86 100755 >>> --- a/git-cvsserver.perl >>> +++ b/git-cvsserver.perl >>> @@ -197,6 +197,34 @@ if ($state->{method} eq 'pserver') { >>>         } >>> >>>         # Fall through to LOVE >>> +    } elsif (exists $cfg->{gitcvs}->{authscript}) { >>> +        my $authscript = $cfg->{gitcvs}->{authscript}; >>> + >>> +        unless (-x $authscript) { >>> +            print "E The authentication script specified in "; >>> +            print "[gitcvs.authscript] cannot be executed\n"; >> >> I *think* you have to prefix anything that's not "I (HATE|LOVE) YOU" >> with "E " if it's an error. I.e. this should probably be: >> >>    print "E The authentication script specified in"; >>    print "E [gitcvs.authscript] cannot be executed\n"; >> >> But that's just my hazy memory. Maybe CVS clients will report the >> error anyway. > > Please note that there is no "\n" after the first line. This is only a > line break in the source code. > Should I have used the following? Ah yes, I missed that there wasn't a \n there. >     print "E The authentication script specified in" . >         "E [gitcvs.authscript] cannot be executed\n"; Probably yeah, at least I managed to misread it :) >>> +            print "I HATE YOU\n"; >>> +            exit 1; >>> +        } >>> + >>> +        open my $script_fd, '|-', "'$authscript'" >>> +            or die "Couldn't open authentication script '$authscript': $!"; >>> + >>> +        if (length($password) > 0) { >>> +            $password = descramble($password); >>> +        } >>> + >>> +        print $script_fd "$user\n"; >>> +        print $script_fd "$password\n"; >>> +        close $script_fd; >>> + >>> +        unless ($? == 0) { >>> +            print "E External script authentication failed.\n"; >>> +            print "I HATE YOU\n"; >>> +            exit 1; >>> +        } >>> + >>> +        # Fall through to LOVE >>>     } else { >>>         # Trying to authenticate a user >>>         if (not exists $cfg->{gitcvs}->{authdb}) { >>> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh >>> index 8639506..0743e9a 100755 >>> --- a/t/t9400-git-cvsserver-server.sh >>> +++ b/t/t9400-git-cvsserver-server.sh >>> @@ -64,6 +64,16 @@ test_expect_success 'basic checkout' \ >>>  # PSERVER AUTHENTICATION >>>  #------------------------ >>> >>> +cat >"$SERVERDIR/authscript.sh" <>> +#!/bin/sh >>> +read username >>> +read password >>> + >>> +test "x\$username" = xcvsuser -a "x\$password" = xcvspassword >> >> This is offtopic. But I've never figured out why you need to do >> >>    test "x$foo" = "xbar" >> >> As opposed to: >> >>    test "$foo" = "bar" >> >> In shellscript. Are there really some shells that get equality tests >> where one term is "" wrong? > > Maybe this is too much. Maybe x was used only where " couldn't be: > >    test x$foo = xbar > > So, if $foo is empty we have to make sure that test gets something to > both side of the equation mark. But if quotation marks can be used it > is possibly unneeded: > >    test "$foo" = "bar" > > My solution is an overkill in this case. But which of the above ones > is preferred then? > I think I can answer my own question. If $foo contains spaces only the > quotation mark based version will do what we want. I will remove the x > characters. Ok. >>> +EOF >>> + >>> +chmod a+x "$SERVERDIR/authscript.sh" >>> + >>>  cat >request-anonymous  <>>  BEGIN AUTH REQUEST >>>  $SERVERDIR >>> @@ -134,6 +144,41 @@ test_expect_success 'pserver authentication >>> failure (login/non-anonymous user)' >>>    fi && >>>    sed -ne \$p log | grep "^I HATE YOU\$"' >>> >>> +GIT_DIR="$SERVERDIR" git config --unset gitcvs.authdb || exit 1 >>> +GIT_DIR="$SERVERDIR" git config gitcvs.authscript >>> "$SERVERDIR/authscript.sh" || exit 1 >>> + >>> +test_expect_success 'pserver authentication (authscript)' \ >>> +  'cat request-anonymous | git-cvsserver pserver >log 2>&1 && >>> +   sed -ne \$p log | grep "^I LOVE YOU\$"' >>> + >>> +test_expect_success 'pserver authentication failure (authscript, >>> non-anonymous user)' \ >>> +  'if cat request-git | git-cvsserver pserver >log 2>&1 >>> +   then >>> +       false >>> +   else >>> +       true >>> +   fi && >> >> This should probably be (untested): >> >>    test_must_fail cat request-git git-cvsserver pserver >log 2>&1 && > > I copied this code from above so this was not my decision. These test > cases are all can be find elsewhere in the code, the only difference > is in their names and the fact that the script based authentication > method is active while they are running. Yeah, a lot of the test code is ancient and unoptimal. Just leave it like that I guess. >>> +   sed -ne \$p log | grep "^I HATE YOU\$"' >>> + >>> +test_expect_success 'pserver authentication success (authscript, >>> non-anonymous user with password)' \ >>> +  'cat login-git-ok | git-cvsserver pserver >log 2>&1 && >>> +   sed -ne \$p log | grep "^I LOVE YOU\$"' >>> + >>> +test_expect_success 'pserver authentication (authscript, login)' \ >>> +  'cat login-anonymous | git-cvsserver pserver >log 2>&1 && >>> +   sed -ne \$p log | grep "^I LOVE YOU\$"' >>> + >>> +test_expect_success 'pserver authentication failure (authscript, >>> login/non-anonymous user)' \ >>> +  'if cat login-git | git-cvsserver pserver >log 2>&1 >>> +   then >>> +       false >>> +   else >>> +       true >>> +   fi && >>> +   sed -ne \$p log | grep "^I HATE YOU\$"' >>> + >>> +GIT_DIR="$SERVERDIR" git config --unset gitcvs.authscript || exit 1 >>> +GIT_DIR="$SERVERDIR" git config gitcvs.authdb "$SERVERDIR/auth.db" || exit 1 >>> >>>  # misuse pserver authentication for testing of req_Root >> >> Otherwise looking good. >> > > I am not familiar the way that a patch goes in this project. Where can > I find the repository my patches will be commited? When will this kind > of changes go to mainline? Patches are: * Discussed on the mailing list (you are here) * Submitted for inclusion (see Documentation/SubmittingPatches). You didn't CC Junio so it isn't there yet. * Maybe show up in a "What's cooking" post if they're more complex and warrant further discussion. * Get merged into git://git.kernel.org/pub/scm/git/git.git, either pu, next, master, maint or some combo of that.