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@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
