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

Setting the servername to an empty string '' should disable sending the SNI extension. #1587

Merged
merged 4 commits into from
Jun 15, 2019

Conversation

holgerkoser
Copy link
Contributor

@holgerkoser holgerkoser commented Jun 14, 2019

The option servername in tls.connect(options[, callback]) is the name of the host being connected to, and must be a host name, and not an IP address. If the server is not a multi-homed server and I want to connect with the IP address it is not possible to disable sending the SNI extension by setting the servername to empty string ''. This PR implements the defaulting of the servername in the same way as it is done in the node.

This PR is related to #1347 .

Example
In node v12 with the following code

new WebSocket('wss://127.0.0.1', {
  ca: fs.readFileSync('ssl/ca.pem'),
  servername: '',
})

I get the deprecation warning [DEP0123]

(node:71346) [DEP0123] DeprecationWarning: Setting the TLS ServerName to an IP address 
is not permitted by RFC 6066. This will be ignored in a future version.

@lpinca
Copy link
Member

lpinca commented Jun 14, 2019

Can you add a test?

@holgerkoser
Copy link
Contributor Author

Yes, of course. I already tried to add a test in the SSL but I not found an easy way to intercept the tls.connect call. I normally use proxyquire for this but don't wanted to add a new dependency for such a small change. I try to find an alternative simple solution for a test.

@lpinca
Copy link
Member

lpinca commented Jun 14, 2019

I think you can "copy" the test included in nodejs/node@98e9de7.

@holgerkoser
Copy link
Contributor Author

Thanks for the hint. I have seen it too late and already added a test. If you think it is better to "copy" the kind of test from node I will do that.

});

it('allows to use empty string to disable sending the SNI extension', () => {
const ws = new WebSocket('wss://127.0.0.1', {
Copy link
Member

Choose a reason for hiding this comment

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

Can you please mock tls.connect() here and remove the describe wrapper and the beforeEach() and afterEach() hook? We will add them if/when needed. Thank you.

@lpinca
Copy link
Member

lpinca commented Jun 14, 2019

I prefer your test.

@lpinca lpinca merged commit b086179 into websockets:master Jun 15, 2019
@lpinca
Copy link
Member

lpinca commented Jun 15, 2019

Thank you.

@holgerkoser
Copy link
Contributor Author

holgerkoser commented Jun 19, 2019

@lpinca Couldn't answer any more, because I had two days off and since today I'm back in office again. Many thanks for your fast reply and your help.

Wouldn't it be better to restore the original net.connect before the assert?

        tls.connect = original;
        assert.strictEqual(options.servername, '');

Or does the test runner stop all tests if the first test fails?

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.

None yet

2 participants