DRIVERS-3329: Configurable DNS domain validation for SRV records

[sleepyStick]

Please complete the following before merging:

• Is the relevant DRIVERS ticket in the PR title?

• Update changelog.
• Test changes in at least one language driver.
  Python PR: PYTHON-5814 Configurable DNS domain validation for SRV records mongo-python-driver#2868
• Test these changes against all server versions and topologies (including standalone, replica set, and sharded
  clusters).

[rozza]

It looks good, I have one comment about if an invalid srvAllowedHostsSuffix should throw an error explicitly? (if so should we update the yml?)

Also the yml and json has diverged for srvAllowedHostsSuffix-without_dot_pass - which is probably the lint error.

[matthewdale]

Optional: Consider linking to the Querying DNS section, which is where the DNS validation is described.

[sleepyStick]

added! thanks!

[codeowners-service-app]

Assigned vector-of-bool for team dbx-spec-maintainers-connection-string because aclark4life is out of office.

[matthewdale]

Looks good! 👍

[aclark4life]

Looks like the convention in yml is no quotes around uri

[aclark4life]

Missing space

[aclark4life]

Also a convention apparently is new lines at the end of yaml files.

[aclark4life]

Pending nit fixes LGTM

[ajcvickers]

Setting srvAllowedHostsSuffix to a public suffix neuters SRV anti-spoofing, turning mongodb+srv:// into an unbounded DNS redirect. A merely broad private domain doesn't make it unbounded, but widens the redirect to anywhere under that domain. There is no PSL/minimum-label guard to prevent either. Example:

Preconditions:

• Misconfiguration. App sets a too-broad suffix:
  mongodb+srv://mycluster.mongodb.example.com/?srvAllowedHostsSuffix=.com
• Attacker runs a malicious/compromised resolver, or poisons the cache. (DNSSEC is not enforced per SECURITY-488).
• Attacker obtains a CA-valid cert for a domain they own.

Step-by-step:

• Client SRV-queries _mongodb._tcp.mycluster.mongodb.example.com.
• Attacker forges the SRV response: target db1.attacker-evil.com:27017.
• Suffix validation passes (db1.attacker-evil.com ends in .com).
• mongodb+srv forces TLS; the client validates the cert against the SRV-returned name (db1.attacker-evil.com), not the typed name (mycluster.mongodb.example.com).
• Attacker owns that name and holds a valid cert → TLS verification passes → client connects to the attacker's server.

[ajcvickers]

I think the parsing needs to be much better specified and tested. It's important to understand what DNS entries allow and how to safely handle these things. These should be specified at the spec level, so all drivers will get the parsing correct. For example, most of these are not specified or tested:

• DNS is case-insensitive. (Also, ASCII-only case-folding.)
• SRV targets often return trailing dots. For example: host.citi.net. These must be normalized.
• IDN / punycode / homographs. Don't compare raw Unicode.
• Probably more... (I'm not an expert, but the above are the obvious ones.)

There needs to be much better test coverage here.

[Jibola]

@sanych-sun I've attempted to address all your statements as they're valid! My larger response here is we can feel pretty good about this option and its blend of simplicity for the end-user whilst also balancing some layer of validation that can let us rest knowing we've done something for this can of worms. Callbacks, whilst also a valid form, leave us providing more override mechanisms than we think we need for the problem we're tackling. For what it's worth, if we see genuine requests for it in the future, it can always be additive to the current system without being backwards breaking.

1. First of all - that suffix validation gives "false security" - what exactly we protect user from? If they worried about the security - they most likely should understand the best practices and try to specify "the more specific suffix as they can" and that validation will just run over and over and take some extra milliseconds on connection establishment. If we want to help - may be we can suggest how to validate if the suffix is good enough.

In both solutions we're providing the same sense of security. We're ultimately doing string validation, but we're choosing to stand by an authority that has a maintained reputation and be strict on guidelines. We're providing a base layer of protection for the user. With the implementation now being to check against a static list, it keeps a minimum layer of security. A callback handler can be done outside of setting this URI. Giving the end-user the option of making something up and then believing they'll do exactly the right thing doesn't always work out.

Even we had to do some research to validate the PSL, create the parser, and then do this base level of validation. Placing all those steps on an end-user that wants this same rudimentary layer is not an ideal experience.

2. Each driver has to embed this dat file - which means bigger artifacts (in case of dotnet: nuget package will grow even more: number of TFMs (we have 3 now) * dat file size 3 * 140kb = almost 0.5 MB)

I can't speak to the filesize preferences across teams. Are you saying that the addition of this 140kb file will generate a 0.5MB dat file? I don't think that's a large enough size uptick to warrant concern but I am okay being incorrect in my assumptions here. However the PSL spec does state that if you would prefer to use some other implementation of PSL, you may so this could offset sizing costs whereby a specific language team is beholden to a more contained PSL.

3. We need to update the embeded dat-file time to time. How often? Do we have somebody to inspect the changes on the list and carry over to the specs repo? Google says the list could be changed relatively often [Public Suffix List](https://publicsuffix.org/) on [publicsuffix.org](http://publicsuffix.org/) typically changes a few times per week. Then how often the updated file should get to each Driver? Will we have a Drivers ticket for each update? twice a week? :-) If we do that on every release, minor and patches - it means it could break end-users if some rules will change and the previously OKish value will turn to forbidden. If we do that on major releases only - we will have a very-very different versions of this file in the wild, because drivers has own release cycle and major releases happens not-too-often.

There was a prior PSL technical design document that stated these syncs would occur once a month and it will be an automated sync with no further work on individual language teams needed; only copy the file over. The cadence of matching the document is up to each team and teams are not expected to be in lock-step because we're currently not in lock-step on releases. This is ultimately a best-effort list since it will always be ever-changing.

4. There is no way for user to supply a newer/different version other then bundled inside the driver. Why would they need this? I've looked into the raw file in github repo and they mention several removed lines because of somebody request or because of a bug - what if some of our customer's domain is listed accidentally?

I understand that accidents or mistakes do happen, but the PSL does promote pull requests and non-acceptance guidelines which back that a customer's domain shouldn't get listed unless published by someone directly associated with the space and not simply a third-party user.

[sanych-sun]

@Jibola Thank you for your response!

we can feel pretty good about this option and its blend of simplicity for the end-user whilst also balancing some layer of validation that can let us rest knowing we've done something for this can of worms.

The problem with current solution that it trying to put the very dynamic dataset into the driver without ability to override it at all. Which means if there is any inconsistency or bug in the dataset - users will have no way to workaround it. And also it means we will need to update this embedded resource to catch up with the changes - correct me if I'm wrong, but proposed changes to spec does not define the update process.

About size: in the PR referenced by @adelinowona we are going to include the dat file as embedded resource into the dotnet assembly (which means the file as is will be included into the dll). Then we have a multi-target package because of how dotnet packaging works - currently we have 3 targets - which basically means there will be 3 version of driver packed - so 3 MongoDB.Driver.dll files - which makes 140kB to be present 3 times in the package. But I do agree - this is the smallest problem.

We're providing a base layer of protection for the user. With the implementation now being to check against a static list, it keeps a minimum layer of security.

Yes, we providing very-basic some-kind of security which does not really protect, but creating additional headache on our side.

A callback handler can be done outside of setting this URI. Giving the end-user the option of making something up and then believing they'll do exactly the right thing doesn't always work out.

Yes. This is the point. If users are interested - they will investigate and create as complex solution as they want. They will be in charge. Why do we think they cannot create comprehensive solution when they totally know their infrastructure and having all that info they can implement more strict and more reliable validation?

Again: callback is generic approach, and if we feel like PSL is the greatest thing ever and it will cover 99.9% of all our use-cases - I'm more then OK to implement extension library which will have different release-cycle then driver itself and will be explicitly added by end-users as a form of callback. But such approach will also unlock some corner cases which we do not think of, because end-user will be able to specify their own callback if needed.

[adelinowona]

Just noting that If we add the two-label MUST, this carve-out stays correct and nothing here needs to change. If we don't, this sentence has to go, because the property it relies on no longer holds.

Dismissing review as we've addressed comments on security in an outside thread and continued on completing the pull request.

[aclark4life]

Since the two-label MUST at step 4 rejects any single-label value first cc and COM never reach the public suffix check. So of the three only psl-not-public-suffix still exercises the PSL and only its accept path.

Nothing covers rejecting a public suffix. Repointing one test at a multi-label public suffix like com.ac would restore that and make tld-only unnecessary since .cc and cc are the same test once step 1 strips the dot.

[sleepyStick]

given that the full/"official" use of the PSL is potentially not going to be used, i think this would be really hard to test -- like how do we decide which multi-label public suffix every driver should deny? I feel like that's a conversation that would take more time to decide (and is not in the scope of this ticket)? idk does that make sense?

[aclark4life]

given that the full/"official" use of the PSL is potentially not going to be used, i think this would be really hard to test -- like how do we decide which multi-label public suffix every driver should deny? I feel like that's a conversation that would take more time to decide? idk does that make sense?

My understanding that we need to make those decisions now and leave the implementation to later, @Jibola ? Maybe the callback rejects it? I'm not sure TBH but worst case I suppose we could just remove these psl tests.

[Jibola]

I don't think we should remove the PSL tests. I believe the tests that are there can be kept and that testing the DENY path would require us to pick a deniable PSL that we can say for absolute certainty isn't needed. I don't think that makes as much sense given things in the PSL are allowed to leave.

In the worst case, we can remove just the acceptance path test.

Overall, the should doesn't invalidate our expectations, it just makes it such that if you do not have something within the PSL you're still a valid implementation.

We do plan on including callbacks but not having a PSL whatsoever is currently not in scope.

I push we leave the testing as is and raise it as a point in our next iteration that leverages custom resolvers.

[aclark4life]

LGTM pending resolution of #1950 (comment)!

[aclark4life]

Just noticed this dup line, can be dropped

[sleepyStick]

good catch, fixed in c7c7726

[aclark4life]

Probably should drop "and the public suffix check is the only thing that can change the outcome."

[sleepyStick]

fixed in c7c7726