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

Created on 2019-02-08 14:43 by schlenk, last changed 2019-03-28 18:56 by miss-islington.

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 open christian.heimes, 2019-03-28 18:40
Messages (13)
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
History
Date User Action Args
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