Subject: Re: Last Call: <draft-ietf-httpbis-header-structure-18.txt> (Structured Field Values for HTTP) to Proposed Standard

From: Julian Reschke <[email-masked]>
Date: Wed, 13 May 2020 05:59:53 +0200

Hi there,

I note that this mail was cc'd to the HTTP WG mailing list
([email-masked]), but apparently didn't make it there (see
<https://lists.w3.org/Archives/Public/ietf-http-wg/2020AprJun/>). We
really should find out what went wrong here.

(the same seems to have happened for the LC on
draft-ietf-httpbis-client-hints-13 a few days later)

I'll use that as somewhat lame excuse for a very late comment on the
document (I've been working on an implementation and this came up just
yesterday):

<https://tools.ietf.org/html/draft-ietf-httpbis-header-structure-18#section-4.2>
says:

>    When generating input_bytes, parsers MUST combine all lines in the
>    same section (header or trailer) that case-insensitively match the
>    field name into one comma-separated field-value, as per [RFC7230],
>    Section 3.2.2; this assures that the entire field value is processed
>    correctly.

It doesn't require *how* to combine these values (and that's ok because
it can't).

Later on it says:

>    Strings split across multiple field lines will have unpredictable
>    results, because comma(s) and whitespace inserted upon combination
>    will become part of the string output by the parser.  Since
>    concatenation might be done by an upstream intermediary, the results
>    are not under the control of the serializer or the parser.

So there are HTTP messages that can lead to different parsing results,
depending on what intermediaries do (how *they* combine the lines), and
what the final recipient does.

That this can happen for certain inputs is a given; *this* spec can't do
anything about that as it doesn't control the other parts in the
transmission chain.

What bugs me is that we have an invalid message to start with, but the
spec apparently *requires* receivers to accept it, albeit with
potentially unpredictable results.

IMHO it would be better to allow those recipients that *can* detect the
brokenness to reject these fields.

If it is really intended to forbid that it might be good to add a short
explanation why this is the case.

Best regards, Julian

---

From: Mark Nottingham <[email-masked]>
Date: Fri, 15 May 2020 12:27:00 +1000

Hi Julian,

> On 13 May 2020, at 1:59 pm, Julian Reschke <[email-masked]> wrote:
> 
> Hi there,
> 

Will look into it.

> It doesn't require *how* to combine these values (and that's ok because

Right; the "how" is in 7230 3.2.2.

> anything about that as it doesn't control the other parts in the

Yes. That part of the text is really an aside explaining how things work, rather than anything normative.

> brokenness to reject these fields.

The problem is that many recipients won't be able to. This includes not only when an intermediary combines multiple field lines into one, but also when a server or library does so (which is more common IME).

We already have potential inconsistency in whitespace caused by such combination. I'm reluctant to add another dimension of inconsistency (whether or not the SH^HF implementation can recognise this situation and reject early).

> explanation why this is the case.

Yes. 

Also, I think we can more clearly prohibit implementations from sending that form (although I doubt it's much of an issue in practice).

Cheers,

--
Mark Nottingham   https://www.mnot.net/

---

From: Julian Reschke <[email-masked]>
Date: Fri, 15 May 2020 10:20:21 +0200

On 15.05.2020 04:27, Mark Nottingham wrote:
>  ...

You know that I know that :-). The point being, RFC 7230 doesn't say
either (except that it needs to be COMMA + optional surrounding whitespace).

Yes.

> We already have potential inconsistency in whitespace caused by such combination. I'm reluctant to add another dimension of inconsistency (whether or not the SH^HF implementation can recognise this…

Understood, but I would see it this way: having *some* implementations
able to detect broken input is better than nobody detecting it, because
this way the problem might be fixed.

I'd really like to hear some more feedback on this. Note that this is
related to what we can say in draft-ietf-httpbis-semantics - is a
recipient allowed *not* to combine field lines when they are clearly in
error, such as with:

   Location: https://example.com/foo
   Location: bar

?

> Also, I think we can more clearly prohibit implementations from sending that form (although I doubt it's much of an issue in practice).

Yes, this is totally an edge case.

Best regards, Julian

---

From: Willy Tarreau <[email-masked]>
Date: Fri, 15 May 2020 17:13:36 +0200

On Fri, May 15, 2020 at 10:20:21AM +0200, Julian Reschke wrote:
> > The problem is that many recipients won't be able to. This includes not
> > only when an intermediary combines multiple field lines into one, but also
> > when a server or library does so (which is more common IME).
> 
> Yes.
> 
> > We already have potential inconsistency in whitespace caused by such
> > combination. I'm reluctant to add another dimension of inconsistency
> > (whether or not the SH^HF implementation can recognise this situation and
> > reject early).
> 

I agree with this. Typically the problem is that clients could abuse
intermediaries to transform their requests. So adding a hint for
intermediaries here would help. If it's explicitly written that a
recipient is allowed to reject a request having such invalid header
value, I'm fine telling haproxy users that what haproxy does on this
or that header field is correct and spec-compliant. Otherwise the best
I can do is encourage them to add blocking rules it if they dare do it.

The problem is always the same: intermediaries are forced to remain as
transparent as possible (even violating specs sometimes) because when
inserting them results in breakage, it's necessary their fault. Here we
have an opportunity to allow them to be a bit stricter, we really ought
to take it!

Just my two cents,
Willy

---

From: Julian Reschke <[email-masked]>
Date: Fri, 15 May 2020 17:20:52 +0200

> have an opportunity to allow them to be a bit stricter, we really ought

+1

Related to this: it just occurred to me that:

   Test: Foo
   Test:
   Test: Bar

yields different results for fields using HTTP's list notation, and
structured header fields.

In the former case, the combined value is equivalent to

   Test: Foo, Bar

while in the latter case, the field is malformed.

I *really* think it would be better if structured header fields would
actually be "proper" applications of the standard HTTP list ABNF.

Best regards, Julian

---

From: "Poul-Henning Kamp" <[email-masked]>
Date: Fri, 15 May 2020 16:06:37 +0000

--------
In message <[email-masked]>, Julian Reschke writes

>actually be "proper" applications of the standard HTTP list ABNF.

Considering how rarely used multiple-headers are (apart from The
Cookie Mistake) I think it would be much simpler, better and wiser
to restrict SH to single, non-concatenated headers.

Ufortunately, that also runs up against the fact that we cannot
possibly know if the header we got was joined by an unwitting
intermediary.

-- 
Poul-Henning Kamp       | UNIX since Zilog Zeus 3.20
[email-masked]         | TCP/IP since RFC 956
FreeBSD committer       | BSD since 4.3-tahoe    
Never attribute to malice what can adequately be explained by incompetence.

---

From: Willy Tarreau <[email-masked]>
Date: Fri, 15 May 2020 18:12:20 +0200

> possibly know if the header we got was joined by an unwitting

Definitely, which is why suggesting that each occurrence must conform
is a good compromise which is much easier to enforce at multiple points
in the chain.

Willy

---

From: Julian Reschke <[email-masked]>
Date: Sat, 16 May 2020 05:39:32 +0200

> to restrict SH to single, non-concatenated headers.

I have no idea what the exact proposal would be. Fail when multiple
instances are there?

Best regards, Julian

---

From: Willy Tarreau <[email-masked]>
Date: Sat, 16 May 2020 06:31:51 +0200

> I have no idea what the exact proposal would be. Fail when multiple

That would be really annoying, because the main reason multiple headers
happen is that most of these are inconditionally added by components
which do not search for them. Look at the famous x-forwarded-for as an
example!

I'd say that multiple headers are the most natural way to produce an
ordered list, and one which will continue to exist.

Willy

---

From: "Poul-Henning Kamp" <[email-masked]>
Date: Sat, 16 May 2020 05:42:21 +0000

--------
In message <[email-masked]>, Julian Reschke writes

>I have no idea what the exact proposal would be. Fail when multiple

That has always been my stance:  Multiple instances of the same
header should have been banned long ago, ideally before the
Cookie-Mistake allowed the total plunder of our privacy.

---

From: Willy Tarreau <[email-masked]>
Date: Sat, 16 May 2020 07:50:42 +0200

> Cookie-Mistake allowed the total plunder of our privacy.

I disagree. Lots of components by then already needed to append
"connection: close", "cache-control: no-cache", "x-forwarded-for: foo"
and so on without having the ressources required to check for their
existence, or having the programmatic flexibility to let the user
express what to do. Don't forget that 20 years ago this was very
common and the amount of available CPU and RAM wasn't the same as
today.

I think that the current definition is fine and reasonable. It doesn't
pose any problem as long as those who care about the field's value are
able to reject partial values. Those in the middle who aggregate the
partial values are not impacted if they don't use it, so the real
recipient for this header is the last one which either sees invalid,
partial values, or a complete, valid one.

Willy

---

From: Mark Nottingham <[email-masked]>
Date: Mon, 18 May 2020 16:06:08 +1000

> On 15 May 2020, at 6:20 pm, Julian Reschke <[email-masked]> wrote:
>> 

I think this means adding something like this near the bottom of 4.2. Parsing Structured Fields:

"""
Parsers MAY fail when processing a field value spread across multiple field lines, when one of those lines does not parse as that field. For example, a parsing handling an Example-String field that's defined as a sf-string is allowed to fail when processing this field section:

Example-String: "foo
Example-String: bar"
"""

Does that do it?

Cheers,

---

From: Julian Reschke <[email-masked]>
Date: Mon, 18 May 2020 08:44:20 +0200

> Parsers MAY fail when processing a field value spread across multiple field lines, when one of those lines does not parse as that field. For example, a parsing handling an Example-String field that…

Looks very good to me.

Best regards, Julian

---

From: Mark Nottingham <[email-masked]>
Date: Mon, 18 May 2020 17:18:40 +1000

> On 18 May 2020, at 4:44 pm, Julian Reschke <[email-masked]> wrote:
> 
> Looks very good to me.

Committed in <https://github.com/httpwg/http-extensions/commit/0baf4b5e9>.

Thanks,

---

From: Julian Reschke <[email-masked]>
Date: Mon, 18 May 2020 15:06:02 +0200

Another late commment...

<https://greenbytes.de/tech/webdav/draft-ietf-httpbis-header-structure-18.html#rfc.section.3.1>:

> 3.1. Lists
>
> Lists are arrays of zero or more members, each of which can be an Item (Section 3.3) or an Inner List (Section 3.1.1), both of which can be Parameterized (Section 3.1.2).
>
> The ABNF for Lists in HTTP fields is:
>
> sh-list       = list-member *( *SP "," *SP list-member )
> list-member   = sh-item / inner-list

So the prose says zero elements are allowed, but the ABNF actually
requires one element.

One of these is incorrect...

Best regards, Julian

---

From: Benjamin Kaduk <[email-masked]>
Date: Mon, 18 May 2020 11:27:46 -0700

> One of these is incorrect...

You saw

   An empty List is denoted by not serializing the field at all.

right?

-Ben

---

From: Julian Reschke <[email-masked]>
Date: Mon, 18 May 2020 21:22:47 +0200

>     An empty List is denoted by not serializing the field at all.

That's about serialization.

4.2.1 seems to parse an empty string into an empty list.

AFAICT, that's in conflict with the ABNF.

Best regards, Julian

---

From: Benjamin Kaduk <[email-masked]>
Date: Mon, 18 May 2020 14:23:34 -0700

> AFAICT, that's in conflict with the ABNF.

Thanks for clarifying.

FWIW I already have a comment staged about how we seem to make the
ABNF normative for serialization but the prose normative for parsing,
which seems like a weird mismatch that requires justification.

-Ben

---

From: Mark Nottingham <[email-masked]>
Date: Tue, 19 May 2020 09:20:57 +1000

> On 19 May 2020, at 7:23 am, Benjamin Kaduk <[email-masked]> wrote:
> 
>>> You saw
>>> 

As per S 1.2 [1], the ABNF is not for parsing; it's for 'illustrat[ing] the range of acceptable wire representations.'

The ABNF requires at least one member because sending an empty field value is not good practice; it's not something we encourage.

> which seems like a weird mismatch that requires justification.

ABNF isn't normative for serialisation; what led you to that conclusion? 

1. https://httpwg.org/http-extensions/draft-ietf-httpbis-header-structure.html#notational-conventions

---

From: Benjamin Kaduk <[email-masked]>
Date: Mon, 18 May 2020 16:35:13 -0700

> 1. https://httpwg.org/http-extensions/draft-ietf-httpbis-header-structure.html#notational-conventions

Well, we are very explicit that "when parsing [...], implementations MUST
follow the algorithms. [...] If there is a disagreement between the parsing
algorithms and the ABNF, the specified algorithms take precedence".  We lack
such strong language for the serialization process, and in fact say that
"[i]mplementations MAY vary from the specified behavior so long as the output
still matches the ABNF", with the algorithms merely being the "recommended way
to produce them".  To me, that implies that the ABNF is controlling, as any
variance in serialization procedure is contingent on the output matching the
ABNF.  If that's not the intended reading, I suggest making the language more
parallel to the parsing case.

-Ben

---

From: Julian Reschke <[email-masked]>
Date: Tue, 19 May 2020 04:49:58 +0200

> The ABNF requires at least one member because sending an empty field value is not good practice; it's not something we encourage.

So the real inconsistency here is that sending an empty value is not ok
(forbidden?, not encouraged?), but an empty string is a valid parser input.

That is confusing; if that's really intended, it should be explained
somewhat more. AFAIC, it would be way easier just to align "what can be
in a field value" with "what the parser accepts as input".

I note that there is a test called "empty list" at
<https://github.com/httpwg/structured-header-tests/blob/master/list.json#L8>:

>     {
>       "name": "empty list",
>       "raw": [""],
>       "header_type": "list",
>       "expected": [],
>       "canonical": []
>     },

<https://github.com/httpwg/structured-header-tests/blob/master/README.md>
says:

> raw: An array of strings, each representing a header field value received

I understand that the tests are not part of the spec, but this seems to
support the case that empty field value should be accepted.

Best regards, Julian

---

From: Benjamin Kaduk <[email-masked]>
Date: Mon, 18 May 2020 19:55:45 -0700

> (forbidden?, not encouraged?), but an empty string is a valid parser input.

That does seem like a key topic for the DISCUSS ballot I just submitted.
(I wanted to get the rest of my comments available, and don't intend for
my DISCUSS text to be "the final word"; let's please continue this discussion
and go for clarity.)

-Ben