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.

Sign up for GitHub

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

tls: fix inconsistent (hostname vs host) #21450

Closed
wants to merge 1 commit into from
Closed

tls: fix inconsistent (hostname vs host) #21450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 6 additions & 6 deletions lib/tls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,15 +86,15 @@ exports.convertALPNProtocols = function(protocols, out) {
}
};

function unfqdn(host) {
return host.replace(/[.]$/, '');
function unfqdn(hostname) {
return hostname.replace(/[.]$/, '');
}

function splitHost(host) {
function splitHost(hostname) {
// String#toLowerCase() is locale-sensitive so we use
// a conservative version that only lowercases A-Z.
const replacer = (c) => String.fromCharCode(32 + c.charCodeAt(0));
return unfqdn(host).replace(/[A-Z]/g, replacer).split('.');
return unfqdn(hostname).replace(/[A-Z]/g, replacer).split('.');
}

function check(hostParts, pattern, wildcards) {
Expand Down Expand Up @@ -221,12 +221,12 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
valid = wildcard(cn);

if (!valid)
reason = `Host: ${hostname}. is not cert's CN: ${cn}`;
reason = `Hostname: ${hostname} is not cert's CN: ${cn}`;
Copy link
Member

Choose a reason for hiding this comment

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

The removal of the dot misses the, ah, point of unfqdn(), see the comment on line 209. Including the FQDN in the error message is intentional and should not be changed without good reason.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, got it, a comment here would be good then because that is most certainly not obvious.

Copy link
Member

@Trott Trott Apr 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe wrapping it in quotation marks might help it be mildly more obvious that it's not a typo?

Suggested change
reason = `Hostname: ${hostname} is not cert's CN: ${cn}`;
reason = `Hostname: "${hostname}." is not cert's CN: ${cn}`;

} else {
valid = dnsNames.some(wildcard) || uriNames.some(noWildcard);
if (!valid)
reason =
`Host: ${hostname}. is not in the cert's altnames: ${altNames}`;
`Hostname: ${hostname} is not in the cert's altnames: ${altNames}`;
}
} else {
reason = 'Cert is empty';
Expand Down
40 changes: 20 additions & 20 deletions test/parallel/test-tls-check-server-identity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,17 @@ const tests = [
{
host: false,
cert: { subject: { CN: 'a.com' } },
error: 'Host: false. is not cert\'s CN: a.com'
error: 'Hostname: false is not cert\'s CN: a.com'
},
{
host: null,
cert: { subject: { CN: 'a.com' } },
error: 'Host: null. is not cert\'s CN: a.com'
error: 'Hostname: null is not cert\'s CN: a.com'
},
{
host: undefined,
cert: { subject: { CN: 'a.com' } },
error: 'Host: undefined. is not cert\'s CN: a.com'
error: 'Hostname: undefined is not cert\'s CN: a.com'
},

// Basic CN handling
Expand All @@ -61,37 +61,37 @@ const tests = [
{
host: 'a.com',
cert: { subject: { CN: 'b.com' } },
error: 'Host: a.com. is not cert\'s CN: b.com'
error: 'Hostname: a.com is not cert\'s CN: b.com'
},
{ host: 'a.com', cert: { subject: { CN: 'a.com.' } } },
{
host: 'a.com',
cert: { subject: { CN: '.a.com' } },
error: 'Host: a.com. is not cert\'s CN: .a.com'
error: 'Hostname: a.com is not cert\'s CN: .a.com'
},

// Wildcards in CN
{ host: 'b.a.com', cert: { subject: { CN: '*.a.com' } } },
{
host: 'ba.com',
cert: { subject: { CN: '*.a.com' } },
error: 'Host: ba.com. is not cert\'s CN: *.a.com'
error: 'Hostname: ba.com is not cert\'s CN: *.a.com'
},
{
host: '\n.b.com',
cert: { subject: { CN: '*n.b.com' } },
error: 'Host: \n.b.com. is not cert\'s CN: *n.b.com'
error: 'Hostname: \n.b.com is not cert\'s CN: *n.b.com'
},
{ host: 'b.a.com', cert: {
subjectaltname: 'DNS:omg.com',
subject: { CN: '*.a.com' } },
error: 'Host: b.a.com. is not in the cert\'s altnames: ' +
error: 'Hostname: b.a.com is not in the cert\'s altnames: ' +
'DNS:omg.com'
},
{
host: 'b.a.com',
cert: { subject: { CN: 'b*b.a.com' } },
error: 'Host: b.a.com. is not cert\'s CN: b*b.a.com'
error: 'Hostname: b.a.com is not cert\'s CN: b*b.a.com'
},

// Empty Cert
Expand All @@ -114,15 +114,15 @@ const tests = [
subjectaltname: 'DNS:*',
subject: { CN: 'b.com' }
},
error: 'Host: a.com. is not in the cert\'s altnames: ' +
error: 'Hostname: a.com is not in the cert\'s altnames: ' +
'DNS:*'
},
{
host: 'a.com', cert: {
subjectaltname: 'DNS:*.com',
subject: { CN: 'b.com' }
},
error: 'Host: a.com. is not in the cert\'s altnames: ' +
error: 'Hostname: a.com is not in the cert\'s altnames: ' +
'DNS:*.com'
},
{
Expand All @@ -136,15 +136,15 @@ const tests = [
subjectaltname: 'DNS:*.a.com',
subject: { CN: 'a.com' }
},
error: 'Host: a.com. is not in the cert\'s altnames: ' +
error: 'Hostname: a.com is not in the cert\'s altnames: ' +
'DNS:*.a.com'
},
{
host: 'a.com', cert: {
subjectaltname: 'DNS:*.a.com',
subject: { CN: 'b.com' }
},
error: 'Host: a.com. is not in the cert\'s altnames: ' +
error: 'Hostname: a.com is not in the cert\'s altnames: ' +
'DNS:*.a.com'
},
{
Expand All @@ -166,7 +166,7 @@ const tests = [
subjectaltname: 'DNS:*.a.com',
subject: {}
},
error: 'Host: a.com. is not in the cert\'s altnames: ' +
error: 'Hostname: a.com is not in the cert\'s altnames: ' +
'DNS:*.a.com'
},
{
Expand All @@ -180,7 +180,7 @@ const tests = [
subjectaltname: 'DNS:*.a.com',
subject: {}
},
error: 'Host: c.b.a.com. is not in the cert\'s altnames: ' +
error: 'Hostname: c.b.a.com is not in the cert\'s altnames: ' +
'DNS:*.a.com'
},
{
Expand All @@ -200,7 +200,7 @@ const tests = [
subjectaltname: 'DNS:*b.a.com',
subject: {}
},
error: 'Host: a.b.a.com. is not in the cert\'s altnames: ' +
error: 'Hostname: a.b.a.com is not in the cert\'s altnames: ' +
'DNS:*b.a.com'
},
// Multiple DNS names
Expand All @@ -222,7 +222,7 @@ const tests = [
subjectaltname: 'URI:http://*.b.a.com/',
subject: {}
},
error: 'Host: a.b.a.com. is not in the cert\'s altnames: ' +
error: 'Hostname: a.b.a.com is not in the cert\'s altnames: ' +
'URI:http://*.b.a.com/'
},
// Invalid URI
Expand All @@ -238,7 +238,7 @@ const tests = [
subjectaltname: 'IP Address:127.0.0.1',
subject: {}
},
error: 'Host: a.b.a.com. is not in the cert\'s altnames: ' +
error: 'Hostname: a.b.a.com is not in the cert\'s altnames: ' +
'IP Address:127.0.0.1'
},
{
Expand Down Expand Up @@ -267,7 +267,7 @@ const tests = [
subjectaltname: 'DNS:a.com',
subject: { CN: 'localhost' }
},
error: 'Host: localhost. is not in the cert\'s altnames: ' +
error: 'Hostname: localhost is not in the cert\'s altnames: ' +
'DNS:a.com'
},
// IDNA
Expand All @@ -281,7 +281,7 @@ const tests = [
{
host: 'xn--bcher-kva.example.com',
cert: { subject: { CN: 'xn--*.example.com' } },
error: 'Host: xn--bcher-kva.example.com. is not cert\'s CN: ' +
error: 'Hostname: xn--bcher-kva.example.com is not cert\'s CN: ' +
'xn--*.example.com',
},
];
Expand Down