| msg335084 - (view) |
Author: Michael Schlenker (schlenk) |
Date: 2019-02-08 14:43 |
| The introduction of the ReadOnly flag in the ssl.enum_certificates() function implementation has introduced a regression. The old version returned certificates for both the current user and the local system, the new function only enumerates system wide certificates and ignores the current user. The old function before Patch from https://bugs.python.org/issue25939 used a different function to open the certificate store (CertOpenStore vs. CertOpenSystemStore). Probably some of the param flags are not identical, the new code explictly lists only local system. Testing: 1. Import a self signed CA only into the 'current user' trustworthy certificates. 2. Use IE to Connect to a https:// website using that trust root. Works. 3. Try to open the website with old python and new python. Old one works, new one fails. Or just enum certificates: 1. Import a self signed CA into the current_user trusted store. 2. Compare outputs of: import ssl len(ssl.enum_certificates('ROOT')) |
|
|
| msg335085 - (view) |
Author: Christian Heimes (christian.heimes) *  |
Date: 2019-02-08 14:59 |
| I guess the flags are wrong. https://hg.python.org/cpython/rev/d6474257ef38 changed flags to CERT_SYSTEM_STORE_LOCAL_MACHINE. To get local user certs, the flag should probably be CERT_SYSTEM_STORE_CURRENT_USER. |
|
|
| msg335087 - (view) |
Author: Michael Schlenker (schlenk) |
Date: 2019-02-08 15:28 |
| It probably is even worse. The flag seems to specifiy the physical locations, and just using CERT_SYSTEM_STORE_LOCAL_SYSTEM probably misses the certificates distributed by Group Policy or AD too, in addition to the stores for the current user. See https://blogs.msdn.microsoft.com/muaddib/2013/10/18/understanding-and-managing-the-certificate-stores-used-for-smart-card-logon/ |
|
|
| msg335983 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2019-02-19 17:14 |
| The PR requires PEP 7 to be applied thoroughly, but I think the concept is sound enough. |
|
|
| msg336011 - (view) |
Author: kc (kc) * |
Date: 2019-02-19 19:57 |
| Adjusted the code to conform with PEP 7. |
|
|
| msg338555 - (view) |
Author: Christian Herdtweck (christian-intra2net) |
Date: 2019-03-21 16:26 |
| Hi, I encountered this problem as well. May I know why you have withdrawn your pull request? |
|
|
| msg338558 - (view) |
Author: kc (kc) * |
Date: 2019-03-21 17:03 |
| To be honest, I think the patch is worth to be merged including other patches I submitted. Yet I believed it was better to close the pull request because I put quite some time into researching and programming the solutions but nobody really cared so I stopped. All I received from my work was a "Awaiting merge" screen on my laptop. It was really discouraging to see how things work here in the python development community so I decided to leave instead of waiting a month or more and at the end looking at the "Awaiting to merge" screen or being rejected again. Thanks Christian for asking why I closed the PR at last. Just look at the date when the patch was submitted until it was put into awaiting patch mode again then you see what I mean. If anybody still wants me to submit patches please say so instead of completely ignoring me and my work. |
|
|
| msg338561 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2019-03-21 17:37 |
| I don't know about your other PRs, and I don't deny they may have been neglected for some time, but you only allowed 12 hours on this one between receiving a review and closing it. Our team of volunteers have limited time (typically 0-8 hours/week) to work on CPython, and a lot of that gets taken up with blocking issues - we simply cannot drop everything for every issue or contribution that comes in. If you'd like to restore your branch and reopen the PR, we will get to it eventually (unfortunately, all my CPython time today has been taken up by writing comments like this, so it won't be today). Or if someone else would like to make their own PR then we'll look at that one. |
|
|
| msg338564 - (view) |
Author: kc (kc) * |
Date: 2019-03-21 19:24 |
| Hello Steve, I reopened the PR from my code base. I will wait for this PR to be processed and afterwards continue with submitting patches. |
|
|
| msg339066 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2019-03-28 17:59 |
| New changeset d93fbbf88e4abdd24a0a55e3ddf85b8420c62052 by Steve Dower (kctherookie) in branch 'master': bpo-35941: Fix ssl certificate enumeration for windows (GH-12486) https://github.com/python/cpython/commit/d93fbbf88e4abdd24a0a55e3ddf85b8420c62052 |
|
|
| msg339068 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2019-03-28 18:13 |
| Steve, why did you add the list_contains() check? Does the new code return one certificate multiple times? I'm worried that the performance of check is rather slow. IMHO it would be more efficient to use a set here. |
|
|
| msg339069 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2019-03-28 18:19 |
| By the way, OpenSSL ignores duplicate certificates. There is no need to filter out duplicate entries. However it is more efficient to filter them out early, because OpenSSL filters after parsing the ASN.1 structure. |
|
|
| msg339073 - (view) |
Author: miss-islington (miss-islington) |
Date: 2019-03-28 18:56 |
| New changeset e9868c5416731f5ca5378a1d36e4b020c349291c by Miss Islington (bot) in branch '3.7': bpo-35941: Fix ssl certificate enumeration for windows (GH-12486) https://github.com/python/cpython/commit/e9868c5416731f5ca5378a1d36e4b020c349291c |
|
|
| msg347726 - (view) |
Author: neonene (neonene) * |
Date: 2019-07-12 07:56 |
| After this patch applied, memory usage increases every https-access and is not released in my Win7x64SP1. I hope this will be fixed or reverted. (case sample) from urllib import request from time import sleep import gc while True: request.urlopen(request.Request('https://...')) gc.collect() sleep(2) |
|
|
| msg347744 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2019-07-12 14:09 |
| Which patch do you mean? Are you referring to the landed PR or my fix for performance regression https://github.com/python/cpython/pull/12610? |
|
|
| msg347752 - (view) |
Author: neonene (neonene) * |
Date: 2019-07-12 19:18 |
| I meant 12609. (x86,x64 Py374rc1,Py380a4 and later) And though I tried merging 12610 and Py374, memory usage still increases. Sorry, I can't find out the cause. |
|
|
| msg351514 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2019-09-09 16:06 |
| New changeset 915cd3f0696cb8a7206754a8fc34d4cd865a1b4a by Steve Dower (Christian Heimes) in branch 'master': bpo-35941: Fix performance regression in new code (GH-12610) https://github.com/python/cpython/commit/915cd3f0696cb8a7206754a8fc34d4cd865a1b4a |
|
|
| msg351591 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2019-09-10 08:46 |
| New changeset b4fb2c29f34c322855ab6be72b491930cf508f64 by Steve Dower in branch '3.7': bpo-35941: Fix performance regression in SSL certificate code (GH-12610) https://github.com/python/cpython/commit/b4fb2c29f34c322855ab6be72b491930cf508f64 |
|
|
| msg351596 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2019-09-10 09:02 |
| New changeset fdd17abc51e363ab19d248375d717512b8b26966 by Steve Dower in branch '3.8': bpo-35941: Fix performance regression in SSL certificate code (GH-12610) https://github.com/python/cpython/commit/fdd17abc51e363ab19d248375d717512b8b26966 |
|
|
| msg351598 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2019-09-10 09:08 |
| That's 3.7 and later dealt with. Apparently this needs a backport to 2.7 still, so I'll leave the bug open. |
|
|