On 29.07.21 21:09, Josh Steadmon wrote:
Thanks for this series, it sounds like a great idea. I have a few
comments, inline below.
Thanks for your review and help with this patch.
On 2021.07.28 19:36, Fabian Stelzer via GitGitGadget wrote:
[snip]
+ ssh_signing_key_file = key_file->filename.buf;
You probably want to call strbuf_detach() here, because...
+ } else {
+ /* We assume a file */
+ ssh_signing_key_file = expand_user_path(signing_key, 1);
+ }
... you need to free the memory returned by expand_user_path(). If you
detach the strbuf above, you can unconditionally
free(ssh_signing_key_file) at the end of this function.
fixed. thanks
+
+ buffer_file = mks_tempfile_t(".git_signing_buffer_tmpXXXXXX");
+ if (!buffer_file) {
+ error_errno(_("could not create temporary file"));
+ goto out;
+ }
+
+ 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);
+ goto out;
+ }
+
+ 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);
+
+ if (ret) {
+ if (strstr(signer_stderr.buf, "usage:"))
+ error(_("ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)"));
I share Jonathan Tan's concern about checking for "usage:" in the stderr
output here. I think in patch 6 the tests rely on a specific return code
to check that "-Y sign" is working as expected; can that be used here
instead?
In the test setup i first check if ssh-keygen at all is present (exit
code 127 means command not found). Afterwards i check for a specific
error message from the command if it is present. Not sure how portable
this is, but i can do that because i give known invalid parameters to
it. I can't do this here without doing an additional call to ssh-keygen
just to check this.
+
+ error("%s", signer_stderr.buf);
+ goto out;
+ }
+
+ bottom = signature->len;
+
+ strbuf_addbuf(&ssh_signature_filename, &buffer_file->filename);
+ strbuf_addstr(&ssh_signature_filename, ".sig");
+ if (strbuf_read_file(signature, ssh_signature_filename.buf, 0) < 0) {
+ error_errno(
+ _("failed reading ssh signing data buffer from '%s'"),
+ ssh_signature_filename.buf);
+ }
+ unlink_or_warn(ssh_signature_filename.buf);
+
+ /* Strip CR from the line endings, in case we are on Windows. */
+ remove_cr_after(signature, bottom);
+
+out:
+ if (key_file)
+ delete_tempfile(&key_file);
+ if (buffer_file)
+ delete_tempfile(&buffer_file);
+ strbuf_release(&signer_stderr);
+ strbuf_release(&ssh_signature_filename);
+ return ret;
+}
--
gitgitgadget
