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.

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

Fix tls.createSecurePair deprecation warning #738

Merged
merged 5 commits into from
Jul 20, 2018

Conversation

arthurschreiber
Copy link
Collaborator

@arthurschreiber arthurschreiber commented May 10, 2018

This is based on the great work done by @joux3 over in #689. 🙇

I forked duplexpair into a native-duplexpair, a version that uses the Duplex stream exposed by Node.js' built-in stream module, instead of the readable-stream module. This solves an issue where the duplexpair usage on Node.JS versions before the 8.x line would cause a crash.

@arthurschreiber
Copy link
Collaborator Author

A few additional notes - I'm not happy about forking duplexpair, but for as long as we support Node.js 6.x, this is what I believe to be the best solution.

Once we drop support for Node.js 6.x, we can switch over to the original duplexpair module. 👍

Copy link

@Hadis-Fard Hadis-Fard left a comment

Choose a reason for hiding this comment

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

LGTM :bowtie: thanks @arthurschreiber

@LiamDotPro
Copy link

Four years, Is it finally time for the depreciation warning to be depreciated, hold me boys.. It's happening 💯

@ovikholt
Copy link

ovikholt commented Jun 5, 2018

How is this coming along?

@LiamDotPro
Copy link

@arthurschreiber Are there any updates to share on this? When can we expect this to be merged?


// Verify that server's identity matches it's certificate's names
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By switching to tls.connect, we no longer need to perform server verification ourselves, and can instead make use of the rejectUnauthorized option. 🎉

// data once we attach a `data` listener. But on Node <= 0.10.x, this is not
// the case. We need to kick the cleartext stream once to get the
// encrypted end of the secure pair to emit the TLS handshake data.
this.securePair.cleartext.write('');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By switching to tls.connect, we no longer need to "kick" the secure socket to start performing the handshake - it's all handled automatically. 👍 (Also, the comment here was severely out-of-date. 🤷‍♂️)

@arthurschreiber arthurschreiber force-pushed the arthur/fix-deprecation-warning branch from d4097fc to 23e5bcd Compare July 20, 2018 07:54
This ensures we actually always perform a proper tls connection setup - 
and that errors are handled correctly.
@arthurschreiber arthurschreiber force-pushed the arthur/fix-deprecation-warning branch from 23e5bcd to 0979ff9 Compare July 20, 2018 08:55
@arthurschreiber arthurschreiber merged commit 31c9d9c into master Jul 20, 2018
@arthurschreiber arthurschreiber deleted the arthur/fix-deprecation-warning branch July 20, 2018 09:40
@LiamDotPro
Copy link

@arthurschreiber Legendary.

@arthurschreiber
Copy link
Collaborator Author

@LiamDotPro I know, right? 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants