Skip to content

Commit

Permalink
tls: better error reporting at cert validation
Browse files Browse the repository at this point in the history 
fix #7417

Signed-off-by: Fedor Indutny <fedor@indutny.com>
  • Loading branch information
@indutny
indutny committed Jul 2, 2014
1 parent ae1e325 commit e345253
Show file tree
Hide file tree
Showing 5 changed files with 88 additions and 54 deletions.
6 changes: 1 addition & 5 deletions lib/_tls_wrap.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -910,11 +910,7 @@ exports.connect = function(/* [port, host], options, cb */) {
// Verify that server's identity matches it's certificate's names
if (!verifyError) {
var cert = result.getPeerCertificate();
var validCert = tls.checkServerIdentity(hostname, cert);
if (!validCert) {
verifyError = new Error('Hostname/IP doesn\'t match certificate\'s ' +
'altnames');
}
verifyError = tls.checkServerIdentity(hostname, cert);
}

if (verifyError) {
Expand Down
30 changes: 28 additions & 2 deletions lib/tls.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@ exports.checkServerIdentity = function checkServerIdentity(host, cert) {
uriNames = [],
ips = [],
matchCN = true,
valid = false;
valid = false,
reason = 'Unknown reason';

// There're several names to perform check against:
// CN and altnames in certificate extension
Expand Down Expand Up @@ -142,6 +143,11 @@ exports.checkServerIdentity = function checkServerIdentity(host, cert) {
valid = ips.some(function(ip) {
return ip === host;
});
if (!valid) {
reason = util.format('IP: %s is not in the cert\'s list: %s',
host,
ips.join(', '));
}
} else {
// Transform hostname to canonical form
if (!/\.$/.test(host)) host += '.';
Expand Down Expand Up @@ -183,9 +189,29 @@ exports.checkServerIdentity = function checkServerIdentity(host, cert) {
valid = dnsNames.some(function(re) {
return re.test(host);
});

if (!valid) {
if (cert.subjectaltname) {
reason = util.format('Host: %s is not in the cert\'s altnames: %s',
host,
cert.subjectaltname);
} else {
reason = util.format('Host: %s is not cert\'s CN: %s',
host,
cert.subject.CN);
}
}
}

return valid;
if (!valid) {
var err = new Error(
util.format('Hostname/IP doesn\'t match certificate\'s altnames: %j',
reason));
err.reason = reason;
err.host = host;
err.cert = cert;
return err;
}
};

// Example:
Expand Down
12 changes: 8 additions & 4 deletions test/simple/test-https-strict.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,10 +180,12 @@ function allListening() {
// server1: host 'agent1', signed by ca1
makeReq('/inv1', port1, 'UNABLE_TO_VERIFY_LEAF_SIGNATURE');
makeReq('/inv1-ca1', port1,
'Hostname/IP doesn\'t match certificate\'s altnames',
'Hostname/IP doesn\'t match certificate\'s altnames: ' +
'"Host: localhost. is not cert\'s CN: agent1"',
null, ca1);
makeReq('/inv1-ca1ca2', port1,
'Hostname/IP doesn\'t match certificate\'s altnames',
'Hostname/IP doesn\'t match certificate\'s altnames: ' +
'"Host: localhost. is not cert\'s CN: agent1"',
null, [ca1, ca2]);
makeReq('/val1-ca1', port1, null, 'agent1', ca1);
makeReq('/val1-ca1ca2', port1, null, 'agent1', [ca1, ca2]);
Expand All @@ -201,10 +203,12 @@ function allListening() {
// server3: host 'agent3', signed by ca2
makeReq('/inv3', port3, 'UNABLE_TO_VERIFY_LEAF_SIGNATURE');
makeReq('/inv3-ca2', port3,
'Hostname/IP doesn\'t match certificate\'s altnames',
'Hostname/IP doesn\'t match certificate\'s altnames: ' +
'"Host: localhost. is not cert\'s CN: agent3"',
null, ca2);
makeReq('/inv3-ca1ca2', port3,
'Hostname/IP doesn\'t match certificate\'s altnames',
'Hostname/IP doesn\'t match certificate\'s altnames: ' +
'"Host: localhost. is not cert\'s CN: agent3"',
null, [ca1, ca2]);
makeReq('/val3-ca2', port3, null, 'agent3', ca2);
makeReq('/val3-ca1ca2', port3, null, 'agent3', [ca1, ca2]);
Expand Down
90 changes: 49 additions & 41 deletions test/simple/test-tls-check-server-identity.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,25 +26,29 @@ var tls = require('tls');

var tests = [
// Basic CN handling
{ host: 'a.com', cert: { subject: { CN: 'a.com' } }, result: true },
{ host: 'a.com', cert: { subject: { CN: 'A.COM' } }, result: true },
{ host: 'a.com', cert: { subject: { CN: 'b.com' } }, result: false },
{ host: 'a.com', cert: { subject: { CN: 'a.com.' } }, result: true },
{ host: 'a.com', cert: { subject: { CN: 'a.com' } } },
{ host: 'a.com', cert: { subject: { CN: 'A.COM' } } },
{
host: 'a.com',
cert: { subject: { CN: 'b.com' } },
error: 'Host: a.com. is not cert\'s CN: b.com'
},
{ host: 'a.com', cert: { subject: { CN: 'a.com.' } } },

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

// Multiple CN fields
{
host: 'foo.com', cert: {
subject: { CN: ['foo.com', 'bar.com'] } // CN=foo.com; CN=bar.com;
},
result: true
}
},

// DNS names and CN
Expand All @@ -53,49 +57,50 @@ var tests = [
subjectaltname: 'DNS:*',
subject: { CN: 'b.com' }
},
result: false
error: 'Host: a.com. is not in the cert\'s altnames: ' +
'DNS:*'
},
{
host: 'a.com', cert: {
subjectaltname: 'DNS:*.com',
subject: { CN: 'b.com' }
},
result: false
error: 'Host: a.com. is not in the cert\'s altnames: ' +
'DNS:*.com'
},
{
host: 'a.co.uk', cert: {
subjectaltname: 'DNS:*.co.uk',
subject: { CN: 'b.com' }
},
result: true
}
},
{
host: 'a.com', cert: {
subjectaltname: 'DNS:*.a.com',
subject: { CN: 'a.com' }
},
result: false
error: 'Host: a.com. is not in the cert\'s altnames: ' +
'DNS:*.a.com'
},
{
host: 'a.com', cert: {
subjectaltname: 'DNS:*.a.com',
subject: { CN: 'b.com' }
},
result: false
error: 'Host: a.com. is not in the cert\'s altnames: ' +
'DNS:*.a.com'
},
{
host: 'a.com', cert: {
subjectaltname: 'DNS:a.com',
subject: { CN: 'b.com' }
},
result: true
}
},
{
host: 'a.com', cert: {
subjectaltname: 'DNS:A.COM',
subject: { CN: 'b.com' }
},
result: true
}
},

// DNS names
Expand All @@ -104,106 +109,109 @@ var tests = [
subjectaltname: 'DNS:*.a.com',
subject: {}
},
result: false
error: 'Host: a.com. is not in the cert\'s altnames: ' +
'DNS:*.a.com'
},
{
host: 'b.a.com', cert: {
subjectaltname: 'DNS:*.a.com',
subject: {}
},
result: true
}
},
{
host: 'c.b.a.com', cert: {
subjectaltname: 'DNS:*.a.com',
subject: {}
},
result: false
error: 'Host: c.b.a.com. is not in the cert\'s altnames: ' +
'DNS:*.a.com'
},
{
host: 'b.a.com', cert: {
subjectaltname: 'DNS:*b.a.com',
subject: {}
},
result: true
}
},
{
host: 'a-cb.a.com', cert: {
subjectaltname: 'DNS:*b.a.com',
subject: {}
},
result: true
}
},
{
host: 'a.b.a.com', cert: {
subjectaltname: 'DNS:*b.a.com',
subject: {}
},
result: false
error: 'Host: a.b.a.com. is not in the cert\'s altnames: ' +
'DNS:*b.a.com'
},
// Mutliple DNS names
{
host: 'a.b.a.com', cert: {
subjectaltname: 'DNS:*b.a.com, DNS:a.b.a.com',
subject: {}
},
result: true
}
},
// URI names
{
host: 'a.b.a.com', cert: {
subjectaltname: 'URI:http://a.b.a.com/',
subject: {}
},
result: true
}
},
{
host: 'a.b.a.com', cert: {
subjectaltname: 'URI:http://*.b.a.com/',
subject: {}
},
result: false
error: 'Host: a.b.a.com. is not in the cert\'s altnames: ' +
'URI:http://*.b.a.com/'
},
// IP addresses
{
host: 'a.b.a.com', cert: {
subjectaltname: 'IP Address:127.0.0.1',
subject: {}
},
result: false
error: 'Host: a.b.a.com. is not in the cert\'s altnames: ' +
'IP Address:127.0.0.1'
},
{
host: '127.0.0.1', cert: {
subjectaltname: 'IP Address:127.0.0.1',
subject: {}
},
result: true
}
},
{
host: '127.0.0.2', cert: {
subjectaltname: 'IP Address:127.0.0.1',
subject: {}
},
result: false
error: 'IP: 127.0.0.2 is not in the cert\'s list: ' +
'127.0.0.1'
},
{
host: '127.0.0.1', cert: {
subjectaltname: 'DNS:a.com',
subject: {}
},
result: false
error: 'IP: 127.0.0.1 is not in the cert\'s list: '
},
{
host: 'localhost', cert: {
subjectaltname: 'DNS:a.com',
subject: { CN: 'localhost' }
},
result: false
error: 'Host: localhost. is not in the cert\'s altnames: ' +
'DNS:a.com'
},
];

tests.forEach(function(test, i) {
assert.equal(tls.checkServerIdentity(test.host, test.cert),
test.result,
'Test#' + i + ' failed: ' + util.inspect(test));
var err = tls.checkServerIdentity(test.host, test.cert);
assert.equal(err && err.reason,
test.error,
'Test#' + i + ' failed: ' + util.inspect(test) + '\n' +
test.error + ' != ' + (err && err.reason));
});
4 changes: 2 additions & 2 deletions test/simple/test-tls-client-verify.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ if (!process.versions.openssl) {
}


var hosterr = 'Hostname/IP doesn\'t match certificate\'s altnames';
var hosterr = /Hostname\/IP doesn\'t match certificate\'s altnames/g;
var testCases =
[{ ca: ['ca1-cert'],
key: 'agent2-key',
Expand Down Expand Up @@ -103,7 +103,7 @@ function testServers(index, servers, clientOptions, cb) {
console.error('connecting...');
var client = tls.connect(clientOptions, function() {
var authorized = client.authorized ||
client.authorizationError === hosterr;
hosterr.test(client.authorizationError);

console.error('expected: ' + ok + ' authed: ' + authorized);

Expand Down

0 comments on commit e345253

Please sign in to comment.