From: Fabian Stelzer Date: Thu, 15 Jul 2021 08:28:33 GMT Subject: Re: [PATCH v3 4/9] ssh signing: sign using either gpg or ssh keys Message-ID: <50eca062-f2e7-28d7-09ae-97250f620be4@gigacodes.de> In-Reply-To: On 14.07.21 22:32, Junio C Hamano wrote: > "Fabian Stelzer via GitGitGadget" writes: > >> int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key) >> { >> - struct child_process gpg = CHILD_PROCESS_INIT; >> + struct child_process signer = CHILD_PROCESS_INIT; >> int ret; >> size_t i, j, bottom; >> - struct strbuf gpg_status = STRBUF_INIT; >> + struct strbuf signer_stderr = STRBUF_INIT; >> + struct tempfile *temp = NULL, *buffer_file = NULL; >> + char *ssh_signing_key_file = NULL; >> + struct strbuf ssh_signature_filename = STRBUF_INIT; >> >> - strvec_pushl(&gpg.args, >> - use_format->program, >> + if (!strcmp(use_format->name, "ssh")) { > I wonder if we can split the body of these if/else clauses into > separate helper functions, point them with fmt structure and > dispatch via use_format->sign_buffer pointer, just like I suggested > how to do the same on the signature validation side. yes, i like the idea and will do that. > >> + if (!signing_key || signing_key[0] == '\0') >> + return error(_("user.signingkey needs to be set for ssh signing")); >> + >> + >> + if (istarts_with(signing_key, "ssh-")) { >> + /* A literal ssh key */ >> + temp = mks_tempfile_t(".git_signing_key_tmpXXXXXX"); >> + if (!temp) >> + return error_errno(_("could not create temporary file")); >> + if (write_in_full(temp->fd, signing_key, strlen(signing_key)) < 0 || >> + close_tempfile_gently(temp) < 0) { >> + error_errno(_("failed writing ssh signing key to '%s'"), >> + temp->filename.buf); >> + delete_tempfile(&temp); >> + return -1; >> + } >> + ssh_signing_key_file= temp->filename.buf; >> + } else { >> + /* We assume a file */ >> + ssh_signing_key_file = expand_user_path(signing_key, 1); >> + } >> + >> + buffer_file = mks_tempfile_t(".git_signing_buffer_tmpXXXXXX"); >> + if (!buffer_file) >> + return error_errno(_("could not create temporary file")); >> + if (write_in_full(buffer_file->fd, buffer->buf, buffer->len) < 0 || >> + close_tempfile_gently(buffer_file) < 0) { >> + error_errno(_("failed writing ssh signing key buffer to '%s'"), >> + buffer_file->filename.buf); >> + delete_tempfile(&buffer_file); >> + return -1; >> + } >> + >> + strvec_pushl(&signer.args, use_format->program , >> + "-Y", "sign", >> + "-n", "git", >> + "-f", ssh_signing_key_file, >> + buffer_file->filename.buf, >> + NULL); >> + >> + sigchain_push(SIGPIPE, SIG_IGN); >> + ret = pipe_command(&signer, NULL, 0, NULL, 0, &signer_stderr, 0); >> + sigchain_pop(SIGPIPE); >> + >> + strbuf_addbuf(&ssh_signature_filename, &buffer_file->filename); >> + strbuf_addstr(&ssh_signature_filename, ".sig"); >> + if (strbuf_read_file(signature, ssh_signature_filename.buf, 2048) < 0) { >> + error_errno(_("failed reading ssh signing data buffer from '%s'"), >> + ssh_signature_filename.buf); >> + } >> + unlink_or_warn(ssh_signature_filename.buf); >> + strbuf_release(&ssh_signature_filename); >> + delete_tempfile(&buffer_file); >> + } else { >> + strvec_pushl(&signer.args, use_format->program , >> "--status-fd=2", >> "-bsau", signing_key, >> NULL); >> >> - bottom = signature->len; >> - >> /* >> * When the username signingkey is bad, program could be terminated >> * because gpg exits without reading and then write gets SIGPIPE. >> */ >> sigchain_push(SIGPIPE, SIG_IGN); >> - ret = pipe_command(&gpg, buffer->buf, buffer->len, >> - signature, 1024, &gpg_status, 0); >> + ret = pipe_command(&signer, buffer->buf, buffer->len, signature, 1024, &signer_stderr, 0); >> sigchain_pop(SIGPIPE); >> + } >> + >> + bottom = signature->len; >> + >> + if (temp) >> + delete_tempfile(&temp); >> >> - ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED "); >> - strbuf_release(&gpg_status); >> + if (!strcmp(use_format->name, "ssh")) { >> + if (strstr(signer_stderr.buf, "usage:")) { >> + error(_("openssh version > 8.2p1 is needed for ssh signing (ssh-keygen needs -Y sign option)")); > This looks iffy. You do call error() to show the error message, but > you do not set "ret", which affects how the return value from the > function is computed at the end of the function. I can of course fix the ret logic, but i'm not happy with this check in general either :/ The problem is that ssh-keygen seems to give different error messages especially in between the versions when the command was added (8.1 -> 8.2) and mac os x has one of those by default. The check in the t/lib-gpg.sh is much safer, but requires an additional call to ssh-keygen which i wanted to avoid here. > >> + } >> + } else { >> + ret |= !strstr(signer_stderr.buf, "\n[GNUPG:] SIG_CREATED "); >> + } >> + strbuf_release(&signer_stderr); >> if (ret) >> return error(_("gpg failed to sign the data")); > And this error message belongs to the GPG half of the logic, not > ssh (you are allowed to have a separate "ssh failed to sign" > message, of course, but the point is that the error message emission > should happen in the codepath dispatched for each crypto backend. > > And of course, again the "if (ssh) {do this shiny new ssh thing} > else {do gpg thing}" structure is questionable. We should be > dispatching with use_format->fn (whatever the method name is), no?  I will rewrite this with the fmt->fn logic. > > THanks. > -- GIGACODES GmbH | Dr. Hermann-Neubauer-Ring 32 | D-63500 Seligenstadt www.gigacodes.de | fs@gigacodes.de Phone +49 6182 8955-114 | Fax +49 6182 8955-299 | HRB 40711 AG Offenbach a. Main Geschäftsführer: Fabian Stelzer | Umsatzsteuer-ID DE219379936