This repository has been archived by the owner on Apr 22, 2023. It is now read-only.
tls: fix handling of SNICallback server option
#5981
Closed
Conversation
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
| options.SNICallback && | ||
| options.server._contexts.length) { | ||
| (options.SNICallback || | ||
| options.server._contexts.length)) { |
There was a problem hiding this comment.
Doesn't this fit on a single line? Also, what if options.SNICallback is undefined but options.server._contexts.length > 0?
There was a problem hiding this comment.
oh... gosh... I think I need to refine it a bit, thanks!
|
Force pushed, please check again. |
| if (process.features.tls_sni && | ||
| options.isServer && | ||
| options.server && | ||
| options.SNICallback && | ||
| options.server._contexts.length) { | ||
| (options.SNICallback !== SNICallback || | ||
| options.server._contexts.length)) { |
There was a problem hiding this comment.
This still doesn't look quite right to me.
Correct me if I'm wrong but I see lib/_tls_wrap calling the TLSSsocket constructor with { SNICallback: self.SNICallback } so there's always an SNICallback, right?
If that's the case, can't you replace (options.SNICallback !== SNICallback || options.server._contexts.length) with just options.server._contexts.length > 0?
Then maybe add a assert(typeof options.SNICallback === 'function') check, just in case.
There was a problem hiding this comment.
Ok, let me describe it a bit. SNICallback is a default version which is using server._contexts for matching server names, this conditions checks if there's a custom callback or if there're any contexts to match against. In both cases, onsniselect should be set to either call custom callback (even if there're no contexts), or default one if there're contexts to match.
It shouldn't ignore it!
|
Commit log should go into a little more detail. LGTM apart from that. |
|
Thanks, landed in ea7b817 |
gibfahn
pushed a commit
to ibmruntimes/node
that referenced
this pull request
Apr 26, 2016
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.
* Buffer
* New Buffer constructors have been added
[nodejs#4682](nodejs/node#4682)
* Previously deprecated Buffer APIs are removed
[nodejs#5048](nodejs/node#5048),
[nodejs#4594](nodejs/node#4594)
* Improved error handling [nodejs#4514](nodejs/node#4514)
* Cluster
* Worker emitted as first argument in 'message' event
[nodejs#5361](nodejs/node#5361).
* Crypto
* Improved error handling [nodejs#3100](nodejs/node#3100),
[nodejs#5611](nodejs/node#5611)
* Simplified Certificate class bindings
[nodejs#5382](nodejs/node#5382)
* Improved control over FIPS mode
[nodejs#5181](nodejs/node#5181)
* pbkdf2 digest overloading is deprecated
[nodejs#4047](nodejs/node#4047)
* Dependencies
* Reintroduce shared c-ares build support
[nodejs#5775](nodejs/node#5775).
* V8 updated to 5.0.71.31 [nodejs#6111](nodejs/node#6111).
* DNS
* Add resolvePtr API to query plain DNS PTR records
[nodejs#4921](nodejs/node#4921).
* Domains
* Clear stack when no error handler
[nodejs#4659](nodejs/node#4659).
* File System
* The `fs.realpath()` and `fs.realpathSync()` methods have been updated
to use a more efficient libuv implementation. This change includes the
removal of the `cache` argument and the method can throw new errors
[nodejs#3594](nodejs/node#3594)
* FS apis can now accept and return paths as Buffers
[nodejs#5616](nodejs/node#5616).
* Error handling and type checking improvements
[nodejs#5616](nodejs/node#5616),
[nodejs#5590](nodejs/node#5590),
[nodejs#4518](nodejs/node#4518),
[nodejs#3917](nodejs/node#3917).
* fs.read's string interface is deprecated
[nodejs#4525](nodejs/node#4525)
* HTTP
* 'clientError' can now be used to return custom errors from an
HTTP server [nodejs#4557](nodejs/node#4557).
* Modules
* Current directory is now prioritized for local lookups
[nodejs#5689](nodejs/node#5689)
* Symbolic links are preserved when requiring modules
[nodejs#5950](nodejs/node#5950)
* Net
* DNS hints no longer implicitly set
[nodejs#6021](nodejs/node#6021).
* Improved error handling and type checking
[nodejs#5981](nodejs/node#5981),
[nodejs#5733](nodejs/node#5733),
[nodejs#2904](nodejs/node#2904)
* Path
* Improved type checking [nodejs#5348](nodejs/node#5348).
* Process
* Introduce process warnings API
[nodejs#4782](nodejs/node#4782).
* Throw exception when non-function passed to nextTick
[nodejs#3860](nodejs/node#3860).
* Readline
* Emit key info unconditionally
[nodejs#6024](nodejs/node#6024)
* REPL
* Assignment to `_` will emit a warning.
[nodejs#5535](nodejs/node#5535)
* Timers
* Fail early when callback is not a function
[nodejs#4362](nodejs/node#4362)
* TLS
* Rename 'clientError' to 'tlsClientError'
[nodejs#4557](nodejs/node#4557)
* SHA1 used for sessionIdContext
[nodejs#3866](nodejs/node#3866)
* TTY
* Previously deprecated setRawMode wrapper is removed
[nodejs#2528](nodejs/node#2528).
* Util
* Changes to Error object formatting
[nodejs#4582](nodejs/node#4582).
* Windows
* Windows XP and Vista are no longer supported
[nodejs#5167](nodejs/node#5167),
[nodejs#5167](nodejs/node#5167).
gibfahn
pushed a commit
to ibmruntimes/node
that referenced
this pull request
Apr 27, 2016
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.
* Buffer
* New Buffer constructors have been added
[nodejs#4682](nodejs/node#4682)
* Previously deprecated Buffer APIs are removed
[nodejs#5048](nodejs/node#5048),
[nodejs#4594](nodejs/node#4594)
* Improved error handling [nodejs#4514](nodejs/node#4514)
* Cluster
* Worker emitted as first argument in 'message' event
[nodejs#5361](nodejs/node#5361).
* Crypto
* Improved error handling [nodejs#3100](nodejs/node#3100),
[nodejs#5611](nodejs/node#5611)
* Simplified Certificate class bindings
[nodejs#5382](nodejs/node#5382)
* Improved control over FIPS mode
[nodejs#5181](nodejs/node#5181)
* pbkdf2 digest overloading is deprecated
[nodejs#4047](nodejs/node#4047)
* Dependencies
* Reintroduce shared c-ares build support
[nodejs#5775](nodejs/node#5775).
* V8 updated to 5.0.71.31 [nodejs#6111](nodejs/node#6111).
* DNS
* Add resolvePtr API to query plain DNS PTR records
[nodejs#4921](nodejs/node#4921).
* Domains
* Clear stack when no error handler
[nodejs#4659](nodejs/node#4659).
* File System
* The `fs.realpath()` and `fs.realpathSync()` methods have been updated
to use a more efficient libuv implementation. This change includes the
removal of the `cache` argument and the method can throw new errors
[nodejs#3594](nodejs/node#3594)
* FS apis can now accept and return paths as Buffers
[nodejs#5616](nodejs/node#5616).
* Error handling and type checking improvements
[nodejs#5616](nodejs/node#5616),
[nodejs#5590](nodejs/node#5590),
[nodejs#4518](nodejs/node#4518),
[nodejs#3917](nodejs/node#3917).
* fs.read's string interface is deprecated
[nodejs#4525](nodejs/node#4525)
* HTTP
* 'clientError' can now be used to return custom errors from an
HTTP server [nodejs#4557](nodejs/node#4557).
* Modules
* Current directory is now prioritized for local lookups
[nodejs#5689](nodejs/node#5689)
* Symbolic links are preserved when requiring modules
[nodejs#5950](nodejs/node#5950)
* Net
* DNS hints no longer implicitly set
[nodejs#6021](nodejs/node#6021).
* Improved error handling and type checking
[nodejs#5981](nodejs/node#5981),
[nodejs#5733](nodejs/node#5733),
[nodejs#2904](nodejs/node#2904)
* OS X
* MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7
[nodejs#6402](nodejs/node#6402).
* Path
* Improved type checking [nodejs#5348](nodejs/node#5348).
* Process
* Introduce process warnings API
[nodejs#4782](nodejs/node#4782).
* Throw exception when non-function passed to nextTick
[nodejs#3860](nodejs/node#3860).
* Readline
* Emit key info unconditionally
[nodejs#6024](nodejs/node#6024)
* REPL
* Assignment to `_` will emit a warning.
[nodejs#5535](nodejs/node#5535)
* Timers
* Fail early when callback is not a function
[nodejs#4362](nodejs/node#4362)
* TLS
* Rename 'clientError' to 'tlsClientError'
[nodejs#4557](nodejs/node#4557)
* SHA1 used for sessionIdContext
[nodejs#3866](nodejs/node#3866)
* TTY
* Previously deprecated setRawMode wrapper is removed
[nodejs#2528](nodejs/node#2528).
* Util
* Changes to Error object formatting
[nodejs#4582](nodejs/node#4582).
* Windows
* Windows XP and Vista are no longer supported
[nodejs#5167](nodejs/node#5167),
[nodejs#5167](nodejs/node#5167).
gibfahn
pushed a commit
to ibmruntimes/node
that referenced
this pull request
May 6, 2016
The following significant (semver-major) changes have been made since the
previous Node v5.0.0 release.
* Buffer
* New Buffer constructors have been added
[nodejs#4682](nodejs/node#4682)
* Previously deprecated Buffer APIs are removed
[nodejs#5048](nodejs/node#5048),
[nodejs#4594](nodejs/node#4594)
* Improved error handling [nodejs#4514](nodejs/node#4514)
* Cluster
* Worker emitted as first argument in 'message' event
[nodejs#5361](nodejs/node#5361).
* Crypto
* Improved error handling [nodejs#3100](nodejs/node#3100),
[nodejs#5611](nodejs/node#5611)
* Simplified Certificate class bindings
[nodejs#5382](nodejs/node#5382)
* Improved control over FIPS mode
[nodejs#5181](nodejs/node#5181)
* pbkdf2 digest overloading is deprecated
[nodejs#4047](nodejs/node#4047)
* Dependencies
* Reintroduce shared c-ares build support
[nodejs#5775](nodejs/node#5775).
* V8 updated to 5.0.71.31 [nodejs#6111](nodejs/node#6111).
* DNS
* Add resolvePtr API to query plain DNS PTR records
[nodejs#4921](nodejs/node#4921).
* Domains
* Clear stack when no error handler
[nodejs#4659](nodejs/node#4659).
* File System
* The `fs.realpath()` and `fs.realpathSync()` methods have been updated
to use a more efficient libuv implementation. This change includes the
removal of the `cache` argument and the method can throw new errors
[nodejs#3594](nodejs/node#3594)
* FS apis can now accept and return paths as Buffers
[nodejs#5616](nodejs/node#5616).
* Error handling and type checking improvements
[nodejs#5616](nodejs/node#5616),
[nodejs#5590](nodejs/node#5590),
[nodejs#4518](nodejs/node#4518),
[nodejs#3917](nodejs/node#3917).
* fs.read's string interface is deprecated
[nodejs#4525](nodejs/node#4525)
* HTTP
* 'clientError' can now be used to return custom errors from an
HTTP server [nodejs#4557](nodejs/node#4557).
* Modules
* Current directory is now prioritized for local lookups
[nodejs#5689](nodejs/node#5689)
* Symbolic links are preserved when requiring modules
[nodejs#5950](nodejs/node#5950)
* Net
* DNS hints no longer implicitly set
[nodejs#6021](nodejs/node#6021).
* Improved error handling and type checking
[nodejs#5981](nodejs/node#5981),
[nodejs#5733](nodejs/node#5733),
[nodejs#2904](nodejs/node#2904)
* OS X
* MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7
[nodejs#6402](nodejs/node#6402).
* Path
* Improved type checking [nodejs#5348](nodejs/node#5348).
* Process
* Introduce process warnings API
[nodejs#4782](nodejs/node#4782).
* Throw exception when non-function passed to nextTick
[nodejs#3860](nodejs/node#3860).
* Readline
* Emit key info unconditionally
[nodejs#6024](nodejs/node#6024)
* REPL
* Assignment to `_` will emit a warning.
[nodejs#5535](nodejs/node#5535)
* Timers
* Fail early when callback is not a function
[nodejs#4362](nodejs/node#4362)
* TLS
* Rename 'clientError' to 'tlsClientError'
[nodejs#4557](nodejs/node#4557)
* SHA1 used for sessionIdContext
[nodejs#3866](nodejs/node#3866)
* TTY
* Previously deprecated setRawMode wrapper is removed
[nodejs#2528](nodejs/node#2528).
* Util
* Changes to Error object formatting
[nodejs#4582](nodejs/node#4582).
* Windows
* Windows XP and Vista are no longer supported
[nodejs#5167](nodejs/node#5167),
[nodejs#5167](nodejs/node#5167).
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
It shouldn't ignore it!