On Aug 28, 2014, at 12:16 PM, Susan Hinrichs <shinr...@network-geographics.com> wrote:
> This was discussed a couple weeks back and some changes made in response. > Here it is again in the proper form. Would like to get this merged up now > that 5.1 is wrapping up. > > JIRA ticket https://issues.apache.org/jira/browse/TS-3006 and related ticket > https://issues.apache.org/jira/browse/TS-2956 > > Motivation: We need to enhance plugin support for SSL processing. > Specifically need to give plugins the ability to add to the SNI callback and > the ability to insert code to be executed after the TCP connection has > completed but before the SSL handshake processing has started. Also added > support for a TS_SSL_HOOK_OP_TUNNEL to enable more granular decision making > on blind tunneling of SSL connections. > > More details, the specific signatures, semantics, and references to example > plugins are at http://network-geographics.com/ats/docs/ssl-api.en.html. > > The current implementation is at > https://github.com/shinrich/trafficserver/tree/ts-3006 > Hi Susan, This looks like a really useful set of APIs. My main feedback is that I think we should generalize the API to work on TSVConn objects rather than introducing a new TSSslVConn object. TSSslVConn This should be TSVConn. I think that having a separate type for SSL connections introduces more problems than it solves. If we represent SSL connections as regular TSVConns that opens up the opportunity for richer hooks in the future and I think that it can simplify this set of APIs. In this case, TS_SSL_CLIENT_PRE_HANDSHAKE_HOOK generalizes to TS_VCONN_PRE_ACCEPT_HOOK which makes sense for all types of connection. TS_SSL_SNI_HOOK would be named TS_VCONN_SSL_ACCEPT_HOOK or TS_VCONN_SSL_HANDSHAKE_HOOK. I don't know whether OpenSSL gives you enough callbacks to implement it, but I think that TS_VCONN_SSL_ACCEPT_HOOK is useful even if SNI is not used, since you could still manipulate the SSL connection object at this time. Do you have to re-enable in the PRE_HANDSHAKE hook? Can a plugin set the SNI name in a PRE_HANDSHAKE hook? Using the return value from the SNI_HOOK is different from how other hooks work. The status (SSL_TLSEXT_ERR_READ_AGAIN, etc) should be given to the reenable function. TSSslVConnOp() TS_SSL_HOOK_OP_TERMINATE is an ambigious name since people commonly talk of "terminating SSL", meaning accepting an SSL connection rather than tunnelling it. Additionally, these are not really "hook" operations, they are VC operations. If you use a TSVConn instead of TSSslVConn, then do you really need TSSslVConnOpSet()? For default case, there's no need for an API. To abort the connection you could use TSVConnClose(), TSVConnAbort() or TSVConnShutdown() (not quite sure which one you should pick!). You would just need a new API to blind tunnel it, maybe TSVConnTunnel(). If you add a TSVConnTunnel() API, then the plugin could have some flexibility about where to tunnel the connection to, eg. to a specific address. TSSslVConnObject I think that this name is too similar to TSSslVConn (even though I'm recommending removing TSSslVConn). How about TSSslConnection? I think this name is a better match for TSSslContext as well. TSSslVConnObjectGet() This should be called TSVConnSSLConnectionGet() to be consistent with TSHttpSsnSSLConnectionGet(). TSHttpSsnSSLConnectionGet() should now return a TSSslConnection. TSSslVConnServernameGet() Why do we need this API? Couldn't we just tell plugins to call SSL_get_servername()? That's more consistent with our policy to not wrap OpenSSL APIs. Assuming we need it, this would be TSVConnServernameGet(), and should return a const char *. It would always return NULL for non-SSL VCs. TSSslVConnReenable() Normally the reenable takes an event, which could also be used to abort the connection. I'm undecided whether this would be a reasonable way to initiate the SSL tunnelling. Possibly not. I guess it depends whether we think the TSVConn should be consistent with TSVIOReenable() or with TSHttp*Reenable(). On balance, I think that a separate function to initiate the tunnel is best. Adding a TSVConnReenable() API has potential for confusion since in all existing API, you deal with TSVConn objects but you always re-enable the corresponding VIO. Unfortunately I don't see a way to avoid that. TSSslCertFindByName(), TSSslCertFindByAddress() These functions return TSSslContext pointers, so they should be called TSSslContextFind*. Conventionally, we use Addr rather than Address :-/ Should document the lifecycle and memory management of the return value. TSSslContextFindByName() should take a "const char *" argument. TSSslHookID Maybe this ship has sailed with the introduction of TSLifecycleHookID, but adding the new hooks into TSHttpHookID would get you TSHttpHookNameLookup() support for free. You also wouldn't need to add TSSslHookAdd() if you did this, because you could just use TSHttpHookAdd(). I'm not sure about this; I could go either way. At any rate, assuming this generalized to TSVConn, this would become TSVConnHookID. If you agree with my suggestions above, then I think the API would end up looking like this: tsapi void TSVConnHookAdd(TSVConnHookID, TSCont); tsapi void TSVConnReenable(TSVConn, TSEvent); tsapi TSReturnCode TSVConnTunnelToAddr(TSVConn, const struct sockaddr *); tsapi TSSslConnection TSVConnSSLConnectionGet(TSVConn); tsapi TSSslContext TSSslContextFindByName(const char *); tsapi TSSslContext TSSslContextFindByAddr(const struct sockaddr *); FWIW, I got the following build errors from your branch: http://fpaste.org/131134/14098719/ cheers, James
