On Sep 5, 2014, at 9:33 AM, Susan Hinrichs <shinr...@network-geographics.com> wrote:
> Quick question on naming consistency. James, you propose > > tsapi TSSslConnection TSVConnSSLConnectionGet(TSVConn); > > In this signature, we are using SSL both all capitalized (in the function > name) and mixed case (in the type name). > > I see that SSL in a function name has precedent in TSHttpSsnSSLConnectionGet. > But it seems in general for protocol abbreviations we are using mixed case > (e.g. Http not HTTP). In any case, we should be consistent and do all SSL or > all Ssl in my opinion. Yes, I agree that this is a bit unfortunate. I think that publishing TSVConnSslConnectionGet() and TSHttpSsnSSLConnectionGet() would be a bad result, and we can't change TSHttpSsnSSLConnectionGet() except by deprecating the old name and adding a new name with consistent capitalization. TSSSLConnection and TSSSLConect both scan poorly to me. Putting on my sophist's hat, I'd say that since TSSslConnection is consistent with one convention and TSVConnSSLConnectionGet is consistent with another, then overall it's a fine :-/ > > On 9/4/2014 7:33 PM, James Peach wrote: >> 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 >> >> >
