-
Notifications
You must be signed in to change notification settings - Fork 344
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
Added in Identity Center Nodes + relationship mapping #1380
Conversation
@achantavy Hey Alex ... how are you today? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Gave it a first pass and left comments about the data model. If something doesn't make sense let's get on a call. I can also push to your branch to show you what I mean.
""" | ||
logger.info(f"Loading {len(role_assignments)} role assignments") | ||
if role_assignments: | ||
neo4j_session.run( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the data model here. I think we can do this by creating a SSOUserToRole
relationship.
We can have a transform
step that shapes the data returned from get_sso_users()
and get_role_assignments()
into what we need.
This way we don't have to use neo4j_session.run()
(since this can fail and does not retry automatically), and the data model does batching and automatic cleanups and all that.
Let's get on a call if you're stuck with this. I'm also happy to push changes up to your branch if that's easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as above. There is no easy transform as it depends on relationships based on data already in the graph. I have to find the specific role by doing
(acc:AWSAccount{id:ra.AccountId}) -[:RESOURCE]->
(role:AWSRole)<-[:ASSIGNED_TO_ROLE]-
(permset:AWSPermissionSet {id: ra.PermissionSetArn})
I am looking for a role in the middle of the AWSAccount -> AWSPermissionSet path
{ | ||
"statements": [ | ||
{ | ||
"query": "MATCH (n:AWSIdentityCenterInstance) WHERE n.lastupdated <> $UPDATE_TAG WITH n LIMIT $LIMIT_SIZE DETACH DELETE (n) RETURN COUNT(*) as TotalDeleted", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the sync job runs one account at a time, this will currently delete all other AWSIdentityCenterInstance nodes in other accounts when we actually want to only delete the stale ones in the current account.
So since you've already built a data mode for this, you don't need to have a manually written cleanup job anymore and you can just do something like this:
cartography/cartography/intel/aws/ec2/instances.py
Lines 307 to 310 in bf8d667
def cleanup(neo4j_session: neo4j.Session, common_job_parameters: Dict[str, Any]) -> None: | |
logger.debug("Running EC2 instance cleanup") | |
GraphJob.from_node_schema(EC2ReservationSchema(), common_job_parameters).run(neo4j_session) | |
GraphJob.from_node_schema(EC2InstanceSchema(), common_job_parameters).run(neo4j_session) |
It will automatically generate and run the cleanup queries for you so you don't have to worry about any mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smart move. I will update
|
||
|
||
@dataclass(frozen=True) | ||
class SSOUserSchema(CartographyNodeSchema): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add in a sub_resource_relationship with label RESOURCE
to the AWSAccount. This makes it so that the autocleanup queries are easy to run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pending
|
||
|
||
@dataclass(frozen=True) | ||
class IdentiyCenterToAwsAccountRelProperties(CartographyRelProperties): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: should be IdentityCenterToAwsAccountRelProperties
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pending fix
### Summary > Describe your changes. We often need to connect 2 nodes based on a fuzzy match like in #1380 (comment). This PR adds a `fuzzy_and_ignore_case` option to the `PropertyRef` so that the final rendered query performs a `CONTAINS` in a case-insensitive way instead of an exact match. ### Related issues or links > Include links to relevant issues or other pages. #1380 ### Checklist Provide proof that this works (this makes reviews move faster). Please perform one or more of the following: - [x] Update/add unit or integration tests. - [ ] Include a screenshot showing what the graph looked like before and after your changes. - [ ] Include console log trace showing what happened before and after your changes. --------- Signed-off-by: Alex Chantavy <achantavy@lyft.com>
All of the fixes have been resolved. Added Fuzzy matching and removed try-catch blocks around boto3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! looks good. I need to figure out how to support some of the weird role mapping stuff with the model but that can come in a follow-up
for page in paginator.paginate(): | ||
instances.extend(page.get('Instances', [])) | ||
except client.exceptions.ClientError as e: | ||
logger.warning(f"Failed to get Identity Center instances in region {region}: {e}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the exception handling so that it looks like this instead?
cartography/cartography/intel/aws/ec2/network_interfaces.py
Lines 35 to 41 in c86e388
def get_network_interface_data(boto3_session: boto3.session.Session, region: str) -> List[Dict[str, Any]]: | |
client = boto3_session.client('ec2', region_name=region, config=get_botocore_config()) | |
paginator = client.get_paginator('describe_network_interfaces') | |
subnets: List[Dict] = [] | |
for page in paginator.paginate(): | |
subnets.extend(page['NetworkInterfaces']) | |
return subnets |
If we don't allow the sync to crash here, it will continue on to the cleanup job, and then the cleanup job will incorrectly believe that all previously synced identity center instances are stale and then delete them.
MATCH (ps:AWSPermissionSet {arn: ra.PermissionSetArn}) | ||
MATCH (role:AWSRole) | ||
WHERE role.arn STARTS WITH ra.RoleArn | ||
MERGE (ps)-[r:ASSIGNED_TO_ROLE]->(role) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fuzzy match
MATCH (acc:AWSAccount{id:ra.AccountId}) -[:RESOURCE]-> | ||
(role:AWSRole)<-[:ASSIGNED_TO_ROLE]- | ||
(permset:AWSPermissionSet {id: ra.PermissionSetArn}) | ||
MATCH (sso:AWSSSOUser {id: ra.UserId}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(notes to self) Per our convo: AWS identitycenter is weird. SSO users are associated with perm sets, and can be assigned to roles, but this assignment only takes place in one account.
…phy-cncf#1383) ### Summary > Describe your changes. We often need to connect 2 nodes based on a fuzzy match like in cartography-cncf#1380 (comment). This PR adds a `fuzzy_and_ignore_case` option to the `PropertyRef` so that the final rendered query performs a `CONTAINS` in a case-insensitive way instead of an exact match. ### Related issues or links > Include links to relevant issues or other pages. cartography-cncf#1380 ### Checklist Provide proof that this works (this makes reviews move faster). Please perform one or more of the following: - [x] Update/add unit or integration tests. - [ ] Include a screenshot showing what the graph looked like before and after your changes. - [ ] Include console log trace showing what happened before and after your changes. --------- Signed-off-by: Alex Chantavy <achantavy@lyft.com> Signed-off-by: chandanchowdhury <chandan.chowdhury@hotmail.com>
…cf#1380) **Summary** Mapped in [AWS Identity Center](https://aws.amazon.com/iam/identity-center/) and the access it provides to AWS accounts. New Nodes: (AWSIdentityCenter), (AWSPermissionSet), (AWSSSOUser) New Relationships: (AWSAccount)-[RESOURCE]->(AWSIdentityCenter) (AWSIdentityCenter)-[HAS_PERMISSION_SET]->(AWSPermissionSet) (AWSSSOUser)<-[ALLOWED_BY]-(AWSRole) (OktaUser)<-[CAN_ASSUME_IDENTITY]-(AWSSSOUser) (AWSPermissionSet)-[ASSIGNED_TO_ROLE]->(AWSRole) ![image](https://github.com/user-attachments/assets/e0e6c746-8ef6-4c89-b08a-d5192277fbda) ![image](https://github.com/user-attachments/assets/6ec645b8-6157-4001-b6f6-f44dbc3df2cc) **Console Trace** INFO:cartography.intel.aws.identitycenter:Syncing Identity Center instances for region us-east-1 INFO:cartography.intel.aws.identitycenter:Loading 1 Identity Center instances for region us-east-1 INFO:cartography.intel.aws.identitycenter:Loading 32 permission sets for instance arn:aws:sso:::instance/ssoins-72237a0dcb8c6df7 in region us-east-1 INFO:cartography.intel.aws.identitycenter:Loading 777 permission set role assignments INFO:cartography.intel.aws.identitycenter:Loading 803 SSO users for identity store d-906747a0b9 in region us-east-1 INFO:cartography.intel.aws.identitycenter:Getting role assignments for 803 users INFO:cartography.intel.aws.identitycenter:Loading 24292 role assignments INFO:cartography.intel.aws.identitycenter:Syncing Identity Center instances for region us-east-2 INFO:cartography.intel.aws.identitycenter:Loading 0 Identity Center instances for region us-east-2 INFO:cartography.intel.aws.identitycenter:Syncing Identity Center instances for region us-west-1 INFO:cartography.intel.aws.identitycenter:Loading 0 Identity Center instances for region us-west-1 INFO:cartography.intel.aws.identitycenter:Syncing Identity Center instances for region us-west-2 INFO:cartography.intel.aws.identitycenter:Loading 0 Identity Center instances for region us-west-2 INFO:cartography.graph.statement:Completed aws_import_identity_center_cleanup statement #1 INFO:cartography.graph.statement:Completed aws_import_identity_center_cleanup statement cartography-cncf#2 INFO:cartography.graph.statement:Completed aws_import_identity_center_cleanup statement cartography-cncf#3 INFO:cartography.graph.statement:Completed aws_import_identity_center_cleanup statement cartography-cncf#4 INFO:cartography.graph.statement:Completed aws_import_identity_center_cleanup statement cartography-cncf#5 INFO:cartography.graph.statement:Completed aws_import_identity_center_cleanup statement cartography-cncf#6 **Related issues or links** Fixes - cartography-cncf#990 Checklist Provide proof that this works (this makes reviews move faster). Please perform one or more of the following: [ x ] Update/add unit or integration tests. [ X ] Include a screenshot showing what the graph looked like before and after your changes. [ X ] Include console log trace showing what happened before and after your changes. If you are changing a node or relationship: [ x ] Update the [schema](https://github.com/lyft/cartography/tree/master/docs/root/modules) and [readme](https://github.com/lyft/cartography/blob/master/docs/schema/README.md). If you are implementing a new intel module: [ X ] Use the NodeSchema [data model](https://cartography-cncf.github.io/cartography/dev/writing-intel-modules.html#defining-a-node). --------- Signed-off-by: chandanchowdhury <chandan.chowdhury@hotmail.com>
Summary
Mapped in AWS Identity Center and the access it provides to AWS accounts.
New Nodes: (AWSIdentityCenter), (AWSPermissionSet), (AWSSSOUser)
New Relationships:
(AWSAccount)-[RESOURCE]->(AWSIdentityCenter)
(AWSIdentityCenter)-[HAS_PERMISSION_SET]->(AWSPermissionSet)
(AWSSSOUser)<-[ALLOWED_BY]-(AWSRole)
(OktaUser)<-[CAN_ASSUME_IDENTITY]-(AWSSSOUser)
(AWSPermissionSet)-[ASSIGNED_TO_ROLE]->(AWSRole)
Console Trace
INFO:cartography.intel.aws.identitycenter:Syncing Identity Center instances for region us-east-1 INFO:cartography.intel.aws.identitycenter:Loading 1 Identity Center instances for region us-east-1 INFO:cartography.intel.aws.identitycenter:Loading 32 permission sets for instance arn:aws:sso:::instance/ssoins-72237a0dcb8c6df7 in region us-east-1 INFO:cartography.intel.aws.identitycenter:Loading 777 permission set role assignments INFO:cartography.intel.aws.identitycenter:Loading 803 SSO users for identity store d-906747a0b9 in region us-east-1 INFO:cartography.intel.aws.identitycenter:Getting role assignments for 803 users INFO:cartography.intel.aws.identitycenter:Loading 24292 role assignments INFO:cartography.intel.aws.identitycenter:Syncing Identity Center instances for region us-east-2 INFO:cartography.intel.aws.identitycenter:Loading 0 Identity Center instances for region us-east-2 INFO:cartography.intel.aws.identitycenter:Syncing Identity Center instances for region us-west-1 INFO:cartography.intel.aws.identitycenter:Loading 0 Identity Center instances for region us-west-1 INFO:cartography.intel.aws.identitycenter:Syncing Identity Center instances for region us-west-2 INFO:cartography.intel.aws.identitycenter:Loading 0 Identity Center instances for region us-west-2 INFO:cartography.graph.statement:Completed aws_import_identity_center_cleanup statement #1 INFO:cartography.graph.statement:Completed aws_import_identity_center_cleanup statement #2 INFO:cartography.graph.statement:Completed aws_import_identity_center_cleanup statement #3 INFO:cartography.graph.statement:Completed aws_import_identity_center_cleanup statement #4 INFO:cartography.graph.statement:Completed aws_import_identity_center_cleanup statement #5 INFO:cartography.graph.statement:Completed aws_import_identity_center_cleanup statement #6
Related issues or links
Fixes - #990
Checklist
Provide proof that this works (this makes reviews move faster). Please perform one or more of the following:
[ x ] Update/add unit or integration tests.
[ X ] Include a screenshot showing what the graph looked like before and after your changes.
[ X ] Include console log trace showing what happened before and after your changes.
If you are changing a node or relationship:
[ x ] Update the schema and readme.
If you are implementing a new intel module:
[ X ] Use the NodeSchema data model.