PYTHON-5814 Configurable DNS domain validation for SRV records#2903
Conversation
sleepyStick
changed the title
| # - test/versioned-api/crud-api-version-1-strict.json:514: nin ==> inn, min, bin, nine | ||
| # - test/test_client.py:188: te ==> the, be, we, to | ||
| args: ["-L", "fle,fo,infinit,isnt,nin,te,aks"] | ||
| exclude: ^pymongo/public_suffix_list\.dat$ |
There was a problem hiding this comment.
psl is full of mostly not words,,, checking it raises lots of "typos" but aren't actually typos lol
Jibola
left a comment
Jibola
left a comment
There was a problem hiding this comment.
Have a few comments, but things otherwise look good.
| @@ -0,0 +1,6 @@ | |||
| { | |||
| "uri": "mongodb+srv://test12.test.build.10gen.cc/?srvAllowedHostsSuffix=test.build.10gen.cc", | |||
There was a problem hiding this comment.
May be a dumb question; what's the mismatch here?
There was a problem hiding this comment.
the comment on the equivalent yml file would say "# DNS record for test12.test.build.10gen.cc returns localhost.build.10gen.cc which would not match test.build.10gen.cc" (obv that's in review on the drivers PR but yeah)
| from pathlib import Path | ||
|
|
||
|
|
||
| def _load_public_suffixes() -> tuple[set[str], set[str], set[str]]: |
There was a problem hiding this comment.
It could be a good idea to do a simple cache this information so that it doesn't do a full file read each time we check for a public suffix.
| srv_allowed_hosts_suffix: Optional[str] = None, | ||
| ): | ||
| self.__fqdn = fqdn | ||
| self.__fqdn = fqdn.lower() |
There was a problem hiding this comment.
Is this lower call necessary? I don't see anywhere that mutates or calls self.__fqdn that has been changed by the newly introduced logic.
There was a problem hiding this comment.
errr not necessary, i think my bot(s) noticed it an existing bug -- because I added a test case about case insensitivity for the new parameter i'm introducing -- for example if a user passed in a URI like "mongodb+srv://test1.TEST.BUILD.10GEN.CC/" instead of "mongodb+srv://test1.test.build.10gen.cc/" the current logic (without the lower) would have this not connect. (i can undo the change if we wanted to tackle this problem separately or add a test to solidify it if we're okay with doing it here -- sorry it slipped my mind to explain it when i was putting up the PR)
| - `srvAllowedHostsSuffix`: (string) Overrides the default requirement that | ||
| hosts returned by SRV DNS records share the same parent domain as the seed | ||
| hostname. When set, the driver accepts any returned host whose name ends | ||
| with this suffix (e.g. ``".atlas.mongodb.com"``). The value must contain | ||
| at least two labels and must not be a public suffix (per the Public Suffix | ||
| List). Only valid with ``mongodb+srv://`` URIs. | ||
|
|
||
| .. warning:: | ||
|
|
||
| This option relaxes a built-in DNS spoofing safeguard. Use the most | ||
| specific suffix possible for your deployment rather than a broad | ||
| company-wide domain. |
There was a problem hiding this comment.
Can you add an example of a good configuration just so the users can have an explicit understanding of what to do?
2 participants
PYTHON-5814
Changes in this PR
THIS IS THE SAME PR AS #2868 BUT THIS ONE MERGES TO A BRANCH OF MAIN AND NOT MAIN ITSELF!! THE OTHER PR IS FOR MAIN AND WILL BE ADDRESSED LATER
srvAllowedHostsSuffixparamTest Plan
Checklist
Checklist for Author
Checklist for Reviewer