|
Description
mdsteele
2011-11-18 19:03:51 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. Also, yes, would prefer a patch rebased against the current trunk -- all new features land there first. 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.
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.
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. 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? 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); 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.
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! Re-opening; this is only in trunk. This is currently behing discussed in https://www.mail-archive.com/dev@httpd.apache.org/msg59893.html (In reply to Yann Ylavic from comment #11) > This is currently behing discussed in > https://www.mail-archive.com/dev@httpd.apache.org/msg59893.html this discussion is dead and it does not take into account that npn is a generic mechanism to be able to switch to alternative protocols not neccessarily limited to be used for spdy support. Also this is part of openssl 1.0.1 - mod_ssl just needs to turn it on. Another aspect why we really want to have NPN support in mod_ssl is: firefox and chrome currently use the SSL optimization "false start" only if the server also supports NPN. So with NPN support in mod_ssl we would get nice the side effect to remove one roundtrip time for ssl handshakes for those clients. NPN has been superseded by ALPN - https://www.imperialviolet.org/2013/03/20/alpn.html With OpenSSL 1.0.1 supporting NPN by default, it would be really great to get this patch into the next 2.4 release. Even though NPN will be replaced by ALPN (one day), there are protocols like SPDY and HTTP2 that will need to support both for some time to allow migration. I cannot speak for the openssl project, but current 1.0.1 release has no ALPN support and the 1.0.2 with support is in beta. To add ALPN support to mod_ssl, it seems to make send to delay that until openssl has a production release that supports it. Stefan, HTTP/2 uses ALPN, not NPN; the only reason some implementations use NPN is for testing while ALPN filtered its way into OpenSSL and other TLS implementations. OpenSSL supports ALPN as of 1.0.2. Browsers are even now starting to drop NPN in their negotiation, because doing both has caused interop problems. NPN is a dead end, as very explicitly stated by AGL and others. Mark, I understand. However OpenSSL 1.0.2 is currently in beta (although according to R$ in production soon) and 1.0.1 does not have ALPN support. I also doubt, giving the history, that everyone will deploy 1.0.2 immediately. So, currently we have the situation where you cannot drop a mod_spdy into a 2.4 server without also replacing mod_ssl. The patch for NPN is in httpd/trunk, but not in 2.4. So, everyone wanting to do ALPN/NPN with a httpd 2.4 needs to replace mod_ssl which is not a good idea, it seems. I today have made a clean patch for the NPN changes in trunk to be applicable to 2.4. I want to enhance this with a second patch that uses ALPN instead of NPN of the underlying openssl supports it (compile time). I think this will allow for an easier transition to newer openssl and introduction of ALPN than having everyone write her own mod_ssl version. My question to httpd developers would be if they want to change the "npn" part of the callback function names to "alpn" - which is worth it imho. I will put this on the mailing list for discussion. Created attachment 32381 [details]
2.4.x patch for adding ALPN+NPN support
This patch adds ALPN support to 2.4.x, building on the previous patch attached to this ticket.
The ALPN support changes the public functions from the previous patch: - renames from "npn" to "alpn" - adds an optional parameter to the advertise/propose function with the protocol names supplied by the client The patch keeps the NPN support, if the underlying openssl does not support ALPN (version >= 1.0.2). The patch introduces a new mod_ssl config directive: SSLAlpnPreference which is a list of protocol names that determine which protocol gets chosen first, if supported by the client. A patch for httpd/trunk can be made available on request. A patch of mod_spdy to use the new functions also. Let me know. Created attachment 32399 [details] Patch update to always announce http/1.1 over NPN As requested in https://issues.apache.org/bugzilla/show_bug.cgi?id=56028 I updated my patch to always announce "http/1.1" over NPN (ALPN already did it). This means that when no hooks are installed, mod_ssl will always advertise http/1.1 to clients on NPN/ALPN. @Stefan Eissing In your patch, I think you have a bug, why do you have HAVE_ALPN_NPN and HAVE_TLS_ALPN? Only HAVE_TLS_ALPN is defined. Hi, I don't understand if the patch: Patch update to always announce http/1.1 over NPN (21.41 KB, patch) 2015-01-27 09:28 UTC, Stefan Eissing applies to httpd-2.2 (more specifically to 2.2.29). The patch starts with these lines: diff -ru gen/httpd-2.4.x/modules/ssl/mod_ssl.c modules/ssl/mod_ssl.c --- gen/httpd-2.4.x/modules/ssl/mod_ssl.c 2015-01-19 16:52:30.000000000 +0100 What do I have to think ? it's for httpd-2.4.x ? If yes than what patch is for httpd-2.2 ? The original patch is from 2011. I think nowadays 2.4 is the target for this. It certainly would be possible to downport it. I updated the ticket to specify 2.4 as target. Created attachment 32452 [details]
Updated patch introducing ALPN/NPN support for mod_ssl
This version of the patch uses correct ALPN/NPN defines and has been tested against 2.4.10.
Created attachment 32463 [details]
Updated patch for ALPN/NPN support in mod_ssl
Fix in DEBUG log of negotiated protocol.
It seems that HAVE_TLS_NPNX is defined instead of HAVE_TLS_NPN in the part of the patch below. Also, probably it shouldn't be defined when OPENSSL_NO_TLSEXT is used (like HAVE_TLS_ALPN). diff -ru gen/httpd-2.4.x/modules/ssl/ssl_private.h modules/ssl/ssl_private.h --- gen/httpd-2.4.x/modules/ssl/ssl_private.h 2015-01-19 16:52:30.000000000 +0100 +++ modules/ssl/ssl_private.h 2015-01-19 15:42:53.908000000 +0100 @@ -176,6 +169,16 @@ [...] +/* Next Protocol Negotiation */ +#if !defined(OPENSSL_NO_NEXTPROTONEG) && defined(OPENSSL_NPN_NEGOTIATED) +#define HAVE_TLS_NPNX +#endif Created attachment 32464 [details]
Add ALPN/NPN support to mod_ssl
Update: fixed stupid NPNX define type, removed compiler warnings
Tested: with 2.4.12 and openssl 1.0.2/1.0.1l on darwin
Created attachment 32521 [details]
Enables ALPN+NPN in mod_ssl simultaneously if supported by openssl
This feature keeps on giving: added support for simultaneous ALPN and NPN support (openssl 1.0.2 onwards), so that old and new clients may negotiate. Necessary as long as there are clients using openssl 1.0.1 versions.
No changes to callback API from previous versions.
Created attachment 32523 [details]
Simul. ALPN+NPN support in mod_ssl, verified against mixed, various clients
* Found a bug when testing an NPN only client against ALPN+NPN server with patch from yesterday.
* replaced TABs with spaces in patch for proper formatting
* verified this patch against httpd 2.4.12 in the following combinations:
- OpenSSL 1.0.2 httpd against Firefox 36, Chrome 40
- OpenSSL 1.0.2 httpd against OpenSSL 1.0.2 nghttp2 client
- OpenSSL 1.0.2 httpd against OpenSSL 1.0.1 nghttp2 client
- OpenSSL 1.0.1 httpd against Firefox 36, Chrome 40
- OpenSSL 1.0.1 httpd against OpenSSL 1.0.2 nghttp2 client
- OpenSSL 1.0.1 httpd against OpenSSL 1.0.1 nghttp2 client
Sorry, if the update frequency of this patch is making noise.
Current status in trunk: the changes in attachment 32523 [details] were committed with r1670397 and r1670434. NPN support was subsequently removed with r1676004. Closing this bug as INVALID (for lack of a better resolution - WONTFIX doesn't really fit), as it relates to NPN, which is now quite unlikely to find its way into a stable release. Comment on attachment 32464 [details]
Add ALPN/NPN support to mod_ssl
Marking 2015-02-12 patch version as obsolete
|