Draft: add support for RFC 8879 TLS Certificate Compression

[t184256]

I'm sketching up RFC 8879 TLS Certificate Compression support. My intent is to use it with tlsfuzzer to test gnutls' implementation of it.

---

This change is 

[tomato42]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @t184256)

---

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

class CompressedCertificate(Certificate):

Why it's a child class of Certificate?

---

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

    def __init__(self, certificateType, version=(3, 4),
                 algorithm=CertificateCompressionAlgorithm.zlib):
        assert version > (3, 3)  # RFC 8879 is TLS 1.3+ only

1. That's not how in tlslite-ng the limitation of protocol version is handled
2. this means it will not be usable for tlsfuzzer, to test that it ineed is ignored when used in TLS 1.2 and earlier
3. the version in messages is generally used when the format of the message changes between protocol versions; here there's just one format of the message...

---

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

        assert version > (3, 3)  # RFC 8879 is TLS 1.3+ only
        super(CompressedCertificate, self).__init__(certificateType, version)
        HandshakeMsg.__init__(self, HandshakeType.compressed_certificate)

to me it shows that using Certificate as parent class is a bad idea...

---

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

    def parse(self, parser):
        assert self.version > (3, 3)  # RFC 8879 is TLS 1.3+ only

same as above

---

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

        # FIXME: why am I peeling length here? is this the right place?
        inner_parser = Parser(parser.getVarBytes(3))

it should be startLengthCheck(3)

---

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

    def write(self):
        assert self.version > (3, 3)  # RFC 8879 is TLS 1.3+ only

same as above

---

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

    def __repr__(self):
        assert self.version > (3, 3)  # RFC 8879 is TLS 1.3+ only

same as above

---

tlslite/tlsconnection.py line 4117 at r1 (raw file):

            if self.version == (3,0):
                handshake_types = (HandshakeType.certificate,
                                   HandshakeType.compressed_certificate)

this is specifically parsing for SSL3.0, we don't want to support this message there...

---

tlslite/tlsconnection.py line 4142 at r1 (raw file):

            elif self.version in ((3,1), (3,2), (3,3)):
                handshake_types = (HandshakeType.certificate,
                                   HandshakeType.compressed_certificate)

same here, this is for parsing TLS 1.0, 1.1 and 1.2, for which compressed_certificate is not permitted

[t184256]

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

---

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

Previously, tomato42 (Hubert Kario) wrote…

Why it's a child class of Certificate?

Replaced with encapsulation.

---

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

Previously, tomato42 (Hubert Kario) wrote…

1. That's not how in tlslite-ng the limitation of protocol version is handled
2. this means it will not be usable for tlsfuzzer, to test that it ineed is ignored when used in TLS 1.2 and earlier
3. the version in messages is generally used when the format of the message changes between protocol versions; here there's just one format of the message...

Removed.

---

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

Previously, tomato42 (Hubert Kario) wrote…

to me it shows that using Certificate as parent class is a bad idea...

Replaced with encapsulation.

---

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

Previously, tomato42 (Hubert Kario) wrote…

same as above

Removed.

---

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

Previously, tomato42 (Hubert Kario) wrote…

it should be startLengthCheck(3)

Used startLengthCheck.

---

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

Previously, tomato42 (Hubert Kario) wrote…

same as above

Removed.

---

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

Previously, tomato42 (Hubert Kario) wrote…

same as above

Removed.

---

tlslite/tlsconnection.py line 4117 at r1 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

this is specifically parsing for SSL3.0, we don't want to support this message there...

Reverted.

---

tlslite/tlsconnection.py line 4142 at r1 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

same here, this is for parsing TLS 1.0, 1.1 and 1.2, for which compressed_certificate is not permitted

Reverted.

[lgtm-com]

This pull request introduces 1 alert when merging 13570ad into dabc18f - view on LGTM.com

new alerts:

• 1 for `__eq__` not overridden when adding attributes

[lgtm-com]

This pull request introduces 1 alert when merging 52edcf7 into dabc18f - view on LGTM.com

new alerts:

• 1 for `__eq__` not overridden when adding attributes

[lgtm-com]

This pull request introduces 1 alert when merging 0ade564 into dabc18f - view on LGTM.com

new alerts:

• 1 for `__eq__` not overridden when adding attributes

[tomato42]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @t184256)

---

tlslite/messages.py line 1292 at r2 (raw file):

        self.algorithm = parser.get(2)
        self.uncompressed_length = parser.get(3)
        compressed_certificate_msg = parser.getFixBytes(parser.get(3))

why not getVarBytes(3)?

---

tlslite/messages.py line 1296 at r2 (raw file):

        if self.algorithm == CertificateCompressionAlgorithm.zlib:
            import zlib

isn't zlib built-in, thus, should always be present?

[t184256]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tomato42)

---

tlslite/messages.py line 1296 at r2 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

isn't zlib built-in, thus, should always be present?

It is. I've put it here to only import as needed and prevent exposing it as tlslite.messages.zlib. Do you want it as an import on the top level?

[t184256]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tomato42)

---

tlslite/messages.py line 1292 at r2 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

why not getVarBytes(3)?

No good reason, changed.

[lgtm-com]

This pull request introduces 1 alert when merging 0d4cdd4 into dabc18f - view on LGTM.com

new alerts:

• 1 for `__eq__` not overridden when adding attributes

[tomato42]

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

a discussion (no related file):
So now we're missing integration with HandshakeSettings, sending the extension, and sending of the compressed cert if peer advertised support for compression...

---

---

tlslite/messages.py line 1296 at r2 (raw file):

Previously, t184256 (Alexander Sosedkin) wrote…

It is. I've put it here to only import as needed and prevent exposing it as tlslite.messages.zlib. Do you want it as an import on the top level?

hmm, while true, I don't think we generally gain anything by importing it locally, while losing on readability, by not having all imports in one place, as is the common practice and a PEP8 recommendation...

[t184256]

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

a discussion (no related file):

Previously, tomato42 (Hubert Kario) wrote…

So now we're missing integration with HandshakeSettings, sending the extension, and sending of the compressed cert if peer advertised support for compression...

Here's a different iteration first: support for multiple compression algorithms and xyzLoaded-style API for feature detection. =)

---

---

tlslite/messages.py line 1296 at r2 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

hmm, while true, I don't think we generally gain anything by importing it locally, while losing on readability, by not having all imports in one place, as is the common practice and a PEP8 recommendation...

Now imported in a separate .utils.compression module.

[lgtm-com]

This pull request introduces 6 alerts when merging 22530e0 into dabc18f - view on LGTM.com

new alerts:

• 4 for Unused import
• 1 for `__eq__` not overridden when adding attributes
• 1 for 'import *' may pollute namespace

[tomato42]

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @t184256)

---

tlslite/messages.py line 1299 at r4 (raw file):

        elif self.algorithm == CertificateCompressionAlgorithm.brotli:
            if not brotliLoaded:
                raise ModuleNotFoundError('brotli is not available')

1. while that's the correct exception, it's the Python interpreter exception, not user code exception
   It should raise tlslite-ng specific exception or a generic ValueError
2. We do need to handle this error on TLS level, but then we have sent a list of supported algorithms to the peer, so we should compare with it before trying any decompression (Certificate is the only message that actually tries to interpret the payload while parsing the message, and frankly, it's been more problematic than helpful, all the other messages and extensions just parse the values, interpretation and checking of them for consistency happens after parsing)

---

tlslite/messages.py line 1328 at r4 (raw file):

        elif self.algorithm == CertificateCompressionAlgorithm.zstd:
            if not zstdLoaded:
                raise ModuleNotFoundError('zstd is not available')

same here, wrong exception

---

tlslite/utils/compression.py line 7 at r4 (raw file):

try:
    import brotli

pylint waivers for unused imports may be nice here...

[lgtm-com]

This pull request introduces 6 alerts when merging c88ac55 into dabc18f - view on LGTM.com

new alerts:

• 4 for Unused import
• 1 for `__eq__` not overridden when adding attributes
• 1 for 'import *' may pollute namespace

[t184256]

Reviewable interface is acting up for me, but the comments above should be addressed.

Added certificate compression support to reference server / client, added tlstest.py tests.

[lgtm-com]

This pull request introduces 8 alerts when merging 38b8384 into dabc18f - view on LGTM.com

new alerts:

• 4 for Unused import
• 2 for Clear-text logging of sensitive information
• 1 for `__eq__` not overridden when adding attributes
• 1 for 'import *' may pollute namespace

---

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

[tomato42]

Reviewed 8 of 8 files at r6, all commit messages.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @t184256)

---

scripts/tls.py line 75 at r6 (raw file):

    else:
        print("  GMPY        : Not Loaded")
    if GMPY2_LOADED:

Maybe add lines for the compression libraries here?

---

scripts/tls.py line 116 at r6 (raw file):

  --cert-compression - comma separated certificate compression algorithms
                       to enable. For ex. zlib,brotli,zstd.
                       Specify 'none' to disable.

that's not how other options work... why not have an empty string mean "disabled"?

---

scripts/tls.py line 117 at r6 (raw file):

                       to enable. For ex. zlib,brotli,zstd.
                       Specify 'none' to disable.
                       Corresponding libraries must be available.

they must be available for the algorithm to work, not for the algorithm to be specifiable....

---

scripts/tls.py line 350 at r6 (raw file):

        print("  Client X.509 SHA1 fingerprint: %s" % 
            connection.session.clientCertChain.getFingerprint())
        if connection.session.clientCertCompressionAlgorithm:

same as for server cert

---

scripts/tls.py line 362 at r6 (raw file):

            connection.session.serverCertChain.getFingerprint())
        if connection.session.serverCertCompressionAlgorithm:
            print("  Server certificate compression algorithm: {0}".format(

honestly, if we're reporting the used algorithm, I'd prefer an explicit "none" if it wasn't used (to not fall into the "out of sight out for mind" trap)

---

tests/tlstest.py line 1688 at r6 (raw file):

            CertificateCompressionAlgorithm.zlib)
    assert(connection.session.serverCertCompressionAlgorithm ==
            CertificateCompressionAlgorithm.zlib)

why not simply add those asserts to the regular TLS 1.3 test with certificates?

---

tests/tlstest.py line 1787 at r6 (raw file):

    assert connection.version == (3, 3)
    assert(connection.session.clientCertCompressionAlgorithm is None)
    assert(connection.session.serverCertCompressionAlgorithm is None)

similarly here, why not just add those asserts to a regular TLS 1.2 test with certificates?

---

tests/tlstest.py line 1802 at r6 (raw file):

    assert connection.version == (3, 3)
    assert(connection.session.clientCertCompressionAlgorithm is None)
    assert(connection.session.serverCertCompressionAlgorithm is None)

I think we can add those asserts to regular TLS 1.2 cert connections

---

tlslite/handshakesettings.py line 191 at r6 (raw file):

        Impacts both advertized and accepted certificate compression
        algorithms.

"affects only TLS 1.3 connections"?

---

tlslite/handshakesettings.py line 479 at r6 (raw file):

        if unknownCertComp:
            raise ValueError("Unknown certificate compression algorithm: {0}"\
                             .format(unknownCertComp))

unit tests?

---

tlslite/handshakesettings.py line 718 at r6 (raw file):

            raise ValueError("No supported ciphers")

    def _sanity_check_cert_compression(self, other):

so two methods with the same name, one just uses camelCase and the other is using underscores... that doesn't look nice to me, especially when both are called at the same time, why not have their contents in same method?

---

tlslite/handshakesettings.py line 726 at r6 (raw file):

            self._remove_all_matches(other.certCompressionAlgorithms, "brotli")
        if not compression.zstdLoaded:
            self._remove_all_matches(other.certCompressionAlgorithms, "zstd")

unit tests?

---

tlslite/messages.py line 1320 at r6 (raw file):

    def write(self):
        if self.override_compressed_data is None:

I think it would be better to just name it "compressed_data" and have it used as the buffer for the real compressed data, so that when the message has write() called twice, the certificate is not compressed twice

---

tlslite/messages.py line 1329 at r6 (raw file):

        elif self.algorithm == CertificateCompressionAlgorithm.brotli:
            if not brotliLoaded:
                raise DependencyMissing('brotli is not available')

unit tests for this code?

---

tlslite/messages.py line 1333 at r6 (raw file):

        elif self.algorithm == CertificateCompressionAlgorithm.zstd:
            if not zstdLoaded:
                raise DependencyMissing('zstd is not available')

unit tests?

---

tlslite/session.py line 143 at r6 (raw file):

        other.clientCertChain = self.clientCertChain
        other.serverCertChain = self.serverCertChain
        other.clientCertChain = self.clientCertChain

duplicate line?

---

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

                client_certificate.create(
                        clientCertChain,
                        algorithm=clientCertCompressionAlgorithm

I think it it would be better to have the algorithm set as a field in the object, not parameter to create (for one, to make buffering work)

---

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

                certificate.create(
                        serverCertChain, bytearray(),
                        algorithm=serverCertCompressionAlgorithm

same here, set algorithm as a field in object, not parameter to create

[t184256]

Reviewable status: 12 of 16 files reviewed, 9 unresolved discussions (waiting on @staticmethod and @tomato42)

---

scripts/tls.py line 75 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

Maybe add lines for the compression libraries here?

added

---

scripts/tls.py line 116 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

that's not how other options work... why not have an empty string mean "disabled"?

changed

---

scripts/tls.py line 117 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

they must be available for the algorithm to work, not for the algorithm to be specifiable....

reworded

---

scripts/tls.py line 350 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

same as for server cert

changed to print explicit "none"

---

scripts/tls.py line 362 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

honestly, if we're reporting the used algorithm, I'd prefer an explicit "none" if it wasn't used (to not fall into the "out of sight out for mind" trap)

changed to print explicit "none"

---

tests/tlstest.py line 1688 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

why not simply add those asserts to the regular TLS 1.3 test with certificates?

I couldn't single out one ultimate regular TLS 1.3 test, which one do you have in mind? Or have you meant all of them?

---

tests/tlstest.py line 1787 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

similarly here, why not just add those asserts to a regular TLS 1.2 test with certificates?

This one is slightly different in that I explicitly specify three algorithms as allowed.

---

tests/tlstest.py line 1802 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

I think we can add those asserts to regular TLS 1.2 cert connections

Which ones? All of them?

---

tlslite/handshakesettings.py line 191 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

"affects only TLS 1.3 connections"?

Added.

---

tlslite/handshakesettings.py line 479 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

unit tests?

added (test_certCompressionAlgorithms*)

---

tlslite/handshakesettings.py line 718 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

so two methods with the same name, one just uses camelCase and the other is using underscores... that doesn't look nice to me, especially when both are called at the same time, why not have their contents in same method?

This is consistent with, say, @staticmethod _sanityCheckCipherSettingserroring out on unknown names and non-@staticmethod _sanity_check_ciphers filtering out unsupported ones. Should I merge these too?

---

tlslite/handshakesettings.py line 726 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

unit tests?

added (test_certCompressionAlgorithms_filtering_out)

---

tlslite/messages.py line 1320 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

I think it would be better to just name it "compressed_data" and have it used as the buffer for the real compressed data, so that when the message has write() called twice, the certificate is not compressed twice

Renamed. With a megaton of asserts plastered around the methods, there's now a high-level interface that performs compression on create or setting cert_chain, and a low-level interface to override compressed_certificate_message and uncompressed_length.

---

tlslite/messages.py line 1329 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

unit tests for this code?

added (test_unavailable_argorithms)

---

tlslite/messages.py line 1333 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

unit tests?

added (test_unavailable_argorithms)

---

tlslite/session.py line 143 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

duplicate line?

indeed, removed

---

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

Previously, tomato42 (Hubert Kario) wrote…

this looks out of place here...

dropped

---

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

Previously, tomato42 (Hubert Kario) wrote…

I think it it would be better to have the algorithm set as a field in the object, not parameter to create (for one, to make buffering work)

Discussed in person, kept under create() for now to not overcomplicate to a sequence of __init__, .algorithm =, .create.

---

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

Previously, tomato42 (Hubert Kario) wrote…

same here, set algorithm as a field in object, not parameter to create

Discussed in person, kept under create() for now to not overcomplicate to a sequence of __init__, .algorithm =, .create.

---

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

Previously, tomato42 (Hubert Kario) wrote…

shouldn't that be client_certificate?

amended

[lgtm-com]

This pull request introduces 8 alerts when merging 8224f16 into dabc18f - view on LGTM.com

new alerts:

• 4 for Unused import
• 2 for Clear-text logging of sensitive information
• 1 for `__eq__` not overridden when adding attributes
• 1 for 'import *' may pollute namespace

---

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

[t184256]

Reviewable status: 12 of 16 files reviewed, 9 unresolved discussions (waiting on @staticmethod and @tomato42)

---

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

Previously, tomato42 (Hubert Kario) wrote…

I don't see this exception caught in the tlsconnection code? Will it still cause the tlslite-ng to send the correct alert if client misbehaves and sends a compression type unsupported by server?

Changed to BadCertificate (in parsing only)

---

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

Previously, tomato42 (Hubert Kario) wrote…

same here as for brotli

Changed to BadCertificate (in parsing only)

---

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

Previously, tomato42 (Hubert Kario) wrote…

and here, is it caught and processed correctly?

Changed to BadCertificate (in parsing only)

[lgtm-com]

This pull request introduces 8 alerts when merging 3673389 into dabc18f - view on LGTM.com

new alerts:

• 4 for Unused import
• 2 for Clear-text logging of sensitive information
• 1 for `__eq__` not overridden when adding attributes
• 1 for 'import *' may pollute namespace

---

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

[tomato42]

Reviewed 2 of 4 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @staticmethod and @t184256)

---

tests/tlstest.py line 1688 at r6 (raw file):

Previously, t184256 (Alexander Sosedkin) wrote…

I couldn't single out one ultimate regular TLS 1.3 test, which one do you have in mind? Or have you meant all of them?

just one, "good mutual X.509"

---

tests/tlstest.py line 1787 at r6 (raw file):

Previously, t184256 (Alexander Sosedkin) wrote…

This one is slightly different in that I explicitly specify three algorithms as allowed.

ok, then we can keep it

---

tests/tlstest.py line 1802 at r6 (raw file):

Previously, t184256 (Alexander Sosedkin) wrote…

Which ones? All of them?

just one is enough, like "good mutual Ed25519 X.509, TLS 1.2"

[tomato42]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @staticmethod and @t184256)

---

tlslite/handshakesettings.py line 718 at r6 (raw file):

Previously, t184256 (Alexander Sosedkin) wrote…

This is consistent with, say, @staticmethod _sanityCheckCipherSettingserroring out on unknown names and non-@staticmethod _sanity_check_ciphers filtering out unsupported ones. Should I merge these too?

if you want to... yes, but that feels like something that belongs to a separate PR

[t184256]

Reviewable status: 15 of 16 files reviewed, 3 unresolved discussions (waiting on @staticmethod and @tomato42)

---

tests/tlstest.py line 1688 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

just one, "good mutual X.509"

OK, moved into "good mutual X.509, TLS v1.3"

---

tests/tlstest.py line 1802 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

just one is enough, like "good mutual Ed25519 X.509, TLS 1.2"

Added to "good mutual Ed25519 X.509, TLS 1.2"

---

tlslite/handshakesettings.py line 718 at r6 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

if you want to... yes, but that feels like something that belongs to a separate PR

let's proliferate the pattern for now

[lgtm-com]

This pull request introduces 8 alerts when merging def9b4c into dabc18f - view on LGTM.com

new alerts:

• 4 for Unused import
• 2 for Clear-text logging of sensitive information
• 1 for `__eq__` not overridden when adding attributes
• 1 for 'import *' may pollute namespace

---

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

[tomato42]

overall looks good, but the test_read_mock_unavailable (unit_tests.test_tlslite_messages.TestCompressedCertificate) needs a fix so it passes in all environments (the exception should be caught with an assertRaises

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

---

tlslite/handshakesettings.py line 718 at r6 (raw file):

Previously, t184256 (Alexander Sosedkin) wrote…

let's proliferate the pattern for now

ok

[tomato42]

also, while the complexity and duplication notifications from CodeClimate are more false positives, the nits around formatting are valid

[lgtm-com]

This pull request introduces 8 alerts when merging 19a5c7b into dabc18f - view on LGTM.com

new alerts:

• 4 for Unused import
• 2 for Clear-text logging of sensitive information
• 1 for `__eq__` not overridden when adding attributes
• 1 for 'import *' may pollute namespace

---

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

[lgtm-com]

This pull request introduces 8 alerts when merging 9fa51bb into dabc18f - view on LGTM.com

new alerts:

• 4 for Unused import
• 2 for Clear-text logging of sensitive information
• 1 for `__eq__` not overridden when adding attributes
• 1 for 'import *' may pollute namespace

---

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

[tomato42]

r+

Reviewed 3 of 3 files at r13, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @t184256)

[tomato42]

(blocked on tlsfuzzer/tlsfuzzer#802 having all relevant test coverage)

[tomato42]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @t184256)

---

tlslite/tlsconnection.py line 1326 at r13 (raw file):

                              (Certificate, CompressedCertificate))
            if isinstance(certificate, CompressedCertificate):
                serverCertCompressionAlgorithm = certificate.algorithm

we should verify here that it's one of the algorithms advertised in Client Hello

---

tlslite/tlsconnection.py line 1690 at r13 (raw file):

            for result in self._getMsg(ContentType.handshake,
                                       (HandshakeType.certificate,
                                        HandshakeType.compressed_certificate,),

it should be accepted only when the extension was sent

---

tlslite/tlsconnection.py line 2935 at r13 (raw file):

            for result in self._getMsg(ContentType.handshake,
                                       (HandshakeType.certificate,
                                        HandshakeType.compressed_certificate,),

should be accepted only when extension was exchanged

---

tlslite/tlsconnection.py line 2946 at r13 (raw file):

            client_cert_chain = client_certificate.cert_chain
            if isinstance(client_certificate, CompressedCertificate):
                clientCertCompressionAlgorithm = client_certificate.algorithm

we should verify that the algorithm is one of the advertised ones in the extension

[lgtm-com]

This pull request introduces 8 alerts when merging 6afad00 into dabc18f - view on LGTM.com

new alerts:

• 4 for Unused import
• 2 for Clear-text logging of sensitive information
• 1 for `__eq__` not overridden when adding attributes
• 1 for 'import *' may pollute namespace

---

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

[lgtm-com]

This pull request introduces 8 alerts when merging 3bc9f37 into dabc18f - view on LGTM.com

new alerts:

• 4 for Unused import
• 2 for Clear-text logging of sensitive information
• 1 for `__eq__` not overridden when adding attributes
• 1 for 'import *' may pollute namespace

---

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

[tomato42]

Reviewed 3 of 3 files at r14, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @t184256)

---

tlslite/extensions.py line 507 at r14 (raw file):

        if self._ceiling is not None and l > self._ceiling:
            raise DecodeError("Extension payload too long "
                              "(>{0})".format(self._ceiling))

no, that's not how we implement this, it should be done by checking sanity of the extension, that the extracted list is not empty. The ceiling limit is implicit as the specified length of encoded length is one byte, and items are two bytes long.

---

tlslite/tlsconnection.py line 1334 at r14 (raw file):

                if not comp_ext:
                    for result in self._sendError(
                            AlertDescription.unexpected_message,

this will never be reached, the _getMsg won't accept a compressed certificate if it's not in the allowed handshake_types

---

tlslite/tlsconnection.py line 1708 at r14 (raw file):

            handshake_types = (HandshakeType.certificate,)
            if ch_cert_comp_ext:
                handshake_types += (HandshakeType.compressed_certificate,)

actually, this shouldn't be here at all: _clientKeyExchange is TLS 1.2 and earlier only, while compresssed_certificate is for TLS 1.3 only, it should be in _clientTLS13Handshake only

---

tlslite/tlsconnection.py line 2973 at r14 (raw file):

                if not comp_ext:
                    for result in self._sendError(
                            AlertDescription.unexpected_message,

same here, this code will never be reached