bpo-23033: consider wildcard in left most segment only for domain names #937

daxlab · 2017-04-01T04:59:27Z

https://bugs.python.org/issue23033

mention-bot · 2017-04-01T04:59:30Z

@daxlab, thanks for your PR! By analyzing the history of the files in this pull request, we identified @janssen, @Yhg1s and @tiran to be potential reviewers.

alex · 2017-04-01T22:10:33Z

Looks like there is a relevant failing ssl test.

daxlab · 2017-04-02T10:02:39Z

@alex review now.

alex

I have a few small comments.

Thanks for working on this! This is a great improvement to making match_hostname behave more like the rest of the PKI infrastructure.

alex · 2017-04-02T12:44:39Z

Lib/ssl.py

-    if wildcards > max_wildcards:
+    if wildcards == 1 and len(leftmost) > 1:
+        """ Only match wildcard in leftmost segment.
+        """


Nit: Please use a # style comment. """ is for docstrings.

alex · 2017-04-02T12:46:57Z

Lib/test/test_ssl.py

@@ -480,10 +480,10 @@ def fail(cert, hostname):
        fail(cert, 'Xa.com')
        fail(cert, '.a.com')

-        # only match one left-most wildcard
+        # only match wildcard in left-most segment


This comment still doesn't fell right, there is a wildcard in the left-most segment. I think it should say something like "only match wildcards when they are the only thing in the left-most segment". What do you think?

Sounds much better, pushing the changes.

alex · 2017-04-02T13:40:51Z

Doc failures are not your fault, sphinx-doc/sphinx#3597 is the cause.

daxlab · 2017-04-02T13:49:42Z

@alex anything else needed from my side ?

alex · 2017-04-02T13:50:17Z

This looks good to me. I'll probably ask for another review before landing. Thanks so much!

reaperhulk

Looks good to me. Wildcards should be honored iff they are the leftmost fragment and are the only character in that fragment.

tiran · 2017-04-02T15:55:12Z

You are almost there! Please add a versionchanged:: 3.7 to match_hostname documentation in Doc/library/ssl.rst.

tiran

versionchanged in documentation is missing.

daxlab · 2017-04-02T17:23:31Z

@tiran version changed docs added. Should that go in Doc/whatsnew/3.7.rst also ?

tiran

Thank you very much for your contribution, @daxlab. Your work is an excellent base to clean up and simplify the host matching code even further.

@alex the change is backwards incompatible. I've asked Ben and Ned if they are fine with backports to 2.7 and 3.6.

daxlab · 2017-04-03T09:27:41Z

@tiran @alex can we merge this ?

tiran · 2017-04-04T06:58:06Z

@daxlab @alex I'm waiting for confirmation regarding backport to 3.6 and 2.7.

berkerpeksag

I think we also need to add an entry to Misc/NEWS.

berkerpeksag · 2017-04-19T04:22:35Z

Doc/whatsnew/3.7.rst

@@ -273,6 +273,10 @@ Changes in the Python API
  ``makedirs()``.
  (Contributed by Serhiy Storchaka in :issue:`19930`.)

+* Now wildcard is supported in hostname when it is one and only character in the
+  left most segment of hostname in second argument of :meth:`match_hostname`.


I'd change :meth:`match_hostname` to :meth:`ssl.match_hostname`. This would create a link to the ssl.match_hostname() documentation.

@berkerpeksag thanks for review. I have pushed a fix.

Mariatta · 2017-06-12T04:16:03Z

Misc/NEWS

@@ -313,6 +313,9 @@ Extension Modules
 Library
 -------

+- bpo-23033: Now wildcard is supported in hostname when it is one and only
+  character in the left most segment of hostname.


Please add "Patch by Mandeep Singh."

Mariatta · 2017-06-12T04:16:34Z

Please add yourself to Misc/ACKS, resolve the conflict and update Misc/NEWS.
Thanks.

daxlab · 2017-07-02T10:32:03Z

@Mariatta changes pushed.

the-knights-who-say-ni · 2017-10-01T07:20:06Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

Mariatta · 2017-11-24T00:44:37Z

Thanks @daxlab for your patience with this PR, sorry it's taking a while.
I think this can go to 3.7.
Could you rebase this to make it up to date? We need the news entry in Misc/NEWS.d/, it can be generated using blurb.

daxlab · 2017-11-24T07:18:13Z

@Mariatta will update the PR tonight.

daxlab · 2017-11-26T11:33:11Z

@Mariatta I have rebased the PR and added news entry in Misc/NEWS.d/

Mariatta · 2017-11-26T15:57:51Z

Thanks. I made minor changes and added you to Misc/ACKs. Waiting until CI is done.

Mariatta · 2017-11-26T22:32:00Z

Thanks again @daxlab and congrats on your first contribution to CPython 🌮

the-knights-who-say-ni added the CLA signed label Apr 1, 2017

alex requested changes Apr 2, 2017

View reviewed changes

alex approved these changes Apr 2, 2017

View reviewed changes

alex added needs backport to 2.7 labels Apr 2, 2017

reaperhulk approved these changes Apr 2, 2017

View reviewed changes

alex mentioned this pull request Apr 2, 2017

Only allow wildcards which form an entire path segment. pyca/service-identity#19

Merged

tiran requested changes Apr 2, 2017

View reviewed changes

tiran approved these changes Apr 2, 2017

View reviewed changes

daxlab force-pushed the bpo-23033 branch 2 times, most recently from 31c911b to 2416d6a Compare April 2, 2017 18:53

berkerpeksag reviewed Apr 19, 2017

View reviewed changes

Mariatta reviewed Jun 12, 2017

View reviewed changes

Mariatta removed needs backport to 2.7 labels Jun 12, 2017

daxlab requested review from 1st1, gpshead and ncoghlan as code owners October 1, 2017 07:19

daxlab requested a review from a team October 1, 2017 07:19

daxlab requested a review from a team as a code owner October 1, 2017 07:19

the-knights-who-say-ni added CLA not signed and removed CLA signed labels Oct 1, 2017

daxlab force-pushed the bpo-23033 branch from 2630b21 to a93277c Compare October 1, 2017 07:24

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Oct 1, 2017

brettcannon changed the title ~~bpo-23033: consider wildcard in left most segment only~~ bpo-23033: consider wildcard in left most segment only for domain names Oct 2, 2017

daxlab added 6 commits November 26, 2017 16:43

consider wildcard in left most segment only

7a0fe23

idna tests should fail

a02fea1

multiple wildcards should raise exception

358a4e1

improve documentation

a8fb42e

add version changed documentation for match_hostname

823eb02

add misc news for bpo 23033

e4d4c22

daxlab force-pushed the bpo-23033 branch from c979ac3 to e4d4c22 Compare November 26, 2017 11:32

Mariatta added 2 commits November 26, 2017 07:47

Updae Misc/ACKS

f270238

Update News.

6c48fdb

Mariatta approved these changes Nov 26, 2017

View reviewed changes

bedevere-bot added the awaiting merge label Nov 26, 2017

Mariatta added 2 commits November 26, 2017 07:50

left most -> the leftmost

d5fd448

left most -> leftmost

d703103

Mariatta merged commit ede2ac9 into python:master Nov 26, 2017

bedevere-bot removed the awaiting merge label Nov 26, 2017

daxlab deleted the bpo-23033 branch November 27, 2017 04:23

Uh oh!

bpo-23033: consider wildcard in left most segment only for domain names #937

bpo-23033: consider wildcard in left most segment only for domain names #937

Uh oh!

Conversation

daxlab commented Apr 1, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Apr 1, 2017

Uh oh!

alex commented Apr 1, 2017

Uh oh!

daxlab commented Apr 2, 2017

Uh oh!

alex left a comment

Choose a reason for hiding this comment

Uh oh!

alex Apr 2, 2017

Choose a reason for hiding this comment

Uh oh!

alex Apr 2, 2017

Choose a reason for hiding this comment

Uh oh!

daxlab Apr 2, 2017

Choose a reason for hiding this comment

Uh oh!

alex commented Apr 2, 2017

Uh oh!

daxlab commented Apr 2, 2017

Uh oh!

alex commented Apr 2, 2017

Uh oh!

reaperhulk left a comment

Choose a reason for hiding this comment

Uh oh!

tiran commented Apr 2, 2017

Uh oh!

tiran left a comment

Choose a reason for hiding this comment

Uh oh!

daxlab commented Apr 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiran left a comment

Choose a reason for hiding this comment

Uh oh!

daxlab commented Apr 3, 2017

Uh oh!

tiran commented Apr 4, 2017

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

berkerpeksag Apr 19, 2017

Choose a reason for hiding this comment

Uh oh!

daxlab Apr 19, 2017

Choose a reason for hiding this comment

Uh oh!

Mariatta Jun 12, 2017

Choose a reason for hiding this comment

Uh oh!

daxlab Jun 12, 2017

Choose a reason for hiding this comment

Uh oh!

Mariatta commented Jun 12, 2017

Uh oh!

daxlab commented Jul 2, 2017

Uh oh!

the-knights-who-say-ni commented Oct 1, 2017

Uh oh!

Mariatta commented Nov 24, 2017

Uh oh!

daxlab commented Nov 24, 2017

Uh oh!

daxlab commented Nov 26, 2017

Uh oh!

Mariatta commented Nov 26, 2017

Uh oh!

Mariatta commented Nov 26, 2017

daxlab commented Apr 1, 2017 •

edited by bedevere-bot

Loading

daxlab commented Apr 2, 2017 •

edited

Loading