mumble-voip / mumble

Mumble is an open-source, low-latency, high quality voice chat software.
https://www.mumble.info
Other
6.22k stars 1.1k forks source link

SIGHUP should reload SSL certificates #1972

Closed simmel closed 7 years ago

simmel commented 8 years ago

When murmurd receives an SIGHUP it should reload the certificates configured in the .ini-file. Especially now when Let's Encrypt has become public (and their certs are currently only valid for 90 days).

On a side note, is it possible to change the server certificate via ICE? I couldn't find any method here https://github.com/mumble-voip/mumble/blob/5026c479/src/murmur/Murmur.ice

simmel commented 8 years ago

Ah, on http://wiki.mumble.info/wiki/Commercial_Hosting#Multiserver_support I found setConf(0,"certificate","-----BEGIN CERTIFICATE-----...").

Still, my SIGHUP question/request still stands.

mkrautz commented 8 years ago

I don't believe Qt's SSL API allows you to change certificates on the fly.

If that is true, this will require us to develop that feature and upstream it. Meaning most distros won't be able to use it for quite a while, since it requires a bleeding edge Qt.

There are two ways to interpret your request:

Murmur should reload the certificate it uses when it receives SIGHUP, but only for new connections. Everyone else stays on the server, and sees the old certificate.

or,

Murmur should reload the certificate it uses when it receives SIGHUP. This should disconnect all users, and update the certificate.

In general though, we tend not to use signals for these things, because of Murmur's virtual server support. For example, a virtual server can have a certificate configured through RPC. In that case, it wouldn't respect the certificate set in the .ini file (those are the "meta" settings, which apply to all virtual servers, unless the servers have overridden them).

So in general, such functionality would go through Ice. A signal would be nice, but would also confuse administrators -- since the .INI isn't the only configuration repository.

Now, I started out by saying that Qt doesn't support changing the certificate on the fly. I am actually not sure about that, and I think it might support it.

If it does, it should be as simple as using setConf() and updating the certificate. New users would see the new certificate.

You could try to play with that, and see if it works. That would be great.

Hope it helps, and thank you for the feature request.

simmel commented 8 years ago

Only new connections sees the new certificate.

simmel commented 8 years ago

And thanks for the very quick response!

mkrautz commented 8 years ago

Was that a response to how you would like it to work?

Or did you try it out? :-)

Sorry, but it's not clear from your message.

simmel commented 8 years ago

Oh sorry, how I would like it to work = )

simmel commented 8 years ago

And I did try it out but nothing happened.

fwaggle commented 8 years ago

@simmel Did setting the certificate via Ice setConf('certificate',<key in pem format>) work for you without restarting the server?

simmel commented 8 years ago

I haven't tried it. Do you have a quick way for me to test it?

fwaggle commented 8 years ago

Not really, but I tested it and it didn't work, but I couldn't tell from your posts whether it worked for you, and I was doing something wrong.

I think I have a two-line patch that will fix it though, I'll have to talk to the other devs and see if there's any reason it can't be this simple (won't solve the sighup thing but it's a step in the right direction).

mkrautz commented 8 years ago

Update: we're working toward implementing this at https://github.com/mumble-voip/mumble/pull/2221

haasn commented 8 years ago

Now that #2221 has been merged, how do I use this (as an administrator)?

What do I write in my shell script / systemd unit file to reload the murmur certificate?

fwaggle commented 8 years ago

@haasn It really needs to happen in a scripting language that Ice supports (perl, python and PHP should all work) - you simply read the key file and certificate file, and pass them as arguments to the new updateCertificate() method via Ice. I might have some Python scripts laying around to do it, they'll need tidying up and I'm buried under school work at the moment, but if someone else doesn't jump in with something first I'll release them in the next week or so. Reading files that are probably closely guarded by root in a scripting language is probably not pleasing.

The proper place for this to happen if you're using Lets Encrypt is in a LE module, which I tried to write but found the documentation for doing so to be extremely poor and gave up - things might have improved by now, so I'll be having another look at that when I get a chance.

mkrautz commented 8 years ago

We really need to add a small RPC clien to the murmurd binary... It's crazy that we have to tell people to write scripts to interface with Ice themselves for things like this...

fwaggle commented 8 years ago

@mkrautz I started doing it with my "mutter" tool - the idea was to have it included in Mumble's tree so that it would be included in the static builds. Slicer then wanted it wrapped up inside the Murmur binary for some reason and I got sidetracked a while after that. I've probably got an old fork lying around I can resurrect without much effort?

It only required boost and Ice to build, was vanilla C++ otherwise. I had considered using Qt inside it so it could grok Murmur.ini too, but I don't recall ever getting that far.

haasn commented 8 years ago

I personally still think the proper way to do this is with a signal like every single other server in existence (even stuff like nginx and apache do it this way, and they have support for multiple “virtual servers” as well, so please don't use that as an excuse)

It's worth noting that nginx etc. use this signal for “reload config” in general, i.e. you can update nginx/apache/postfix/whatever configuration without needing to restart the process at all - the experience is seamless for users. (Assuming you don't change the port or whatever)

I think it would also be a good idea to make murmur work like that, so stuff like systemctl reload can be implemented properly.

mkrautz commented 8 years ago

The problem is the way you configure Murmur. The config file isn't the whole truth, because virtual servers can override the Meta settings.

So setting an option in the ini and reloading (if it were implemented) might not reload all settings.

For example if a virtual server uses an auto-generated self-signed certificate, it is stored in the DB. In that case, setting a global cert/key in the ini and reloading wouldn't have an effect, because the virtual server's own setting takes precedence.

So, we could implement a reload mechanism that will reload values from the ini, only if a virtual server doesn't override it.

But to be friendly, to avoid confusion, we really need a tool that can show a configuration dump for a virtual server. The idea being that such a tool would make it very clear where a particular config value was set...

haasn commented 8 years ago

@mkrautz In that case, restarting murmur would also not change the configuration, right?

I think the reload command should be considered a strictly weaker version of restart that doesn't require dropping the connections or restarting the whole process. So if restart does not update a setting, reload should not either.

Essentially, I think the right way to approach SIGHUP semantically would be as a “reload files from disk” option. If something is not loaded from disk, it would not be affected. But my personal use case for systemctl reload is pretty much universally this: “I've made changes to the config file, now I want these to be applied”

To this end, I think it would be a good idea to separate the concerns of “reload certificates” and “reload files”, since as you said, some certificates are not stored in files. For simple configurations (like mine), a simple solution (like restarting mumble - or ideally, just reloading) would suffice.

For a more complicated configuration, some ice-based python script would be more appropriate, yes, but I'm not sure how many users have need for such complicated configurations.

simmel commented 8 years ago

So, we could implement a reload mechanism that will reload values from the ini, only if a virtual server doesn't override it.

Yes, please. And log if a virtual server overrides it and you're not going to update the cert.

But to be friendly, to avoid confusion, we really need a tool that can show a configuration dump for a virtual server. The idea being that such a tool would make it very clear where a particular config value was set...

Yes, also a good idea. I like @fwaggle's mutter tool and I also like Slicer's idea to integrate it into murmurd BUT let's make that another issue/PR so you can close this one.

oplehtinen commented 7 years ago

@fwaggle Found those scripts? Mumble server is the last one to figure out for automatic renewal :)

fwaggle commented 7 years ago

@dimits afraid not sorry, i seem to have destroyed the VM that probably contained them. I'll try and whip something up soon - it's not terribly hard to do if you like a scripting language that Ice supports, it's one or two lines to change the certificate then just run that script after letsencrypt does it's thing.

I suspect what might have happened is I rage quit and deleted the entire VM after I couldn't figure out how to write an automation module for LetsEncrypt itself. :(

oplehtinen commented 7 years ago

This is is somewhat messy, but does the job for me, combined with a cron shell script.

https://github.com/Astraalivankila/Update-Mumble-Server-Certificates

simmel commented 7 years ago

Sweet, thanks for that!

mkrautz commented 7 years ago

I'd like, for 1.3.0 final, to have SIGHUP be able to reload the certificates for all virtual servers. But not anything else.

Simply loop over them, and try to update the server's certificate live, if it uses a certificate/private key from a file.

The operation can be dangerous, because you can't get any feedback if it fails. I suppose it should verify that the certificate and key matches before updating -- and log a warning in the log file if the operation failed for a particular server.

In the future, it would be nice if we could drop Murmur's self-signed certificates, and be able to rely on an ACME library to automatically provision certificates. But let's not get ahead of ourselves. :-)

fwaggle commented 7 years ago

@mkrautz could you just have the loop that iterates over the certificates leverage the updateCertificate() method internally? That way you get feedback if the certificate/key don't match, aren't any good, etc (does updateCertificate() check if the hostname configured in registerhostname is listed on the certificate? that sort of thing) and you could very easily scream loudly into the log file but have the server continue with the old certificate.

Also regarding the second point, I think that would be getting rather nasty, as you need a way to verify the hostname to LE - usually done by dropping a file in /.well-known/ on the www server attached to that hostname (if no web server is running you can start one, but that brings other issues on Unix-like OSes, such as privileged ports), which would complicate things a lot. It'd probably almost be simpler to have the registration server run it's own low-sec cert authority. :/

mkrautz commented 7 years ago

@fwaggle To the second point - there are various libraries out there. Something like https://godoc.org/golang.org/x/crypto/acme/autocert + grumble would be a good first start.

mkrautz commented 7 years ago

@mkrautz could you just have the loop that iterates over the certificates leverage the updateCertificate() method internally? That way you get feedback if the certificate/key don't match, aren't any good, etc (does updateCertificate() check if the hostname configured in registerhostname is listed on the certificate? that sort of thing) and you could very easily scream loudly into the log file but have the server continue with the old certificate.

Yes, it would use something like updateCertificate() internally -- or we'd refactor updateCertificate into another method and use that.

I think hostname checking might be too much to check, and not the norm to check at config-time for a server-type program.

simmel commented 7 years ago

On Tue, 2017-02-14 at 15:21:55 -0800, Mikkel Krautz wrote:

I'd like, for 1.3.0 final, to have SIGHUP be able to reload the certificates for all virtual servers. But not anything else.

But also the configured one in the config file?

Looking forward to the feature!

mkrautz commented 7 years ago

@simmel Only the one configured in the config file. That's the only place you can target a file. :-)

WIP PR at https://github.com/mumble-voip/mumble/pull/2850 - don't mind the messy commits.... I'll need to clean it up and document it before it's ready.

mkrautz commented 7 years ago

Oh, and it'll be SIGUSR1, not SIGHUP.

SIGHUP is already used by Murmur for log rotation.

anlutro commented 7 years ago

Have you considered simply reloading the certificate from the filesystem on every new connection?

mkrautz commented 7 years ago

@anlutro I don't think that's a good idea. Changing the certificate and private key is an operation that can potentially fail. That would leave people unable to join the server.

But! We've already implemented SIGUSR1-based reload. It's in https://github.com/mumble-voip/mumble/pull/2850

Closing. :-)

GigabyteProductions commented 7 years ago

I'm not familiar with the release times; what version did this make it into? murmur 1.2.18 closes all connections upon receiving SIGUSR1.

mkrautz commented 7 years ago

@GigabyteProductions It is available in 1.3.0, which is not yet released. Snapshot builds are available on our website, https://www.mumble.info/.