Subject: Misc review notes for draft-18 p1

From: Willy Tarreau <[email-masked]>
Date: Thu, 26 Jan 2012 16:56:37 +0100

Hi,

I haven't finished reading p1 but I already have some comments, so
I'm sending them here and will proceed with what remains.

2.1. Client/Server Messaging, page 11

>   Note that 1xx responses (Section 7.1 of [Part2]) are not final;
>   therefore, a server can send zero or more 1xx responses, followed by
>   exactly one final response (with any other status code).

This parts falls here quite out of context in my opinion. Neither
responses nor status core nor messaging has been defined yet and all
of a sudden we get this. I suggest we move this to P2 7.1 and replace
it with a small note such as :

  Note that sometimes a server may send multiple responses, see Section
  7.1 of [Part2] for more details about interim responses.

2.4. Intermediaries, page 13

Context :
>            >             >             >             >
>       UA =========== A =========== B =========== C =========== O
>                  <             <             <             <
...

>   For example, B might be receiving
>   requests from many clients other than A, and/or forwarding requests
>   to servers other than C, at the same time that it is handling A's
>   request.

I'd underline that there is no single path between a UA and an intermediary,
and that sometimes direct and indirect communications are possible. It helps
remind people that rewriting URLs along the path is not always a good idea.
I'd suggest this then :

    For example, B might be receiving requests from many clients other than A
    including UA/C/O, and/or forwarding requests to servers other than C, at
    the same time that it is handling A's request.

Later :

>   An HTTP-to-HTTP proxy is called a "transforming proxy" if it is
>   designed or configured to modify request or response messages in a
>   semantically meaningful way (i.e., modifications, beyond those
>   required by normal HTTP processing, that change the message in a way
>   that would be significant to the original sender or potentially
>   significant to downstream recipients).

It is not totally clear to me if a compressing proxy is a transforming
proxy, nor if one that rewrites Location headers to normalize them is
a transforming proxy.

2.7.1. http URI scheme

>    If the host identifier is provided as an IP literal or IPv4 address,

I did not find a clear definition of the term "IP literal". Also, does it
cover the bracketed format of IPv6 ?

3.3. Message Body

>   The length of the message-body is determined by one of the following
>   (in order of precedence):
>
>   1.  Any response to a HEAD request and any response with a status
>       code of 100-199, 204, or 304 is always terminated by the first
>       empty line after the header fields, regardless of the header
>       fields present in the message, and thus cannot contain a message-
>       body.

Now that we've included the CONNECT method in the spec, I think it makes
sense to define whether it has a body or not in case of success. I've
found myself sometimes adding "Content-length: 0" as well as huge values
in the past on some CONNECT requests to help interoperability with broken
proxies, as well as "Connection: close" on these similar requests. Obviously
the implementations were faulty but a faulty implementation often results
from ambiguous specs.

Could we suggest that as a first rule, a 200 response to a CONNECT request
implies an infinite content length (I don't like that very much since it's
false), or that it has no message body and that the connection is immediately
switched to a tunnel ?

   0. Any response with a status code of 200 to a CONNECT request does not
      contain any message-body and immediately switches to a tunnel (Section
      6.9 of [Part2]).

Also, since I've seen some implementations send "Content-length: 0" on
CONNECT requests (which I happened to mimmick once), I'm realizing that
it's not always obvious what to send on responses where no content is
expected. Would it make sense to insist on the fact that it is not
necessary to send "Content-length: 0" on messages which do not have a
body by the rules above ?

3.5. Message Parsing Robustness

>   Likewise, although the line terminator for the start-line and header
>   fields is the sequence CRLF, we recommend that recipients recognize a
>   single LF as a line terminator and ignore any CR.

Does this mean that CR CR CR CR CR CR LF should be interpreted as a single
LF ? It kinds of scares me on the risk of smuggling attacks. I'd rather
suggest :

    ... we recommend that recipients recognize a single LF as a line
    terminator and ignore the optional preceeding CR. Messages containing
    a CR not followed by an LF MUST be rejected.

>   When a server listening only for HTTP request messages, or processing
>   what appears from the start-line to be an HTTP request message,
>   receives a sequence of octets that does not match the HTTP-message

Wouldn't "does not *exactly* match" be better ? I'm used to find
crappy requests in my logs which are blocked but which some not-so-lazy
implementations would let pass (eg: multiple SP).

>   grammar aside from the robustness exceptions listed above, the server
>   MUST respond with an HTTP/1.1 400 (Bad Request) response.

I would also suggest that clients and proxies protect themselves against
malformed response messages, which are problematic in shared hosting
environments. This could be summarized like this :

    In general, any agent which receives a malformed message MUST NOT try
    to fix it if there is any possibility that any other implementation
    along the chain understands it differently. In such conditions, the
    message MUST be rejected.

4.1. Types of Request Target

> Note: The "no rewrite" rule prevents the proxy from changing the

I did not find reference to this "no rewrite" rule.

4.2. The Resource Identified by a Request

>   1.  If request-target is an absolute-URI, the host is part of the
>       request-target.  Any Host header field value in the request MUST
>       be ignored.
>
>   2.  If the request-target is not an absolute-URI, and the request
>       includes a Host header field, the host is determined by the Host
>       header field value.
>
>   3.  If the host as determined by rule 1 or 2 is not a valid host on
>       the server, the response MUST be a 400 (Bad Request) error
>       message.

Rule 3 might be difficult to apply in massively hosted environments, as
I easily imagine that there could be a large "vhosts" directory with
all the hosts roots presented by their names there. The server would
then simply try to "cd $host" to check for the host's validity, which
might seem appropriate at first. But using a host of ".." or a host
containing a slash would have dramatic effects.

I don't know what recommendation we could add here because we can't
add boring long sentences, but avoiding such simple traps would be
nice. Maybe we should just add :

    For instance, a host should never be ".." nor contain a slash.

7.4. Use of HTTP by other protocols

It would make sense to list WebSocket here too since it's the first large
scale user of the Upgrade mechanism.

8.4. TE

>   The presence of the keyword "trailers" indicates that the client is
>   willing to accept trailer fields in a chunked transfer-coding, as

Is it only limited to the client ? Nowhere it's said that a server cannot
advertise "TE: trailers" in responses so that a client knows it can emit
chunked-encoded messages with trailers in further requests (eg: backups
with SHA1 at the end). Replace "client" with "sender" maybe ?

8.5. Trailer

>   If no Trailer header field is present, the trailer SHOULD NOT include
>   any header fields.  See Section 5.1.1 for restrictions on the use of
>   trailer fields in a "chunked" transfer-coding.

in 5.1.1 p37, it's said :

>   A server using chunked transfer-coding in a response MUST NOT use the
>   trailer for any header fields unless at least one of the following is
>   true:

Is the SHOULD NOT vs MUST NOT difference on purpose ? It seems to indicate
that there should be a tolerance when parsing unadvertised trailers (I'm
fine with this, just checking whether the wording is expected).

A.1.2 Keep-Alive Connections

>   Clients are also encouraged to consider the use of Connection: keep-
>   alive in requests carefully; while they can enable persistent
>   connections with HTTP/1.0 servers, clients using them need will need
>   to monitor the connection for "hung" requests (which indicate that
>   the client ought stop sending the header),

I know a number of people who use the term "the header" to designate all
the headers section. I must say that when I read this sentence, it was
unclear to me upon first reading that the intent was in fact to stop
sending "Connection: keep-alive" in subsequent requests, as it can also
be understood as "stop sending the headers as long as the connection
hangs" (which does not make sense).

I'd suggest the following change :

-   the client ought stop sending the header),
+   the client ought stop using this header in further communications with
+   the server),

At a number of places it is suggested to "close the connection". I
think we could add an annex such as the following one, with references
everywhere we suggest closing the connection, as well as one pointer
in "6.1.2.2 Pipelining" :

    A.x.x Closing a Connection
    
    When a server needs to close a connection, it must ensure that doing so
    will not risk prematurely terminate any previous response. When TCP
    segments are still in flight during a socket close, operating systems
    generally turn the socket to orphaned state, during which lingering data
    will still be emitted but any received data would cause an immediate
    connection abort. The connection may also be aborted when the system
    is getting low on orphaned sockets. This means that a close before all
    lingering data are acknowledged by the client might result in a loss of
    unacknowledged data. This is a very common issue when performing a
    redirect upon a POST request before all the client's body has been read.
    While this is not always an issue when a server wants to abort a current
    request, it becomes a real issue when the client tries to pipeline requests,
    because aborting the current request may also result in destroying previous
    unacknowledged response too, possibly causing a client to retry already
    processed requests that it believes were ignored.
    
    The proper way for a server to close a connection without risking issues
    described above is the following :
    
       1) shutdown the transmit channel, usually using the shutdown() system
          call.
       2) drain any incoming data and (if possible) check for any lingering
          data in the transmit queue.
       3) when the receive channel reports a shutdown, or when all transmitted
          data have been acknowledged, or when enough time has elapsed, perform
          the close() on the socket.
    
    Operating systems do not always easily report the amount of lingering data
    and will not always wake up when the queue is empty. A tradeoff has to be
    found between keeping connections alive for too long a time and risking
    closing too early and having some clients get truncated or empty responses.

That's all for me now, I'll probably have other comments later.

Regards,
Willy

---

From: Julian Reschke <[email-masked]>
Date: Thu, 26 Jan 2012 19:15:05 +0100

>    7.1 of [Part2] for more details about interim responses.

We did that totally on purpose, see 
<http://trac.tools.ietf.org/wg/httpbis/trac/ticket/300>.

>      the same time that it is handling A's request.

UA I see, but C and O?

> cover the bracketed format of IPv6 ?

I think we need to ref 
<http://greenbytes.de/tech/webdav/rfc3986.html#rfc.section.3.2.2> here.

>      a CR not followed by an LF MUST be rejected.

Sounds good to me.

> implementations would let pass (eg: multiple SP).

"match" means "match"; I don't think there's any ambiguity here...

>      message MUST be rejected.

-0.5.

- it's a requirement hard to test for, and

- it's not going to be implemented by browsers.

> I did not find reference to this "no rewrite" rule.

It's the rule above the note.

-> <http://trac.tools.ietf.org/wg/httpbis/trac/changeset/1517>

>      For instance, a host should never be ".." nor contain a slash.

Are those allowed in a host name anyway?

> with SHA1 at the end). Replace "client" with "sender" maybe ?

We seem to be confused about who can set TE anyway:

"The "TE" header field indicates what extension transfer-codings it is 
willing to accept in the response, and whether or not it is willing to 
accept trailer fields in a chunked transfer-coding."

We need to state who "it" is...

> +   the client ought stop using this header in further communications with

"...ought to stop using this header field in further ..."?

> That's all for me now, I'll probably have other comments later.

Thanks a lot for that; I tried to comment where I had some confidence on 
the resolution.

We probably need to figure out a way to manage the feedback better; 
maybe recommend sending smaller chunks with meaningful subject lines, so 
threading works properly?

Best regards, Julian

---

From: Zhong Yu <[email-masked]>
Date: Thu, 26 Jan 2012 12:32:36 -0600

On CRLF:

Are there actually any HTTP clients using a single LF as line
terminator? That sounds improbable. (Unless you count telnet + manual
input)

Zhong yu

---

From: Jeroen de Borst <[email-masked]>
Date: Thu, 26 Jan 2012 18:38:42 +0000

This is frequently encountered with HTTP clients that are not browsers....

- scripts to test their webserver
- robots to find broken links
- programs that post updates of some kind (blog clients)
- etc

Jeroen

-----Original Message-----
From: Zhong Yu [mailto:[email-masked]] 
Sent: Thursday, January 26, 2012 10:33 AM
To: Julian Reschke
Cc: Willy Tarreau; [email-masked]
Subject: Re: Misc review notes for draft-18 p1

On CRLF:

Are there actually any HTTP clients using a single LF as line terminator? That sounds improbable. (Unless you count telnet + manual
input)

Zhong yu

---

From: Zhong Yu <[email-masked]>
Date: Thu, 26 Jan 2012 12:58:12 -0600

It appears that RFC 2616 section 19.3 also grants both LF and CRLF,
but not CRCRLF etc.

   The line terminator for message-header fields is the sequence CRLF.
   However, we recommend that applications, when parsing such headers,
   recognize a single LF as a line terminator and ignore the leading CR.

---

From: Willy Tarreau <[email-masked]>
Date: Thu, 26 Jan 2012 20:36:20 +0100

Hi Julian,

> <http://trac.tools.ietf.org/wg/httpbis/trac/ticket/300>.

OK, but don't you think that moving the same text to its place and having
just an explicit reference as I proposed would satisfy the ticket ? The
text itself is fine, it's just that when I read the beginning of the spec,
it suddenly speaks a language that was not even introduced earlier, which
makes one think this was added late or is a leftover of some old text.

> >     the same time that it is handling A's request.

Both are common cases when B is a load balancer and C is an accelerator
(eg: cache). Servers are also clients of other servers in many places,
and then they reach them via the LB. For the same reason, some caches
will reach some origin server farms via the LB on misses.

> <http://greenbytes.de/tech/webdav/rfc3986.html#rfc.section.3.2.2> here.

Ah perfect, thanks for the link, indeed I guessed right.

(...)
> "match" means "match"; I don't think there's any ambiguity here...

There's no ambiguity, it's just to emphasize on the need to perform
strict matching. A large number of HTTP parsers are much too lazy,
causing nightmares when trying to filter undesired communications,
or even to define new protocol extensions. For instance on my old
Apache 1.3 here :

    $ telnet www 60080
    Connected to www.
    Escape character is '^]'.
    HEAD     /           HTTP/1.1     ergeargoaejgoiejgaoeg
    Host:   ,,,,
    Invalid/header name: blah
    
    HTTP/1.1 200 OK
    Date: Thu, 26 Jan 2012 19:07:02 GMT
    Server: Apache
    Last-Modified: Mon, 01 Jun 2009 16:47:12 GMT
    ETag: "47038-3ad7-46b4c2d81a400"
    Accept-Ranges: bytes
    Content-Length: 15063
    Connection: close
    Content-Type: text/html
    
    Connection closed by foreign host.

"SP" is *one* SP, still multiple SPs are accepted in the request
line. Same for forbidden chars in the header name. And I'm not
specifically targeting Apache here, I just took the first example
I had handy, it's far from being alone. It looks like strchr(),
strtok(), sscanf() or split() depending on the language and
implementation are common ways to parse requests. This is part
of what caused all the mess in the hybi WG, delaying it by one
year trying to find solutions against various implementations.

> - it's not going to be implemented by browsers.

I agree on your last point, so maybe we could refine this to target
intermediaries only. The reason is that (as an implementor) I'm
regularly asked to implement ugly tricks to help fix issues that
are caused by application bugs. Among the funny requests "could you
please fix haproxy to ignore multiple empty lines after the headers"
or "it would help me if haproxy could only focus on the first
occurrence of header X or Y and ignore all other ones because
a bug in my application repeats them with empty contents".

I've always rejected such dangerous crap and only accepted to
offer options to temporarily relax some checks but I think that
all the web would benefit from a reminder on these dangerous
practices which may lead to smuggling attacks.

> -> <http://trac.tools.ietf.org/wg/httpbis/trac/changeset/1517>

Ah OK thanks for the fix.

(...)
> Are those allowed in a host name anyway?

Should not but the characters are allowed, so the name at least
matches the basic regex '[a-zA-Z0-9.-]+' that simple implementations
would at least check. I'm not saying I've seen that in field, but I
see how easy it is to get trapped. Perhaps something like this might
be enough :

    Note: multi-homed web servers are encouraged to perform very strict
          checks on the host name syntax and validity, and not simply
          rely on the existence of a directory name to accept it.

> We need to state who "it" is...

Yes I noted this one too and forgot to report it. How about this :

   A client may use the "TE" header field in a request to indicate what
   extension transfer-codings it is willing to accept in the response, and
   whether or not it is willing to accept trailer fields in a chunked
   transfer-coding.

   The "TE" header's value consists ...

> >A.1.2 Keep-Alive Connections
(...)
> "...ought to stop using this header field in further ..."?

Perfect.

> threading works properly?

That's what I intended to do first, because I only had minor issues
that could be grouped together, but I'm talking too much. I was planning
on reporting complex ones in individual mails as well as at least split
the mails by part.

Probably that in a few days I'll resend the parts without any response
in individual mails.

Thanks,
Willy

---

From: Willy Tarreau <[email-masked]>
Date: Thu, 26 Jan 2012 20:38:27 +0100

> terminator? That sounds improbable. (Unless you count telnet + manual

Most monitoring scripts using "echo".

Willy

---

From: Amos Jeffries <[email-masked]>
Date: Mon, 30 Jan 2012 13:00:29 +1300

> body by the rules above ?

+1 to avoiding Content-Length corruption tricks. But ...

I have a client in a situation which requires relaying CONNECT requests 
between proxies. We can safely negotiate chunking between two 1.1 
proxies to enable keep-alive optimizations on these, if it were to be 
defined as having no body at all that would break.

The same problem may also apply to Upgrade for unknown protocols. When 
the upgraded protocol is completed, having the handlers close the 
connection and cycle through TCP TIME_WAIT instead of cleanly releasing 
it for re-use by the outer layer protocol again would be a major annoyance.

>      a CR not followed by an LF MUST be rejected.

+1.

> implementations would let pass (eg: multiple SP).

+1. I've just finished my second round of convincing peoples not to pad 
out the end of the line to a nice round length with SP.

> +   the client ought stop using this header in further communications with

Nit:
     s/ought/ought to/

AYJ

---

From: Amos Jeffries <[email-masked]>
Date: Mon, 30 Jan 2012 13:17:32 +1300

> year trying to find solutions against various implementations.

FWIW: we argued this out in Squid a while back.
The conclusion was to accept any series of non-wrapping BWS before/after 
the method and URL. Ignoring the BWS. All other formats and garbage to 
be treated as HTTP/0.9 mess and 400 the result if the suspected 
URL(+garbage) fails to parse as a usable URI in its entirety.

A few vendors have hit it with their SP padding practices so far. But by 
and large it works.

AYJ