-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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); | ||
|
||
|
@@ -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; | ||
|
||
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); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't you call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did that originally, but EDIT: I guess one option would be to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bnoordhuis long term, I'd prefer to deprecate/remove |
||
|
||
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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||
|
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); | ||
}); | ||
}); | ||
} |
There was a problem hiding this comment.
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...
Not sure it's actually that much better tho ;-)
There was a problem hiding this comment.
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.