Skip to content

Commit

Permalink
feat(NODE-6289): allow valid srv hostnames with less than 3 parts (#4197
Browse files Browse the repository at this point in the history
)
aditi-khare-mongoDB authored Oct 15, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent 7fde8dd commit 3d5bd51
Showing 5 changed files with 406 additions and 50 deletions.
11 changes: 2 additions & 9 deletions src/connection_string.ts
Original file line number Diff line number Diff line change
@@ -34,11 +34,11 @@ import { ReadPreference, type ReadPreferenceMode } from './read_preference';
import { ServerMonitoringMode } from './sdam/monitor';
import type { TagSet } from './sdam/server_description';
import {
checkParentDomainMatch,
DEFAULT_PK_FACTORY,
emitWarning,
HostAddress,
isRecord,
matchesParentDomain,
parseInteger,
setDifference,
squashError
@@ -64,11 +64,6 @@ export async function resolveSRVRecord(options: MongoOptions): Promise<HostAddre
throw new MongoAPIError('Option "srvHost" must not be empty');
}

if (options.srvHost.split('.').length < 3) {
// TODO(NODE-3484): Replace with MongoConnectionStringError
throw new MongoAPIError('URI must include hostname, domain name, and tld');
}

// Asynchronously start TXT resolution so that we do not have to wait until
// the SRV record is resolved before starting a second DNS query.
const lookupAddress = options.srvHost;
@@ -86,9 +81,7 @@ export async function resolveSRVRecord(options: MongoOptions): Promise<HostAddre
}

for (const { name } of addresses) {
if (!matchesParentDomain(name, lookupAddress)) {
throw new MongoAPIError('Server record does not share hostname with parent URI');
}
checkParentDomainMatch(name, lookupAddress);
}

const hostAddresses = addresses.map(r => HostAddress.fromString(`${r.name}:${r.port ?? 27017}`));
7 changes: 5 additions & 2 deletions src/sdam/srv_polling.ts
Original file line number Diff line number Diff line change
@@ -3,7 +3,7 @@ import { clearTimeout, setTimeout } from 'timers';

import { MongoRuntimeError } from '../error';
import { TypedEventEmitter } from '../mongo_types';
import { HostAddress, matchesParentDomain, squashError } from '../utils';
import { checkParentDomainMatch, HostAddress, squashError } from '../utils';

/**
* @internal
@@ -127,8 +127,11 @@ export class SrvPoller extends TypedEventEmitter<SrvPollerEvents> {

const finalAddresses: dns.SrvRecord[] = [];
for (const record of srvRecords) {
if (matchesParentDomain(record.name, this.srvHost)) {
try {
checkParentDomainMatch(record.name, this.srvHost);
finalAddresses.push(record);
} catch (error) {
squashError(error);
}
}

29 changes: 24 additions & 5 deletions src/utils.ts
Original file line number Diff line number Diff line change
@@ -18,6 +18,7 @@ import type { FindCursor } from './cursor/find_cursor';
import type { Db } from './db';
import {
type AnyError,
MongoAPIError,
MongoCompatibilityError,
MongoInvalidArgumentError,
MongoNetworkTimeoutError,
@@ -1142,29 +1143,47 @@ export function parseUnsignedInteger(value: unknown): number | null {
}

/**
* Determines whether a provided address matches the provided parent domain.
* This function throws a MongoAPIError in the event that either of the following is true:
* * If the provided address domain does not match the provided parent domain
* * If the parent domain contains less than three `.` separated parts and the provided address does not contain at least one more domain level than its parent
*
* If a DNS server were to become compromised SRV records would still need to
* advertise addresses that are under the same domain as the srvHost.
*
* @param address - The address to check against a domain
* @param srvHost - The domain to check the provided address against
* @returns Whether the provided address matches the parent domain
* @returns void
*/
export function matchesParentDomain(address: string, srvHost: string): boolean {
export function checkParentDomainMatch(address: string, srvHost: string): void {
// Remove trailing dot if exists on either the resolved address or the srv hostname
const normalizedAddress = address.endsWith('.') ? address.slice(0, address.length - 1) : address;
const normalizedSrvHost = srvHost.endsWith('.') ? srvHost.slice(0, srvHost.length - 1) : srvHost;

const allCharacterBeforeFirstDot = /^.*?\./;
const srvIsLessThanThreeParts = normalizedSrvHost.split('.').length < 3;
// Remove all characters before first dot
// Add leading dot back to string so
// an srvHostDomain = '.trusted.site'
// will not satisfy an addressDomain that endsWith '.fake-trusted.site'
const addressDomain = `.${normalizedAddress.replace(allCharacterBeforeFirstDot, '')}`;
const srvHostDomain = `.${normalizedSrvHost.replace(allCharacterBeforeFirstDot, '')}`;
let srvHostDomain = srvIsLessThanThreeParts
? normalizedSrvHost
: `.${normalizedSrvHost.replace(allCharacterBeforeFirstDot, '')}`;

return addressDomain.endsWith(srvHostDomain);
if (!srvHostDomain.startsWith('.')) {
srvHostDomain = '.' + srvHostDomain;
}
if (
srvIsLessThanThreeParts &&
normalizedAddress.split('.').length <= normalizedSrvHost.split('.').length
) {
throw new MongoAPIError(
'Server record does not have at least one more domain level than parent URI'
);
}
if (!addressDomain.endsWith(srvHostDomain)) {
throw new MongoAPIError('Server record does not share hostname with parent URI');
}
}

interface RequestOptions {
Original file line number Diff line number Diff line change
@@ -0,0 +1,293 @@
import { expect } from 'chai';
import * as dns from 'dns';
import * as sinon from 'sinon';

import { MongoAPIError, Server, ServerDescription, Topology } from '../../mongodb';
import { topologyWithPlaceholderClient } from '../../tools/utils';

describe('Initial DNS Seedlist Discovery (Prose Tests)', () => {
describe('1. Allow SRVs with fewer than 3 . separated parts', function () {
context('when running validation on an SRV string before DNS resolution', function () {
/**
* When running validation on an SRV string before DNS resolution, do not throw a error due to number of SRV parts.
* - mongodb+srv://localhost
* - mongodb+srv://mongo.localhost
*/

let client;

beforeEach(async function () {
// this fn stubs DNS resolution to always pass - so we are only checking pre-DNS validation

sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
return [
{
name: 'resolved.mongo.localhost',
port: 27017,
weight: 0,
priority: 0
}
];
});

sinon.stub(dns.promises, 'resolveTxt').callsFake(async () => {
throw { code: 'ENODATA' };
});

sinon.stub(Topology.prototype, 'selectServer').callsFake(async () => {
return new Server(
topologyWithPlaceholderClient([], {} as any),
new ServerDescription('a:1'),
{} as any
);
});
});

afterEach(async function () {
sinon.restore();
await client.close();
});

it('does not error on an SRV because it has one domain level', async function () {
client = await this.configuration.newClient('mongodb+srv://localhost', {});
await client.connect();
});

it('does not error on an SRV because it has two domain levels', async function () {
client = await this.configuration.newClient('mongodb+srv://mongo.localhost', {});
await client.connect();
});
});
});

describe('2. Throw when return address does not end with SRV domain', function () {
context(
'when given a host from DNS resolution that does NOT end with the original SRVs domain name',
function () {
/**
* When given a returned address that does NOT end with the original SRV's domain name, throw a runtime error.
* For this test, run each of the following cases:
* - the SRV mongodb+srv://localhost resolving to localhost.mongodb
* - the SRV mongodb+srv://mongo.local resolving to test_1.evil.local
* - the SRV mongodb+srv://blogs.mongodb.com resolving to blogs.evil.com
* Remember, the domain of an SRV with one or two . separated parts is the SRVs entire hostname.
*/

beforeEach(async function () {
sinon.stub(dns.promises, 'resolveTxt').callsFake(async () => {
throw { code: 'ENODATA' };
});
});

afterEach(async function () {
sinon.restore();
});

it('an SRV with one domain level causes a runtime error', async function () {
sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
return [
{
name: 'localhost.mongodb',
port: 27017,
weight: 0,
priority: 0
}
];
});
const err = await this.configuration
.newClient('mongodb+srv://localhost', {})
.connect()
.catch((e: any) => e);
expect(err).to.be.instanceOf(MongoAPIError);
expect(err.message).to.equal('Server record does not share hostname with parent URI');
});

it('an SRV with two domain levels causes a runtime error', async function () {
sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
return [
{
name: 'test_1.evil.local', // this string only ends with part of the domain, not all of it!
port: 27017,
weight: 0,
priority: 0
}
];
});
const err = await this.configuration
.newClient('mongodb+srv://mongo.local', {})
.connect()
.catch(e => e);
expect(err).to.be.instanceOf(MongoAPIError);
expect(err.message).to.equal('Server record does not share hostname with parent URI');
});

it('an SRV with three or more domain levels causes a runtime error', async function () {
sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
return [
{
name: 'blogs.evil.com',
port: 27017,
weight: 0,
priority: 0
}
];
});
const err = await this.configuration
.newClient('mongodb+srv://blogs.mongodb.com', {})
.connect()
.catch(e => e);
expect(err).to.be.instanceOf(MongoAPIError);
expect(err.message).to.equal('Server record does not share hostname with parent URI');
});
}
);
});

describe('3. Throw when return address is identical to SRV hostname', function () {
/**
* When given a returned address that is identical to the SRV hostname and the SRV hostname has fewer than three . separated parts, throw a runtime error.
* For this test, run each of the following cases:
* - the SRV mongodb+srv://localhost resolving to localhost
* - the SRV mongodb+srv://mongo.local resolving to mongo.local
*/

context(
'when given a host from DNS resolution that is identical to the original SRVs hostname',
function () {
beforeEach(async function () {
sinon.stub(dns.promises, 'resolveTxt').callsFake(async () => {
throw { code: 'ENODATA' };
});
});

afterEach(async function () {
sinon.restore();
});

it('an SRV with one domain level causes a runtime error', async function () {
sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
return [
{
name: 'localhost',
port: 27017,
weight: 0,
priority: 0
}
];
});
const err = await this.configuration
.newClient('mongodb+srv://localhost', {})
.connect()
.catch(e => e);
expect(err).to.be.instanceOf(MongoAPIError);
expect(err.message).to.equal(
'Server record does not have at least one more domain level than parent URI'
);
});

it('an SRV with two domain levels causes a runtime error', async function () {
sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
return [
{
name: 'mongo.local',
port: 27017,
weight: 0,
priority: 0
}
];
});
const err = await this.configuration
.newClient('mongodb+srv://mongo.local', {})
.connect()
.catch(e => e);
expect(err).to.be.instanceOf(MongoAPIError);
expect(err.message).to.equal(
'Server record does not have at least one more domain level than parent URI'
);
});
}
);
});

describe('4. Throw when return address does not contain . separating shared part of domain', function () {
/**
* When given a returned address that does NOT share the domain name of the SRV record because it's missing a ., throw a runtime error.
* For this test, run each of the following cases:
* - the SRV mongodb+srv://localhost resolving to test_1.cluster_1localhost
* - the SRV mongodb+srv://mongo.local resolving to test_1.my_hostmongo.local
* - the SRV mongodb+srv://blogs.mongodb.com resolving to cluster.testmongodb.com
*/

context(
'when given a returned address that does NOT share the domain name of the SRV record because its missing a `.`',
function () {
beforeEach(async function () {
sinon.stub(dns.promises, 'resolveTxt').callsFake(async () => {
throw { code: 'ENODATA' };
});
});

afterEach(async function () {
sinon.restore();
});

it('an SRV with one domain level causes a runtime error', async function () {
sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
return [
{
name: 'test_1.cluster_1localhost',
port: 27017,
weight: 0,
priority: 0
}
];
});
const err = await this.configuration
.newClient('mongodb+srv://localhost', {})
.connect()
.catch(e => e);
expect(err).to.be.instanceOf(MongoAPIError);
expect(err.message).to.equal('Server record does not share hostname with parent URI');
});

it('an SRV with two domain levels causes a runtime error', async function () {
sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
return [
{
name: 'test_1.my_hostmongo.local',
port: 27017,
weight: 0,
priority: 0
}
];
});
const err = await this.configuration
.newClient('mongodb+srv://mongo.local', {})
.connect()
.catch(e => e);
expect(err).to.be.instanceOf(MongoAPIError);
expect(err.message).to.equal('Server record does not share hostname with parent URI');
});

it('an SRV with three domain levels causes a runtime error', async function () {
sinon.stub(dns.promises, 'resolveSrv').callsFake(async () => {
return [
{
name: 'cluster.testmongodb.com',
port: 27017,
weight: 0,
priority: 0
}
];
});
const err = await this.configuration
.newClient('mongodb+srv://blogs.mongodb.com', {})
.connect()
.catch(e => e);
expect(err).to.be.instanceOf(MongoAPIError);
expect(err.message).to.equal('Server record does not share hostname with parent URI');
});
}
);
});
});
116 changes: 82 additions & 34 deletions test/unit/utils.test.ts
Original file line number Diff line number Diff line change
@@ -3,6 +3,7 @@ import { expect } from 'chai';
import {
BufferPool,
ByteUtils,
checkParentDomainMatch,
compareObjectId,
decorateWithExplain,
Explain,
@@ -12,7 +13,6 @@ import {
isUint8Array,
LEGACY_HELLO_COMMAND,
List,
matchesParentDomain,
MongoDBCollectionNamespace,
MongoDBNamespace,
MongoRuntimeError,
@@ -939,46 +939,94 @@ describe('driver utils', function () {
});
});

describe('matchesParentDomain()', () => {
const exampleSrvName = 'i-love-javascript.mongodb.io';
const exampleSrvNameWithDot = 'i-love-javascript.mongodb.io.';
const exampleHostNameWithoutDot = 'i-love-javascript-00.mongodb.io';
const exampleHostNamesWithDot = exampleHostNameWithoutDot + '.';
const exampleHostNamThatDoNotMatchParent = 'i-love-javascript-00.evil-mongodb.io';
const exampleHostNamThatDoNotMatchParentWithDot = 'i-love-javascript-00.evil-mongodb.io.';
describe('checkParentDomainMatch()', () => {
const exampleSrvName = ['i-love-js', 'i-love-js.mongodb', 'i-love-javascript.mongodb.io'];
const exampleSrvNameWithDot = [
'i-love-js.',
'i-love-js.mongodb.',
'i-love-javascript.mongodb.io.'
];
const exampleHostNameWithoutDot = [
'js-00.i-love-js',
'js-00.i-love-js.mongodb',
'i-love-javascript-00.mongodb.io'
];
const exampleHostNamesWithDot = [
'js-00.i-love-js.',
'js-00.i-love-js.mongodb.',
'i-love-javascript-00.mongodb.io.'
];
const exampleHostNameThatDoNotMatchParent = [
'js-00.i-love-js-a-little',
'js-00.i-love-js-a-little.mongodb',
'i-love-javascript-00.evil-mongodb.io'
];
const exampleHostNameThatDoNotMatchParentWithDot = [
'i-love-js',
'',
'i-love-javascript-00.evil-mongodb.io.'
];

context('when address does not match parent domain', () => {
it('without a trailing dot returns false', () => {
expect(matchesParentDomain(exampleHostNamThatDoNotMatchParent, exampleSrvName)).to.be.false;
});
for (let num = 0; num < 3; num += 1) {
context(`when srvName has ${num + 1} part${num !== 0 ? 's' : ''}`, () => {
context('when address does not match parent domain', () => {
it('without a trailing dot throws', () => {
expect(() =>
checkParentDomainMatch(exampleHostNameThatDoNotMatchParent[num], exampleSrvName[num])
).to.throw('Server record does not share hostname with parent URI');
});

it('with a trailing dot returns false', () => {
expect(matchesParentDomain(exampleHostNamThatDoNotMatchParentWithDot, exampleSrvName)).to.be
.false;
});
});
it('with a trailing dot throws', () => {
expect(() =>
checkParentDomainMatch(
exampleHostNameThatDoNotMatchParentWithDot[num],
exampleSrvName[num]
)
).to.throw();
});
});

context('when addresses in SRV record end with a dot', () => {
it('accepts address since it is considered to still match the parent domain', () => {
expect(matchesParentDomain(exampleHostNamesWithDot, exampleSrvName)).to.be.true;
});
});
context('when addresses in SRV record end with a dot', () => {
it('accepts address since it is considered to still match the parent domain', () => {
expect(() =>
checkParentDomainMatch(exampleHostNamesWithDot[num], exampleSrvName[num])
).to.not.throw();
});
});

context('when SRV host ends with a dot', () => {
it('accepts address if it ends with a dot', () => {
expect(matchesParentDomain(exampleHostNamesWithDot, exampleSrvNameWithDot)).to.be.true;
});
context('when SRV host ends with a dot', () => {
it('accepts address if it ends with a dot', () => {
expect(() =>
checkParentDomainMatch(exampleHostNamesWithDot[num], exampleSrvNameWithDot[num])
).to.not.throw();
});

it('accepts address if it does not end with a dot', () => {
expect(matchesParentDomain(exampleHostNameWithoutDot, exampleSrvName)).to.be.true;
});
});
it('accepts address if it does not end with a dot', () => {
expect(() =>
checkParentDomainMatch(exampleHostNameWithoutDot[num], exampleSrvNameWithDot[num])
).to.not.throw();
});

context('when addresses in SRV record end without dots', () => {
it('accepts address since it matches the parent domain', () => {
expect(matchesParentDomain(exampleHostNamesWithDot, exampleSrvName)).to.be.true;
if (num < 2) {
it('does not accept address if it does not contain an extra domain level', () => {
expect(() =>
checkParentDomainMatch(exampleSrvNameWithDot[num], exampleSrvNameWithDot[num])
).to.throw(
'Server record does not have at least one more domain level than parent URI'
);
});
}
});

context('when addresses in SRV record end without dots', () => {
it('accepts address since it matches the parent domain', () => {
expect(() =>
checkParentDomainMatch(exampleHostNamesWithDot[num], exampleSrvName[num])
).to.not.throw();
});
});
});
});
}
});

describe('isUint8Array()', () => {

0 comments on commit 3d5bd51

Please sign in to comment.