-
Notifications
You must be signed in to change notification settings - Fork 3
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
Issue 1291: Implement storage of ClamAV scan results #1321
Issue 1291: Implement storage of ClamAV scan results #1321
Conversation
…V, added new security app to house auditing models
…s, added permissions for ClamAVFileScan model
write_only=True, | ||
validators=[validate_data_file] | ||
) | ||
file = serializers.FileField(write_only=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This validation was moved to the serializers validate_file
method instead so we could access serializer context.
data_file = DataFile.create_new_version(validated_data) | ||
|
||
# Determine the matching ClamAVFileScan for this DataFile. | ||
av_scan = ClamAVFileScan.objects.filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DataFile
doesn't get created until after the scan is complete and recorded so we have to retroactively link these records if the creation was successful.
"""Perform all validation steps on a given file.""" | ||
file, user = attrs['file'], attrs['user'] | ||
validate_file_extension(file.name) | ||
validate_file_infection(file, user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic previously existed in the validators validate_data_file
method.
"""Validate file is not infected by scanning with ClamAV.""" | ||
av_client = ClamAVClient() | ||
try: | ||
is_file_clean = av_client.scan_file(file, file.name) | ||
is_file_clean = av_client.scan_file(file, uploaded_by) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to track the user that uploaded the file or we won't have anything to link the LogEntry
to.
|
||
# Log and create audit records with the results of this scan | ||
logger.debug(msg) | ||
ClamAVFileScan.objects.record_scan( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The execution flow was altered here so we only return at the end, after creating the ClamAVFileScan
instance. This allows us to track all results from ClamAV - not just the CLEAN
files.
except ConnectionError as err: | ||
logger.debug(f'Encountered error scanning file: {err}') | ||
logger.error(f'ClamAV connection failure: {err}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case we can't connect to ClamAV we will not store a result for the scan.
class ClamAVFileScanManager(models.Manager): | ||
"""Extends object manager functionality with common operations.""" | ||
|
||
def record_scan( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a helper function to easily create a ClamAVFileScan
instance at the same time as a linked LogEntry
.
) | ||
|
||
data_file = models.ForeignKey( | ||
DataFile, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is nullable, since we don't store INFECTED or ERROR files we will only have this link if the file scan was CLEAN.
from django.db import migrations | ||
|
||
from tdpservice.users.permissions import ( | ||
add_permissions_q, | ||
create_perms, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulled this logic out so it can be re-used as new models are added.
|
||
operations = [ | ||
# We need to call this again to ensure permissions are created for | ||
# the newly added ClamAVFileScan model. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the comment above
|
||
def set_clamav_file_scan_permissions(apps, schema_editor): | ||
"""Set permissions for the ClamAVFileScan model for relevant groups.""" | ||
ofa_admin = apps.get_model('auth', 'Group').objects.get(name='OFA Admin') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only OFA Admin and OFA System Admin will be able to see these results. No one may edit them other than the superuser.
|
||
def validate_file(self, file): | ||
"""Perform all validation steps on a given file.""" | ||
user = self.context.get('user') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This context needs to be passed in by the implementing view(s) in the get_serializer_context
method.
"""If a serializer has valid data it will return a valid object.""" | ||
create_serializer = DataFileSerializer(data=data_file_data) | ||
create_serializer = DataFileSerializer( | ||
context={'user': user}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to manually pass this context in tests as it is used in validation steps.
def get_serializer_context(self): | ||
"""Retrieve additional context required by serializer.""" | ||
context = super().get_serializer_context() | ||
context['user'] = self.request.user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what gives the validate_file
method on the serializer access to the user.
Codecov Report
@@ Coverage Diff @@
## raft-tdp-main #1321 +/- ##
=================================================
+ Coverage 98.40% 99.42% +1.01%
=================================================
Files 38 41 +3
Lines 939 1036 +97
Branches 47 56 +9
=================================================
+ Hits 924 1030 +106
+ Misses 10 2 -8
+ Partials 5 4 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Ok, got some time to review here more deeply and looks good. Will approve/move to QASP once you have been able to resolve that 500 error we're seeing once deployed. |
I'm not sure why it was missing, but that error was due to a missing AV_SCAN_URL environment variable. I added that back and everything worked as expected in sandbox. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great @jtwillis92! reference re: our async discussion about frontend alert message for other unsuccessful submissions is captured in #831 here. as discussed, we'll also need to handle that scenario where multiple files are submitted, with at least one infected
and one clean
.
ticked off qasp checklist in main PR summary, so @abottoms-coder ready to merge!
Co-authored-by: Alex P. <63075587+ADPennington@users.noreply.github.com>
Summary of Changes
Closes issue #1291
Adds the capability for the
tdrs-backend
application to store all results from files scanned by ClamAV and adds an associated LogEntry that is viewable in the Django admin.This is done through the use of a new model
ClamAVFileScan
which represents an individual file scan performed by ClamAV. When theClamAVClient.scan_file
method is called it will now create an instance of this model along with aLogEntry
instance that is tied to theClamAVFileScan
instance. In the event the scan is successful and aDataFile
is produced, it will also be linked to thisClamAVFileScan
so that the scanned file can be retrieved as necessary for auditing purposes. In the event the scan is rejected, we will not store the file itself so thedata_file
link will be empty for these records.Additionally, adds a new Django app called
security
to house all of the ClamAV related code and the new model. This app is intended to be used for any security auditing requirements (such as the upcoming #1161)How to Test
This change is only visible in Django Admin to
OFA Admin
andOFA System Admin
users. For the purposes of testing this, it is simplest to just work through as an OFA Admin.Log Entries
listsecurity | Clam AV File Scans
:ClamAVFileScan
instances:DataFile
instance and has all relevant details:DataFile
instance and all relevant details:Log Entries can be filtered by month if more than one month is present in the logs:
Clam AV File Scans can also be viewed altogether in their own admin list:
Deliverable 1: Accepted Features
ClamAVFileScan
)As Product Owner, @lfrohlich will decide if ACs are met.
Deliverable 2: Tested Code
Deliverable 3: Properly Styled Code
Deliverable 4: Accessible
n/a
Deliverable 5: Deployed
Deliverable 6: Documented
Deliverable 7: Secure
no new issues detected
Deliverable 8: Context
reviewed by @ADPennington