tls: throw an error on getLegacyCipher #14572
Per the feedback from Julien, refactor the test-tls-cipher-list test to avoid using the confusing switch statement. Add tests to ensure that using `--enable-legacy-cipher-list=v0.10.38` or `NODE_LEGACY_CIPHER_LIST=v0.10.38` disables the use of the default cipher list but setting the `--cipher-list` equal to the v0.10.38 list explicitly does not. Minor refactor to tls.js to ensure the above behavior.
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,84 +19,21 @@ | |
| // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE | ||
| // USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
|
|
||
| var spawn = require('child_process').spawn; | ||
|
There was a problem hiding this comment. Style: the current style used does not align the |
||
| var assert = require('assert'); | ||
| var tls = require('tls'); | ||
| var crypto = process.binding('crypto'); | ||
| var common = require('../common'); | ||
| var fs = require('fs'); | ||
|
|
||
| var V1038Ciphers = tls.getLegacyCiphers('v0.10.38'); | ||
|
|
||
| function doTest(checklist, additional_args, env) { | ||
| var options; | ||
| if (env) options = {env:env}; | ||
| additional_args = additional_args || []; | ||
| var args = additional_args.concat([ | ||
| '-e', 'console.log(process.binding(\'crypto\').DEFAULT_CIPHER_LIST)']); | ||
| var out = ''; | ||
| spawn(process.execPath, args, options). | ||
| stdout. | ||
|
|
@@ -108,27 +45,88 @@ function doTest(checklist, env, useswitch) { | |
| }); | ||
| } | ||
|
|
||
| // test that the command line switchs takes precedence | ||
|
|
||
| // over the environment variables | ||
| function doTestPrecedence() { | ||
| // test that --cipher-list takes precedence over NODE_CIPHER_LIST | ||
| doTest('ABC', ['--cipher-list=ABC'], {'NODE_CIPHER_LIST': 'XYZ'}); | ||
|
|
||
| // test that --enable-legacy-cipher-list takes precedence | ||
| // over NODE_CIPHER_LIST | ||
| doTest(V1038Ciphers, | ||
| ['--enable-legacy-cipher-list=v0.10.38'], | ||
| {'NODE_CIPHER_LIST': 'XYZ'}); | ||
|
|
||
| // test that --cipher-list takes precedence over NODE_LEGACY_CIPHER_LIST | ||
| doTest('ABC', | ||
| ['--cipher-list=ABC'], | ||
| {'NODE_LEGACY_CIPHER_LIST': 'v0.10.38'}); | ||
|
|
||
|
|
||
|
|
||
| // test that --enable-legacy-cipher-list takes precence over both envars | ||
| doTest(V1038Ciphers, | ||
| ['--enable-legacy-cipher-list=v0.10.38'], | ||
| { | ||
| 'NODE_LEGACY_CIPHER_LIST': 'v0.10.38', | ||
|
There was a problem hiding this comment. Don't we want another value for
Member
Author
There was a problem hiding this comment. In v0.10.39 there is only one legal value for this. Passing in anything but
|
||
| 'NODE_CIPHER_LIST': 'XYZ' | ||
| }); | ||
|
|
||
| // test the right-most command line option takes precedence | ||
| doTest(V1038Ciphers, | ||
| [ | ||
| '--cipher-list=XYZ', | ||
| '--enable-legacy-cipher-list=v0.10.38' | ||
| ]); | ||
|
|
||
| // test the right-most command line option takes precedence | ||
| doTest('XYZ', | ||
| [ | ||
| '--enable-legacy-cipher-list=v0.10.38', | ||
| '--cipher-list=XYZ' | ||
| ]); | ||
|
|
||
| // test the right-most command line option takes precedence | ||
| doTest('XYZ', | ||
| [ | ||
| '--cipher-list=XYZ', | ||
|
There was a problem hiding this comment. Do we want another value for the first
Member
Author
There was a problem hiding this comment. Yeah, actually that's just a typo... it should have been different |
||
| '--enable-legacy-cipher-list=v0.10.38', | ||
| '--cipher-list=XYZ' | ||
| ]); | ||
|
|
||
| // test that NODE_LEGACY_CIPHER_LIST takes precedence over | ||
| // NODE_CIPHER_LIST | ||
|
|
||
|
|
||
| doTest(V1038Ciphers, [], | ||
| { | ||
| 'NODE_LEGACY_CIPHER_LIST': 'v0.10.38', | ||
| 'NODE_CIPHER_LIST': 'ABC' | ||
| }); | ||
|
|
||
| } | ||
|
|
||
| // Start running the tests... | ||
|
|
||
| doTest(crypto.DEFAULT_CIPHER_LIST); // test the default | ||
|
|
||
| // Test the NODE_CIPHER_LIST environment variable | ||
| doTest('ABC', [], {'NODE_CIPHER_LIST':'ABC'}); | ||
|
|
||
| // Test the --cipher-list command line switch | ||
| doTest('ABC', ['--cipher-list=ABC']); | ||
|
|
||
| // Test the --enable-legacy-cipher-list and NODE_LEGACY_CIPHER_LIST envar | ||
| ['v0.10.38'].forEach(function(arg) { | ||
| var checklist = tls.getLegacyCiphers(arg); | ||
| // command line switch | ||
| doTest(checklist, ['--enable-legacy-cipher-list=' + arg]); | ||
| // environment variable | ||
| doTest(checklist, [], {'NODE_LEGACY_CIPHER_LIST': arg}); | ||
| }); | ||
|
|
||
| // Test the precedence order for the various options | ||
| doTestPrecedence(); | ||
|
|
||
| // Test that we throw properly | ||
| // invalid value | ||
| assert.throws(function() {tls.getLegacyCiphers('foo');}, Error); | ||
| // no parameters | ||
|
|
@@ -139,3 +137,93 @@ assert.throws(function() {tls.getLegacyCiphers(1);}, TypeError); | |
| assert.throws(function() {tls.getLegacyCiphers('abc', 'extra');}, TypeError); | ||
| // ah, just right | ||
| assert.doesNotThrow(function() {tls.getLegacyCiphers('v0.10.38');}); | ||
|
|
||
|
|
||
|
|
||
| // Test to ensure default ciphers are not set when v0.10.38 legacy cipher | ||
| // switch is used. This is a bit involved... we need to first set up the | ||
| // TLS server, then spawn a second node instance using the v0.10.38 cipher, | ||
| // then connect and check to make sure the options are correct. Since there | ||
| // is no direct way of testing it, an alternate createCredentials shim is | ||
| // created that intercepts the call to createCredentials and checks the output. | ||
| // The following server code was adopted from test-tls-connect-simple. | ||
|
|
||
| // note that the following function is written out to a string and | ||
| // passed in as an argument to a child node instance. | ||
| var script = ( | ||
| function() { | ||
| var tls = require('tls'); | ||
| var orig_createCredentials = require('crypto').createCredentials; | ||
| require('crypto').createCredentials = function(options) { | ||
|
There was a problem hiding this comment. Instead of monkey patching Since it's explicitly excluded from the ciphers list after node v0.10.38, the test could make sure that the client connection can only be established when passing I believe it would make the implementation of this test much simpler.
Member
Author
There was a problem hiding this comment. That's not quite what this is testing. This tests to make sure the default
There was a problem hiding this comment. OK, thank you for the clarification. I would suggest also asserting that the monkey-patched version of Also, do we really need to spawn a server to run that test? If I understand correctly,
Member
Author
There was a problem hiding this comment. I spawn the server purposefully to do a full roundtrip to make sure that the connection still works without those defaults being set on the client side. There was a problem hiding this comment. @jasnell OK, I understand now. Generally, testing a single characteristic/behavior in a test helps make it clearer and avoid false negatives. In this case, the test doesn't make a difference between the child process writing to stderr because the client could not connect or because the If I understand correctly, I think splitting that in two different simpler tests would make the tests more reliable and easier to understand. |
||
| if (options.ciphers !== undefined) { | ||
| console.error(options.ciphers); | ||
| process.exit(1); | ||
| } | ||
| return orig_createCredentials(options); | ||
| }; | ||
| var socket = tls.connect({ | ||
| port: 0, | ||
| rejectUnauthorized: false | ||
| }, function() { | ||
| socket.end(); | ||
|
There was a problem hiding this comment. If the client cannot connect, an |
||
| }); | ||
| } | ||
| ).toString(); | ||
|
|
||
| var test_count = 0; | ||
|
|
||
| function doDefaultCipherTest(additional_args, env, failexpected) { | ||
|
There was a problem hiding this comment. Minor: instead of passing booleans, I find passing options objects easier to read. In this specific case: turns into |
||
| var options = {}; | ||
| if (env) options.env = env; | ||
| var out = '', err = ''; | ||
| additional_args = additional_args || []; | ||
| var args = additional_args.concat([ | ||
|
There was a problem hiding this comment. Another way to achieve the same thing would be to use That would require to have this test in a separate file, and I think this would make things simpler and cleaner overall. |
||
| '-e', require('util').format('(%s)()', script).replace( | ||
| 'port: 0', 'port: ' + common.PORT) | ||
| ]); | ||
| var child = spawn(process.execPath, args, options); | ||
| child.stdout. | ||
| on('data', function(data) { | ||
| out += data; | ||
| }). | ||
| on('end', function() { | ||
| if (failexpected && err === '') { | ||
| // if we get here, there's a problem because the default cipher | ||
| // list was not set when it should have been | ||
| assert.fail('options.cipher list was not set'); | ||
| } | ||
| }); | ||
| child.stderr. | ||
| on('data', function(data) { | ||
| err += data; | ||
| }). | ||
| on('end', function() { | ||
| if (err !== '') { | ||
| if (!failexpected) { | ||
| assert.fail(err.substr(0,err.length-1)); | ||
| } | ||
| } | ||
| }); | ||
| child.on('close', function() { | ||
| test_count++; | ||
| if (test_count === 4) server.close(); | ||
| }); | ||
| } | ||
|
|
||
| var options = { | ||
| key: fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem'), | ||
| cert: fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem') | ||
| }; | ||
| var server = tls.Server(options, function(socket) {}); | ||
| server.listen(common.PORT, function() { | ||
| doDefaultCipherTest(['--enable-legacy-cipher-list=v0.10.38']); | ||
| doDefaultCipherTest([], {'NODE_LEGACY_CIPHER_LIST': 'v0.10.38'}); | ||
| // this variant checks to ensure that the default cipher list IS set | ||
| var test_uses_default_cipher_list = true; | ||
| doDefaultCipherTest([], {}, test_uses_default_cipher_list); | ||
| // test that setting the cipher list explicitly to the v0.10.38 | ||
| // string without using the legacy cipher switch causes the | ||
| // default ciphers to be set. | ||
| doDefaultCipherTest(['--cipher-list=' + V1038Ciphers], {}, | ||
| test_uses_default_cipher_list); | ||
| }); | ||
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.
Maybe add a comment to clarify why there's an exception for v0.10.38 here? Otherwise I'm concerned it's going to be difficult to understand a few months/years from now.