tls: support changing credentials dynamically #23644

cjihrig · 2018-10-13T18:57:36Z

This commit adds a setSecureContext() method to TLS servers. In order to maintain backwards compatibility, the method takes the options needed to create a new SecureContext, rather than an instance of SecureContext.

Fixes: #4464
Refs: #10349
Refs: nodejs/help#603
Refs: #15115

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

Green CI: https://ci.nodejs.org/job/node-test-pull-request/17941/

nodejs-github-bot · 2018-10-13T18:57:37Z

@cjihrig build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1232/pipeline

bnoordhuis · 2018-10-15T11:16:21Z

lib/_tls_wrap.js

+                                  .update(process.argv.join(' '))
+                                  .digest('hex')
+                                  .slice(0, 32);
+  }


Can't you call this.setOptions(options)?

I did that originally, but setOptions() doesn't unset values that might have been passed on the first call, but omitted on subsequent calls. I also didn't want to change the behavior of setOptions().

EDIT: I guess one option would be to call setOptions() to set the options, and leave the undefined assignments in this function to clear old values.
EDIT2: Actually, setOptions() sets values unrelated to the secure context (requestCert, rejectUnauthorized, etc.), so I think I'd prefer to not call it from setSecureContext().

@bnoordhuis long term, I'd prefer to deprecate/remove setOptions() because it's undocumented and only used in the tls server constructor. The few fields that don't overlap with setSecureContext() could be inlined in the constructor.

bnoordhuis · 2018-10-15T11:18:32Z

lib/_tls_wrap.js

+    this.ticketKeys = options.ticketKeys;
+    this.setTicketKeys(this.ticketKeys);
+  } else {
+    this.setTicketKeys(this.getTicketKeys());


For my understanding: that means there is no way to disable ticket keys using .setSecureContext() if they've been enabled before?

I was trying to focus on maintaining any keys that were already set, and figured the {set,get}TicketKeys() APIs could be used for any additional configuration.

@cjihrig I'm somehow not understanding this code. get returns a buffer with the wrap->ticket_key_ data, and set copies its buffer argument into the wrap->ticket_key_ data... making this a no-op AFAICT. What am I missing? If the previous value of _sharedCreds (if any) had been saved, and that was used for the get, then I would understand @bnoordhuis's comment.

@sam-github I think it is just a mistake on my part. I just ran the test suite locally with that line commented out and everything passed.

OK, thanks for looking into it. I'm doing some session/ticket work, I'll remove the code in that PR, unless you want to.

If you want to include it in #25713, I think that would be fine.

sam-github · 2018-10-15T16:20:56Z

doc/api/tls.md

+  `ca`, etc).
+
+The `server.setSecureContext()` method replaces the secure context of an
+existing server.


What is the effect on current connections? Will those that have completed handshaking continue to be valid?

@sam-github existing connections should not be impacted. I've updated the docs to specify this, and updated the tests to validate it.

jasnell · 2018-10-16T21:30:16Z

lib/_tls_wrap.js

+  if (options.pfx)
+    this.pfx = options.pfx;
+  else
+    this.pfx = undefined;


It could get a bit messy with so many options, but the following pattern may save a bit of boilerplate code in this method...

Server.prototype.setSecureContext = function({ pfx = undefined, key = undefined, passphrase = undefined, cert = undefined, clientCertEngine = undefined, ca = undefined, secureProtocol = undefined, crl = undefined, ciphers = undefined, ecdhCurve = undefined, dhparam = undefined, honorCipherOrder = false, secureOptions, sessionIdContext } = {}) { this.pfx = pfx; this.key = key; // ... }

Not sure it's actually that much better tho ;-)

@jasnell I like that pattern, because I think it would be helpful for code coverage purposes. But I'm a little worried that it's just slightly different enough from the existing options handling (setOptions()) to cause problems. For example, right now falsy values get filtered out in most cases, but that would no longer be the case. The current structure also allows us to make the validation stricter, because right now it seems rather loose.

I already plan to follow this up with a deprecation PR for setOptions(). I'd like to also revisit the argument validation here, but that will be semver-major.

This commit adds a setSecureContext() method to TLS servers. In order to maintain backwards compatibility, the method takes the options needed to create a new SecureContext, rather than an instance of SecureContext. Fixes: nodejs#4464 Refs: nodejs#10349 Refs: nodejs/help#603 Refs: nodejs#15115 PR-URL: nodejs#23644 Reviewed-By: Ben Noordhuis <[email protected]>

targos · 2018-10-21T18:02:16Z

It looks like the new test test-tls-set-secure-context is flaky (timeouts): https://ci.nodejs.org/job/node-test-pull-request/18030/

This commit adds a setSecureContext() method to TLS servers. In order to maintain backwards compatibility, the method takes the options needed to create a new SecureContext, rather than an instance of SecureContext. Fixes: #4464 Refs: #10349 Refs: nodejs/help#603 Refs: #15115 PR-URL: #23644 Reviewed-By: Ben Noordhuis <[email protected]>

coolaj86 · 2018-12-22T08:39:24Z

Would it be reasonable to allow setSecureContext() to take an object created by createSecureContext()?

Either:

server.setSecureContext(secureContext);

Or:

server.setSecureContext({ secureContext: secureContext });

sam-github · 2019-04-26T19:48:53Z

Backported in #27432, to reduce TLS1.3 conflicts. PTAL if I did it correctly.

EDIT: I had to add min/maxversion back into the options (I guess due to out-of-order landing of features), and to remove a semver-major SNICallback type check.

This commit adds a setSecureContext() method to TLS servers. In order to maintain backwards compatibility, the method takes the options needed to create a new SecureContext, rather than an instance of SecureContext. Fixes: nodejs#4464 Refs: nodejs#10349 Refs: nodejs/help#603 Refs: nodejs#15115 PR-URL: nodejs#23644 Reviewed-By: Ben Noordhuis <[email protected]>

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

vsemozhetbyt added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 13, 2018

bnoordhuis reviewed Oct 15, 2018

View reviewed changes

sam-github reviewed Oct 15, 2018

View reviewed changes

cjihrig force-pushed the swap-credentials branch from b105ef4 to 44be199 Compare October 16, 2018 01:10

bnoordhuis approved these changes Oct 16, 2018

View reviewed changes

jasnell reviewed Oct 16, 2018

View reviewed changes

cjihrig force-pushed the swap-credentials branch from 44be199 to 96a986d Compare October 21, 2018 13:33

cjihrig merged commit 96a986d into nodejs:master Oct 21, 2018

cjihrig deleted the swap-credentials branch October 21, 2018 13:37

targos mentioned this pull request Oct 21, 2018

Investigate flaky test-tls-set-secure-context #23807

Closed

BethGriggs added the backport-requested-v10.x label Apr 8, 2019

sam-github mentioned this pull request Oct 17, 2019

https server should allow changing credentials dynamically #4464

Closed

sigseg1v mentioned this pull request Feb 10, 2020

HTTPS certificate should hot reload when updated sigseg1v/endless-expedition#52

Closed

Ethan-Arrowood mentioned this pull request Aug 6, 2024

Root certificate is prioritized over SNICallback context in HTTPS Server #54235

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: support changing credentials dynamically #23644

tls: support changing credentials dynamically #23644

cjihrig commented Oct 13, 2018 •

edited

Loading

nodejs-github-bot commented Oct 13, 2018

bnoordhuis Oct 15, 2018

cjihrig Oct 15, 2018 •

edited

Loading

cjihrig Oct 16, 2018

bnoordhuis Oct 15, 2018

cjihrig Oct 15, 2018

sam-github Jan 25, 2019

cjihrig Jan 25, 2019

sam-github Jan 25, 2019

cjihrig Jan 25, 2019

sam-github Oct 15, 2018

cjihrig Oct 16, 2018 •

edited

Loading

jasnell Oct 16, 2018 •

edited

Loading

cjihrig Oct 17, 2018

targos commented Oct 21, 2018

coolaj86 commented Dec 22, 2018 •

edited

Loading

sam-github commented Apr 26, 2019 •

edited

Loading

tls: support changing credentials dynamically #23644

tls: support changing credentials dynamically #23644

Conversation

cjihrig commented Oct 13, 2018 • edited Loading

Checklist

nodejs-github-bot commented Oct 13, 2018

Choose a reason for hiding this comment

cjihrig Oct 15, 2018 • edited Loading

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

Choose a reason for hiding this comment

cjihrig Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

jasnell Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Oct 21, 2018

coolaj86 commented Dec 22, 2018 • edited Loading

sam-github commented Apr 26, 2019 • edited Loading

cjihrig commented Oct 13, 2018 •

edited

Loading

cjihrig Oct 15, 2018 •

edited

Loading

cjihrig Oct 16, 2018 •

edited

Loading

jasnell Oct 16, 2018 •

edited

Loading

coolaj86 commented Dec 22, 2018 •

edited

Loading

sam-github commented Apr 26, 2019 •

edited

Loading