Re: [PATCH] Authentication support for pserver

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 14, 2007, 05:32 UTC
Message-ID: <7vd4t9x2lw.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <87wsrhex4c.fsf@cpan.org>

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

Show 18 quoted lines

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

Show 10 quoted lines

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

Show 6 quoted lines

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