tls: throw an error on getLegacyCipher#14574
Conversation
|
@misterdjules @mdawsonibm |
|
Can we add a test to make sure that |
|
Done |
|
One minor nit. Should the tests validate the type of the Error, ie a TypeError or an Error as appropriate ? Otherwise lgtm. |
1 similar comment
|
One minor nit. Should the tests validate the type of the Error, ie a TypeError or an Error as appropriate ? Otherwise lgtm. |
|
/cc @mhdawson @misterdjules ok, please check the two additional commits. if these look good I'll squash the pr down and land. |
This commit squashes down several previous commits on this change to clean up before landing. There are several changes to the new cipher list command line switches: 1. It includes the type checking fix on getLegacyCiphers 2. It has the command line switches override the environment variables 3. It documents it order of precedence 4. Expands the test case to test for order of precedence
test that the type of error thrown is correct
jasnell
force-pushed
the
error-on-getLegacyCiphers
branch
from
cb14192 to
a011e07
Compare
for consistency with the v0.10.38, if the v0.10.38 cipher list is being used, do not set the default ciphers on the options in connect.
|
@mhdawson @misterdjules ok, made the additional edits discussed and squashed the commits. Please verify. |
Fix a minor typo in the tls.markdown and add additional precedence tests in test-tls-cipher-list
There was a problem hiding this comment.
If one passes --cipher-list='ECDHE-RSA-AES128-SHA256:AES128-GCM-SHA256:RC4:HIGH:!MD5:!aNULL:!EDH', my understanding is that tls.DEFAULT_CIPHERS will === tls.getLegacyCiphers('v0.10.38')), and default.ciphers won't be set. I haven't tested it, so it may be a wrong assumption.
So I guess that here what is meant is more if (user didn't pass --enable-legacy-cipher-list=v0.10.38) { ... }, which is not equivalent to if (tls.DEFAULT_CIPHERS != tls.getLegacyCiphers('v0.10.38')).
Thoughts?
There was a problem hiding this comment.
Also, in general please use strict equality unless not possible. In this occurence, please use !== instead of !=.
There was a problem hiding this comment.
Yes, that is the case. I personally don't see this as a significant problem. If someone passes in the exact default cipher list as v0.10.38 then the likely intent is to revert the behavior back to v0.10.38, which means the default.ciphers not being set. It's an acceptable edge case IMHO but I'm good either way. It's a bit of a larger change tho since this does not yet specifically track whether --enable-legacy-cipher-list is passed vs. --cipher-list
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.
Per the latest round of feedback from julien. Fix a couple of typos, add some explanatory text, refactor the test case.
Per feedback from Julien.
Refactor the default cipher list test to separate concerns a bit, make the test clearer based on Julien's feedback.
|
Closing this as it cannot land here now. Will be refactoring this change and opening a new PR against master on nodejs/node. |
6 participants
throw an error on unknown getLegacyCipher input
rather than returning the default. Makes the
behavior more explicit and matches up with the
behavior on the command line switches.