Scoutsuite: Formalize the providers architecture

[Aboisier]

_Once upon a time, there was only AWS. The realm of Scout was dominated by this provider only, and as it grew bigger, jealous grew the other providers. GCP was then given a piece of land. GCP, receiving a lot of attention from the Supreme Beings of Scout, was making Azure jealous. And so Azure was given a piece of land. Even though it seemed like the providers were happily cohabiting, the Supreme Beings knew something was not right._

And this is why we need to refactor the providers' architecture. Basically, there is a whole lot of provider-checking conditions, which is a huge code smell. There is already a provider model in place, but a lot of attributes should be bubbled up to the generic provider class, and some others should be pushed down to their respective provider. Basically, we should take advantage of polymorphism.

List of tasks

Services to migrate

AWS

• [x] ~Lambdas (#199)~
• [x] ~CloudFormation (#216)~
• [x] ~CloudTrail (#219)~
• [x] ~CloudWatch (#221)~
• [x] ~DirectConnect (#222)~
• [x] ~EC2 (#202)~
• [x] ~EFS (#227)~
• [x] ~ElastiCache (#245)~
• [x] ~ELB (#257)~
• [x] ~ELBv2 (#259 )~
• [x] ~EMR (#248)~
• [x] ~IAM (#264)~
• [x] ~RDS (#271 )~
• [x] ~RedShift (#261)~
• [x] ~Route53 (#267)~
• [x] ~S3 (#270)~
• [x] ~SES (#269)~
• [x] ~SNS (#265)~
• [x] ~SQS (#262)~
• [x] ~VPC (#273)~
• [x] ~Config (#272 and https://github.com/nccgroup/ScoutSuite-Proprietary/pull/115)~
• [x] ~DynamoDb (#274 and https://github.com/nccgroup/ScoutSuite-Proprietary/pull/115)~
• [x] ~KMS (#280)~
• [x] ~Clean metadata.json~

Azure

• [x] ~App Gateway (https://github.com/nccgroup/ScoutSuite-Proprietary/pull/109)~
• [x] ~App Service (https://github.com/nccgroup/ScoutSuite-Proprietary/pull/110)~
• [x] ~Key Vault (#239)~
• [x] ~Load Balancer (https://github.com/nccgroup/ScoutSuite-Proprietary/pull/111#event-2195042800)~
• [x] ~Monitor (#236)~
• [x] ~Network (#242)~
• [x] ~Redis Cache (https://github.com/nccgroup/ScoutSuite-Proprietary/pull/112#event-2195042800)~
• [x] ~Security Center (#243)~
• [x] ~SQL Database (#214)~
• [x] ~Storage Accounts (#235)~

GCP

• [x] ~CloudResourcesManager (#231)~
• [x] ~CloudSQL (#232)~
• [x] ~CloudStorage (#304)~
• [x] ~ComputeEngine (#319)~
• [x] ~IAM (#230)~
• [x] ~KubernetesEngine~
• [x] ~StackDiverLogging (#229)~
• [x] ~StackDriverMonitoring (currently has no logic)~

Documentation

• [x] Document the architecture in the wiki (a few figures would also come a long way)
• [x] Document [at a high level] how to add a provider
• [x] Document how to add a service for AWS
• [x] Document how to add a service for Azure
• [x] Document how to add a service for GCP
• [x] Write docstrings for the classes and methods involved in the implementation of a new service

Other

• [x] ~Ensure that the old logging system works or implement a new one (moved to issue #283 )~
• [x] Go through all the todos we left
• [x] Ensure that the performance is still acceptable, if not better
• [x] [Re-]implement functionality that allows throttling API calls (similar to the current --threads). This is necessary as sometimes you need to run Scout while making sure it doesn't affect the call quota
• [x] Explicitly deprecate python < 2.x

[Aboisier]

After discussing with @misg and @Remi05, we decided we would try implementing a composite pattern. Here's our current state of mind:

Create a hierarchical structure

When improving support for Azure, we noticed there was a lot of friction associated with implementing a new service. This is because Azure's API is very granular; a lot of API calls are needed to get one config. And regarding GCP, Scout Suite current support doesn't reflect very well the hierarchy of organization/project/region/zone used by Google (see #80 and #97). Thus, we tend to think that a composite pattern could address those issues (and make AWS support more readable and maintenable by the way, see the next section below). Basically, each time you would put an id in a resource path, you split in a new config. Each config is responsible for calling fetch_all on its children configs.

Prefer explicitness over magicness

We noticed there was a lot of magic going on. A lot of the code was over-generalized, which encouraged the use of service-specific hacks. Also, there was a lot of magic string wizardry stuff (including heavy use of Python metaprogramming). For example, AWS's targets: they were used for injecting the right API call. This seemed unnecessary and made the code flow very hard to follow and test. We decided to move the API calls to their own configs. This might be a bit redundant, but we feel like it will make the code more readable and maintainable. It will also be more resilient to potential API breaking changes. The API clients instances will be provided by some kind of container class. This will allow for easier per-service unit testing by making the mocking of API clients easy. Another idea is to use a façade. This makes testing easier (less stuff to mock) and removes some code from the service. It makes sense to put the least code possible in the configs since they are basically data models.

Refactor iteratively

This is a huge refactoring. We want to do this the right way, and the right way is to not do one humongous pull request with the whole refactoring at once. In order to do so, we are going to keep BaseConfig's current interface. This is going to allow us to migrate the services iteratively without changing the main structure. We will then remove the unused code.

Collateral improvements

We believe we will be able to make some other changes on the way. For example, we plan on making fetch_all async to allow better management of concurrency and improve scanning performance.

Current state

We are not sure this solution works, so we are currently working on a proof of concept. We will try migrating one service for each provider. We created a branch with the changes common to the three providers. We will each work on a separate branch and do our PRs onto the common branch.

@aboisier: AWS Lambda (see branch)
@misg: Azure SQL Databases (see branch)
@remi05: GCP ? (see branch)

[x4v13r64]

I like the approach, going with a Composite (and a bit of Facade) pattern and taking advantage of polymorphism will significantly improve the architecture.

There is indeed a lot of magic happening, especially in RegionConfig's interaction with the metadata.json file. One reason for this is that the architecture is mostly undocumented, so it's a free-for-all of using BaseConfigs, using/not using callbacks, etc. Also a lot of poorly documented and obscure functions such as recurse() and go_to_and_do()/new_go_to_and_do() make understanding the code hard. Let's not make that mistake twice and ensure the Wiki clearly details Scout's inner workings. The changes you propose should also make understanding the code flow less painful.

For GCP I'd recommend doing the PoC with Cloud SQL, as it's a mostly straightforward service.

When improving support for Azure, we noticed there was a lot of friction associated with implementing a new service. This is because Azure's API is very granular; a lot of API calls are needed to get one config.

You could say the same thing of AWS, that's likely why so many callbacks were used.

regarding GCP, Scout Suite current support doesn't reflect very well the hierarchy of organization/project/region/zone

Exactly, as GCP doesn't have a RegionConfig it's impossible to distinguish resource hierarchy. I know this isn't directly related to this thread but Scout's data model (a big dict which acts as a tree) is very much tied to the code's architecture. While you're thinking of refactoring it might be worth giving some thought to whether this approach is still valid given the growth of the tool (as we've already seen it hit some limitations, i.e. the memory limit of browsers).

Also, there was a lot of magic string wizardry stuff (including heavy use of Python metaprogramming). For example, AWS's targets: they were used for injecting the right API call

Agreed. I looks pretty "cool" as you get this large metadata file to define all the API calls, but it's not intuitive (especially without explicit documentation).

Another idea is to use a façade

This will certainly be useful in some cases, e.g. how __main__.py interacts with the providers objects.

we plan on making fetch_all async to allow better management of concurrency and improve scanning performance.

I'm not sure how much this will improve over the current multi threaded approach, but give it a shot!
You'll have to make sure that even if a composite node has a lot of leaves, resource consumption doesn't skyrocket.

In addition, will this break python 2.7 compatibility?

[misg]

After review, here's a list of remaining discrepancies between the refactored cloud providers that should be handled in order to make the refactoring as clear as possible:

• [x] Azure resource counts should be computed the same way used in AWS and GCP supports
• [x] Azure facade attribute should be defined as a root attribute (e.g. in a class AzureResources that inherits from the base class Resources, like AWSResources) instead of passed as a constructor argument at each level of the hierarchy
• [x] scope terminology/concept used in AWS support should be explicitly used as well in Azure (and GCP) support rather than implicitly through the use of kwargs which is a bit obscure
• [x] In theory, methods _fetch_children and _fetch_children_of_all_resources defined in AzureCompositeResources and AWSCompositeResources could be moved up to the base class CompositeResources since they are basically duplicated code (need to check with GCP implementation, although)
• [x] File name convention between AWS, Azure and GCP supports should be unified
• [x] Some Azure facades could be factorized using the get_and_set_concurrently helper
• [ ] Some AWS resources should be back (see #298)

[x4v13r64]

Azure resource counts should be computed the same way used in AWS and GCP supports

I liked best how Azure implements it (directly in the composite resource class)

My bad, I had confused Azure with AWS.

[Aboisier]

Azure resource counts should be computed the same way used in AWS and GCP supports

I liked best how Azure implements it (directly in the composite resource class)

I'm confused, it seems like you're describing AWS's implementation: https://github.com/nccgroup/ScoutSuite/blob/master/ScoutSuite/providers/aws/resources/resources.py#L70

[x4v13r64]

I'm confused, it seems like you're describing AWS's implementation:

Yes sorry, I had mixed up the two.

[Aboisier]

I think we can officially say the refactoring is done, can we?

[Aboisier]

I declare this issue CLOSED! 🎉