Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 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
6 changes: 6 additions & 0 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,12 @@ function requestOCSP(self, hello, ctx, cb) {

if (!ctx)
ctx = self.server._sharedCreds;

// Running on non-TLS server
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment confused me when I read it, and I only understood when reading the unit test and PR history. Can I suggest:

TLS socket is using a net.Server, instead of a tls.TLSServer, so some TLS properties will not be present.

Copy link
Contributor

Choose a reason for hiding this comment

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

ping

Copy link
Member

Choose a reason for hiding this comment

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

Agreed this comment is rather cryptic.

if (!ctx)
return cb(null);

// TODO(indutny): eventually disallow raw `SecureContext`
if (ctx.context)
ctx = ctx.context;

Expand Down
49 changes: 49 additions & 0 deletions test/parallel/test-tls-starttls-server.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
'use strict';
const common = require('../common');

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure

A test should start with a comment containing a brief description of what it is designed to test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

if (!common.hasCrypto) {
common.skip('missing crypto');
return;
}

const assert = require('assert');
Copy link
Contributor

Choose a reason for hiding this comment

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

sort requires

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

const net = require('net');
const tls = require('tls');
const fs = require('fs');

const key = fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem');
const cert = fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem');

const server = net.createServer(common.mustCall((s) => {
const tlsSocket = new tls.TLSSocket(s, {
isServer: true,
server: server,

secureContext: tls.createSecureContext({
key: key,
cert: cert
}),

SNICallback: common.mustCall((hostname, callback) => {
assert.deepEqual(hostname, 'test.test');

callback(null, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

From https://github.com/nodejs/node/blob/master/doc/api/tls.md#tlscreateserveroptions-secureconnectionlistener

SNICallback should invoke cb(null, ctx), where ctx is a SecureContext instance.

Is (null, null) valid? Should it be valid?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is valid and it should be valid.

})
});

tlsSocket.on('secure', common.mustCall(() => {
tlsSocket.end();
server.close();
}));
})).listen(0, () => {
const opts = {
servername: 'test.test',
port: server.address().port,
rejectUnauthorized: false,
requestOCSP: true
};

const client = tls.connect(opts, function() {
client.end();
Copy link
Contributor

Choose a reason for hiding this comment

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

can just use this, and delete the const client =

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack.

});
});