Re: [PATCH v6 2/9] ssh signing: add ssh signature format and signing using ssh keys
From: Jonathan Tan <hidden>
Date: 2021-07-28 22:45:30
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.
quoted hunk ↗ jump to hunk
@@ -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?