Replies inline
On 6/11/2015 12:28 PM, James Peach wrote:
On Jun 11, 2015, at 8:09 AM, Susan Hinrichs <shinr...@network-geographics.com>
wrote:
James, thanks for the comments. I had some old references and as you noted did
not discuss how memory is tracked through the API's.
Thanks Susan. I actually find it pretty difficult to review API changes from
documentation. It would be much easier to see the actual API diffs, with the
documentation as supporting material.
Looking at the diffs in your branch
- apidefs.h should not be pulling in openssl/ssl.h
You're probably right. I don't remember why I had that in. Will try
pulling it out.
- The TS_EVENT_SESSION_* names should be more specific, eg.
TS_EVENT_SSL_SESSION_GET
Sounds fine. Will update
- TSSslSessionID probably should be an opaque type
- TSSslSessionID_t should be removed
- The API type TSSslSessionID should not leak into SSLSessionCache.h
- The use of TSSslSessionID* an an input parameter is inconsistent with
other APIs
I'm trying to avoid a copy with the TSSslSessionID. Also the plugin
writer will need to dig into the session ID is some cases (e.g. to
communicate with other boxes or print the session). In an earlier
version, I had an API that took an opaque TSSslSessionID and a buffer
and pointer to length to copy out the session data, but that seemed kind
of clunky.
In the current branch, I've rearranged the internal SSLSessionID so it
inherits the data portion from TSSslSessionID. The C plugin writer can
interact with the POD TSSslSessionID. Since a SSLSessionID is-a
TSSslSessionID, we can just cast the internal data structure to be used
by the plugin, avoiding the copy unless the plugin writer needs the
session ID for a longer period. In that case, he can just make his own
copy.
I think that the TS_EVENT_SESSION_GET is interesting. There's a number of
things I could imagine a plugin wanting to do, but the way it is described this
is just a notification. If you have the SSL VC, then you could allow the plugin
to decide whether to use a cached session or not.
I played a bit with the idea of having another utility function to
over-ride the current cache selection, the TSSslSessionGetCurrent()
function. I didn't get that worked out though. Agreed if we could make
the sslVC available as well, then the plugin writer would have more
freedom to adjust.
AFAICT this is in support of only server-side session caching? What happens
with session tickets?
Yes, only hooking in on the server-side, session ID-based caching. The
current code only registered callbacks to decrypt the tickets. Might be
reasonable to hook in with the SESSION_GET plugin callback at that point
as well to give the plugin writer some visibility on that type of
connection reuse. Would probably want to pass in some indication that
this is coming from a ticket rather than the session cache.
I'm wondering whether this should really be a HTTP hook in the first place. If
you can use it to manage SSL sessions or tickets for a specific VC, then I
think it is. If it is only for events from the session cache, then maybe it is
not. For example, does it make sense to pass TS_EVENT_SSL_SESSION_GET to
TSHttpSsnHookAdd? That seems to be legal with this API construction, but not
particularly useful.
I selected TSHttpHookAdd primarily because the SNI callbacks used that
call to register those callbacks. I'm not familiar with the other
HookAdd's. This callback is not for a particular VC instance, but
rather for all SSL VC's if that makes a difference. Agreed that this
callback (and the SNI callbacks) don't really have anything to do with
the HTTP level.
Random code review comments
- SSLSessionBucket::getSessionBuffer should return the byte count so
you don't need the true_len output parameter
True, would be tidier
- How is a plugin supposed to know how large a buffer it needs for
TSSslSessionGetBuffer?
Pass in NULL for the buffer and use the return value to decide how big
the buffer should be. Or guess, and check the return value to see if it
should be larger.
I've updated the document
http://network-geographics.com/ats/docs/ssl-session-api.en.html
On my branch, I have a very simple example plugin which exercises the new hook
https://github.com/shinrich/trafficserver/blob/TS-3527/example/ssl-session/ssl-session.cc
I've also created a pull request on my working branch.
https://github.com/apache/trafficserver/pull/217https://github.com/apache/trafficserver/pull/217z
On 6/10/2015 6:11 PM, James Peach wrote:
On Jun 10, 2015, at 3:24 PM, Susan Hinrichs <shinr...@network-geographics.com>
wrote:
I haven't heard anything about this. We did discuss it at the Austin Summit.
There was general agreement to the idea. I've updated the document to reflect
what I have implemented. I will create a pull request with the code changes
later this evening.
The details of the API are at
http://network-geographics.com/ats/docs/ssl-session-api.en.html
Please share your comments and concerns.
How do you register this hook? What is TSSslSessionCurrentInsert()?
Many of the naming is inconsistent. For example, TSSslSessionId_t. We "Id" should be
spelled "ID", etc.
This API needs to talk about the lifecycle of the session and what locking is
involved. Who owns the object returned by TSSslSessionGet()? Does
TSSslSessionInsert() replace an existing entry?
On 3/18/2015 10:52 AM, Susan Hinrichs wrote:
The motivation is to leverage the core SSL session support of ATS while
enabling others to add things like multi-ATS session state sharing, additional
stats, additional checks, etc.
Proposed interface write up here
http://network-geographics.com/ats/docs/ssl-session-api.en.html
Please give me your comments.
Thanks,
Susan