git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 5/9] gpg-interface: introduce an abstraction for multiple gpg formats

From
Junio C Hamano <gitster@pobox.com>
Date
Jul 10, 2018, 17:16 UTC
Message-ID
<xmqqlgaj80zs.fsf@gitster-ct.c.googlers.com>
In-Reply-To
<93d74df91b9f5c68dfd8a03f576d7428491976c0.1531208187.git.henning.schild@siemens.com>
Henning Schild <henning.schild@siemens.com> writes:
Show 26 quoted lines
> Create a struct that holds the format details for the supported formats.
> At the moment that is still just "openpgp". This commit prepares for the
> introduction of more formats, that might use other programs and match
> other signatures.
>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  gpg-interface.c | 74 ++++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 58 insertions(+), 16 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index ed0e55917..0a8d1bff3 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -7,12 +7,46 @@
>  #include "tempfile.h"
>  
>  static char *configured_signing_key;
> -static const char *gpg_format = "openpgp";
> -static const char *gpg_program = "gpg";
> +struct gpg_format_data {
> +	const char *format;
> +	const char *program;
> +	const char *extra_args_verify[1];
> +	const char *sigs[2];
> +};

Do you use identifier "gpg_format" elsewhere as a structure name? A structure is there always to hold "data", so often "_data" suffix is meaningless in a structure type name like this.

Show 13 quoted lines
>  #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
>  #define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
>  
> +enum gpgformats { PGP_FMT };
> +struct gpg_format_data gpg_formats[] = {
> +	{ .format = "openpgp", .program = "gpg",
> +	  .extra_args_verify = { "--keyid-format=long" },
> +	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
> +	},
> +};
> +static const char *gpg_format = "openpgp";
> +
> +static struct gpg_format_data *get_format_data(const char *str)

get_format_data_by_name() or something like that, for consistency with the next function, perhaps?

Show 30 quoted lines
> +{
> +	int i;
> +	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> +		if (!strcasecmp(gpg_formats[i].format, str))
> +			return gpg_formats + i;
> +	return NULL;
> +}
> +
> +static struct gpg_format_data *get_format_data_by_sig(const char *sig)
> +{
> +	int i, j;
> +	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> +		for (j = 0; j < ARRAY_SIZE(gpg_formats[i].sigs); j++)
> +			if (gpg_formats[i].sigs[j] && 
> +			    !strncmp(gpg_formats[i].sigs[j], sig,
> +				     strlen(gpg_formats[i].sigs[j])))
> +				return gpg_formats + i;
> +	return NULL;
> +}
> +
>  void signature_check_clear(struct signature_check *sigc)
>  {
>  	FREE_AND_NULL(sigc->payload);
> @@ -104,8 +138,7 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
>  
>  static int is_gpg_start(const char *line)
>  {
> -	return starts_with(line, PGP_SIGNATURE) ||
> -		starts_with(line, PGP_MESSAGE);
> +	return (get_format_data_by_sig(line) != NULL);

If this is still called "is_gpg_start()", then shouldn't the implementation be more like this?

	struct gpg_format *found = get_format_data_by_sig(line);
	return found && found == &gpg_formats[PGP_FMT];

It probably does not make much difference in the end, as you won't have functions is_(gpg|gpgsm|somethingelse|yetanother)_start() but either have a single function "does some x509 looking block start here?" or "I am expecting gpg and nothing else, does a block for that start here?" and at that point this will no longer be called is_gpg_start(), I would expect.

Show 9 quoted lines
>  size_t parse_signature(const char *buf, size_t size)
> @@ -140,18 +173,14 @@ int git_gpg_config(const char *var, const char *value, void *cb)
>  	}
>  
>  	if (!strcmp(var, "gpg.format")) {
> -		if (strcasecmp(value, "openpgp"))
> +		if (!get_format_data(value))
>  			return error("malformed value for %s: %s", var, value);
>  		return git_config_string(&gpg_format, var, value);

This is a bug introduced in an earlier step in the series, but you will segfault when given a configuration that looks like this:

	[gpg]
		format

Imitate the way how !value is diagnosed upfront for "gpg.program" below before this patch.

Show 12 quoted lines
>  	}
>  
> -	if (!strcmp(var, "gpg.program")) {
> -		if (!value)
> -			return config_error_nonbool(var);
> -		gpg_program = xstrdup(value);
> -		return 0;
> -	}
> -
> +	if (!strcmp(var, "gpg.program"))
> +		return git_config_string(&gpg_formats[PGP_FMT].program, var,
> +					 value);

git_config_string() has its own "do not feed boolean 'true' to me" check, so this change does not introduce a regression.

Show 42 quoted lines
> @@ -165,12 +194,16 @@ const char *get_signing_key(void)
>  int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
>  {
>  	struct child_process gpg = CHILD_PROCESS_INIT;
> +	struct gpg_format_data *fmt;
>  	int ret;
>  	size_t i, j, bottom;
>  	struct strbuf gpg_status = STRBUF_INIT;
>  
> +	fmt = get_format_data(gpg_format);
> +	if (!fmt)
> +		BUG("bad gpg_format '%s'", gpg_format);
>  	argv_array_pushl(&gpg.args,
> -			 gpg_program,
> +			 fmt->program,
>  			 "--status-fd=2",
>  			 "-bsau", signing_key,
>  			 NULL);
> @@ -208,8 +241,9 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
>  			 struct strbuf *gpg_output, struct strbuf *gpg_status)
>  {
>  	struct child_process gpg = CHILD_PROCESS_INIT;
> +	struct gpg_format_data *fmt;
>  	struct tempfile *temp;
> -	int ret;
> +	int ret, i;
>  	struct strbuf buf = STRBUF_INIT;
>  
>  	temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
> @@ -223,10 +257,18 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
>  		return -1;
>  	}
>  
> +	fmt = get_format_data_by_sig(signature);
> +	assert(fmt);
> +
> +	argv_array_pushl(&gpg.args,
> +			 fmt->program, NULL);
> +	for (i = 0; i < ARRAY_SIZE(fmt->extra_args_verify); i++)
> +		if (fmt->extra_args_verify[i])
> +			argv_array_pushl(&gpg.args,
> +					 fmt->extra_args_verify[i], NULL);

This loop allows us to extend .extra_args_verify when we need to support another variant that needs to take two arguments by making .extra_args field larger for all variants and stuffing a NULL to unused slots, and it even allows a nonsense like this

	.extra_args_verify = { NULL, "--keyid-format=long" },

I cannot quite shake the feeling that .extra_args_verify and .sigs fields should not be of fixed sized array of strings, but rather be of type "const char **" that is NULL terminated. Such a clean-up will make it harder to write initializers, so it may not be worth it, I guess.

Show 6 quoted lines
>  	argv_array_pushl(&gpg.args,
> -			 gpg_program,
>  			 "--status-fd=1",
> -			 "--keyid-format=long",
>  			 "--verify", temp->filename.buf, "-",
>  			 NULL);
Previous: Henning SchildNext: Henning Schild
Message 29 of 57 in “X509 (gpgsm) commit signing support”
  1. 0/9 X509 (gpgsm) commit signing supportHenning Schild, Jul 10, 2018
  2. 7/9 gpg-interface: introduce new config to select per gpg format programHenning Schild, Jul 10, 2018
  3. Jeff KingJul 10, 2018
  4. Jeff KingJul 10, 2018
  5. brian m. carlsonJul 14, 2018
  6. Jeff KingJul 16, 2018
  7. Junio C HamanoJul 16, 2018
  8. Jeff KingJul 16, 2018
  9. Junio C HamanoJul 16, 2018
  10. Junio C HamanoJul 10, 2018
  11. Henning SchildJul 13, 2018
  12. 8/9 gpg-interface: introduce new signature format "x509" using gpgsmHenning Schild, Jul 10, 2018
  13. Jeff KingJul 10, 2018
  14. Junio C HamanoJul 10, 2018
  15. Jeff KingJul 10, 2018
  16. Henning SchildJul 11, 2018
  17. 1/9 builtin/receive-pack: use check_signature from gpg-interfaceHenning Schild, Jul 10, 2018
  18. 4/9 t/t7510: check the validation of the new config gpg.formatHenning Schild, Jul 10, 2018
  19. Jeff KingJul 10, 2018
  20. Henning SchildJul 11, 2018
  21. Junio C HamanoJul 10, 2018
  22. Henning SchildJul 11, 2018
  23. 2/9 gpg-interface: make parse_gpg_output static and remove from interface headerHenning Schild, Jul 10, 2018
  24. Junio C HamanoJul 10, 2018
  25. Henning SchildJul 11, 2018
  26. 5/9 gpg-interface: introduce an abstraction for multiple gpg formatsHenning Schild, Jul 10, 2018
  27. Jeff KingJul 10, 2018
  28. Henning SchildJul 13, 2018
  29. Junio C HamanoJul 10, 2018
  30. Henning SchildJul 13, 2018
  31. 6/9 gpg-interface: do not hardcode the key string len anymoreHenning Schild, Jul 10, 2018
  32. Jeff KingJul 10, 2018
  33. Henning SchildJul 11, 2018
  34. Jeff KingJul 11, 2018
  35. Henning SchildJul 11, 2018
  36. Jeff KingJul 11, 2018
  37. Henning SchildJul 11, 2018
  38. Jeff KingJul 11, 2018
  39. 9/9 gpg-interface t: extend the existing GPG tests with GPGSMHenning Schild, Jul 10, 2018
  40. Jeff KingJul 10, 2018
  41. Jeff KingJul 10, 2018
  42. Henning SchildJul 11, 2018
  43. Jeff KingJul 11, 2018
  44. Henning SchildJul 11, 2018
  45. Jeff KingJul 11, 2018
  46. Henning SchildJul 11, 2018
  47. Junio C HamanoJul 11, 2018
  48. brian m. carlsonJul 14, 2018
  49. Junio C HamanoJul 10, 2018
  50. Henning SchildJul 11, 2018
  51. Jeff KingJul 11, 2018
  52. Henning SchildJul 11, 2018
  53. 3/9 gpg-interface: add new config to select how to sign a commitHenning Schild, Jul 10, 2018
  54. Jeff KingJul 10, 2018
  55. Jeff KingJul 10, 2018
  56. brian m. carlsonJul 14, 2018
  57. Jeff KingJul 16, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.