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

errors: alter and test ERR_TLS_REQUIRED_SERVER_NAME #19988

Closed
wants to merge 2 commits 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
15 changes: 9 additions & 6 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1665,12 +1665,6 @@ An attempt to renegotiate the TLS session failed.

An attempt was made to renegotiate TLS on a socket instance with TLS disabled.

<a id="ERR_TLS_REQUIRED_SERVER_NAME"></a>
### ERR_TLS_REQUIRED_SERVER_NAME

While using TLS, the `server.addContext()` method was called without providing
a hostname in the first parameter.

<a id="ERR_TLS_SESSION_ATTACK"></a>
### ERR_TLS_SESSION_ATTACK

Expand Down Expand Up @@ -2030,6 +2024,15 @@ removed: v10.0.0

Used when a TLS renegotiation request has failed in a non-specific way.

<a id="ERR_TLS_REQUIRED_SERVER_NAME"></a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be removed now, as its no longer used.

### ERR_TLS_REQUIRED_SERVER_NAME
<!-- YAML
removed: REPLACEME
-->

While using TLS, the `server.addContext()` method was called without providing
a hostname in the first parameter.

<a id="ERR_UNKNOWN_BUILTIN_MODULE"></a>
### ERR_UNKNOWN_BUILTIN_MODULE
<!-- YAML
Expand Down
6 changes: 3 additions & 3 deletions doc/api/tls.md
Original file line number Diff line number Diff line change
Expand Up @@ -347,18 +347,18 @@ called:
* `tlsSocket` {tls.TLSSocket} The `tls.TLSSocket` instance from which the
error originated.

### server.addContext(hostname, context)
### server.addContext(servername, context)
<!-- YAML
added: v0.5.3
-->

* `hostname` {string} A SNI hostname or wildcard (e.g. `'*'`)
* `servername` {string} A SNI hostname or wildcard (e.g. `'*'`)
Copy link
Contributor

Choose a reason for hiding this comment

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

"hostname" --> "server name", consistent with https://github.com/nodejs/node/blob/master/doc/api/tls.md#event-secureconnection

a string containing the server name requested via SNI.

* `context` {Object} An object containing any of the possible properties
from the [`tls.createSecureContext()`][] `options` arguments (e.g. `key`,
`cert`, `ca`, etc).

The `server.addContext()` method adds a secure context that will be used if
the client request's SNI name matches the supplied `hostname` (or wildcard).
the client request's SNI name matches the supplied `severname` (or wildcard).
Copy link
Contributor

Choose a reason for hiding this comment

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

"severname" -> "servername" (typo)


### server.address()
<!-- YAML
Expand Down
4 changes: 2 additions & 2 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,13 @@ const { owner_symbol } = require('internal/async_hooks').symbols;
const { SecureContext: NativeSecureContext } = internalBinding('crypto');
const {
ERR_INVALID_ARG_TYPE,
ERR_MISSING_ARGS,
ERR_MULTIPLE_CALLBACK,
ERR_SOCKET_CLOSED,
ERR_TLS_DH_PARAM_SIZE,
ERR_TLS_HANDSHAKE_TIMEOUT,
ERR_TLS_RENEGOTIATE,
ERR_TLS_RENEGOTIATION_DISABLED,
ERR_TLS_REQUIRED_SERVER_NAME,
ERR_TLS_SESSION_ATTACK,
ERR_TLS_SNI_FROM_SERVER
} = require('internal/errors').codes;
Expand Down Expand Up @@ -1051,7 +1051,7 @@ Server.prototype.setOptions = util.deprecate(function(options) {
// SNI Contexts High-Level API
Server.prototype.addContext = function(servername, context) {
if (!servername) {
throw new ERR_TLS_REQUIRED_SERVER_NAME();
throw new ERR_MISSING_ARGS('servername');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the convention here would be:

if (typeof servername !== 'string')
  throw new ERR_INVALID_ARG_TYPE('servername', 'string', arg);

to test both the presence of the arg, but also its type. If it's not a string it will throw anyway now just below on the call to .replace(), but throwing a more descriptive error makes it look less like a node.js bug.

}

var re = new RegExp('^' +
Expand Down
4 changes: 1 addition & 3 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -864,10 +864,8 @@ E('ERR_TLS_HANDSHAKE_TIMEOUT', 'TLS handshake timeout', Error);
E('ERR_TLS_RENEGOTIATE', 'Attempt to renegotiate TLS session failed', Error);
E('ERR_TLS_RENEGOTIATION_DISABLED',
'TLS session renegotiation disabled for this socket', Error);

// This should probably be a `TypeError`.
E('ERR_TLS_REQUIRED_SERVER_NAME',
'"servername" is required parameter for Server.addContext', Error);
'"servername" is a required parameter', TypeError);
Copy link
Member

Choose a reason for hiding this comment

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

This is now unused and should be removed.

E('ERR_TLS_SESSION_ATTACK', 'TLS session renegotiation attack detected', Error);
E('ERR_TLS_SNI_FROM_SERVER',
'Cannot issue SNI from a TLS server-side socket', Error);
Expand Down
7 changes: 7 additions & 0 deletions test/parallel/test-tls-sni-server-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,13 @@ const clientsOptions = [{
const serverResults = [];
const clientResults = [];

// check for throwing case where servername is not supplied
Copy link
Contributor

Choose a reason for hiding this comment

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

Puncutate the comment, please (leading capital, trailing period).

common.expectsError(tls.createServer(serverOptions, () => {}).addContext, {
type: TypeError,
code: 'ERR_MISSING_ARGS',
message: 'The "servername" argument must be specified'
});

const server = tls.createServer(serverOptions, function(c) {
serverResults.push(c.servername);
});
Expand Down