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

WIP: add OpenRC service scripts #132

Closed
wants to merge 8 commits into from
Closed

WIP: add OpenRC service scripts #132

wants to merge 8 commits into from

Conversation

orlitzky
Copy link
Contributor

As promised, here are four upstream service scripts for clamav-miler, clamd, clamonacc, and freshclam that make use of the new root-owned PID files.

To test this out, I've been running

$ git clean -x -f -d
$ ./autogen.sh
$ ./configure --disable-clamdtop --enable-milter --enable-clamonacc --enable-openrc --prefix=/home/mjo/tmp/clamav
$ make runstatedir=/home/mjo/tmp/clamav/run
$ make install

but of course you'll want to replace /home/mjo/tmp/clamav with a path of your own. This needs some more testing, but I believe everything kinda works the way it should. Feedback would be most welcome at this point!

@orlitzky
Copy link
Contributor Author

Some additional notes for testers:

  1. You'll have to rename all of the *.conf.sample files sans the .sample before anything will work. You also have to comment out the Example line in each conf file.
  2. In the resulting clamd.conf and clamav-milter.conf, the PidFile line has to be uncommented (I opened a feature request on bugzilla that would let us skip this step).
  3. Some additional settings in the conf file need to be specified, like the clamd socket path for clamd/clamav-milter and the include/exlude paths for clamonacc.

Copy link
Contributor

@micahsnyder micahsnyder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of the openrc subdirectory containing only 1 file. I would prefer if it the file was colocated next to the systemd service scripts. One option might be to name them like so:

clamd/clamav-daemon.openrc.in
clamonacc/clamav-clamonacc.openrc.in
clamav-milter/clamav-milter.openrc.in
freshclam/clamav-freshclam.openrc.in

and then CMake + Autotools could rename them to something else to match openrc's expectations at compile time?
Thoughts?

@@ -18,13 +18,32 @@
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
# MA 02110-1301, USA.

EXTRA_DIST = clamd.conf.sample freshclam.conf.sample clamav-milter.conf.sample
EXTRA_DIST = clamd.conf.sample.in.in \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like the idea of generating the sample files.

Is the double-.in.in a typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe here it's overkill, see below.

@@ -48,7 +48,7 @@ Example

# This option allows you to save the process identifier of the daemon
# Default: disabled
#PidFile /var/run/freshclam.pid
#PidFile @RUNSTATEDIR@/freshclam.pid
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to generate this file, there are a lot of other options (and Default: values we could generate as well.

Potentially, with the CMake support, we could generate the proper values for Windows as well and maybe even eventually do away with the win32-specific samples. There are some options which don't exist on Windows though (on-access and such), so maybe not. 🤷‍♂️

clamd/Makefile.am Show resolved Hide resolved
@orlitzky
Copy link
Contributor Author

I'm not a huge fan of the openrc subdirectory containing only 1 file.

I tried this first, here's my anti-sales-pitch. To get autotools to rename the file, I had two options:

  1. Change the "edit" rule to rename foo.openrc.in -> foo, as opposed to foo.openrc.in -> foo.openrc as it is now.
  2. Use an install-exec-hook to rename the file at install-time.

I rejected the first option because it makes the rule harder to reuse; afterwards, it only works on files that you've given an openrc.in suffix, rather than being a general "perform substitution on foo.in to produce foo" rule. The second option isn't great, either. The hook runs after every executable is installed, so right away you have to do a test for [ -f foo.openrc ] within the hook because those files may not exist yet. And then that gets run every time any executable file is installed, cluttering up the output and wasting time. That's not such a big deal, but the rename also breaks uninstallation. Automake only knows how to install/uninstall the things you've listed in Makefile.am. After the hook renames the file, make uninstall attempts to remove the old name, and leaves the new name on the system. Again, you can hack around that... but at this point, I said "you know what, this is stupid," and moved it to a separate directory.

@orlitzky
Copy link
Contributor Author

Change the "edit" rule to rename foo.openrc.in -> foo, as opposed to foo.openrc.in -> foo.openrc as it is now.

I'm pretty sure that I didn't want to copy/paste a special rule for OpenRC-type files at the time, but in hindsight, this wouldn't have worked anyway because the name conflicts with the clamd, freshclam,... executables.

@orlitzky
Copy link
Contributor Author

We'll let our users abuse this in Gentoo for the 0.103 release cycle. If it actually works, I'll come back and ask for it to be merged in 0.104!

This commit adds a workaround to configure.ac to support the many
distributions who patch "runstatedir" support into their autoconf.
The point of "runstatedir" variable is to make the location of the
runtime state directory configurable independent of "localstatedir",
so that, for example, PID files can be placed in /run instead of the
traditional /var/run.

Autoconf v2.70 supports runstatedir out of the box, but it has not
been released yet. When it is, and after AC_PREREQ has been bumped,
this chunk of code can simply be deleted. In the meantime it lets
us replace @runstatedir@ within certain files by running e.g.

  $ make runstatedir=/run

to override the default at build time. The default value is
@localstatedir@/run, and is backwards-compatible.
The sample configuration files for clamav-milter, clamd, and freshclam
all have PidFile directives that can make use of the @RUNSTATEDIR@
variable, since one primary purpose of the runstatedir is to hold PID
files. Particularly clamav-milter and clamd must keep the
configuration file directive and the "pidfile" service script
variables synchronized. This commit inserts @RUNSTATEDIR@ into those
commented directives, such that they agree with the corresponding
OpenRC service scripts. The PidFile directive in freshclam.conf is
ignored by its OpenRC service script -- as should be the case with the
others eventually -- but the same change was made there for consistency.

This commit also adds an "editgnudirs" command to the top-level
Makefile.am that is used to substitute @bindir@, @sbindir@, and
@RUNSTATEDIR@ into a file. Or more precisely, it substitutes those
variables into a template file named "foo.in" to produce "foo". That
new rule is then applied to the sample configuration files which have
been renamed to (for example) clamd.conf.sample.in.in. The ./configure
script processes that to clamd.conf.sample.in, and "make" applies
variable substitutions to produce clamd.conf.sample.

The end result of this change is that the OpenRC service scripts work
out-of-the-box when the clamd and clamav-milter PidFile directives are
merely uncommented, and do not need to be edited.
This commit adds four new OpenRC service scripts under <daemon>/openrc:

  * clamav-milter
  * clamd
  * clamonacc
  * freshclam

These scripts are primarily useful on Gentoo, where OpenRC is the
default service manager, and on Debian (and derivatives) where it is a
supported option. The scripts belong upstream because they need to
know a few paths that are provided to the build system, namely the
place where the executables themselves are located, and the place
where PID files are stored. So, the (s)bindir and runstatedir.

A forthcoming commit will perform substitutions for those directory
variables at build-time, eliminating the need for every distribution
to customize its own service scripts.
This commit adds the necessary magic to Makefile.am for clamav-milter,
clamd, clamonacc, and freshclam to convinces the build system to
install the corresponding OpenRC service scripts. For now, the scripts
are installed to $(sysconfdir)/init.d, which is generally the correct
location -- unless OpenRC itself was pointed at a non-standard path
The location could be made configurable (as with the systemd service
path), but at the moment that seems like overkill.
Most users won't want to install four OpenRC service scripts to
$(sysconfdir)/init.d, so this commit makes it optional, and hides it
behind an --enable-openrc (default: no) flag.
When using a local socket, the service manager needs to create a
directory that is writable only by the clamav user. Typically
/run/clamav is used. The clamd daemon then creates a socket named, for
example, /run/clamav/clamd.ctl. That's currently what the clamd
systemd service does, and this commit brings the sample configuration
file and the OpenRC service to parity.

First, the default LocalSocket location was updated in the sample
configutation file to point to @RUNSTATEDIR@/clamav/clamd.ctl. The
previous path "/tmp/clamd.socket" is a security vulnerability, the
setting is disabled by default, and the new path is what everyone is
using anyway modulo perhaps the name of the socket. So the change is
relatively backwards-compatible.

Second, the clamd OpenRC service was updated to create the directory
@RUNSTATEDIR@/clamav and to make it writable by the clamav user/group.
This makes local socket configurations work almost out-of-the-box
(after you uncomment the setting), and is harmless otherwise.
The changes made for clamd in the previous commit are valid for the
milter as well, which uses its own socket.
There's already a TCP socket example for the ClamdSocket setting in
clamav-milter's sample configuration file. This commit adds one
for a local socket, too, based on the (disabled) default value
in the sample clamd.conf file.
@orlitzky
Copy link
Contributor Author

We'll let our users abuse this in Gentoo for the 0.103 release cycle. If it actually works, I'll come back and ask for it to be merged in 0.104!

No major complaints, so I guess let's do this.

@micahsnyder micahsnyder changed the base branch from dev/0.103 to dev/0.104 December 5, 2020 02:26
@micahsnyder
Copy link
Contributor

This feels a little mean to ask -- but would you be willing to make this work with CMake (too or instead)?
We'll be deprecating autotools in 0.104 and 🤞 removing it one day.

@orlitzky
Copy link
Contributor Author

orlitzky commented Dec 5, 2020

I don't know CMake well enough to do so... it may not even be possible.

@micahsnyder
Copy link
Contributor

CMake is in general easier to read and write than autoconf+automake. It works similarly where you can set variables in the CMakeLists.txt file, and then have them filled out when it generates a file from a .in file.

The syntax for the .in file when using CMake is similar but not always identical to that when using autotools. For our CMake build we configure (generate) a handful of files here:
https://github.com/Cisco-Talos/clamav-devel/blob/dev/0.104/CMakeLists.txt#L621

Note that for a couple of these I use .cmake.in because I couldn't simply reuse the autotools .in files:

@orlitzky
Copy link
Contributor Author

orlitzky commented Dec 5, 2020

CMake is worse than autotools from my perspective. Its fatal flaw is that the thing that generates your build system has to be present at build time. Compare with autotools, where the end users need only a POSIX shell to run ./configure. This has several nasty consequences:

  • You now need C++ support everywhere to build plain C packages.
  • CMake is a moving target, so some packages may need an older version while others need a newer version. Dependency conflict.
  • As CMake gets larger and more convoluted, it accrues more and more dependencies itself. Guess what some of them use for their build systems? Circular dependencies.
  • Your package can't run on any systems that CMake hasn't been ported to.

If it had any tangible benefits, I might be willing to invest the time. But as it stands, I'd have to...

  • Learn enough CMake to rewrite the whole thing
  • Spend probably a week rewriting and testing it
  • Rewrite all of the other autotools fixes that we carry for CMake
  • Figure out how to redo the Gentoo ebuild to use CMake instead of autotools
  • Test the new CMake version of the patch on our users all over again

all with the goal (in the best possible case, which happens with 0% probability) of.... everything working exactly the same afterwards?

I won't be mad if you don't want to merge this, but rewriting it all isn't a good use of my time. I can keep it as a patch until someone else decides to tackle the CMake migration.

@micahsnyder
Copy link
Contributor

I'm sad to hear that you prefer autotools. CMake is significantly better from my perspective.

CMake is worse than autotools from my perspective. Its fatal flaw is that the thing that generates your build system has to be present at build time. Compare with autotools, where the end users need only a POSIX shell to run ./configure. This has several nasty consequences:

* You now need C++ support everywhere to build plain C packages.

Autotools still requires make (or gmake). With CMake you may use make or preferably ninja (which is crazy fast ❤).

At least for ClamAV the C++ requirement doesn't make any difference. ClamAV has code written in C, C++, Objective C (when on macOS), and in 0.104 it will have Rust code. Speaking of which, Rust (aka Cargo) will be a new requirement to build ClamAV in 0.104+.

* CMake is a moving target, so some packages may need an older version while others need a newer version. Dependency conflict.

CMake is very conscious about backwards compatibility though it's true that CMake is under very active development. In addition to a minimum CMake version, you can set a maximum CMake version so newer versions of CMake will maintain the behavior of older CMake versions for your project.

One exciting thing about newer versions of CMake: there was a major paradigm change in CMake 3 to build object files, libraries, and executables as "targets". Targets dramatically improve how include directory and linker dependencies are propagated from your dependencies to your libraries and executables. This is the main reason CMake is way better than autotools from a developer standpoint. Add to that the fact that CMake works for our Windows builds as well means we could have 1 build system instead of 2. I believe ClamAV 0.104 will require CMake 3.14 which is pretty recent, though they are up to 3.19.1 now. Despite the rapid development on CMake, CMake is generally easy to install a recent version. Even if your distro doesn't have a new enough version you can get CMake from Python's pip / PyPI package system.

* As CMake gets larger and more convoluted, it accrues more and more dependencies itself. Guess what some of them use for their build systems? Circular dependencies.

TBH I've never thought about CMake's build dependencies. CMake being 1 application seemed like a huge improvement dependency-wise to me, versus autotools which requires make, autoconf, automake, m4, and libtool (not all of which are made by the same people or seem to be maintained).

* Your package can't run on any systems that CMake hasn't been ported to.

I'm really curious to know if there are platforms CMake hasn't been ported to. I haven't heard any complaints along this line, though I haven't investigated it much either. CMake is very widely adopted. It seems more likely you'll have concerns about platforms that aren't "tier 1" for the Rust toolchain.

If it had any tangible benefits, I might be willing to invest the time. But as it stands, I'd have to...

* Learn enough CMake to rewrite the whole thing

* Spend probably a week rewriting and testing it

* Rewrite all of the other autotools fixes that we carry for CMake

* Figure out how to redo the Gentoo ebuild to use CMake instead of autotools

* Test the new CMake version of the patch on our users all over again

all with the goal (in the best possible case, which happens with 0% probability) of.... everything working exactly the same afterwards?

I won't be mad if you don't want to merge this, but rewriting it all isn't a good use of my time. I can keep it as a patch until someone else decides to tackle the CMake migration.

That does sound like a bit more work than I expected. I 100% understand - I don't have much spare time either.

Food for thought - for organizational purposes, it would probably be better to do:

services/clamav-milter.openrc.in
services/clamav-milter.service.in
services/clamd.openrc.in
services/clamd.service.in
services/clamonacc.openrc.in
services/clamonacc.service.in
services/freshclam.openrc.in
services/freshclam.service.in

instead of:

clamav-milter/openrc/clamav-milter.in.in
clamav-milter/clamav-milter.service.in
clamd/openrc/clamd.in.in
clamd/clamd.service.in
clamonacc/openrc/clamonacc.in.in
clamonacc/clamonacc.service.in
freshclam/openrc/freshclam.in.in
freshclam/freshclam.service.in

But I don't know about renaming etc/freshclam.conf.sample -> etc/freshclam.conf.sample.in to turn the config samples into config sample templates. If we do it - this one would certainly require CMake support to be added at the same time and ideally using the same template, to reduce duplication. As a side note, I only relatively recently realized that our current samples originated from the clamconf utility, which has it's defaults compiled (a feature which I suspect no-one uses and actually complicates our option parser quite a bit ☹). I've been contemplating removing that from clamconf and from our option parser in order to simplify the amount of things we have to maintain. It certainly doesn't help that the option descriptions are also duplicated in our manpage templates... sigh.

Anyways, given that we don't intend to keep autotools for the long haul I'm inclined to decline this PR until someone decides to tackle CMake migration, as you suggested.

@orlitzky
Copy link
Contributor Author

orlitzky commented Dec 6, 2020

At least for ClamAV the C++ requirement doesn't make any difference. ClamAV has code written in C, C++, Objective C (when on macOS), and in 0.104 it will have Rust code. Speaking of which, Rust (aka Cargo) will be a new requirement to build ClamAV in 0.104+.

Yikes.

@micahsnyder micahsnyder added the 🚧experiment just an experiemnt, do not merge label Oct 29, 2021
@micahsnyder micahsnyder marked this pull request as draft January 14, 2022 19:05
@orlitzky orlitzky closed this Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚧experiment just an experiemnt, do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants