Closed Bug 1026609 Opened 6 years ago Closed 6 years ago

http/2 draft 13

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: u408661, Assigned: u408661)

References

Details

(Whiteboard: [spdy] [http2release])

Attachments

(2 files, 2 obsolete files)

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.
Attached patch part 1 - client implementation (obsolete) — Splinter Review
Attachment #8442372 - Flags: review?(mcmanus)
Attached patch part 2 - testsSplinter Review
Attachment #8442373 - Flags: review?(mcmanus)
Tested with nghttp2 locally, as well as node-http2 updates both in unit tests and in an independent test, everything seems ok so far :)
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.
(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)
Argh, existing patch has a compile error on linux. New patch incoming.
Attached patch part 1 - client implementation (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=c1ba3e0b78b1
Attachment #8442372 - Attachment is obsolete: true
Attachment #8442372 - Flags: review?(mcmanus)
Attachment #8442446 - Flags: review?(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.
Depends on: 1027720
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 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
Attachment #8442446 - Flags: review?(mcmanus) → review-
Attachment #8442373 - Flags: review?(mcmanus) → review+
(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).
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
Attachment #8442446 - Attachment is obsolete: true
Attachment #8446021 - Flags: review?(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 :)
Attachment #8446021 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/mozilla-central/rev/838a3af27397
https://hg.mozilla.org/mozilla-central/rev/bd8457579ad5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.