Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EC2 migration to the new architecture #202

Merged
merged 90 commits into from Mar 4, 2019

Conversation

Aboisier
Copy link
Contributor

@Aboisier Aboisier commented Feb 22, 2019

This PR is a work in progress for refactoring the EC2 service, as part of the #183 issue.

I was doing a lot of copy-pasting in between the Lambdas service and this one, and between the facade methods, so I generalized some classes and methods.

EC2 has a pretty complex structure, but it seems like our new architecture holds up.

  • EC2
    • regions
      • region_id
        • images (contains a dictionary of images)
        • snapshots (contains a dictionary of snapshots )
        • volumes (contains a dictionary of volumes )
        • vpcs
          • vpc_id
            • instances (contains a dictionary of instances)
            • network_interface (contains a dictionary of nics)
            • security groups (contains a dictionary of sgs)

Stuff I generalized:

Regions

Fetches a list of all the regions and builds a regions dictionary with <region_id, empty_dictionary> key pairs. It also sets the regions_count

ScopedResource

Abstract class which fetches resources in a given scope. The child class needs to define a get_resources_in_scope(scope) method. The scope can be anything; a region, some VPC id, etc. It also needs to define a parse_resource(resource) method which returns a (resource_id, resource) tuple. The ScopedResource class loops and sets the resources in its own dictionary.

Aboisier and others added 27 commits February 19, 2019 09:05
Co-Authored-By: Aboisier <aboisiermichaud@gmail.com>
@Aboisier Aboisier requested review from Remi05, vifor2 and misg March 2, 2019 21:49
Copy link
Contributor

@misg misg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just left a couple of comments that are mainly details and not very important ones, I'm up for a merge! 🎆 Great job overall, special mention to your unit tests for AWS resources, very neat! 🥇

ScoutSuite/providers/aws/facade/utils.py Outdated Show resolved Hide resolved
ScoutSuite/providers/aws/facade/utils.py Outdated Show resolved Hide resolved
self[name] = resource

def _parse_instance(self, raw_instance):
instance = {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we got an opportunity with this migration to be PEP 8 compliant and start defining dictionaries like the following:

        instance = {
                'id' = raw_instance['InstanceId'],
                'reservation_id' = raw_instance['ReservationId'],
                'monitoring_enabled' = raw_instance['Monitoring']['State'] == 'enabled',
                'user_data' = self.facade.ec2.get_instance_user_data(self.scope['region'], id)
        }

Copy link
Contributor Author

@Aboisier Aboisier Mar 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a discussion about that with @Remi05 last Thursday and we came to the conclusion it's probably a better idea to continue with the current implementation. There were a couple of reasons why, but here are the most important IMHO:

  1. Easier debugging: let's say raw_instance['InstanceId'] fails because the dict does not have this key. Using the PEP 8 convention, we would not know exactly what caused the crash.
  2. Add stuff iteratively: Sometimes, you want to add stuff iteratively. That's not possible with the PEP 8 convention unless you want to use a hybrid of the two notations, but then we might as well keep up with the current solution

self[name] = resource

def _parse_security_group(self, raw_security_group):
security_group = {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.

@Aboisier Aboisier changed the title WIP - EC2 migration to the new architecture EC2 migration to the new architecture Mar 4, 2019
@Aboisier Aboisier requested review from zer0x64, Remi05 and vifor2 and removed request for zer0x64, Remi05 and vifor2 March 4, 2019 03:00
@Aboisier Aboisier merged commit 70108b4 into refactoring/resource-configs Mar 4, 2019
Scout Suite automation moved this from In progress to Done Mar 4, 2019
@Aboisier Aboisier deleted the refactoring/aws/ec2 branch March 4, 2019 16:46
@Aboisier Aboisier mentioned this pull request Mar 13, 2019
53 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Scout Suite
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants