Netflix / security_monkey

Security Monkey monitors AWS, GCP, OpenStack, and GitHub orgs for assets and their changes over time.
Apache License 2.0
4.36k stars 800 forks source link

Python3.7 conversion #1234

Closed CPCJ79 closed 4 years ago

CPCJ79 commented 4 years ago

updated all tests and modules to python3.7.6, end to end validation including an active account scan and population of facts with reporting etc.

Updated travis for build and input validation

updated dox for mac development

mikegrima commented 4 years ago

Hello! Is this PR ready to go? I know that some of the tests will fail because things will need to be upgraded to reflect changes in moto (thanks to yours truly, actually)

CPCJ79 commented 4 years ago

Hey Mike

I just need to wrap up the docker items. I have been down with the plague for the past week, apologies for leaving it. I will clean this up.

The code is all done and all tests pass locally, only one test fails on Travis, and I just need to clean up the docker test.

Thank you for reaching out,

Casey Reed Product Security

On Sat, Mar 7, 2020 at 6:12 PM Mike Grima notifications@github.com wrote:

Hello! Is this PR ready to go? I know that some of the tests will fail because things will need to be upgraded to reflect changes in moto (thanks to yours truly, actually)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Netflix/security_monkey/pull/1234?email_source=notifications&email_token=AN635H3LFRVSV4QVMUESP5DRGL5JLA5CNFSM4LAA2N7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOEKNAA#issuecomment-596158080, or unsubscribe https://github.com/notifications/unsubscribe-auth/AN635HY63FJCJ2MAIUQXAJLRGL5JLANCNFSM4LAA2N7A .

CPCJ79 commented 4 years ago

Hey Mike,

I think we are finally good to go.

with a clean venv all tests and docker ops pass locally. It looks like one test fails on travis, but I have repeatedly verified it locally from a scratch install so Im it has something to do with changes in boto, mock, and how the list operation was manually mocked out. There are additional controls that did not exist at the time. I'm trying to avoid rewriting too much making this a uni-directional port to simplify the next step of updating things that depend on boto/moto/mock.

So if you are comfortable proceeding with scrutiny, this has been tested, and exercised against a live aws account for audit.

Please let me know what you would want to do with next steps, I'm going to install this side by side with our install to have a running comparison until we are all happy and ready to shift.

Thank you

Casey Reed Product Security

On Mon, Mar 9, 2020 at 1:55 PM Casey Reed caseyr@23andme.com wrote:

Hey Mike

I just need to wrap up the docker items. I have been down with the plague for the past week, apologies for leaving it. I will clean this up.

The code is all done and all tests pass locally, only one test fails on Travis, and I just need to clean up the docker test.

Thank you for reaching out,

Casey Reed Product Security

On Sat, Mar 7, 2020 at 6:12 PM Mike Grima notifications@github.com wrote:

Hello! Is this PR ready to go? I know that some of the tests will fail because things will need to be upgraded to reflect changes in moto (thanks to yours truly, actually)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Netflix/security_monkey/pull/1234?email_source=notifications&email_token=AN635H3LFRVSV4QVMUESP5DRGL5JLA5CNFSM4LAA2N7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOEKNAA#issuecomment-596158080, or unsubscribe https://github.com/notifications/unsubscribe-auth/AN635HY63FJCJ2MAIUQXAJLRGL5JLANCNFSM4LAA2N7A .

mikegrima commented 4 years ago

Looking at this now.

Also cool that this important PR is numbered 1234 :)

CPCJ79 commented 4 years ago

Thank you for the compliment.

The tests moved to not_used are merely because their functions now exist in boto, so I did not want so spend time re-replicating a function I will immediately update with boto.

Once this PR is merged, the first thing I will do is begin re-implementing boto3 to realign with aws functions and place the burden of support of those functions on the AWS community. I have reviewed this with my internal management and have received positive feedback, and alignment to follow this directive.

Please let me know if you have any other questions, I would be happy to review if I have gone off course.

Thank you kindly

Casey Reed Product Security

On Fri, Mar 20, 2020 at 8:37 AM Mike Grima notifications@github.com wrote:

@mikegrima commented on this pull request.

I noticed that some tests were moved to not_used. Were these too entrenched with Python 2 code?

Other than that, this is an outstanding PR.

In security_monkey/auditors/iam/iam_policy.py https://github.com/Netflix/security_monkey/pull/1234#discussion_r395709673 :

@@ -210,10 +210,10 @@ def library_check_attached_managed_policies(self, iam_item, iam_type): mp_arn = mp_item.config.get('arn', mp_item.config.get('Arn')) if mp_arn == item_mp_arn: found = True

  • for issue in mp_item.db_item.issues:

Disabling managed policy checks?

Personally, I don't have an issue with it since the issue system is mostly broken. But just wanted to point out.

In security_monkey/tests/watchers/test_iam_role.py https://github.com/Netflix/security_monkey/pull/1234#discussion_r395716650 :

@@ -72,7 +74,7 @@ def pre_test_setup(self):

     for x in range(0, self.total_roles):
         # Create the IAM Role via Moto:
  • aspd["Statement"][0]["Resource"] = ARN_PREFIX + ":iam:012345678910:role/roleNumber{}".format(x)
  • aspd["Statement"][0]["Resource"] = ARN_PREFIX + "arn:aws:iam:012345678910:role/roleNumber{}".format(x)

is ARN_PREFIX not set to anything?

In security_monkey/tests/watchers/test_s3.py https://github.com/Netflix/security_monkey/pull/1234#discussion_r395717208 :

@@ -66,6 +66,3 @@ def test_slurp(self): item_list, exception_map = s3_watcher.slurp()

     assert len(item_list) == 3  # We created 3 buckets

-

  • mock_s3().stop()

Noticing the removal of the stop()s on the mocks. Is this OK?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Netflix/security_monkey/pull/1234#pullrequestreview-378577699, or unsubscribe https://github.com/notifications/unsubscribe-auth/AN635H3T7EBWVL6BQBC2RUDRIOESRANCNFSM4LAA2N7A .

mikegrima commented 4 years ago

Some of the not_used tests were for GitHub org checking and OpenStack.

I'm OK with keeping this as is for now, but a follow-up PR should re-enable them.