Subject: Alissa Cooper's Yes on draft-ietf-httpbis-http2-16: (with COMMENT)

From: "Alissa Cooper" <[email-masked]>
Date: Tue, 20 Jan 2015 13:54:48 -0800

Alissa Cooper has entered the following ballot position for
draft-ietf-httpbis-http2-16: Yes

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to http://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
http://datatracker.ietf.org/doc/draft-ietf-httpbis-http2/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Thanks for all of your work on this. I'm not thrilled with the decisions
around TLS and opportunistic security, but I understand how we got here.

= Section 3.1 =
It might be worth keeping this sentence from the text that is marked for
deletion:

"Examples and text throughout the rest of this document use "h2" as a
   matter of editorial convenience only."

= Section 3.2 =
"A server that supports HTTP/2 can accept the upgrade with a 101
   (Switching Protocols) response."

Is there a reason why this behavior is not normatively described? Is
there some other response that clients should expect that has the same
semantic?

= Section 5.1 =
"WINDOW_UPDATE or RST_STREAM frames can be received in this state
      for a short period after a DATA or HEADERS frame containing an
      END_STREAM flag is sent. ...
      endpoints MAY choose to treat frames that arrive a significant
      time after sending END_STREAM as a connection error
      (Section 5.4.1) of type PROTOCOL_ERROR."

Can some ballpark guidance be given about how long "a short period" and
"a significant time" are expected to be? Or how an implementation might
calibrate these values?

s/Frame of unknown types/Frames of unknown types/

= Section 5.2.2 =
"Deployments that do not require this capability can advertise a flow
   control window of the maximum size, incrementing the available space
   when new data is received."

I found this a little vague. Does this mean deployments can advertise a
flow control window of size 2^31-1 and then locally increment available
memory as new data is received? Would be good to state here both what the
maximum size is and what is meant by "available space."

= Section 6.8 =
s/the remote can know/the remote peer can know/

= Section 8.2 =

s/that is not cacheable, unsafe or that/that is not cacheable, safe or
that/

= Section 10.7 =
"Intermediaries SHOULD retain padding for DATA frames, but MAY drop
   padding for HEADERS and PUSH_PROMISE frames.  A valid reason for an
   intermediary to change the amount of padding of frames is to improve
   the protections that padding provides."

The different recommendations for different frame types here are a bit
puzzling. Why are intermediaries expected to be able to improve
padding-based protection for HEADERS and PUSH_PROMISE frames more so than
for DATA frames?

= Section 10.8 =

Is there anything more you could say about possible mitigations for the
fingerprinting issue? Or there might not be, absent some coordination
between endpoints, which would likewise be useful to note.

---

From: Martin Thomson <[email-masked]>
Date: Tue, 20 Jan 2015 16:36:45 -0800

Thanks Alissa, I have a few updates here:

https://github.com/http2/http2-spec/pull/698

I haven't done anything about a couple of your trickier comments.  If
you think that the explanations I've given suggests a need for text,
I'll see if I can work something out.

>    matter of editorial convenience only."

Will do.

> there some other response that clients should expect that has the same

Mainly because trying to be normative here risks conflicting with
RFC7230, which we rely on for the real normative statements.  This is
really just a statement of fact at this point.

> "a significant time" are expected to be? Or how an implementation might

This is tricky; a good implementation will maintain shallow send
buffers and so be able to respond to changes in state fairly quickly.
In that case, one RTT is enough.  However, keeping a buffer protects
against local hiccups and as soon as you buffer anything, you are
potentially subject to long stalls if packet loss or other problems
occur.

Maybe "significant" is 1RTT plus a complete CWND worth of received
data.  Even then I think that some implementations could buffer more
than that to allow them to encipher larger blocks.

> s/Frame of unknown types/Frames of unknown types/

I think you might be the 4th person to notice that gaffe.  That's how we roll :)

> maximum size is and what is meant by "available space."

Would "advertise a flow control window of the maximum size (2^31-1),
and by maintaining this by sending a WINDOW_UPDATE frame when any data
is received."

> s/the remote can know/the remote peer can know/

First!  Thanks.

> s/that is not cacheable, unsafe or that/that is not cacheable, safe or

Second.  Julian noted the same flaw.  We currently have:

+ that is not cacheable, is not known to be safe or that indicates the
+ presence of a request body

-- https://github.com/http2/http2-spec/pull/687/files

> padding-based protection for HEADERS and PUSH_PROMISE frames more so than

Noting that both are merely soft requirements; the available
protections are different.  The expectation is that header fields are
protected differently: HPACK defines a never-indexed encoding for
headers that is intended to be used outside of this.  An intermediary
is also highly likely to alter header fields and therefore needs to be
more intimately aware of the security consequences of altering length.

Do you think that a short note would help?

> between endpoints, which would likewise be useful to note.

Without completely eliminating product differentiation at this
protocol layer, I think we're basically hosed.  Though the number of
new bits this exposes is probably relatively low.   I was reluctant to
put that in an RFC though.

I expect it to be possible to cut the exposure fairly easily for
instances of the same implementation (i.e., the Tor bundle) by
ensuring consistent behaviour and by adding a little noise to the
various signals (bucketing send times might help, for instance).
That's more of a research subject though.

---

From: Amos Jeffries <[email-masked]>
Date: Wed, 21 Jan 2015 18:00:56 +1300

On 21/01/2015 1:36 p.m., Martin Thomson wrote:
> I haven't done anything about a couple of your trickier comments.
> If you think that the explanations I've given suggests a need for
> text, I'll see if I can work something out.
> 
> On 20 January 2015 at 13:54, Alissa Cooper wrote:
> 
>> = Section 3.1 = It might be worth keeping this sentence from the
>> text that is marked for deletion:
>> 
>> "Examples and text throughout the rest of this document use "h2"
>> as a matter of editorial convenience only."
> 
> Will do.

Is that encompassing the ALPN token registration then?

Perhapse just replace "h2" with "HTTP/2" when the time comes to remove
that text (now in LC?). Or formally noting "h2" as an acceptible
abbreviation to represent HTTP/2.

Amos

---

From: Martin Thomson <[email-masked]>
Date: Wed, 21 Jan 2015 10:41:28 -0800

> Is that encompassing the ALPN token registration then?

Actually, I incorrectly thought there were examples that use a generic
ALPN token (i.e., places where either could be used).  There are not.
Those are all AltSvc things.  I will remove the note.

---

From: Greg Wilkins <[email-masked]>
Date: Fri, 23 Jan 2015 12:13:38 +0100

> than that to allow them to encipher larger blocks.

As I've said before, I think we really need to do more to simplify the
CLOSE state, as 7 paragraphs to describe the handling of the terminal state
is too complex.

Currently it is as if the TCP RFC left TIME_WAIT and CLOSE_WAIT as an
implementation specific exercise. It is a very common problem for OS's to
have issues with large numbers of sockets left in TIME_WAIT, so I can
similar issues with servers keeping stream in memory in
CLOSE_before_the_undefined_short_period.

We could introduce a RESET state, which means that CLOSE will always be
entered gracefully and there will be no late packets.   RESET could then
have defined timeout before entering CLOSE.

Alternately, we could just define all the frame handling in CLOSE and let
that take place for all time so that CLOSE is not a stateful state.   It is
mostly ignore, other that PP which is reset.

-- 
Greg Wilkins <[email-masked]>  @  Webtide - *an Intalio subsidiary*
http://eclipse.org/jetty HTTP, SPDY, Websocket server and client that scales
http://www.webtide.com  advice and support for jetty and cometd.

---

From: Greg Wilkins <[email-masked]>
Date: Fri, 23 Jan 2015 12:26:46 +0100

> have defined timeout before entering CLOSE.

Actually, if we made RESET an acknowledged frame, then we could avoid the
timeout completely as once you have both sent and received a RESET you can
enter close gracefully.

I know it is late to be hammering on this bugbear, but I'll prepare a PR
none the less.

cheers

---

From: Greg Wilkins <[email-masked]>
Date: Fri, 23 Jan 2015 14:27:29 +0100

Here is a PR for a reset state:
https://github.com/http2/http2-spec/pull/700/files?diff=split

All the conditional behaviour within the close state indicates that it is
really multiple states, so this pull request moves the conditional
behaviour to the new "reset" state.  Then rather than use "a short period"
to end the conditional behaviour, I have made the RST_STREAM be
acknowledged so we don't have to guess when the peer has seen the reset.

---

From: Greg Wilkins <[email-masked]>
Date: Fri, 23 Jan 2015 15:39:46 +0100

I've remade the PR as the branch I originally used had some reverted
commits in it.  Here it is again with some corrections from Scottmitch

https://github.com/http2/http2-spec/pull/701

---

From: Nicholas Hurley <[email-masked]>
Date: Fri, 23 Jan 2015 08:33:47 -0800

On Fri, Jan 23, 2015, at 03:26, Greg Wilkins wrote:
>> We could introduce a RESET state, which means that CLOSE will always
>> be entered gracefully and there will be no late packets. RESET could
>> then have defined timeout before entering CLOSE.
>
> Actually, if we made RESET an acknowledged frame, then we could avoid
> the timeout completely as once you have both sent and received a RESET
> you can enter close gracefully.
>
> I know it is late to be hammering on this bugbear, but I'll prepare a
> PR none the less.

Well, yes, you're right there, it absolutely is too late to be
hammering on this. Not only are you just moving complexity from one
state to another (that actually means the same thing, with a different
name), you're adding more normative text and changing the behavior of
the protocol. Given that we're past WGLC and well into dealing with
the IESG's feedback, I don't see how adding a "rst ack" could possibly
have any chance of meeting the high bar for changes to be accepted at
this point.

I'm a solid -1 on this, and I'm confident in saying there is no argument
that will change my mind on this one.
--
Peace, -Nick

---

From: Greg Wilkins <[email-masked]>
Date: Fri, 23 Jan 2015 18:20:51 +0100

On 23 January 2015 at 17:33, Nicholas Hurley <[email-masked]> wrote:

> Well, yes, you're right there, it absolutely is too late to be hammering
> on this. Not only are you just moving complexity from one state to another
> (that actually means the same thing, with a different name),
>

A state that has behaviour that is conditional on the the entry into the
state is not one state.   A state that changes behaviour after a timeout is
not one state.  As I have frequently said, this state machine is a fantasy
that does not represent reality.     All implementations are going to have
a state change within closed state as they expire the undefined timeout and
change behaviour.

you're adding more normative text and changing the behavior of the
> protocol. Given that we're past WGLC and well into dealing with the IESG's
> feedback, I don't see how adding a "rst ack" could possibly have any chance
> of meeting the high bar for changes to be accepted at this point.
>

All true and good process arguments for not making this good technical
change.     But I'd say it is a pretty fundamental failure of the process
that we have such a broken state machine this late in the process.

> that will change my mind on this one.

What if I split the PR into two parts.

The first would simply define the reset state and the behaviour associated
with it.   The "a short period" timeout would then be used to move to
closed state.    This would not change the wire protocol, but would improve
the description of the current behaviour.

The second part would be the "rst ack", which we can put on hold until a
subsequent iteration

---

From: Nicholas Hurley <[email-masked]>
Date: Fri, 23 Jan 2015 09:55:04 -0800

On Fri, Jan 23, 2015, at 09:20, Greg Wilkins wrote:
>> state to another (that actually means the same thing, with a
>> different name),
>
> A state that has behaviour that is conditional on the the entry into
> the state is not one state. A state that changes behaviour after a
> timeout is not one state. As I have frequently said, this state
> machine is a fantasy that does not represent reality. All
> implementations are going to have a state change within closed state
> as they expire the undefined timeout and change behaviour.

If we accept your assertion that the state machine is a fantasy that
doesn't represent reality, then there are likely a lot more changes that
should be made to the state machine than just this one, and I haven't
seen anyone making PRs for other state machine changes.

I don't accept that assertion, though. I think the state machine
represents reality just as much as any protocol state machine you see
does - there are *always* hidden details in the way things work. It's
not a failure of this WG, though, that's just the way of the world.

>> protocol. Given that we're past WGLC and well into dealing with the
>> IESG's feedback, I don't see how adding a "rst ack" could possibly

I would argue that the amount of interoperability we have going on
here indicates that the state machine is far from broken. Large and
complex (as most protocol state machines are), perhaps, but definitely
not broken.

>> I'm a solid -1 on this, and I'm confident in saying there is no
>> argument that will change my mind on this one.
>
> The first would simply define the reset state and the behaviour
> associated with it. The "a short period" timeout would then be used to
> move to closed state. This would not change the wire protocol, but
> would improve the description of the current behaviour.
>
> The second part would be the "rst ack", which we can put on hold until
> a subsequent iteration

This would remain a -1 from me. The reset state in this separated PR in
fact adds complexity to the state machine in the form of more states.
(And, if the state machine is so horribly broken as you assert, then
making this one change isn't going to fix the horrible brokenness.)
Furthermore, it forces changes on implementations that explicitly
implement the state machine as a state machine, which does, in fact,
invalidate interop testing (even though the protocol on the wire doesn't
change) - not something we should be doing this late in the game (unless
it meets the aforementioned incredibly high bar.)
--
Peace, -Nick

---

From: Greg Wilkins <[email-masked]>
Date: Fri, 23 Jan 2015 21:50:21 +0100

On 23 January 2015 at 18:55, Nicholas Hurley <[email-masked]> wrote:

> I would argue that the amount of interoperability we have going on here
> indicates that the state machine is far from broken. Large and complex (as
> most protocol state machines are), perhaps, but definitely not broken.

The interoperability has been for the most part not on this part of the
state machine.   This part of the state machine has probably hardly been
tested outside of unit tests, as I don't think many connections will be
testing the "short period" of frame handling on the other side of a reset.

Getting protocols to work is the easy part.  Getting them to fail nicely is
the hard part.

I fail to see how adding the reset state adds complexity to implementations
that are already implementing the "short period" behavioural change.   It
will only add complexity to those that are not implementing that part of
the spec, which is exactly the sort of interoperability problem that would
be good to solve.