Use FQDN only for DNS and drop trailing dot for other operations #1255

haikuginger · 2017-09-02T00:04:26Z

haikuginger · 2017-09-02T00:20:42Z

Looks like the new version of Tox doesn't like our config file. Looking into it; easy fix would be to pin to 2.7.0 for now.

Lukasa · 2017-09-02T17:33:59Z

Cool, let's see how this goes.

codecov-io · 2017-09-02T17:56:14Z

Codecov Report

Merging #1255 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1255   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          21      21           
  Lines        1985    1989    +4     
======================================
+ Hits         1985    1989    +4

Impacted Files	Coverage Δ
urllib3/connection.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 945f2a6...68f3475. Read the comment docs.

Lukasa

Cool, generally looks good! Some thoughts inline.

Lukasa · 2017-09-02T17:58:59Z

appveyor.yml

@@ -57,7 +57,7 @@ install:
  # Upgrade to the latest version of pip to avoid it displaying warnings
  # about it being out of date.
  - pip install --disable-pip-version-check --user --upgrade pip wheel
-  - pip install tox virtualenv
+  - pip install tox==2.1.1 virtualenv


It would be nice if we could work out why tox is mad at us so we can remove these pins. Any idea?

Lukasa · 2017-09-02T18:02:00Z

test/with_dummyserver/test_socketlevel.py

+                (key, value) = header.split(b': ')
+                if key.decode('ascii').startswith(u'Host'):
+                    received_host.append(value.decode('ascii'))
+


Might be nice to pull this header parsing logic out into a separate helper function. It's the kind of thing that might come up from time to time.

Makes sense! Something like this?

def assert_header_received(self, received_headers, header_name, expected_value=None): """ Asserts that a header with the given name was received; if a value is passed to `expected_value`, asserts that the header has that value. """ pass

Yeah, sounds good to me!

Lukasa · 2017-09-02T18:04:07Z

urllib3/connection.py

+        Setter for the `host` property.
+        """
+        self._dns_host = value
+


Hrm, we might want to put a note here that points out that this assumes that only urllib3 code ever sets this property.

More generally, I wonder if we should just remove the setter and change the __init__ to do the right thing. What are your thoughts on that?

Makes sense to me.

I would tend towards not changing __init__; due to the way it's structured in httplib, we'd essentially have to copy out a chunk of stdlib code (not a huge chunk, sure, but a chunk) and make one small tweak; this is a relatively small change in comparison.

Fair enough. 😄

sigmavirus24 · 2017-09-03T16:24:24Z

Tox looks to be broken on parsing setenv settings in tox.ini my suspicion would be in trying to parse https://github.com/shazow/urllib3/blob/5206734fda1f2365afc49a070efa46c9ba68372e/tox.ini#L38 since it's expecting that it can split on an =.

Lukasa · 2017-09-03T19:53:48Z

Did that stop being valid syntax?

sigmavirus24 · 2017-09-04T11:49:24Z

I don't think so, I suspect it's a regression. They've had other regressions in the past involving references to other sections like this

haikuginger · 2017-09-04T13:01:44Z

Looks like this is fixed now, so I'll remove the pins and check again.

haikuginger · 2017-09-04T13:16:57Z

Okay, I think the single Appveyor fail is spurious, but I don't see a way to restart the build without pushing another commit, so that can wait until @Lukasa okays the test method I suggested. The GAE failure on Travis is pretty consistent though; @jonparrott, any thoughts on that as the resident GAE expert?

haikuginger · 2017-09-08T12:35:20Z

Rebased and squashed from 62b9e1c.

haikuginger · 2017-09-08T15:56:33Z

@Lukasa, it looks like Travis is currently experiencing issues with their macOS builds; if you want to give a runthrough and a final 👍, I can merge once those builds complete.

haikuginger · 2017-09-11T02:04:23Z

Rebased from 88bcf98.

haikuginger · 2017-09-14T17:02:35Z

Gentle ping to @Lukasa; please ignore if you're taking a break from OSS.

@sigmavirus24, can you provide a review in case @Lukasa is unavailable?

Lukasa

One tiny note and then we're good to go.

Lukasa · 2017-09-15T19:19:15Z

dummyserver/testcase.py

+            received_headers,
+            header_name,
+            expected_value=None
+    ):


I'm not wild about this indentation. 😉 Mind matching it up with the rest of the project?

Sure! Another project I'm involved with had linting set up to whine if the parameters line up with the first line of the function, and that's why I did it here, but I agree it looks skeevy.

haikuginger · 2017-10-13T13:16:51Z

@Lukasa, finally addressed that note.

Lukasa · 2017-10-14T02:24:58Z

Looks like we need to add some pins to some of our dependencies for 2.6.

haikuginger · 2017-12-12T13:34:54Z

Rebased from 36bf9cc to pick up test changes.

haikuginger · 2017-12-12T14:16:34Z

@Lukasa, can you do a final run-through on this? I believe it should be set to merge once we have a green light from Travis (test issues were resolved elsewhere).

Lukasa

LGTM, ship it

…tificate Use FQDN only for DNS and drop trailing dot for other operations

haikuginger requested a review from Lukasa September 2, 2017 00:04

Lukasa requested changes Sep 2, 2017

View reviewed changes

haikuginger force-pushed the haikuginger/fqdn-certificate branch from 62b9e1c to 88bcf98 Compare September 8, 2017 12:35

haikuginger force-pushed the haikuginger/fqdn-certificate branch from 88bcf98 to 6fa796a Compare September 11, 2017 02:04

Lukasa requested changes Sep 15, 2017

View reviewed changes

haikuginger force-pushed the haikuginger/fqdn-certificate branch 2 times, most recently from 12d98c4 to 36bf9cc Compare October 13, 2017 13:16

haikuginger force-pushed the haikuginger/fqdn-certificate branch from 36bf9cc to 050a5f6 Compare December 12, 2017 13:34

Lukasa approved these changes Dec 13, 2017

View reviewed changes

Use FQDN only for DNS and drop trailing dot for other operations

68f3475

haikuginger force-pushed the haikuginger/fqdn-certificate branch from 050a5f6 to 68f3475 Compare December 14, 2017 21:15

haikuginger merged commit da05fe2 into urllib3:master Dec 15, 2017

haikuginger deleted the haikuginger/fqdn-certificate branch December 15, 2017 13:18

paveldedik mentioned this pull request Mar 4, 2019

aiohttp raises SSLError when requesting URLs with FQDN aio-libs/aiohttp#3636

Closed

el-deano mentioned this pull request May 25, 2021

Hostname matching doesn't strip trailing '.' when connecting with proxy #2244

Closed

Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022

Merge pull request urllib3#1255 from haikuginger/haikuginger/fqdn-cer…

544ecd1

…tificate Use FQDN only for DNS and drop trailing dot for other operations

garethsb mentioned this pull request May 26, 2022

handle hostname trailing dot AMWA-TV/nmos-testing#678

Merged

webknjaz mentioned this pull request Jul 13, 2023

Strip trailing dot from FQDNs in Host and TLS aio-libs/aiohttp#7364

Merged

5 tasks

Uh oh!

Use FQDN only for DNS and drop trailing dot for other operations #1255

Use FQDN only for DNS and drop trailing dot for other operations #1255

Uh oh!

Conversation

haikuginger commented Sep 2, 2017

Uh oh!

haikuginger commented Sep 2, 2017

Uh oh!

Lukasa commented Sep 2, 2017

Uh oh!

codecov-io commented Sep 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sigmavirus24 commented Sep 3, 2017

Uh oh!

Lukasa commented Sep 3, 2017

Uh oh!

sigmavirus24 commented Sep 4, 2017

Uh oh!

haikuginger commented Sep 4, 2017

Uh oh!

haikuginger commented Sep 4, 2017

Uh oh!

haikuginger commented Sep 8, 2017

Uh oh!

haikuginger commented Sep 8, 2017

Uh oh!

haikuginger commented Sep 11, 2017

Uh oh!

haikuginger commented Sep 14, 2017

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haikuginger Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haikuginger commented Oct 13, 2017

Uh oh!

Lukasa commented Oct 14, 2017

Uh oh!

haikuginger commented Dec 12, 2017

Uh oh!

haikuginger commented Dec 12, 2017

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Sep 2, 2017 •

edited

Loading

haikuginger Sep 15, 2017 •

edited

Loading