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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 3, 2010, 22:14 UTC
Message-ID: <AANLkTil4C_oF972J5iRApUGXpwoFNqcYTgkzvorCseZZ@mail.gmail.com>
In-Reply-To: <AANLkTikbPyZ-q8XgTBX1P9HQlFwUhiJSpiOgYG75mtiq@mail.gmail.com>

On Sat, Jul 3, 2010 at 21:21, Áshin László <ashinlaszlo@gmail.com> wrote:

Show 13 quoted lines

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

Show 8 quoted lines

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

Show 13 quoted lines

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

Show 11 quoted lines

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

Show 36 quoted lines

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

Show 39 quoted lines

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

Show 28 quoted lines

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

Show 25 quoted lines

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