Murmur: allow certificates without full chain, but do not deem such certificates 'strong'. #2336
Conversation
…ertificates 'strong'. This allows users to connect to Murmur with CA-signed certificates without a complete certificate chain. Such cases will be treated as non-strong, juts like self-signed certificates. Before this, if a client attempted to connect to Murmur with a client certificate from a OS-trusted CA, but forgot to include the correct intermediate certificates, the client would not be allowed in -- and got no proper warning. With this change, the user is let in. However, because Murmur can't verify the certificate against the CA store, it is deemed non-strong. Ideally, the certificate wizard should warn about cases where an incomplete certificate chain is used, but I believe this is a good first step.
|
LGTM solely judging by the user readable error string for that ("No certificates could be verified") and the surrounding keys. If you found any actual documentation on those switches or their original source (do they come from OpenSSL) it would be great if you could cite it before that switch block. |
|
I'll review this myself too before landing... In Qt 5, maybe it makes more sense for Murmur to set QSslSocket::VerifyNone, and verify the client certs ourselves. (Maybe via libmumble's X509Verifier* classes.) Doing this stuff via "opting out" from errors seems so fragile. :( |
davidebeatrici
force-pushed
the
master
branch
from
55a6687
to
501651b
Compare
There was a problem hiding this comment.
Commit message typos:
- juts like self-signed
- from a*[n]* OS-trusted CA
QSslError constants in Qt 5 and Qt 4
Gets returned for OpenSSL error X509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE
21
509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE: unable to verify the first certificate
no signatures could be verified because the chain contains only one certificate and it is not self signed.
I did not test myself.
I think this should be tested to make sure it works manually. Also, the build is missing, but that should be triggered after fixing the commit message.
|
@mkrautz Aside from the typos, changes approved. |
|
I guess if we fix these typos, we can merge this PR then right? |
|
Right. |
|
I can't seem to be able to push my changes to his repo (don't have the permission). Thus my suggestion would be to merge this branch and follow directly follow this up with a rebase fixing the typos. This should only affect this specific commit and as I doubt anyone will be fast enough to pull from the repo in that time, there shouldn't be problems with force-pushing to master... |
|
I fixed the typos in #3943 |
This allows users to connect to Murmur with CA-signed certificates
without a complete certificate chain.
Such cases will be treated as non-strong, juts like self-signed
certificates.
Before this, if a client attempted to connect to Murmur with a
client certificate from a OS-trusted CA, but forgot to include
the correct intermediate certificates, the client would not be
allowed in -- and got no proper warning.
With this change, the user is let in. However, because Murmur
can't verify the certificate against the CA store, it is deemed
non-strong.
Ideally, the certificate wizard should warn about cases where
an incomplete certificate chain is used, but I believe this is
a good first step.
Fixes #993