From cf98a52ba4d8176a3ec73c72d296275999ecb52d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Apr 2018 15:18:29 -0600 Subject: [PATCH 1/7] t7004: fix mistaken tag name We have a series of tests which create signed tags with various properties, but one test accidentally verifies a tag from much earlier in the series. Signed-off-by: Jeff King Signed-off-by: Ben Toews Signed-off-by: Junio C Hamano --- t/t7004-tag.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 2aac77af70..ee093b393d 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1056,7 +1056,7 @@ test_expect_success GPG \ git tag -s -F sigblanknonlfile blanknonlfile-signed-tag && get_tag_msg blanknonlfile-signed-tag >actual && test_cmp expect actual && - git tag -v signed-tag + git tag -v blanknonlfile-signed-tag ' # messages with commented lines for signed tags: From 1b0eeec3f359888f8a638de8af253f621a5b836e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Apr 2018 15:18:30 -0600 Subject: [PATCH 2/7] gpg-interface: handle bool user.signingkey The config handler for user.signingkey does not check for a boolean value, and thus: git -c user.signingkey tag will segfault. We could fix this and even shorten the code by using git_config_string(). But our set_signing_key() helper is used by other code outside of gpg-interface.c, so we must keep it (and we may as well use it, because unlike git_config_string() it does not leak when we overwrite an old value). Ironically, the handler for gpg.program just below _could_ use git_config_string() but doesn't. But since we're going to touch that in a future patch, we'll leave it alone for now. We will add some whitespace and returns in preparation for adding more config keys, though. Signed-off-by: Jeff King Signed-off-by: Ben Toews Signed-off-by: Junio C Hamano --- gpg-interface.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/gpg-interface.c b/gpg-interface.c index 4feacf16e5..61c0690e12 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -128,13 +128,19 @@ void set_signing_key(const char *key) int git_gpg_config(const char *var, const char *value, void *cb) { if (!strcmp(var, "user.signingkey")) { + if (!value) + return config_error_nonbool(var); set_signing_key(value); + return 0; } + if (!strcmp(var, "gpg.program")) { if (!value) return config_error_nonbool(var); gpg_program = xstrdup(value); + return 0; } + return 0; } From f80bee27e3c3fc9085427945f97a2f7756500ea9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Apr 2018 15:18:31 -0600 Subject: [PATCH 3/7] gpg-interface: modernize function declarations Let's drop "extern" from our declarations, which brings us in line with our modern style guidelines. While we're here, let's wrap some of the overly long lines, and move docstrings for public functions to their declarations, since they document the interface. Signed-off-by: Jeff King Signed-off-by: Ben Toews Signed-off-by: Junio C Hamano --- gpg-interface.c | 17 ----------------- gpg-interface.h | 49 ++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 38 insertions(+), 28 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index 61c0690e12..08de0daa41 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -101,12 +101,6 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags) fputs(output, stderr); } -/* - * Look at GPG signed content (e.g. a signed tag object), whose - * payload is followed by a detached signature on it. Return the - * offset where the embedded detached signature begins, or the end of - * the data when there is no such signature. - */ size_t parse_signature(const char *buf, unsigned long size) { char *eol; @@ -151,12 +145,6 @@ const char *get_signing_key(void) return git_committer_info(IDENT_STRICT|IDENT_NO_DATE); } -/* - * Create a detached signature for the contents of "buffer" and append - * it after "signature"; "buffer" and "signature" can be the same - * strbuf instance, which would cause the detached signature appended - * at the end. - */ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key) { struct child_process gpg = CHILD_PROCESS_INIT; @@ -198,11 +186,6 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig return 0; } -/* - * Run "gpg" to see if the payload matches the detached signature. - * gpg_output, when set, receives the diagnostic output from GPG. - * gpg_status, when set, receives the status output from GPG. - */ int verify_signed_buffer(const char *payload, size_t payload_size, const char *signature, size_t signature_size, struct strbuf *gpg_output, struct strbuf *gpg_status) diff --git a/gpg-interface.h b/gpg-interface.h index d2d4fd3a65..2c40a9175f 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -23,16 +23,43 @@ struct signature_check { char *key; }; -extern void signature_check_clear(struct signature_check *sigc); -extern size_t parse_signature(const char *buf, unsigned long size); -extern void parse_gpg_output(struct signature_check *); -extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key); -extern int verify_signed_buffer(const char *payload, size_t payload_size, const char *signature, size_t signature_size, struct strbuf *gpg_output, struct strbuf *gpg_status); -extern int git_gpg_config(const char *, const char *, void *); -extern void set_signing_key(const char *); -extern const char *get_signing_key(void); -extern int check_signature(const char *payload, size_t plen, - const char *signature, size_t slen, struct signature_check *sigc); -void print_signature_buffer(const struct signature_check *sigc, unsigned flags); +void signature_check_clear(struct signature_check *sigc); + +/* + * Look at GPG signed content (e.g. a signed tag object), whose + * payload is followed by a detached signature on it. Return the + * offset where the embedded detached signature begins, or the end of + * the data when there is no such signature. + */ +size_t parse_signature(const char *buf, unsigned long size); + +void parse_gpg_output(struct signature_check *); + +/* + * Create a detached signature for the contents of "buffer" and append + * it after "signature"; "buffer" and "signature" can be the same + * strbuf instance, which would cause the detached signature appended + * at the end. + */ +int sign_buffer(struct strbuf *buffer, struct strbuf *signature, + const char *signing_key); + +/* + * Run "gpg" to see if the payload matches the detached signature. + * gpg_output, when set, receives the diagnostic output from GPG. + * gpg_status, when set, receives the status output from GPG. + */ +int verify_signed_buffer(const char *payload, size_t payload_size, + const char *signature, size_t signature_size, + struct strbuf *gpg_output, struct strbuf *gpg_status); + +int git_gpg_config(const char *, const char *, void *); +void set_signing_key(const char *); +const char *get_signing_key(void); +int check_signature(const char *payload, size_t plen, + const char *signature, size_t slen, + struct signature_check *sigc); +void print_signature_buffer(const struct signature_check *sigc, + unsigned flags); #endif From e6fa6cde5bec648f1b8fd7e3f9e5939c5093985a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Apr 2018 15:18:32 -0600 Subject: [PATCH 4/7] gpg-interface: use size_t for signature buffer size Even though our object sizes (from which these buffers would come) are typically "unsigned long", this is something we'd like to eventually fix (since it's only 32-bits even on 64-bit Windows). It makes more sense to use size_t when taking an in-memory buffer. Signed-off-by: Jeff King Signed-off-by: Ben Toews Signed-off-by: Junio C Hamano --- gpg-interface.c | 2 +- gpg-interface.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index 08de0daa41..ac852ad4b9 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -101,7 +101,7 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags) fputs(output, stderr); } -size_t parse_signature(const char *buf, unsigned long size) +size_t parse_signature(const char *buf, size_t size) { char *eol; size_t len = 0; diff --git a/gpg-interface.h b/gpg-interface.h index 2c40a9175f..a5e6517ae6 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -31,7 +31,7 @@ void signature_check_clear(struct signature_check *sigc); * offset where the embedded detached signature begins, or the end of * the data when there is no such signature. */ -size_t parse_signature(const char *buf, unsigned long size); +size_t parse_signature(const char *buf, size_t size); void parse_gpg_output(struct signature_check *); From 17ef3a421e0a1019592a0b14b95f09df61730071 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Apr 2018 15:18:33 -0600 Subject: [PATCH 5/7] gpg-interface: fix const-correctness of "eol" pointer We accidentally shed the "const" of our buffer by passing it through memchr. Let's fix that, and while we're at it, move our variable declaration inside the loop, which is the only place that uses it. Signed-off-by: Jeff King Signed-off-by: Ben Toews Signed-off-by: Junio C Hamano --- gpg-interface.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index ac852ad4b9..3414af38b9 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -103,11 +103,10 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags) size_t parse_signature(const char *buf, size_t size) { - char *eol; size_t len = 0; while (len < size && !starts_with(buf + len, PGP_SIGNATURE) && !starts_with(buf + len, PGP_MESSAGE)) { - eol = memchr(buf + len, '\n', size - len); + const char *eol = memchr(buf + len, '\n', size - len); len += eol ? eol - (buf + len) + 1 : size - len; } return len; From f68f2dd57f55e0b1782b20b615dd7a96d7fb6a41 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Apr 2018 15:18:34 -0600 Subject: [PATCH 6/7] gpg-interface: extract gpg line matching helper Let's separate the actual line-by-line parsing of signatures from the notion of "is this a gpg signature line". That will make it easier to do more refactoring of this loop in future patches. Signed-off-by: Jeff King Signed-off-by: Ben Toews Signed-off-by: Junio C Hamano --- gpg-interface.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index 3414af38b9..79333c1ee8 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -101,11 +101,16 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags) fputs(output, stderr); } +static int is_gpg_start(const char *line) +{ + return starts_with(line, PGP_SIGNATURE) || + starts_with(line, PGP_MESSAGE); +} + size_t parse_signature(const char *buf, size_t size) { size_t len = 0; - while (len < size && !starts_with(buf + len, PGP_SIGNATURE) && - !starts_with(buf + len, PGP_MESSAGE)) { + while (len < size && !is_gpg_start(buf + len)) { const char *eol = memchr(buf + len, '\n', size - len); len += eol ? eol - (buf + len) + 1 : size - len; } From 8b44b2be89bf59c0fada6095bdfea66ff53c6074 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Apr 2018 15:18:35 -0600 Subject: [PATCH 7/7] gpg-interface: find the last gpg signature line A signed tag has a detached signature like this: object ... [...more header...] This is the tag body. -----BEGIN PGP SIGNATURE----- [opaque gpg data] -----END PGP SIGNATURE----- Our parser finds the _first_ line that appears to start a PGP signature block, meaning we may be confused by a signature (or a signature-like line) in the actual body. Let's keep parsing and always find the final block, which should be the detached signature over all of the preceding content. Signed-off-by: Jeff King Signed-off-by: Ben Toews Signed-off-by: Junio C Hamano --- gpg-interface.c | 12 +++++++++--- t/t7004-tag.sh | 11 +++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index 79333c1ee8..0647bd6348 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -110,11 +110,17 @@ static int is_gpg_start(const char *line) size_t parse_signature(const char *buf, size_t size) { size_t len = 0; - while (len < size && !is_gpg_start(buf + len)) { - const char *eol = memchr(buf + len, '\n', size - len); + size_t match = size; + while (len < size) { + const char *eol; + + if (is_gpg_start(buf + len)) + match = len; + + eol = memchr(buf + len, '\n', size - len); len += eol ? eol - (buf + len) + 1 : size - len; } - return len; + return match; } void set_signing_key(const char *key) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index ee093b393d..e3f1e014aa 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1059,6 +1059,17 @@ test_expect_success GPG \ git tag -v blanknonlfile-signed-tag ' +test_expect_success GPG 'signed tag with embedded PGP message' ' + cat >msg <<-\EOF && + -----BEGIN PGP MESSAGE----- + + this is not a real PGP message + -----END PGP MESSAGE----- + EOF + git tag -s -F msg confusing-pgp-message && + git tag -v confusing-pgp-message +' + # messages with commented lines for signed tags: cat >sigcommentsfile <