Re: [PATCH 1/4] git-cvsserver: implement script based pserver authentication

> Hi,
>
> On Sun, Jul 4, 2010 at 00:14, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> On Sat, Jul 3, 2010 at 21:21, Áshin László <ashinlaszlo@gmail.com> 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.

>>> -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.

>>> +  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."

>>> +------
>>> +--
>>> +
>>> +  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 <at> server/path/repo.git co <HEAD_name>
>>> 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?

>>> +            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" <<EOF
>>> +#!/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.

>>> +EOF
>>> +
>>> +chmod a+x "$SERVERDIR/authscript.sh"
>>> +
>>>  cat >request-anonymous  <<EOF
>>>  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.

>>> +   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?