Refactor the certificate selection process

[inikolcev]

fixes #366

The current certificate selection process is based on signature_algorithms only, this PR also puts ciphers and curves into consideration. It also adds parsing of the SignatureAlgorithm and Issuer filed of the certificate.

---

This change is 

[inikolcev]

@tomato42
From my POV there are two things missing from the PR:

1. Getting the certificates from the virtual host specified by the client.
   Currently I can get the cert/key pair from the virtual host but I assume the VirtualHost class should be made to also store handshake settings and get those too?
2. How to know if the certificate is self-signed? I only see two options:

• Check if the Issuer and Subject are the same(but that doesn't have to be always the case)
• Check if the subject and authority key identifiers are the same. This is from rfc3280#section-4.2.1.1

What's your opinion on those two things?

[tomato42]

1. yes, in the future the VirutalHost will need to keep more settings from HandshakeSettings, but those likely will be overrides for HandshakeSettings, not complete replacement
2. I think just checking the names is good enough for now, we're doing the certificate selection on best effort basis anyway, so doing stuff like path building and checking subject key identifiers is a bit much for now

[inikolcev]

@tomato42
I don't see a simple way how to include the the certificates from a single virtual host with how virtual hosts work now. Like we discussed earlier, currently it looks like they are there just so we can hold more than one cert,key pair. Fixing that would at least require some rewrite in tls.py. After that should be really easy to pick the correct certificates after parsing the SNI extension.

[tomato42]

yes, implementing virtual hosts is orthogonal to #366 so we don't have to do it here

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @inikolcev)

a discussion (no related file):
I see just one test case updated, where's the rest of the test coverage for this?

---

---

tlslite/constants.py, line 210 at r1 (raw file):

    Algorithm OIDs as defined in rfc5758(ecdsa),
    rfc5754(rsa, sha), rfc3447(rss-pss).
    The key is the DER encoded OID as a int and

"DER encoded OID as a int"

can't we use bytes?

---

tlslite/constants.py, line 211 at r1 (raw file):

    rfc5754(rsa, sha), rfc3447(rss-pss).
    The key is the DER encoded OID as a int and
    the value is the algorithm id.

it's not, the values should be either tuples or ints, not both

---

tlslite/constants.py, line 218 at r1 (raw file):

    oid[111196837196800525313] = (2, 3)
    #ecdsa_sha224
    oid[28484837066454644032257] = (3, 3)

I think using SignatureScheme.ecdsa_secp256r1_sha256 (and similar) would be more readable

---

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

                possible_certs.append((cipher, sig_scheme, cert, key))

            except:

this will catch also SystemExit and so on, it should be Exception

---

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

            # If the client did send signature_algorithm_cert extension,
            # those check also apply to all the other certs in the chain
            # with exception on self-signed certs.

no, that's not the case

if there are no sig_alg_certs then the implementation needs to assume that sig_algs_certs is the same as sig_algs extension

---

tlslite/x509.py, line 93 at r1 (raw file):

        # Get the DER encoded OID as hex string
        self.sigalg = bytearray([self.sigalg.type.tag_id]) + \
                      bytearray([self.sigalg.length]) + \

why it's re-encoding the header into the value? why not use getChildBytes?

[inikolcev]

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

a discussion (no related file):

Previously, tomato42 (Hubert Kario) wrote…

I see just one test case updated, where's the rest of the test coverage for this?

Not sure what the new test coverage would be? We already have tests in tlstest.py that test the cert selection based on signatures, good/bad curves, connections with rsa, rsapss and ecdsa certs for both TLS1.2 and TLS1.3

---

---

tlslite/constants.py, line 210 at r1 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

"DER encoded OID as a int"

can't we use bytes?

The parseBinary function uses bytes as parameter. I'll have to call the builtin function directly and the calling is done different for python2 and 3. There is small benefit of storing it as hex string as you can copy it into an online OID decoder and it will give you the OID in dot notation.

---

tlslite/constants.py, line 211 at r1 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

it's not, the values should be either tuples or ints, not both

done

---

tlslite/constants.py, line 218 at r1 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

I think using SignatureScheme.ecdsa_secp256r1_sha256 (and similar) would be more readable

done

---

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

Previously, tomato42 (Hubert Kario) wrote…

this will catch also SystemExit and so on, it should be Exception

done

---

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

Previously, tomato42 (Hubert Kario) wrote…

no, that's not the case

if there are no sig_alg_certs then the implementation needs to assume that sig_algs_certs is the same as sig_algs extension

Right, that was misunderstanding on my part. Fixed.

---

tlslite/x509.py, line 93 at r1 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

why it's re-encoding the header into the value? why not use getChildBytes?

True, that is better.

[tomato42]

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @inikolcev)

a discussion (no related file):

Previously, inikolcev (Ivan Nikolchev) wrote…

Not sure what the new test coverage would be? We already have tests in tlstest.py that test the cert selection based on signatures, good/bad curves, connections with rsa, rsapss and ecdsa certs for both TLS1.2 and TLS1.3

I don't think we have test cases that load multiple certificates on server side and check that the server gives out correct certificate based on extensions from the client

---

---

tlslite/constants.py, line 210 at r1 (raw file):

Previously, inikolcev (Ivan Nikolchev) wrote…

The parseBinary function uses bytes as parameter. I'll have to call the builtin function directly and the calling is done different for python2 and 3. There is small benefit of storing it as hex string as you can copy it into an online OID decoder and it will give you the OID in dot notation.

Can't we have both? use a2b in the definition, but convert to bytes, so that the a2b and b2a calls aren't made in the tight loop when processing requests?

---

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

    ecdsa_secp384r1_sha384 = (5, 3)
    ecdsa_secp521r1_sha512 = (6, 3)
    rsa_pss_rsae_sha224 = (8, 3)

no, there isn't a rsa_pss_rsae_sha224 and the second byte doesn't encode the hash used, the first did that: https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-signaturescheme

---

tlslite/x509.py, line 94 at r2 (raw file):

        # If it is rsa-pss we need to check the aditional parameters field
        # to extract the hash algorithm
        if self.sigalg == '06092a864886f70d01010a':

magic variable

[inikolcev]

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

a discussion (no related file):

Previously, tomato42 (Hubert Kario) wrote…

I don't think we have test cases that load multiple certificates on server side and check that the server gives out correct certificate based on extensions from the client

We have "good RSA and ECDSA" test that loads RSA and ECDSA certs on server side and selects one based on sigalgs sent from client.
I think it would be good to test with different sigalgs in signature_algorithm_cert extension, but I don't think it is currently possible to send sigalg_cert extension. At least I don't see how I can send it.

---

---

tlslite/constants.py, line 210 at r1 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

Can't we have both? use a2b in the definition, but convert to bytes, so that the a2b and b2a calls aren't made in the tight loop when processing requests?

done

---

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

Previously, tomato42 (Hubert Kario) wrote…

no, there isn't a rsa_pss_rsae_sha224 and the second byte doesn't encode the hash used, the first did that: https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-signaturescheme

rsa_pss_rsae_sha224 is removed

---

tlslite/x509.py, line 94 at r2 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

magic variable

added in constants.py

[tomato42]

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

a discussion (no related file):

Previously, inikolcev (Ivan Nikolchev) wrote…

We have "good RSA and ECDSA" test that loads RSA and ECDSA certs on server side and selects one based on sigalgs sent from client.
I think it would be good to test with different sigalgs in signature_algorithm_cert extension, but I don't think it is currently possible to send sigalg_cert extension. At least I don't see how I can send it.

yes, but RSA and ECDSA are controlled by signature algorithms, something that was already implemented, two ECDSA certs selected by advertised curve (in TLS 1.2) would test this code

---

[tomato42]

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

[inikolcev]

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @tomato42)

a discussion (no related file):

Previously, tomato42 (Hubert Kario) wrote…

yes, but RSA and ECDSA are controlled by signature algorithms, something that was already implemented, two ECDSA certs selected by advertised curve (in TLS 1.2) would test this code

Added those tests.

---

[tomato42]

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @inikolcev)

a discussion (no related file):

Previously, inikolcev (Ivan Nikolchev) wrote…

Added those tests.

looking at test coverage it doesn't seem we hit all important cases in _server_select_certificate, comment inline

---

---

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

                        if cert.x509List[i].sigalg not in client_sigalgs:
                            cert_chain_ok = False
                            break

this code is not executed by the test suite

---

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

                            break
                if not cert_chain_ok:
                    break

this break is not executed by the test suite

---

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

        # If we can't find cert that passed all the checks, return the first usable one.
        return possible_certs[0]

and as a result of break not executed, this is never executed

[tomato42]

Reviewed 6 of 6 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved