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

Various minor fixes in preparation for hot-cert reload #2879

Merged
merged 7 commits into from Feb 26, 2017

Conversation

@mkrautz
Copy link
Member

commented Feb 26, 2017

  • Commit 1: Change MetaParams::typeCheckedFromSettings to take an optional QSettings parameter.
  • Commit 2: Add USR1 signal handler implementation.
  • Commit 3: Move callsite of qmConfig.clear() in MetaParams::read() to allow MetaParams::read() to be split out into separate functions.
  • Commit 4: Add Server::bUsingMetaCert flag, such that hot cert reload knows which servers to reload.
  • Commit 5: Add qAbsSettingsFn, such that hot cert reload knows where to read murmur.ini.
  • Commit 6: Move SSL check and OpenSSL version advertisement from MetaParams::read() to main.cpp.
  • Commit 7: Avoid use of global QSslSocket::defaultCiphers() list; prefer applying the default list to each QSslSocket/QSslConfiguration individually.

@mkrautz mkrautz force-pushed the mkrautz:hot-reload-certs-prep branch 4 times, most recently from 4f7e28f to 4585f6d Feb 26, 2017

@mkrautz mkrautz requested review from Kissaki, hacst and davidebeatrici Feb 26, 2017

@Kissaki
Copy link
Member

left a comment

A QSettings that is not the one known to Meta? Seems strange with Meta still holding a QSettings. Will have to see how it looks when used.

Commit 3 d5b773c has a logically duplicate paragraph it seems.

Commit 4 4e3b6c0 says hot reloading will only be possible for meta certs. Why? Does it mean the new codepath that will be added via usr signal? Isn't hot loading already possible for vservers?

Commit 5 fdc87ad "in order to re-load the
settings from the same file.".

Meta.cpp#L156 and L161 should be qsAbsSettingsFn rather than fname.

We are only using the ciphers list in two places:

  • New user connections
  • Network requests to our registration service

Shouldn't there also be our initial server listening?

@@ -121,20 +127,31 @@ UnixMurmur::UnixMurmur() {
if (sigaction(SIGTERM, &term, NULL))
qFatal("Failed to install SIGTERM handler");

usr1.sa_handler = usr1SignalHandler;
sigemptyset(&term.sa_mask);
term.sa_flags = SA_RESTART;

This comment has been minimized.

Copy link
@Kissaki

Kissaki Feb 26, 2017

Member

This should be usr1 and not term?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Feb 26, 2017

Author Member

Yes

@@ -121,20 +127,31 @@ UnixMurmur::UnixMurmur() {
if (sigaction(SIGTERM, &term, NULL))
qFatal("Failed to install SIGTERM handler");

usr1.sa_handler = usr1SignalHandler;
sigemptyset(&term.sa_mask);

This comment has been minimized.

Copy link
@Kissaki

Kissaki Feb 26, 2017

Member

This should be usr1 and not term?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Feb 26, 2017

Author Member

Yes

char a = 1;
ssize_t len = ::write(iUsr1Fd[0], &a, sizeof(a));
Q_UNUSED(len);
}

This comment has been minimized.

Copy link
@Kissaki

Kissaki Feb 26, 2017

Member

What is this strance logic? What does this even do?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Feb 26, 2017

Author Member

It writes to iUser1Fd, which will then trigger a QSocketNotifier to read from the fd, which can then trigger the signal handling inside Qt's event loop.

It's basically a trick to move the signal handling into Qt, because signal contexts are very fragile.

void UnixMurmur::handleSigUsr1() {
qsnUsr1->setEnabled(false);
char tmp;
ssize_t len = ::read(iUsr1Fd[1], &tmp, sizeof(tmp));

This comment has been minimized.

Copy link
@Kissaki

Kissaki Feb 26, 2017

Member

What does this even do?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Feb 26, 2017

Author Member

See previous explanation.

@@ -131,6 +131,7 @@ class Server : public QThread {
QVariant qvSuggestPositional;
QVariant qvSuggestPushToTalk;

bool bUsingMetaCert;

This comment has been minimized.

Copy link
@Kissaki

Kissaki Feb 26, 2017

Member

Needs to be initialize in the constructor to false.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Feb 26, 2017

Author Member

Done

@@ -125,6 +129,9 @@ class MetaParams {
QVariant qvSuggestPositional;
QVariant qvSuggestPushToTalk;

/// qsAbsSettingsFn is the absolute path to
/// the murmur.ini used by this Meta instance.
QString qsAbsSettingsFn;

This comment has been minimized.

Copy link
@Kissaki

Kissaki Feb 26, 2017

Member

Why is it called qsAbsSettingsFn when it's not a function pointer?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Feb 26, 2017

Author Member

fn as in filename

Should I avoid the short-hand?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Feb 26, 2017

Author Member

Changed it to Filename.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Feb 26, 2017

Member

It’s a file path, not a file name tho, right? :)

The commit description still talks about qAbsSettingsFn

qVersion());
}

qlCiphers = filtered;

This comment has been minimized.

Copy link
@Kissaki

Kissaki Feb 26, 2017

Member

The entire codepath is broken (maybe from a conflict resolving?). filtered is defined twice, and the one that is never filled is used here.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Feb 26, 2017

Author Member

No, it is actually the correct one that is used. However, you are correct that there is a duplicate.
I moved the filtered var into the block when I refactored.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

A QSettings that is not the one known to Meta? Seems strange with Meta still holding a QSettings. Will have to see how it looks when used.

We need to reload the settings. The way that happens is that I create a new QSettings and update the MetaParams with the new SSL settings.

I don't even think MetaParam's own QSettings object is necessary once we've called MetaParams:.read(), will look into it...

Commit 3 d5b773c has a logically duplicate paragraph it seems.

Fixed.

Commit 4 4e3b6c0 says hot reloading will only be possible for meta certs. Why? Does it mean the new codepath that will be added via usr signal? Isn't hot loading already possible for vservers?

Kinda. When I say hot cert reloading, I mean the "send USR1 to the murmur process to reload the cert files specified in murmur.ini".

Should I be more specific? The USR1 signal will not load certs for vservers, since they can't change without using RPC, and if you're using RPC, you should use the updateCertificate() server method instead.

Commit 5 fdc87ad "in order to re-load the settings from the same file.".

Done

Meta.cpp#L156 and L161 should be qsAbsSettingsFn rather than fname.

Fixed.

We are only using the ciphers list in two places:

New user connections
Network requests to our registration service
Shouldn't there also be our initial server listening?

Qt doesn't provide a SSL server socket, so the way we do it is we have a TCP server, and for each incoming connection, we wrap it in QSslSocket to "upgrade" it to a SSL connection.
This is how you're supposed to do it in Qt.
So: there isn't a listning SSL socket... They're all client sockets as far as the server is concerned.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

@mkrautz mkrautz force-pushed the mkrautz:hot-reload-certs-prep branch from 4585f6d to 8aa042b Feb 26, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

This PR is now @ hot-reload-certs-prep-v2.

@Kissaki

This comment has been minimized.

Copy link
Member

commented Feb 26, 2017

Should I be more specific?

Yes, I think so.

we will only be able to perform our live reload for those
servers.

implies there is no live reloading for vservers, when it's just a different mechanism (that already can live reload/set). Not sure how to formulate (point to separate mechanism, or word differently...).

if (ciphers.size() != filtered.size()) {
qWarning("Warning: all cipher suites in sslCiphers using Diffie-Hellman key exchange "
"have been removed. Qt %s does not support custom Diffie-Hellman parameters.",
qVersion());

This comment has been minimized.

Copy link
@Kissaki

Kissaki Feb 26, 2017

Member

This is aligned with tabs and a space, but the above line with spaces. Should all be spaces?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Feb 26, 2017

Author Member

Done.

@Kissaki

This comment has been minimized.

Copy link
Member

commented Feb 26, 2017

Commit description still talks about qAbsSettingsFn

mkrautz added 7 commits Feb 26, 2017
Meta: add 'QSettings *' parameter to MetaParams::typeCheckedFromSetti…
…ngs().

This allows the setting to be loaded from an arbitrary QSettings instance.

We need this to be able to implement live reloading of SSL settings via the SIGUSR1 signal.
UnixMurmur: Add USR1 signal handler for reloading SSL settings.
Hot certificate reloading will use the USR1 signal.

This adds the meat of the implementation to UnixMurmur, such that
the PR for live-reloading SSL settings via the SIGUSR1 signal is
more easily digestable.
Meta: move qmConfig.clear().
This moves the qmConfig.clear() call in MetaParams::read() to the
top of the function.

This allows the SSL part of MetaParams::read() to be moved out to a
separate function in the upcoming PR for live-reloading SSL settings
via the SIGUSR1 signal.
Server: add bUsingMetaCert flag.
This flag is necessary for hot certificate reload.

We need to know which servers are using the Meta certificate/key,
since we will only be able to live-reload SSL settings via SIGUSR1
for those servers. Servers that use their own SSL certificate/key
can't be reloaded via the SIGUSR1 mechanism.

This is because servers that use their own SSL certificate/key store
them in the database. Thus, it is only possible to update those via
RPC using the updateCertificate() method.
Meta: add qAbsSettingsFilePath variable.
In order to reload the SSL settings live, we need to know the .ini file
Murmur used when initially loading its settings, in order to re-load the
settings from the same file.
Move SSL check and version log message to main.cpp from MetaParams::r…
…ead().

This change moves the QSslSocket::supportsSsl() check from MetaParams:read()
to main.cpp. This also ensures that the SSL check happens very early in Murmur's
initialization process, rather than after we might have already attempted to
call some SSL-related APIs.

This change also moves the log message that prints the OpenSSL version to main.cpp,
and integrates it with the QSslSocket::supportsSsl() check.
Meta: remove use of global QSslSocket::defaultCiphers() list.
This removes use of the global QSslSocket::defaultCiphers() list.

Instead, we set the ciphers on each socket individually.

@mkrautz mkrautz force-pushed the mkrautz:hot-reload-certs-prep branch from 8aa042b to 1a0e145 Feb 26, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

@kisski said:

Should I be more specific?

Yes, I think so.

we will only be able to perform our live reload for those
servers.

implies there is no live reloading for vservers, when it's just a different mechanism (that already can live reload/set). Not sure how to formulate (point to separate mechanism, or word differently...).

Rewrote the commit messages.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

Commit description still talks about qAbsSettingsFn

Done. It's now called qsAbsSettingsFilePath.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2017

@Kissaki PTAL

@mkrautz mkrautz merged commit 1a1bd8c into mumble-voip:master Feb 26, 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.