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

Potential security-related issues #468

Open
mromascanu123 opened this issue Oct 23, 2024 · 3 comments
Open

Potential security-related issues #468

mromascanu123 opened this issue Oct 23, 2024 · 3 comments

Comments

@mromascanu123
Copy link
Collaborator

mromascanu123 commented Oct 23, 2024

In package 7-Fortigate

The PBR rules implemented steer through the Firewall only the following traffic (prod, nonprod, dev)

  • Pub => App
  • App => Data
    However, all the communication flows except the 2 above seem to be allowed in terms of intra-vnet routing and should not be e.g. App => Pub, Pub => Data, etc

Missing PBR for 0.0.0.0/0 => all traffic to Internet will go via NAT gateway and not inspected by the Firewall

In package (in 3-networks-hub-and-spoke
Because in 3-networks-hub-and-spoke/modules/base_shared_vpc/firewall.tf all intra-VPC traffic allowed, in fact should have allowed only egress via Firewall

This also blocks all inbound traffic from Internet, via Firewall (only intra-VPC traffic can pass) and also blocks any traffic SNAT-ed in the Fortigate rules

Missing PBR for Management and Identity spokes

Hierarchical firewall (in 3-networks-hub-and-spoke / shared) not associated w/ Management and Identity spokes

In 6 - Org Policies
The list-type constraints/compute.restrictVpcPeering does not provide any list nor wildcard and that probably means users in business-units project can freely peer their VPCs allowing traffic to bypass the firewall

@tackaberry
Copy link
Collaborator

We need to keep this repo flexible if users choose to not use 7-fortigate. Only a few organizations will be using it. Some will choose to use GCP NGFW instead. We can't build in dependency in 3-networks-hub-and-spoke to support FG. This will have to be in the downstream user repos. Same applies to egress traffic through the NAT. The default will be to use non-Fortigate approach, and can be modified as needed.

The usage of Management and Identity concepts should be optional. I know that we have it woven in, however, they're not going to be needed by many customers. We should avoid deeper, default integration within the repo with these aspects (eg. PBR for Mgmt and Ident) and implment as needed within the known workload.

Regarding the default state of the org policies, this repo is only intended on being a starting point. Implementations will still need to consider the final state of peering and org policies - amongst many other technical security controls - to ensure it meets requirements. By default, a Network Admin for one network can peer with any other network. This can be restricted based on the needs of the project.

@mromascanu123
Copy link
Collaborator Author

mromascanu123 commented Nov 1, 2024

From what is visible in the repo, configurability and extensibility seems built in via the config folder (including for the management or identity spokes) but some parts of the code seem hardcoded rather than relying on the configuration. IN the configuration everything seems optional and just a matter of adjusting the yaml configs. . Also conceptually, in terms of spoke topology and firewall choice instead of 7-Fortigate maybe this should be called 7-NGFW because Fortigate is very much an NGFW comparable with Palo Alto and routing-wise PBR would still be a must. Ideally and from what I see this code should (and could) be, not just an example but something deployable as-is and providing a minimum viable hub & spoke environment, network security included. Hopefully the user community would be able and willing to give it a try and contribute with necessary missing bits and pieces. From what I see though a glaring miss is meaningful / usable documentation - seems to have been removed from main (why?) but still present in some recent branches

@tackaberry
Copy link
Collaborator

Agreed. When we publish the NGFW folder it will be as 7-ngfw. We have consolidated documentation and we will be publishing more substantial documentation for the repo.

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

No branches or pull requests

2 participants