You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by Susan Hinrichs <sh...@network-geographics.com> on 2014/08/28 21:16:44 UTC
[API REVIEW] SSL support extensions
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
Thanks,
Susan
Re: [API REVIEW] SSL support extensions
Posted by James Peach <ja...@me.com>.
On Sep 5, 2014, at 9:33 AM, Susan Hinrichs <sh...@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 <sh...@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
>>
>>
>
Re: [API REVIEW] SSL support extensions
Posted by Susan Hinrichs <sh...@network-geographics.com>.
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 <sh...@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
>
>
Re: [API REVIEW] SSL support extensions
Posted by James Peach <jp...@apache.org>.
On Sep 5, 2014, at 8:15 AM, Susan Hinrichs <sh...@network-geographics.com> wrote:
>
> On 9/4/2014 7:33 PM, James Peach wrote:
>> On Aug 28, 2014, at 12:16 PM, Susan Hinrichs <sh...@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
>>
>>
>
Re: [API REVIEW] SSL support extensions
Posted by Susan Hinrichs <sh...@network-geographics.com>.
On 9/4/2014 7:33 PM, James Peach wrote:
> On Aug 28, 2014, at 12:16 PM, Susan Hinrichs <sh...@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. 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.
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.
>
> 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.
>
> 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?
>
> 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.
>
> 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
>
>
Re: [API REVIEW] SSL support extensions
Posted by James Peach <jp...@apache.org>.
On Aug 28, 2014, at 12:16 PM, Susan Hinrichs <sh...@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