*Re: [PATCH v2 0/9] X509 (gpgsm) commit signing support
2018-07-10 8:52 [PATCH v2 0/9] X509 (gpgsm) commit signing support Henning Schild
` (8 preceding siblings ...)
2018-07-10 8:52 ` [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM Henning Schild
@ 2018-07-10 17:12 ` Jeff King
2018-07-14 18:33 ` brian m. carlson9 siblings, 1 reply; 57+ messages in thread
From: Jeff King @ 2018-07-10 17:12 UTC (permalink / raw)
To: Henning Schild
Cc: git, Eric Sunshine, Junio C Hamano, Martin Ågren, Ben Toews,
Taylor Blau, brian m . carlson
On Tue, Jul 10, 2018 at 10:52:22AM +0200, Henning Schild wrote:
> This series adds support for signing commits with gpgsm.
Thanks for working on this. I left a bunch of comments, but overall the
direction looks good.
We talked about this a bit off-list, but just for the public record:
> This series can be seen as a follow up of a series that appeared under
> the name "gpg-interface: Multiple signing tools" in april 2018 [1]. After
> that series was not merged i decided to get my patches ready. The
> original series aimed at being generic for any sort of signing tool, while
> this series just introduced the X509 variant of gpg. (gpgsm)
> I collected authors and reviewers of that first series and already put them
> on cc.
This series is a fine replacement for that earlier work. It's flexible
enough to allow what we really wanted out of that series (gpgsm support,
or another drop-in tool that uses the same interface). It doesn't lay
any groundwork for further tools (like signify), but I think the
consensus on the list was to punt on that until somebody had more
concrete plans for adding such a tool.
-Peff
^permalinkrawreply [flat|nested] 57+ messages in thread

*Re: [PATCH v2 8/9] gpg-interface: introduce new signature format "x509" using gpgsm
2018-07-10 17:40 ` Junio C Hamano@ 2018-07-10 17:50 ` Jeff King0 siblings, 0 replies; 57+ messages in thread
From: Jeff King @ 2018-07-10 17:50 UTC (permalink / raw)
To: Junio C Hamano
Cc: Henning Schild, git, Eric Sunshine, Martin Ågren, Ben Toews,
Taylor Blau, brian m . carlson
On Tue, Jul 10, 2018 at 10:40:22AM -0700, Junio C Hamano wrote:
> > Extremely minor nit, but if there are no other uses of PGP_SIGNATURE etc
> > outside of this array (as I hope there wouldn't be after this series),
> > would it make more sense to just include the literals inline in the
> > array definition? That's one less layer of indirection when somebody is
> > reading the code.
>
> It is good design-sense to shoot for fewer levels of indirection,
> but I suspect that "'const char **' instead of maximally-sized fixed
> array of strings" would require a named array and constants like
> this:
Yes, I agree with that direction (because it drops the magic numbers and
lets us use existing argv_array helpers).
> [...]
> so we may end up having the same number of levels of indirection
> anyway in the long-term final form.
True, but at least this level of indirection is buying us something. :)
> As readers may be able to read from the above, I also have a
> suspicion that it is a mistake to pretend that "--verify" etc.,
> which merely happen to be common across the variants the series
> covers, will stay forever to be common across _all_ variants and
> that is why the field no longer is called "extra" args but is meant
> to contain the full args.
I'd be fine going in that direction, too. But I don't actually foresee
adding new variants in the future. The point of this series versus the
signingtool one is that it's limited to gpg and gpg-alikes. And I doubt
we're likely to see more than the two that exist.
So even if we do end up adding support for more tools in the long run, I
think it will outgrow this config scheme.
-Peff
^permalinkrawreply [flat|nested] 57+ messages in thread

*Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore
2018-07-11 8:54 ` Henning Schild@ 2018-07-11 12:34 ` Jeff King
2018-07-11 13:46 ` Henning Schild0 siblings, 1 reply; 57+ messages in thread
From: Jeff King @ 2018-07-11 12:34 UTC (permalink / raw)
To: Henning Schild
Cc: git, Eric Sunshine, Junio C Hamano, Martin Ågren, Ben Toews,
Taylor Blau, brian m . carlson
On Wed, Jul 11, 2018 at 10:54:59AM +0200, Henning Schild wrote:
> > In the general case you need:
> >
> > found = *next ? next + 1 : next;
> >
> > or similar. In this case, you can actually do:
> >
> > found = next;
> >
> > because we know that it's OK to search over the literal space again.
> > But that's pretty subtle, so we're probably better off just doing the
> > conditional above.
> >
> > (And yes, looking at the existing code, I think it's even worse, as
> > there does not seem to be a guarantee that we even have 16 characters
> > in the string).
>
> The existing code works only on expected output and the same is true
> for the version after this patch. Making the parser robust against
> random input would imho be a sort of cleanup patch on top of my
> series. .. or before, in which case i would become responsible for
> making sure that still works after my modification.
> This argument is twofold. I do not really want to fix that as well and
> it might be a good idea to separate concerns anyways.
I think it's worth addressing in the near term, if only because this
kind of off-by-one is quite subtle, and I don't want to forget to deal
with it. Whether that happens as part of this patch, or as a cleanup
before or after, I'm not picky. :)
-Peff
^permalinkrawreply [flat|nested] 57+ messages in thread

*Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM
2018-07-11 10:38 ` Henning Schild@ 2018-07-11 12:51 ` Jeff King
2018-07-11 13:40 ` Henning Schild
2018-07-14 18:26 ` brian m. carlson1 sibling, 1 reply; 57+ messages in thread
From: Jeff King @ 2018-07-11 12:51 UTC (permalink / raw)
To: Henning Schild
Cc: git, Eric Sunshine, Junio C Hamano, Martin Ågren, Ben Toews,
Taylor Blau, brian m . carlson
On Wed, Jul 11, 2018 at 12:38:24PM +0200, Henning Schild wrote:
> > Can we save a dummy generated key and just import it? That's what we
> > do for the regular gpg case.
>
> I will look into storing a binary and leaving notes how it was
> generated, just like regular gpg does. The reason i did not do that in
> the first place is that x509 certs have a validity and we introduce
> time into the picture. But i will see if i can generate epoch->infinity
> to get the host clock or just the future out of the picture.
That would be preferable. But even if we can just get something like a
10-year expiration, that may be enough. Somebody dealing with failing
tests and regenerating keys in ten years is probably not the end of the
world.
It could hurt people with drastically incorrect system clocks, but I
suspect there are other tests with similar problems (especially if your
clock is in the past).
> > We're going to have a lot of duplicated tests here. That's a
> > maintenance burden when one of them needs fixes later. And when new
> > tests are added, we won't automatically get them tested under each
> > format.
> >
> > Can we move the battery of tests into a function that takes a few
> > parameters (prereq name, branch to look at, etc) and then call it for
> > both the gpg/gpgsm cases?
>
> I guess this is part of the earlier "allow GPGSM without GPG" and i
> can ignore it if we agree that this is not needed?
I think it's orthogonal. Even if GPGSM requires GPG, you'd still want to
make sure that whatever exercise we give to the GPG code is also
exercised using GPGSM.
I will note, though, that _some_ tests are not really exercising
gpg-specific bits, but more how we react to it (e.g., how we format %G
placeholders). And it's probably OK for those to just be run once.
So in an ideal world, the test script would probably look something
like:
# this function holds tests which exercise the interactions
# with the gpg binary itself
type_specific_tests() {
prereq=$1
branch=$2
test_expect_success $prereq "test whatever ($prereq)" '
some test using $branch here
'
}
test_expect_success 'setup' '
set up both openpgp and x509 branches here
'
type_specific_tests GPG openpgp
type_specific_tests GPGSM x509
# and now tests that generically care about getting _some_ signature
# result (e.g., the way we format signature info)
# and then probably a few tests specific to how the config is handled,
# like your new gpg.format coverage
But in practice, the type-specific bits are often muddled together with
the type-independent ones (e.g., we happen to test the parsing of a
failed signature from gpg by checking how %G? is formatted or similar).
So it may be simplest to just run most of the tests twice, once with gpg
and once with gpgsm. I kind of wonder if all of t7510 could just be
bumped into a function. Or even into a sourced file and run from two
different scripts. See the way that t8001 and t8002 use
annotate-tests.sh for an example.
-Peff
^permalinkrawreply [flat|nested] 57+ messages in thread

*Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM
2018-07-11 12:51 ` Jeff King@ 2018-07-11 13:40 ` Henning Schild
2018-07-11 14:35 ` Jeff King0 siblings, 1 reply; 57+ messages in thread
From: Henning Schild @ 2018-07-11 13:40 UTC (permalink / raw)
To: Jeff King
Cc: git, Eric Sunshine, Junio C Hamano, Martin Ågren, Ben Toews,
Taylor Blau, brian m . carlson
Am Wed, 11 Jul 2018 08:51:10 -0400
schrieb Jeff King <peff@peff.net>:
> On Wed, Jul 11, 2018 at 12:38:24PM +0200, Henning Schild wrote:
>
> > > Can we save a dummy generated key and just import it? That's what
> > > we do for the regular gpg case.
> >
> > I will look into storing a binary and leaving notes how it was
> > generated, just like regular gpg does. The reason i did not do that
> > in the first place is that x509 certs have a validity and we
> > introduce time into the picture. But i will see if i can generate
> > epoch->infinity to get the host clock or just the future out of the
> > picture.
>
> That would be preferable. But even if we can just get something like a
> 10-year expiration, that may be enough. Somebody dealing with failing
> tests and regenerating keys in ten years is probably not the end of
> the world.
>
> It could hurt people with drastically incorrect system clocks, but I
> suspect there are other tests with similar problems (especially if
> your clock is in the past).
I now have a working version with a pregenerated certificate from
1970-3000. I would be surprised if git is still around at that time ;).
> > > We're going to have a lot of duplicated tests here. That's a
> > > maintenance burden when one of them needs fixes later. And when
> > > new tests are added, we won't automatically get them tested under
> > > each format.
> > >
> > > Can we move the battery of tests into a function that takes a few
> > > parameters (prereq name, branch to look at, etc) and then call it
> > > for both the gpg/gpgsm cases?
> >
> > I guess this is part of the earlier "allow GPGSM without GPG" and i
> > can ignore it if we agree that this is not needed?
>
> I think it's orthogonal. Even if GPGSM requires GPG, you'd still want
> to make sure that whatever exercise we give to the GPG code is also
> exercised using GPGSM.
>
> I will note, though, that _some_ tests are not really exercising
> gpg-specific bits, but more how we react to it (e.g., how we format %G
> placeholders). And it's probably OK for those to just be run once.
>
> So in an ideal world, the test script would probably look something
> like:
>
> # this function holds tests which exercise the interactions
> # with the gpg binary itself
> type_specific_tests() {
> prereq=$1
> branch=$2
>
> test_expect_success $prereq "test whatever ($prereq)" '
> some test using $branch here
> '
> }
>
> test_expect_success 'setup' '
> set up both openpgp and x509 branches here
> '
>
> type_specific_tests GPG openpgp
> type_specific_tests GPGSM x509
>
> # and now tests that generically care about getting _some_ signature
> # result (e.g., the way we format signature info)
>
> # and then probably a few tests specific to how the config is
> handled, # like your new gpg.format coverage
>
> But in practice, the type-specific bits are often muddled together
> with the type-independent ones (e.g., we happen to test the parsing
> of a failed signature from gpg by checking how %G? is formatted or
> similar).
>
> So it may be simplest to just run most of the tests twice, once with
> gpg and once with gpgsm. I kind of wonder if all of t7510 could just
> be bumped into a function. Or even into a sourced file and run from
> two different scripts. See the way that t8001 and t8002 use
> annotate-tests.sh for an example.
I do not agree and would like to leave the tests as they are. Instead
of introducing a whole lot of very similar copies, i added just a few.
The original ones are even very similar between each other.
We are again talking about two problems. 1. we need test cases for
gpgsm if we want to merge gpgsm 2. the testsuite is very repetitive
While addressing 1 make 2 obvious and worse, addressing 2 is a whole
different story and should probably be discussed outside of this
thread. And i would not like to inherit responsibility for 2. In
fact the whole discussion emphasizes that it was a good idea to make
GPGSM depend on GPG, because it allows to somewhat reuse existing tests.
Henning
> -Peff
^permalinkrawreply [flat|nested] 57+ messages in thread

*Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore
2018-07-11 12:34 ` Jeff King@ 2018-07-11 13:46 ` Henning Schild
2018-07-11 14:27 ` Jeff King0 siblings, 1 reply; 57+ messages in thread
From: Henning Schild @ 2018-07-11 13:46 UTC (permalink / raw)
To: Jeff King
Cc: git, Eric Sunshine, Junio C Hamano, Martin Ågren, Ben Toews,
Taylor Blau, brian m . carlson
Am Wed, 11 Jul 2018 08:34:25 -0400
schrieb Jeff King <peff@peff.net>:
> On Wed, Jul 11, 2018 at 10:54:59AM +0200, Henning Schild wrote:
>
> > > In the general case you need:
> > >
> > > found = *next ? next + 1 : next;
> > >
> > > or similar. In this case, you can actually do:
> > >
> > > found = next;
> > >
> > > because we know that it's OK to search over the literal space
> > > again. But that's pretty subtle, so we're probably better off
> > > just doing the conditional above.
> > >
> > > (And yes, looking at the existing code, I think it's even worse,
> > > as there does not seem to be a guarantee that we even have 16
> > > characters in the string).
> >
> > The existing code works only on expected output and the same is true
> > for the version after this patch. Making the parser robust against
> > random input would imho be a sort of cleanup patch on top of my
> > series. .. or before, in which case i would become responsible for
> > making sure that still works after my modification.
> > This argument is twofold. I do not really want to fix that as well
> > and it might be a good idea to separate concerns anyways.
>
> I think it's worth addressing in the near term, if only because this
> kind of off-by-one is quite subtle, and I don't want to forget to deal
> with it. Whether that happens as part of this patch, or as a cleanup
> before or after, I'm not picky. :)
I get that and if anyone is willing to write that code, i will base my
patches on it. What i want to avoid is taking responsibility for
problems i did not introduce, just because i happen to work on that
code at the moment. Keeping track of that (not forgetting) is also not
for the random contributor like myself.
Henning
> -Peff
^permalinkrawreply [flat|nested] 57+ messages in thread

*Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore
2018-07-11 13:46 ` Henning Schild@ 2018-07-11 14:27 ` Jeff King
2018-07-11 16:15 ` Henning Schild0 siblings, 1 reply; 57+ messages in thread
From: Jeff King @ 2018-07-11 14:27 UTC (permalink / raw)
To: Henning Schild
Cc: git, Eric Sunshine, Junio C Hamano, Martin Ågren, Ben Toews,
Taylor Blau, brian m . carlson
On Wed, Jul 11, 2018 at 03:46:19PM +0200, Henning Schild wrote:
> > I think it's worth addressing in the near term, if only because this
> > kind of off-by-one is quite subtle, and I don't want to forget to deal
> > with it. Whether that happens as part of this patch, or as a cleanup
> > before or after, I'm not picky. :)
>
> I get that and if anyone is willing to write that code, i will base my
> patches on it. What i want to avoid is taking responsibility for
> problems i did not introduce, just because i happen to work on that
> code at the moment. Keeping track of that (not forgetting) is also not
> for the random contributor like myself.
It doesn't make sense to do a patch before your series, since it would
just be:
if (strlen(found) > 16)
...
which would get obliterated by your patch. The patch after is shown
below. But frankly, it seems a lot easier to just handle this while you
are rewriting the code.
-- >8 --
Subject: [PATCH] gpg-interface: handle off-by-one parsing gpg output
When parsing gpg's VALIDSIG lines, we look for a space
followed by the signer information. Because we use
strchrnul(), though, if the space is missing we'll end up
pointing to the trailing NUL. When we try to move past that
space, we have to handle the NUL case separately to avoid
accidentally stepping out of the string entirely.
Signed-off-by: Jeff King <peff@peff.net>
---
gpg-interface.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index bf8d567a4c..139b0f561e 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -97,7+97,7 @@ static void parse_gpg_output(struct signature_check *sigc)
sigc->key = xmemdupz(found, next - found);
/* The ERRSIG message is not followed by signer information */
if (sigc-> result != 'E') {
- found = next + 1;
+ found = *next ? next + 1 : next;
next = strchrnul(found, '\n');
sigc->signer = xmemdupz(found, next - found);
}
--
2.18.0.400.g702e398724
^permalinkrawreply [flat|nested] 57+ messages in thread

*Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM
2018-07-11 13:40 ` Henning Schild@ 2018-07-11 14:35 ` Jeff King
2018-07-11 15:48 ` Henning Schild
2018-07-11 16:26 ` Junio C Hamano0 siblings, 2 replies; 57+ messages in thread
From: Jeff King @ 2018-07-11 14:35 UTC (permalink / raw)
To: Henning Schild
Cc: git, Eric Sunshine, Junio C Hamano, Martin Ågren, Ben Toews,
Taylor Blau, brian m . carlson
On Wed, Jul 11, 2018 at 03:40:19PM +0200, Henning Schild wrote:
> > So it may be simplest to just run most of the tests twice, once with
> > gpg and once with gpgsm. I kind of wonder if all of t7510 could just
> > be bumped into a function. Or even into a sourced file and run from
> > two different scripts. See the way that t8001 and t8002 use
> > annotate-tests.sh for an example.
>
> I do not agree and would like to leave the tests as they are. Instead
> of introducing a whole lot of very similar copies, i added just a few.
I'm not sure I understand why you added the ones you did, though. For
instance, "--no-show-signature overrides --show-signature x509" seems
like it has nothing to do with the gpg/gpgsm distinction.
So I'd have expected that to be _outside_ of the shared battery of
tests.
> The original ones are even very similar between each other.
> We are again talking about two problems. 1. we need test cases for
> gpgsm if we want to merge gpgsm 2. the testsuite is very repetitive
>
> While addressing 1 make 2 obvious and worse, addressing 2 is a whole
> different story and should probably be discussed outside of this
> thread. And i would not like to inherit responsibility for 2. In
> fact the whole discussion emphasizes that it was a good idea to make
> GPGSM depend on GPG, because it allows to somewhat reuse existing tests.
IMHO there is a big difference between inheriting responsibility for
something, and not making it worse. But I'm not all that interested in
fighting about it.
-Peff
^permalinkrawreply [flat|nested] 57+ messages in thread

*Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM
2018-07-11 14:35 ` Jeff King@ 2018-07-11 15:48 ` Henning Schild
2018-07-11 16:26 ` Junio C Hamano1 sibling, 0 replies; 57+ messages in thread
From: Henning Schild @ 2018-07-11 15:48 UTC (permalink / raw)
To: Jeff King
Cc: git, Eric Sunshine, Junio C Hamano, Martin Ågren, Ben Toews,
Taylor Blau, brian m . carlson
Am Wed, 11 Jul 2018 10:35:54 -0400
schrieb Jeff King <peff@peff.net>:
> On Wed, Jul 11, 2018 at 03:40:19PM +0200, Henning Schild wrote:
>
> > > So it may be simplest to just run most of the tests twice, once
> > > with gpg and once with gpgsm. I kind of wonder if all of t7510
> > > could just be bumped into a function. Or even into a sourced file
> > > and run from two different scripts. See the way that t8001 and
> > > t8002 use annotate-tests.sh for an example.
> >
> > I do not agree and would like to leave the tests as they are.
> > Instead of introducing a whole lot of very similar copies, i added
> > just a few.
>
> I'm not sure I understand why you added the ones you did, though. For
> instance, "--no-show-signature overrides --show-signature x509" seems
> like it has nothing to do with the gpg/gpgsm distinction.
>
> So I'd have expected that to be _outside_ of the shared battery of
> tests.
True, it took my quite some time to figure out a way to configure gpgsm
non-interactively. Generate the key etc. without even a single popup of
the gpg-agent... After that i just added random tests to create
"coverage", without much focus. I would be happy to revisit that and
drop test cases, and add some that are missing.
Henning
> > The original ones are even very similar between each other.
> > We are again talking about two problems. 1. we need test cases for
> > gpgsm if we want to merge gpgsm 2. the testsuite is very repetitive
> >
> > While addressing 1 make 2 obvious and worse, addressing 2 is a whole
> > different story and should probably be discussed outside of this
> > thread. And i would not like to inherit responsibility for 2. In
> > fact the whole discussion emphasizes that it was a good idea to make
> > GPGSM depend on GPG, because it allows to somewhat reuse existing
> > tests.
>
> IMHO there is a big difference between inheriting responsibility for
> something, and not making it worse. But I'm not all that interested in
> fighting about it.
>
> -Peff
^permalinkrawreply [flat|nested] 57+ messages in thread

*Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore
2018-07-11 14:27 ` Jeff King@ 2018-07-11 16:15 ` Henning Schild
2018-07-11 16:38 ` Jeff King0 siblings, 1 reply; 57+ messages in thread
From: Henning Schild @ 2018-07-11 16:15 UTC (permalink / raw)
To: Jeff King
Cc: git, Eric Sunshine, Junio C Hamano, Martin Ågren, Ben Toews,
Taylor Blau, brian m . carlson
Am Wed, 11 Jul 2018 10:27:52 -0400
schrieb Jeff King <peff@peff.net>:
> On Wed, Jul 11, 2018 at 03:46:19PM +0200, Henning Schild wrote:
>
> > > I think it's worth addressing in the near term, if only because
> > > this kind of off-by-one is quite subtle, and I don't want to
> > > forget to deal with it. Whether that happens as part of this
> > > patch, or as a cleanup before or after, I'm not picky. :)
> >
> > I get that and if anyone is willing to write that code, i will base
> > my patches on it. What i want to avoid is taking responsibility for
> > problems i did not introduce, just because i happen to work on that
> > code at the moment. Keeping track of that (not forgetting) is also
> > not for the random contributor like myself.
>
> It doesn't make sense to do a patch before your series, since it would
> just be:
>
> if (strlen(found) > 16)
> ...
Instead of randomly crashing on unexpected input, we would now silently
ignore it.
> which would get obliterated by your patch. The patch after is shown
> below. But frankly, it seems a lot easier to just handle this while
> you are rewriting the code.
>
> -- >8 --
> Subject: [PATCH] gpg-interface: handle off-by-one parsing gpg output
>
> When parsing gpg's VALIDSIG lines, we look for a space
> followed by the signer information. Because we use
> strchrnul(), though, if the space is missing we'll end up
> pointing to the trailing NUL. When we try to move past that
> space, we have to handle the NUL case separately to avoid
> accidentally stepping out of the string entirely.
True.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> gpg-interface.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index bf8d567a4c..139b0f561e 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -97,7 +97,7 @@ static void parse_gpg_output(struct signature_check
> *sigc) sigc->key = xmemdupz(found, next - found);
> /* The ERRSIG message is not followed by
> signer information */ if (sigc-> result != 'E') {
> - found = next + 1;
> + found = *next ? next + 1 : next;
This would keep us in bounds of the unexpected string. But ignore the
line instead of "complaining" or crashing.
But you are right, it is easy enough and ignoring the line is probably
the best way of dealing with it.
i will change the condition to
> if (*next && sigc-> result != 'E')
also skipping the following strchrnul and xmemdupz
Henning
> next = strchrnul(found, '\n');
> sigc->signer = xmemdupz(found, next
> - found); }
^permalinkrawreply [flat|nested] 57+ messages in thread

*Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM
2018-07-11 14:35 ` Jeff King
2018-07-11 15:48 ` Henning Schild@ 2018-07-11 16:26 ` Junio C Hamano1 sibling, 0 replies; 57+ messages in thread
From: Junio C Hamano @ 2018-07-11 16:26 UTC (permalink / raw)
To: Jeff King
Cc: Henning Schild, git, Eric Sunshine, Martin Ågren, Ben Toews,
Taylor Blau, brian m . carlson
Jeff King <peff@peff.net> writes:
>> While addressing 1 make 2 obvious and worse, addressing 2 is a whole
>> different story and should probably be discussed outside of this
>> thread. And i would not like to inherit responsibility for 2. In
>> fact the whole discussion emphasizes that it was a good idea to make
>> GPGSM depend on GPG, because it allows to somewhat reuse existing tests.
>
> IMHO there is a big difference between inheriting responsibility for
> something, and not making it worse.
Well said.
^permalinkrawreply [flat|nested] 57+ messages in thread

*Re: [PATCH v2 0/9] X509 (gpgsm) commit signing support
2018-07-10 17:12 ` [PATCH v2 0/9] X509 (gpgsm) commit signing support Jeff King
@ 2018-07-14 18:33 ` brian m. carlson
2018-07-16 21:32 ` Jeff King0 siblings, 1 reply; 57+ messages in thread
From: brian m. carlson @ 2018-07-14 18:33 UTC (permalink / raw)
To: Jeff King
Cc: Henning Schild, git, Eric Sunshine, Junio C Hamano,
Martin Ågren, Ben Toews, Taylor Blau
[-- Attachment #1: Type: text/plain, Size: 1619 bytes --]
On Tue, Jul 10, 2018 at 01:12:24PM -0400, Jeff King wrote:
> On Tue, Jul 10, 2018 at 10:52:22AM +0200, Henning Schild wrote:
> > This series can be seen as a follow up of a series that appeared under
> > the name "gpg-interface: Multiple signing tools" in april 2018 [1]. After
> > that series was not merged i decided to get my patches ready. The
> > original series aimed at being generic for any sort of signing tool, while
> > this series just introduced the X509 variant of gpg. (gpgsm)
> > I collected authors and reviewers of that first series and already put them
> > on cc.
>
> This series is a fine replacement for that earlier work. It's flexible
> enough to allow what we really wanted out of that series (gpgsm support,
> or another drop-in tool that uses the same interface). It doesn't lay
> any groundwork for further tools (like signify), but I think the
> consensus on the list was to punt on that until somebody had more
> concrete plans for adding such a tool.
I actually think this moves in a nice direction for adding support for
minisign/signify and other schemes. There's a way to look up what
algorithm is in use in a particular context based on the first line and
a general interface for deciding what format to write. Granted, it
currently still is very specific to gpg-style tools, but I think this is
an improvement in that regard.
As an OpenPGP user, I have no interest in adding support for other
tools, but I think this should make it easier if someone else wants to
do that.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]^permalinkrawreply [flat|nested] 57+ messages in thread

*Re: [PATCH v2 0/9] X509 (gpgsm) commit signing support
2018-07-14 18:33 ` brian m. carlson@ 2018-07-16 21:32 ` Jeff King0 siblings, 0 replies; 57+ messages in thread
From: Jeff King @ 2018-07-16 21:32 UTC (permalink / raw)
To: brian m. carlson, Henning Schild, git, Eric Sunshine,
Junio C Hamano, Martin Ågren, Ben Toews, Taylor Blau
On Sat, Jul 14, 2018 at 06:33:12PM +0000, brian m. carlson wrote:
> > This series is a fine replacement for that earlier work. It's flexible
> > enough to allow what we really wanted out of that series (gpgsm support,
> > or another drop-in tool that uses the same interface). It doesn't lay
> > any groundwork for further tools (like signify), but I think the
> > consensus on the list was to punt on that until somebody had more
> > concrete plans for adding such a tool.
>
> I actually think this moves in a nice direction for adding support for
> minisign/signify and other schemes. There's a way to look up what
> algorithm is in use in a particular context based on the first line and
> a general interface for deciding what format to write. Granted, it
> currently still is very specific to gpg-style tools, but I think this is
> an improvement in that regard.
My issue with this for helping with signify is that it creates a new
gpg.<tool>.* hierarchy with two slots (openpgp and x509). But we would
not want gpg.signify.program, would we? That makes no sense, as neither
the signature-matching nor the program invocation are gpg-like.
But if we later moved to "signingtool.<tool>.*", now we have an extra
layer of compatibility to deal with. E.g., signingtool.openpgp.program
is the same as gpg.openpgp.program which is the same as gpg.program.
I think we can do that, but it means more historical baggage.
I'm OK with that since signify support is purely hypothetical at this
point. But that's why I say that this doesn't lay the groundwork in the
way that the other series did.
> As an OpenPGP user, I have no interest in adding support for other
> tools, but I think this should make it easier if someone else wants to
> do that.
I don't plan to work on signify (or other tools) anytime soon either. My
interest here is in x509, since that's what enterprises would use over
pgp.
I actually dislike pgp for this application, too, because I find the key
management kind of complicated and tedious. But at least it's a standard
among open source folks.
-Peff
^permalinkrawreply [flat|nested] 57+ messages in thread

*Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program
2018-07-16 21:35 ` Jeff King@ 2018-07-16 21:56 ` Junio C Hamano
2018-07-16 22:23 ` Jeff King0 siblings, 1 reply; 57+ messages in thread
From: Junio C Hamano @ 2018-07-16 21:56 UTC (permalink / raw)
To: Jeff King
Cc: brian m. carlson, Henning Schild, git, Eric Sunshine,
Martin Ågren, Ben Toews, Taylor Blau
Jeff King <peff@peff.net> writes:
> On Sat, Jul 14, 2018 at 06:13:47PM +0000, brian m. carlson wrote:
>
>> On Tue, Jul 10, 2018 at 12:56:38PM -0400, Jeff King wrote:
>> > On Tue, Jul 10, 2018 at 12:54:13PM -0400, Jeff King wrote:
>> >
>> > > Should we allow:
>> > >
>> > > [gpg "OpenPGP"]
>> > > program = whatever
>> > >
>> > > given that we allow:
>> > >
>> > > [gpg]
>> > > format = OpenPGP
>> > >
>> > > ? I think just using strcasecmp() here would be sufficient. But I wonder
>> > > if it is a symptom of using the wrong tool (subsections) when we don't
>> > > need it.
>> >
>> > I did just read the discussion in response to v1, where everybody told
>> > you the opposite. ;)
>> >
>> > So I guess my question/points are more for brian and Junio.
>>
>> I'm okay with us forcing "openpgp". That seems sane enough for now, and
>> if people scream loudly, we can loosen it.
>
> Well, I specifically meant "are you sure subsections like this are a
> good idea". But it seems like people think so?
I admit that I did not even consider that there may be better tool
than using subsections to record this information. What are the
possibilities you have in mind (if you have one)?
>
> If so, then I think the best route is just dictating that yes,
> gpg.format is case-sensitive because it is referencing
> gpg.<format>.program, which is itself case-sensitive (and "openpgp" is
> the right spelling).
>
> -Peff
^permalinkrawreply [flat|nested] 57+ messages in thread

*Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program
2018-07-16 21:56 ` Junio C Hamano@ 2018-07-16 22:23 ` Jeff King
2018-07-16 23:12 ` Junio C Hamano0 siblings, 1 reply; 57+ messages in thread
From: Jeff King @ 2018-07-16 22:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: brian m. carlson, Henning Schild, git, Eric Sunshine,
Martin Ågren, Ben Toews, Taylor Blau
On Mon, Jul 16, 2018 at 02:56:34PM -0700, Junio C Hamano wrote:
> >> I'm okay with us forcing "openpgp". That seems sane enough for now, and
> >> if people scream loudly, we can loosen it.
> >
> > Well, I specifically meant "are you sure subsections like this are a
> > good idea". But it seems like people think so?
>
> I admit that I did not even consider that there may be better tool
> than using subsections to record this information. What are the
> possibilities you have in mind (if you have one)?
I don't think there is another tool except two-level options, like
"gpg.openpgpprogram" and "gpg.x509program".
Although those are a bit ugly, I just wondered if they might make things
simpler, since AFAIK we are not planning to add more config options
here. Like gpg.x509.someotherflag, nor gpg.someothertool.program.
Of course one reason _for_ the tri-level is that we might one day add
gpg.x509.someotherflag, and this gives us room to do it with less
awkwardness (i.e., a proliferation of gpg.x509someflag options).
-Peff
^permalinkrawreply [flat|nested] 57+ messages in thread

*Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program
2018-07-16 22:23 ` Jeff King@ 2018-07-16 23:12 ` Junio C Hamano0 siblings, 0 replies; 57+ messages in thread
From: Junio C Hamano @ 2018-07-16 23:12 UTC (permalink / raw)
To: Jeff King
Cc: brian m. carlson, Henning Schild, git, Eric Sunshine,
Martin Ågren, Ben Toews, Taylor Blau
Jeff King <peff@peff.net> writes:
> On Mon, Jul 16, 2018 at 02:56:34PM -0700, Junio C Hamano wrote:
>
>> >> I'm okay with us forcing "openpgp". That seems sane enough for now, and
>> >> if people scream loudly, we can loosen it.
>> >
>> > Well, I specifically meant "are you sure subsections like this are a
>> > good idea". But it seems like people think so?
>>
>> I admit that I did not even consider that there may be better tool
>> than using subsections to record this information. What are the
>> possibilities you have in mind (if you have one)?
>
> I don't think there is another tool except two-level options, like
> "gpg.openpgpprogram" and "gpg.x509program".
>
> Although those are a bit ugly, I just wondered if they might make things
> simpler, since AFAIK we are not planning to add more config options
> here. Like gpg.x509.someotherflag, nor gpg.someothertool.program.
>
> Of course one reason _for_ the tri-level is that we might one day add
> gpg.x509.someotherflag, and this gives us room to do it with less
> awkwardness (i.e., a proliferation of gpg.x509someflag options).
Yes, and signingtool.<name>.<key> is probably a good (ultra-)long
term direction. Preparing the code may be quite a lot of work that
nobody may be interested in, and nothing other than the GPG family
might materialize for a long time, but if we can cheaply prepare
external interface less dependent on GPG/PGP, that by itself would
be a good thing to have, I would guess.
Thanks.
^permalinkrawreply [flat|nested] 57+ messages in thread