Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

tls: throw an error on getLegacyCipher #14572

Closed
jasnell wants to merge 7 commits into from
Closed

tls: throw an error on getLegacyCipher #14572

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
41ec32f
tls: order of precedence on cipher list options, type checking
jasnell Apr 23, 2015
b450336
test: test-tls-cipher-list test error type
jasnell Apr 23, 2015
2d1d2a0
test: fix typo in tls.markdown doc and additional tests
jasnell Apr 28, 2015
14a26f9
tls: refactor test-tls-cipher-list and v0.10.38 legacy test
jasnell May 1, 2015
7f4d098
test: additional refinements in the test
jasnell May 5, 2015
30f46c3
test: additional round of edits on the test-tls-cipher-list.js
jasnell May 9, 2015
8a61f11
test: refactor default cipher list test
jasnell May 12, 2015
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
Prev Previous commit
Next Next commit
test: additional refinements in the test 
Per the latest round of feedback from julien.
Fix a couple of typos, add some explanatory text,
refactor the test case.
  • Loading branch information
@jasnell
jasnell committed May 5, 2015
commit 7f4d0985d34e283b9b148885ec57cfee20ea4fea
3 changes: 1 addition & 2 deletions doc/api/tls.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,7 @@ v0.10.38 behavior of only using the default cipher list on the server.
### Cipher List Precedence

Note that the `--enable-legacy-cipher-list`, `NODE_LEGACY_CIPHER_LIST`,
`--cipher-list` and `NODE_CIPHER_LIST` options are mutually exclusive. Only
_one_ should be used at a time.
`--cipher-list` and `NODE_CIPHER_LIST` options are mutually exclusive.

If the `NODE_CIPHER_LIST` and `NODE_LEGACY_CIPHER_LIST` environment variables
are both specified, the `NODE_LEGACY_CIPHER_LIST` setting will take precedence.
Expand Down
8 changes: 7 additions & 1 deletion lib/tls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1353,7 +1353,13 @@ exports.connect = function(/* [port, host], options, cb */) {
rejectUnauthorized: '0' !== process.env.NODE_TLS_REJECT_UNAUTHORIZED
};
if (!usingV1038Ciphers()) {
Copy link

@misterdjules misterdjules May 5, 2015

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.

// only set the default ciphers
// only set the default ciphers if we are _not_ using the
// v0.10.38 legacy cipher list. Node v0.10.38 had a bug
// that failed to set the default ciphers on the default
// options. This has been fixed in v0.10.39 and above.
// However, when the user explicitly tells node to revert
// back to using the v0.10.38 cipher list, node should
// revert back to the original v0.10.38 behavior.
defaults.ciphers = DEFAULT_CIPHERS;
}
options = util._extend(defaults, options || {});
Expand Down
23 changes: 15 additions & 8 deletions test/simple/test-tls-cipher-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ function doTest(checklist, additional_args, env) {
});
}

// test that the command line switchs takes precedence
// test that the command line switches takes precedence
// over the environment variables
function doTestPrecedence() {
// test that --cipher-list takes precedence over NODE_CIPHER_LIST
Expand All @@ -64,6 +64,10 @@ function doTestPrecedence() {

Copy link

@misterdjules misterdjules May 7, 2015

Choose a reason for hiding this comment

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

Style: unnecessary blank line.


// test that --enable-legacy-cipher-list takes precence over both envars
// note: in this release, there's only one legal value for the legacy
// switch so this test is largely a non-op. When multiple values
// are supported, this test should be changed to test that the
// command line switch actually does override
doTest(V1038Ciphers,
['--enable-legacy-cipher-list=v0.10.38'],
{
Expand All @@ -88,7 +92,7 @@ function doTestPrecedence() {
// test the right-most command line option takes precedence
doTest('XYZ',
[
'--cipher-list=XYZ',
'--cipher-list=ABC',
'--enable-legacy-cipher-list=v0.10.38',
'--cipher-list=XYZ'
]);
Expand Down Expand Up @@ -155,6 +159,10 @@ var script = (
var tls = require('tls');
var orig_createCredentials = require('crypto').createCredentials;
require('crypto').createCredentials = function(options) {
Copy link

@misterdjules misterdjules May 5, 2015

Choose a reason for hiding this comment

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

Instead of monkey patching crypto.createCredentials, what do you think about starting a server that sets its ciphers list to RC4-MD5 only?

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 --enable-legacy-cipher-list=v0.10.38 or setting equivalent environment variables.

I believe it would make the implementation of this test much simpler.

Copy link
Member Author

@jasnell jasnell May 5, 2015

Choose a reason for hiding this comment

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

That's not quite what this is testing. This tests to make sure the default
ciphers are not set on the options when v0.10.38 legacy is set. The monkey
patch allows us to intercept the call and verify that the ciphers property
is indeed not set.
On May 4, 2015 6:05 PM, "Julien Gilli" [email protected] wrote:

In test/simple/test-tls-cipher-list.js
#14572 (comment):

+// 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) {

Instead of monkey patching crypto.createCredentials, what do you think
about starting a server that sets its ciphers list to RC4-MD5 only?

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 --enable-legacy-cipher-list=v0.10.38 or setting equivalent
environment variables.

I believe it would make the implementation of this test much simpler.

—
Reply to this email directly or view it on GitHub
https://github.com/joyent/node/pull/14572/files#r29638490.

Copy link

@misterdjules misterdjules May 7, 2015

Choose a reason for hiding this comment

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

OK, thank you for the clarification. I would suggest also asserting that the monkey-patched version of crypto.createCredentials was called to avoid missing potential future regressions if crypto.createCredentials ends up being refactored and is not called anymore (like in v0.12).

Also, do we really need to spawn a server to run that test? If I understand correctly, crypto.createCredentials is called before any connection is initiated, so I would expect that we can perform the same test without starting a server.

Copy link
Member Author

@jasnell jasnell May 8, 2015

Choose a reason for hiding this comment

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

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.

Copy link

@misterdjules misterdjules May 11, 2015

Choose a reason for hiding this comment

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

@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 options.ciphers value passed to crypto.createCredentials was not expected. See my comment here: https://github.com/joyent/node/pull/14572/files#r30068923.

If I understand correctly, doDefaultCipherTest seems to test more than one thing. It tests that crypto.createCredentials is called with the proper value for its options.ciphers argument, and that a client that uses --enable-legacy-cipher-list can connect to a tls.Server instance started with the default options.

I think splitting that in two different simpler tests would make the tests more reliable and easier to understand.

// since node was started with the --enable-legacy-cipher-list
// switch equal to v0.10.38, the options.ciphers should be
// undefined. If it's not undefined, we have a problem and
// the test fails
if (options.ciphers !== undefined) {
console.error(options.ciphers);
process.exit(1);
Expand All @@ -172,7 +180,7 @@ var script = (

var test_count = 0;

function doDefaultCipherTest(additional_args, env, failexpected) {
function doDefaultCipherTest(additional_args, env, opts) {
var options = {};
if (env) options.env = env;
var out = '', err = '';
Expand All @@ -187,7 +195,7 @@ function doDefaultCipherTest(additional_args, env, failexpected) {
out += data;
}).
on('end', function() {
if (failexpected && err === '') {
if (opts.failExpected && err === '') {
Copy link

@misterdjules misterdjules May 11, 2015

Choose a reason for hiding this comment

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

Why is this test done in the listener for the 'end' event on the child process' standard output? The child seems to only write to the standard error output.

// 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');
Expand All @@ -199,7 +207,7 @@ function doDefaultCipherTest(additional_args, env, failexpected) {
}).
on('end', function() {
if (err !== '') {
if (!failexpected) {
if (!opts.failExpected) {
assert.fail(err.substr(0,err.length-1));
}
}
Expand All @@ -219,11 +227,10 @@ 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);
doDefaultCipherTest([], {}, {failedExpected:true});
// 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);
{failedExpected:true});
});