Support ALPN/NPN

[Lukasa]

@glyph, as discussed, this fixes #9. It's not that small though (+146/-3), so we may want to approach this a different way.

[Lukasa]

It's also entirely possible that I'm an idiot and that this code is overly complex. Who knows? There aren't any tests, as well, so that makes me nervous.

[codecov-io]

Current coverage is 85.31% (diff: 88.27%)

Merging #10 into master will increase coverage by 45.10%

@@             master        #10   diff @@
==========================================
  Files             6          8     +2   
  Lines            92        361   +269   
  Methods           0          0          
  Messages          0          0          
  Branches          9         21    +12   
==========================================
+ Hits             37        308   +271   
+ Misses           55         41    -14   
- Partials          0         12    +12   

Powered by Codecov. Last update e8a243a...bd968bf

[glyph]

@Lukasa Looks like codecov disagrees (did it just break)

[glyph]

Oh okay there are actually no tests.

@Lukasa - This actually looks like a fine direction, but it needs some tweaking:

1. SNIMap.getContext is dead code now, and can be removed
2. It should no longer inherit from ContextFactory
3. Similarly we should be calling serverConnectionForTLS to get the Connection rather than instantiating it ourselves; we can retrieve its context via Connection.get_context after the fact.
4. write an test

Also we should probably check in with @mithrandi before doing a release

[mithrandi]

txacme + h2 seems to work just fine with this branch:

*        issuer: CN=Fake LE Intermediate X1
* ALPN, server accepted to use h2
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)

Anything else you need from me? :)

[glyph]

@mithrandi Huh. Good to know! I am wondering if we need to leave getContext in place though; does txacme touch it at that level?

[mithrandi]

Nope; the only "touching" that happens is passing a wrapped host mapping to SNIMap.

[mithrandi]

Basically the only thing txacme does with SNIMap is TLSMemoryBIOFactory(contextFactory=SNIMap(...)) so it doesn't care about the implementation details at all.

[glyph]

Fantastic, then the cleanup shouldn't matter.

[mithrandi]

Could someone put "fixes #9" in the description of this PR? :)

[glyph]

@mithrandi I stuffed some words into @Lukasa's mouth on the first comment there, hopefully that will do

[Lukasa]

Ok, assuming I'm not caught up with family stuff this weekend imma have a swing at @glyph's changes for this PR.

[Lukasa]

Ok I've done the first two. The third one I can't do (@glyph incorrectly believed that CertificateOptions implemented IOpenSSLServerConnectionCreator, but it doesn't). The fourth one is coming.

[Lukasa]

Ok, I added tests.

I'm not really happy about this. There are a lot of things I can't easily test in this manner: for example, TxSNI's fallback to use the DEFAULT.pem file is hard to test with Twisted's endpoints because they don't actually let you omit SNI.

Anyway, these tests include a basic bit of "does this legitimately work at all" stuff, including for protocol negotiation. So that is something.

[Lukasa]

@glyph, you monster! The tests against Twisted 13.2 blow up because IOpenSSLConnectionCreator doesn't exist. So...why did I add it? ;)

[mithrandi]

@Lukasa As the one who picked "Twisted 13.2" as the minimum version to test on, I can tell you that the choice was essentially arbitrary; let's just bump the minimum version to whatever we need to support this?

[Lukasa]

Twisted 14.0, I think, then.

[Lukasa]

So on the coverage front, the thing we seem to be missing is the ALPN stuff. This isn't a surprise: it turns out ubuntu sucks and doesn't have a recent OpenSSL.

[glyph]

14.0 is the minimum acceptable version of Twisted for anything in production. Before that we don't even verify certs.

[Lukasa]

Bump for @glyph.

[glyph]

@Lukasa If I understand the question for me, it is: Travis-CI isn't covering the NPN stuff in this PR because the base image that Travis is testing with has an OpenSSL that's too old?

My solution to that would be: could you submit a separate PR that just bumps the OpenSSL version, either by building one in the travis config, or by selecting a more recent OS to run on travis (perhaps with their "docker" support, I dunno) and then we can land that first?

[glyph]

Oops. I should have landed this some time ago!

[glyph]

On second thought, I don't want to get email from security scanners about this :). Can these keys be generated on the fly (or at least "once per developer") rather than checked in?

[glyph]

@Lukasa - I'd be happy to merge this but if you could remove the private keys from the repo first that would be great. If it would be inordinately difficult, then I suppose it is something I could live with for now, though :)

[Lukasa]

Ok @glyph, this should remove the certs from the repo.

[Lukasa]

Many thanks to @alex and @reaperhulk for writing something that makes it really easy to build certs on the fly.

[reaperhulk]

@sigmavirus24 gets some credit too -- he shepherded at least one of the builder PRs through our review gauntlet :)

[sigmavirus24]

looks around after finding himself in an unfamiliar place, spots a rock, and hides behind it

[glyph]

what the heck happened to codecov :(. 0% of diff hit now?

[mithrandi]

@glyph The secret is in the build log: "Coverage.py warning: No data was collected." Adding PYTHONPATH=. to the build should fix it. Switching to coverage run -m twisted.trial may also fix this, but cause problems if the tests rely on __file__.

[mithrandi]

Oh right, we're using tox, so PYTHONPATH=. will probably do completely the wrong thing. We should really switch to separate-source-dir, I guess.

[Lukasa]

Yeah, this is also a facet of the fact that trial changed and now only tests installed code, rather than code that is in the tree. That means that the coverage path is probably no longer right.

[Lukasa]

Alrighty, progress made. =D

[glyph]

Fantastic! Looks good to me.