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/19 16:50:10 UTC
SSL support extensions - design review request
Good morning, all.
I've been working on implementing the SSL plugin extensions described in
TS-3006. Documentation is at
http://network-geographics.com/ats/docs/ssl-api.en.html. The features
have been implemented, and I am in the testing stage right now.
Looking for feedback on the design. Please comment on the bug or reply
to the mailing list.
Thanks,
Susan
Re: SSL support extensions - design review request
Posted by Susan Hinrichs <sh...@network-geographics.com>.
Updated documents and code to add default hook op and remove the
TSSslVConn argument from the FindBy functions.
On 8/19/2014 2:46 PM, Susan Hinrichs wrote:
> James,
>
> Thanks for the feedback. I think Alan already addressed most of the
> issues. Here are my comments on the remaining items.
>
>
>>
>> Why do TSSslCertFindByName() and TSSslCertFindByAddress() take a
>> TSSslVConn argument?
>
> I'm using the TSSslVConn to cache a pointer to the global cert table
> (loaded from ssl_multicert.config). Since in theory the
> ssl_multicert.config could be reloaded at any point, we acquire() a
> copy of the pointer before going into the PRE_HANDSHAKE and SNI hooks,
> and release() that pointer at the end. Originally I thought of
> having a TSSslCertTableGet() and then pass that into the
> TSSslCertFind*() functions, but decided to squish out the explicit
> LookupTable call.
>
> Actually, as I'm writing this up, I realize that since I removed the
> explicit reference to the Lookup table from the API, I don't need to
> cache a copy of the lookup table in the framework. The
> TSSslCertFind*() functions can just acquire and release as they go.
> I'll go ahead and do that API simplification. That will remove the
> TSSslVConn argument.
>
>> Previously, then we looked at SSL integration APIs, we eschewed
>> wrapped types like TSSslContext in favour of directly returning
>> OpenSSL pointers (as void *). My argument at the time was that
>> there's a lot of API surface needed to deal with SSL and not much
>> value in just wrapping OpenSSL.
>
> Can definitely go either way here. Another benefit of the opaque
> types is that the compiler can do some minimal type checking for you.
> Not a huge issue for me either way.
>>
>>
>> There's no TSSslVConnOp value for the default action? ie. to just
>> accept the SSL session?
> Definitely would be good to have the default to undo. Will get that
> added
>>
>> IIRC, OpenSSL doesn't guarantee anything about the SNI name except
>> that is is a bag of bytes. Is it OK for TSSslVConnServernameGet() to
>> present that as a C string?
> The servername is not null terminated in the packet, but OpenSSL does
> null terminate it before handing back the value via
> SSL_get_servername(). I went back through the openssl to verify the
> null termination. Interestingly some data structures in there are
> storing the servername as buffer plus length, but the one returned is
> NULL terminated and data only. Internally they are doing many strlen
> and strcmp operations on it.
>>
>> Finally, I'm not thrilled with the idea of using ssl_multicert.config
>> to specify the tunneling action. What would you do if you had a
>> wildcard cert and only wanted to tunnel a specific name?
> In the wildcard case, e.g, a cert for *.bob.com, you could create a
> self signed cert to represent fred.bob.com and then use that cert to
> enter a config line to blind tunnel traffic to fred.bob.com. In the
> tunnel case the cert is only used select traffic, so it need not be
> signed by a 'real' CA.
>
> Of course, I'm open to other options. As Alan noted, one could write
> a plugin for any complex scenarios, but adding a config-only option
> for simple cases seems like a good idea.
>
> Thanks,
> Susan
Re: SSL support extensions - design review request
Posted by James Peach <jp...@apache.org>.
On Aug 20, 2014, at 5:33 AM, Igor Galić <i....@brainsware.org> wrote:
>
>
> ----- Original Message -----
>> James,
>>
>> Thanks for the feedback. I think Alan already addressed most of the
>> issues. Here are my comments on the remaining items.
>>
>>
>>>
>>> Why do TSSslCertFindByName() and TSSslCertFindByAddress() take a TSSslVConn
>>> argument?
>>
>> I'm using the TSSslVConn to cache a pointer to the global cert table
>> (loaded from ssl_multicert.config). Since in theory the
>> ssl_multicert.config could be reloaded at any point, we acquire() a copy
>
> does this mean we would now support reloading of the ssl config w/o restart?
that's been supported for a coupe of years now
>
> [snip]
>>>
>>> IIRC, OpenSSL doesn't guarantee anything about the SNI name except that is
>>> is a bag of bytes. Is it OK for TSSslVConnServernameGet() to present that
>>> as a C string?
>> The servername is not null terminated in the packet, but OpenSSL does
>> null terminate it before handing back the value via
>> SSL_get_servername(). I went back through the openssl to verify the
>> null termination. Interestingly some data structures in there are
>> storing the servername as buffer plus length, but the one returned is
>> NULL terminated and data only. Internally they are doing many strlen
>> and strcmp operations on it.
>
> my recommendation for reading openssl code is to read libressl's version.
>
> [snip]
>
> -- i
> Igor Galić
>
> Tel: +43 (0) 664 886 22 883
> Mail: i.galic@brainsware.org
> URL: http://brainsware.org/
> GPG: 8716 7A9F 989B ABD5 100F 4008 F266 55D6 2998 1641
>
Re: SSL support extensions - design review request
Posted by "Alan M. Carroll" <am...@network-geographics.com>.
Igor,
>> I'm using the TSSslVConn to cache a pointer to the global cert table
>> (loaded from ssl_multicert.config). Since in theory the
>> ssl_multicert.config could be reloaded at any point, we acquire() a copy
> does this mean we would now support reloading of the ssl config w/o restart?
No. It just avoids making the problem worse, the original problem is still there.
Re: SSL support extensions - design review request
Posted by Igor Galić <i....@brainsware.org>.
----- Original Message -----
> James,
>
> Thanks for the feedback. I think Alan already addressed most of the
> issues. Here are my comments on the remaining items.
>
>
> >
> > Why do TSSslCertFindByName() and TSSslCertFindByAddress() take a TSSslVConn
> > argument?
>
> I'm using the TSSslVConn to cache a pointer to the global cert table
> (loaded from ssl_multicert.config). Since in theory the
> ssl_multicert.config could be reloaded at any point, we acquire() a copy
does this mean we would now support reloading of the ssl config w/o restart?
[snip]
> >
> > IIRC, OpenSSL doesn't guarantee anything about the SNI name except that is
> > is a bag of bytes. Is it OK for TSSslVConnServernameGet() to present that
> > as a C string?
> The servername is not null terminated in the packet, but OpenSSL does
> null terminate it before handing back the value via
> SSL_get_servername(). I went back through the openssl to verify the
> null termination. Interestingly some data structures in there are
> storing the servername as buffer plus length, but the one returned is
> NULL terminated and data only. Internally they are doing many strlen
> and strcmp operations on it.
my recommendation for reading openssl code is to read libressl's version.
[snip]
-- i
Igor Galić
Tel: +43 (0) 664 886 22 883
Mail: i.galic@brainsware.org
URL: http://brainsware.org/
GPG: 8716 7A9F 989B ABD5 100F 4008 F266 55D6 2998 1641
Re: SSL support extensions - design review request
Posted by James Peach <jp...@apache.org>.
On Aug 19, 2014, at 12:46 PM, Susan Hinrichs <sh...@network-geographics.com> wrote:
> James,
>
> Thanks for the feedback. I think Alan already addressed most of the issues. Here are my comments on the remaining items.
>
>
>>
>> Why do TSSslCertFindByName() and TSSslCertFindByAddress() take a TSSslVConn argument?
>
> I'm using the TSSslVConn to cache a pointer to the global cert table (loaded from ssl_multicert.config). Since in theory the ssl_multicert.config could be reloaded at any point, we acquire() a copy of the pointer before going into the PRE_HANDSHAKE and SNI hooks, and release() that pointer at the end. Originally I thought of having a TSSslCertTableGet() and then pass that into the TSSslCertFind*() functions, but decided to squish out the explicit LookupTable call.
>
> Actually, as I'm writing this up, I realize that since I removed the explicit reference to the Lookup table from the API, I don't need to cache a copy of the lookup table in the framework. The TSSslCertFind*() functions can just acquire and release as they go. I'll go ahead and do that API simplification. That will remove the TSSslVConn argument.
Great!
>> Previously, then we looked at SSL integration APIs, we eschewed wrapped types like TSSslContext in favour of directly returning OpenSSL pointers (as void *). My argument at the time was that there's a lot of API surface needed to deal with SSL and not much value in just wrapping OpenSSL.
>
> Can definitely go either way here. Another benefit of the opaque types is that the compiler can do some minimal type checking for you. Not a huge issue for me either way.
Fair enough. We should go back and make typedefs for the other API that exposes SSL * then.
>>
>>
>> There's no TSSslVConnOp value for the default action? ie. to just accept the SSL session?
> Definitely would be good to have the default to undo. Will get that added
>>
>> IIRC, OpenSSL doesn't guarantee anything about the SNI name except that is is a bag of bytes. Is it OK for TSSslVConnServernameGet() to present that as a C string?
> The servername is not null terminated in the packet, but OpenSSL does null terminate it before handing back the value via SSL_get_servername(). I went back through the openssl to verify the null termination. Interestingly some data structures in there are storing the servername as buffer plus length, but the one returned is NULL terminated and data only. Internally they are doing many strlen and strcmp operations on it.
>>
>> Finally, I'm not thrilled with the idea of using ssl_multicert.config to specify the tunneling action. What would you do if you had a wildcard cert and only wanted to tunnel a specific name?
> In the wildcard case, e.g, a cert for *.bob.com, you could create a self signed cert to represent fred.bob.com and then use that cert to enter a config line to blind tunnel traffic to fred.bob.com. In the tunnel case the cert is only used select traffic, so it need not be signed by a 'real' CA.
>
> Of course, I'm open to other options. As Alan noted, one could write a plugin for any complex scenarios, but adding a config-only option for simple cases seems like a good idea.
Yeh, my concern here is that ssl_multicert is purely a data source right now. This change adds processing rules to it, which is a bit of a change of character. In fact, as your example above points out, it ends up doing 2 rather different things.
What I'm planning to do later in the year is to rewrite ssl_multicert.config so that it works more like overridable records configuration. There are a number of SSL certificate settings that are only configurable globally, and duplicating everything in a different syntax in ssl_multicert is becoming unwieldy. I think the action is probably ok, but I hope we don't go too far down this path :)
J
Re: SSL support extensions - design review request
Posted by Susan Hinrichs <sh...@network-geographics.com>.
James,
Thanks for the feedback. I think Alan already addressed most of the
issues. Here are my comments on the remaining items.
>
> Why do TSSslCertFindByName() and TSSslCertFindByAddress() take a TSSslVConn argument?
I'm using the TSSslVConn to cache a pointer to the global cert table
(loaded from ssl_multicert.config). Since in theory the
ssl_multicert.config could be reloaded at any point, we acquire() a copy
of the pointer before going into the PRE_HANDSHAKE and SNI hooks, and
release() that pointer at the end. Originally I thought of having a
TSSslCertTableGet() and then pass that into the TSSslCertFind*()
functions, but decided to squish out the explicit LookupTable call.
Actually, as I'm writing this up, I realize that since I removed the
explicit reference to the Lookup table from the API, I don't need to
cache a copy of the lookup table in the framework. The TSSslCertFind*()
functions can just acquire and release as they go. I'll go ahead and do
that API simplification. That will remove the TSSslVConn argument.
> Previously, then we looked at SSL integration APIs, we eschewed wrapped types like TSSslContext in favour of directly returning OpenSSL pointers (as void *). My argument at the time was that there's a lot of API surface needed to deal with SSL and not much value in just wrapping OpenSSL.
Can definitely go either way here. Another benefit of the opaque types
is that the compiler can do some minimal type checking for you. Not a
huge issue for me either way.
>
>
> There's no TSSslVConnOp value for the default action? ie. to just accept the SSL session?
Definitely would be good to have the default to undo. Will get that added
>
> IIRC, OpenSSL doesn't guarantee anything about the SNI name except that is is a bag of bytes. Is it OK for TSSslVConnServernameGet() to present that as a C string?
The servername is not null terminated in the packet, but OpenSSL does
null terminate it before handing back the value via
SSL_get_servername(). I went back through the openssl to verify the
null termination. Interestingly some data structures in there are
storing the servername as buffer plus length, but the one returned is
NULL terminated and data only. Internally they are doing many strlen
and strcmp operations on it.
>
> Finally, I'm not thrilled with the idea of using ssl_multicert.config to specify the tunneling action. What would you do if you had a wildcard cert and only wanted to tunnel a specific name?
In the wildcard case, e.g, a cert for *.bob.com, you could create a self
signed cert to represent fred.bob.com and then use that cert to enter a
config line to blind tunnel traffic to fred.bob.com. In the tunnel case
the cert is only used select traffic, so it need not be signed by a
'real' CA.
Of course, I'm open to other options. As Alan noted, one could write a
plugin for any complex scenarios, but adding a config-only option for
simple cases seems like a good idea.
Thanks,
Susan
Re: SSL support extensions - design review request
Posted by James Peach <jp...@apache.org>.
On Aug 19, 2014, at 10:13 AM, Alan M. Carroll <am...@network-geographics.com> wrote:
> James,
>
> I can answer a few of these.
>
>> Thanks for the docs, this looks very promising. When you are ready to submit patches, this will need API review <https://cwiki.apache.org/confluence/display/TS/API+Review+Process>.
>
> Actually, Susan forgot to mention that you can review the code at
> https://github.com/shinrich/trafficserver/tree/ts-3006
>
>> There's already and API to get the SSL *, TSHttpSsnSSLConnectionGet. I don't see the need for a TSSslVConn, the TSHttpSsn should be sufficient.
>
> I don't think that works, because the pre handshake hook is called before a client session exists so TSHttpSsnSSLConnectionGet() will fail in that context.
>
>> Are TS_SSL_CLIENT_PRE_HANDSHAKE_HOOK and TS_SSL_SNI_HOOK session hooks? If so, they should follow the session hook naming conventions. If not, how do do register these hooks?
>
> Not really, as there's no client session instance at that time. Both of these hooks are called before any of the session accept logic is executed. This makes for some ugly choices in the API.
Hmm. I would like to avoid additional API object is possible. Could you use a TSVConn rather than creating TSSslVConn?
>> Previously, then we looked at SSL integration APIs, we eschewed wrapped types like TSSslContext in favour of directly returning OpenSSL pointers (as void *). My argument at the time was that there's a lot of API surface needed to deal with SSL and not much value in just wrapping OpenSSL.
>
> I recommended that because there was a discussion about potentially using a different SSL library. The expectation is that these wrapper types can be directly cast to a SSL library type, which is dependent on how ATS was built.
I'm happy to assume that we will always use OpenSSL ;)
>> There's no TSSslVConnOp value for the default action? ie. to just accept the SSL session?
>
> In that case, don't call TSSslVConnOpSet. But I suppose you might want to undo, so that should be added.
I think it's better to include the default for completeness and documentation.
>> IIRC, OpenSSL doesn't guarantee anything about the SNI name except that is is a bag of bytes. Is it OK for TSSslVConnServernameGet() to present that as a C string?
>
> Ah, yes. It should return a void* and size value. There's also an issue on what maximum length can be presumed and how storage for that is handled. The spec, as I read it, puts a limit of 64K, but in practice openSSL seems to limit it to 256 (TS_MAX_HOST_NAME_LEN).
>
>> Finally, I'm not thrilled with the idea of using ssl_multicert.config to specify the tunneling action. What would you do if you had a wildcard cert and only wanted to tunnel a specific name?
>
> Write a plugin :-).
Re: SSL support extensions - design review request
Posted by "Alan M. Carroll" <am...@network-geographics.com>.
James,
I can answer a few of these.
> Thanks for the docs, this looks very promising. When you are ready to submit patches, this will need API review <https://cwiki.apache.org/confluence/display/TS/API+Review+Process>.
Actually, Susan forgot to mention that you can review the code at
https://github.com/shinrich/trafficserver/tree/ts-3006
> There's already and API to get the SSL *, TSHttpSsnSSLConnectionGet. I don't see the need for a TSSslVConn, the TSHttpSsn should be sufficient.
I don't think that works, because the pre handshake hook is called before a client session exists so TSHttpSsnSSLConnectionGet() will fail in that context.
> Are TS_SSL_CLIENT_PRE_HANDSHAKE_HOOK and TS_SSL_SNI_HOOK session hooks? If so, they should follow the session hook naming conventions. If not, how do do register these hooks?
Not really, as there's no client session instance at that time. Both of these hooks are called before any of the session accept logic is executed. This makes for some ugly choices in the API.
> Previously, then we looked at SSL integration APIs, we eschewed wrapped types like TSSslContext in favour of directly returning OpenSSL pointers (as void *). My argument at the time was that there's a lot of API surface needed to deal with SSL and not much value in just wrapping OpenSSL.
I recommended that because there was a discussion about potentially using a different SSL library. The expectation is that these wrapper types can be directly cast to a SSL library type, which is dependent on how ATS was built.
> There's no TSSslVConnOp value for the default action? ie. to just accept the SSL session?
In that case, don't call TSSslVConnOpSet. But I suppose you might want to undo, so that should be added.
> IIRC, OpenSSL doesn't guarantee anything about the SNI name except that is is a bag of bytes. Is it OK for TSSslVConnServernameGet() to present that as a C string?
Ah, yes. It should return a void* and size value. There's also an issue on what maximum length can be presumed and how storage for that is handled. The spec, as I read it, puts a limit of 64K, but in practice openSSL seems to limit it to 256 (TS_MAX_HOST_NAME_LEN).
> Finally, I'm not thrilled with the idea of using ssl_multicert.config to specify the tunneling action. What would you do if you had a wildcard cert and only wanted to tunnel a specific name?
Write a plugin :-).
Re: SSL support extensions - design review request
Posted by James Peach <jp...@apache.org>.
On Aug 19, 2014, at 7:50 AM, Susan Hinrichs <sh...@network-geographics.com> wrote:
> Good morning, all.
>
> I've been working on implementing the SSL plugin extensions described in TS-3006. Documentation is at http://network-geographics.com/ats/docs/ssl-api.en.html. The features have been implemented, and I am in the testing stage right now.
Thanks for the docs, this looks very promising. When you are ready to submit patches, this will need API review <https://cwiki.apache.org/confluence/display/TS/API+Review+Process>.
> Looking for feedback on the design. Please comment on the bug or reply to the mailing list.
There's already and API to get the SSL *, TSHttpSsnSSLConnectionGet. I don't see the need for a TSSslVConn, the TSHttpSsn should be sufficient.
The type of SSL_callback is TSEventFunc, right?
Why do TSSslCertFindByName() and TSSslCertFindByAddress() take a TSSslVConn argument?
Are TS_SSL_CLIENT_PRE_HANDSHAKE_HOOK and TS_SSL_SNI_HOOK session hooks? If so, they should follow the session hook naming conventions. If not, how do do register these hooks?
Ok, in the examples I see TSSslHookAdd ... could this be implemented with session hooks?
Previously, then we looked at SSL integration APIs, we eschewed wrapped types like TSSslContext in favour of directly returning OpenSSL pointers (as void *). My argument at the time was that there's a lot of API surface needed to deal with SSL and not much value in just wrapping OpenSSL.
How do you get the client and server sockaddr for calling TSSslCertFindByAddress? This is another reason I'd prefer to use TSHttpSsn. Oh, wow, I did not know we had TSNetVConn*AddrGet :)
There's no TSSslVConnOp value for the default action? ie. to just accept the SSL session?
IIRC, OpenSSL doesn't guarantee anything about the SNI name except that is is a bag of bytes. Is it OK for TSSslVConnServernameGet() to present that as a C string?
Finally, I'm not thrilled with the idea of using ssl_multicert.config to specify the tunneling action. What would you do if you had a wildcard cert and only wanted to tunnel a specific name?
cheers,
James