| Message ID | pull.1041.v8.git.git.1631304462.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ssh signing: Add commit & tag signing/verification via SSH keys using ssh-keygen | expand |
"Fabian Stelzer via GitGitGadget" <gitgitgadget@gmail.com> writes: > v8: > > * fixes a bug around find-principals buffer i was releasing while still > iterating over it. Uses separate strbufs now. > * rename a wrong variable in the tests > * use git_config_pathname instead of string where applicable I guess I'd better kick the topic out of 'next' before doing anything else, as it still seems to want to be replaceable wholesale. Somehow I was given a (probably false) impression that the previous one was in a more or less testable shape and we can go incremental already, which was why I merged v7 to 'next'. Will queue later, but may not get around to it today. Thanks.
On 10.09.21 22:23, Junio C Hamano wrote: > "Fabian Stelzer via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> v8: >> >> * fixes a bug around find-principals buffer i was releasing while still >> iterating over it. Uses separate strbufs now. >> * rename a wrong variable in the tests >> * use git_config_pathname instead of string where applicable > I guess I'd better kick the topic out of 'next' before doing > anything else, as it still seems to want to be replaceable > wholesale. Somehow I was given a (probably false) impression that > the previous one was in a more or less testable shape and we can go > incremental already, which was why I merged v7 to 'next'. Sorry, i think i'm just not familiar with the process. What do i do when the patch is in next and someone (or myself) find other bugs during testing? Do i send a new patch based on "next" or update my patchset but not squashing the fixup commits?
Fabian Stelzer <fs@gigacodes.de> writes: > Sorry, i think i'm just not familiar with the process. What do i do when > the patch is in next and someone (or myself) find other bugs during testing? > Do i send a new patch based on "next" or update my patchset but not > squashing the fixup commits? In general, you'd send an incremental update on top of what you submitted and has been queued in my tree so far. Looking for the merge of the topic from the tip of 'next': $ git show -s "next^{/^Merge branch 'fs/ssh-signing' into next}" | grep "^Merge:" Merge: 348fe07b87 b88bcd013b $ git log --oneline --reverse master..b88bcd013b c222385164 ssh signing: preliminary refactoring and clean-up 3a3fdc0b4e ssh signing: add test prereqs c7e2d30efe ssh signing: add ssh key format and signing code 5493722122 ssh signing: retrieve a default key from ssh-agent 6869f1f60c ssh signing: provide a textual signing_key_id 9048bb3c9b ssh signing: verify signatures using ssh-keygen 587967698a ssh signing: duplicate t7510 tests for commits 52ac6bd36f ssh signing: tests for logs, tags & push certs b88bcd013b ssh signing: test that gpg fails for unknown keys we learn that b88bcd013b is the tip, so you'd send follow-up patches to either fix a bug that exists in the tree of b88bcd013b, or enhance a feature on top of the tree of b88bcd013b. But since I am already ejecting the previous round out of 'next', let's remember to do so the next time. We will have to wait until mid October (if I recall what I thought I read from you correctly) anyway, so until then we can iterate outside the 'next' branch. Thanks.
openssh 8.7 will add valid-after, valid-before options to the allowed keys keyring. This allows us to pass the commit timestamp to the verification call and make key rollover possible and still be able to verify older commits. Set valid-after to the current date when adding your key to the keyring and set valid-before to make it fail if used after a certain date. Software like gitolite/github or corporate automation can do this automatically when ssh push keys are addded / removed I will add this feature in a follow up patch afterwards since the released 8.7 version has a broken ssh-keygen implementation which will break ssh signing completely. v7: * change unknown signing key behavior to fail verify-commit/tag just like gpg does * add test for unknown signing keys for ssh & gpg * made default signing key retrieval configurable (gpg.ssh.defaultKeyCommand). We could default this to "ssh-add -L" but would risk some users signing with a wrong key * die() instead of error in case of incompatible signatures to match current BUG() behaviour more * various review fixes (early return for config parse, missing free, comments) * got rid of strcmp("ssh") branches and used format configurable callbacks everywhere * moved documentation changes into the commits adding the specific functionality v8: * fixes a bug around find-principals buffer i was releasing while still iterating over it. Uses separate strbufs now. * rename a wrong variable in the tests * use git_config_pathname instead of string where applicable Fabian Stelzer (9): ssh signing: preliminary refactoring and clean-up ssh signing: add test prereqs ssh signing: add ssh key format and signing code ssh signing: retrieve a default key from ssh-agent ssh signing: provide a textual signing_key_id ssh signing: verify signatures using ssh-keygen ssh signing: duplicate t7510 tests for commits ssh signing: tests for logs, tags & push certs ssh signing: test that gpg fails for unknown keys Documentation/config/gpg.txt | 45 ++- Documentation/config/user.txt | 7 + builtin/receive-pack.c | 4 + fmt-merge-msg.c | 6 +- gpg-interface.c | 577 ++++++++++++++++++++++++++++--- gpg-interface.h | 8 +- log-tree.c | 8 +- pretty.c | 4 +- send-pack.c | 8 +- t/lib-gpg.sh | 28 ++ t/t4202-log.sh | 23 ++ t/t5534-push-signed.sh | 101 ++++++ t/t7031-verify-tag-signed-ssh.sh | 161 +++++++++ t/t7510-signed-commit.sh | 29 +- t/t7528-signed-commit-ssh.sh | 398 +++++++++++++++++++++ 15 files changed, 1341 insertions(+), 66 deletions(-) create mode 100755 t/t7031-verify-tag-signed-ssh.sh create mode 100755 t/t7528-signed-commit-ssh.sh base-commit: 8463beaeb69fe0b7f651065813def4aa6827cd5d Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1041%2FFStelzer%2Fsshsign-v8 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1041/FStelzer/sshsign-v8 Pull-Request: https://github.com/git/git/pull/1041 Range-diff vs v7: 1: 91fd0159e1f = 1: b0bee197a05 ssh signing: preliminary refactoring and clean-up 2: fe98052a3ea = 2: d08327ecb25 ssh signing: add test prereqs 3: 80d2d55d22e = 3: c1e9bba8da0 ssh signing: add ssh key format and signing code 4: 83ece42e1de = 4: 8c430fc7a1b ssh signing: retrieve a default key from ssh-agent 5: 76bc9eb4079 = 5: 0864ed04670 ssh signing: provide a textual signing_key_id 6: dc092c79796 ! 6: cfd66180249 ssh signing: verify signatures using ssh-keygen @@ gpg-interface.c: static int verify_gpg_signed_buffer(struct signature_check *sig + const char *line; + size_t trust_size; + char *principal; ++ struct strbuf ssh_principals_out = STRBUF_INIT; ++ struct strbuf ssh_principals_err = STRBUF_INIT; + struct strbuf ssh_keygen_out = STRBUF_INIT; + struct strbuf ssh_keygen_err = STRBUF_INIT; + @@ gpg-interface.c: static int verify_gpg_signed_buffer(struct signature_check *sig + "-f", ssh_allowed_signers, + "-s", buffer_file->filename.buf, + NULL); -+ ret = pipe_command(&ssh_keygen, NULL, 0, &ssh_keygen_out, 0, -+ &ssh_keygen_err, 0); -+ if (ret && strstr(ssh_keygen_err.buf, "usage:")) { ++ ret = pipe_command(&ssh_keygen, NULL, 0, &ssh_principals_out, 0, ++ &ssh_principals_err, 0); ++ if (ret && strstr(ssh_principals_err.buf, "usage:")) { + error(_("ssh-keygen -Y find-principals/verify is needed for ssh signature verification (available in openssh version 8.2p1+)")); + goto out; + } -+ if (ret || !ssh_keygen_out.len) { ++ if (ret || !ssh_principals_out.len) { + /* + * We did not find a matching principal in the allowedSigners + * Check without validation @@ gpg-interface.c: static int verify_gpg_signed_buffer(struct signature_check *sig + ret = -1; + } else { + /* Check every principal we found (one per line) */ -+ for (line = ssh_keygen_out.buf; *line; ++ for (line = ssh_principals_out.buf; *line; + line = strchrnul(line + 1, '\n')) { + while (*line == '\n') + line++; @@ gpg-interface.c: static int verify_gpg_signed_buffer(struct signature_check *sig + sigc->payload = xmemdupz(payload, payload_size); + strbuf_stripspace(&ssh_keygen_out, 0); + strbuf_stripspace(&ssh_keygen_err, 0); ++ /* Add stderr outputs to show the user actual ssh-keygen errors */ ++ strbuf_add(&ssh_keygen_out, ssh_principals_err.buf, ssh_principals_err.len); + strbuf_add(&ssh_keygen_out, ssh_keygen_err.buf, ssh_keygen_err.len); + sigc->output = strbuf_detach(&ssh_keygen_out, NULL); + sigc->gpg_status = xstrdup(sigc->output); @@ gpg-interface.c: static int verify_gpg_signed_buffer(struct signature_check *sig +out: + if (buffer_file) + delete_tempfile(&buffer_file); ++ strbuf_release(&ssh_principals_out); ++ strbuf_release(&ssh_principals_err); + strbuf_release(&ssh_keygen_out); + strbuf_release(&ssh_keygen_err); + @@ gpg-interface.c: int git_gpg_config(const char *var, const char *value, void *cb + if (!strcmp(var, "gpg.ssh.allowedsignersfile")) { + if (!value) + return config_error_nonbool(var); -+ return git_config_string(&ssh_allowed_signers, var, value); ++ return git_config_pathname(&ssh_allowed_signers, var, value); + } + + if (!strcmp(var, "gpg.ssh.revocationfile")) { + if (!value) + return config_error_nonbool(var); -+ return git_config_string(&ssh_revocation_file, var, value); ++ return git_config_pathname(&ssh_revocation_file, var, value); + } + if (!strcmp(var, "gpg.program") || !strcmp(var, "gpg.openpgp.program")) 7: c17441566d9 ! 7: c8e21dc97f1 ssh signing: duplicate t7510 tests for commits @@ t/t7528-signed-commit-ssh.sh (new) + git show --pretty=short --show-signature $commit >actual && + grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && -+ grep "${KEY_NOT_TRUSTED}" actual && ++ grep "${GPGSSH_KEY_NOT_TRUSTED}" actual && + echo $commit OK || exit 1 + done + ) @@ t/t7528-signed-commit-ssh.sh (new) + test_must_fail git verify-commit eighth-signed-alt 2>actual && + grep "${GPGSSH_GOOD_SIGNATURE_UNTRUSTED}" actual && + ! grep "${GPGSSH_BAD_SIGNATURE}" actual && -+ grep "${KEY_NOT_TRUSTED}" actual ++ grep "${GPGSSH_KEY_NOT_TRUSTED}" actual +' + +test_expect_success GPGSSH 'verify-commit exits success with matching minTrustLevel' ' 8: 0763517d62d = 8: b66e3e0284c ssh signing: tests for logs, tags & push certs 9: a5add98197a ! 9: 07afb94ed83 ssh signing: test that gpg fails for unkown keys @@ Metadata Author: Fabian Stelzer <fs@gigacodes.de> ## Commit message ## - ssh signing: test that gpg fails for unkown keys + ssh signing: test that gpg fails for unknown keys Test that verify-commit/tag will fail when a gpg key is completely unknown. To do this we have to generate a key, use it for a signature