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 26 commits into
base: master
Choose a base branch
from

Conversation

arizon-dread
Copy link

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

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