tls: fix session resumption check

[indutny]

Before checking server's identity call to OpenSSL internals to ensure
that the session was not reused. In such case - certificate is going to
be empty and validation will crash.

Fix: #2304

P.S.

This is introducing big thing as a part of commit, which should probably be eventually exposed to user-land.

cc @nodejs/crypto

[indutny]

The problem with current implementation is that the Certificate is not sent in case of TLS Session Ticket renewal.

[shigeki]

@indutny As far as I look the patch, it seems that socket.isSessionReused() is better in resumption with ticket renewal in client but I could not figure out why this includes to add onticketkeycallback callback to fix the issue in a server side. Could you explain it in more detail?

[indutny]

@shigeki thanks for taking a look! The introduction of (private atm) onticketkeycallback is needed to force the TLS Ticket renewal in OpenSSL. Therefore it is used only in test at the moment.

[shigeki]

@indutny Thanks. I got it for it is only for the unit test. I think it is a pity to add a dead code only for testing. I will check this again later.

[indutny]

@shigeki this could be potentially exposed to the user-land, I can imagine that it might be useful in many cases. We just need to come up with API for it.

This is why I want it to be private for now.

[shigeki]

We should care r=0 case. Is it better return 0?

[indutny]

There are -1, 0, 1, 2 return values. It is not better to return 0, it is just use-case dependent.

[shigeki]

r=0 means

       0   This indicates that it was not possible to set/retrieve a session
           ticket and the SSL/TLS session will continue by by negiotationing a
           set of cryptographic parameters or using the alternate SSL/TLS
           resumption mechanism, session ids.

So when enableTicketKeyCallback is returned with r=0, does it cause issues in iniitalizing the following HMAC, EVP_Encrypt/EVP_Decrypt?

[indutny]

Yep, OpenSSL is using them when r = 0

[shigeki]

So I tried,

diff --git a/test/parallel/test-https-resume-after-renew.js b/test/parallel/test-https-resume-after-renew.js
index 23626cc..25e1e80 100644
--- a/test/parallel/test-https-resume-after-renew.js
+++ b/test/parallel/test-https-resume-after-renew.js
@@ -21,6 +21,7 @@ hmac.fill('H');

 server._sharedCreds.context.enableTicketKeyCallback();
 server._sharedCreds.context.onticketkeycallback = function(name, iv, enc) {
+/*
   if (enc) {
     var newName = new Buffer(16);
     var newIV = crypto.randomBytes(16);
@@ -31,6 +32,8 @@ server._sharedCreds.context.onticketkeycallback = function(name, iv, enc) {
   }

   return [ 1, hmac, aes, newName, newIV ];
+*/
+  return [0];
 };

causes

ohtsu@ubuntu:~/github/io.js$ ./iojs test/parallel/test-https-resume-after-renew.js
iojs: ../src/node_buffer.cc:196: size_t node::Buffer::Length(v8::Handle<v8::Value>): Assertion `val->IsObject()' failed.
Aborted (core dumped)

Is is better to avoid this?

[shigeki]

@indutny Sorry, this is buffer error. Not related crypt.

[shigeki]

@indutny return [0, hmac, aes, name, iv]; has no error. Never mind.

[indutny]

No worries. This function will be provided in core, and core will be responsible for setting return array right.

[shigeki]

I agree that add ticket callback is great as we can add a new feature such as ticket rotation. If it keeps private for some time , please put its comment somewhere.

[shigeki]

@indutny Put some comments to the patch.

[shigeki]

@indutny Through the discussions, I found that I could not imagine how much high level API will be provided through onticketkeycallback. I supposed that something like options.TicketKeyCallback = function(name, iv, enc) {..}; would be permitted and user can directly pass some of args.

The main issue of this fix to use socket.isSessionReused() is good to me. The current of TicketKeyCallback() is also good as long as it is used only for the test to fix this issue. but I can't say it will be good for the future use.
Do you mind to separate this into different PRs or do you still want to both in this one PR?

[indutny]

@shigeki I can split them into two commits. IMO, there is not much point in splitting PR into two. They will be dependent on each other anyway.

Does it sounds good?

[shigeki]

@indutny Yes, thanks very much for accepting my offer. LGTM.

[indutny]

Thank you @shigeki . I'll land it later today.

[indutny]

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/231/

@shigeki I just realized that I forgot to commit test. Sorry for this. PTAL

[indutny]

CI is green. @shigeki ;)

[shigeki]

Sorry, I've just gotten back to Hotel now. Landed in 6ad99ac and c399d17. Thank for great work. I hope to see your work of ticket rotation next :-)

[indutny]

Yay, thanks mate!