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

Allow users to specify RFC 7919 Diffie-Hellman parameters for the sslDHParams murmur.ini option #3185

Merged
merged 7 commits into from Aug 7, 2017

Conversation

@mkrautz
Copy link
Member

commented Jul 19, 2017

This PR uses the newly introduced FFDHE class to allow users to specify RFC 7919 Diffie-Hellman parameters for the sslDHParams option in murmur.ini.

Also, this PR changes the default value for the sslDHParams option. Instead of being blank by default (which would previously cause Murmur to auto-generate Diffie-Hellman parameters for all virtual servers), we now use @ffdhe2048.

This PR depends on
#3183

Fixes #2204

@mkrautz mkrautz changed the title Allow the use of RFC 7919 Diffie-Hellman parameters for sslDHParams option WIP: Allow the use of RFC 7919 Diffie-Hellman parameters for sslDHParams option Jul 19, 2017

@mkrautz mkrautz force-pushed the mkrautz:ffdhe-integration branch from 9042ff2 to e3a59db Jul 29, 2017

@mkrautz mkrautz changed the title WIP: Allow the use of RFC 7919 Diffie-Hellman parameters for sslDHParams option Allow the use of RFC 7919 Diffie-Hellman parameters for sslDHParams option Jul 29, 2017

@mkrautz mkrautz changed the title Allow the use of RFC 7919 Diffie-Hellman parameters for sslDHParams option Allow users to specify RFC 7919 Diffie-Hellman parameters for the sslDHParams murmur.ini option Jul 29, 2017

@mkrautz mkrautz force-pushed the mkrautz:ffdhe-integration branch from e3a59db to 8dfbbdd Jul 29, 2017

@mkrautz mkrautz requested review from Kissaki, hacst and davidebeatrici Jul 29, 2017

@mkrautz mkrautz force-pushed the mkrautz:ffdhe-integration branch from 8dfbbdd to 1ebc09f Jul 29, 2017

@hacst
hacst approved these changes Aug 5, 2017
Copy link
Member

left a comment

See comment. LGTM.

QString group = qsSSLDHParams.mid(1).trimmed();
QByteArray pem = FFDHE::PEMForNamedGroup(group);
if (pem.isEmpty()) {
qCritical("MetaParms: Diffie-Hellman parameters with name '%s' is not available in Murmur.", qPrintable(qsSSLDHParams));

This comment has been minimized.

Copy link
@hacst

hacst Aug 5, 2017

Member

Ideally we should either mention RFC 7919 or directly list the possible options so users don't have to rely on their ini being our original one with our comments to figure out the valid options. Are we likely to implement further named groups in the forseeable future?

mkrautz added 6 commits Jul 29, 2017
FFDHE: add NamedGroups method for getting a list of supported named g…
…roups.

Also updates TestFFDHE to exercise the new method (and, in doing so,
refactors the tests a bit).

The core check logic from exercise() is moved into a tryFFDHELookupByName
function, which is now used by both the exercise() test, as well as the
namedGroupsMethod() test that is added by this commit.
Meta: improve error message when sslDHParams contains unknown named g…
…roup.

This commit uses the new FFDHE::NamedGroups() method to improve the error
message Murmur shows when sslDHParams is set to an unknown "bundled"
group, i.e. a name that starts with @, such as @ffdhe2048.
Meta: make sslDHParams errors fatal instead of critical.
I expected qCritial to terminate Murmur. It doesn't.

@mkrautz mkrautz force-pushed the mkrautz:ffdhe-integration branch from 1ebc09f to 560261a Aug 6, 2017

@hacst
hacst approved these changes Aug 6, 2017
Copy link
Member

left a comment

Doesn't compile. Concept LGTM though.

return false;
}
}
#else
if (! qsSSLDHParams.isEmpty()) {
qCritical("MetaParams: This version of Murmur does not support Diffie-Hellman parameters (sslDHParams). Murmur will not start unless you remove the option from your murmur.ini file.");
QString qsSSLDHParamsIniValue = qsSettings->value(QLatin1String("sslDHParams"));

This comment has been minimized.

Copy link
@hacst

hacst Aug 6, 2017

Member

Missing .toString().

Meta: fix error message shown when using sslDHParams option with Qt w…
…ithout DH support.

The QSslDiffieHellmanParameters class is only available in Qt 5.8 or
greater. (And in our buildenv's Qt 5.6 as well.)

When using Qt 4, or Qt 5 below 5.8, Murmur should refuse to run if
sslDHParams is set in the .ini.

However, with the introduction of the "@ffdhe2048" default value,
this mechanism broke: since the default value is not empty, users
of a Qt without QSslDiffieHellmanParameters would not be able to run
Murmur unless the explicitly set sslDHParams to the empty string.

This commit fixes the problem by comparing against the value from the
.ini, instead of potentially also checking against the default value.

@mkrautz mkrautz force-pushed the mkrautz:ffdhe-integration branch from 560261a to 1ea4b92 Aug 6, 2017

@mkrautz mkrautz merged commit 9550487 into mumble-voip:master Aug 7, 2017

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
2 participants
You can’t perform that action at this time.