Re: [PATCH] Authentication support for pserver

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

avar@cpan.org (Ævar Arnfjörð Bjarmason) writes:

> +    unless ($user eq 'anonymous') {
> +        # Trying to authenticate a user
> +        if (not exists $cfg->{gitcvs}->{users}) {
> +            print "E the repo config file needs a [gitcvs.users] section with user/password key-value pairs\n";
> +            print "I HATE YOU\n";
> +            exit 1;
> +        } elsif (exists $cfg->{gitcvs}->{users} and not exists $cfg->{gitcvs}->{users}->{$user}) {
> +            print "E the repo config file has a [gitcvs.users] section but the user $user is not defined in it\n";
> +            print "I HATE YOU\n";
> +            exit 1;
> +        } else {
> +            my $descrambled_password = descramble($password);
> +            my $cleartext_password = $cfg->{gitcvs}->{users}->{$user};
> +            if ($descrambled_password ne $cleartext_password) {
> +                print "E The password supplied for user $user was incorrect\n";
> +                print "I HATE YOU\n";
> +                exit 1;
> +            }

I do not know what the real pserver does but by sending these E lines in
the latter two different forms back to the client you are leaking
sensitive information, which is probably not what you want (the first
one is Ok, though.  It would help the server administrator to notice
misconfiguration, and until it is corrected nobody would be able to log
in anyway).

Admittedly, the pserver password scrambling is not a real security, but
if we were paranoid, we would probably be even adding random delay in
"no user found" case and "password does not match" case, so that the
client cannot even tell from the response latency if a username exists
at the server.

> @@ -1176,12 +1196,6 @@ sub req_ci
>  
>      $log->info("req_ci : " . ( defined($data) ? $data : "[NULL]" ));
>  
> -    if ( $state->{method} eq 'pserver')
> -    {
> -        print "error 1 pserver access cannot commit\n";
> -        exit;
> -    }
> -

Is this correct?  You are still allowing anonymous pserver access, so
shouldn't you check if this was an anonymous access or authenticated one
and refuse access like before for anonymous people?

> +    my ($str) = @_;
> +
> +    # This should never happen, the same format has been used since
> +    # CVS was spawned
> +    $str =~ s/^(.)//;
> +    die "invalid password format $1" unless $1 eq 'A';

I do not quite understand what "spawn" means in this sentence.