1026609 - http/2 draft 13

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[u408661]

http://tools.ietf.org/html/draft-ietf-httpbis-http2-13
http://tools.ietf.org/html/draft-ietf-httpbis-header-compression-08

Marking this as blocking http2 release, since this will (hopefully) be the wglc version of the spec.

Comment 1

[u408661]

Attachment: part 1 - client implementation

Comment 2

[u408661]

Attachment: part 2 - tests

Comment 3

[u408661]

https://tbpl.mozilla.org/?tree=Try&rev=8931eea3ca9f

Comment 4

[u408661]

Tested with nghttp2 locally, as well as node-http2 updates both in unit tests and in an independent test, everything seems ok so far :)

Comment 5

[Dave Garrett]

http://tools.ietf.org/html/draft-ietf-httpbis-http2-13#appendix-A.1

Draft 13 restricts TLS cipher suites to AEAD. Amend Http2Session::ConfirmTLSProfile() to enforce this?

http://tools.ietf.org/html/draft-ietf-httpbis-http2-13#section-9.2.2
> HTTP MUST NOT be used with cipher suites that
> use stream or block ciphers.  Authenticated Encryption with
> Additional Data (AEAD) modes, such as the Galois Counter Model (GCM)
> mode for AES [RFC5288] are acceptable.

Comment 6

[u408661]

(In reply to Dave Garrett from comment #5)
> http://tools.ietf.org/html/draft-ietf-httpbis-http2-13#appendix-A.1
> 
> Draft 13 restricts TLS cipher suites to AEAD. Amend
> Http2Session::ConfirmTLSProfile() to enforce this?

Indeed it does. That's a bigger bit of work to expose the cipher suite when we need it, and it makes sense (in my opinion) to move forward with the main bits of the protocol and get that in later (which is what we've done with other TLS restrictions in http/2 that required more work than the main protocol bits)

Comment 7

[u408661]

Argh, existing patch has a compile error on linux. New patch incoming.

Comment 8

[u408661]

Attachment: part 1 - client implementation

https://tbpl.mozilla.org/?tree=Try&rev=c1ba3e0b78b1

Comment 9

[Patrick McManus [:mcmanus]]

(In reply to Nicholas Hurley [:hurley] from comment #6)
> (In reply to Dave Garrett from comment #5)
> > http://tools.ietf.org/html/draft-ietf-httpbis-http2-13#appendix-A.1
> > 
> > Draft 13 restricts TLS cipher suites to AEAD. Amend
> > Http2Session::ConfirmTLSProfile() to enforce this?
> 
> Indeed it does. That's a bigger bit of work to expose the cipher suite when
> we need it, and it makes sense (in my opinion) to move forward with the main
> bits of the protocol and get that in later (which is what we've done with
> other TLS restrictions in http/2 that required more work than the main
> protocol bits)

file the followup and tag it http2release - we'll want to do it nowish rather than downtheroadish.

Comment 10

[Adrian Cole]

non-technical nudge. I'm headed to uberconf and have 2 90m sessions on http/2 this coming weds, planning to use FF for one of them. It would be super awesome if this could land so that the attendees could use FF against twitter which is on draft 13 as of this morning.

Comment 11

[Patrick McManus [:mcmanus]]

Comment on attachment 8442446 [details] [diff] [review]
part 1 - client implementation

Review of attachment 8442446 [details] [diff] [review]:
-----------------------------------------------------------------

right now we just rst_stream a push promise when we have push disabled.. but I would like to protocol_error the session when it can be done reliably. I only worry about race conditions - so we need to track a settings ack to set a bool when we get the ack for the disable push promise setting. can we add that into a separate bug but do it soon?

I was checking on the extensibility stuff and I found a log of "unknow frame type" - can you fix my typo in this patch set?

::: netwerk/protocol/http/Http2Session.cpp
@@ +777,5 @@
>  {
>    MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
>    LOG3(("Http2Session::SendHello %p\n", this));
>  
> +  // sized for magic + 4 settings and a session window update to follow

3 settings, right?

@@ +812,5 @@
>  
>    // Advertise the Push RWIN for the session, and on each new pull stream
>    // send a window update with END_FLOW_CONTROL
> +  CopyAsNetwork16(packet + 8 + 6 * numberOfEntries, SETTINGS_TYPE_INITIAL_WINDOW);
> +  CopyAsNetwork32(packet + 8 + 6 * numberOfEntries + 2, mPushAllowance);

+ (6 * numberOfEntries) + 2 .. just for clarity

@@ +1307,1 @@
>      LOG3(("Settings ID %d, Value %d", id, value));

you can make those formatters unsigned if you get a chance

::: netwerk/protocol/http/Http2Session.h
@@ -119,1 @@
>    const static uint8_t kFlag_COMPRESSED = 0x20; // data

we removed FLAG_COMPRESSED, so it can be removed from the header and the error handler in the cpp

Comment 12

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #11)
> right now we just rst_stream a push promise when we have push disabled.. but
> I would like to protocol_error the session when it can be done reliably. I
> only worry about race conditions - so we need to track a settings ack to set
> a bool when we get the ack for the disable push promise setting. can we add
> that into a separate bug but do it soon?

Bug 1030203 has been filed. For the rest of your comments, a new patch will be incoming (likely after lunch, when I have more free time).

Comment 13

[u408661]

Attachment: part 1 - client implementation

All comments fixed!

New try run just because platforms other than os x debug seem to like to yell at me when I mess up something to do with signed/unsigned integers :)

https://tbpl.mozilla.org/?tree=Try&rev=0ee90b2e9a48

Comment 14

[Patrick McManus [:mcmanus]]

Comment on attachment 8446021 [details] [diff] [review]
part 1 - client implementation

Review of attachment 8446021 [details] [diff] [review]:
-----------------------------------------------------------------

keep the r+ if osx makes you rearrange the formatters :)

Comment 15

[u408661]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/838a3af27397
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bd8457579ad5

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/838a3af27397
https://hg.mozilla.org/mozilla-central/rev/bd8457579ad5