diff mbox series

[6/6] ssh signing: fmt-merge-msg/check_signature with tag date

Message ID 20211022150949.1754477-7-fs@gigacodes.de (mailing list archive)
State Superseded
Headers show
Series ssh signing: verify key lifetime | expand

Commit Message

Fabian Stelzer Oct. 22, 2021, 3:09 p.m. UTC 
Pass the tag date and tagger ident to check_signature when generating merge
messages to verify merged tags signatures.
Implements the same tests as for verify-commit.

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 fmt-merge-msg.c          | 13 +++++++++-
 t/t6200-fmt-merge-msg.sh | 54 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 1 deletion(-)

Comments

Ævar Arnfjörð Bjarmason Oct. 22, 2021, 6:12 p.m. UTC | #1 
On Fri, Oct 22 2021, Fabian Stelzer wrote:

>  			buf = payload.buf;
>  			len = payload.len;
> -			if (check_signature(payload.buf, payload.len, 0, NULL,
> +
> +			if (parse_signed_buffer_metadata(payload.buf, "tagger",
> +							 &payload_timestamp,
> +							 &payload_signer))
> +				strbuf_addstr(&sig,
> +					_("failed to parse timestamp and signer info from payload"));
> +
> +			if (check_signature(payload.buf, payload.len,
> +					    payload_timestamp, &payload_signer,
>  					    sig.buf, sig.len, &sigc) &&
>  			    !sigc.output)
>  				strbuf_addstr(&sig, "gpg verification failed.\n");

I haven't tested this, but your addition to &sig here lacks a \n,
compared to the &sig seen right above here in the diff context.

Isn't one or the other either missing a \n, or shouldn't have one?
*Looks ath the surrounding code*, yeah if I'm not wrong it's the \n in
the new code here that's missing.

The whole business of seemingly mixing error messages and a signature
payload in the same variable seems a bit odd, but maybe I'm misreading
it. In any case it seems to pre-date this series...
Fabian Stelzer Oct. 25, 2021, 8:39 a.m. UTC | #2 
On 22.10.21 20:12, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Oct 22 2021, Fabian Stelzer wrote:
> 
>>  			buf = payload.buf;
>>  			len = payload.len;
>> -			if (check_signature(payload.buf, payload.len, 0, NULL,
>> +
>> +			if (parse_signed_buffer_metadata(payload.buf, "tagger",
>> +							 &payload_timestamp,
>> +							 &payload_signer))
>> +				strbuf_addstr(&sig,
>> +					_("failed to parse timestamp and signer info from payload"));
>> +
>> +			if (check_signature(payload.buf, payload.len,
>> +					    payload_timestamp, &payload_signer,
>>  					    sig.buf, sig.len, &sigc) &&
>>  			    !sigc.output)
>>  				strbuf_addstr(&sig, "gpg verification failed.\n");
> 
> I haven't tested this, but your addition to &sig here lacks a \n,
> compared to the &sig seen right above here in the diff context.

You are correct. I have added the newline.

> 
> The whole business of seemingly mixing error messages and a signature
> payload in the same variable seems a bit odd, but maybe I'm misreading
> it. In any case it seems to pre-date this series...
> 

True, it is a bit odd. This function generates the message text when
merging a (signed) tag. It verifies the tag upon doing so and includes
the result information in the message.

I'm not sure what should happen in this error case. (especially since
this is one of those "this should never happen" errors). I don't know if
i can just warn() here or if that would be lost or even corrupt the
merge message :/
If the buffer parsing fails this could result in ssh signatures being
marked as valid even though they expired. So i think it it important to
include this info. But the error has no effect on gpg signatures. So
telling users about not being able to check key lifetime here might
paint a wrong picture.

Thanks,
Fabian
diff mbox series

Patch

diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index d2cedad6b7..4f49dd9341 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -524,6 +524,8 @@  static void fmt_merge_msg_sigs(struct strbuf *out)
 		unsigned long len = size;
 		struct signature_check sigc = { NULL };
 		struct strbuf payload = STRBUF_INIT, sig = STRBUF_INIT;
+		struct strbuf payload_signer = STRBUF_INIT;
+		timestamp_t payload_timestamp = 0;
 
 		if (!buf || type != OBJ_TAG)
 			goto next;
@@ -533,7 +535,15 @@  static void fmt_merge_msg_sigs(struct strbuf *out)
 		else {
 			buf = payload.buf;
 			len = payload.len;
-			if (check_signature(payload.buf, payload.len, 0, NULL,
+
+			if (parse_signed_buffer_metadata(payload.buf, "tagger",
+							 &payload_timestamp,
+							 &payload_signer))
+				strbuf_addstr(&sig,
+					_("failed to parse timestamp and signer info from payload"));
+
+			if (check_signature(payload.buf, payload.len,
+					    payload_timestamp, &payload_signer,
 					    sig.buf, sig.len, &sigc) &&
 			    !sigc.output)
 				strbuf_addstr(&sig, "gpg verification failed.\n");
@@ -564,6 +574,7 @@  static void fmt_merge_msg_sigs(struct strbuf *out)
 		}
 		strbuf_release(&payload);
 		strbuf_release(&sig);
+		strbuf_release(&payload_signer);
 	next:
 		free(origbuf);
 	}
diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 06c5fb5615..2dd2423643 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -91,6 +91,26 @@  test_expect_success GPGSSH 'created ssh signed commit and tag' '
 	git tag -s -u"${GPGSSH_KEY_UNTRUSTED}" -m signed-ssh-tag-msg-untrusted signed-untrusted-ssh-tag left
 '
 
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'create signed tags with keys having defined lifetimes' '
+	test_when_finished "test_unconfig commit.gpgsign" &&
+	test_config gpg.format ssh &&
+	git checkout -b signed-expiry-ssh &&
+	touch file &&
+	git add file &&
+
+	echo expired >file && test_tick && git commit -a -m expired -S"${GPGSSH_KEY_EXPIRED}" &&
+	git tag -s -u "${GPGSSH_KEY_EXPIRED}" -m expired-signed expired-signed &&
+
+	echo notyetvalid >file && test_tick && git commit -a -m notyetvalid -S"${GPGSSH_KEY_NOTYETVALID}" &&
+	git tag -s -u "${GPGSSH_KEY_NOTYETVALID}" -m notyetvalid-signed notyetvalid-signed &&
+
+	echo timeboxedvalid >file && test_tick && git commit -a -m timeboxedvalid -S"${GPGSSH_KEY_TIMEBOXEDVALID}" &&
+	git tag -s -u "${GPGSSH_KEY_TIMEBOXEDVALID}" -m timeboxedvalid-signed timeboxedvalid-signed &&
+
+	echo timeboxedinvalid >file && test_tick && git commit -a -m timeboxedinvalid -S"${GPGSSH_KEY_TIMEBOXEDINVALID}" &&
+	git tag -s -u "${GPGSSH_KEY_TIMEBOXEDINVALID}" -m timeboxedinvalid-signed timeboxedinvalid-signed
+'
+
 test_expect_success 'message for merging local branch' '
 	echo "Merge branch ${apos}left${apos}" >expected &&
 
@@ -137,6 +157,40 @@  test_expect_success GPGSSH 'message for merging local tag signed by unknown ssh
 	! grep "${GPGSSH_BAD_SIGNATURE}" actual &&
 	grep "${GPGSSH_KEY_NOT_TRUSTED}" actual
 '
+
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'message for merging local tag signed by expired ssh key' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	git checkout main &&
+	git fetch . expired-signed &&
+	git fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 &&
+	! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
+'
+
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'message for merging local tag signed by not yet valid ssh key' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	git checkout main &&
+	git fetch . notyetvalid-signed &&
+	git fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 &&
+	! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
+'
+
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'message for merging local tag signed by valid timeboxed ssh key' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	git checkout main &&
+	git fetch . timeboxedvalid-signed &&
+	git fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 &&
+	grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual &&
+	! grep "${GPGSSH_BAD_SIGNATURE}" actual
+'
+
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'message for merging local tag signed by invalid timeboxed ssh key' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	git checkout main &&
+	git fetch . timeboxedinvalid-signed &&
+	git fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 &&
+	! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
+'
+
 test_expect_success 'message for merging external branch' '
 	echo "Merge branch ${apos}left${apos} of $(pwd)" >expected &&