This repository has been archived by the owner on Apr 22, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 7.3k
tls: throw an error on getLegacyCipher #14572
Closed
Closed
Changes from 4 commits
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 b450336
test: test-tls-cipher-list test error type
jasnell 2d1d2a0
test: fix typo in tls.markdown doc and additional tests
jasnell 14a26f9
tls: refactor test-tls-cipher-list and v0.10.38 legacy test
jasnell 7f4d098
test: additional refinements in the test
jasnell 30f46c3
test: additional round of edits on the test-tls-cipher-list.js
jasnell 8a61f11
test: refactor default cipher list test
jasnell File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1330,6 +1330,20 @@ function normalizeConnectArgs(listArgs) { | |
return (cb) ? [options, cb] : [options]; | ||
} | ||
|
||
// return true if the --enable-legacy-cipher-list command line | ||
// switch, or the NODE_LEGACY_CIPHER_LIST environment variable | ||
// are set to v0.10.38 and the DEFAULT_CIPHERS equal the v0.10.38 | ||
// list. | ||
function usingV1038Ciphers() { | ||
var argv = process.execArgv; | ||
if ((argv.indexOf('--enable-legacy-cipher-list=v0.10.38') > -1 || | ||
process.env['NODE_LEGACY_CIPHER_LIST'] === 'v0.10.38') && | ||
DEFAULT_CIPHERS === _crypto.getLegacyCiphers('v0.10.38')) { | ||
return true; | ||
} | ||
return false; | ||
} | ||
|
||
exports.connect = function(/* [port, host], options, cb */) { | ||
var args = normalizeConnectArgs(arguments); | ||
var options = args[0]; | ||
|
@@ -1338,7 +1352,8 @@ exports.connect = function(/* [port, host], options, cb */) { | |
var defaults = { | ||
rejectUnauthorized: '0' !== process.env.NODE_TLS_REJECT_UNAUTHORIZED | ||
}; | ||
if (DEFAULT_CIPHERS != _crypto.getLegacyCiphers('v0.10.38')) { | ||
if (!usingV1038Ciphers()) { | ||
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. 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 | ||
defaults.ciphers = DEFAULT_CIPHERS; | ||
} | ||
options = util._extend(defaults, options || {}); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4199,12 +4199,18 @@ const char* ToCString(const node::Utf8Value& value) { | |
|
||
Handle<Value> DefaultCiphers(const Arguments& args) { | ||
HandleScope scope; | ||
unsigned int len = args.Length(); | ||
if (len != 1 || !args[0]->IsString()) { | ||
return ThrowException(Exception::TypeError(String::New("A single string parameter is required"))); | ||
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. Style: wrap at 80 columns. |
||
} | ||
node::Utf8Value key(args[0]); | ||
const char * list = legacy_cipher_list(ToCString(key)); | ||
if (list == NULL) { | ||
list = DEFAULT_CIPHER_LIST_HEAD; | ||
if (list != NULL) { | ||
return scope.Close(v8::String::New(list)); | ||
} else { | ||
return ThrowException(Exception::Error(String::New( | ||
"Unknown legacy cipher list"))); | ||
} | ||
return scope.Close(v8::String::New(list)); | ||
} | ||
|
||
Handle<Value> GetCiphers(const Arguments& args) { | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I would suggest removing
Only _one_ should be used at a time
, since the support for having some options/env vars override others is intentional and corresponds to valid use cases.