[PATCH 2 of 2] HTTP/2: finalize request as bad if header validation fails

[Roman Arutyunyan]

Hi,

On Thu, Mar 09, 2023 at 03:40:26AM +0300, Maxim Dounin wrote:
> Hello!
> 
> On Wed, Mar 08, 2023 at 06:15:18PM +0400, Roman Arutyunyan wrote:
> 
> > Hi,
> > 
> > On Wed, Feb 22, 2023 at 03:55:28PM +0300, Maxim Dounin wrote:
> > > # HG changeset patch
> > > # User Maxim Dounin <mdounin at mdounin.ru>
> > > # Date 1677070454 -10800
> > > #      Wed Feb 22 15:54:14 2023 +0300
> > > # Node ID ec3819e66f40924efad183c291c850d9ccef16e7
> > > # Parent  61bd779a868c4021c232dddfe7abda7e8ad32575
> > > HTTP/2: finalize request as bad if header validation fails.
> > > 
> > > Similarly to 7192:d5a535774861, this avoids spurious zero statuses
> > > in access.log, and in line with other header-related errors.
> > 
> > This is what RFC 9113, 8.2.1. Field Validity, says about header field
> > validation:
> > 
> >   A request or response that contains a field that violates
> >   any of these conditions MUST be treated as malformed (Section 8.1.1).
> > 
> > Also (the previous RFC 7540 did not have this statement):
> > 
> >   When a request message violates one of these requirements, an implementation
> >   SHOULD generate a 400 (Bad Request) status code (see Section 15.5.1 of [HTTP])
> > 
> > Considering this, responding with HTTP 400 is certainly an improvement compared
> > to the current code.  Also, it allows to set a custom error page handler.
> > 
> > However, not resetting the stream as before is a less obvious change.  If we
> > consider nginx an intermediary, the RFC 9113 prescribes us to send stream reset:
> > 
> >   Intermediaries that process HTTP requests or responses (i.e., any
> >   intermediary not acting as a tunnel) MUST NOT forward a malformed
> >   request or response. Malformed requests or responses that are detected
> >   MUST be treated as a stream error (Section 5.4.2) of type PROTOCOL_ERROR.
> > 
> > This restriction probably makes sense for the intermediaries unable to handle
> > the error in a more flexible way.
> > 
> > I like the change, but I'd like to hear what you think about the reset issue.
> 
> We already do exactly the same for quite similar cases, as 
> introduced in 7192:d5a535774861, for unknown and invalid 
> pseudo-headers.  This patch simply makes handling of 
> errors detected by ngx_http_v2_validate_header() equivalent.

Yes, I have read that discussion.  It's seemed suspicious though that only the
pseudo-headers error was treated as HTTP 400 by that change, while
ngx_http_v2_validate_header(), which is just a few lines above, wasn't
affected.

> I does not look like returning 400 instead of sending RST_STREAM 
> causes any issues.  Further, given that HTTP/2 error handling is 
> mostly unimplemented by clients, I tend to think that returning 
> 400 instead of RST_STREAM is a better option.

I personally like it too.

> Note well that nothing in RFC 7540 actually requires to send 
> RST_STREAM: while it says that (detected) malformed requests "MUST 
> be treated as a stream error", handling of stream errors does not 
> require to actually send RST_STREAM, as there is no such normative 
> requirement in section 5.4.2.  

Indeed there's no requirement level here, which makes the whole paragraph
pointless:

   An endpoint that detects a stream error sends a RST_STREAM frame
   (Section 6.4) that contains the stream identifier of the stream where
   the error occurred.  The RST_STREAM frame includes an error code that
   indicates the type of error.

> Further, RFC 9113 explicitly states 
> that (https://www.rfc-editor.org/rfc/rfc9113#section-8.1.1):
> 
>   For malformed requests, a server MAY send an HTTP response prior 
>   to closing or resetting the stream. 
> 
> Note "closing or resetting", which implies that RST_STREAM is not 
> mandatory.

If the server is an origin server, and the request is malformed, stream error
is indeed not mandatory.  But for an intermediary, it is.  It's however not
clear, whether stream error should trigger RST_STREAM, considering the lack
of a requirement level in 5.4.2.

> While we might consider changing the code to generate RST_STREAM 
> after sending the response in such cases, this does not seem to be 
> needed (and might actually make things worse).  In either case 
> this doesn't look like something to be addressed in this patch 
> series.

The discussion is also relevant to HTTP/3 which may trigger either QUIC or
HTTP errors for malformed request streams.

As I said before, I like HTTP errors more since they provide more flexibility
and are more compatible with the way nginx treats HTTP/1 requests.  Please
commit the patch.

--
Roman Arutyunyan