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

Please support logging of SSL master secret by env variable SSLKEYLOGFILE #78452

Closed
jmfrank63 mannequin opened this issue Jul 29, 2018 · 13 comments
Closed

Please support logging of SSL master secret by env variable SSLKEYLOGFILE #78452

jmfrank63 mannequin opened this issue Jul 29, 2018 · 13 comments
Assignees
Labels
3.8 only security fixes topic-SSL type-feature A feature request or enhancement

Comments

@jmfrank63
Copy link
Mannequin

jmfrank63 mannequin commented Jul 29, 2018

BPO 34271
Nosy @tiran, @njsmith, @dimaqq, @mhsmith, @yan12125, @jmfrank63
PRs
  • bpo-34271: Add ssl debugging helpers (GH-10031) #10031
  • bpo-34271: Fix compatibility with 1.0.2 (GH-13728) #13728
  • Files
  • pycurl-get.py: Working example with pycurl module compiled against openssl 1.1.0h
  • 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 = 'https://github.com/tiran'
    closed_at = <Date 2021-04-17.19:36:53.473>
    created_at = <Date 2018-07-29.12:38:09.130>
    labels = ['expert-SSL', 'type-feature', '3.8']
    title = 'Please support logging of SSL master secret by env variable SSLKEYLOGFILE'
    updated_at = <Date 2021-04-17.19:36:53.473>
    user = 'https://github.com/jmfrank63'

    bugs.python.org fields:

    activity = <Date 2021-04-17.19:36:53.473>
    actor = 'christian.heimes'
    assignee = 'christian.heimes'
    closed = True
    closed_date = <Date 2021-04-17.19:36:53.473>
    closer = 'christian.heimes'
    components = ['SSL']
    creation = <Date 2018-07-29.12:38:09.130>
    creator = 'jmfrank63'
    dependencies = []
    files = ['47719']
    hgrepos = []
    issue_num = 34271
    keywords = ['patch']
    message_count = 13.0
    messages = ['322632', '326427', '326429', '326430', '326431', '328219', '328221', '332181', '332196', '344050', '344459', '362992', '363007']
    nosy_count = 8.0
    nosy_names = ['christian.heimes', 'jonozzz', 'njs', 'sascha_silbe', 'Dima.Tisnek', 'Malcolm Smith', 'yan12125', 'jmfrank63']
    pr_nums = ['10031', '13728']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue34271'
    versions = ['Python 3.8']

    @jmfrank63
    Copy link
    Mannequin Author

    jmfrank63 mannequin commented Jul 29, 2018

    As discussed on the EuroPython 2018 it would be a great improvement if the python SSL module would respect the SSLKEYLOGFILE environment variable to log the master secret and the client random for packet trace decryption.

    The pycurl module compiled against libopenssl 1.1.0h does already work.

    OpenSSL 1.1.1 will offer to register a callback that will log the keys.

    There is also c code available using LD_PRELOAD here:

    https://git.lekensteyn.nl/peter/wireshark-notes/tree/src/sslkeylog.c

    It would be great if a call to the requests, aiohttp, urllib3 or asks library would lead to the keys logged if the environment variable is set from within python.

    Thank you

    @jmfrank63 jmfrank63 mannequin added the 3.7 (EOL) end of life label Jul 29, 2018
    @jmfrank63 jmfrank63 mannequin assigned tiran Jul 29, 2018
    @jmfrank63 jmfrank63 mannequin added topic-SSL type-feature A feature request or enhancement labels Jul 29, 2018
    @jmfrank63 jmfrank63 mannequin changed the title Please support logging of SSL master secret by env variable SSLKEYLOGFILe Please support logging of SSL master secret by env variable SSLKEYLOGFILE Jul 29, 2018
    @njsmith
    Copy link
    Contributor

    njsmith commented Sep 26, 2018

    I didn't know this, but apparently the SSLKEYLOGFILE envvar is a de-facto standard: chrome, firefox, and libcurl all check for this envvar, and if found they log TLS secrets to the file in a specific format.

    Reports of projects supporting this:

    Also, people are using gross ctypes hacks to convince Python to do this too: https://github.com/joernheissler/SslMasterKey

    Also, now that I know this exists I kind of wish it was supported because I've been frustrated by this problem before myself :-).

    My first thought was that the ssl module should provide methods to extract the various secret values (e.g., wrappers for SSL_SESSION_get_master_key and SSL_get_client_random), and leave the environment variable checking to user code. But... looking at the file format docs:

    https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Key_Log_Format

    ...it appears that TLS 1.3 has more and different secrets than previous versions, and trying to expose all these different pieces seems pretty messy. If we simply implement SSLKEYLOGFILE, that would give people what they want, and since we would be writing it out ourselves we could make it handle different TLS versions internally without exposing that complexity as part of the API.

    We would of course have to disable this if -E was passed on the command line.

    As an FYI to anyone looking at this bug, Christian (the main ssl module maintainer) is generally *very* overloaded, so I would say that the chances of this actually being implemented go *way* up if someone puts together a PR.

    @tiran
    Copy link
    Member

    tiran commented Sep 26, 2018

    Cory contributed a high level API for OpenSSL 1.1.1, https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_keylog_callback.html I already have a patch somewhere on my disk. The patch is trivial if we ignore OpenSSL < 1.1.1.

    @tiran
    Copy link
    Member

    tiran commented Sep 26, 2018

    Here is a horribly hacky and simple implementation. I have a more elaborate implementation that does correct locking and has no global state.

    static BIO *bio_keylog = NULL;

    static void keylog_callback(const SSL *ssl, const char *line)
    {
        BIO_printf(bio_keylog, "%s\n", line);
        (void)BIO_flush(bio_keylog);
    }
    
    int PySSL_set_keylog_file(SSL_CTX *ctx, const char *keylog_file)
    {
        /* Close any open files */
        BIO_free_all(bio_keylog);
        bio_keylog = NULL;
    
        if (ctx == NULL || keylog_file == NULL) {
            /* Keylogging is disabled, OK. */
            return 0;
        }
    
        /*
         * Append rather than write in order to allow concurrent modification.
         * Furthermore, this preserves existing keylog files which is useful when
         * the tool is run multiple times.
         */
        bio_keylog = BIO_new_file(keylog_file, "a");
        if (bio_keylog == NULL) {
            BIO *b = BIO_new_fp(stderr, BIO_NOCLOSE | BIO_FP_TEXT);
            BIO_printf(b, "Error writing keylog file %s\n", keylog_file);
            BIO_free_all(b);
            return 1;
        }
    
        /* Write a header for seekable, empty files (this excludes pipes). */
        if (BIO_tell(bio_keylog) == 0) {
            BIO_puts(bio_keylog,
                     "# SSL/TLS secrets log file, generated by OpenSSL\n");
            (void)BIO_flush(bio_keylog);
        }
        SSL_CTX_set_keylog_callback(ctx, keylog_callback);
        return 0;
    }

    @tiran tiran added 3.8 only security fixes and removed 3.7 (EOL) end of life labels Sep 26, 2018
    @jmfrank63
    Copy link
    Mannequin Author

    jmfrank63 mannequin commented Sep 26, 2018

    Hi Christian
    I would be willing to give this a try, could you publish or send me that
    more elaborate code?
    Thanks Johannes

    On Wed, 26 Sep 2018 at 09:25, Christian Heimes <report@bugs.python.org>
    wrote:

    Christian Heimes <lists@cheimes.de> added the comment:

    Here is a horribly hacky and simple implementation. I have a more
    elaborate implementation that does correct locking and has no global state.

    static BIO *bio_keylog = NULL;

    static void keylog_callback(const SSL *ssl, const char *line)
    {
    BIO_printf(bio_keylog, "%s\n", line);
    (void)BIO_flush(bio_keylog);
    }

    int PySSL_set_keylog_file(SSL_CTX *ctx, const char *keylog_file)
    {
    /* Close any open files */
    BIO_free_all(bio_keylog);
    bio_keylog = NULL;

    if (ctx == NULL || keylog_file == NULL) {
        /* Keylogging is disabled, OK. \*/
        return 0;
    }
    
    /*
     * Append rather than write in order to allow concurrent modification.
     * Furthermore, this preserves existing keylog files which is useful
    

    when
    * the tool is run multiple times.
    */
    bio_keylog = BIO_new_file(keylog_file, "a");
    if (bio_keylog == NULL) {
    BIO *b = BIO_new_fp(stderr, BIO_NOCLOSE | BIO_FP_TEXT);
    BIO_printf(b, "Error writing keylog file %s\n", keylog_file);
    BIO_free_all(b);
    return 1;
    }

    /* Write a header for seekable, empty files (this excludes pipes). \*/
    if (BIO_tell(bio_keylog) == 0) {
        BIO_puts(bio_keylog,
                 "# SSL/TLS secrets log file, generated by OpenSSL\\n");
        (void)BIO_flush(bio_keylog);
    }
    SSL_CTX_set_keylog_callback(ctx, keylog_callback);
    return 0;
    

    }

    ----------
    stage: -> needs patch
    versions: +Python 3.8 -Python 3.7


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue34271\>


    @tiran
    Copy link
    Member

    tiran commented Oct 21, 2018

    Nathaniel, I created a PR with keylog and message callback patch. The message callback is really useful to investigate handshake and trace messages like close alert.

    I decided against making the key log feature a callback and rather make it a filename attribute. In almost all case users want to log to a file anz way. It's easier to implement and makes locking simpler, too.

    @jmfrank63
    Copy link
    Mannequin Author

    jmfrank63 mannequin commented Oct 21, 2018

    Hello Christian,

    much appreciated. Thank you so much.

    Johannes

    @dimaqq
    Copy link
    Mannequin

    dimaqq mannequin commented Dec 20, 2018

    Perhaps https://stackoverflow.com/questions/42332792/chrome-not-firefox-are-not-dumping-to-sslkeylogfile-variable is outdated, but it suggests that:

    in firefox, this feature os not on by default

    in chrome, this feature is not available

    I would be vary of "too much magic"... Though I'd use this in development, I feel that's a bit risky for desktop apps, production, etc...

    @njsmith
    Copy link
    Contributor

    njsmith commented Dec 20, 2018

    It's confusing, but AFAICT what happened is that Mozilla started to disable it in release builds, but got a bunch of pushback from users and changed their minds and decided to keep it enabled. But then there was a snafu tracking the patch for that, so there ended up being a few releases where it was disabled, before everything got sorted out. But, it is enabled by default now. The very confusing thread is here: https://bugzilla.mozilla.org/show_bug.cgi?id=1188657

    And I don't see how this is any riskier than other envvars like PYTHONSTARTUP, which lets you name an arbitrary file that the interpreter will execute at startup.

    @tiran
    Copy link
    Member

    tiran commented May 31, 2019

    New changeset c7f7069 by Christian Heimes in branch 'master':
    bpo-34271: Add ssl debugging helpers (GH-10031)
    c7f7069

    @tiran
    Copy link
    Member

    tiran commented Jun 3, 2019

    New changeset e35d1ba by Christian Heimes in branch 'master':
    bpo-34271: Fix compatibility with 1.0.2 (GH-13728)
    e35d1ba

    @mhsmith
    Copy link
    Mannequin

    mhsmith mannequin commented Feb 29, 2020

    It looks like this has now been done and released. Can the issue be closed?

    @jmfrank63
    Copy link
    Mannequin Author

    jmfrank63 mannequin commented Feb 29, 2020

    Yes, I didn't revisit the issue since, but Malcolm is right. Implemented in
    python 3.8.

    Thanks to all the contributors.

    On Sat, Feb 29, 2020 at 8:58 AM Malcolm Smith <report@bugs.python.org>
    wrote:

    Malcolm Smith <malcolm.smith@gmail.com> added the comment:

    It looks like this has now been done and released. Can the issue be closed?

    ----------
    nosy: +Malcolm Smith


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue34271\>


    @tiran tiran closed this as completed Apr 17, 2021
    @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
    3.8 only security fixes topic-SSL type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants