Add support for CertificateRequest (TLS 1.3)

[simo5]

Fixes #207

---

This change is 

[tomato42]

This pull request introduces 3 alerts when merging f3a4cfd into 3023516 - view on LGTM.com

new alerts:

• 3 for Unused local variable

---

Comment posted by LGTM.com

[tomato42]

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

a discussion (no related file):
unit test coverage for new CertificateRequest (in 93e2b03) missing

---

---

tlslite/constants.py, line 278 at r3 (raw file):

    @staticmethod
    def getSchemeName(signature_scheme):

what's wrong with toRepr()?

---

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

            p2 = Parser(p.getVarBytes(2))
            while p2.getRemainingLength():
                self.extensions.append(TLSExtension().parse(p2))

TLSExtension()

all extensions that can be placed in CertificateRequest have the same syntax as they have in ClientHello?

---

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

                certificateRequest = CertificateRequest(self.version)
                certificateRequest.create(None, None, None, ctx, cr_extensions)

specifying them through keyword arguments would be cleaner, so we probably should also add default values to certificate_types and certificate_authorities

---

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

                clientCertChain = clientCertificate.cert_chain
        else:
            raise AssertionError()

why the error here? it's fine to not ask for client certificate...

---

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

            validSigAlgs = self._sigHashesToList(settings)
            if signature_scheme not in validSigAlgs:
                for result in self._sendError(\

nit: escaping newlines in brackets is not necessary

---

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

            if signature_scheme not in validSigAlgs:
                for result in self._sendError(\
                        AlertDescription.decryption_failed,

sending ID that the server didn't advertise as one it recognises seems to me like illegal_parameter, not decryption_failed

---

tlslite/tlsconnection.py, line 1184 at r3 (raw file):

                #abort if Certificate Request with inappropriate ciphersuite
                if serverHello.cipher_suite not in CipherSuite.tls13Suites:

this is purely TLS 1.3 code, I'm not sure what this code aims to achieve...

client certificates are forbidden with PSK key exchange though

---

tlslite/tlsconnection.py, line 1185 at r3 (raw file):

                #abort if Certificate Request with inappropriate ciphersuite
                if serverHello.cipher_suite not in CipherSuite.tls13Suites:
                    for result in self._sendError(\

nit: unnecesary newline escape

---

tlslite/tlsconnection.py, line 1190 at r3 (raw file):

                        yield result

                # we got CertificateRequest so now we'll get ServerHelloDone

shouldn't that be Certificate?

---

tlslite/tlsconnection.py, line 1279 at r3 (raw file):

            if clientCertChain:
                #Check to make sure we have the same type of
                #certificates the server requested

nit: either PEP8 or PEP257 requires # as comment starter

---

tlslite/tlsconnection.py, line 1282 at r3 (raw file):

                if serverHello.certificate_type == CertificateType.x509 \
                    and not isinstance(clientCertChain, X509CertChain):
                    for result in self._sendError(\

nit: unnecessary newline escape

---

tlslite/tlsconnection.py, line 1296 at r3 (raw file):

                for ext in certificateRequest.extensions:
                    if isinstance(ext, SignatureAlgorithmsExtension):
                        validSigAlgs = ext.sigalgs

using HelloMessage as a superclass of CertificateRequest so that getExtension can be used would lead to more code reuse

---

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

                        validSigAlgs = ext.sigalgs
                if not validSigAlgs:
                    for result in self._sendError(\

nit: unnecessary newline escape

---

tlslite/tlsconnection.py, line 1344 at r3 (raw file):

msgags

typo

---

tlslite/tlsconnection.py, line 2667 at r3 (raw file):

                        real_version = v
                        break
        if real_version < settings.minVersion:

this likely should be a separate patch

[simo5]

On Tue, 2018-12-18 at 09:41 -0800, Hubert Kario wrote: what's wrong with toRepr()?

Nothing, only I did not realize it would produce the desired output, will replace.

tlslite/messages.py, line 1239 at r1 (raw file): > p2 = Parser(p.getVarBytes(2)) > while p2.getRemainingLength(): > self.extensions.append(TLSExtension().parse(p2)) TLSExtension() all extensions that can be placed in CertificateRequest have the same syntax as they have in ClientHello?

As far as I know ? Haven't checked exactly, do you think there may be extensions with the same name but that behave differently ?

specifying them through keyword arguments would be cleaner, so we probably should also add default values to certificate_types and certificate_authorities

Ok, will do.

why the error here? it's fine to not ask for client certificate...

It was a mistake, I already removed it in my latest revision as I was going through git bisect to check test are clean on each patch.

nit: escaping newlines in brackets is not necessary

I probably simply copied this from pre-existing code, will remove.

sending ID that the server didn't advertise as one it recognises seems to me like illegal_parameter, not decryption_failed

I will check.

this is purely TLS 1.3 code, I'm not sure what this code aims to achieve... client certificates are forbidden with PSK key exchange though

Just mimicked the same checks we do in TLS1.2, should I remove the check ?

nit: unnecesary newline escape

will fix

shouldn't that be Certificate?

yeah, will replace

nit: either PEP8 or PEP257 requires # as comment starter

will fix

using HelloMessage as a superclass of CertificateRequest so that getExtension can be used would lead to more code reuse

Id it worth the possible confusion? Sounds like we should have an "Exensible" superclass that encapsulates this function for both if we go this direction.

msgags typo

will fix

tlslite/tlsconnection.py, line 2667 at r3 (raw file): > real_version = v > break > if real_version < settings.minVersion: this likely should be a separate patch

ok

[simo5]

Updated, commits reordered and/or split as needed, this is so that tests do not fail between commits, or as have been requested.
I think there is one or two open question to resolve, but otherwise should be re-reviewable.

[tomato42]

This pull request introduces 3 alerts when merging 480918f into 3023516 - view on LGTM.com

new alerts:

• 3 for Unused local variable

---

Comment posted by LGTM.com

[tomato42]

all extensions that can be placed in CertificateRequest have the same syntax as they have in ClientHello?

As far as I know ? Haven't checked exactly, do you think there may be extensions with the same name but that behave differently ?

I think it would be more future-proof to pass to TLSExtension that we are parsing a CR extension

this is purely TLS 1.3 code, I'm not sure what this code aims to achieve... client certificates are forbidden with PSK key exchange though

Just mimicked the same checks we do in TLS1.2, should I remove the check ?

yes, in TLS 1.2 there are cipher suites for which server can't ask for a certificate, but in TLS 1.3 it isn't a function of ciphersuite so it doesn't make sense to check ciphersuite; the test should check used key exchange

using HelloMessage as a superclass of CertificateRequest so that getExtension can be used would lead to more code reuse

Id it worth the possible confusion? Sounds like we should have an "Exensible" superclass that encapsulates this function for both if we go this direction.

I think HelloMessage was added during the current alpha (please check), so I think we can rename it to ExtensibleMessage or ExtensionMessage without technically breaking API

[tomato42]

Reviewed 3 of 4 files at r4, 1 of 1 files at r5, 1 of 2 files at r6, 1 of 1 files at r7, 1 of 1 files at r8.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @simo5)

---

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

        self.supported_signature_algs = sig_algs
        self.certificate_request_context = context
        self.extensions = extensions

definition missing in __init__ for both of the above

---

tlslite/tlsconnection.py, line 2491 at r4 (raw file):

                for v in ext.versions:
                    if v >= settings.minVersion:
                        real_version = v

that would make it the first version advertised by client that's higher than minVersion, not "real version"
real_version = max(real_version, *ext.versions) would give us the highest value, but that in turn may be a GREASE value, not actual TLS 1.3 ID; so we probably should look at either constants.KNOWN_VERSIONSor at settings.versions when inspecting supported_versions

---

tlslite/tlsconnection.py, line 1342 at r8 (raw file):

            # Server didn't ask for cer, zeroise so session doesn't store them
            privateKey = None
            clientCerChain = None

I don't see it being added to session though

---

tlslite/tlsconnection.py, line 2540 at r8 (raw file):

            validSigAlgs = self._sigHashesToList(settings)
            if signature_scheme not in validSigAlgs:
                for result in self._sendError(\

nit: newline escape

[tomato42]

f8a663a - I don't see anything related to new test coverage in that patch, despite message claiming otherwise

5474c6c - unit test for this still missing

[simo5]

I think it would be more future-proof to pass to TLSExtension that we are parsing a CR extension

How does this work ?
I looked at TLSExtension() but I do not see an obvious way, and I am not sure what you are asking.

[tomato42]

This pull request introduces 4 alerts when merging 995c183 into 7c1b350 - view on LGTM.com

new alerts:

• 4 for Unused local variable

---

Comment posted by LGTM.com

[tomato42]

This pull request introduces 1 alert and fixes 1 when merging a959f3b into 7c1b350 - view on LGTM.com

new alerts:

• 1 for Missing call to __init__ during object initialization

fixed alerts:

• 1 for __eq__ not overridden when adding attributes

---

Comment posted by LGTM.com

[tomato42]

This pull request fixes 1 alert when merging b8ccfce into 7c1b350 - view on LGTM.com

fixed alerts:

• 1 for __eq__ not overridden when adding attributes

---

Comment posted by LGTM.com

[tomato42]

This pull request fixes 1 alert when merging 1ffc78a into 7c1b350 - view on LGTM.com

fixed alerts:

• 1 for __eq__ not overridden when adding attributes

---

Comment posted by LGTM.com

All should have een handled, PTAL

[tomato42]

This pull request fixes 1 alert when merging 3117b96 into 7c1b350 - view on LGTM.com

fixed alerts:

• 1 for __eq__ not overridden when adding attributes

---

Comment posted by LGTM.com

[tomato42]

I looked at TLSExtension() but I do not see an obvious way, and I am not sure what you are asking.

the parameters to __init__: server, encExt, cert, hrr

[simo5]

Added Session resumption test with client certs

[tomato42]

This pull request fixes 1 alert when merging d9a02ef into 3696909 - view on LGTM.com

fixed alerts:

• 1 for __eq__ not overridden when adding attributes

---

Comment posted by LGTM.com

[simo5]

We do not implement all CR extensions, but at a cursory look they all seem to fall in the universal extension category.
Atm I added also server=True just in case, but I wonder if I really should ...

Perhaps a follow-up PR should actually properly handle this by categories given RFC8446 4.2 says:

   If an implementation receives an extension
   which it recognizes and which is not specified for the message in
   which it appears, it MUST abort the handshake with an
   "illegal_parameter" alert.

At the moment we do not do this in TLSExtension() nor in ClientHello() or ServerHello()

[tomato42]

Reviewed 4 of 4 files at r22, 1 of 1 files at r23, 2 of 2 files at r24, 1 of 1 files at r25, 1 of 1 files at r26.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @simo5)

---

tests/tlstest.py, line 432 at r26 (raw file):

    connection.handshakeClientCert(settings=settings)
    testConnClient(connection)
    assert(isinstance(connection.session.serverCertChain, X509CertChain))

nit: superfluous () (assert is a statement, not a function)

---

tlslite/messages.py, line 2024 at r22 (raw file):

    def __init__(self):
        """Create instance of the object."""
        self.version = 0

why 1 shouldn't be the default?

---

tlslite/messages.py, line 2042 at r22 (raw file):

    @cli_cert_chain.setter
    def cli_cert_chain(self, cert_chain):
        """Setter for the cert_chain property."""

cert_chain or cli_cert_chain? also, it's not obvious what the "cli" stands for

the property should also be documented in the class doc string too

---

tlslite/messages.py, line 2095 at r22 (raw file):

            for entry in self._cert_chain:
                wcert.bytes += entry.write()
            writer.addVarSeq(wcert.bytes, 1, 3)

unit test coverage for this code?

---

tlslite/tlsconnection.py, line 2231 at r25 (raw file):

                                                       [new_sess_ticket])

            return [(identity.identity, psk, prf), ticket]

nit: canonically tuples are used to pass multiple return values, not arrays (same below where it's used)

---

tlslite/tlsconnection.py, line 2520 at r25 (raw file):

        #Get and check CertificateVerify, if relevant
        cli_cert_verify_hh = self._handshake_hash.copy()
        if client_cert_chain and client_cert_chain.getNumCerts() > 0:

fyi, if x: is equivalent to if x > 0: when x is non-negative

[simo5]

nit: superfluous `()` (`assert` is a statement, not a function)

ok

> """Create instance of the object.""" > self.version = 0 > ``` why `1` shouldn't be the default?

0 is sufficient for most uses given cli certs are not very commonly used.

> @cli_cert_chain.setter > def cli_cert_chain(self, cert_chain): > """Setter for the cert_chain property.""" > ``` `cert_chain` or `cli_cert_chain`? also, it's not obvious what the "cli" stands for

please suggest better naming

the property should also be documented in the class doc string too

ok

> for entry in self._cert_chain: > wcert.bytes += entry.write() > writer.addVarSeq(wcert.bytes, 1, 3) > ``` unit test coverage for this code?

I'll check

> return [(identity.identity, psk, prf), ticket] > ``` nit: canonically tuples are used to pass multiple return values, not arrays (same below where it's used)

should I use ((a, b, c), y) ?

#Get and check CertificateVerify, if relevant > cli_cert_verify_hh = self._handshake_hash.copy() > if client_cert_chain and client_cert_chain.getNumCerts() > 0: > ``` fyi, `if x:` is equivalent to `if x > 0:` when x is non-negative

sure, does this comment mean you prefer the implicit form ?

[tomato42]

please suggest better naming

client_cert_chain and doc string to explain it?

should I use ((a, b, c), y) ?

yes, please do

sure, does this comment mean you prefer the implicit form ?

prefer? yes. insist on? no

[simo5]

Pushed a version that should address all nits

[tomato42]

This pull request fixes 1 alert when merging 678ccb2 into 3696909 - view on LGTM.com

fixed alerts:

• 1 for __eq__ not overridden when adding attributes

---

Comment posted by LGTM.com

[tomato42]

looks good, I'll wait with merging until we know that no APIs need changing for tlsfuzzer

Reviewed 1 of 4 files at r27, 1 of 1 files at r29, 2 of 2 files at r30, 1 of 1 files at r31, 1 of 1 files at r32.
Reviewable status: complete! all files reviewed, all discussions resolved

[tomato42]

This pull request fixes 2 alerts when merging 83ad0c8 into 3696909 - view on LGTM.com

fixed alerts:

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

---

Comment posted by LGTM.com

[tomato42]

This pull request fixes 2 alerts when merging d903985 into 3696909 - view on LGTM.com

fixed alerts:

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

---

Comment posted by LGTM.com

[tomato42]

This pull request fixes 2 alerts when merging a01b574 into 3696909 - view on LGTM.com

fixed alerts:

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

---

Comment posted by LGTM.com

[tomato42]

Reviewed 1 of 4 files at r33, 1 of 1 files at r35, 1 of 2 files at r36, 1 of 1 files at r38, 2 of 2 files at r39, 1 of 1 files at r40.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @simo5)

a discussion (no related file):
CI is failing across the board

---

---

tlslite/keyexchange.py, line 234 at r39 (raw file):

                                    b'\x00') + \
                          handshakeHashes.digest(prf_name)
            verifyBytes = secureHash(verifyBytes, hash_name)

unit test coverage for this?

---

tlslite/messages.py, line 1228 at r36 (raw file):

    def create(self, certificate_types=None, certificate_authorities=None,
               sig_algs=(), context=b'', extensions=[]):

[] as a default value creates array once that will be then shared with all class instances

I don't think we want that here

---

tlslite/messages.py, line 1258 at r36 (raw file):

                # Populate supported_signature_algs if we find them
                if ext.extType == ExtensionType.signature_algorithms:
                    self.supported_signature_algs = ext.sigalgs

that's very fragile, I'd much prefer supported_signature_algs promoted to a property so that extensions and the value for tls 1.2 is always synchronised – setting class fields without calling create() (or after that) is entirely valid use

combined with the upper level code it also will stop detecting duplicated signature_algorithms extensions (previously getExtension() made sure of that)

---

tlslite/messages.py, line 1287 at r36 (raw file):

if self.supported_signature_algs:

is sufficient

---

tlslite/messages.py, line 1290 at r36 (raw file):

            ext = SignatureAlgorithmsExtension().create(
                    self.supported_signature_algs)
            sub_writer.bytes += ext.write()

won't parsing and then serialising the message will result in extension getting duplicated?

[tomato42]

This pull request fixes 2 alerts when merging 2313ba9 into 3696909 - view on LGTM.com

fixed alerts:

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

---

Comment posted by LGTM.com

[simo5]

Changed the handling of supported_signature_algs as discussed offline

Should all be handled as discussed

[tomato42]

Reviewed 4 of 6 files at r41, 1 of 1 files at r43, 1 of 2 files at r44, 1 of 1 files at r46, 2 of 2 files at r47, 1 of 1 files at r48, 1 of 1 files at r49.
Reviewable status: complete! all files reviewed, all discussions resolved

[tomato42]

looks good, thank you!