{"thread":{"id":"48850","subject":"[PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program","startedAt":"2018-07-10T08:53:08Z","lastAt":"2018-07-16T23:12:58Z","messageCount":57,"participants":["Henning Schild","Jeff King","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":2,"patchTotal":9},"messages":[{"id":"352067","messageId":"4905c1907a866c0fd1a4dac978dd6ca3e468ac43.1531208187.git.henning.schild@siemens.com","threadId":"48850","inReplyTo":"cover.1531208187.git.henning.schild@siemens.com","subject":"[PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-10T08:52:29Z","receivedAt":"2018-07-10T08:53:08Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Supporting multiple signing formats we will have the need to configure a\ncustom program each. Add a new config value to cater for that.\n\nSigned-off-by: Henning Schild \n---\n Documentation/config.txt | 5 +++++\n gpg-interface.c | 2 +-\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex ac373e3f4..c0bd80954 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1832,6 +1832,11 @@ gpg.format::\n \tSpecifies which key format to use when signing with `--gpg-sign`.\n \tDefault is \"openpgp\", that is also the only supported value.\n \n+gpg..program::\n+\tUse this to customize the program used for the signing format you\n+\tchose. (see gpg.program) gpg.openpgp.program is a synonym for the\n+\tlegacy gpg.program.\n+\n gui.commitMsgWidth::\n \tDefines how wide the commit message window is in the\n \tlinkgit:git-gui[1]. \"75\" is the default.\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex ac2df498d..65098430f 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -179,7 +179,7 @@ int git_gpg_config(const char *var, const char *value, void *cb)\n \t\treturn git_config_string(&gpg_format, var, value);\n \t}\n \n-\tif (!strcmp(var, \"gpg.program\"))\n+\tif (!strcmp(var, \"gpg.program\") || !strcmp(var, \"gpg.openpgp.program\"))\n \t\treturn git_config_string(&gpg_formats[PGP_FMT].program, var,\n \t\t\t\t\t value);\n \treturn 0;\n-- \n2.16.4\n\n"},{"id":"352068","messageId":"4a2cf83a63d25776cb1996490240ce3e5df8ada4.1531208187.git.henning.schild@siemens.com","threadId":"48850","inReplyTo":"cover.1531208187.git.henning.schild@siemens.com","subject":"[PATCH v2 8/9] gpg-interface: introduce new signature format \"x509\" using gpgsm","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-10T08:52:30Z","receivedAt":"2018-07-10T08:53:10Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"This commit allows git to create and check x509 type signatures using\ngpgsm.\n\nSigned-off-by: Henning Schild \n---\n Documentation/config.txt | 2 +-\n gpg-interface.c | 10 +++++++++-\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex c0bd80954..b6f9b47d5 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1830,7 +1830,7 @@ gpg.program::\n \n gpg.format::\n \tSpecifies which key format to use when signing with `--gpg-sign`.\n-\tDefault is \"openpgp\", that is also the only supported value.\n+\tDefault is \"opengpg\" and another possible value is \"x509\".\n \n gpg..program::\n \tUse this to customize the program used for the signing format you\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 65098430f..bf8d567a4 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -16,13 +16,18 @@ struct gpg_format_data {\n \n #define PGP_SIGNATURE \"-----BEGIN PGP SIGNATURE-----\"\n #define PGP_MESSAGE \"-----BEGIN PGP MESSAGE-----\"\n+#define X509_SIGNATURE \"-----BEGIN SIGNED MESSAGE-----\"\n \n-enum gpgformats { PGP_FMT };\n+enum gpgformats { PGP_FMT, X509_FMT };\n struct gpg_format_data gpg_formats[] = {\n \t{ .format = \"openpgp\", .program = \"gpg\",\n \t .extra_args_verify = { \"--keyid-format=long\" },\n \t .sigs = { PGP_SIGNATURE, PGP_MESSAGE }\n \t},\n+\t{ .format = \"x509\", .program = \"gpgsm\",\n+\t .extra_args_verify = { NULL },\n+\t .sigs = { X509_SIGNATURE, NULL }\n+\t},\n };\n static const char *gpg_format = \"openpgp\";\n \n@@ -182,6 +187,9 @@ int git_gpg_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"gpg.program\") || !strcmp(var, \"gpg.openpgp.program\"))\n \t\treturn git_config_string(&gpg_formats[PGP_FMT].program, var,\n \t\t\t\t\t value);\n+\tif (!strcmp(var, \"gpg.x509.program\"))\n+\t\treturn git_config_string(&gpg_formats[X509_FMT].program, var,\n+\t\t\t\t\t value);\n \treturn 0;\n }\n \n-- \n2.16.4\n\n"},{"id":"352069","messageId":"cover.1531208187.git.henning.schild@siemens.com","threadId":"48850","inReplyTo":null,"subject":"[PATCH v2 0/9] X509 (gpgsm) commit signing support","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-10T08:52:22Z","receivedAt":"2018-07-10T08:53:12Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Changes in v2:\n - removed trailing commas in array initializers and add leading space\n - replaced assert(0) with BUG in p5\n - consolidated 2 format lookups reusing get_format_data p5\n - changed from format \"PGP\" to \"openpgp\", later X509 to \"x509\"\n - use strcasecmp instead of strcmp for format matching\n - introduce gpg..program in p8, no gpg.programX509 anymore\n - moved t/7510 patch inbetween the two patches changing validation code\n - changed t/7510 patch to use test_config and test_must_fail\n\nThis series adds support for signing commits with gpgsm.\n\nThe first two patches are cleanups of gpg-interface, they are already\nclose to being merged. But since they have not been pulled to next i am\nresending them.\nThe following 5 patches (p3-p7) prepare for the introduction of the\nactual feature in patch 8.\nFinally patch 9 extends the testsuite to cover the new feature.\n\nThis series can be seen as a follow up of a series that appeared under\nthe name \"gpg-interface: Multiple signing tools\" in april 2018 [1]. After\nthat series was not merged i decided to get my patches ready. The\noriginal series aimed at being generic for any sort of signing tool, while\nthis series just introduced the X509 variant of gpg. (gpgsm)\nI collected authors and reviewers of that first series and already put them\non cc.\n\n[1] https://public-inbox.org/git/20180409204129.43537-1-mastahyeti@gmail.com/\n\nHenning Schild (9):\n builtin/receive-pack: use check_signature from gpg-interface\n gpg-interface: make parse_gpg_output static and remove from interface\n header\n gpg-interface: add new config to select how to sign a commit\n t/t7510: check the validation of the new config gpg.format\n gpg-interface: introduce an abstraction for multiple gpg formats\n gpg-interface: do not hardcode the key string len anymore\n gpg-interface: introduce new config to select per gpg format program\n gpg-interface: introduce new signature format \"x509\" using gpgsm\n gpg-interface t: extend the existing GPG tests with GPGSM\n\n Documentation/config.txt | 9 +++++\n builtin/receive-pack.c | 17 ++-------\n gpg-interface.c | 88 ++++++++++++++++++++++++++++++++++++++--------\n gpg-interface.h | 2 --\n t/lib-gpg.sh | 9 ++++-\n t/lib-gpg/gpgsm-gen-key.in | 6 ++++\n t/t4202-log.sh | 66 ++++++++++++++++++++++++++++++++++\n t/t5534-push-signed.sh | 52 +++++++++++++++++++++++++++\n t/t7003-filter-branch.sh | 15 ++++++++\n t/t7030-verify-tag.sh | 47 +++++++++++++++++++++++--\n t/t7510-signed-commit.sh | 10 ++++++\n t/t7600-merge.sh | 31 ++++++++++++++++\n 12 files changed, 317 insertions(+), 35 deletions(-)\n create mode 100644 t/lib-gpg/gpgsm-gen-key.in\n\n-- \n2.16.4\n\n"},{"id":"352070","messageId":"f9e371c8dd2a17ddb5fd5989a7fdad1c0d1bb6e7.1531208187.git.henning.schild@siemens.com","threadId":"48850","inReplyTo":"cover.1531208187.git.henning.schild@siemens.com","subject":"[PATCH v2 1/9] builtin/receive-pack: use check_signature from gpg-interface","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-10T08:52:23Z","receivedAt":"2018-07-10T08:53:14Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"The combination of verify_signed_buffer followed by parse_gpg_output is\navailable as check_signature. Use that instead of implementing it again.\n\nSigned-off-by: Henning Schild \n---\n builtin/receive-pack.c | 17 ++---------------\n 1 file changed, 2 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 68d36e0a5..9f0583deb 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -629,8 +629,6 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n \t\treturn;\n \n \tif (!already_done) {\n-\t\tstruct strbuf gpg_output = STRBUF_INIT;\n-\t\tstruct strbuf gpg_status = STRBUF_INIT;\n \t\tint bogs /* beginning_of_gpg_sig */;\n \n \t\talready_done = 1;\n@@ -639,22 +637,11 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n \t\t\toidclr(&push_cert_oid);\n \n \t\tmemset(&sigcheck, '\\0', sizeof(sigcheck));\n-\t\tsigcheck.result = 'N';\n \n \t\tbogs = parse_signature(push_cert.buf, push_cert.len);\n-\t\tif (verify_signed_buffer(push_cert.buf, bogs,\n-\t\t\t\t\t push_cert.buf + bogs, push_cert.len - bogs,\n-\t\t\t\t\t &gpg_output, &gpg_status) < 0) {\n-\t\t\t; /* error running gpg */\n-\t\t} else {\n-\t\t\tsigcheck.payload = push_cert.buf;\n-\t\t\tsigcheck.gpg_output = gpg_output.buf;\n-\t\t\tsigcheck.gpg_status = gpg_status.buf;\n-\t\t\tparse_gpg_output(&sigcheck);\n-\t\t}\n+\t\tcheck_signature(push_cert.buf, bogs, push_cert.buf + bogs,\n+\t\t\t\tpush_cert.len - bogs, &sigcheck);\n \n-\t\tstrbuf_release(&gpg_output);\n-\t\tstrbuf_release(&gpg_status);\n \t\tnonce_status = check_nonce(push_cert.buf, bogs);\n \t}\n \tif (!is_null_oid(&push_cert_oid)) {\n-- \n2.16.4\n\n"},{"id":"352071","messageId":"b02154496033220897f6f773e1149a98b21ccba7.1531208187.git.henning.schild@siemens.com","threadId":"48850","inReplyTo":"cover.1531208187.git.henning.schild@siemens.com","subject":"[PATCH v2 4/9] t/t7510: check the validation of the new config gpg.format","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-10T08:52:26Z","receivedAt":"2018-07-10T08:53:17Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Test setting gpg.format to both invalid and valid values.\n\nSigned-off-by: Henning Schild \n---\n t/t7510-signed-commit.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\nindex 6e2015ed9..7e1e9caf4 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature behaves like --show-signature' '\n \tgrep \"gpg: Good signature\" actual\n '\n \n+test_expect_success GPG 'check config gpg.format values' '\n+\trm .git/config &&\n+\ttest_config gpg.format openpgp &&\n+\tgit commit -S --amend -m \"success\" &&\n+\ttest_config gpg.format OpEnPgP &&\n+\tgit commit -S --amend -m \"success\" &&\n+\ttest_config gpg.format malformed &&\n+\ttest_must_fail git commit -S --amend -m \"fail\" 2>result\n+'\n+\n test_done\n-- \n2.16.4\n\n"},{"id":"352072","messageId":"192cf9fc4e7a601d2639ec2d82c777d4c7b26e99.1531208187.git.henning.schild@siemens.com","threadId":"48850","inReplyTo":"cover.1531208187.git.henning.schild@siemens.com","subject":"[PATCH v2 2/9] gpg-interface: make parse_gpg_output static and remove from interface header","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-10T08:52:24Z","receivedAt":"2018-07-10T08:53:18Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"This commit turns parse_gpg_output into an internal function, the only\noutside user was migrated in an earlier commit.\n\nSigned-off-by: Henning Schild \n---\n gpg-interface.c | 2 +-\n gpg-interface.h | 2 --\n 2 files changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 0647bd634..09ddfbc26 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -35,7 +35,7 @@ static struct {\n \t{ 'R', \"\\n[GNUPG:] REVKEYSIG \"},\n };\n \n-void parse_gpg_output(struct signature_check *sigc)\n+static void parse_gpg_output(struct signature_check *sigc)\n {\n \tconst char *buf = sigc->gpg_status;\n \tint i;\ndiff --git a/gpg-interface.h b/gpg-interface.h\nindex a5e6517ae..5ecff4aa0 100644\n--- a/gpg-interface.h\n+++ b/gpg-interface.h\n@@ -33,8 +33,6 @@ void signature_check_clear(struct signature_check *sigc);\n */\n size_t parse_signature(const char *buf, size_t size);\n \n-void parse_gpg_output(struct signature_check *);\n-\n /*\n * Create a detached signature for the contents of \"buffer\" and append\n * it after \"signature\"; \"buffer\" and \"signature\" can be the same\n-- \n2.16.4\n\n"},{"id":"352073","messageId":"93d74df91b9f5c68dfd8a03f576d7428491976c0.1531208187.git.henning.schild@siemens.com","threadId":"48850","inReplyTo":"cover.1531208187.git.henning.schild@siemens.com","subject":"[PATCH v2 5/9] gpg-interface: introduce an abstraction for multiple gpg formats","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-10T08:52:27Z","receivedAt":"2018-07-10T08:53:19Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Create a struct that holds the format details for the supported formats.\nAt the moment that is still just \"openpgp\". This commit prepares for the\nintroduction of more formats, that might use other programs and match\nother signatures.\n\nSigned-off-by: Henning Schild \n---\n gpg-interface.c | 74 ++++++++++++++++++++++++++++++++++++++++++++-------------\n 1 file changed, 58 insertions(+), 16 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex ed0e55917..0a8d1bff3 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -7,12 +7,46 @@\n #include \"tempfile.h\"\n \n static char *configured_signing_key;\n-static const char *gpg_format = \"openpgp\";\n-static const char *gpg_program = \"gpg\";\n+struct gpg_format_data {\n+\tconst char *format;\n+\tconst char *program;\n+\tconst char *extra_args_verify[1];\n+\tconst char *sigs[2];\n+};\n \n #define PGP_SIGNATURE \"-----BEGIN PGP SIGNATURE-----\"\n #define PGP_MESSAGE \"-----BEGIN PGP MESSAGE-----\"\n \n+enum gpgformats { PGP_FMT };\n+struct gpg_format_data gpg_formats[] = {\n+\t{ .format = \"openpgp\", .program = \"gpg\",\n+\t .extra_args_verify = { \"--keyid-format=long\" },\n+\t .sigs = { PGP_SIGNATURE, PGP_MESSAGE }\n+\t},\n+};\n+static const char *gpg_format = \"openpgp\";\n+\n+static struct gpg_format_data *get_format_data(const char *str)\n+{\n+\tint i;\n+\tfor (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n+\t\tif (!strcasecmp(gpg_formats[i].format, str))\n+\t\t\treturn gpg_formats + i;\n+\treturn NULL;\n+}\n+\n+static struct gpg_format_data *get_format_data_by_sig(const char *sig)\n+{\n+\tint i, j;\n+\tfor (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n+\t\tfor (j = 0; j < ARRAY_SIZE(gpg_formats[i].sigs); j++)\n+\t\t\tif (gpg_formats[i].sigs[j] && \n+\t\t\t !strncmp(gpg_formats[i].sigs[j], sig,\n+\t\t\t\t strlen(gpg_formats[i].sigs[j])))\n+\t\t\t\treturn gpg_formats + i;\n+\treturn NULL;\n+}\n+\n void signature_check_clear(struct signature_check *sigc)\n {\n \tFREE_AND_NULL(sigc->payload);\n@@ -104,8 +138,7 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)\n \n static int is_gpg_start(const char *line)\n {\n-\treturn starts_with(line, PGP_SIGNATURE) ||\n-\t\tstarts_with(line, PGP_MESSAGE);\n+\treturn (get_format_data_by_sig(line) != NULL);\n }\n \n size_t parse_signature(const char *buf, size_t size)\n@@ -140,18 +173,14 @@ int git_gpg_config(const char *var, const char *value, void *cb)\n \t}\n \n \tif (!strcmp(var, \"gpg.format\")) {\n-\t\tif (strcasecmp(value, \"openpgp\"))\n+\t\tif (!get_format_data(value))\n \t\t\treturn error(\"malformed value for %s: %s\", var, value);\n \t\treturn git_config_string(&gpg_format, var, value);\n \t}\n \n-\tif (!strcmp(var, \"gpg.program\")) {\n-\t\tif (!value)\n-\t\t\treturn config_error_nonbool(var);\n-\t\tgpg_program = xstrdup(value);\n-\t\treturn 0;\n-\t}\n-\n+\tif (!strcmp(var, \"gpg.program\"))\n+\t\treturn git_config_string(&gpg_formats[PGP_FMT].program, var,\n+\t\t\t\t\t value);\n \treturn 0;\n }\n \n@@ -165,12 +194,16 @@ const char *get_signing_key(void)\n int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)\n {\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n+\tstruct gpg_format_data *fmt;\n \tint ret;\n \tsize_t i, j, bottom;\n \tstruct strbuf gpg_status = STRBUF_INIT;\n \n+\tfmt = get_format_data(gpg_format);\n+\tif (!fmt)\n+\t\tBUG(\"bad gpg_format '%s'\", gpg_format);\n \targv_array_pushl(&gpg.args,\n-\t\t\t gpg_program,\n+\t\t\t fmt->program,\n \t\t\t \"--status-fd=2\",\n \t\t\t \"-bsau\", signing_key,\n \t\t\t NULL);\n@@ -208,8 +241,9 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\t\t struct strbuf *gpg_output, struct strbuf *gpg_status)\n {\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n+\tstruct gpg_format_data *fmt;\n \tstruct tempfile *temp;\n-\tint ret;\n+\tint ret, i;\n \tstruct strbuf buf = STRBUF_INIT;\n \n \ttemp = mks_tempfile_t(\".git_vtag_tmpXXXXXX\");\n@@ -223,10 +257,18 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\treturn -1;\n \t}\n \n+\tfmt = get_format_data_by_sig(signature);\n+\tassert(fmt);\n+\n+\targv_array_pushl(&gpg.args,\n+\t\t\t fmt->program, NULL);\n+\tfor (i = 0; i < ARRAY_SIZE(fmt->extra_args_verify); i++)\n+\t\tif (fmt->extra_args_verify[i])\n+\t\t\targv_array_pushl(&gpg.args,\n+\t\t\t\t\t fmt->extra_args_verify[i], NULL);\n+\n \targv_array_pushl(&gpg.args,\n-\t\t\t gpg_program,\n \t\t\t \"--status-fd=1\",\n-\t\t\t \"--keyid-format=long\",\n \t\t\t \"--verify\", temp->filename.buf, \"-\",\n \t\t\t NULL);\n \n-- \n2.16.4\n\n"},{"id":"352074","messageId":"7300c85a1c6d484c781cc80b307b5e7f085ac226.1531208187.git.henning.schild@siemens.com","threadId":"48850","inReplyTo":"cover.1531208187.git.henning.schild@siemens.com","subject":"[PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-10T08:52:28Z","receivedAt":"2018-07-10T08:53:24Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"gnupg does print the keyid followed by a space and the signer comes\nnext. The same pattern is also used in gpgsm, but there the key length\nwould be 40 instead of 16. Instead of hardcoding the expected length,\nfind the first space and calculate it.\n\nSigned-off-by: Henning Schild \n---\n gpg-interface.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 0a8d1bff3..ac2df498d 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -88,10 +88,11 @@ static void parse_gpg_output(struct signature_check *sigc)\n \t\tsigc->result = sigcheck_gpg_status[i].result;\n \t\t/* The trust messages are not followed by key/signer information */\n \t\tif (sigc->result != 'U') {\n-\t\t\tsigc->key = xmemdupz(found, 16);\n+\t\t\tnext = strchrnul(found, ' ');\n+\t\t\tsigc->key = xmemdupz(found, next - found);\n \t\t\t/* The ERRSIG message is not followed by signer information */\n \t\t\tif (sigc-> result != 'E') {\n-\t\t\t\tfound += 17;\n+\t\t\t\tfound = next + 1;\n \t\t\t\tnext = strchrnul(found, '\\n');\n \t\t\t\tsigc->signer = xmemdupz(found, next - found);\n \t\t\t}\n-- \n2.16.4\n\n"},{"id":"352075","messageId":"f97d2d79f6a46ddffcd0065239f99b084708e813.1531208187.git.henning.schild@siemens.com","threadId":"48850","inReplyTo":"cover.1531208187.git.henning.schild@siemens.com","subject":"[PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-10T08:52:31Z","receivedAt":"2018-07-10T08:53:25Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Add test cases to cover the new X509/gpgsm support. Most of them\nresemble existing ones. They just switch the format to x509 and set the\nsigningkey when creating signatures. Validation of signatures does not\nneed any configuration of git, it does need gpgsm to be configured to\ntrust the key(-chain).\nWe generate a self-signed key for committer@example.com and configure\ngpgsm to trust it.\n\nSigned-off-by: Henning Schild \n---\n t/lib-gpg.sh | 9 ++++++-\n t/lib-gpg/gpgsm-gen-key.in | 6 +++++\n t/t4202-log.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++\n t/t5534-push-signed.sh | 52 ++++++++++++++++++++++++++++++++++++\n t/t7003-filter-branch.sh | 15 +++++++++++\n t/t7030-verify-tag.sh | 47 +++++++++++++++++++++++++++++++--\n t/t7600-merge.sh | 31 ++++++++++++++++++++++\n 7 files changed, 223 insertions(+), 3 deletions(-)\n create mode 100644 t/lib-gpg/gpgsm-gen-key.in\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex a5d3b2cba..9dcb4e990 100755\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -38,7 +38,14 @@ then\n \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n \t\tgpg --homedir \"${GNUPGHOME}\" /dev/null 2>&1 \\\n \t\t\t--sign -u committer@example.com &&\n-\t\ttest_set_prereq GPG\n+\t\ttest_set_prereq GPG &&\n+\t\techo | gpgsm --homedir \"${GNUPGHOME}\" -o \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user --passphrase-fd 0 --pinentry-mode loopback --generate-key --batch \"$TEST_DIRECTORY\"/lib-gpg/gpgsm-gen-key.in &&\n+\t\tgpgsm --homedir \"${GNUPGHOME}\" --import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user &&\n+\t\tgpgsm --homedir \"${GNUPGHOME}\" -K | grep fingerprint: | cut -d\" \" -f4 | tr -d '\\n' > ${GNUPGHOME}/trustlist.txt &&\n+\t\techo \" S relax\" >> ${GNUPGHOME}/trustlist.txt &&\n+\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n+\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" -u committer@example.com -o /dev/null --sign - 2>&1 &&\n+\t\ttest_set_prereq GPGSM\n \t\t;;\n \tesac\n fi\ndiff --git a/t/lib-gpg/gpgsm-gen-key.in b/t/lib-gpg/gpgsm-gen-key.in\nnew file mode 100644\nindex 000000000..3470b9dc7\n--- /dev/null\n+++ b/t/lib-gpg/gpgsm-gen-key.in\n@@ -0,0 +1,6 @@\n+Key-Type: RSA\n+Key-Length: 2048\n+Key-Usage: sign\n+Serial: random\n+Name-DN: CN=C O Mitter, O=Example, SN=C O, GN=Mitter\n+Name-Email: committer@example.com\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 25b1f8cc7..abbfa648c 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1556,12 +1556,30 @@ test_expect_success GPG 'setup signed branch' '\n \tgit commit -S -m signed_commit\n '\n \n+test_expect_success GPGSM 'setup signed branch x509' '\n+\ttest_when_finished \"git reset --hard && git checkout master\" &&\n+\tgit checkout -b signed-x509 master &&\n+\techo foo >foo &&\n+\tgit add foo &&\n+\tgit config gpg.format x509 &&\n+\tgit config user.signingkey $GIT_COMMITTER_EMAIL &&\n+\tgit commit -S -m signed_commit &&\n+\tgit config --unset gpg.format &&\n+\tgit config --unset user.signingkey\n+'\n+\n test_expect_success GPG 'log --graph --show-signature' '\n \tgit log --graph --show-signature -n1 signed >actual &&\n \tgrep \"^| gpg: Signature made\" actual &&\n \tgrep \"^| gpg: Good signature\" actual\n '\n \n+test_expect_success GPGSM 'log --graph --show-signature x509' '\n+\tgit log --graph --show-signature -n1 signed-x509 >actual &&\n+\tgrep \"^| gpgsm: Signature made\" actual &&\n+\tgrep \"^| gpgsm: Good signature\" actual\n+'\n+\n test_expect_success GPG 'log --graph --show-signature for merged tag' '\n \ttest_when_finished \"git reset --hard && git checkout master\" &&\n \tgit checkout -b plain master &&\n@@ -1581,11 +1599,39 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '\n \tgrep \"^| | gpg: Good signature\" actual\n '\n \n+test_expect_success GPGSM 'log --graph --show-signature for merged tag x509' '\n+\ttest_when_finished \"git reset --hard && git checkout master\" &&\n+\tgit config gpg.format x509 &&\n+\tgit config user.signingkey $GIT_COMMITTER_EMAIL &&\n+\tgit checkout -b plain-x509 master &&\n+\techo aaa >bar &&\n+\tgit add bar &&\n+\tgit commit -m bar_commit &&\n+\tgit checkout -b tagged-x509 master &&\n+\techo bbb >baz &&\n+\tgit add baz &&\n+\tgit commit -m baz_commit &&\n+\tgit tag -s -m signed_tag_msg signed_tag_x509 &&\n+\tgit checkout plain-x509 &&\n+\tgit merge --no-ff -m msg signed_tag_x509 &&\n+\tgit log --graph --show-signature -n1 plain-x509 >actual &&\n+\tgrep \"^|\\\\\\ merged tag\" actual &&\n+\tgrep \"^| | gpgsm: Signature made\" actual &&\n+\tgrep \"^| | gpgsm: Good signature\" actual &&\n+\tgit config --unset gpg.format &&\n+\tgit config --unset user.signingkey\n+'\n+\n test_expect_success GPG '--no-show-signature overrides --show-signature' '\n \tgit log -1 --show-signature --no-show-signature signed >actual &&\n \t! grep \"^gpg:\" actual\n '\n \n+test_expect_success GPGSM '--no-show-signature overrides --show-signature x509' '\n+\tgit log -1 --show-signature --no-show-signature signed-x509 >actual &&\n+\t! grep \"^gpgsm:\" actual\n+'\n+\n test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '\n \ttest_config log.showsignature true &&\n \tgit log -1 signed >actual &&\n@@ -1593,12 +1639,25 @@ test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '\n \tgrep \"gpg: Good signature\" actual\n '\n \n+test_expect_success GPGSM 'log.showsignature=true behaves like --show-signature x509' '\n+\ttest_config log.showsignature true &&\n+\tgit log -1 signed-x509 >actual &&\n+\tgrep \"gpgsm: Signature made\" actual &&\n+\tgrep \"gpgsm: Good signature\" actual\n+'\n+\n test_expect_success GPG '--no-show-signature overrides log.showsignature=true' '\n \ttest_config log.showsignature true &&\n \tgit log -1 --no-show-signature signed >actual &&\n \t! grep \"^gpg:\" actual\n '\n \n+test_expect_success GPGSM '--no-show-signature overrides log.showsignature=true x509' '\n+\ttest_config log.showsignature true &&\n+\tgit log -1 --no-show-signature signed-x509 >actual &&\n+\t! grep \"^gpgsm:\" actual\n+'\n+\n test_expect_success GPG '--show-signature overrides log.showsignature=false' '\n \ttest_config log.showsignature false &&\n \tgit log -1 --show-signature signed >actual &&\n@@ -1606,6 +1665,13 @@ test_expect_success GPG '--show-signature overrides log.showsignature=false' '\n \tgrep \"gpg: Good signature\" actual\n '\n \n+test_expect_success GPGSM '--show-signature overrides log.showsignature=false x509' '\n+\ttest_config log.showsignature false &&\n+\tgit log -1 --show-signature signed-x509 >actual &&\n+\tgrep \"gpgsm: Signature made\" actual &&\n+\tgrep \"gpgsm: Good signature\" actual\n+'\n+\n test_expect_success 'log --graph --no-walk is forbidden' '\n \ttest_must_fail git log --graph --no-walk\n '\ndiff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh\nindex 1cea758f7..ef2789afa 100755\n--- a/t/t5534-push-signed.sh\n+++ b/t/t5534-push-signed.sh\n@@ -218,4 +218,56 @@ test_expect_success GPG 'fail without key and heed user.signingkey' '\n \ttest_cmp expect dst/push-cert-status\n '\n \n+test_expect_success GPGSM 'fail without key and heed user.signingkey x509' '\n+\tgit config gpg.format x509 &&\n+\tenv | grep GIT > envfile &&\n+\tprepare_dst &&\n+\tmkdir -p dst/.git/hooks &&\n+\tgit -C dst config receive.certnonceseed sekrit &&\n+\twrite_script dst/.git/hooks/post-receive <<-\\EOF &&\n+\t# discard the update list\n+\tcat >/dev/null\n+\t# record the push certificate\n+\tif test -n \"${GIT_PUSH_CERT-}\"\n+\tthen\n+\t\tgit cat-file blob $GIT_PUSH_CERT >../push-cert\n+\tfi &&\n+\n+\tcat >../push-cert-status <expect.in &&\n+\tkey=$(cat \"${GNUPGHOME}/trustlist.txt\" | cut -d\" \" -f1 | tr -d \":\") &&\n+\tsed -e \"s/^KEY=/KEY=${key}/\" expect.in > expect &&\n+\n+\tnoop=$(git rev-parse noop) &&\n+\tff=$(git rev-parse ff) &&\n+\tnoff=$(git rev-parse noff) &&\n+\tgrep \"$noop $ff refs/heads/ff\" dst/push-cert &&\n+\tgrep \"$noop $noff refs/heads/noff\" dst/push-cert &&\n+\ttest_cmp expect dst/push-cert-status\n+'\n+\n+\n test_done\ndiff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh\nindex ec4b160dd..d1ce7a189 100755\n--- a/t/t7003-filter-branch.sh\n+++ b/t/t7003-filter-branch.sh\n@@ -309,6 +309,21 @@ test_expect_success GPG 'Filtering retains message of gpg signed commit' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success GPGSM 'Filtering retains message of gpgsm signed commit' '\n+\tmkdir gpgsm &&\n+\ttouch gpgsm/foo &&\n+\tgit add gpgsm &&\n+\tgit config gpg.format x509 &&\n+\tgit config user.signingkey $GIT_COMMITTER_EMAIL &&\n+\ttest_tick &&\n+\tgit commit -S -m \"Adding gpgsm\" &&\n+\n+\tgit log -1 --format=\"%s\" > expect &&\n+\tgit filter-branch -f --msg-filter \"cat\" &&\n+\tgit log -1 --format=\"%s\" > actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'Tag name filtering allows slashes in tag names' '\n \tgit tag -m tag-with-slash X/1 &&\n \tgit cat-file tag X/1 | sed -e s,X/1,X/2, > expect &&\ndiff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh\nindex 291a1e2b0..510e643cf 100755\n--- a/t/t7030-verify-tag.sh\n+++ b/t/t7030-verify-tag.sh\n@@ -41,6 +41,13 @@ test_expect_success GPG 'create signed tags' '\n \tgit tag -uB7227189 -m eighth eighth-signed-alt\n '\n \n+test_expect_success GPGSM 'create signed tags x509 ' '\n+\tgit config gpg.format x509 &&\n+\tgit config user.signingkey $GIT_COMMITTER_EMAIL &&\n+\techo 9 >file && test_tick && git commit -a -m \"nineth gpgsm-signed\" &&\n+\tgit tag -s -m nineth nineth-signed-x509\n+'\n+\n test_expect_success GPG 'verify and show signatures' '\n \t(\n \t\tfor tag in initial second merge fourth-signed sixth-signed seventh-signed\n@@ -72,6 +79,18 @@ test_expect_success GPG 'verify and show signatures' '\n \t)\n '\n \n+test_expect_success GPGSM 'verify and show signatures x509' '\n+\t(\n+\t\tfor tag in nineth-signed-x509\n+\t\tdo\n+\t\t\tgit verify-tag $tag 2>actual &&\n+\t\t\tgrep \"Good signature from\" actual &&\n+\t\t\t! grep \"BAD signature from\" actual &&\n+\t\t\techo $tag OK || exit 1\n+\t\tdone\n+\t)\n+'\n+\n test_expect_success GPG 'detect fudged signature' '\n \tgit cat-file tag seventh-signed >raw &&\n \tsed -e \"/^tag / s/seventh/7th forged/\" raw >forged1 &&\n@@ -112,8 +131,32 @@ test_expect_success GPG 'verify signatures with --raw' '\n \t)\n '\n \n-test_expect_success GPG 'verify multiple tags' '\n-\ttags=\"fourth-signed sixth-signed seventh-signed\" &&\n+test_expect_success GPGSM 'verify signatures with --raw x509' '\n+\t(\n+\t\tfor tag in nineth-signed-x509\n+\t\tdo\n+\t\t\tgit verify-tag --raw $tag 2>actual &&\n+\t\t\tgrep \"GOODSIG\" actual &&\n+\t\t\t! grep \"BADSIG\" actual &&\n+\t\t\techo $tag OK || exit 1\n+\t\tdone\n+\t)\n+'\n+test_expect_success GPGSM 'verify multiple tags' '\n+\ttags=\"fourth-signed sixth-signed seventh-signed nineth-signed-x509\" &&\n+\tfor i in $tags\n+\tdo\n+\t\tgit verify-tag -v --raw $i || return 1\n+\tdone >expect.stdout 2>expect.stderr.1 &&\n+\tgrep \"^.GNUPG:.\" expect.stderr &&\n+\tgit verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&\n+\tgrep \"^.GNUPG:.\" actual.stderr &&\n+\ttest_cmp expect.stdout actual.stdout &&\n+\ttest_cmp expect.stderr actual.stderr\n+'\n+\n+test_expect_success GPGSM 'verify multiple tags x509' '\n+\ttags=\"fourth-signed sixth-signed seventh-signed nineth-signed-x509\" &&\n \tfor i in $tags\n \tdo\n \t\tgit verify-tag -v --raw $i || return 1\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 6736d8d13..db18b6c31 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -747,6 +747,21 @@ test_expect_success GPG 'merge --ff-only tag' '\n \tgit rev-parse HEAD >actual &&\n \ttest_cmp expect actual\n '\n+test_expect_success GPGSM 'merge --ff-only tag x509' '\n+\tgit reset --hard c0 &&\n+\tgit commit --allow-empty -m \"A newer commit\" &&\n+\tgit config gpg.format x509 &&\n+\tgit config user.signingkey $GIT_COMMITTER_EMAIL &&\n+\tgit tag -s -m \"A newer commit\" signed-x509 &&\n+\tgit reset --hard c0 &&\n+\n+\tgit merge --ff-only signed &&\n+\tgit rev-parse signed^0 >expect &&\n+\tgit rev-parse HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\tgit config --unset gpg.format &&\n+\tgit config --unset user.signingkey\n+'\n \n test_expect_success GPG 'merge --no-edit tag should skip editor' '\n \tgit reset --hard c0 &&\n@@ -760,6 +775,22 @@ test_expect_success GPG 'merge --no-edit tag should skip editor' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success GPGSM 'merge --no-edit tag should skip editor x509' '\n+\tgit reset --hard c0 &&\n+\tgit commit --allow-empty -m \"A newer commit\" &&\n+\tgit config gpg.format x509 &&\n+\tgit config user.signingkey $GIT_COMMITTER_EMAIL &&\n+\tgit tag -f -s -m \"A newer commit\" signed &&\n+\tgit reset --hard c0 &&\n+\n+\tEDITOR=false git merge --no-edit --no-ff signed-x509 &&\n+\tgit rev-parse signed^0 >expect &&\n+\tgit rev-parse HEAD^2 >actual &&\n+\ttest_cmp expect actual &&\n+\tgit config --unset gpg.format &&\n+\tgit config --unset user.signingkey\n+'\n+\n test_expect_success 'set up mod-256 conflict scenario' '\n \t# 256 near-identical stanzas...\n \tfor i in $(test_seq 1 256); do\n-- \n2.16.4\n\n"},{"id":"352076","messageId":"911565bda250ebffc9a87203f2a5ae453f87e5da.1531208187.git.henning.schild@siemens.com","threadId":"48850","inReplyTo":"cover.1531208187.git.henning.schild@siemens.com","subject":"[PATCH v2 3/9] gpg-interface: add new config to select how to sign a commit","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-10T08:52:25Z","receivedAt":"2018-07-10T08:53:32Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Add \"gpg.format\" where the user can specify which type of signature to\nuse for commits. At the moment only \"openpgp\" is supported and the value is\nnot even used. This commit prepares for a new types of signatures.\n\nSigned-off-by: Henning Schild \n---\n Documentation/config.txt | 4 ++++\n gpg-interface.c | 7 +++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1cc18a828..ac373e3f4 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1828,6 +1828,10 @@ gpg.program::\n \tsigned, and the program is expected to send the result to its\n \tstandard output.\n \n+gpg.format::\n+\tSpecifies which key format to use when signing with `--gpg-sign`.\n+\tDefault is \"openpgp\", that is also the only supported value.\n+\n gui.commitMsgWidth::\n \tDefines how wide the commit message window is in the\n \tlinkgit:git-gui[1]. \"75\" is the default.\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 09ddfbc26..ed0e55917 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -7,6 +7,7 @@\n #include \"tempfile.h\"\n \n static char *configured_signing_key;\n+static const char *gpg_format = \"openpgp\";\n static const char *gpg_program = \"gpg\";\n \n #define PGP_SIGNATURE \"-----BEGIN PGP SIGNATURE-----\"\n@@ -138,6 +139,12 @@ int git_gpg_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"gpg.format\")) {\n+\t\tif (strcasecmp(value, \"openpgp\"))\n+\t\t\treturn error(\"malformed value for %s: %s\", var, value);\n+\t\treturn git_config_string(&gpg_format, var, value);\n+\t}\n+\n \tif (!strcmp(var, \"gpg.program\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-- \n2.16.4\n\n"},{"id":"352118","messageId":"20180710154931.GA23624@sigill.intra.peff.net","threadId":"48850","inReplyTo":"7300c85a1c6d484c781cc80b307b5e7f085ac226.1531208187.git.henning.schild@siemens.com","subject":"Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T15:49:31Z","receivedAt":"2018-07-10T15:49:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 10:52:28AM +0200, Henning Schild wrote:\n\n> gnupg does print the keyid followed by a space and the signer comes\n> next. The same pattern is also used in gpgsm, but there the key length\n> would be 40 instead of 16. Instead of hardcoding the expected length,\n> find the first space and calculate it.\n\nSounds good, but I think there's an off-by-one in the patch.\n\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index 0a8d1bff3..ac2df498d 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -88,10 +88,11 @@ static void parse_gpg_output(struct signature_check *sigc)\n> \t\tsigc->result = sigcheck_gpg_status[i].result;\n> \t\t/* The trust messages are not followed by key/signer information */\n> \t\tif (sigc->result != 'U') {\n> -\t\t\tsigc->key = xmemdupz(found, 16);\n> +\t\t\tnext = strchrnul(found, ' ');\n> +\t\t\tsigc->key = xmemdupz(found, next - found);\n\nHere \"next\" may point to the trailing NUL of the string...\n\n> \t\t\t/* The ERRSIG message is not followed by signer information */\n> \t\t\tif (sigc-> result != 'E') {\n> -\t\t\t\tfound += 17;\n> +\t\t\t\tfound = next + 1;\n> \t\t\t\tnext = strchrnul(found, '\\n');\n\n...in which case \"found\" points past the end of the string, and we\nsearch random memory. That's presumably impossible with well-formed gpg\noutput (you don't get 'E' without an extra message), but we should be\nrobust against bogus input.\n\nIn the general case you need:\n\n found = *next ? next + 1 : next;\n\nor similar. In this case, you can actually do:\n\n found = next;\n\nbecause we know that it's OK to search over the literal space again. But\nthat's pretty subtle, so we're probably better off just doing the\nconditional above.\n\n(And yes, looking at the existing code, I think it's even worse, as\nthere does not seem to be a guarantee that we even have 16 characters in\nthe string).\n\n-Peff\n"},{"id":"352122","messageId":"20180710155536.GB23624@sigill.intra.peff.net","threadId":"48850","inReplyTo":"b02154496033220897f6f773e1149a98b21ccba7.1531208187.git.henning.schild@siemens.com","subject":"Re: [PATCH v2 4/9] t/t7510: check the validation of the new config gpg.format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T15:55:36Z","receivedAt":"2018-07-10T15:55:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 10:52:26AM +0200, Henning Schild wrote:\n\n> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\n> index 6e2015ed9..7e1e9caf4 100755\n> --- a/t/t7510-signed-commit.sh\n> +++ b/t/t7510-signed-commit.sh\n> @@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature behaves like --show-signature' '\n> \tgrep \"gpg: Good signature\" actual\n> '\n> \n> +test_expect_success GPG 'check config gpg.format values' '\n> +\trm .git/config &&\n> +\ttest_config gpg.format openpgp &&\n> +\tgit commit -S --amend -m \"success\" &&\n\nYou shouldn't need this \"rm\" here. test_config will add your config, and\nthen delete it after the test finishes.\n\nI know you probably saw that in t1300 or nearby tests, but IMHO they are\nwrong to do so. It's a historical wart that should be cleaned up.\n\n> +\ttest_config gpg.format OpEnPgP &&\n> +\tgit commit -S --amend -m \"success\" &&\n\nA bit of a funny side effect is that we'll unset gpg.format three times\nat the end of the test, since each test_config doesn't know that the\nearlier invocations touched the same variable.\n\nIt's probably not worth addressing, but we could do it with an explicit:\n\n test_when_finished \"test_unconfig gpg.format\" &&\n git config gpg.format openpgp &&\n ...\n git config gpg.format OpEnPgP &&\n\nOr alternatively, this could be three independent tests, which would\ngive the opportunity to describe each.\n\n> +\ttest_config gpg.format malformed &&\n> +\ttest_must_fail git commit -S --amend -m \"fail\" 2>result\n> +'\n\nIf you're not going to look at the saved \"result\", we are better to just\nleave stderr un-redirected. It will go to /dev/null by default, or to\nthe user-visible output of the test is run in verbose mode.\n\n-Peff\n"},{"id":"352123","messageId":"20180710155638.GC23624@sigill.intra.peff.net","threadId":"48850","inReplyTo":"911565bda250ebffc9a87203f2a5ae453f87e5da.1531208187.git.henning.schild@siemens.com","subject":"Re: [PATCH v2 3/9] gpg-interface: add new config to select how to sign a commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T15:56:38Z","receivedAt":"2018-07-10T15:56:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 10:52:25AM +0200, Henning Schild wrote:\n\n> @@ -138,6 +139,12 @@ int git_gpg_config(const char *var, const char *value, void *cb)\n> \t\treturn 0;\n> \t}\n> \n> +\tif (!strcmp(var, \"gpg.format\")) {\n> +\t\tif (strcasecmp(value, \"openpgp\"))\n> +\t\t\treturn error(\"malformed value for %s: %s\", var, value);\n> +\t\treturn git_config_string(&gpg_format, var, value);\n> +\t}\n\nI know we discussed names and case-sensitivity a little bit off-list.\nFWIW, this name looks good to me.\n\n-Peff\n"},{"id":"352127","messageId":"20180710162332.GD23624@sigill.intra.peff.net","threadId":"48850","inReplyTo":"93d74df91b9f5c68dfd8a03f576d7428491976c0.1531208187.git.henning.schild@siemens.com","subject":"Re: [PATCH v2 5/9] gpg-interface: introduce an abstraction for multiple gpg formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T16:23:32Z","receivedAt":"2018-07-10T16:23:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 10:52:27AM +0200, Henning Schild wrote:\n\n> Create a struct that holds the format details for the supported formats.\n> At the moment that is still just \"openpgp\". This commit prepares for the\n> introduction of more formats, that might use other programs and match\n> other signatures.\n\nGreat, this looks like a good incremental step.\n\n> static char *configured_signing_key;\n> -static const char *gpg_format = \"openpgp\";\n> -static const char *gpg_program = \"gpg\";\n> +struct gpg_format_data {\n> +\tconst char *format;\n> +\tconst char *program;\n> +\tconst char *extra_args_verify[1];\n> +\tconst char *sigs[2];\n> +};\n\nThese magic numbers are at a weird distance from where we fill them in:\n\n> +struct gpg_format_data gpg_formats[] = {\n> +\t{ .format = \"openpgp\", .program = \"gpg\",\n> +\t .extra_args_verify = { \"--keyid-format=long\" },\n> +\t .sigs = { PGP_SIGNATURE, PGP_MESSAGE }\n> +\t},\n> +};\n\nI'm not sure if we can easily do any better in C, though. Declaring the\nstruct with an open-ended \"[]\" would make the compiler unhappy. We could\ndo something like:\n\n struct gpg_format_data {\n\t...\n\tconst char **extra_args_verify;\n };\n ...\n static const char *openpgp_verify_args[] = {\n\t\"--key-id-format=long\"\n };\n ...\n static struct gpg_format_data gpg_formats[] = {\n\t{ ...\n\t .extra_args_verify = openpgp_verify_args\n\t}\n };\n\nI'm not sure if that's more horrible or less. It's worse to write in the\nfirst place, but it's slightly easier to maintain going forward. I\ndunno.\n\n> +enum gpgformats { PGP_FMT };\n\nLooks like we use this only for indexing the gpg_formats array. I know\nthat C guarantees 0-indexing, but if we're depending on it, it might be\nworth writing out \"PGP_FMT = 0\" explicitly. And probably adding a\ncomment that this needs to remain in sync with the array.\n\nThe other alternative is that we could simply use\nget_format_data(\"openpgp\"), though that does add a minor runtime cost.\n\n> +struct gpg_format_data gpg_formats[] = {\n> +\t{ .format = \"openpgp\", .program = \"gpg\",\n> +\t .extra_args_verify = { \"--keyid-format=long\" },\n> +\t .sigs = { PGP_SIGNATURE, PGP_MESSAGE }\n> +\t},\n> +};\n\nThis array should be marked static, I think.\n\n> +static struct gpg_format_data *get_format_data(const char *str)\n> +{\n> +\tint i;\n> +\tfor (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n> +\t\tif (!strcasecmp(gpg_formats[i].format, str))\n> +\t\t\treturn gpg_formats + i;\n> +\treturn NULL;\n> +}\n\nThis looks much nicer than the assert()-ing version from v1.\n\n> +static struct gpg_format_data *get_format_data_by_sig(const char *sig)\n> +{\n> +\tint i, j;\n> +\tfor (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n> +\t\tfor (j = 0; j < ARRAY_SIZE(gpg_formats[i].sigs); j++)\n> +\t\t\tif (gpg_formats[i].sigs[j] && \n> +\t\t\t !strncmp(gpg_formats[i].sigs[j], sig,\n> +\t\t\t\t strlen(gpg_formats[i].sigs[j])))\n> +\t\t\t\treturn gpg_formats + i;\n> +\treturn NULL;\n> +}\n\nThis might be a little more readable with:\n\n starts_with(sig, gpg_formats[i].sigs[j])\n\ninstead of the strncmp. It may also be more efficient, as we don't have\nto compute the strlen of the prefix for each non-matching line (the\ncompiler _might_ be smart enough to realize these are all string\nliterals, but it's pretty buried).\n\nI also wondered if our prefix matching here is overly loose. We have to\ndo a prefix match, since \"sig\" isn't terminated at the line buffer. So I\nthink we'd match:\n\n --- BEGIN PGP MESSAGE --- AND SOME OTHER STUFF ---\n\non a line. But I think that's no different than the current code. If we\ncare, I guess we could look for '\\n' or '\\0' immediately after.\n\n> static int is_gpg_start(const char *line)\n> {\n> -\treturn starts_with(line, PGP_SIGNATURE) ||\n> -\t\tstarts_with(line, PGP_MESSAGE);\n> +\treturn (get_format_data_by_sig(line) != NULL);\n> }\n\nI don't know if we've ever discussed this style explicitly, but we'd\nusually omit the unnecessary parentheses for the return here.\n\n> @@ -140,18 +173,14 @@ int git_gpg_config(const char *var, const char *value, void *cb)\n> \t}\n> \n> \tif (!strcmp(var, \"gpg.format\")) {\n> -\t\tif (strcasecmp(value, \"openpgp\"))\n> +\t\tif (!get_format_data(value))\n> \t\t\treturn error(\"malformed value for %s: %s\", var, value);\n> \t\treturn git_config_string(&gpg_format, var, value);\n> \t}\n\nMuch nicer than v1.\n\n> @@ -165,12 +194,16 @@ const char *get_signing_key(void)\n> int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)\n> {\n> \tstruct child_process gpg = CHILD_PROCESS_INIT;\n> +\tstruct gpg_format_data *fmt;\n> \tint ret;\n> \tsize_t i, j, bottom;\n> \tstruct strbuf gpg_status = STRBUF_INIT;\n> \n> +\tfmt = get_format_data(gpg_format);\n> +\tif (!fmt)\n> +\t\tBUG(\"bad gpg_format '%s'\", gpg_format);\n\nThis makes sense as a BUG, because we would already have validated it\nwhen parsing gpg.format earlier. That does make me wonder if we should\nsimply be storing a \"struct gpg_format_data\" instead of a string,\nthough. I.e., at the top-level:\n\n /* default to signing with openpgp */\n static struct gpg_format_data *gpg_format = &gpg_formats[0];\n\n> @@ -223,10 +257,18 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n> \t\treturn -1;\n> \t}\n> \n> +\tfmt = get_format_data_by_sig(signature);\n> +\tassert(fmt);\n\nIs this assert() right? The signature data comes from the user. I guess\nto get here we'll already have matched their signature via\nis_gpg_start(), and this is just a cross-check? If so, then it's OK to\nassert, but a BUG() with a descriptive message would be better still.\n\nI also wonder if whoever parses the signature should get back a\ngpg_format_data and just pass it in here, so we don't have to reparse.\nThat's what my earlier series did. It requires tweaking the function\nsignatures, but IMHO the result was a lot more obvious.\n\n> +\targv_array_pushl(&gpg.args,\n> +\t\t\t fmt->program, NULL);\n\nIf you're just pushing one thing, you don't need pushl(). You can just:\n\n argv_array_push(&gpg.args, fmt->program);\n\n> +\tfor (i = 0; i < ARRAY_SIZE(fmt->extra_args_verify); i++)\n> +\t\tif (fmt->extra_args_verify[i])\n> +\t\t\targv_array_pushl(&gpg.args,\n> +\t\t\t\t\t fmt->extra_args_verify[i], NULL);\n\nLikewise here. Though if you made extra_args_verify a NULL-terminated\nlist, this whole loop could become:\n\n argv_array_pushv(&gpg.args, fmt->extra_args_verify);\n\nIt's not _that_ much code, but I think using NULL-terminated lists in a\nsituation like this is more idiomatic for our code base.\n\n-Peff\n"},{"id":"352132","messageId":"xmqqwou382ch.fsf@gitster-ct.c.googlers.com","threadId":"48850","inReplyTo":"192cf9fc4e7a601d2639ec2d82c777d4c7b26e99.1531208187.git.henning.schild@siemens.com","subject":"Re: [PATCH v2 2/9] gpg-interface: make parse_gpg_output static and remove from interface header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-10T16:47:26Z","receivedAt":"2018-07-10T16:47:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Henning Schild writes:\n\n> This commit turns parse_gpg_output into an internal function, the only\n> outside user was migrated in an earlier commit.\n\nIt is not too big a deal but as we prefer to see our history speak\nin consistent voice, we would usually phrase the above as if we are\ngiving an order to \"make it so\" to the codebase, e.g.\n\n\tTurn parse_gpg_output() into a static function, as the only\n\toutside user was removed in the previous step.\n\nor something like that.\n\nThese two steps, as you said earlier, are nice clean-up patches\nwhose goodness can be measured independently, regardless of the\ngpgsm support which is the primary focus of this series.\n\nThanks.\n\n>\n> Signed-off-by: Henning Schild \n> ---\n> gpg-interface.c | 2 +-\n> gpg-interface.h | 2 --\n> 2 files changed, 1 insertion(+), 3 deletions(-)\n>\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index 0647bd634..09ddfbc26 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -35,7 +35,7 @@ static struct {\n> \t{ 'R', \"\\n[GNUPG:] REVKEYSIG \"},\n> };\n> \n> -void parse_gpg_output(struct signature_check *sigc)\n> +static void parse_gpg_output(struct signature_check *sigc)\n> {\n> \tconst char *buf = sigc->gpg_status;\n> \tint i;\n> diff --git a/gpg-interface.h b/gpg-interface.h\n> index a5e6517ae..5ecff4aa0 100644\n> --- a/gpg-interface.h\n> +++ b/gpg-interface.h\n> @@ -33,8 +33,6 @@ void signature_check_clear(struct signature_check *sigc);\n> */\n> size_t parse_signature(const char *buf, size_t size);\n> \n> -void parse_gpg_output(struct signature_check *);\n> -\n> /*\n> * Create a detached signature for the contents of \"buffer\" and append\n> * it after \"signature\"; \"buffer\" and \"signature\" can be the same\n"},{"id":"352133","messageId":"20180710165412.GE23624@sigill.intra.peff.net","threadId":"48850","inReplyTo":"4905c1907a866c0fd1a4dac978dd6ca3e468ac43.1531208187.git.henning.schild@siemens.com","subject":"Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T16:54:13Z","receivedAt":"2018-07-10T16:54:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 10:52:29AM +0200, Henning Schild wrote:\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index ac373e3f4..c0bd80954 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1832,6 +1832,11 @@ gpg.format::\n> \tSpecifies which key format to use when signing with `--gpg-sign`.\n> \tDefault is \"openpgp\", that is also the only supported value.\n> \n> +gpg..program::\n> +\tUse this to customize the program used for the signing format you\n> +\tchose. (see gpg.program) gpg.openpgp.program is a synonym for the\n> +\tlegacy gpg.program.\n\nThis seems like a good step forward. This is similar to the\nsigningtool.$name.program I proposed earlier, but keeping it specific to\ngpg, which makes sense.\n\nOn the other hand, do we anticipate the user ever being able to add\ngpg.foo.program? I don't think so; we'll provide a limited set of\noptions. So we _could_ go with \"gpg.openpgpProgram\" or similar, and\nlater add \"gpg.x509Program\".\n\nAnd one reason to do so might be...\n\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index ac2df498d..65098430f 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -179,7 +179,7 @@ int git_gpg_config(const char *var, const char *value, void *cb)\n> \t\treturn git_config_string(&gpg_format, var, value);\n> \t}\n> \n> -\tif (!strcmp(var, \"gpg.program\"))\n> +\tif (!strcmp(var, \"gpg.program\") || !strcmp(var, \"gpg.openpgp.program\"))\n> \t\treturn git_config_string(&gpg_formats[PGP_FMT].program, var,\n> \t\t\t\t\t value);\n\nWe normally match config keys with strcmp() because the config machinery\nwill have already normalized them to lowercase. But in Git's config\nformat, the subsection (the middle in a three-dot name) is less\nrestricted and is case-sensitive.\n\nShould we allow:\n\n [gpg \"OpenPGP\"]\n program = whatever\n\ngiven that we allow:\n\n [gpg]\n format = OpenPGP\n\n? I think just using strcasecmp() here would be sufficient. But I wonder\nif it is a symptom of using the wrong tool (subsections) when we don't\nneed it.\n\n-Peff\n"},{"id":"352134","messageId":"xmqqr2kb81zw.fsf@gitster-ct.c.googlers.com","threadId":"48850","inReplyTo":"b02154496033220897f6f773e1149a98b21ccba7.1531208187.git.henning.schild@siemens.com","subject":"Re: [PATCH v2 4/9] t/t7510: check the validation of the new config gpg.format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-10T16:54:59Z","receivedAt":"2018-07-10T16:55:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Henning Schild writes:\n\n> Test setting gpg.format to both invalid and valid values.\n>\n> Signed-off-by: Henning Schild \n> ---\n> t/t7510-signed-commit.sh | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n>\n> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\n> index 6e2015ed9..7e1e9caf4 100755\n> --- a/t/t7510-signed-commit.sh\n> +++ b/t/t7510-signed-commit.sh\n> @@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature behaves like --show-signature' '\n> \tgrep \"gpg: Good signature\" actual\n> '\n> \n> +test_expect_success GPG 'check config gpg.format values' '\n> +\trm .git/config &&\n\nPlease don't. .git/config has stuff that are more important than\njust collection of random configuration these days, and we can even\nexpect that future versions of Git may not store its config in a\nflat file .git/config but in a different mechanism \"git config\"\ncommand knows how to access. A low-level test for \"git config\"\ncommand's operation may have to be implemented by inspecting the\nresulting .git/config, but as this test is not about \"git config\"'s\ninner workins but is about one feature \"git commit\" command has,\nwe prefer not to depend too much on the internal implementation\ndetail such as \"local config is stored in .git/config file\".\n\nLet's hear why you want to remove this file; what things that have\npreviously been placed in the file do you want not to see, before\nperforming the following actions? Once we know that, we can suggest\na way to do so better than removing the entire file.\n\n> +\ttest_config gpg.format openpgp &&\n> +\tgit commit -S --amend -m \"success\" &&\n> +\ttest_config gpg.format OpEnPgP &&\n> +\tgit commit -S --amend -m \"success\" &&\n> +\ttest_config gpg.format malformed &&\n> +\ttest_must_fail git commit -S --amend -m \"fail\" 2>result\n> +'\n> +\n> test_done\n"},{"id":"352135","messageId":"20180710165638.GF23624@sigill.intra.peff.net","threadId":"48850","inReplyTo":"20180710165412.GE23624@sigill.intra.peff.net","subject":"Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T16:56:38Z","receivedAt":"2018-07-10T16:56:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 12:54:13PM -0400, Jeff King wrote:\n\n> Should we allow:\n> \n> [gpg \"OpenPGP\"]\n> program = whatever\n> \n> given that we allow:\n> \n> [gpg]\n> format = OpenPGP\n> \n> ? I think just using strcasecmp() here would be sufficient. But I wonder\n> if it is a symptom of using the wrong tool (subsections) when we don't\n> need it.\n\nI did just read the discussion in response to v1, where everybody told\nyou the opposite. ;)\n\nSo I guess my question/points are more for brian and Junio.\n\n-Peff\n"},{"id":"352136","messageId":"20180710170109.GG23624@sigill.intra.peff.net","threadId":"48850","inReplyTo":"4a2cf83a63d25776cb1996490240ce3e5df8ada4.1531208187.git.henning.schild@siemens.com","subject":"Re: [PATCH v2 8/9] gpg-interface: introduce new signature format \"x509\" using gpgsm","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T17:01:10Z","receivedAt":"2018-07-10T17:01:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 10:52:30AM +0200, Henning Schild wrote:\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index c0bd80954..b6f9b47d5 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1830,7 +1830,7 @@ gpg.program::\n> \n> gpg.format::\n> \tSpecifies which key format to use when signing with `--gpg-sign`.\n> -\tDefault is \"openpgp\", that is also the only supported value.\n> +\tDefault is \"opengpg\" and another possible value is \"x509\".\n\nopengpg?\n\nSince we're having so much fun with naming discussions, let's talk about\n\"x509\". :)\n\nThat's the cert format. I think of these signatures as S/MIME, but\nreally that's the mail-oriented parts of the standard. I think\ntechnically this is \"CMS\".\n\nThat said, we should pick what most people will find natural when\nreferring to it. So maybe x509 isn't the worst choice, as I doubt most\npeople know the term CMS. Probably the term they know _most_ is \"gpgsm\",\nbut I think the point is that one does not have to be using gpgsm in the\nfirst place.\n\nSo I dunno. I think I talked myself back into x509. ;)\n\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index 65098430f..bf8d567a4 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -16,13 +16,18 @@ struct gpg_format_data {\n> \n> #define PGP_SIGNATURE \"-----BEGIN PGP SIGNATURE-----\"\n> #define PGP_MESSAGE \"-----BEGIN PGP MESSAGE-----\"\n> +#define X509_SIGNATURE \"-----BEGIN SIGNED MESSAGE-----\"\n> \n> -enum gpgformats { PGP_FMT };\n> +enum gpgformats { PGP_FMT, X509_FMT };\n> struct gpg_format_data gpg_formats[] = {\n> \t{ .format = \"openpgp\", .program = \"gpg\",\n> \t .extra_args_verify = { \"--keyid-format=long\" },\n> \t .sigs = { PGP_SIGNATURE, PGP_MESSAGE }\n> \t},\n> +\t{ .format = \"x509\", .program = \"gpgsm\",\n> +\t .extra_args_verify = { NULL },\n> +\t .sigs = { X509_SIGNATURE, NULL }\n> +\t},\n\nExtremely minor nit, but if there are no other uses of PGP_SIGNATURE etc\noutside of this array (as I hope there wouldn't be after this series),\nwould it make more sense to just include the literals inline in the\narray definition? That's one less layer of indirection when somebody is\nreading the code.\n\n-Peff\n"},{"id":"352137","messageId":"20180710170901.GH23624@sigill.intra.peff.net","threadId":"48850","inReplyTo":"f97d2d79f6a46ddffcd0065239f99b084708e813.1531208187.git.henning.schild@siemens.com","subject":"Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T17:09:01Z","receivedAt":"2018-07-10T17:09:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 10:52:31AM +0200, Henning Schild wrote:\n\n> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\n> index a5d3b2cba..9dcb4e990 100755\n> --- a/t/lib-gpg.sh\n> +++ b/t/lib-gpg.sh\n> @@ -38,7 +38,14 @@ then\n> \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n> \t\tgpg --homedir \"${GNUPGHOME}\" /dev/null 2>&1 \\\n> \t\t\t--sign -u committer@example.com &&\n> -\t\ttest_set_prereq GPG\n> +\t\ttest_set_prereq GPG &&\n> +\t\techo | gpgsm --homedir \"${GNUPGHOME}\" -o \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user --passphrase-fd 0 --pinentry-mode loopback --generate-key --batch \"$TEST_DIRECTORY\"/lib-gpg/gpgsm-gen-key.in &&\n\nIs this --generate-key going to require high-quality entropy? That could\nslow the test suite down (and maybe even stall it quite a bit on servers\ndoing automated CI).\n\nCan we save a dummy generated key and just import it? That's what we do\nfor the regular gpg case.\n\n> +\t\tgpgsm --homedir \"${GNUPGHOME}\" --import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user &&\n> +\t\tgpgsm --homedir \"${GNUPGHOME}\" -K | grep fingerprint: | cut -d\" \" -f4 | tr -d '\\n' > ${GNUPGHOME}/trustlist.txt &&\n> +\t\techo \" S relax\" >> ${GNUPGHOME}/trustlist.txt &&\n> +\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n> +\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" -u committer@example.com -o /dev/null --sign - 2>&1 &&\n> +\t\ttest_set_prereq GPGSM\n\nThis &&-chain means we can't have GPGSM without GPG. In theory the two\ncould be tested independently. I don't know if it's worth the trouble to\nmake that work, though. I wouldn't be surprised if there are some subtle\ndependencies within the test scripts, and I'm not sure how common it is\nfor somebody to have gpgsm and not gpg. So it may make sense to just\npunt on it until such a person appears.\n\n> test_expect_success GPG 'log --graph --show-signature' '\n> \tgit log --graph --show-signature -n1 signed >actual &&\n> \tgrep \"^| gpg: Signature made\" actual &&\n> \tgrep \"^| gpg: Good signature\" actual\n> '\n> \n> +test_expect_success GPGSM 'log --graph --show-signature x509' '\n> +\tgit log --graph --show-signature -n1 signed-x509 >actual &&\n> +\tgrep \"^| gpgsm: Signature made\" actual &&\n> +\tgrep \"^| gpgsm: Good signature\" actual\n> +'\n\nWe're going to have a lot of duplicated tests here. That's a maintenance\nburden when one of them needs fixes later. And when new tests are added,\nwe won't automatically get them tested under each format.\n\nCan we move the battery of tests into a function that takes a few\nparameters (prereq name, branch to look at, etc) and then call it for\nboth the gpg/gpgsm cases?\n\n-Peff\n"},{"id":"352138","messageId":"20180710171224.GI23624@sigill.intra.peff.net","threadId":"48850","inReplyTo":"cover.1531208187.git.henning.schild@siemens.com","subject":"Re: [PATCH v2 0/9] X509 (gpgsm) commit signing support","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T17:12:24Z","receivedAt":"2018-07-10T17:12:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 10:52:22AM +0200, Henning Schild wrote:\n\n> This series adds support for signing commits with gpgsm.\n\nThanks for working on this. I left a bunch of comments, but overall the\ndirection looks good.\n\nWe talked about this a bit off-list, but just for the public record:\n\n> This series can be seen as a follow up of a series that appeared under\n> the name \"gpg-interface: Multiple signing tools\" in april 2018 [1]. After\n> that series was not merged i decided to get my patches ready. The\n> original series aimed at being generic for any sort of signing tool, while\n> this series just introduced the X509 variant of gpg. (gpgsm)\n> I collected authors and reviewers of that first series and already put them\n> on cc.\n\nThis series is a fine replacement for that earlier work. It's flexible\nenough to allow what we really wanted out of that series (gpgsm support,\nor another drop-in tool that uses the same interface). It doesn't lay\nany groundwork for further tools (like signify), but I think the\nconsensus on the list was to punt on that until somebody had more\nconcrete plans for adding such a tool.\n\n-Peff\n"},{"id":"352140","messageId":"20180710171628.GC23438@sigill.intra.peff.net","threadId":"48850","inReplyTo":"20180710170901.GH23624@sigill.intra.peff.net","subject":"Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T17:16:29Z","receivedAt":"2018-07-10T17:16:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 01:09:01PM -0400, Jeff King wrote:\n\n> > +\t\tgpgsm --homedir \"${GNUPGHOME}\" --import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user &&\n> > +\t\tgpgsm --homedir \"${GNUPGHOME}\" -K | grep fingerprint: | cut -d\" \" -f4 | tr -d '\\n' > ${GNUPGHOME}/trustlist.txt &&\n> > +\t\techo \" S relax\" >> ${GNUPGHOME}/trustlist.txt &&\n> > +\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n> > +\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" -u committer@example.com -o /dev/null --sign - 2>&1 &&\n> > +\t\ttest_set_prereq GPGSM\n> \n> This &&-chain means we can't have GPGSM without GPG. In theory the two\n> could be tested independently. I don't know if it's worth the trouble to\n> make that work, though. I wouldn't be surprised if there are some subtle\n> dependencies within the test scripts, and I'm not sure how common it is\n> for somebody to have gpgsm and not gpg. So it may make sense to just\n> punt on it until such a person appears.\n\nOof, sorry, I thought I had read all of the v1 review, but I just\nnoticed I missed a similar comment from brian. So it sounds like you\nalready considered this, and just ignore what I wrote here.\n\n-Peff\n"},{"id":"352141","messageId":"xmqqlgaj80zs.fsf@gitster-ct.c.googlers.com","threadId":"48850","inReplyTo":"93d74df91b9f5c68dfd8a03f576d7428491976c0.1531208187.git.henning.schild@siemens.com","subject":"Re: [PATCH v2 5/9] gpg-interface: introduce an abstraction for multiple gpg formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-10T17:16:39Z","receivedAt":"2018-07-10T17:16:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Henning Schild writes:\n\n> Create a struct that holds the format details for the supported formats.\n> At the moment that is still just \"openpgp\". This commit prepares for the\n> introduction of more formats, that might use other programs and match\n> other signatures.\n>\n> Signed-off-by: Henning Schild \n> ---\n> gpg-interface.c | 74 ++++++++++++++++++++++++++++++++++++++++++++-------------\n> 1 file changed, 58 insertions(+), 16 deletions(-)\n>\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index ed0e55917..0a8d1bff3 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -7,12 +7,46 @@\n> #include \"tempfile.h\"\n> \n> static char *configured_signing_key;\n> -static const char *gpg_format = \"openpgp\";\n> -static const char *gpg_program = \"gpg\";\n> +struct gpg_format_data {\n> +\tconst char *format;\n> +\tconst char *program;\n> +\tconst char *extra_args_verify[1];\n> +\tconst char *sigs[2];\n> +};\n\nDo you use identifier \"gpg_format\" elsewhere as a structure name? A\nstructure is there always to hold \"data\", so often \"_data\" suffix is\nmeaningless in a structure type name like this.\n\n> #define PGP_SIGNATURE \"-----BEGIN PGP SIGNATURE-----\"\n> #define PGP_MESSAGE \"-----BEGIN PGP MESSAGE-----\"\n> \n> +enum gpgformats { PGP_FMT };\n> +struct gpg_format_data gpg_formats[] = {\n> +\t{ .format = \"openpgp\", .program = \"gpg\",\n> +\t .extra_args_verify = { \"--keyid-format=long\" },\n> +\t .sigs = { PGP_SIGNATURE, PGP_MESSAGE }\n> +\t},\n> +};\n> +static const char *gpg_format = \"openpgp\";\n> +\n> +static struct gpg_format_data *get_format_data(const char *str)\n\nget_format_data_by_name() or something like that, for consistency\nwith the next function, perhaps?\n\n> +{\n> +\tint i;\n> +\tfor (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n> +\t\tif (!strcasecmp(gpg_formats[i].format, str))\n> +\t\t\treturn gpg_formats + i;\n> +\treturn NULL;\n> +}\n> +\n> +static struct gpg_format_data *get_format_data_by_sig(const char *sig)\n> +{\n> +\tint i, j;\n> +\tfor (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n> +\t\tfor (j = 0; j < ARRAY_SIZE(gpg_formats[i].sigs); j++)\n> +\t\t\tif (gpg_formats[i].sigs[j] && \n> +\t\t\t !strncmp(gpg_formats[i].sigs[j], sig,\n> +\t\t\t\t strlen(gpg_formats[i].sigs[j])))\n> +\t\t\t\treturn gpg_formats + i;\n> +\treturn NULL;\n> +}\n> +\n> void signature_check_clear(struct signature_check *sigc)\n> {\n> \tFREE_AND_NULL(sigc->payload);\n> @@ -104,8 +138,7 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)\n> \n> static int is_gpg_start(const char *line)\n> {\n> -\treturn starts_with(line, PGP_SIGNATURE) ||\n> -\t\tstarts_with(line, PGP_MESSAGE);\n> +\treturn (get_format_data_by_sig(line) != NULL);\n\nIf this is still called \"is_gpg_start()\", then shouldn't the\nimplementation be more like this?\n\n\tstruct gpg_format *found = get_format_data_by_sig(line);\n\n\treturn found && found == &gpg_formats[PGP_FMT];\n\nIt probably does not make much difference in the end, as you won't\nhave functions is_(gpg|gpgsm|somethingelse|yetanother)_start() but\neither have a single function \"does some x509 looking block start\nhere?\" or \"I am expecting gpg and nothing else, does a block for\nthat start here?\" and at that point this will no longer be called\nis_gpg_start(), I would expect.\n\n> size_t parse_signature(const char *buf, size_t size)\n> @@ -140,18 +173,14 @@ int git_gpg_config(const char *var, const char *value, void *cb)\n> \t}\n> \n> \tif (!strcmp(var, \"gpg.format\")) {\n> -\t\tif (strcasecmp(value, \"openpgp\"))\n> +\t\tif (!get_format_data(value))\n> \t\t\treturn error(\"malformed value for %s: %s\", var, value);\n> \t\treturn git_config_string(&gpg_format, var, value);\n\nThis is a bug introduced in an earlier step in the series, but you\nwill segfault when given a configuration that looks like this:\n\n\t[gpg]\n\t\tformat\n\nImitate the way how !value is diagnosed upfront for \"gpg.program\"\nbelow before this patch.\n\n> \t}\n> \n> -\tif (!strcmp(var, \"gpg.program\")) {\n> -\t\tif (!value)\n> -\t\t\treturn config_error_nonbool(var);\n> -\t\tgpg_program = xstrdup(value);\n> -\t\treturn 0;\n> -\t}\n> -\n> +\tif (!strcmp(var, \"gpg.program\"))\n> +\t\treturn git_config_string(&gpg_formats[PGP_FMT].program, var,\n> +\t\t\t\t\t value);\n\ngit_config_string() has its own \"do not feed boolean 'true' to me\"\ncheck, so this change does not introduce a regression.\n\n> @@ -165,12 +194,16 @@ const char *get_signing_key(void)\n> int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)\n> {\n> \tstruct child_process gpg = CHILD_PROCESS_INIT;\n> +\tstruct gpg_format_data *fmt;\n> \tint ret;\n> \tsize_t i, j, bottom;\n> \tstruct strbuf gpg_status = STRBUF_INIT;\n> \n> +\tfmt = get_format_data(gpg_format);\n> +\tif (!fmt)\n> +\t\tBUG(\"bad gpg_format '%s'\", gpg_format);\n> \targv_array_pushl(&gpg.args,\n> -\t\t\t gpg_program,\n> +\t\t\t fmt->program,\n> \t\t\t \"--status-fd=2\",\n> \t\t\t \"-bsau\", signing_key,\n> \t\t\t NULL);\n> @@ -208,8 +241,9 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n> \t\t\t struct strbuf *gpg_output, struct strbuf *gpg_status)\n> {\n> \tstruct child_process gpg = CHILD_PROCESS_INIT;\n> +\tstruct gpg_format_data *fmt;\n> \tstruct tempfile *temp;\n> -\tint ret;\n> +\tint ret, i;\n> \tstruct strbuf buf = STRBUF_INIT;\n> \n> \ttemp = mks_tempfile_t(\".git_vtag_tmpXXXXXX\");\n> @@ -223,10 +257,18 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n> \t\treturn -1;\n> \t}\n> \n> +\tfmt = get_format_data_by_sig(signature);\n> +\tassert(fmt);\n> +\n> +\targv_array_pushl(&gpg.args,\n> +\t\t\t fmt->program, NULL);\n> +\tfor (i = 0; i < ARRAY_SIZE(fmt->extra_args_verify); i++)\n> +\t\tif (fmt->extra_args_verify[i])\n> +\t\t\targv_array_pushl(&gpg.args,\n> +\t\t\t\t\t fmt->extra_args_verify[i], NULL);\n\nThis loop allows us to extend .extra_args_verify when we need to\nsupport another variant that needs to take two arguments by making\n.extra_args field larger for all variants and stuffing a NULL to\nunused slots, and it even allows a nonsense like this\n\n\t.extra_args_verify = { NULL, \"--keyid-format=long\" },\n\nI cannot quite shake the feeling that .extra_args_verify and .sigs\nfields should not be of fixed sized array of strings, but rather be\nof type \"const char **\" that is NULL terminated. Such a clean-up\nwill make it harder to write initializers, so it may not be worth\nit, I guess.\n\n> \targv_array_pushl(&gpg.args,\n> -\t\t\t gpg_program,\n> \t\t\t \"--status-fd=1\",\n> -\t\t\t \"--keyid-format=long\",\n> \t\t\t \"--verify\", temp->filename.buf, \"-\",\n> \t\t\t NULL);\n"},{"id":"352142","messageId":"20180710175020.GA25861@sigill.intra.peff.net","threadId":"48850","inReplyTo":"xmqqbmbf7zw9.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 8/9] gpg-interface: introduce new signature format \"x509\" using gpgsm","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-10T17:50:20Z","receivedAt":"2018-07-10T18:17:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 10:40:22AM -0700, Junio C Hamano wrote:\n\n> > Extremely minor nit, but if there are no other uses of PGP_SIGNATURE etc\n> > outside of this array (as I hope there wouldn't be after this series),\n> > would it make more sense to just include the literals inline in the\n> > array definition? That's one less layer of indirection when somebody is\n> > reading the code.\n> \n> It is good design-sense to shoot for fewer levels of indirection,\n> but I suspect that \"'const char **' instead of maximally-sized fixed\n> array of strings\" would require a named array and constants like\n> this:\n\nYes, I agree with that direction (because it drops the magic numbers and\nlets us use existing argv_array helpers).\n\n> [...]\n> so we may end up having the same number of levels of indirection\n> anyway in the long-term final form.\n\nTrue, but at least this level of indirection is buying us something. :)\n\n> As readers may be able to read from the above, I also have a\n> suspicion that it is a mistake to pretend that \"--verify\" etc.,\n> which merely happen to be common across the variants the series\n> covers, will stay forever to be common across _all_ variants and\n> that is why the field no longer is called \"extra\" args but is meant\n> to contain the full args.\n\nI'd be fine going in that direction, too. But I don't actually foresee\nadding new variants in the future. The point of this series versus the\nsigningtool one is that it's limited to gpg and gpg-alikes. And I doubt\nwe're likely to see more than the two that exist.\n\nSo even if we do end up adding support for more tools in the long run, I\nthink it will outgrow this config scheme.\n\n-Peff\n"},{"id":"352158","messageId":"xmqqbmbf7zw9.fsf@gitster-ct.c.googlers.com","threadId":"48850","inReplyTo":"20180710170109.GG23624@sigill.intra.peff.net","subject":"Re: [PATCH v2 8/9] gpg-interface: introduce new signature format \"x509\" using gpgsm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-10T17:40:22Z","receivedAt":"2018-07-10T18:43:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> @@ -16,13 +16,18 @@ struct gpg_format_data {\n>> \n>> #define PGP_SIGNATURE \"-----BEGIN PGP SIGNATURE-----\"\n>> #define PGP_MESSAGE \"-----BEGIN PGP MESSAGE-----\"\n>> +#define X509_SIGNATURE \"-----BEGIN SIGNED MESSAGE-----\"\n>> \n>> -enum gpgformats { PGP_FMT };\n>> +enum gpgformats { PGP_FMT, X509_FMT };\n>> struct gpg_format_data gpg_formats[] = {\n>> \t{ .format = \"openpgp\", .program = \"gpg\",\n>> \t .extra_args_verify = { \"--keyid-format=long\" },\n>> \t .sigs = { PGP_SIGNATURE, PGP_MESSAGE }\n>> \t},\n>> +\t{ .format = \"x509\", .program = \"gpgsm\",\n>> +\t .extra_args_verify = { NULL },\n>> +\t .sigs = { X509_SIGNATURE, NULL }\n>> +\t},\n>\n> Extremely minor nit, but if there are no other uses of PGP_SIGNATURE etc\n> outside of this array (as I hope there wouldn't be after this series),\n> would it make more sense to just include the literals inline in the\n> array definition? That's one less layer of indirection when somebody is\n> reading the code.\n\nIt is good design-sense to shoot for fewer levels of indirection,\nbut I suspect that \"'const char **' instead of maximally-sized fixed\narray of strings\" would require a named array and constants like\nthis:\n\n\tstatic const char *gpg_verify_args[] = {\n\t\t\"--verify\",\n\t\t\"--status-fd=1\",\n\t\t\"--keyid-format=long\",\n\t\tNULL\n\t};\n\tstatic const char *gpg_sigs[] = {\n\t\t\"-----BEGIN PGP SIGNATURE-----\",\n\t\t\"-----BEGIN PGP MESSAGE-----\",\n\t\tNULL\n\t};\n\n\tstruct gpg_format {\n\t\tconst char *name;\n\t\tconst char *program;\n\t\tconst char * const *verify_args;\n\t\tconst char * const *sigs;\n\t} gpg_format[] = {\n\t\t{\n\t\t\t.name = \"openpgp\",\n\t\t\t.program = \"gpg',\n\t\t\t.verify_args = gpg_verify_args,\n\t\t\t.sigs = gpg_sigs,\n\t\t},\n\t\t{\n\t\t\t...\n\t\t},\n\t};\n\nso we may end up having the same number of levels of indirection\nanyway in the long-term final form.\n\nAs readers may be able to read from the above, I also have a\nsuspicion that it is a mistake to pretend that \"--verify\" etc.,\nwhich merely happen to be common across the variants the series\ncovers, will stay forever to be common across _all_ variants and\nthat is why the field no longer is called \"extra\" args but is meant\nto contain the full args.\n"},{"id":"352168","messageId":"xmqqh8l780e9.fsf@gitster-ct.c.googlers.com","threadId":"48850","inReplyTo":"20180710165412.GE23624@sigill.intra.peff.net","subject":"Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-10T17:29:34Z","receivedAt":"2018-07-10T19:10:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Should we allow:\n>\n> [gpg \"OpenPGP\"]\n> program = whatever\n>\n> given that we allow:\n>\n> [gpg]\n> format = OpenPGP\n\nIf the latter is allowed then we should allow the former. \n\nBut because allowing the former is cumbersome, we may be better off\nnot parsing the value case-insensitively like an earlier step in\nthis series did.\n"},{"id":"352181","messageId":"xmqqlgai6bhi.fsf@gitster-ct.c.googlers.com","threadId":"48850","inReplyTo":"f97d2d79f6a46ddffcd0065239f99b084708e813.1531208187.git.henning.schild@siemens.com","subject":"Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-10T21:12:57Z","receivedAt":"2018-07-10T21:13:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Henning Schild writes:\n\n> Add test cases to cover the new X509/gpgsm support. Most of them\n> resemble existing ones. They just switch the format to x509 and set the\n> signingkey when creating signatures. Validation of signatures does not\n> need any configuration of git, it does need gpgsm to be configured to\n> trust the key(-chain).\n> We generate a self-signed key for committer@example.com and configure\n> gpgsm to trust it.\n>\n> Signed-off-by: Henning Schild \n> ---\n> t/lib-gpg.sh | 9 ++++++-\n> t/lib-gpg/gpgsm-gen-key.in | 6 +++++\n> t/t4202-log.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++\n> t/t5534-push-signed.sh | 52 ++++++++++++++++++++++++++++++++++++\n> t/t7003-filter-branch.sh | 15 +++++++++++\n> t/t7030-verify-tag.sh | 47 +++++++++++++++++++++++++++++++--\n> t/t7600-merge.sh | 31 ++++++++++++++++++++++\n> 7 files changed, 223 insertions(+), 3 deletions(-)\n> create mode 100644 t/lib-gpg/gpgsm-gen-key.in\n\nI saw my post-integration tests taking forever to finish for 'pu'\nand \"ps x\" output showed that many tests (I ran tests in parallel)\nwere stuck and all of them were running gpgsm. \n\nFor now, I've ejected the topic out of 'pu', as I want to finish\ntoday's integration run with all the other topics first before\ncoming back to this topic.\n\n\n"},{"id":"352213","messageId":"20180711100208.34387665@md1pvb1c.ad001.siemens.net","threadId":"48850","inReplyTo":"20180710155536.GB23624@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/9] t/t7510: check the validation of the new config gpg.format","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-11T08:02:08Z","receivedAt":"2018-07-11T08:02:34Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Tue, 10 Jul 2018 11:55:36 -0400\nschrieb Jeff King :\n\n> On Tue, Jul 10, 2018 at 10:52:26AM +0200, Henning Schild wrote:\n> \n> > diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\n> > index 6e2015ed9..7e1e9caf4 100755\n> > --- a/t/t7510-signed-commit.sh\n> > +++ b/t/t7510-signed-commit.sh\n> > @@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature\n> > behaves like --show-signature' ' grep \"gpg: Good signature\" actual\n> > '\n> > \n> > +test_expect_success GPG 'check config gpg.format values' '\n> > +\trm .git/config &&\n> > +\ttest_config gpg.format openpgp &&\n> > +\tgit commit -S --amend -m \"success\" && \n> \n> You shouldn't need this \"rm\" here. test_config will add your config,\n> and then delete it after the test finishes.\n\nRight, got rid of it.\n\n> I know you probably saw that in t1300 or nearby tests, but IMHO they\n> are wrong to do so. It's a historical wart that should be cleaned up.\n> \n> > +\ttest_config gpg.format OpEnPgP &&\n> > +\tgit commit -S --amend -m \"success\" && \n> \n> A bit of a funny side effect is that we'll unset gpg.format three\n> times at the end of the test, since each test_config doesn't know\n> that the earlier invocations touched the same variable.\n> \n> It's probably not worth addressing, but we could do it with an\n> explicit:\n\nI think it is not worth addressing. Manual unsetting would mess with\ntest_config and both alternatives would just bloat the code. The\nadditional two unsets do not hurt, imho.\n\n> test_when_finished \"test_unconfig gpg.format\" &&\n> git config gpg.format openpgp &&\n> ...\n> git config gpg.format OpEnPgP &&\n> \n> Or alternatively, this could be three independent tests, which would\n> give the opportunity to describe each.\n> \n> > +\ttest_config gpg.format malformed &&\n> > +\ttest_must_fail git commit -S --amend -m \"fail\" 2>result\n> > +' \n> \n> If you're not going to look at the saved \"result\", we are better to\n> just leave stderr un-redirected. It will go to /dev/null by default,\n> or to the user-visible output of the test is run in verbose mode.\n\nRight, dropped the redirect.\n\nHenning\n \n> -Peff\n\n"},{"id":"352217","messageId":"20180711104100.289b1569@md1pvb1c.ad001.siemens.net","threadId":"48850","inReplyTo":"xmqqwou382ch.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/9] gpg-interface: make parse_gpg_output static and remove from interface header","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-11T08:41:00Z","receivedAt":"2018-07-11T08:41:24Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Tue, 10 Jul 2018 09:47:26 -0700\nschrieb Junio C Hamano :\n\n> Henning Schild writes:\n> \n> > This commit turns parse_gpg_output into an internal function, the\n> > only outside user was migrated in an earlier commit. \n> \n> It is not too big a deal but as we prefer to see our history speak\n> in consistent voice, we would usually phrase the above as if we are\n> giving an order to \"make it so\" to the codebase, e.g.\n> \n> \tTurn parse_gpg_output() into a static function, as the only\n> \toutside user was removed in the previous step.\n> \n> or something like that.\n> \n> These two steps, as you said earlier, are nice clean-up patches\n> whose goodness can be measured independently, regardless of the\n> gpgsm support which is the primary focus of this series.\n\nI updated the comment of the second one and send them as a seperate\nseries. I do not expect them to change since there was -- so far -- no\nfeedback suggesting that. So i am tempted to promise that ;).\n\nHenning\n\n> Thanks.\n> \n> >\n> > Signed-off-by: Henning Schild \n> > ---\n> > gpg-interface.c | 2 +-\n> > gpg-interface.h | 2 --\n> > 2 files changed, 1 insertion(+), 3 deletions(-)\n> >\n> > diff --git a/gpg-interface.c b/gpg-interface.c\n> > index 0647bd634..09ddfbc26 100644\n> > --- a/gpg-interface.c\n> > +++ b/gpg-interface.c\n> > @@ -35,7 +35,7 @@ static struct {\n> > \t{ 'R', \"\\n[GNUPG:] REVKEYSIG \"},\n> > };\n> > \n> > -void parse_gpg_output(struct signature_check *sigc)\n> > +static void parse_gpg_output(struct signature_check *sigc)\n> > {\n> > \tconst char *buf = sigc->gpg_status;\n> > \tint i;\n> > diff --git a/gpg-interface.h b/gpg-interface.h\n> > index a5e6517ae..5ecff4aa0 100644\n> > --- a/gpg-interface.h\n> > +++ b/gpg-interface.h\n> > @@ -33,8 +33,6 @@ void signature_check_clear(struct signature_check\n> > *sigc); */\n> > size_t parse_signature(const char *buf, size_t size);\n> > \n> > -void parse_gpg_output(struct signature_check *);\n> > -\n> > /*\n> > * Create a detached signature for the contents of \"buffer\" and\n> > append\n> > * it after \"signature\"; \"buffer\" and \"signature\" can be the same \n\n"},{"id":"352218","messageId":"20180711104719.1bf5d835@md1pvb1c.ad001.siemens.net","threadId":"48850","inReplyTo":"xmqqr2kb81zw.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 4/9] t/t7510: check the validation of the new config gpg.format","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-11T08:47:19Z","receivedAt":"2018-07-11T08:47:36Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Tue, 10 Jul 2018 09:54:59 -0700\nschrieb Junio C Hamano :\n\n> Henning Schild writes:\n> \n> > Test setting gpg.format to both invalid and valid values.\n> >\n> > Signed-off-by: Henning Schild \n> > ---\n> > t/t7510-signed-commit.sh | 10 ++++++++++\n> > 1 file changed, 10 insertions(+)\n> >\n> > diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\n> > index 6e2015ed9..7e1e9caf4 100755\n> > --- a/t/t7510-signed-commit.sh\n> > +++ b/t/t7510-signed-commit.sh\n> > @@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature\n> > behaves like --show-signature' ' grep \"gpg: Good signature\" actual\n> > '\n> > \n> > +test_expect_success GPG 'check config gpg.format values' '\n> > +\trm .git/config && \n> \n> Please don't. .git/config has stuff that are more important than\n> just collection of random configuration these days, and we can even\n> expect that future versions of Git may not store its config in a\n> flat file .git/config but in a different mechanism \"git config\"\n> command knows how to access. A low-level test for \"git config\"\n> command's operation may have to be implemented by inspecting the\n> resulting .git/config, but as this test is not about \"git config\"'s\n> inner workins but is about one feature \"git commit\" command has,\n> we prefer not to depend too much on the internal implementation\n> detail such as \"local config is stored in .git/config file\".\n> \n> Let's hear why you want to remove this file; what things that have\n> previously been placed in the file do you want not to see, before\n> performing the following actions? Once we know that, we can suggest\n> a way to do so better than removing the entire file.\n\nThat was a leftover of parsing the result and expecting the malformed\nvalue in a certain line. Now i do not look at result anymore. If the\nfirst two do succeed and the third fails it is probably fair enough to\nassume that the format \"malformed\" triggerd the config validation path.\n\nThat rm is gone from what will be v3.\n\nHenning\n\n> > +\ttest_config gpg.format openpgp &&\n> > +\tgit commit -S --amend -m \"success\" &&\n> > +\ttest_config gpg.format OpEnPgP &&\n> > +\tgit commit -S --amend -m \"success\" &&\n> > +\ttest_config gpg.format malformed &&\n> > +\ttest_must_fail git commit -S --amend -m \"fail\" 2>result\n> > +'\n> > +\n> > test_done \n\n"},{"id":"352219","messageId":"20180711105459.57ed99cc@md1pvb1c.ad001.siemens.net","threadId":"48850","inReplyTo":"20180710154931.GA23624@sigill.intra.peff.net","subject":"Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-11T08:54:59Z","receivedAt":"2018-07-11T08:55:26Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Tue, 10 Jul 2018 11:49:31 -0400\nschrieb Jeff King :\n\n> On Tue, Jul 10, 2018 at 10:52:28AM +0200, Henning Schild wrote:\n> \n> > gnupg does print the keyid followed by a space and the signer comes\n> > next. The same pattern is also used in gpgsm, but there the key\n> > length would be 40 instead of 16. Instead of hardcoding the\n> > expected length, find the first space and calculate it. \n> \n> Sounds good, but I think there's an off-by-one in the patch.\n> \n> > diff --git a/gpg-interface.c b/gpg-interface.c\n> > index 0a8d1bff3..ac2df498d 100644\n> > --- a/gpg-interface.c\n> > +++ b/gpg-interface.c\n> > @@ -88,10 +88,11 @@ static void parse_gpg_output(struct\n> > signature_check *sigc) sigc->result = sigcheck_gpg_status[i].result;\n> > \t\t/* The trust messages are not followed by\n> > key/signer information */ if (sigc->result != 'U') {\n> > -\t\t\tsigc->key = xmemdupz(found, 16);\n> > +\t\t\tnext = strchrnul(found, ' ');\n> > +\t\t\tsigc->key = xmemdupz(found, next -\n> > found); \n> \n> Here \"next\" may point to the trailing NUL of the string...\n> \n> > \t\t\t/* The ERRSIG message is not followed by\n> > signer information */ if (sigc-> result != 'E') {\n> > -\t\t\t\tfound += 17;\n> > +\t\t\t\tfound = next + 1;\n> > \t\t\t\tnext = strchrnul(found, '\\n'); \n> \n> ...in which case \"found\" points past the end of the string, and we\n> search random memory. That's presumably impossible with well-formed\n> gpg output (you don't get 'E' without an extra message), but we\n> should be robust against bogus input.\n> \n> In the general case you need:\n> \n> found = *next ? next + 1 : next;\n> \n> or similar. In this case, you can actually do:\n> \n> found = next;\n> \n> because we know that it's OK to search over the literal space again.\n> But that's pretty subtle, so we're probably better off just doing the\n> conditional above.\n> \n> (And yes, looking at the existing code, I think it's even worse, as\n> there does not seem to be a guarantee that we even have 16 characters\n> in the string).\n\nThe existing code works only on expected output and the same is true\nfor the version after this patch. Making the parser robust against\nrandom input would imho be a sort of cleanup patch on top of my\nseries. .. or before, in which case i would become responsible for\nmaking sure that still works after my modification.\nThis argument is twofold. I do not really want to fix that as well and\nit might be a good idea to separate concerns anyways.\n\nHenning\n\n> -Peff\n\n"},{"id":"352221","messageId":"20180711111829.2dc33ec2@md1pvb1c.ad001.siemens.net","threadId":"48850","inReplyTo":"20180710170109.GG23624@sigill.intra.peff.net","subject":"Re: [PATCH v2 8/9] gpg-interface: introduce new signature format \"x509\" using gpgsm","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-11T09:18:29Z","receivedAt":"2018-07-11T09:18:57Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Tue, 10 Jul 2018 13:01:10 -0400\nschrieb Jeff King :\n\n> On Tue, Jul 10, 2018 at 10:52:30AM +0200, Henning Schild wrote:\n> \n> > diff --git a/Documentation/config.txt b/Documentation/config.txt\n> > index c0bd80954..b6f9b47d5 100644\n> > --- a/Documentation/config.txt\n> > +++ b/Documentation/config.txt\n> > @@ -1830,7 +1830,7 @@ gpg.program::\n> > \n> > gpg.format::\n> > \tSpecifies which key format to use when signing with\n> > `--gpg-sign`.\n> > -\tDefault is \"openpgp\", that is also the only supported\n> > value.\n> > +\tDefault is \"opengpg\" and another possible value is\n> > \"x509\". \n> \n> opengpg?\n\nRight, thanks!\n\n> Since we're having so much fun with naming discussions, let's talk\n> about \"x509\". :)\n> \n> That's the cert format. I think of these signatures as S/MIME, but\n> really that's the mail-oriented parts of the standard. I think\n> technically this is \"CMS\".\n> \n> That said, we should pick what most people will find natural when\n> referring to it. So maybe x509 isn't the worst choice, as I doubt most\n> people know the term CMS. Probably the term they know _most_ is\n> \"gpgsm\", but I think the point is that one does not have to be using\n> gpgsm in the first place.\n\nOk, but now that you mention it, i will include the string \"gpgsm\" into\nDocumentation/config.txt somewhere. Maybe other documentation bits\ncould use hints that gpg is not the only kid in town anymore.\n\n> So I dunno. I think I talked myself back into x509. ;)\n\nOk, will stick to it.\n\nHenning\n\n> > diff --git a/gpg-interface.c b/gpg-interface.c\n> > index 65098430f..bf8d567a4 100644\n> > --- a/gpg-interface.c\n> > +++ b/gpg-interface.c\n> > @@ -16,13 +16,18 @@ struct gpg_format_data {\n> > \n> > #define PGP_SIGNATURE \"-----BEGIN PGP SIGNATURE-----\"\n> > #define PGP_MESSAGE \"-----BEGIN PGP MESSAGE-----\"\n> > +#define X509_SIGNATURE \"-----BEGIN SIGNED MESSAGE-----\"\n> > \n> > -enum gpgformats { PGP_FMT };\n> > +enum gpgformats { PGP_FMT, X509_FMT };\n> > struct gpg_format_data gpg_formats[] = {\n> > \t{ .format = \"openpgp\", .program = \"gpg\",\n> > \t .extra_args_verify = { \"--keyid-format=long\" },\n> > \t .sigs = { PGP_SIGNATURE, PGP_MESSAGE }\n> > \t},\n> > +\t{ .format = \"x509\", .program = \"gpgsm\",\n> > +\t .extra_args_verify = { NULL },\n> > +\t .sigs = { X509_SIGNATURE, NULL }\n> > +\t}, \n> \n> Extremely minor nit, but if there are no other uses of PGP_SIGNATURE\n> etc outside of this array (as I hope there wouldn't be after this\n> series), would it make more sense to just include the literals inline\n> in the array definition? That's one less layer of indirection when\n> somebody is reading the code.\n> \n> -Peff\n\n"},{"id":"352226","messageId":"20180711123824.7e0be91a@md1pvb1c.ad001.siemens.net","threadId":"48850","inReplyTo":"20180710170901.GH23624@sigill.intra.peff.net","subject":"Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-11T10:38:24Z","receivedAt":"2018-07-11T10:38:46Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Tue, 10 Jul 2018 13:09:01 -0400\nschrieb Jeff King :\n\n> On Tue, Jul 10, 2018 at 10:52:31AM +0200, Henning Schild wrote:\n> \n> > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\n> > index a5d3b2cba..9dcb4e990 100755\n> > --- a/t/lib-gpg.sh\n> > +++ b/t/lib-gpg.sh\n> > @@ -38,7 +38,14 @@ then\n> > \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n> > \t\tgpg --homedir \"${GNUPGHOME}\" /dev/null\n> > 2>&1 \\ --sign -u committer@example.com &&\n> > -\t\ttest_set_prereq GPG\n> > +\t\ttest_set_prereq GPG &&\n> > +\t\techo | gpgsm --homedir \"${GNUPGHOME}\" -o\n> > \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user --passphrase-fd 0\n> > --pinentry-mode loopback --generate-key --batch\n> > \"$TEST_DIRECTORY\"/lib-gpg/gpgsm-gen-key.in && \n> \n> Is this --generate-key going to require high-quality entropy? That\n> could slow the test suite down (and maybe even stall it quite a bit\n> on servers doing automated CI).\n\nYes, i also saw that getting \"stuck\" on one machine. But i blamed an\nexperimental kernel.\n\n> Can we save a dummy generated key and just import it? That's what we\n> do for the regular gpg case.\n\nI will look into storing a binary and leaving notes how it was\ngenerated, just like regular gpg does. The reason i did not do that in\nthe first place is that x509 certs have a validity and we introduce\ntime into the picture. But i will see if i can generate epoch->infinity\nto get the host clock or just the future out of the picture.\n\n> > +\t\tgpgsm --homedir \"${GNUPGHOME}\" --import\n> > \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user &&\n> > +\t\tgpgsm --homedir \"${GNUPGHOME}\" -K | grep\n> > fingerprint: | cut -d\" \" -f4 | tr -d '\\n' >\n> > ${GNUPGHOME}/trustlist.txt &&\n> > +\t\techo \" S relax\" >> ${GNUPGHOME}/trustlist.txt &&\n> > +\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n> > +\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" -u\n> > committer@example.com -o /dev/null --sign - 2>&1 &&\n> > +\t\ttest_set_prereq GPGSM \n> \n> This &&-chain means we can't have GPGSM without GPG. In theory the two\n> could be tested independently. I don't know if it's worth the trouble\n> to make that work, though. I wouldn't be surprised if there are some\n> subtle dependencies within the test scripts, and I'm not sure how\n> common it is for somebody to have gpgsm and not gpg. So it may make\n> sense to just punt on it until such a person appears.\n\nAs you found later, i already commented on that and do not think it is\nworth the effort. Users will be able to just have gpgsm, testers will\nneed gpg to test gpgsm.\n \n> > test_expect_success GPG 'log --graph --show-signature' '\n> > \tgit log --graph --show-signature -n1 signed >actual &&\n> > \tgrep \"^| gpg: Signature made\" actual &&\n> > \tgrep \"^| gpg: Good signature\" actual\n> > '\n> > \n> > +test_expect_success GPGSM 'log --graph --show-signature x509' '\n> > +\tgit log --graph --show-signature -n1 signed-x509 >actual &&\n> > +\tgrep \"^| gpgsm: Signature made\" actual &&\n> > +\tgrep \"^| gpgsm: Good signature\" actual\n> > +' \n> \n> We're going to have a lot of duplicated tests here. That's a\n> maintenance burden when one of them needs fixes later. And when new\n> tests are added, we won't automatically get them tested under each\n> format.\n> \n> Can we move the battery of tests into a function that takes a few\n> parameters (prereq name, branch to look at, etc) and then call it for\n> both the gpg/gpgsm cases?\n\nI guess this is part of the earlier \"allow GPGSM without GPG\" and i\ncan ignore it if we agree that this is not needed?\n\nHenning\n \n> -Peff\n\n"},{"id":"352227","messageId":"20180711123819.1e4880c3@md1pvb1c.ad001.siemens.net","threadId":"48850","inReplyTo":"xmqqlgai6bhi.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-11T10:38:19Z","receivedAt":"2018-07-11T10:38:48Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Tue, 10 Jul 2018 14:12:57 -0700\nschrieb Junio C Hamano :\n\n> Henning Schild writes:\n> \n> > Add test cases to cover the new X509/gpgsm support. Most of them\n> > resemble existing ones. They just switch the format to x509 and set\n> > the signingkey when creating signatures. Validation of signatures\n> > does not need any configuration of git, it does need gpgsm to be\n> > configured to trust the key(-chain).\n> > We generate a self-signed key for committer@example.com and\n> > configure gpgsm to trust it.\n> >\n> > Signed-off-by: Henning Schild \n> > ---\n> > t/lib-gpg.sh | 9 ++++++-\n> > t/lib-gpg/gpgsm-gen-key.in | 6 +++++\n> > t/t4202-log.sh | 66\n> > ++++++++++++++++++++++++++++++++++++++++++++++\n> > t/t5534-push-signed.sh | 52\n> > ++++++++++++++++++++++++++++++++++++ t/t7003-filter-branch.sh |\n> > 15 +++++++++++ t/t7030-verify-tag.sh | 47\n> > +++++++++++++++++++++++++++++++-- t/t7600-merge.sh | 31\n> > ++++++++++++++++++++++ 7 files changed, 223 insertions(+), 3\n> > deletions(-) create mode 100644 t/lib-gpg/gpgsm-gen-key.in \n> \n> I saw my post-integration tests taking forever to finish for 'pu'\n> and \"ps x\" output showed that many tests (I ran tests in parallel)\n> were stuck and all of them were running gpgsm. \n> \n> For now, I've ejected the topic out of 'pu', as I want to finish\n> today's integration run with all the other topics first before\n> coming back to this topic.\n\nThat must have been the key generation, see my reply to one of\nJeffs mails.\n\nHenning\n\n"},{"id":"352231","messageId":"20180711123425.GB23835@sigill.intra.peff.net","threadId":"48850","inReplyTo":"20180711105459.57ed99cc@md1pvb1c.ad001.siemens.net","subject":"Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-11T12:34:25Z","receivedAt":"2018-07-11T12:34:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 11, 2018 at 10:54:59AM +0200, Henning Schild wrote:\n\n> > In the general case you need:\n> > \n> > found = *next ? next + 1 : next;\n> > \n> > or similar. In this case, you can actually do:\n> > \n> > found = next;\n> > \n> > because we know that it's OK to search over the literal space again.\n> > But that's pretty subtle, so we're probably better off just doing the\n> > conditional above.\n> > \n> > (And yes, looking at the existing code, I think it's even worse, as\n> > there does not seem to be a guarantee that we even have 16 characters\n> > in the string).\n> \n> The existing code works only on expected output and the same is true\n> for the version after this patch. Making the parser robust against\n> random input would imho be a sort of cleanup patch on top of my\n> series. .. or before, in which case i would become responsible for\n> making sure that still works after my modification.\n> This argument is twofold. I do not really want to fix that as well and\n> it might be a good idea to separate concerns anyways.\n\nI think it's worth addressing in the near term, if only because this\nkind of off-by-one is quite subtle, and I don't want to forget to deal\nwith it. Whether that happens as part of this patch, or as a cleanup\nbefore or after, I'm not picky. :)\n\n-Peff\n"},{"id":"352236","messageId":"20180711125109.GC23835@sigill.intra.peff.net","threadId":"48850","inReplyTo":"20180711123824.7e0be91a@md1pvb1c.ad001.siemens.net","subject":"Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-11T12:51:10Z","receivedAt":"2018-07-11T12:51:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 11, 2018 at 12:38:24PM +0200, Henning Schild wrote:\n\n> > Can we save a dummy generated key and just import it? That's what we\n> > do for the regular gpg case.\n> \n> I will look into storing a binary and leaving notes how it was\n> generated, just like regular gpg does. The reason i did not do that in\n> the first place is that x509 certs have a validity and we introduce\n> time into the picture. But i will see if i can generate epoch->infinity\n> to get the host clock or just the future out of the picture.\n\nThat would be preferable. But even if we can just get something like a\n10-year expiration, that may be enough. Somebody dealing with failing\ntests and regenerating keys in ten years is probably not the end of the\nworld.\n\nIt could hurt people with drastically incorrect system clocks, but I\nsuspect there are other tests with similar problems (especially if your\nclock is in the past).\n\n> > We're going to have a lot of duplicated tests here. That's a\n> > maintenance burden when one of them needs fixes later. And when new\n> > tests are added, we won't automatically get them tested under each\n> > format.\n> > \n> > Can we move the battery of tests into a function that takes a few\n> > parameters (prereq name, branch to look at, etc) and then call it for\n> > both the gpg/gpgsm cases?\n> \n> I guess this is part of the earlier \"allow GPGSM without GPG\" and i\n> can ignore it if we agree that this is not needed?\n\nI think it's orthogonal. Even if GPGSM requires GPG, you'd still want to\nmake sure that whatever exercise we give to the GPG code is also\nexercised using GPGSM.\n\nI will note, though, that _some_ tests are not really exercising\ngpg-specific bits, but more how we react to it (e.g., how we format %G\nplaceholders). And it's probably OK for those to just be run once.\n\nSo in an ideal world, the test script would probably look something\nlike:\n\n # this function holds tests which exercise the interactions\n # with the gpg binary itself\n type_specific_tests() {\n\tprereq=$1\n\tbranch=$2\n\n\ttest_expect_success $prereq \"test whatever ($prereq)\" '\n\t\tsome test using $branch here\n\t'\n }\n\n test_expect_success 'setup' '\n\tset up both openpgp and x509 branches here\n '\n\n type_specific_tests GPG openpgp\n type_specific_tests GPGSM x509\n\n # and now tests that generically care about getting _some_ signature\n # result (e.g., the way we format signature info)\n\n # and then probably a few tests specific to how the config is handled,\n # like your new gpg.format coverage\n\nBut in practice, the type-specific bits are often muddled together with\nthe type-independent ones (e.g., we happen to test the parsing of a\nfailed signature from gpg by checking how %G? is formatted or similar).\n\nSo it may be simplest to just run most of the tests twice, once with gpg\nand once with gpgsm. I kind of wonder if all of t7510 could just be\nbumped into a function. Or even into a sourced file and run from two\ndifferent scripts. See the way that t8001 and t8002 use\nannotate-tests.sh for an example.\n\n-Peff\n"},{"id":"352243","messageId":"20180711154019.202e75c5@md1pvb1c.ad001.siemens.net","threadId":"48850","inReplyTo":"20180711125109.GC23835@sigill.intra.peff.net","subject":"Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-11T13:40:19Z","receivedAt":"2018-07-11T13:40:47Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Wed, 11 Jul 2018 08:51:10 -0400\nschrieb Jeff King :\n\n> On Wed, Jul 11, 2018 at 12:38:24PM +0200, Henning Schild wrote:\n> \n> > > Can we save a dummy generated key and just import it? That's what\n> > > we do for the regular gpg case. \n> > \n> > I will look into storing a binary and leaving notes how it was\n> > generated, just like regular gpg does. The reason i did not do that\n> > in the first place is that x509 certs have a validity and we\n> > introduce time into the picture. But i will see if i can generate\n> > epoch->infinity to get the host clock or just the future out of the\n> > picture. \n> \n> That would be preferable. But even if we can just get something like a\n> 10-year expiration, that may be enough. Somebody dealing with failing\n> tests and regenerating keys in ten years is probably not the end of\n> the world.\n> \n> It could hurt people with drastically incorrect system clocks, but I\n> suspect there are other tests with similar problems (especially if\n> your clock is in the past).\n\nI now have a working version with a pregenerated certificate from\n1970-3000. I would be surprised if git is still around at that time ;).\n\n> > > We're going to have a lot of duplicated tests here. That's a\n> > > maintenance burden when one of them needs fixes later. And when\n> > > new tests are added, we won't automatically get them tested under\n> > > each format.\n> > > \n> > > Can we move the battery of tests into a function that takes a few\n> > > parameters (prereq name, branch to look at, etc) and then call it\n> > > for both the gpg/gpgsm cases? \n> > \n> > I guess this is part of the earlier \"allow GPGSM without GPG\" and i\n> > can ignore it if we agree that this is not needed? \n> \n> I think it's orthogonal. Even if GPGSM requires GPG, you'd still want\n> to make sure that whatever exercise we give to the GPG code is also\n> exercised using GPGSM.\n> \n> I will note, though, that _some_ tests are not really exercising\n> gpg-specific bits, but more how we react to it (e.g., how we format %G\n> placeholders). And it's probably OK for those to just be run once.\n> \n> So in an ideal world, the test script would probably look something\n> like:\n> \n> # this function holds tests which exercise the interactions\n> # with the gpg binary itself\n> type_specific_tests() {\n> \tprereq=$1\n> \tbranch=$2\n> \n> \ttest_expect_success $prereq \"test whatever ($prereq)\" '\n> \t\tsome test using $branch here\n> \t'\n> }\n> \n> test_expect_success 'setup' '\n> \tset up both openpgp and x509 branches here\n> '\n> \n> type_specific_tests GPG openpgp\n> type_specific_tests GPGSM x509\n> \n> # and now tests that generically care about getting _some_ signature\n> # result (e.g., the way we format signature info)\n> \n> # and then probably a few tests specific to how the config is\n> handled, # like your new gpg.format coverage\n> \n> But in practice, the type-specific bits are often muddled together\n> with the type-independent ones (e.g., we happen to test the parsing\n> of a failed signature from gpg by checking how %G? is formatted or\n> similar).\n> \n> So it may be simplest to just run most of the tests twice, once with\n> gpg and once with gpgsm. I kind of wonder if all of t7510 could just\n> be bumped into a function. Or even into a sourced file and run from\n> two different scripts. See the way that t8001 and t8002 use\n> annotate-tests.sh for an example.\n\nI do not agree and would like to leave the tests as they are. Instead\nof introducing a whole lot of very similar copies, i added just a few.\nThe original ones are even very similar between each other.\nWe are again talking about two problems. 1. we need test cases for\ngpgsm if we want to merge gpgsm 2. the testsuite is very repetitive\n\nWhile addressing 1 make 2 obvious and worse, addressing 2 is a whole\ndifferent story and should probably be discussed outside of this\nthread. And i would not like to inherit responsibility for 2. In\nfact the whole discussion emphasizes that it was a good idea to make\nGPGSM depend on GPG, because it allows to somewhat reuse existing tests.\n\nHenning\n\n> -Peff\n\n"},{"id":"352244","messageId":"20180711154619.7b574dff@md1pvb1c.ad001.siemens.net","threadId":"48850","inReplyTo":"20180711123425.GB23835@sigill.intra.peff.net","subject":"Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-11T13:46:19Z","receivedAt":"2018-07-11T13:46:42Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Wed, 11 Jul 2018 08:34:25 -0400\nschrieb Jeff King :\n\n> On Wed, Jul 11, 2018 at 10:54:59AM +0200, Henning Schild wrote:\n> \n> > > In the general case you need:\n> > > \n> > > found = *next ? next + 1 : next;\n> > > \n> > > or similar. In this case, you can actually do:\n> > > \n> > > found = next;\n> > > \n> > > because we know that it's OK to search over the literal space\n> > > again. But that's pretty subtle, so we're probably better off\n> > > just doing the conditional above.\n> > > \n> > > (And yes, looking at the existing code, I think it's even worse,\n> > > as there does not seem to be a guarantee that we even have 16\n> > > characters in the string). \n> > \n> > The existing code works only on expected output and the same is true\n> > for the version after this patch. Making the parser robust against\n> > random input would imho be a sort of cleanup patch on top of my\n> > series. .. or before, in which case i would become responsible for\n> > making sure that still works after my modification.\n> > This argument is twofold. I do not really want to fix that as well\n> > and it might be a good idea to separate concerns anyways. \n> \n> I think it's worth addressing in the near term, if only because this\n> kind of off-by-one is quite subtle, and I don't want to forget to deal\n> with it. Whether that happens as part of this patch, or as a cleanup\n> before or after, I'm not picky. :)\n\nI get that and if anyone is willing to write that code, i will base my\npatches on it. What i want to avoid is taking responsibility for\nproblems i did not introduce, just because i happen to work on that\ncode at the moment. Keeping track of that (not forgetting) is also not\nfor the random contributor like myself.\n\nHenning\n\n> -Peff\n\n"},{"id":"352247","messageId":"20180711142751.GF23835@sigill.intra.peff.net","threadId":"48850","inReplyTo":"20180711154619.7b574dff@md1pvb1c.ad001.siemens.net","subject":"Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-11T14:27:52Z","receivedAt":"2018-07-11T14:27:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 11, 2018 at 03:46:19PM +0200, Henning Schild wrote:\n\n> > I think it's worth addressing in the near term, if only because this\n> > kind of off-by-one is quite subtle, and I don't want to forget to deal\n> > with it. Whether that happens as part of this patch, or as a cleanup\n> > before or after, I'm not picky. :)\n> \n> I get that and if anyone is willing to write that code, i will base my\n> patches on it. What i want to avoid is taking responsibility for\n> problems i did not introduce, just because i happen to work on that\n> code at the moment. Keeping track of that (not forgetting) is also not\n> for the random contributor like myself.\n\nIt doesn't make sense to do a patch before your series, since it would\njust be:\n\n if (strlen(found) > 16)\n ...\n\nwhich would get obliterated by your patch. The patch after is shown\nbelow. But frankly, it seems a lot easier to just handle this while you\nare rewriting the code.\n\n-- >8 --\nSubject: [PATCH] gpg-interface: handle off-by-one parsing gpg output\n\nWhen parsing gpg's VALIDSIG lines, we look for a space\nfollowed by the signer information. Because we use\nstrchrnul(), though, if the space is missing we'll end up\npointing to the trailing NUL. When we try to move past that\nspace, we have to handle the NUL case separately to avoid\naccidentally stepping out of the string entirely.\n\nSigned-off-by: Jeff King \n---\n gpg-interface.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex bf8d567a4c..139b0f561e 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -97,7 +97,7 @@ static void parse_gpg_output(struct signature_check *sigc)\n \t\t\tsigc->key = xmemdupz(found, next - found);\n \t\t\t/* The ERRSIG message is not followed by signer information */\n \t\t\tif (sigc-> result != 'E') {\n-\t\t\t\tfound = next + 1;\n+\t\t\t\tfound = *next ? next + 1 : next;\n \t\t\t\tnext = strchrnul(found, '\\n');\n \t\t\t\tsigc->signer = xmemdupz(found, next - found);\n \t\t\t}\n-- \n2.18.0.400.g702e398724\n\n"},{"id":"352248","messageId":"20180711143352.GA14600@sigill.intra.peff.net","threadId":"48850","inReplyTo":"f97d2d79f6a46ddffcd0065239f99b084708e813.1531208187.git.henning.schild@siemens.com","subject":"Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-11T14:33:52Z","receivedAt":"2018-07-11T14:33:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2018 at 10:52:31AM +0200, Henning Schild wrote:\n\n> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\n> index a5d3b2cba..9dcb4e990 100755\n> --- a/t/lib-gpg.sh\n> +++ b/t/lib-gpg.sh\n> @@ -38,7 +38,14 @@ then\n> \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n> \t\tgpg --homedir \"${GNUPGHOME}\" /dev/null 2>&1 \\\n> \t\t\t--sign -u committer@example.com &&\n> -\t\ttest_set_prereq GPG\n> +\t\ttest_set_prereq GPG &&\n> +\t\techo | gpgsm --homedir \"${GNUPGHOME}\" -o \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user --passphrase-fd 0 --pinentry-mode loopback --generate-key --batch \"$TEST_DIRECTORY\"/lib-gpg/gpgsm-gen-key.in &&\n> +\t\tgpgsm --homedir \"${GNUPGHOME}\" --import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user &&\n> +\t\tgpgsm --homedir \"${GNUPGHOME}\" -K | grep fingerprint: | cut -d\" \" -f4 | tr -d '\\n' > ${GNUPGHOME}/trustlist.txt &&\n> +\t\techo \" S relax\" >> ${GNUPGHOME}/trustlist.txt &&\n> +\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n> +\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" -u committer@example.com -o /dev/null --sign - 2>&1 &&\n> +\t\ttest_set_prereq GPGSM\n\nBy the way, I think these gpgsm invocations need some redirection of\ntheir stderr. This isn't inside a test_expect block, so they spew to the\nterminal even in non-verbose mode.\n\nIdeally they'd redirect to descriptor 4, which then respects \"-v\"\nproperly (though the existing gpg invocations just go to 2).\n\n-Peff\n"},{"id":"352249","messageId":"20180711143554.GG23835@sigill.intra.peff.net","threadId":"48850","inReplyTo":"20180711154019.202e75c5@md1pvb1c.ad001.siemens.net","subject":"Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-11T14:35:54Z","receivedAt":"2018-07-11T14:35:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 11, 2018 at 03:40:19PM +0200, Henning Schild wrote:\n\n> > So it may be simplest to just run most of the tests twice, once with\n> > gpg and once with gpgsm. I kind of wonder if all of t7510 could just\n> > be bumped into a function. Or even into a sourced file and run from\n> > two different scripts. See the way that t8001 and t8002 use\n> > annotate-tests.sh for an example.\n> \n> I do not agree and would like to leave the tests as they are. Instead\n> of introducing a whole lot of very similar copies, i added just a few.\n\nI'm not sure I understand why you added the ones you did, though. For\ninstance, \"--no-show-signature overrides --show-signature x509\" seems\nlike it has nothing to do with the gpg/gpgsm distinction.\n\nSo I'd have expected that to be _outside_ of the shared battery of\ntests.\n\n> The original ones are even very similar between each other.\n> We are again talking about two problems. 1. we need test cases for\n> gpgsm if we want to merge gpgsm 2. the testsuite is very repetitive\n> \n> While addressing 1 make 2 obvious and worse, addressing 2 is a whole\n> different story and should probably be discussed outside of this\n> thread. And i would not like to inherit responsibility for 2. In\n> fact the whole discussion emphasizes that it was a good idea to make\n> GPGSM depend on GPG, because it allows to somewhat reuse existing tests.\n\nIMHO there is a big difference between inheriting responsibility for\nsomething, and not making it worse. But I'm not all that interested in\nfighting about it.\n\n-Peff\n"},{"id":"352262","messageId":"20180711174846.12618aea@md1pvb1c.ad001.siemens.net","threadId":"48850","inReplyTo":"20180711143554.GG23835@sigill.intra.peff.net","subject":"Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-11T15:48:46Z","receivedAt":"2018-07-11T15:49:15Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Wed, 11 Jul 2018 10:35:54 -0400\nschrieb Jeff King :\n\n> On Wed, Jul 11, 2018 at 03:40:19PM +0200, Henning Schild wrote:\n> \n> > > So it may be simplest to just run most of the tests twice, once\n> > > with gpg and once with gpgsm. I kind of wonder if all of t7510\n> > > could just be bumped into a function. Or even into a sourced file\n> > > and run from two different scripts. See the way that t8001 and\n> > > t8002 use annotate-tests.sh for an example. \n> > \n> > I do not agree and would like to leave the tests as they are.\n> > Instead of introducing a whole lot of very similar copies, i added\n> > just a few. \n> \n> I'm not sure I understand why you added the ones you did, though. For\n> instance, \"--no-show-signature overrides --show-signature x509\" seems\n> like it has nothing to do with the gpg/gpgsm distinction.\n> \n> So I'd have expected that to be _outside_ of the shared battery of\n> tests.\n\nTrue, it took my quite some time to figure out a way to configure gpgsm\nnon-interactively. Generate the key etc. without even a single popup of\nthe gpg-agent... After that i just added random tests to create\n\"coverage\", without much focus. I would be happy to revisit that and\ndrop test cases, and add some that are missing.\n\nHenning\n\n> > The original ones are even very similar between each other.\n> > We are again talking about two problems. 1. we need test cases for\n> > gpgsm if we want to merge gpgsm 2. the testsuite is very repetitive\n> > \n> > While addressing 1 make 2 obvious and worse, addressing 2 is a whole\n> > different story and should probably be discussed outside of this\n> > thread. And i would not like to inherit responsibility for 2. In\n> > fact the whole discussion emphasizes that it was a good idea to make\n> > GPGSM depend on GPG, because it allows to somewhat reuse existing\n> > tests. \n> \n> IMHO there is a big difference between inheriting responsibility for\n> something, and not making it worse. But I'm not all that interested in\n> fighting about it.\n>\n> -Peff\n\n"},{"id":"352265","messageId":"20180711181505.31ea530b@md1pvb1c.ad001.siemens.net","threadId":"48850","inReplyTo":"20180711142751.GF23835@sigill.intra.peff.net","subject":"Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-11T16:15:05Z","receivedAt":"2018-07-11T16:15:28Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Wed, 11 Jul 2018 10:27:52 -0400\nschrieb Jeff King :\n\n> On Wed, Jul 11, 2018 at 03:46:19PM +0200, Henning Schild wrote:\n> \n> > > I think it's worth addressing in the near term, if only because\n> > > this kind of off-by-one is quite subtle, and I don't want to\n> > > forget to deal with it. Whether that happens as part of this\n> > > patch, or as a cleanup before or after, I'm not picky. :) \n> > \n> > I get that and if anyone is willing to write that code, i will base\n> > my patches on it. What i want to avoid is taking responsibility for\n> > problems i did not introduce, just because i happen to work on that\n> > code at the moment. Keeping track of that (not forgetting) is also\n> > not for the random contributor like myself. \n> \n> It doesn't make sense to do a patch before your series, since it would\n> just be:\n> \n> if (strlen(found) > 16)\n> ...\n\nInstead of randomly crashing on unexpected input, we would now silently\nignore it.\n\n> which would get obliterated by your patch. The patch after is shown\n> below. But frankly, it seems a lot easier to just handle this while\n> you are rewriting the code.\n> \n> -- >8 -- \n> Subject: [PATCH] gpg-interface: handle off-by-one parsing gpg output\n> \n> When parsing gpg's VALIDSIG lines, we look for a space\n> followed by the signer information. Because we use\n> strchrnul(), though, if the space is missing we'll end up\n> pointing to the trailing NUL. When we try to move past that\n> space, we have to handle the NUL case separately to avoid\n> accidentally stepping out of the string entirely.\n\nTrue.\n\n> Signed-off-by: Jeff King \n> ---\n> gpg-interface.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index bf8d567a4c..139b0f561e 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -97,7 +97,7 @@ static void parse_gpg_output(struct signature_check\n> *sigc) sigc->key = xmemdupz(found, next - found);\n> \t\t\t/* The ERRSIG message is not followed by\n> signer information */ if (sigc-> result != 'E') {\n> -\t\t\t\tfound = next + 1;\n> +\t\t\t\tfound = *next ? next + 1 : next;\n\nThis would keep us in bounds of the unexpected string. But ignore the\nline instead of \"complaining\" or crashing.\n\nBut you are right, it is easy enough and ignoring the line is probably\nthe best way of dealing with it.\n\ni will change the condition to\n> if (*next && sigc-> result != 'E')\n\nalso skipping the following strchrnul and xmemdupz\n\nHenning\n\n> \t\t\t\tnext = strchrnul(found, '\\n');\n> \t\t\t\tsigc->signer = xmemdupz(found, next\n> - found); }\n\n"},{"id":"352266","messageId":"xmqqa7qx4u2k.fsf@gitster-ct.c.googlers.com","threadId":"48850","inReplyTo":"20180711143554.GG23835@sigill.intra.peff.net","subject":"Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-11T16:26:43Z","receivedAt":"2018-07-11T16:26:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> While addressing 1 make 2 obvious and worse, addressing 2 is a whole\n>> different story and should probably be discussed outside of this\n>> thread. And i would not like to inherit responsibility for 2. In\n>> fact the whole discussion emphasizes that it was a good idea to make\n>> GPGSM depend on GPG, because it allows to somewhat reuse existing tests.\n>\n> IMHO there is a big difference between inheriting responsibility for\n> something, and not making it worse.\n\nWell said.\n"},{"id":"352269","messageId":"20180711183554.308bee82@md1pvb1c.ad001.siemens.net","threadId":"48850","inReplyTo":"20180711143352.GA14600@sigill.intra.peff.net","subject":"Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-11T16:35:54Z","receivedAt":"2018-07-11T16:36:16Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Wed, 11 Jul 2018 10:33:52 -0400\nschrieb Jeff King :\n\n> On Tue, Jul 10, 2018 at 10:52:31AM +0200, Henning Schild wrote:\n> \n> > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\n> > index a5d3b2cba..9dcb4e990 100755\n> > --- a/t/lib-gpg.sh\n> > +++ b/t/lib-gpg.sh\n> > @@ -38,7 +38,14 @@ then\n> > \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n> > \t\tgpg --homedir \"${GNUPGHOME}\" /dev/null\n> > 2>&1 \\ --sign -u committer@example.com &&\n> > -\t\ttest_set_prereq GPG\n> > +\t\ttest_set_prereq GPG &&\n> > +\t\techo | gpgsm --homedir \"${GNUPGHOME}\" -o\n> > \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user --passphrase-fd 0\n> > --pinentry-mode loopback --generate-key --batch\n> > \"$TEST_DIRECTORY\"/lib-gpg/gpgsm-gen-key.in &&\n> > +\t\tgpgsm --homedir \"${GNUPGHOME}\" --import\n> > \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user &&\n> > +\t\tgpgsm --homedir \"${GNUPGHOME}\" -K | grep\n> > fingerprint: | cut -d\" \" -f4 | tr -d '\\n' >\n> > ${GNUPGHOME}/trustlist.txt &&\n> > +\t\techo \" S relax\" >> ${GNUPGHOME}/trustlist.txt &&\n> > +\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n> > +\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" -u\n> > committer@example.com -o /dev/null --sign - 2>&1 &&\n> > +\t\ttest_set_prereq GPGSM \n> \n> By the way, I think these gpgsm invocations need some redirection of\n> their stderr. This isn't inside a test_expect block, so they spew to\n> the terminal even in non-verbose mode.\n> \n> Ideally they'd redirect to descriptor 4, which then respects \"-v\"\n> properly (though the existing gpg invocations just go to 2).\n\nThanks for pointing that out. I decided to go for 2 as well, for\nconsistency with gpg and not having to change that.\n\nHenning\n\n> -Peff\n\n"},{"id":"352270","messageId":"20180711163851.GA20988@sigill.intra.peff.net","threadId":"48850","inReplyTo":"20180711181505.31ea530b@md1pvb1c.ad001.siemens.net","subject":"Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-11T16:38:51Z","receivedAt":"2018-07-11T16:38:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 11, 2018 at 06:15:05PM +0200, Henning Schild wrote:\n\n> > diff --git a/gpg-interface.c b/gpg-interface.c\n> > index bf8d567a4c..139b0f561e 100644\n> > --- a/gpg-interface.c\n> > +++ b/gpg-interface.c\n> > @@ -97,7 +97,7 @@ static void parse_gpg_output(struct signature_check\n> > *sigc) sigc->key = xmemdupz(found, next - found);\n> > \t\t\t/* The ERRSIG message is not followed by\n> > signer information */ if (sigc-> result != 'E') {\n> > -\t\t\t\tfound = next + 1;\n> > +\t\t\t\tfound = *next ? next + 1 : next;\n> \n> This would keep us in bounds of the unexpected string. But ignore the\n> line instead of \"complaining\" or crashing.\n> \n> But you are right, it is easy enough and ignoring the line is probably\n> the best way of dealing with it.\n> \n> i will change the condition to\n> > if (*next && sigc-> result != 'E')\n> \n> also skipping the following strchrnul and xmemdupz\n\nThat sounds good to me. Thanks.\n\n-Peff\n"},{"id":"352453","messageId":"20180713104118.4eafa3cc@md1pvb1c.ad001.siemens.net","threadId":"48850","inReplyTo":"xmqqlgaj80zs.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 5/9] gpg-interface: introduce an abstraction for multiple gpg formats","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-13T08:41:18Z","receivedAt":"2018-07-13T08:41:46Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Tue, 10 Jul 2018 10:16:39 -0700\nschrieb Junio C Hamano :\n\n> Henning Schild writes:\n> \n> > Create a struct that holds the format details for the supported\n> > formats. At the moment that is still just \"openpgp\". This commit\n> > prepares for the introduction of more formats, that might use other\n> > programs and match other signatures.\n> >\n> > Signed-off-by: Henning Schild \n> > ---\n> > gpg-interface.c | 74\n> > ++++++++++++++++++++++++++++++++++++++++++++------------- 1 file\n> > changed, 58 insertions(+), 16 deletions(-)\n> >\n> > diff --git a/gpg-interface.c b/gpg-interface.c\n> > index ed0e55917..0a8d1bff3 100644\n> > --- a/gpg-interface.c\n> > +++ b/gpg-interface.c\n> > @@ -7,12 +7,46 @@\n> > #include \"tempfile.h\"\n> > \n> > static char *configured_signing_key;\n> > -static const char *gpg_format = \"openpgp\";\n> > -static const char *gpg_program = \"gpg\";\n> > +struct gpg_format_data {\n> > +\tconst char *format;\n> > +\tconst char *program;\n> > +\tconst char *extra_args_verify[1];\n> > +\tconst char *sigs[2];\n> > +}; \n> \n> Do you use identifier \"gpg_format\" elsewhere as a structure name? A\n> structure is there always to hold \"data\", so often \"_data\" suffix is\n> meaningless in a structure type name like this.\n\ni renamed the struct to \"gpg_format\" and what was \"gpg_format\" before,\na const char* holding the format we use for signing, is now a pointer\ninto gpg_formats and called \"current_format\"\n \n> > #define PGP_SIGNATURE \"-----BEGIN PGP SIGNATURE-----\"\n> > #define PGP_MESSAGE \"-----BEGIN PGP MESSAGE-----\"\n> > \n> > +enum gpgformats { PGP_FMT };\n> > +struct gpg_format_data gpg_formats[] = {\n> > +\t{ .format = \"openpgp\", .program = \"gpg\",\n> > +\t .extra_args_verify = { \"--keyid-format=long\" },\n> > +\t .sigs = { PGP_SIGNATURE, PGP_MESSAGE }\n> > +\t},\n> > +};\n> > +static const char *gpg_format = \"openpgp\";\n> > +\n> > +static struct gpg_format_data *get_format_data(const char *str) \n> \n> get_format_data_by_name() or something like that, for consistency\n> with the next function, perhaps?\n\nI went for get_format_by_name and also renamed \"format\" inside the\nstruct to be \"name\"\n\n> > +{\n> > +\tint i;\n> > +\tfor (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n> > +\t\tif (!strcasecmp(gpg_formats[i].format, str))\n> > +\t\t\treturn gpg_formats + i;\n> > +\treturn NULL;\n> > +}\n> > +\n> > +static struct gpg_format_data *get_format_data_by_sig(const char\n> > *sig) +{\n> > +\tint i, j;\n> > +\tfor (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n> > +\t\tfor (j = 0; j < ARRAY_SIZE(gpg_formats[i].sigs);\n> > j++)\n> > +\t\t\tif (gpg_formats[i].sigs[j] && \n> > +\t\t\t !strncmp(gpg_formats[i].sigs[j], sig,\n> > +\n> > strlen(gpg_formats[i].sigs[j])))\n> > +\t\t\t\treturn gpg_formats + i;\n> > +\treturn NULL;\n> > +}\n> > +\n> > void signature_check_clear(struct signature_check *sigc)\n> > {\n> > \tFREE_AND_NULL(sigc->payload);\n> > @@ -104,8 +138,7 @@ void print_signature_buffer(const struct\n> > signature_check *sigc, unsigned flags) \n> > static int is_gpg_start(const char *line)\n> > {\n> > -\treturn starts_with(line, PGP_SIGNATURE) ||\n> > -\t\tstarts_with(line, PGP_MESSAGE);\n> > +\treturn (get_format_data_by_sig(line) != NULL); \n> \n> If this is still called \"is_gpg_start()\", then shouldn't the\n> implementation be more like this?\n> \n> \tstruct gpg_format *found = get_format_data_by_sig(line);\n> \n> \treturn found && found == &gpg_formats[PGP_FMT];\n> \n> It probably does not make much difference in the end, as you won't\n> have functions is_(gpg|gpgsm|somethingelse|yetanother)_start() but\n> either have a single function \"does some x509 looking block start\n> here?\" or \"I am expecting gpg and nothing else, does a block for\n> that start here?\" and at that point this will no longer be called\n> is_gpg_start(), I would expect.\n\nI dropped the whole thing since it is now a one-liner and only called\non one place.\n\n> > size_t parse_signature(const char *buf, size_t size)\n> > @@ -140,18 +173,14 @@ int git_gpg_config(const char *var, const\n> > char *value, void *cb) }\n> > \n> > \tif (!strcmp(var, \"gpg.format\")) {\n> > -\t\tif (strcasecmp(value, \"openpgp\"))\n> > +\t\tif (!get_format_data(value))\n> > \t\t\treturn error(\"malformed value for %s: %s\",\n> > var, value); return git_config_string(&gpg_format, var, value); \n> \n> This is a bug introduced in an earlier step in the series, but you\n> will segfault when given a configuration that looks like this:\n> \n> \t[gpg]\n> \t\tformat\n> \n> Imitate the way how !value is diagnosed upfront for \"gpg.program\"\n> below before this patch.\n\nThe next version will take care of this.\n\n> > \t}\n> > \n> > -\tif (!strcmp(var, \"gpg.program\")) {\n> > -\t\tif (!value)\n> > -\t\t\treturn config_error_nonbool(var);\n> > -\t\tgpg_program = xstrdup(value);\n> > -\t\treturn 0;\n> > -\t}\n> > -\n> > +\tif (!strcmp(var, \"gpg.program\"))\n> > +\t\treturn\n> > git_config_string(&gpg_formats[PGP_FMT].program, var,\n> > +\t\t\t\t\t value); \n> \n> git_config_string() has its own \"do not feed boolean 'true' to me\"\n> check, so this change does not introduce a regression.\n> \n> > @@ -165,12 +194,16 @@ const char *get_signing_key(void)\n> > int sign_buffer(struct strbuf *buffer, struct strbuf *signature,\n> > const char *signing_key) {\n> > \tstruct child_process gpg = CHILD_PROCESS_INIT;\n> > +\tstruct gpg_format_data *fmt;\n> > \tint ret;\n> > \tsize_t i, j, bottom;\n> > \tstruct strbuf gpg_status = STRBUF_INIT;\n> > \n> > +\tfmt = get_format_data(gpg_format);\n> > +\tif (!fmt)\n> > +\t\tBUG(\"bad gpg_format '%s'\", gpg_format);\n> > \targv_array_pushl(&gpg.args,\n> > -\t\t\t gpg_program,\n> > +\t\t\t fmt->program,\n> > \t\t\t \"--status-fd=2\",\n> > \t\t\t \"-bsau\", signing_key,\n> > \t\t\t NULL);\n> > @@ -208,8 +241,9 @@ int verify_signed_buffer(const char *payload,\n> > size_t payload_size, struct strbuf *gpg_output, struct strbuf\n> > *gpg_status) {\n> > \tstruct child_process gpg = CHILD_PROCESS_INIT;\n> > +\tstruct gpg_format_data *fmt;\n> > \tstruct tempfile *temp;\n> > -\tint ret;\n> > +\tint ret, i;\n> > \tstruct strbuf buf = STRBUF_INIT;\n> > \n> > \ttemp = mks_tempfile_t(\".git_vtag_tmpXXXXXX\");\n> > @@ -223,10 +257,18 @@ int verify_signed_buffer(const char *payload,\n> > size_t payload_size, return -1;\n> > \t}\n> > \n> > +\tfmt = get_format_data_by_sig(signature);\n> > +\tassert(fmt);\n> > +\n> > +\targv_array_pushl(&gpg.args,\n> > +\t\t\t fmt->program, NULL);\n> > +\tfor (i = 0; i < ARRAY_SIZE(fmt->extra_args_verify); i++)\n> > +\t\tif (fmt->extra_args_verify[i])\n> > +\t\t\targv_array_pushl(&gpg.args,\n> > +\n> > fmt->extra_args_verify[i], NULL); \n> \n> This loop allows us to extend .extra_args_verify when we need to\n> support another variant that needs to take two arguments by making\n> .extra_args field larger for all variants and stuffing a NULL to\n> unused slots, and it even allows a nonsense like this\n> \n> \t.extra_args_verify = { NULL, \"--keyid-format=long\" },\n> \n> I cannot quite shake the feeling that .extra_args_verify and .sigs\n> fields should not be of fixed sized array of strings, but rather be\n> of type \"const char **\" that is NULL terminated. Such a clean-up\n> will make it harder to write initializers, so it may not be worth\n> it, I guess.\n\nI now have NULL terminated variable size arrays.\n\nThanks,\nHenning\n \n> > \targv_array_pushl(&gpg.args,\n> > -\t\t\t gpg_program,\n> > \t\t\t \"--status-fd=1\",\n> > -\t\t\t \"--keyid-format=long\",\n> > \t\t\t \"--verify\", temp->filename.buf, \"-\",\n> > \t\t\t NULL); \n\n"},{"id":"352455","messageId":"20180713104126.06017661@md1pvb1c.ad001.siemens.net","threadId":"48850","inReplyTo":"20180710162332.GD23624@sigill.intra.peff.net","subject":"Re: [PATCH v2 5/9] gpg-interface: introduce an abstraction for multiple gpg formats","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-13T08:41:26Z","receivedAt":"2018-07-13T08:41:47Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Am Tue, 10 Jul 2018 12:23:32 -0400\nschrieb Jeff King :\n\n> On Tue, Jul 10, 2018 at 10:52:27AM +0200, Henning Schild wrote:\n> \n> > Create a struct that holds the format details for the supported\n> > formats. At the moment that is still just \"openpgp\". This commit\n> > prepares for the introduction of more formats, that might use other\n> > programs and match other signatures. \n> \n> Great, this looks like a good incremental step.\n> \n> > static char *configured_signing_key;\n> > -static const char *gpg_format = \"openpgp\";\n> > -static const char *gpg_program = \"gpg\";\n> > +struct gpg_format_data {\n> > +\tconst char *format;\n> > +\tconst char *program;\n> > +\tconst char *extra_args_verify[1];\n> > +\tconst char *sigs[2];\n> > +}; \n> \n> These magic numbers are at a weird distance from where we fill them\n> in:\n> \n> > +struct gpg_format_data gpg_formats[] = {\n> > +\t{ .format = \"openpgp\", .program = \"gpg\",\n> > +\t .extra_args_verify = { \"--keyid-format=long\" },\n> > +\t .sigs = { PGP_SIGNATURE, PGP_MESSAGE }\n> > +\t},\n> > +}; \n> \n> I'm not sure if we can easily do any better in C, though. Declaring\n> the struct with an open-ended \"[]\" would make the compiler unhappy.\n> We could do something like:\n> \n> struct gpg_format_data {\n> \t...\n> \tconst char **extra_args_verify;\n> };\n> ...\n> static const char *openpgp_verify_args[] = {\n> \t\"--key-id-format=long\"\n> };\n> ...\n> static struct gpg_format_data gpg_formats[] = {\n> \t{ ...\n> \t .extra_args_verify = openpgp_verify_args\n> \t}\n> };\n> \n> I'm not sure if that's more horrible or less. It's worse to write in\n> the first place, but it's slightly easier to maintain going forward. I\n> dunno.\n\nI switched to that, looks good but i am also not sure which one is\nbetter.\n\n> > +enum gpgformats { PGP_FMT }; \n> \n> Looks like we use this only for indexing the gpg_formats array. I know\n> that C guarantees 0-indexing, but if we're depending on it, it might\n> be worth writing out \"PGP_FMT = 0\" explicitly. And probably adding a\n> comment that this needs to remain in sync with the array.\n> \n> The other alternative is that we could simply use\n> get_format_data(\"openpgp\"), though that does add a minor runtime cost.\n\nThanks, i got rid of the enum and use get_format_data with two fixed\nstrings for the two possible matches.\n\n> > +struct gpg_format_data gpg_formats[] = {\n> > +\t{ .format = \"openpgp\", .program = \"gpg\",\n> > +\t .extra_args_verify = { \"--keyid-format=long\" },\n> > +\t .sigs = { PGP_SIGNATURE, PGP_MESSAGE }\n> > +\t},\n> > +}; \n> \n> This array should be marked static, I think.\n\nYes.\n\n> > +static struct gpg_format_data *get_format_data(const char *str)\n> > +{\n> > +\tint i;\n> > +\tfor (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n> > +\t\tif (!strcasecmp(gpg_formats[i].format, str))\n> > +\t\t\treturn gpg_formats + i;\n> > +\treturn NULL;\n> > +} \n> \n> This looks much nicer than the assert()-ing version from v1.\n> \n> > +static struct gpg_format_data *get_format_data_by_sig(const char\n> > *sig) +{\n> > +\tint i, j;\n> > +\tfor (i = 0; i < ARRAY_SIZE(gpg_formats); i++)\n> > +\t\tfor (j = 0; j < ARRAY_SIZE(gpg_formats[i].sigs);\n> > j++)\n> > +\t\t\tif (gpg_formats[i].sigs[j] && \n> > +\t\t\t !strncmp(gpg_formats[i].sigs[j], sig,\n> > +\n> > strlen(gpg_formats[i].sigs[j])))\n> > +\t\t\t\treturn gpg_formats + i;\n> > +\treturn NULL;\n> > +} \n> \n> This might be a little more readable with:\n> \n> starts_with(sig, gpg_formats[i].sigs[j])\n> \n> instead of the strncmp. It may also be more efficient, as we don't\n> have to compute the strlen of the prefix for each non-matching line\n> (the compiler _might_ be smart enough to realize these are all string\n> literals, but it's pretty buried).\n\nThanks, will do.\n\n> I also wondered if our prefix matching here is overly loose. We have\n> to do a prefix match, since \"sig\" isn't terminated at the line\n> buffer. So I think we'd match:\n> \n> --- BEGIN PGP MESSAGE --- AND SOME OTHER STUFF ---\n> \n> on a line. But I think that's no different than the current code. If\n> we care, I guess we could look for '\\n' or '\\0' immediately after.\n\nLet us not care, just like current code.\n\n> > static int is_gpg_start(const char *line)\n> > {\n> > -\treturn starts_with(line, PGP_SIGNATURE) ||\n> > -\t\tstarts_with(line, PGP_MESSAGE);\n> > +\treturn (get_format_data_by_sig(line) != NULL);\n> > } \n> \n> I don't know if we've ever discussed this style explicitly, but we'd\n> usually omit the unnecessary parentheses for the return here.\n\nWill remove those braces.\n\n> > @@ -140,18 +173,14 @@ int git_gpg_config(const char *var, const\n> > char *value, void *cb) }\n> > \n> > \tif (!strcmp(var, \"gpg.format\")) {\n> > -\t\tif (strcasecmp(value, \"openpgp\"))\n> > +\t\tif (!get_format_data(value))\n> > \t\t\treturn error(\"malformed value for %s: %s\",\n> > var, value); return git_config_string(&gpg_format, var, value);\n> > \t} \n> \n> Much nicer than v1.\n> \n> > @@ -165,12 +194,16 @@ const char *get_signing_key(void)\n> > int sign_buffer(struct strbuf *buffer, struct strbuf *signature,\n> > const char *signing_key) {\n> > \tstruct child_process gpg = CHILD_PROCESS_INIT;\n> > +\tstruct gpg_format_data *fmt;\n> > \tint ret;\n> > \tsize_t i, j, bottom;\n> > \tstruct strbuf gpg_status = STRBUF_INIT;\n> > \n> > +\tfmt = get_format_data(gpg_format);\n> > +\tif (!fmt)\n> > +\t\tBUG(\"bad gpg_format '%s'\", gpg_format); \n> \n> This makes sense as a BUG, because we would already have validated it\n> when parsing gpg.format earlier. \n> That does make me wonder if we should\n> simply be storing a \"struct gpg_format_data\" instead of a string,\n> though. I.e., at the top-level:\n> \n> /* default to signing with openpgp */\n> static struct gpg_format_data *gpg_format = &gpg_formats[0];\n\nGood idea, implemented that.\n\n> > @@ -223,10 +257,18 @@ int verify_signed_buffer(const char *payload,\n> > size_t payload_size, return -1;\n> > \t}\n> > \n> > +\tfmt = get_format_data_by_sig(signature);\n> > +\tassert(fmt); \n> \n> Is this assert() right? The signature data comes from the user. I\n> guess to get here we'll already have matched their signature via\n> is_gpg_start(), and this is just a cross-check? If so, then it's OK to\n> assert, but a BUG() with a descriptive message would be better still.\n\nTurned that into a BUG(). I knew there where 2 such asserts but must\nhave been looking for assert(0) when i fixed the first.\n\n> I also wonder if whoever parses the signature should get back a\n> gpg_format_data and just pass it in here, so we don't have to reparse.\n> That's what my earlier series did. It requires tweaking the function\n> signatures, but IMHO the result was a lot more obvious.\n\nIt would make sense to return the type of signature right with\nparse_signature, and would probably safe a few lookups. I started the\nrefactoring and it turned out to become a pretty big change. That is\nwhy i would prefer to do that on top of the series or leave it the way\nit is now.\n\n> > +\targv_array_pushl(&gpg.args,\n> > +\t\t\t fmt->program, NULL); \n> \n> If you're just pushing one thing, you don't need pushl(). You can\n> just:\n> \n> argv_array_push(&gpg.args, fmt->program);\n> \n> > +\tfor (i = 0; i < ARRAY_SIZE(fmt->extra_args_verify); i++)\n> > +\t\tif (fmt->extra_args_verify[i])\n> > +\t\t\targv_array_pushl(&gpg.args,\n> > +\n> > fmt->extra_args_verify[i], NULL); \n> \n> Likewise here. Though if you made extra_args_verify a NULL-terminated\n> list, this whole loop could become:\n> \n> argv_array_pushv(&gpg.args, fmt->extra_args_verify);\n> \n> It's not _that_ much code, but I think using NULL-terminated lists in\n> a situation like this is more idiomatic for our code base.\n\nThanks, good point!\n\n> -Peff\n\n"},{"id":"352456","messageId":"20180713104122.09845ad0@md1pvb1c.ad001.siemens.net","threadId":"48850","inReplyTo":"4905c1907a866c0fd1a4dac978dd6ca3e468ac43.1531208187.git.henning.schild@siemens.com","subject":"Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program","fromName":"Henning Schild","fromEmail":"henning.schild@siemens.com","sentAt":"2018-07-13T08:41:22Z","receivedAt":"2018-07-13T08:41:50Z","isPatch":true,"sender":{"key":"henning.schild@siemens.com","avatar":"https://avatars.githubusercontent.com/u/7237877?v=4"},"body":"Replies to this one have been ignored for v3. I do not know how to\nproceed here.\n\nHenning\n\nAm Tue, 10 Jul 2018 10:52:29 +0200\nschrieb Henning Schild :\n\n> Supporting multiple signing formats we will have the need to\n> configure a custom program each. Add a new config value to cater for\n> that.\n> \n> Signed-off-by: Henning Schild \n> ---\n> Documentation/config.txt | 5 +++++\n> gpg-interface.c | 2 +-\n> 2 files changed, 6 insertions(+), 1 deletion(-)\n> \n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index ac373e3f4..c0bd80954 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1832,6 +1832,11 @@ gpg.format::\n> \tSpecifies which key format to use when signing with\n> `--gpg-sign`. Default is \"openpgp\", that is also the only supported\n> value. \n> +gpg..program::\n> +\tUse this to customize the program used for the signing\n> format you\n> +\tchose. (see gpg.program) gpg.openpgp.program is a synonym\n> for the\n> +\tlegacy gpg.program.\n> +\n> gui.commitMsgWidth::\n> \tDefines how wide the commit message window is in the\n> \tlinkgit:git-gui[1]. \"75\" is the default.\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index ac2df498d..65098430f 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -179,7 +179,7 @@ int git_gpg_config(const char *var, const char\n> *value, void *cb) return git_config_string(&gpg_format, var, value);\n> \t}\n> \n> -\tif (!strcmp(var, \"gpg.program\"))\n> +\tif (!strcmp(var, \"gpg.program\") || !strcmp(var,\n> \"gpg.openpgp.program\")) return\n> git_config_string(&gpg_formats[PGP_FMT].program, var, value);\n> \treturn 0;\n\n"},{"id":"352550","messageId":"20180714181347.GE1042117@genre.crustytoothpaste.net","threadId":"48850","inReplyTo":"20180710165638.GF23624@sigill.intra.peff.net","subject":"Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-07-14T18:13:47Z","receivedAt":"2018-07-14T18:13:57Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Jul 10, 2018 at 12:56:38PM -0400, Jeff King wrote:\n> On Tue, Jul 10, 2018 at 12:54:13PM -0400, Jeff King wrote:\n> \n> > Should we allow:\n> > \n> > [gpg \"OpenPGP\"]\n> > program = whatever\n> > \n> > given that we allow:\n> > \n> > [gpg]\n> > format = OpenPGP\n> > \n> > ? I think just using strcasecmp() here would be sufficient. But I wonder\n> > if it is a symptom of using the wrong tool (subsections) when we don't\n> > need it.\n> \n> I did just read the discussion in response to v1, where everybody told\n> you the opposite. ;)\n> \n> So I guess my question/points are more for brian and Junio.\n\nI'm okay with us forcing \"openpgp\". That seems sane enough for now, and\nif people scream loudly, we can loosen it.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"352551","messageId":"20180714182625.GF1042117@genre.crustytoothpaste.net","threadId":"48850","inReplyTo":"20180711123824.7e0be91a@md1pvb1c.ad001.siemens.net","subject":"Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-07-14T18:26:26Z","receivedAt":"2018-07-14T18:26:36Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Jul 11, 2018 at 12:38:24PM +0200, Henning Schild wrote:\n> Am Tue, 10 Jul 2018 13:09:01 -0400\n> schrieb Jeff King :\n> \n> > On Tue, Jul 10, 2018 at 10:52:31AM +0200, Henning Schild wrote:\n> > \n> > > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\n> > > index a5d3b2cba..9dcb4e990 100755\n> > > --- a/t/lib-gpg.sh\n> > > +++ b/t/lib-gpg.sh\n> > > @@ -38,7 +38,14 @@ then\n> > > \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n> > > \t\tgpg --homedir \"${GNUPGHOME}\" /dev/null\n> > > 2>&1 \\ --sign -u committer@example.com &&\n> > > -\t\ttest_set_prereq GPG\n> > > +\t\ttest_set_prereq GPG &&\n> > > +\t\techo | gpgsm --homedir \"${GNUPGHOME}\" -o\n> > > \"$TEST_DIRECTORY\"/lib-gpg/gpgsm.crt.user --passphrase-fd 0\n> > > --pinentry-mode loopback --generate-key --batch\n> > > \"$TEST_DIRECTORY\"/lib-gpg/gpgsm-gen-key.in && \n> > \n> > Is this --generate-key going to require high-quality entropy? That\n> > could slow the test suite down (and maybe even stall it quite a bit\n> > on servers doing automated CI).\n> \n> Yes, i also saw that getting \"stuck\" on one machine. But i blamed an\n> experimental kernel.\n> \n> > Can we save a dummy generated key and just import it? That's what we\n> > do for the regular gpg case.\n> \n> I will look into storing a binary and leaving notes how it was\n> generated, just like regular gpg does. The reason i did not do that in\n> the first place is that x509 certs have a validity and we introduce\n> time into the picture. But i will see if i can generate epoch->infinity\n> to get the host clock or just the future out of the picture.\n\nYeah, I agree that would be good. If gpgsm does anything like what gpg\ndoes, it will require the use of /dev/random, which means this will\ndefinitely be slow on build servers and other noninteractive systems.\nWe have this problem at $DAYJOB when automating generation of gpg keys.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"352552","messageId":"20180714183312.GG1042117@genre.crustytoothpaste.net","threadId":"48850","inReplyTo":"20180710171224.GI23624@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/9] X509 (gpgsm) commit signing support","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-07-14T18:33:12Z","receivedAt":"2018-07-14T18:33:20Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Jul 10, 2018 at 01:12:24PM -0400, Jeff King wrote:\n> On Tue, Jul 10, 2018 at 10:52:22AM +0200, Henning Schild wrote:\n> > This series can be seen as a follow up of a series that appeared under\n> > the name \"gpg-interface: Multiple signing tools\" in april 2018 [1]. After\n> > that series was not merged i decided to get my patches ready. The\n> > original series aimed at being generic for any sort of signing tool, while\n> > this series just introduced the X509 variant of gpg. (gpgsm)\n> > I collected authors and reviewers of that first series and already put them\n> > on cc.\n> \n> This series is a fine replacement for that earlier work. It's flexible\n> enough to allow what we really wanted out of that series (gpgsm support,\n> or another drop-in tool that uses the same interface). It doesn't lay\n> any groundwork for further tools (like signify), but I think the\n> consensus on the list was to punt on that until somebody had more\n> concrete plans for adding such a tool.\n\nI actually think this moves in a nice direction for adding support for\nminisign/signify and other schemes. There's a way to look up what\nalgorithm is in use in a particular context based on the first line and\na general interface for deciding what format to write. Granted, it\ncurrently still is very specific to gpg-style tools, but I think this is\nan improvement in that regard.\n\nAs an OpenPGP user, I have no interest in adding support for other\ntools, but I think this should make it easier if someone else wants to\ndo that.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"352690","messageId":"20180716213210.GI25189@sigill.intra.peff.net","threadId":"48850","inReplyTo":"20180714183312.GG1042117@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 0/9] X509 (gpgsm) commit signing support","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T21:32:11Z","receivedAt":"2018-07-16T21:32:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 14, 2018 at 06:33:12PM +0000, brian m. carlson wrote:\n\n> > This series is a fine replacement for that earlier work. It's flexible\n> > enough to allow what we really wanted out of that series (gpgsm support,\n> > or another drop-in tool that uses the same interface). It doesn't lay\n> > any groundwork for further tools (like signify), but I think the\n> > consensus on the list was to punt on that until somebody had more\n> > concrete plans for adding such a tool.\n> \n> I actually think this moves in a nice direction for adding support for\n> minisign/signify and other schemes. There's a way to look up what\n> algorithm is in use in a particular context based on the first line and\n> a general interface for deciding what format to write. Granted, it\n> currently still is very specific to gpg-style tools, but I think this is\n> an improvement in that regard.\n\nMy issue with this for helping with signify is that it creates a new\ngpg..* hierarchy with two slots (openpgp and x509). But we would\nnot want gpg.signify.program, would we? That makes no sense, as neither\nthe signature-matching nor the program invocation are gpg-like.\n\nBut if we later moved to \"signingtool..*\", now we have an extra\nlayer of compatibility to deal with. E.g., signingtool.openpgp.program\nis the same as gpg.openpgp.program which is the same as gpg.program.\n\nI think we can do that, but it means more historical baggage.\n\nI'm OK with that since signify support is purely hypothetical at this\npoint. But that's why I say that this doesn't lay the groundwork in the\nway that the other series did.\n\n> As an OpenPGP user, I have no interest in adding support for other\n> tools, but I think this should make it easier if someone else wants to\n> do that.\n\nI don't plan to work on signify (or other tools) anytime soon either. My\ninterest here is in x509, since that's what enterprises would use over\npgp.\n\nI actually dislike pgp for this application, too, because I find the key\nmanagement kind of complicated and tedious. But at least it's a standard\namong open source folks.\n\n-Peff\n"},{"id":"352691","messageId":"20180716213510.GJ25189@sigill.intra.peff.net","threadId":"48850","inReplyTo":"20180714181347.GE1042117@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T21:35:11Z","receivedAt":"2018-07-16T21:35:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 14, 2018 at 06:13:47PM +0000, brian m. carlson wrote:\n\n> On Tue, Jul 10, 2018 at 12:56:38PM -0400, Jeff King wrote:\n> > On Tue, Jul 10, 2018 at 12:54:13PM -0400, Jeff King wrote:\n> > \n> > > Should we allow:\n> > > \n> > > [gpg \"OpenPGP\"]\n> > > program = whatever\n> > > \n> > > given that we allow:\n> > > \n> > > [gpg]\n> > > format = OpenPGP\n> > > \n> > > ? I think just using strcasecmp() here would be sufficient. But I wonder\n> > > if it is a symptom of using the wrong tool (subsections) when we don't\n> > > need it.\n> > \n> > I did just read the discussion in response to v1, where everybody told\n> > you the opposite. ;)\n> > \n> > So I guess my question/points are more for brian and Junio.\n> \n> I'm okay with us forcing \"openpgp\". That seems sane enough for now, and\n> if people scream loudly, we can loosen it.\n\nWell, I specifically meant \"are you sure subsections like this are a\ngood idea\". But it seems like people think so?\n\nIf so, then I think the best route is just dictating that yes,\ngpg.format is case-sensitive because it is referencing\ngpg..program, which is itself case-sensitive (and \"openpgp\" is\nthe right spelling).\n\n-Peff\n"},{"id":"352697","messageId":"xmqq36wisv3h.fsf@gitster-ct.c.googlers.com","threadId":"48850","inReplyTo":"20180716213510.GJ25189@sigill.intra.peff.net","subject":"Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-16T21:56:34Z","receivedAt":"2018-07-16T21:56:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Jul 14, 2018 at 06:13:47PM +0000, brian m. carlson wrote:\n>\n>> On Tue, Jul 10, 2018 at 12:56:38PM -0400, Jeff King wrote:\n>> > On Tue, Jul 10, 2018 at 12:54:13PM -0400, Jeff King wrote:\n>> > \n>> > > Should we allow:\n>> > > \n>> > > [gpg \"OpenPGP\"]\n>> > > program = whatever\n>> > > \n>> > > given that we allow:\n>> > > \n>> > > [gpg]\n>> > > format = OpenPGP\n>> > > \n>> > > ? I think just using strcasecmp() here would be sufficient. But I wonder\n>> > > if it is a symptom of using the wrong tool (subsections) when we don't\n>> > > need it.\n>> > \n>> > I did just read the discussion in response to v1, where everybody told\n>> > you the opposite. ;)\n>> > \n>> > So I guess my question/points are more for brian and Junio.\n>> \n>> I'm okay with us forcing \"openpgp\". That seems sane enough for now, and\n>> if people scream loudly, we can loosen it.\n>\n> Well, I specifically meant \"are you sure subsections like this are a\n> good idea\". But it seems like people think so?\n\nI admit that I did not even consider that there may be better tool\nthan using subsections to record this information. What are the\npossibilities you have in mind (if you have one)?\n\n>\n> If so, then I think the best route is just dictating that yes,\n> gpg.format is case-sensitive because it is referencing\n> gpg..program, which is itself case-sensitive (and \"openpgp\" is\n> the right spelling).\n>\n> -Peff\n"},{"id":"352703","messageId":"20180716222354.GA12482@sigill.intra.peff.net","threadId":"48850","inReplyTo":"xmqq36wisv3h.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-16T22:23:54Z","receivedAt":"2018-07-16T22:23:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 16, 2018 at 02:56:34PM -0700, Junio C Hamano wrote:\n\n> >> I'm okay with us forcing \"openpgp\". That seems sane enough for now, and\n> >> if people scream loudly, we can loosen it.\n> >\n> > Well, I specifically meant \"are you sure subsections like this are a\n> > good idea\". But it seems like people think so?\n> \n> I admit that I did not even consider that there may be better tool\n> than using subsections to record this information. What are the\n> possibilities you have in mind (if you have one)?\n\nI don't think there is another tool except two-level options, like\n\"gpg.openpgpprogram\" and \"gpg.x509program\".\n\nAlthough those are a bit ugly, I just wondered if they might make things\nsimpler, since AFAIK we are not planning to add more config options\nhere. Like gpg.x509.someotherflag, nor gpg.someothertool.program.\n\nOf course one reason _for_ the tri-level is that we might one day add\ngpg.x509.someotherflag, and this gives us room to do it with less\nawkwardness (i.e., a proliferation of gpg.x509someflag options).\n\n-Peff\n"},{"id":"352725","messageId":"xmqqo9f6rczt.fsf@gitster-ct.c.googlers.com","threadId":"48850","inReplyTo":"20180716222354.GA12482@sigill.intra.peff.net","subject":"Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-16T23:12:54Z","receivedAt":"2018-07-16T23:12:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jul 16, 2018 at 02:56:34PM -0700, Junio C Hamano wrote:\n>\n>> >> I'm okay with us forcing \"openpgp\". That seems sane enough for now, and\n>> >> if people scream loudly, we can loosen it.\n>> >\n>> > Well, I specifically meant \"are you sure subsections like this are a\n>> > good idea\". But it seems like people think so?\n>> \n>> I admit that I did not even consider that there may be better tool\n>> than using subsections to record this information. What are the\n>> possibilities you have in mind (if you have one)?\n>\n> I don't think there is another tool except two-level options, like\n> \"gpg.openpgpprogram\" and \"gpg.x509program\".\n>\n> Although those are a bit ugly, I just wondered if they might make things\n> simpler, since AFAIK we are not planning to add more config options\n> here. Like gpg.x509.someotherflag, nor gpg.someothertool.program.\n>\n> Of course one reason _for_ the tri-level is that we might one day add\n> gpg.x509.someotherflag, and this gives us room to do it with less\n> awkwardness (i.e., a proliferation of gpg.x509someflag options).\n\nYes, and signingtool.. is probably a good (ultra-)long\nterm direction. Preparing the code may be quite a lot of work that\nnobody may be interested in, and nothing other than the GPG family\nmight materialize for a long time, but if we can cheaply prepare\nexternal interface less dependent on GPG/PGP, that by itself would\nbe a good thing to have, I would guess.\n\nThanks.\n"}]}