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

https: add extra options to Agent#getName() #16402

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all 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
11 changes: 6 additions & 5 deletions doc/api/https.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ separate module.
added: v0.4.5
-->

An Agent object for HTTPS similar to [`http.Agent`][]. See [`https.request()`][]
An [`Agent`][] object for HTTPS similar to [`http.Agent`][]. See [`https.request()`][]
for more information.

## Class: https.Server
Expand Down Expand Up @@ -167,9 +167,10 @@ changes:

Makes a request to a secure web server.

The following additional `options` from [`tls.connect()`][] are also accepted
when using a custom [`Agent`][]: `ca`, `cert`, `ciphers`, `clientCertEngine`,
`key`, `passphrase`, `pfx`, `rejectUnauthorized`, `secureProtocol`, `servername`
The following additional `options` from [`tls.connect()`][] are also accepted:
`ca`, `cert`, `ciphers`, `clientCertEngine`, `crl`, `dhparam`, `ecdhCurve`,
`honorCipherOrder`, `key`, `passphrase`, `pfx`, `rejectUnauthorized`,
`secureOptions`, `secureProtocol`, `servername`, `sessionIdContext`

`options` can be an object, a string, or a [`URL`][] object. If `options` is a
string, it is automatically parsed with [`url.parse()`][]. If it is a [`URL`][]
Expand Down Expand Up @@ -219,7 +220,7 @@ const req = https.request(options, (res) => {
});
```

Alternatively, opt out of connection pooling by not using an `Agent`.
Alternatively, opt out of connection pooling by not using an [`Agent`][].

Example:

Expand Down
24 changes: 24 additions & 0 deletions lib/https.js
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,30 @@ Agent.prototype.getName = function getName(options) {
if (options.secureProtocol)
name += options.secureProtocol;

name += ':';
if (options.crl)
name += options.crl;

name += ':';
if (options.honorCipherOrder !== undefined)
name += options.honorCipherOrder;

name += ':';
if (options.ecdhCurve)
name += options.ecdhCurve;

name += ':';
if (options.dhparam)
name += options.dhparam;

name += ':';
if (options.secureOptions !== undefined)
name += options.secureOptions;

name += ':';
if (options.sessionIdContext)
name += options.sessionIdContext;
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this is kind of starting to look like working with a list of options might be an easier choice…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. How about something like:

const components = [
  options.ca,
  options.cert,
  options.ciphers,
  options.key,
  options.pfx,
  options.rejectUnauthorized,
  options.servername !== options.host ? options.servername : undefined,
  options.secureProtocol,
  options.crl,
  options.honorCipherOrder,
  options.ecdhCurve,
  options.dhparam,
  options.secureOptions,
  options.sessionIdContext
];

for (const component of components) {
  name += ':';
  if (component != null) {
    name += component;
  }
}

// or:
// name += components
//   .map(component => component != null ? `:${component}` : ':')
//   .join('');

There's a potential slight difference in serialization (nulls wouldn't be added for rejectUnauthorized, honorCipherOrder, or secureOptions any more), but it seems like null would either have no impact or be invalid for all three so it feels alright to me.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

but it seems like null would either have no impact or be invalid for all three so it feels alright to me.

I agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated


return name;
};

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

const assert = require('assert');
const crypto = require('crypto');
const https = require('https');
const fixtures = require('../common/fixtures');

const options = {
key: fixtures.readKey('agent1-key.pem'),
cert: fixtures.readKey('agent1-cert.pem'),
ca: fixtures.readKey('ca1-cert.pem')
};

const server = https.Server(options, function(req, res) {
res.writeHead(200);
res.end('hello world\n');
});

function getBaseOptions(port) {
return {
path: '/',
port: port,
ca: options.ca,
rejectUnautorized: true,
servername: 'agent1',
};
}

const updatedValues = new Map([
['dhparam', fixtures.readKey('dh2048.pem')],
['ecdhCurve', 'secp384r1'],
['honorCipherOrder', true],
['secureOptions', crypto.constants.SSL_OP_CIPHER_SERVER_PREFERENCE],
['secureProtocol', 'TLSv1_method'],
['sessionIdContext', 'sessionIdContext'],
]);

function variations(iter, port, cb) {
const { done, value } = iter.next();
if (done) {
return common.mustCall(cb);
} else {
const [key, val] = value;
return common.mustCall(function(res) {
res.resume();
https.globalAgent.once('free', common.mustCall(function() {
https.get(
Object.assign({}, getBaseOptions(port), { [key]: val }),
variations(iter, port, cb)
);
}));
});
}
}

server.listen(0, common.mustCall(function() {
const port = this.address().port;
const globalAgent = https.globalAgent;
globalAgent.keepAlive = true;
https.get(getBaseOptions(port), variations(
updatedValues.entries(),
port,
common.mustCall(function(res) {
res.resume();
globalAgent.once('free', common.mustCall(function() {
// Verify that different keep-alived connections are created
// for the base call and each variation
const keys = Object.keys(globalAgent.freeSockets);
assert.strictEqual(keys.length, 1 + updatedValues.size);
let i = 1;
for (const [, value] of updatedValues) {
assert.ok(
keys[i].startsWith(value.toString() + ':') ||
keys[i].endsWith(':' + value.toString()) ||
keys[i].includes(':' + value.toString() + ':')
);
i++;
}
globalAgent.destroy();
server.close();
}));
})
));
}));
12 changes: 10 additions & 2 deletions test/parallel/test-https-agent-getname.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const agent = new https.Agent();
// empty options
assert.strictEqual(
agent.getName({}),
'localhost:::::::::::'
'localhost:::::::::::::::::'
);

// pass all options arguments
Expand All @@ -23,13 +23,21 @@ const options = {
ca: 'ca',
cert: 'cert',
ciphers: 'ciphers',
crl: [Buffer.from('c'), Buffer.from('r'), Buffer.from('l')],
dhparam: 'dhparam',
ecdhCurve: 'ecdhCurve',
honorCipherOrder: false,
key: 'key',
pfx: 'pfx',
rejectUnauthorized: false,
secureOptions: 0,
secureProtocol: 'secureProtocol',
servername: 'localhost',
sessionIdContext: 'sessionIdContext'
};

assert.strictEqual(
agent.getName(options),
'0.0.0.0:443:192.168.1.1:ca:cert::ciphers:key:pfx:false:localhost:'
'0.0.0.0:443:192.168.1.1:ca:cert::ciphers:key:pfx:false:localhost:' +
'secureProtocol:c,r,l:false:ecdhCurve:dhparam:0:sessionIdContext'
);
57 changes: 0 additions & 57 deletions test/parallel/test-https-agent-secure-protocol.js

This file was deleted.