Add support for custom account metadata #408
Conversation
Change summary:
* Fix Error in exception handling
* Adding audit class to distinguish items created by different
auditors of the same type
* Adding custom directories
* Adding development guidelines documentation
* Fixing links in documentation
* Removing duplicate auditors
* Adding init test for scheduler
* Fixing DB error in scheduler test
* Removing unneeded DB insert from test case
* Race condition with watcher_registry
* Fixing DB migration conflict
* Picking up pubspec.lock changes
* Code style cleanup
* Update file headers for contribution prep
Change-Id: Id72322f6dbccedc701e9c17dc9a5b8dc26bf30c1
List of added watchers:
* CloudTrail
* AWSConfig
* AWSConfigRecorder
* DirectConnect::Connection
* DirectConnect::VirtualGateway
* EC2::EbsSnapshot
* EC2::EbsVolume
* EC2::Image
* EC2::Instance
* ENI
* KMS::Grant
* KMS::Key
* Lambda
* RDS::ClusterSnapshot
* RDS::DBCluster
* RDS::DBInstace
* RDS::Snapshot
* RDS::SubnetGroup
* Route53
* Route53Domains
* TrustedAdvisor
* VPC::DHCP
* VPC::Endpoint
* VPC::FlowLog
* VPC::NatGateway
* VPC::NetworkACL
* VPC::Peering
Additional changes:
* Move rds[security_group] to rds/ directory.
* Update vpc/route_table to use boto3 lib.
* Add tests for tech types supported by moto
* Initialize name to avoid UnboundLocalError
* Update RDS watcher ephemeral values
* Use boto3.session.get_available_regions in select watchers
* Convert routetable watcher to decorator pattern
* Convert route53 domains to decorator pattern
* Handle the case where the aws principle is a string
* Catching assume role exception in decorator
zollman
changed the title
zollman
force-pushed
the
custom_accounts
branch
from
c0480e0 to
dae5366
Compare
|
@MonkeySecurity I have a change to this PR that fixes the merge conflicts between this PR and #407 that is based on some streamlining of the common code, but now I see that 407 has been pushed back to 0.7.5 as well. It looks like there are quite a few other merge conflicts and I'd like to submit the merge conflict resolution closer to the release time. Can you ping us when you get to this point? |
Type: Bugfix Why is this change necessary: When find_changes is run, it has the ability to update specific tech types outside of the normal reporter run. In these cases the monitor must rerun auditors for the updated tech types, plus any other auditors for other tech types that are dependent on the updated types. It determines this by checking the support_watcher_indexes and support_auditor_indexs for each auditor associated with the tech types updated. The check for the support_watcher_indexes were only being run for the last auditor because it was indented incorrectly. This change addresses the need by: Changing the indent so that it is run for all auditors
|
Applying "Needs Work" label as I believe Curtis is working on updating this PR. (See gitter.im chat from Oct 13 15:55) |
|
UDPATE: still working on these updates. Hope to post w/in 1 week |
Type: generic-large-feature Why is this change necessary? Feature allowing for custom account types, which can either be non-AWS accounts or AWS accounts extended with additional attributes. By default, the new custom attributes are stored in the database but can be configured to be retrieved from some other source. This change addresses the need by: Implementing account_manager framework
zollman
force-pushed
the
custom_accounts
branch
from
dae5366 to
416659f
Compare
|
Build will fail until #438 is merged |
|
Looks like #438 fixed any build issues :) This PR should be good to go as well. Let us know if you have any questions |
| items = relationship("Item", backref="account", cascade="all, delete, delete-orphan") | ||
| issue_categories = relationship("AuditorSettings", backref="account") | ||
| role_name = Column(String(256)) | ||
| role_name = Column(String(256)) # (deprecated-custom) |
There was a problem hiding this comment.
These fields are marked as deprecated, but still referenced throughout the codebase.
s3_name, number, role_name.
I'm worried about storing data in multiple places because inconsistencies could occur.
I see the migration is copying account.number to the account.identifier, but it is not copying account.s3_name or account.role_name to a custom field.
I think we should delete these fields instead of deprecating them, and use the upgrade script to move the existing values into the custom fields. (And the reverse in the downgrade script.)
All the tests and the rest of the project will need to be updated to use the new identifier and custom fields. Maybe you could override the Account.__getattr__() to check the custom fields.
Thoughts?
There was a problem hiding this comment.
@MonkeySecurity we were planning to do that as a seperate task because this change is already rather large, and deprecating these fields would make it a much larger code change, and would require a DB migration for existing data along with a rollback. We could go ahead do that if you would prefer to roll it all into one PR.
There was a problem hiding this comment.
If you'd like to keep account.identifier and account.number, I would think you'd need to update security_monkey/account_manager.py::_populate_account(...) so that it sets account.number as well as account.identifier (~ line 128)
Otherwise old and new accounts will be inconsistent.
def _populate_account(self, account, account_type_id, name, active, third_party,
notes, identifier, custom_fields=None):
"""
Creates account DB object to be stored in the DB by create or update.
May be overridden to store additional data
"""
account.name = name
account.identifier = identifier
account.notes = notes
What would the timeline be for removing the deprecated fields?
There was a problem hiding this comment.
As I recall, this is handled in the AWSAccountManager. Thinking was that some non-aws accounts types would not case and the AWS account would handle any derived types.
As for the timeline, it wouldn't take more than a day or two, but we would still need to go though the approval process. I would expect at least a working week or so for that.
There was a problem hiding this comment.
@MonkeySecurity Did you want us to add the changes above based on my last comment ^^^?
There was a problem hiding this comment.
No this is good. I'll merge in and hope to see a migration update soon.
…being merged first because of the Alembic upgrade.
…being merged first because of the Alembic upgrade.
* Adding index to region. Dropping unused item.cloud. Depends on PR #408 being merged first because of the Alembic upgrade. * Allowing Alembic to recreate foreign keys on Exceptions table to include a deleting cascade. * Refactored exception logging to account for new `AccountType` class. - Rebased the `develop` branch here so all DB changes are aligned properly.
4 participants
We need to run different watchers and auditors depending on which
account is being monitored. This is primarily for two reasons: first,
different AWS accounts are expected to have different security
postures (i.e., dev vs. prod); and second, some of the accounts may
not be AWS accounts at all (e.g. Azure or other accounts that need to
be scanned for watched items).
This PR extends the dynamic watchers & auditors framework from #405 .