Bug: send-pack does not respect http.signingkey

Bug: send-pack does not respect http.signingkey

[Dave Borowitz]

When git-send-pack is exec'ed, as is done by git-remote-http, it does
not reread the config, so it does not respect the configured
http.signingkey, either from the config file or -c on the command
line. Thus it is currently impossible to specify a signing key over
HTTP, other than the default one matching the "Name <email>" format in
the keyring.

This is not an issue for git:// as send-pack is executed directly in
the same process that reads the config.

---
 gpg-interface.c | 1 +
 run-command.c   | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 68b0c81..e0ffcb0 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -87,6 +87,7 @@ void set_signing_key(const char *key)
 int git_gpg_config(const char *var, const char *value, void *cb)
 {
  if (!strcmp(var, "user.signingkey")) {
+ fprintf(stderr, "setting %s\n", value);
  set_signing_key(value);
  }
  if (!strcmp(var, "gpg.program")) {
diff --git a/run-command.c b/run-command.c
index 4d73e90..39ae8d5 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "run-command.h"
+#include "gpg-interface.h"
 #include "exec_cmd.h"
 #include "sigchain.h"
 #include "argv-array.h"
@@ -344,7 +345,7 @@ fail_pipe:
  cmd->err = fderr[0];
  }

- trace_argv_printf(cmd->argv, "trace: run_command:");
+ trace_argv_printf(cmd->argv, "trace: run_command (%s):", get_signing_key());
  fflush(NULL);

 #ifndef GIT_WINDOWS_NATIVE
-- 
2.4.3.573.g4eafbef

Re: Bug: send-pack does not respect http.signingkey

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> When git-send-pack is exec'ed, as is done by git-remote-http, it does
> not reread the config, so it does not respect the configured
> http.signingkey, either from the config file or -c on the command
> line. Thus it is currently impossible to specify a signing key over
> HTTP, other than the default one matching the "Name <email>" format in
> the keyring.
>
> This is not an issue for git:// as send-pack is executed directly in
> the same process that reads the config.

Interesting.  I agree that it would be a problem not to be able to
specify which signing key to use.

Perhaps something like this?

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index b564a77..57c3a9c 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -11,6 +11,7 @@
 #include "transport.h"
 #include "version.h"
 #include "sha1-array.h"
+#include "gpg-interface.h"
 
 static const char send_pack_usage[] =
 "git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]\n"
@@ -113,6 +114,8 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	int from_stdin = 0;
 	struct push_cas_option cas = {0};
 
+	git_config(git_gpg_config, NULL);
+
 	argv++;
 	for (i = 1; i < argc; i++, argv++) {
 		const char *arg = *argv;

Re: Bug: send-pack does not respect http.signingkey

[Dave Borowitz]

On Thu, Jul 16, 2015 at 1:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dave Borowitz <dborowitz@google.com> writes:
>
>> When git-send-pack is exec'ed, as is done by git-remote-http, it does
>> not reread the config, so it does not respect the configured
>> http.signingkey, either from the config file or -c on the command
>> line. Thus it is currently impossible to specify a signing key over
>> HTTP, other than the default one matching the "Name <email>" format in
>> the keyring.
>>
>> This is not an issue for git:// as send-pack is executed directly in
>> the same process that reads the config.
>
> Interesting.  I agree that it would be a problem not to be able to
> specify which signing key to use.
>
> Perhaps something like this?

Seems like it should work.

Jonathan had suggested there might be some principled reason why
send-pack does not respect config options, and suggested passing it in
as a flag. But that would be more work, certainly, as it would also
have to get passed through git-remote-http somehow.

> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index b564a77..57c3a9c 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -11,6 +11,7 @@
>  #include "transport.h"
>  #include "version.h"
>  #include "sha1-array.h"
> +#include "gpg-interface.h"
>
>  static const char send_pack_usage[] =
>  "git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]\n"
> @@ -113,6 +114,8 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
>         int from_stdin = 0;
>         struct push_cas_option cas = {0};
>
> +       git_config(git_gpg_config, NULL);
> +
>         argv++;
>         for (i = 1; i < argc; i++, argv++) {
>                 const char *arg = *argv;

Re: Bug: send-pack does not respect http.signingkey

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> On Thu, Jul 16, 2015 at 1:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Perhaps something like this?
>
> Seems like it should work.
>
> Jonathan had suggested there might be some principled reason why
> send-pack does not respect config options, and suggested passing it in
> as a flag. But that would be more work, certainly, as it would also
> have to get passed through git-remote-http somehow.

I actually was wondering about exactly the same thing as Jonathan,
and that is where my "Perhaps" came from.

Re: Bug: send-pack does not respect http.signingkey

[Dave Borowitz]

On Thu, Jul 16, 2015 at 1:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dave Borowitz <dborowitz@google.com> writes:
>
>> On Thu, Jul 16, 2015 at 1:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> Perhaps something like this?
>>
>> Seems like it should work.
>>
>> Jonathan had suggested there might be some principled reason why
>> send-pack does not respect config options, and suggested passing it in
>> as a flag. But that would be more work, certainly, as it would also
>> have to get passed through git-remote-http somehow.
>
> I actually was wondering about exactly the same thing as Jonathan,
> and that is where my "Perhaps" came from.

I will say, though, as the maintainer of a handful of custom remote
helpers, I would prefer a solution that does not involve changing the
implementation of those just to pass this configuration through. So my
vote would be for send-pack to respect the normal config options.

Not sure what that would mean for -c on the command line, though.

Re: Bug: send-pack does not respect http.signingkey

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> On Thu, Jul 16, 2015 at 1:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Dave Borowitz <dborowitz@google.com> writes:
>>
>>> On Thu, Jul 16, 2015 at 1:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>>> Perhaps something like this?
>>>
>>> Seems like it should work.
>>>
>>> Jonathan had suggested there might be some principled reason why
>>> send-pack does not respect config options, and suggested passing it in
>>> as a flag. But that would be more work, certainly, as it would also
>>> have to get passed through git-remote-http somehow.
>>
>> I actually was wondering about exactly the same thing as Jonathan,
>> and that is where my "Perhaps" came from.
>
> I will say, though, as the maintainer of a handful of custom remote
> helpers, I would prefer a solution that does not involve changing the
> implementation of those just to pass this configuration through. 

That is not a controversial part ;-)

> So my
> vote would be for send-pack to respect the normal config options.

The thing is what should be included in the "normal" config options.

The "something like this?" patch was deliberately narrow, including
only the GPG thing and nothing else.  But anticipating that the ref
backend would be per repo configuration, and send-pack would want to
read from refs (and possibly write back tracking?), we may want to
prepare ourselves by reading a bit wider than "GPG thing and nothing
else", e.g. git_default_config() or something like that.

Re: Bug: send-pack does not respect http.signingkey

[Dave Borowitz]

On Thu, Jul 16, 2015 at 2:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dave Borowitz <dborowitz@google.com> writes:
>
>> On Thu, Jul 16, 2015 at 1:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Dave Borowitz <dborowitz@google.com> writes:
>>>
>>>> On Thu, Jul 16, 2015 at 1:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>
>>>>> Perhaps something like this?
>>>>
>>>> Seems like it should work.
>>>>
>>>> Jonathan had suggested there might be some principled reason why
>>>> send-pack does not respect config options, and suggested passing it in
>>>> as a flag. But that would be more work, certainly, as it would also
>>>> have to get passed through git-remote-http somehow.
>>>
>>> I actually was wondering about exactly the same thing as Jonathan,
>>> and that is where my "Perhaps" came from.
>>
>> I will say, though, as the maintainer of a handful of custom remote
>> helpers, I would prefer a solution that does not involve changing the
>> implementation of those just to pass this configuration through.
>
> That is not a controversial part ;-)
>
>> So my
>> vote would be for send-pack to respect the normal config options.
>
> The thing is what should be included in the "normal" config options.
>
> The "something like this?" patch was deliberately narrow, including
> only the GPG thing and nothing else.  But anticipating that the ref
> backend would be per repo configuration, and send-pack would want to
> read from refs (and possibly write back tracking?), we may want to
> prepare ourselves by reading a bit wider than "GPG thing and nothing
> else", e.g. git_default_config() or something like that.

Ah, now I understand the question. I have no opinion other than that
we shouldn't let discussion about future features prevent us from
fixing this obvious signed push bug :)

Re: Bug: send-pack does not respect http.signingkey

[Dave Borowitz]

On Thu, Jul 16, 2015 at 3:08 PM, Dave Borowitz <dborowitz@google.com> wrote:
> On Thu, Jul 16, 2015 at 2:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Dave Borowitz <dborowitz@google.com> writes:
>>
>>> On Thu, Jul 16, 2015 at 1:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Dave Borowitz <dborowitz@google.com> writes:
>>>>
>>>>> On Thu, Jul 16, 2015 at 1:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>>
>>>>>> Perhaps something like this?
>>>>>
>>>>> Seems like it should work.
>>>>>
>>>>> Jonathan had suggested there might be some principled reason why
>>>>> send-pack does not respect config options, and suggested passing it in
>>>>> as a flag. But that would be more work, certainly, as it would also
>>>>> have to get passed through git-remote-http somehow.
>>>>
>>>> I actually was wondering about exactly the same thing as Jonathan,
>>>> and that is where my "Perhaps" came from.
>>>
>>> I will say, though, as the maintainer of a handful of custom remote
>>> helpers, I would prefer a solution that does not involve changing the
>>> implementation of those just to pass this configuration through.
>>
>> That is not a controversial part ;-)
>>
>>> So my
>>> vote would be for send-pack to respect the normal config options.
>>
>> The thing is what should be included in the "normal" config options.
>>
>> The "something like this?" patch was deliberately narrow, including
>> only the GPG thing and nothing else.  But anticipating that the ref
>> backend would be per repo configuration, and send-pack would want to
>> read from refs (and possibly write back tracking?), we may want to
>> prepare ourselves by reading a bit wider than "GPG thing and nothing
>> else", e.g. git_default_config() or something like that.
>
> Ah, now I understand the question. I have no opinion other than that
> we shouldn't let discussion about future features prevent us from
> fixing this obvious signed push bug :)

Should I formally send a patch with your configuration one-liner?

Re: Bug: send-pack does not respect http.signingkey

[Junio C Hamano]

Dave Borowitz <dborowitz@google.com> writes:

> Should I formally send a patch with your configuration one-liner?

Yeah, the log message, that explains the motivation of the change
and the decision to read which part of the configuration, is much
more important than the actual patch, so please do so ;-)