[PATCH v2 19/24] tag: store SHA-256 signatures in a header
- From
brian m. carlson <sandals@crustytoothpaste.net>
- Date
- Feb 22, 2020, 20:17 UTC
- Message-ID
- <20200222201749.937983-20-sandals@crustytoothpaste.net>
- In-Reply-To
- <20200222201749.937983-1-sandals@crustytoothpaste.net>
In the future, we'll want to allow a user to sign both the SHA-1 version of a tag and the SHA-256 version at the same time. Since for SHA-1 the signature is appended to the tag message, we must use a different way to allow multiple signature.
The transition plan envisions this using a gpgsig-sha256 header, much as for commits. Refactor the commit code that performs parsing of this header and use it for tags that use SHA-256. Check that we get tags in the correct format depending on what algorithm we're using.
Note that currently we have no way to rewrite an object into another hash algorithm, and therefore we don't have a way to verify the signatures of the other hash algorithm. Because of the way the signatures are stored, we'll reject commits signed with both algorithms, which is essential for security. If we allowed both signatures despite not being able to verify them and one signature were invalid, we'd end up with a security problem.
There are, however, a few things to note.
In the ref-filter code, we hoist the signature parsing above the blank line delimiting headers and body so we can find the signature when using SHA-256. For similar reasons, t6300 no longer emits the signature as part of the body since it's no longer part of the body.
We mark a test for exporting signatures with remote helpers as SHA-1 only. Since we no longer have signatures in the body (those are for SHA-1 only), they cannot be exported this way.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- builtin/mktag.c | 14 ++++++++++++++ builtin/tag.c | 4 +++- commit.c | 19 +++++++++++++++---- commit.h | 8 ++++++++ gpg-interface.c | 16 ++++++++++------ ref-filter.c | 7 +++---- t/t5801-remote-helpers.sh | 4 +++- t/t6300-for-each-ref.sh | 34 +++++++++++++++++++++++++--------- t/t7004-tag.sh | 8 +++++++- t/t7030-verify-tag.sh | 17 +++++++++++++++++ 10 files changed, 105 insertions(+), 26 deletions(-)
diff --git a/builtin/mktag.c b/builtin/mktag.c index 4982d3a93e..ce1665c4e4 100644 --- a/builtin/mktag.c +++ b/builtin/mktag.c @@ -144,6 +144,20 @@ static int verify_tag(char *buffer, unsigned long size) (uintmax_t) (tagger_line - buffer)); tagger_line += 6; + if (hash_algo_by_ptr(the_hash_algo) == GIT_HASH_SHA256 && + !memcmp(tagger_line, "gpgsig-sha256 ", 14)) { + char *p = strpbrk(tagger_line + 1, "\n"); + if (!p) + return error("char%"PRIuMAX": could not find end of line", + (uintmax_t) (tagger_line - buffer)); + tagger_line = p + 1; + while (*tagger_line == ' ' && (p = strpbrk(tagger_line, "\n"))) + tagger_line = p + 1; + if (!p) + return error("char%"PRIuMAX": could not find end of line", + (uintmax_t) (tagger_line - buffer)); + } + /* Verify the blank line separating the header from the body */ if (*tagger_line != '\n') return error("char%"PRIuMAX": trailing garbage in tag header", diff --git a/builtin/tag.c b/builtin/tag.c index 6b95c6a2ea..ab6b5044e6 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -128,7 +128,9 @@ static int verify_tag(const char *name, const char *ref, static int do_sign(struct strbuf *buffer) { - return sign_buffer(buffer, buffer, get_signing_key()); + if (hash_algo_by_ptr(the_hash_algo) == GIT_HASH_SHA1) + return sign_buffer(buffer, buffer, get_signing_key()); + return sign_with_header(buffer, get_signing_key()); } static const char tag_template[] = diff --git a/commit.c b/commit.c index d39ce5076d..e3e4f74a36 100644 --- a/commit.c +++ b/commit.c @@ -970,7 +970,7 @@ static const char *gpg_sig_headers[] = { "gpgsig-sha256", }; -static int do_sign_commit(struct strbuf *buf, const char *keyid) +int sign_with_header(struct strbuf *buf, const char *keyid) { struct strbuf sig = STRBUF_INIT; int inspos, copypos; @@ -1010,12 +1010,24 @@ static int do_sign_commit(struct strbuf *buf, const char *keyid) return 0; } + + int parse_signed_commit(const struct commit *commit, struct strbuf *payload, struct strbuf *signature) { - unsigned long size; const char *buffer = get_commit_buffer(commit, &size); + int ret = parse_buffer_signed_by_header(buffer, size, payload, signature); + + unuse_commit_buffer(commit, buffer); + return ret; +} + +int parse_buffer_signed_by_header(const char *buffer, + unsigned long size, + struct strbuf *payload, + struct strbuf *signature) +{ int in_signature, saw_signature = -1; const char *line, *tail; const char *gpg_sig_header = gpg_sig_headers[hash_algo_by_ptr(the_hash_algo)]; @@ -1048,7 +1060,6 @@ int parse_signed_commit(const struct commit *commit, } line = next; } - unuse_commit_buffer(commit, buffer); return saw_signature; } @@ -1490,7 +1501,7 @@ int commit_tree_extended(const char *msg, size_t msg_len, if (encoding_is_utf8 && !verify_utf8(&buffer)) fprintf(stderr, _(commit_utf8_warn)); - if (sign_commit && do_sign_commit(&buffer, sign_commit)) { + if (sign_commit && sign_with_header(&buffer, sign_commit)) { result = -1; goto out; } diff --git a/commit.h b/commit.h index 008a0fa4a0..e9898df412 100644 --- a/commit.h +++ b/commit.h @@ -402,4 +402,12 @@ int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void LAST_ARG_MUST_BE_NULL int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...); +/* Sign a commit or tag buffer, storing the result in a header. */ +int sign_with_header(struct strbuf *buf, const char *keyid); +/* Parse the signature out of a header. */ +int parse_buffer_signed_by_header(const char *buffer, + unsigned long size, + struct strbuf *payload, + struct strbuf *signature); + #endif /* COMMIT_H */ diff --git a/gpg-interface.c b/gpg-interface.c index b25f5c21d8..a63766a19b 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -1,4 +1,5 @@ #include "cache.h" +#include "commit.h" #include "config.h" #include "run-command.h" #include "strbuf.h" @@ -363,13 +364,16 @@ size_t parse_signed_buffer(const char *buf, size_t size) int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct strbuf *signature) { - size_t match = parse_signed_buffer(buf, size); - if (match != size) { - strbuf_add(payload, buf, match); - strbuf_add(signature, buf + match, size - match); - return 1; + if (hash_algo_by_ptr(the_hash_algo) == GIT_HASH_SHA1) { + size_t match = parse_signed_buffer(buf, size); + if (match != size) { + strbuf_add(payload, buf, match); + strbuf_add(signature, buf + match, size - match); + return 1; + } + return 0; } - return 0; + return parse_buffer_signed_by_header(buf, size, payload, signature); } void set_signing_key(const char *key) diff --git a/ref-filter.c b/ref-filter.c index 212f1165bb..933530a14c 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1167,6 +1167,8 @@ static void find_subpos(const char *buf, const char *end = buf + strlen(buf); const char *sigstart; + /* parse signature first; we might not even have a subject line */ + parse_signature(buf, end - buf, &payload, &signature); /* skip past header until we hit empty line */ while (*buf && *buf != '\n') { @@ -1178,9 +1180,6 @@ static void find_subpos(const char *buf, /* skip any empty lines */ while (*buf == '\n') buf++; - - /* parse signature first; we might not even have a subject line */ - parse_signature(buf, end - buf, &payload, &signature); *sig = strbuf_detach(&signature, siglen); sigstart = buf + parse_signed_buffer(buf, strlen(buf)); @@ -1267,7 +1266,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf) v->s = xmemdupz(sigpos, siglen); else if (atom->u.contents.option == C_LINES) { struct strbuf s = STRBUF_INIT; - const char *contents_end = bodylen + bodypos - siglen; + const char *contents_end = bodypos + nonsiglen; /* Size is the length of the message after removing the signature */ append_lines(&s, subpos, contents_end - subpos, atom->u.contents.nlines); diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index 121e5c6edb..801802be9d 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -192,7 +192,9 @@ test_expect_success GPG 'push signed tag' ' test_must_fail compare_refs local signed-tag server signed-tag ' -test_expect_success GPG 'push signed tag with signed-tags capability' ' +# SHA-256 signatures are stored in the header and can't be round-tripped through +# fast-export. +test_expect_success GPG,SHA1 'push signed tag with signed-tags capability' ' (cd local && git checkout master && git tag -s -m signed-tag signed-tag-2 && diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index b3c1092338..caeabfb293 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -54,6 +54,17 @@ test_atom() { " } +test_atom_hash () { + local val + if [ "$(test_oid algo)" = sha1 ] + then + val="$3" + else + val="$4" + fi + test_atom "$1" "$2" "$val" +} + hexlen=$(test_oid hexsz) disklen=$(test_oid disklen) @@ -625,30 +636,35 @@ sig='-----BEGIN PGP SIGNATURE----- PREREQ=GPG test_atom refs/tags/signed-empty subject '' test_atom refs/tags/signed-empty contents:subject '' -test_atom refs/tags/signed-empty body "$sig" +test_atom_hash refs/tags/signed-empty body "$sig" '' test_atom refs/tags/signed-empty contents:body '' test_atom refs/tags/signed-empty contents:signature "$sig" -test_atom refs/tags/signed-empty contents "$sig" +test_atom_hash refs/tags/signed-empty contents "$sig" '' test_atom refs/tags/signed-short subject 'subject line' test_atom refs/tags/signed-short contents:subject 'subject line' -test_atom refs/tags/signed-short body "$sig" +test_atom_hash refs/tags/signed-short body "$sig" '' test_atom refs/tags/signed-short contents:body '' test_atom refs/tags/signed-short contents:signature "$sig" -test_atom refs/tags/signed-short contents "subject line -$sig" +test_atom_hash refs/tags/signed-short contents "subject line +$sig" 'subject line +' test_atom refs/tags/signed-long subject 'subject line' test_atom refs/tags/signed-long contents:subject 'subject line' -test_atom refs/tags/signed-long body "body contents -$sig" +test_atom_hash refs/tags/signed-long body "body contents +$sig" 'body contents +' test_atom refs/tags/signed-long contents:body 'body contents ' test_atom refs/tags/signed-long contents:signature "$sig" -test_atom refs/tags/signed-long contents "subject line +test_atom_hash refs/tags/signed-long contents "subject line body contents -$sig" +$sig" 'subject line + +body contents +' sort >expected <<EOF $(git rev-parse refs/tags/bogo) <committer@example.com> refs/tags/bogo diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 6db92bd3ba..bd74b2d7e0 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -425,7 +425,13 @@ test_expect_success \ # creating annotated tags: get_tag_msg () { - git cat-file tag "$1" | sed -e "/BEGIN PGP/q" + if [ "$(test_oid algo)" = sha1 ] + then + git cat-file tag "$1" | sed -e "/BEGIN PGP/q" + else + + git cat-file tag "$1" | sed -e '/^gpgsig-sha256/{s/^gpgsig-sha256 //;h;d};/^ /d;${p;x;/^$/d}' + fi } # run test_tick before committing always gives the time in that timezone diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh index 5c5bc32ccb..d95a03d315 100755 --- a/t/t7030-verify-tag.sh +++ b/t/t7030-verify-tag.sh @@ -79,6 +79,23 @@ test_expect_success GPG 'verify and show signatures' ' ) ' +test_expect_success GPG 'signature has expected format' ' + for tag in initial second merge fourth-signed sixth-signed seventh-signed + do + if [ "$(test_oid algo)" = sha1 ] + then + git cat-file tag seventh-signed >output && + ! grep gpgsig output && + grep "^-----BEGIN PGP SIGNATURE-----" output + else + git cat-file tag seventh-signed >output && + grep gpgsig-sha256 output && + ! grep "^-----BEGIN PGP SIGNATURE-----" output + fi && + echo $tag OK || exit 1 + done +' + test_expect_success GPGSM 'verify and show signatures x509' ' git verify-tag ninth-signed-x509 2>actual && grep "Good signature from" actual &&