Skip to content

DRIVERS-3329: Configurable DNS domain validation for SRV records - #1950

Open
sleepyStick wants to merge 21 commits into
mongodb:masterfrom
sleepyStick:DRIVERS-3329
Open

DRIVERS-3329: Configurable DNS domain validation for SRV records#1950
sleepyStick wants to merge 21 commits into
mongodb:masterfrom
sleepyStick:DRIVERS-3329

Conversation

@sleepyStick

@sleepyStick sleepyStick commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Please complete the following before merging:

  • Is the relevant DRIVERS ticket in the PR title?

@rozza rozza left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@sleepyStick
sleepyStick marked this pull request as ready for review June 16, 2026 19:50
@sleepyStick
sleepyStick requested review from a team as code owners June 16, 2026 19:50

#### srvAllowedHostsSuffix

This option is used to validate hosts. If present, its value MUST be treated as the domain for DNS validation. For

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added! thanks!

Comment thread source/initial-dns-seedlist-discovery/initial-dns-seedlist-discovery.md Outdated
Comment thread source/initial-dns-seedlist-discovery/initial-dns-seedlist-discovery.md Outdated
@codeowners-service-app

Copy link
Copy Markdown

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

@Jibola
Jibola requested a review from rozza June 22, 2026 15:37
sleepyStick and others added 5 commits June 22, 2026 11:40
…covery.md

Co-authored-by: Matt Dale <9760375+matthewdale@users.noreply.github.com>
…covery.md

Co-authored-by: Matt Dale <9760375+matthewdale@users.noreply.github.com>
@sleepyStick
sleepyStick requested a review from matthewdale June 22, 2026 19:07
matthewdale
matthewdale previously approved these changes Jun 23, 2026

@matthewdale matthewdale left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! 👍

@@ -0,0 +1,6 @@
{
"uri": "mongodb+srv://test12.test.build.10gen.cc/?srvAllowedHostsSuffix=uild.10gen.cc",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks like the convention in yml is no quotes around uri

This option is used to validate hosts. If present, its value MUST be treated as the `{domainname}` for
[DNS validation](#querying-dns). For example, `srvAllowedHostsSuffix=.mydomain.net`. If the value does not begin with a
`.`, for example, `srvAllowedHostsSuffix=mydomain.net`, the `.` MUST be automatically prepended prior to validation. If
this option is not present, the`{domainname}` MUST be inferred from the `{hostname}` (as described in

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing space

uri: "mongodb+srv://test12.test.build.10gen.cc/?srvAllowedHostsSuffix=test.build.10gen.cc"
seeds: []
hosts: []
error: true No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

aclark4life
aclark4life previously approved these changes Jun 23, 2026

@aclark4life aclark4life left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pending nit fixes LGTM

@sleepyStick
sleepyStick requested review from Jibola and ajcvickers June 23, 2026 20:20
@ajcvickers

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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.

@aclark4life

Copy link
Copy Markdown

question about IDN / punycode / homographs -- even if a user passed in a non-ascii only value for srvAllowedHostsSuffix not pass the check because DNS query would only return ascii values?

I don't know. I'm not an expert on this type of parsing, or on what DNS actually does with non-Ascii characters. Do we have any domain experts in DNS that we can ask?

DNS does not support non-ascii chars e.g.

mongo-python-driver|PYTHON-5867 ⇒ whois °.co.uk | tail -20

status:       ACTIVE
remarks:      Registration information: http://www.nic.uk/

created:      1985-07-24
changed:      2023-03-07
source:       IANA

# whois.nic.uk


    Error for "â°.co.uk".

    This domain cannot be registered because it contravenes the Nominet UK
    naming rules.  The reason is:
      Domain names may only comprise the characters A-Z, a-z, 0-9, hyphen (-)
    and dot (.)..

    WHOIS lookup made at 13:35:57 06-Jul-2026

@sleepyStick

sleepyStick commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

tldr: i don't think the hook is a better idea, the current approach achieves the same thing. but for more details:

We're exploring some additional safeguards with respect to disallowing public suffixes but short of having a decision on that at the moment, I've added that " [srvAllowedHostsSuffix] MUST contain at least two dot-separated labels (i.e. the portion after the leading . must itself contain a .). For example, srvAllowedHostsSuffix=.net MUST raise an error."

This doesn't really help very much. Consider .co.uk.

Agreed that the current one wouldn't help and that more is needed. I believe we're still contemplating the best way to "manage" a public suffix list across the drivers but the goal is to address this concern.

@damieng Suggested a different approach here. Rather than us trying to handle every case safely, we could instead provide a hook so that the customer could handle their specific case. It's much easier to handle very specific cases safely, because you are matching actual specific domains, rather than attempting to match a pattern. We could write the safe implementation ourselves and give it to important customers.

I messaged @damieng this morning after i read your reply (mainly because i didn't understand what the other approach was and was hoping for a clearer explanation) -- it appears that Damien didn't have the full context of the problem when he came up with this suggestion but I'll clarify on what this suggestion is (for anyone else reading) and then after share my thoughts on it for further discussion.
What the suggestion is: the aforementioned hook that would be provided to the customer would be like a function. So for example the code might look like this:

def is_allow_host(...) -> bool:
     # whatever the current validation is

and then cusomers can do the following:

@override
def is_allow_host(host: str, ...) -> bool:
     return host.endswith("<their allow host suffix>"))

notably this idea wouldn't involve adding srvAllowedHostsSuffix as a URI parameter. Thus the inferred host would be the existing logic.
my thoughts: okay so my first thought was, what's stopping the customer from doing "return true" in their overridden is_allow_host method? that would obviously be a silly thing for them to do but its basically the same thing as not adding any form of validation for the existing srvAllowedHostsSuffix, so my evolution of this idea is to basically always do some form of basic validation on the host (which i think would be the same type of validation that we need to perform on srvAllowedHostsSuffix if we were to go the URI approach) and thus the code would look like this:

def is_allow_host_hook(...) -> bool:
     # this is where we'd place "optional" / "recommended" validation that the customer can then override if they decide they know better

def is_allow_host(...) -> bool:
     # whatever validation we deem to be an absolute minimum safeguard that all allowed hosts should satisfy 
     # AND if those pass then
     return is_allow_host_hook(...)

and then customers can do the following:

@override
def is_allow_host_hook(host: str, ...) -> bool:
     return host.endswith("<their allow host suffix>")) # or whatever they want

But at this point, all that absolute minimum validation is the same as the validation that we need to figure out for srvAllowedHostsSuffix + the potential code architecture changes to allow said hook to exist for customers makes me feel like its not really a great alternative idea. Additionally, i feel like a URI option is more likely to be visible in logs making it easier to debug whereas this hook would be harder to, should a mistake be made somewhere?
Additionally, I presently don't know what I'd want to go into the is_allow_host_hook (from my evolution of the idea) -- this isn't to say we wouldn't think of something in the future? -- but if this function remains a no-op forever (aka return True) then it doesn't address the root cause of the entire project since it'd only allow customers to add more restrictive requirements to the host and not make it less restrictive?
Now, Damien did point out that this override would allow customers to more easily have different allow hosts depending on their various environments / configs but I'm personally not convinced how useful they'll be given the, what i believe would be, increased complexity of the code change? (I could probably be convinced otherwise tho? sometimes i'm pretty easy to convince lol)
But if I'm misunderstanding the suggestion, or otherwise failing to see a pro of this suggestion, please do let me know! (I am but a lowly code monkey after all haha)

@damieng

damieng commented Jul 8, 2026

Copy link
Copy Markdown

what's stopping the customer from doing "return true" i

Nothing, but they would have explicitly chosen to have relaxed that security. It gives them the ultimate flexibility and power but only if they clearly opt into it.

The problem with doing things like srvAllowedHostsSuffix is it's very limited. The moment a customer wants to say - use two hosts under different suffixes perhaps on different cloud providers they're out of luck again. With a hook they can either hard-code it, read a comma-separated list in from their own configuration etc.

If the user wants to also perform our existing validation that's easy enough to do by them calling down to the base method/hook either before adding their own validation or after having allowed through their exception.

@adelinowona adelinowona left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewing as the secondary implementer (C# driver). I've read this PR alongside #1972 and also looked at the Pymongo implementation. Most of what's here looks good but I have just a few comments.

Raising this here:
We could use an uppercase public suffix test. Both PSL tests are lowercase (cc, 10gen.cc), so nothing covers srvAllowedHostsSuffix=COM. See my inline comment on the normalization bullets for why that's the one ordering bug the current suite can't catch. Could live here or with the srvAllowedHostsSuffix-psl-* tests in #1972 — your call which.


This option is used to validate hosts. If present, its value MUST be treated as the `{domainname}` for
[DNS validation](#querying-dns) and
[SRV polling](../polling-srv-records-for-mongos-discovery/polling-srv-records-for-mongos-discovery.md). For example,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The option description now says it applies to SRV polling but that spec document isn't touched by this PR and still carries its own normative sentence: "A driver MUST verify that the host names returned through SRV records have the same parent {domainname}."

More subtly, the two documents now use {domainname} to mean different things — in the polling spec it's the {hostname}.{domainname} split; here it's the entire configured suffix when the option is set. WE should update SRV polling spec accordingly as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oo thanks for pointing that out! fixed in d2731ce

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's no security-considerations text for this new option. The Design Rationale section is a single pre-existing sentence and doesn't mention srvAllowedHostsSuffix. I think we could benefit from a subsection covering: why the option exists; that the PSL bounds the blast radius but doesn't make a configuration safe; guidance to use the narrowest suffix that work. PyMongo already has good material for most of this in its MongoClient docstring warning. Also helps future spec readers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added in d2731ce
honestly i feel a bit off about my wording though? so lmk if you have any suggestions on the wording here!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the section reads well! I wouldn't change anything. One thing I would suggest adding though is info around the alternative @damieng raised - the driver provided hook. Something like

Why not a validation callback?
An alternative considered was a driver-provided hook....bla.....bla.... bla

Comment thread source/initial-dns-seedlist-discovery/initial-dns-seedlist-discovery.md Outdated
Comment on lines +1 to +5
# srvAllowedHostsSuffix must contain at least two labels. A value of .cc (TLD only)
# has only one label after the leading dot and MUST raise an error.
# Note: .cc is used (rather than an unrelated TLD) because the DNS records return
# hosts ending in .cc, ensuring the test fails only due to TLD validation and not
# a host mismatch.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this rationale is now stale — it describes the two-label minimum, which this PR replaced with the PSL rule. The test still passes, but for a different reason now (cc is an ordinary PSL rule).

@sleepyStick Looking at the most recent PyMongo implementation, it still enforces the deleted rule ("srvAllowedHostsSuffix must contain at least two labels"), and that check runs before is_public_suffix — so PyMongo passes this test without the PSL path ever executing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed the test description and thank you for calling out the pymongo change!! Past Iris thought keeping it would be fine because if it only had one label then it must be a public suffix (and this felt like a "shortcut" for the code to conclude that it was a public suffix, if that makes any sense) but I had forgotten that pymongo gives out a unique error message in that case. Removed it now, thanks for calling it out!

Comment on lines 150 to 152
A driver MUST verify that the host names returned through SRV records share the original SRV's `{domainname}`. In
addition, SRV records with fewer than three `.` separated parts, the returned hostname MUST have at least one more
domain level than the SRV record hostname. Drivers MUST raise an error and MUST NOT initiate a connection to any

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this part "In addition, SRV records with fewer than three . separated parts…" doesn't apply when srvAllowedHostsSuffix is set. The clause needs {hostname} to have ≤2 labels, the suffix must have ≥2 labels (a 1-label suffix is already a public suffix under the * fallback), and the mandatory leading dot forces returned hosts to ≥3 labels. So the "at least one more domain level" property holds by construction. I suggest updating this part of the spec.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch! done in d2731ce

Comment on lines +40 to +41
For the purposes of this document, `{hostname}` will be divided using the following terminology. If
`srvAllowedHostsSuffix` has been configured, then that will act as the `{domainname}`. Otherwise, if an SRV `{hostname}`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question for the rationale rather than a change request: nothing requires {hostname} itself to end with the suffix, so mongodb+srv://cluster.example.org/?srvAllowedHostsSuffix=bank.com is legal.

I initially thought that should be forbidden, but I don't think it should — it would break the vanity-CNAME shape, where the SRV name is a corporate alias and the hosts live in a provider domain, which seems like one of the cases this option might be used for. So I'd just like the rationale to say that decoupling is deliberate and why, since it's the point where the trust anchor stops being tied to the name the user typed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

that is correct, I've added this to the rationale in d2731ce

sleepyStick and others added 2 commits August 24, 2026 17:50
…covery.md

Co-authored-by: Adelin Owona <51498470+adelinowona@users.noreply.github.com>
@sleepyStick

sleepyStick commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

We could use an uppercase public suffix test. Both PSL tests are lowercase (cc, 10gen.cc), so nothing covers srvAllowedHostsSuffix=COM. See my inline comment on the normalization bullets for why that's the one ordering bug the current suite can't catch. Could live here or with the srvAllowedHostsSuffix-psl-* tests in #1972 — your call which.

I've added them in #1972!

(and the failing lint check is because I'm linking to the currently non-existent public suffix spec (I assume this PR would be merged after #1972))

Comment thread source/initial-dns-seedlist-discovery/initial-dns-seedlist-discovery.md Outdated
Comment on lines +149 to +152
Before validating returned hostnames, drivers MUST normalize them as follows:

- Trailing dots MUST be stripped (e.g. `host.example.com.` becomes `host.example.com`).
- Hostnames MUST be normalized to lowercase using ASCII case folding.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Because this addition sits in a PR titled after the new option, it could be read as a detail of that option — I think it earns its own entry in this document's ChangeLog so other drivers audit their comparison instead of skipping it:

  • 2026-08-24: Specify that host names returned through SRV records are normalized -- trailing dot stripped, ASCII lowercase -- before validation.

Also, nothing in the test suite can catch a driver that skips the fold — every build.10gen.cc record returns lowercase targets, so it's unobservable, and srvAllowedHostsSuffix-case-insensitive covers the suffix side only. I think we need a fixture with a mixed-case SRV target, which is test-infra work but worth doing in this PR. Flagging it because a driver can be compliant on the seedlist path and not on the polling path.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

makes sense, added changelog suggestion

oh that's a good catch -- filed a devprod ticket (DEVPROD-42090) I hope that's the right thing to do? (and hopefully it doesn't take long for them to get back to us?)

sleepyStick and others added 2 commits August 25, 2026 16:05
…covery.md

Co-authored-by: Adelin Owona <51498470+adelinowona@users.noreply.github.com>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should add a test similar to this one, but using only a period as an allowed hosts suffix. The test should expect an error.

For example:

uri: "mongodb+srv://test1.test.build.10gen.cc/?srvAllowedHostsSuffix=."
seeds: []
hosts: []
error: true

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good idea! done in 0521b1b

@matthewdale matthewdale left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! 👍

@adelinowona adelinowona left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!
C# implementation here: mongodb/mongo-csharp-driver#2109

@sanych-sun

sanych-sun commented Sep 1, 2026

Copy link
Copy Markdown
Member

I'm seconding the hook idea suggested by @damieng instead of hardcoded list of suffixes. If customer is worried about the security - they will have a way to express any possible rules they want, and they will have to do that in code - which means no way attackers can inject anything into the connection string to make driver connect to a wrong host. If customer does not really care about security - yes, they can have return true as the hook. More over if we really want to use public suffixes list - it could be implemented as the hook via extension library.

And yes, I see this file as hardcoded list of magic string. Even though the current approach most likely will address the immediate needs there are number of problems which we will have to cover:

  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.
  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)
  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.
  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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants