Skip to content

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

Merged
Mariatta merged 10 commits intopython:masterfrom
daxlab:bpo-23033
Nov 26, 2017
Merged

bpo-23033: consider wildcard in left most segment only for domain names#937
Mariatta merged 10 commits intopython:masterfrom
daxlab:bpo-23033

Conversation

@daxlab
Copy link
Copy Markdown
Contributor

@daxlab daxlab commented Apr 1, 2017 •
edited by bedevere-bot
Loading

@mention-bot
Copy link
Copy Markdown

mention-bot commented Apr 1, 2017

@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
Copy link
Copy Markdown
Member

alex commented Apr 1, 2017

Looks like there is a relevant failing ssl test.

@daxlab
Copy link
Copy Markdown
Contributor Author

daxlab commented Apr 2, 2017

@alex review now.

alex
alex requested changes Apr 2, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@alex alex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Lib/ssl.py Outdated
Copy link
Copy Markdown
Member

@alex alex Apr 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread Lib/test/test_ssl.py Outdated
Copy link
Copy Markdown
Member

@alex alex Apr 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@daxlab daxlab Apr 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds much better, pushing the changes.

@alex
Copy link
Copy Markdown
Member

alex commented Apr 2, 2017

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

@daxlab
Copy link
Copy Markdown
Contributor Author

daxlab commented Apr 2, 2017

@alex anything else needed from my side ?

@alex
Copy link
Copy Markdown
Member

alex commented Apr 2, 2017

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

reaperhulk
reaperhulk approved these changes Apr 2, 2017
View reviewed changes
Copy link
Copy Markdown

@reaperhulk reaperhulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alex alex mentioned this pull request Apr 2, 2017
Merged
@tiran
Copy link
Copy Markdown
Member

tiran commented Apr 2, 2017

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

tiran
tiran requested changes Apr 2, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

versionchanged in documentation is missing.

@daxlab
Copy link
Copy Markdown
Contributor Author

daxlab commented Apr 2, 2017 •
edited
Loading

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

tiran
tiran approved these changes Apr 2, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 daxlab force-pushed the bpo-23033 branch 2 times, most recently from 31c911b to 2416d6a Compare April 2, 2017 18:53
@daxlab
Copy link
Copy Markdown
Contributor Author

daxlab commented Apr 3, 2017

@tiran @alex can we merge this ?

@tiran
Copy link
Copy Markdown
Member

tiran commented Apr 4, 2017

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

berkerpeksag
berkerpeksag reviewed Apr 19, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread Doc/whatsnew/3.7.rst Outdated
Copy link
Copy Markdown
Member

@berkerpeksag berkerpeksag Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@daxlab daxlab Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Mariatta
Mariatta reviewed Jun 12, 2017
View reviewed changes
Comment thread Misc/NEWS Outdated
Copy link
Copy Markdown
Member

@Mariatta Mariatta Jun 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add "Patch by Mandeep Singh."

Copy link
Copy Markdown
Contributor Author

@daxlab daxlab Jun 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@Mariatta
Copy link
Copy Markdown
Member

Mariatta commented Jun 12, 2017

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

@daxlab
Copy link
Copy Markdown
Contributor Author

daxlab commented Jul 2, 2017

@Mariatta changes pushed.

@daxlab daxlab requested a review from a team October 1, 2017 07:19
@daxlab daxlab requested a review from a team as a code owner October 1, 2017 07:19
@the-knights-who-say-ni
Copy link
Copy Markdown

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

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!

@brettcannon 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
@Mariatta
Copy link
Copy Markdown
Member

Mariatta commented Nov 24, 2017

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
Copy link
Copy Markdown
Contributor Author

daxlab commented Nov 24, 2017

@Mariatta will update the PR tonight.

@daxlab
Copy link
Copy Markdown
Contributor Author

daxlab commented Nov 26, 2017

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

@Mariatta
Copy link
Copy Markdown
Member

Mariatta commented Nov 26, 2017

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

@Mariatta Mariatta merged commit ede2ac9 into python:master Nov 26, 2017
@Mariatta
Copy link
Copy Markdown
Member

Mariatta commented Nov 26, 2017

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

@daxlab daxlab deleted the bpo-23033 branch November 27, 2017 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksag berkerpeksag berkerpeksag left review comments

@alex alex alex approved these changes

@tiran tiran tiran approved these changes

@Mariatta Mariatta Mariatta approved these changes

@1st1 1st1 Awaiting requested review from 1st1

@gpshead gpshead Awaiting requested review from gpshead

@ncoghlan ncoghlan Awaiting requested review from ncoghlan

@rhettinger rhettinger Awaiting requested review from rhettinger

@skrah skrah Awaiting requested review from skrah

+1 more reviewer

@reaperhulk reaperhulk reaperhulk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@daxlab @mention-bot @alex @tiran @Mariatta @the-knights-who-say-ni @berkerpeksag @reaperhulk @bedevere-bot