[PATCH 0/6] ssh signing: verify key lifetime

[PATCH 0/6] ssh signing: verify key lifetime

[Fabian Stelzer]

This series adds key lifetime validity checks by passing commit/tag
dates to the check operaion.

I'm not totally sure about the way i'm parsing payload info via
parse_signed_buffer_metadata(). payload was just an opaque buffer before.
Now gpg-interface actually looks at it (thats wy i used a seperate api
function for it and did not hide it in check_signature).
I chose this approach since it requires the least changes for the consumers
of this functionality.

A cleaner way would maybe be introducing a check_object_signature()
which takes a struct object instead of the payload directly to avoid the
parse function altogether. However only some call sites already have
this struct. Tags & fmt-merge-msg use different ways to produce the
payload and i'm not sure how involved the objects creation would be or what
side-effects this could have(performance?). Since the push-certs use case
will never produce a object struct we would still have to keep the
existing check_signature function anyway (or encapsulate those in some
pseudo-object :/).
The now used parse function also opens the usage up to sites already having
both infos (date & ident), although there is currently none.

Fabian Stelzer (6):
  ssh signing: extend check_signature to accept payload metadata
  ssh signing: add key lifetime test prereqs
  ssh signing: verify-commit/check_signature with commit date
  ssh signing: git log/check_signature with commit date
  ssh signing: verify-tag/check_signature with tag date
  ssh signing: fmt-merge-msg/check_signature with tag date

 Documentation/config/gpg.txt     |  5 ++
 builtin/receive-pack.c           |  5 +-
 commit.c                         | 12 ++++-
 fmt-merge-msg.c                  | 15 +++++-
 gpg-interface.c                  | 79 ++++++++++++++++++++++++++++----
 gpg-interface.h                  | 13 +++++-
 log-tree.c                       | 22 ++++++++-
 t/lib-gpg.sh                     | 19 +++++++-
 t/t4202-log.sh                   | 43 +++++++++++++++++
 t/t6200-fmt-merge-msg.sh         | 54 ++++++++++++++++++++++
 t/t7031-verify-tag-signed-ssh.sh | 42 +++++++++++++++++
 t/t7528-signed-commit-ssh.sh     | 42 +++++++++++++++++
 tag.c                            | 12 ++++-
 13 files changed, 341 insertions(+), 22 deletions(-)


base-commit: d3b4e01def5a9517c919f0b815c1b12296dc3dc2
-- 
2.31.1

[PATCH 1/6] ssh signing: extend check_signature to accept payload metadata

[Fabian Stelzer]

Adds two new parameters to the check_signature api and passes them to the
internal verification ssh/gpg methods.
A payload timestamp that will be used to verify signatures at the time of their
objects creation if the signing method supports it (only ssh for now).
And a signer strbuf containing ident information about the signer that
we will need for implementing "Trust on first use" in a future patch
series.

Adding the ident info right now instead of a later patch series makes
certain choices in this patch series clearer. Only passing the timestamp
could be done a bit simpler on some call sites but i am certain that we
will need the ident info for "Trust on first use" no matter how exactly
it will be implemented later.

To start with we pass 0, NULL on all invocations to keep the current behaviour
as is.

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 builtin/receive-pack.c |  5 +++--
 commit.c               |  2 +-
 fmt-merge-msg.c        |  4 ++--
 gpg-interface.c        | 36 +++++++++++++++++++++++++++---------
 gpg-interface.h        |  5 +++--
 log-tree.c             |  4 ++--
 tag.c                  |  2 +-
 7 files changed, 39 insertions(+), 19 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 49b846d960..761c70642b 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -769,8 +769,9 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 		memset(&sigcheck, '\0', sizeof(sigcheck));
 
 		bogs = parse_signed_buffer(push_cert.buf, push_cert.len);
-		check_signature(push_cert.buf, bogs, push_cert.buf + bogs,
-				push_cert.len - bogs, &sigcheck);
+		check_signature(push_cert.buf, bogs, 0, NULL,
+				push_cert.buf + bogs, push_cert.len - bogs,
+				&sigcheck);
 
 		nonce_status = check_nonce(push_cert.buf, bogs);
 	}
diff --git a/commit.c b/commit.c
index 551de4903c..1704d9df0a 100644
--- a/commit.c
+++ b/commit.c
@@ -1212,7 +1212,7 @@ int check_commit_signature(const struct commit *commit, struct signature_check *
 
 	if (parse_signed_commit(commit, &payload, &signature, the_hash_algo) <= 0)
 		goto out;
-	ret = check_signature(payload.buf, payload.len, signature.buf,
+	ret = check_signature(payload.buf, payload.len, 0, NULL, signature.buf,
 		signature.len, sigc);
 
  out:
diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 5216191488..d2cedad6b7 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -533,8 +533,8 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
 		else {
 			buf = payload.buf;
 			len = payload.len;
-			if (check_signature(payload.buf, payload.len, sig.buf,
-					    sig.len, &sigc) &&
+			if (check_signature(payload.buf, payload.len, 0, NULL,
+					    sig.buf, sig.len, &sigc) &&
 			    !sigc.output)
 				strbuf_addstr(&sig, "gpg verification failed.\n");
 			else
diff --git a/gpg-interface.c b/gpg-interface.c
index 800d8caa67..6049f7cbf7 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -20,7 +20,10 @@ struct gpg_format {
 	const char **sigs;
 	int (*verify_signed_buffer)(struct signature_check *sigc,
 				    struct gpg_format *fmt, const char *payload,
-				    size_t payload_size, const char *signature,
+				    size_t payload_size,
+				    timestamp_t payload_timestamp,
+				    struct strbuf *payload_signer,
+				    const char *signature,
 				    size_t signature_size);
 	int (*sign_buffer)(struct strbuf *buffer, struct strbuf *signature,
 			   const char *signing_key);
@@ -54,11 +57,17 @@ static const char *ssh_sigs[] = {
 
 static int verify_gpg_signed_buffer(struct signature_check *sigc,
 				    struct gpg_format *fmt, const char *payload,
-				    size_t payload_size, const char *signature,
+				    size_t payload_size,
+				    timestamp_t payload_timestamp,
+				    struct strbuf *payload_signer,
+				    const char *signature,
 				    size_t signature_size);
 static int verify_ssh_signed_buffer(struct signature_check *sigc,
 				    struct gpg_format *fmt, const char *payload,
-				    size_t payload_size, const char *signature,
+				    size_t payload_size,
+				    timestamp_t payload_timestamp,
+				    struct strbuf *payload_signer,
+				    const char *signature,
 				    size_t signature_size);
 static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
 			   const char *signing_key);
@@ -315,7 +324,10 @@ static void parse_gpg_output(struct signature_check *sigc)
 
 static int verify_gpg_signed_buffer(struct signature_check *sigc,
 				    struct gpg_format *fmt, const char *payload,
-				    size_t payload_size, const char *signature,
+				    size_t payload_size,
+				    timestamp_t payload_timestamp,
+				    struct strbuf *payload_signer,
+				    const char *signature,
 				    size_t signature_size)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
@@ -425,7 +437,10 @@ static void parse_ssh_output(struct signature_check *sigc)
 
 static int verify_ssh_signed_buffer(struct signature_check *sigc,
 				    struct gpg_format *fmt, const char *payload,
-				    size_t payload_size, const char *signature,
+				    size_t payload_size,
+				    timestamp_t payload_timestamp,
+				    struct strbuf *payload_signer,
+				    const char *signature,
 				    size_t signature_size)
 {
 	struct child_process ssh_keygen = CHILD_PROCESS_INIT;
@@ -560,8 +575,10 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 	return ret;
 }
 
-int check_signature(const char *payload, size_t plen, const char *signature,
-	size_t slen, struct signature_check *sigc)
+int check_signature(const char *payload, size_t plen,
+		    timestamp_t payload_timestamp,
+		    struct strbuf *payload_signer, const char *signature,
+		    size_t slen, struct signature_check *sigc)
 {
 	struct gpg_format *fmt;
 	int status;
@@ -573,8 +590,9 @@ int check_signature(const char *payload, size_t plen, const char *signature,
 	if (!fmt)
 		die(_("bad/incompatible signature '%s'"), signature);
 
-	status = fmt->verify_signed_buffer(sigc, fmt, payload, plen, signature,
-					   slen);
+	status = fmt->verify_signed_buffer(sigc, fmt, payload, plen,
+					   payload_timestamp, payload_signer,
+					   signature, slen);
 
 	if (status && !sigc->output)
 		return !!status;
diff --git a/gpg-interface.h b/gpg-interface.h
index beefacbb1e..f7c5389c90 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -71,8 +71,9 @@ const char *get_signing_key(void);
  */
 const char *get_signing_key_id(void);
 int check_signature(const char *payload, size_t plen,
-		    const char *signature, size_t slen,
-		    struct signature_check *sigc);
+		    timestamp_t payload_timestamp,
+		    struct strbuf *payload_signer, const char *signature,
+		    size_t slen, struct signature_check *sigc);
 void print_signature_buffer(const struct signature_check *sigc,
 			    unsigned flags);
 
diff --git a/log-tree.c b/log-tree.c
index 644893fd8c..3c3aec5c40 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -513,7 +513,7 @@ static void show_signature(struct rev_info *opt, struct commit *commit)
 	if (parse_signed_commit(commit, &payload, &signature, the_hash_algo) <= 0)
 		goto out;
 
-	status = check_signature(payload.buf, payload.len, signature.buf,
+	status = check_signature(payload.buf, payload.len, 0, NULL, signature.buf,
 				 signature.len, &sigc);
 	if (status && !sigc.output)
 		show_sig_lines(opt, status, "No signature\n");
@@ -583,7 +583,7 @@ static int show_one_mergetag(struct commit *commit,
 	status = -1;
 	if (parse_signature(extra->value, extra->len, &payload, &signature)) {
 		/* could have a good signature */
-		status = check_signature(payload.buf, payload.len,
+		status = check_signature(payload.buf, payload.len, 0, NULL,
 					 signature.buf, signature.len, &sigc);
 		if (sigc.output)
 			strbuf_addstr(&verify_message, sigc.output);
diff --git a/tag.c b/tag.c
index 3e18a41841..3459a0867c 100644
--- a/tag.c
+++ b/tag.c
@@ -25,7 +25,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 		return error("no signature found");
 	}
 
-	ret = check_signature(payload.buf, payload.len, signature.buf,
+	ret = check_signature(payload.buf, payload.len, 0, NULL, signature.buf,
 				signature.len, &sigc);
 
 	if (!(flags & GPG_VERIFY_OMIT_STATUS))
-- 
2.31.1

[PATCH 2/6] ssh signing: add key lifetime test prereqs

[Fabian Stelzer]

if ssh-keygen supports -Overify-time, add test keys marked as expired,
not yet valid and valid both within the test_tick timeframe and outside of it.

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 t/lib-gpg.sh | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index f99ef3e859..139e7a45ea 100644
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -90,6 +90,10 @@ test_lazy_prereq RFC1991 '
 GPGSSH_KEY_PRIMARY="${GNUPGHOME}/ed25519_ssh_signing_key"
 GPGSSH_KEY_SECONDARY="${GNUPGHOME}/rsa_2048_ssh_signing_key"
 GPGSSH_KEY_UNTRUSTED="${GNUPGHOME}/untrusted_ssh_signing_key"
+GPGSSH_KEY_EXPIRED="${GNUPGHOME}/expired_ssh_signing_key"
+GPGSSH_KEY_NOTYETVALID="${GNUPGHOME}/notyetvalid_ssh_signing_key"
+GPGSSH_KEY_TIMEBOXEDVALID="${GNUPGHOME}/timeboxed_valid_ssh_signing_key"
+GPGSSH_KEY_TIMEBOXEDINVALID="${GNUPGHOME}/timeboxed_invalid_ssh_signing_key"
 GPGSSH_KEY_WITH_PASSPHRASE="${GNUPGHOME}/protected_ssh_signing_key"
 GPGSSH_KEY_PASSPHRASE="super_secret"
 GPGSSH_ALLOWED_SIGNERS="${GNUPGHOME}/ssh.all_valid.allowedSignersFile"
@@ -112,7 +116,20 @@ test_lazy_prereq GPGSSH '
 	echo "\"principal with number 2\" $(cat "${GPGSSH_KEY_SECONDARY}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" &&
 	ssh-keygen -t ed25519 -N "${GPGSSH_KEY_PASSPHRASE}" -C "git ed25519 encrypted key" -f "${GPGSSH_KEY_WITH_PASSPHRASE}" >/dev/null &&
 	echo "\"principal with number 3\" $(cat "${GPGSSH_KEY_WITH_PASSPHRASE}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" &&
-	ssh-keygen -t ed25519 -N "" -f "${GPGSSH_KEY_UNTRUSTED}" >/dev/null
+	ssh-keygen -t ed25519 -N "" -C "git ed25519 key" -f "${GPGSSH_KEY_UNTRUSTED}" >/dev/null
+'
+
+test_lazy_prereq GPGSSH_VERIFYTIME '
+	# Check if ssh-keygen has a verify-time option by passing an invalid date to it
+	ssh-keygen -Overify-time=INVALID -Y check-novalidate -s doesnotmatter 2>&1 | grep -q -F "Invalid \"verify-time\"" &&
+	ssh-keygen -t ed25519 -N "" -C "timeboxed valid key" -f "${GPGSSH_KEY_TIMEBOXEDVALID}" >/dev/null &&
+	echo "\"timeboxed valid key\" valid-after=\"20050407000000\",valid-before=\"200504100000\" $(cat "${GPGSSH_KEY_TIMEBOXEDVALID}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" &&
+	ssh-keygen -t ed25519 -N "" -C "timeboxed invalid key" -f "${GPGSSH_KEY_TIMEBOXEDINVALID}" >/dev/null &&
+	echo "\"timeboxed invalid key\" valid-after=\"20050401000000\",valid-before=\"20050402000000\" $(cat "${GPGSSH_KEY_TIMEBOXEDINVALID}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" &&
+	ssh-keygen -t ed25519 -N "" -C "expired key" -f "${GPGSSH_KEY_EXPIRED}" >/dev/null &&
+	echo "\"principal with expired key\" valid-before=\"20000101000000\" $(cat "${GPGSSH_KEY_EXPIRED}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" &&
+	ssh-keygen -t ed25519 -N "" -C "not yet valid key" -f "${GPGSSH_KEY_NOTYETVALID}" >/dev/null &&
+	echo "\"principal with not yet valid key\" valid-after=\"29990101000000\" $(cat "${GPGSSH_KEY_NOTYETVALID}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}"
 '
 
 sanitize_pgp() {
-- 
2.31.1

[PATCH 3/6] ssh signing: verify-commit/check_signature with commit date

[Fabian Stelzer]

If valid-before/after dates are configured for this signatures key in the
allowedSigners file then the verification will check if the key was valid at
the time the commit was made. This allows for graceful key rollover and
revoking keys without invalidating all previous commits.
This feature needs openssh > 8.8. Older ssh-keygen versions will simply
ignore this flag and use the current time.
Strictly speaking this feature is available in 8.7, but since 8.7 has a
bug that makes it unusable in another needed call we require 8.8.

Timestamp information is present on most invocations of check_signature.
However signer ident is not. We will need the signer email / name to be able
to implement "Trust on first use" functionality.
Therefore we provide parse_signed_buffer_metadata() for parsing both from
the payload buffer.

Passes commit date for verify-commit on to check_signature.
Adds test for keys being expired, not yet valid, having a commit date outside
of key validity and within.

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 Documentation/config/gpg.txt |  5 +++++
 commit.c                     | 12 ++++++++--
 gpg-interface.c              | 43 ++++++++++++++++++++++++++++++++++++
 gpg-interface.h              |  8 +++++++
 t/t7528-signed-commit-ssh.sh | 42 +++++++++++++++++++++++++++++++++++
 5 files changed, 108 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt
index 4f30c7dbdd..c9be554c73 100644
--- a/Documentation/config/gpg.txt
+++ b/Documentation/config/gpg.txt
@@ -64,6 +64,11 @@ A repository that only allows signed commits can store the file
 in the repository itself using a path relative to the top-level of the working tree.
 This way only committers with an already valid key can add or change keys in the keyring.
 +
+Since OpensSSH 8.8 this file allows specifying a key lifetime using valid-after &
+valid-before options. Git will mark signatures as valid if the signing key was
+valid at the time of the signatures creation. This allows users to change a
+signing key without invalidating all previously made signatures.
++
 Using a SSH CA key with the cert-authority option
 (see ssh-keygen(1) "CERTIFICATES") is also valid.
 
diff --git a/commit.c b/commit.c
index 1704d9df0a..e5dfbfcf8e 100644
--- a/commit.c
+++ b/commit.c
@@ -1207,17 +1207,25 @@ int check_commit_signature(const struct commit *commit, struct signature_check *
 	struct strbuf payload = STRBUF_INIT;
 	struct strbuf signature = STRBUF_INIT;
 	int ret = 1;
+	struct strbuf payload_signer = STRBUF_INIT;
+	timestamp_t payload_timestamp = 0;
 
 	sigc->result = 'N';
 
 	if (parse_signed_commit(commit, &payload, &signature, the_hash_algo) <= 0)
 		goto out;
-	ret = check_signature(payload.buf, payload.len, 0, NULL, signature.buf,
-		signature.len, sigc);
+	if (parse_signed_buffer_metadata(payload.buf, "committer",
+					 &payload_timestamp, &payload_signer))
+		goto out;
+
+	ret = check_signature(payload.buf, payload.len, payload_timestamp,
+			      &payload_signer, signature.buf, signature.len,
+			      sigc);
 
  out:
 	strbuf_release(&payload);
 	strbuf_release(&signature);
+	strbuf_release(&payload_signer);
 
 	return ret;
 }
diff --git a/gpg-interface.c b/gpg-interface.c
index 6049f7cbf7..fed5113297 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -453,6 +453,13 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 	struct strbuf ssh_principals_err = STRBUF_INIT;
 	struct strbuf ssh_keygen_out = STRBUF_INIT;
 	struct strbuf ssh_keygen_err = STRBUF_INIT;
+	struct strbuf verify_time = STRBUF_INIT;
+	const struct date_mode verify_date_mode = {
+		.type = DATE_STRFTIME,
+		.strftime_fmt = "%Y%m%d%H%M%S",
+		/* SSH signing key validity has no timezone information - Use the local timezone */
+		.local = 1,
+	};
 
 	if (!ssh_allowed_signers) {
 		error(_("gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification"));
@@ -470,11 +477,17 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 		return -1;
 	}
 
+	if (payload_timestamp) {
+		strbuf_addf(&verify_time, "-Overify-time=%s",
+			    show_date(payload_timestamp, 0, &verify_date_mode));
+	}
+
 	/* Find the principal from the signers */
 	strvec_pushl(&ssh_keygen.args, fmt->program,
 		     "-Y", "find-principals",
 		     "-f", ssh_allowed_signers,
 		     "-s", buffer_file->filename.buf,
+		     verify_time.buf,
 		     NULL);
 	ret = pipe_command(&ssh_keygen, NULL, 0, &ssh_principals_out, 0,
 			   &ssh_principals_err, 0);
@@ -482,6 +495,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 		error(_("ssh-keygen -Y find-principals/verify is needed for ssh signature verification (available in openssh version 8.2p1+)"));
 		goto out;
 	}
+
 	if (ret || !ssh_principals_out.len) {
 		/*
 		 * We did not find a matching principal in the allowedSigners
@@ -492,6 +506,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 			     "-Y", "check-novalidate",
 			     "-n", "git",
 			     "-s", buffer_file->filename.buf,
+			     verify_time.buf,
 			     NULL);
 		pipe_command(&ssh_keygen, payload, payload_size,
 				   &ssh_keygen_out, 0, &ssh_keygen_err, 0);
@@ -526,6 +541,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 				     "-f", ssh_allowed_signers,
 				     "-I", principal,
 				     "-s", buffer_file->filename.buf,
+				     verify_time.buf,
 				     NULL);
 
 			if (ssh_revocation_file) {
@@ -571,10 +587,37 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
 	strbuf_release(&ssh_principals_err);
 	strbuf_release(&ssh_keygen_out);
 	strbuf_release(&ssh_keygen_err);
+	strbuf_release(&verify_time);
 
 	return ret;
 }
 
+int parse_signed_buffer_metadata(const char *payload, const char *signer_header,
+				 timestamp_t *payload_timestamp,
+				 struct strbuf *payload_signer)
+{
+	const char *ident_line = NULL;
+	size_t ident_len;
+	struct ident_split ident;
+
+	ident_line = find_commit_header(payload, signer_header, &ident_len);
+	if (ident_line && ident_len) {
+		if (!split_ident_line(&ident, ident_line, ident_len)) {
+			if (payload_timestamp && ident.date_begin &&
+			    ident.date_end)
+				*payload_timestamp = parse_timestamp(
+					ident.date_begin, NULL, 10);
+			if (payload_signer)
+				strbuf_add(payload_signer, ident.mail_begin,
+					(ident.mail_end - ident.mail_begin));
+
+			return 0;
+		}
+	}
+
+	return 1;
+}
+
 int check_signature(const char *payload, size_t plen,
 		    timestamp_t payload_timestamp,
 		    struct strbuf *payload_signer, const char *signature,
diff --git a/gpg-interface.h b/gpg-interface.h
index f7c5389c90..1cbecbf4c1 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -65,6 +65,14 @@ int git_gpg_config(const char *, const char *, void *);
 void set_signing_key(const char *);
 const char *get_signing_key(void);
 
+/*
+ * Tries to find "signer_header" (usually committer or tagger) in the payload,
+ * and sets the corresponding timestamp and signer parameters if found.
+ */
+int parse_signed_buffer_metadata(const char *payload, const char *signer_header,
+				 timestamp_t *payload_timestamp,
+				 struct strbuf *payload_signer);
+
 /*
  * Returns a textual unique representation of the signing key in use
  * Either a GPG KeyID or a SSH Key Fingerprint
diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
index badf3ed320..dae76ded0c 100755
--- a/t/t7528-signed-commit-ssh.sh
+++ b/t/t7528-signed-commit-ssh.sh
@@ -76,6 +76,23 @@ test_expect_success GPGSSH 'create signed commits' '
 	git tag twelfth-signed-alt $(cat oid)
 '
 
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'create signed commits with keys having defined lifetimes' '
+	test_when_finished "test_unconfig commit.gpgsign" &&
+	test_config gpg.format ssh &&
+
+	echo expired >file && test_tick && git commit -a -m expired -S"${GPGSSH_KEY_EXPIRED}" &&
+	git tag expired-signed &&
+
+	echo notyetvalid >file && test_tick && git commit -a -m notyetvalid -S"${GPGSSH_KEY_NOTYETVALID}" &&
+	git tag notyetvalid-signed &&
+
+	echo timeboxedvalid >file && test_tick && git commit -a -m timeboxedvalid -S"${GPGSSH_KEY_TIMEBOXEDVALID}" &&
+	git tag timeboxedvalid-signed &&
+
+	echo timeboxedinvalid >file && test_tick && git commit -a -m timeboxedinvalid -S"${GPGSSH_KEY_TIMEBOXEDINVALID}" &&
+	git tag timeboxedinvalid-signed
+'
+
 test_expect_success GPGSSH 'verify and show signatures' '
 	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
 	test_config gpg.mintrustlevel UNDEFINED &&
@@ -122,6 +139,31 @@ test_expect_success GPGSSH 'verify-commit exits failure on untrusted signature'
 	grep "${GPGSSH_KEY_NOT_TRUSTED}" actual
 '
 
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-commit exits failure on expired signature key' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	test_must_fail git verify-commit expired-signed 2>actual &&
+	! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
+'
+
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-commit exits failure on not yet valid signature key' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	test_must_fail git verify-commit notyetvalid-signed 2>actual &&
+	! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
+'
+
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-commit succeeds with commit date and key validity matching' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	git verify-commit timeboxedvalid-signed 2>actual &&
+	grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual &&
+	! grep "${GPGSSH_BAD_SIGNATURE}" actual
+'
+
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-commit exits failure with commit date outside of key validity' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	test_must_fail git verify-commit timeboxedinvalid-signed 2>actual &&
+	! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
+'
+
 test_expect_success GPGSSH 'verify-commit exits success with matching minTrustLevel' '
 	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
 	test_config gpg.minTrustLevel fully &&
-- 
2.31.1

[PATCH 4/6] ssh signing: git log/check_signature with commit date

[Fabian Stelzer]

Pass the commit date and ident to check_signature when calling git log.
Implements the same tests as for verify-commit.

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 log-tree.c     | 24 +++++++++++++++++++++---
 t/t4202-log.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 3c3aec5c40..2b29874265 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -509,12 +509,19 @@ static void show_signature(struct rev_info *opt, struct commit *commit)
 	struct strbuf signature = STRBUF_INIT;
 	struct signature_check sigc = { 0 };
 	int status;
+	struct strbuf payload_signer = STRBUF_INIT;
+	timestamp_t payload_timestamp = 0;
 
 	if (parse_signed_commit(commit, &payload, &signature, the_hash_algo) <= 0)
 		goto out;
 
-	status = check_signature(payload.buf, payload.len, 0, NULL, signature.buf,
-				 signature.len, &sigc);
+	if (parse_signed_buffer_metadata(payload.buf, "committer",
+					 &payload_timestamp, &payload_signer))
+		goto out;
+
+	status = check_signature(payload.buf, payload.len, payload_timestamp,
+				 &payload_signer, signature.buf, signature.len,
+				 &sigc);
 	if (status && !sigc.output)
 		show_sig_lines(opt, status, "No signature\n");
 	else
@@ -524,6 +531,7 @@ static void show_signature(struct rev_info *opt, struct commit *commit)
  out:
 	strbuf_release(&payload);
 	strbuf_release(&signature);
+	strbuf_release(&payload_signer);
 }
 
 static int which_parent(const struct object_id *oid, const struct commit *commit)
@@ -558,6 +566,8 @@ static int show_one_mergetag(struct commit *commit,
 	int status, nth;
 	struct strbuf payload = STRBUF_INIT;
 	struct strbuf signature = STRBUF_INIT;
+	struct strbuf payload_signer = STRBUF_INIT;
+	timestamp_t payload_timestamp = 0;
 
 	hash_object_file(the_hash_algo, extra->value, extra->len,
 			 type_name(OBJ_TAG), &oid);
@@ -582,8 +592,15 @@ static int show_one_mergetag(struct commit *commit,
 
 	status = -1;
 	if (parse_signature(extra->value, extra->len, &payload, &signature)) {
+		if (parse_signed_buffer_metadata(payload.buf, "tagger",
+						 &payload_timestamp,
+						 &payload_signer))
+			strbuf_addstr(&verify_message,
+				_("failed to parse timestamp and signer info from payload"));
+
 		/* could have a good signature */
-		status = check_signature(payload.buf, payload.len, 0, NULL,
+		status = check_signature(payload.buf, payload.len,
+					 payload_timestamp, &payload_signer,
 					 signature.buf, signature.len, &sigc);
 		if (sigc.output)
 			strbuf_addstr(&verify_message, sigc.output);
@@ -597,6 +614,7 @@ static int show_one_mergetag(struct commit *commit,
 	strbuf_release(&verify_message);
 	strbuf_release(&payload);
 	strbuf_release(&signature);
+	strbuf_release(&payload_signer);
 	return 0;
 }
 
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 6a650dacd6..2b12baab77 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1626,6 +1626,24 @@ test_expect_success GPGSSH 'setup sshkey signed branch' '
 	git commit -S -m signed_commit
 '
 
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'create signed commits with keys having defined lifetimes' '
+	test_config gpg.format ssh &&
+	touch file &&
+	git add file &&
+
+	echo expired >file && test_tick && git commit -a -m expired -S"${GPGSSH_KEY_EXPIRED}" &&
+	git tag expired-signed &&
+
+	echo notyetvalid >file && test_tick && git commit -a -m notyetvalid -S"${GPGSSH_KEY_NOTYETVALID}" &&
+	git tag notyetvalid-signed &&
+
+	echo timeboxedvalid >file && test_tick && git commit -a -m timeboxedvalid -S"${GPGSSH_KEY_TIMEBOXEDVALID}" &&
+	git tag timeboxedvalid-signed &&
+
+	echo timeboxedinvalid >file && test_tick && git commit -a -m timeboxedinvalid -S"${GPGSSH_KEY_TIMEBOXEDINVALID}" &&
+	git tag timeboxedinvalid-signed
+'
+
 test_expect_success GPGSM 'log x509 fingerprint' '
 	echo "F8BF62E0693D0694816377099909C779FA23FD65 | " >expect &&
 	git log -n1 --format="%GF | %GP" signed-x509 >actual &&
@@ -1663,6 +1681,31 @@ test_expect_success GPGSSH 'log --graph --show-signature ssh' '
 	grep "${GOOD_SIGNATURE_TRUSTED}" actual
 '
 
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'log shows failure on expired signature key' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	git log --graph --show-signature -n1 expired-signed >actual &&
+	! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
+'
+
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'log shows failure on not yet valid signature key' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	git log --graph --show-signature -n1 notyetvalid-signed >actual &&
+	! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
+'
+
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'log show success with commit date and key validity matching' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	git log --graph --show-signature -n1 timeboxedvalid-signed >actual &&
+	grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual &&
+	! grep "${GPGSSH_BAD_SIGNATURE}" actual
+'
+
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'log shows failure with commit date outside of key validity' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	git log --graph --show-signature -n1 timeboxedinvalid-signed >actual &&
+	! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
+'
+
 test_expect_success GPG 'log --graph --show-signature for merged tag' '
 	test_when_finished "git reset --hard && git checkout main" &&
 	git checkout -b plain main &&
-- 
2.31.1

[PATCH 5/6] ssh signing: verify-tag/check_signature with tag date

[Fabian Stelzer]

Passes tag date and tagger ident to to check_signature.
Implements the same tests as for verify-commit.

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 t/t7031-verify-tag-signed-ssh.sh | 42 ++++++++++++++++++++++++++++++++
 tag.c                            | 12 +++++++--
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/t/t7031-verify-tag-signed-ssh.sh b/t/t7031-verify-tag-signed-ssh.sh
index 06c9dd6c93..1cb36b9ab8 100755
--- a/t/t7031-verify-tag-signed-ssh.sh
+++ b/t/t7031-verify-tag-signed-ssh.sh
@@ -48,6 +48,23 @@ test_expect_success GPGSSH 'create signed tags ssh' '
 	git tag -u"${GPGSSH_KEY_UNTRUSTED}" -m eighth eighth-signed-alt
 '
 
+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 &&
+
+	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 GPGSSH 'verify and show ssh signatures' '
 	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
 	(
@@ -80,6 +97,31 @@ test_expect_success GPGSSH 'verify and show ssh signatures' '
 	)
 '
 
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-tag exits failure on expired signature key' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	test_must_fail git verify-tag expired-signed 2>actual &&
+	! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
+'
+
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-tag exits failure on not yet valid signature key' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	test_must_fail git verify-tag notyetvalid-signed 2>actual &&
+	! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
+'
+
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-tag succeeds with tag date and key validity matching' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	git verify-tag timeboxedvalid-signed 2>actual &&
+	grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual &&
+	! grep "${GPGSSH_BAD_SIGNATURE}" actual
+'
+
+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-tag failes with tag date outside of key validity' '
+	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
+	test_must_fail git verify-tag timeboxedinvalid-signed 2>actual &&
+	! grep "${GPGSSH_GOOD_SIGNATURE_TRUSTED}" actual
+'
+
 test_expect_success GPGSSH 'detect fudged ssh signature' '
 	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
 	git cat-file tag seventh-signed >raw &&
diff --git a/tag.c b/tag.c
index 3459a0867c..d26053ff4d 100644
--- a/tag.c
+++ b/tag.c
@@ -16,6 +16,8 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 	struct strbuf payload = STRBUF_INIT;
 	struct strbuf signature = STRBUF_INIT;
 	int ret;
+	struct strbuf payload_signer = STRBUF_INIT;
+	timestamp_t payload_timestamp = 0;
 
 	memset(&sigc, 0, sizeof(sigc));
 
@@ -25,8 +27,13 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 		return error("no signature found");
 	}
 
-	ret = check_signature(payload.buf, payload.len, 0, NULL, signature.buf,
-				signature.len, &sigc);
+	if (parse_signed_buffer_metadata(payload.buf, "tagger",
+					 &payload_timestamp, &payload_signer))
+		return error(_("failed to parse timestamp and signer info from payload"));
+
+	ret = check_signature(payload.buf, payload.len, payload_timestamp,
+			      &payload_signer, signature.buf, signature.len,
+			      &sigc);
 
 	if (!(flags & GPG_VERIFY_OMIT_STATUS))
 		print_signature_buffer(&sigc, flags);
@@ -34,6 +41,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 	signature_check_clear(&sigc);
 	strbuf_release(&payload);
 	strbuf_release(&signature);
+	strbuf_release(&payload_signer);
 	return ret;
 }
 
-- 
2.31.1

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

[Fabian Stelzer]

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(-)

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 &&
 
-- 
2.31.1

Re: [PATCH 3/6] ssh signing: verify-commit/check_signature with commit date

[Ævar Arnfjörð Bjarmason]

On Fri, Oct 22 2021, Fabian Stelzer wrote:

[Just nits]

> +	if (payload_timestamp) {
> +		strbuf_addf(&verify_time, "-Overify-time=%s",
> +			    show_date(payload_timestamp, 0, &verify_date_mode));
> +	}

No braces needed.

> @@ -482,6 +495,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,
>  		error(_("ssh-keygen -Y find-principals/verify is needed for ssh signature verification (available in openssh version 8.2p1+)"));
>  		goto out;
>  	}
> +
>  	if (ret || !ssh_principals_out.len) {
>  		/*
>  		 * We did not find a matching principal in the allowedSigners

Stray whitespace change.

> +int parse_signed_buffer_metadata(const char *payload, const char *signer_header,
> +				 timestamp_t *payload_timestamp,
> +				 struct strbuf *payload_signer)
> +{
> +	const char *ident_line = NULL;
> +	size_t ident_len;
> +	struct ident_split ident;
> +
> +	ident_line = find_commit_header(payload, signer_header, &ident_len);
> +	if (ident_line && ident_len) {
> +		if (!split_ident_line(&ident, ident_line, ident_len)) {
> +			if (payload_timestamp && ident.date_begin &&
> +			    ident.date_end)
> +				*payload_timestamp = parse_timestamp(
> +					ident.date_begin, NULL, 10);
> +			if (payload_signer)
> +				strbuf_add(payload_signer, ident.mail_begin,
> +					(ident.mail_end - ident.mail_begin));
> +
> +			return 0;
> +		}
> +	}
> +
> +	return 1;
> +}

This would be more readable with less nesting, i.e. instead of:

    if (x) {
        if (y) {
            [...]

Doing:

    if (!x)
        return 1;
    if (!y)
        return 1;

I.e. only if you get zero from split_ident_line() do you return 0.

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

[Ævar Arnfjörð Bjarmason]

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...

Re: [PATCH 1/6] ssh signing: extend check_signature to accept payload metadata

[Junio C Hamano]

Fabian Stelzer <fs@gigacodes.de> writes:

> Adds two new parameters to the check_signature api and passes them to the

"Add". "pass".

> internal verification ssh/gpg methods.

> A payload timestamp that will be used to verify signatures at the time of their
> objects creation if the signing method supports it (only ssh for now).

OK.

> And a signer strbuf containing ident information about the signer that
> we will need for implementing "Trust on first use" in a future patch
> series.

OK.

It would flow better in our "git log" stream if you explain upfront
what problem you are trying to solve, before starting to give orders
to the codebase to pass these pieces of information.  Something
like:

    In order to implement the "trust on the first use of a key"
    behaviour in later steps, allow callers to optionally pass the
    timestamp of the payload and the identity of the signer to
    check_signature().

It is unclear what "payload timestamp" is without actually seeing
how it is used, so if you cannot explain it in easy terms in the log
message for this step, it may be an indication that it is not a such
good idea to add these parameters in a separate step.

>  int check_signature(const char *payload, size_t plen,
> -		    const char *signature, size_t slen,
> -		    struct signature_check *sigc);
> +		    timestamp_t payload_timestamp,
> +		    struct strbuf *payload_signer, const char *signature,
> +		    size_t slen, struct signature_check *sigc);

Funny line wrapping.  Just like payload and plen form a pair (hence
they sit next to each other on the same line), signature and slen
should sit next to each other on the same line.

What's the reason why payload-signer MUST come in a strbuf?  A
caller that has only ptr and len must invent a new strbuf that is
otherwise unused to call this function because of that calling
convention, which looks suboptimal.

If the function accepts <ptr, len>, just like <payload, plen> or
<signature, slen> are taken by the function, such a caller can just
call the function without having to have an extra instance of
strbuf, while a caller that happens to already have a strbuf can
pass <buf.buf, buf.len> as two separate parameters.

What's the driving principle that decided where in the list of
parameters these two new ones are added?

I would explain one possible order I may find "logical" using the
level of detail I expect from an answer to "what's the guiding
principle?" as an example here:

 - we should move 'sigc' to the beginning, because the convention
   used by many of our API functions that have some central
   structure to work with is to have such a structure as the first
   thing in the list of parameters;

 - we should then append 'payload_timestamp', 'payload_signer', and
   'payload_signer_len' at the end, as the function is about
   "validate <signature, slen> is valid for <payload, plen> and
   leave the result in <sigc>", and anything we add is auxiliary
   input to the process, which are of less significance than the
   existing ones.

Another possible direction to go might be to add these auxiliary
input to the process to the signature_check structure.  Then this
step can disappear, or just flip the order of the parameter to move
sigc to the beginning, and then the later stemp that does the "first
use" thing can add the necessary members to the structure *and* use
the members to do its thing, which helps readers better understand
what is going on.

One possible downside is that sigc has been mostly output-only
structure, and turning it into a structure that also has some input
members might make it too confusing.  I dunno.

Thanks.

Re: [PATCH 1/6] ssh signing: extend check_signature to accept payload metadata

[Fabian Stelzer]

On 24.10.21 01:13, Junio C Hamano wrote:
> Fabian Stelzer <fs@gigacodes.de> writes:
> 
> It would flow better in our "git log" stream if you explain upfront
> what problem you are trying to solve, before starting to give orders
> to the codebase to pass these pieces of information.  Something
> like:
> 
>     In order to implement the "trust on the first use of a key"
>     behaviour in later steps, allow callers to optionally pass the
>     timestamp of the payload and the identity of the signer to
>     check_signature().

Thanks, you're right. I will update the commit message.

> 
> It is unclear what "payload timestamp" is without actually seeing
> how it is used, so if you cannot explain it in easy terms in the log
> message for this step, it may be an indication that it is not a such
> good idea to add these parameters in a separate step.
> 
>>  int check_signature(const char *payload, size_t plen,
>> -		    const char *signature, size_t slen,
>> -		    struct signature_check *sigc);
>> +		    timestamp_t payload_timestamp,
>> +		    struct strbuf *payload_signer, const char *signature,
>> +		    size_t slen, struct signature_check *sigc);
> 
> Funny line wrapping.  Just like payload and plen form a pair (hence
> they sit next to each other on the same line), signature and slen
> should sit next to each other on the same line.

clang-format enforced the 80 char limit here i think :/
i think we will change this function anyway (see below) and i will keep
it in mind then.

> 
> What's the reason why payload-signer MUST come in a strbuf?  A
> caller that has only ptr and len must invent a new strbuf that is
> otherwise unused to call this function because of that calling
> convention, which looks suboptimal.

It does not necessarily need to be. I just like the convenience of the
strbuf_ functions for working with strings. Also i think it makes it
very specific and easy to grasp in the calling function to STRBUF_INIT
and then release it at the end. When working with char* it can often be
not immediately clear (at least not t ome) if i need to provide an
existing buffer or just a pointer and if need to free what was returned.
char*/const char* usually indicates this, but this is not always the
case. Maybe my "C foo" is just not strong enough and i like the higher
level api :D
If my change proposed below is acceptable this problem solves itself as
well.

> 
> 
> What's the driving principle that decided where in the list of
> parameters these two new ones are added?
> 
> I would explain one possible order I may find "logical" using the
> level of detail I expect from an answer to "what's the guiding
> principle?" as an example here:
> 
>  - we should move 'sigc' to the beginning, because the convention
>    used by many of our API functions that have some central
>    structure to work with is to have such a structure as the first
>    thing in the list of parameters;
> 
>  - we should then append 'payload_timestamp', 'payload_signer', and
>    'payload_signer_len' at the end, as the function is about
>    "validate <signature, slen> is valid for <payload, plen> and
>    leave the result in <sigc>", and anything we add is auxiliary
>    input to the process, which are of less significance than the
>    existing ones.
> 
> Another possible direction to go might be to add these auxiliary
> input to the process to the signature_check structure.  Then this
> step can disappear, or just flip the order of the parameter to move
> sigc to the beginning, and then the later stemp that does the "first
> use" thing can add the necessary members to the structure *and* use
> the members to do its thing, which helps readers better understand
> what is going >
> One possible downside is that sigc has been mostly output-only
> structure, and turning it into a structure that also has some input
> members might make it too confusing.  I dunno.

When starting this series my plan was indeed to add these parameters to
the signature_check struct. As you correctly mentioned it is "output
only" at the moment. That's why i chose to add the extra parameters. I
added them in the middle of the parameter list next to the payload
itself since i consider them to be part of to the payload. And since
sigc is output only i would put it at the end (not sure if any
convention for things like this exist for git).

To put it in words:
check_signature uses payload/plen with timestamp & ident to check
signature/slen and return the result via sigc.

Or maybe alternatively:
check_signature checks payload/plen to check signature/slen, constrained
by timestamp & ident and return the result via sigc.

However if everyone is ok with changing the struct to be used for input
as well then i would adjust the function to have it as the first parameter.

The sigc struct already has a payload member (only used for verbose
output) which is populated by the check function with a xmemdupz. I
would then change it to a const char, add the length var and use it to
pass the payload into the function as well. This way we also avoid the
unnecessary mem copy.

The function would just become:
check_signature(struct signature_check *sigc, const char *signature,
size_t slen)

> 
> Thanks.
> 

Thanks for your review.

Re: [PATCH 3/6] ssh signing: verify-commit/check_signature with commit date

[Fabian Stelzer]

On 22.10.21 19:37, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Oct 22 2021, Fabian Stelzer wrote:
> 
> [Just nits]
> 

Thanks for your review. I will fix/adjust all of those in the next
iteration.

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

[Fabian Stelzer]

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

Re: [PATCH 1/6] ssh signing: extend check_signature to accept payload metadata

[Junio C Hamano]

Fabian Stelzer <fs@gigacodes.de> writes:

> On 24.10.21 01:13, Junio C Hamano wrote:
>>
>> One possible downside is that sigc has been mostly output-only
>> structure, and turning it into a structure that also has some input
>> members might make it too confusing.  I dunno.
>
> However if everyone is ok with changing the struct to be used for input
> as well then i would adjust the function to have it as the first parameter.
>
> The sigc struct already has a payload member (only used for verbose
> output) which is populated by the check function with a xmemdupz. I
> would then change it to a const char, add the length var and use it to
> pass the payload into the function as well. This way we also avoid the
> unnecessary mem copy.
>
> The function would just become:
> check_signature(struct signature_check *sigc, const char *signature,
> size_t slen)

I do not offhand think of a huge downside going that route myself.
We seem to end up with unusually large number of folks on the CC
list for some reason, so hopefully somebody will stop us if it is a
stupid idea ;-)

Thanks.