Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return hostnames from ThreadedResolver #5118

Merged
merged 1 commit into from
Oct 24, 2020
Merged

Conversation

djmitche
Copy link
Contributor

@djmitche djmitche commented Oct 24, 2020

What do these changes do?

Fix a variable-shadowing bug causing ThreadedResolver.resolve to
return the resolved IP as the "hostname" in each record, which prevented
validation of HTTPS connections. Fixes #5110.

Are there changes in behavior for the user?

Failed HTTPS connections will work again.

Related issue number

Fixes #5110.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes (N/A: bugfix)
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt (N/A: no new content)
    • 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."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 24, 2020
@asvetlov
Copy link
Member

Please fix failed tests, I'm pretty sure that they are related to your change

@djmitche
Copy link
Contributor Author

djmitche commented Oct 24, 2020

(sorry, didn't mean to click "comment" yet!)

@djmitche
Copy link
Contributor Author

It looks like c48f2d1, which introduced this bug, also modified the test to assert incorrect behavior. @socketpair can you comment on why this was done? Should resolve return an address as "hostname" for the link-local case? Or was this just a change to the test to make it assert what the function now returns.

Fix a variable-shadowing bug causing `ThreadedResolver.resolve` to
return the resolved IP as the "hostname" in each record, which prevented
validation of HTTPS connections.  Fixes aio-libs#2110.
@djmitche
Copy link
Contributor Author

(the updated version here reverts the test change in c48f2d1)

@asvetlov
Copy link
Member

Or was this just a change to the test to make it assert what the function now returns.

Yes, it was. I made it by mistake a few days ago.

@asvetlov asvetlov merged commit 6fed31f into aio-libs:master Oct 24, 2020
aio-libs-github-bot bot pushed a commit that referenced this pull request Oct 24, 2020
Fix a variable-shadowing bug causing `ThreadedResolver.resolve` to
return the resolved IP as the "hostname" in each record, which prevented
validation of HTTPS connections.  Fixes #2110.
@aio-libs-github-bot
Copy link
Contributor

💚 Backport successful

The PR was backported to the following branches:

aio-libs-github-bot bot added a commit that referenced this pull request Oct 24, 2020
Backports the following commits to 3.7:
 - Return hostnames from ThreadedResolver (#5118)

Co-authored-by: Dustin J. Mitchell <[email protected]>
djmitche added a commit to djmitche/taskcluster that referenced this pull request Oct 25, 2020
aio-libs/aiohttp#5110 means that https URLs
don't work.  aio-libs/aiohttp#5118 fixes the
issue, and will be included in 3.7.1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

certificate is not valid for IP
2 participants