| msg184650 - (view) |
Author: Daniel Wozniak (dwoz) |
Date: 2013-03-19 17:55 |
| In the urllib.request.urlopen on line 154 the code path that sets check_hostname to False will never run. I'm not sure what the desired behavior is. If we want to have a way to tell urlopen not to check the hostname for https connections it looks as though there needs to be some re-factoring. |
|
|
| msg184653 - (view) |
Author: Jeff Knupp (jeffknupp) * |
Date: 2013-03-19 18:07 |
| Was this discovered when you were trying "to tell urlopen not to check the hostname for https connections?" If so, that should be reflected in the title so others know what the observable effect is. Referencing specific variables is less useful both for searching later and because the fix may not touch that part at all (as you alluded to, it may require refactoring). |
|
|
| msg184658 - (view) |
Author: Daniel Wozniak (dwoz) |
Date: 2013-03-19 18:33 |
| Yes, I was trying to add tests for the behavior. I'll try to make the titles more meaningful in the future. Thanks. |
|
|
| msg185547 - (view) |
Author: Senthil Kumaran (orsenthil) *  |
Date: 2013-03-30 06:06 |
| Here is an approach to solve this. I think, going this way is safe. I shall append with the tests and docs. |
|
|
| msg185721 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2013-04-01 06:39 |
| Here is the final patch (17483.patch) for this support. I am including Ezio & David for their review comments before I check this in. Thanks! |
|
|
| msg185724 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-04-01 09:05 |
| I'm sorry, I'm -1 on this. It simply doesn't make sense to check the certificate but skip hostname checking. |
|
|
| msg185735 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2013-04-01 12:25 |
| It may not make sense, but I've seen it supported in the wild, in a different library. Of course, we *did* treat it as a bug in our code and fix it once we realized that's what the library was doing (we were inadvertently passing it None for the hostname, and its response to that was to not check it). So I think Antoine is probably right here. Especially since this has never been reported as a bug by someone trying to use it in an application. Since it wasn't documented, perhaps we should just add a deprecation warning (that says it is a noop) instead. |
|
|
| msg185744 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2013-04-01 17:04 |
| New changeset 4ed8a8e781c3 by Antoine Pitrou in branch 'default': Issue #17483: remove unreachable code in urlopen(). http://hg.python.org/cpython/rev/4ed8a8e781c3 |
|
|
| msg185755 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2013-04-01 18:27 |
| Antoine - I approached it from idea that check_hostname "as a setting" is allowed from HTTPSConnection (http/client.py) but it not controllable from urllib. Is there a case where it is useful in HTTPSConnection, but it should not be from urllib? - Thanks for the changes. One comment on the changeset 4ed8a8e781c3 + https_handler = HTTPSHandler(context=context, check_hostname=True) check_hostname=True is redundant here. You could send the context and by default it checks the hostname. |
|
|
| msg185756 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-04-01 18:42 |
| > Antoine - I approached it from idea that check_hostname "as a setting" > is allowed from HTTPSConnection (http/client.py) but it not > controllable from urllib. Is there a case where it is useful in > HTTPSConnection, but it should not be from urllib? HTTPSConnection is lower-level, so it makes sense to allow more deviations there. That's why HTTPSConnection also takes the context directly. |
|
|
| msg185786 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2013-04-02 01:37 |
| > HTTPSConnection is lower-level, so it makes sense to allow more > deviations there. That's why HTTPSConnection also takes the context > directly. That's okay of an explanation. HTTPSHandler in urllib module provides an option to send the context and the check_hostname and we are using that here. I can understand the security benefits in sending the check_hostname=True (which is sane thing to do). But if someone explicitly wants to use the check_hostname=False in the HTTPSHandler, he will have to create a new local opener object and then use the opener.open to carry on with the request as he would expect it to behave. ( Do you think this warrants a doc update. Personally I am 0 on it). I am okay this change. This should be back ported as the code was unreachable 3.3 in the first place and there is no behavior change. Thanks! |
|
|
| msg185947 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2013-04-03 19:58 |
| IMO, there's not much point backporting such a simple cleanup, so I'm simply closing the issue. |
|
|
| msg185949 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2013-04-03 20:07 |
| I shall do it. Just for keeping it clean and it may help when future patches can be merged across branches cleanly. There is no loss IMO. |
|
|
| msg186062 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2013-04-05 02:35 |
| New changeset fc39b8f0348d by Senthil Kumaran in branch '3.3': Issue #17483: 3.3 Branch - Remove unreachable code in urllib.request http://hg.python.org/cpython/rev/fc39b8f0348d |
|
|
| msg223856 - (view) |
Author: Alecz (Alecz) |
Date: 2014-07-24 16:29 |
| Actually, because of issue 18543, urlopen will not use the custom opener if one was defined, instead, it will create a new opener with check_hostname = True. So it is impossible to skip hostname checking without overriding the urlopen method. I don't understand why can't we allow check_hostmane to be set to False in urlopen and also why we don't allow custom openers to be used by urlopen if specifying a ca*. Moreover this forced restriction is not documented. I had to go to the source code to figure out why it wasn't working. This issue being resolved makes it even more misleading. |
|
|
| msg223860 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2014-07-24 17:00 |
| This issue was about setting hostname checking to false when the cert was being checked, and we rejected making that even possible in urlopen. If there is an issue with not being able to use a custom opener, that would be a different issue and you should open a new ticket. Also if you think there is a documentation bug, that should be a new issue, which can refer to this one for background. |
|
|
| msg223896 - (view) |
Author: Alecz (Alecz) |
Date: 2014-07-24 20:50 |
| If this request was rejected shouldn't the Resolution be something such as "Rejected", "Not a Bug", or "Wont fix"? At the first glance, I believe it is very misleading to see this as fixed. I even installed the latest version and was surprised to see that "the fix" did not work as expected. |
|
|