threads / patch / 24262

patchgit-cvsserver: pserver-auth-script

Subject: [PATCH] git-cvsserver: pserver-auth-script

## tl;dr

8 messages between Jul 2, 2010 and Jul 3, 2010. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

László ÁSHIN· Jul 2, 2010, 07:54 UTC · lore
Hi,
The following patch makes git-cvsserver capable of authenticating users through an external executable script using pserver method.
The script can be specified in the gitcvs section of the config file:
[gitcvs]
	enabled = 1
	authscript = /some/where/script.sh
The script, itself will get username and password on its standard input, so it can look like something like this:

#!/bin/sh read username read password

wbinfo -a "$username%$password"

-- Only a return value of zero means a successful authentication.

Please comment and keep me on cc.
-- 
Regards,
László Áshin

diff -ruN a/git-cvsserver b/git-cvsserver
--- a/git-cvsserver	2010-07-01 15:31:18.000000000 +0200
+++ b/git-cvsserver	2010-07-01 15:35:41.000000000 +0200
@@ -200,35 +200,54 @@
         # Fall through to LOVE
     } else {
         # Trying to authenticate a user
-        if (not exists $cfg->{gitcvs}->{authdb}) {
-            print "E the repo config file needs a [gitcvs] section with an 'authdb' parameter set to the filename of the authentication database\n";
-            print "I HATE YOU\n";
-            exit 1;
-        }
-
-        my $authdb = $cfg->{gitcvs}->{authdb};
-
-        unless (-e $authdb) {
-            print "E The authentication database specified in [gitcvs.authdb] does not exist\n";
-            print "I HATE YOU\n";
-            exit 1;
-        }
-
-        my $auth_ok;
-        open my $passwd, "<", $authdb or die $!;
-        while (<$passwd>) {
-            if (m{^\Q$user\E:(.*)}) {
-                if (crypt($user, descramble($password)) eq $1) {
-                    $auth_ok = 1;
-                }
-            };
-        }
-        close $passwd;
+        if (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 SCRIPTIN, '|' . $authscript or die $!;
+            print SCRIPTIN $user . "\n";
+            print SCRIPTIN descramble($password) . "\n";
+            close SCRIPTIN;
+            if ($? != 0) {
+                print "E External script authentication failed.\n";
+                print "I HATE YOU\n";
+                exit 1;
+            }
+        } else {
+            if (not exists $cfg->{gitcvs}->{authdb}) {
+                print "E the repo config file needs a [gitcvs] section with an 'authdb' parameter set to the filename of the authentication database\n";
+                print "I HATE YOU\n";
+                exit 1;
+            }
+
+            my $authdb = $cfg->{gitcvs}->{authdb};
+
+            unless (-e $authdb) {
+                print "E The authentication database specified in [gitcvs.authdb] does not exist\n";
+                print "I HATE YOU\n";
+                exit 1;
+            }
+
+            my $auth_ok;
+            open my $passwd, "<", $authdb or die $!;
+            while (<$passwd>) {
+                if (m{^\Q$user\E:(.*)}) {
+                    if (crypt($user, descramble($password)) eq $1) {
+                        $auth_ok = 1;
+                    }
+                };
+            }
+            close $passwd;
 
-        unless ($auth_ok) {
-            print "I HATE YOU\n";
-            exit 1;
-        }
+            unless ($auth_ok) {
+                print "I HATE YOU\n";
+                exit 1;
+            }
+	}
 
         # Fall through to LOVE
     }
Ævar Arnfjörð Bjarmason· Jul 2, 2010, 14:33 UTC · re: László ÁSHIN · lore

Re: [PATCH] git-cvsserver: pserver-auth-script

On Fri, Jul 2, 2010 at 07:54, László ÁSHIN <laszlo.ashin@neti.hu> wrote:
Show 116 quoted lines
> Hi,
>
> The following patch makes git-cvsserver capable of authenticating users through an external executable script using pserver method.
> The script can be specified in the gitcvs section of the config file:
> [gitcvs]
>        enabled = 1
>        authscript = /some/where/script.sh
>
> The script, itself will get username and password on its standard input, so it can look like something like this:
>
> #!/bin/sh
> read username
> read password
>
> wbinfo -a "$username%$password"
>
> --
> Only a return value of zero means a successful authentication.
>
> Please comment and keep me on cc.
>
> --
> Regards,
> László Áshin
>
> diff -ruN a/git-cvsserver b/git-cvsserver
> --- a/git-cvsserver     2010-07-01 15:31:18.000000000 +0200
> +++ b/git-cvsserver     2010-07-01 15:35:41.000000000 +0200
> @@ -200,35 +200,54 @@
>         # Fall through to LOVE
>     } else {
>         # Trying to authenticate a user
> -        if (not exists $cfg->{gitcvs}->{authdb}) {
> -            print "E the repo config file needs a [gitcvs] section with an 'authdb' parameter set to the filename of the authentication database\n";
> -            print "I HATE YOU\n";
> -            exit 1;
> -        }
> -
> -        my $authdb = $cfg->{gitcvs}->{authdb};
> -
> -        unless (-e $authdb) {
> -            print "E The authentication database specified in [gitcvs.authdb] does not exist\n";
> -            print "I HATE YOU\n";
> -            exit 1;
> -        }
> -
> -        my $auth_ok;
> -        open my $passwd, "<", $authdb or die $!;
> -        while (<$passwd>) {
> -            if (m{^\Q$user\E:(.*)}) {
> -                if (crypt($user, descramble($password)) eq $1) {
> -                    $auth_ok = 1;
> -                }
> -            };
> -        }
> -        close $passwd;
> +        if (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 SCRIPTIN, '|' . $authscript or die $!;
> +            print SCRIPTIN $user . "\n";
> +            print SCRIPTIN descramble($password) . "\n";
> +            close SCRIPTIN;
> +            if ($? != 0) {
> +                print "E External script authentication failed.\n";
> +                print "I HATE YOU\n";
> +                exit 1;
> +            }
> +        } else {
> +            if (not exists $cfg->{gitcvs}->{authdb}) {
> +                print "E the repo config file needs a [gitcvs] section with an 'authdb' parameter set to the filename of the authentication database\n";
> +                print "I HATE YOU\n";
> +                exit 1;
> +            }
> +
> +            my $authdb = $cfg->{gitcvs}->{authdb};
> +
> +            unless (-e $authdb) {
> +                print "E The authentication database specified in [gitcvs.authdb] does not exist\n";
> +                print "I HATE YOU\n";
> +                exit 1;
> +            }
> +
> +            my $auth_ok;
> +            open my $passwd, "<", $authdb or die $!;
> +            while (<$passwd>) {
> +                if (m{^\Q$user\E:(.*)}) {
> +                    if (crypt($user, descramble($password)) eq $1) {
> +                        $auth_ok = 1;
> +                    }
> +                };
> +            }
> +            close $passwd;
>
> -        unless ($auth_ok) {
> -            print "I HATE YOU\n";
> -            exit 1;
> -        }
> +            unless ($auth_ok) {
> +                print "I HATE YOU\n";
> +                exit 1;
> +            }
> +       }
>
>         # Fall through to LOVE
>     }
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Ævar Arnfjörð Bjarmason· Jul 2, 2010, 14:39 UTC · re: László ÁSHIN · lore

Re: [PATCH] git-cvsserver: pserver-auth-script

On Fri, Jul 2, 2010 at 07:54, László ÁSHIN <laszlo.ashin@neti.hu> wrote:

Disregard the last E-Mail. I bumped into the wrong button on my keyboard.

Show 18 quoted lines
> The following patch makes git-cvsserver capable of authenticating users through an external executable script using pserver method.
> The script can be specified in the gitcvs section of the config file:
> [gitcvs]
>        enabled = 1
>        authscript = /some/where/script.sh
>
> The script, itself will get username and password on its standard input, so it can look like something like this:
>
> #!/bin/sh
> read username
> read password
>
> wbinfo -a "$username%$password"
>
> --
> Only a return value of zero means a successful authentication.
>
> Please comment and keep me on cc.

Good to see someone use the pserver auth code I added, even though I'm not doing so.

The idea looks good, please send another patch that adds documentation to git-cvsserver.txt too.

> diff -ruN a/git-cvsserver b/git-cvsserver
> --- a/git-cvsserver     2010-07-01 15:31:18.000000000 +0200
> +++ b/git-cvsserver     2010-07-01 15:35:41.000000000 +0200

Why isn't this a patch against git-cvsserver.perl? Presumably you made it without using the Git tools. It doesn't apply like this.

Show 40 quoted lines
> @@ -200,35 +200,54 @@
>         # Fall through to LOVE
>     } else {
>         # Trying to authenticate a user
> -        if (not exists $cfg->{gitcvs}->{authdb}) {
> -            print "E the repo config file needs a [gitcvs] section with an 'authdb' parameter set to the filename of the authentication database\n";
> -            print "I HATE YOU\n";
> -            exit 1;
> -        }
> -
> -        my $authdb = $cfg->{gitcvs}->{authdb};
> -
> -        unless (-e $authdb) {
> -            print "E The authentication database specified in [gitcvs.authdb] does not exist\n";
> -            print "I HATE YOU\n";
> -            exit 1;
> -        }
> -
> -        my $auth_ok;
> -        open my $passwd, "<", $authdb or die $!;
> -        while (<$passwd>) {
> -            if (m{^\Q$user\E:(.*)}) {
> -                if (crypt($user, descramble($password)) eq $1) {
> -                    $auth_ok = 1;
> -                }
> -            };
> -        }
> -        close $passwd;
> +        if (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 SCRIPTIN, '|' . $authscript or die $!;
> +            print SCRIPTIN $user . "\n";
> +            print SCRIPTIN descramble($password) . "\n";
> +            close SCRIPTIN;
Nit: Nice use of three-arg open, but you should use lexical
filehandles instead. I.e.:
    open my $script, '|' . $authscript or die $!;
    ...
Show 43 quoted lines
> +            if ($? != 0) {
> +                print "E External script authentication failed.\n";
> +                print "I HATE YOU\n";
> +                exit 1;
> +            }
> +        } else {
> +            if (not exists $cfg->{gitcvs}->{authdb}) {
> +                print "E the repo config file needs a [gitcvs] section with an 'authdb' parameter set to the filename of the authentication database\n";
> +                print "I HATE YOU\n";
> +                exit 1;
> +            }
> +
> +            my $authdb = $cfg->{gitcvs}->{authdb};
> +
> +            unless (-e $authdb) {
> +                print "E The authentication database specified in [gitcvs.authdb] does not exist\n";
> +                print "I HATE YOU\n";
> +                exit 1;
> +            }
> +
> +            my $auth_ok;
> +            open my $passwd, "<", $authdb or die $!;
> +            while (<$passwd>) {
> +                if (m{^\Q$user\E:(.*)}) {
> +                    if (crypt($user, descramble($password)) eq $1) {
> +                        $auth_ok = 1;
> +                    }
> +                };
> +            }
> +            close $passwd;
>
> -        unless ($auth_ok) {
> -            print "I HATE YOU\n";
> -            exit 1;
> -        }
> +            unless ($auth_ok) {
> +                print "I HATE YOU\n";
> +                exit 1;
> +            }
> +       }
>
>         # Fall through to LOVE
>     }

Otherwise this looks good. Submit something that's against the *.perl (and uses git format-patch / git send-email .. ) & has docs and I'll ack it.

Ævar Arnfjörð Bjarmason· Jul 2, 2010, 15:04 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] git-cvsserver: pserver-auth-script

On Fri, Jul 2, 2010 at 14:39, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Otherwise this looks good. Submit something that's against the *.perl
> (and uses git format-patch / git send-email .. ) & has docs and I'll
> ack it.

Oh, and it also needs tests. See the existing authdb tests in t9400-git-cvsserver-server.sh.

Jakub Narebski· Jul 2, 2010, 21:31 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] git-cvsserver: pserver-auth-script

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 13 quoted lines
> On Fri, Jul 2, 2010 at 07:54, László ÁSHIN <laszlo.ashin@neti.hu> wrote:
> 
> > +
> > +            open SCRIPTIN, '|' . $authscript or die $!;
> > +            print SCRIPTIN $user . "\n";
> > +            print SCRIPTIN descramble($password) . "\n";
> > +            close SCRIPTIN;
> 
> Nit: Nice use of three-arg open, but you should use lexical
> filehandles instead. I.e.:
> 
>     open my $script, '|' . $authscript or die $!;
>     ...

This is two-argument open, not three-argument magic open. There is string concatenation operator '.' there, not a comma ',' delimiting arguments.

It should be
   open my $script_fd, '|-', $authscript
   	or die "Couldn't open authentication script '$authscript': $!";
 
> > +        } else {
> > +            if (not exists $cfg->{gitcvs}->{authdb}) {
Why not elsif?
> > +                print "E the repo config file needs a [gitcvs] section with an 'authdb' parameter set to the filename of the authentication database\n";

Overly long line. Perhaps it would be better to split it into concatenated parts.

> > +            my $auth_ok;
> > +            open my $passwd, "<", $authdb or die $!;
And here you use three-argument form of (ordinary) open.
> > +            while (<$passwd>) {
> > +                if (m{^\Q$user\E:(.*)}) {
> > +                    if (crypt($user, descramble($password)) eq $1) {
Why nested if, and not short-circuit &&?
    +                if (/^\Q$user\E:(.*)/ &&
    +                    crypt($user, descramble($password)) eq $1) {
    
-- 
Jakub Narebski
Poland
ShadeHawk on #git
Ævar Arnfjörð Bjarmason· Jul 2, 2010, 21:34 UTC · re: Jakub Narebski · lore

Re: [PATCH] git-cvsserver: pserver-auth-script

On Fri, Jul 2, 2010 at 21:31, Jakub Narebski <jnareb@gmail.com> wrote:
Show 18 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> On Fri, Jul 2, 2010 at 07:54, László ÁSHIN <laszlo.ashin@neti.hu> wrote:
>>
>> > +
>> > +            open SCRIPTIN, '|' . $authscript or die $!;
>> > +            print SCRIPTIN $user . "\n";
>> > +            print SCRIPTIN descramble($password) . "\n";
>> > +            close SCRIPTIN;
>>
>> Nit: Nice use of three-arg open, but you should use lexical
>> filehandles instead. I.e.:
>>
>>     open my $script, '|' . $authscript or die $!;
>>     ...
>
> This is two-argument open, not three-argument magic open.  There is
> string concatenation operator '.' there, not a comma ',' delimiting
> arguments.
Well spotted, I misread that.
Show 20 quoted lines
> It should be
>
>   open my $script_fd, '|-', $authscript
>        or die "Couldn't open authentication script '$authscript': $!";
>
>> > +        } else {
>> > +            if (not exists $cfg->{gitcvs}->{authdb}) {
>
> Why not elsif?
>
>> > +                print "E the repo config file needs a [gitcvs] section with an 'authdb' parameter set to the filename of the authentication database\n";
>
> Overly long line.  Perhaps it would be better to split it into
> concatenated parts.
>
>
>> > +            my $auth_ok;
>> > +            open my $passwd, "<", $authdb or die $!;
>
> And here you use three-argument form of (ordinary) open.

This and the code below were already part of the code. But the patch would be better if it didn't move so much code around so that this would be obvious.

Show 14 quoted lines
>> > +            while (<$passwd>) {
>> > +                if (m{^\Q$user\E:(.*)}) {
>> > +                    if (crypt($user, descramble($password)) eq $1) {
>
> Why nested if, and not short-circuit &&?
>
>    +                if (/^\Q$user\E:(.*)/ &&
>    +                    crypt($user, descramble($password)) eq $1) {
>
> --
> Jakub Narebski
> Poland
> ShadeHawk on #git
>
Áshin László· Jul 3, 2010, 09:28 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] git-cvsserver: pserver-auth-script

Hi,
On Fri, Jul 2, 2010 at 23:34, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 27 quoted lines
> On Fri, Jul 2, 2010 at 21:31, Jakub Narebski <jnareb@gmail.com> wrote:
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>> On Fri, Jul 2, 2010 at 07:54, László ÁSHIN <laszlo.ashin@neti.hu> wrote:
>>>
>>> > +
>>> > +            open SCRIPTIN, '|' . $authscript or die $!;
>>> > +            print SCRIPTIN $user . "\n";
>>> > +            print SCRIPTIN descramble($password) . "\n";
>>> > +            close SCRIPTIN;
>>>
>>> Nit: Nice use of three-arg open, but you should use lexical
>>> filehandles instead. I.e.:
>>>
>>>     open my $script, '|' . $authscript or die $!;
>>>     ...
>>
>> This is two-argument open, not three-argument magic open.  There is
>> string concatenation operator '.' there, not a comma ',' delimiting
>> arguments.
>
> Well spotted, I misread that.
>
>> It should be
>>
>>   open my $script_fd, '|-', $authscript
>>        or die "Couldn't open authentication script '$authscript': $!";
>>
It will be fixed, I will resend the patch.
>>> > +        } else {
>>> > +            if (not exists $cfg->{gitcvs}->{authdb}) {
>>
>> Why not elsif?

Because the else branch continues below. But good point, it can be done that way too. Will be fixed.

Show 6 quoted lines
>>
>>> > +                print "E the repo config file needs a [gitcvs] section with an 'authdb' parameter set to the filename of the authentication database\n";
>>
>> Overly long line.  Perhaps it would be better to split it into
>> concatenated parts.
>>
OK. Next patch will be fixed from this point of view as well.
Show 10 quoted lines
>>
>>> > +            my $auth_ok;
>>> > +            open my $passwd, "<", $authdb or die $!;
>>
>> And here you use three-argument form of (ordinary) open.
>
> This and the code below were already part of the code. But the patch
> would be better if it didn't move so much code around so that this
> would be obvious.
>

I wanted to keep the old functionality (authdb), so I moved all of its code to this else branch. That move meant I had to increase indentation level. But as far as I see now I could have solved it somehow else to keep the indentation level. I will fix this too.

Show 9 quoted lines
>>> > +            while (<$passwd>) {
>>> > +                if (m{^\Q$user\E:(.*)}) {
>>> > +                    if (crypt($user, descramble($password)) eq $1) {
>>
>> Why nested if, and not short-circuit &&?
>>
>>    +                if (/^\Q$user\E:(.*)/ &&
>>    +                    crypt($user, descramble($password)) eq $1) {
>>
Good point, but it is not my code.

So, I will resend this patch - the corrected one without the mistakes I taken in the first version of it. Should I make one patch for each of the code, doc, and test case, or can they all go into one patch?

Regards, László ÁSHIN

Ævar Arnfjörð Bjarmason· Jul 3, 2010, 10:49 UTC · re: Áshin László · lore

Re: [PATCH] git-cvsserver: pserver-auth-script

On Sat, Jul 3, 2010 at 09:28, Áshin László <ashinlaszlo@gmail.com> wrote:
> So, I will resend this patch - the corrected one without the mistakes
> I taken in the first version of it.
Good.
> Should I make one patch for each of the code, doc, and test case, or
> can they all go into one patch?
They should all be part of the same patch.

← back to recent threads