[PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile

[PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile

[redoste]

Detaching the filename string from the tempfile structure used to cause
delete_tempfile() to fail and the temporary file was not cleaned up.

Signed-off-by: redoste <redoste@redoste.xyz>
---
 gpg-interface.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 0896458de5..bdcc8c2a2e 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -1048,7 +1048,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
 				    key_file->filename.buf);
 			goto out;
 		}
-		ssh_signing_key_file = strbuf_detach(&key_file->filename, NULL);
+		ssh_signing_key_file = xstrdup(key_file->filename.buf);
 	} else {
 		/* We assume a file */
 		ssh_signing_key_file = interpolate_path(signing_key, 1);
-- 
2.49.0

Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile

[Jeff King]

On Sat, Jul 05, 2025 at 01:08:28AM +0200, redoste wrote:

> Detaching the filename string from the tempfile structure used to cause
> delete_tempfile() to fail and the temporary file was not cleaned up.

Good catch. I can reproduce this easily with:

  git -c gpg.format=ssh \
      -c user.signingkey=key::does-not-exist \
      commit --allow-empty -S -m foo

which creates /tmp/.git_signing_key_tmp* and never cleans it up.

I wonder if it is worth adding a test, or if it would be too weirdly
focused on this obscure case to be very useful against future
regressions.

> Signed-off-by: redoste <redoste@redoste.xyz>

We look for a real name in the sign-off trailer, since it indicates an
acceptance of the DCO and the ability to legally contribute the patch to
the project. See the section of Documentation/SubmittingPatches starting
with the '[[dco]]'. Or here:

  https://git-scm.com/docs/SubmittingPatches#sign-off

Looking at your web page, it looks like you may prefer not to associate
your online identity with a legal name. I can't remember if we've dealt
with this before. I'm adding brian to the cc, who has given a lot of
thought to naming and privacy issues.

(In this particular case, I suspect the patch is trivial enough not to
even be copyright-able, so we may be able to just let it go in this
instance.)


The patch itself looks good:

> diff --git a/gpg-interface.c b/gpg-interface.c
> index 0896458de5..bdcc8c2a2e 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -1048,7 +1048,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
>  				    key_file->filename.buf);
>  			goto out;
>  		}
> -		ssh_signing_key_file = strbuf_detach(&key_file->filename, NULL);
> +		ssh_signing_key_file = xstrdup(key_file->filename.buf);
>  	} else {
>  		/* We assume a file */
>  		ssh_signing_key_file = interpolate_path(signing_key, 1);

I think we could avoid even xstrdup() here and just use the key_file
pointer directly (it lasts until the end of the function, which is all
we need). But the call to interpolate_path() on the other side of
conditional does always allocate, so we'd need extra book-keeping to
decide whether to free or not. The extra copy is worth it to keep the
code simpler.


One other thing I noticed while looking at this function: the tempfile
deletion code should be OK with a NULL entry (that is considered an
"inactive" tempfile). So:

diff --git a/gpg-interface.c b/gpg-interface.c
index 0896458de5..fc48f93e11 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -1102,10 +1102,8 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
 	remove_cr_after(signature, bottom);
 
 out:
-	if (key_file)
-		delete_tempfile(&key_file);
-	if (buffer_file)
-		delete_tempfile(&buffer_file);
+	delete_tempfile(&key_file);
+	delete_tempfile(&buffer_file);
 	if (ssh_signature_filename.len)
 		unlink_or_warn(ssh_signature_filename.buf);
 	strbuf_release(&signer_stderr);

would simplify the code a little bit. But it may not be worth worrying
too much about (and is certainly orthogonal to your patch).

-Peff

Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 3448 bytes --]

On 2025-07-05 at 19:21:13, Jeff King wrote:
> On Sat, Jul 05, 2025 at 01:08:28AM +0200, redoste wrote:
> 
> > Detaching the filename string from the tempfile structure used to cause
> > delete_tempfile() to fail and the temporary file was not cleaned up.
> 
> Good catch. I can reproduce this easily with:
> 
>   git -c gpg.format=ssh \
>       -c user.signingkey=key::does-not-exist \
>       commit --allow-empty -S -m foo
> 
> which creates /tmp/.git_signing_key_tmp* and never cleans it up.
> 
> I wonder if it is worth adding a test, or if it would be too weirdly
> focused on this obscure case to be very useful against future
> regressions.

I don't have a strong view either way, but I do wonder if it's a good
idea to have the testsuite poking around in `/tmp`, although maybe if we
honour `TMPDIR` then it would be possible to do in a tidy way.

> > Signed-off-by: redoste <redoste@redoste.xyz>
> 
> We look for a real name in the sign-off trailer, since it indicates an
> acceptance of the DCO and the ability to legally contribute the patch to
> the project. See the section of Documentation/SubmittingPatches starting
> with the '[[dco]]'. Or here:
> 
>   https://git-scm.com/docs/SubmittingPatches#sign-off
> 
> Looking at your web page, it looks like you may prefer not to associate
> your online identity with a legal name. I can't remember if we've dealt
> with this before. I'm adding brian to the cc, who has given a lot of
> thought to naming and privacy issues.

I don't know if we have a strict policy.  I do know that there are
developers who always go by a pseudonym, such as chromatic[0], the
contributor to Perl, and obviously we'd want to allow them to
contribute. We also let people use shortened forms of their names or
initials (for instance, Jeff King).

I also have some friends who are trans and have transitioned or are in
the process of transitioning but have simply not gotten around to
getting legal paperwork done[1].  Obviously they have a distinct and
identifiable name that they go by and we'd allow them to use a preferred
name.

There might also be good reasons that a contributor might not want to
use a legal name: harassment, threats, employer hostility, fame[2], or a
hostile government, to name a few.  I think those are legitimate reasons
to contribute pseudonymously.

So I would say that if someone has a distinct and identifiable identity
that is pseudonymous and that is generally used and visible in the
public sphere online, that's probably good enough.  While I'm not a
lawyer, it's my understanding that in many locales, making a legal
promise of sorts (such as a sign-off) is equally binding whether made
with one's real name or a pseudonym, so I don't see a problem with the
legal aspect of it.

[0] https://en.wikipedia.org/wiki/Chromatic_(programmer)
[1] In some locales this involves hiring an attorney, getting paperwork
from a doctor, and getting a court order, so it can be expensive and
kind of a hassle to do.  It may also not be legally possible to do that
in some places.
[2] Notably the frontman of the band Weezer, Rivers Cuomo, is involved
in coding under his real name (https://github.com/riverscuomo), but
perhaps a CEO, musician, actor, or other famous person might not want
their open-source contributions to be associated with their real name.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile

[redoste]

On Sat Jul 5, 2025 at 22:07 CEST, brian m. carlson wrote:
> On 2025-07-05 at 19:21:13, Jeff King wrote:
>> I wonder if it is worth adding a test, or if it would be too weirdly
>> focused on this obscure case to be very useful against future
>> regressions.
>
> I don't have a strong view either way, but I do wonder if it's a good
> idea to have the testsuite poking around in `/tmp`, although maybe if we
> honour `TMPDIR` then it would be possible to do in a tidy way.
I looked into adding a test, but I didn't find any other tests checking
for temporary files and I agree that messing in /tmp doesn't feel really
appropriate for the testsuite.

Maybe something like this?

diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
index 065f780636..359dc8eba8 100755
--- a/t/t7528-signed-commit-ssh.sh
+++ b/t/t7528-signed-commit-ssh.sh
@@ -85,6 +85,7 @@ test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agen
	eval $(ssh-agent) &&
	test_when_finished "kill ${SSH_AGENT_PID}" &&
	ssh-add "${GPGSSH_KEY_PRIMARY}" &&
+	export TMPDIR=$(pwd) &&
	echo 1 >file && git add file &&
	git commit -a -m rsa-inline -S"$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
	echo 2 >file &&
@@ -95,7 +96,8 @@ test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agen
	git commit -a -m ecdsa-inline -S"key::$(cat "${GPGSSH_KEY_ECDSA}.pub")" &&
	echo 4 >file &&
	test_config user.signingkey "key::$(cat "${GPGSSH_KEY_ECDSA}.pub")" &&
-	git commit -a -m ecdsa-config -S
+	git commit -a -m ecdsa-config -S &&
+	! ls .git_signing_key_tmp*
 '
 
 test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'create signed commits with keys having defined lifetimes' '

I can add it in a v2 if you think it's a good way to test it.

> I also have some friends who are trans and have transitioned or are in
> the process of transitioning but have simply not gotten around to
> getting legal paperwork done[1].
This is the exact reason why I'm not very comfortable with using my
legal or real name, (well, it's mostly because I still can't find a name
I like).
And since it's a simple patch that's probably not even copyrightable, I
figured out that using a pseudonym was fine.

Since I knew that the Linux kernel changed their documentation to remove
the use of "real name", I thought it was more common and didn't relly
think about it a lot.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d4563201f33a022fc0353033d9dfeb1606a88330

I'm sorry, I should have read the git documentation more thoroughly.

If it's really an issue I don't mind signing off with a different and
more distinctive name.

-- 
redoste

Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile

[redoste]

On Sun Jul 6, 2025 at 00:50 CEST, redoste wrote:
> Maybe something like this?
Sorry, it's my first time contributing via a mailing list and I stupidly
copy-pasted the output of `git diff` without checking the whitespaces.
Here is the proper version of the diff that can be applied:

diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
index 065f780636..359dc8eba8 100755
--- a/t/t7528-signed-commit-ssh.sh
+++ b/t/t7528-signed-commit-ssh.sh
@@ -85,6 +85,7 @@ test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agen
 	eval $(ssh-agent) &&
 	test_when_finished "kill ${SSH_AGENT_PID}" &&
 	ssh-add "${GPGSSH_KEY_PRIMARY}" &&
+	export TMPDIR=$(pwd) &&
 	echo 1 >file && git add file &&
 	git commit -a -m rsa-inline -S"$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
 	echo 2 >file &&
@@ -95,7 +96,8 @@ test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agen
 	git commit -a -m ecdsa-inline -S"key::$(cat "${GPGSSH_KEY_ECDSA}.pub")" &&
 	echo 4 >file &&
 	test_config user.signingkey "key::$(cat "${GPGSSH_KEY_ECDSA}.pub")" &&
-	git commit -a -m ecdsa-config -S
+	git commit -a -m ecdsa-config -S &&
+	! ls .git_signing_key_tmp*
 '
 
 test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'create signed commits with keys having defined lifetimes' '

-- 
redoste

Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 4201 bytes --]

On 2025-07-05 at 22:50:43, redoste wrote:
> On Sat Jul 5, 2025 at 22:07 CEST, brian m. carlson wrote:
> > On 2025-07-05 at 19:21:13, Jeff King wrote:
> > I don't have a strong view either way, but I do wonder if it's a good
> > idea to have the testsuite poking around in `/tmp`, although maybe if we
> > honour `TMPDIR` then it would be possible to do in a tidy way.
> I looked into adding a test, but I didn't find any other tests checking
> for temporary files and I agree that messing in /tmp doesn't feel really
> appropriate for the testsuite.
> 
> Maybe something like this?
> 
> diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
> index 065f780636..359dc8eba8 100755
> --- a/t/t7528-signed-commit-ssh.sh
> +++ b/t/t7528-signed-commit-ssh.sh
> @@ -85,6 +85,7 @@ test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agen
> 	eval $(ssh-agent) &&
> 	test_when_finished "kill ${SSH_AGENT_PID}" &&
> 	ssh-add "${GPGSSH_KEY_PRIMARY}" &&
> +	export TMPDIR=$(pwd) &&
> 	echo 1 >file && git add file &&
> 	git commit -a -m rsa-inline -S"$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
> 	echo 2 >file &&
> @@ -95,7 +96,8 @@ test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agen
> 	git commit -a -m ecdsa-inline -S"key::$(cat "${GPGSSH_KEY_ECDSA}.pub")" &&
> 	echo 4 >file &&
> 	test_config user.signingkey "key::$(cat "${GPGSSH_KEY_ECDSA}.pub")" &&
> -	git commit -a -m ecdsa-config -S
> +	git commit -a -m ecdsa-config -S &&
> +	! ls .git_signing_key_tmp*
>  '

What I would recommend is adding another test (that is, another
`test_expect_success` assertion) that when signing we don't leave any
temporary files behind.  You can then squash that into your patch and
send a v2.

As for `ls`, I wouldn't use that in scripting.  I'd use something like
this:

----
mkdir tmpdir &&
TMPDIR="$(pwd)/tmpdir" &&
export TMPDIR &&
# Other code
find tmpdir -type f >files &&
test_line_count = 0 files
----

I think `find` is typically better in scripting and also in this case
with a separate directory as the temporary directory we'll find any left
over temporary files, not just the pattern we've fixed here.

Our coding guidelines point out that some shells don't like `export`
with assignments, so we do those as two lines.  Normally I would write
what you have in my day-to-day life, but we run on a variety of systems
that most people will never use.

>  test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'create signed commits with keys having defined lifetimes' '
> 
> I can add it in a v2 if you think it's a good way to test it.

I think the basic premise is sound, but a separate assertion would make
it easier to readers why we're setting the temporary directory.

> > I also have some friends who are trans and have transitioned or are in
> > the process of transitioning but have simply not gotten around to
> > getting legal paperwork done[1].
> This is the exact reason why I'm not very comfortable with using my
> legal or real name, (well, it's mostly because I still can't find a name
> I like).
> And since it's a simple patch that's probably not even copyrightable, I
> figured out that using a pseudonym was fine.
> 
> Since I knew that the Linux kernel changed their documentation to remove
> the use of "real name", I thought it was more common and didn't relly
> think about it a lot.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d4563201f33a022fc0353033d9dfeb1606a88330
> 
> I'm sorry, I should have read the git documentation more thoroughly.
> 
> If it's really an issue I don't mind signing off with a different and
> more distinctive name.

No, I think this is fine.  You weren't obligated to explain that and our
policies should gracefully handle this situation regardless, but given
what I said before and this context, I think the name you have is fine.
Git unfortunately has poor support for replacing names and I would't
want you to have to put your deadname into our history for all of
eternity.

I'll send a patch to fix the policy.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile

[Jeff King]

On Sun, Jul 06, 2025 at 12:28:43AM +0000, brian m. carlson wrote:

> On 2025-07-05 at 22:50:43, redoste wrote:
> [...]
> > Since I knew that the Linux kernel changed their documentation to remove
> > the use of "real name", I thought it was more common and didn't relly
> > think about it a lot.
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d4563201f33a022fc0353033d9dfeb1606a88330
> > 
> > I'm sorry, I should have read the git documentation more thoroughly.
> > 
> > If it's really an issue I don't mind signing off with a different and
> > more distinctive name.
> 
> No, I think this is fine.  You weren't obligated to explain that and our
> policies should gracefully handle this situation regardless, but given
> what I said before and this context, I think the name you have is fine.
> Git unfortunately has poor support for replacing names and I would't
> want you to have to put your deadname into our history for all of
> eternity.
> 
> I'll send a patch to fix the policy.

Ah, I hadn't heard about that change in the kernel. I agree that it
would make sense to loosen our text to match.

Thanks (in advance) for sending that doc update, and for your earlier
comments in the thread. I agreed with everything you wrote there. FWIW,
my personal inclinations lean much more towards allowing privacy and
anonymity, but as a reviewer for the project I wanted to make sure we
were not violating our own policy in a way that would eventually bite us
in a legal sense. The notion of "identifiable but pseudonymous" you
brought up seems like a good middle ground (at least to my layman's
ears).

And thanks redoste for your patch. ;) Sorry if bringing up the signoff
issue caused any hassle (but hopefully it will lead to an improvement to
our docs).

-Peff

Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile

[redoste]

On Sun Jul 6, 2025 at 02:28 CEST, brian m. carlson wrote:
> I think `find` is typically better in scripting and also in this case
> with a separate directory as the temporary directory we'll find any left
> over temporary files, not just the pattern we've fixed here.
Thanks for the feedback!

I used `ls` since it was the easiest way to have something that fails
quickly if a file doesn't exists (and `test -e` doesn't support having
multiple files), but I agree, using `find` this way is definitely
cleaner.

> I'll send a patch to fix the policy.
Thanks!

-- 
redoste

Re: [PATCH] ssh signing: don't detach the filename strbuf from key_file tempfile

[redoste]

On Sun Jul 6, 2025 at 05:13 CEST, Jeff King wrote:
> And thanks redoste for your patch. ;) Sorry if bringing up the signoff
> issue caused any hassle (but hopefully it will lead to an improvement to
> our docs).

No worries! I completely understand your position as a reviewer and I
don't mind if it helps to improve the situation and will help future
contributors.

-- 
redoste

[PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile

[redoste]

Detaching the filename string from the tempfile structure used to cause
delete_tempfile() to fail and the temporary file was not cleaned up.

While it's possible to get rid of the allocation and copy from
xstrdup(), it keeps the code symetric with the other branch since
interpolate_path() also allocates and ssh_signing_key_file is freed
in both cases.

Helped-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: redoste <redoste@redoste.xyz>
---
v1->v2:
 * add a test case that checks for temporary files after signing commits
 * add small explaination about the use of xstrdup() in the commit
   body

 gpg-interface.c              |  2 +-
 t/t7528-signed-commit-ssh.sh | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 0896458de5..bdcc8c2a2e 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -1048,7 +1048,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
 				    key_file->filename.buf);
 			goto out;
 		}
-		ssh_signing_key_file = strbuf_detach(&key_file->filename, NULL);
+		ssh_signing_key_file = xstrdup(key_file->filename.buf);
 	} else {
 		/* We assume a file */
 		ssh_signing_key_file = interpolate_path(signing_key, 1);
diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
index 065f780636..1a8d96f355 100755
--- a/t/t7528-signed-commit-ssh.sh
+++ b/t/t7528-signed-commit-ssh.sh
@@ -390,6 +390,22 @@ test_expect_success GPGSSH 'check config gpg.format values' '
 	test_must_fail git commit -S --amend -m "fail"
 '
 
+test_expect_success GPGSSH 'check temporary files clean up when signing commits' '
+	test_config gpg.format ssh &&
+	eval $(ssh-agent) &&
+	test_when_finished "kill ${SSH_AGENT_PID}" &&
+	mkdir tmpdir &&
+	TMPDIR="$(pwd)/tmpdir" &&
+	export TMPDIR &&
+	ssh-add "${GPGSSH_KEY_PRIMARY}" &&
+	echo 1 >file && git add file &&
+	git commit -a -m inline -S"$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
+	echo 2 >file &&
+	git commit -a -m file -S"${GPGSSH_KEY_PRIMARY}" &&
+	find tmpdir -type f >tmpfiles &&
+	test_line_count = 0 tmpfiles
+'
+
 test_expect_failure GPGSSH 'detect fudged commit with double signature (TODO)' '
 	sed -e "/gpgsig/,/END PGP/d" forged1 >double-base &&
 	sed -n -e "/gpgsig/,/END PGP/p" forged1 | \
-- 
2.49.0

Re: [PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

On 2025-07-06 at 17:34:49, redoste wrote:
> Detaching the filename string from the tempfile structure used to cause
> delete_tempfile() to fail and the temporary file was not cleaned up.
> 
> While it's possible to get rid of the allocation and copy from
> xstrdup(), it keeps the code symetric with the other branch since
> interpolate_path() also allocates and ssh_signing_key_file is freed
> in both cases.
> 
> Helped-by: brian m. carlson <sandals@crustytoothpaste.net>
> Signed-off-by: redoste <redoste@redoste.xyz>

Yup, this looks good to me.  Thanks so much for the patch; I always
appreciate keeping the temporary directory tidy.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile

[Patrick Steinhardt]

On Sun, Jul 06, 2025 at 07:34:49PM +0200, redoste wrote:
> diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
> index 065f780636..1a8d96f355 100755
> --- a/t/t7528-signed-commit-ssh.sh
> +++ b/t/t7528-signed-commit-ssh.sh
> @@ -390,6 +390,22 @@ test_expect_success GPGSSH 'check config gpg.format values' '
>  	test_must_fail git commit -S --amend -m "fail"
>  '
>  
> +test_expect_success GPGSSH 'check temporary files clean up when signing commits' '
> +	test_config gpg.format ssh &&
> +	eval $(ssh-agent) &&
> +	test_when_finished "kill ${SSH_AGENT_PID}" &&
> +	mkdir tmpdir &&
> +	TMPDIR="$(pwd)/tmpdir" &&
> +	export TMPDIR &&

I think this exported environment variable now leaks into subsequent
tests, doesn't it? We may want to do it in a subshell.

	mkdir tmpdir &&
	TMPDIR="$(pwd)/tmpdir" &&
	(
		export TMPDIR &&
		ssh-add "${GPGSSH_KEY_PRIMARY}" &&
		echo 1 >file && git add file &&
		git commit -a -m inline -S"$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
		echo 2 >file &&
		git commit -a -m file -S"${GPGSSH_KEY_PRIMARY}"
	) && 
	find tmpdir -type f >tmpfiles &&
	test_line_count = 0 tmpfiles

Patrick

> +	ssh-add "${GPGSSH_KEY_PRIMARY}" &&
> +	echo 1 >file && git add file &&
> +	git commit -a -m inline -S"$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
> +	echo 2 >file &&
> +	git commit -a -m file -S"${GPGSSH_KEY_PRIMARY}" &&
> +	find tmpdir -type f >tmpfiles &&
> +	test_line_count = 0 tmpfiles
> +'
> +
>  test_expect_failure GPGSSH 'detect fudged commit with double signature (TODO)' '
>  	sed -e "/gpgsig/,/END PGP/d" forged1 >double-base &&
>  	sed -n -e "/gpgsig/,/END PGP/p" forged1 | \
> -- 
> 2.49.0
> 
> 

Re: [PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile

[Phillip Wood]

On 07/07/2025 10:02, Patrick Steinhardt wrote:
> On Sun, Jul 06, 2025 at 07:34:49PM +0200, redoste wrote:
>> +test_expect_success GPGSSH 'check temporary files clean up when signing commits' '
>> +	test_config gpg.format ssh &&
>> +	eval $(ssh-agent) &&
>> +	test_when_finished "kill ${SSH_AGENT_PID}" &&
>> +	mkdir tmpdir &&
>> +	TMPDIR="$(pwd)/tmpdir" &&
>> +	export TMPDIR &&
> 
> I think this exported environment variable now leaks into subsequent
> tests, doesn't it? We may want to do it in a subshell.

That's a very good point.

> 
> 	mkdir tmpdir &&
> 	TMPDIR="$(pwd)/tmpdir" &&
> 	(
> 		export TMPDIR &&
> 		ssh-add "${GPGSSH_KEY_PRIMARY}" &&
> 		echo 1 >file && git add file &&
> 		git commit -a -m inline -S"$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
> 		echo 2 >file &&
> 		git commit -a -m file -S"${GPGSSH_KEY_PRIMARY}"
> 	) &&
> 	find tmpdir -type f >tmpfiles &&
> 	test_line_count = 0 tmpfiles

The idiomatic way to test for an empty file is "test_file_is_empty 
<file>" which avoids spawning wc.

As this test is an abridged version of the other test that uses 
ssh-agent I think it would be more efficient to tweak that test to check 
there are no temporary files left over. Our test suite is slow and 
tweaking an existing test to improve our coverage instead of adding a 
new test means we don't make it any slower than necessary.

Thanks

Phillip

> 
> Patrick
> 
>> +	ssh-add "${GPGSSH_KEY_PRIMARY}" &&
>> +	echo 1 >file && git add file &&
>> +	git commit -a -m inline -S"$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
>> +	echo 2 >file &&
>> +	git commit -a -m file -S"${GPGSSH_KEY_PRIMARY}" &&
>> +	find tmpdir -type f >tmpfiles &&
>> +	test_line_count = 0 tmpfiles
>> +'
>> +
>>   test_expect_failure GPGSSH 'detect fudged commit with double signature (TODO)' '
>>   	sed -e "/gpgsig/,/END PGP/d" forged1 >double-base &&
>>   	sed -n -e "/gpgsig/,/END PGP/p" forged1 | \
>> -- 
>> 2.49.0
>>
>>
> 

Re: [PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile

[redoste]

On Mon Jul 7, 2025 at 11:35 CEST, Phillip Wood wrote:
> On 07/07/2025 10:02, Patrick Steinhardt wrote:
>> I think this exported environment variable now leaks into subsequent
>> tests, doesn't it? We may want to do it in a subshell.
>
> That's a very good point.
Sure thing, I took inspiration from the previous ssh-agent test and it
exports the ssh-agent variables without spawning a subshell, so I
figured it was fine, but it was only because test_when_finished will
fail in a subshell.
I guess it's probably fine to leak only the ssh-agent variables
accross tests and keep TMPDIR in a subshell.

> The idiomatic way to test for an empty file is "test_file_is_empty 
> <file>" which avoids spawning wc.
Thanks, I will update this.

> As this test is an abridged version of the other test that uses 
> ssh-agent I think it would be more efficient to tweak that test to check 
> there are no temporary files left over. Our test suite is slow and 
> tweaking an existing test to improve our coverage instead of adding a 
> new test means we don't make it any slower than necessary.
I moved this test out of the other ssh-agent one according to the advice
from brian on the previous version. I agree that spawning multiple
ssh-agent is definitly not the best for speed, but is it worth it to
make the test failure less clear?
I feel like it's not the best to keep checks for temporary files clean
up in a test that (initially) only claims to sign commits.

-- 
redoste

Re: [PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile

[Phillip Wood]

On 07/07/2025 15:25, redoste wrote:
> On Mon Jul 7, 2025 at 11:35 CEST, Phillip Wood wrote:
>> On 07/07/2025 10:02, Patrick Steinhardt wrote:
>>> I think this exported environment variable now leaks into subsequent
>>> tests, doesn't it? We may want to do it in a subshell.
>>
>> That's a very good point.
> Sure thing, I took inspiration from the previous ssh-agent test and it
> exports the ssh-agent variables without spawning a subshell, so I
> figured it was fine, but it was only because test_when_finished will
> fail in a subshell.
> I guess it's probably fine to leak only the ssh-agent variables
> accross tests and keep TMPDIR in a subshell.

Hopefully having the ssh-agent variables set after the agent has been 
killed doesn't affect the later tests

>> The idiomatic way to test for an empty file is "test_file_is_empty
>> <file>" which avoids spawning wc.
> Thanks, I will update this.
> 
>> As this test is an abridged version of the other test that uses
>> ssh-agent I think it would be more efficient to tweak that test to check
>> there are no temporary files left over. Our test suite is slow and
>> tweaking an existing test to improve our coverage instead of adding a
>> new test means we don't make it any slower than necessary.
> I moved this test out of the other ssh-agent one according to the advice
> from brian on the previous version. I agree that spawning multiple
> ssh-agent is definitly not the best for speed, but is it worth it to
> make the test failure less clear?
test_file_is_empty will print a diagnostic message if it fails so it 
should be clear what has caused the test failure

> I feel like it's not the best to keep checks for temporary files clean
> up in a test that (initially) only claims to sign commits.

You can always tweak the test title if you want. The way I see it is 
that the changes that are being tested are related to commit signing as 
the invariant that we want to assert is that temporary files are cleaned 
up after signing commits.

Thanks

Phillip

Re: [PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile

[redoste]

On Mon Jul 7, 2025 at 17:26 CEST, Phillip Wood wrote:
> test_file_is_empty will print a diagnostic message if it fails so it 
> should be clear what has caused the test failure
Uh, okay, this makes sense and I think it will be clear enough.
However there is only `test_file_not_empty`, `test_file_is_empty`
doesn't exist. I will implement it in an other commit.

> The way I see it is that the changes that are being tested are related
> to commit signing as the invariant that we want to assert is that
> temporary files are cleaned up after signing commits.
I see.

I will update this for a v3.
Thanks for the feedback!

-- 
redoste

Re: [PATCH v2] ssh signing: don't detach the filename strbuf from key_file tempfile

[Phillip Wood]

On 07/07/2025 17:22, redoste wrote:
> On Mon Jul 7, 2025 at 17:26 CEST, Phillip Wood wrote:
>> test_file_is_empty will print a diagnostic message if it fails so it
>> should be clear what has caused the test failure
> Uh, okay, this makes sense and I think it will be clear enough.
> However there is only `test_file_not_empty`, `test_file_is_empty`
> doesn't exist. I will implement it in an other commit.

Sorry I'd misremembered the name, it is "test_must_be_empty"

Phillip
>> The way I see it is that the changes that are being tested are related
>> to commit signing as the invariant that we want to assert is that
>> temporary files are cleaned up after signing commits.
> I see.
> 
> I will update this for a v3.
> Thanks for the feedback!
> 

[PATCH v3] ssh signing: don't detach the filename strbuf from key_file tempfile

[redoste]

Detaching the filename string from the tempfile structure used to cause
delete_tempfile() to fail and the temporary file was not cleaned up.

While it's possible to get rid of the allocation and copy from
xstrdup(), it keeps the code symetric with the other branch since
interpolate_path() also allocates and ssh_signing_key_file is freed
in both cases.

The exisiting test was updated to check if the temporary files are
properly deleted. To prevent TMPDIR from leaking into the other tests, a
new subshell is created, however this prevents test_config from working.
The cleanup of the config changed in the subshell is done by
test_unconfig in a call to test_when_finished outside of it.

Helped-by: brian m. carlson <sandals@crustytoothpaste.net>
Helped-by: Patrick Steinhardt <ps@pks.im>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: redoste <redoste@redoste.xyz>
---
v1->v2:
 * add a test case that checks for temporary files after signing commits
 * add small explaination about the use of xstrdup() in the commit
   body
v2->v3:
 * merge the test with the previous ssh-agent test
 * export TMPDIR in a subshell to prevent the environment variable from
   leaking in the next tests
 * use test_must_be_empty instead of test_line_count

 gpg-interface.c              |  2 +-
 t/t7528-signed-commit-ssh.sh | 32 ++++++++++++++++++++------------
 2 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 0896458de5..bdcc8c2a2e 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -1048,7 +1048,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
 				    key_file->filename.buf);
 			goto out;
 		}
-		ssh_signing_key_file = strbuf_detach(&key_file->filename, NULL);
+		ssh_signing_key_file = xstrdup(key_file->filename.buf);
 	} else {
 		/* We assume a file */
 		ssh_signing_key_file = interpolate_path(signing_key, 1);
diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
index 065f780636..0f887a3ebe 100755
--- a/t/t7528-signed-commit-ssh.sh
+++ b/t/t7528-signed-commit-ssh.sh
@@ -84,18 +84,26 @@ test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agen
 	test_config gpg.format ssh &&
 	eval $(ssh-agent) &&
 	test_when_finished "kill ${SSH_AGENT_PID}" &&
-	ssh-add "${GPGSSH_KEY_PRIMARY}" &&
-	echo 1 >file && git add file &&
-	git commit -a -m rsa-inline -S"$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
-	echo 2 >file &&
-	test_config user.signingkey "$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
-	git commit -a -m rsa-config -S &&
-	ssh-add "${GPGSSH_KEY_ECDSA}" &&
-	echo 3 >file &&
-	git commit -a -m ecdsa-inline -S"key::$(cat "${GPGSSH_KEY_ECDSA}.pub")" &&
-	echo 4 >file &&
-	test_config user.signingkey "key::$(cat "${GPGSSH_KEY_ECDSA}.pub")" &&
-	git commit -a -m ecdsa-config -S
+	test_when_finished "test_unconfig user.signingkey" &&
+	mkdir tmpdir &&
+	TMPDIR="$(pwd)/tmpdir" &&
+	(
+		export TMPDIR &&
+		ssh-add "${GPGSSH_KEY_PRIMARY}" &&
+		echo 1 >file && git add file &&
+		git commit -a -m rsa-inline -S"$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
+		echo 2 >file &&
+		git config user.signingkey "$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
+		git commit -a -m rsa-config -S &&
+		ssh-add "${GPGSSH_KEY_ECDSA}" &&
+		echo 3 >file &&
+		git commit -a -m ecdsa-inline -S"key::$(cat "${GPGSSH_KEY_ECDSA}.pub")" &&
+		echo 4 >file &&
+		git config user.signingkey "key::$(cat "${GPGSSH_KEY_ECDSA}.pub")" &&
+		git commit -a -m ecdsa-config -S
+	) &&
+	find tmpdir -type f >tmpfiles &&
+	test_must_be_empty tmpfiles
 '
 
 test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'create signed commits with keys having defined lifetimes' '
-- 
2.49.0

Re: [PATCH v3] ssh signing: don't detach the filename strbuf from key_file tempfile

[Junio C Hamano]

redoste <redoste@redoste.xyz> writes:

> v2->v3:
>  * merge the test with the previous ssh-agent test
>  * export TMPDIR in a subshell to prevent the environment variable from
>    leaking in the next tests
>  * use test_must_be_empty instead of test_line_count

These sound all good.  Will queue.

Thanks.

Re: [PATCH v3] ssh signing: don't detach the filename strbuf from key_file tempfile

[Patrick Steinhardt]

On Mon, Jul 07, 2025 at 01:57:39PM -0700, Junio C Hamano wrote:
> redoste <redoste@redoste.xyz> writes:
> 
> > v2->v3:
> >  * merge the test with the previous ssh-agent test
> >  * export TMPDIR in a subshell to prevent the environment variable from
> >    leaking in the next tests
> >  * use test_must_be_empty instead of test_line_count
> 
> These sound all good.  Will queue.

Yup, the patch looks good to me. Thanks!

Patrick

Re: [PATCH v3] ssh signing: don't detach the filename strbuf from key_file tempfile

[Phillip Wood]

On 08/07/2025 07:47, Patrick Steinhardt wrote:
> On Mon, Jul 07, 2025 at 01:57:39PM -0700, Junio C Hamano wrote:
>> redoste <redoste@redoste.xyz> writes:
>>
>>> v2->v3:
>>>   * merge the test with the previous ssh-agent test
>>>   * export TMPDIR in a subshell to prevent the environment variable from
>>>     leaking in the next tests
>>>   * use test_must_be_empty instead of test_line_count
>>
>> These sound all good.  Will queue.
> 
> Yup, the patch looks good to me. Thanks!

Yes it does to me too, thanks

Phillip