Skip to content

AWS nacl analysis #732

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

Merged
merged 26 commits into from
Aug 8, 2024
Merged

AWS nacl analysis #732

merged 26 commits into from
Aug 8, 2024

Conversation

olasaadi99
Copy link
Contributor

No description provided.

@olasaadi99 olasaadi99 marked this pull request as ready for review July 28, 2024 14:25
Copy link
Contributor

@haim-kermany haim-kermany left a comment

Choose a reason for hiding this comment

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

looks good
please let me know if you have any question

@olasaadi99 olasaadi99 requested a review from haim-kermany August 4, 2024 12:21
haim-kermany
haim-kermany previously approved these changes Aug 4, 2024
Copy link
Member

Choose a reason for hiding this comment

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

@kyorav , can you please verify that this is indeed the intended connectivity?

Copy link

Choose a reason for hiding this comment

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

@zivnevo it is difficult to do without a mapping of the SubnetIds to the name. Can we perhaps prioritize fixing the anonymizer to leave the Name tags and then using the name here instead of the CRN?

For this instance I will re-deploy the example and run the analysis on the original collected object, then figure out the mapping myself.

Copy link
Member

Choose a reason for hiding this comment

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

@olasaadi99 , please see if the anonymizer can be fixed as suggested above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kyorav send me an example to test it please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

names handled

Copy link

Choose a reason for hiding this comment

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

I reran on the original object and then manually translated subnetIDs to names. This is what I got:

Subnet connectivity for VPC vpc-054b122b2690e1c3b
workloads2 => workloads1 : protocol: ICMP,UDP
workloads2 => workloads1 : protocol: TCP * 
edge1 => workloads1 : protocol: ICMP,UDP
edge1 => workloads1 : protocol: TCP * 
edge2 => workloads1 : protocol: TCP dst-ports: 443 *

I think there should not be connectivity from edge1 to workloads1. They are both connected to ACL1 and the relevant rule opens up (ingress & egress) only to workloads1. This means that when ACL1 is applied to workloads1 it will block both ingress and egress to edge1.

Also, all of the rules in ACL1 have the protocol set to "All", so there should not be specific protocols on this line.

I suspect additional lines in the report have problems. Unfortunately, this is a rather bad example because of mistakes I made when creating it. I will modify the example to include a series of simple situations instead of this complicated scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you check again please after the fixes @kyorav

@olasaadi99 olasaadi99 requested a review from zivnevo August 7, 2024 12:07
Copy link
Member

@zivnevo zivnevo left a comment

Choose a reason for hiding this comment

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

Just a few small things

@zivnevo
Copy link
Member

zivnevo commented Aug 8, 2024

Also, please run go mod tidy on this branch, and commit go.mod

@olasaadi99 olasaadi99 requested a review from zivnevo August 8, 2024 08:20
@olasaadi99 olasaadi99 merged commit 1df8466 into main Aug 8, 2024
4 checks passed
@olasaadi99 olasaadi99 deleted the 730_nacl branch August 8, 2024 08:31
@olasaadi99 olasaadi99 linked an issue Aug 8, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support nacl analysis
4 participants