# [PATCH] git-cvsserver: pserver-auth-script 8 messages from 2010-07-02 to 2010-07-03. Participants: László ÁSHIN, Ævar Arnfjörð Bjarmason, Jakub Narebski, Áshin László. Thread: https://gitlist.dev/t/24262 ## László ÁSHIN, 2010-07-02 07:54 Subject: [PATCH] git-cvsserver: pserver-auth-script Message-ID: <20100702095406.7af15d20@ashinlaszlo.pta.neti.hu> URL: https://gitlist.dev/e/20100702095406.7af15d20%40ashinlaszlo.pta.neti.hu ``` 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, 2010-07-02 14:33 Subject: Re: [PATCH] git-cvsserver: pserver-auth-script Message-ID: URL: https://gitlist.dev/e/AANLkTil29nTVJS_dqsdyoH0BmRE8-U7HqNeeghDkbuEq%40mail.gmail.com In-Reply-To: <20100702095406.7af15d20@ashinlaszlo.pta.neti.hu> ``` On Fri, Jul 2, 2010 at 07:54, László ÁSHIN wrote: > 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, 2010-07-02 14:39 Subject: Re: [PATCH] git-cvsserver: pserver-auth-script Message-ID: URL: https://gitlist.dev/e/AANLkTinGZsCLF8CFKgQJaB63Jh9anGfQK4ZSHFPl4R4N%40mail.gmail.com In-Reply-To: <20100702095406.7af15d20@ashinlaszlo.pta.neti.hu> ``` On Fri, Jul 2, 2010 at 07:54, László ÁSHIN wrote: Disregard the last E-Mail. I bumped into the wrong button on my keyboard. > 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. > @@ -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 $!; ... > +            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, 2010-07-02 15:04 Subject: Re: [PATCH] git-cvsserver: pserver-auth-script Message-ID: URL: https://gitlist.dev/e/AANLkTin_Pd2_uJciKDzW8wLhCSFtvOOzdlwJ8eFmWc7q%40mail.gmail.com In-Reply-To: ``` On Fri, Jul 2, 2010 at 14:39, Ævar Arnfjörð Bjarmason 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, 2010-07-02 21:31 Subject: Re: [PATCH] git-cvsserver: pserver-auth-script Message-ID: URL: https://gitlist.dev/e/m31vbloa6m.fsf%40localhost.localdomain In-Reply-To: ``` Ævar Arnfjörð Bjarmason writes: > On Fri, Jul 2, 2010 at 07:54, László ÁSHIN 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, 2010-07-02 21:34 Subject: Re: [PATCH] git-cvsserver: pserver-auth-script Message-ID: URL: https://gitlist.dev/e/AANLkTimts_MlrWGKaiu3frk7vOnK9B70RE7It2Wml3on%40mail.gmail.com In-Reply-To: ``` On Fri, Jul 2, 2010 at 21:31, Jakub Narebski wrote: > Ævar Arnfjörð Bjarmason writes: >> On Fri, Jul 2, 2010 at 07:54, László ÁSHIN 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': $!"; > >> > +        } 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. >> > +            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ó, 2010-07-03 09:28 Subject: Re: [PATCH] git-cvsserver: pserver-auth-script Message-ID: URL: https://gitlist.dev/e/AANLkTinLgbrd-tOLntZt-FZkGVtTHXU2lteB-t7NAjSm%40mail.gmail.com In-Reply-To: ``` Hi, On Fri, Jul 2, 2010 at 23:34, Ævar Arnfjörð Bjarmason wrote: > On Fri, Jul 2, 2010 at 21:31, Jakub Narebski wrote: >> Ævar Arnfjörð Bjarmason writes: >>> On Fri, Jul 2, 2010 at 07:54, László ÁSHIN 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. >> >>> > +                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. >> >>> > +            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. >>> > +            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, 2010-07-03 10:49 Subject: Re: [PATCH] git-cvsserver: pserver-auth-script Message-ID: URL: https://gitlist.dev/e/AANLkTilRfqg62NcvuLjCmNhJRlt8GFXFPAhG_M4fEa56%40mail.gmail.com In-Reply-To: ``` On Sat, Jul 3, 2010 at 09:28, Áshin László 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. ```