TLS: Add asynchronous event listener suppport to 'newSession' event #7105

Closed
benurb opened this Issue Feb 12, 2014 · 11 comments

Projects

None yet

4 participants

@benurb
benurb commented Feb 12, 2014

Currently the TLS module offers support for asynchronous event listeners in the 'resumeSession' event, but not for 'newSession'. Because of that, one cannot use external storage with an asynchronous interface.

When one has consecutive requests the session data might not yet be stored after the first request and so later requests cannot resume the session. This leads for example to the following results with sslyze 0.8 testing against a cluster of one or two worker processes (the results between one and two workers do not differ much).

* Session Resumption :
  With Session IDs: Partially supported (2 successful, 3 failed, 0 errors, 5 total attempts)
  With TLS Session Tickets: Not Supported - TLS ticket assigned but not accepted.
* Resumption Rate with Session IDs : Partially supported (57 successful, 43 failed, 0 errors, 100 total attempts).

An immediatly following test leads to different results (especially for the TLS Session Ticket test):

* Session Resumption :
  With Session IDs: Partially supported (3 successful, 2 failed, 0 errors, 5 total attempts)
  With TLS Session Tickets: Supported
* Resumption Rate with Session IDs : Partially supported (52 successful, 48 failed, 0 errors, 100 total attempts).

When using a synchronous storage backend (meaning a local variable 😄 ), the result is like I expect it to be (so my implementation seems to be right):

* Session Resumption :
  With Session IDs: Supported (5 successful, 0 failed, 0 errors, 5 total attempts)
  With TLS Session Tickets: Supported
* Resumption Rate with Session IDs : Supported (100 successful, 0 failed, 0 errors, 100 total attempts).

Unfortunately my C/C++ knowledge converges to zero, so I cannot provide a pull request here. Nontheless, if someone decides to work on this, I will help as much as I can.

@indutny
Member
indutny commented Feb 12, 2014

@tjfontaine do we still have time to do it before v0.12?

@indutny indutny modified the milestone: v0.13, v0.12 Feb 12, 2014
@indutny
Member
indutny commented Feb 12, 2014

Ok, I'll try to do it before friday, if it won't go well - it'll be introduced in v0.13.

@benurb
benurb commented Feb 13, 2014

Thank you 👍

@indutny indutny added a commit to indutny/node that referenced this issue Feb 14, 2014
@indutny indutny tls: introduce asynchronous `newSession`
fix #7105
1803625
@indutny
Member
indutny commented Feb 14, 2014

Should be fixed by #7118

@diasdavid diasdavid added the tls label Feb 14, 2014
@indutny indutny closed this in 75ea11f Feb 17, 2014
@tlivings

We basically need the same thing for client connections. Wrapping createConnection lets us inject a session into a client connection but not asynchronously because createConnection must return the connection.

@indutny
Member
indutny commented Feb 25, 2014

I think you could load session before initiating connection, isn't that what you want?

@tlivings

Yup, that's what I do. But for example, if there is a centralized session store, it would be nice to be able to look it up at connection time, example:

var createConnection = agent.createConnection;

agent.createConnection = function (options, callback) {
    someAsyncMethod(function (session) {
        options.session = session;
        createConnection.call(agent, options, callback);
    });
};

tls.connect is sync and returns a connection, but rather than createConnection being sync as well, we could pass the connection back in a callback which would allow for async session lookup.

That being said, as you mentioned this could be done before hand to provide a synchronous lookup. The suggestion is more along the lines of a convenience mechanism so that you don't have to lookup everything in advance.

@indutny
Member
indutny commented Feb 25, 2014

I'm afraid the mechanism is totally different in server and client cases. In latter one, it is either core that will wait for user to provide session (all in javascript) and deferring actual connection, or user doing this explicitly. In my personal opinion, explicit is better :)

@tlivings

Yeah I realize I am piggy backing on someone else's issue — I was just following the commit and started typing. I'll take this up offline :)

@indutny
Member
indutny commented Feb 25, 2014

Are you going to Russia? ;)

@tlivings

Ha, I meant off-github :octocat:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment