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.
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
