Improved search for network addresses within a given string (#355)

* Also check suffixes of value for being a network address * e2e test * A few more tests Signed-off-by: ZIV NEVO <[email protected]>
np-guard · Feb 12, 2024 · 48e3b23 · 48e3b23
1 parent 631eb6c
commit 48e3b23
Show file tree

Hide file tree

Showing 4 changed files with 152 additions and 4 deletions.
diff --git a/cmd/nettop/main_test.go b/cmd/nettop/main_test.go
@@ -137,6 +137,15 @@ var (
 			false,
 			[]string{"qotd", "expected_netpol_output.json"},
 		},
+		{
+			"CronJobWithNontrivialNetworkAddresses",
+			[][]string{{"openshift", "openshift-operator-lifecycle-manager-resources.yaml"}},
+			yamlFormat,
+			true,
+			[]string{"-v"},
+			false,
+			[]string{"openshift", "expected_netpol_output.yaml"},
+		},
 		{
 			"SpecifyDNSPort",
 			[][]string{{"acs-security-demos"}},

diff --git a/pkg/analyzer/info_to_resource.go b/pkg/analyzer/info_to_resource.go
@@ -11,6 +11,7 @@ import (
 	"net/url"
 	"strconv"
 	"strings"
+	"unicode"
 
 	ocroutev1 "github.com/openshift/api/route/v1"
 	appsv1 "k8s.io/api/apps/v1"
@@ -218,16 +219,35 @@ func appendNetworkAddresses(networkAddresses, values []string) []string {
 // because it decides if the given string is an evidence for a potentially required connectivity.
 // If it succeeds, a "cleaned" network address is returned as a string, together with the value true.
 // Otherwise (there does not seem to be a network address in "value"), it returns "" with the value false.
+// As value may be in the form of e.g. "key:val" where "val" holds the network address, we will check several possible suffixes.
 func networkAddressFromStr(value string) (string, bool) {
+	suffixes := possibleSuffixes(value)
+	for _, suffix := range suffixes {
+		addr, ok := networkAddressFromSuffix(suffix)
+		if ok {
+			return addr, ok
+		}
+	}
+	return "", false
+}
+
+func networkAddressFromSuffix(value string) (string, bool) {
 	host, err := getHostFromURL(value)
 	if err != nil {
 		return "", false // value cannot be interpreted as a URL
 	}
 
 	hostNoPort := host
 	colonPos := strings.Index(host, ":")
-	if colonPos >= 0 {
+	if colonPos >= 0 { // host includes port number or port name
 		hostNoPort = host[:colonPos]
+		port := host[colonPos+1:] // now validate the port
+		if len(validation.IsValidPortName(port)) > 0 {
+			portInt, _ := strconv.Atoi(port)
+			if len(validation.IsValidPortNum(portInt)) > 0 {
+				return "", false
+			}
+		}
 	}
 
 	errs := validation.IsDNS1123Subdomain(hostNoPort)
@@ -242,6 +262,24 @@ func networkAddressFromStr(value string) (string, bool) {
 	return host, true
 }
 
+// Sometimes the given value includes the network address as its suffix.
+// For example, a command-line arg may look like "server-addr=my_server:5000"
+// If we are unable to convert "value" to a network address, we may also want to check its suffixes.
+// This function returns all suffixes that start with a letter, and have ':', ' ' or '=' just before this initial letter.
+func possibleSuffixes(value string) []string {
+	res := []string{value}
+
+	var prevRune rune
+	for i, r := range value {
+		if i > 1 && unicode.IsLetter(r) && (prevRune == ':' || prevRune == '=' || prevRune == ' ') {
+			res = append(res, value[i:])
+		}
+		prevRune = r
+	}
+
+	return res
+}
+
 // Attempts to parse the given string as a URL, and extract its Host part.
 // Returns an error if the string cannot be interpreted as a URL
 func getHostFromURL(urlStr string) (string, error) {

diff --git a/pkg/analyzer/info_to_resource_test.go b/pkg/analyzer/info_to_resource_test.go
@@ -27,6 +27,9 @@ func TestNetworkAddressValue(t *testing.T) {
 		strings.Repeat("abc", 500):      {"", false},
 		"not%a*url":                     {"", false},
 		"123":                           {"", false},
+		"olm-operator-heap-:https://olm-operator-metrics:8443/debug/pprof/heap": {"olm-operator-metrics:8443", true},
+		"-server=my-server:5024":  {"my-server:5024", true},
+		"-server=my-server:502%4": {"", false}, // port number is invalid
 	}
 
 	for val, expectedAnswer := range valuesToCheck {
@@ -53,8 +56,9 @@ func TestScanningDeploymentWithArgs(t *testing.T) {
 	res, err := k8sWorkloadObjectFromInfo(resourceInfo)
 	require.Nil(t, err)
 	require.Equal(t, "carts", res.Resource.Name)
-	require.Len(t, res.Resource.NetworkAddrs, 1)
-	require.Equal(t, "carts-db:27017", res.Resource.NetworkAddrs[0])
+	require.Len(t, res.Resource.NetworkAddrs, 2)
+	require.Equal(t, "false", res.Resource.NetworkAddrs[0])
+	require.Equal(t, "carts-db:27017", res.Resource.NetworkAddrs[1])
 	require.Len(t, res.Resource.Labels, 1)
 	require.Equal(t, "carts", res.Resource.Labels["name"])
 }
@@ -124,7 +128,7 @@ func TestScanningCronJob(t *testing.T) {
 	require.Nil(t, err)
 	require.Equal(t, "collect-profiles", res.Resource.Name)
 	require.Equal(t, cronJob, res.Resource.Kind)
-	require.Len(t, res.Resource.NetworkAddrs, 1)
+	require.Len(t, res.Resource.NetworkAddrs, 3)
 	require.Len(t, res.Resource.Labels, 0)
 }
 

diff --git a/tests/openshift/expected_netpol_output.yaml b/tests/openshift/expected_netpol_output.yaml
@@ -0,0 +1,97 @@
+apiVersion: networking.k8s.io/v1
+items:
+    - apiVersion: networking.k8s.io/v1
+      kind: NetworkPolicy
+      metadata:
+        creationTimestamp: null
+        name: catalog-operator-netpol
+        namespace: openshift-operator-lifecycle-manager
+      spec:
+        ingress:
+            - from:
+                - podSelector: {}
+              ports:
+                - port: 8443
+                  protocol: TCP
+        podSelector:
+            matchLabels:
+                app: catalog-operator
+        policyTypes:
+            - Ingress
+            - Egress
+    - apiVersion: networking.k8s.io/v1
+      kind: NetworkPolicy
+      metadata:
+        creationTimestamp: null
+        name: collect-profiles-netpol
+        namespace: openshift-operator-lifecycle-manager
+      spec:
+        egress:
+            - ports:
+                - port: 8443
+                  protocol: TCP
+              to:
+                - podSelector:
+                    matchLabels:
+                        app: catalog-operator
+            - ports:
+                - port: 8443
+                  protocol: TCP
+              to:
+                - podSelector:
+                    matchLabels:
+                        app: olm-operator
+            - ports:
+                - port: 53
+                  protocol: UDP
+              to:
+                - namespaceSelector: {}
+        podSelector: {}
+        policyTypes:
+            - Ingress
+            - Egress
+    - apiVersion: networking.k8s.io/v1
+      kind: NetworkPolicy
+      metadata:
+        creationTimestamp: null
+        name: olm-operator-netpol
+        namespace: openshift-operator-lifecycle-manager
+      spec:
+        ingress:
+            - from:
+                - podSelector: {}
+              ports:
+                - port: 8443
+                  protocol: TCP
+        podSelector:
+            matchLabels:
+                app: olm-operator
+        policyTypes:
+            - Ingress
+            - Egress
+    - apiVersion: networking.k8s.io/v1
+      kind: NetworkPolicy
+      metadata:
+        creationTimestamp: null
+        name: packageserver-netpol
+        namespace: openshift-operator-lifecycle-manager
+      spec:
+        podSelector:
+            matchLabels:
+                app: packageserver
+        policyTypes:
+            - Ingress
+            - Egress
+    - apiVersion: networking.k8s.io/v1
+      kind: NetworkPolicy
+      metadata:
+        creationTimestamp: null
+        name: default-deny-in-namespace-openshift-operator-lifecycle-manager
+        namespace: openshift-operator-lifecycle-manager
+      spec:
+        podSelector: {}
+        policyTypes:
+            - Ingress
+            - Egress
+kind: NetworkPolicyList
+metadata: {}