np-guard / vpc-network-config-analyzer

A tool for analyzing the configured network connectivity of VPCs as specified by various VPC resources
Apache License 2.0
6 stars 0 forks source link

Implement Linting #116

Open kyorav opened 11 months ago

kyorav commented 11 months ago

This issue is for brainstorming ideas for checks we may include as part of a linting functionality. At some point we will review the list with a stakeholder and decide which ones make sense.

ShiriMoran commented 1 month ago

This issue is for brainstorming ideas for checks we may include as part of a linting functionality. At some point we will review the list with a stakeholder and decide which ones make sense.

...

* LB analysis: warn about SG that split subnet of a configured LB (without running full connectivity analysis)

Only SG are relevant in this regard? what about s.g. prefix filters? Perhaps rephrase:

  • warn when different (random) selections of private IPs may yield different connectivity results for LB
kyorav commented 1 month ago

warn when different (random) selections of private IPs may yield different connectivity results for LB

This is not linting, you need to apply the complete analysis to figure this out. And yes, our analysis will highlight this.

zivnevo commented 1 month ago

warn when different (random) selections of private IPs may yield different connectivity results for LB

This is not linting, you need to apply the complete analysis to figure this out. And yes, our analysis will highlight this.

I'm not sure we want to limit linting to just shallow analysis. Wikipedia says:

Lint is the computer science term for a static code analysis tool used to flag programming errors, bugs, stylistic errors and suspicious constructs.

so I think I would like to include everything suspicious we can find.

kyorav commented 1 month ago

so I think I would like to include everything suspicious we can find

What I mean is that our analysis functionality checks this (by inserting "fake" LB endpoints in all subnets and notifying the user when they don't all have the same connectivity) so I don't think we need to add it to the linting functionality. Or, at least, we don't need to implement it as part of this issue because it is already implemented. Unless you somehow want to implement it differently for the linting functionality?

zivnevo commented 1 month ago

It makes sense for both the analysis (vpcanalyzer report) and the linter (vpcanalyzer lint) to notify the user about such issues, as these subcommands are used for different use-cases. We should make sure the code for this check is not duplicated.

ShiriMoran commented 2 days ago
* nACL splits a subnet. When a cidr that does not cover the entire subnet is used in the source of an outbound rule or the destination of an inbound rule.

What about (local/remote cidr blocks splitting subnets in rules of) SG? @kyorav

kyorav commented 2 days ago

What about (local/remote cidr blocks splitting subnets in rules of) SG

This is a weaker indication of error, but can still fit as a linting rule. A common correct pattern is to have a security group rule that allows access to the address of a load balancer, DNS, proxy, etc. So if the cidr size is a single address ("/32") we probably don't want to flag it, so as not to create too much noise.