From 624b2a83853d0ae9b0afa09efa00657f469ca7f4 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 13 Dec 2016 22:51:29 -0500 Subject: [PATCH] Have security scanner analyze only send notifications for *new* layers Following this change, anytime a layer is indexed by the security scanner, we only send notifications out if the layer previously had a security_indexed_engine value of `-1`, thus ensuring it has *never* been indexed previously. This will allow us to change to version of the security scanner upwards, and have all the images be re-indexed, without firing off notifications in a spammy manner. --- data/database.py | 6 +++++- test/test_secscan.py | 39 +++++++++++++++++++++++++++++++++++++++ util/secscan/analyzer.py | 13 +++++++++---- 3 files changed, 53 insertions(+), 5 deletions(-) diff --git a/data/database.py b/data/database.py index a377ee6eb..d3c816cec 100644 --- a/data/database.py +++ b/data/database.py @@ -30,6 +30,10 @@ logger = logging.getLogger(__name__) DEFAULT_DB_CONNECT_TIMEOUT = 10 # seconds +# IMAGE_NOT_SCANNED_ENGINE_VERSION is the version found in security_indexed_engine when the +# image has not yet been scanned. +IMAGE_NOT_SCANNED_ENGINE_VERSION = -1 + _SCHEME_DRIVERS = { 'mysql': MySQLDatabase, 'mysql+pymysql': MySQLDatabase, @@ -665,7 +669,7 @@ class Image(BaseModel): v1_checksum = CharField(null=True) security_indexed = BooleanField(default=False, index=True) - security_indexed_engine = IntegerField(default=-1, index=True) + security_indexed_engine = IntegerField(default=IMAGE_NOT_SCANNED_ENGINE_VERSION, index=True) # We use a proxy here instead of 'self' in order to disable the foreign key constraint parent = ForeignKeyField(_ImageProxy, null=True, related_name='children') diff --git a/test/test_secscan.py b/test/test_secscan.py index 4a5fc44db..c2d1bbb08 100644 --- a/test/test_secscan.py +++ b/test/test_secscan.py @@ -301,6 +301,45 @@ class TestSecurityScanner(unittest.TestCase): self.assertEquals('Low', body['event_data']['vulnerability']['priority']) self.assertTrue(body['event_data']['vulnerability']['has_fix']) + # Ensure its security indexed engine was updated. + updated_layer = model.tag.get_tag_image(ADMIN_ACCESS_USER, SIMPLE_REPO, 'latest') + self.assertEquals(updated_layer.id, layer.id) + self.assertTrue(updated_layer.security_indexed_engine > 0) + + + def test_analyze_layer_success_no_notification(self): + layer = model.tag.get_tag_image(ADMIN_ACCESS_USER, SIMPLE_REPO, 'latest', include_storage=True) + self.assertFalse(layer.security_indexed) + self.assertEquals(-1, layer.security_indexed_engine) + + # Ensure there are no existing events. + self.assertIsNone(notification_queue.get()) + + # Set the security_indexed_engine of the layer to 0 to ensure it is marked as having been + # indexed (in some form) before this call. + layer.security_indexed_engine = 0 + layer.save() + + # Add a repo event for the layer. + repo = model.repository.get_repository(ADMIN_ACCESS_USER, SIMPLE_REPO) + model.notification.create_repo_notification(repo, 'vulnerability_found', 'quay_notification', {}, {'level': 100}) + + with HTTMock(analyze_layer_success_mock, get_layer_success_mock, response_content): + analyzer = LayerAnalyzer(app.config, self.api) + analyzer.analyze_recursively(layer) + + layer = model.tag.get_tag_image(ADMIN_ACCESS_USER, SIMPLE_REPO, 'latest') + self.assertAnalyzed(layer, True, 1) + + # Ensure no event was written for the tag, as the layer was being re-indexed. + time.sleep(1) + self.assertIsNone(notification_queue.get()) + + # Ensure its security indexed engine was updated. + updated_layer = model.tag.get_tag_image(ADMIN_ACCESS_USER, SIMPLE_REPO, 'latest') + self.assertEquals(updated_layer.id, layer.id) + self.assertTrue(updated_layer.security_indexed_engine > 0) + def _get_notification_data(self, new_layer_ids, old_layer_ids, new_severity='Low'): return { diff --git a/util/secscan/analyzer.py b/util/secscan/analyzer.py index 3b2fa39fa..f2d78336a 100644 --- a/util/secscan/analyzer.py +++ b/util/secscan/analyzer.py @@ -6,7 +6,7 @@ import features from collections import defaultdict from endpoints.notificationhelper import spawn_notification -from data.database import Image, ExternalNotificationEvent +from data.database import Image, ExternalNotificationEvent, IMAGE_NOT_SCANNED_ENGINE_VERSION from data.model.tag import filter_tags_have_repository_event, get_tags_for_image from data.model.image import set_secscan_status, get_image_with_storage_and_parent_base from util.secscan.api import APIRequestFailure @@ -69,6 +69,7 @@ class LayerAnalyzer(object): # Analyze the image. logger.info('Analyzing layer %s', layer.docker_image_id) + previous_security_indexed_engine = layer.security_indexed_engine (analyzed_version, should_requeue) = self._api.analyze_layer(layer) # If analysis failed, then determine whether we need to requeue. @@ -88,9 +89,13 @@ class LayerAnalyzer(object): analyzed_version) set_status = set_secscan_status(layer, True, analyzed_version) - # If we are the one who've done the job successfully first, get the vulnerabilities and - # send notifications to the repos that have a tag on that layer. - if features.SECURITY_NOTIFICATIONS and set_status: + # If we are the one who've done the job successfully first, and this is a *new* layer, + # as indicated by having a version of -1, get the vulnerabilities and + # send notifications to the repos that have a tag on that layer. We don't always send + # notifications as if we are re-indexing a layer for a newer feature set in the security + # scanner, notifications will be spammy. + if (features.SECURITY_NOTIFICATIONS and set_status and + previous_security_indexed_engine == IMAGE_NOT_SCANNED_ENGINE_VERSION): # Get the tags of the layer we analyzed. repository_map = defaultdict(list) event = ExternalNotificationEvent.get(name='vulnerability_found')