treat empty NetworkPolicyPort as "all ports on TCP" during parsing

[mattfenwick]

Fixes: #14678

Treat empty NetworkPolicyPort as "all ports on TCP" during network policy parsing

Test plan

cd cilium

pushd pkg/k8s && go test
popd

pushd pkg/policy && go test
popd

pushd pkg/policy/api && go test
popd

[maintainer-s-little-helper]

Commit d7bd86fe0b03a390feec04bbf1ab44e2a908b045 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[mattfenwick]

👋 wanted to put this up for preliminary feedback even though it's failing some tests!

I think I'm running into this at pkg/policy/api/rule_validation.go:388:

func (pp *PortProtocol) Sanitize() error {
	if pp.Port == "" {
		return fmt.Errorf("Port must be specified")
	}

	// Port names are formatted as IANA Service Names.  This means that
	// some legal numeric literals are no longer considered numbers, e.g,
	// 0x10 is now considered a name rather than number 16.
	if iana.IsSvcName(pp.Port) {
		pp.Port = strings.ToLower(pp.Port) // Normalize for case insensitive comparison
	} else {
		p, err := strconv.ParseUint(pp.Port, 0, 16)
		if err != nil {
			return fmt.Errorf("Unable to parse port: %s", err)
		}
		if p == 0 {
			return fmt.Errorf("Port cannot be 0")
		}
	}

	var err error
	pp.Protocol, err = ParseL4Proto(string(pp.Protocol))
	if err != nil {
		return err
	}

	return nil
}

What's the best way to handle that? Making this change:

--- a/pkg/policy/api/rule_validation.go
+++ b/pkg/policy/api/rule_validation.go
@@ -380,13 +380,10 @@ func (pp *PortProtocol) Sanitize() error {
        if iana.IsSvcName(pp.Port) {
                pp.Port = strings.ToLower(pp.Port) // Normalize for case insensitive comparison
        } else {
-               p, err := strconv.ParseUint(pp.Port, 0, 16)
+               _, err := strconv.ParseUint(pp.Port, 0, 16)
                if err != nil {
                        return fmt.Errorf("Unable to parse port: %s", err)
                }
-               if p == 0 {
-                       return fmt.Errorf("Port cannot be 0")
-               }
        }

gets the tests to pass, but not sure if that would have other bad downstream consequences?

[aanm]

@jrajahalme do you mind taking a look. The port number is optional in a k8s network policy so we it can be 0. To me the proposed fixed by @mattfenwick makes sense.

[jrajahalme]

gets the tests to pass, but not sure if that would have other bad downstream consequences?

Removing the zero test seems ok, my only concern is that L7 policies can only be applied if a port number has been specified. Would be good to add a test case with a port with default values and make sure it is rejected. Policy could be like this:

...
    toPorts:
    - ports:
      - {}
      rules:
        http:
        - method: "POST"
...

To be on the safe side this should be rejected by rule validation, maybe like this:

	// Sanitize L7 rules
	if !pr.Rules.IsEmpty() {
+		// Check if any of the ports was zero ("0"), maybe via a return value from 'pr.Ports[i].Sanitize()'
+		If haveZeroPort {
+			return fmt.Errorf("Port cannot be 0")
+		}
		if err := pr.Rules.sanitize(pr.Ports); err != nil {
			return err
		}
	}

[joestringer]

Marking as "draft" while feedback is addressed. If you would like clarification on the feedback, feel free to respond and ping the person who provided the feedback 🙂 When you would like additional review, scroll to the bottom of the page and click the "Ready for review" button.

[mattfenwick]

Marking as "draft" while feedback is addressed. If you would like clarification on the feedback, feel free to respond and ping the person who provided the feedback 🙂 When you would like additional review, scroll to the bottom of the page and click the "Ready for review" button.

Thanks for the reminder! 😄

[mattfenwick]

gets the tests to pass, but not sure if that would have other bad downstream consequences?

Removing the zero test seems ok, my only concern is that L7 policies can only be applied if a port number has been specified. Would be good to add a test case with a port with default values and make sure it is rejected. Policy could be like this:

...
    toPorts:
    - ports:
      - {}
      rules:
        http:
        - method: "POST"
...

To be on the safe side this should be rejected by rule validation, maybe like this:

	// Sanitize L7 rules
	if !pr.Rules.IsEmpty() {
+		// Check if any of the ports was zero ("0"), maybe via a return value from 'pr.Ports[i].Sanitize()'
+		If haveZeroPort {
+			return fmt.Errorf("Port cannot be 0")
+		}
		if err := pr.Rules.sanitize(pr.Ports); err != nil {
			return err
		}
	}

thanks for the tip @jrajahalme !

I pushed an update to add an L7, but there's still something not quite right and one of the tests in the pkg/policy/api package is now failing.

I think the problem is I'm not separating the code paths for validation for L3/L4 from L7, can you take a look?

[jrajahalme]

The change I proposed above was a bit too brittle, as it would allow port values such as "00" pass the new check. It's better to do by tracking the parsed zero values in PortProtocol validation and return a boolean telling if the port was zero:

diff --git a/pkg/policy/api/rule_validation.go b/pkg/policy/api/rule_validation.go
index 695de01559..d4c8778efb 100644
--- a/pkg/policy/api/rule_validation.go
+++ b/pkg/policy/api/rule_validation.go
@@ -338,14 +338,20 @@ func (pr *L7Rules) sanitize(ports []PortProtocol) error {
 }
 
 func (pr *PortRule) sanitize(ingress bool) error {
+	haveZeroPort := false
+
 	if len(pr.Ports) > maxPorts {
 		return fmt.Errorf("too many ports, the max is %d", maxPorts)
 	}
 	for i := range pr.Ports {
-		if err := pr.Ports[i].Sanitize(); err != nil {
+		var isZero bool
+		var err error
+		if isZero, err = pr.Ports[i].sanitize(); err != nil {
 			return err
 		}
-
+		if isZero {
+			haveZeroPort = true
+		}
 		hasDNSRules := pr.Rules != nil && len(pr.Rules.DNS) > 0
 		// DNS L7 rules can be TCP, UDP or ANY, all others are TCP only.
 		switch {
@@ -362,11 +368,10 @@ func (pr *PortRule) sanitize(ingress bool) error {
 
 	// Sanitize L7 rules
 	if !pr.Rules.IsEmpty() {
-		for _, pp := range pr.Ports {
-			if err := pp.Sanitize(); err != nil {
-				return err
-			}
+		if haveZeroPort {
+			return fmt.Errorf("L7 rules can not be used when a port is 0")
 		}
+
 		if err := pr.Rules.sanitize(pr.Ports); err != nil {
 			return err
 		}
@@ -374,9 +379,9 @@ func (pr *PortRule) sanitize(ingress bool) error {
 	return nil
 }
 
-func (pp *PortProtocol) Sanitize() error {
+func (pp *PortProtocol) sanitize() (isZero bool, err error) {
 	if pp.Port == "" {
-		return fmt.Errorf("Port must be specified")
+		return isZero, fmt.Errorf("Port must be specified")
 	}
 
 	// Port names are formatted as IANA Service Names.  This means that
@@ -385,19 +390,15 @@ func (pp *PortProtocol) Sanitize() error {
 	if iana.IsSvcName(pp.Port) {
 		pp.Port = strings.ToLower(pp.Port) // Normalize for case insensitive comparison
 	} else {
-		_, err := strconv.ParseUint(pp.Port, 0, 16)
+		p, err := strconv.ParseUint(pp.Port, 0, 16)
 		if err != nil {
-			return fmt.Errorf("Unable to parse port: %s", err)
+			return isZero, fmt.Errorf("Unable to parse port: %s", err)
 		}
+		isZero = p == 0
 	}
 
-	var err error
 	pp.Protocol, err = ParseL4Proto(string(pp.Protocol))
-	if err != nil {
-		return err
-	}
-
-	return nil
+	return isZero, err
 }
 
 // sanitize the given CIDR. If successful, returns the prefixLength specified

[jrajahalme]

@mattfenwick Sorry for the churn, I was kind of hoping I could have sent the latter comment above before you do any additional work on this! Anyway, there is a new diff w.r.t to your PR before the changes I suggested a bit earlier.

[mattfenwick]

@mattfenwick Sorry for the churn, I was kind of hoping I could have sent the latter comment above before you do any additional work on this! Anyway, there is a new diff w.r.t to your PR before the changes I suggested a bit earlier.

np 😄 applied the diff and updated!

[twpayne]

test-me-please

[twpayne]

The runtime tests all failed, which looks like a flake: https://jenkins.cilium.io/job/Cilium-PR-Runtime-4.9/4480/

I'll re-trigger the tests.

[twpayne]

retest-runtime

[twpayne]

test-runtime

[aanm]

test-1.21-4.9 (previous failure https://jenkins.cilium.io/job/Cilium-PR-K8s-1.21-kernel-4.9/271/)

[twpayne]

LGTM, thanks so much for the fix :)

[christarazi]

Merging as we have approval and CI is all green.