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

doc, tls: deprecate createSecurePair #6063

Closed
wants to merge 1 commit into from

Conversation

jhamhader
Copy link
Contributor

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

Please provide affected core subsystem(s) (like buffer, cluster, crypto, etc)

Description of change

createSecurePair uses tls_legacy and the legacy Connection from
node_crypto.cc. Deprecate them in favor of TLSSocket.

See #5924

@mscdex mscdex added tls Issues and PRs related to the tls subsystem. doc Issues and PRs related to the documentations. labels Apr 5, 2016
@benjamingr
Copy link
Member

I think we should start providing examples of how to use different things when posting deprecations but I'm not saying this PR is where we should start.

The contents of the PR are fine but this sort of thing needs to be discussed by the CTC in a meeting.

@jasnell jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. ctc-agenda labels Apr 7, 2016
@jasnell
Copy link
Member

jasnell commented Apr 7, 2016

/cc @nodejs/ctc @nodejs/crypto

@indutny
Copy link
Member

indutny commented Apr 8, 2016

I agree with @benjamingr.

@jasnell
Copy link
Member

jasnell commented Apr 8, 2016

I agree that an example showing how the alternative TLSSocket would be used.

@ChALkeR
Copy link
Member

ChALkeR commented Apr 13, 2016

@jasnell
Copy link
Member

jasnell commented Apr 20, 2016

@nodejs/ctc discussed this today and had no objections. If there are no objections by Monday I will land before v6 is cut. There may need to be additional edits to this PR before then

@jhamhader
Copy link
Contributor Author

@jasnell - sure. What is missing?

@indutny
Copy link
Member

indutny commented Apr 20, 2016

Suggestion of how createSecurePair() could be replaced by modern APIs :)

@jasnell
Copy link
Member

jasnell commented Apr 20, 2016

Yeah, nothing specific and nothing critical, but an example would be helpful.

@jhamhader
Copy link
Contributor Author

On it

@indutny
Copy link
Member

indutny commented Apr 20, 2016

Yay! @jhamhader you rock!

createSecurePair uses tls_legacy and the legacy Connection from
node_crypto.cc. Deprecate them in favor of TLSSocket.
@jhamhader
Copy link
Contributor Author

Updated (tried to keep it short)

@indutny
Copy link
Member

indutny commented Apr 25, 2016

Not sure about markdown formatting in docs, but text SGTM

@jasnell jasnell added this to the 6.0.0 milestone Apr 25, 2016
@@ -770,6 +774,19 @@ stream.

NOTE: `cleartext` has the same API as [`tls.TLSSocket`][]

**Deprecated** `tls.createSecurePair()` is now deprecated in favor of
`tls.TLSSocket()`. For example:
```
Copy link
Member

Choose a reason for hiding this comment

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

style nit, can you add the js to the end of the three backticks and put a blank line before the examples

@jasnell
Copy link
Member

jasnell commented Apr 26, 2016

Added a nit that I can fix on landing. LGTM

jasnell pushed a commit that referenced this pull request Apr 26, 2016
createSecurePair uses tls_legacy and the legacy Connection from
node_crypto.cc. Deprecate them in favor of TLSSocket.

PR-URL: #6063
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Apr 26, 2016

Landed in 31de5cc

@jasnell jasnell closed this Apr 26, 2016
jasnell pushed a commit that referenced this pull request Apr 26, 2016
createSecurePair uses tls_legacy and the legacy Connection from
node_crypto.cc. Deprecate them in favor of TLSSocket.

PR-URL: #6063
Reviewed-By: James M Snell <jasnell@gmail.com>
@arthurschreiber
Copy link

Hey @jasnell and @indutny,

I'm the maintainer of a npm package called tedious, which is a database driver for Microsofts SQLServer. We have been using ´tls.createSecurePair´ to implement tls connection handling as specified by MS-TDS, Microsofts specification of the TDS protocol.

One peculiarity of this protocol was the way the TLS connection is established - the TLS data for the handshake needs to be wrapped inside TDS protocol packets. With ´tls.createSecurePair´, this was easily possible. I looked into switching to tls.connect instead, but I did not find any way how to implement this without the deprecated API.

The existing code can be found here:
https://github.com/pekim/tedious/blob/master/src/message-io.js (in the startTls method).

Could you please take a look and let me know how something similar could be achieved with the new API? I did not find anything in the documentation. Thanks!

@bnoordhuis
Copy link
Member

@nodejs/crypto I think we should undo the deprecation. I was under the impression that CleartextStream and EncryptedStream were exported, letting you cobble a SecurePair together, but they're not.

The use case of streaming TLS cipher/decipher not tied to a socket doesn't seem to be supported by non-deprecated APIs at the moment.

@indutny
Copy link
Member

indutny commented Jul 10, 2016

@bnoordhuis it is supported, actually. A regular stream.Duplex instance may be passed to TLSSocket as a socket option.

@bnoordhuis
Copy link
Member

That isn't very obvious. I did notice we have a few tests that pass in a Duplex instance but the documentation says that socket should be "an instance of net.Socket" and that it constructs "a new tls.TLSSocket object from an existing TCP socket."

@indutny
Copy link
Member

indutny commented Jul 11, 2016

Yeah, docs lack clarity, but this is certainly possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. semver-major PRs that contain breaking changes and should be released in the next major version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants