Next Protocol Negotiation redux

[Lukasa]

This is the same as #86, but with all the changes that @exarkun requested in place.

Tests pass for me locally, let's hope Travis is happy.

[Lukasa]

I totally left a debug statement in my code, which broke the build on Py 3. Rebased without it. =)

[Lukasa]

OK, all green except for OpenSSL 0.9.8, which isn't a surprise as that version of OpenSSL doesn't support NPN.

If you want, I can add a quick check to the NPN functions that throw nicer errors (NotImplementedError?) when using those versions of OpenSSL?

[alex]

You may find six.indexbytes more pleasant

[Lukasa]

@alex: Thanks! I've made those changes.

[Lukasa]

Arg, Travis didn't email me about this failed build. I'll chase it later today.

[Lukasa]

Well, I'm a little confused. The relevant error is this one:

Traceback (most recent call last):
  File "/home/travis/build/pyca/pyopenssl/OpenSSL/SSL.py", line 876, in wrapper
    proto = instr[1:l+1]
TypeError: can't concat bytes to int

This error suggests that l is a bytestring, but that seems impossible because it comes out of six.indexbytes. So I'm a little confused.

[Lukasa]

Oh, hang on, this is probably CFFI playing poorly with six. What do we think happens if we call six.indexbytes on a CFFI buffer object? Presumably not what we expect.

[alex]

instr isn't a bytes object here, so indexbytes doesn't convert the resulting thing to an int, you may want instr = _ffi.buffer(in_, inlen)[:] to get bytes out

[coveralls]

Coverage increased (+0.1%) when pulling 4969c22 on Lukasa:npn into 06ddbf3 on pyca:master.

[Lukasa]

There we go. Python 3.2 fell over against Cryptography master, but that's not to do with this change, and my previous note about OpenSSL 0.9.8 remains true.

[Lukasa]

@exarkun Just a gentle reminder that this is here. =)

[sigmavirus24]

Any update about this?

[Lukasa]

@sigmavirus24 This isn't going to get merged until JP has more time, and IIRC #15 is higher up his priority list than this is.

[exarkun]

This call probably merits some exception handling so that Python-level exceptions are turned in to OpenSSL-level error codes? Also test coverage for that case.

[Lukasa]

What worries me is debuggability. Swallowing exceptions here will make it really tricky for developers to spot their mistakes. Ideally I'd call log.exception before swallowing it, but I'm a bit nervous about adding logging where there previously was none.

Do you have any ideas?

[exarkun]

What worries me is debuggability. Swallowing exceptions here will make it really tricky for developers to spot their mistakes.

Yep. I'd definitely like to see something better than just swallowing the exception.

In other places, pyOpenSSL saves the exception and re-raises it at a later point where OpenSSL isn't going to get in the way. This can make sense in cases where "a later point" is obvious and soon - for example, certificate verification happens in response to a SSL_read call (typically). So it's somewhat straightforward to save verification exceptions and raise them from Connection.recv (which is the wrapper for SSL_read).

Does NPN fit into a model like that? Maybe NPN is advertised in response to an SSL_read call as well? After the server receives the ClientHello, perhaps? Take a look at _VerifyHelper to see how this is implemented for the certificate verification callback. I'm not sure any of the code is re-usable (at least, not as currently written) but maybe it will give you some ideas.

[Lukasa]

NPN is handled in the handshake, which means the appropriate place to raise them would be in Connection.connect, Connection.connect_ex (I think? I'm not entirely sure about what this method is actually for), and Connection.accept. I think that the approach in the _VerifyHelper will work here for these methods. I'll prototype it up and see.

[exarkun]

Cool. Keep in mind that it's also possible to drive the handshake manually via do_handshake.

[exarkun]

Sorry about the long delay in getting to this. Thanks for your work so far Lukasa. Are you still interested in getting this into pyOpenSSL? If so I'll be sure to get back to you about further revisions more quickly.

[Lukasa]

@exarkun I'm happy to keep moving this forward. =) I'd rather this got merged than abandon the patch, I'm probably best placed to keep it going.

[Lukasa]

Ok @exarkun, I believe I've made the changes you requested in 0ea76e7. =)

[coveralls]

Coverage increased (+0.19%) to 94.63% when pulling 0ea76e7 on Lukasa:npn into 06ddbf3 on pyca:master.

[Lukasa]

\o/ Thanks @exarkun!