Implement draft 10 of the http2 spec

[alekstorm]

Besides the frame/settings renumbering, the biggest single change is the new DATA frame padding format.

Unfortunately, it's currently impossible to fully conform to the v10 spec with solely the ssl standard library module at our disposal, so merging this should probably wait until a pyOpenSSL compatibility layer can be written that exposes the necessary TLS options.

[Lukasa]

You're doing awesome work, @alekstorm, thankyou!

I'll review this as well, but your concerns about TLS are well-founded. I think what I'll do is, when we're happy, merge this code to a new branch (not development). We can therefore keep track of the work you've done here and make it my job not to break it, rather than your job to fix it if I do.

I'm also going to warn you that I plan to merge the Python 2.7 support first, so you may need to rebase this pull request once I do that.

[Lukasa]

Can we get a docstring on this, just to explain its purpose?

[Lukasa]

This is also probably more clearly expressed as a bitshift, e.g. (high << 8) + low.

[Lukasa]

Some other notes that you'll want to consider and probably work on:

• Padding is included in flow control, but you haven't updated the flow control logic to account for it. This doesn't matter when we send frames since we never use padding on them, but the incoming frame logic needs updating.
• If we're holding off until PyOpenSSL/PEP 466 support for TLS1.2 we should stop using the SSLv23 handshake and instead require TLSv12.
• There's some MUSTs about ephemeral cipher suites in the new spec that are potentially going to cause us some fun, though I think PyOpenSSL and PEP 466 should cover us.

[alekstorm]

Rebased on py27 and added the SSL compatibility layer (see hyper/ssl_compat.py). Since this PR is feature-complete, I'm removing the "WIP" designation from the title (whether it meets code review standards is, of course, another question). Will finish up other review comments tomorrow (or tonight, from your viewpoint in London :)).

[Lukasa]

Oh man, I missed this, but we're technically not feature complete unless we also move to HPACK draft 6. Don't rush to implementing it, I'm happy to do it tonight, but if you feel like doing it you're of course welcome to.

[Lukasa]

Ok, rebasing onto py27 makes this challenging to review, so I'll hold off reviewing until we've merged py27.

[alekstorm]

Did you start on the HPACK update? If not, I'd be more than willing to take care of it; I just don't want to duplicate work.

[Lukasa]

I've started on it. Don't expect it to move quickly though. =)

[Lukasa]

Ok, done. =)

[Lukasa]

Don't forget to rebase against the new development branch. =D

[alekstorm]

Done.

[Lukasa]

Might be worth examining what Requests does here. =)

[alekstorm]

Just realized padding was also added to the HEADERS and CONTINUATION frames (the changelog in the spec only mentions DATA, for some reason). Updating now.

[Lukasa]

No update should be required for HEADERS at least.

[Lukasa]

Sorry, only minor change: flag types should be enough.

[Lukasa]

A couple of notes here. We can't say this a test requirement because it's a requirement on 2.7. We'll need custom code in setup.py to make it a Python-version-specific dependency. I'm going to recommend having a function in setup.py called resolve_additional_dependencies, because I want to add pypy support as well and I'd like to keep those concerns encapsulated.

Secondly, that version number is wrong (as NPN isn't in PyOpenSSL yet).

[Lukasa]

Oh, I just noticed it's also a requirement on 3.3 because 3.3 doesn't have PROTOCOL_TLSv1_2 in the ssl module.

[alekstorm]

Okay, I've moved pyOpenSSL into the extra_requires parameter in setup.py (is that what you wanted?). I've also added logic to make it a soft dependency, which required enabling non-HTTPS URLs - however, there appears to be some race condition in the tests that causes the client-side socket.recv() call to time out in a randomly-chosen test (it gets through all of test_integration.py about a third of the time) unless the socket is wrapped in an SSL context. Might need some help solving that one.

Still have a few other review comments to take care of; should finish that up soon.

As for the TODO comments, I suppose they can be taken care of on a case-by-case basis, since this is an (exceptionally well-tested) alpha project. For background, I generally use TODO to mark code that should simply be revisited later, and FIXME to mark code that shouldn't be merged in its present state (hence, you should only see FIXMEs in WIP PRs).

[alekstorm]

Looks like properly supporting the DHE/ECDHE cipher suites is impossible until pyca/pyopenssl#82 (or whatever its successor is) gets merged, so I suppose the only action item left (correct me if I'm wrong) is fixing the sporadically-broken tests.

[Lukasa]

Ah, yes, sorry about that, I failed to adequately communicate what I meant. =D

I don't want to support plaintext HTTP/2 until we have Upgrade working. In the meantime, we should be mandating TLS HTTP/2. What I wanted for PyOpenSSL was to add it to the install_requires list. This list would vary based on platform (as you've done now). That means when you pip install hyper, you'll potentially just get given PyOpenSSL.

After I merge I'll go through and take a look at the TODOs and see if I can fix them straight away or if I should turn them into issues (for better discovery). As for DHE/ECDHE, we can wait on that. =) I'll look at the tests shortly.

[Lukasa]

That means we can remove the non-TLS stuff, which should help fix up your tests. =) Let me know when you've done that and changed setup.py. =)

[alekstorm]

Done. Side note: I'm at UTC-8; you're at UTC+0, correct? Just curious.

[alekstorm]

Does this mean you're not interested in supporting non-TLS connections with prior knowledge, i.e. that don't use the Upgrade header, and immediately send HTTP/2 frames?

[Lukasa]

Heh, this snuck in! Can you pull this commit out and have it in a different pull request?

[alekstorm]

Done.

[Lukasa]

I don't think these environment variables are needed anymore.

[alekstorm]

gah fixed, thanks

[Lukasa]

Yeah, I'm in London. =) Hence the weird offsets.

I haven't come up with a good API to do prior-knowledge, and I also don't foresee it as the primary use-case of the library. The end-goal is to be able to transparently interop between HTTP/1.1 and HTTP/2, which obviates (hopefully) the need to do much in the way of prior knowledge.

So, let's see how the test run goes. =)

[Lukasa]

Oh, wait, it goes inevitable failure because now both Python 2.7 and Python 3.3 rely on a PyOpenSSL release that doesn't exist yet. =D

[alekstorm]

What do you mean? pyOpenSSL 0.14 exists; I have it installed on my machine right now. The tests only failed because ssl_compat.py wasn't excluded from the coverage tests; I've remedied that, so the build should be green... now! No wait, now. No...

[Lukasa]

Looks like there's some PyOpenSSL fun on 3.3.

[alekstorm]

Indeed, I just found out (my bad; should've been more diligent about
checking on both platforms). Looking into it now.

On Sat, Apr 5, 2014 at 11:26 AM, Cory Benfield notifications@git.luolix.topwrote:

Looks like there's some PyOpenSSL fun on 3.3.

Reply to this email directly or view it on GitHubhttps://github.com//pull/34#issuecomment-39646589
.

[Lukasa]

I added tox support which should help with this on your local machine. =)

[alekstorm]

Aaand she's green!

[Lukasa]

Victory! Ok, let's get synced up on where this is.

We have full http/2-10 support except:

• we don't have NPN support on Pythons 2.7 or 3.3
• we have a few outstanding TODOs.

Does that match your assessment?

[alekstorm]

Yes. Are the TODOs and NPN support blockers? If so, which TODOs would you like me to fix? Or did you mean all of them? If not, the fact that this is actually a partial regression on Py3.3 means that 3.4 should be officially supported, just to keep the ssl_compat API honest, do you agree?

Am I correct in assuming you have HPACKv6 sitting in a local branch somewhere, ready to merge on top of this once it gets merged? Also, this is still going into a branch, since development is reserved for IETF implementation drafts, right?

Oh, and technically we're not h2-10 compliant until DHE/EDCHE ciphers are supported in pyOpenSSL (hopefully in 0.15) - you correctly pointed this out before. For context, nghttp2 just uses the cipher list HIGH:!aNULL:!MD5 and calls itself h2-10 compliant. I don't think we should invest a lot of effort in the matter; the spec suddenly became suprisingly strict on this, and I predict it'll be relaxed later - after all, it's not so long ago that the protocol was HTTPS-only.

[Lukasa]

OK, let's take those questions in order.

1. They're blockers on the release, not on the merge.
2. 3.4 is already officially supported, being grandfathered in by our "3.3 or later" clause. I also added 3.4 to tox, though I'm a bit saddened that I can't add it to Travis yet.
3. I actually already checked HPACKv6 in. =)
4. This will go in a branch mainly because it regresses our 3.3 support. Otherwise, I'm happy to move to http2/10 when we can get NPN back in 3.3.
5. Agreed, I'm not too worried about the DHE/ECDHE support, it'll work out.

So, I'll check out all the TODOs and move the ones I'm worried about into issues, and then go ahead and merge this to a branch.

[Lukasa]

Ok, this has been merged to the h2-10 branch.

@alekstorm, thankyou so much for your work on this. You did a huge amount of work here and in #33, and it's been an enormous help. You're a model contributor! I owe you a substantial number of beverages of your choice.

[alekstorm]

Thanks! The feeling is mutual; I can't overstate how refreshing it is to work with such a supportive, courteous project maintainer. If only the rest of the open-source world had the same outlook, I think we'd all accomplish a lot more.

[Lukasa]

I'm glad you're enjoying the collaboration, I learned from the best. =) I'm glad to see your contributions keep coming, as well! In the event you're looking for more supportive types, the #positivepython IRC channel is a nice place to hang out.