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

Bugfix #23 and improvements as suggested in #48 #50

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

arizon-dread
Copy link
Collaborator

@arizon-dread arizon-dread commented Jun 19, 2024

Bugfix and suggestions for improvement implemented

Fixes #23 by checking for error from clamd.ScanStream, which closes the connection if the file size is exceeded. The error was previously ignored. This PR includes code that assumes that a closed connection is caused by file size exceeded. Unfortunately, there is no way to detect the ^INSTREAM: file size limit exceeded error from the clamd process in the api. A custom clamd.ScanResult is created inside the error handling if statement, to handle the response in the same switch/case logic as other responses. This is also handled in the scanHandlerBody func the same way, so the client actually get a response.

Fixes #48 by creating a new endpoint (/v2/scan) and a scanResult struct that contains status, description and httpStatus (httpStatus is ignored in the json annotation, and only used in the code logic). An array of all scanned files (as []scanResult) is then marshalled to json, creating a proper json response for one to many files, returning an array of json objects to the client. The old /scan endpoint will also use this response (which is formatted the same way as before) but it will not be returned in an array, but as before, one to many json objects without proper json array structure, to keep previous behavior intact. Although, deprecation and link headers indicating that there's a new endpoint available, is returned from the old endpoint.

Previously, the first file's status would be the http status of the entire response, this PR includes code that will always return a 406 http status if any file contains a virus, and only return a 200 OK status if all files are clean, for both the old and new endpoint. I figured this was a bug.

I also added a prometheus metrics counter that increments on each found virus.

I also added go.mod and go.sum to use go modules instead of vendor directory etc.

@arizon-dread arizon-dread marked this pull request as draft June 19, 2024 09:00
@arizon-dread arizon-dread marked this pull request as ready for review June 19, 2024 09:08
@arizon-dread
Copy link
Collaborator Author

arizon-dread commented Oct 17, 2024

I got this working on my local machine yesterday after merging the original repo's master branch into my fork's scan-v2, successfully tested all the endpoints. Then I built the docker image and when starting that up, it can't parse the clamd.conf nor the freshclam.conf. I looked into it and it didn't look off from what I could see, although the "Foreground" setting seems to take "yes" as the value rather than "true", but changing that in the sed command in the dockerfile did nothing to solve the issue, I will look into it further tonight.

@arizon-dread
Copy link
Collaborator Author

@davosian Do you have any insights into this issue parsing the config files? It seems lilke the version that gets installed in alpine 3.20 is 1.22.r0 which is a little bit weird considering it's not a LTS release, but a release candidate.

@arizon-dread
Copy link
Collaborator Author

@davosian Hi, It seems to be ready for merge now. I have just successfully built and run it as a container using podman on linux mint 22, and using curl to try out the endpoints, it seems to work as expected. Feel free to pull my fork and give it a go if you like.

I have also investigated building from clamav/clamav:1.4.1 which also works (if you comment/remove the line installing clamav with apk, because that installs clamav 1.2.2.r0 instead of the newer 1.4.1). Should I make a new PR for that or do we want to follow the apk repo of alpine instead, climbing versions in a pace that alpine decides (this is also fine, just asking where the project is going)?

The clamav/clamav:1.4.1 also builds on alpine.

If there is some other place were project roadmap is discussed, I'd be willing to participate if there's interest.

@davosian
Copy link
Contributor

Hi @arizon-dread , I am happy to review your changes. Unfortunately, I will not get to it before next week though.

Regarding your question on following alpine for releases, I think being conservative for such an application makes sense. It would also keep things cleaner in our codebase and would would be less likely run into any alpine compatibility issues. Or do you have strong arguments for upgrading to 1.4.x now?

@davosian
Copy link
Contributor

Btw. I am currently the only maintainer which is not ideal for such a project. Would you be billing to help me maintain this project @arizon-dread?

@arizon-dread
Copy link
Collaborator Author

arizon-dread commented Oct 24, 2024

Hi @davosian!
It's great that you want to review it and know it's there. Waiting for a review for a little bit longer is no problem.

I think your view on keeping to the alpine version of clamav makes sense, the only issue I have with it is that it seems that the current 1.2.2.r0 in the apk repo is a release candidate and not a stable version, which I find a bit weird, either I have misunderstood it or some apk maintainer is too eager to upgrade before there's a stable release, or too slow to bump it to the stable release after the release candidate cycle to stable version is finished.
1.2.2-r0

I would just like to point out that the image I tested was the official clamav image (which also builds on alpine but packs and installs clamav on top of that). I will not stress this change, the way I see it now, it's not necessary to switch the base image, I don't have any strong arguments for it either (except what I have written above) so we can keep rolling with the alpine versions instead and the clamav package in the apk repo.

I would be willing to help you maintain it, but at least initially, I would appreciate if you could make the majority of decisions on the project direction, as you have history and knowledge that I don't have. I can certainly help with issues and PR's that include patches like bumping versions and reviewing code etc. like the ones we have seen in the project for the last few months.

@davosian
Copy link
Contributor

Hi @arizon-dread,

I finally got around going through the latest PR so that is about time get your changes merged into the project. Since you were kind enough to review PR #59, I was wondering whether your tests included the changes from this PR as well?

Great to hear about your openness to support the project! As you can tell my reactions are on the slow end (which I am hoping to improve) so any support I can get to keep this project active is a very welcome change.

I also just checked the version included in Alpine Edge and even there it is a release candidate (1.3.2-r0). Not sure though whether this is normal for Alpine's Edge versions.

@arizon-dread
Copy link
Collaborator Author

arizon-dread commented Nov 13, 2024

Hi @davosian

My tests in #59 did not include my code in this PR, I tested the PR's code as it was indented to be merged (checked out the branch from @christianbumann's fork). I have now merged the master branch into my scan-v2 after the PR merge of #59 and built it. Building and starting the container works but it seems I need to wait until tomorrow before new signatures are published so I can verify the version update, like @christianbumann had to do when he was working with #59, so I'll get back with results tomorrow.

Yes, the Alpine repos seems to be a bit off with the clamav versions, if you check the official clamav download page: clamav, they promote 1.4.1 LTS and 1.0.7 LTS. The 1.4.1 version is also the version on the official docker hub repo clamav dockerhub. That's why I was contemplating on if we should switch the base image to the official clamav image to get a LTS release of it instead, but if we decide to do that, I think it needs to be it's own PR from a clean fork from master. Not mixed with other stuff like the changes I have made in this PR. In that case, we need to make sure to look through the config even more thoroughly so they haven't made breaking changes or changed syntax, which would break our sed approach when building this image.

I'll get back tomorrow with version update results!

@arizon-dread
Copy link
Collaborator Author

It seems to be working as expected. I also fixed the centos.Dockerfile which was very outdated and wouldn't even build. Centos:stream8 is EOL and the repos are offline so I upgraded to stream9 and modified the dockerfile to create the folder structure used in the standard docker file to get it up and running (to match the entrypoint.sh script). The centos base image has clamav 1.0.7 in the repos so the version is slightly older.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants