Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -774,6 +774,7 @@ TLSSocket.prototype._finishInit = function() {
return;

this.alpnProtocol = this._handle.getALPNNegotiatedProtocol();
// The servername could be set by TLSWrap.
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 be specific here and say "set by TLSWrap::SelectSNIContextCallback()"? That should save the next guy/gal some searching.

Would it make sense to make the assignment conditional on if (this.servername === undefined) to avoid duplicate work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you be specific here and say "set by TLSWrap::SelectSNIContextCallback()"? That should save the next guy/gal some searching.

Done.

Would it make sense to make the assignment conditional on if (this.servername === undefined) to avoid duplicate work?

Yes, it makes sense to me. And the default value of this.servername is null so that I have used if (this.servername === null) instead.

this.servername = this._handle.getServername();

debug('%s _finishInit',
Expand Down
5 changes: 5 additions & 0 deletions src/tls_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1033,6 +1033,11 @@ int TLSWrap::SelectSNIContextCallback(SSL* s, int* ad, void* arg) {
Local<Object> object = p->object();
Local<Value> ctx;

// Set the servername as early as possible
Local<Object> owner = p->GetOwner();
USE(owner->Set(env->context(), env->servername_string(),
OneByteString(env->isolate(), servername)));
Copy link
Member

Choose a reason for hiding this comment

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

Tiny style nit: can you line up the arguments?

I'm a little ambivalent about discarding the return value with USE(...). I'm inclined to say it should return SSL_TLSEXT_ERR_NOACK when setting the property fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tiny style nit: can you line up the arguments?

Done.

I'm a little ambivalent about discarding the return value with USE(...). I'm inclined to say it should return SSL_TLSEXT_ERR_NOACK when setting the property fails.

Good reminding! I think returning SSL_TLSEXT_ERR_NOACK is okay as the owner looks abnormal in this scenario.


if (!object->Get(env->context(), env->sni_context_string()).ToLocal(&ctx))
return SSL_TLSEXT_ERR_NOACK;

Expand Down
56 changes: 56 additions & 0 deletions test/parallel/test-tls-sni-servername.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
'use strict';
const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');

const assert = require('assert');
const tls = require('tls');

// We could get the `tlsSocket.servername` even if the event of "tlsClientError"
// is emitted.

const serverOptions = {
requestCert: true,
rejectUnauthorized: false,
SNICallback: function(servername, callback) {
if (servername === 'c.another.com') {
callback(null, {});
} else {
callback(new Error('Invalid SNI context'), null);
}
}
};

function test(options) {
const server = tls.createServer(serverOptions, common.mustNotCall());

server.on('tlsClientError', common.mustCall((err, socket) => {
assert.strictEqual(err.message, 'Invalid SNI context');
// The `servername` should match.
assert.strictEqual(socket.servername, options.servername);
}));

server.listen(0, () => {
options.port = server.address().port;
const client = tls.connect(options, common.mustNotCall());

client.on('error', common.mustCall((err) => {
assert.strictEqual(err.message, 'Client network socket' +
' disconnected before secure TLS connection was established');
}));

client.on('close', common.mustCall(() => server.close()));
});
}

test({
port: undefined,
servername: 'c.another.com',
rejectUnauthorized: false
});

test({
port: undefined,
servername: 'c.wrong.com',
rejectUnauthorized: false
});