ZoneMinder / zoneminder

ZoneMinder is a free, open source Closed-circuit television software application developed for Linux which supports IP, USB and Analog cameras.
http://www.zoneminder.com/
GNU General Public License v2.0
4.9k stars 1.2k forks source link

initial support for systemd #474

Closed knight-of-ni closed 9 years ago

knight-of-ni commented 9 years ago

* DO NOT MERGE (yet) *

DESCRIPTION systemd replaces traditional init. It is currently in use on Fedora, OpenSUSE, and Mageia. It has just been implemented in the newly release CentOS 7, and as of February, Debian has announced they will be replacing upstart with systemd, which means that Ubuntu will follow. Since it seems nearly all distros are moving to systemd, we should make sure zoneminder integrates well with it.

PROBLEM One of the "features" of systemd is that it tracks, internally, the PID of the service at the time the service was started. Consequently, if the administrator restarts the service in question using something other than the systemctl tool, then systemd becomes out of sync with the service, and one runs into issues where the service is reported as failed when, in fact, it is running fine (or vice versa). In the case of zoneminder, this problem occurs when zoneminder is stopped or restarted from the web console.

SOLUTION I have attempted to solve this by adding a wrapper script and a polkit policy file. Instead of calling zmpkg.pl directly, the web console will call zmsystemctl.pl, which in turn calls systemctl to stop/start/restart the service. A polkit policy is required during this process to elevate the webuser account permissions.

TO-DO This pull request is not quite finished, but the basics are there.

Specifically, here is what still needs to be done:

Let me know if anyone knows of a better way to implement this.

coling commented 9 years ago

Hiya!

Firstly thanks for this! This is the same approach I suggested as a possible solution on a Mageia bug so it's good that someone has found the time to actually do it, unlike me!!

Can't speak for any typos and it actually working but from a quick look over, it looks good!

Thanks!

akuckartz commented 9 years ago

+1

barjac commented 9 years ago

Excellent ! Well done. I have tested a new package build for Mageia patched to git master with this change applied and it works perfectly. Stopping/starting zm from either web interface or systemctl (or mixing both) works without issues. @coling thanks for your help - it was your suggestion in the Mageia bug that prompted this :)

knight-of-ni commented 9 years ago

My primary motivation came from this bug report: https://bugs.mageia.org/show_bug.cgi?id=6475

I had to learn what polkit was.

Note that things will still get messed up if the root user executes zmpkg.pl start/stop/restart directly from the command line, but that seems to be the nature of systemd. Let's call that a "feature", rather than a bug.

I will finish up this pull request as time allows.

coling commented 9 years ago

Yeah, I did a lot of polkit work not too long ago, so I feel your pain there!

As for the zmpkg.pl and zmdc.pl scripts (disclaimer: I've only given them the most cursory of glances), it seems that these do a lot of the work that an good init+logging system should be doing, so the fact that they conflict somewhat with systemd isn't massively surprising I guess!

In a fictitious world where you only supported systemd, these scripts (or a good chunk of their functionality) could probably disappear. Certainly the zmdc.pl could probably be replaced with a central controlling zoneminder.target file that collected all the individual daemon processes which themselves were all split into separate .service units with appropriate dependency and ordering information between them (allowing proper resource scheduling if needed but still with the convenience of a single start/stop/enable command)

The logrot command would no longer be necessary with fully automatic log rotation in the journal and the status command could be replaced with the adoption of the notification protocol to allow daemons to send back status information to the service manager which is then displayed in any service management interface (e.g. running the "systemctl status httpd" command currently includes a line:

Status: "Total requests: 254; Current requests/sec: 0; Current traffic: 0 B/sec" )

So while I think the scripts do a great job as is, if you were to fully embrace systemd (or drink the kool aid depending on your leanings! :p), then a lot of the stuff that was perhaps previously unique, would end up just fitting in with general service management and fully utilize the features that are now available!

Not that I'm suggesting this should be done, just trying to make sure you're aware of the possibilities should you wish to explore them at any time in the future!

Having just bought myself a little camera thingy, I really should take a look at jumping into the ZM world properly :D Keep up the Good Work!

kylejohnson commented 9 years ago

zmpkg.pl is a wrapper to zmdc.pl - so we are wrapping our wrapper.

While I haven't been involved in this issue yet, something what @coling said made a lot of sense to me.

Perhaps we should rethink our current scripts. Which distros, which ZoneMinder runs on, do not use systemd? There is also FreeBSD to consider.

knight-of-ni commented 9 years ago

Good feedback here. There is a table in systemd Wiki, which tells us which distros use systemd as the default init: http://en.wikipedia.org/wiki/Systemd

The only Linux distro I noticed was not mentioned was Slackware. After Googling, it does not appear that FreeBSD will be using systemd.

zmpkg.pl is a wrapper to zmdc.pl - so we are wrapping our wrapper.

Yes, exactly. I noticed that too. My spidy sense is telling me there is a better way, but I've yet to figure that out. There is a lot going on in zmpkg.pl which precludes us from simply calling zmdc.pl directly.

Perhaps a better solution resides in combining zmdc & zmpkg into a single package, and stripping out the stuff that is no longer necessary. So I think @kylejohnson and @coling we are all thinking the same thing. I just need to keep thinking about this to figure out a plan.

Ultimately, my feelings won't be hurt if we decide to ditch the wrapper script altogether for something more streamlined.

connortechnology commented 9 years ago

I'm totally down with rethinking our scripts. I think fully integrating with systemd is the way to go, with our scripts becoming legacy support for non-systemd systems.

PX03AFK commented 9 years ago

I had also started looking at a solution for this but I was looking at a mechanism whereby zmpkg.pl itself would recognise the environment and call systemctl via a new piece of code which would elevate itself. My thinking was that we wouldn't need to change anything more than zmpkg.pl and add the new piece of code, which would be specific to zoneminder.

coling commented 9 years ago

Out of curiosity, how were you intending to make the new bit of code elevate itself? I presume it would involve a setuid binary of some kind? IMO, it makes sense to avoid setuid binaries if possible as it lowers the general attach surface. This is why I proposed to use polkit/pkexec to do the privilege escalation whenever possible (and using polkit is effectively just the same thing as a self-contained setuid binary anyway, although comes with more auditing via logging and gives the admin the ability to override this with policy overrides if they so desire)

Longer term, it should just be possible to ship appropriate policy rules regarding the user running the command and the systemctl operations that that user is allowed to run, but I'm not sure all the necessary bits are in place for that right now, but in theory calling it via a simple pkexec'd or setuid wrapper now and switching over to specific policy file when it's supported seems sensible.

knight-of-ni commented 9 years ago

@coling No, the wrapper script is not setuid. The way I'm implementing this is via polkit's pkexec, but it's integrated right into the shebang of the perl script. The first line of the zmsystemctl.pl script reads:

#!/usr/bin/pkexec /usr/bin/perl

The included polkit policy & rule file elevate the permission of zmsystemctl.pl only if it is called by the webuser account.

I ended up going this route because polkit does not seem to be as flexible as adding a command alias into sudoers, for example. Apparently polkit can only match up to one commandline argument, unlike sudoers. So to elevate a command like "systemctl restart zoneminder", I have to write a wrapper script if I don't want to give the webuser account the ability to stop/start any service on the system.

@PX03AFK I'd be interested to know what criteria you were using to autodetect a systemd environment. I'm sure there are a lot of ways to do it, but I'm not sure what would be the best.

Right now I'm being distracted by some autotools issues. I hate autotools. I think I've just gotten past them so I should be able to get back to working on this pull request.

coling commented 9 years ago

Sorry @knnniggett, my previous comment was actually directed at @PX03AFK (i.e. he mentioned that his solution would "elevate itself" so I was curious as to how this would be achieved and what benefit it has over the polkit/pkexec approach you took :)

PX03AFK commented 9 years ago

Hi Colin,

I hadn't looked at this for a while and I started to look again today to remind myself where I was heading. I was concerned about providing something which might be misused so I went along the path of having a service identity in the zm.conf file and the code simply getting that service id and only allowing start, stop or restart with it. I've attached the source of the application I have been working on, you might recognise the code which reads through zm.conf.

I started looking a zmpkg.pl and zmdc.pl with a view to having an additional set of 'hidden' commands which (sysstart, sysstop and sysrestart) which would only be invoked if the SERVICE record exists in zm.conf and zmpkg.pl would invoke itself with the 'hidden' command and then within zmdc.pl at the relevant point zmtcl would be invoked. I hadn't worked out all the details but that was the direction I was heading in. By doing that way whereve zmpkg.pl was called from it would result in systemctl being used in all instances.

David


From: Colin Guthrie [mailto:notifications@github.com] Sent: 20 July 2014 15:18 To: ZoneMinder/ZoneMinder Cc: David Wilcox Subject: Re: [ZoneMinder] initial support for systemd (#474)

Out of curiosity, how were you intending to make the new bit of code elevate itself? I presume it would involve a setuid binary of some kind? IMO, it makes sense to avoid setuid binaries if possible as it lowers the general attach surface. This is why I proposed to use polkit/pkexec to do the privilege escalation whenever possible (and using polkit is effectively just the same thing as a self-contained setuid binary anyway, although comes with more auditing via logging and gives the admin the ability to override this with policy overrides if they so desire)

Longer term, it should just be possible to ship appropriate policy rules regarding the user running the command and the systemctl operations that that user is allowed to run, but I'm not sure all the necessary bits are in place for that right now, but in theory calling it via a simple pkexec'd or setuid wrapper now and switching over to specific policy file when it's supported seems sensible.

Reply to this email directly or view https://github.com/ZoneMinder/ZoneMinder/pull/474#issuecomment-49547542 it on GitHub. https://github.com/notifications/beacon/6486570__eyJzY29wZSI6Ik5ld3NpZXM6Qm VhY29uIiwiZXhwaXJlcyI6MTcyMTQ4NTA2OCwiZGF0YSI6eyJpZCI6MzY4NzI5NzB9fQ==--3ea1 a332735ebc44c9f6269399a2c0dfd98f3187.gif

PX03AFK commented 9 years ago

I'd not quite finished the work I had been looking at but I was going to use a relatively simple method (which I've now remembered) of getting zmpkg.pl to know it was being called from systemd - not that this is foolproof.

zmpkg.pl would need to check the presence of a ZM_SERVICE_NAME entry in zm.conf and if present and the command line was start, restart or stop it would call zmctl (the new app) with the same parameter and then exit

The systcmctl service file would call zmpkg.pl with the same parameter but prefixed with sys

zmpkg.pl would check whether the command line started with sys and if so skip the check above but then change the command to remove the sys prefix and then continue as at present.

That represents a fairly small change and means that it doesn't matter where zmpkg.pl is called from as the action will always be the same.

The gap it leaves is that if a user calls zmpkg.pl using the sys prefix commands it would bypass systemctl. An alternative might be that zmpkg.pl finds out who is calling it and if systemctl runs as at present and if not calls the app.

David


From: Andrew Bauer [mailto:notifications@github.com] Sent: 20 July 2014 16:33 To: ZoneMinder/ZoneMinder Cc: David Wilcox Subject: Re: [ZoneMinder] initial support for systemd (#474)

@coling https://github.com/coling No, the wrapper script is not setuid. The way I'm implementing this is via polkit's pkexec, but it's integrated right into the shebang of the perl script. The first line of the zmsystemctl.pl script reads:

!/usr/bin/pkexec /usr/bin/perl

The included polkit policy & rule file elevate the permission of zmsystemctl.pl only if it is called by the wbeuser account.

I ended up going this route because polkit does not seem to be as flexible as adding a command alias into sudoers, for example. Apparently polkit can only match up to one commandline argument, unlike sudoers. So to elevate a command like "systemctl restart zoneminder", I have to write a wrapper script if I don't want to give the webuser account the ability to stop/start any service on the system.

@PX03AFK https://github.com/PX03AFK I'd be interested to know what criteria you were using to autodetect a systemd environment. I'm sure there are a lot of ways to do it, but I'm not sure what would be the best.

Right now I'm being distracted by some autotools issues. I hate autotools. I think I've just gotten past them so I should be able to get back to working on this pull request.

Reply to this email directly or view https://github.com/ZoneMinder/ZoneMinder/pull/474#issuecomment-49550130 it on GitHub. https://github.com/notifications/beacon/6486570__eyJzY29wZSI6Ik5ld3NpZXM6Qm VhY29uIiwiZXhwaXJlcyI6MTcyMTQ4OTU2NCwiZGF0YSI6eyJpZCI6MzY4NzI5NzB9fQ==--9b30 336e1fdccdcf1d812cf48c034657649993b9.gif

PX03AFK commented 9 years ago

Hi Colin,

It's taken me a little time to get back into what I was trying to do but I do now have something working which only changes zmpkg.pl but in such a way as wherever it is called it redirects to systemctl for start, stop and restart unless it is called from systemd, in which case it carries as normal. The redirect is using my binary but what I had forgotten is that I probably have to set the sticky bit for this binary for it work from everywhere and as yet I still haven't quite got it sorted for general use. At present I can run zmpkg.pl from root (which is where I have zoneminder installed) and from the console but not another user - not sure if that is important.

As the binary will only direct a command at systemctl for the service defined in zm.conf I don't see that as too high a risk, unless somebody wanted to deliberately use zm.conf to direct the command to another service, but then they would have to be able to modify zm.conf to achieve it.

David

PX03AFK commented 9 years ago

I realised after posting the last comment that the problem is with using zmpkg.pl from root - my proposed binary will work from anywhere

knight-of-ni commented 9 years ago

@PX03AFK Can you create a feature branch and then generate a pull request? That way we can compare the two potential solutions. It doesn't have to be perfect. As long as the pull request is not merged, we can always push changes to it later to tweak things.

PX03AFK commented 9 years ago

OK, but it might take me a few days to put it all together.


From: Andrew Bauer [mailto:notifications@github.com] Sent: 23 July 2014 15:22 To: ZoneMinder/ZoneMinder Cc: David Wilcox Subject: Re: [ZoneMinder] initial support for systemd (#474)

@PX03AFK https://github.com/PX03AFK Can you create a feature branch and then generate a pull request? That way we can compare the two potential solutions. It doesn't have to be perfect. As long as the pull request is not merged, we can always push changes to it later to tweak things.

Reply to this email directly or view https://github.com/ZoneMinder/ZoneMinder/pull/474#issuecomment-49880120 it on GitHub. https://github.com/notifications/beacon/6486570__eyJzY29wZSI6Ik5ld3NpZXM6Qm VhY29uIiwiZXhwaXJlcyI6MTcyMTc0NDQ5NCwiZGF0YSI6eyJpZCI6MzY4NzI5NzB9fQ==--d897 f40b32a096d61a6f91ba1ebaa14b5b195fdc.gif

knight-of-ni commented 9 years ago

No problem. I'm not going to be able to wrap this up for a few weeks anyway as my availability is limited.

PX03AFK commented 9 years ago

I've completed the work I was doing on my proposed solution and have created the branch as requested and issued a pull. I'm obviously open to comments and thoughts if there are better ways of doing it. Also, I'm not a C++ or perl programmer so my code may not look as neat as others would doubtless make it.

knight-of-ni commented 9 years ago

I'm going to create a new feature branch that combine some of the ideas from this pull request and from David's pull request.

Since my head starts to hurt each time I try to think through the logic steps, I've written them down (my head hurts less now). I am going implement this entirely in zmpkg.pl. This means that I'll have to use polkit to elevate the permissions of the web user anytime zmpkg.pl is called, but I don't believe this will be an issue (let me know if you think otherwise). This may have the added side effect of allowing zmpkg to create any missing temp folders in a few cases in which it could not.

systemd_flowchart

PX03AFK commented 9 years ago

Can't fault the direction you are going in. I'm not familiar with polkit but as it obviously provides what is needed and my solution was the wrong way to go then it is the better choice. I see that you liked my idea of keeping the changes confined to zmpkg.pl as your flowchart appears to be the way I approached it.

One thought about elevating the permissions when zmpkg.pl is called, if this is not a systemd environment or there is no service defined there would be no necessity to elevate. I can see the potential benefit in doing so in all cases but if it isn't necessary then would it be better not to.

David


From: Andrew Bauer [mailto:notifications@github.com] Sent: 08 August 2014 22:55 To: ZoneMinder/ZoneMinder Cc: David Wilcox Subject: Re: [ZoneMinder] initial support for systemd (#474)

I'm going to create a new feature branch that combine some of the ideas from this pull request and from David's pull request.

Since my head starts to hurt each time I try to think through the logic steps, I've written them down (my head hurts less now). I am going implement this entirely in zmpkg.pl. This means that I'll have to use polkit to elevate the permissions of the web user anytime zmpkg.pl is called, but I don't believe this will be an issue (let me know if you think otherwise). This may have the added side effect of allowing zmpkg to create any missing temp folders in a few cases in which it could not.

https://cloud.githubusercontent.com/assets/5150042/3863225/35f54918-1f46-11 e4-9e19-406f23b707f1.png systemd_flowchart

Reply to this email directly or view https://github.com/ZoneMinder/ZoneMinder/pull/474#issuecomment-51662235 it on GitHub. https://github.com/notifications/beacon/6486570__eyJzY29wZSI6Ik5ld3NpZXM6Qm VhY29uIiwiZXhwaXJlcyI6MTcyMzE1NDEwMSwiZGF0YSI6eyJpZCI6MzY4NzI5NzB9fQ==--5687 2d708e9ccbccb915f02c9c5bb3883f946d55.gif

knight-of-ni commented 9 years ago

Change that. I'm moving the call to systemctl into its own wrapper script and elevating the permission of just that script.

PX03AFK commented 9 years ago

OK, I see the logic of doing that. Are you happy to carry on with that or would you like me to do it.


From: Andrew Bauer [mailto:notifications@github.com] Sent: 09 August 2014 21:29 To: ZoneMinder/ZoneMinder Cc: David Wilcox Subject: Re: [ZoneMinder] initial support for systemd (#474)

Change that. I'm moving the call to systemctl into its own wrapper script and elevating the permission of just that script.

Reply to this email directly or view https://github.com/ZoneMinder/ZoneMinder/pull/474#issuecomment-51697638 it on GitHub. https://github.com/notifications/beacon/6486570__eyJzY29wZSI6Ik5ld3NpZXM6Qm VhY29uIiwiZXhwaXJlcyI6MTcyMzIzNTM1OSwiZGF0YSI6eyJpZCI6MzY4NzI5NzB9fQ==--fa6b 73834f38b3a49bce70e94d9bd9fdf39066c5.gif

knight-of-ni commented 9 years ago

I've got it all written, and I'm testing it now. Testing is going to take me a bit since I've got to build & test it four times to cover all the combinations of autotools/cmake and systemd/init.

knight-of-ni commented 9 years ago

I am closing this request out in favor of this one: #502