{"thread":{"id":"56763","subject":"[PATCH 0/6] ssh signing: verify key lifetime","startedAt":"2021-10-22T15:10:07Z","lastAt":"2021-10-25T17:16:59Z","messageCount":14,"participants":["Fabian Stelzer","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"439381","messageId":"20211022150949.1754477-1-fs@gigacodes.de","threadId":"56763","inReplyTo":null,"subject":"[PATCH 0/6] ssh signing: verify key lifetime","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-10-22T15:09:43Z","receivedAt":"2021-10-22T15:10:07Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"This series adds key lifetime validity checks by passing commit/tag\ndates to the check operaion.\n\nI'm not totally sure about the way i'm parsing payload info via\nparse_signed_buffer_metadata(). payload was just an opaque buffer before.\nNow gpg-interface actually looks at it (thats wy i used a seperate api\nfunction for it and did not hide it in check_signature).\nI chose this approach since it requires the least changes for the consumers\nof this functionality.\n\nA cleaner way would maybe be introducing a check_object_signature()\nwhich takes a struct object instead of the payload directly to avoid the\nparse function altogether. However only some call sites already have\nthis struct. Tags & fmt-merge-msg use different ways to produce the\npayload and i'm not sure how involved the objects creation would be or what\nside-effects this could have(performance?). Since the push-certs use case\nwill never produce a object struct we would still have to keep the\nexisting check_signature function anyway (or encapsulate those in some\npseudo-object :/).\nThe now used parse function also opens the usage up to sites already having\nboth infos (date & ident), although there is currently none.\n\nFabian Stelzer (6):\n  ssh signing: extend check_signature to accept payload metadata\n  ssh signing: add key lifetime test prereqs\n  ssh signing: verify-commit/check_signature with commit date\n  ssh signing: git log/check_signature with commit date\n  ssh signing: verify-tag/check_signature with tag date\n  ssh signing: fmt-merge-msg/check_signature with tag date\n\n Documentation/config/gpg.txt     |  5 ++\n builtin/receive-pack.c           |  5 +-\n commit.c                         | 12 ++++-\n fmt-merge-msg.c                  | 15 +++++-\n gpg-interface.c                  | 79 ++++++++++++++++++++++++++++----\n gpg-interface.h                  | 13 +++++-\n log-tree.c                       | 22 ++++++++-\n t/lib-gpg.sh                     | 19 +++++++-\n t/t4202-log.sh                   | 43 +++++++++++++++++\n t/t6200-fmt-merge-msg.sh         | 54 ++++++++++++++++++++++\n t/t7031-verify-tag-signed-ssh.sh | 42 +++++++++++++++++\n t/t7528-signed-commit-ssh.sh     | 42 +++++++++++++++++\n tag.c                            | 12 ++++-\n 13 files changed, 341 insertions(+), 22 deletions(-)\n\n\nbase-commit: d3b4e01def5a9517c919f0b815c1b12296dc3dc2\n-- \n2.31.1\n\n"},{"id":"439382","messageId":"20211022150949.1754477-2-fs@gigacodes.de","threadId":"56763","inReplyTo":"20211022150949.1754477-1-fs@gigacodes.de","subject":"[PATCH 1/6] ssh signing: extend check_signature to accept payload metadata","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-10-22T15:09:44Z","receivedAt":"2021-10-22T15:10:07Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"Adds two new parameters to the check_signature api and passes them to the\ninternal verification ssh/gpg methods.\nA payload timestamp that will be used to verify signatures at the time of their\nobjects creation if the signing method supports it (only ssh for now).\nAnd a signer strbuf containing ident information about the signer that\nwe will need for implementing \"Trust on first use\" in a future patch\nseries.\n\nAdding the ident info right now instead of a later patch series makes\ncertain choices in this patch series clearer. Only passing the timestamp\ncould be done a bit simpler on some call sites but i am certain that we\nwill need the ident info for \"Trust on first use\" no matter how exactly\nit will be implemented later.\n\nTo start with we pass 0, NULL on all invocations to keep the current behaviour\nas is.\n\nSigned-off-by: Fabian Stelzer <fs@gigacodes.de>\n---\n builtin/receive-pack.c |  5 +++--\n commit.c               |  2 +-\n fmt-merge-msg.c        |  4 ++--\n gpg-interface.c        | 36 +++++++++++++++++++++++++++---------\n gpg-interface.h        |  5 +++--\n log-tree.c             |  4 ++--\n tag.c                  |  2 +-\n 7 files changed, 39 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 49b846d960..761c70642b 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -769,8 +769,9 @@ static void prepare_push_cert_sha1(struct child_process *proc)\n \t\tmemset(&sigcheck, '\\0', sizeof(sigcheck));\n \n \t\tbogs = parse_signed_buffer(push_cert.buf, push_cert.len);\n-\t\tcheck_signature(push_cert.buf, bogs, push_cert.buf + bogs,\n-\t\t\t\tpush_cert.len - bogs, &sigcheck);\n+\t\tcheck_signature(push_cert.buf, bogs, 0, NULL,\n+\t\t\t\tpush_cert.buf + bogs, push_cert.len - bogs,\n+\t\t\t\t&sigcheck);\n \n \t\tnonce_status = check_nonce(push_cert.buf, bogs);\n \t}\ndiff --git a/commit.c b/commit.c\nindex 551de4903c..1704d9df0a 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1212,7 +1212,7 @@ int check_commit_signature(const struct commit *commit, struct signature_check *\n \n \tif (parse_signed_commit(commit, &payload, &signature, the_hash_algo) <= 0)\n \t\tgoto out;\n-\tret = check_signature(payload.buf, payload.len, signature.buf,\n+\tret = check_signature(payload.buf, payload.len, 0, NULL, signature.buf,\n \t\tsignature.len, sigc);\n \n  out:\ndiff --git a/fmt-merge-msg.c b/fmt-merge-msg.c\nindex 5216191488..d2cedad6b7 100644\n--- a/fmt-merge-msg.c\n+++ b/fmt-merge-msg.c\n@@ -533,8 +533,8 @@ static void fmt_merge_msg_sigs(struct strbuf *out)\n \t\telse {\n \t\t\tbuf = payload.buf;\n \t\t\tlen = payload.len;\n-\t\t\tif (check_signature(payload.buf, payload.len, sig.buf,\n-\t\t\t\t\t    sig.len, &sigc) &&\n+\t\t\tif (check_signature(payload.buf, payload.len, 0, NULL,\n+\t\t\t\t\t    sig.buf, sig.len, &sigc) &&\n \t\t\t    !sigc.output)\n \t\t\t\tstrbuf_addstr(&sig, \"gpg verification failed.\\n\");\n \t\t\telse\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 800d8caa67..6049f7cbf7 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -20,7 +20,10 @@ struct gpg_format {\n \tconst char **sigs;\n \tint (*verify_signed_buffer)(struct signature_check *sigc,\n \t\t\t\t    struct gpg_format *fmt, const char *payload,\n-\t\t\t\t    size_t payload_size, const char *signature,\n+\t\t\t\t    size_t payload_size,\n+\t\t\t\t    timestamp_t payload_timestamp,\n+\t\t\t\t    struct strbuf *payload_signer,\n+\t\t\t\t    const char *signature,\n \t\t\t\t    size_t signature_size);\n \tint (*sign_buffer)(struct strbuf *buffer, struct strbuf *signature,\n \t\t\t   const char *signing_key);\n@@ -54,11 +57,17 @@ static const char *ssh_sigs[] = {\n \n static int verify_gpg_signed_buffer(struct signature_check *sigc,\n \t\t\t\t    struct gpg_format *fmt, const char *payload,\n-\t\t\t\t    size_t payload_size, const char *signature,\n+\t\t\t\t    size_t payload_size,\n+\t\t\t\t    timestamp_t payload_timestamp,\n+\t\t\t\t    struct strbuf *payload_signer,\n+\t\t\t\t    const char *signature,\n \t\t\t\t    size_t signature_size);\n static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \t\t\t\t    struct gpg_format *fmt, const char *payload,\n-\t\t\t\t    size_t payload_size, const char *signature,\n+\t\t\t\t    size_t payload_size,\n+\t\t\t\t    timestamp_t payload_timestamp,\n+\t\t\t\t    struct strbuf *payload_signer,\n+\t\t\t\t    const char *signature,\n \t\t\t\t    size_t signature_size);\n static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,\n \t\t\t   const char *signing_key);\n@@ -315,7 +324,10 @@ static void parse_gpg_output(struct signature_check *sigc)\n \n static int verify_gpg_signed_buffer(struct signature_check *sigc,\n \t\t\t\t    struct gpg_format *fmt, const char *payload,\n-\t\t\t\t    size_t payload_size, const char *signature,\n+\t\t\t\t    size_t payload_size,\n+\t\t\t\t    timestamp_t payload_timestamp,\n+\t\t\t\t    struct strbuf *payload_signer,\n+\t\t\t\t    const char *signature,\n \t\t\t\t    size_t signature_size)\n {\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n@@ -425,7 +437,10 @@ static void parse_ssh_output(struct signature_check *sigc)\n \n static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \t\t\t\t    struct gpg_format *fmt, const char *payload,\n-\t\t\t\t    size_t payload_size, const char *signature,\n+\t\t\t\t    size_t payload_size,\n+\t\t\t\t    timestamp_t payload_timestamp,\n+\t\t\t\t    struct strbuf *payload_signer,\n+\t\t\t\t    const char *signature,\n \t\t\t\t    size_t signature_size)\n {\n \tstruct child_process ssh_keygen = CHILD_PROCESS_INIT;\n@@ -560,8 +575,10 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \treturn ret;\n }\n \n-int check_signature(const char *payload, size_t plen, const char *signature,\n-\tsize_t slen, struct signature_check *sigc)\n+int check_signature(const char *payload, size_t plen,\n+\t\t    timestamp_t payload_timestamp,\n+\t\t    struct strbuf *payload_signer, const char *signature,\n+\t\t    size_t slen, struct signature_check *sigc)\n {\n \tstruct gpg_format *fmt;\n \tint status;\n@@ -573,8 +590,9 @@ int check_signature(const char *payload, size_t plen, const char *signature,\n \tif (!fmt)\n \t\tdie(_(\"bad/incompatible signature '%s'\"), signature);\n \n-\tstatus = fmt->verify_signed_buffer(sigc, fmt, payload, plen, signature,\n-\t\t\t\t\t   slen);\n+\tstatus = fmt->verify_signed_buffer(sigc, fmt, payload, plen,\n+\t\t\t\t\t   payload_timestamp, payload_signer,\n+\t\t\t\t\t   signature, slen);\n \n \tif (status && !sigc->output)\n \t\treturn !!status;\ndiff --git a/gpg-interface.h b/gpg-interface.h\nindex beefacbb1e..f7c5389c90 100644\n--- a/gpg-interface.h\n+++ b/gpg-interface.h\n@@ -71,8 +71,9 @@ const char *get_signing_key(void);\n  */\n const char *get_signing_key_id(void);\n int check_signature(const char *payload, size_t plen,\n-\t\t    const char *signature, size_t slen,\n-\t\t    struct signature_check *sigc);\n+\t\t    timestamp_t payload_timestamp,\n+\t\t    struct strbuf *payload_signer, const char *signature,\n+\t\t    size_t slen, struct signature_check *sigc);\n void print_signature_buffer(const struct signature_check *sigc,\n \t\t\t    unsigned flags);\n \ndiff --git a/log-tree.c b/log-tree.c\nindex 644893fd8c..3c3aec5c40 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -513,7 +513,7 @@ static void show_signature(struct rev_info *opt, struct commit *commit)\n \tif (parse_signed_commit(commit, &payload, &signature, the_hash_algo) <= 0)\n \t\tgoto out;\n \n-\tstatus = check_signature(payload.buf, payload.len, signature.buf,\n+\tstatus = check_signature(payload.buf, payload.len, 0, NULL, signature.buf,\n \t\t\t\t signature.len, &sigc);\n \tif (status && !sigc.output)\n \t\tshow_sig_lines(opt, status, \"No signature\\n\");\n@@ -583,7 +583,7 @@ static int show_one_mergetag(struct commit *commit,\n \tstatus = -1;\n \tif (parse_signature(extra->value, extra->len, &payload, &signature)) {\n \t\t/* could have a good signature */\n-\t\tstatus = check_signature(payload.buf, payload.len,\n+\t\tstatus = check_signature(payload.buf, payload.len, 0, NULL,\n \t\t\t\t\t signature.buf, signature.len, &sigc);\n \t\tif (sigc.output)\n \t\t\tstrbuf_addstr(&verify_message, sigc.output);\ndiff --git a/tag.c b/tag.c\nindex 3e18a41841..3459a0867c 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -25,7 +25,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n \t\treturn error(\"no signature found\");\n \t}\n \n-\tret = check_signature(payload.buf, payload.len, signature.buf,\n+\tret = check_signature(payload.buf, payload.len, 0, NULL, signature.buf,\n \t\t\t\tsignature.len, &sigc);\n \n \tif (!(flags & GPG_VERIFY_OMIT_STATUS))\n-- \n2.31.1\n\n"},{"id":"439383","messageId":"20211022150949.1754477-4-fs@gigacodes.de","threadId":"56763","inReplyTo":"20211022150949.1754477-1-fs@gigacodes.de","subject":"[PATCH 3/6] ssh signing: verify-commit/check_signature with commit date","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-10-22T15:09:46Z","receivedAt":"2021-10-22T15:10:11Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"If valid-before/after dates are configured for this signatures key in the\nallowedSigners file then the verification will check if the key was valid at\nthe time the commit was made. This allows for graceful key rollover and\nrevoking keys without invalidating all previous commits.\nThis feature needs openssh > 8.8. Older ssh-keygen versions will simply\nignore this flag and use the current time.\nStrictly speaking this feature is available in 8.7, but since 8.7 has a\nbug that makes it unusable in another needed call we require 8.8.\n\nTimestamp information is present on most invocations of check_signature.\nHowever signer ident is not. We will need the signer email / name to be able\nto implement \"Trust on first use\" functionality.\nTherefore we provide parse_signed_buffer_metadata() for parsing both from\nthe payload buffer.\n\nPasses commit date for verify-commit on to check_signature.\nAdds test for keys being expired, not yet valid, having a commit date outside\nof key validity and within.\n\nSigned-off-by: Fabian Stelzer <fs@gigacodes.de>\n---\n Documentation/config/gpg.txt |  5 +++++\n commit.c                     | 12 ++++++++--\n gpg-interface.c              | 43 ++++++++++++++++++++++++++++++++++++\n gpg-interface.h              |  8 +++++++\n t/t7528-signed-commit-ssh.sh | 42 +++++++++++++++++++++++++++++++++++\n 5 files changed, 108 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt\nindex 4f30c7dbdd..c9be554c73 100644\n--- a/Documentation/config/gpg.txt\n+++ b/Documentation/config/gpg.txt\n@@ -64,6 +64,11 @@ A repository that only allows signed commits can store the file\n in the repository itself using a path relative to the top-level of the working tree.\n This way only committers with an already valid key can add or change keys in the keyring.\n +\n+Since OpensSSH 8.8 this file allows specifying a key lifetime using valid-after &\n+valid-before options. Git will mark signatures as valid if the signing key was\n+valid at the time of the signatures creation. This allows users to change a\n+signing key without invalidating all previously made signatures.\n++\n Using a SSH CA key with the cert-authority option\n (see ssh-keygen(1) \"CERTIFICATES\") is also valid.\n \ndiff --git a/commit.c b/commit.c\nindex 1704d9df0a..e5dfbfcf8e 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1207,17 +1207,25 @@ int check_commit_signature(const struct commit *commit, struct signature_check *\n \tstruct strbuf payload = STRBUF_INIT;\n \tstruct strbuf signature = STRBUF_INIT;\n \tint ret = 1;\n+\tstruct strbuf payload_signer = STRBUF_INIT;\n+\ttimestamp_t payload_timestamp = 0;\n \n \tsigc->result = 'N';\n \n \tif (parse_signed_commit(commit, &payload, &signature, the_hash_algo) <= 0)\n \t\tgoto out;\n-\tret = check_signature(payload.buf, payload.len, 0, NULL, signature.buf,\n-\t\tsignature.len, sigc);\n+\tif (parse_signed_buffer_metadata(payload.buf, \"committer\",\n+\t\t\t\t\t &payload_timestamp, &payload_signer))\n+\t\tgoto out;\n+\n+\tret = check_signature(payload.buf, payload.len, payload_timestamp,\n+\t\t\t      &payload_signer, signature.buf, signature.len,\n+\t\t\t      sigc);\n \n  out:\n \tstrbuf_release(&payload);\n \tstrbuf_release(&signature);\n+\tstrbuf_release(&payload_signer);\n \n \treturn ret;\n }\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 6049f7cbf7..fed5113297 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -453,6 +453,13 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \tstruct strbuf ssh_principals_err = STRBUF_INIT;\n \tstruct strbuf ssh_keygen_out = STRBUF_INIT;\n \tstruct strbuf ssh_keygen_err = STRBUF_INIT;\n+\tstruct strbuf verify_time = STRBUF_INIT;\n+\tconst struct date_mode verify_date_mode = {\n+\t\t.type = DATE_STRFTIME,\n+\t\t.strftime_fmt = \"%Y%m%d%H%M%S\",\n+\t\t/* SSH signing key validity has no timezone information - Use the local timezone */\n+\t\t.local = 1,\n+\t};\n \n \tif (!ssh_allowed_signers) {\n \t\terror(_(\"gpg.ssh.allowedSignersFile needs to be configured and exist for ssh signature verification\"));\n@@ -470,11 +477,17 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \t\treturn -1;\n \t}\n \n+\tif (payload_timestamp) {\n+\t\tstrbuf_addf(&verify_time, \"-Overify-time=%s\",\n+\t\t\t    show_date(payload_timestamp, 0, &verify_date_mode));\n+\t}\n+\n \t/* Find the principal from the signers */\n \tstrvec_pushl(&ssh_keygen.args, fmt->program,\n \t\t     \"-Y\", \"find-principals\",\n \t\t     \"-f\", ssh_allowed_signers,\n \t\t     \"-s\", buffer_file->filename.buf,\n+\t\t     verify_time.buf,\n \t\t     NULL);\n \tret = pipe_command(&ssh_keygen, NULL, 0, &ssh_principals_out, 0,\n \t\t\t   &ssh_principals_err, 0);\n@@ -482,6 +495,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \t\terror(_(\"ssh-keygen -Y find-principals/verify is needed for ssh signature verification (available in openssh version 8.2p1+)\"));\n \t\tgoto out;\n \t}\n+\n \tif (ret || !ssh_principals_out.len) {\n \t\t/*\n \t\t * We did not find a matching principal in the allowedSigners\n@@ -492,6 +506,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \t\t\t     \"-Y\", \"check-novalidate\",\n \t\t\t     \"-n\", \"git\",\n \t\t\t     \"-s\", buffer_file->filename.buf,\n+\t\t\t     verify_time.buf,\n \t\t\t     NULL);\n \t\tpipe_command(&ssh_keygen, payload, payload_size,\n \t\t\t\t   &ssh_keygen_out, 0, &ssh_keygen_err, 0);\n@@ -526,6 +541,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \t\t\t\t     \"-f\", ssh_allowed_signers,\n \t\t\t\t     \"-I\", principal,\n \t\t\t\t     \"-s\", buffer_file->filename.buf,\n+\t\t\t\t     verify_time.buf,\n \t\t\t\t     NULL);\n \n \t\t\tif (ssh_revocation_file) {\n@@ -571,10 +587,37 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n \tstrbuf_release(&ssh_principals_err);\n \tstrbuf_release(&ssh_keygen_out);\n \tstrbuf_release(&ssh_keygen_err);\n+\tstrbuf_release(&verify_time);\n \n \treturn ret;\n }\n \n+int parse_signed_buffer_metadata(const char *payload, const char *signer_header,\n+\t\t\t\t timestamp_t *payload_timestamp,\n+\t\t\t\t struct strbuf *payload_signer)\n+{\n+\tconst char *ident_line = NULL;\n+\tsize_t ident_len;\n+\tstruct ident_split ident;\n+\n+\tident_line = find_commit_header(payload, signer_header, &ident_len);\n+\tif (ident_line && ident_len) {\n+\t\tif (!split_ident_line(&ident, ident_line, ident_len)) {\n+\t\t\tif (payload_timestamp && ident.date_begin &&\n+\t\t\t    ident.date_end)\n+\t\t\t\t*payload_timestamp = parse_timestamp(\n+\t\t\t\t\tident.date_begin, NULL, 10);\n+\t\t\tif (payload_signer)\n+\t\t\t\tstrbuf_add(payload_signer, ident.mail_begin,\n+\t\t\t\t\t(ident.mail_end - ident.mail_begin));\n+\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n+\treturn 1;\n+}\n+\n int check_signature(const char *payload, size_t plen,\n \t\t    timestamp_t payload_timestamp,\n \t\t    struct strbuf *payload_signer, const char *signature,\ndiff --git a/gpg-interface.h b/gpg-interface.h\nindex f7c5389c90..1cbecbf4c1 100644\n--- a/gpg-interface.h\n+++ b/gpg-interface.h\n@@ -65,6 +65,14 @@ int git_gpg_config(const char *, const char *, void *);\n void set_signing_key(const char *);\n const char *get_signing_key(void);\n \n+/*\n+ * Tries to find \"signer_header\" (usually committer or tagger) in the payload,\n+ * and sets the corresponding timestamp and signer parameters if found.\n+ */\n+int parse_signed_buffer_metadata(const char *payload, const char *signer_header,\n+\t\t\t\t timestamp_t *payload_timestamp,\n+\t\t\t\t struct strbuf *payload_signer);\n+\n /*\n  * Returns a textual unique representation of the signing key in use\n  * Either a GPG KeyID or a SSH Key Fingerprint\ndiff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh\nindex badf3ed320..dae76ded0c 100755\n--- a/t/t7528-signed-commit-ssh.sh\n+++ b/t/t7528-signed-commit-ssh.sh\n@@ -76,6 +76,23 @@ test_expect_success GPGSSH 'create signed commits' '\n \tgit tag twelfth-signed-alt $(cat oid)\n '\n \n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'create signed commits with keys having defined lifetimes' '\n+\ttest_when_finished \"test_unconfig commit.gpgsign\" &&\n+\ttest_config gpg.format ssh &&\n+\n+\techo expired >file && test_tick && git commit -a -m expired -S\"${GPGSSH_KEY_EXPIRED}\" &&\n+\tgit tag expired-signed &&\n+\n+\techo notyetvalid >file && test_tick && git commit -a -m notyetvalid -S\"${GPGSSH_KEY_NOTYETVALID}\" &&\n+\tgit tag notyetvalid-signed &&\n+\n+\techo timeboxedvalid >file && test_tick && git commit -a -m timeboxedvalid -S\"${GPGSSH_KEY_TIMEBOXEDVALID}\" &&\n+\tgit tag timeboxedvalid-signed &&\n+\n+\techo timeboxedinvalid >file && test_tick && git commit -a -m timeboxedinvalid -S\"${GPGSSH_KEY_TIMEBOXEDINVALID}\" &&\n+\tgit tag timeboxedinvalid-signed\n+'\n+\n test_expect_success GPGSSH 'verify and show signatures' '\n \ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n \ttest_config gpg.mintrustlevel UNDEFINED &&\n@@ -122,6 +139,31 @@ test_expect_success GPGSSH 'verify-commit exits failure on untrusted signature'\n \tgrep \"${GPGSSH_KEY_NOT_TRUSTED}\" actual\n '\n \n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-commit exits failure on expired signature key' '\n+\ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\ttest_must_fail git verify-commit expired-signed 2>actual &&\n+\t! grep \"${GPGSSH_GOOD_SIGNATURE_TRUSTED}\" actual\n+'\n+\n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-commit exits failure on not yet valid signature key' '\n+\ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\ttest_must_fail git verify-commit notyetvalid-signed 2>actual &&\n+\t! grep \"${GPGSSH_GOOD_SIGNATURE_TRUSTED}\" actual\n+'\n+\n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-commit succeeds with commit date and key validity matching' '\n+\ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\tgit verify-commit timeboxedvalid-signed 2>actual &&\n+\tgrep \"${GPGSSH_GOOD_SIGNATURE_TRUSTED}\" actual &&\n+\t! grep \"${GPGSSH_BAD_SIGNATURE}\" actual\n+'\n+\n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-commit exits failure with commit date outside of key validity' '\n+\ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\ttest_must_fail git verify-commit timeboxedinvalid-signed 2>actual &&\n+\t! grep \"${GPGSSH_GOOD_SIGNATURE_TRUSTED}\" actual\n+'\n+\n test_expect_success GPGSSH 'verify-commit exits success with matching minTrustLevel' '\n \ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n \ttest_config gpg.minTrustLevel fully &&\n-- \n2.31.1\n\n"},{"id":"439384","messageId":"20211022150949.1754477-5-fs@gigacodes.de","threadId":"56763","inReplyTo":"20211022150949.1754477-1-fs@gigacodes.de","subject":"[PATCH 4/6] ssh signing: git log/check_signature with commit date","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-10-22T15:09:47Z","receivedAt":"2021-10-22T15:10:15Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"Pass the commit date and ident to check_signature when calling git log.\nImplements the same tests as for verify-commit.\n\nSigned-off-by: Fabian Stelzer <fs@gigacodes.de>\n---\n log-tree.c     | 24 +++++++++++++++++++++---\n t/t4202-log.sh | 43 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 64 insertions(+), 3 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 3c3aec5c40..2b29874265 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -509,12 +509,19 @@ static void show_signature(struct rev_info *opt, struct commit *commit)\n \tstruct strbuf signature = STRBUF_INIT;\n \tstruct signature_check sigc = { 0 };\n \tint status;\n+\tstruct strbuf payload_signer = STRBUF_INIT;\n+\ttimestamp_t payload_timestamp = 0;\n \n \tif (parse_signed_commit(commit, &payload, &signature, the_hash_algo) <= 0)\n \t\tgoto out;\n \n-\tstatus = check_signature(payload.buf, payload.len, 0, NULL, signature.buf,\n-\t\t\t\t signature.len, &sigc);\n+\tif (parse_signed_buffer_metadata(payload.buf, \"committer\",\n+\t\t\t\t\t &payload_timestamp, &payload_signer))\n+\t\tgoto out;\n+\n+\tstatus = check_signature(payload.buf, payload.len, payload_timestamp,\n+\t\t\t\t &payload_signer, signature.buf, signature.len,\n+\t\t\t\t &sigc);\n \tif (status && !sigc.output)\n \t\tshow_sig_lines(opt, status, \"No signature\\n\");\n \telse\n@@ -524,6 +531,7 @@ static void show_signature(struct rev_info *opt, struct commit *commit)\n  out:\n \tstrbuf_release(&payload);\n \tstrbuf_release(&signature);\n+\tstrbuf_release(&payload_signer);\n }\n \n static int which_parent(const struct object_id *oid, const struct commit *commit)\n@@ -558,6 +566,8 @@ static int show_one_mergetag(struct commit *commit,\n \tint status, nth;\n \tstruct strbuf payload = STRBUF_INIT;\n \tstruct strbuf signature = STRBUF_INIT;\n+\tstruct strbuf payload_signer = STRBUF_INIT;\n+\ttimestamp_t payload_timestamp = 0;\n \n \thash_object_file(the_hash_algo, extra->value, extra->len,\n \t\t\t type_name(OBJ_TAG), &oid);\n@@ -582,8 +592,15 @@ static int show_one_mergetag(struct commit *commit,\n \n \tstatus = -1;\n \tif (parse_signature(extra->value, extra->len, &payload, &signature)) {\n+\t\tif (parse_signed_buffer_metadata(payload.buf, \"tagger\",\n+\t\t\t\t\t\t &payload_timestamp,\n+\t\t\t\t\t\t &payload_signer))\n+\t\t\tstrbuf_addstr(&verify_message,\n+\t\t\t\t_(\"failed to parse timestamp and signer info from payload\"));\n+\n \t\t/* could have a good signature */\n-\t\tstatus = check_signature(payload.buf, payload.len, 0, NULL,\n+\t\tstatus = check_signature(payload.buf, payload.len,\n+\t\t\t\t\t payload_timestamp, &payload_signer,\n \t\t\t\t\t signature.buf, signature.len, &sigc);\n \t\tif (sigc.output)\n \t\t\tstrbuf_addstr(&verify_message, sigc.output);\n@@ -597,6 +614,7 @@ static int show_one_mergetag(struct commit *commit,\n \tstrbuf_release(&verify_message);\n \tstrbuf_release(&payload);\n \tstrbuf_release(&signature);\n+\tstrbuf_release(&payload_signer);\n \treturn 0;\n }\n \ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 6a650dacd6..2b12baab77 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1626,6 +1626,24 @@ test_expect_success GPGSSH 'setup sshkey signed branch' '\n \tgit commit -S -m signed_commit\n '\n \n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'create signed commits with keys having defined lifetimes' '\n+\ttest_config gpg.format ssh &&\n+\ttouch file &&\n+\tgit add file &&\n+\n+\techo expired >file && test_tick && git commit -a -m expired -S\"${GPGSSH_KEY_EXPIRED}\" &&\n+\tgit tag expired-signed &&\n+\n+\techo notyetvalid >file && test_tick && git commit -a -m notyetvalid -S\"${GPGSSH_KEY_NOTYETVALID}\" &&\n+\tgit tag notyetvalid-signed &&\n+\n+\techo timeboxedvalid >file && test_tick && git commit -a -m timeboxedvalid -S\"${GPGSSH_KEY_TIMEBOXEDVALID}\" &&\n+\tgit tag timeboxedvalid-signed &&\n+\n+\techo timeboxedinvalid >file && test_tick && git commit -a -m timeboxedinvalid -S\"${GPGSSH_KEY_TIMEBOXEDINVALID}\" &&\n+\tgit tag timeboxedinvalid-signed\n+'\n+\n test_expect_success GPGSM 'log x509 fingerprint' '\n \techo \"F8BF62E0693D0694816377099909C779FA23FD65 | \" >expect &&\n \tgit log -n1 --format=\"%GF | %GP\" signed-x509 >actual &&\n@@ -1663,6 +1681,31 @@ test_expect_success GPGSSH 'log --graph --show-signature ssh' '\n \tgrep \"${GOOD_SIGNATURE_TRUSTED}\" actual\n '\n \n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'log shows failure on expired signature key' '\n+\ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\tgit log --graph --show-signature -n1 expired-signed >actual &&\n+\t! grep \"${GPGSSH_GOOD_SIGNATURE_TRUSTED}\" actual\n+'\n+\n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'log shows failure on not yet valid signature key' '\n+\ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\tgit log --graph --show-signature -n1 notyetvalid-signed >actual &&\n+\t! grep \"${GPGSSH_GOOD_SIGNATURE_TRUSTED}\" actual\n+'\n+\n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'log show success with commit date and key validity matching' '\n+\ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\tgit log --graph --show-signature -n1 timeboxedvalid-signed >actual &&\n+\tgrep \"${GPGSSH_GOOD_SIGNATURE_TRUSTED}\" actual &&\n+\t! grep \"${GPGSSH_BAD_SIGNATURE}\" actual\n+'\n+\n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'log shows failure with commit date outside of key validity' '\n+\ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\tgit log --graph --show-signature -n1 timeboxedinvalid-signed >actual &&\n+\t! grep \"${GPGSSH_GOOD_SIGNATURE_TRUSTED}\" actual\n+'\n+\n test_expect_success GPG 'log --graph --show-signature for merged tag' '\n \ttest_when_finished \"git reset --hard && git checkout main\" &&\n \tgit checkout -b plain main &&\n-- \n2.31.1\n\n"},{"id":"439385","messageId":"20211022150949.1754477-6-fs@gigacodes.de","threadId":"56763","inReplyTo":"20211022150949.1754477-1-fs@gigacodes.de","subject":"[PATCH 5/6] ssh signing: verify-tag/check_signature with tag date","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-10-22T15:09:48Z","receivedAt":"2021-10-22T15:10:17Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"Passes tag date and tagger ident to to check_signature.\nImplements the same tests as for verify-commit.\n\nSigned-off-by: Fabian Stelzer <fs@gigacodes.de>\n---\n t/t7031-verify-tag-signed-ssh.sh | 42 ++++++++++++++++++++++++++++++++\n tag.c                            | 12 +++++++--\n 2 files changed, 52 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7031-verify-tag-signed-ssh.sh b/t/t7031-verify-tag-signed-ssh.sh\nindex 06c9dd6c93..1cb36b9ab8 100755\n--- a/t/t7031-verify-tag-signed-ssh.sh\n+++ b/t/t7031-verify-tag-signed-ssh.sh\n@@ -48,6 +48,23 @@ test_expect_success GPGSSH 'create signed tags ssh' '\n \tgit tag -u\"${GPGSSH_KEY_UNTRUSTED}\" -m eighth eighth-signed-alt\n '\n \n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'create signed tags with keys having defined lifetimes' '\n+\ttest_when_finished \"test_unconfig commit.gpgsign\" &&\n+\ttest_config gpg.format ssh &&\n+\n+\techo expired >file && test_tick && git commit -a -m expired -S\"${GPGSSH_KEY_EXPIRED}\" &&\n+\tgit tag -s -u \"${GPGSSH_KEY_EXPIRED}\" -m expired-signed expired-signed &&\n+\n+\techo notyetvalid >file && test_tick && git commit -a -m notyetvalid -S\"${GPGSSH_KEY_NOTYETVALID}\" &&\n+\tgit tag -s -u \"${GPGSSH_KEY_NOTYETVALID}\" -m notyetvalid-signed notyetvalid-signed &&\n+\n+\techo timeboxedvalid >file && test_tick && git commit -a -m timeboxedvalid -S\"${GPGSSH_KEY_TIMEBOXEDVALID}\" &&\n+\tgit tag -s -u \"${GPGSSH_KEY_TIMEBOXEDVALID}\" -m timeboxedvalid-signed timeboxedvalid-signed &&\n+\n+\techo timeboxedinvalid >file && test_tick && git commit -a -m timeboxedinvalid -S\"${GPGSSH_KEY_TIMEBOXEDINVALID}\" &&\n+\tgit tag -s -u \"${GPGSSH_KEY_TIMEBOXEDINVALID}\" -m timeboxedinvalid-signed timeboxedinvalid-signed\n+'\n+\n test_expect_success GPGSSH 'verify and show ssh signatures' '\n \ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n \t(\n@@ -80,6 +97,31 @@ test_expect_success GPGSSH 'verify and show ssh signatures' '\n \t)\n '\n \n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-tag exits failure on expired signature key' '\n+\ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\ttest_must_fail git verify-tag expired-signed 2>actual &&\n+\t! grep \"${GPGSSH_GOOD_SIGNATURE_TRUSTED}\" actual\n+'\n+\n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-tag exits failure on not yet valid signature key' '\n+\ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\ttest_must_fail git verify-tag notyetvalid-signed 2>actual &&\n+\t! grep \"${GPGSSH_GOOD_SIGNATURE_TRUSTED}\" actual\n+'\n+\n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-tag succeeds with tag date and key validity matching' '\n+\ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\tgit verify-tag timeboxedvalid-signed 2>actual &&\n+\tgrep \"${GPGSSH_GOOD_SIGNATURE_TRUSTED}\" actual &&\n+\t! grep \"${GPGSSH_BAD_SIGNATURE}\" actual\n+'\n+\n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'verify-tag failes with tag date outside of key validity' '\n+\ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\ttest_must_fail git verify-tag timeboxedinvalid-signed 2>actual &&\n+\t! grep \"${GPGSSH_GOOD_SIGNATURE_TRUSTED}\" actual\n+'\n+\n test_expect_success GPGSSH 'detect fudged ssh signature' '\n \ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n \tgit cat-file tag seventh-signed >raw &&\ndiff --git a/tag.c b/tag.c\nindex 3459a0867c..d26053ff4d 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -16,6 +16,8 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n \tstruct strbuf payload = STRBUF_INIT;\n \tstruct strbuf signature = STRBUF_INIT;\n \tint ret;\n+\tstruct strbuf payload_signer = STRBUF_INIT;\n+\ttimestamp_t payload_timestamp = 0;\n \n \tmemset(&sigc, 0, sizeof(sigc));\n \n@@ -25,8 +27,13 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n \t\treturn error(\"no signature found\");\n \t}\n \n-\tret = check_signature(payload.buf, payload.len, 0, NULL, signature.buf,\n-\t\t\t\tsignature.len, &sigc);\n+\tif (parse_signed_buffer_metadata(payload.buf, \"tagger\",\n+\t\t\t\t\t &payload_timestamp, &payload_signer))\n+\t\treturn error(_(\"failed to parse timestamp and signer info from payload\"));\n+\n+\tret = check_signature(payload.buf, payload.len, payload_timestamp,\n+\t\t\t      &payload_signer, signature.buf, signature.len,\n+\t\t\t      &sigc);\n \n \tif (!(flags & GPG_VERIFY_OMIT_STATUS))\n \t\tprint_signature_buffer(&sigc, flags);\n@@ -34,6 +41,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)\n \tsignature_check_clear(&sigc);\n \tstrbuf_release(&payload);\n \tstrbuf_release(&signature);\n+\tstrbuf_release(&payload_signer);\n \treturn ret;\n }\n \n-- \n2.31.1\n\n"},{"id":"439386","messageId":"20211022150949.1754477-7-fs@gigacodes.de","threadId":"56763","inReplyTo":"20211022150949.1754477-1-fs@gigacodes.de","subject":"[PATCH 6/6] ssh signing: fmt-merge-msg/check_signature with tag date","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-10-22T15:09:49Z","receivedAt":"2021-10-22T15:10:21Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"Pass the tag date and tagger ident to check_signature when generating merge\nmessages to verify merged tags signatures.\nImplements the same tests as for verify-commit.\n\nSigned-off-by: Fabian Stelzer <fs@gigacodes.de>\n---\n fmt-merge-msg.c          | 13 +++++++++-\n t/t6200-fmt-merge-msg.sh | 54 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 66 insertions(+), 1 deletion(-)\n\ndiff --git a/fmt-merge-msg.c b/fmt-merge-msg.c\nindex d2cedad6b7..4f49dd9341 100644\n--- a/fmt-merge-msg.c\n+++ b/fmt-merge-msg.c\n@@ -524,6 +524,8 @@ static void fmt_merge_msg_sigs(struct strbuf *out)\n \t\tunsigned long len = size;\n \t\tstruct signature_check sigc = { NULL };\n \t\tstruct strbuf payload = STRBUF_INIT, sig = STRBUF_INIT;\n+\t\tstruct strbuf payload_signer = STRBUF_INIT;\n+\t\ttimestamp_t payload_timestamp = 0;\n \n \t\tif (!buf || type != OBJ_TAG)\n \t\t\tgoto next;\n@@ -533,7 +535,15 @@ static void fmt_merge_msg_sigs(struct strbuf *out)\n \t\telse {\n \t\t\tbuf = payload.buf;\n \t\t\tlen = payload.len;\n-\t\t\tif (check_signature(payload.buf, payload.len, 0, NULL,\n+\n+\t\t\tif (parse_signed_buffer_metadata(payload.buf, \"tagger\",\n+\t\t\t\t\t\t\t &payload_timestamp,\n+\t\t\t\t\t\t\t &payload_signer))\n+\t\t\t\tstrbuf_addstr(&sig,\n+\t\t\t\t\t_(\"failed to parse timestamp and signer info from payload\"));\n+\n+\t\t\tif (check_signature(payload.buf, payload.len,\n+\t\t\t\t\t    payload_timestamp, &payload_signer,\n \t\t\t\t\t    sig.buf, sig.len, &sigc) &&\n \t\t\t    !sigc.output)\n \t\t\t\tstrbuf_addstr(&sig, \"gpg verification failed.\\n\");\n@@ -564,6 +574,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)\n \t\t}\n \t\tstrbuf_release(&payload);\n \t\tstrbuf_release(&sig);\n+\t\tstrbuf_release(&payload_signer);\n \tnext:\n \t\tfree(origbuf);\n \t}\ndiff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh\nindex 06c5fb5615..2dd2423643 100755\n--- a/t/t6200-fmt-merge-msg.sh\n+++ b/t/t6200-fmt-merge-msg.sh\n@@ -91,6 +91,26 @@ test_expect_success GPGSSH 'created ssh signed commit and tag' '\n \tgit tag -s -u\"${GPGSSH_KEY_UNTRUSTED}\" -m signed-ssh-tag-msg-untrusted signed-untrusted-ssh-tag left\n '\n \n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'create signed tags with keys having defined lifetimes' '\n+\ttest_when_finished \"test_unconfig commit.gpgsign\" &&\n+\ttest_config gpg.format ssh &&\n+\tgit checkout -b signed-expiry-ssh &&\n+\ttouch file &&\n+\tgit add file &&\n+\n+\techo expired >file && test_tick && git commit -a -m expired -S\"${GPGSSH_KEY_EXPIRED}\" &&\n+\tgit tag -s -u \"${GPGSSH_KEY_EXPIRED}\" -m expired-signed expired-signed &&\n+\n+\techo notyetvalid >file && test_tick && git commit -a -m notyetvalid -S\"${GPGSSH_KEY_NOTYETVALID}\" &&\n+\tgit tag -s -u \"${GPGSSH_KEY_NOTYETVALID}\" -m notyetvalid-signed notyetvalid-signed &&\n+\n+\techo timeboxedvalid >file && test_tick && git commit -a -m timeboxedvalid -S\"${GPGSSH_KEY_TIMEBOXEDVALID}\" &&\n+\tgit tag -s -u \"${GPGSSH_KEY_TIMEBOXEDVALID}\" -m timeboxedvalid-signed timeboxedvalid-signed &&\n+\n+\techo timeboxedinvalid >file && test_tick && git commit -a -m timeboxedinvalid -S\"${GPGSSH_KEY_TIMEBOXEDINVALID}\" &&\n+\tgit tag -s -u \"${GPGSSH_KEY_TIMEBOXEDINVALID}\" -m timeboxedinvalid-signed timeboxedinvalid-signed\n+'\n+\n test_expect_success 'message for merging local branch' '\n \techo \"Merge branch ${apos}left${apos}\" >expected &&\n \n@@ -137,6 +157,40 @@ test_expect_success GPGSSH 'message for merging local tag signed by unknown ssh\n \t! grep \"${GPGSSH_BAD_SIGNATURE}\" actual &&\n \tgrep \"${GPGSSH_KEY_NOT_TRUSTED}\" actual\n '\n+\n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'message for merging local tag signed by expired ssh key' '\n+\ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\tgit checkout main &&\n+\tgit fetch . expired-signed &&\n+\tgit fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 &&\n+\t! grep \"${GPGSSH_GOOD_SIGNATURE_TRUSTED}\" actual\n+'\n+\n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'message for merging local tag signed by not yet valid ssh key' '\n+\ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\tgit checkout main &&\n+\tgit fetch . notyetvalid-signed &&\n+\tgit fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 &&\n+\t! grep \"${GPGSSH_GOOD_SIGNATURE_TRUSTED}\" actual\n+'\n+\n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'message for merging local tag signed by valid timeboxed ssh key' '\n+\ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\tgit checkout main &&\n+\tgit fetch . timeboxedvalid-signed &&\n+\tgit fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 &&\n+\tgrep \"${GPGSSH_GOOD_SIGNATURE_TRUSTED}\" actual &&\n+\t! grep \"${GPGSSH_BAD_SIGNATURE}\" actual\n+'\n+\n+test_expect_success GPGSSH,GPGSSH_VERIFYTIME 'message for merging local tag signed by invalid timeboxed ssh key' '\n+\ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\tgit checkout main &&\n+\tgit fetch . timeboxedinvalid-signed &&\n+\tgit fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 &&\n+\t! grep \"${GPGSSH_GOOD_SIGNATURE_TRUSTED}\" actual\n+'\n+\n test_expect_success 'message for merging external branch' '\n \techo \"Merge branch ${apos}left${apos} of $(pwd)\" >expected &&\n \n-- \n2.31.1\n\n"},{"id":"439387","messageId":"20211022150949.1754477-3-fs@gigacodes.de","threadId":"56763","inReplyTo":"20211022150949.1754477-1-fs@gigacodes.de","subject":"[PATCH 2/6] ssh signing: add key lifetime test prereqs","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-10-22T15:09:45Z","receivedAt":"2021-10-22T15:10:33Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"if ssh-keygen supports -Overify-time, add test keys marked as expired,\nnot yet valid and valid both within the test_tick timeframe and outside of it.\n\nSigned-off-by: Fabian Stelzer <fs@gigacodes.de>\n---\n t/lib-gpg.sh | 19 ++++++++++++++++++-\n 1 file changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex f99ef3e859..139e7a45ea 100644\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -90,6 +90,10 @@ test_lazy_prereq RFC1991 '\n GPGSSH_KEY_PRIMARY=\"${GNUPGHOME}/ed25519_ssh_signing_key\"\n GPGSSH_KEY_SECONDARY=\"${GNUPGHOME}/rsa_2048_ssh_signing_key\"\n GPGSSH_KEY_UNTRUSTED=\"${GNUPGHOME}/untrusted_ssh_signing_key\"\n+GPGSSH_KEY_EXPIRED=\"${GNUPGHOME}/expired_ssh_signing_key\"\n+GPGSSH_KEY_NOTYETVALID=\"${GNUPGHOME}/notyetvalid_ssh_signing_key\"\n+GPGSSH_KEY_TIMEBOXEDVALID=\"${GNUPGHOME}/timeboxed_valid_ssh_signing_key\"\n+GPGSSH_KEY_TIMEBOXEDINVALID=\"${GNUPGHOME}/timeboxed_invalid_ssh_signing_key\"\n GPGSSH_KEY_WITH_PASSPHRASE=\"${GNUPGHOME}/protected_ssh_signing_key\"\n GPGSSH_KEY_PASSPHRASE=\"super_secret\"\n GPGSSH_ALLOWED_SIGNERS=\"${GNUPGHOME}/ssh.all_valid.allowedSignersFile\"\n@@ -112,7 +116,20 @@ test_lazy_prereq GPGSSH '\n \techo \"\\\"principal with number 2\\\" $(cat \"${GPGSSH_KEY_SECONDARY}.pub\")\" >> \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n \tssh-keygen -t ed25519 -N \"${GPGSSH_KEY_PASSPHRASE}\" -C \"git ed25519 encrypted key\" -f \"${GPGSSH_KEY_WITH_PASSPHRASE}\" >/dev/null &&\n \techo \"\\\"principal with number 3\\\" $(cat \"${GPGSSH_KEY_WITH_PASSPHRASE}.pub\")\" >> \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n-\tssh-keygen -t ed25519 -N \"\" -f \"${GPGSSH_KEY_UNTRUSTED}\" >/dev/null\n+\tssh-keygen -t ed25519 -N \"\" -C \"git ed25519 key\" -f \"${GPGSSH_KEY_UNTRUSTED}\" >/dev/null\n+'\n+\n+test_lazy_prereq GPGSSH_VERIFYTIME '\n+\t# Check if ssh-keygen has a verify-time option by passing an invalid date to it\n+\tssh-keygen -Overify-time=INVALID -Y check-novalidate -s doesnotmatter 2>&1 | grep -q -F \"Invalid \\\"verify-time\\\"\" &&\n+\tssh-keygen -t ed25519 -N \"\" -C \"timeboxed valid key\" -f \"${GPGSSH_KEY_TIMEBOXEDVALID}\" >/dev/null &&\n+\techo \"\\\"timeboxed valid key\\\" valid-after=\\\"20050407000000\\\",valid-before=\\\"200504100000\\\" $(cat \"${GPGSSH_KEY_TIMEBOXEDVALID}.pub\")\" >> \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\tssh-keygen -t ed25519 -N \"\" -C \"timeboxed invalid key\" -f \"${GPGSSH_KEY_TIMEBOXEDINVALID}\" >/dev/null &&\n+\techo \"\\\"timeboxed invalid key\\\" valid-after=\\\"20050401000000\\\",valid-before=\\\"20050402000000\\\" $(cat \"${GPGSSH_KEY_TIMEBOXEDINVALID}.pub\")\" >> \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\tssh-keygen -t ed25519 -N \"\" -C \"expired key\" -f \"${GPGSSH_KEY_EXPIRED}\" >/dev/null &&\n+\techo \"\\\"principal with expired key\\\" valid-before=\\\"20000101000000\\\" $(cat \"${GPGSSH_KEY_EXPIRED}.pub\")\" >> \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n+\tssh-keygen -t ed25519 -N \"\" -C \"not yet valid key\" -f \"${GPGSSH_KEY_NOTYETVALID}\" >/dev/null &&\n+\techo \"\\\"principal with not yet valid key\\\" valid-after=\\\"29990101000000\\\" $(cat \"${GPGSSH_KEY_NOTYETVALID}.pub\")\" >> \"${GPGSSH_ALLOWED_SIGNERS}\"\n '\n \n sanitize_pgp() {\n-- \n2.31.1\n\n"},{"id":"439395","messageId":"211022.86ee8dj6uv.gmgdl@evledraar.gmail.com","threadId":"56763","inReplyTo":"20211022150949.1754477-4-fs@gigacodes.de","subject":"Re: [PATCH 3/6] ssh signing: verify-commit/check_signature with commit date","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-22T17:37:03Z","receivedAt":"2021-10-22T17:40:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Oct 22 2021, Fabian Stelzer wrote:\n\n[Just nits]\n\n> +\tif (payload_timestamp) {\n> +\t\tstrbuf_addf(&verify_time, \"-Overify-time=%s\",\n> +\t\t\t    show_date(payload_timestamp, 0, &verify_date_mode));\n> +\t}\n\nNo braces needed.\n\n> @@ -482,6 +495,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n>  \t\terror(_(\"ssh-keygen -Y find-principals/verify is needed for ssh signature verification (available in openssh version 8.2p1+)\"));\n>  \t\tgoto out;\n>  \t}\n> +\n>  \tif (ret || !ssh_principals_out.len) {\n>  \t\t/*\n>  \t\t * We did not find a matching principal in the allowedSigners\n\nStray whitespace change.\n\n> +int parse_signed_buffer_metadata(const char *payload, const char *signer_header,\n> +\t\t\t\t timestamp_t *payload_timestamp,\n> +\t\t\t\t struct strbuf *payload_signer)\n> +{\n> +\tconst char *ident_line = NULL;\n> +\tsize_t ident_len;\n> +\tstruct ident_split ident;\n> +\n> +\tident_line = find_commit_header(payload, signer_header, &ident_len);\n> +\tif (ident_line && ident_len) {\n> +\t\tif (!split_ident_line(&ident, ident_line, ident_len)) {\n> +\t\t\tif (payload_timestamp && ident.date_begin &&\n> +\t\t\t    ident.date_end)\n> +\t\t\t\t*payload_timestamp = parse_timestamp(\n> +\t\t\t\t\tident.date_begin, NULL, 10);\n> +\t\t\tif (payload_signer)\n> +\t\t\t\tstrbuf_add(payload_signer, ident.mail_begin,\n> +\t\t\t\t\t(ident.mail_end - ident.mail_begin));\n> +\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t}\n> +\n> +\treturn 1;\n> +}\n\nThis would be more readable with less nesting, i.e. instead of:\n\n    if (x) {\n        if (y) {\n            [...]\n\nDoing:\n\n    if (!x)\n        return 1;\n    if (!y)\n        return 1;\n\nI.e. only if you get zero from split_ident_line() do you return 0.\n\n"},{"id":"439397","messageId":"211022.86a6j0kjoi.gmgdl@evledraar.gmail.com","threadId":"56763","inReplyTo":"20211022150949.1754477-7-fs@gigacodes.de","subject":"Re: [PATCH 6/6] ssh signing: fmt-merge-msg/check_signature with tag date","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-22T18:12:11Z","receivedAt":"2021-10-22T18:18:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Oct 22 2021, Fabian Stelzer wrote:\n\n>  \t\t\tbuf = payload.buf;\n>  \t\t\tlen = payload.len;\n> -\t\t\tif (check_signature(payload.buf, payload.len, 0, NULL,\n> +\n> +\t\t\tif (parse_signed_buffer_metadata(payload.buf, \"tagger\",\n> +\t\t\t\t\t\t\t &payload_timestamp,\n> +\t\t\t\t\t\t\t &payload_signer))\n> +\t\t\t\tstrbuf_addstr(&sig,\n> +\t\t\t\t\t_(\"failed to parse timestamp and signer info from payload\"));\n> +\n> +\t\t\tif (check_signature(payload.buf, payload.len,\n> +\t\t\t\t\t    payload_timestamp, &payload_signer,\n>  \t\t\t\t\t    sig.buf, sig.len, &sigc) &&\n>  \t\t\t    !sigc.output)\n>  \t\t\t\tstrbuf_addstr(&sig, \"gpg verification failed.\\n\");\n\nI haven't tested this, but your addition to &sig here lacks a \\n,\ncompared to the &sig seen right above here in the diff context.\n\nIsn't one or the other either missing a \\n, or shouldn't have one?\n*Looks ath the surrounding code*, yeah if I'm not wrong it's the \\n in\nthe new code here that's missing.\n\nThe whole business of seemingly mixing error messages and a signature\npayload in the same variable seems a bit odd, but maybe I'm misreading\nit. In any case it seems to pre-date this series...\n"},{"id":"439468","messageId":"xmqqfssr8hd5.fsf@gitster.g","threadId":"56763","inReplyTo":"20211022150949.1754477-2-fs@gigacodes.de","subject":"Re: [PATCH 1/6] ssh signing: extend check_signature to accept payload metadata","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-23T23:13:26Z","receivedAt":"2021-10-23T23:13:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer <fs@gigacodes.de> writes:\n\n> Adds two new parameters to the check_signature api and passes them to the\n\n\"Add\". \"pass\".\n\n> internal verification ssh/gpg methods.\n\n> A payload timestamp that will be used to verify signatures at the time of their\n> objects creation if the signing method supports it (only ssh for now).\n\nOK.\n\n> And a signer strbuf containing ident information about the signer that\n> we will need for implementing \"Trust on first use\" in a future patch\n> series.\n\nOK.\n\nIt would flow better in our \"git log\" stream if you explain upfront\nwhat problem you are trying to solve, before starting to give orders\nto the codebase to pass these pieces of information.  Something\nlike:\n\n    In order to implement the \"trust on the first use of a key\"\n    behaviour in later steps, allow callers to optionally pass the\n    timestamp of the payload and the identity of the signer to\n    check_signature().\n\nIt is unclear what \"payload timestamp\" is without actually seeing\nhow it is used, so if you cannot explain it in easy terms in the log\nmessage for this step, it may be an indication that it is not a such\ngood idea to add these parameters in a separate step.\n\n>  int check_signature(const char *payload, size_t plen,\n> -\t\t    const char *signature, size_t slen,\n> -\t\t    struct signature_check *sigc);\n> +\t\t    timestamp_t payload_timestamp,\n> +\t\t    struct strbuf *payload_signer, const char *signature,\n> +\t\t    size_t slen, struct signature_check *sigc);\n\nFunny line wrapping.  Just like payload and plen form a pair (hence\nthey sit next to each other on the same line), signature and slen\nshould sit next to each other on the same line.\n\nWhat's the reason why payload-signer MUST come in a strbuf?  A\ncaller that has only ptr and len must invent a new strbuf that is\notherwise unused to call this function because of that calling\nconvention, which looks suboptimal.\n\nIf the function accepts <ptr, len>, just like <payload, plen> or\n<signature, slen> are taken by the function, such a caller can just\ncall the function without having to have an extra instance of\nstrbuf, while a caller that happens to already have a strbuf can\npass <buf.buf, buf.len> as two separate parameters.\n\nWhat's the driving principle that decided where in the list of\nparameters these two new ones are added?\n\nI would explain one possible order I may find \"logical\" using the\nlevel of detail I expect from an answer to \"what's the guiding\nprinciple?\" as an example here:\n\n - we should move 'sigc' to the beginning, because the convention\n   used by many of our API functions that have some central\n   structure to work with is to have such a structure as the first\n   thing in the list of parameters;\n\n - we should then append 'payload_timestamp', 'payload_signer', and\n   'payload_signer_len' at the end, as the function is about\n   \"validate <signature, slen> is valid for <payload, plen> and\n   leave the result in <sigc>\", and anything we add is auxiliary\n   input to the process, which are of less significance than the\n   existing ones.\n\nAnother possible direction to go might be to add these auxiliary\ninput to the process to the signature_check structure.  Then this\nstep can disappear, or just flip the order of the parameter to move\nsigc to the beginning, and then the later stemp that does the \"first\nuse\" thing can add the necessary members to the structure *and* use\nthe members to do its thing, which helps readers better understand\nwhat is going on.\n\nOne possible downside is that sigc has been mostly output-only\nstructure, and turning it into a structure that also has some input\nmembers might make it too confusing.  I dunno.\n\nThanks.\n"},{"id":"439516","messageId":"02fff111-865d-a2a0-ba06-d0166edfb9dd@gigacodes.de","threadId":"56763","inReplyTo":"xmqqfssr8hd5.fsf@gitster.g","subject":"Re: [PATCH 1/6] ssh signing: extend check_signature to accept payload metadata","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-10-25T08:28:58Z","receivedAt":"2021-10-25T08:29:07Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 24.10.21 01:13, Junio C Hamano wrote:\n> Fabian Stelzer <fs@gigacodes.de> writes:\n> \n> It would flow better in our \"git log\" stream if you explain upfront\n> what problem you are trying to solve, before starting to give orders\n> to the codebase to pass these pieces of information.  Something\n> like:\n> \n>     In order to implement the \"trust on the first use of a key\"\n>     behaviour in later steps, allow callers to optionally pass the\n>     timestamp of the payload and the identity of the signer to\n>     check_signature().\n\nThanks, you're right. I will update the commit message.\n\n> \n> It is unclear what \"payload timestamp\" is without actually seeing\n> how it is used, so if you cannot explain it in easy terms in the log\n> message for this step, it may be an indication that it is not a such\n> good idea to add these parameters in a separate step.\n> \n>>  int check_signature(const char *payload, size_t plen,\n>> -\t\t    const char *signature, size_t slen,\n>> -\t\t    struct signature_check *sigc);\n>> +\t\t    timestamp_t payload_timestamp,\n>> +\t\t    struct strbuf *payload_signer, const char *signature,\n>> +\t\t    size_t slen, struct signature_check *sigc);\n> \n> Funny line wrapping.  Just like payload and plen form a pair (hence\n> they sit next to each other on the same line), signature and slen\n> should sit next to each other on the same line.\n\nclang-format enforced the 80 char limit here i think :/\ni think we will change this function anyway (see below) and i will keep\nit in mind then.\n\n> \n> What's the reason why payload-signer MUST come in a strbuf?  A\n> caller that has only ptr and len must invent a new strbuf that is\n> otherwise unused to call this function because of that calling\n> convention, which looks suboptimal.\n\nIt does not necessarily need to be. I just like the convenience of the\nstrbuf_ functions for working with strings. Also i think it makes it\nvery specific and easy to grasp in the calling function to STRBUF_INIT\nand then release it at the end. When working with char* it can often be\nnot immediately clear (at least not t ome) if i need to provide an\nexisting buffer or just a pointer and if need to free what was returned.\nchar*/const char* usually indicates this, but this is not always the\ncase. Maybe my \"C foo\" is just not strong enough and i like the higher\nlevel api :D\nIf my change proposed below is acceptable this problem solves itself as\nwell.\n\n> \n> \n> What's the driving principle that decided where in the list of\n> parameters these two new ones are added?\n> \n> I would explain one possible order I may find \"logical\" using the\n> level of detail I expect from an answer to \"what's the guiding\n> principle?\" as an example here:\n> \n>  - we should move 'sigc' to the beginning, because the convention\n>    used by many of our API functions that have some central\n>    structure to work with is to have such a structure as the first\n>    thing in the list of parameters;\n> \n>  - we should then append 'payload_timestamp', 'payload_signer', and\n>    'payload_signer_len' at the end, as the function is about\n>    \"validate <signature, slen> is valid for <payload, plen> and\n>    leave the result in <sigc>\", and anything we add is auxiliary\n>    input to the process, which are of less significance than the\n>    existing ones.\n> \n> Another possible direction to go might be to add these auxiliary\n> input to the process to the signature_check structure.  Then this\n> step can disappear, or just flip the order of the parameter to move\n> sigc to the beginning, and then the later stemp that does the \"first\n> use\" thing can add the necessary members to the structure *and* use\n> the members to do its thing, which helps readers better understand\n> what is going >\n> One possible downside is that sigc has been mostly output-only\n> structure, and turning it into a structure that also has some input\n> members might make it too confusing.  I dunno.\n\nWhen starting this series my plan was indeed to add these parameters to\nthe signature_check struct. As you correctly mentioned it is \"output\nonly\" at the moment. That's why i chose to add the extra parameters. I\nadded them in the middle of the parameter list next to the payload\nitself since i consider them to be part of to the payload. And since\nsigc is output only i would put it at the end (not sure if any\nconvention for things like this exist for git).\n\nTo put it in words:\ncheck_signature uses payload/plen with timestamp & ident to check\nsignature/slen and return the result via sigc.\n\nOr maybe alternatively:\ncheck_signature checks payload/plen to check signature/slen, constrained\nby timestamp & ident and return the result via sigc.\n\nHowever if everyone is ok with changing the struct to be used for input\nas well then i would adjust the function to have it as the first parameter.\n\nThe sigc struct already has a payload member (only used for verbose\noutput) which is populated by the check function with a xmemdupz. I\nwould then change it to a const char, add the length var and use it to\npass the payload into the function as well. This way we also avoid the\nunnecessary mem copy.\n\nThe function would just become:\ncheck_signature(struct signature_check *sigc, const char *signature,\nsize_t slen)\n\n> \n> Thanks.\n> \n\nThanks for your review.\n"},{"id":"439517","messageId":"51099904-a962-eb23-8baf-9ce15fff7d10@gigacodes.de","threadId":"56763","inReplyTo":"211022.86ee8dj6uv.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 3/6] ssh signing: verify-commit/check_signature with commit date","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-10-25T08:31:13Z","receivedAt":"2021-10-25T08:31:18Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 22.10.21 19:37, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, Oct 22 2021, Fabian Stelzer wrote:\n> \n> [Just nits]\n> \n\nThanks for your review. I will fix/adjust all of those in the next\niteration.\n"},{"id":"439518","messageId":"8ff17f0e-9877-1004-4057-4f4af21531a1@gigacodes.de","threadId":"56763","inReplyTo":"211022.86a6j0kjoi.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 6/6] ssh signing: fmt-merge-msg/check_signature with tag date","fromName":"Fabian Stelzer","fromEmail":"fs@gigacodes.de","sentAt":"2021-10-25T08:39:17Z","receivedAt":"2021-10-25T08:39:22Z","isPatch":true,"sender":{"key":"fs@gigacodes.de","avatar":"https://avatars.githubusercontent.com/u/564858?v=4"},"body":"On 22.10.21 20:12, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, Oct 22 2021, Fabian Stelzer wrote:\n> \n>>  \t\t\tbuf = payload.buf;\n>>  \t\t\tlen = payload.len;\n>> -\t\t\tif (check_signature(payload.buf, payload.len, 0, NULL,\n>> +\n>> +\t\t\tif (parse_signed_buffer_metadata(payload.buf, \"tagger\",\n>> +\t\t\t\t\t\t\t &payload_timestamp,\n>> +\t\t\t\t\t\t\t &payload_signer))\n>> +\t\t\t\tstrbuf_addstr(&sig,\n>> +\t\t\t\t\t_(\"failed to parse timestamp and signer info from payload\"));\n>> +\n>> +\t\t\tif (check_signature(payload.buf, payload.len,\n>> +\t\t\t\t\t    payload_timestamp, &payload_signer,\n>>  \t\t\t\t\t    sig.buf, sig.len, &sigc) &&\n>>  \t\t\t    !sigc.output)\n>>  \t\t\t\tstrbuf_addstr(&sig, \"gpg verification failed.\\n\");\n> \n> I haven't tested this, but your addition to &sig here lacks a \\n,\n> compared to the &sig seen right above here in the diff context.\n\nYou are correct. I have added the newline.\n\n> \n> The whole business of seemingly mixing error messages and a signature\n> payload in the same variable seems a bit odd, but maybe I'm misreading\n> it. In any case it seems to pre-date this series...\n> \n\nTrue, it is a bit odd. This function generates the message text when\nmerging a (signed) tag. It verifies the tag upon doing so and includes\nthe result information in the message.\n\nI'm not sure what should happen in this error case. (especially since\nthis is one of those \"this should never happen\" errors). I don't know if\ni can just warn() here or if that would be lost or even corrupt the\nmerge message :/\nIf the buffer parsing fails this could result in ssh signatures being\nmarked as valid even though they expired. So i think it it important to\ninclude this info. But the error has no effect on gpg signatures. So\ntelling users about not being able to check key lifetime here might\npaint a wrong picture.\n\nThanks,\nFabian\n"},{"id":"439568","messageId":"xmqqwnm110u3.fsf@gitster.g","threadId":"56763","inReplyTo":"02fff111-865d-a2a0-ba06-d0166edfb9dd@gigacodes.de","subject":"Re: [PATCH 1/6] ssh signing: extend check_signature to accept payload metadata","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-25T17:16:52Z","receivedAt":"2021-10-25T17:16:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Stelzer <fs@gigacodes.de> writes:\n\n> On 24.10.21 01:13, Junio C Hamano wrote:\n>>\n>> One possible downside is that sigc has been mostly output-only\n>> structure, and turning it into a structure that also has some input\n>> members might make it too confusing.  I dunno.\n>\n> However if everyone is ok with changing the struct to be used for input\n> as well then i would adjust the function to have it as the first parameter.\n>\n> The sigc struct already has a payload member (only used for verbose\n> output) which is populated by the check function with a xmemdupz. I\n> would then change it to a const char, add the length var and use it to\n> pass the payload into the function as well. This way we also avoid the\n> unnecessary mem copy.\n>\n> The function would just become:\n> check_signature(struct signature_check *sigc, const char *signature,\n> size_t slen)\n\nI do not offhand think of a huge downside going that route myself.\nWe seem to end up with unusually large number of folks on the CC\nlist for some reason, so hopefully somebody will stop us if it is a\nstupid idea ;-)\n\nThanks.\n"}]}