mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Fabian Stelzer <>
To: Junio C Hamano <>
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: Mon, 25 Oct 2021 10:28:58 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <xmqqfssr8hd5.fsf@gitster.g>

On 24.10.21 01:13, Junio C Hamano wrote:
> Fabian Stelzer <> 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

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

  reply	other threads:[~2021-10-25  8:29 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
2021-10-25  8:28     ` Fabian Stelzer [this message]
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 \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

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