Keep the commit titles to 50 characters or fewer. E.g.:
gpg-interface: teach "ssh" gpg.format
> implements the actual sign_buffer_ssh operation and move some shared
> cleanup code into a strbuf function
Capitalization and punctuation.
> Set gpg.format = ssh and user.signingkey to either a ssh public key
> string (like from an authorized_keys file), or a ssh key file.
> If the key file or the config value itself contains only a public key
> then the private key needs to be available via ssh-agent.
>
> gpg.ssh.program can be set to an alternative location of ssh-keygen.
> A somewhat recent openssh version (8.2p1+) of ssh-keygen is needed for
> this feature. Since only ssh-keygen is needed it can this way be
> installed seperately without upgrading your system openssh packages.
I notice that end-user documentation (e.g. about gpg.ssh.program) is in
its own patch, but could that be added as functionality is being
implemented? That makes it easier for reviewers to understand what's
being implemented in each patch.
> @@ -463,12 +482,30 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
> return use_format->sign_buffer(buffer, signature, signing_key);
> }
>
> +/*
> + * Strip CR from the line endings, in case we are on Windows.
> + * NEEDSWORK: make it trim only CRs before LFs and rename
> + */
> +static void remove_cr_after(struct strbuf *buffer, size_t offset)
> +{
> + size_t i, j;
> +
> + for (i = j = offset; i < buffer->len; i++) {
> + if (buffer->buf[i] != '\r') {
> + if (i != j)
> + buffer->buf[j] = buffer->buf[i];
> + j++;
> + }
> + }
> + strbuf_setlen(buffer, j);
> +}
In the future, I would prefer refactoring like this to be in its own
patch. For the moment, this should probably be called "remove_cr" (no
"after" as CRs are removed wherever they are in the string).
> +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, keylen;
> + struct strbuf signer_stderr = STRBUF_INIT;
> + struct tempfile *key_file = 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 (starts_with(signing_key, "ssh-")) {
> + /* A literal ssh key */
> + key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX");
> + if (!key_file)
> + return error_errno(
> + _("could not create temporary file"));
> + keylen = strlen(signing_key);
> + if (write_in_full(key_file->fd, signing_key, keylen) < 0 ||
> + close_tempfile_gently(key_file) < 0) {
> + error_errno(_("failed writing ssh signing key to '%s'"),
> + key_file->filename.buf);
> + goto out;
> + }
> + ssh_signing_key_file = key_file->filename.buf;
> + } else {
> + /* We assume a file */
> + ssh_signing_key_file = expand_user_path(signing_key, 1);
> + }
A config that has 2 modes of operation is quite error-prone, I think.
For example, a user could put a path starting with "ssh-" (admittedly
unlikely since it would usually be an absolute path, but not
impossible). And also from an implementation point of view, here the
"ssh-" is case-sensitive, but in a future patch, there is a "ssh-" that
is case-insensitive.
Can this just always take a path?
> + if (ret) {
> + if (strstr(signer_stderr.buf, "usage:"))
> + error(_("ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)"));
> +
> + error("%s", signer_stderr.buf);
> + goto out;
> + }
Checking for "usage:" seems fragile - a binary running in a different
locale might emit a different string, and legitimate output may somehow
contain the string "usage:". Is there a different way to detect a
version mismatch?
