From: Fabian Stelzer Date: Mon, 12 Jul 2021 20:35:55 GMT Subject: Re: [PATCH v2] Add commit, tag & push signing/verification via SSH keys using ssh-keygen Message-ID: In-Reply-To: <87y2ab30yr.fsf@evledraar.gmail.com> On 12.07.21 18:55, Ævar Arnfjörð Bjarmason wrote: I'll change all the whitespace / comments / style issues with the next commit. Thanks >> + sigc->result = 'G'; >> + sigc->trust_level = TRUST_FULLY; >> + >> + next = strchrnul(output, ' '); // 'principal' >> + replace_cstring(&sigc->signer, output, next); >> + output = next + 1; >> + next = strchrnul(output, ' '); // 'with' >> + output = next + 1; >> + next = strchrnul(output, ' '); // KEY Type >> + output = next + 1; >> + next = strchrnul(output, ' '); // 'key' >> + output = next + 1; > FWIW for new code we'd probably use string_list_split() or > string_list_split_in_place() or strbuf_split_buf() or something, but I > see this is following the existing pattern in the file... I agree. This is my first patch in the git codebase so it takes a bit getting used to all the available utilities. >> + const char *payload, size_t payload_size, >> + const char *signature, size_t signature_size) >> { >> - struct child_process gpg = CHILD_PROCESS_INIT; >> - struct gpg_format *fmt; >> + struct child_process ssh_keygen = CHILD_PROCESS_INIT; >> struct tempfile *temp; >> int ret; >> - struct strbuf buf = STRBUF_INIT; >> + const char *line; >> + size_t trust_size; >> + char *principal; >> + struct strbuf ssh_keygen_out = STRBUF_INIT; >> + struct strbuf ssh_keygen_err = STRBUF_INIT; >> >> temp = mks_tempfile_t(".git_vtag_tmpXXXXXX"); >> if (!temp) >> @@ -279,29 +342,125 @@ static int verify_signed_buffer(const char *payload, size_t payload_size, >> return -1; >> } >> >> - fmt = get_format_by_sig(signature); >> - if (!fmt) >> - BUG("bad signature '%s'", signature); >> + // Find the principal from the signers >> + strvec_pushl(&ssh_keygen.args, fmt->program, >> + "-Y", "find-principals", >> + "-f", get_ssh_allowed_signers(), >> + "-s", temp->filename.buf, >> + NULL); >> + ret = pipe_command(&ssh_keygen, NULL, 0, &ssh_keygen_out, 0, &ssh_keygen_err, 0); >> + if (strstr(ssh_keygen_err.buf, "unknown option")) { >> + error(_("openssh version > 8.2p1 is needed for ssh signature verification (ssh-keygen needs -Y find-principals/verify option)")); >> + } >> + if (ret || !ssh_keygen_out.len) { >> + // We did not find a matching principal in the keyring - Check without validation >> + child_process_init(&ssh_keygen); >> + strvec_pushl(&ssh_keygen.args, fmt->program, >> + "-Y", "check-novalidate", >> + "-n", "git", >> + "-s", temp->filename.buf, >> + NULL); >> + ret = pipe_command(&ssh_keygen, payload, payload_size, &ssh_keygen_out, 0, &ssh_keygen_err, 0); >> + } else { >> + // Check every principal we found (one per line) >> + for (line = ssh_keygen_out.buf; *line; line = strchrnul(line + 1, '\n')) { > Hrm, can't we use strbuf_getline() here with the underlying io_pump API > that pipe_command() uses, instead of slurping it all up, and then > splitting on '\n' ourselves? (I'm not sure) Sounds good. I'll give it a try. >> + while (*line == '\n') >> + line++; >> + if (!*line) >> + break; >> + >> + trust_size = strcspn(line, " \n"); >> + principal = xmemdupz(line, trust_size); >> + >> + child_process_init(&ssh_keygen); >> + strbuf_release(&ssh_keygen_out); >> + strbuf_release(&ssh_keygen_err); >> + strvec_push(&ssh_keygen.args,fmt->program); >> + // We found principals - Try with each until we find a match >> + strvec_pushl(&ssh_keygen.args, "-Y", "verify", >> + //TODO: sprintf("-Overify-time=%s", commit->date...), >> + "-n", "git", >> + "-f", get_ssh_allowed_signers(), >> + "-I", principal, >> + "-s", temp->filename.buf, >> + NULL); >> + >> + if (ssh_revocation_file && file_exists(ssh_revocation_file)) { >> + strvec_pushl(&ssh_keygen.args, "-r", ssh_revocation_file, NULL); > Do we want to silently ignore missing but configured revocation files? I'll add a warning > >> + } >> + >> + sigchain_push(SIGPIPE, SIG_IGN); >> + ret = pipe_command(&ssh_keygen, payload, payload_size, >> + &ssh_keygen_out, 0, &ssh_keygen_err, 0); >> + sigchain_pop(SIGPIPE); >> + >> + ret &= starts_with(ssh_keygen_out.buf, "Good"); >> + if (ret == 0) >> + break; >> + } >> + } >> + >> + sigc->payload = xmemdupz(payload, payload_size); >> + strbuf_stripspace(&ssh_keygen_out, 0); >> + strbuf_stripspace(&ssh_keygen_err, 0); >> + strbuf_add(&ssh_keygen_out, ssh_keygen_err.buf, ssh_keygen_err.len); >> + sigc->output = strbuf_detach(&ssh_keygen_out, NULL); >> + >> + //sigc->gpg_output = strbuf_detach(&ssh_keygen_err, NULL); // This flip around is broken... > Broken how? And why the commented-out code as part of the patch? Sorry, i should have removed it. The original code assigned gpg's stdout to gpg_status and stdout to gpg_output which can be a bit confusing. > >> - status = verify_signed_buffer(payload, plen, signature, slen, >> - &gpg_output, &gpg_status); >> - if (status && !gpg_output.len) >> - goto out; >> - sigc->payload = xmemdupz(payload, plen); >> - sigc->gpg_output = strbuf_detach(&gpg_output, NULL); >> - sigc->gpg_status = strbuf_detach(&gpg_status, NULL); >> - parse_gpg_output(sigc); >> + fmt = get_format_by_sig(signature); >> + if (!fmt) >> + BUG("bad signature '%s'", signature); > So if we run this from receive-pack or whatever we'll BUG() out? I.e. I > think this should be an fsck check or something, but not a BUG(), or > does this not rely on potentially bad object-store state? The BUG() call is also from the original code. I agree that it should be handled differently. Unfortunately this call is also the reason that when trying to verify a new SSH signature with a current git version you'll get a segfault from this BUG() :/ I'm not sure if i can do anything about this other than adding a completely new tag in the commit itself instead of "gpgsig" which might be quite involved. I haven't looked into that too much yet. > >> +static char *get_ssh_key_fingerprint(const char *signing_key) { >> + struct child_process ssh_keygen = CHILD_PROCESS_INIT; >> + int ret = -1; >> + struct strbuf fingerprint_stdout = STRBUF_INIT; >> + struct strbuf **fingerprint; >> + >> + /* For SSH Signing this can contain a filename or a public key >> + * For textual representation we usually want a fingerprint >> + */ >> + if (istarts_with(signing_key, "ssh-")) { >> + strvec_pushl(&ssh_keygen.args, "ssh-keygen", >> + "-lf", "-", >> + NULL); >> + ret = pipe_command(&ssh_keygen, signing_key, strlen(signing_key), &fingerprint_stdout, 0, NULL, 0); >> + } else { >> + strvec_pushl(&ssh_keygen.args, "ssh-keygen", >> + "-lf", configured_signing_key, >> + NULL); >> + ret = pipe_command(&ssh_keygen, NULL, 0, &fingerprint_stdout, 0, NULL, 0); >> + if (!!ret) >> + die_errno(_("failed to get the ssh fingerprint for key '%s'"), signing_key); >> + fingerprint = strbuf_split_max(&fingerprint_stdout, ' ', 3); >> + if (fingerprint[1]) { >> + return strbuf_detach(fingerprint[1], NULL); >> + } >> + } >> + die_errno(_("failed to get the ssh fingerprint for key '%s'"), signing_key); >> +} > Her you declare a ret that's not used at all in the "istarts_with" > branch, and we fall through to die_errno()? I'll clean up the logic. Thanks > > [I stopped reading mostly at this point] > >> [...] >> +# test_expect_success GPGSSH 'detect fudged commit with double signature' ' >> +# sed -e "/gpgsig/,/END PGP/d" forged1 >double-base && >> +# sed -n -e "/gpgsig/,/END PGP/p" forged1 | \ >> +# sed -e "s/^$(test_oid header)//;s/^ //" | gpg --dearmor >double-sig1.sig && >> +# gpg -o double-sig2.sig -u 29472784 --detach-sign double-base && >> +# cat double-sig1.sig double-sig2.sig | gpg --enarmor >double-combined.asc && >> +# sed -e "s/^\(-.*\)ARMORED FILE/\1SIGNATURE/;1s/^/$(test_oid header) /;2,\$s/^/ /" \ >> +# double-combined.asc > double-gpgsig && >> +# sed -e "/committer/r double-gpgsig" double-base >double-commit && >> +# git hash-object -w -t commit double-commit >double-commit.commit && >> +# test_must_fail git verify-commit $(cat double-commit.commit) && >> +# git show --pretty=short --show-signature $(cat double-commit.commit) >double-actual && >> +# grep "BAD signature from" double-actual && >> +# grep "Good signature from" double-actual >> +# ' >> + >> +# test_expect_success GPGSSH 'show double signature with custom format' ' >> +# cat >expect <<-\EOF && >> +# E >> + >> + >> + >> + >> +# EOF >> +# git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" $(cat double-commit.commit) >actual && >> +# test_cmp expect actual >> +# ' > Perhaps you're looking for test_expect_failure for TODO tests? Yes. Although this test explicitly i'm having a hard time to duplicate for ssh. I'm still trying to find out if the duplicate signature thing is actually an issue with ssh. > > I think this patch is *way* past the point of benefitting from being > split into a patch series. It grew from ~200 lines added to ~1k. Sure, I can easily split the patch into seperate commits. But do i create a v3 patch from this or issue a new pull request? The diff between v2 & v3 would be quite useless otherwise wouldn't it? And maybe another beginner contribution question: When i make changes to a patchset do i put new changes from the review on top as new commits or do i edit the existing commits? If so what is the workflow you normally use for this? fixup commits? I know about those but haven't worked with them before. Thanks for your help!