From 0513ab4bff08ac18d03449379230ec7c65398ab2 Mon Sep 17 00:00:00 2001 From: Mike Grima Date: Fri, 18 May 2018 14:04:12 -0700 Subject: [PATCH] Fix for DB error documented in #1005 fixes #1005 --- env-config/config-local.py | 6 +-- env-config/config.py | 6 +-- migrations/versions/7c54b06e227b_.py | 7 ++- security_monkey/auditor.py | 15 ++++-- security_monkey/tests/core/test_watcher.py | 63 ++++++++++++++++++++++ security_monkey/watcher.py | 63 +++++++++++++++++++++- 6 files changed, 141 insertions(+), 19 deletions(-) diff --git a/env-config/config-local.py b/env-config/config-local.py index 9f48f1ecf..e8246aaea 100644 --- a/env-config/config-local.py +++ b/env-config/config-local.py @@ -43,11 +43,7 @@ 'loggers': { 'security_monkey': { 'handlers': ['file', 'console'], - 'level': 'INFO' - }, - 'apscheduler': { - 'handlers': ['file', 'console'], - 'level': 'WARN' + 'level': 'DEBUG' } } } diff --git a/env-config/config.py b/env-config/config.py index 94eb3329e..296e020eb 100644 --- a/env-config/config.py +++ b/env-config/config.py @@ -44,11 +44,7 @@ 'loggers': { 'security_monkey': { 'handlers': ['file', 'console'], - 'level': 'WARN' - }, - 'apscheduler': { - 'handlers': ['file', 'console'], - 'level': 'INFO' + 'level': 'DEBUG' } } } diff --git a/migrations/versions/7c54b06e227b_.py b/migrations/versions/7c54b06e227b_.py index 72cebcce9..dbe7ca31a 100644 --- a/migrations/versions/7c54b06e227b_.py +++ b/migrations/versions/7c54b06e227b_.py @@ -206,10 +206,9 @@ class Technology(Base): def upgrade(): """ - Need to detect duplicate items. - This needs to be done by looking for all items with the same: - 1. region, name, tech_id, account_id. - With these items, pick the one that has the bigger latest_revision_id and delete the others. + Need to detect duplicate items. + This needs to be done by looking for all items with the same: region, name, tech_id, account_id. + With these items, pick the one that has the bigger latest_revision_id and delete the others. :return: """ bind = op.get_bind() diff --git a/security_monkey/auditor.py b/security_monkey/auditor.py index ee2972676..06bcba06e 100644 --- a/security_monkey/auditor.py +++ b/security_monkey/auditor.py @@ -23,7 +23,7 @@ from six import string_types, text_type from security_monkey import app, datastore, db -from security_monkey.watcher import ChangeItem +from security_monkey.watcher import ChangeItem, ensure_item_has_latest_revision_id from security_monkey.common.jinja import get_jinja_env from security_monkey.datastore import User, AuditorSettings, Item, ItemAudit, Technology, Account, ItemAuditScore, AccountPatternAuditScore from security_monkey.common.utils import send_email @@ -102,6 +102,7 @@ class Categories: # TODO # INSECURE_CERTIFICATE = 'Insecure Certificate' + class Entity: """ Entity instances provide a place to map policy elements like s3:my_bucket to the related account. """ def __init__(self, category, value, account_name=None, account_identifier=None): @@ -398,10 +399,18 @@ def _load_userids(cls): role_results = cls._load_related_items('iamrole') for item in user_results: - add(cls.OBJECT_STORE['userid'], item.latest_config.get('UserId'), item.account.identifier) + fixed_item = ensure_item_has_latest_revision_id(item) + if not fixed_item: + continue + + add(cls.OBJECT_STORE['userid'], fixed_item.latest_config.get('UserId'), fixed_item.account.identifier) for item in role_results: - add(cls.OBJECT_STORE['userid'], item.latest_config.get('RoleId'), item.account.identifier) + fixed_item = ensure_item_has_latest_revision_id(item) + if not fixed_item: + continue + + add(cls.OBJECT_STORE['userid'], fixed_item.latest_config.get('RoleId'), fixed_item.account.identifier) @classmethod def _load_accounts(cls): diff --git a/security_monkey/tests/core/test_watcher.py b/security_monkey/tests/core/test_watcher.py index 34e467050..9c004886f 100644 --- a/security_monkey/tests/core/test_watcher.py +++ b/security_monkey/tests/core/test_watcher.py @@ -21,6 +21,7 @@ """ import datetime +from datetime import timedelta import json from security_monkey.watcher import Watcher, ChangeItem @@ -490,3 +491,65 @@ def test_find_deleted_batch(self): assert item_revision.active assert len(ItemAudit.query.filter(ItemAudit.item_id == item_revision.item_id).all()) == 2 + + def test_ensure_item_has_latest_revision_id(self): + """ + Test that items always have a proper current revision set. Otherwise, the item needs to be deleted. + :return: + """ + from security_monkey.watchers.iam.iam_role import IAMRole + from security_monkey.watcher import ensure_item_has_latest_revision_id + from security_monkey.datastore import Datastore + + # Stop the watcher registry from stepping on everyone's toes: + import security_monkey.watcher + old_watcher_registry = security_monkey.watcher.watcher_registry + security_monkey.watcher.watcher_registry = {IAMRole.index: IAMRole} + + # Set everything up: + self.setup_batch_db() + watcher = IAMRole(accounts=[self.account.name]) + watcher.current_account = (self.account, 0) + watcher.technology = self.technology + + # Test case #1: Create an item in the DB that has no current revision ID: + no_revision_item = Item(region="us-east-1", name="NOREVISION", account_id=self.account.id, + tech_id=self.technology.id) + db.session.add(no_revision_item) + db.session.commit() + + assert db.session.query(Item).filter(Item.name == no_revision_item.name).one() + + # Should delete the item from the DB: + result = ensure_item_has_latest_revision_id(no_revision_item) + assert not result + assert not db.session.query(Item).filter(Item.name == no_revision_item.name).first() + + # Test case #2: Create two item revisions for the given item, but don't attach them to the item. + # After the fixer runs, it should return the item with proper hashes and a proper + # link to the latest version. + ds = Datastore() + no_revision_item = Item(region="us-east-1", name="NOREVISION", account_id=self.account.id, + tech_id=self.technology.id) + db.session.add(no_revision_item) + db.session.commit() + + ir_one = ItemRevision(config=ACTIVE_CONF, date_created=datetime.datetime.utcnow(), + item_id=no_revision_item.id) + ir_two = ItemRevision(config=ACTIVE_CONF, + date_created=(datetime.datetime.utcnow() - timedelta(days=1)), + item_id=no_revision_item.id) + + db.session.add(ir_one) + db.session.add(ir_two) + db.session.commit() + + assert len(db.session.query(ItemRevision).filter(ItemRevision.item_id == no_revision_item.id).all()) == 2 + result = ensure_item_has_latest_revision_id(no_revision_item) + assert result + assert result.latest_revision_id == ir_one.id + assert ds.hash_config(ACTIVE_CONF) == no_revision_item.latest_revision_complete_hash + assert ds.durable_hash(ACTIVE_CONF, watcher.ephemeral_paths) == no_revision_item.latest_revision_durable_hash + + # Undo the mock: + security_monkey.watcher.watcher_registry = old_watcher_registry diff --git a/security_monkey/watcher.py b/security_monkey/watcher.py index 435970ce3..7e64a27ae 100644 --- a/security_monkey/watcher.py +++ b/security_monkey/watcher.py @@ -13,8 +13,8 @@ from security_monkey.common.PolicyDiff import PolicyDiff from security_monkey.common.utils import sub_dict from security_monkey import app, datastore -from security_monkey.datastore import Account, IgnoreListEntry, db -from security_monkey.datastore import Technology, WatcherConfig, store_exception +from security_monkey.datastore import Technology, WatcherConfig, store_exception, Account, IgnoreListEntry, db, \ + ItemRevision, Datastore from security_monkey.common.jinja import get_jinja_env from security_monkey.alerters.custom_alerter import report_watcher_changes @@ -652,3 +652,62 @@ def save(self, datastore, ephemeral=False): new_issues=self.audit_issues, ephemeral=ephemeral, source_watcher=self.watcher) + + +def ensure_item_has_latest_revision_id(item): + """ + This is a function that will attempt to correct an item that does not have a latest revision id set. + There are two outcomes that result: + 1. If there is a revision with the item id, find the latest revision, and update the item such that it + point to that latest revision. + 2. If not -- then we will treat the item as rancid and delete it from the database. + :param item: + :return The item if it was fixed -- or None if it was deleted from the DB: + """ + if not item.latest_revision_id: + current_revision = db.session.query(ItemRevision).filter(ItemRevision.item_id == item.id)\ + .order_by(ItemRevision.date_created.desc()).first() + + if not current_revision: + db.session.delete(item) + db.session.commit() + + app.logger.error("[?] Item: {name}/{tech}/{account}/{region} does NOT have a latest revision attached, " + "and no current revisions were located. The item has been deleted.".format( + name=item.name, + tech=item.technology.name, + account=item.account.name, + region=item.region)) + + return None + + else: + # Update the latest revision ID: + item.latest_revision_id = current_revision.id + + # Also need to generate the hashes: + # 1. Get the watcher class of the item: + watcher_cls = watcher_registry[item.technology.name] + watcher = watcher_cls(accounts=[item.account.name]) + ds = Datastore() + + # 2. Generate the hashes: + if watcher.honor_ephemerals: + ephemeral_paths = watcher.ephemeral_paths + else: + ephemeral_paths = [] + + item.latest_revision_complete_hash = ds.hash_config(current_revision.config) + item.latest_revision_durable_hash = ds.durable_hash(current_revision.config, ephemeral_paths) + + db.session.add(item) + db.session.commit() + + app.logger.error("[?] Item: {name}/{tech}/{account}/{region} does NOT have a latest revision attached, " + "but a current revision was located. The item has been fixed.".format( + name=item.name, + tech=item.technology.name, + account=item.account.name, + region=item.region)) + + return item