Bug 52210

Summary: Add TLS Next Protocol Negotiation (NPN) support to mod_ssl
Product: Apache httpd-2 Reporter: mdsteele
Component: mod_sslAssignee: Apache HTTPD Bugs Mailing List <bugs>
Status: REOPENED ---    
Severity: enhancement CC: apache, apache_bugzilla, bmcquade, candrews, issues-apache, Neustradamus
Priority: P2 Keywords: FixedInTrunk, PatchAvailable
Version: 2.2-HEAD   
Target Milestone: ---   
Hardware: PC   
OS: Linux   
Attachments: Patch for mod_ssl to add NPN hooks
Updated patch for mod_ssl to add NPN hooks
Updated patch for mod_ssl to add NPN hooks
Updated patch for mod_ssl to add NPN hooks

Description mdsteele 2011-11-18 19:03:51 UTC
Created attachment 27969 [details]
Patch for mod_ssl to add NPN hooks

OpenSSL 1.0.1 added support for TLS Next Protocol Negotiation (NPN) [1], a feature which allows client and server to negotiate what protocol should be used over the secure connection.  I propose adding hooks into mod_ssl to allow other modules to access this feature.  In particular, this would open the door for a module that would support SPDY [2], a performance-improving protocol that is now supported by (at least) Google Chrome, Amazon Silk, Firefox (targeting FF11), and Strangeloop, but not yet by Apache httpd.  (Not coincidentally, I am working on implementing such a module.)

The changes needed to mod_ssl are pretty simple; I have a small patch here that adds these hooks.  The patch attached below was made against the httpd-2.2.x branch, but of course I would be happy to modify the patch as necessary for other version(s).

[1] NPN is described here: http://technotes.googlecode.com/git/nextprotoneg.html
[2] SPDY is described here: http://dev.chromium.org/spdy/spdy-protocol
Comment 1 Paul Querna 2012-03-12 23:12:06 UTC
Issues:

* Patch includes a few inline data declarations that won't compile with a strict c89 compiler.
* I'd suggest adding a log level debug with the negotiated protocol.
Comment 2 Paul Querna 2012-03-12 23:15:56 UTC
Also, yes, would prefer a patch rebased against the current trunk -- all new features land there first.
Comment 3 mdsteele 2012-03-20 16:06:29 UTC
Created attachment 28486 [details]
Updated patch for mod_ssl to add NPN hooks

Thanks for the comments; here's an updated patch.  I've rebased the patch against trunk, and I believe I've fixed the C89 issues (tested with gcc -std=c89 -pedantic).  I've also added a debug-level logging message for the negotiated protocol, as suggested.
Comment 4 mdsteele 2012-03-27 20:55:18 UTC
Created attachment 28513 [details]
Updated patch for mod_ssl to add NPN hooks

Some minor tweaks to the patch to better conform to surrounding code style.
Comment 5 Joe Orton 2012-04-10 14:46:02 UTC
Patch looks fine to me, though this:

+        const char *next_proto = NULL;
+        unsigned next_proto_len = 0;
+        SSL_get0_next_proto_negotiated(
+            inctx->ssl, (const unsigned char**)&next_proto, &next_proto_len);

is going to trip up gcc strict-aliasing tests, it should pass next_proto without a cast.
Comment 6 mdsteele 2012-04-10 15:28:27 UTC
Thanks for taking a look.  I confess that I am not terribly familiar with the strict-aliasing rules, so I am not sure how best to avoid the problem here.  The issue is that SSL_get0_next_proto_negotiated requires an unsigned char**, but apr_pstrmemdup and ssl_run_npn_proto_negotiated_hook each require a (plain, not unsigned) char*.  If I have no casts at all, the compiler complains.  Is the aliasing issue avoided if I cast to char* when I call apr_pstrmemdup and ssl_run_npn_proto_negotiated_hook, rather than casting to unsigned char** when I call SSL_get0_next_proto_negotiated, as I do now?  If so, I'll fix that right away; if not, what should I do instead?
Comment 7 Anders Kaseorg 2012-04-11 22:43:00 UTC
It’s not safe to const char ** to const unsigned char **, but it is safe to cast const unsigned char * to const char * (because a specific exemption in the strict aliasing rules allows any type of object to be accessed through a pointer to a character type).  So do this instead:

const unsigned char *next_proto = NULL;
SSL_get0_next_proto_negotiated(inctx->ssl, &next_proto, &next_proto_len);
ssl_run_npn_proto_negotiated_hook(f->c, (const char *)next_proto, next_proto_len);
Comment 8 mdsteele 2012-04-11 23:04:13 UTC
Created attachment 28588 [details]
Updated patch for mod_ssl to add NPN hooks

Aha, that is good to know, thanks.  I've updated the patch as you suggested.
Comment 9 Joe Orton 2012-05-01 13:30:13 UTC
Added in r1332643 - I made a few tweaks:

1) added HAVE_TLS_NPN in ssl_private.h with the OpenSSL version logic

2) tweaked the hook name to use "modssl_" rather than "ssl_" to avoid further polluting the "ssl_" namespace

Please shout if I broke something, I only tested compilation.  Thanks a lot for the contribution!
Comment 10 Eric Covener 2013-08-27 16:53:22 UTC
Re-opening; this is only in trunk.