Add key logging for TLS 1.3 and TLS 1.2 (#523)

[nickrabbott]

SSLKEYLOGFILE support

fixes #523

---

This change is 

[tomato42]

(as the CI verifies that tests can be executed after every commit, you will have to squash/fixup the commits together)

[nickrabbott]

@tomato42 considering how people may use this library, this may not be process or thread safe. I'm thinking about changing this to use logger class that can be passed to the TLSConnection's init which can handle IO in a safer way. Also taking into consideration your comments

[tomato42]

@tomato42 considering how people may use this library, this may not be process or thread safe. I'm thinking about changing this to use logger class that can be passed to the TLSConnection's init which can handle IO in a safer way. Also taking into consideration your comments

yes, we can have two TLSConnections running in parallel with the same settings, so we should at least try to not overwrite the other's writes, but I don't think locking on process level will be enough... I'm afraid that OS level thing will need to be used...

[nickrabbott]

I'm afraid that OS level thing will need to be used...

@tomato42 supporting all common OSes like Linux, macOS, and Windows? Fortunately, for posix it should be the same using fcntl

[tomato42]

I'm afraid that OS level thing will need to be used...

@tomato42 supporting all common OSes like Linux, macOS, and Windows? Fortunately, for posix it should be the same using fcntl

sigh yeah, we probably should just give up and simply open the file in append mode, dump our stuff as quickly as possible, and close it; then document that it is not thread-safe and intended for debugging only... 😔

[nickrabbott]

sigh yeah, we probably should just give up and simply open the file in append mode, dump our stuff as quickly as possible, and close it; then document that it is not thread-safe and intended for debugging only... 😔

I have it working for POSIX using fnctl with a unit test (thread testing only) and smoke tested both thread + file locking and haven't had issues. Pushing soon. As far as windows I'll need to figure something out... I don't have a windows machine. That or we can for now just dump it in append mode and document as not being implemented for windows

[tomato42]

That or we can for now just dump it in append mode and document as not being implemented for windows

yes, let's do that; I don't have a windows machine to test it on either

[tomato42]

Reviewed 1 of 3 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @nickrabbott)

---

-- commits line 2 at r2:
"WIP"?

---

tlslite/tlsconnection.py line 90 at r2 (raw file):

        :param logging_enabled: Enable logging session secrets to the SSLKEYLOGFILE env variable.
        :type logging_enabled: bool

Why have it controlled by two settings at once?

presence of environment variable should be enough; with the ability to override it by setting the name of the file to log to, not if the logging should work

---

tlslite/session.py line 102 at r2 (raw file):

        self.ec_point_format = 0
        self.cl_handshake_traffic_secret = None
        self.sr_handshake_traffic_secret = None

they are not read anywhere?

---

tlslite/sslkeylogging.py line 26 at r2 (raw file):

class SSLKeyLogger:
    """
    Write session secrets to the SSLKEYLOGFILE environment variable. Implemented to be thread-safe at the class

"...to the file pointed to by SSLKEYLOGFILE..."

---

tlslite/sslkeylogging.py line 50 at r2 (raw file):

            return

        if isinstance(keys, tuple):

I don't like this... there's no functional difference between passing the values to log in tuples vs lists...

honestly I think a cleaner check would be for length of the iterable being 3 and first element being str...

[nickrabbott]

"WIP"?

yeah I wasn't ready to say it was done yet and still had some tests I wanted to add. "WIP" has been removed

Why have it controlled by two settings at once?

presence of environment variable should be enough; with the ability to override it by setting the name of the file to log to, not if the logging should work

The thought here was to have a way to opt-out of the logging, but as you say the presence of the env variable should be enough. That change is in effect

they are not read anywhere?

        self.ec_point_format = 0
        self.cl_handshake_traffic_secret = None
        self.sr_handshake_traffic_secret = None

I added these to the session instance so I could assert their values in the log when the tests used real sockets. They're in use now that I have a test using socketpair

I don't like this... there's no functional difference between passing the values to log in tuples vs lists...

honestly I think a cleaner check would be for length of the iterable being 3 and first element being str...

The reason I made this method accept both a tuple and a list of tuples is because 1.2 only has one to log, but 1.3 has many. I understand not liking that, so it's probably best to just have the caller provide a list of tuples, even if the list only contains one (in the case of 1.2). That makes a check not necessary since the purpose of the check was whether the type was a tuple or list

[tomato42]

Reviewed 1 of 4 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nickrabbott)

---

tlslite/sslkeylogging.py line 49 at r4 (raw file):

            self.lock_write = unsafe_write

    def log_session_keys(self, keys):

doc string missing

[tomato42]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nickrabbott)

[tomato42]

@nickrabbott looks like there is still some code missing, could you take a look at the test failures?

[nickrabbott]

@tomato42 I see, it's an issue with the SRP fault tests. In these tests the handshake intentionally aborts, so self.session is None, causing "NoneType object has no attribute..." error when this code attempts to log self.session.masterSecret. I added a check that the session object is created before logging and all the tests pass

I have one question before pushing the fix. After the tests passed locally, I'm seeing issues with the pylint and diff-quality stage of the pipeline. It could just be an issue with my local environment... Are those steps expected to pass locally before pushing, or are they primarily intended in CI?

[tomato42]

I'm seeing issues with the pylint and diff-quality stage of the pipeline. It could just be an issue with my local environment...

those are guidelines that you might have insufficient test coverage for the changes, they are not blocking for the PR

[nickrabbott]

I'm on a mac and ".db" was the file extension

[nickrabbott]

@tomato42 I saw you approved, but when I pushed the fix for the test that dismissed the approval. Is there anything else needed?