diff mbox series

[v6,3/9] ssh signing: retrieve a default key from ssh-agent

Message ID 071e6173d8e418349d94fea97624e8cee9f1dde5.1627501009.git.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series ssh signing: Add commit & tag signing/verification via SSH keys using ssh-keygen | expand

Commit Message

Fabian Stelzer July 28, 2021, 7:36 p.m. UTC
From: Fabian Stelzer <fs@gigacodes.de>

if user.signingkey is not set and a ssh signature is requested we call
ssh-add -L and use the first key we get

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 gpg-interface.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

Comments

Junio C Hamano July 28, 2021, 9:29 p.m. UTC | #1
"Fabian Stelzer via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Fabian Stelzer <fs@gigacodes.de>
>
> if user.signingkey is not set and a ssh signature is requested we call
> ssh-add -L and use the first key we get
>
> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
> ---
>  gpg-interface.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)

I would have expected that this also would become a method call into
*use_format object (instead of dispatching on use_format->name), but
let's not go overboard.  I think this is good enough for now.


> diff --git a/gpg-interface.c b/gpg-interface.c
> index c131977b347..3afacb48900 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -470,11 +470,35 @@ int git_gpg_config(const char *var, const char *value, void *cb)
>  	return 0;
>  }
>  
> +/* Returns the first public key from an ssh-agent to use for signing */
> +static char *get_default_ssh_signing_key(void)
> +{
> +	struct child_process ssh_add = CHILD_PROCESS_INIT;
> +	int ret = -1;
> +	struct strbuf key_stdout = STRBUF_INIT;
> +	struct strbuf **keys;
> +
> +	strvec_pushl(&ssh_add.args, "ssh-add", "-L", NULL);
> +	ret = pipe_command(&ssh_add, NULL, 0, &key_stdout, 0, NULL, 0);
> +	if (!ret) {
> +		keys = strbuf_split_max(&key_stdout, '\n', 2);
> +		if (keys[0])
> +			return strbuf_detach(keys[0], NULL);
> +	}
> +
> +	strbuf_release(&key_stdout);
> +	return "";
> +}
> +
>  const char *get_signing_key(void)
>  {
>  	if (configured_signing_key)
>  		return configured_signing_key;
> -	return git_committer_info(IDENT_STRICT|IDENT_NO_DATE);
> +	if (!strcmp(use_format->name, "ssh")) {
> +		return get_default_ssh_signing_key();
> +	} else {
> +		return git_committer_info(IDENT_STRICT | IDENT_NO_DATE);
> +	}
>  }
>  
>  int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
Jonathan Tan July 28, 2021, 10:48 p.m. UTC | #2
> if user.signingkey is not set and a ssh signature is requested we call
> ssh-add -L and use the first key we get

[snip]

> +/* Returns the first public key from an ssh-agent to use for signing */
> +static char *get_default_ssh_signing_key(void)
> +{
> +	struct child_process ssh_add = CHILD_PROCESS_INIT;
> +	int ret = -1;
> +	struct strbuf key_stdout = STRBUF_INIT;
> +	struct strbuf **keys;
> +
> +	strvec_pushl(&ssh_add.args, "ssh-add", "-L", NULL);
> +	ret = pipe_command(&ssh_add, NULL, 0, &key_stdout, 0, NULL, 0);
> +	if (!ret) {
> +		keys = strbuf_split_max(&key_stdout, '\n', 2);
> +		if (keys[0])
> +			return strbuf_detach(keys[0], NULL);
> +	}
> +
> +	strbuf_release(&key_stdout);
> +	return "";
> +}

Could the commit message have a better explanation of why we need this?
(Also, I would think that the command being run needs to be configurable
instead of being just the first "ssh-add" in $PATH, and the parsing of
the output should be more rigorous. But this is moot if we don't need
this feature in the first place.)
Fabian Stelzer July 29, 2021, 8:59 a.m. UTC | #3
On 29.07.21 00:48, Jonathan Tan wrote:
>> if user.signingkey is not set and a ssh signature is requested we call
>> ssh-add -L and use the first key we get
> 
> [snip]
> 
> Could the commit message have a better explanation of why we need this?
> (Also, I would think that the command being run needs to be configurable
> instead of being just the first "ssh-add" in $PATH, and the parsing of
> the output should be more rigorous. But this is moot if we don't need
> this feature in the first place.)
> 

How about:
If user.signingkey ist not set and a ssh signature is requested we call 
ssh-add -L und use the first key we get. This enables us to activate 
commit signing globally for all users on a shared server when ssh-agent 
forwarding is already in use without the need to touch an individual 
users gitconfig.

Maybe a general gpg.ssh.signingKeyDefaultCommand that we call and use 
the first returned line as key would be useful and achieve the same goal 
without having this default for everyone.
On the other hand i like having less configuration / good defaults for 
individual users. But I'm coming from a corporate environment, not an 
open source project.
Josh Steadmon July 29, 2021, 7:09 p.m. UTC | #4
On 2021.07.29 10:59, Fabian Stelzer wrote:
> On 29.07.21 00:48, Jonathan Tan wrote:
> > > if user.signingkey is not set and a ssh signature is requested we call
> > > ssh-add -L and use the first key we get
> > 
> > [snip]
> > 
> > Could the commit message have a better explanation of why we need this?
> > (Also, I would think that the command being run needs to be configurable
> > instead of being just the first "ssh-add" in $PATH, and the parsing of
> > the output should be more rigorous. But this is moot if we don't need
> > this feature in the first place.)
> > 
> 
> How about:
> If user.signingkey ist not set and a ssh signature is requested we call
> ssh-add -L und use the first key we get. This enables us to activate commit
> signing globally for all users on a shared server when ssh-agent forwarding
> is already in use without the need to touch an individual users gitconfig.
> 
> Maybe a general gpg.ssh.signingKeyDefaultCommand that we call and use the
> first returned line as key would be useful and achieve the same goal without
> having this default for everyone.
> On the other hand i like having less configuration / good defaults for
> individual users. But I'm coming from a corporate environment, not an open
> source project.

Doesn't this run the risk of using the wrong key (and potentially
exposing someone's identity)? On my work machine, my corporate SSH key
is not actually the first key in my SSH agent.

Rather than making this behavior the default, could it instead be
enabled only if the signing key is set to "use-ssh-agent" or something
similar?
Junio C Hamano July 29, 2021, 7:56 p.m. UTC | #5
Josh Steadmon <steadmon@google.com> writes:

> Rather than making this behavior the default, could it instead be
> enabled only if the signing key is set to "use-ssh-agent" or something
> similar?

Interesting.  But is it too much trouble to find out the string that
is used to identify the ssh key you want to use to sign, which would
make it worth supporting "use-ssh-agent" feature?  Unless you want
to use multiple keys in a single project, and choose one of them
depending on whatever condition, and find it convenient to specify
the key-of-the-day by loading it to your ssh-agent, I do not quite
see why you'd want to explicitly configure it to "use-ssh-agent" and
not the actual key (either the textual key itself or some key-id to
choose one of your keys).  Care to clarify your expected use case a
bit more?

Thanks.
Fabian Stelzer July 29, 2021, 9:21 p.m. UTC | #6
On 29.07.21 21:09, Josh Steadmon wrote:
> On 2021.07.29 10:59, Fabian Stelzer wrote:
>> On 29.07.21 00:48, Jonathan Tan wrote:
>>>> if user.signingkey is not set and a ssh signature is requested we call
>>>> ssh-add -L and use the first key we get
>>>
>>> [snip]
>>>
>>> Could the commit message have a better explanation of why we need this?
>>> (Also, I would think that the command being run needs to be configurable
>>> instead of being just the first "ssh-add" in $PATH, and the parsing of
>>> the output should be more rigorous. But this is moot if we don't need
>>> this feature in the first place.)
>>>
>>
>> How about:
>> If user.signingkey ist not set and a ssh signature is requested we call
>> ssh-add -L und use the first key we get. This enables us to activate commit
>> signing globally for all users on a shared server when ssh-agent forwarding
>> is already in use without the need to touch an individual users gitconfig.
>>
>> Maybe a general gpg.ssh.signingKeyDefaultCommand that we call and use the
>> first returned line as key would be useful and achieve the same goal without
>> having this default for everyone.
>> On the other hand i like having less configuration / good defaults for
>> individual users. But I'm coming from a corporate environment, not an open
>> source project.
> 
> Doesn't this run the risk of using the wrong key (and potentially
> exposing someone's identity)? On my work machine, my corporate SSH key
> is not actually the first key in my SSH agent.
> 
> Rather than making this behavior the default, could it instead be
> enabled only if the signing key is set to "use-ssh-agent" or something
> similar?
> 

If we introduce a signingKeyDefaultComand we don't need the 
"use-ssh-agent" flag.

If user.signingkey is set it is used no matter what. A private key needs 
to be available either in the specified file or via ssh agent.

If it is not set then an automatic way to get a default key would be great.
So if we set signingKeyDefaultCommand to "ssh-add" (or a script 
returning a key) then the first available key could be used.
If this variable is unset and no user.signingkey is specified we fail 
and tell the user to set a signingkey.

If this variable is set to "ssh-add" by default or unset and needs to be
set explicitly set to have an automatic default key can be decided.
diff mbox series

Patch

diff --git a/gpg-interface.c b/gpg-interface.c
index c131977b347..3afacb48900 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -470,11 +470,35 @@  int git_gpg_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
+/* Returns the first public key from an ssh-agent to use for signing */
+static char *get_default_ssh_signing_key(void)
+{
+	struct child_process ssh_add = CHILD_PROCESS_INIT;
+	int ret = -1;
+	struct strbuf key_stdout = STRBUF_INIT;
+	struct strbuf **keys;
+
+	strvec_pushl(&ssh_add.args, "ssh-add", "-L", NULL);
+	ret = pipe_command(&ssh_add, NULL, 0, &key_stdout, 0, NULL, 0);
+	if (!ret) {
+		keys = strbuf_split_max(&key_stdout, '\n', 2);
+		if (keys[0])
+			return strbuf_detach(keys[0], NULL);
+	}
+
+	strbuf_release(&key_stdout);
+	return "";
+}
+
 const char *get_signing_key(void)
 {
 	if (configured_signing_key)
 		return configured_signing_key;
-	return git_committer_info(IDENT_STRICT|IDENT_NO_DATE);
+	if (!strcmp(use_format->name, "ssh")) {
+		return get_default_ssh_signing_key();
+	} else {
+		return git_committer_info(IDENT_STRICT | IDENT_NO_DATE);
+	}
 }
 
 int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)