Add ALPN support. #120
Add ALPN support. #120
Conversation
| :param callback: The callback function. It will be invoked with two | ||
| arguments: the Connection, and a list of offered protocols as | ||
| bytestrings, e.g ``[b'http/1.1', b'spdy/2']``. It should return | ||
| one of those bytestrings, then chosen protocol. |
|
FYI: ALPN support landed in the recent 1.0.2 version release of openssl. |
|
Alright, I believe I've cleaned up this patch now. The tests are a problem because OpenSSL 1.0.2 isn't present. I'd be happy to add a conditional test skip, but I think we need Travis to test against 1.0.2 before that's a good idea. |
|
Also, thanks @rbtcollins for your feedback, I've integrated all of it. 🍰 |
|
@reaperhulk As promised, my patch. |
|
Thanks @Lukasa! |
|
There are a few avenues to get 1.0.2 coverage, but the easiest will probably be to ask Travis to enable us for OS X and linux. Then we can run builds against homebrew OpenSSL. |
|
If we can do that in the next two days I'll happily add the skips. |
|
pyOpenSSL also has an 0.9.8 builder set up on travis already. It relies on an 0.9.8 Ubuntu package existing and being installable so I'm not sure if the same solution can necessarily be applied to testing 1.0.2 - but if so, it might be easier than roping in the Travis folks (or maybe not, if someone already has an in with them; fwiw, I heard some rumors that they had reached capacity for OS X and weren't accepting more projects for that - but it was third-hand at least). |
|
pyca/pyopenssl is now white listed for OS X |
2 similar comments
|
|
||
|
|
||
| def get_alpn_proto_negotiated(self): | ||
| """Get the protocol that was negotiated by ALPN.""" |
There was a problem hiding this comment.
I know PEP8 says to use this single-line style of docstring. Can you change this to put the triple quotes on their own lines, anyway? (I recognize what a jerk-like comment this is). (This message approved by Hynek, future king of pyOpenSSL)
|
|
||
| def test_alpn_success(self): | ||
| """ | ||
| Tests that clients and servers that agree on the negotiated ALPN |
There was a problem hiding this comment.
Can strike the "Tests that ..." part of this and other test method docstrings if you want. I think pyOpenSSL is currently inconsistent about this but the most-recently-preferred style assumes the reader can figure out this is a test without the extra hint. 😄
| def test_alpn_no_server(self): | ||
| """ | ||
| Tests that when clients and servers cannot agree on what protocol | ||
| to use next because the server doesn't offer ALPN. |
There was a problem hiding this comment.
When this happens, what's the expected behavior?
| conn.set_alpn_protos, | ||
| ] | ||
| for method in fail_methods: | ||
| self.assertRaises(NotImplementedError, method) |
There was a problem hiding this comment.
Loops in test methods are a bit annoying (sometimes hard to see which iteration of the loop has failed; often information about failures that might have happened in later iterations is lost).
I'd probably split this into three test methods (one for each of the methods being tested). Failing that, just calling each of the three methods explicitly/directly rather than in a loop probably makes the test a little bit more maintainable (and maybe someday pyOpenSSL will use a testing library that supports some kind of parameterized or multi-result testing to make it possible to express this test better).
| .. py:method:: Context.set_alpn_protos(protos) | ||
|
|
||
| Specify the protocols that the client is prepared to speak after the TLS | ||
| connection has been negotiated, using Application Layer Protocol |
|
Cool. BTW stock unittest/unittest2 can do test parameterisation if you test_require testscenarios |
|
\o/ :sparkles: :shipit: Big thanks @reaperhulk @hynek @exarkun! 10/10 would sprint with again. |
This pull request adds support for Application Layer Protocol Negotiation, the spec-standardised version of NPN.
This was added to OpenSSL in v1.0.2. The keen eye might detect that OpenSSL v1.0.2 is only in beta at the moment, which means that unsurprisingly this doesn't work right now. I'm opening this Pull Request to track the work: I don't expect it to be merged any time soon.
The relevant
cryptographysupport was merged in pyca/cryptography#1105.This is the second half of #79.