Added support for AWS KMS #527

mpurusottamc · 2021-02-02T20:47:25Z

This is an initial version of pulling KMS key, alias and grant details and populating graph db.

# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

…to aws-kms

achantavy · 2021-02-10T06:12:52Z

@mpurusottamc Thanks so much for your PR and sorry for the delays. We've all been a bit more tied up than normal but I'll be able to get you a review late this week/early next week if someone else doesn't beat me to it.

docs/schema/aws.md

cartography/intel/aws/kms.py

achantavy

Thanks for your PR! I left a few comments about performance, schema conventions, and potentially using resource permission relationships. Let me know if you have any questions and hope this helps.

cartography/intel/aws/kms.py

achantavy · 2021-02-11T15:37:10Z

cartography/intel/aws/kms.py

+    for kms_key in data:
+        neo4j_session.run(
+            ingest_keys,
+            KeyId=kms_key["KeyId"],


What is the difference between a KeyId and an Arn?

For AWS syncs, we should to do something like

ON CREATE SET kmskey.id = kms_key.arn, kmskey.firstseen = timestamp() SET kmskey.arn = kms_key.arn, [...]

It's a bit duplicative, but at least viewers of your data will be able to tell what field(s) the id was based off of.

KeyId is just the Key Identifier, whereas ARN is fully formed URI for the Key. While populating Neo4j, we are using KeyId as the primary identifier.

As suggested, we have used UNWIND method for populating keys, grants and aliases.

achantavy · 2021-02-11T15:37:44Z

cartography/intel/aws/kms.py

+@timeit
+def parse_policy(key, policy):
+    """
+    Uses PolicyUniverse to parse S3 policies and returns the internet accessibility results


Copypasta; this isn't an s3 policy

Removed (oops!)

achantavy · 2021-02-11T15:41:07Z

cartography/intel/aws/kms.py

+    ingest_policies = """
+    UNWIND {policies} AS policy
+    MATCH (k:KMSKey) where k.name = policy.kms_key
+    SET k.anonymous_access = (coalesce(k.anonymous_access, false) OR policy.internet_accessible),


Your schema documentation should include what kmskey.anonymous_access means.

schema updated

achantavy · 2021-02-11T15:41:20Z

cartography/intel/aws/kms.py

+    Ingest KMS Key Grants into neo4j.
+    """
+    ingest_grants = """
+    MERGE (g:KMSGrant{id: {GrantId}})


Same comment about using unwind.

achantavy · 2021-02-11T15:41:45Z

cartography/intel/aws/kms.py

+    for grant in grants_list:
+        neo4j_session.run(
+            ingest_grants,
+            GrantId=grant["GrantId"],


Is this based off an Arn?

Each KMS Key Grant has an Id. but no ARN. That's why we are using the GrantId as the primary field.

achantavy · 2021-02-11T15:43:42Z

cartography/intel/aws/kms.py

+
+
+@timeit
+def load_kms_key_details(neo4j_session, policy_alias_grants_data, region, aws_account_id, update_tag):


I realize you used the s3 sync as a model here, and while this works, I wonder if using Resource Permission Relationships would be more powerful. See https://github.com/lyft/cartography/blob/master/docs/usage/permissions-mapping.md and (for a longer read) https://eng.lyft.com/iam-whatever-you-say-iam-febce59d1e3b.

The rough idea is that you edit the permission_relationships.yaml file and cartography draws links from the IAM principals to the resource nodes based on the allowed actions:

This RPR change for KMS keys might not get merged here depending on how it impacts performance (which is why we have that yaml config file that lets users decide which permissions they care about syncing) but I wanted to let you know that this feature exists.

@achantavy you are right, we used S3 as the reference while populating KMS entities into Cartography. Our idea is to capture all the policies, statements, etc. which would allow the users for more fine grained queries

achantavy · 2021-02-11T15:52:21Z

Oh another thing, please write integration tests to ensure your new nodes and relationships are created as expected; see https://github.com/lyft/cartography/blob/master/tests/integration/cartography/intel/aws/test_eks.py.

mpurusottamc · 2021-02-16T06:24:58Z

@achantavy we have made all the required changes based on your feedback.

achantavy

Thank you very much for your contribution! :)

achantavy · 2021-02-18T07:17:31Z

cartography/intel/aws/kms.py

+        key['DeletionDate'] = str(key.get('DeletionDate'))
+        key['ValidTo'] = str(key.get('ValidTo'))
+
+    # The owner data returned by the API maps to the aws account nickname and not the IAM user


I see that this comment was copied from the s3 sync, is it still relevant here?

@achantavy No it's not relevant. I have removed it in latest commit :)

I think you need to update your branch with the latest master and then I can merge it in.

@achantavy Done :D

mpurusottamc and others added 14 commits December 5, 2020 07:47

Updated Readme to add Cloudanix as a Cartography user

9eaba96

Merge remote-tracking branch 'upstream/master' into master

2cd649d

# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

Merge branch 'master' of github.com:lyft/cartography

8d33f8d

Added KMS module in AWS for cartography

62e9b7d

Merge branch 'aws-kms' of https://github.com/Cloudanix/cartography in…

bf4ac34

…to aws-kms

Added indexes and removed TODOs

b5f5999

fixed kms policy key issue

639118e

updated documentation for AWS KMS

6811fb2

removed unnecessary references from kms

bfe44e3

Merge branch 'upstream-master' into aws-kms

eac9dd6

Merge branch 'master' into aws-kms

9cbac13

Minor changes as per linter

2728f35

Minor changes

b5d6f40

removed trailing space

cc136d7

marco-lancini reviewed Feb 10, 2021

View reviewed changes

docs/schema/aws.md Show resolved Hide resolved

cartography/intel/aws/kms.py Outdated Show resolved Hide resolved

cartography/intel/aws/kms.py Show resolved Hide resolved

mpurusottamc added 3 commits February 10, 2021 07:32

added @aws_handle_regions for functions which fetch data by region

95bd761

added KMSAlias and KMSGrant details into aws schema document

59e1971

removed verbose logs

c7596f9

achantavy requested changes Feb 11, 2021

View reviewed changes

kedarghule and others added 8 commits February 15, 2021 22:40

Used UNWIND instead of the loops, removed copypasta (Oops!)

4555717

Trimmed trailing whitespace

562e8ac

Integration tests added

6d64be6

Added trailing commas

f9b7ab7

Added trailing commas 2

4af174d

Minor updates

3ea24c0

Added trailing commas 3

6f97387

kms schema updated to add anonymous access & actions

c14d378

mpurusottamc requested a review from achantavy February 16, 2021 06:24

achantavy previously approved these changes Feb 18, 2021

View reviewed changes

Removed unnecessary comment

d7b4d12

kedarghule dismissed achantavy’s stale review via d7b4d12 February 19, 2021 04:37

Merge branch 'master' into aws-kms

197d1ba

achantavy approved these changes Feb 19, 2021

View reviewed changes

achantavy merged commit 69eda2e into lyft:master Feb 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for AWS KMS #527

Added support for AWS KMS #527

mpurusottamc commented Feb 2, 2021

achantavy commented Feb 10, 2021

achantavy left a comment

achantavy Feb 11, 2021

mpurusottamc Feb 16, 2021

achantavy Feb 11, 2021

kedarghule Feb 15, 2021

achantavy Feb 11, 2021

mpurusottamc Feb 16, 2021

achantavy Feb 11, 2021

kedarghule Feb 15, 2021

achantavy Feb 11, 2021

mpurusottamc Feb 16, 2021

achantavy Feb 11, 2021

mpurusottamc Feb 16, 2021

achantavy commented Feb 11, 2021

mpurusottamc commented Feb 16, 2021

achantavy left a comment

achantavy Feb 18, 2021

kedarghule Feb 19, 2021 •

edited

Loading

achantavy Feb 19, 2021

kedarghule Feb 19, 2021



		@timeit
		def load_kms_key_details(neo4j_session, policy_alias_grants_data, region, aws_account_id, update_tag):

Added support for AWS KMS #527

Added support for AWS KMS #527

Conversation

mpurusottamc commented Feb 2, 2021

achantavy commented Feb 10, 2021

achantavy 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

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

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

achantavy commented Feb 11, 2021

mpurusottamc commented Feb 16, 2021

achantavy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kedarghule Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kedarghule Feb 19, 2021 •

edited

Loading