On Sep 5, 2014, at 8:15 AM, Susan Hinrichs <shinr...@network-geographics.com> wrote:
> > 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. > > James, > > Thanks for the thorough feedback. I talked about the TSVconn vs TSSslVConn > issue with Alan. I'm going to take a look and make sure the conversion isn't > too onerous. Alan suggests adding a call to test whether a VConn is a > regular VConn or a SSLVConn. The (former) TSClientProtoStack API was an attempt at this. With this API you can distinguish a non-SSL TSVConn when TSVConnSSLConnectionGet returns NULL. This would match the behavior of TSHttpSsnSSLConnectionGet. > One of his arguments for keeping things separate is to give the user type > checking to distinguish between the two types of connections. If there is a > call, then the plugin writer can do different processing in the bowels of > otherwise generic connection processing code. I think that we are constrained to using TSVConn. Any existing plugin that transforms or accepts SSL connections already deals with TSVConns that refer to SSLNetVConnections under the hood. Introducing a separate type for SSL connections creates ambiguity in those cases and forces you to cast the down to TSVConn when calling other TSVConn API. > Other quick comments inline below. I'll go mess with the implementation a > bit, and I'm sure that I'll have more comments/questions. > >> >> 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? > > Yes. >> >> Can a plugin set the SNI name in a PRE_HANDSHAKE hook? > I suppose, but the PRE_HANDSHAKE hook only gets called on the server side. > The client has not yet sent the SNI name, so I'm not sure what setting a SNI > name here means. I think I was thinking of an application that would set a SNI name based on the client IP address, or some other out of band data. That makes no sense, however, because you can do the same thing in the SNI hook. I assumed that it is OK to set server certificates and adjust the SSL context in the SNI hook. Is that correct? >> 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. > Actually, that is what the current implementation does. If you don't call > reenable, the overall SNI callback return value is set to > SSL_TLSEXT_ERR_READ_AGAIN. I'll update the document to clarify. So if you > have three SNI callbacks set, if one of them does not call reenable, the > SSL_TLS_EXT_ERR_READ_AGAIN value is returned. Ah, I think I just misinterpreted this text as describing the API rather than the implementation. >> 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. > Would it make sense to blind tunnel non SSL connections as well? Maybe, what do you think? I think that a separate API call to tunnel could allow that, but I don't know enough about the use cases to know whether that makes sense or is implementable. >> 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. > Ok >> >> 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. > Because I wasn't familiar with the SSL_get_servername call. My wrap was > accessing the copy I made in the SNI callback. Agree that the openssl call > should be used. >> >> 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. > I have to ponder on this some more. Particularly what the event would mean > in this case. I think that re-enabling the TSVConn in this context makes sense. My concern is that it doesn't make sense in the other places where plugin authors use TSVConns. So there's potential for confusion there. >> 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. > ok >> >> 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 >> >> >
