feat(NODE-6289): allow valid srv hostnames with less than 3 parts #4197

aditi-khare-mongoDB · 2024-08-15T15:49:15Z

Description

Downstream changes for DRIVERS-2922 (PR).

What is changing?

Remove logic asserting that SRV URIs need 3 parts
When we check if a returned address matches its parent SRV, an SRV with <3 parts must assert that the returned address contains an additional domain level
Add in prose tests.

Is there new documentation needed for these changes?

No

What is the motivation for this change?

Do not throw an error on valid URI formats pre-DNS resolution, and require stricter domain matching post-DNS resolution.

Release Highlight

Allow SRV hostnames with less than three `.` separated parts

The client now accepts SRV hostname strings with one or two . separated parts.

For example, the following code no longer throws an error.

await new MongoClient('mongodb+srv://localhost').connect();

For security reasons, the returned addresses of SRV strings with less than three parts must end with the entire SRV hostname and contain at least one additional domain level. This is because this added validation ensures that the returned address(es) are from a known host. In future releases, we plan on extending this validation to SRV strings with three or more parts, as well.

// Example 1: Validation fails since the returned address doesn't end with the entire SRV hostname
'mongodb+srv://mySite.com' => 'myEvilSite.com' 

// Example 2: Validation fails since the returned address is identical to the SRV hostname
'mongodb+srv://mySite.com' => 'mySite.com' 

// Example 3: Validation passes since the returned address ends with the entire SRV hostname and contains an additional domain level
'mongodb+srv://mySite.com' => 'cluster_1.mySite.com'

Double check the following

Ran npm run check:lint script
Self-review completed using the steps outlined here
PR title follows the correct format: type(NODE-xxxx)[!]: description
- Example: feat(NODE-1234)!: rewriting everything in coffeescript
Changes are covered by tests
New TODOs have a related JIRA ticket

test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.prose.test.ts

test/unit/utils.test.ts

test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.prose.test.ts

dariakp · 2024-09-30T21:10:32Z

@aditi-khare-mongoDB @W-A-James Can we confirm the failing tests are all accounted for?

aditi-khare-mongoDB · 2024-09-30T21:34:01Z

@dariakp Seems like the two red failing tests are flaky. I doubt the purple failures would contain any failures if they weren't system failures. For example, 5.0-node-latest-server fails but not 4.0-node-latest or 6.0-node-latest, which makes me assume 5.0-node-latest would pass if not for the CI issues.

dariakp · 2024-09-30T21:30:06Z

src/sdam/srv_polling.ts

@@ -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 {


I couldn't find a kickoff document for this work, so I am not sure if this is something that has already been discussed, but how frequently is this function invoked? Is it in the hot path for anything? This construction, while cleaner, is less performant (since try catch is more expensive than a simple if/then, and the additional function call for the no-op may or may not get optimized).

dariakp · 2024-09-30T21:35:56Z

src/utils.ts

+    normalizedAddress.split('.').length <= normalizedSrvHost.split('.').length
+  ) {
+    // TODO(NODE-3484): Replace with MongoConnectionStringError
+    throw new MongoAPIError('Server record does not have least one more domain than parent URI');


I think the grammar on this needs some adjustment - could you double check the wording? I think it was probably meant to be Server record does not have at least one more domain level than parent URI

good catch!

dariakp · 2024-09-30T21:39:29Z

src/utils.ts

+    throw new MongoAPIError('Server record does not have least one more domain than parent URI');
+  }
+  if (!addressDomain.endsWith(srvHostDomain)) {
+    // TODO(NODE-3484): Replace with MongoConnectionStringError


NODE-3484 is about MongoParseError specifically, is that the right ticket for this work and do we definitely want a more specific error here? Even if so, I am not sure that we'd want to call it "MongoConnectionString", since, if I understand correcty. we are testing resolved records and not something that is specified in the connection string itself.

I've removed the TODO, it's a remanent from when this function was first implemented, and the ticket has sat in the backlog for years at this point. More importantly, it wouldn't be a connection string error technically, more of a 'SRV resolution returns invalid values' error, so i think MongoAPIError is sufficient here.

dariakp · 2024-09-30T22:04:51Z

test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.prose.test.ts

+import { topologyWithPlaceholderClient } from '../../tools/utils';
+
+describe('Initial DNS Seedlist Discovery (Prose Tests)', () => {
+  context('1. When running validation on an SRV string before DNS resolution', function () {


For easy audit purposes (e.g. in evg), we should match the number and title of the prose test in the context block and capture the full instructions text in comments.

dariakp · 2024-09-30T22:22:13Z

test/unit/utils.test.ts

@@ -939,7 +939,7 @@ describe('driver utils', function () {
    });
  });

-  describe('matchesParentDomain()', () => {
+  describe('checkParentDomainMatch()', () => {


should we add analogous tests for what happens when we have a trailing dot on a 2- or 1- part srv?

Good idea! Just added them in

dariakp · 2024-09-30T22:33:10Z

src/utils.ts

  // 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 = srvHost.split('.').length < 3;


Is this the correct check? I would think we are interested in checking whether the normalized host has fewer than 3 parts, since I don't think we want to consider the trailing dot on a 2- part host any differently, if I understand correctly. If you agree, could you make sure that there are new tests in the unit test section that fail with the code as it currently is and pass when the code is updated to check the normalized part count instead?

Yep, you're right, I've added in a new test that would have failed otherwise (the 'extra domain level' one) and fixed a typo in these section.

drivers-2922 downstream changes first pass

73ca011

aditi-khare-mongoDB mentioned this pull request Aug 15, 2024

DRIVERS-2922: Allow valid SRV hostnames with fewer than 3 parts mongodb/specifications#1628

Merged

4 tasks

aditi-khare-mongoDB changed the title ~~feat(NODE-6289): DRIVERS-2922 Downstream Changes PoC~~ feat(NODE-6289): DRIVERS 2922 Downstream Changes PoC Aug 15, 2024

aditi-khare-mongoDB added 2 commits August 19, 2024 16:25

temp commit

ad151f7

added more prose tests

15dc8ee

aditi-khare-mongoDB changed the title ~~feat(NODE-6289): DRIVERS 2922 Downstream Changes PoC~~ feat(NODE-6289): Allow valid SRV hostnames with less than 3 parts Sep 5, 2024

wording fix

586f7c0

aditi-khare-mongoDB force-pushed the uri-validate-less branch from bda0451 to 586f7c0 Compare September 5, 2024 05:02

aditi-khare-mongoDB and others added 3 commits September 5, 2024 01:02

Merge branch 'main' into uri-validate-less

36b0b49

remove stray comment

bea037e

fix up

ae79ac5

aditi-khare-mongoDB changed the title ~~feat(NODE-6289): Allow valid SRV hostnames with less than 3 parts~~ feat(NODE-6289): allow valid srv hostnames with less than 3 parts Sep 5, 2024

aditi-khare-mongoDB and others added 4 commits September 5, 2024 01:34

add back in comment

b2f843e

fix failing unit tests

154e2ad

Merge branch 'main' into uri-validate-less

1059175

Merge branch 'main' into uri-validate-less

d1209df

aditi-khare-mongoDB marked this pull request as ready for review September 19, 2024 15:45

W-A-James self-assigned this Sep 19, 2024

W-A-James self-requested a review September 19, 2024 15:47

W-A-James added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Sep 19, 2024

W-A-James requested changes Sep 19, 2024

View reviewed changes

requested changes 1

9dd438e

aditi-khare-mongoDB requested a review from W-A-James September 19, 2024 21:49

W-A-James requested changes Sep 20, 2024

View reviewed changes

aditi-khare-mongoDB and others added 4 commits September 24, 2024 16:59

requested changes + review from drivers ticket updates

f680380

fix failing tests

4d3efd8

fix failing tests 2

062c139

Merge branch 'main' into uri-validate-less

a3b489c

aditi-khare-mongoDB requested a review from W-A-James September 25, 2024 21:06

W-A-James requested changes Sep 26, 2024

View reviewed changes

test/integration/initial-dns-seedlist-discovery/initial_dns_seedlist_discovery.prose.test.ts Outdated Show resolved Hide resolved

aditi-khare-mongoDB and others added 2 commits September 26, 2024 14:01

await close

7895835

Merge branch 'main' into uri-validate-less

4879b78

aditi-khare-mongoDB requested a review from W-A-James September 26, 2024 18:02

W-A-James previously approved these changes Sep 27, 2024

View reviewed changes

W-A-James added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Sep 27, 2024

nbbeeken requested a review from dariakp September 30, 2024 14:45

dariakp requested changes Sep 30, 2024

View reviewed changes

ready for rereivew

79f41e4

aditi-khare-mongoDB dismissed W-A-James’s stale review via 79f41e4 October 8, 2024 20:45

aditi-khare-mongoDB force-pushed the uri-validate-less branch from cdc2501 to 79f41e4 Compare October 8, 2024 20:45

ready for rereivew

eafaf61

aditi-khare-mongoDB force-pushed the uri-validate-less branch from d17f346 to eafaf61 Compare October 8, 2024 20:45

Merge branch 'main' into uri-validate-less

1de28de

aditi-khare-mongoDB requested review from dariakp and W-A-James October 8, 2024 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-6289): allow valid srv hostnames with less than 3 parts #4197

feat(NODE-6289): allow valid srv hostnames with less than 3 parts #4197

aditi-khare-mongoDB commented Aug 15, 2024 •

edited

Loading

dariakp commented Sep 30, 2024

aditi-khare-mongoDB commented Sep 30, 2024

dariakp Sep 30, 2024

dariakp Sep 30, 2024

aditi-khare-mongoDB Oct 8, 2024

dariakp Sep 30, 2024

aditi-khare-mongoDB Oct 8, 2024 •

edited

Loading

dariakp Sep 30, 2024

dariakp Sep 30, 2024

aditi-khare-mongoDB Oct 8, 2024

dariakp Sep 30, 2024

aditi-khare-mongoDB Oct 8, 2024

feat(NODE-6289): allow valid srv hostnames with less than 3 parts #4197

Are you sure you want to change the base?

feat(NODE-6289): allow valid srv hostnames with less than 3 parts #4197

Conversation

aditi-khare-mongoDB commented Aug 15, 2024 • edited Loading

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Release Highlight

Allow SRV hostnames with less than three . separated parts

Double check the following

dariakp commented Sep 30, 2024

aditi-khare-mongoDB commented Sep 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditi-khare-mongoDB Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditi-khare-mongoDB commented Aug 15, 2024 •

edited

Loading

Allow SRV hostnames with less than three `.` separated parts

aditi-khare-mongoDB Oct 8, 2024 •

edited

Loading