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.

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

SelfSignedCertificate: fix automatic certificate generator #3322

Merged

Conversation

@davidebeatrici
Copy link
Member

commented Jan 24, 2018

Previously, the generator was choosing the certificate type (server or client) basing on the presence of the name and email, which broke it.

This PR also includes various other small fixes.

Fixes #3284.

@hacst

This comment has been minimized.

Copy link
Member

commented Jan 24, 2018

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2018

I checked the style in multiple files before committing, apparently we always use:

enum CamelCase { CC_FIRST, CC_SECOND, CC_THIRD };
@@ -10,9 +10,11 @@
#include <QtNetwork/QSslCertificate>
#include <QtNetwork/QSslKey>

enum CertificateType { ServerCertificate, ClientCertificate };

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jan 24, 2018

Member

I'd probably write them out as CertificateTypeServerCertificate, CertificateTypeClientCertificate.

@@ -39,7 +39,6 @@ bool SelfSignedCertificate::generate(QString clientCertName, QString clientCertE
ASN1_TIME *notBefore = NULL;
ASN1_TIME *notAfter = NULL;
QString commonName;
bool isServerCert = clientCertName.isEmpty() && clientCertEmail.isEmpty();

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jan 24, 2018

Member

Let's keep this to keep the code simple.
Just let bool isServerCert = certificateType == CertificateTypeServerCertificate; or so.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jan 24, 2018

Member

... and drop most other changes in this function.

SelfSignedCertificate: Fix automatic certificate generator, by specif…
…ying the certificate type with an enum

Previously, the generator was choosing the certificate type (server or client) basing on the presence of the name and email, which is not ideal.

Fixes #3284.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:auto-certificate-generator-fix branch from 6740a34 to d47f2e3 Jan 24, 2018

davidebeatrici and others added 5 commits Jan 25, 2018
TestSelfSignedCertificate: Change empty name and email test to work w…
…ith SelfSignedCertificate's new logic

Now it shouldn't fail, since we don't rely on the name and email to detect the certificate type anymore.
SelfSignedCertificate: add missing 'goto out's.
In these two places, we forgot to call invoke 'goto out'
to return early, after setting ok to false.
SelfSignedCertificate: only add email SAN to client certs if the pass…
…ed-in email is non-empty.

This commit ensures that we only add the email subject alt name (SAN) if the passed-in
email is non-empty.

It turns out that the call to X509V3_EXT_conf_nid() in add_ext() will fail
if the SAN value is formatted incorrectly. In our case, we passed "email:"
as the value, and X509V3_EXT_conf_nid() would return NULL.

To fix this, we now check whether the email we've been given is empty or not.
If it's empty, we don't add the SAN. If it's there, we do.
TestSelfSignedCertificate: add tests that exercise new email SAN beha…
…vior.

This commit adds tests to ensure that it's now possible to create a
client certificate without an email, and that creating a client certificate
with an email still works.

This is added because SelfSignedCertificate now conditionally adds an email
subject alt name (SAN) to the generated certificate. If the passed-in email
is empty, SelfSignedCertificat will not include the SAN.

@mkrautz mkrautz changed the title SelfSignedCertificate: Fix automatic certificate generator, by specifying the certificate type with an enum SelfSignedCertificate: fix automatic certificate generator Jan 27, 2018

@mkrautz mkrautz merged commit 16810dd into mumble-voip:master Jan 27, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.