Re: [PATCH 1/5] git-cvsserver: implement script based pserver auth

[Junio C Hamano <gitster@pobox.com>]

Áshin László <ashinlaszlo@gmail.com> writes:

> ---

Sign-off?

>  Documentation/git-cvsserver.txt |   42 +++++++++++++++++++++++++++---
>  git-cvsserver.perl              |   34 ++++++++++++++++++++++++
>  t/t9400-git-cvsserver-server.sh |   55 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 127 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt
> index 7004dd2..59c8e5d 100644
> --- a/Documentation/git-cvsserver.txt
> +++ b/Documentation/git-cvsserver.txt
> @@ -100,10 +100,44 @@ looks like
>  ------
>
>  Only anonymous access is provided by pserve by default. To commit you

It is ok to squash 5/5 here, as that is so trivial.

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

s/options are/options/;

> +pserver in git-cvsserver: one through an authenticator script and an other
> +through a textual authentication database.
> +
> +  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:

Why "simply"?  Adding that word does not make the procedure nor the
explanation any simpler than it is.  Just drop it.

More importantly, you should explicitly say what _value_ the variable
should be set to, and what it means.  E.g.

	...method, set the gitcvs.authscript variable to the path of your
        custom authentication script in the repository.

You already said that the user needs to specify an auth option to allow
write access, so there is no need to repeat "you want ... to allow writes
to" here.

> +The file specified here must be executable by the user the git-cvsserver runs
> +under. The script will receive two lines on standard input, the first is the

s/under/as/; s/standard input/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.
> +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}"

Exposing username/password pair on a command line looks like a not-so-good
example.  Also it is a bit disturbing to see that wbinfo(1) says:

       -a username%password
              Attempt to authenticate a user via winbindd.  This  checks
              both
              authenticaion methods and reports its results.

              Note

              Do  not  be tempted to use this functionality for
              authentication
              in third-party applications. Instead use ntlm_auth(1).

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

Likewise.

> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> index e9f3037..c89d999 100755
> --- a/git-cvsserver.perl
> +++ b/git-cvsserver.perl
> @@ -197,6 +197,40 @@ if ($state->{method} eq 'pserver') {
>         }
>
>         # Fall through to LOVE
> +    } elsif (exists $cfg->{gitcvs}->{authscript} and
> +             exists $cfg->{gitcvs}->{authdb}) {
> +        print "E Ambiguous configuration of authentication methods. " .
> +            "Only one authentication method can be enabled at once\n";
> +        print "I HATE YOU\n";
> +        exit 1;

Hmph.  It is not unconceivable that you would want to use an authscript
that gives company or group wide user database, augmented with an authdb
that is for the particular repository...  Of course if we do that we would
need a way to specify which one wins when both knows about the user.

> +    } elsif (exists $cfg->{gitcvs}->{authscript}) {
> +        my $authscript = $cfg->{gitcvs}->{authscript};
> +
> +        unless (-x $authscript) {
> +            print "E The authentication script specified in " .
> +                "[gitcvs.authscript] cannot be executed\n";
> +            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;

Don't you need to check the return value of "close" here, even if you will
also check the value of $? next?

> +        unless ($? == 0) {
> +            print "E External script authentication failed.\n";
> +            print "I HATE YOU\n";

I don't think this "E" is necessary nor desirable.  Does the existing
password database authenticator codepath say "authentication failed" like
this, when it gets a wrong password?

> @@ -134,6 +144,51 @@ test_expect_success 'pserver authentication
> failure (login/non-anonymous user)'

Corrupt patch.