From 46087d5e6422cf59b122e234409cc303949c49d9 Mon Sep 17 00:00:00 2001 From: Jimmy Zelinskie Date: Wed, 21 Jun 2017 15:27:56 -0400 Subject: [PATCH 1/2] util.secscan.api: more robust API failures cases Addresses QUAY-672 by handling all status codes that are not 404 and 5xx and moving response decoding inside the try/except block to ensure that the response object is in scope. --- util/secscan/api.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/util/secscan/api.py b/util/secscan/api.py index 48b5d84f2..918c3fd23 100644 --- a/util/secscan/api.py +++ b/util/secscan/api.py @@ -387,18 +387,27 @@ class ImplementedSecurityScannerAPI(SecurityScannerAPIInterface): response = self._call('GET', _API_METHOD_GET_LAYER % layer_id, params=params) logger.debug('Got response %s for vulnerabilities for layer %s', response.status_code, layer_id) + try: + return response.json() + except ValueError: + logger.exception('Failed to decode response JSON') + return None + except Non200ResponseException as ex: logger.debug('Got failed response %s for vulnerabilities for layer %s', ex.response.status_code, layer_id) if ex.response.status_code == 404: return None - elif ex.response.status_code // 100 == 5: + else: logger.error( 'downstream security service failure: status %d, text: %s', ex.response.status_code, ex.response.text, ) - raise APIRequestFailure('Downstream service returned 5xx') + if ex.response.status_code // 100 == 5: + raise APIRequestFailure('Downstream service returned 5xx') + else: + raise APIRequestFailure('Downstream service returned non-200') except requests.exceptions.Timeout: raise APIRequestFailure('API call timed out') except requests.exceptions.ConnectionError: @@ -407,11 +416,6 @@ class ImplementedSecurityScannerAPI(SecurityScannerAPIInterface): logger.exception('Failed to get layer data response for %s', layer_id) raise APIRequestFailure() - try: - return response.json() - except ValueError: - logger.exception('Failed to decode response JSON') - def _request(self, method, endpoint, path, body, params, timeout): """ Issues an HTTP request to the security endpoint. """ From 1d2640e012243b36a3dbd2ad391c906588335d5b Mon Sep 17 00:00:00 2001 From: Jimmy Zelinskie Date: Wed, 28 Jun 2017 13:40:04 -0400 Subject: [PATCH 2/2] util.secscan.fake: add test for unexpected status --- test/test_secscan.py | 23 ++++++++++++++++++++++- util/secscan/fake.py | 14 ++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/test/test_secscan.py b/test/test_secscan.py index db466689a..95e74d53f 100644 --- a/test/test_secscan.py +++ b/test/test_secscan.py @@ -160,7 +160,7 @@ class TestSecurityScanner(unittest.TestCase): security_scanner.set_internal_error_layer_id(security_scanner.layer_id(layer)) analyzer = LayerAnalyzer(app.config, self.api) - with self.assertRaises(APIRequestFailure) as ctx: + with self.assertRaises(APIRequestFailure): analyzer.analyze_recursively(layer) layer = model.tag.get_tag_image(ADMIN_ACCESS_USER, SIMPLE_REPO, 'latest') @@ -185,6 +185,27 @@ class TestSecurityScanner(unittest.TestCase): layer = model.tag.get_tag_image(ADMIN_ACCESS_USER, SIMPLE_REPO, 'latest') self.assertAnalyzed(layer, security_scanner, False, 1) + def test_analyze_layer_unexpected_status(self): + """ Tests that a response from a scanner with an unexpected status code fails correctly. """ + + 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) + + with fake_security_scanner() as security_scanner: + # Make is so trying to analyze the parent will fail with an error. + security_scanner.set_unexpected_status_layer_id(security_scanner.layer_id(layer.parent)) + + # Try to the layer and its parents, but with one request causing an error. + analyzer = LayerAnalyzer(app.config, self.api) + with self.assertRaises(APIRequestFailure): + analyzer.analyze_recursively(layer) + + # Make sure it isn't analyzed. + layer = model.tag.get_tag_image(ADMIN_ACCESS_USER, SIMPLE_REPO, 'latest') + self.assertAnalyzed(layer, security_scanner, False, -1) + + def test_analyze_layer_missing_parent_handled(self): """ Tests that a missing parent causes an automatic reanalysis, which succeeds. """ diff --git a/util/secscan/fake.py b/util/secscan/fake.py index 849d0c2ca..69fc30cc2 100644 --- a/util/secscan/fake.py +++ b/util/secscan/fake.py @@ -33,6 +33,7 @@ class FakeSecurityScanner(object): self.fail_layer_id = None self.internal_error_layer_id = None self.error_layer_id = None + self.unexpected_status_layer_id = None def set_ok_layer_id(self, ok_layer_id): """ Sets a layer ID that, if encountered when the analyze call is made, causes a 200 @@ -58,6 +59,12 @@ class FakeSecurityScanner(object): """ self.error_layer_id = error_layer_id + def set_unexpected_status_layer_id(self, layer_id): + """ Sets a layer ID that, if encountered when the analyze call is made, causes an HTTP 600 + to be raised. This is useful in testing the robustness of the to unknown status codes. + """ + self.unexpected_status_layer_id = layer_id + def has_layer(self, layer_id): """ Returns true if the layer with the given ID has been analyzed. """ return layer_id in self.layers @@ -252,6 +259,13 @@ class FakeSecurityScanner(object): 'content': json.dumps({'Error': {'Message': 'Some sort of error'}}), } + if layer['Name'] == self.unexpected_status_layer_id: + return { + 'status_code': 600, + 'content': json.dumps({'Error': {'Message': 'Some sort of error'}}), + } + + parent_id = layer.get('ParentName', None) parent_layer = None