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

[Maxim Dounin]

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.

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.

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

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.

-- 
Maxim Dounin
http://mdounin.ru/