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

From: Ted Zlatanov <tzz@lifelogs.com>
Date: Feb 5, 2013, 20:47 UTC
Message-ID: <877gmmqyho.fsf@lifelogs.com>
In-Reply-To: <7vhalqsfkf.fsf@alter.siamese.dyndns.org>

On Tue, 05 Feb 2013 11:53:20 -0800 Junio C Hamano <gitster@pobox.com> wrote:

JCH> Ted Zlatanov <tzz@lifelogs.com> writes:

Show 13 quoted lines

>> Changes since PATCHv3:
>> 
>> - simple tests in Makefile
>> - support multiple files, code refactored
>> - documentation and comments updated
>> - fix IO::File for GPG pipe
>> - exit peacefully in almost every situation, die on bad invocation or query
>> - use log_verbose() and -v for logging for the user
>> - use log_debug() and -d for logging for the developer
>> - use Net::Netrc parser and `man netrc' to improve parsing
>> - ignore 'default' and 'macdef' netrc entries
>> - require 'machine' token in netrc lines
>> - ignore netrc files with bad permissions or owner (from Net::Netrc)

JCH> Please place the above _after_ the three-dashes.

JCH> The space here, above "---", is to justify why this change is a good JCH> idea to people who see this patch for the first time who never saw JCH> the earlier rounds of this patch, e.g. reading "git log" output 6 JCH> months down the road (see Documentation/SubmittingPatches "(2) JCH> Describe your changes well").

Will do in PATCHv5.

JCH> Avoid starting an argument to "echo" with a dash; some JCH> implementations choke with "unknown option".

Nice, thanks.  It's purely decorative so I use '=>' now.

Show 5 quoted lines

>> +my %options = (
>> +               help => 0,
>> +               debug => 0,
>> +               verbose => 0,
>> +	       file => [],

JCH> Looks like there is some funny indentation going on here.

Fixed.

>> +  -f|--file AUTHFILE : specify netrc-style files.  Files with the .gpg extension
>> +                       will be decrypted by GPG before parsing.  Multiple -f
>> +                       arguments are OK, and the order is respected.

JCH> Saying "order is respected" without mentioning the collision JCH> resolution rules is not helpful to the users when deciding in what JCH> order they should give these files. First one wins, or last one JCH> wins? Later you say "looks for the first entry", but it will be JCH> much easier to read the above to mention it here as well.

Right.  Reworded.

Show 20 quoted lines

>> +Thus, when we get this query on STDIN:
>> +
>> +protocol=https
>> +username=tzz
>> +
>> +this credential helper will look for the first entry in every AUTHFILE that
>> +matches
>> +
>> +port https login tzz
>> +
>> +OR
>> +
>> +protocol https login tzz
>> +
>> +OR... etc. acceptable tokens as listed above.  Any unknown tokens are
>> +simply ignored.
>> +
>> +Then, the helper will print out whatever tokens it got from the entry, including
>> +"password" tokens, mapping back to Git's helper protocol; e.g. "port" is mapped
>> +back to "protocol".

JCH> Isn't "hostname" typically what users expect to see? It is somewhat JCH> unnerving to see an example that throws the same password back to JCH> any host you happen to have an accoutn "tzz" on, even though that is JCH> not technically an invalid way to use this helper.

Yeah, I changed it to show "machine" in the query (which would be more typical).

>> +			if ($stat[2] & 077) {
>> +				log_verbose("Insecure $file (mode=%04o); skipping it",
>> +					    $stat[2] & 07777);

JCH> Nice touch, although I am not sure rejecting world or group readable JCH> encrypted file is absolutely necessary.

Right.  Fixed.

>> +			if ($stat[4] != $<) {
>> +				log_verbose("Not owner of $file; skipping it");
>> +				next FILE;

JCH> OK. A group of local users may share the same account at the JCH> remote, but that would be unusual.

I added --insecure/-k to override this check.

>> +	if ($mode & 077)

JCH> Again?  Didn't you just do this?

Damn, sorry.

JCH> I think the prevalent style is to

JCH> if (condition) { JCH> do this; JCH> } elsif (another condition) { JCH> do that JCH> } else { JCH> do that other thing; JCH> }

JCH> (this comment applies to all if/elsif/else cascades in this patch).

Yup.  I was working with Net::Netrc code and forgot to reformat it.  Sorry.

>> +
>> +		next FILE;

JCH> Isn't this outermost loop, by the way? What the motivation to have JCH> an explicit label everywhere (not complaining---it could be your own JCH> discipline thing---just wondering).

I think it's more readable with large loops, and it actually makes sense when you read the code. Not a big deal to me either, I just felt for this particular script it was OK.

>> +	if ($file =~ m/\.gpg$/) {
>> +		log_verbose("Using GPG to open $file");
>> +		# GPG doesn't work well with 2- or 3-argument open

JCH> If that is the case, please quote $file properly against shell JCH> munging it.

Ahhh that gets ugly.  OK, quoted.

JCH> The only thing you do on $io is to read from it via "while (<$io>)", JCH> so I would personally have written this part like this without JCH> having to use IO::File(), though:

JCH> 	$io = open("-|", qw(gpg --decrypt), $file);

That doesn't work for me, unfortunately. I'm trying to avoid the IPC::* modules and such. Please test it yourself with GPG. I'm on Perl 5.14.2.

I think it's OK with the quoting, as you'll see in PATCHv5.

JCH> Similarly for the plain file:

JCH> 	$io = open("<", $file);

You mean "open $io, '<', $file". open() returns nonzero on success and undef on failure. OK, I'll use open() instead of IO::File.

>> +	my ($mach, $macdef, $tok, @tok) = (0, 0);

JCH> I think you meant to use $mach as a reference to a hash and $macdef JCH> as a reference to an array; do you want to initialize them to JCH> numeric zeros?

That actually came from Net::Netrc. The $mach default is OK either way; I left it undefined so it's clearer. I think the $macdef initial value is a bug (which, I guess, shows macros are very rare); I just made it a boolean for our purposes.

Ted