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

Robert Wilton's QPACK Comment 3 #4802

Closed
LPardue opened this issue Jan 21, 2021 · 16 comments · Fixed by #4823
Closed

Robert Wilton's QPACK Comment 3 #4802

LPardue opened this issue Jan 21, 2021 · 16 comments · Fixed by #4823
Labels
-qpack design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. iesg An issue raised during IESG review. proposal-ready An issue which has a proposal that is believed to be ready for a consensus call.
Milestone

Comments

@LPardue
Copy link
Member

LPardue commented Jan 21, 2021

@rgwilton said

Section 7.4 talks about implementation limits, but it wasn't obvious to me how
a receiver is expected to behave if one of those limits is exceeded. Further
in the case of strings, there seems to be a simple upper bound on the maximum
size of the string of the table size.

@LPardue LPardue added -qpack iesg An issue raised during IESG review. labels Jan 21, 2021
@LPardue LPardue added this to the qpack-iesg milestone Jan 21, 2021
@LPardue LPardue added this to Triage in Late Stage Processing via automation Jan 21, 2021
@afrind
Copy link
Contributor

afrind commented Jan 26, 2021

Section 3.2.2 mentions that it is an error to attempt to insert an entry larger than the table size:

It is an error if the encoder attempts to add an entry that is larger than the dynamic table capacity; the decoder MUST treat this as a connection error of type QPACK_ENCODER_STREAM_ERROR.

I can't actually find a place where the document says that if you fail to decode an instruction or reference (for any reason, which would include exceeding implementation limits) that it's an error. I think we might need one.

@afrind
Copy link
Contributor

afrind commented Jan 26, 2021

Actually, section 6 says:

QPACK_DECOMPRESSION_FAILED (0x200):
The decoder failed to interpret an encoded field section and is not able to continue decoding that field section.
QPACK_ENCODER_STREAM_ERROR (0x201):
The decoder failed to interpret an encoder instruction received on the encoder stream.
QPACK_DECODER_STREAM_ERROR (0x202):
The encoder failed to interpret a decoder instruction received on the decoder stream.

So "failing to interpret" anything results in an error. I think this includes being unable to decode an integer or string because of implementation limits.

Is this sufficient or should we add something a bit more explicit?

@rgwilton
Copy link

If section 3.2.2 already imposes a limit on the length of an entry, then it would be good to referenced that text from section 7.4.

E.g. section 7.4 states: In the same way, it has to set a limit to the length it accepts for string literals; see Section 4.1.2.

But I suspect that really the only limit that is needed here is the one in 3.2.2? I.e. it seems reasonable that a receiver should accept any string up to the maximum table size?

But my other point is that it suggests that a receiver can arbitrarily choose to set a limit on the size of integers that it receives, but section 4.1.1 states: QPACK implementations MUST be able to decode integers up to and including 62 bits long.

So, I guess that I'm really suggesting that this paragraph (in section 7.4):

An implementation has to set a limit for the values it accepts for integers, as well as for the encoded length; see Section 4.1.1. In the same way, it has to set a limit to the length it accepts for string literals; see Section 4.1.2.

is reworded to say something along the lines of:

  • Receivers MUST accept integers up to 62 bits long section 4.1.1, but MAY choose to not handle larger integers, in which case they return QPACK_DECOMPRESSION_FAILED.
  • Receivers MUST accept any strings up to maximum table length -32, or return QPACK_DECOMPRESSION_FAILED.

It wasn't clear to me whether integers bigger than 62 bits are ever likely in practice, I presume not ...

@dtikhonov
Copy link
Member

A decoder that actually counts insertions (instead of using modulo arithmetic) will likely fail when the integer value it uses overflows.

Of course, one does not have to use uint32_t or uint64_t for the count. To avoid an overflow, there are math libraries that support arbitrarily large numbers.

@MikeBishop
Copy link
Contributor

The text is really attempting to say that an implementation MUST NOT continue to process to infinity; bounds must exist for basic sanity. What those bounds will be is implementation-dependent. Saying that they're limited to the table size is only true on the encoder stream; request streams can have string literals greater than the table size (consider the degenerate case of zero).

HTTP limits could be pushed down to QPACK such that the library will refuse to decode a larger string than the implementation would accept; I don't think that's necessarily required though.

@rgwilton
Copy link

I don't think that using arbitrarily sized integer libraries is the answer here due to the performance overheads of such libraries.

My concerns are two fold:

(1) It is unclear what arbitrary limit receivers should apply. E.g., is 255 bytes enough for a string? Is a 64 bit signed integer enough for an integer?

(2) Senders probably won't really know that the reason the stream has failed is because they are hitting some arbitrary limit in the receiver, and even if they did then I'm not sure there is anything that they can do about it.

Hence, another suggestion is to specifying minimum expected bounds for these values? Or if these bounds are effectively provided by the HTTP spec, perhaps the appropriate section in that spec. could be referenced.

@larseggert
Copy link
Member

Editors, could we come to a conclusion here?

@afrind
Copy link
Contributor

afrind commented Jan 28, 2021

There is already a requirement that implementations be able to decode 62 bit integers. Anything larger is really unnecessary -- we intentionally encode the "Required Insert Count" on the wire in a way that bounds its size to be much smaller practically. The requirement stems from the need to decode QUIC stream IDs. I think @dtikhonov's comment about bignums is about what an implementation might do if it overflowed its internal representation decoding the Required Insert Count. This is not a practical concern, and can be completely worked around with a "clever" implementation that is worried about more than 2^64 table insertions.

HTTP also provides a setting to inform the peer of a maximum size of a field section (MAX_FIELD_SECTION_SIZE -- see HTTP/3 section 4.1.1.3), which is also a practical upper bound for any single QPACK string for HTTP/3. Perhaps the QPACK doc could provide this hint? I'll draft a PR to this effect.

@martinthomson
Copy link
Member

FWIW, exceeding 2^60 insertions would be a tall order given data limits in QUIC; a 64-bit integer is definitely enough space for any size or length field. Implementations can use smaller values, probably reliably. There is some need for care in those cases, but I don't see the spec as bearing any responsibility for that.

@afrind, remember that MAX_FIELD_SECTION_SIZE is advisory, so I wouldn't go so far as to say "practical".

@larseggert
Copy link
Member

So, do we have a proposal on this one?

@MikeBishop
Copy link
Contributor

@afrind, remember that MAX_FIELD_SECTION_SIZE is advisory, so I wouldn't go so far as to say "practical".

Every (sane) implementation has a limit; what's advisory is informing your peer about your limit before they hit it, or enforcing downstream limits to short-circuit errors. Any implementation could absolutely tell the decoder not to bother decoding anything larger than what they will process and just return an error.

@afrind
Copy link
Contributor

afrind commented Feb 1, 2021

I'm trying to address Robert's second point:

(2) Senders probably won't really know that the reason the stream has failed is because they are hitting some arbitrary limit in the receiver, and even if they did then I'm not sure there is anything that they can do about it.

The mechanism to address that in HTTP/3 is with the advisory setting, though it's slightly different in that the setting applies to the entire field section, and it's conceivable an implementation could have another, smaller limit for any individual field name or value. It's easier to pretend that isn't the case.

For these reasons, I stumbled a bit on the exact wording of the text last week and ran out of time because of other commitments.

I'm thinking something along the lines of

"an implementation really ought to be able to decode a string as long as their advisory setting"

and/or

"if an implementation has a limit on the maximum size of string it can decode, it would be really nice to inform the peer about it with the advisory setting".

Is that reasonable? Or am I barking up the completely wrong tree?

@rgwilton
Copy link

rgwilton commented Feb 2, 2021

If HTTP/3 already sets practical limits on what the QPACK needs to accept then it is fine to say that.

Or to put this another way, the HTTP/3 implementations presumably are setting limits (or perhaps they are just relying on language/decoder limits). How are they deciding what those limits should be? How can they be sure that they will interoperate with all senders?

I'm not an HTTP expert, so perhaps I'm missing something obvious here.

@MikeBishop
Copy link
Contributor

HTTP (not HTTP/3) has historically not communicated those limits, which are implementation-dependent. You just find out if you hit them -- status codes 413 (Content Too Large) and 414 (URI Too Long) cover these, but some implementations will also send a 400 out of discretion.

HTTP/2 introduced a mechanism to inform the peer what your limit is (SETTINGS_MAX_HEADER_LIST_SIZE), which lets the failure occur further downstream and saves everyone effort -- a client or downstream proxy can fail a too-large request on behalf of the server before actually transferring it, rather than discovering the limit only after the server rejects it. It's optional, though; some endpoints prefer not to disclose their limits to avoid giving information to attackers, and some endpoints don't know how big the message is going to be when it starts processing it. HTTP/3 maintains parity with that (SETTINGS_MAX_FIELD_SECTION_SIZE) and the feature is similarly optional.

Regardless of whether they're communicated, though, sane implementations have a limit. In at least some implementations, this limit is configurable, so it can be increased in situations where you have mostly-trusted clients and a known usecase for very large headers or URIs. That's not a protocol element; I don't think we can reasonably say much more than that the QPACK library SHOULD/MUST be able to process the largest individual header or header section that its HTTP implementation can be configured to accept.

@rgwilton
Copy link

rgwilton commented Feb 2, 2021

Mike, thanks for the additional context.

Stating something along the lines of the following would be fine with me:

  • sane implementations have a limit (and what they do if that limit is reached), and
  • the QPACK library SHOULD/MUST be able to process the largest individual header or header section that its HTTP implementation can be configured to accept.

@larseggert larseggert added the design An issue that affects the design of the protocol; resolution requires consensus. label Feb 2, 2021
@project-bot project-bot bot moved this from Triage to Design Issues in Late Stage Processing Feb 2, 2021
Late Stage Processing automation moved this from Design Issues to Issue Handled Feb 2, 2021
@afrind afrind reopened this Feb 2, 2021
Late Stage Processing automation moved this from Issue Handled to Triage Feb 2, 2021
@afrind afrind added the proposal-ready An issue which has a proposal that is believed to be ready for a consensus call. label Feb 2, 2021
@project-bot project-bot bot moved this from Triage to Consensus Emerging in Late Stage Processing Feb 2, 2021
@LPardue LPardue added the call-issued An issue that the Chairs have issued a Consensus call for. label Feb 3, 2021
@project-bot project-bot bot moved this from Consensus Emerging to Consensus Call issued in Late Stage Processing Feb 3, 2021
@LPardue LPardue added has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. and removed call-issued An issue that the Chairs have issued a Consensus call for. labels Feb 21, 2021
@project-bot project-bot bot moved this from Consensus Call issued to Consensus Declared in Late Stage Processing Feb 21, 2021
@LPardue
Copy link
Member Author

LPardue commented Feb 21, 2021

Closing this now that the IESG have approved the document and it's with the RFC editor.

@LPardue LPardue closed this as completed Feb 21, 2021
Late Stage Processing automation moved this from Consensus Declared to Issue Handled Feb 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-qpack design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. iesg An issue raised during IESG review. 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

Successfully merging a pull request may close this issue.

7 participants