On Wed, Apr 9, 2014 at 10:24 AM, Jeff Trawick <[email protected]> wrote:
> On Fri, Apr 4, 2014 at 7:48 PM, Jeff Trawick <[email protected]> wrote: > >> On Tue, Feb 18, 2014 at 3:50 PM, Scott Deboy <[email protected]>wrote: >> >>> Hi folks, >>> >>> I was wondering if someone would be willing/interested in reviewing the >>> patch I've attached to issue 55467. >>> >>> https://issues.apache.org/bugzilla/show_bug.cgi?id=55467 >>> >>> The patch adds hooks to mod_ssl which give third-party modules the >>> ability to send and receive custom TLS hello extensions TLS supplemental >>> data. It also gives third-party modules the ability to trigger >>> renegotiation. It leverages APIs recently added to OpenSSL master and >>> 1.0.2 stable branches. >>> >>> Any feedback is appreciated! >>> >>> >> Any thoughts out there on passing SSL* to the hook as void* as in the >> patch? I've been experimenting with some hooks to enable Certificate >> Transparency in a module, and it seemed feasible to me to let mod_ssl.h own >> the job of getting the right headers included in order to specify the right >> OpenSSL datatype on the API. Is that asking for trouble? >> >> If building with OpenSSL < 1.0.2, the affected optional hooks shouldn't >> be available. >> >> I anticipate syncing my CT code with the pieces >> for SSL_CTX_set_custom_cli_ext()/SSL_CTX_set_custom_srv_ext() and >> committing the relevant parts of your patch (not that the rest is much >> different). Hopefully some "genuine" mod_ssl developers will render an >> opinion on placement and any other details. >> >> > I started "really" looking at the TLS extension APIs today... Here are > some notes, hopefully based on a proper understanding of the code :) : > > The optional functions can only be called at startup -- not thread-safe, > no impact on connection anyway. I guess this would be handled in API > documentation. > > SSL_CTX_set_custom_cli_ext() and SSL_CTX_set_custom_srv_ext() fail if a > callback is already registered for the extension type or if a memory > allocation error occurs. As long as any modules that need to participate > use your APIs, the duplicate registration is handled. However, the memory > allocation error (or any other future error) is not handled. > > In the optional function the module passes userdata (userdata which must > be known at initialization, which severely limits its usefulness). During > the handshake every module that handles any TLS extension via this API will > be called, and all but one must ignore the call if it isn't the extension > type they registered. The hook only gets the init-time value passed in, > not the conn_rec (from which the server_rec and all sorts of > context-specific configuration could be found). > > I think some changes are appropriate: > > a) the module code that actually handles the TLS extension (receiving or > generating) must get the conn_rec (which is easy since mod_ssl retrieves > the conn_rec in the call from OpenSSL. Given the conn_rec, it doesn't seem > important that the module call also receives its init-time userdata, but it > doesn't hurt anything. > > b) since only one module can register to handle a particular TLS > extension, there is no need for mod_ssl to run a hook (which could be > accidentally subverted by some other module failing a call for an extension > that it doesn't handle); instead, the init-time registration of a module's > interest in a TLS extension should provide a callback function; so OpenSSL > call's mod_ssl's callback which retrieves the conn_rec and looks up the > module callback function by TLS extension id > > Does that make sense so far? > > Another concern is that a module may not actually care about extensions > depending on vhost-specific configuration and/or mode. The server-side > callbacks are simply ignored if running in a proxy, and similar for the > client-side callbacks. As long as a module can get to its configuration > (via conn_rec presumably) in the other cases it can filter out calls that > it shouldn't do anything for. > > --/-- > > So why not let the module handle all of this itself, without mod_ssl going > to the trouble of keeping track stuff that OpenSSL already handles? For my > initial C-T implementation ( > https://github.com/trawick/ct-httpd/blob/master/src/proto1/) I added a > hook which the ssl_ct module implements as follows: > > static int ssl_ct_ssl_init_ctx(server_rec *s, apr_pool_t *p, int is_proxy, > SSL_CTX *ssl_ctx) > { > ct_callback_info *cbi = apr_pcalloc(p, sizeof *cbi); > ct_server_config *sconf = ap_get_module_config(s->module_config, > &ssl_ct_module); > > cbi->s = s; > > if (is_proxy && sconf->proxy_awareness != PROXY_OBLIVIOUS) { > /* _cli_ = "client" */ > if (!SSL_CTX_set_custom_cli_ext(ssl_ctx, CT_EXTENSION_TYPE, > client_extension_callback_1, > client_extension_callback_2, cbi)) > { > /* error, abort startup */ > } > } > else if configured to do stuff in server mode { > } > > /* do other stuff unrelated to TLS extensions */ > > return OK; > } > > mod_ssl calls this in ssl_init_ctx() after all of its existing > configuration. > > Thoughts? > FYI... I just spoke with Eric at AC, who brought up the value of toolkit-agnostic APIs that could be implemented by mod_nss or mod_gnutls or other such third-party modules that don't use OpenSSL. We have a few APIs in mod_ssl, such as enabling SSL on a backend proxy connection or looking up SSL vars, that are "httpd" APIs in practice since the APIs are implemented by other SSL/TLS implementations and some modules rely on that. > > >> >> >>> Thanks much, >>> >>> Scott >>> >>> On Feb 6, 2014, at 2:20 PM, Scott Deboy <[email protected]> wrote: >>> >>> > Support for sending and receiving TLS hello extensions and TLS >>> supplemental data messages has recently been added to the OpenSSL GitHub >>> master branch. >>> > >>> > I've submitted a patch to mod_ssl which allows third-party modules to >>> send and receive TLS hello extensions and TLS supplemental data via >>> optional hooks and functions. >>> > >>> > The patch can be found here: >>> https://issues.apache.org/bugzilla/show_bug.cgi?id=55467 >>> > >>> > I'm happy to update the patch based on feedback. >>> > >>> > Thanks much, >>> > >>> > Scott Deboy >>> > >>> >>> >> >> >> -- >> Born in Roswell... married an alien... >> http://emptyhammock.com/ >> http://edjective.org/ >> >> > > > -- > Born in Roswell... married an alien... > http://emptyhammock.com/ > http://edjective.org/ > > -- Born in Roswell... married an alien... http://emptyhammock.com/ http://edjective.org/
