Updated the pull request.
On 6/11/2015 1:57 PM, Susan Hinrichs wrote:
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
