Skip to content
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

Fix IAM policy naming for inline policies with empty string Sid field. #1389

Merged
merged 2 commits into from
Nov 23, 2024

Conversation

SecPrez
Copy link
Contributor

@SecPrez SecPrez commented Nov 19, 2024

Summary

AWS is breaking its own rules for some roles it has created in IdentityCenter. IdentityCenter created roles with inline policies that can have multiple statements, all with the Sid of an empty string. This means cartography will only load the last statement as they will all have the same AWSPolicyStatement.id. This change checks if the Sid is an empty string and uses the index numbering if it is encountered.

Related issues or links

Include links to relevant issues or other pages.

Checklist

Provide proof that this works (this makes reviews move faster). Please perform one or more of the following:

  • 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.

If you are changing a node or relationship:

If you are implementing a new intel module:

Copy link
Contributor

@achantavy achantavy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally there'd be a test included for this or something to show the before and after but the change looks reasonable and small enough to me. Thank you!

@achantavy achantavy enabled auto-merge (squash) November 23, 2024 05:37
@achantavy achantavy merged commit 9b2a3e4 into cartography-cncf:master Nov 23, 2024
7 checks passed
chandanchowdhury pushed a commit to chandanchowdhury/cartography that referenced this pull request Nov 27, 2024
cartography-cncf#1389)

### Summary
> AWS is breaking its own rules for some roles it has created in
IdentityCenter. IdentityCenter created roles with inline policies that
can have multiple statements, all with the Sid of an empty string. This
means cartography will only load the last statement as they will all
have the same AWSPolicyStatement.id. This change checks if the Sid is an
empty string and uses the index numbering if it is encountered.

### Related issues or links
> Include links to relevant issues or other pages.

- https://github.com/lyft/cartography/issues/...

### Checklist

Provide proof that this works (this makes reviews move faster). Please
perform one or more of the following:
- [ ] 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.

If you are changing a node or relationship:
- [ ] 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:
- [ ] Use the NodeSchema [data
model](https://cartography-cncf.github.io/cartography/dev/writing-intel-modules.html#defining-a-node).

Co-authored-by: = <=>
Co-authored-by: Alex Chantavy <[email protected]>
Signed-off-by: chandanchowdhury <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants