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.cert_time_to_seconds() returns wrong results if local timezone is not UTC #64139

Closed
4kir4 mannequin opened this issue Dec 10, 2013 · 26 comments
Closed

ssl.cert_time_to_seconds() returns wrong results if local timezone is not UTC #64139

4kir4 mannequin opened this issue Dec 10, 2013 · 26 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@4kir4
Copy link
Mannequin

4kir4 mannequin commented Dec 10, 2013

BPO 19940
Nosy @pitrou, @tiran, @4kir4
Files
  • patch.txt: Patch
  • patch.txt: Patch with the combined test case and code chages. This overrides the previous patch patch.txt.
  • ssl_cert_time_seconds.py: example cert_time_to_seconds(cert_time) implementation
  • ssl_cert_time_to_seconds.patch: Fixed code (timezone and locale issues), added tests, updated documention
  • ssl_cert_time_to_seconds-ps3.patch: changes following the review
  • ssl_cert_time_to_seconds-462470859e57.patch: simplify implementation
  • ssl_cert_time_to_seconds-ps5.patch
  • ssl_cert_time_to_seconds-ps6.patch: fix issues pointed out in the review
  • 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 2014-04-28.18:58:17.945>
    created_at = <Date 2013-12-10.07:29:12.424>
    labels = ['type-bug', 'library']
    title = 'ssl.cert_time_to_seconds() returns wrong results if local timezone is not UTC'
    updated_at = <Date 2014-04-28.22:46:14.088>
    user = 'https://github.com/4kir4'

    bugs.python.org fields:

    activity = <Date 2014-04-28.22:46:14.088>
    actor = 'akira'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-04-28.18:58:17.945>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2013-12-10.07:29:12.424>
    creator = 'akira'
    dependencies = []
    files = ['33217', '33254', '34197', '34201', '34594', '35050', '35051', '35075']
    hgrepos = []
    issue_num = 19940
    keywords = ['patch']
    message_count = 26.0
    messages = ['205774', '205803', '205814', '205815', '205860', '206616', '206617', '206620', '206629', '206635', '206666', '206810', '206825', '207083', '211988', '211992', '212022', '212380', '214637', '217243', '217245', '217373', '217392', '217394', '217395', '217435']
    nosy_count = 7.0
    nosy_names = ['janssen', 'pitrou', 'christian.heimes', 'docs@python', 'akira', 'python-dev', 'gudge']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue19940'
    versions = ['Python 3.5']

    @4kir4
    Copy link
    Mannequin Author

    4kir4 mannequin commented Dec 10, 2013

    cert_time_to_seconds() uses time.mktime() 1 to convert utc time tuple to seconds since epoch. mktime() works with local time. It should use calendar.timegm() analog instead.

    Example from the docs 2 is seven hours off (it shows utc offset of the local timezone of the person who created it):

        >>> import ssl
        >>> ssl.cert_time_to_seconds("May  9 00:00:00 2007 GMT")
        1178694000.0

    It should be 1178668800:

        >>> from datetime import datetime
        >>> datetime.utcfromtimestamp(1178668800)
        datetime.datetime(2007, 5, 9, 0, 0)
        >>> import time
        >>> time.gmtime(1178668800)
        time.struct_time(tm_year=2007, tm_mon=5, tm_mday=9, tm_hour=0, tm_min=0, tm_sec=0, tm_wday=2, tm_yday=129, tm_isdst=0)

    And calendar.timegm returns correct result:

        >>> calendar.timegm(time.strptime("May 9 00:00:00 2007 GMT", "%b %d %H:%M:%S %Y GMT"))
        1178668800

    @4kir4 4kir4 mannequin assigned docspython Dec 10, 2013
    @4kir4 4kir4 mannequin added docs Documentation in the Doc dir stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Dec 10, 2013
    @gudge
    Copy link
    Mannequin

    gudge mannequin commented Dec 10, 2013

    Will work on this.
    Please assign the issue to me.

    Instructions before proceeding by Tim Golden(python mailing list):

    Having just glanced at that issue, I would point out that there's been a
    lot of development around the ssl module for the 3.4 release, so you
    definitely want to confirm the issue against the hg tip to ensure it
    still applies.

    @pitrou pitrou removed the docs Documentation in the Doc dir label Dec 10, 2013
    @pitrou
    Copy link
    Member

    pitrou commented Dec 10, 2013

    Indeed the example in the docs is wrong, and so is the current behaviour.

    The example shows "round-tripping" using ssl.cert_time_to_seconds() and then time.ctime(), except that it is bogus as it takes a GMT time and ctime() returns a local time ("""Convert a time expressed in seconds since the epoch to a string representing local time""").

    Still, we should only fix it in 3.4, as code written for prior versions may rely on the current (bogus) behaviour.

    @pitrou
    Copy link
    Member

    pitrou commented Dec 10, 2013

    gudge, your contribution is welcome! If you need guidance about how to write a patch, you can read the developer's guide: http://docs.python.org/devguide/

    Also you will have to sign a contributor's agreement: http://www.python.org/psf/contrib/

    @4kir4
    Copy link
    Mannequin Author

    4kir4 mannequin commented Dec 10, 2013

    gudge,

    There is also an issue with the current strptime format 1 ("%b %d %H:%M:%S %Y GMT"). It is locale-dependent and it may fail if a non-English locale is in effect. I don't know whether I should open a new issue on this or are you going to fix it too.

    cert_time_to_seconds() is documented 2 to parse notBefore, notAfter fields from a certificate. As far as I can tell they do not depend on current locale. Thus the following code should not fail:

        >>> timestr = "May  9 00:00:00 2007 GMT"
        >>> import ssl
        >>> ssl.cert_time_to_seconds(timestr)
        1178661600.0
        >>> import locale
        >>> locale.setlocale(locale.LC_TIME, 'pl_PL.utf8')
        'pl_PL.utf8'
        >>> ssl.cert_time_to_seconds(timestr)
        Traceback (most recent call last):
        ...[snip]...
        ValueError: time data 'May  9 00:00:00 2007 GMT' does not match format '%b %d %H:%M:%S %Y GMT'

    @gudge
    Copy link
    Mannequin

    gudge mannequin commented Dec 19, 2013

    1. Can I get a list of failures. The summary of test results which I compare on my machine.

    -----------------------------------------------------------------------------------------------------

    >>> import ssl
    >>> ssl.cert_time_to_seconds("May  9 00:00:00 2007 GMT")
    1178649000.0
    >>> from datetime import datetime
    >>> datetime.utcfromtimestamp(1178668800)
    datetime.datetime(2007, 5, 9, 0, 0)
    >>> import time
    >>> time.gmtime(1178668800)
    time.struct_time(tm_year=2007, tm_mon=5, tm_mday=9, tm_hour=0, tm_min=0, tm_sec=0, tm_wday=2, tm_yday=129, tm_isdst=0)
    >>> import calender
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    ImportError: No module named 'calender'
    >>> import callendar
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    ImportError: No module named 'callendar'
    >>> import calendar
    >>> calendar.timegm(time.strptime("May 9 00:00:00 2007 GMT", "%b %d %H:%M:%S %Y GMT"))
    1178668800

    I am running a VM on windows host machine.
    In your comment ou have specified:

    >>> import ssl
        >>> ssl.cert_time_to_seconds("May  9 00:00:00 2007 GMT")
        1178694000.0

    It should be 1178668800:

    But I get also get the same answer with the Python build from latest sources?
    Therefore I do not get you?

    1. 3 tests omitted:
      test___all__ test_site test_urllib2net
      348 tests OK.
      3 tests failed:
      test_codecs test_distutils test_ioctl
      2 tests altered the execution environment:
      test___all__ test_site
      33 tests skipped:
      test_bz2 test_codecmaps_cn test_codecmaps_hk test_codecmaps_jp
      test_codecmaps_kr test_codecmaps_tw test_curses test_dbm_gnu
      test_dbm_ndbm test_devpoll test_gzip test_idle test_kqueue
      test_lzma test_msilib test_ossaudiodev test_readline test_smtpnet
      test_socketserver test_sqlite test_ssl test_startfile test_tcl
      test_timeout test_tk test_ttk_guionly test_ttk_textonly
      test_urllibnet test_winreg test_winsound test_xmlrpc_net
      test_zipfile64 test_zlib

    Are these results fine. These results are with no changes.
    How can I make all tests (skipped and omiited pass)

    What about the 3 tests which failed. Are these known failures?

    Now say I have to pull time again to get the latest code. Does it help
    to do a make. Or I will have o do configure again.

    1. I had posted a query on core-metorship? No answers? Not that I am entitled to.

    Thanks

    @gudge
    Copy link
    Mannequin

    gudge mannequin commented Dec 19, 2013

    Sorry I think I did not read msg205774 (1st comment) correctly.
    It clearly says:

    "cert_time_to_seconds() uses time.mktime() [1] to convert utc time tuple to seconds since epoch. mktime() works with local time. It should use calendar.timegm() analog instead."

    So the function cert_time_to_seconds() has to be fixed?

    Thanks

    @pitrou
    Copy link
    Member

    pitrou commented Dec 19, 2013

    So the function cert_time_to_seconds() has to be fixed?

    Yes!

    @gudge
    Copy link
    Mannequin

    gudge mannequin commented Dec 19, 2013

    Patch is uploaded.
    I will also copy paste it.

    I have created the patch with git. Let me know if it is okay with you.
    If it is unacceptable I will try and create one for mercury

    Patch:
    ------------------------------------------------------------------

    diff --combined Doc/library/ssl.rst
    index a6ce5d6,30cb732..0000000
    --- a/Doc/library/ssl.rst
    +++ b/Doc/library/ssl.rst
    @@@ -366,7 -366,7 +366,7 @@@ Certificate handlin
      
           >>> import ssl
           >>> ssl.cert_time_to_seconds("May  9 00:00:00 2007 GMT")
     -     1178694000.0
     +     1178668800 
           >>> import time
           >>> time.ctime(ssl.cert_time_to_seconds("May  9 00:00:00 2007 GMT"))
           'Wed May  9 00:00:00 2007'
    diff --combined Lib/ssl.py
    index f81ef91,052a118..0000000
    --- a/Lib/ssl.py
    +++ b/Lib/ssl.py
    @@@ -852,8 -852,7 +852,8 @@@ def cert_time_to_seconds(cert_time)
          a Python time value in seconds past the epoch."""
      
          import time
     -    return time.mktime(time.strptime(cert_time, "%b %d %H:%M:%S %Y GMT"))
     +    import calendar 
     +    return calendar.timegm(time.strptime(cert_time, "%b %d %H:%M:%S %Y GMT"))
      
      PEM_HEADER = "-----BEGIN CERTIFICATE-----"
      PEM_FOOTER = "-----END CERTIFICATE-----"

    Test Results:
    358 tests OK.
    1 test failed:
    test_compileall
    1 test altered the execution environment:
    test___all__
    28 tests skipped:
    test_codecmaps_cn test_codecmaps_hk test_codecmaps_jp
    test_codecmaps_kr test_codecmaps_tw test_curses test_dbm_gnu
    test_dbm_ndbm test_devpoll test_idle test_kqueue test_lzma
    test_msilib test_ossaudiodev test_smtpnet test_socketserver
    test_sqlite test_startfile test_tcl test_timeout test_tk
    test_ttk_guionly test_ttk_textonly test_urllibnet test_winreg
    test_winsound test_xmlrpc_net test_zipfile64
    ------------------------------------------------------------------------

    Doc changes won't effect the code. The tests would not fail.
    How would I check if the doc changes are coming up fine in the
    final version.

    >>> import ssl
    >>> ssl.cert_time_to_seconds("May  9 00:00:00 2007 GMT")
    1178668800

    I do not have a printer curretly. I will sign the license agreement
    in a few days.

    @pitrou
    Copy link
    Member

    pitrou commented Dec 19, 2013

    Answering to your questions:

    I have created the patch with git. Let me know if it is okay with you.

    Yes, it's ok.
    Also, please don't copy / paste it. Uploading is enough.

    Doc changes won't effect the code. The tests would not fail.
    How would I check if the doc changes are coming up fine in the
    final version.

    The devguide has detailed documentation about how to modify and build
    the documentation :)
    http://docs.python.org/devguide/documenting.html#building-the-documentation

    As for the tests:

    1. for this issue you should probably concentrate on test_ssl: to run it
      in verbose mode, "./python -m test -v test_ssl"
      (please read http://docs.python.org/devguide/runtests.html)

    2. you will need to add a new test to test_ssl, to check that this bug
      is indeed fixed

    @4kir4
    Copy link
    Mannequin Author

    4kir4 mannequin commented Dec 20, 2013

    gudge, have you seen http://bugs.python.org/msg205860 (the locale issue)?

    If you can't fix it; say so, I'll open another issue after this issue is fixed.

    @gudge
    Copy link
    Mannequin

    gudge mannequin commented Dec 22, 2013

    Akira, I will fix it. I will put in the patch in the same bug.

    @gudge
    Copy link
    Mannequin

    gudge mannequin commented Dec 22, 2013

    1. I understand I can run a whole test suite as
      ./python -m test -v test_abc
      as mentioned in
      http://docs.python.org/devguide/runtests.html

    How do I run a particluar test case, like the test I added
    test_cert_time_to_seconds

    1. I have a added a test case test_cert_time_to_seconds to test_ssl.py.

    2. ./python -m test -v test_ssl
      is all PASS.

    3. I will start my work on http://bugs.python.org/issue19940#msg205860.

    4. The patch is attached.

    @gudge
    Copy link
    Mannequin

    gudge mannequin commented Dec 29, 2013

    Can you please provide some hints on how to handle
    http://bugs.python.org/issue19940#msg205860.

    The value of format_regex

    1. Without locale set:
      re.compile('(?P<b>jan|feb|mar|apr|may|jun|jul|aug|sep|oct|nov|dec)\\s+(?P<d>3[0-1]|[1-2]\\d|0[1- 9]|[1-9]| [1-9])\\s+(?P<H>2[0-3]|[0-1]\\d|\\d):(?P<M>[0-5]\\d|\\d):(?P<S>6[0-1]|[0-5]\\d|\\d)\\s +(?P<Y>\\d\\d\\d\\d, re.IGNORECASE)

    2. With locale set:
      re.compile('(?P<b>sty|lut|mar|kwi|maj|cze|lip|sie|wrz|pa\\ź|lis|gru)\\s+(?P<d>3[0-1]|[1-2]\\d|0[ 1-9]|[1-9]| [1-9])\\s+(?P<H>2[0-3]|[0-1]\\d|\\d):(?P<M>[0-5]\\d|\\d):(?P<S>6[0-1]|[0-5]\\d|\\d)\ \s+(?P<Y>\\d\\d\\d\, re.IGNORECASE)

    The value of months are different.

    Thanks

    @4kir4
    Copy link
    Mannequin Author

    4kir4 mannequin commented Feb 23, 2014

    The point of the locale issue is that "notBefore", "notAfter" strings do not change if your locale changes. You don't need a new regex for each locale.

    I've attached ssl_cert_time_seconds.py file that contains example cert_time_to_seconds(cert_time) implementation that fixes both the timezone and the locale issues.

    @pitrou
    Copy link
    Member

    pitrou commented Feb 23, 2014

    Akira, do you want to write a proper patch with tests? If you are interested, you can take a look at http://docs.python.org/devguide/

    You'll also have to sign a contributor's agreement at http://www.python.org/psf/contrib/contrib-form/

    @4kir4
    Copy link
    Mannequin Author

    4kir4 mannequin commented Feb 23, 2014

    Antoine, I've signed the agreement. I've added ssl_cert_time_toseconds.patch with code, tests, and documention updates.

    @pitrou
    Copy link
    Member

    pitrou commented Feb 27, 2014

    Akira, thanks. I have posted a review; if you haven't received the e-mail notification, you can still access it at http://bugs.python.org/review/19940/#ps11142

    @4kir4
    Copy link
    Mannequin Author

    4kir4 mannequin commented Mar 23, 2014

    Antoine, I haven't received the e-mail notification.

    I've replied to the comments on Rietveld.

    Here's the updated patch with the corresponding changes.

    @4kir4
    Copy link
    Mannequin Author

    4kir4 mannequin commented Apr 27, 2014

    Here's a new patch with a simplified ssl.cert_time_to_seconds()
    implementation that brings strptime() back.

    The behaviour is changed:

    • accept both %e and %d strftime formats for days as strptime-based implementation did before
    • return an integer instead of a float (input date has not fractions of a second)

    I've added more tests.

    Please, review.

    @4kir4
    Copy link
    Mannequin Author

    4kir4 mannequin commented Apr 27, 2014

    Replace IndexError with ValueError in the patch because tuple.index raises ValueError.

    @4kir4
    Copy link
    Mannequin Author

    4kir4 mannequin commented Apr 28, 2014

    I've updated the patch:

    • fixed the code example in the documentation to use int instead of
      float result
    • removed assertion on the int returned type (float won't lose precision
      for the practical dates but guaranteeing an integer would be nice)
    • reworded the scary comment
    • removed tests that test the tests

    Ready for review.

    @pitrou
    Copy link
    Member

    pitrou commented Apr 28, 2014

    Thanks for the updated patch, Akira! I'm gonna take a look right now.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 28, 2014

    New changeset 7191c37238d5 by Antoine Pitrou in branch 'default':
    Issue bpo-19940: ssl.cert_time_to_seconds() now interprets the given time string in the UTC timezone (as specified in RFC 5280), not the local timezone.
    http://hg.python.org/cpython/rev/7191c37238d5

    @pitrou
    Copy link
    Member

    pitrou commented Apr 28, 2014

    I've committed the patch. Thank you very much for contributing!

    @pitrou pitrou closed this as completed Apr 28, 2014
    @4kir4
    Copy link
    Mannequin Author

    4kir4 mannequin commented Apr 28, 2014

    Antoine, thank you for reviewing. I appreciate the patience.

    @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
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant