Bugfix #23 and improvements as suggested in #48

[arizon-dread]

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]

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]

@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]

@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]

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]

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]

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.