WIP - Refactoring/aws lambda config #199

Aboisier · 2019-02-20T20:15:36Z

Here's a work in progress for migrating the Aws Lambda service to the new architecture. I had to implement a RegionConfig, which is generic. I think it's not too ugly.

…da-config

misg

Great job! I really like your example of facade to manage API calls, very neat!
I left a couple of comments about the general design of your refactoring, hope they will be useful to feed the discussion about our global refactoring!

ScoutSuite/providers/aws/aws_facade.py

misg · 2019-02-20T21:58:28Z

ScoutSuite/providers/aws/aws_facade.py

+        regions = Session().get_available_regions(service, partition_name)
+
+        if chosen_regions:
+            return list((Counter(regions) & Counter(chosen_regions)).elements())


Very elegant! 💯

Thanks <3 It comes from opinel. I feel like this line could be simplified to this:

Suggested change

return list((Counter(regions) & Counter(chosen_regions)).elements())

return list((Set(regions) & Set(chosen_regions))

I didn't do it cause I'm not sure.

misg · 2019-02-20T22:44:32Z

ScoutSuite/providers/aws/configs/regions_config.py

+from ScoutSuite.providers.aws.aws_facade import AwsFacade
+
+
+class RegionsConfig(ResourceConfig):


I'm not really convinced about the design of RegionsConfig and its "child" LambdaServiceConfig. It looks like you're not really taking advantages of our composite structure and inheritance principles, by having RegionsConfig and LambdaServiceConfig inherited from ResourceConfig, with RegionsConfig instantiated in LambdaServiceConfig with LambdasConfig passed as an argument.

Why not defining RegionsConfig as a base class, something like:

class RegionsConfig(ResourceConfig): # TODO: add service as arg async def fetch_all(self, credentials, regions=None, partition_name='aws', targets=None): # TODO: Should be injected facade = AwsFacade() self['regions'] = await facade.build_region_list(service, regions, partition_name) self['regions_count'] = len(self['regions'])

and then just defining LambdasConfig as a child class of RegionsConfig (and removing LambdaServiceConfig which doesn't seem very useful, even though probably used in the current codebase), as following:

class LambdasConfig(RegionsConfig): async def fetch_all(self, credentials, region, partition_name='aws', targets=None): super(LambdasConfig, self).fetch_all('lambda', region, partition_name) # TODO: modify fetch_all to match this signature # TODO: Should be injected facade = AwsFacade() for region in self['regions']: functions = {} for raw_function in facade.get_lambda_functions(region): name, function = self.parse_function(raw_function) functions[name] = function self['functions_count'] = len(functions) self['functions'] = functions @staticmethod def parse_function(function): function['name'] = function.pop('FunctionName')

But we would have to deal with two things first, I believe:

removing LambdaServiceConfig safely, ensuring compliance with the interface of the current codebase (mainly with the rule engine, in the end)

modifiying fetch_all signature, to be able to pass a service argument. More than that, it seems like you don't need credentials and targets argument. I would suggest to modify the base class ResourceConfig to be more general:

class ResourceConfig(dict): async def fetch_all(self, **kwargs): raise NotImplementedError() async def finalize(self): pass

My idea was really trying to respect the composite structure as much as possible. I like the idea of making RegionConfig a base class. I feel like having one class handle two levels of the tree (in this case regions and functions) is somewhat against the purpose of having a composite pattern, because you are hiding multiple layers of the structure in one node. Splitting the service/region-specific configs in one class and the instance-specific configs in another one is a sweet spot.

I'll explore a few different implementations and push the most interesting here for review.

[M]odifiying fetch_all signature, to be able to pass a service argument. More than that, it seems like you don't need credentials and targets argument. I would suggest to modify the base class ResourceConfig to be more general

I agree, the fetch_all signature should be more general.

My idea was really trying to respect the composite structure as much as possible. I like the idea of making RegionConfig a base class. I feel like having one class handle two levels of the tree (in this case regions and functions) is somewhat against the purpose of having a composite pattern, because you are hiding multiple layers of the structure in one node. Splitting the service/region-specific configs in one class and the instance-specific configs in another one is a sweet spot.

I'll explore a few different implementations and push the most interesting here for review.

I really like your new implementation! I think I finally got your point about having two separated classes, I was mainly confused by the terminology being used, I believe. Would you mind if I suggest some changes in the name of your classes, to make things clearer?

I agree, the fetch_all signature should be more general.

Done, I just pushed a more general fetch_all method in refactoring/resource_config.

I think I finally got your point about having two separated classes [...]

I think too! Great teamwork ✋

Would you mind if I suggest some changes in the name of your classes, to make things clearer?

On the contrary; I'm definitely not satisfied with my classes names. I know we talked about removing the Config suffix. This would give us something like: Resource, Region, LambdaService, LambdaFunction. Did you have another idea in mind?

EDIT: just saw your other comments. I like those names too, I'll apply your suggestions.

ScoutSuite/providers/aws/services/awslambda.py

misg · 2019-02-21T03:22:06Z

ScoutSuite/providers/aws/configs/regions_config.py

+from ScoutSuite.providers.aws.aws_facade import AwsFacade
+
+
+class RegionsConfig(ResourceConfig):


My idea was really trying to respect the composite structure as much as possible. I like the idea of making RegionConfig a base class. I feel like having one class handle two levels of the tree (in this case regions and functions) is somewhat against the purpose of having a composite pattern, because you are hiding multiple layers of the structure in one node. Splitting the service/region-specific configs in one class and the instance-specific configs in another one is a sweet spot.

I'll explore a few different implementations and push the most interesting here for review.

I really like your new implementation! I think I finally got your point about having two separated classes, I was mainly confused by the terminology being used, I believe. Would you mind if I suggest some changes in the name of your classes, to make things clearer?

I agree, the fetch_all signature should be more general.

Done, I just pushed a more general fetch_all method in refactoring/resource_config.

ScoutSuite/providers/aws/configs/regions_config.py

ScoutSuite/providers/aws/services/awslambda.py

misg · 2019-02-21T03:30:44Z

ScoutSuite/providers/aws/services/awslambda.py

+            self['regions'][region] = functions
+
+
+class LambdasConfig(ResourceConfig):


And since this classe stores lambdas of a specific region, I would call it RegionalLambdas.

ScoutSuite/providers/aws/services/awslambda.py

Co-Authored-By: Aboisier <aboisiermichaud@gmail.com>

x4v13r64 · 2019-02-21T08:34:15Z

ScoutSuite/providers/aws/aws_facade.py

@@ -0,0 +1,34 @@
+import boto3


why not just facade.py?

Yea I guess this would be more consistent with the codebase. It's so annoying though when you use the fuzzy search to open a file and it suggests 4 different files all named provider.py 😆

x4v13r64 · 2019-02-21T08:37:11Z

ScoutSuite/providers/aws/aws_facade.py

+from botocore.session import Session
+from collections import Counter
+
+# TODO: Handle authentication better. I don't even know how it currently works. I think connect_service is called somewhere.


It's handled by the authenticate method of each provider (e.g. https://github.com/nccgroup/ScoutSuite/blob/master/ScoutSuite/providers/aws/provider.py#L52). For AWS we're passing the CLI params to Opinel, which does most of the work.

ScoutSuite/providers/aws/aws_facade.py

x4v13r64 · 2019-02-21T08:40:16Z

ScoutSuite/providers/aws/configs/services.py

@@ -64,7 +64,7 @@ def __init__(self, metadata=None, thread_config=4, **kwargs):
        self.emr = EMRConfig(metadata['analytics']['emr'], thread_config)
        self.iam = IAMConfig(thread_config)
        self.kms = KMSConfig(metadata['security']['kms'], thread_config)
-        self.awslambda = LambdaConfig(metadata['compute']['awslambda'], thread_config)


Out with the metadata ⛏ ⛏ ⛏

…da-config

…nto refactoring/aws-lambda-config

…da-config

…bdas

…da-config

Remi05 · 2019-02-22T18:38:14Z

Great work! I feel like this new architecture will really make the code more maintainable and easier to understand going forward! 👍

ScoutSuite/providers/aws/configs/regions_config.py

Aboisier · 2019-02-25T01:53:04Z

Thanks for reviewing and all! I'm just going to close this though as most of the changes are contained in #202 and it has a whole lot of conflicts with this branch.

Aboisier added 7 commits February 19, 2019 09:05

Merge branch 'develop' into refactoring/aws-lambda-config

323794f

Merge branch 'refactoring/resource-configs' into refactoring/aws-lamb…

fbac59b

…da-config

Implemented aws facade

021854f

Implemented regions config

82eb22f

Migrated aws lambda to new architecture

5c2a02a

Merge branch 'refactoring/resource-configs' into refactoring/aws-lamb…

d6f0248

…da-config

Moved build region list to aws facade

ce9e9c1

Aboisier requested review from misg and Remi05 February 20, 2019 20:15

Aboisier self-assigned this Feb 20, 2019

Aboisier added refactoring WIP labels Feb 20, 2019

misg reviewed Feb 20, 2019

View reviewed changes

Aboisier added 4 commits February 20, 2019 17:49

Update aws_facade.py

0f3445a

Removed unnecessary line

d4a22df

Made the code more pythonic

26108c7

Made RegionConfig a base class

f8ac389

misg reviewed Feb 21, 2019

View reviewed changes

misg and others added 2 commits February 20, 2019 22:45

Used [] instead of set default

7f7ab96

Co-Authored-By: Aboisier <aboisiermichaud@gmail.com>

Removed unused arguments

5e9eec1

x4v13r64 reviewed Feb 21, 2019

View reviewed changes

ScoutSuite/providers/aws/aws_facade.py Outdated Show resolved Hide resolved

x4v13r64 reviewed Feb 21, 2019

View reviewed changes

Aboisier added 4 commits February 21, 2019 08:46

Merge branch 'refactoring/resource-configs' into refactoring/aws-lamb…

6ae133a

…da-config

Merge remote-tracking branch 'origin/refactoring/aws-lambda-config' i…

cdc465a

…nto refactoring/aws-lambda-config

Merge branch 'refactoring/resource-configs' into refactoring/aws-lamb…

9e4d2d3

…da-config

Added a bit of syntactic sugar to simplify the code

c3ba74e

Aboisier changed the title ~~Refactoring/aws lambda config~~ WIP - Refactoring/aws lambda config Feb 21, 2019

Renamed AwsFactory to AWSFactory

b000ea4

Aboisier added 6 commits February 21, 2019 09:16

Renamed LambdasConfig to RegionalLambdas

f5295ab

Added parameter for compatibility, renamed LambdaServiceConfig to Lam…

bac7a11

…bdas

Tried fixing tests.

bbcbf8d

Tried making some test methods async

522a6b9

Merge branch 'refactoring/resource-configs' into refactoring/aws-lamb…

4504cab

…da-config

Renamed aws_facade to facade

febbdbe

Remi05 approved these changes Feb 22, 2019

View reviewed changes

misg approved these changes Feb 24, 2019

View reviewed changes

ScoutSuite/providers/aws/configs/regions_config.py Show resolved Hide resolved

Aboisier closed this Feb 25, 2019

Aboisier mentioned this pull request Mar 13, 2019

Formalize the providers architecture #183

Closed

53 tasks

Aboisier deleted the refactoring/aws-lambda-config branch March 20, 2019 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP - Refactoring/aws lambda config #199

WIP - Refactoring/aws lambda config #199

Aboisier commented Feb 20, 2019 •

edited

Loading

misg left a comment

misg Feb 20, 2019

Aboisier Feb 21, 2019

misg Feb 20, 2019

Aboisier Feb 21, 2019 •

edited

Loading

misg Feb 21, 2019

Aboisier Feb 21, 2019 •

edited

Loading

misg Feb 21, 2019

misg Feb 21, 2019

x4v13r64 Feb 21, 2019

Aboisier Feb 21, 2019

x4v13r64 Feb 21, 2019 •

edited

Loading

x4v13r64 Feb 21, 2019

Remi05 commented Feb 22, 2019

Aboisier commented Feb 25, 2019

	return list((Counter(regions) & Counter(chosen_regions)).elements())
	return list((Set(regions) & Set(chosen_regions))

		from ScoutSuite.providers.aws.aws_facade import AwsFacade


		class RegionsConfig(ResourceConfig):

		self['regions'][region] = functions


		class LambdasConfig(ResourceConfig):

WIP - Refactoring/aws lambda config #199

WIP - Refactoring/aws lambda config #199

Conversation

Aboisier commented Feb 20, 2019 • edited Loading

misg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aboisier Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aboisier Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x4v13r64 Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remi05 commented Feb 22, 2019

Aboisier commented Feb 25, 2019

Aboisier commented Feb 20, 2019 •

edited

Loading

Aboisier Feb 21, 2019 •

edited

Loading

Aboisier Feb 21, 2019 •

edited

Loading

x4v13r64 Feb 21, 2019 •

edited

Loading