Fix #820 - add new option --fail-if-cvd-older-than=days #867

rzvncj · 2023-03-22T15:15:13Z

When passed, causes clamscan to exit with a non-zero return code if the virus database is older than the specified number of days.

rzvncj · 2023-03-22T15:19:17Z

Please let me know if this is a good start. I'll continue with FailIfCvdOlderThan after we're good with the clamscan part (probably in its own commit).

clamscan/manager.c

micahsnyder

This is a great start!

I have some suggested changes but nothing functional.

I forgot to mention this on the previous PR, but in the spirit of #841, I would suggest adding both a clamd.conf option as well as a --fail-if-cvd-older-than command line option when you extend this for clamd. Which of course means adding documentatino as well to

clamd.conf.sample
clamd.conf manpage
clamd manpage

libclamav/cvd.c

clamscan/manager.c

libclamav/cvd.c

clamscan/manager.c

rzvncj · 2023-03-24T11:44:04Z

I assume the clamscan part is OK, so I'll start work shortly on the clamd part. Thanks for the quick review so far!

clamscan/manager.c

micahsnyder · 2023-03-25T23:05:53Z

As a heads up, we're prepping the 1.1 release candidate early next week. It'd be fun to get this in there, but if not it will have to wait for merge until after release in a few weeks, and be included in the following version (1.2) which will be released around August/September-ish.

Edit: To clarify, we don't like adding new features between the release candidate and release, though we may merge minor bug fixes during that time.

rzvncj · 2023-03-26T07:04:29Z

As a heads up, we're prepping the 1.1 release candidate early next week. It'd be fun to get this in there, but if not it will have to wait for merge until after release in a few weeks, and be included in the following version (1.2) which will be released around August/September-ish.

Edit: To clarify, we don't like adding new features between the release candidate and release, though we may merge minor bug fixes during that time.

Understood. No problem, I'll do my best to get to the clamd part ASAP. Thanks!

rzvncj · 2023-03-26T13:23:45Z

There's an additional clamd cl_load() call in server-th.c (in reload_th()) that I've not treated. I'm assuming that that code only runs after an actual database update so it would be pointless to guard it with check_if_cvd_outdated() but I could be wrong. Please let me know if that's something I should look at again. Thanks!

rzvncj · 2023-03-26T13:56:54Z

Also, if you prefer this PR to be in a single commit please let me know and I'm happy to squash and force-commit the result.

micahsnyder · 2023-03-27T20:54:30Z

There's an additional clamd cl_load() call in server-th.c (in reload_th()) that I've not treated. I'm assuming that that code only runs after an actual database update so it would be pointless to guard it with check_if_cvd_outdated() but I could be wrong. Please let me know if that's something I should look at again. Thanks!

The reload_th use of cl_load() is also done on the self-check that runs every x minutes (default every 10 minutes).
I can imagine someone complaining that they thought it would fail if the database hasn't been updated. But I don't have any strong feels about whether or not is should fail to reload if the database ages past the threshold.

If we do make the reload fail for old cvds, then to solve it I think you'd have to add an engine option to set the max database age. That's maybe more work than you want to do?

If we don't make the reload fail for old cvds, then we should adjust the wording in the clamd.conf.sample and clamd.conf manpage to say something along the lines of "This will not cause a database reload to fail if the database found on reload is older than this limit."

Also, if you prefer this PR to be in a single commit please let me know and I'm happy to squash and force-commit the result.

Don't worry about it, I will squash the commits and reword slightly when I merge it.

Speaking of which, I pushed a change to the win32 equivalent of the clamd.conf.sample for you, because I forgot to mention that it exists and must also be updated.

I'd like your opinion on what you think we should do regarding the reload_th cl_load. And @candrews too if you have thoughts you're welcome to chime in.

micahsnyder

In terms of code and documentation, everything looks great to me.

I manually tested clamd and clamscan behavior, including verifying that the clamd reload is not affected by the new options.

Marking as "request changes" because we'll need either a change to the clamd.conf documentation if we choose to keep the reload behavior as-is, or because we'd need more clamd changes to support the option in the reload use case.

rzvncj · 2023-03-27T21:21:46Z

There's an additional clamd cl_load() call in server-th.c (in reload_th()) that I've not treated. I'm assuming that that code only runs after an actual database update so it would be pointless to guard it with check_if_cvd_outdated() but I could be wrong. Please let me know if that's something I should look at again. Thanks!

The reload_th use of cl_load() is also done on the self-check that runs every x minutes (default every 10 minutes). I can imagine someone complaining that they thought it would fail if the database hasn't been updated. But I don't have any strong feels about whether or not is should fail to reload if the database ages past the threshold.

If we do make the reload fail for old cvds, then to solve it I think you'd have to add an engine option to set the max database age. That's maybe more work than you want to do?

If we don't make the reload fail for old cvds, then we should adjust the wording in the clamd.conf.sample and clamd.conf manpage to say something along the lines of "This will not cause a database reload to fail if the database found on reload is older than this limit."

The amount of needed work is irrelevant to me, there are only two important factors here IMHO:

Do we want this to be done ASAP (so it makes it into the upcoming release before the freeze this week)?
Do users want this feature? I can't really tell, because it's not something I thought of using before seeing @candrews' open issue, so in this respect his opinion probably carries more weight than mine.

If it's OK to postpone this for the next release (i.e. the answer to the first question is "not necessarily"), and if it makes sense for the cl_load() thread to fail, then I have no problem to continue working on this.

Also, if you prefer this PR to be in a single commit please let me know and I'm happy to squash and force-commit the result.

Don't worry about it, I will squash the commits and reword slightly when I merge it.

Speaking of which, I pushed a change to the win32 equivalent of the clamd.conf.sample for you, because I forgot to mention that it exists and must also be updated.

Thanks! Sorry about missing the Windows part - I guess it shows I'm a UNIX person. :)

micahsnyder · 2023-03-27T23:32:45Z

The amount of needed work is irrelevant to me, there are only two important factors here IMHO:

Do we want this to be done ASAP (so it makes it into the upcoming release before the freeze this week)?

Do users want this feature? I can't really tell, because it's not something I thought of using before seeing @candrews' open issue, so in this respect his opinion probably carries more weight than mine.

If it's OK to postpone this for the next release (i.e. the answer to the first question is "not necessarily"), and if it makes sense for the cl_load() thread to fail, then I have no problem to continue working on this.

I tested this change to the reload_th behavior to check the CVD age:
cvd_age_check_clamd_reload.patch

What I realized with this patch is that failing the CVD update does not cause clamd to die. Instead it intentionally continues on with the existing databases, which is very likely the preferred behavior.

So then I thought about it a little more about the problem. I realize that the scenario I was trying to account for should not happen. That is, it should only try to run the reload_th function if there was an update to the databases. And if there was an update to the databases then the CVD age should not exceed the limit.

In short, I'm happy with the PR as-is and don't care about any further documentation change after all.

Thanks! Sorry about missing the Windows part - I guess it shows I'm a UNIX person. :)

😆 No worries, I should have said something.

I'll make sure this all passes our automated Jenkins tests, and if so will merge it.

micahsnyder

I had to make a small change because initializing arrays with = {}; doesn't work on windows -- but you can initialize with = {0};. Outside of that testing looked good.

When passed, causes clamscan to exit with a non-zero return code if the virus database is older than the specified number of days.

When passed, causes clamd to exit (on start-up) with a non-zero return code if the virus database is older than the specified number of days. Additionally, we introduce FailIfCvdOlderThan as a clamd.conf synonym for --fail-if-cvd-older-than.

micahsnyder · 2023-03-28T04:56:43Z

Actually - one more change. The new clamav.h API in libclamav.map should be in a different namespace: CLAMAV_1.1.0.

rzvncj · 2023-03-28T05:39:49Z

Thanks!

micahsnyder · 2023-03-28T22:25:58Z

Thank you so much for the help @rzvncj

candrews · 2023-03-28T22:42:54Z

Thank you all! This is really fantastic!

rzvncj · 2023-03-29T06:43:00Z

Thank you so much for the help @rzvncj

You're welcome! It's been quick and pleasant. Thanks for the prompt reviews!

* Add new clamd and clamscan option --cache-size This option allows you to set the number of entries the cache can store. Additionally, introduce CacheSize as a clamd.conf synonym for --cache-size. Fixes Cisco-Talos#867

* Add new clamd and clamscan option --cache-size This option allows you to set the number of entries the cache can store. Additionally, introduce CacheSize as a clamd.conf synonym for --cache-size. Fixes #867

rzvncj mentioned this pull request Mar 22, 2023

Fix #820 - add new option --fail-when-outdated #832

Closed

rzvncj commented Mar 22, 2023

View reviewed changes

clamscan/manager.c Show resolved Hide resolved

micahsnyder reviewed Mar 23, 2023

View reviewed changes

micahsnyder reviewed Mar 25, 2023

View reviewed changes

clamscan/manager.c Outdated Show resolved Hide resolved

micahsnyder requested changes Mar 27, 2023

View reviewed changes

micahsnyder approved these changes Mar 28, 2023

View reviewed changes

rzvncj and others added 5 commits March 27, 2023 21:51

Fix #820 - add new clamscan option --fail-if-cvd-older-than=days

4ca6a0d

When passed, causes clamscan to exit with a non-zero return code if the virus database is older than the specified number of days.

Add FailIfCvdOlderThan option to windows clamd.conf.sample as well

04921fe

Initialize array in way compatible with MSVC compiler

284bc88

Move new cl_cvdgetage symbol to CLAMAV_1.1.0 namespace in libclamav.map

3888ce1

micahsnyder mentioned this pull request Mar 28, 2023

Clam 2291 rc soversion news #871

Merged

micahsnyder merged commit e4fe665 into Cisco-Talos:main Mar 28, 2023

rzvncj deleted the new-option2 branch March 28, 2023 21:36

candrews mentioned this pull request Mar 31, 2023

Add options: --cache-size, CacheSize #882

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #820 - add new option --fail-if-cvd-older-than=days #867

Fix #820 - add new option --fail-if-cvd-older-than=days #867

rzvncj commented Mar 22, 2023

rzvncj commented Mar 22, 2023

micahsnyder left a comment

rzvncj commented Mar 24, 2023

micahsnyder commented Mar 25, 2023 •

edited

Loading

rzvncj commented Mar 26, 2023

rzvncj commented Mar 26, 2023

rzvncj commented Mar 26, 2023

micahsnyder commented Mar 27, 2023

micahsnyder left a comment

rzvncj commented Mar 27, 2023

micahsnyder commented Mar 27, 2023

micahsnyder left a comment

micahsnyder commented Mar 28, 2023

rzvncj commented Mar 28, 2023

micahsnyder commented Mar 28, 2023

candrews commented Mar 28, 2023

rzvncj commented Mar 29, 2023

Fix #820 - add new option --fail-if-cvd-older-than=days #867

Fix #820 - add new option --fail-if-cvd-older-than=days #867

Conversation

rzvncj commented Mar 22, 2023

rzvncj commented Mar 22, 2023

micahsnyder left a comment

Choose a reason for hiding this comment

rzvncj commented Mar 24, 2023

micahsnyder commented Mar 25, 2023 • edited Loading

rzvncj commented Mar 26, 2023

rzvncj commented Mar 26, 2023

rzvncj commented Mar 26, 2023

micahsnyder commented Mar 27, 2023

micahsnyder left a comment

Choose a reason for hiding this comment

rzvncj commented Mar 27, 2023

micahsnyder commented Mar 27, 2023

micahsnyder left a comment

Choose a reason for hiding this comment

micahsnyder commented Mar 28, 2023

rzvncj commented Mar 28, 2023

micahsnyder commented Mar 28, 2023

candrews commented Mar 28, 2023

rzvncj commented Mar 29, 2023

micahsnyder commented Mar 25, 2023 •

edited

Loading