Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP/3: Conflicting error codes for DATA frame on control stream #4842

Closed
dtikhonov opened this issue Mar 17, 2021 · 8 comments
Closed

HTTP/3: Conflicting error codes for DATA frame on control stream #4842

dtikhonov opened this issue Mar 17, 2021 · 8 comments
Labels
-http proposal-ready An issue which has a proposal that is believed to be ready for a consensus call.

Comments

@dtikhonov
Copy link
Member

When a DATA frame is received as the first frame on the control stream, HTTP/3 ID-34 mandates two different error codes:

Section 6.2.1:

   Each side MUST initiate a single control stream at the beginning of
   the connection and send its SETTINGS frame as the first frame on this
   stream.  If the first frame of the control stream is any other frame
   type, this MUST be treated as a connection error of type
   H3_MISSING_SETTINGS.

Section 7.2.1:

   DATA frames MUST be associated with an HTTP request or response.  If
   a DATA frame is received on a control stream, the recipient MUST
   respond with a connection error of type H3_FRAME_UNEXPECTED;

(Found by @kazu-yamamoto's h3spec tool while testing lsquic.)

@LPardue
Copy link
Member

LPardue commented Mar 17, 2021

Seems fine to me. Its always an error to send DATA on a control stream. There's a precedence between the two conditions, if we try to articulate that in every case the text could become quite unwieldy.

@larseggert larseggert added this to Triage in Late Stage Processing via automation Mar 17, 2021
@MikeBishop MikeBishop added the proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. label Mar 18, 2021
@project-bot project-bot bot moved this from Triage to Consensus Emerging in Late Stage Processing Mar 18, 2021
@MikeBishop
Copy link
Contributor

I don't feel the need to address this. There are multiple situations in which a particular behavior can be wrong in more than one way, and it's implementation-dependent which failure it notices and errors on first. @quicwg/chairs, does this need any process, as this is a design question?

@LPardue
Copy link
Member

LPardue commented Mar 18, 2021

I'm not seeing any compelling reason to challenge a design principle. In this case specifically there's never a reason to send DATA on control streams, so it's a completely broken implementation.

We try to make it clear that endpoints shouldn't read too much into the specific error codes that they get and in this case the error is terminal for the connection, so there's not much the offender can do when it receives an error.

@dtikhonov
Copy link
Member Author

Responses along the lines of "a behavior may be wrong in more than one way" and "endpoints should not read too much into error codes" go against the spirit of the specification, with its registry of error codes and big bold MUST pronouncements about which error code is to be used under which circumstances. It's inconsistent: what's the point of all the error codes, then, if we allow an implementation just to pick one of the error codes (as in this example)? This is not a rhetorical question.

From the application perspective, how is a spec-verification tool, such as h3spec, to deal with this ambiguity?

@LPardue
Copy link
Member

LPardue commented Mar 18, 2021

Responses along the lines of "a behavior may be wrong in more than one way" and "endpoints should not read too much into error codes" go against the spirit of the specification,

Not really. Section 8 is pretty clear: Stream errors can be upgraded to connection errors and any error condition can be reported as H3_GENERAL_PROTOCOL_ERROR if the implementation chooses to do so.

with its registry of error codes and big bold MUST pronouncements about which error code is to be used under which circumstances. It's inconsistent: what's the point of all the error codes, then, if we allow an implementation just to pick one of the error codes (as in this example)? This is not a rhetorical question.

the MUST applies to how to treat the event of a protocol violation - must close the connection. Error codes are a nice feature on the top - users of HTTP/3 should be entirely prepared for them to be used wrongly.

In this specific case, it sounds like you're asking for something like a blanket requirement for every frame that cannot be sent on the control stream to say something like

> If a <non-control-stream> frame is received on a control stream, the recipient MUST respond with a connection error. The type of error depends on the state of the control stream. If the receiver has not processed a SETTINGS frame, the error code MUST be H3_MISSING_SETTINGS. If the receiver has process a SETTINGS frame, the error code MUST be H3_FRAME_UNEXPECTED.

As @MikeBishop points out, this can be viewed as a design change for implementations that don't follow the guidance exactly like that. I think its too late to revisit this because there is no interoperability or security issues associated with the current design.

From the application perspective, how is a spec-verification tool, such as h3spec, to deal with this ambiguity?

Since such tools have to accommodate any error condition being a connection error, and any error type being a H3_GENERAL_PROTOCOL_ERROR, my advise is that they validate error test cases as so:

  1. Did stream or connection error occur?
  2. Did error type contain one of the set of error codes (H3_GENERAL_PROTOCOL_ERROR, more-specfic error code, ...)

@martinthomson
Copy link
Member

The first clause in this general guidance (in transport, but it should apply to h3 well enough) would seem to make this question moot:

In particular, an endpoint MAY use any applicable error code when it detects an error condition; a generic error code (such as PROTOCOL_VIOLATION or INTERNAL_ERROR) can always be used in place of specific error codes.

Indeed, this exact kind of situation is what motivated the inclusion of this text. These debates are commonplace and waste a lot of time and they also place unnatural constraints on implementations. What an implementation does here might determine which code is reported and that is OK. Both codes are applicable. Reporting either achieves the ultimate goal, which is identifying a bug. Forcing a choice only leads to wasteful discussions like this one.

The answer is that h3spec should accept either error code as valid in this case. In addition to any generally applicable codes.

kazu-yamamoto added a commit to kazu-yamamoto/h3spec that referenced this issue Mar 19, 2021
@kazu-yamamoto
Copy link
Contributor

Talking about h3spec. It is possible to change the test case for Sec 6.2.1 to also accept 3_FRAME_UNEXPECTED. But I changed it to use MAX_PUSH_ID instead of DATA:

kazu-yamamoto/h3spec@2d74a59

@LPardue
Copy link
Member

LPardue commented Jun 14, 2021

Just circling back to this. The proposal was to do nothing and there's since the document is with the RFC editor, the next logical action is to close the issue.

Late Stage Processing automation moved this from Consensus Emerging to Issue Handled Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-http proposal-ready An issue which has a proposal that is believed to be ready for a consensus call.
Projects
Late Stage Processing
  
Issue Handled
Development

No branches or pull requests

6 participants