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.
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.
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.
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.
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.
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.
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.
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.
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.
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. |
|
|
||
| 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.
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.
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.
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.
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. |
andrew-jameson
added
QASP Review
and removed
raft review
ADPennington
added
Deploy with CircleCI-qasp
ADPennington
added
Deploy with CircleCI-qasp
There was a problem hiding this comment.
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!
ADPennington
added
Ready to Merge
and removed
QASP Review
Deploy with CircleCI-qasp
Co-authored-by: Alex P. <63075587+ADPennington@users.noreply.github.com>
Summary of Changes
Closes issue #1291
Adds the capability for the
tdrs-backendapplication 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
ClamAVFileScanwhich represents an individual file scan performed by ClamAV. When theClamAVClient.scan_filemethod is called it will now create an instance of this model along with aLogEntryinstance that is tied to theClamAVFileScaninstance. In the event the scan is successful and aDataFileis produced, it will also be linked to thisClamAVFileScanso 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_filelink will be empty for these records.Additionally, adds a new Django app called
securityto 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 AdminandOFA System Adminusers. For the purposes of testing this, it is simplest to just work through as an OFA Admin.Log Entrieslistsecurity | Clam AV File Scans:ClamAVFileScaninstances:DataFileinstance and has all relevant details:DataFileinstance 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