Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tls: TLSSocket options not initialized #2614

Closed
wants to merge 1 commit into from
Closed

tls: TLSSocket options not initialized #2614

wants to merge 1 commit into from

Conversation

jhamhader
Copy link
Contributor

Upon creating a TLSSocket object without options, default options will be used, which
set the socket as isServer: false
Updated tls docs and added test-tls-socket-default-options

See issue #2394

@targos targos added the tls Issues and PRs related to the tls subsystem. label Aug 29, 2015
mscdex
mscdex reviewed Aug 29, 2015
View reviewed changes
doc/api/tls.markdown
@@ -460,7 +460,7 @@ Construct a new TLSSocket object from existing TCP socket.
- `secureContext`: An optional TLS context object from
`tls.createSecureContext( ... )`

- `isServer`: If true - TLS socket will be instantiated in server-mode
- `isServer`: If true - TLS socket will be instantiated in server-mode. Default: `false`
Copy link
Contributor

Choose a reason for hiding this comment

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

The true here should include backticks also for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@thefourtheye
Copy link
Contributor

If isServer is not set explicitly then it will be undefined. Does the code really check if the value is false anywhere?

Edit: I see that the code relies only on the truthiness/falsiness of isServer.

@jhamhader jhamhader force-pushed the tls-socket-default-options branch 2 times, most recently from 9fc32cd to f61c44a Compare September 19, 2015 12:17
@indutny
Copy link
Member

indutny commented Sep 24, 2015

cc @nodejs/crypto

indutny
indutny reviewed Sep 24, 2015
View reviewed changes
lib/_tls_wrap.js
@@ -228,7 +228,7 @@ function initRead(tls, wrapped) {
*/

function TLSSocket(socket, options) {
this._tlsOptions = options;
this._tlsOptions = options || { isServer: false };
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that this can possibly suffice. There are also context and various other things that needs to be specified. It can't be possible to omit them, so I don't think that there is any point in providing this default values.

Copy link
Member

Choose a reason for hiding this comment

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

If you would like to make isServer false by default - let's do it explicitly!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make isServer: false by default (set to false if options is undefined or options.isServer is undefined).
Regarding the context - TLS API says the rest of the options (besides isServer) are optional and seems like _wrapHandle() creates a secureContext if such is not provided.

Copy link
Member

Choose a reason for hiding this comment

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

Oh... sorry then!

@jhamhader jhamhader force-pushed the tls-socket-default-options branch from f61c44a to 324bb28 Compare September 26, 2015 20:01
@jhamhader
Copy link
Contributor Author

  • Changed isServer to be false by default
  • Updated test to include two kinds of arguments of TLSSocket()

@jhamhader
Copy link
Contributor Author

@indutny can you confirm?

@indutny
Copy link
Member

indutny commented Oct 13, 2015

Why is it needed? Would it be enough to do !!isServer when passing it to the C++ layer?

@jhamhader
Copy link
Contributor Author

Well, it is not only passed to the C++ layer - it is also used extensively in _tls_wrap.js.
Instead of the current change, we could do in TLSSocket():

this._tlsOptions.isServer = !!options.isServer

but initializing this value with a default boolean (in either way) seems like a harmless and safe thing to do.

@indutny
Copy link
Member

indutny commented Oct 18, 2015

@jhamhader It feels like doing !! in one place is simpler.

@jhamhader jhamhader force-pushed the tls-socket-default-options branch from 324bb28 to 7f69c59 Compare October 20, 2015 19:06
@jhamhader
Copy link
Contributor Author

Done

indutny
indutny reviewed Oct 20, 2015
View reviewed changes
doc/api/tls.markdown

- `secureContext`: An optional TLS context object from
`tls.createSecureContext( ... )`

- `isServer`: If true - TLS socket will be instantiated in server-mode
- `isServer`: If `true` - TLS socket will be instantiated in server-mode. Default: `false`
Copy link
Member

Choose a reason for hiding this comment

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

Is this over 80 column limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the thorough review. Fixed that

@jhamhader
tls: TLSSocket options default isServer false
62354f6 
Upon creating a TLSSocket object, set the default isServer option to false
Updated tls docs and added test-tls-socket-default-options
@jhamhader jhamhader force-pushed the tls-socket-default-options branch from 7f69c59 to 62354f6 Compare October 20, 2015 19:33
@indutny
Copy link
Member

indutny commented Oct 20, 2015

LGTM

@indutny
Copy link
Member

indutny commented Oct 20, 2015

CI: https://ci.nodejs.org/job/node-test-pull-request/543/

@indutny
Copy link
Member

indutny commented Oct 20, 2015

Landed in adfd20b, thank you!

@indutny indutny closed this Oct 20, 2015
indutny pushed a commit that referenced this pull request Oct 20, 2015
@jhamhader @indutny
tls: TLSSocket options default isServer false
adfd20b 
Upon creating a TLSSocket object, set the default isServer option to false
Updated tls docs and added test-tls-socket-default-options

PR-URL: #2614
Reviewed-By: Fedor Indutny <fedor@indutny.com>
rvagg pushed a commit that referenced this pull request Oct 21, 2015
@jhamhader @rvagg
tls: TLSSocket options default isServer false
c64af7d 
Upon creating a TLSSocket object, set the default isServer option to false
Updated tls docs and added test-tls-socket-default-options

PR-URL: #2614
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@rvagg rvagg mentioned this pull request Oct 21, 2015
Merged
@jhamhader jhamhader mentioned this pull request Oct 21, 2015
Closed
@MylesBorins
Copy link
Contributor

LTS?

@MylesBorins
Copy link
Contributor

/cc @jasnell

jasnell pushed a commit to jasnell/node that referenced this pull request Oct 26, 2015
@jhamhader @jasnell
tls: TLSSocket options default isServer false
590378c 
Upon creating a TLSSocket object, set the default isServer option to false
Updated tls docs and added test-tls-socket-default-options

PR-URL: nodejs#2614
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@jasnell
Copy link
Member

jasnell commented Oct 26, 2015

Landed in v4.x-staging in 590378c

jasnell pushed a commit that referenced this pull request Oct 26, 2015
@jhamhader @jasnell
tls: TLSSocket options default isServer false
f6ea99f 
Upon creating a TLSSocket object, set the default isServer option to false
Updated tls docs and added test-tls-socket-default-options

PR-URL: #2614
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@rvagg rvagg mentioned this pull request Oct 28, 2015
Closed
jasnell pushed a commit that referenced this pull request Oct 29, 2015
@jhamhader @jasnell
tls: TLSSocket options default isServer false
e69c869 
Upon creating a TLSSocket object, set the default isServer option to false
Updated tls docs and added test-tls-socket-default-options

PR-URL: #2614
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@jasnell jasnell mentioned this pull request Oct 29, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jhamhader @thefourtheye @indutny @MylesBorins @jasnell @mscdex @targos