Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FIX(server): Build the servers certificate chain once instead of abusing the trust store #5312

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

FIX(server): Build the servers certificate chain once instead of abusing the trust store #5312

wants to merge 1 commit into from

Commits on Jan 8, 2022

  1. FIX(server): Build the servers certificate chain once instead of abus… 

    …ing the trust store

While there are serveral options to configure, which ssl certificates a server should use, the way that they were processed was a bit hacky and messy. The original process, how the certificates were used is the following:
1. Read all certs from sslCA; these were Meta::mp.qlCA
2. If sslKey set and valid, use it as private key
3. If sslKey NOT set, try to get a private key from sslCert
4. If we have no key by now, fail, else we have found our key Meta::mp.qskKey
5. Read ALL certs from sslCert and ALL certs from sslKey to a cert list
6. Use the private key to find a matching cert in the cert list
7. If there is NO match, fail
8. If theres a match, remove it from the list and consider it our cert Meta::mp.qscCert
9. Treat all leftover certs in the list as intermediates; these were Meta::mp.qlIntermediates

Now, when a client connection was made, the server sets the certificate itself presents to the client to qscCert. Secondly, it configured its own ssl context to trust all certificates from Meta::mp.qlCA and Meta::mp.qlIntermediates.
But according to the wiki sslCA should be used to denote the servers intermediates, not certificates it want to trust. The reason why these certificates were added to the truststore is that, if they were added to the truststore Qt (or whatever is beneath Qt in ssl) will caltulate the servers chain of trust and present them to the client. So the server actually abused the truststore to do the chain calculation.
This has the undesired side effect that the server will always trust all certificates in its own chain of trust itself (which for most usecases seem desirable, but not for all).

This commit changes the chain calculation: Steps 2 to 9 from above are not altered, but instead of storing the leftover certificates from step 9 into Meta::mp.qlIntermediates, they are stored in a temperary list, called pool. And instead of doing step 1, the certificates form sslCA are feed directly to pool. Then, qscCert and pool are used to calculate the chain (using openssl), just the chain is stored and everything else is discarded. Calculating the chain once in advance and then only use it is actually better then building it every time it is needed.

Since the certificates from sslCA and sslCert are no longer added to the trust store, separating the things the server presents to the client, and certificates the server wants to trust is possible now.

Fixes mumble-voip#3523 (partially)

Co-authored-by: Davide Beatrici <[email protected]>
    @EPNW-Eric @davidebeatrici @Krzmbrzl
    2 people authored and Krzmbrzl committed Jan 8, 2022
    Configuration menu
    Copy the full SHA
    0779084 View commit details
    Browse the repository at this point in the history