Add support for compress_certificate extension #491

charxhit · 2023-05-23T11:29:21Z

This implements Certificate Compression extension and msg (rfc 8879). Unfortunately, I saw that there was already an open pull request #484 addressing the same a little too late. To avoid repetition, I skimmed through the discussion and implemented relevant feedback. The two main differences in the implementation of the above pull request and this one is that:

We keep class Certificate and class CompressedCertificate separate (instead of the latter being a wrapper around the first)
We handle validation checks in the calling code, not during message parsing

This keeps integration with tlslite-ng relatively simple. To help with the review, here's what the pull request is meant to add:

Support for sending compress_certificate extension as a client
Support for parsing CompressedCertificate msg as a client
Support for parsing compress_certificate extension in CertificateRequest msg
Ability to send a CompressedCertificate msg if the server advertises support for compression

The server equivalent of the above points are not present yet but can be trivially added after initial review. One area that needs attention is the code for CompressedCertificate class, specifically the .create() and decompress() methods (I have added relevant comments explaining the problem). The rfc was a bit vague to me on what exact bytes the encoded compressed certificate msg will have (whether it's type + length + encoded message after decompression or just length + encoded msg), so some changes might be needed there but they should be minimal. For this reason, I have not added tests for CompressedCertificate msg

This change is

tomato42

We keep class Certificate and class CompressedCertificate separate (instead of the latter being a wrapper around the first)

this will make it not transparent and complicate the rest of the code

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 2 of 2 files at r6, 1 of 1 files at r7, 5 of 5 files at r8, 1 of 1 files at r9, 2 of 2 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @charxhit)

-- commits line 23 at r8:
fixup commit, so should be squashed

tlslite/constants.py line 192 at r2 (raw file):

    zstd = 3
    all = (1, 2, 3)
    default = (1,)

we generally don't create such values and they will interact badly with enum handling code of extensions

Code quote:

    all = (1, 2, 3)
    default = (1,)

tlslite/extensions.py line 2103 at r3 (raw file):

class CompressCertificateExtension(TLSExtension):

why it's not a VarListExtension ?

tlslite/extensions.py line 2124 at r3 (raw file):

        if not all([algo in CompressionAlgorithms.all for algo in self.advertised_algorithms]):
            raise TLSInternalError("Unknown algorithm provided for certificate compression")

for tlsfuzzer we generally want the ability to send unassigned values, making sure that the user can't set invalid values should happen like 2 API levels higher (in HandshakeSettings)

tlslite/extensions.py line 2148 at r3 (raw file):

TLSExtension._universalExtensions = \

why CompressCertificateExtension is not added here?

tlslite/extensions.py line 2116 at r8 (raw file):

            advertised_algorithms: an iterable of integers denoting which algorithms the peer
                                   supports for compression
        """

that's not correct Sphinx syntax for param docs

tlslite/extensions.py line 2120 at r9 (raw file):

        # generic code allows empty, this ext does not
        if not parser.getRemainingLength():
            raise DecodeError("Empty payload in extension")

That's not what I see in the VarListExtension...

tlslite/extensions.py line 2130 at r9 (raw file):

                                   supports for compression
        """
        return super(CompressCertificateExtension, self).create(advertised_algorithms)

why have it in the first place then?

tlslite/handshakesettings.py line 384 at r4 (raw file):

        # certificate compression extensions
        self.use_certificate_compression = False
        self.certificate_compression_algorithms = CompressionAlgorithms.default

why split use and list of allowed? why an empty list can't mean "don't use"?

tlslite/handshakesettings.py line 616 at r4 (raw file):

        if other.use_certificate_compression not in (True, False):
            raise ValueError("use_heartbeat_extension must be True or False")

it's no heartbeat

tlslite/handshakesettings.py line 628 at r10 (raw file):

        if other.use_certificate_compression not in (True, False):
            raise ValueError("use_certificate_compression must be True or False")

squash fixup commits, don't add them

tlslite/messages.py line 24 at r5 (raw file):

from .extensions import *
from .utils.format_output import none_as_unknown
from utils import compression

use relative imports

tlslite/messages.py line 24 at r7 (raw file):

from .extensions import *
from .utils.format_output import none_as_unknown
from .utils import compression

fixup commits need to be squashed as CI verifies that the tests pass after every commit

tlslite/messages.py line 1375 at r8 (raw file):

        Args:
            chosen_algorithm - An int denoting an algorithm to be used to create the compressed msg
            certificate_message - Object of class messages.Certificate whose data will be encoded

not valid Sphinx syntax

tlslite/tlsconnection.py line 1281 at r6 (raw file):

        certificate_request = None
        certificate = None
        compress_cert_ext = clientHello.getExtension(ExtensionType.compress_certificate)

no? The extension is negotiated when the server sends the extension, not when the client advertised support for it

tlslite/tlsconnection.py line 1469 at r6 (raw file):

            # Problem: it's unclear whether the rfc wants to us to send an alert incase certificate_verify message
            # contains a compress_certificate extension if we did not advertise one. For now, do nothing.

extensions in ClientHello and CertificateRequest are completely independent, so yes, server can send request for compression if we didn't advertise

tlslite/tlsconnection.py line 1475 at r6 (raw file):

                # Certificate message instead
                for requested_algo in verify_compress_cert_ext.advertised_algorithms:
                    if requested_algo in hello_compress_cert_ext.advertised_algorithms:

client hello algorithms are irrelevant for this

tlslite/utils/compression.py line 35 at r8 (raw file):

    Raises:
        KeyError: if provided algorithm integer is unknown
    """

wrong Sphinx syntax

tlslite/utils/compression.py line 48 at r8 (raw file):

        BadCertificateError: if decompression failed
        ValueError: if chosen_algorithm is unknown
    """

wrong Sphinx syntax

tlslite/utils/compression.py line 77 at r8 (raw file):

        BadCertificateError: if compression failed
        ValueError: if chosen_algorithm is unknown
    """

wrong Sphinx syntax

unit_tests/test_tlslite_extensions.py line 1962 at r11 (raw file):

class TestCompressCertificateExtension(unittest.TestCase):

code and test cases for it should be added in the same patch

charxhit

Clarification: CompressedCertificate still depends on Certificate class to make sense of the compressed message, but unlike in the previous pull request, it doesn't attempt to act as a proxy to it. Can you give an example of a possible complication here so I understand what you are referring to?

Reviewable status: 3 of 9 files reviewed, 21 unresolved discussions (waiting on @tomato42)

-- commits line 23 at r8: