Re: [PATCH 3/7] pack-protocol.txt: Mark all LFs in push-cert as required

[Dave Borowitz <dborowitz@google.com>]

On Mon, Jul 6, 2015 at 1:11 PM, Dave Borowitz <dborowitz@google.com> wrote:
> On Mon, Jul 6, 2015 at 12:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Dave Borowitz <dborowitz@google.com> writes:
>>
>>> The server can munge pkt-lines and reinsert LFs, but it _must_ have
>>> some way of reconstructing the payload that the client signed in order
>>> to verify the signature. If we just naively insert LFs where missing,
>>> we lose the ability to verify the signature.
>>
>> I still do not understand this part.
>>
>> There is no way to "naively" insert, is there?  You have an array of
>> lines (each of which you have already stripped its terminating LF at
>> its end).  How else other than adding one LF at the end of each
>> element do you reconstruct the original multi-line string the client
>> signed?  Are there other ways that makes the result ambiguous??
>
> I think I understand the confusion now. I think you and I are working
> from different assumptions about the client behavior.
>
> My assumption was: the intended behavior for the client is to sign the
> exact payload that was sent over the wire, minus pkt-line headers.
>
> For example, under my assumption, if the client sent:
>
> 0008foo\n
> 0007bar
> 0008baz\n
>
> then this indicates the client signed:
>
> "foo\nbarbaz\n"
>
> Under this assumption, "naively inserting LF" means inserting an LF
> after "bar". Thus the server would record the following in a
> persistent store:
>
> "foo\nbar\nbaz\n"
>
> If we only store this string, and do not remember the fact that the
> client originally omitted one of those LFs, then when we go back to
> verify that signature later, it will fail.
>
> That was my assumption.
>
> Your assumption, IIUC, is that the payload the client signed MUST have
> contained LFs in between each line. When framing the content for the
> wire, the client MUST send one "logical line", which has no trailing
> LF, per pkt-line, and furthermore the pkt-line content MAY contain an
> additional trailing LF.
>
> Under your assumption, the server always has enough information to
> reconstruct the original signed payload.
>
>
> The problem with the documentation, then, is that the documentation
> does not say anything to indicate that the signed payload is anything
> other than what is on the wire.

Another way of looking at the problem with my assumptions is, I was
assuming "pkt-line framing" was the same thing as "pkt-line header".
You seem to be saying the definition of "pkt-line framing" is "header,
and optional trailing newline".

A quick scan of pack-protocol.txt did not turn up anything one way or
the other on this issue, so perhaps we could make it more explicit.
The additional upside here is that we could then potentially remove
all or almost all LFs from this document.

> So maybe this series should include an explicit description of the
> singed payload outside of the context of a push. Then, in the push
> section, we can describe the set of transformations that the client
> MUST perform (splitting on LF; adding pkt-line headers) and MAY
> perform (adding LFs).
>
>>> If we say the payload the client signs MUST have LFs only in certain
>>> places, then that gives the server enough information to reconstruct
>>> the payload and verify the signature.
>>>
>>> But if we say the signed payload MUST have LFs and the wire payload
>>> MAY have LFs, then now we have two completely different formats, only
>>> one of which is documented.
>>
>> I thought that was what I was saying.  The wire protocol sends the
>> contents of each line (both what is signed and the signature) on a
>> separate packet.  When I say "contents of a line", I do not include
>> the terminating LF as part of the line (iow, LF is not even
>> optional; the terminating LF is not considered as part of "the
>> contents of a line").  It becomes irrelevant that a pkt-line may or
>> may not have a trailing optional LF.  If there is LF at the end of a
>> packet between "push-cert" and "push-cert-end" packets, that LF by
>> definition cannot be part of the "contents of a line" in a
>> certificate.
>>
>> It is just a pkt-line framing artifact you can and should remove if
>> you are doing a "split to array, join with LF" implementation to
>> recover the original string.
>>
>> And that is very much consistent with the way we send other things
>> with pkt-line protocol.  Each packet up to the first flush is
>> expected to have <object name> and <refname> as ref advertisement.
>> The pkt-line framing may or may not add a trailing LF, but LF is not
>> part of <refname>.  It is not even part of the payload; merely an
>> artifact of pkt-line framing.