Skip to content
Closed
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions doc/api/tls.md
Original file line number Diff line number Diff line change
Expand Up @@ -876,6 +876,31 @@ socket.on('end', () => {
});
```

When using an instance of `net.Socket`, use `net.Socket` to upgrade an existing socket.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, this is still 88 chars) But it is a small nit and can be fixed later on landing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah dang, I can't count! Sorry, now fixed - cuts off before 80.

Do not wrap the `net.Socket` in a `TLSSocket`.

See the example below for usage of upgrading an existing socket:

```js
const { Socket } = require('net');
const tls = require('tls');
const sock = new Socket();
Copy link
Member

Choose a reason for hiding this comment

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

Where is this variable used?

Copy link
Member

Choose a reason for hiding this comment

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

Uhm something went wrong here... The original code (see first commit) looked like tls.connect({ socket: sock }). @VerteDinde You probably don't want to use tls.connect({ port: 6697, host: 'irc.freenode.net' }) in both cases, right? This won't upgrade an existing socket, it establishes a new connection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right! Just updated to reflect that variable and changed the irc.freenode.net to https://example.org:443/.

const secureSock = tls.connect({ socket: s }, () => {
console.log('The tls socket connected.');
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer 'The TLS socket has been connected.'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. 😄 Altered!

});
sock.connect({ port: 6697, host: 'irc.freenode.net' });
Copy link
Member

Choose a reason for hiding this comment

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

I think this works, but our docs say

Usually, a socket is already connected when passed to tls.connect()

It might be a bit counter-intuitive to call tls.connect() before socket.connect(). I don't use the TLS module much, is there any reason for this order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question - I thought that seemed off as well, but worked when I tested it. Let me test it by calling sock.connect first and see; it does seem redundant to call sock.connect after tls.connect.

Copy link
Contributor Author

@VerteDinde VerteDinde Jul 3, 2017

Choose a reason for hiding this comment

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

I replaced { socket: s } with the port and host information, and removed sock.connect entirely. It appears to be passing, and now the sample code is more in line with the docs' recommendations.

```

If using TLS as the initial default rather than net.Socket,
Copy link
Contributor

Choose a reason for hiding this comment

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

`net.Socket`)

use only `tls.connect()` to upgrade the socket:
Copy link
Member

Choose a reason for hiding this comment

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

I think "upgrade" is a bit of a misnomer as there is no initial socket to be upgraded. I'd go with just:

If no socket is provided, this function will create a new TLS socket.

as opposed to

If a net.Socket is provided, this function will upgrade that TCP socket to a TLS one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhhh this is actually really clarifying - I was wondering what the distinction was with net.Socket. Have clarified by removing my old sentence, and adding yours. Thanks!


```js
const tls = require('tls');
const secureSock = tls.connect({ port: 6697, host: 'irc.freenode.net' }, () => {
console.log('The tls socket connected.');
});
```

## tls.connect(path[, options][, callback])
<!-- YAML
added: v0.11.3
Expand Down