search

mailserver2 / mailserver

Simple and full-featured mail server using Docker
https://store.docker.com/community/images/mailserver2/mailserver
MIT License
133 stars 28 forks source link

config changes for latest clamav-unofficial-sigs 7.2.5 #46

Closed diroots closed 1 year ago

diroots commented 1 year ago

Thanks for submitting a pull request ! Please provide enough information so that others can review your changes.

For more information, see the CONTRIBUTING guide.

Description

clamav-unofficial-sigs script version loaded in mailserver2 repo is Version: v5.6.2 (2017-03-19)

actual latest version is 7.2.5 : https://github.com/extremeshok/clamav-unofficial-sigs/releases/tag/7.2.5

Fixes https://github.com/mailserver2/mailserver/issues/47

Type of change

Status

Todo List

there is a config validation to perform on the os.conf file, as in the mailserver version of the file, only clamd_reload_opt is defined, and in the latest version of this file in the original repo, only clamd_restart_opt is defined.

also in this file, there is the socket param activated in mailserver version, but not in the original repo version. i did let it enabled for now, to reflect mailserver's setup

How has this been tested ?

tested on a custom implementation of mailserver stack

AndrewSav commented 1 year ago

It looks like we are copyingclamav-unofficial-sigs.sh and master.conf from https://github.com/extremeshok/clamav-unofficial-sigs. What do you guys thing of the following suggestion:

This will make obvious that this is an external dependency, will save time checking that no errors were made copying files from another repo and we will have more or less straight forward process of updating it in future.

@diroots @sknight80 @SaraSmiseth opinions?

sknight80 commented 1 year ago

Sounds good to me. How we will collect the right hash tag and checksum? Do you think we can automate it?

AndrewSav commented 1 year ago

How we will collect the right hash tag and checksum? Do you think we can automate it?

That's what I mean by Perhaps create a simple shell script for getting the checksum(s) we can paste on update. We will always need a human to test that the latest of that other repo works for us. When time comes to update our repo from theirs we select commit hash / tag on the other repo and run that script which will get the checksums, Now we can put the new commit hash / tag and the new checksums in our repo.

This should not be difficult, but it requires a bit of time to implement.

We can get even fancier with GitHub actions, and detect that if someone committed a new commit hash / tag from the other repo, we can go get the checksum and author another commit with the checksums - I know this should be possible I did something similar elsewhere. I would not burden our contributor with this part though and do not have time for that right now myself. We can always improve this later once we get started.

diroots commented 1 year ago

It looks like we are copyingclamav-unofficial-sigs.sh and master.conf from https://github.com/extremeshok/clamav-unofficial-sigs. What do you guys thing of the following suggestion:

* Pull the file during the docker build instead of copying

* Identify it for pulling by commit hash / tag

* Authenticate it with checksum(s) that we will store in our repo and re-calculate each time we change the commit hash / tag to pull from

* Perhaps create a simple shell script for getting the checksum(s) we can paste on update

This will make obvious that this is an external dependency, will save time checking that no errors were made copying files from another repo and we will have more or less straight forward process of updating it in future.

@diroots @sknight80 @SaraSmiseth opinions?

it should be interesting to get files based on their latest tag, ie : https://github.com/extremeshok/clamav-unofficial-sigs/tree/7.2.5

and then grab directly all 3 files from there :

see https://github.com/extremeshok/clamav-unofficial-sigs/blob/7.2.5/guides/ubuntu-debian.md#install

AndrewSav commented 1 year ago

it should be interesting to get files based on their latest tag, ie : https://github.com/extremeshok/clamav-unofficial-sigs/tree/7.2.5

and then grab directly all 3 files from there :

  • clamav-unofficial-sigs.sh
  • config/master.conf
  • config/os/os.debian.conf (this one needing renaming to os.conf and also the specific changes for mailserver setup, ie. s6svc and socket)

see https://github.com/extremeshok/clamav-unofficial-sigs/blob/7.2.5/guides/ubuntu-debian.md#install

Sounds reasonable. I understand that os.debian.conf require some modifications though, so cannot be grabbed as it, how do we resolve it? Also @diroots would you be willing to implement this?

AndrewSav commented 1 year ago

For what it's worth I had a conversation on skarnet irc with regards to the restart issue. They informed me that using s6-svc -d in a finish script across the board as we do is misguided. It can only make sense in a rare case when supervise should not bring back the process after it finished, e.g it's not a daemon. Since most of our processes are daemons, this finish script is not very suitable for them, and this is something that probably could be improved in future. For me the main snag is to identify which process can legitimately finish.

diroots commented 1 year ago

Since most of our processes are daemons, this finish script is not very suitable for them, and this is something that probably could be improved in future.

https://github.com/mailserver2/mailserver/blob/71688532934217c117d79e80b89b2bd36eeadefc/rootfs/usr/local/bin/setup.sh#L731 there "Foreground" is set to true, so clamd might not be demonized

diroots commented 1 year ago

Sounds reasonable. I understand that os.debian.conf require some modifications though, so cannot be grabbed as it, how do we resolve it?

in the MR i proposed to get the os.conf from their os.debian.conf but in the end I modified it to make it similar to your os.conf based on s6,... so could be not worth it to use the debian based one proposed, as we nearly do not keep any params from it.

sknight80 commented 1 year ago

Sounds reasonable. I understand that os.debian.conf require some modifications though, so cannot be grabbed as it, how do we resolve it?

in the MR i proposed to get the os.conf from their os.debian.conf but in the end I modified it to make it similar to your os.conf based on s6,... so could be not worth it to use the debian based one proposed, as we nearly do not keep any params from it.

I am okay with keeping the os.conf file that we had before. But I am also fine, to grab the os.debian.conf and write a script that is going to update to our setup (this potentially more work)

sknight80 commented 1 year ago

@AndrewSav are you okay with this PR? If so, can we merge it?

AndrewSav commented 1 year ago

@sknight80 Sorry, I did not realise it was waiting on me which it seems it is. I will get back to you soon, I think that some of the changes that we discussed above are better be done in scope of this PR, but I cannot tell you which one just at this second, I'll try to review and get back before Monday.

AndrewSav commented 1 year ago

there "Foreground" is set to true, so clamd might not be demonized

I think conceptually the are still demons, we use Foregraound simply because we are using s6 for supervising them. As such I think my earlier comment you are replying to still stands.

AndrewSav commented 1 year ago

@sknight80 @diroots I left my comments in the change thread above. Thanks! Sorry for the delay I have no idea how this has fallen through the cracks I was somehow sure it was not on my plate.

AndrewSav commented 1 year ago

@diroots hey it seems that the test fails on something around clamav, do you mind having a look?

diroots commented 1 year ago

@AndrewSav fix pushed, can u retry tests?

AndrewSav commented 1 year ago

@diroots can you rebase on master please?

AndrewSav commented 1 year ago

I built mailserver2/mailserver:test_cus from this PR, I rebased it locally. It seems to run fine. I think once we rebase it and see that the tests are green we will be ready to merge. Thank you again for all your hard work, and apologies for the very slow pace from our side.

diroots commented 1 year ago

@diroots can you rebase on master please?

done @AndrewSav

AndrewSav commented 1 year ago

@diroots this is in 1.1.14 image now. Thank you again for making this happen!