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
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
Copy Markdown
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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ping

Copy link
Copy Markdown
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
50 changes: 50 additions & 0 deletions test/parallel/test-tls-starttls-server.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
'use strict';
const common = require('../common');

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.equal(hostname, 'test.test');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you change this to assert.strictEqual().

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed, and removed that console.log() below.


callback(null, null);
Copy link
Copy Markdown
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
Copy Markdown
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(() => {
console.log('hello');
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
Copy Markdown
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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ack.

});
});