Post handshake KeyUpdate support

[NoName115]

Support for handling KeyUpdate post-handshake msg.

fixes #210

---

This change is 

[tomato42]

This pull request introduces 2 alerts and fixes 7 when merging b84a30e into 0294251 - view on LGTM.com

new alerts:

• 1 for __eq__ not overridden when adding attributes
• 1 for Syntax error

fixed alerts:

• 6 for 'import *' may pollute namespace
• 1 for Testing equality to None

---

Comment posted by LGTM.com

[tomato42]

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @NoName115)

---

tlslite/messages.py, line 2324 at r1 (raw file):

    def parse(self, p):
        """Deserialize keyupdate message from parser."""
        self.message_type = p.get(1)

it should check if it's not longer than 1 byte

---

tlslite/tlsrecordlayer.py, line 1088 at r3 (raw file):

            # Calculate new keys
        elif request.message_type == KeyUpdateMessageType.update_requested:
            # Calculate new keys

calculating new receive keys is common between handling of those types, while calculating send keys should happen after the message was sent

---

tlslite/tlsrecordlayer.py, line 1093 at r3 (raw file):

                KeyUpdateMessageType.update_not_requested)
            for _ in self._sendMsg(keyupdate_request):
                pass

it needs to be a generator (use yield) and the method needs to be called as generator (for result in self._handle_keyupdate_request()...

---

tlslite/tlsrecordlayer.py, line 1095 at r3 (raw file):

                pass
        else:
            for _ in self._sendError(\

unnecessary newline escape inside parentheses

[tomato42]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @NoName115)

---

tlslite/tlsrecordlayer.py, line 1086 at r3 (raw file):

        """
        if request.message_type == KeyUpdateMessageType.update_not_requested:
            # Calculate new keys

see https://tools.ietf.org/html/rfc8446#section-7.2 and https://tools.ietf.org/html/rfc8446#section-7.3 for relevant algorithm

[NoName115]

Updated.

[tomato42]

This pull request introduces 1 alert when merging 8cd0e17 into 0294251 - view on LGTM.com

new alerts:

• 1 for __eq__ not overridden when adding attributes

---

Comment posted by LGTM.com

[NoName115]

Updated.

[tomato42]

This pull request introduces 1 alert when merging 89f3b8a into 0294251 - view on LGTM.com

new alerts:

• 1 for __eq__ not overridden when adding attributes

---

Comment posted by LGTM.com

[tomato42]

Reviewed 2 of 2 files at r4, 2 of 2 files at r5.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @NoName115)

---

tlslite/recordlayer.py, line 1318 at r5 (raw file):

            except AttributeError:
                state.key['server application traffic secret'] = \
                    new_sr_app_secret

why that try ... except is needed?

---

tlslite/recordlayer.py, line 1424 at r5 (raw file):

                                                       iv_length,
                                                       prf_name)
            self._writeState = serverState

1. isn't the calculation identical for client and server, with the only difference being what is the original secret and where it is saved after it is updated and which connection state is updated?
2. it's basically the same as in calcTLS1_3PendingState? (as in, there could be a method called by both of them and the only difference being where the resulting ConectionState object is saved?

---

tlslite/tlsrecordlayer.py, line 1086 at r5 (raw file):

        @param request: Recieved KeyUpdate message.
        """
        if request.message_type == KeyUpdateMessageType.update_not_requested:

I wonder if it wouldn't be cleaner (less code duplication) to:

1. check if the message_type is one of update_requested and update_not_requested, if not, send illegal_paramter
2. update the sender keys
3. if message_type is update_requested, send key update of our own, update receiver keys

[NoName115]

Fixed.

[tomato42]

This pull request introduces 1 alert when merging 7f889c2 into 0294251 - view on LGTM.com

new alerts:

• 1 for __eq__ not overridden when adding attributes

---

Comment posted by LGTM.com

[tomato42]

This pull request introduces 1 alert when merging d810043 into 0294251 - view on LGTM.com

new alerts:

• 1 for __eq__ not overridden when adding attributes

---

Comment posted by LGTM.com

[tomato42]

Reviewed 3 of 3 files at r6, 2 of 2 files at r7.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @NoName115)

---

tlslite/recordlayer.py, line 1364 at r7 (raw file):

                                                    b"iv", b"",
                                                    iv_length,
                                                    prf_name)

isn't this calculation identical for client and server side?

---

tlslite/recordlayer.py, line 1373 at r7 (raw file):

        if self.client:
            self._readState = srState
            return cl_app_secret, new_sr_app_secret

so the clState is thrown out, after we paid the price of deriving keys and initialising AES, Chacha and GCM or Poly1305?

also, the clState name is camelCase

---

tlslite/tlsrecordlayer.py, line 1153 at r7 (raw file):

                for result in self._sendMsg(keyupdate_request):
                    yield result
                self.cl_app_secret, self.sr_app_secret = self._recordLayer.\

why it's not updating the session? won't that make it derive the same keys over and over?

[tomato42]

This pull request introduces 1 alert when merging 637c5c6 into 0294251 - view on LGTM.com

new alerts:

• 1 for __eq__ not overridden when adding attributes

---

Comment posted by LGTM.com

[tomato42]

Reviewed 2 of 2 files at r8.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @NoName115)

a discussion (no related file):
how a user can ask for sending a KeyUpdate?

---

[NoName115]

updated.

[tomato42]

This pull request introduces 2 alerts when merging 3334691 into 0294251 - view on LGTM.com

new alerts:

• 1 for __eq__ not overridden when adding attributes
• 1 for Unused local variable

---

Comment posted by LGTM.com

[tomato42]

This pull request introduces 1 alert when merging 64761b1 into 0294251 - view on LGTM.com

new alerts:

• 1 for __eq__ not overridden when adding attributes

---

Comment posted by LGTM.com

[tomato42]

Reviewed 1 of 1 files at r9.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @NoName115)

a discussion (no related file):
looks good, but I won't merge it just yet as the test coverage is missing (but like I said before, please focus on tlsfuzzer test cases for now)

---

[tomato42]

This pull request introduces 1 alert when merging 22ae125 into 849f1d1 - view on LGTM.com

new alerts:

• 1 for __eq__ not overridden when adding attributes

---

Comment posted by LGTM.com

[lgtm-com]

This pull request introduces 1 alert when merging 22f1887 into 75dbe54 - view on LGTM.com

new alerts:

• 1 for __eq__ not overridden when adding attributes

[lgtm-com]

This pull request introduces 1 alert when merging b057a7d into 3d63ee4 - view on LGTM.com

new alerts:

• 1 for __eq__ not overridden when adding attributes

[lgtm-com]

This pull request introduces 1 alert when merging 4b1dcdd into 322f5ff - view on LGTM.com

new alerts:

• 1 for __eq__ not overridden when adding attributes

[lgtm-com]

This pull request introduces 2 alerts when merging ce04342 into 322f5ff - view on LGTM.com

new alerts:

• 1 for __eq__ not overridden when adding attributes
• 1 for Unused import

[lgtm-com]

This pull request introduces 2 alerts when merging 3960672 into 322f5ff - view on LGTM.com

new alerts:

• 1 for __eq__ not overridden when adding attributes
• 1 for Unused import

[tomato42]

The issue returned by LGTM is a false positive: github/codeql#2408

[lgtm-com]

This pull request introduces 1 alert when merging a65c36d into 322f5ff - view on LGTM.com

new alerts:

• 1 for __eq__ not overridden when adding attributes

[tomato42]

remaining test coverage is provided by tlsfuzzer/tlsfuzzer#501

[tomato42]

Reviewed 5 of 5 files at r10, 3 of 3 files at r11, 1 of 1 files at r12, 1 of 1 files at r13.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @NoName115 and @The-Mule)

a discussion (no related file):

Previously, tomato42 (Hubert Kario) wrote…

looks good, but I won't merge it just yet as the test coverage is missing (but like I said before, please focus on tlsfuzzer test cases for now)

ok, test coverage added, but now somebody else will have to review it

---

[The-Mule]

Reviewed 5 of 5 files at r10, 3 of 3 files at r11, 1 of 1 files at r12, 1 of 1 files at r13.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @NoName115)

[The-Mule]

I checked test coverage only. The rest was already reviewed. AFAIK all changes requested were already addressed and this can be merged, finally.