From 9d12546de9d75be70440e340a5f4bb6f9e41a89f Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Tue, 12 Oct 2021 11:22:35 +0200 Subject: [PATCH 1/4] ssh signing: fmt-merge-msg tests & config parse When merging a signed tag fmt-merge-msg was unable to verify its validity missing the necessary ssh allowedSignersFile config. Adds gpg config parsing to fmt-merge-msg. Adds tests for ssh signed tags to fmt-merge-msg tests. Signed-off-by: Fabian Stelzer Signed-off-by: Junio C Hamano --- fmt-merge-msg.c | 6 ++++++ t/t6200-fmt-merge-msg.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c index 2901c5e4f8..5216191488 100644 --- a/fmt-merge-msg.c +++ b/fmt-merge-msg.c @@ -9,6 +9,7 @@ #include "branch.h" #include "fmt-merge-msg.h" #include "commit-reach.h" +#include "gpg-interface.h" static int use_branch_desc; static int suppress_dest_pattern_seen; @@ -16,6 +17,8 @@ static struct string_list suppress_dest_patterns = STRING_LIST_INIT_DUP; int fmt_merge_msg_config(const char *key, const char *value, void *cb) { + int status = 0; + if (!strcmp(key, "merge.log") || !strcmp(key, "merge.summary")) { int is_bool; merge_log_config = git_config_bool_or_int(key, value, &is_bool); @@ -34,6 +37,9 @@ int fmt_merge_msg_config(const char *key, const char *value, void *cb) string_list_append(&suppress_dest_patterns, value); suppress_dest_pattern_seen = 1; } else { + status = git_gpg_config(key, value, NULL); + if (status) + return status; return git_default_config(key, value, cb); } return 0; diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh index 44f55d93fe..06c5fb5615 100755 --- a/t/t6200-fmt-merge-msg.sh +++ b/t/t6200-fmt-merge-msg.sh @@ -81,6 +81,16 @@ test_expect_success GPG 'set up a signed tag' ' git tag -s -m signed-tag-msg signed-good-tag left ' +test_expect_success GPGSSH 'created ssh signed commit and tag' ' + test_config gpg.format ssh && + git checkout -b signed-ssh && + touch file && + git add file && + git commit -m "ssh signed" -S"${GPGSSH_KEY_PRIMARY}" && + git tag -s -u"${GPGSSH_KEY_PRIMARY}" -m signed-ssh-tag-msg signed-good-ssh-tag left && + git tag -s -u"${GPGSSH_KEY_UNTRUSTED}" -m signed-ssh-tag-msg-untrusted signed-untrusted-ssh-tag left +' + test_expect_success 'message for merging local branch' ' echo "Merge branch ${apos}left${apos}" >expected && @@ -109,6 +119,24 @@ test_expect_success GPG 'message for merging local tag signed by unknown key' ' grep -E "^# gpg: Can${apos}t check signature: (public key not found|No public key)" actual ' +test_expect_success GPGSSH 'message for merging local tag signed by good ssh key' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + git checkout main && + git fetch . signed-good-ssh-tag && + git fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 && + grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual +' + +test_expect_success GPGSSH 'message for merging local tag signed by unknown ssh key' ' + test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + git checkout main && + git fetch . signed-untrusted-ssh-tag && + git fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 && + grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && + grep "${GPGSSH_KEY_NOT_TRUSTED}" actual +' test_expect_success 'message for merging external branch' ' echo "Merge branch ${apos}left${apos} of $(pwd)" >expected && From 9fb391bff9258dd83b7c218fd9bb6ddb6e7b425e Mon Sep 17 00:00:00 2001 From: Fabian Stelzer Date: Wed, 13 Oct 2021 09:51:07 +0200 Subject: [PATCH 2/4] ssh signing: clarify trustlevel usage in docs facca53ac added verification for ssh signatures but incorrectly described the usage of gpg.minTrustLevel. While the verifications trustlevel is stil set to fully or undefined depending on if the key is known or not it has no effect on the verification result. Unknown keys will always fail verification. This commit updates the docs to match this behaviour. Signed-off-by: Fabian Stelzer Signed-off-by: Junio C Hamano --- Documentation/config/gpg.txt | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt index 51a756b2f1..4f30c7dbdd 100644 --- a/Documentation/config/gpg.txt +++ b/Documentation/config/gpg.txt @@ -52,9 +52,7 @@ gpg.ssh.allowedSignersFile:: SSH has no concept of trust levels like gpg does. To be able to differentiate between valid signatures and trusted signatures the trust level of a signature verification is set to `fully` when the public key is present in the allowedSignersFile. -Therefore to only mark fully trusted keys as verified set gpg.minTrustLevel to `fully`. -Otherwise valid but untrusted signatures will still verify but show no principal -name of the signer. +Otherwise the trust level is `undefined` and git verify-commit/tag will fail. + This file can be set to a location outside of the repository and every developer maintains their own trust store. A central repository server could generate this From 78d468f1a9c7bf9d1724840ff322b9144061b308 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 18 Oct 2021 13:15:00 -0400 Subject: [PATCH 3/4] gpg-interface: fix leak of "line" in parse_ssh_output() We xmemdupz() this buffer, but never free it. Let's do so. We'll use a cleanup label, since there are multiple exits from the function. Note that it was also declared a "const char *". We could switch that to "char *" to indicate that it's allocated, but that make it awkward to use with skip_prefix(). So instead, we'll introduce an extra non-const pointer. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- gpg-interface.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index 433482307c..c60b9cd19d 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -365,6 +365,7 @@ static int verify_gpg_signed_buffer(struct signature_check *sigc, static void parse_ssh_output(struct signature_check *sigc) { const char *line, *principal, *search; + char *to_free; char *key = NULL; /* @@ -383,7 +384,7 @@ static void parse_ssh_output(struct signature_check *sigc) sigc->result = 'B'; sigc->trust_level = TRUST_NEVER; - line = xmemdupz(sigc->output, strcspn(sigc->output, "\n")); + line = to_free = xmemdupz(sigc->output, strcspn(sigc->output, "\n")); if (skip_prefix(line, "Good \"git\" signature for ", &line)) { /* Valid signature and known principal */ @@ -403,7 +404,7 @@ static void parse_ssh_output(struct signature_check *sigc) sigc->result = 'G'; sigc->trust_level = TRUST_UNDEFINED; } else { - return; + goto cleanup; } key = strstr(line, "key"); @@ -417,6 +418,9 @@ static void parse_ssh_output(struct signature_check *sigc) */ sigc->result = 'B'; } + +cleanup: + free(to_free); } static int verify_ssh_signed_buffer(struct signature_check *sigc, From f3af71c947cdf2e5acd16cacf50586b829a68f6e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 18 Oct 2021 13:15:37 -0400 Subject: [PATCH 4/4] gpg-interface: fix leak of strbufs in get_ssh_key_fingerprint() We read stdout from gpg into a strbuf, then split it into a list of strbufs, pull out one element, and return it. But we don't free either the original stdout buffer, nor the list returned from strbuf_split(). This patch fixes both. Note that we have to detach the returned string from its strbuf before calling strbuf_list_free(), as that would otherwise throw it away. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- gpg-interface.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/gpg-interface.c b/gpg-interface.c index c60b9cd19d..800d8caa67 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -711,6 +711,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key) int ret = -1; struct strbuf fingerprint_stdout = STRBUF_INIT; struct strbuf **fingerprint; + char *fingerprint_ret; /* * With SSH Signing this can contain a filename or a public key @@ -737,7 +738,10 @@ static char *get_ssh_key_fingerprint(const char *signing_key) die_errno(_("failed to get the ssh fingerprint for key '%s'"), signing_key); - return strbuf_detach(fingerprint[1], NULL); + fingerprint_ret = strbuf_detach(fingerprint[1], NULL); + strbuf_list_free(fingerprint); + strbuf_release(&fingerprint_stdout); + return fingerprint_ret; } /* Returns the first public key from an ssh-agent to use for signing */