Re: [PATCH v2 8/9] gpg-interface: introduce new signature format "x509" using gpgsm
Am Tue, 10 Jul 2018 13:01:10 -0400
schrieb Jeff King :
> On Tue, Jul 10, 2018 at 10:52:30AM +0200, Henning Schild wrote:
>
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index c0bd80954..b6f9b47d5 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -1830,7 +1830,7 @@ gpg.program::
> >
> > gpg.format::
> > Specifies which key format to use when signing with
> > `--gpg-sign`.
> > - Default is "openpgp", that is also the only supported
> > value.
> > + Default is "opengpg" and another possible value is
> > "x509".
>
> opengpg?
Right, thanks!
> Since we're having so much fun with naming discussions, let's talk
> about "x509". :)
>
> That's the cert format. I think of these signatures as S/MIME, but
> really that's the mail-oriented parts of the standard. I think
> technically this is "CMS".
>
> That said, we should pick what most people will find natural when
> referring to it. So maybe x509 isn't the worst choice, as I doubt most
> people know the term CMS. Probably the term they know _most_ is
> "gpgsm", but I think the point is that one does not have to be using
> gpgsm in the first place.
Ok, but now that you mention it, i will include the string "gpgsm" into
Documentation/config.txt somewhere. Maybe other documentation bits
could use hints that gpg is not the only kid in town anymore.
> So I dunno. I think I talked myself back into x509. ;)
Ok, will stick to it.
Henning
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index 65098430f..bf8d567a4 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -16,13 +16,18 @@ struct gpg_format_data {
> >
> > #define PGP_SIGNATURE "-BEGIN PGP SIGNATURE-"
> > #define PGP_MESSAGE "-BEGIN PGP MESSAGE-"
> > +#define X509_SIGNATURE "-BEGIN SIGNED MESSAGE-"
> >
> > -enum gpgformats { PGP_FMT };
> > +enum gpgformats { PGP_FMT, X509_FMT };
> > struct gpg_format_data gpg_formats[] = {
> > { .format = "openpgp", .program = "gpg",
> > .extra_args_verify = { "--keyid-format=long" },
> > .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
> > },
> > + { .format = "x509", .program = "gpgsm",
> > + .extra_args_verify = { NULL },
> > + .sigs = { X509_SIGNATURE, NULL }
> > + },
>
> 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.
>
> -Peff
Re: [PATCH v2 8/9] gpg-interface: introduce new signature format "x509" using gpgsm
Jeff King writes:
>> @@ -16,13 +16,18 @@ struct gpg_format_data {
>>
>> #define PGP_SIGNATURE "-BEGIN PGP SIGNATURE-"
>> #define PGP_MESSAGE "-BEGIN PGP MESSAGE-"
>> +#define X509_SIGNATURE "-BEGIN SIGNED MESSAGE-"
>>
>> -enum gpgformats { PGP_FMT };
>> +enum gpgformats { PGP_FMT, X509_FMT };
>> struct gpg_format_data gpg_formats[] = {
>> { .format = "openpgp", .program = "gpg",
>>.extra_args_verify = { "--keyid-format=long" },
>>.sigs = { PGP_SIGNATURE, PGP_MESSAGE }
>> },
>> +{ .format = "x509", .program = "gpgsm",
>> + .extra_args_verify = { NULL },
>> + .sigs = { X509_SIGNATURE, NULL }
>> +},
>
> 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:
static const char *gpg_verify_args[] = {
"--verify",
"--status-fd=1",
"--keyid-format=long",
NULL
};
static const char *gpg_sigs[] = {
"-BEGIN PGP SIGNATURE-",
"-BEGIN PGP MESSAGE-",
NULL
};
struct gpg_format {
const char *name;
const char *program;
const char * const *verify_args;
const char * const *sigs;
} gpg_format[] = {
{
.name = "openpgp",
.program = "gpg',
.verify_args = gpg_verify_args,
.sigs = gpg_sigs,
},
{
...
},
};
so we may end up having the same number of levels of indirection
anyway in the long-term final form.
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.
Re: [PATCH v2 8/9] gpg-interface: introduce new signature format "x509" using gpgsm
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
Re: [PATCH v2 8/9] gpg-interface: introduce new signature format "x509" using gpgsm
On Tue, Jul 10, 2018 at 10:52:30AM +0200, Henning Schild wrote:
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c0bd80954..b6f9b47d5 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1830,7 +1830,7 @@ gpg.program::
>
> gpg.format::
> Specifies which key format to use when signing with `--gpg-sign`.
> - Default is "openpgp", that is also the only supported value.
> + Default is "opengpg" and another possible value is "x509".
opengpg?
Since we're having so much fun with naming discussions, let's talk about
"x509". :)
That's the cert format. I think of these signatures as S/MIME, but
really that's the mail-oriented parts of the standard. I think
technically this is "CMS".
That said, we should pick what most people will find natural when
referring to it. So maybe x509 isn't the worst choice, as I doubt most
people know the term CMS. Probably the term they know _most_ is "gpgsm",
but I think the point is that one does not have to be using gpgsm in the
first place.
So I dunno. I think I talked myself back into x509. ;)
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 65098430f..bf8d567a4 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -16,13 +16,18 @@ struct gpg_format_data {
>
> #define PGP_SIGNATURE "-BEGIN PGP SIGNATURE-"
> #define PGP_MESSAGE "-BEGIN PGP MESSAGE-"
> +#define X509_SIGNATURE "-BEGIN SIGNED MESSAGE-"
>
> -enum gpgformats { PGP_FMT };
> +enum gpgformats { PGP_FMT, X509_FMT };
> struct gpg_format_data gpg_formats[] = {
> { .format = "openpgp", .program = "gpg",
> .extra_args_verify = { "--keyid-format=long" },
> .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
> },
> + { .format = "x509", .program = "gpgsm",
> + .extra_args_verify = { NULL },
> + .sigs = { X509_SIGNATURE, NULL }
> + },
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.
-Peff
[PATCH v2 8/9] gpg-interface: introduce new signature format "x509" using gpgsm
This commit allows git to create and check x509 type signatures using
gpgsm.
Signed-off-by: Henning Schild
---
Documentation/config.txt | 2 +-
gpg-interface.c | 10 +-
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index c0bd80954..b6f9b47d5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1830,7 +1830,7 @@ gpg.program::
gpg.format::
Specifies which key format to use when signing with `--gpg-sign`.
- Default is "openpgp", that is also the only supported value.
+ Default is "opengpg" and another possible value is "x509".
gpg..program::
Use this to customize the program used for the signing format you
diff --git a/gpg-interface.c b/gpg-interface.c
index 65098430f..bf8d567a4 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -16,13 +16,18 @@ struct gpg_format_data {
#define PGP_SIGNATURE "-BEGIN PGP SIGNATURE-"
#define PGP_MESSAGE "-BEGIN PGP MESSAGE-"
+#define X509_SIGNATURE "-BEGIN SIGNED MESSAGE-"
-enum gpgformats { PGP_FMT };
+enum gpgformats { PGP_FMT, X509_FMT };
struct gpg_format_data gpg_formats[] = {
{ .format = "openpgp", .program = "gpg",
.extra_args_verify = { "--keyid-format=long" },
.sigs = { PGP_SIGNATURE, PGP_MESSAGE }
},
+ { .format = "x509", .program = "gpgsm",
+ .extra_args_verify = { NULL },
+ .sigs = { X509_SIGNATURE, NULL }
+ },
};
static const char *gpg_format = "openpgp";
@@ -182,6 +187,9 @@ int git_gpg_config(const char *var, const char *value, void
*cb)
if (!strcmp(var, "gpg.program") || !strcmp(var, "gpg.openpgp.program"))
return git_config_string(_formats[PGP_FMT].program, var,
value);
+ if (!strcmp(var, "gpg.x509.program"))
+ return git_config_string(_formats[X509_FMT].program, var,
+value);
return 0;
}
--
2.16.4
