ssl.enum_certificates() regression

[schlenk]

BPO	35941
Nosy	@pfmoore, @tiran, @tjguk, @zware, @zooba, @miss-islington, @kctherookie, @christian-intra2net, @neonene
PRs	bpo-35941: Fix ssl certificate enumeration for windows #11923 bpo-35941: Fix ssl certificate enumeration for windows #12486 [3.7] bpo-35941: Fix ssl certificate enumeration for windows (GH-12486) #12609 bpo-35941: Fix performance regression in new code #12610 [3.8] bpo-35941: Fix performance regression in SSL certificate code (GH-12610) #15803 [3.7] bpo-35941: Fix performance regression in SSL certificate code (GH-12610) #15804

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-09-10.11:19:16.194>
created_at = <Date 2019-02-08.14:43:29.692>
labels = ['expert-SSL', 'type-bug', 'OS-windows']
title = 'ssl.enum_certificates() regression'
updated_at = <Date 2019-09-10.11:19:16.193>
user = 'https://bugs.python.org/schlenk'

bugs.python.org fields:

activity = <Date 2019-09-10.11:19:16.193>
actor = 'christian.heimes'
assignee = 'none'
closed = True
closed_date = <Date 2019-09-10.11:19:16.194>
closer = 'christian.heimes'
components = ['Windows', 'SSL']
creation = <Date 2019-02-08.14:43:29.692>
creator = 'schlenk'
dependencies = []
files = []
hgrepos = []
issue_num = 35941
keywords = ['patch']
message_count = 20.0
messages = ['335084', '335085', '335087', '335983', '336011', '338555', '338558', '338561', '338564', '339066', '339068', '339069', '339073', '347726', '347744', '347752', '351514', '351591', '351596', '351598']
nosy_count = 10.0
nosy_names = ['paul.moore', 'christian.heimes', 'tim.golden', 'zach.ware', 'steve.dower', 'schlenk', 'miss-islington', 'kc', 'christian-intra2net', 'neonene']
pr_nums = ['11923', '12486', '12609', '12610', '15803', '15804']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue35941'
versions = ['Python 2.7']

[schlenk]

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'))

[tiran]

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.

[schlenk]

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/

[zooba]

The PR requires PEP-7 to be applied thoroughly, but I think the concept is sound enough.

[kctherookie]

Adjusted the code to conform with PEP-7.

[christian-intra2net]

Hi, I encountered this problem as well. May I know why you have withdrawn your pull request?

[kctherookie]

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.

[zooba]

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.

[kctherookie]

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.

[zooba]

New changeset d93fbbf by Steve Dower (kctherookie) in branch 'master':
bpo-35941: Fix ssl certificate enumeration for windows (GH-12486)
d93fbbf

[tiran]

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.

[tiran]

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.

[miss-islington]

New changeset e9868c5 by Miss Islington (bot) in branch '3.7':
bpo-35941: Fix ssl certificate enumeration for windows (GH-12486)
e9868c5

[neonene]

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)

[tiran]

Which patch do you mean? Are you referring to the landed PR or my fix for performance regression #12610?

[neonene]

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.

[zooba]

New changeset 915cd3f by Steve Dower (Christian Heimes) in branch 'master':
bpo-35941: Fix performance regression in new code (GH-12610)
915cd3f

[zooba]

New changeset b4fb2c2 by Steve Dower in branch '3.7':
bpo-35941: Fix performance regression in SSL certificate code (GH-12610)
b4fb2c2

[zooba]

New changeset fdd17ab by Steve Dower in branch '3.8':
bpo-35941: Fix performance regression in SSL certificate code (GH-12610)
fdd17ab

[zooba]

That's 3.7 and later dealt with. Apparently this needs a backport to 2.7 still, so I'll leave the bug open.