Re: [PATCH_v1] add git credential login to remote mediawiki
On Sun, Jun 10, 2012 at 03:37:42PM +0200, Matthieu Moy wrote:
Show 11 quoted lines
> > Is there any reason not to add this to perl/Git.pm? I suspect that other
> > scripts will want to use it, too (for example, send-email could probably
> > use it for SMTP credentials).
>
> Currently, git-remote-mediawiki is a standalone script (doesn't use
> Git.pm). This is good because it makes it trivial to install, but bad in
> the sense that it may force us (or others) to reinvent the wheel.
>
> Until now, the wheels we reinvented were very simple (run_git
> essentially), but we may be reaching the point where it makes sense to
> use and contribute to Git.pm.
Yeah, I noticed that. But hopefully since they are at least in the same distribution, it is just a matter of getting the Makefiles right, and will not be an extra burden on the user.
Show 5 quoted lines
> Unfortunately, from a non-technical point of view, Javier is
> contributing this as part of a student project, which ends this week,
> and it's probably not reasonable to introduce such change so late. So,
> I'd keep it here at least for now, and a move to Git.pm could be a
> separate future topic.
Totally understood. I review from git's perspective, but it is up to you to manage your students' workload. We can migrate the code to Git.pm later (probably as part of a series which actually introduces a second caller).
Show 24 quoted lines
> > sub fill_credential {
> > my $quoted_url = quotemeta(shift);
> >
> > my $verbatim = `git credential fill $quoted_url`;
> > $? and die "git-credential failed";
> >
> > $verbatim =~ /^username=(.*)$/m
> > or die "git-credential did not give us a username";
> > my $username = $1;
> > $verbatim =~ /^password=(.*)$/m
> > or die "git-credential did not give us a password";
> >
> > return ($username, $password, $verbatim);
> > }
> >
> > sub report_credential {
> > my ($type, $verbatim) = @_;
> > open(my $fh, '|-', "git credential $type");
> > print $fh $verbatim;
> > }
>
> That sounds sensible too. We should be careful not to give a password as
> argument (or users of the same machine will be able to find it with e.g.
> "ps u"), but your proposal is OK with that.Yes, that was intentional (and is the reason why helpers do so much over stdin, even though it would reduce their parsing load if they could use command-line arguments). Unfortunately, there is still one case that reveals a password on the command-line: if a caller has a URL that contains an embedded password like:
https://bob:secret@example.com/foo.git
It's tempting to say "well, then they don't need to ask git-credential at all!". But the point of handing the whole URL to git-credential is so that the caller doesn't _have_ to do the parsing. So how would it know that the URL contains a password? :)
So instead of a URL on the command-line, it might make sense to simply let the caller send an extra "url=" parameter on stdin (in addition to any broken-down parameters, if it also wishes). It's way less convenient (you are stuck with open2 from perl, rather than simple backticks), but I think we should be cautious due to the security implications.
Show 23 quoted lines
> >> + # error if key undef
> >> + if (not defined $key) {
> >> + print STDERR "ERROR reciving reponse git credential fill\n";
> >> + exit 1;
> >> + }
> [...]
> >> + } else {
> >> + while (<Reader>) {
> >> + print STDERR "\nERROR while running git credential $op:\n$_";
> >> + }
> >> + }
> >> +}
> >
> > This isn't a good way to check for errors. The non-fill actions will
> > never produce output on stdout, and you are not intercepting their
> > stderr. Besides which, checking for errors by reading stderr is not a
> > good practice; you should check the return value of the command in $?
> > after it finishes.
>
> I think it should do both. In case "git credential fill" returns
> something that doesn't match the regexp, we don't want perl to error
> with "use of undefined value", but that's just being defensive because
> it shouldn't happen.Sorry, I just meant the latter block. It is checking for non-fill actions to send any output, which they will never do (yes, it would be an error for them to do so, but it is a very unlikely bug, and IMHO not really worth checking).
-Peff
