Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
On Tue, 05 Feb 2013 12:23:00 -0800 Junio C Hamano wrote: JCH> Ted Zlatanov writes: JCH> You still need to parse a file that has a "default" entry correctly; JCH> otherwise the users won't be able to share existing .netrc files JCH> with other applications e.g. ftp, which is the whole point of this JCH> series. Not using values from the "default" entry is probably fine, JCH> though. >> >> OK; done in PATCHv4. JCH> Hmph. JCH> Didn't you remove that from your version of net_netrc_loader when JCH> you borrowed the bulk of the code from Net::Netrc::_readrc? I see JCH> "default" token handled at the beginning of "TOKEN: while (@tok)" JCH> loop in the original but not in your version I see in v4. Damn, I accidentally moved that check to /dev/null. OK, I added it in PATCHv5. Thanks for catching that. Ted -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
Ted Zlatanov writes: > JCH> You still need to parse a file that has a "default" entry correctly; > JCH> otherwise the users won't be able to share existing .netrc files > JCH> with other applications e.g. ftp, which is the whole point of this > JCH> series. Not using values from the "default" entry is probably fine, > JCH> though. > > OK; done in PATCHv4. Hmph. Didn't you remove that from your version of net_netrc_loader when you borrowed the bulk of the code from Net::Netrc::_readrc? I see "default" token handled at the beginning of "TOKEN: while (@tok)" loop in the original but not in your version I see in v4. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
On Tue, 05 Feb 2013 11:47:56 -0800 Junio C Hamano wrote: JCH> Ted Zlatanov writes: JCH> Oh, another thing. 'default' is like 'machine' followed by any JCH> machine name, so the above while loop that reads two tokens JCH> pair-wise needs to be aware that 'default' is not followed by a JCH> value. I think the loop will fail to parse this: >> JCH> default login anonymouspassword me@home JCH> machine k.org login me password mysecret >> >> I'd prefer to ignore "default" because it should not be used for the Git >> credential helpers (its only use case is for anonymous services AFAIK). >> So I'll add a case to ignore it in PATCHv4, if that's OK. JCH> You still need to parse a file that has a "default" entry correctly; JCH> otherwise the users won't be able to share existing .netrc files JCH> with other applications e.g. ftp, which is the whole point of this JCH> series. Not using values from the "default" entry is probably fine, JCH> though. OK; done in PATCHv4. Ted -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
Ted Zlatanov writes: > JCH> Oh, another thing. 'default' is like 'machine' followed by any > JCH> machine name, so the above while loop that reads two tokens > JCH> pair-wise needs to be aware that 'default' is not followed by a > JCH> value. I think the loop will fail to parse this: > > JCH> default login anonymouspassword me@home > JCH> machine k.org login me password mysecret > > I'd prefer to ignore "default" because it should not be used for the Git > credential helpers (its only use case is for anonymous services AFAIK). > So I'll add a case to ignore it in PATCHv4, if that's OK. You still need to parse a file that has a "default" entry correctly; otherwise the users won't be able to share existing .netrc files with other applications e.g. ftp, which is the whole point of this series. Not using values from the "default" entry is probably fine, though. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
On Tue, 05 Feb 2013 08:15:48 -0800 Junio C Hamano wrote:
JCH> Ted Zlatanov writes:
>> +# build reverse token map
>> +my %rmap;
>> +foreach my $k (keys %{$options{tmap}}) {
>> +push @{$rmap{$options{tmap}->{$k}}}, $k;
>> +}
JCH> Mental note: "$rmap{foo} -eq 'bar'" means that what Git calls 'bar'
JCH> is found as 'foo' in the netrc/authinfo file. Keys in %rmap are
JCH> what we expect to read from the netrc/authinfo file.
I'll document that better in PATCHv4.
JCH> So you grabbed one line of input, split them into token pairs, and
JCH> built %tokens = ('key Git may want to see' => 'value read from file')
JCH> mapping.
This will be fixed with PATCHv4 to do multiple lines (as defined by the
netrc manpage, etc.).
>> +# for "host X port Y" where Y is an integer (captured by
>> +# $num_port above), set the host to "X:Y"
>> +$tokens{host} = join(':', $tokens{host}, $num_port)
>> +if defined $tokens{host} && defined $num_port;
JCH> What happens when 'host' does not exist? netrc/authinfo should be a
JCH> stream of SP/HT/LF delimited tokens and 'machine' token (or
JCH> 'default') begins a new entry, so it would mean the input file is
JCH> corrupt if we do not have $tokens{host} when we get here, I think.
Yes. I'll make the host/machine token required, which will avoid this
issue.
JCH> Oh, another thing. 'default' is like 'machine' followed by any
JCH> machine name, so the above while loop that reads two tokens
JCH> pair-wise needs to be aware that 'default' is not followed by a
JCH> value. I think the loop will fail to parse this:
JCH> default login anonymouspassword me@home
JCH> machine k.org login me password mysecret
I'd prefer to ignore "default" because it should not be used for the Git
credential helpers (its only use case is for anonymous services AFAIK).
So I'll add a case to ignore it in PATCHv4, if that's OK.
JCH> Hmph, aren't you checking what you read a bit too early? This is a
JCH> valid input:
JCH> default
JCH> login anonymous
JCH> password me@home
JCH> machine k.org
JCH> login me
JCH> password mysecret
JCH> but does this loop gives mysecret back to me when asked for
JCH> host=k.org and user=me?
To be fixed in PATCHv4, which will require the host/machine, use it as
the primary key, and only examine entries with it.
JCH> I would probably structure this part like this: [...]
I will do it like that, thank you for the suggestion.
I'll also add a simple testing Makefile for my own use, and you can
consider adding tests to the general framework later.
Ted
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
Junio C Hamano writes: > I thought I've given a more concrete outline than "I'll read > Net::Netrc and do whatever I think it does" in a separate message. And it turns out that the message was sitting in my outbox. Sorry. I just told the outbox to send it out. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
Ted Zlatanov writes:
> +# build reverse token map
> +my %rmap;
> +foreach my $k (keys %{$options{tmap}}) {
> + push @{$rmap{$options{tmap}->{$k}}}, $k;
> +}
Mental note: "$rmap{foo} -eq 'bar'" means that what Git calls 'bar'
is found as 'foo' in the netrc/authinfo file. Keys in %rmap are
what we expect to read from the netrc/authinfo file.
> +# there are CPAN modules to do this better, but we want to avoid
> +# dependencies and generally, complex netrc-style files are rare
> +
> +if ($debug) {
> + printf STDERR "searching for %s = %s\n", $_, $q{$_} || '(any value)'
> + foreach sort keys %q;
> +}
> +
> +LINE: foreach my $line (@data) {
> +
> + print STDERR "line [$line]\n" if $debug;
> + my @tok;
> + # gratefully stolen from Net::Netrc
> + while (length $line &&
> +$line =~ s/^("((?:[^"]+|\\.)*)"|((?:[^\\\s]+|\\.)*))\s*//) {
> + (my $tok = $+) =~ s/\\(.)/$1/g;
> + push(@tok, $tok);
> + }
> +
> + # skip blank lines, comments, etc.
> + next LINE unless scalar @tok;
> +
> + my %tokens;
> + my $num_port;
> + while (@tok) {
> + my ($k, $v) = (shift @tok, shift @tok);
> + next unless defined $v;
> + next unless exists $options{tmap}->{$k};
> + $tokens{$options{tmap}->{$k}} = $v;
> + $num_port = $v if $k eq 'port' && $v =~ m/^\d+$/;
> + }
So you grabbed one line of input, split them into token pairs, and
built %tokens = ('key Git may want to see' => 'value read from file')
mapping.
> + # for "host X port Y" where Y is an integer (captured by
> + # $num_port above), set the host to "X:Y"
> + $tokens{host} = join(':', $tokens{host}, $num_port)
> + if defined $tokens{host} && defined $num_port;
What happens when 'host' does not exist? netrc/authinfo should be a
stream of SP/HT/LF delimited tokens and 'machine' token (or
'default') begins a new entry, so it would mean the input file is
corrupt if we do not have $tokens{host} when we get here, I think.
Oh, another thing. 'default' is like 'machine' followed by any
machine name, so the above while loop that reads two tokens
pair-wise needs to be aware that 'default' is not followed by a
value. I think the loop will fail to parse this:
default login anonymouspassword me@home
machine k.org login me password mysecret
> + foreach my $check (sort keys %q) {
Hmph, aren't you checking what you read a bit too early? This is a
valid input:
default
login anonymous
password me@home
machine k.org
login me
password mysecret
but does this loop gives mysecret back to me when asked for
host=k.org and user=me?
> + if (exists $tokens{$check} && defined $q{$check}) {
> + print STDERR "comparing [$tokens{$check}] to
> [$q{$check}] in line [$line]\n" if $debug;
> + next LINE unless $tokens{$check} eq $q{$check};
> + }
> + else {
> + print STDERR "we could not find [$check] but it's OK\n"
> if $debug;
> + }
> + }
I would probably structure this part like this:
%pending = ();
split the whole input into tokens, regardless of lines;
iterate over the tokens {
peek the token
if (it is not "default") {
take (token, value) pair;
} else {
take "default" as token; value does not matter.
}
if (token is "default" or "machine") {
# finished reading one entry and we are
# at the beginning of the next entry.
# see if this entry matches
if (%pending is not empty &&
%pending matches %q) {
found a match; use %pending;
}
# done with that entry. now start a new one.
%pending = ();
}
$pending{token} = value;
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
Ted Zlatanov writes: > On Mon, 04 Feb 2013 16:15:47 -0800 Junio C Hamano wrote: > > JCH> Ted Zlatanov writes: > >>> - do you want to support backslashed newlines? > > JCH> What for? netrc/authinfo is not a line oriented file format at all, > JCH> and > > JCH> machine k.org > JCH> login me > JCH> password mysecret > > JCH> is a single entry; you do not need backslash at the end of any line. > > Hmm. The parser I implemented only does single-line parsing, and I > misunderstood the format to be single-line (partly because I have never > seen anyone using the multi-line format you show). Looking at > Net::Netrc more carefully, it seems that the "machine" token is what > defines an entry, so a new entry starts with a new line that contains a > "machine" token. Is that acceptable and does it match your > understanding of the format? It matches Net::Netrc, at least. I thought I've given a more concrete outline than "I'll read Net::Netrc and do whatever I think it does" in a separate message. It would be better to read "man netrc" carefully at least once ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
On Mon, 04 Feb 2013 16:15:47 -0800 Junio C Hamano wrote: JCH> Ted Zlatanov writes: >> - do you want to support backslashed newlines? JCH> What for? netrc/authinfo is not a line oriented file format at all, JCH> and JCH>machine k.org JCH>login me JCH> password mysecret JCH> is a single entry; you do not need backslash at the end of any line. Hmm. The parser I implemented only does single-line parsing, and I misunderstood the format to be single-line (partly because I have never seen anyone using the multi-line format you show). Looking at Net::Netrc more carefully, it seems that the "machine" token is what defines an entry, so a new entry starts with a new line that contains a "machine" token. Is that acceptable and does it match your understanding of the format? It matches Net::Netrc, at least. I'll add this change to PATCHv4 with the assumption you agree. >> - should all die() calls just print to STDERR and exit(0)? JCH> Where "when unhandled, the helper should silently exit with 0" is JCH> expected by the invoker, we shouldn't say anything to error stream, JCH> and exit with zero. Please leave a comment to make it easy to JCH> understand to the readers that is what is going on there. JCH> If on the other hand it diagnosed an error (not a bug in the JCH> implementation but a misconfiguration on the user's side), I _think_ JCH> it should loudly die() so that the user can notice and take JCH> corrective action. OK, I'll review these for PATCHv4 (also see below). Thanks. >> - do you want to support multiple netrc files, as you and Peff suggested? JCH> I didn't even suggest such thing IIRC---I expected it to iterate JCH> from the most desirable (.authinfo.gpg) to the least (.netrc) and JCH> stop at the first found one. There may be use cases people use more JCH> than one and expect an entry to be found in any file, but I suspect JCH> that might be more confusing than it is worth. But I do not care JCH> very deeply myself either way. After thinking about it, I agree with Peff multiple sources make sense and will simplify the code flow (especially the default case, which won't need to be handled separately). And the functionality doesn't have to be confusing with the right debugging messages. So I'll add them in PATCHv4. The debugging messages will be fewer and simpler with this approach, which makes it feel like the right track :) Thanks Ted -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
Ted Zlatanov writes: > On Mon, 04 Feb 2013 15:40:32 -0800 Junio C Hamano wrote: > > JCH> "Sorry we couldn't" sounded like an error messag to me. If this is > JCH> a normal exit, then please make sure it is a normal exit. > > OK; done in PATCHv4: removed all "Sorry" because they are not abnormal > exits. I'll hold PATCHv4 until the below are known. > > JCH> The review cycle is not like reviewers give you instructions and > JCH> designs and you blindly implement them. It is a creative process > JCH> where you show the design and a clear implementation of that design. > > OK. I would like you to make the decisions I asked for, though: > > - do you want to support backslashed newlines? What for? netrc/authinfo is not a line oriented file format at all, and machine k.org login me password mysecret is a single entry; you do not need backslash at the end of any line. Perhaps you are asking something different? > - do you want me to remove the statement modifiers? I do not think we are at that "implementation nitpick" level yet. > - should all die() calls just print to STDERR and exit(0)? Where "when unhandled, the helper should silently exit with 0" is expected by the invoker, we shouldn't say anything to error stream, and exit with zero. Please leave a comment to make it easy to understand to the readers that is what is going on there. If on the other hand it diagnosed an error (not a bug in the implementation but a misconfiguration on the user's side), I _think_ it should loudly die() so that the user can notice and take corrective action. > - do you want to support multiple netrc files, as you and Peff suggested? I didn't even suggest such thing IIRC---I expected it to iterate from the most desirable (.authinfo.gpg) to the least (.netrc) and stop at the first found one. There may be use cases people use more than one and expect an entry to be found in any file, but I suspect that might be more confusing than it is worth. But I do not care very deeply myself either way. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
On Mon, 04 Feb 2013 15:40:32 -0800 Junio C Hamano wrote: JCH> "Sorry we couldn't" sounded like an error messag to me. If this is JCH> a normal exit, then please make sure it is a normal exit. OK; done in PATCHv4: removed all "Sorry" because they are not abnormal exits. I'll hold PATCHv4 until the below are known. JCH> The review cycle is not like reviewers give you instructions and JCH> designs and you blindly implement them. It is a creative process JCH> where you show the design and a clear implementation of that design. OK. I would like you to make the decisions I asked for, though: - do you want to support backslashed newlines? - do you want me to remove the statement modifiers? - should all die() calls just print to STDERR and exit(0)? - do you want to support multiple netrc files, as you and Peff suggested? On all of those, I can go either way, it's just a little more work for me. Ted -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
On Mon, 4 Feb 2013 18:23:17 -0500 Jeff King wrote:
>> Perhaps "-r $file", if you say "is not accessible"?
JK> Even better: look at whether opening the file was successful. Though I
JK> guess that is complicated by the use of gpg, who will probably not
JK> distinguish ENOENT from other failures for us.
Yup. I think the outcome for the user will be the same, so this is
mostly for debugging, right? And we do look at the outcome of opening
the file, and die if that failed (which would change if your suggestion
below is implemented).
JK> I was trying not to be too nit-picky with my review, but here is how I
JK> would have written the outer logic of the script:
JK> my $tokens = read_credential_data_from_stdin();
JK> if ($options{file}) {
JK> my @entries = load_netrc($options{file})
JK> or die "unable to open $options{file}: $!";
JK> check_netrc($tokens, @entries);
JK> }
JK> else {
JK> foreach my $ext ('.gpg', '') {
JK> foreach my $base (qw(authinfo netrc)) {
JK> my @entries = load_netrc("$base$ext")
JK> or next;
JK> if (check_netrc($tokens, @entries)) {
JK> last;
JK> }
JK> }
JK> }
JK> }
JK> I.e., to fail on "-f", but otherwise treat unreadable auto-selected
JK> files as a no-op, for whatever reason.
JK> I'd also consider checking all files if they are available, in case
JK> the user has multiple (e.g., they keep low-quality junk unencrypted
JK> but some high-security passwords in a .gpg file). Not that likely,
JK> but not any harder to implement.
I think that makes everything more complicated, and the user can name a
specific netrc file in the helper spec if he wants it. It's too
automagic for me. But if you and Junio feel this is the right approach,
I'll rewrite to basically allow --file to take a list of filenames and
default that list to the base list of ~/.{authinfo,netrc}{,.gpg}
Ted
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
Ted Zlatanov writes:
>>> +my $mode = shift @ARGV;
>>> +
>>> +# credentials may get 'get', 'store', or 'erase' as parameters but
>>> +# only acknowledge 'get'
>>> +die "Syntax: $0 [-f AUTHFILE] [-d] get" unless defined $mode;
>>> +
>>> +# only support 'get' mode
>>> +exit unless $mode eq 'get';
>
> JCH> The above looks strange. Why does the invoker get the error message
> JCH> only when it runs this without arguments? Did you mean to say more
> JCH> like this?
>
> JCH> unless (defined $mode && $mode eq 'get') {
> JCH> die "...";
> JCH> }
>
> I mean:
>
> - if the mode is not given, exit badly (since it's required)
>
> - if the mode is given but we don't support it, exit pleasantly
>
> I thought that was the right thing, according to my reading of the
> credentials API. If not, I'll be glad to change it.
As Peff noted, I mistead what the code was doing, especially with
somewhat cryptic "only support x mode" comment, as if it is
rejecting other modes.
>>> + print STDERR "Sorry, we could not load data from [$file]\n" if $debug;
>>> + exit;
>
> JCH> Is this really an error? The file perhaps was empty. Shouldn't
> JCH> that case treated the same way as the case where no entry that
> JCH> matches the criteria invoker gave you was found?
>
> exit(0) is not an error, so the behavior is exactly the same, we just
> don't print anything to STDOUT because there was no data, with a nicer
> error message. I think that's what we want?
"Sorry we couldn't" sounded like an error messag to me. If this is
a normal exit, then please make sure it is a normal exit.
The review cycle is not like reviewers give you instructions and
designs and you blindly implement them. It is a creative process
where you show the design and a clear implementation of that design.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
Jeff King writes:
> On Mon, Feb 04, 2013 at 02:56:06PM -0800, Junio C Hamano wrote:
>
>> > +my $mode = shift @ARGV;
>> > +
>> > +# credentials may get 'get', 'store', or 'erase' as parameters but
>> > +# only acknowledge 'get'
>> > +die "Syntax: $0 [-f AUTHFILE] [-d] get" unless defined $mode;
>> > +
>> > +# only support 'get' mode
>> > +exit unless $mode eq 'get';
>>
>> The above looks strange. Why does the invoker get the error message
>> only when it runs this without arguments? Did you mean to say more
>> like this?
>>
>> unless (defined $mode && $mode eq 'get') {
>> die "...";
>> }
>
> Not having a mode is an invocation error; the credential-helper
> documentation indicates that the helper will always be invoked with an
> action. The likely culprit for not having one is the user invoking it
> manually, and showing the usage there is a sensible action.
>
> Whereas invoking it with a mode other than "get" is not an error at all.
> Git will run it with the "store" and "erase" actions, too. Those happen
> to be no-ops for this helper, so it exits silently. The credential docs
> specify that any other actions should be ignored, too, to allow for
> future expansion.
OK. The code didn't express the above reasoning clearly enough.
> I was trying not to be too nit-picky with my review,...
I wasn't either. Mine was still at design level review to get the
semantics right (e.g. what to consider as errors, the input is _not_
one entry per line, etc.), before reviewing the details of the
implementation.
> but here is how I
> would have written the outer logic of the script:
>
> my $tokens = read_credential_data_from_stdin();
> if ($options{file}) {
> my @entries = load_netrc($options{file})
> or die "unable to open $options{file}: $!";
> check_netrc($tokens, @entries);
> }
> else {
> foreach my $ext ('.gpg', '') {
> foreach my $base (qw(authinfo netrc)) {
> my @entries = load_netrc("$base$ext")
> or next;
> if (check_netrc($tokens, @entries)) {
> last;
> }
> }
> }
> }
>
> I.e., to fail on "-f", but otherwise treat unreadable auto-selected
> files as a no-op, for whatever reason. I'd also consider checking all
> files if they are available, in case the user has multiple (e.g., they
> keep low-quality junk unencrypted but some high-security passwords in a
> .gpg file). Not that likely, but not any harder to implement.
Yeah, I think that looks like the right top-level codeflow.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
On Mon, 04 Feb 2013 14:56:06 -0800 Junio C Hamano wrote:
JCH> I recall that netrc/authinfo files are _not_ line oriented. Earlier
JCH> you said "looks for entries that match" which is a lot more correct,
JCH> but then we see "look for lines in authfile".
Hmm, do you mean backslashed newlines? I think the Net::Netrc parser
doesn't support them, and I haven't seen them in the wild, but I could
support them if you think that's useful.
>> +my $mode = shift @ARGV;
>> +
>> +# credentials may get 'get', 'store', or 'erase' as parameters but
>> +# only acknowledge 'get'
>> +die "Syntax: $0 [-f AUTHFILE] [-d] get" unless defined $mode;
>> +
>> +# only support 'get' mode
>> +exit unless $mode eq 'get';
JCH> The above looks strange. Why does the invoker get the error message
JCH> only when it runs this without arguments? Did you mean to say more
JCH> like this?
JCH>unless (defined $mode && $mode eq 'get') {
JCH>die "...";
JCH>}
I mean:
- if the mode is not given, exit badly (since it's required)
- if the mode is given but we don't support it, exit pleasantly
I thought that was the right thing, according to my reading of the
credentials API. If not, I'll be glad to change it.
JCH> By the way, I think statement modifiers tend to get overused and
JCH> make the resulting program harder to read. die "..." at the
JCH> beginning of line makes the reader go "Whoa, it already is done and
JCH> existing on error", and then forces the eyes to scan the error
JCH> message to find "unless" and the condition.
JCH> It may be a cute syntax and some may find it even cool, but cuteness
JCH> or coolness is less valuable compared with the readability.
Your coding guidelines said you prefer one-line if statements, and I
thought it would be OK to lean on modifiers. I changed many of the
modifiers but not all; please let me know if you'd like me to change
them all. It's no problem.
JCH> Is it sensible to squelch the error message by default and force
JCH> user to specify --debug? You could argue that the option is to
JCH> debug the user's configuration, but the name of the option sounds
JCH> more like it is for debugging this script itself.
It's both... without a clear separation because it's such a small
script. Let me know how you'd like to change it, if at all.
JCH> I saw Peff already pointed out error conditions, but I am not sure
JCH> why all of these exit with 0. If the user has configured
JCH>git config credential.helper 'netrc -f $HOME/.netcr'
JCH> shouldn't it be diagnosed as an error? It is understandable to let
JCH> this go silently
JCH>git config credential.helper 'netrc'
JCH> and let other credential helpers take over when no
$HOME/.{netrc,authinfo}{,.gpg}
JCH> file exist, but in that case the user may still want to remove the
JCH> config item that is not doing anything useful and erroring out with
JCH> a message may be a way to help the user know about the situation.
You and Peff should tell me how it should behave, or perhaps make the
changes after it's in. I'm happy to change it any way you like, but at
this point I'm just following instructions, not really contributing,
about the exit statuses. I thought I knew what you wanted 2 iterations
ago :)
>> +print STDERR "Sorry, we could not load data from [$file]\n" if $debug;
>> +exit;
JCH> Is this really an error? The file perhaps was empty. Shouldn't
JCH> that case treated the same way as the case where no entry that
JCH> matches the criteria invoker gave you was found?
exit(0) is not an error, so the behavior is exactly the same, we just
don't print anything to STDOUT because there was no data, with a nicer
error message. I think that's what we want?
PATCHv3 is out with the rest of your suggestions. Thank you for the
thorough review. I am happy to improve the script to meet your standards.
Thanks
Ted
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
On Mon, Feb 04, 2013 at 02:56:06PM -0800, Junio C Hamano wrote:
> > +my $mode = shift @ARGV;
> > +
> > +# credentials may get 'get', 'store', or 'erase' as parameters but
> > +# only acknowledge 'get'
> > +die "Syntax: $0 [-f AUTHFILE] [-d] get" unless defined $mode;
> > +
> > +# only support 'get' mode
> > +exit unless $mode eq 'get';
>
> The above looks strange. Why does the invoker get the error message
> only when it runs this without arguments? Did you mean to say more
> like this?
>
> unless (defined $mode && $mode eq 'get') {
> die "...";
> }
Not having a mode is an invocation error; the credential-helper
documentation indicates that the helper will always be invoked with an
action. The likely culprit for not having one is the user invoking it
manually, and showing the usage there is a sensible action.
Whereas invoking it with a mode other than "get" is not an error at all.
Git will run it with the "store" and "erase" actions, too. Those happen
to be no-ops for this helper, so it exits silently. The credential docs
specify that any other actions should be ignored, too, to allow for
future expansion.
> > +my $debug = $options{debug};
> > +my $file = $options{file};
> > +
> > +unless (defined $file) {
> > + print STDERR "Please specify an existing netrc file (with or without a
> > .gpg extension) with -f AUTHFILE\n" if $debug;
> > + exit 0;
> > +}
> > +
> > +unless (-f $file) {
> > + print STDERR "Sorry, the specified netrc $file is not accessible\n" if
> > $debug;
> > + exit 0;
> > +}
>
> Perhaps "-r $file", if you say "is not accessible"?
Even better: look at whether opening the file was successful. Though I
guess that is complicated by the use of gpg, who will probably not
distinguish ENOENT from other failures for us.
> Is it sensible to squelch the error message by default and force
> user to specify --debug? You could argue that the option is to
> debug the user's configuration, but the name of the option sounds
> more like it is for debugging this script itself.
>
> I saw Peff already pointed out error conditions, but I am not sure
> why all of these exit with 0. If the user has configured
It was from my suggestion to ignore missing files, which is that the
user might have the helper configured (e.g., via /etc/gitconfig, or by a
shared ~/.gitconfig) but not actually have a netrc.
It gets confusing because the contents of $file may have been
auto-detected, or it may have come from the command-line, and we do not
remember which at this point.
I was trying not to be too nit-picky with my review, but here is how I
would have written the outer logic of the script:
my $tokens = read_credential_data_from_stdin();
if ($options{file}) {
my @entries = load_netrc($options{file})
or die "unable to open $options{file}: $!";
check_netrc($tokens, @entries);
}
else {
foreach my $ext ('.gpg', '') {
foreach my $base (qw(authinfo netrc)) {
my @entries = load_netrc("$base$ext")
or next;
if (check_netrc($tokens, @entries)) {
last;
}
}
}
}
I.e., to fail on "-f", but otherwise treat unreadable auto-selected
files as a no-op, for whatever reason. I'd also consider checking all
files if they are available, in case the user has multiple (e.g., they
keep low-quality junk unencrypted but some high-security passwords in a
.gpg file). Not that likely, but not any harder to implement.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support, try #2
Ted Zlatanov writes:
> Signed-off-by: Ted Zlatanov
The space above your S-o-b: could be utilized a bit better ;-)
> @@ -0,0 +1,236 @@
> +#!/usr/bin/perl
> +
> +use strict;
> +use warnings;
> +
> +use Getopt::Long;
> +use File::Basename;
> +
> +my $VERSION = "0.1";
> +
> +my %options = (
> + help => 0,
> + debug => 0,
> +
> + # identical token maps, e.g. host -> host, will be inserted
> later
> + tmap => {
> +port => 'protocol',
> +machine => 'host',
> +path => 'path',
> +login => 'username',
> +user => 'username',
> +password => 'password',
> + }
> + );
> +
> +# map each credential protocol token to itself on the netrc side
> +$options{tmap}->{$_} = $_ foreach values %{$options{tmap}};
> +
> +foreach my $suffix ('.gpg', '') {
> + foreach my $base (qw/authinfo netrc/) {
> + my $file = glob("~/.$base$suffix");
> + next unless (defined $file && -f $file);
> + $options{file} = $file ;
> + }
> +}
This checks .gpg first and then unencrypted, and checks authinfo
first and netrc second, both of which makes sense. It is good to
encourage use of encrypted files, and it is good to use newer
authinfo files over netrc files.
However, it is strange that you let the ones that are discovered
later in the loop to override the ones that are discovered earlier.
Perhaps you meant
next unless (... exists there ...);
$options{"file"} = $file;
last;
instead?
> +Getopt::Long::Configure("bundling");
Hmm, OK.
> +# TODO: maybe allow the token map $options{tmap} to be configurable.
> +GetOptions(\%options,
> + "help|h",
> + "debug|d",
> + "file|f=s",
> + );
> +
> +if ($options{help}) {
> + my $shortname = basename($0);
> + $shortname =~ s/git-credential-//;
> +
> + print < +
> +$0 [-f AUTHFILE] [-d] get
> +
> +Version $VERSION by tzz\@lifelogs.com. License: BSD.
> +
> +Options:
> + -f AUTHFILE: specify a netrc-style file
> + -d: turn on debugging
> +
> +To enable (note that Git will prepend "git-credential-" to the helper
> +name and look for it in the path):
> +
> + git config credential.helper '$shortname -f AUTHFILE'
> +
> +And if you want lots of debugging info:
> +
> + git config credential.helper '$shortname -f AUTHFILE -d'
> +
> +Only "get" mode is supported by this credential helper. It opens
> +AUTHFILE and looks for entries that match the requested search
> +criteria:
> +
> + 'port|protocol':
> + The protocol that will be used (e.g., https). (protocol=X)
> +
> + 'machine|host':
> + The remote hostname for a network credential. (host=X)
> +
> + 'path':
> + The path with which the credential will be used. (path=X)
> +
> + 'login|user|username':
> + The credential’s username, if we already have one. (username=X)
> +
> +Thus, when we get this query on STDIN:
> +
> +protocol=https
> +username=tzz
> +
> +this credential helper will look for lines in AUTHFILE that match
> +
> +port https login tzz
> +
> +OR
> +
> +protocol https login tzz
> +
> +OR... etc. acceptable tokens as listed above. Any unknown tokens are
> +simply ignored.
I recall that netrc/authinfo files are _not_ line oriented. Earlier
you said "looks for entries that match" which is a lot more correct,
but then we see "look for lines in authfile".
> +Then, the helper will print out whatever tokens it got from the line,
> +including "password" tokens, mapping e.g. "port" back to "protocol".
Again "line" is mentioned twice, above and below.
> +The first matching line is used. Tokens can be quoted as 'STRING' or
> +"STRING".
> +
> +No caching is performed by this credential helper.
> +
> +EOHIPPUS
> +
> + exit;
> +}
> +my $mode = shift @ARGV;
> +
> +# credentials may get 'get', 'store', or 'erase' as parameters but
> +# only acknowledge 'get'
> +die "Syntax: $0 [-f AUTHFILE] [-d] get" unless defined $mode;
> +
> +# only support 'get' mode
> +exit unless $mode eq 'get';
The above looks strange. Why does the invoker get the error message
only when it runs this without arguments? Did you mean to say more
like this?
unless (defined $mode && $mode eq 'get') {
die "...";
}
By the way, I think statement modifiers tend to get overused and
make the resulting program harder to read. die "..." at the
beginning of line makes the reader go "Whoa, it already is done and
existing on error", and then forces the eyes to scan the error
message to find "unless" and the condition.
It may be a cute syntax and some may find it even cool, but cuteness
or coolness is less valuable compared with the readability.
> +my $debug = $options{debug};
> +my $file = $options{file};
> +
> +unless (defined $file) {
> + print STDERR "Please specify an existing netrc file (with
[PATCH] Add contrib/credentials/netrc with GPG support, try #2
Signed-off-by: Ted Zlatanov
---
contrib/credential/netrc/git-credential-netrc | 236 +
1 files changed, 236 insertions(+), 0 deletions(-)
create mode 100755 contrib/credential/netrc/git-credential-netrc
diff --git a/contrib/credential/netrc/git-credential-netrc
b/contrib/credential/netrc/git-credential-netrc
new file mode 100755
index 000..d265fde
--- /dev/null
+++ b/contrib/credential/netrc/git-credential-netrc
@@ -0,0 +1,236 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use Getopt::Long;
+use File::Basename;
+
+my $VERSION = "0.1";
+
+my %options = (
+ help => 0,
+ debug => 0,
+
+ # identical token maps, e.g. host -> host, will be inserted
later
+ tmap => {
+port => 'protocol',
+machine => 'host',
+path => 'path',
+login => 'username',
+user => 'username',
+password => 'password',
+ }
+ );
+
+# map each credential protocol token to itself on the netrc side
+$options{tmap}->{$_} = $_ foreach values %{$options{tmap}};
+
+foreach my $suffix ('.gpg', '') {
+ foreach my $base (qw/authinfo netrc/) {
+ my $file = glob("~/.$base$suffix");
+ next unless (defined $file && -f $file);
+ $options{file} = $file ;
+ }
+}
+
+Getopt::Long::Configure("bundling");
+
+# TODO: maybe allow the token map $options{tmap} to be configurable.
+GetOptions(\%options,
+ "help|h",
+ "debug|d",
+ "file|f=s",
+ );
+
+if ($options{help}) {
+ my $shortname = basename($0);
+ $shortname =~ s/git-credential-//;
+
+ print < undef } values(%{$options{tmap}});
+
+while () {
+ next unless m/^([^=]+)=(.+)/;
+
+ my ($token, $value) = ($1, $2);
+ die "Unknown search token $1" unless exists $q{$token};
+ $q{$token} = $value;
+}
+
+# build reverse token map
+my %rmap;
+foreach my $k (keys %{$options{tmap}}) {
+ push @{$rmap{$options{tmap}->{$k}}}, $k;
+}
+
+# there are CPAN modules to do this better, but we want to avoid
+# dependencies and generally, complex netrc-style files are rare
+
+if ($debug) {
+ printf STDERR "searching for %s = %s\n", $_, $q{$_} || '(any value)'
+ foreach sort keys %q;
+}
+
+LINE: foreach my $line (@data) {
+
+ print STDERR "line [$line]\n" if $debug;
+ my @tok;
+ # gratefully stolen from Net::Netrc
+ while (length $line &&
+ $line =~ s/^("((?:[^"]+|\\.)*)"|((?:[^\\\s]+|\\.)*))\s*//) {
+ (my $tok = $+) =~ s/\\(.)/$1/g;
+ push(@tok, $tok);
+ }
+
+ # skip blank lines, comments, etc.
+ next LINE unless scalar @tok;
+
+ my %tokens;
+ my $num_port;
+ while (@tok) {
+ my ($k, $v) = (shift @tok, shift @tok);
+ next unless defined $v;
+ next unless exists $options{tmap}->{$k};
+ $tokens{$options{tmap}->{$k}} = $v;
+ $num_port = $v if $k eq 'port' && $v =~ m/^\d+$/;
+ }
+
+ # for "host X port Y" where Y is an integer (captured by
+ # $num_port above), set the host to "X:Y"
+ $tokens{host} = join(':', $tokens{host}, $num_port)
+ if defined $tokens{host} && defined $num_port;
+
+ foreach my $check (sort keys %q) {
+ if (exists $tokens{$check} && defined $q{$check}) {
+ print STDERR "comparing [$tokens{$check}] to
[$q{$check}] in line [$line]\n" if $debug;
+ next LINE unless $tokens{$check} eq $q{$check};
+ }
+ else {
+ print STDERR "we could not find [$check] but it's OK\n"
if $debug;
+ }
+ }
+
+ print STDERR "line has passed all the search checks\n" if $debug;
+ TOKEN:
+ foreach my $token (sort keys %rmap) {
+ print STDERR "looking for useful token $token\n" if $debug;
+ next unless exists $tokens{$token}; # did we match?
+
+ foreach my $rctoken (@{$rmap{$token}}) {
+ next TOKEN if defined $q{$rctoken}; # don't
re-print given tokens
+ }
+
+ print STDERR "FOUND: $token=$tokens{$token}\n" if $debug;
+ printf "%s=%s\n", $token, $tokens{$token};
+ }
+
+ last;
+}
+
+sub load {
+ # this supports pipes too
+ my $io = new IO::File(@_) or die "Could not open [@_]: $!\n";
+ return <$io>; # whole file
+}
--
1.7.9.rc2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support
On Mon, 4 Feb 2013 16:17:26 -0500 Jeff King wrote:
JK> Do you need to quote "\n" here?
Fixed.
JK> Hmm, so it's not an error (just a warning) to say:
JK> git credential-netrc -f /does/not/exist
JK> but it is an error to say:
JK> git credential-netrc
JK> and have it fail to find any netrc files. Shouldn't the latter be a
JK> lesser error than the former?
Fixed, they should both exit(0).
>> +next unless m/([^=]+)=(.+)/;
JK> Should this regex be anchored at the start of the string?
Fixed.
>> +printf STDERR "searching for %s = %s\n", $_, $q{$_} || '(any value)'
>> + foreach sort keys %q;
JK> Leftover one-char indent.
Fixed.
Ted
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add contrib/credentials/netrc with GPG support
On Mon, Feb 04, 2013 at 02:54:30PM -0500, Ted Zlatanov wrote:
> + print < +
> +$0 [-f AUTHFILE] [-d] get
> +
> +Version $VERSION by tzz\@lifelogs.com. License: BSD.
This here-doc is interpolated so you can use $0 and $VERSION, and
therefore have to quote the @-sign. But later in the here-doc...
> +Thus, when we get "protocol=https\nusername=tzz", this credential
> +helper will look for lines in AUTHFILE that match
Do you need to quote "\n" here?
> +die "Sorry, you need to specify an existing netrc file (with or without a
> .gpg extension) with -f AUTHFILE"
> + unless defined $file;
> +
> +unless (-f $file) {
> + print STDERR "Sorry, the specified netrc $file is not accessible\n" if
> $debug;
> + exit 0;
> +}
Hmm, so it's not an error (just a warning) to say:
git credential-netrc -f /does/not/exist
but it is an error to say:
git credential-netrc
and have it fail to find any netrc files. Shouldn't the latter be a
lesser error than the former?
> +while () {
> + next unless m/([^=]+)=(.+)/;
> +
> + my ($token, $value) = ($1, $2);
> + die "Unknown search token $1" unless exists $q{$token};
> + $q{$token} = $value;
> +}
Should this regex be anchored at the start of the string? I think the
left-to-right matching means we will correctly match:
key=value with=in it
so it may be OK.
> +if ($debug) {
> + printf STDERR "searching for %s = %s\n", $_, $q{$_} || '(any value)'
> + foreach sort keys %q;
> +}
Leftover one-char indent.
> [...]
The rest looks OK to me.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Add contrib/credentials/netrc with GPG support
Signed-off-by: Ted Zlatanov
---
contrib/credential/netrc/git-credential-netrc | 223 +
1 files changed, 223 insertions(+), 0 deletions(-)
create mode 100755 contrib/credential/netrc/git-credential-netrc
diff --git a/contrib/credential/netrc/git-credential-netrc
b/contrib/credential/netrc/git-credential-netrc
new file mode 100755
index 000..7b43aa9
--- /dev/null
+++ b/contrib/credential/netrc/git-credential-netrc
@@ -0,0 +1,223 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use Getopt::Long;
+use File::Basename;
+
+my $VERSION = "0.1";
+
+my %options = (
+ help => 0,
+ debug => 0,
+
+ # identical token maps, e.g. host -> host, will be inserted
later
+ tmap => {
+port => 'protocol',
+machine => 'host',
+path => 'path',
+login => 'username',
+user => 'username',
+password => 'password',
+ }
+ );
+
+# map each credential protocol token to itself on the netrc side
+$options{tmap}->{$_} = $_ foreach my $v (values %{$options{tmap}});
+
+foreach my $suffix ('.gpg', '') {
+ foreach my $base (qw/authinfo netrc/) {
+ my $file = glob("~/.$base$suffix");
+ next unless (defined $file && -f $file);
+ $options{file} = $file ;
+ }
+}
+
+Getopt::Long::Configure("bundling");
+
+# TODO: maybe allow the token map $options{tmap} to be configurable.
+GetOptions(\%options,
+ "help|h",
+ "debug|d",
+ "file|f=s",
+ );
+
+if ($options{help}) {
+ my $shortname = basename($0);
+ $shortname =~ s/git-credential-//;
+
+ print < undef } values(%{$options{tmap}});
+
+while () {
+ next unless m/([^=]+)=(.+)/;
+
+ my ($token, $value) = ($1, $2);
+ die "Unknown search token $1" unless exists $q{$token};
+ $q{$token} = $value;
+}
+
+# build reverse token map
+my %rmap;
+foreach my $k (keys %{$options{tmap}}) {
+ push @{$rmap{$options{tmap}->{$k}}}, $k;
+}
+
+# there are CPAN modules to do this better, but we want to avoid
+# dependencies and generally, complex netrc-style files are rare
+
+if ($debug) {
+ printf STDERR "searching for %s = %s\n", $_, $q{$_} || '(any value)'
+foreach sort keys %q;
+}
+
+LINE: foreach my $line (@data) {
+
+ print STDERR "line [$line]\n" if $debug;
+ my @tok;
+ # gratefully stolen from Net::Netrc
+ while (length $line &&
+ $line =~ s/^("((?:[^"]+|\\.)*)"|((?:[^\\\s]+|\\.)*))\s*//) {
+ (my $tok = $+) =~ s/\\(.)/$1/g;
+ push(@tok, $tok);
+ }
+
+ # skip blank lines, comments, etc.
+ next LINE unless scalar @tok;
+
+ my %tokens;
+ while (@tok) {
+ my ($k, $v) = (shift @tok, shift @tok);
+ next unless defined $v;
+ next unless exists $options{tmap}->{$k};
+ $tokens{$options{tmap}->{$k}} = $v;
+ }
+
+ foreach my $check (sort keys %q) {
+ if (exists $tokens{$check} && defined $q{$check}) {
+ print STDERR "comparing [$tokens{$check}] to
[$q{$check}] in line [$line]\n" if $debug;
+ next LINE unless $tokens{$check} eq $q{$check};
+ }
+ else {
+ print STDERR "we could not find [$check] but it's OK\n"
if $debug;
+ }
+ }
+
+ print STDERR "line has passed all the search checks\n" if $debug;
+ TOKEN:
+ foreach my $token (sort keys %rmap) {
+ print STDERR "looking for useful token $token\n" if $debug;
+ next unless exists $tokens{$token}; # did we match?
+
+ foreach my $rctoken (@{$rmap{$token}}) {
+ next TOKEN if defined $q{$rctoken}; # don't
re-print given tokens
+ }
+
+ print STDERR "FOUND: $token=$tokens{$token}\n" if $debug;
+ printf "%s=%s\n", $token, $tokens{$token};
+ }
+
+ last;
+}
+
+sub load {
+ # this supports pipes too
+ my $io = new IO::File(@_) or die "Could not open [@_]: $!\n";
+ return <$io>; # whole file
+}
--
1.7.9.rc2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
