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

Tolerate unknown stream types, add greasing #1525

Merged
merged 5 commits into from Jul 6, 2018
Merged

Conversation

MikeBishop
Copy link
Contributor

Fixes #1490.

@MikeBishop MikeBishop added design An issue that affects the design of the protocol; resolution requires consensus. -http labels Jul 5, 2018
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me.

the peer's discretion (see {{errors}}).
known to support. Recipients of unknown stream types MAY trigger a QUIC
STOP_SENDING frame with an error code of HTTP_UNKNOWN_STREAM_TYPE, but MUST NOT
consider such streams to be an error of any kind.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to point out that these cannot include QPACK header blocks unless that is explicitly negotiated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Borrowed the extension language that you MUST NOT send anything that modifies the state of the core protocol, QPACK, or any other extension unless you know the peer supports it. Do you think that's sufficient?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah.


Stream types of the format `0x1f * N` are reserved to exercise the requirement
that unknown types be ignored. These streams have no semantic meaning, and can
be sent when application-layer padding is desired. They MAY also be sent on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by application-layer padding in this case? If this is some property of the mapping that can be used to add additional padding to QUIC's PADDING frames then is it worth pointing out in security considerations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps. HTTP/2 allowed padding on the HEADERS and DATA frames, to make analysis of the traffic more difficult. QUIC can pad individual packets to different / uniform sizes, but I'm assuming that most stacks won't generate traffic where none exists. An application layer can make analysis more difficult by sending traffic even when no requests are happening.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so I get the problem, this is more like a background level of noise.

I guess I had been expecting transport libraries to handle packing and padding with some influence from the application. It wouldn't be a stretch for them to provide an API that would allow the caller to declare a background level of traffic, however the transport won't have any stream to send this on.

In that case, I like the side effect of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a mention in Security Considerations.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Padding using bogus frames might work, but there are two problems with this:

  1. Not all streams use frames.
  2. Padding that uses bogus unidirectional streams isn't guaranteed to be placed in the same packet as the data that it pads.

@LPardue
Copy link
Member

LPardue commented Jul 6, 2018

LGTM

Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments

streams to be an error of any kind.

Implementations SHOULD NOT send stream types the peer is not already known to
support. Stream types which could modify the state or semantics of existing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This text recommends (SHOULD NOT) negotiating supported stream types. Is this recommendation necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recommendation is purely because if you send types that aren't supported, you're wasting bandwidth. It's not an interop issue, so perhaps that doesn't rise to the level of SHOULD NOT. Would "MAY send" be more affirming?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to make that change it would make sense to add the sentence from this paragraph to the previous one e.g.

Implementations MAY send stream types the peer is not already known to support. If the recipient does not support a stream type, the remainder of the stream cannot be consumed as the semantics are unknown. (optional: add comment about wasted bandwidth)...

@@ -1651,6 +1670,18 @@ The entries in the following table are registered by this document.
| Push Stream | 0x50 | {{server-push}} | Server |
| ---------------- | ------ | -------------------------- | ------ |

Additionally, each code of the format `0x1f * N` for values of N in the
range (0..8) (that is, `0x00`, `0x1f`, etc., through `0xf8`), the following
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's list them all; it's only 8 values. ('0x00', '0x1f', '0x3e', '0x5d', '0x7c', '0x9b', '0xba', '0xd9', '0xf8').

@MikeBishop MikeBishop merged commit 346f371 into master Jul 6, 2018
@martinthomson martinthomson deleted the http_unidirectional branch July 9, 2018 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-http design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants