Re: [PATCH v7 4/5] merge/pull Check for untrusted good GPG signatures
Sebastian Götte writes:
> On 03/31/2013 05:03 PM, Thomas Rast wrote:
} sigcheck_gpg_status[] = {
{ 'G', "[GNUPG:] GOODSIG " },
{ 'B', "[GNUPG:] BADSIG " },
+ { 'U', "[GNUPG:] TRUST_NEVER" },
+ { 'U', "[GNUPG:] TRUST_UNDEFINED" },
[...]
>> And furthermore, to use an enum instead of a char so that you can easily
>> spell out the details in the code? This also has the advantage that the
>> compiler can check that your 'switch'es cover all cases.
> This char is actually from Junios original code. I think we can afford
> three chars. This could be changed if we ever need more than that.
*shrug*
I'm tempted to count the above as an argument in favor of the enum,
since there are in fact *four* chars... 'N' also counts. ;-)
But either way... I don't care too deeply and I don't know this corner
of the code. I just came here because of the valgrind discovery.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
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 v7 4/5] merge/pull Check for untrusted good GPG signatures
On Sun, Mar 31, 2013 at 05:03:44PM +0200, Thomas Rast wrote:
> John Keeping writes:
>
> > On Sun, Mar 31, 2013 at 04:33:57PM +0200, Sebastian Götte wrote:
> >> diff --git a/commit.c b/commit.c
> >> index eda7f90..bb2d9ad 100644
> >> --- a/commit.c
> >> +++ b/commit.c
> >> @@ -1029,6 +1029,8 @@ static struct {
> >> } sigcheck_gpg_status[] = {
> >>{ 'G', "[GNUPG:] GOODSIG " },
> >>{ 'B', "[GNUPG:] BADSIG " },
> >> + { 'U', "[GNUPG:] TRUST_NEVER" },
> >> + { 'U', "[GNUPG:] TRUST_UNDEFINED" },
> >> };
> >>
> >> static void parse_gpg_output(struct signature_check *sigc)
> >> @@ -1050,11 +1052,12 @@ static void parse_gpg_output(struct
> >> signature_check *sigc)
> >>found += strlen(sigcheck_gpg_status[i].check);
> >>}
> >>sigc->result = sigcheck_gpg_status[i].result;
> >> - sigc->key = xmemdupz(found, 16);
> >> - found += 17;
> >> - next = strchrnul(found, '\n');
> >> - sigc->signer = xmemdupz(found, next - found);
> >> - break;
> >> + if (sigc->result != 'U') {
> >
> > This could use a comment; we know now that only GOODSIG and BADSIG
> > are followed by a signature, but someone looking at this code in the
> > future will probably appreciate an explanation.
>
> Wouldn't it be even less magical if there was an explicit field in the
> struct that says whether we need to read a sig from such a line?
Yeah, that would be even better.
> And furthermore, to use an enum instead of a char so that you can easily
> spell out the details in the code? This also has the advantage that the
> compiler can check that your 'switch'es cover all cases.
>
> That's of course assuming that I interpret the logic right; my current
> understanding is that:
>
> * U means untrusted, which is bettern than B but worse than G;
Yes, although I wonder if we should split TRUST_NEVER and
TRUST_UNDEFINED (and maybe handle TRUST_MARGINAL as well) and print
different messages in each case.
> * GPG guarantees that the last line matching any of the patterns is the
> one we care about, so we can blindly override one with the other
The DETAILS file in GPG says:
For each signature only one of the codes GOODSIG, BADSIG, EXPSIG,
EXPKEYSIG, REVKEYSIG or ERRSIG will be emitted.
so we should be OK here.
--
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 v7 4/5] merge/pull Check for untrusted good GPG signatures
On 03/31/2013 05:03 PM, Thomas Rast wrote:
> John Keeping writes:
>
>> On Sun, Mar 31, 2013 at 04:33:57PM +0200, Sebastian Götte wrote:
>>> diff --git a/commit.c b/commit.c
>>> index eda7f90..bb2d9ad 100644
>>> --- a/commit.c
>>> +++ b/commit.c
>>> @@ -1029,6 +1029,8 @@ static struct {
>>> } sigcheck_gpg_status[] = {
>>> { 'G', "[GNUPG:] GOODSIG " },
>>> { 'B', "[GNUPG:] BADSIG " },
>>> + { 'U', "[GNUPG:] TRUST_NEVER" },
>>> + { 'U', "[GNUPG:] TRUST_UNDEFINED" },
>>> };
>>>
>>> static void parse_gpg_output(struct signature_check *sigc)
>>> @@ -1050,11 +1052,12 @@ static void parse_gpg_output(struct signature_check
>>> *sigc)
>>> found += strlen(sigcheck_gpg_status[i].check);
>>> }
>>> sigc->result = sigcheck_gpg_status[i].result;
>>> - sigc->key = xmemdupz(found, 16);
>>> - found += 17;
>>> - next = strchrnul(found, '\n');
>>> - sigc->signer = xmemdupz(found, next - found);
>>> - break;
>>> + if (sigc->result != 'U') {
>>
>> This could use a comment; we know now that only GOODSIG and BADSIG
>> are followed by a signature, but someone looking at this code in the
>> future will probably appreciate an explanation.
>
> Wouldn't it be even less magical if there was an explicit field in the
> struct that says whether we need to read a sig from such a line?
I think that special-case if a few lines below is OK for now.
> And furthermore, to use an enum instead of a char so that you can easily
> spell out the details in the code? This also has the advantage that the
> compiler can check that your 'switch'es cover all cases.
This char is actually from Junios original code. I think we can afford
three chars. This could be changed if we ever need more than that. Another
possible future feature would be to distinguish between "no signature" and
"public key not found" and to allow pass-through of that GPG "retrieve
missing public keys from keyserver" option.
> That's of course assuming that I interpret the logic right; my current
> understanding is that:
>
> * U means untrusted, which is bettern than B but worse than G;
Correct. Also, BADSIG is never followed by trust information.
> * GPG guarantees that the last line matching any of the patterns is the
> one we care about, so we can blindly override one with the other
Actually, we are searching *all* GPG status lines for every search string
in the array. This way, first GOODSIG may be matched to fill in the key
and signer information, but a subsequent TRUST_* match finally sets the
result code.
--
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 v7 4/5] merge/pull Check for untrusted good GPG signatures
John Keeping writes:
> On Sun, Mar 31, 2013 at 04:33:57PM +0200, Sebastian Götte wrote:
>> diff --git a/commit.c b/commit.c
>> index eda7f90..bb2d9ad 100644
>> --- a/commit.c
>> +++ b/commit.c
>> @@ -1029,6 +1029,8 @@ static struct {
>> } sigcheck_gpg_status[] = {
>> { 'G', "[GNUPG:] GOODSIG " },
>> { 'B', "[GNUPG:] BADSIG " },
>> +{ 'U', "[GNUPG:] TRUST_NEVER" },
>> +{ 'U', "[GNUPG:] TRUST_UNDEFINED" },
>> };
>>
>> static void parse_gpg_output(struct signature_check *sigc)
>> @@ -1050,11 +1052,12 @@ static void parse_gpg_output(struct signature_check
>> *sigc)
>> found += strlen(sigcheck_gpg_status[i].check);
>> }
>> sigc->result = sigcheck_gpg_status[i].result;
>> -sigc->key = xmemdupz(found, 16);
>> -found += 17;
>> -next = strchrnul(found, '\n');
>> -sigc->signer = xmemdupz(found, next - found);
>> -break;
>> +if (sigc->result != 'U') {
>
> This could use a comment; we know now that only GOODSIG and BADSIG
> are followed by a signature, but someone looking at this code in the
> future will probably appreciate an explanation.
Wouldn't it be even less magical if there was an explicit field in the
struct that says whether we need to read a sig from such a line?
And furthermore, to use an enum instead of a char so that you can easily
spell out the details in the code? This also has the advantage that the
compiler can check that your 'switch'es cover all cases.
That's of course assuming that I interpret the logic right; my current
understanding is that:
* U means untrusted, which is bettern than B but worse than G;
* GPG guarantees that the last line matching any of the patterns is the
one we care about, so we can blindly override one with the other
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
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 v7 4/5] merge/pull Check for untrusted good GPG signatures
On Sun, Mar 31, 2013 at 04:33:57PM +0200, Sebastian Götte wrote:
> When --verify-signatures is specified, abort the merge in case a good
> GPG signature from an untrusted key is encountered.
>
> Signed-off-by: Sebastian Götte
> ---
> Documentation/merge-options.txt| 4 ++--
> builtin/merge.c| 2 ++
> commit.c | 13 -
> commit.h | 10 +-
> gpg-interface.h| 1 +
> t/lib-gpg/pubring.gpg | Bin 1164 -> 2359 bytes
> t/lib-gpg/random_seed | Bin 600 -> 600 bytes
> t/lib-gpg/secring.gpg | Bin 1237 -> 3734 bytes
> t/lib-gpg/trustdb.gpg | Bin 1280 -> 1360 bytes
> t/t7612-merge-verify-signatures.sh | 9 +
> 10 files changed, 27 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index 31f1067..a0f022b 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -85,8 +85,8 @@ option can be used to override --squash.
>
> --verify-signatures::
> --no-verify-signatures::
> - Verify that the commits being merged have good GPG signatures and abort
> the
> - merge in case they do not.
> + Verify that the commits being merged have good and trusted GPG
> signatures
> + and abort the merge in case they do not.
>
> --summary::
> --no-summary::
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 7a33d03..752e3a9 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1248,6 +1248,8 @@ int cmd_merge(int argc, const char **argv, const char
> *prefix)
> switch(signature_check.result){
> case 'G':
> break;
> + case 'U':
> + die(_("Commit %s has a good, untrusted
> GPG signature allegedly by %s."), hex, signature_check.signer);
> case 'B':
> die(_("Commit %s has a bad GPG
> signature allegedly by %s."), hex, signature_check.signer);
> default: /* 'N' */
> diff --git a/commit.c b/commit.c
> index eda7f90..bb2d9ad 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1029,6 +1029,8 @@ static struct {
> } sigcheck_gpg_status[] = {
> { 'G', "[GNUPG:] GOODSIG " },
> { 'B', "[GNUPG:] BADSIG " },
> + { 'U', "[GNUPG:] TRUST_NEVER" },
> + { 'U', "[GNUPG:] TRUST_UNDEFINED" },
> };
>
> static void parse_gpg_output(struct signature_check *sigc)
> @@ -1050,11 +1052,12 @@ static void parse_gpg_output(struct signature_check
> *sigc)
> found += strlen(sigcheck_gpg_status[i].check);
> }
> sigc->result = sigcheck_gpg_status[i].result;
> - sigc->key = xmemdupz(found, 16);
> - found += 17;
> - next = strchrnul(found, '\n');
> - sigc->signer = xmemdupz(found, next - found);
> - break;
> + if (sigc->result != 'U') {
This could use a comment; we know now that only GOODSIG and BADSIG
are followed by a signature, but someone looking at this code in the
future will probably appreciate an explanation.
> + sigc->key = xmemdupz(found, 16);
> + found += 17;
> + next = strchrnul(found, '\n');
> + sigc->signer = xmemdupz(found, next - found);
> + }
> }
> }
>
--
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
