905848 - Custom TLS Extensions

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect, P5)

Description

[danielj]

Attachment: Patch, as of 8/15

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.95 Safari/537.36




Expected results:

I have put together the beginnings of a patch to allow clients of the NSS library to implement support for arbitrary TLS extensions. The motivation is to allow clients of NSS to implement new proposals that bolster the CA trust model, such as TACK[1] and Certificate Transparency[2]. However, the goal is to make a broadly-useful patch allowing for a wide array of TLS extensions.

My in-progress patch is attached. It is not done, but the major functionality is more-or-less all there. There are still some needed changes that I know aren't implemented, a number of test failures I need to hunt down, and a number of style problems. But I'm getting close, and for some of what I still need to do (especially defining some parts of the public interface), feedback would be very useful.

So I'd love to hear any feedback and guidance, as well as any concerns that might prevent this from eventually getting committed.

One of the biggest issues I still need to tackle is the public interface for acting on the sslSocket struct. I am passing the client-provided callbacks the associated PRFileDesc structs instead of the sslSocket because sslSocket is private. However, at the moment this means that the client cannot even send any data on the socket, as I don't see any way to do that from the PRFileDesc, and I haven't started adding to that interface yet. There is probably other data and functionality associated with the sslSocket that client-provided callbacks would have good reason to need to access, so I'll have to figure out a good way to provide that access.

Other issues I still need to consider include:
* Consider simplifying handler code by changing the builtin ones to be of type SSL_HelloExtensionHandlerFunc and dumping all the Abstract stuff. This is basically what I did when doing the Sender stuff, and it made things way better. I expect to have to change one or the other for continuity, so we should figure out which way we want to do it and then I'll change accordingly.
* Style--Right now, the internal names for the handler stuff doesn't fit the style at all. A lot of lines are too long. Some whitespace is bad (only in places where I inherited inconsistent spacing). Will fix. Also, some names are really long, but I'm not sure what to do about that.
* Make existing tests pass
* Write tests
* Finish ripping out some assumptions and make it actually work.
* Public interface--eg. how much of the stuff in sslSocket might an extension handler/sender need to touch?
* Balance between simplicity of the new functionality vs. keeping it separate from the new functionality. Eg. the question of whether to change all the existing callbacks to the public type signature to make that easier.

[1] http://tack.io/
[2] http://www.certificate-transparency.org/

Comment 1

[danielj]

Attachment: custom_tls_extensions.diff

I've addressed the major shortcomings I know about in this patch (save a few style issues), as well as nearly all of the test failures. So I think it is ready for major feedback.

My biggest questions are:

* Am on the right track with the interface for functionality that is associated with the sslSocket? For now I'm wrapping functions that the existing Senders and Handlers seemed to need with functions that take a PRFileDesc and find the corresponding sslSocket, but I could see that getting a bit ugly, as different extensions may need different little pieces of sslSocket functionality.

* Feedback on my storage and management of Handlers and Senders would be great. I tried two different models--first with the Handlers, I left the existing Handler functions entirely alone, and as a result wrote a lot more code with uglier data structures because I had to accommodate two different types of handler functions. So when I did the Senders, I decided to make small changes to the existing Sender functions to change them to the same type as the custom senders (and thus taking a PRFileDesc instead of an sslSocket). I think this made for nicer code, but I'm not sure how acceptable it is to change those functions in the way I did. Which model should I choose, or is there a better way?

Those are the main places where I think significant thought and work may still be needed, but I'd love any feedback, both high-level (Am I on the right basic track? Is this functionality that can fit into NSS?) and low-level (eg. Are the data structures I'm introducing acceptable? Style points I need to fix?)

Comment 2

[Robert Relyea]

Comment on attachment 792406 [details] [diff] [review]
custom_tls_extensions.diff

This is isn't a code review, but the basic idea is fine. I'm OK with the way you abstracted the connection stuff.

 I'm a little disappointed to see how much work the internal handlers have to go to get the ss, it might be best to pass an opaque ss to the external handlers rather than an fd (or in addition to the fd). It really depends on how the fd is expected to be used, I don't think the handlers need to actually write to the fd directly, so using consume/append wrappers taking an opaque sslsock struct should be fine. That will illiminate the ssl_FindSock(fd).

bob