Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ssl.enum_certificates() regression #80122

Closed
schlenk mannequin opened this issue Feb 8, 2019 · 20 comments
Closed

ssl.enum_certificates() regression #80122

schlenk mannequin opened this issue Feb 8, 2019 · 20 comments
Labels
OS-windows topic-SSL type-bug An unexpected behavior, bug, or error

Comments

@schlenk
Copy link
Mannequin

schlenk mannequin commented Feb 8, 2019

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
    Copy link
    Mannequin Author

    schlenk mannequin commented Feb 8, 2019

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

    @schlenk schlenk mannequin assigned tiran Feb 8, 2019
    @schlenk schlenk mannequin added 3.7 (EOL) end of life 3.8 only security fixes OS-windows topic-SSL type-bug An unexpected behavior, bug, or error labels Feb 8, 2019
    @tiran
    Copy link
    Member

    tiran commented Feb 8, 2019

    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.

    @tiran tiran assigned zooba and unassigned tiran Feb 8, 2019
    @schlenk
    Copy link
    Mannequin Author

    schlenk mannequin commented Feb 8, 2019

    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
    Copy link
    Member

    zooba commented Feb 19, 2019

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

    @kctherookie
    Copy link
    Mannequin

    kctherookie mannequin commented Feb 19, 2019

    Adjusted the code to conform with PEP-7.

    @zooba zooba removed their assignment Mar 21, 2019
    @christian-intra2net
    Copy link
    Mannequin

    christian-intra2net mannequin commented Mar 21, 2019

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

    @kctherookie
    Copy link
    Mannequin

    kctherookie mannequin commented Mar 21, 2019

    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
    Copy link
    Member

    zooba commented Mar 21, 2019

    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
    Copy link
    Mannequin

    kctherookie mannequin commented Mar 21, 2019

    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
    Copy link
    Member

    zooba commented Mar 28, 2019

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

    @tiran
    Copy link
    Member

    tiran commented Mar 28, 2019

    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
    Copy link
    Member

    tiran commented Mar 28, 2019

    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
    Copy link
    Contributor

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

    @neonene
    Copy link
    Mannequin

    neonene mannequin commented Jul 12, 2019

    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
    Copy link
    Member

    tiran commented Jul 12, 2019

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

    @neonene
    Copy link
    Mannequin

    neonene mannequin commented Jul 12, 2019

    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
    Copy link
    Member

    zooba commented Sep 9, 2019

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

    @zooba
    Copy link
    Member

    zooba commented Sep 10, 2019

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

    @zooba
    Copy link
    Member

    zooba commented Sep 10, 2019

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

    @zooba
    Copy link
    Member

    zooba commented Sep 10, 2019

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

    @zooba zooba removed the 3.7 (EOL) end of life label Sep 10, 2019
    @zooba zooba removed the 3.8 only security fixes label Sep 10, 2019
    @tiran tiran closed this as completed Sep 10, 2019
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    OS-windows topic-SSL type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants