116 lint intrf+first (#675)

lint infrastructure and first lint checker

Author: ShiriMoran

cmd/analyzer/subcmds/lint.go

@@ -0,0 +1,38 @@
+/*
+Copyright 2023- IBM Inc. All Rights Reserved.
+
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package subcmds
+
+import (
+	"github.com/spf13/cobra"
+
+	"github.com/np-guard/vpc-network-config-analyzer/pkg/linter"
+)
+
+func NewLintCommand(args *inArgs) *cobra.Command {
+	cmd := &cobra.Command{
+		Use:   "lint",
+		Short: "Run various checks for ensuring best-practices",
+		Long:  `Execute various (configurable) linting and provides findings`,
+		Args:  cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			return lintVPCConfigs(cmd, args)
+		},
+	}
+	return cmd
+}
+
+func lintVPCConfigs(cmd *cobra.Command, inArgs *inArgs) error {
+	cmd.SilenceUsage = true  // if we got this far, flags are syntactically correct, so no need to print usage
+	cmd.SilenceErrors = true // also, error will be printed to logger in main(), so no need for cobra to also print it
+
+	multiConfigs, err1 := buildConfigs(inArgs)
+	if err1 != nil {
+		return err1
+	}
+	_, _, err2 := linter.LinterExecute(multiConfigs.Configs())
+	return err2
+}

cmd/analyzer/subcmds/root.go

@@ -110,6 +110,7 @@ func NewRootCommand() *cobra.Command {
 	rootCmd.AddCommand(NewReportCommand(args))
 	rootCmd.AddCommand(NewDiffCommand(args))
 	rootCmd.AddCommand(NewExplainCommand(args))
+	rootCmd.AddCommand(NewLintCommand(args))
 	rootCmd.CompletionOptions.HiddenDefaultCmd = true
 	rootCmd.SetHelpCommand(&cobra.Command{Hidden: true}) // disable help command. should use --help flag instead
 

pkg/ibmvpc/analysis_output_test.go

@@ -910,9 +910,8 @@ func compareOrRegenerateOutputPerTest(t *testing.T,
 			compareTextualResult(expectedOutputStr, actualOutput)
 			t.Fatalf("output mismatch expected-vs-actual on test name: %s, use case: %d", tt.name, uc)
 		}
-	}
-
-	if mode == outputGeneration {
+	} else if mode == outputGeneration {
+		fmt.Printf("outputGeneration\n")
 		// create or override expected output file
 		if _, err := vpcmodel.WriteToFile(actualOutput, tt.expectedOutput[uc]); err != nil {
 			return err

pkg/ibmvpc/examples/out/lint_out/basic_acl3_Lint

@@ -0,0 +1,4 @@
+"Firewall rules implying different connectivity for different endpoints within a subnet" issues:
+------------------------------------------------------------------------------------------------
+In VPC test-vpc1-ky, network ACL acl3-ky rule's indexed 1 splits subnet subnet3-ky (10.240.30.0/24). Splitting rule details: index: 1, direction: outbound , src: 10.240.30.0/31 , dst: 10.240.20.0/24, conn: all, action: allow
+In VPC test-vpc1-ky, network ACL acl3-ky rule's indexed 3 splits subnet subnet3-ky (10.240.30.0/24). Splitting rule details: index: 3, direction: inbound , src: 10.240.20.0/24 , dst: 10.240.30.0/31, conn: all, action: allow

pkg/ibmvpc/examples/out/lint_out/basic_sg1_Lint

@@ -0,0 +1,8 @@
+"Firewall rules implying different connectivity for different endpoints within a subnet" issues:
+------------------------------------------------------------------------------------------------
+In VPC test-vpc1-ky, security group sg1-ky rule's indexed 1 splits subnet subnet1-ky (10.240.10.0/24). Splitting rule details: index: 1, direction: inbound,  conns: protocol: all, remote: sg1-ky (10.240.10.4/32), local: 0.0.0.0/0
+In VPC test-vpc1-ky, security group sg1-ky rule's indexed 3 splits subnets subnet2-ky (10.240.20.0/24), subnet3-ky (10.240.30.0/24). Splitting rule details: index: 3, direction: inbound,  conns: protocol: all, remote: sg2-ky (10.240.20.4/32,10.240.30.4/32), local: 0.0.0.0/0
+In VPC test-vpc1-ky, security group sg1-ky rule's indexed 4 splits subnet subnet3-ky (10.240.30.0/24). Splitting rule details: index: 4, direction: inbound,  conns: protocol: all, remote: sg3-ky (10.240.30.5/32,10.240.30.6/32), local: 0.0.0.0/0
+In VPC test-vpc1-ky, security group sg2-ky rule's indexed 4 splits subnet subnet1-ky (10.240.10.0/24). Splitting rule details: index: 4, direction: inbound,  conns: protocol: all, remote: sg1-ky (10.240.10.4/32), local: 0.0.0.0/0
+In VPC test-vpc1-ky, security group sg2-ky rule's indexed 6 splits subnets subnet2-ky (10.240.20.0/24), subnet3-ky (10.240.30.0/24). Splitting rule details: index: 6, direction: outbound,  conns: protocol: tcp,  dstPorts: 1-65535, remote: sg2-ky (10.240.20.4/32,10.240.30.4/32), local: 0.0.0.0/0
+In VPC test-vpc1-ky, security group sg2-ky rule's indexed 7 splits subnets subnet2-ky (10.240.20.0/24), subnet3-ky (10.240.30.0/24). Splitting rule details: index: 7, direction: inbound,  conns: protocol: tcp,  dstPorts: 1-65535, remote: sg2-ky (10.240.20.4/32,10.240.30.4/32), local: 0.0.0.0/0

pkg/ibmvpc/examples/out/lint_out/multivpc_Lint

@@ -0,0 +1 @@
+no lint "Firewall rules implying different connectivity for different endpoints within a subnet" issues

pkg/ibmvpc/lint_test.go

@@ -0,0 +1,110 @@
+/*
+Copyright 2023- IBM Inc. All Rights Reserved.
+
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package ibmvpc
+
+import (
+	"fmt"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/np-guard/vpc-network-config-analyzer/pkg/linter"
+	"github.com/np-guard/vpc-network-config-analyzer/pkg/vpcmodel"
+)
+
+const lintOut = "lint_out"
+
+var lintTests = []*vpcGeneralTest{
+	{
+		name:        "basic_acl3",
+		inputConfig: "acl_testing3",
+	},
+	{
+		name:        "basic_sg1",
+		inputConfig: "sg_testing1_new",
+	},
+	{
+		name:        "multivpc",
+		inputConfig: "tgw_larger_example",
+	},
+}
+
+func TestAllLint(t *testing.T) {
+	// lintTests is the list of tests to run
+	for testIdx := range lintTests {
+		tt := lintTests[testIdx]
+		tt.mode = outputComparison
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			tt.runLintTest(t)
+		})
+	}
+	fmt.Println("done")
+}
+
+// uncomment the function below for generating the expected output files instead of comparing
+/*
+func TestAllLintWithGeneration(t *testing.T) {
+	// tests is the list of tests to run
+	for testIdx := range lintTests {
+		tt := lintTests[testIdx]
+		tt.mode = outputGeneration
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			tt.runLintTest(t)
+		})
+	}
+	fmt.Println("done")
+}
+*/
+func (tt *vpcGeneralTest) runLintTest(t *testing.T) {
+	// all tests in lint mode
+	// output use case is not significant here, but being used so that lint test can rely on existing mechanism
+	tt.useCases = []vpcmodel.OutputUseCase{vpcmodel.AllEndpoints}
+	// init test - set the input/output file names according to test name
+	tt.initTest()
+
+	// get vpcConfigs obj from parsing + analyzing input config file
+	vpcConfigs := getVPCConfigs(t, tt, true)
+
+	// generate actual output for all use cases specified for this test
+	err := runLintTestPerUseCase(t, tt, vpcConfigs.Configs(), lintOut)
+	require.Equal(t, tt.errPerUseCase[vpcmodel.AllEndpoints], err, "comparing actual err to expected err")
+	for uc, outFile := range tt.actualOutput {
+		fmt.Printf("test %s use-case %d - generated output file: %s\n", tt.name, uc, outFile)
+	}
+}
+
+// runExplainTestPerUseCase executes lint for the required use case and compares/generates the output
+func runLintTestPerUseCase(t *testing.T,
+	tt *vpcGeneralTest,
+	cConfigs map[string]*vpcmodel.VPCConfig,
+	outDir string) error {
+	// output use case is not significant here, but being used so that lint test can rely on existing mechanism
+	initLintTestFileNames(tt, outDir)
+	_, actualOutput, _ := linter.LinterExecute(cConfigs)
+	if err := compareOrRegenerateOutputPerTest(t, tt.mode, actualOutput, tt, vpcmodel.AllEndpoints); err != nil {
+		return err
+	}
+	return nil
+}
+
+func initLintTestFileNames(tt *vpcGeneralTest, testDir string) {
+	expectedFileName, actualFileName := getLintTestFileName(tt.name)
+	// output use case is not significant here, but being used so that lint test can rely on existing mechanism
+	tt.actualOutput[vpcmodel.AllEndpoints] = filepath.Join(getTestsDirOut(testDir), actualFileName)
+	tt.expectedOutput[vpcmodel.AllEndpoints] = filepath.Join(getTestsDirOut(testDir), expectedFileName)
+}
+
+// getLintTestFileName returns expected file name and actual file name, for the relevant use case
+func getLintTestFileName(testName string) (expectedFileName, actualFileName string) {
+	res := testName + "_Lint"
+	expectedFileName = res
+	actualFileName = actualOutFilePrefix + res
+	return expectedFileName, actualFileName
+}

pkg/ibmvpc/vpc.go

@@ -21,6 +21,10 @@ import (
 )
 
 const doubleTab = "\t\t"
+const emptyNameError = "empty name for %s indexed %d"
+
+const securityGroup = "security group"
+const networkACL = "network ACL"
 
 ///////////////////////////////////////////////////////////////////////////////////////////////////
 
@@ -477,6 +481,25 @@ func (nl *NaclLayer) ReferencedIPblocks() []*ipblock.IPBlock {
 	return res
 }
 
+func (nl *NaclLayer) GetRules() ([]vpcmodel.RuleOfFilter, error) {
+	resRules := []vpcmodel.RuleOfFilter{}
+	for naclIndx, nacl := range nl.naclList {
+		naclRules := nacl.analyzer.egressRules
+		naclRules = append(naclRules, nacl.analyzer.ingressRules...)
+		if nacl.analyzer.naclResource.Name == nil {
+			return nil, fmt.Errorf(emptyNameError, networkACL, naclIndx)
+		}
+		naclName := *nacl.analyzer.naclResource.Name
+		for _, rule := range naclRules {
+			ruleBlocks := []*ipblock.IPBlock{rule.src, rule.dst}
+			ruleDesc, _, _, _ := nacl.analyzer.getNACLRule(rule.index)
+			resRules = append(resRules, *vpcmodel.NewRuleOfFilter(networkACL, naclName, ruleDesc, rule.index,
+				ruleBlocks))
+		}
+	}
+	return resRules, nil
+}
+
 func getHeaderRulesType(filter string, rType vpcmodel.RulesType) string {
 	switch rType {
 	case vpcmodel.NoRules:
@@ -679,6 +702,28 @@ func (sgl *SecurityGroupLayer) ReferencedIPblocks() []*ipblock.IPBlock {
 	return res
 }
 
+func (sgl *SecurityGroupLayer) GetRules() ([]vpcmodel.RuleOfFilter, error) {
+	resRules := []vpcmodel.RuleOfFilter{}
+	for sgIndx, sg := range sgl.sgList {
+		sgRules := sg.analyzer.egressRules
+		sgRules = append(sgRules, sg.analyzer.ingressRules...)
+		if sg.analyzer.sgResource.Name == nil {
+			return nil, fmt.Errorf(emptyNameError, securityGroup, sgIndx)
+		}
+		sgName := *sg.analyzer.sgResource.Name
+		for _, rule := range sgRules {
+			ruleBlocks := []*ipblock.IPBlock{rule.remote.cidr}
+			if rule.local != nil {
+				ruleBlocks = append(ruleBlocks, rule.local)
+			}
+			ruleDesc, _, _, _ := sg.analyzer.getSGRule(rule.index)
+			resRules = append(resRules, *vpcmodel.NewRuleOfFilter(securityGroup, sgName, ruleDesc, rule.index,
+				ruleBlocks))
+		}
+	}
+	return resRules, nil
+}
+
 type SecurityGroup struct {
 	vpcmodel.VPCResource
 	analyzer *SGAnalyzer

pkg/linter/lintFilterRuleSplitSubnet.go

@@ -0,0 +1,128 @@
+/*
+Copyright 2023- IBM Inc. All Rights Reserved.
+
+SPDX-License-Identifier: Apache-2.0
+*/
+
+package linter
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/np-guard/models/pkg/ipblock"
+	"github.com/np-guard/vpc-network-config-analyzer/pkg/vpcmodel"
+)
+
+const splitRuleSubnetName = "rules-splitting-subnets"
+
+// filterRuleSplitSubnet: rules of filters that are inconsistent w.r.t. subnets.
+type filterRuleSplitSubnet struct {
+	basicLinter
+}
+
+// a rule with the list of subnets it splits
+type splitRuleSubnet struct {
+	vpcName      string
+	rule         vpcmodel.RuleOfFilter
+	splitSubnets []vpcmodel.Subnet
+}
+
+// /////////////////////////////////////////////////////////
+// lint interface implementation for filterRuleSplitSubnet
+// ////////////////////////////////////////////////////////
+func (lint *filterRuleSplitSubnet) lintName() string {
+	return splitRuleSubnetName
+}
+
+func (lint *filterRuleSplitSubnet) lintDescription() string {
+	return "Firewall rules implying different connectivity for different endpoints within a subnet"
+}
+
+func (lint *filterRuleSplitSubnet) check() error {
+	for _, config := range lint.configs {
+		if config.IsMultipleVPCsConfig {
+			continue // no use in executing lint on dummy vpcs
+		}
+		for _, layer := range vpcmodel.FilterLayers {
+			filterLayer := config.GetFilterTrafficResourceOfKind(layer)
+			rules, err := filterLayer.GetRules()
+			if err != nil {
+				return err
+			}
+			for _, rule := range rules {
+				subnetsSplitByRule := []vpcmodel.Subnet{}
+				for _, subnet := range config.Subnets {
+					splitSubnet := ruleSplitSubnet(subnet, rule.IPBlocks)
+					if splitSubnet {
+						subnetsSplitByRule = append(subnetsSplitByRule, subnet)
+					}
+				}
+				if len(subnetsSplitByRule) > 0 {
+					lint.addFinding(&splitRuleSubnet{vpcName: config.VPC.Name(), rule: rule,
+						splitSubnets: subnetsSplitByRule})
+				}
+			}
+		}
+	}
+	return nil
+}
+
+// given a subnet and IPBlocks mentioned in a rule, returns the list
+func ruleSplitSubnet(subnet vpcmodel.Subnet, ruleIPBlocks []*ipblock.IPBlock) bool {
+	subnetCidrIPBlock := subnet.AddressRange()
+	for _, ruleIPBlock := range ruleIPBlocks {
+		if ruleIPBlock.Overlap(subnetCidrIPBlock) && !subnetCidrIPBlock.ContainedIn(ruleIPBlock) {
+			return true
+		}
+	}
+	return false
+}
+
+///////////////////////////////////////////////////////////
+// finding interface implementation for splitRuleSubnet
+//////////////////////////////////////////////////////////
+
+func (finding *splitRuleSubnet) vpc() string {
+	return finding.vpcName
+}
+
+func (finding *splitRuleSubnet) string() string {
+	rule := finding.rule
+	subnetsStrSlice := make([]string, len(finding.splitSubnets))
+	for i, subnet := range finding.splitSubnets {
+		subnetsStrSlice[i] = fmt.Sprintf("%s (%s)", subnet.Name(), subnet.CIDR())
+	}
+	subnetStr := strings.Join(subnetsStrSlice, ", ")
+	if len(subnetsStrSlice) > 1 {
+		subnetStr = "subnets " + subnetStr
+	} else {
+		subnetStr = "subnet " + subnetStr
+	}
+	return fmt.Sprintf("In VPC %s, %s %s rule's indexed %d splits %s. Splitting rule details: %s",
+		finding.vpc(), finding.rule.LayerName, rule.FilterName, rule.RuleIndex, subnetStr, rule.RuleDesc)
+}
+
+// for json: a rule with the list of subnets it splits
+type splitRuleSubnetJSON struct {
+	VpcName      string                `json:"vpc_name"`
+	Rule         vpcmodel.RuleOfFilter `json:"rule_details"`
+	SplitSubnets []subnetJSON          `json:"splitted_subnets"`
+}
+
+type subnetJSON struct {
+	Name string `json:"name"`
+	CIDR string `json:"cidr"`
+}
+
+func (finding *splitRuleSubnet) toJSON() any {
+	rule := finding.rule
+	splitSubnetsJSON := make([]subnetJSON, len(finding.splitSubnets))
+	for i, splitSubnet := range finding.splitSubnets {
+		splitSubnetsJSON[i] = subnetJSON{Name: splitSubnet.Name(), CIDR: splitSubnet.CIDR()}
+	}
+	res := splitRuleSubnetJSON{VpcName: finding.vpcName, Rule: vpcmodel.RuleOfFilter{LayerName: rule.LayerName,
+		FilterName: rule.FilterName, RuleIndex: rule.RuleIndex, RuleDesc: rule.RuleDesc},
+		SplitSubnets: splitSubnetsJSON}
+	return res
+}