crypto: add ticketKeys option in createSecureContext #20916

ryzokuken · 2018-05-23T21:06:23Z

There's a method to initialize a TLS Server using tls.createSever by
specifying a ticketKeys option, but none in the underlying constructor,
tls.createSecureContext.

This PR adds the ticketKeys option to tls.createSecureContext.

Fixes: #20908

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

/cc @nodejs/crypto @bnoordhuis @silverwind @DiegoTUI

I guess this qualifies as a semver-minor?

There's a method to initialize a TLS Server using tls.createSever by specifying a ticketKeys option, but none in the underlying constructor, tls.createSecureContext. This PR adds the ticketKeys option to tls.createSecureContext. Fixes: nodejs#20908

ryzokuken · 2018-05-23T21:06:38Z

Running tests locally.

silverwind · 2018-05-23T21:27:56Z

Change seems good to me. Couly you update the docs and add a test that verifies that both createServer and createSecureContext honor the option (compare the given buffer with the output of .getTicketKeys())?

ryzokuken · 2018-05-23T21:36:14Z

@silverwind exactly what I had in mind once the tests passed. I guess I'd also have to add a "Changes" entry to the docs.

ryzokuken · 2018-05-24T07:43:39Z

Tests pass. Let me add the docs for this.

DiegoTUI · 2018-05-24T08:42:38Z

Other than the docs, I think it also makes sense to add the methods getTicketKeys() and setTicketKeys(keys) to SecureContext. tls.Server has them and it ends up getting the keys from the context. Both methods are pretty convenient for sharing keys among contexts.

ryzokuken · 2018-05-24T08:52:37Z

@DiegoTUI that does sound like a logical thing to do, thanks. I will add those today.

If I had any doubts regarding the change, this makes it certain that it'll indeed be a semver-minor.

bnoordhuis

Needs documentation and tests. It's not useful to expose this without a documented way to:

get the ticket keys
configure the session timeout

See tls.Server#getTicketKeys() and the sessionTimeout option to tls.createServer().

bnoordhuis · 2018-05-24T08:31:40Z

lib/_tls_common.js

@@ -223,6 +223,11 @@ exports.createSecureContext = function createSecureContext(options, context) {
                                   options.clientCertEngine);
  }

+  // Set ticketKeys right inside createSecureContext


Superfluous comment.

Will remove it, in that case.

bnoordhuis · 2018-05-24T08:32:34Z

lib/_tls_wrap.js

@@ -885,7 +885,8 @@ function Server(options, listener) {
    secureOptions: this.secureOptions,
    honorCipherOrder: this.honorCipherOrder,
    crl: this.crl,
-    sessionIdContext: this.sessionIdContext
+    sessionIdContext: this.sessionIdContext,
+    ticketKeys: this.ticketKeys


Can you add a trailing comma?

I certainly will! I didn't add one because there wasn't one to begin with 😅

silverwind · 2018-05-24T17:36:27Z

Noticed that some documentation links to the ticketKeys option of createServe in tls.md, those references have to be updated.

ryzokuken · 2018-05-24T19:54:48Z

ryzokuken · 2018-05-24T20:19:50Z

@bnoordhuis @silverwind No docs or tests still, but added the required functions and option, so PTAL

bnoordhuis · 2018-05-25T06:46:13Z

lib/_tls_common.js

@@ -56,6 +56,15 @@ function SecureContext(secureProtocol, secureOptions, context) {
  if (secureOptions) this.context.setOptions(secureOptions);
 }

+SecureContext.prototype.getTicketKeys = function getTicketKeys(keys) {
+  return this.context.getTicketKeys(keys);


I realize you copied this from elsewhere but this method doesn't take any arguments, it returns a new buffer.

@bnoordhuis quite right, I actually took my cues from https://github.com/ryzokuken/node/blob/6269853a9a444a3fbed351ddeb12a6e97114543d/lib/_tls_wrap.js#L928

I'd look into the original context's getTicketKeys() method and try working with that one.

Umm, it can take arguments? Idk, the code was:

void SecureContext::GetTicketKeys(const FunctionCallbackInfo<Value>& args) { #if !defined(OPENSSL_NO_TLSEXT) && defined(SSL_CTX_get_tlsext_ticket_keys) SecureContext* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); Local<Object> buff = Buffer::New(wrap->env(), 48).ToLocalChecked(); memcpy(Buffer::Data(buff), wrap->ticket_key_name_, 16); memcpy(Buffer::Data(buff) + 16, wrap->ticket_key_hmac_, 16); memcpy(Buffer::Data(buff) + 32, wrap->ticket_key_aes_, 16); args.GetReturnValue().Set(buff); #endif // !def(OPENSSL_NO_TLSEXT) && def(SSL_CTX_get_tlsext_ticket_keys) }

Anyway, I think most if not all people call it without any args plus no arguments mentioned in the docs, so I'll make the change.

You mean args? Any C++ API function takes that as its argument.

If you're referring to args.Holder(), that's the this object. Positional arguments are accessed using array notation, i.e., args[0].

@bnoordhuis TIL, I guess 😅 Not super good at the C++ part of things.

In that case, getTicketKeys indeed doesn't use any arguments while setTicketKeys takes one, perfect.

ryzokuken · 2018-05-26T05:57:27Z

@bnoordhuis added a few tests, they're running locally. PTAL and let me know if I missed anything.

ryzokuken · 2018-05-26T06:03:01Z

I needed deepStrictEqual instead! Made the change, will push alongside others, please ignore that part.

ryzokuken · 2018-05-26T07:35:31Z

@bnoordhuis I have added the sessionTimeout option, but cannot decide how to test it. I could either add a getter for it and assert the value from the getter with the one I enter at the constructor or do something closer to the real world but a tad hacky like what we do at test-tls-session-timeout.js.

test-tls-session-timeout.js

'use strict';
const common = require('../common');

if (!common.opensslCli)
  common.skip('node compiled without OpenSSL CLI.');

if (!common.hasCrypto)
  common.skip('missing crypto');

const tmpdir = require('../common/tmpdir');

doTest();

// This test consists of three TLS requests --
// * The first one should result in a new connection because we don't have
//   a valid session ticket.
// * The second one should result in connection resumption because we used
//   the session ticket we saved from the first connection.
// * The third one should result in a new connection because the ticket
//   that we used has expired by now.

function doTest() {
  const assert = require('assert');
  const tls = require('tls');
  const fs = require('fs');
  const join = require('path').join;
  const fixtures = require('../common/fixtures');
  const spawn = require('child_process').spawn;

  const SESSION_TIMEOUT = 1;

  const key = fixtures.path('agent.key');
  const cert = fixtures.path('agent.crt');
  const options = {
    key: key,
    cert: cert,
    ca: [cert],
    sessionTimeout: SESSION_TIMEOUT
  };

  // We need to store a sample session ticket in the fixtures directory because
  // `s_client` behaves incorrectly if we do not pass in both the `-sess_in`
  // and the `-sess_out` flags, and the `-sess_in` argument must point to a
  // file containing a proper serialization of a session ticket.
  // To avoid a source control diff, we copy the ticket to a temporary file.

  const sessionFileName = (function() {
    const ticketFileName = 'tls-session-ticket.txt';
    const tmpPath = join(tmpdir.path, ticketFileName);
    fs.writeFileSync(tmpPath, fixtures.readSync(ticketFileName));
    return tmpPath;
  }());

  // Expects a callback -- cb(connectionType : enum ['New'|'Reused'])

  function Client(cb) {
    const flags = [
      's_client',
      '-connect', `localhost:${common.PORT}`,
      '-sess_in', sessionFileName,
      '-sess_out', sessionFileName
    ];
    const client = spawn(common.opensslCli, flags, {
      stdio: ['ignore', 'pipe', 'ignore']
    });

    let clientOutput = '';
    client.stdout.on('data', function(data) {
      clientOutput += data.toString();
    });
    client.on('exit', function(code) {
      let connectionType;
      const grepConnectionType = (line) => {
        const matches = line.match(/(New|Reused), /);
        if (matches) {
          connectionType = matches[1];
          return true;
        }
      };
      const lines = clientOutput.split('\n');
      if (!lines.some(grepConnectionType)) {
        throw new Error('unexpected output from openssl client');
      }
      cb(connectionType);
    });
  }

  const server = tls.createServer(options, function(cleartext) {
    cleartext.on('error', function(er) {
      if (er.code !== 'ECONNRESET')
        throw er;
    });
    cleartext.end();
  });

  server.listen(common.PORT, function() {
    Client(function(connectionType) {
      assert.strictEqual(connectionType, 'New');
      Client(function(connectionType) {
        assert.strictEqual(connectionType, 'Reused');
        setTimeout(function() {
          Client(function(connectionType) {
            assert.strictEqual(connectionType, 'New');
            server.close();
          });
        }, (SESSION_TIMEOUT + 1) * 1000);
      });
    });
  });
}

bnoordhuis · 2018-05-26T07:53:36Z

You could test sessionTimeout by setting it to 1s, creating a ticket, then checking that it's gone after one second.

ryzokuken · 2018-05-26T16:55:22Z

@bnoordhuis I assigned the sessionTimeout property in a way similar to how we did it in tls.Server, but it probably doesn't work. Pushing the failing test (or passing, in this case, but it shouldn't) for inspection.

ryzokuken · 2018-05-26T19:14:15Z

Built Node in debug mode and checked it out myself, SSL_CTX_set_timeout is infact being called. I wonder what else is wrong?

Breakpoint and Backtrace

~/Code/nodejs/node crypto-20908*
❯ lldb -- ./out/Debug/node test/parallel/test-tls-securecontext-ticketkeys.js
(lldb) target create "./out/Debug/node"
Current executable set to './out/Debug/node' (x86_64).
(lldb) settings set -- target.run-args  "test/parallel/test-tls-securecontext-ticketkeys.js"
(lldb) breakpoint set --name SSL_CTX_set_timeout
Breakpoint 1: where = node`SSL_CTX_set_timeout + 12 at ssl_sess.c:916, address = 0x0000000101a910bc
(lldb) r
Process 56423 launched: './out/Debug/node' (x86_64)
Process 56423 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x0000000101a910bc node`SSL_CTX_set_timeout(s=0x00000001053064b0, t=1) at ssl_sess.c:916
   913 	long SSL_CTX_set_timeout(SSL_CTX *s, long t)
   914 	{
   915 	    long l;
-> 916 	    if (s == NULL)
   917 	        return (0);
   918 	    l = s->session_timeout;
   919 	    s->session_timeout = t;
Target 0: (node) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x0000000101a910bc node`SSL_CTX_set_timeout(s=0x00000001053064b0, t=1) at ssl_sess.c:916
    frame #1: 0x00000001002b5d44 node`node::crypto::SecureContext::SetSessionTimeout(args=0x00007ffeefbfcf70) at node_crypto.cc:1025
    frame #2: 0x0000000100607cb3 node`v8::internal::FunctionCallbackArguments::Call(this=0x00007ffeefbfd0d0, handler=0x00003969690bdd41) at api-arguments.cc:29
    frame #3: 0x00000001007283ee node`v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(isolate=0x0000000105804c00, function=Handle<v8::internal::HeapObject> @ 0x00007ffeefbfd1e0, new_target=Handle<v8::internal::HeapObject> @ 0x00007ffeefbfd1d8, fun_data=Handle<v8::internal::FunctionTemplateInfo> @ 0x00007ffeefbfd1d0, receiver=Handle<v8::internal::Object> @ 0x00007ffeefbfd1c8, args=BuiltinArguments @ 0x00007ffeefbfd200) at builtins-api.cc:108
    frame #4: 0x0000000100726d0a node`v8::internal::Builtin_Impl_HandleApiCall(args=BuiltinArguments @ 0x00007ffeefbfd330, isolate=0x0000000105804c00) at builtins-api.cc:138
    frame #5: 0x000000010072683d node`v8::internal::Builtin_HandleApiCall(args_length=6, args_object=0x00007ffeefbfd410, isolate=0x0000000105804c00) at builtins-api.cc:126
    frame #6: 0x00001f0f67a042c4
    frame #7: 0x00001f0f67a1cf78
    frame #8: 0x00001f0f67a108e3
    frame #9: 0x00001f0f67a1cf78
    frame #10: 0x00001f0f67a1cf78
    frame #11: 0x00001f0f67a1cf78
    frame #12: 0x00001f0f67a1cf78
    frame #13: 0x00001f0f67a1cf78
    frame #14: 0x00001f0f67a1cf78
    frame #15: 0x00001f0f67a1cf78
    frame #16: 0x00001f0f67a1cf78
    frame #17: 0x00001f0f67a1cf78
    frame #18: 0x00001f0f67a16e15
    frame #19: 0x00001f0f67a0b581
    frame #20: 0x0000000100fd0efd node`v8::internal::GeneratedCode<v8::internal::Object*, v8::internal::Object*, v8::internal::Object*, v8::internal::Object*, int, v8::internal::Object***>::Call(this=0x00007ffeefbfdd50, args=0x00003969a12822e1, args=0x000039699cc0e4d9, args=0x00003969a1282201, args=2, args=0x00007ffeefbfe3e0) at simulator.h:110
    frame #21: 0x0000000100fcf182 node`v8::internal::(anonymous namespace)::Invoke(isolate=0x0000000105804c00, is_construct=false, target=Handle<v8::internal::Object> @ 0x00007ffeefbfde70, receiver=Handle<v8::internal::Object> @ 0x00007ffeefbfde68, argc=2, args=0x00007ffeefbfe3e0, new_target=Handle<v8::internal::Object> @ 0x00007ffeefbfde60, message_handling=kReport, execution_target=kCallable) at execution.cc:153
    frame #22: 0x0000000100fcea03 node`v8::internal::(anonymous namespace)::CallInternal(isolate=0x0000000105804c00, callable=Handle<v8::internal::Object> @ 0x00007ffeefbfdf40, receiver=Handle<v8::internal::Object> @ 0x00007ffeefbfdf38, argc=2, argv=0x00007ffeefbfe3e0, message_handling=kReport, target=kCallable) at execution.cc:189
    frame #23: 0x0000000100fce89d node`v8::internal::Execution::Call(isolate=0x0000000105804c00, callable=Handle<v8::internal::Object> @ 0x00007ffeefbfdfa0, receiver=Handle<v8::internal::Object> @ 0x00007ffeefbfdf98, argc=2, argv=0x00007ffeefbfe3e0) at execution.cc:200
    frame #24: 0x00000001005b0f48 node`v8::Function::Call(this=0x000000010703ea68, context=(val_ = 0x00000001058216e0), recv=(val_ = 0x0000000105804c78), argc=2, argv=0x00007ffeefbfe3e0) at api.cc:5077
    frame #25: 0x000000010008a872 node`node::ExecuteBootstrapper(env=0x00007ffeefbfe4f0, bootstrapper=(val_ = 0x000000010703ea68), argc=2, argv=0x00007ffeefbfe3e0, out=0x00007ffeefbfe258) at node.cc:3067
    frame #26: 0x0000000100089a0c node`node::LoadEnvironment(env=0x00007ffeefbfe4f0) at node.cc:3167
    frame #27: 0x000000010009460e node`node::Start(isolate=0x0000000105804c00, isolate_data=0x0000000107040400, argc=2, argv=0x0000000105200050, exec_argc=0, exec_argv=0x00000001052001f0) at node.cc:4256
    frame #28: 0x000000010008f19b node`node::Start(event_loop=0x0000000102da7a40, argc=2, argv=0x0000000105200050, exec_argc=0, exec_argv=0x00000001052001f0) at node.cc:4363
    frame #29: 0x000000010008e7c5 node`node::Start(argc=2, argv=0x0000000105200050) at node.cc:4414
    frame #30: 0x0000000101cb912e node`main(argc=2, argv=0x00007ffeefbff170) at node_main.cc:124
    frame #31: 0x0000000100001034 node`start + 52
(lldb) c
Process 56423 resuming
<Buffer 38 67 52 0b 46 dc 16 f1 df df cf 62 81 f8 a0 3b ed 45 27 75 d4 f9 9e 00 2d 5b fc 1d 8a 3e 60 20 eb ae ea 7f 31 45 0d f8 24 f6 4a 48 6d e1 ca 04> <Buffer 38 67 52 0b 46 dc 16 f1 df df cf 62 81 f8 a0 3b ed 45 27 75 d4 f9 9e 00 2d 5b fc 1d 8a 3e 60 20 eb ae ea 7f 31 45 0d f8 24 f6 4a 48 6d e1 ca 04>
Process 56423 exited with status = 0 (0x00000000)
(lldb) exit

ryzokuken · 2018-05-28T14:12:59Z

@nodejs/crypto @bnoordhuis PTAL.

ryzokuken · 2018-05-31T10:14:27Z

@nodejs/crypto anyone willing to give this a shot with me during the summit?

P.S. giving this a shot == helping me debug this, I'm clueless.

ryzokuken · 2018-06-10T18:25:38Z

ping @nodejs/crypto please help?

tniessen · 2018-06-10T21:11:34Z

Sorry, didn't have time to look at this yet, and I am not that confident with the TLS APIs of OpenSSL. Are you sure that the test case is correct?

Edit: Also, I believe the subsystem should be tls, not crypto.

ryzokuken · 2018-06-11T02:11:12Z

@tniessen that's what I'm afraid of. I'm so bad with the specifics of this that I don't know if the test case is correct. Let me try looking deeper too.

jasnell · 2018-09-10T18:15:30Z

Ping @ryzokuken

Trott · 2018-11-20T21:10:45Z

@ryzokuken Still working on this?

BridgeAR · 2019-03-10T00:12:42Z

Closing due to long inactivity. @ryzokuken please reopen / open a new PR in case you want to continue working on this.

ryzokuken · 2019-03-10T17:42:26Z

I'm sorry I put this in the backseat, @jasnell @Trott @BridgeAR. Hopefully will publish a fix later.

nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label May 23, 2018

ryzokuken mentioned this pull request May 23, 2018

Unable to share ticketKeys in secureContext #20908

Closed

ryzokuken mentioned this pull request May 24, 2018

test: test-async-wrap-pop-id-during-load failure on mac #20888

Closed

bnoordhuis reviewed May 24, 2018

View reviewed changes

silverwind added the semver-minor PRs that contain new features and should be released in the next minor version. label May 24, 2018

fixup! crypto: add ticketKeys option in createSecureContext

6269853

bnoordhuis reviewed May 25, 2018

View reviewed changes

fixup! crypto: add ticketKeys option in createSecureContext

38e22dc

fixup! crypto: add ticketKeys option in createSecureContext

3964032

jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018

BridgeAR closed this Mar 10, 2019

sam-github mentioned this pull request Jun 3, 2020

A possible TODO list for new (or current) contributors #33715

Closed

mkrawczuk mentioned this pull request Jun 19, 2020

tls: make 'createSecureContext' honor more options. #33974

Closed

3 tasks

crypto: add ticketKeys option in createSecureContext #20916

crypto: add ticketKeys option in createSecureContext #20916

Conversation

ryzokuken commented May 23, 2018 • edited Loading

Checklist

ryzokuken commented May 23, 2018

silverwind commented May 23, 2018

ryzokuken commented May 23, 2018

ryzokuken commented May 24, 2018

DiegoTUI commented May 24, 2018

ryzokuken commented May 24, 2018

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind commented May 24, 2018 • edited Loading

ryzokuken commented May 24, 2018 • edited Loading

Helpful checklist:

ryzokuken commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented May 26, 2018

ryzokuken commented May 26, 2018 • edited Loading

ryzokuken commented May 26, 2018 • edited Loading

bnoordhuis commented May 26, 2018

ryzokuken commented May 26, 2018

ryzokuken commented May 26, 2018 • edited Loading

ryzokuken commented May 28, 2018

ryzokuken commented May 31, 2018 • edited Loading

ryzokuken commented Jun 10, 2018

tniessen commented Jun 10, 2018 • edited Loading

ryzokuken commented Jun 11, 2018

jasnell commented Sep 10, 2018

Trott commented Nov 20, 2018

BridgeAR commented Mar 10, 2019

ryzokuken commented Mar 10, 2019

ryzokuken commented May 23, 2018 •

edited

Loading

silverwind commented May 24, 2018 •

edited

Loading

ryzokuken commented May 24, 2018 •

edited

Loading

ryzokuken commented May 26, 2018 •

edited

Loading

ryzokuken commented May 26, 2018 •

edited

Loading

ryzokuken commented May 26, 2018 •

edited

Loading

ryzokuken commented May 31, 2018 •

edited

Loading

tniessen commented Jun 10, 2018 •

edited

Loading