Draft: tls13-certificate-compression: smoke-test RFC 8879 #802

t184256 · 2022-10-11T13:19:50Z

Description

Smoke-test RFC 8879 TLS Certificate Compression.
Depends on tlsfuzzer/tlslite-ng#484

Motivation and Context

GnuTLS has support for it now, I want it to test it against tlsfuzzer.

Checklist

This change is

tomato42

very bare-bones, but looks good as first draft

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

tomato42

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

scripts/test-tls13-certificate-compression.py line 153 at r2 (raw file):

        supported_algorithms['brotli'] = CertificateCompressionAlgorithm.brotli
    if zstdLoaded:
        supported_algorithms['zstd'] = CertificateCompressionAlgorithm.zstd

Shouldn't we abort if those algorithms aren't available when running a test in tlsfuzzer? it won't be a complete test otherwise...

scripts/test-tls13-certificate-compression.py line 309 at r2 (raw file):

        node = node.add_child(ClientHelloGenerator(ciphers, extensions=ext))
        node = node.add_child(ExpectAlert(AlertLevel.fatal,
                                          AlertDescription.illegal_parameter))

that doesn't seem right... If the client asks for an algorithm that the server doesn't support, then the server should simply continue with no compression, not abort the connection

scripts/test-tls13-certificate-compression.py line 311 at r2 (raw file):

                                          AlertDescription.illegal_parameter))
        node.add_child(ExpectClose())
        name = "unreasonable algorithm {}"\

use {0} for python 2.6 compat

scripts/test-tls13-certificate-compression-verify.py line 1 at r2 (raw file):

# Author: Simo Sorce, (c) 2018

doesn't look correct...

also, since it's about client certs, the script name should reflect that

scripts/test-tls13-certificate-compression-verify.py line 152 at r2 (raw file):

    node = node.add_child(ExpectChangeCipherSpec())
    node = node.add_child(ExpectEncryptedExtensions())
    node = node.add_child(ExpectCertificateRequest())

we should have at least one conversation in which we verify that the server sent specific algorithms in specific order (and have it configurable by the script command line)

scripts/test-tls13-certificate-compression-verify.py line 180 at r2 (raw file):

        supported_algorithms['brotli'] = CertificateCompressionAlgorithm.brotli
    if zstdLoaded:
        supported_algorithms['zstd'] = CertificateCompressionAlgorithm.zstd

same as the other script

scripts/test-tls13-certificate-compression-verify.py line 231 at r2 (raw file):

        node = node.add_child(ExpectAlert())
        node.next_sibling = ExpectClose()
        conversations["smoke, {} server certificate".format(alg_name)] =\

you need to use {0} for python 2.6 compatibility

scripts/test-tls13-certificate-compression-verify.py line 388 at r2 (raw file):

            None,
            algorithm=algorithm,
            override_uncompressed_length=2**24-1,

while technically correct, I worry that it may hit a hardcoded limit for cert size (2**24 is a very large certificate, a server can easily reject it based on that alone)
I think it may be a good idea to have the same connection but with uncompressed length that's just a little bit bigger than the actual compressed payload...

tlsfuzzer/messages.py line 907 at r2 (raw file):

    def __init__(self, certs=None, cert_type=None, version=None, context=None,
                 algorithm=CertificateCompressionAlgorithm.zlib,

wouldn't it be nicer (for positive, happy path tests) not to hardcode it, but rather select the first algorithm advertised by the server that we actually support?

tmshort · 2022-11-16T14:30:30Z

that doesn't seem right... If the client asks for an algorithm that the server doesn't support, then the server should simply continue with no compression, not abort the connection

It's less a negotiation, and more of a notification. The client (and server if we flip it), is saying "these are the compression algorithms I support". It's up to the peer to decide if they actually want to (or can!) send a compressed certificate. But yes, unsupported algorithms ought to be ignored.

tomato42

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

scripts/test-tls13-certificate-compression.py line 55 at r3 (raw file):

    print(" -n num         run 'num' or all(if 0) tests instead of default(all)")
    print("                (\"sanity\" tests are always executed)")
    print(" -a algorithms  comma-separated list of compression algorithms: zlib,brotli,zstd")

single letter options are generally reserved for parameters that are useful in a wide range of tests/scripts, I don't think it applies here...

scripts/test-tls13-certificate-compression.py line 355 at r3 (raw file):

        node = node.add_child(ExpectAlert())
        node.add_child(ExpectClose())
        name = "unreasonable algorithm {0}"\

I think we should have both positive and negative test with them: advertise both an invalid and a valid algorithm (in this order), verify that server picks the valid one

scripts/test-tls13-certificate-compression-verify.py line 388 at r2 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

while technically correct, I worry that it may hit a hardcoded limit for cert size (2**24 is a very large certificate, a server can easily reject it based on that alone)
I think it may be a good idea to have the same connection but with uncompressed length that's just a little bit bigger than the actual compressed payload...

why you didn't modify it?

scripts/test-tls13-certificate-compression-verify.py line 78 at r3 (raw file):

    print("                besides compress_certificate(27).")
    print("                The ids can be specified by name (\"status_request\")")
    print("                or by number (\"5\"). IDs must be separated by spaces.")

I'm not sure if having different syntax for the same argument is a good idea...

tlsfuzzer/expect.py line 496 at r3 (raw file):

                               CertificateCompressionAlgorithm,
                               f_str="Unexpected compression algorithms. "
                                     "Got: {1}, expected: {0}")

the point of the extension handlers is to test sanity of the passed in extension, it's not used here at all...

tomato42

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

a discussion (no related file):

test case with post-handshake authentication?
I'm a bit on the fence if we should have a test case with session resumption that changes if the extension is sent or not; we probably can skip it
duplicated code points handling?
very large list of compression formats (max size) with the actually valid ones specified last (to check that the server actually parsed and interpreted the full extension)
empty extension (no payload) and empty list inside the extension? (both should be rejected as malformed)
malformed extension? truncated and padded list of algorithms?
impossible length (i.e. odd number of bytes)?
interaction with HRR? what if the values change between 1st and 2nd CH?
test to see if the extension is rejected by server when it is added to Certificate message?

scripts/test-tls13-certificate-compression-verify.py line 75 at r4 (raw file):

    print(" -k keyfile     file with private key of client")
    print(" -c certfile    file with the certificate of client")
    print(" -a algorithms  comma-separated list of compression algorithms: zlib,brotli,zstd")

-a shouldn't be used here either

scripts/test-tls13-certificate-compression-verify.py line 164 at r4 (raw file):

                CertificateCompressionAlgorithm.brotli
    if 'zstd' in compression_algorithms_list:
        compression_algorithms['zstd'] = CertificateCompressionAlgorithm.zstd

wouldn't it be cleaner with some getattr on CertificateCompressionAlgorithm?

Code quote:

    if 'zlib' in compression_algorithms_list:
        compression_algorithms['zlib'] = CertificateCompressionAlgorithm.zlib
    if 'brotli' in compression_algorithms_list:
        compression_algorithms['brotli'] = \
                CertificateCompressionAlgorithm.brotli
    if 'zstd' in compression_algorithms_list:
        compression_algorithms['zstd'] = CertificateCompressionAlgorithm.zstd

scripts/test-tls13-certificate-compression-verify.py line 589 at r4 (raw file):

        node = node.add_child(ExpectAlert())
        node.next_sibling = ExpectClose()
        title = "smoke, {0}-compress client cert, correct uncompressed length"

what's the purpose of this test case? Won't it do exactly the same thing as "smoke, {0}-compress client cert"?

scripts/test-tls13-certificate-compression-verify.py line 770 at r4 (raw file):

            X509CertChain([]),
            algorithm=algorithm,
            override_uncompressed_length=2**24-1,

why max size here? Wouldn't a realistic size, maybe even taken directly from the cert we have, be more appropriate?

t184256

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

scripts/test-tls13-certificate-compression.py line 55 at r3 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

single letter options are generally reserved for parameters that are useful in a wide range of tests/scripts, I don't think it applies here...

addressed

scripts/test-tls13-certificate-compression.py line 355 at r3 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

I think we should have both positive and negative test with them: advertise both an invalid and a valid algorithm (in this order), verify that server picks the valid one

added

scripts/test-tls13-certificate-compression-verify.py line 388 at r2 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

why you didn't modify it?

didn't get to it back then. added cases with incremented and decremented uncompressed_length

scripts/test-tls13-certificate-compression-verify.py line 78 at r3 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

I'm not sure if having different syntax for the same argument is a good idea...

I see your point and I'm open to specific suggestions, but I like all other variants less even with this slight inconsistency.

scripts/test-tls13-certificate-compression-verify.py line 589 at r4 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

what's the purpose of this test case? Won't it do exactly the same thing as "smoke, {0}-compress client cert"?

it specifies override_uncompressed_length=base_uncompressed_length explicitly, ensuring I haven't messed it up.

scripts/test-tls13-certificate-compression-verify.py line 770 at r4 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

why max size here? Wouldn't a realistic size, maybe even taken directly from the cert we have, be more appropriate?

because I want to use these to ensure (through external checks ) that the server doesn't actually allocate 2**24-1. in fact, that's the primary case that made me start all that tlsfuzzer, and the other ones are passengers to that driver =)

tlsfuzzer/messages.py line 907 at r2 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

wouldn't it be nicer (for positive, happy path tests) not to hardcode it, but rather select the first algorithm advertised by the server that we actually support?

should be addressed (to the best of my understanding)

tomato42

Reviewable status: 3 of 5 files reviewed, 10 unresolved discussions (waiting on @t184256)

scripts/test-tls13-certificate-compression-verify.py line 589 at r4 (raw file):

Previously, t184256 (Alexander Sosedkin) wrote…

it specifies override_uncompressed_length=base_uncompressed_length explicitly, ensuring I haven't messed it up.

well, that should be covered by unit tests, shouldn't it?

scripts/test-tls13-certificate-compression-verify.py line 770 at r4 (raw file):

Previously, t184256 (Alexander Sosedkin) wrote…

because I want to use these to ensure (through external checks ) that the server doesn't actually allocate 2**24-1. in fact, that's the primary case that made me start all that tlsfuzzer, and the other ones are passengers to that driver =)

then I think those should be special dedicated test cases, not the default (like I wrote elsewhere, some libraries have hard limits for Handshake message sizes, so by using max size we may end up testing different code than we intend to)

t184256 · 2022-12-06T18:01:08Z

scripts/test-tls13-certificate-compression-verify.py line 589 at r4 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

well, that should be covered by unit tests, shouldn't it?

no, I meant, that I haven't messed up calculating base_uncompressed_length earlier in this script

t184256 · 2022-12-06T18:02:36Z

scripts/test-tls13-certificate-compression-verify.py line 770 at r4 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

then I think those should be special dedicated test cases, not the default (like I wrote elsewhere, some libraries have hard limits for Handshake message sizes, so by using max size we may end up testing different code than we intend to)

as in, moved "bomb" ones to a separate file?

tomato42

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

scripts/test-tls13-certificate-compression-verify.py line 589 at r4 (raw file):

Previously, t184256 (Alexander Sosedkin) wrote…

no, I meant, that I haven't messed up calculating base_uncompressed_length earlier in this script

I guess... ok

scripts/test-tls13-certificate-compression-verify.py line 770 at r4 (raw file):

Previously, t184256 (Alexander Sosedkin) wrote…

as in, moved "bomb" ones to a separate file?

no, they're fine in this file
Consider this: you have an implementation that won't accept a Certificate message that's bigger than 1 MiB (entirely reasonable, even for RSA keys). That check, if the Certificate is larger than that may be implemented right there on record layer. Then instead of testing if the compressed certificate handling is correct, we hit the hard limit on Certificate message length, not the handling of a compressed message that decompresses to an empty string.

t184256 · 2022-12-08T10:15:27Z

scripts/test-tls13-certificate-compression-verify.py line 770 at r4 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

no, they're fine in this file
Consider this: you have an implementation that won't accept a Certificate message that's bigger than 1 MiB (entirely reasonable, even for RSA keys). That check, if the Certificate is larger than that may be implemented right there on record layer. Then instead of testing if the compressed certificate handling is correct, we hit the hard limit on Certificate message length, not the handling of a compressed message that decompresses to an empty string.

ah. I failed to notice this is the empty case, not the bomb case

tomato42

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

a discussion (no related file):

Previously, tomato42 (Hubert Kario) wrote…

test case with post-handshake authentication?

I'm a bit on the fence if we should have a test case with session resumption that changes if the extension is sent or not; we probably can skip it

duplicated code points handling?

very large list of compression formats (max size) with the actually valid ones specified last (to check that the server actually parsed and interpreted the full extension)

empty extension (no payload) and empty list inside the extension? (both should be rejected as malformed)

malformed extension? truncated and padded list of algorithms?

impossible length (i.e. odd number of bytes)?

interaction with HRR? what if the values change between 1st and 2nd CH?

test to see if the extension is rejected by server when it is added to Certificate message?

Actually, those are just about the extension, we should also check what if the CompressedCertificate message itself is malformed

scripts/test-tls13-certificate-compression-verify.py line 388 at r2 (raw file):

Previously, t184256 (Alexander Sosedkin) wrote…

didn't get to it back then. added cases with incremented and decremented uncompressed_length

while good, I don't think it addresses the core issue: that the parser can reject the messages based on this value alone, without trying to decompress the certificate

scripts/test-tls13-certificate-compression-verify.py line 78 at r3 (raw file):

Previously, t184256 (Alexander Sosedkin) wrote…

I see your point and I'm open to specific suggestions, but I like all other variants less even with this slight inconsistency.

it should be parsed by the tlsfuzzer.helpers.expected_ext_parser

scripts/test-tls13-certificate-compression-verify.py line 86 at r5 (raw file):

    print("                          compression algorithms: zlib,brotli,zstd")
    print(" --disabled algorithms    comma-separated list of disabled")
    print("                          compression algorithms: \"\"")

why that's necessary, shouldn't --algorithms just be the listing of algorithms supported on server side?
for client side we can require all libraries to be installed

scripts/test-tls13-certificate-compression-verify.py line 181 at r5 (raw file):

    for alg_name in ('zlib', 'brotli', 'zstd'):
        algorithm = getattr(CertificateCompressionAlgorithm, alg_name)
        if alg_name in compression_algorithms_list:

we've already verified that compression_algorithm_list has only valid names, so this can be written as

compression_algorithms = dict((name, getattr(CertificateCompressionAlgorithm) for name in compression_algorithm_list))

same for disabled algorithms

scripts/test-tls13-certificate-nocompression-verify.py line 147 at r5 (raw file):

            cert.parse(text_cert)
        elif opt == '-E':
            ext_spec = arg

should use helpers.expected_ext_parser for parsing

scripts/test-tls13-certificate-nocompression-verify.py line 165 at r5 (raw file):

    if 'zstd' in compression_algorithms_list and not zstdLoaded:
        raise RuntimeError('unsupported algorithm `zstd`, install zstd python '
                           'bindings or disable zstd testing with `-a`')

I think we can require both brotli and zstd to be loaded before executing the script, the point of it is to verify that no algorithms are supported after all

scripts/test-tls13-certificate-nocompression-verify.py line 256 at r5 (raw file):

            CompressCertificateExtension().create([
                CertificateCompressionAlgorithm.zlib
            ])

doesn't this show that just the zlib isn't supported? Shouldn't we advertise as many algorithms as possible, and then verify that under no situation does the server send a compressed certificate?

tlsfuzzer/expect.py line 496 at r3 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

the point of the extension handlers is to test sanity of the passed in extension, it's not used here at all...

and given that the algorithm list must not be empty, we should check that it is not empty here

tlsfuzzer/expect.py line 461 at r5 (raw file):

    del state  # kept for compatibility
    if extension.status_type is None:
        return  # HOTFIX, do not push

??

tlsfuzzer/expect.py line 487 at r5 (raw file):

def _ext_handler_compress_certificate(state, extension,
                                      algorithms=None, server=True):
    """Process record_size_limit extension from server or client."""

that's not a record_size_limit extension handler

tlsfuzzer/expect.py line 490 at r5 (raw file):

    assert server == False  # FIXME
    cr_or_ch = state.get_last_message_of_type(ClientHello if server else
                                              CertificateRequest)

usually I have two handlers if the code inside needs to be different, but then, why we need a ClientHello version of it?

tlsfuzzer/expect.py line 500 at r5 (raw file):

                                   f_str="Unexpected compression algorithms. "
                                         "Got: {1}, expected: {0}")
    state.compress_certificate = algorithms

That's not a field declared in the connection state?

tlsfuzzer/messages.py line 931 at r5 (raw file):

        if self.algorithm is None:  # pick one from compress_certificate
            assert hasattr(state, 'compress_certificate')
            algorithms = state.compress_certificate

no? we should inspect exchanged messages, look for a CertificateRequest, get the extension from it...c

tlsfuzzer/messages.py line 935 at r5 (raw file):

                if alg == CertificateCompressionAlgorithm.brotli:
                    if brotliLoaded:
                        self.algorithm = alg

break?

tlsfuzzer/messages.py line 938 at r5 (raw file):

                elif alg == CertificateCompressionAlgorithm.zstd:
                    if zstdLoaded:
                        self.algorithm = alg

break?

tlsfuzzer/messages.py line 940 at r5 (raw file):

                        self.algorithm = alg
                elif alg == CertificateCompressionAlgorithm.zlib:
                    self.algorithm = alg

break?

t184256 marked this pull request as draft October 11, 2022 13:19

tomato42 previously approved these changes Oct 17, 2022

View reviewed changes

t184256 dismissed tomato42’s stale review via 60781e9 October 27, 2022 15:40

t184256 force-pushed the cert-compression branch from 47596f4 to 60781e9 Compare October 27, 2022 15:40

t184256 mentioned this pull request Oct 27, 2022

zlib-compressed certificates aren't interoperable openssl/openssl#19520

Closed

Jasonbuchanan1983 mentioned this pull request Oct 28, 2022

 mapbox/mapbox-gl-native-ios#661

Open

10 tasks

t184256 force-pushed the cert-compression branch from 60781e9 to e0bd105 Compare November 3, 2022 12:42

tomato42 requested changes Nov 3, 2022

View reviewed changes

t184256 force-pushed the cert-compression branch 3 times, most recently from 913f30b to 586c23a Compare November 8, 2022 11:05

tmshort mentioned this pull request Nov 16, 2022

Add fuzzing for the certificate compression PR openssl/openssl#19685

Open

tomato42 requested changes Nov 16, 2022

View reviewed changes

t184256 force-pushed the cert-compression branch from 586c23a to 50139fd Compare November 24, 2022 17:19

tomato42 mentioned this pull request Nov 30, 2022

Draft: add support for RFC 8879 TLS Certificate Compression tlsfuzzer/tlslite-ng#484

Open

t184256 force-pushed the cert-compression branch 2 times, most recently from d26519c to e6e701a Compare December 6, 2022 12:20

tomato42 requested changes Dec 6, 2022

View reviewed changes

t184256 commented Dec 6, 2022

View reviewed changes

tomato42 requested changes Dec 6, 2022

View reviewed changes

t184256 force-pushed the cert-compression branch from e6e701a to 40e6a06 Compare December 7, 2022 16:15

tls13-certificate-compression: smoke-test RFC 8879

639eb4f

t184256 force-pushed the cert-compression branch from 40e6a06 to 639eb4f Compare December 8, 2022 10:15

tomato42 requested changes Dec 8, 2022

View reviewed changes

tomato42 mentioned this pull request May 31, 2023

Add support for compress_certificate extension tlsfuzzer/tlslite-ng#491

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: tls13-certificate-compression: smoke-test RFC 8879 #802

Draft: tls13-certificate-compression: smoke-test RFC 8879 #802

t184256 commented Oct 11, 2022 •

edited by tomato42

tomato42 left a comment

tomato42 left a comment

tmshort commented Nov 16, 2022

tomato42 left a comment

tomato42 left a comment

t184256 left a comment

tomato42 left a comment

t184256 commented Dec 6, 2022

t184256 commented Dec 6, 2022

tomato42 left a comment

t184256 commented Dec 8, 2022

tomato42 left a comment

Draft: tls13-certificate-compression: smoke-test RFC 8879 #802

Are you sure you want to change the base?

Draft: tls13-certificate-compression: smoke-test RFC 8879 #802

Conversation

t184256 commented Oct 11, 2022 • edited by tomato42

Description

Motivation and Context

Checklist

tomato42 left a comment

Choose a reason for hiding this comment

tomato42 left a comment

Choose a reason for hiding this comment

tmshort commented Nov 16, 2022

tomato42 left a comment

Choose a reason for hiding this comment

tomato42 left a comment

Choose a reason for hiding this comment

t184256 left a comment

Choose a reason for hiding this comment

tomato42 left a comment

Choose a reason for hiding this comment

t184256 commented Dec 6, 2022

t184256 commented Dec 6, 2022

tomato42 left a comment

Choose a reason for hiding this comment

t184256 commented Dec 8, 2022

tomato42 left a comment

Choose a reason for hiding this comment

t184256 commented Oct 11, 2022 •

edited by tomato42