mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <>
To: Fabian Stelzer <>
Cc:, "Han-Wen Nienhuys" <>,
	"brian m. carlson" <>,
	"Randall S. Becker" <>,
	"Bagas Sanjaya" <>,
	"Hans Jerry Illikainen" <>,
	"Ævar Arnfjörð Bjarmason" <>,
	"Felipe Contreras" <>,
	"Eric Sunshine" <>,
	"Gwyneth Morgan" <>,
	"Jonathan Tan" <>,
	"Josh Steadmon" <>
Subject: Re: [PATCH 1/6] ssh signing: extend check_signature to accept payload metadata
Date: Sat, 23 Oct 2021 16:13:26 -0700	[thread overview]
Message-ID: <xmqqfssr8hd5.fsf@gitster.g> (raw)
In-Reply-To: <> (Fabian Stelzer's message of "Fri, 22 Oct 2021 17:09:44 +0200")

Fabian Stelzer <> 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).


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


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

    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

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.


  reply	other threads:[~2021-10-23 23:14 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-22 15:09 [PATCH 0/6] ssh signing: verify key lifetime Fabian Stelzer
2021-10-22 15:09 ` [PATCH 1/6] ssh signing: extend check_signature to accept payload metadata Fabian Stelzer
2021-10-23 23:13   ` Junio C Hamano [this message]
2021-10-25  8:28     ` Fabian Stelzer
2021-10-25 17:16       ` Junio C Hamano
2021-10-22 15:09 ` [PATCH 2/6] ssh signing: add key lifetime test prereqs Fabian Stelzer
2021-10-22 15:09 ` [PATCH 3/6] ssh signing: verify-commit/check_signature with commit date Fabian Stelzer
2021-10-22 17:37   ` Ævar Arnfjörð Bjarmason
2021-10-25  8:31     ` Fabian Stelzer
2021-10-22 15:09 ` [PATCH 4/6] ssh signing: git log/check_signature " Fabian Stelzer
2021-10-22 15:09 ` [PATCH 5/6] ssh signing: verify-tag/check_signature with tag date Fabian Stelzer
2021-10-22 15:09 ` [PATCH 6/6] ssh signing: fmt-merge-msg/check_signature " Fabian Stelzer
2021-10-22 18:12   ` Ævar Arnfjörð Bjarmason
2021-10-25  8:39     ` Fabian Stelzer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqfssr8hd5.fsf@gitster.g \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).