[PATCH 0/4] Make git-send-email git-credential

[PATCH 0/4] Make git-send-email git-credential

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

As discussed on the list, adding git-credential interface to Git.pm
(sort of copied from git-remote-mediawiki) and making git-send-email
use it.

I see git-remote-mediawiki does not have “use Git” so I did not touch
it.  On top of that I'd have no way to tests the changes anyway.

Michal Nazarewicz (4):
  Git.pm: Allow command_close_bidi_pipe() to be called as method
  Git.pm: Allow pipes to be closed prior to calling
    command_close_bidi_pipe
  Git.pm: Add interface for git credential command.
  git-send-email: Use git credential to obtain password.

 Documentation/git-send-email.txt |   4 +-
 git-send-email.perl              |  59 +++++++++++---------
 perl/Git.pm                      | 116 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 149 insertions(+), 30 deletions(-)

-- 
1.8.1.2.549.g4fa355e

[PATCH 1/4] Git.pm: Allow command_close_bidi_pipe() to be called as method

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

The documentation of command_close_bidi_pipe() claims that it can
be called as a method, but it does not check whether the first
argument is $self or not assuming the latter.  Using _maybe_self()
fixes this.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 perl/Git.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 931047c..bbb753a 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -430,7 +430,7 @@ have more complicated structure.
 
 sub command_close_bidi_pipe {
 	local $?;
-	my ($pid, $in, $out, $ctx) = @_;
+	my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_);
 	foreach my $fh ($in, $out) {
 		unless (close $fh) {
 			if ($!) {
-- 
1.8.1.2.549.g4fa355e

[PATCH 2/4] Git.pm: Allow pipes to be closed prior to calling command_close_bidi_pipe

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

The command_close_bidi_pipe() function will insist on closing both
input and output pipes returned by command_bidi_pipe().  With this
change it is possible to close one of the pipes in advance and
pass undef as an argument.

This allows for something like:

  my ($pid, $in, $out, $ctx) = command_bidi_pipe(...);
  print $out "write data";
  close $out;
  # ... do stuff with $in
  command_close_bidi_pipe($pid, $in, undef, $ctx);

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 perl/Git.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index bbb753a..6a2d52d 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -432,7 +432,7 @@ sub command_close_bidi_pipe {
 	local $?;
 	my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_);
 	foreach my $fh ($in, $out) {
-		unless (close $fh) {
+		if (defined $fh && !close $fh) {
 			if ($!) {
 				carp "error closing pipe: $!";
 			} elsif ($? >> 8) {
-- 
1.8.1.2.549.g4fa355e

[PATCH 3/4] Git.pm: Add interface for git credential command.

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

Add a credential() function which is an interface to the
git credential command.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 perl/Git.pm | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 111 insertions(+), 1 deletion(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 6a2d52d..5a18921 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -59,7 +59,8 @@ require Exporter;
                 command_bidi_pipe command_close_bidi_pipe
                 version exec_path html_path hash_object git_cmd_try
                 remote_refs prompt
-                temp_acquire temp_release temp_reset temp_path);
+                temp_acquire temp_release temp_reset temp_path
+                credential);
 
 
 =head1 DESCRIPTION
@@ -1000,6 +1001,115 @@ sub _close_cat_blob {
 }
 
 
+sub _credential_read {
+	my %credential;
+	my ($reader, $op) = (@_);
+	while (<$reader>) {
+		chomp;
+		my ($key, $value) = /([^=]*)=(.*)/;
+		if (not defined $key) {
+			throw Error::Simple("unable to parse git credential $op response:\n$_\n");
+		}
+		$credential{$key} = $value;
+	}
+	return %credential;
+}
+
+sub _credential_write {
+	my ($credential, $writer) = @_;
+
+	for my $key (sort {
+		# url overwrites other fields, so it must come first
+		return -1 if $a eq 'url';
+		return  1 if $b eq 'url';
+		return $a cmp $b;
+	} keys %$credential) {
+		if (defined $credential->{$key} && length $credential->{$key}) {
+			print $writer $key, '=', $credential->{$key}, "\n";
+		}
+	}
+	print $writer "\n";
+}
+
+sub _credential_run {
+	my ($self, $credential, $op) = _maybe_self(@_);
+
+	my ($pid, $reader, $writer, $ctx) = command_bidi_pipe('credential', $op);
+
+	_credential_write $credential, $writer;
+	close $writer;
+
+	if ($op eq "fill") {
+		%$credential = _credential_read $reader, $op;
+	} elsif (<$reader>) {
+		throw Error::Simple("unexpected output from git credential $op response:\n$_\n");
+	}
+
+	command_close_bidi_pipe($pid, $reader, undef, $ctx);
+}
+
+=item credential( CREDENTIAL_HASH [, OPERATION ] )
+
+=item credential( CREDENTIAL_HASH, CODE )
+
+Executes C<git credential> for a given set of credentials and
+specified operation.  In both form C<CREDENTIAL_HASH> needs to be
+a reference to a hash which stores credentials.  Under certain
+conditions the hash can change.
+
+In the first form, C<OPERATION> can be C<'fill'> (or omitted),
+C<'approve'> or C<'reject'>, and function will execute corresponding
+C<git credential> sub-command.  In case of C<'fill'> the values stored
+in C<CREDENTIAL_HASH> will be changed to the ones returned by the
+C<git credential> command.  The usual usage would look something like:
+
+	my %cred = (
+		'protocol' => 'https',
+		'host' => 'example.com',
+		'username' => 'bob'
+	);
+	Git::credential \%cred;
+	if (try_to_authenticate($cred{'username'}, $cred{'password'})) {
+		Git::credential \%cred, 'approve';
+		... do more stuff ...
+	} else {
+		Git::credential \%cred, 'reject';
+	}
+
+In the second form, C<CODE> needs to be a reference to a subroutine.
+The function will execute C<git credential fill> to fill provided
+credential hash, than call C<CODE> with C<CREDENTIAL> as the sole
+argument, and finally depending on C<CODE>'s return value execute
+C<git credential approve> (if return value yields true) or C<git
+credential reject> (otherwise).  The return value is the same as what
+C<CODE> returned.  With this form, the usage might look as follows:
+
+	if (Git::credential {
+		'protocol' => 'https',
+		'host' => 'example.com',
+		'username' => 'bob'
+	}, sub {
+		my $cred = shift;
+		return try_to_authenticate($cred->{'username'}, $cred->{'password'});
+	}) {
+		... do more stuff ...
+	}
+
+=cut
+
+sub credential {
+	my ($self, $credential, $op_or_code) = (_maybe_self(@_), 'fill');
+
+	if ('CODE' eq ref $op_or_code) {
+		_credential_run $credential, 'fill';
+		my $ret = $op_or_code->($credential);
+		_credential_run $credential, $ret ? 'approve' : 'reject';
+		return $ret;
+	} else {
+		_credential_run $credential, $op_or_code;
+	}
+}
+
 { # %TEMP_* Lexical Context
 
 my (%TEMP_FILEMAP, %TEMP_FILES);
-- 
1.8.1.2.549.g4fa355e

[PATCH 4/4] git-send-email: Use git credential to obtain password.

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

If smtp_user is provided but smtp_pass is not, instead of prompting
for password, make git-send-email use git credential command
instead.

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 Documentation/git-send-email.txt |  4 +--
 git-send-email.perl              | 59 +++++++++++++++++++++++-----------------
 2 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 44a1f7c..0cffef8 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -164,8 +164,8 @@ Sending
 Furthermore, passwords need not be specified in configuration files
 or on the command line. If a username has been specified (with
 '--smtp-user' or a 'sendemail.smtpuser'), but no password has been
-specified (with '--smtp-pass' or 'sendemail.smtppass'), then the
-user is prompted for a password while the input is masked for privacy.
+specified (with '--smtp-pass' or 'sendemail.smtppass'), then
+a password is obtained using 'git-credential'.
 
 --smtp-server=<host>::
 	If set, specifies the outgoing SMTP server to use (e.g.
diff --git a/git-send-email.perl b/git-send-email.perl
index be809e5..76bbfc3 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1045,6 +1045,39 @@ sub maildomain {
 	return maildomain_net() || maildomain_mta() || 'localhost.localdomain';
 }
 
+# Returns 1 if authentication succeeded or was not necessary
+# (smtp_user was not specified), and 0 otherwise.
+
+sub smtp_auth_maybe {
+	if (!defined $smtp_authuser || $auth) {
+		return 1;
+	}
+
+	# Workaround AUTH PLAIN/LOGIN interaction defect
+	# with Authen::SASL::Cyrus
+	eval {
+		require Authen::SASL;
+		Authen::SASL->import(qw(Perl));
+	};
+
+	# TODO: Authentication may fail not because credentials were
+	# invalid but due to other reasons, in which we should not
+	# reject credentials.
+	$auth = Git::credential({
+		'protocol' => 'smtp',
+		'host' => join(':', $smtp_server, $smtp_server_port),
+		'username' => $smtp_authuser,
+		# if there's no password, "git credential fill" will
+		# give us one, otherwise it'll just pass this one.
+		'password' => $smtp_authpass
+	}, sub {
+		my $cred = shift;
+		return !!$smtp->auth($cred->{'username'}, $cred->{'password'});
+	});
+
+	return $auth;
+}
+
 # Returns 1 if the message was sent, and 0 otherwise.
 # In actuality, the whole program dies when there
 # is an error sending a message.
@@ -1185,31 +1218,7 @@ X-Mailer: git-send-email $gitversion
 			    defined $smtp_server_port ? " port=$smtp_server_port" : "";
 		}
 
-		if (defined $smtp_authuser) {
-			# Workaround AUTH PLAIN/LOGIN interaction defect
-			# with Authen::SASL::Cyrus
-			eval {
-				require Authen::SASL;
-				Authen::SASL->import(qw(Perl));
-			};
-
-			if (!defined $smtp_authpass) {
-
-				system "stty -echo";
-
-				do {
-					print "Password: ";
-					$_ = <STDIN>;
-					print "\n";
-				} while (!defined $_);
-
-				chomp($smtp_authpass = $_);
-
-				system "stty echo";
-			}
-
-			$auth ||= $smtp->auth( $smtp_authuser, $smtp_authpass ) or die $smtp->message;
-		}
+		smtp_auth_maybe or die $smtp->message;
 
 		$smtp->mail( $raw_from ) or die $smtp->message;
 		$smtp->to( @recipients ) or die $smtp->message;
-- 
1.8.1.2.549.g4fa355e

Re: [PATCH 2/4] Git.pm: Allow pipes to be closed prior to calling command_close_bidi_pipe

[Jeff King]

On Wed, Feb 06, 2013 at 09:47:04PM +0100, Michal Nazarewicz wrote:

> From: Michal Nazarewicz <mina86@mina86.com>
> 
> The command_close_bidi_pipe() function will insist on closing both
> input and output pipes returned by command_bidi_pipe().  With this
> change it is possible to close one of the pipes in advance and
> pass undef as an argument.
> 
> This allows for something like:
> 
>   my ($pid, $in, $out, $ctx) = command_bidi_pipe(...);
>   print $out "write data";
>   close $out;
>   # ... do stuff with $in
>   command_close_bidi_pipe($pid, $in, undef, $ctx);

Should this part go into the documentation for command_close_bidi_pipe
in Git.pm?

-Peff

Re: [PATCH 3/4] Git.pm: Add interface for git credential command.

[Jeff King]

On Wed, Feb 06, 2013 at 09:47:05PM +0100, Michal Nazarewicz wrote:

> +sub _credential_read {
> +	my %credential;
> +	my ($reader, $op) = (@_);
> +	while (<$reader>) {
> +		chomp;
> +		my ($key, $value) = /([^=]*)=(.*)/;

Empty keys are not valid. Can we make this:

  /^([^=]+)=(.*)/

to fail the regex? Otherwise, I think this check:

> +		if (not defined $key) {
> +			throw Error::Simple("unable to parse git credential $op response:\n$_\n");
> +		}

would not pass because $key would be the empty string.

> +sub _credential_write {
> +	my ($credential, $writer) = @_;
> +
> +	for my $key (sort {
> +		# url overwrites other fields, so it must come first
> +		return -1 if $a eq 'url';
> +		return  1 if $b eq 'url';
> +		return $a cmp $b;
> +	} keys %$credential) {
> +		if (defined $credential->{$key} && length $credential->{$key}) {
> +			print $writer $key, '=', $credential->{$key}, "\n";
> +		}
> +	}

There are a few disallowed characters, like "\n" in key or value, and
"=" in a key. They should never happen unless the caller is buggy, but
should we check and catch them here?

> +In the second form, C<CODE> needs to be a reference to a subroutine.
> +The function will execute C<git credential fill> to fill provided
> +credential hash, than call C<CODE> with C<CREDENTIAL> as the sole
> +argument, and finally depending on C<CODE>'s return value execute
> +C<git credential approve> (if return value yields true) or C<git
> +credential reject> (otherwise).  The return value is the same as what
> +C<CODE> returned.  With this form, the usage might look as follows:

This is a nice touch. It makes the normal calling code a lot simpler.

-Peff

Re: [PATCH 4/4] git-send-email: Use git credential to obtain password.

[Jeff King]

On Wed, Feb 06, 2013 at 09:47:06PM +0100, Michal Nazarewicz wrote:

> From: Michal Nazarewicz <mina86@mina86.com>
> 
> If smtp_user is provided but smtp_pass is not, instead of prompting
> for password, make git-send-email use git credential command
> instead.
> 
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
> ---
>  Documentation/git-send-email.txt |  4 +--
>  git-send-email.perl              | 59 +++++++++++++++++++++++-----------------
>  2 files changed, 36 insertions(+), 27 deletions(-)

Nice. I don't see anything obviously wrong with the code, but I didn't
try it myself. I wonder how hard it would be to have some tests in
t9001. It looks like we don't test the smtp code paths at all, since we
would have to implement a fake smtp server. Which probably means the
answer is is "pretty hard", unless there is an easy-to-use CPAN smtp
server module we can plug in.

-Peff

Re: [PATCH 2/4] Git.pm: Allow pipes to be closed prior to calling command_close_bidi_pipe

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Feb 06, 2013 at 09:47:04PM +0100, Michal Nazarewicz wrote:
>
>> From: Michal Nazarewicz <mina86@mina86.com>
>> 
>> The command_close_bidi_pipe() function will insist on closing both
>> input and output pipes returned by command_bidi_pipe().  With this
>> change it is possible to close one of the pipes in advance and
>> pass undef as an argument.
>> 
>> This allows for something like:
>> 
>>   my ($pid, $in, $out, $ctx) = command_bidi_pipe(...);
>>   print $out "write data";
>>   close $out;
>>   # ... do stuff with $in
>>   command_close_bidi_pipe($pid, $in, undef, $ctx);
>
> Should this part go into the documentation for command_close_bidi_pipe
> in Git.pm?

Yeah, it probably should.

Re: [PATCH 3/4] Git.pm: Add interface for git credential command.

[Matthieu Moy]

Michal Nazarewicz <mpn@google.com> writes:

> From: Michal Nazarewicz <mina86@mina86.com>
>
> Add a credential() function which is an interface to the
> git credential command.

Nice.

I think you should credit git-remote-mediawiki for the code in the
commit message. Perhaps have a first "copy/paste" commit, and then an
"adaptation" commit to add sort, ^ anchor in regexp, doc and your
callback mechanism, but I won't insist on that.

Other than that, it all looks good, thanks. I'll take care of deleting
the old code in git-remote-mediawiki and use Git.pm instead.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 3/4] Git.pm: Add interface for git credential command.

[Matthieu Moy]

Michal Nazarewicz <mpn@google.com> writes:

> Subject: [PATCH 3/4] Git.pm: Add interface for git credential command.

Ah, just a nitpick: usually we write the message without capital after
":" and without the final ".".

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/