"Fabian Stelzer via GitGitGadget" [off-list ref] 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.
+ 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.
+ }
+ } 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?
THanks.
On 14.07.21 22:32, Junio C Hamano wrote:
"Fabian Stelzer via GitGitGadget" [off-list ref] writes:
quoted
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.
quoted
+ 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.
quoted
+ }
+ } 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