FEAT(server): Certificate Handling improvements

[EP-u-NW]

This is a reopening of #4963 with a commit targeting #4960 removed, since it was rejected after discussion.

This PR addresses SSL Error Configuration and Client Certificate Trusting (both #3523), as well as Username Client Certificate CN Equality (#4940).
It is structured the following way:

1. What is this and why do we need it?
2. User perspective
3. How we achieve this technically
4. Testing

What is this and why do we need it?

In current murmur, clients present certificates to murmur to proof their identity. This is in theory a very strong security approach, but murmur is limited when it comes to processing these client certificates. While validating a clients certificate, multiple ssl exceptions can occur, but murmur ignores many of them (see #3523). In some usecases, this makes total sense, but in others it is necessary to act on them. So this PR makes them configurable.

After being able to configure which errors should be ignored, a logical next step is to make it configurable whom to trust. In the current implementation of ssl handling, this is actually possible, but only to a certain extend and only by abusing some configuration options (more on this in the technical part). To clean this up, this PR introduces the Murmur Client Truststore (MCTS), which can be freely configured and is then used when deciding whether to trust a clients certificate or not.

Finally, when a user provided a trusted certificate accepted by murmur, it may be desirable to force that user to use the name the certificate was issued to (as requested in #4940).

User perspective

To use this improvements, new options for murmur.ini are created. The default values of these options are set in a way, that if a server admin just ignores them, murmurs behaviour does not change compared to how it acts currently. Without further ado, these are the options (the below explanations should be added to to wiki if this PR is accepted):

allowedClientSslErrors (default: AllowNoPeerCertificate, AllowSelfSignedCertificate, AllowSelfSignedCertificateInChain, AllowUnableToGetLocalIssuerCertificate, AllowUnableToVerifyFirstCertificate, AllowHostNameMismatch, AllowCertificateNotYetValid, AllowCertificateExpired)
If a client presents a certificate to the server, the certificate is validated according to Murmurs Client Truststore (MCTS), which by default contains murmurs own chain-of-trust as well as the hostmachines CAs. Errors might arrise during validation of the clients certificate, but some are not critical and thus ignored. A server admin can configure, what errors should ignored using a comma separated list of the following options: AllowUnableToGetIssuerCertificate, AllowUnableToDecryptCertificateSignature, AllowUnableToDecodeIssuerPublicKey, AllowCertificateSignatureFailed, AllowCertificateNotYetValid, AllowCertificateExpired, AllowInvalidNotBeforeField, AllowInvalidNotAfterField, AllowSelfSignedCertificate, AllowSelfSignedCertificateInChain, AllowUnableToGetLocalIssuerCertificate, AllowUnableToVerifyFirstCertificate, AllowCertificateRevoked, AllowInvalidCaCertificate, AllowPathLengthExceeded, AllowCertificateUntrusted, AllowCertificateRejected, AllowSubjectIssuerMismatch, AllowAuthorityIssuerSerialNumberMismatch, AllowNoPeerCertificate, AllowHostNameMismatch, AllowUnspecifiedError, AllowNoSslSupport, AllowCertificateBlacklisted. If the server was compiled using a Qt version greater or equal 5.13.0, these additional values can be used: AllowCertificateStatusUnknown, AllowOcspNoResponseFound, AllowOcspMalformedRequest, AllowOcspMalformedResponse, AllowOcspInternalError, AllowOcspTryLater, AllowOcspSigRequred, AllowOcspUnauthorized, AllowOcspResponseCannotBeTrusted, AllowOcspResponseCertIdUnknown, AllowOcspResponseExpired, AllowOcspStatusUnknown. If no errors should be allowed, an empty list can be provided, or the value "None". An exception to the above rule are "Invalid Purpose" ssl errors, which are not configurable and always allowed.

mctsAddOwnCertificateAsCA (default: true)
Adds murmurs own certificate (without the chain) as CA to the MCTS.

mctsAddOwnCAs (default: true)
Adds the chain of murmurs own certificate (but not the its own certificate) as CAs to the MCTS. 

mctsHostsCAs (default: true)
If this is set to true, the CAs of the hostmachine are added to the MCTS.

mctsExtraCAs (default: empty)
Path to a pem file containing one to multiple certificates to add to the MCTS.

usernameMustMatchCertCommonName (default: false)
If this is set to true, a user can only enter the server, if her username equals the common-name-subject-field of the provided certificate. If client certificates are not required on a server (set using "certrequired"), this property is ignored.

How we achieve this technically

Most of this is also discussed in depth in #3523, here is a short summary, starting with murmur gets its own certificate from the murmur.ini:

1. Read all certs from sslCA; these are 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 there's 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 are Meta::mp.qlIntermediates

Now, when a client connection is made, murmur sets the certificate itself presents to the client to qscCert. Secondly, it configures its own ssl context to trust all certificates from Meta::mp.qlCA and Meta::mp.qlIntermediates. This is the abusive way I mentioned above to get murmur to trust certain client certificates: According to the wiki we should use sslCA to denote our intermediates, not certificates we want to trust. If influencing the current client certificates murmur trusts by abusing the sslCA is possible, why do we need this PR and code changes and can not just update the wiki, saying that the servers own certificate and intermediates should go into sslCert, and the certificates we want to trust into sslCA? Well, firstly we would still tell our ssl context to trust Meta::mp.qlIntermediates. This would still force murmur to trust all certificates that relate to the servers own certificate chain. While this seems reasonable there are usecases where one dont want to trust certificates in its own chain of trust. Secondly, a ssl context is initialized with the hostmachines CAs and maybe we don't want them either.

The key to getting this right is asking: Why do we currently add Meta::mp.qlCA and Meta::mp.qlIntermediates to our truststore? You know, according to the wiki, we don't want to trust them, we only want to construct our own certificate chain using the intermediates and CAs from these lists. The answer is simple: If we add them to our trust store, Qt (or whatever is beneath Qt in ssl) will calculate this chain for us. So murmur actually abuses the truststore itself.

Now, with this PR we do steps 2 to 9 from above as we currently do them, but instead of storing the leftover certificates from step 9 into Meta::mp.qlIntermediates, we store them in a temporary list, called pool. And instead of doing step 1, the certificates form sslCA are feed directly to pool. Then, we use the qscCert and pool to calculate the chain (using openssl), store just the chain and discard everything else. Calculating the chain once in advance and then only use it is actually better then building it every time we need it (see here).

For the client certificates we want to trust we use the MCTS, so it is clearly separated: Things we want to trust start with mcts, things we want to present to clients start with ssl.

This was the interesting part. The allowed ssl errors are simply parsed from the murmur.ini and transformed to QSslError:SslError by comparision using a map. As suggested by @Krzmbrzl, this matching is actually case insensitive. You can see the names used to match the errors and vice versa in Meta::getSslNameErrorMap() and Meta::getSslErrorNameMap().

usernameMustMatchCertCommonName simply rejects users when a authenticate message is received and username and certificate subject common name don't match. A new reject type was introduced for this, it might be necessary to implement this type in the mumble client.

Additionally, this PR also fixes a bug in Server.cpp where certs.last(); is used instead of certs.first();. Again, see #3523 for details.

Testing

With this many internal changes, testing is very important. I also want to mention that I've very little C++ experience, and in particular never worked with Qt before. We have done some functionality testing ourselves, and while we could not find a bug it doesn't mean that there are no more ;)

I really hope that this changes get implemented in the next version, but take your time reviewing :)
I'm glad for every question and suggestion!

[Kissaki]

This is an exceptionally well described PR. Great job on that! It provides great detail and context which will allow for an easier review and discussion. It also allowed me (and anyone curious now or in the future) to go through and learn about it without deep diving into implementation or code, and I could raise any concerns if I had any on a conceptual level.

I just wanted to point this out. Thank you for that. I would love to see more of these kind of PRs. :)

FYI I fixed a bunch of typos in the text, so it should be (even) more easily readable than before.

[EP-u-NW]

@Kissaki Thanks! I really appriciate your feedback😄

@Krzmbrzl made already clear that:

What I can tell you though is that this probably won't make it into the 1.4.x series as we're about to feature-freeze that. However we plan to do more regular releases after that so it should not be too long until this is widely available :)

Originally posted by @Krzmbrzl in #4963 (comment)

what I can totally understand.

The thing I recently notices is that the first two commits in this PR are not features but fixes, so maybe they could be considered for 1.4.x (hasn't to be 1.4.0 right away)?

[Krzmbrzl]

Yes the first two commits can probably be backported to the 1.4.x branch once this PR is merged 👍

[Kissaki]

If they make sense by themselves you can create a separate PR for them.

[Krzmbrzl]

@davidebeatrici if you have the time, it'd be great if you could review this PR (as well) since for me it will probably still take quite some more time until I have the necessary time to dive into this topic...

[EP-u-NW]

Take your time! We are currently using this with an own static build, so we are not in a hurry (althought it would be cool to be able to use an official build, for security reasons). Speaking of that static build, I encountered some problems with the PRs code there and hadn't had the time to create something pushable for this PR yet. It will (unfortunately 😢) take at least two more weeks until I can start working on mumble server again to provide these fixes and rebase everything on upstream, so again, there is no need to hurry right now (except of course you guys have the capacity to do this fixes and rebases on your own 😄).

[EP-u-NW]

So it has been longer than two weeks, sorry about that... but now I finally got time to start working on this again. I have several questions to the procedure now:

• Since so much time passed, this PR should be rebased on a more recent commit I guess? Whats the best way to do this? A new PR maybe?
• How should I deal with @davidebeatrici comments? Most of them are just typos/language so I would resolve them by adopting them. How to do this? Edit the commits they originate from or simply press the Commit suggestion button here on github? Can I even edit commits that are already part of a PR?
• As mentined above I myself found some bugs, how shall I deliver the fixes for them? As a new FIX commit or try to incorperate them in the original commits?

So, as you can see I'm a bit lost here and I'm looking forward to your advice😅.

[Krzmbrzl]

Updating to the most recent code is probably a good idea (even though no conflicts have surfaced yet).

You can simply rebase your branch against the current master and then force-push to GitHub in order to update this PR.

As for fixing any typos or actual bugs, please fix these in the respective commits and then again force-push to update this PR.
Alternatively you can also fix these things by individual commits appended to the current ones and then just before this is going to be merged, squash these commits into the already existing commits.

Therefore: Eventually all changes that are changes to you changes, should not end up in a new commit at the time this is merged but instead be incorporated into the commit that introduces the initial set of changes.

[EP-u-NW]

So working through the comments and integrating them takes more time and effort then planned. I also realized that while this PR adds a lot of usefull things, it might be easier to split it in multiple PRs. This way, the features can also be tested more independendly and we can identify possible bugs easier.

If we agree on this procedere I will open a new PR for the first commit in this PR, and if that is merged for the next one and so on. I would also let this PR open to keep track of the whole process.

[Krzmbrzl]

Sounds good to me 👍