Re: [PATCHv6] Add contrib/credentials/netrc with GPG support

[Jeff King <peff@peff.net>]

On Tue, Feb 05, 2013 at 07:38:58PM -0500, Ted Zlatanov wrote:

> Add Git credential helper that can parse netrc/authinfo files.
> 
> This credential helper supports multiple files, returning the first one
> that matches.  It checks file permissions and owner.  For *.gpg files,
> it will run GPG to decrypt the file.
> 
> Signed-off-by: Ted Zlatanov <tzz@lifelogs.com>


> +	# the following check is copied from Net::Netrc, for non-GPG files
> +	# OS/2 and Win32 do not handle stat in a way compatable with this check :-(

s/compatable/compatible/

You mention os/2 and Win32 here, but the check has more:

> +	unless ($gpgmode || $options{insecure} ||
> +		$^O eq 'os2'
> +		|| $^O eq 'MSWin32'
> +		|| $^O eq 'MacOS'
> +		|| $^O =~ /^cygwin/) {

Does MacOS really not handle stat? Or is this old MacOS, not OS X?

> +sub load_netrc {
> [...]
> +	foreach my $nentry (@netrc_entries) {
> +		my %entry;
> +		my $num_port;
> +
> +		if (!defined $nentry->{machine}) {
> +			next;
> +		}
> +		if (defined $nentry->{port} && $nentry->{port} =~ m/^\d+$/) {
> +			$num_port = $nentry->{port};
> +			delete $nentry->{port};
> +		}
> +
> +		# create the new entry for the credential helper protocol
> +		$entry{$options{tmap}->{$_}} = $nentry->{$_} foreach keys %$nentry;
> +
> +		# for "host X port Y" where Y is an integer (captured by
> +		# $num_port above), set the host to "X:Y"
> +		if (defined $entry{host} && defined $num_port) {
> +			$entry{host} = join(':', $entry{host}, $num_port);
> +		}

So this will convert:

  machine foo port smtp

in the netrc into (protocol => "smtp", host => "foo"), but:

  machine foo port 25

into (protocol => undef, host => "foo:25"), right? That makes sense to
me.

> +sub net_netrc_loader {
> [...]

I won't comment here, as I know very little about netrc (I always
thought it was line-oriented, too!) and Junio has covered it.

> +# takes the search tokens and then a list of entries
> +# each entry is a hash reference
> +sub find_netrc_entry {
> +	my $query = shift @_;
> +
> +    ENTRY:
> +	foreach my $entry (@_)
> +	{
> +		my $entry_text = join ', ', map { "$_=$entry->{$_}" } keys %$entry;
> +		foreach my $check (sort keys %$query) {
> +			if (defined $query->{$check}) {
> +				log_debug("compare %s [%s] to [%s] (entry: %s)",
> +					  $check,
> +					  $entry->{$check},
> +					  $query->{$check},
> +					  $entry_text);
> +				unless ($query->{$check} eq $entry->{$check}) {
> +					next ENTRY;
> +				}
> +			} else {
> +				log_debug("OK: any value satisfies check $check");
> +			}

This looks right to me.

> +sub print_credential_data {

I don't know if you want to take the hit of relying on Git.pm (it is
nice for the helper to be totally standalone and copy-able), but one
obvious possible refactor would be to use the credential read/write
functions recently added there. I'm OK with not doing that, though.

> +	my $entry = shift @_;
> +	my $query = shift @_;
> +
> +	log_debug("entry has passed all the search checks");
> + TOKEN:
> +	foreach my $git_token (sort keys %$entry) {
> +		log_debug("looking for useful token $git_token");
> +		# don't print unknown (to the credential helper protocol) tokens
> +		next TOKEN unless exists $query->{$git_token};
> +
> +		# don't print things asked in the query (the entry matches them)
> +		next TOKEN if defined $query->{$git_token};
> +
> +		log_debug("FOUND: $git_token=$entry->{$git_token}");
> +		printf "%s=%s\n", $git_token, $entry->{$git_token};
> +	}

Printf? Bleh, isn't this supposed to be perl? :P

I don't see anything wrong from the credential-handling side of things.
As I said, I didn't look closely at the netrc parsing bits. From my
reading of "perldoc macos", the answer to my question above is "yes,
stat doesn't work on MacOS Classic". So I think the script itself is
fine.

In your tests:

> +++ b/contrib/credential/netrc/Makefile
> @@ -0,0 +1,12 @@
> +test_netrc:
> +       @(echo "bad data" | ./git-credential-netrc -f A -d -v) || echo "Bad invocation test, ignoring
> failure"
> +       @echo "=> Silent invocation... nothing should show up here with a missing file"
> +       @echo "bad data" | ./git-credential-netrc -f A get
> +       @echo "=> Back to noisy: -v and -d used below, missing file"
> +       echo "bad data" | ./git-credential-netrc -f A -d -v get
> +       @echo "=> Look for any entry in the default file set"
> +       echo "" | ./git-credential-netrc -d -v get
> +       @echo "=> Look for github.com in the default file set"
> +       echo "host=google.com" | ./git-credential-netrc -d -v get
> +       @echo "=> Look for a nonexistent machine in the default file set"
> +       echo "host=korovamilkbar" | ./git-credential-netrc -d -v get

You are depending on whatever the user has in their ~/.netrc, no?
Wouldn't it make more sense to ship a sample netrc and run all of the
tests with "-f netrc.example"?

It may also be worth building on top of the regular git test harness.
It's more work, but the resulting code (and the output) will be much
more readable.

-Peff