Strip trailing dot from FQDNs in Host and TLS by martin-sucha · Pull Request #7364 · aio-libs/aiohttp

martin-sucha · 2023-07-12T13:33:53Z

The TLS verification fails with an exception if the client uses a fully-qualified domain name with a trailing dot, like https://github.com./ :

aiohttp.client_exceptions.ClientConnectorCertificateError: Cannot connect to host github.com.:443 ssl:True
[SSLCertVerificationError: (1, "[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: Hostname mismatch,
certificate is not valid for 'github.com.'. (_ssl.c:1051)")]

The reason is that TLS certificates do not contain the trailing dot, as per RFC 6066:

"HostName" contains the fully qualified DNS hostname of the server,
as understood by the client. The hostname is represented as a byte
string using ASCII encoding without a trailing dot.

We need to strip the trailing dot for TLS context and Host header, where trailing dots are not present.
For DNS resolution, we need to include the trailing dot as it signifies a fully-qualified domain name (FQDN).
DNS lookups of FQDNs are faster as the resolver does not need to check DNS search path, like for relative DNS names.

Closes #3636

What do these changes do?

Strip trailing dot from hostname in Host header and TLS context.

Are there changes in behavior for the user?

Allows client to connect to server if URL has dot at the end of the hostname: https://example.com./

Related issue number

#3636

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

The TLS verification fails with an exception if the client uses a fully-qualified domain name with a trailing dot, like https://github.com./ : aiohttp.client_exceptions.ClientConnectorCertificateError: Cannot connect to host github.com.:443 ssl:True [SSLCertVerificationError: (1, "[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: Hostname mismatch, certificate is not valid for 'github.com.'. (_ssl.c:1051)")] The reason is that TLS certificates do not contain the trailing dot, as per RFC 6066: "HostName" contains the fully qualified DNS hostname of the server, as understood by the client. The hostname is represented as a byte string using ASCII encoding without a trailing dot. We need to strip the trailing dot for TLS context and Host header, where trailing dots are not present. For DNS resolution, we need to include the trailing dot as it signifies a fully-qualified domain name (FQDN). DNS lookups of FQDNs are faster as the resolver does not need to check DNS search path, like for relative DNS names. Closes aio-libs#3636

martin-sucha · 2023-07-12T13:36:01Z

I tried looking into adding a test for this but it was not clear to me if there is some test I could easily modify to check. Please advise what is the best way to write a test for this.

The colon was a typo.

Dreamsorcerer · 2023-07-12T17:05:55Z

I tried looking into adding a test for this but it was not clear to me if there is some test I could easily modify to check. Please advise what is the best way to write a test for this.

I'd assume tests in test_client_request.py and test_connector.py could be added that atleast asserts that something is called with the truncated domain?

codecov · 2023-07-12T17:22:29Z

Codecov Report

Merging #7364 (b774736) into master (ac29dea) will decrease coverage by 0.01%.
Report is 12 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7364      +/-   ##
==========================================
- Coverage   97.35%   97.35%   -0.01%     
==========================================
  Files         106      106              
  Lines       31490    31513      +23     
  Branches     3577     3589      +12     
==========================================
+ Hits        30657    30679      +22     
  Misses        630      630              
- Partials      203      204       +1

Flag	Coverage Δ
CI-GHA	`97.28% <100.00%> (-0.03%)`	⬇️
OS-Linux	`96.95% <100.00%> (-0.03%)`	⬇️
OS-Windows	`95.41% <100.00%> (-0.03%)`	⬇️
OS-macOS	`96.60% <100.00%> (-0.06%)`	⬇️
Py-3.10.11	`95.34% <100.00%> (-0.02%)`	⬇️
Py-3.10.12	`?`
Py-3.10.13	`96.81% <100.00%> (?)`
Py-3.11.4	`?`
Py-3.11.5	`96.52% <100.00%> (?)`
Py-3.8.10	`95.30% <100.00%> (-0.03%)`	⬇️
Py-3.8.17	`?`
Py-3.8.18	`96.74% <100.00%> (?)`
Py-3.9.13	`95.29% <100.00%> (-0.03%)`	⬇️
Py-3.9.17	`?`
Py-3.9.18	`96.77% <100.00%> (?)`
Py-pypy7.3.11	`96.31% <100.00%> (-0.05%)`	⬇️
VM-macos	`96.60% <100.00%> (-0.06%)`	⬇️
VM-ubuntu	`96.95% <100.00%> (-0.03%)`	⬇️
VM-windows	`95.41% <100.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
tests/conftest.py	`92.17% <ø> (ø)`
aiohttp/client_reqrep.py	`97.57% <100.00%> (-0.18%)`	⬇️
aiohttp/connector.py	`94.23% <100.00%> (+0.01%)`	⬆️
tests/test_client_request.py	`99.58% <100.00%> (+<0.01%)`	⬆️
tests/test_connector.py	`97.90% <100.00%> (+<0.01%)`	⬆️

... and 10 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

CHANGES/3636.bugfix

aiohttp/client_reqrep.py

aiohttp/connector.py

webknjaz · 2023-07-13T01:20:27Z

I tried looking into adding a test for this but it was not clear to me if there is some test I could easily modify to check. Please advise what is the best way to write a test for this.

You'll probably want to use these fixtures https://github.com/aio-libs/aiohttp/blob/7911f1e/tests/conftest.py#L50-L61 to test that this patch works well with TLS.

Co-authored-by: Sviatoslav Sydorenko <[email protected]>

martin-sucha · 2023-07-13T08:49:52Z

Thank you for the review. I've applied the suggestions and added tests.

webknjaz · 2023-07-19T14:15:01Z

@martin-sucha hey, it looks like some linting checks are failing. Could you fix those so we could proceed?

aiohttp/client_reqrep.py

tests/test_client_request.py

tests/test_connector.py

Co-authored-by: Sviatoslav Sydorenko <[email protected]>

aio-libs#7364 (comment)

tests/test_connector.py

aiohttp/connector.py

Co-authored-by: Sviatoslav Sydorenko <[email protected]>

aiohttp/connector.py

Co-authored-by: Sviatoslav Sydorenko <[email protected]>

Dreamsorcerer · 2023-08-24T17:31:02Z

@martin-sucha There's a black formatting error (I'd update it myself, but you've disabled maintainer access to your branch).

martin-sucha · 2023-08-25T13:42:54Z

There's a black formatting error (I'd update it myself, but you've disabled maintainer access to your branch).

Oops. Fixed.

tests/test_client_request.py

webknjaz

Love it! Thanks for working on this! I think it's ready now.

webknjaz · 2023-09-07T23:09:09Z

@martin-sucha looks like there are some legitimate failures in the CI, could you take a look?

On macOS the real resolver returns NXDOMAIN for something.localhost, so we need to use a mock resolver to ensure that the loopback IP is always used.

martin-sucha · 2023-09-11T06:50:15Z

@webknjaz Seems that on macOS the resolver won't resolve something.localhost. I've mocked _resolve_host to always return the loopback address. I think that should be okay since the test is checking TLS.

webknjaz · 2023-09-11T13:30:10Z

@martin-sucha thank you! I've enabled auto-merge and there's a backporting bot label. If it fails to produce a backport PR, please, follow the instructions to do this manually.

webknjaz · 2023-09-11T14:19:48Z

RTD build was stuck for some reason. I restarted it so GH could perform merging.

patchback · 2023-09-11T14:20:36Z

Backport to 3.9: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply d84fcf7 on top of patchback/backports/3.9/d84fcf7c52e7f44b81aae24dc2a7ad2bccc76c9b/pr-7364

Backporting merged PR #7364 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.9/d84fcf7c52e7f44b81aae24dc2a7ad2bccc76c9b/pr-7364 upstream/3.9

Now, cherry-pick PR Strip trailing dot from FQDNs in Host and TLS #7364 contents into that branch:
```
$ git cherry-pick -x d84fcf7c52e7f44b81aae24dc2a7ad2bccc76c9b
```
If it'll yell at you with something like fatal: Commit d84fcf7c52e7f44b81aae24dc2a7ad2bccc76c9b is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x d84fcf7c52e7f44b81aae24dc2a7ad2bccc76c9b
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Strip trailing dot from FQDNs in Host and TLS #7364 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.9/d84fcf7c52e7f44b81aae24dc2a7ad2bccc76c9b/pr-7364

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

Before this patch, the TLS verification fails with an exception if the client uses a fully-qualified domain name with a trailing dot, like https://github.com./ : ```console aiohttp.client_exceptions.ClientConnectorCertificateError: Cannot connect to host github.com.:443 ssl:True [SSLCertVerificationError: (1, "[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: Hostname mismatch, certificate is not valid for 'github.com.'. (_ssl.c:1051)")] ``` The reason is that TLS certificates do not contain the trailing dot, as per RFC 6066: "HostName" contains the fully qualified DNS hostname of the server, as understood by the client. The hostname is represented as a byte string using ASCII encoding without a trailing dot. This change makes aiohttp strip the trailing dot for TLS context and Host header, where trailing dots are not present. For DNS resolution, we include the trailing dot as it signifies a fully-qualified domain name (FQDN). DNS lookups of FQDNs are faster as the resolver does not need to check DNS search path, like for relative DNS names. This effectively allows clients to connect to server if URL has dot at the end of the hostname, e.g. `https://example.com./. Fixes aio-libs#3636 PR aio-libs#7364 Co-authored-by: Sviatoslav Sydorenko <[email protected]> (cherry picked from commit d84fcf7)

martin-sucha · 2023-09-12T08:15:42Z

@webknjaz I opened #7601 for the backport.

Before this patch, the TLS verification fails with an exception if the client uses a fully-qualified domain name with a trailing dot, like https://github.com./ : ```console aiohttp.client_exceptions.ClientConnectorCertificateError: Cannot connect to host github.com.:443 ssl:True [SSLCertVerificationError: (1, "[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: Hostname mismatch, certificate is not valid for 'github.com.'. (_ssl.c:1051)")] ``` The reason is that TLS certificates do not contain the trailing dot, as per RFC 6066: "HostName" contains the fully qualified DNS hostname of the server, as understood by the client. The hostname is represented as a byte string using ASCII encoding without a trailing dot. This change makes aiohttp strip the trailing dot for TLS context and Host header, where trailing dots are not present. For DNS resolution, we include the trailing dot as it signifies a fully-qualified domain name (FQDN). DNS lookups of FQDNs are faster as the resolver does not need to check DNS search path, like for relative DNS names. This effectively allows clients to connect to server if URL has dot at the end of the hostname, e.g. `https://example.com./. Fixes #3636 PR #7364 Co-authored-by: Sviatoslav Sydorenko <[email protected]> (cherry picked from commit d84fcf7)  ## What do these changes do? Backport #7364 into 3.9  ## Are there changes in behavior for the user?  ## Related issue number  ## Checklist - [ ] I think the code is well written - [ ] Unit tests for the changes exist - [ ] Documentation reflects the changes - [ ] If you provide code modification, please add yourself to `CONTRIBUTORS.txt` * The format is <Name> <Surname>. * Please keep alphabetical order, the file is sorted by names. - [ ] Add a new news fragment into the `CHANGES` folder * name it `<issue_id>.<type>` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

martin-sucha requested review from asvetlov and webknjaz as code owners July 12, 2023 13:33

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jul 12, 2023

Strip dot, not colon

4df573b

The colon was a typo.