PYTHON-5814 Configurable DNS domain validation for SRV records - #2868
PYTHON-5814 Configurable DNS domain validation for SRV records#2868sleepyStick wants to merge 26 commits into
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…was only here for the initial delivery of the project, removing now
…ver into PYTHON-5814
NoahStapp
left a comment
There was a problem hiding this comment.
Looks good, just some documentation/clarity questions!
| srv|SRV|initial-dns-seedlist-discovery|srv_seedlist) | ||
| cpjson initial-dns-seedlist-discovery/tests/ srv_seedlist | ||
| # srvAllowedHostsSuffix validation uses the bundled Public Suffix List. | ||
| cp_psl |
There was a problem hiding this comment.
So this will resync every time there is a change to the Public Suffix List?
There was a problem hiding this comment.
it syncs from the spec's copy of the PSL -- which is automatically updated once a month but could be manually triggered (in the specs repo)
There was a problem hiding this comment.
(the actual PSL has like an avg of 3 changes per week which is far too much noise to always keep up to date)
There was a problem hiding this comment.
And our automated weekly sync will pick up that monthly change in the spec repo?
|
|
||
| PyMongo 4.18 brings a number of changes including: | ||
|
|
||
| - Added the ``srvAllowedHostsSuffix`` URI option and :class:`~pymongo.mongo_client.MongoClient` |
There was a problem hiding this comment.
We also need an AsyncMongoClient reference here and below.
| keyword argument. When connecting via ``mongodb+srv://``, this option overrides the default | ||
| requirement that SRV-returned hosts share the same parent domain as the seed hostname, | ||
| allowing hosts under a different domain suffix to be accepted. The suffix must not be a | ||
| public suffix (per the Public Suffix List). See the |
There was a problem hiding this comment.
Should the Public Suffix List be linked here?
There was a problem hiding this comment.
lol that'd be a nice thing to do, wouldn't it! (I initially was like ppl can look it up if they need it, they know what to google now LOLOL) done!
| requirement that SRV-returned hosts share the same parent domain as the seed hostname, | ||
| allowing hosts under a different domain suffix to be accepted. The suffix must not be a | ||
| public suffix (per the Public Suffix List). See the | ||
| :class:`~pymongo.mongo_client.MongoClient` documentation for security considerations. |
There was a problem hiding this comment.
Is there a blog post or non-API doc link we can add here for an example?
There was a problem hiding this comment.
not at the moment, will reach out to docs team about this!
| if domain in suffixes: | ||
| return True | ||
| parts = domain.split(".") | ||
| return len(parts) == 1 or (len(parts) > 1 and ".".join(parts[1:]) in wildcards) |
There was a problem hiding this comment.
Can you add a short comment explaining this line? I'm not following how this results in a suffix being public or not.
There was a problem hiding this comment.
i assume "this line" is the return line, and if so, done! lmk if the comment isn't clear tho
There was a problem hiding this comment.
Pull request overview
This PR adds a new SRV-only configuration option, srvAllowedHostsSuffix, to make DNS domain validation for mongodb+srv:// connections configurable while still preventing overly-broad suffixes via Public Suffix List (PSL) checks.
Changes:
- Adds
srvAllowedHostsSuffixto URI parsing and to the (Async)MongoClient keyword/URI option plumbing through to SRV resolution. - Introduces a bundled PSL parser (
pymongo/_psl.py) and uses it to rejectsrvAllowedHostsSuffixvalues that are public suffixes. - Adds SRV seedlist spec test fixtures and a connection-string invalid-URI case covering SRV-only enforcement.
Reviewed changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_dns.py | Adds PSL parsing unit tests (sync suite). |
| test/asynchronous/test_dns.py | Adds PSL parsing unit tests (async suite). |
| test/srv_seedlist/replica-set/srvAllowedHostsSuffix-without_dot_pass.json | SRV seedlist fixture validating suffix matching without a leading dot. |
| test/srv_seedlist/replica-set/srvAllowedHostsSuffix-without_dot_fail.json | SRV seedlist fixture validating mismatch failure without a leading dot. |
| test/srv_seedlist/replica-set/srvAllowedHostsSuffix-with_dot.json | SRV seedlist fixture validating suffix matching with a leading dot. |
| test/srv_seedlist/replica-set/srvAllowedHostsSuffix-trailing-dot.json | SRV seedlist fixture validating suffix normalization with trailing dot. |
| test/srv_seedlist/replica-set/srvAllowedHostsSuffix-tld-only.json | SRV seedlist fixture rejecting TLD-only suffix. |
| test/srv_seedlist/replica-set/srvAllowedHostsSuffix-psl-public-suffix.json | SRV seedlist fixture rejecting a PSL public suffix. |
| test/srv_seedlist/replica-set/srvAllowedHostsSuffix-psl-public-suffix-capitalized.json | SRV seedlist fixture rejecting a capitalized PSL public suffix. |
| test/srv_seedlist/replica-set/srvAllowedHostsSuffix-psl-not-public-suffix.json | SRV seedlist fixture accepting a non-public suffix. |
| test/srv_seedlist/replica-set/srvAllowedHostsSuffix-period-only.json | SRV seedlist fixture rejecting a period-only suffix. |
| test/srv_seedlist/replica-set/srvAllowedHostsSuffix-mismatch.json | SRV seedlist fixture rejecting a suffix that doesn’t match returned hosts. |
| test/srv_seedlist/replica-set/srvAllowedHostsSuffix-case-insensitive.json | SRV seedlist fixture validating case-insensitive suffix matching. |
| test/connection_string/test/invalid-uris.json | Adds invalid case for srvAllowedHostsSuffix used with non-SRV URIs. |
| pymongo/uri_parser_shared.py | Allows the new option in URI option validation and enforces SRV-only usage. |
| pymongo/common.py | Adds option validator entry for srvallowedhostssuffix. |
| pymongo/asynchronous/uri_parser.py | Plumbs srvAllowedHostsSuffix into async SRV parsing and resolver construction. |
| pymongo/asynchronous/srv_resolver.py | Implements suffix-based SRV host validation + PSL restriction (async). |
| pymongo/asynchronous/settings.py | Stores/exposes srv_allowed_hosts_suffix in async topology settings. |
| pymongo/asynchronous/monitor.py | Passes srv_allowed_hosts_suffix into SRV resolution in the async monitor. |
| pymongo/asynchronous/mongo_client.py | Adds keyword/docs + wires option into TopologySettings construction (async). |
| pymongo/synchronous/uri_parser.py | Plumbs srvAllowedHostsSuffix into sync SRV parsing and resolver construction. |
| pymongo/synchronous/srv_resolver.py | Implements suffix-based SRV host validation + PSL restriction (sync). |
| pymongo/synchronous/settings.py | Stores/exposes srv_allowed_hosts_suffix in sync topology settings. |
| pymongo/synchronous/monitor.py | Passes srv_allowed_hosts_suffix into SRV resolution in the sync monitor. |
| pymongo/synchronous/mongo_client.py | Adds keyword/docs + wires option into TopologySettings construction (sync). |
| pymongo/_psl.py | Adds PSL loading/parsing utility used to validate srvAllowedHostsSuffix. |
| doc/changelog.rst | Documents the new option and its security implications. |
| .pre-commit-config.yaml | Excludes the PSL data file from codespell. |
| .evergreen/resync-specs.sh | Adds logic to pull PSL data from the specs repo during resync. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return suffixes, wildcards, exceptions | ||
|
|
||
|
|
||
| def is_public_suffix(domain: str) -> bool: | ||
| """Return True if domain is a public suffix per the bundled Public Suffix List.""" | ||
| global _PUBLIC_SUFFIXES # noqa: PLW0603 | ||
| if _PUBLIC_SUFFIXES is None: | ||
| _PUBLIC_SUFFIXES = _load_public_suffixes() | ||
| suffixes, wildcards, exceptions = _PUBLIC_SUFFIXES |
There was a problem hiding this comment.
on the other hand, it feels silly to load the list if the user might not use the list,,?
idk i can be convinced either way,, no strong opinions on this,,
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
PYTHON-5814
Changes in this PR
srvAllowedHostsSuffixparamTest Plan
Checklist
Checklist for Author
Checklist for Reviewer