On 20.07.21 01:53, Junio C Hamano wrote:
"Fabian Stelzer via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
@@ -65,6 +73,14 @@ static struct gpg_format gpg_format[] = {
.verify_signed_buffer = verify_gpg_signed_buffer,
.sign_buffer = sign_buffer_gpg,
},
+ {
+ .name = "ssh",
+ .program = "ssh-keygen",
+ .verify_args = ssh_verify_args,
+ .sigs = ssh_sigs,
+ .verify_signed_buffer = NULL, /* TODO */
+ .sign_buffer = sign_buffer_ssh
+ },
};
A payload a malicious person may feed this version of Git can have a
pattern that happens to match the ssh_sigs[] string, and the code
will blindly try to call .verify_signed_buffer==NULL and die, no?
That is not the end of the world; as long as we know that with the
above "TODO" comment it is probably OK.
I thought about adding an if(!fmt->sign) BUG() but since these callbacks
are static it shoud really only be an issue between patches of the set.
@@ -463,12 +482,26 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
return use_format->sign_buffer(buffer, signature, signing_key);
}
+static void strbuf_trim_trailing_cr(struct strbuf *buffer, int offset)
+{
This removes any and all CR, not just trimming the trailing ones, so
the function is misnamed. Call it remove_cr_after() perhaps?
Alternatively we could tighten the implementation and strip only the
CR that come immediately before a LF. That would be a better longer
term thing to do, but because you are lifting an existing code from
the end of the gpg side of the thing, it may make sense to keep the
implementation as-is, but give it a name that is more faithful to
what it actually does. When the dust settles, we may want to
revisit and fix this helper function to actually trim CRLF into LF
(and leave CR in the middle of lines intact), but I do not think it
is urgent. Just leaving "NEEDSWORK: make it trim only CRs before LFs
and rename" comment would be OK.
Agreed. I've renamed it and added the comment.
Shouldn't the offset (aka bottom) be of type size_t?
fixed
+static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
+ const char *signing_key)
+{
+ struct child_process signer = CHILD_PROCESS_INIT;
+ int ret = -1;
+ size_t bottom;
+ 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;
+
+ if (!signing_key || signing_key[0] == '\0')
+ return error(
+ _("user.signingkey needs to be set for ssh signing"));
+
+ if (istarts_with(signing_key, "ssh-")) {
Is it common in the ssh world to treat ssh- prefix as case
insensitive? Not a strong objection but I tend to prefer to start
strict unless there is a good reason to be loose when we do not have
to, as loosening after the fact is much easier than tightening after
starting with a loose definition.
I don't think so. I will make it case sensitive.
+ /* 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 ||
"keylen = strlen(signing_key)" before that line, for example, could
have easily avoided the line-wrapping at such a place. Wrapping at
places like after ||, i.e. after an operator with a low precedence,
would make the code easier to follow.
agreed.
+ close_tempfile_gently(temp) < 0) {
+ error_errno(_("failed writing ssh signing key to '%s'"),
+ temp->filename.buf);
+ goto out;
+ }
+ ssh_signing_key_file = temp->filename.buf;
It is kind'a sad that we need a fresh temporary file every time, but
we can easily tell the user in the documentation that they can use a
file with a key in it to avoid it, so it's OK (actually, better than
OK, as without this, we may not consume temporary files but we won't
offer an ability to take a literal key string).
Is ".git_whatever file in the current directory" a good place to
have this temporary file? I would have expected that we would use
either $GIT_DIR, $HOME, or $TMPDIR for a thing like this (with
different pros-and-cons discussion). At least it is consistent with
how a temporary file for the payload to be sign-verified is created,
so let's leave it as-is.
Intuitively i thought just using mks_tempfile_t() would choose a good
dir for such files.
+ } 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) {
+ 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,
Wrap the line before "-n" to keep "-n" and "git" together, if "git"
is meant as an argument to the "-n" option.
done. some things clang-format can't really understand. overall it is
quite helpful but a few things i still had to reformat.
+ 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 && strstr(signer_stderr.buf, "usage:")) {
+ error(_("ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)"));
+ goto out;
This error message is important to give to the end users, but is it
enough? That is, unless "usage:" does not appear, we show the whole
raw error message and that would help end users and those helping
them to diagnose the issue, but once the underlying program says
"usage:", no matter what else it says, it is hidden by this code,
since we assume it is a wrong version of openssh.
+ }
+
+ if (ret) {
+ error("%s", signer_stderr.buf);
+ goto out;
+ }
Also, prehaps
if (ret) {
if (strstr(..., "usage"))
error(_("ssh-keygen -Y sign is needed..."));
else
error("%s", signer_stderr.buf);
goto out;
}
would be easier to follow.
I have changed this to:
if (ret) {
if (strstr(..., "usage"))
error(_("ssh-keygen -Y sign is needed..."));
error("%s", signer_stderr.buf);
goto out;
}
and removed the if (ret) further down in the function that had no effect.
I had removed the raw stderr output from verify & sign because it
becomes quite unreadable when doing a "git log --show-signature" with
ssh signatures present and not support in ssh for it.
I think in case of signing the full output is good. The user is taking
an active action (wanting to sign something) so we should give them all
the help we can when things go wrong.
The output for verification is debatable. The config might have a
"log.showSignature" for verifying gpg signatures and when ssh signatures
show up we should tell them it's not supported on their setup but
probably not showing endless lines of errors when they do a "git log". A
"git verify-commit" might be a different case. But code-path-wise this
is the same thing at the moment.
+ error(_("ssh failed to sign the data"));
+ goto out;
+ }
+
+ /* Strip CR from the line endings, in case we are on Windows. */
+ strbuf_trim_trailing_cr(signature, bottom);
+
+out:
+ if (temp)
+ delete_tempfile(&temp);
+ if (buffer_file)
+ delete_tempfile(&buffer_file);
It is clear that the latter one was holding the contents of the
buffer to be signed, but reminding the readers what "temp" was about
would be a good move. Perhaps renaming the variable to "key_file"
or something may help?
renamed to "key_file"
+ strbuf_release(&signer_stderr);
+ strbuf_release(&ssh_signature_filename);
+ return ret;
+}
Looking good, except for the "when does 'ret' get updated?
shouldn't we refrain from reading the resulting buffer when it is
set?" question.
Thanks for a pleasant read.
Thanks a lot for your support with this!
