This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: ssl.enum_certificates() regression
Type: behavior Stage: resolved
Components: SSL, Windows Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: christian-intra2net, christian.heimes, kc, miss-islington, neonene, paul.moore, schlenk, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2019-02-08 14:43 by schlenk, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 11923 closed kc, 2019-02-18 18:09
PR 12486 merged kc, 2019-03-21 19:18
PR 12609 merged miss-islington, 2019-03-28 17:59
PR 12610 merged christian.heimes, 2019-03-28 18:40
PR 15803 merged steve.dower, 2019-09-09 16:10
PR 15804 merged steve.dower, 2019-09-09 16:13
Messages (20)
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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.
History
Date User Action Args
2022-04-11 14:59:11adminsetgithub: 80122
2019-09-22 22:15:46steve.dowerlinkissue38251 superseder
2019-09-10 11:19:16christian.heimessetstatus: open -> closed
resolution: fixed
stage: backport needed -> resolved
2019-09-10 09:08:14steve.dowersetstage: patch review -> backport needed
messages: + msg351598
versions: - Python 3.7, Python 3.8
2019-09-10 09:02:08steve.dowersetmessages: + msg351596
2019-09-10 08:46:59steve.dowersetmessages: + msg351591
2019-09-09 16:13:33steve.dowersetpull_requests: + pull_request15455
2019-09-09 16:10:42steve.dowersetpull_requests: + pull_request15454
2019-09-09 16:06:58steve.dowersetmessages: + msg351514
2019-07-12 19:18:12neonenesetmessages: + msg347752
2019-07-12 14:09:04christian.heimessetmessages: + msg347744
2019-07-12 07:56:29neonenesetnosy: + neonene
messages: + msg347726
2019-03-28 18:56:56miss-islingtonsetnosy: + miss-islington
messages: + msg339073
2019-03-28 18:40:25christian.heimessetpull_requests: + pull_request12549
2019-03-28 18:19:27christian.heimessetmessages: + msg339069
2019-03-28 18:13:48christian.heimessetmessages: + msg339068
2019-03-28 17:59:28miss-islingtonsetpull_requests: + pull_request12548
2019-03-28 17:59:12steve.dowersetmessages: + msg339066
2019-03-21 19:24:29kcsetmessages: + msg338564
2019-03-21 19:18:33kcsetstage: needs patch -> patch review
pull_requests: + pull_request12439
2019-03-21 17:37:55steve.dowersetmessages: + msg338561
2019-03-21 17:03:59kcsetmessages: + msg338558
2019-03-21 16:26:42christian-intra2netsetnosy: + christian-intra2net
messages: + msg338555
2019-03-21 15:58:20steve.dowersetassignee: steve.dower ->
stage: patch review -> needs patch
2019-03-21 15:58:02steve.dowerlinkissue36343 superseder
2019-02-19 19:57:46kcsetmessages: + msg336011
2019-02-19 17:14:18steve.dowersetnosy: + kc
messages: + msg335983
2019-02-18 18:09:47kcsetkeywords: + patch
stage: patch review
pull_requests: + pull_request11948
2019-02-08 15:28:09schlenksetmessages: + msg335087
2019-02-08 15:00:49christian.heimessetversions: - Python 3.4, Python 3.5, Python 3.6
2019-02-08 14:59:06christian.heimessetassignee: christian.heimes -> steve.dower
messages: + msg335085
2019-02-08 14:43:29schlenkcreate