Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: support changing credentials dynamically #23644

Merged
merged 1 commit into from
Oct 21, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions doc/api/tls.md
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,18 @@ encryption/decryption of the [TLS Session Tickets][].
Starts the server listening for encrypted connections.
This method is identical to [`server.listen()`][] from [`net.Server`][].

### server.setSecureContext(options)
<!-- YAML
added: REPLACEME
-->

* `options` {Object} An object containing any of the possible properties from
the [`tls.createSecureContext()`][] `options` arguments (e.g. `key`, `cert`,
`ca`, etc).

The `server.setSecureContext()` method replaces the secure context of an
existing server. Existing connections to the server are not interrupted.

### server.setTicketKeys(keys)
<!-- YAML
added: v3.0.0
Expand Down
138 changes: 114 additions & 24 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -833,22 +833,11 @@ function Server(options, listener) {
// Handle option defaults:
this.setOptions(options);

this._sharedCreds = tls.createSecureContext({
pfx: this.pfx,
key: this.key,
passphrase: this.passphrase,
cert: this.cert,
clientCertEngine: this.clientCertEngine,
ca: this.ca,
ciphers: this.ciphers,
ecdhCurve: this.ecdhCurve,
dhparam: this.dhparam,
secureProtocol: this.secureProtocol,
secureOptions: this.secureOptions,
honorCipherOrder: this.honorCipherOrder,
crl: this.crl,
sessionIdContext: this.sessionIdContext
});
// setSecureContext() overlaps with setOptions() quite a bit. setOptions()
// is an undocumented API that was probably never intended to be exposed
// publicly. Unfortunately, it would be a breaking change to just remove it,
// and there is at least one test that depends on it.
this.setSecureContext(options);

this[kHandshakeTimeout] = options.handshakeTimeout || (120 * 1000);
this[kSNICallback] = options.SNICallback;
Expand All @@ -863,14 +852,6 @@ function Server(options, listener) {
'options.SNICallback', 'function', options.SNICallback);
}

if (this.sessionTimeout) {
this._sharedCreds.context.setSessionTimeout(this.sessionTimeout);
}

if (this.ticketKeys) {
this._sharedCreds.context.setTicketKeys(this.ticketKeys);
}

// constructor call
net.Server.call(this, tlsConnectionListener);

Expand All @@ -886,6 +867,115 @@ exports.createServer = function createServer(options, listener) {
};


Server.prototype.setSecureContext = function(options) {
if (options === null || typeof options !== 'object')
throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);

if (options.pfx)
this.pfx = options.pfx;
else
this.pfx = undefined;
Copy link
Member

@jasnell jasnell Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


if (options.key)
this.key = options.key;
else
this.key = undefined;

if (options.passphrase)
this.passphrase = options.passphrase;
else
this.passphrase = undefined;

if (options.cert)
this.cert = options.cert;
else
this.cert = undefined;

if (options.clientCertEngine)
this.clientCertEngine = options.clientCertEngine;
else
this.clientCertEngine = undefined;

if (options.ca)
this.ca = options.ca;
else
this.ca = undefined;

if (options.secureProtocol)
this.secureProtocol = options.secureProtocol;
else
this.secureProtocol = undefined;

if (options.crl)
this.crl = options.crl;
else
this.crl = undefined;

if (options.ciphers)
this.ciphers = options.ciphers;
else
this.ciphers = undefined;

if (options.ecdhCurve !== undefined)
this.ecdhCurve = options.ecdhCurve;
else
this.ecdhCurve = undefined;

if (options.dhparam)
this.dhparam = options.dhparam;
else
this.dhparam = undefined;

if (options.honorCipherOrder !== undefined)
this.honorCipherOrder = !!options.honorCipherOrder;
else
this.honorCipherOrder = true;

const secureOptions = options.secureOptions || 0;

if (secureOptions)
this.secureOptions = secureOptions;
else
this.secureOptions = undefined;

if (options.sessionIdContext) {
this.sessionIdContext = options.sessionIdContext;
} else {
this.sessionIdContext = crypto.createHash('sha1')
.update(process.argv.join(' '))
.digest('hex')
.slice(0, 32);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@cjihrig cjihrig Oct 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


this._sharedCreds = tls.createSecureContext({
pfx: this.pfx,
key: this.key,
passphrase: this.passphrase,
cert: this.cert,
clientCertEngine: this.clientCertEngine,
ca: this.ca,
ciphers: this.ciphers,
ecdhCurve: this.ecdhCurve,
dhparam: this.dhparam,
secureProtocol: this.secureProtocol,
secureOptions: this.secureOptions,
honorCipherOrder: this.honorCipherOrder,
crl: this.crl,
sessionIdContext: this.sessionIdContext
});

if (this.sessionTimeout)
this._sharedCreds.context.setSessionTimeout(this.sessionTimeout);

if (options.ticketKeys) {
this.ticketKeys = options.ticketKeys;
this.setTicketKeys(this.ticketKeys);
} else {
this.setTicketKeys(this.getTicketKeys());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
};


Server.prototype._getServerData = function() {
return {
ticketKeys: this.getTicketKeys().toString('hex')
Expand Down
88 changes: 88 additions & 0 deletions test/parallel/test-tls-set-secure-context.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
'use strict';
const common = require('../common');

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

const assert = require('assert');
const https = require('https');
const fixtures = require('../common/fixtures');
const credentialOptions = [
{
key: fixtures.readKey('agent1-key.pem'),
cert: fixtures.readKey('agent1-cert.pem'),
ca: fixtures.readKey('ca1-cert.pem')
},
{
key: fixtures.readKey('agent2-key.pem'),
cert: fixtures.readKey('agent2-cert.pem'),
ca: fixtures.readKey('ca2-cert.pem')
}
];
let requestsCount = 0;
let firstResponse;

const server = https.createServer(credentialOptions[0], (req, res) => {
requestsCount++;

if (requestsCount === 1) {
firstResponse = res;
firstResponse.write('multi-');
return;
} else if (requestsCount === 3) {
firstResponse.write('success-');
}

res.end('success');
});

server.listen(0, common.mustCall(async () => {
const { port } = server.address();
const firstRequest = makeRequest(port);

assert.strictEqual(await makeRequest(port), 'success');

server.setSecureContext(credentialOptions[1]);
firstResponse.write('request-');
await assert.rejects(async () => {
await makeRequest(port);
}, /^Error: self signed certificate$/);

server.setSecureContext(credentialOptions[0]);
assert.strictEqual(await makeRequest(port), 'success');

server.setSecureContext(credentialOptions[1]);
firstResponse.end('fun!');
await assert.rejects(async () => {
await makeRequest(port);
}, /^Error: self signed certificate$/);

assert.strictEqual(await firstRequest, 'multi-request-success-fun!');
server.close();
}));

function makeRequest(port) {
return new Promise((resolve, reject) => {
const options = {
rejectUnauthorized: true,
ca: credentialOptions[0].ca,
servername: 'agent1'
};

https.get(`https://localhost:${port}`, options, (res) => {
let response = '';

res.setEncoding('utf8');

res.on('data', (chunk) => {
response += chunk;
});

res.on('end', common.mustCall(() => {
resolve(response);
}));
}).on('error', (err) => {
reject(err);
});
});
}