Validations and Fixes (#68)

api/v1/clusterwidenetworkpolicy_types.go

@@ -17,8 +17,14 @@ limitations under the License.
 package v1
 
 import (
+	"fmt"
+	"net"
+
+	"github.com/hashicorp/go-multierror"
+	corev1 "k8s.io/api/core/v1"
 	networking "k8s.io/api/networking/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
 )
 
 // ClusterwideNetworkPolicy contains the desired state for a cluster wide network policy to be applied.
@@ -101,6 +107,67 @@ type EgressRule struct {
 	To []networking.IPBlock `json:"to,omitempty"`
 }
 
+// Validate validates the spec of a ClusterwideNetworkPolicy
+func (p *PolicySpec) Validate() error {
+	var errors *multierror.Error
+	for _, e := range p.Egress {
+		errors = multierror.Append(errors, validatePorts(e.Ports), validateIPBlocks(e.To))
+	}
+	for _, i := range p.Ingress {
+		errors = multierror.Append(errors, validatePorts(i.Ports), validateIPBlocks(i.From))
+	}
+
+	return errors.ErrorOrNil()
+}
+
+func validatePorts(ports []networking.NetworkPolicyPort) *multierror.Error {
+	var errors *multierror.Error
+	for _, p := range ports {
+		if p.Port != nil && p.Port.Type != intstr.Int {
+			errors = multierror.Append(errors, fmt.Errorf("only int ports are supported, but %v given", p.Port))
+		}
+
+		if p.Protocol != nil {
+			proto := *p.Protocol
+			if proto != corev1.ProtocolUDP && proto != corev1.ProtocolTCP {
+				errors = multierror.Append(errors, fmt.Errorf("only TCP and UDP are supported as protocol, but %v given", proto))
+			}
+		}
+	}
+	return errors
+}
+
+func validateIPBlocks(blocks []networking.IPBlock) *multierror.Error {
+	var errors *multierror.Error
+	for _, b := range blocks {
+		_, blockNet, err := net.ParseCIDR(b.CIDR)
+		if err != nil {
+			errors = multierror.Append(errors, fmt.Errorf("%v is not a valid IP CIDR", b.CIDR))
+			continue
+		}
+
+		for _, e := range b.Except {
+			exceptIP, exceptNet, err := net.ParseCIDR(b.CIDR)
+			if err != nil {
+				errors = multierror.Append(errors, fmt.Errorf("%v is not a valid IP CIDR", e))
+				continue
+			}
+
+			if !blockNet.Contains(exceptIP) {
+				errors = multierror.Append(errors, fmt.Errorf("%v is not contained in the IP CIDR %v", exceptIP, blockNet))
+				continue
+			}
+
+			blockSize, _ := blockNet.Mask.Size()
+			exceptSize, _ := exceptNet.Mask.Size()
+			if exceptSize > blockSize {
+				errors = multierror.Append(errors, fmt.Errorf("netmask size of network to be excluded must be smaller than netmask of the block CIDR"))
+			}
+		}
+	}
+	return errors
+}
+
 func init() {
 	SchemeBuilder.Register(&ClusterwideNetworkPolicy{}, &ClusterwideNetworkPolicyList{})
 }

api/v1/clusterwidenetworkpolicy_types_test.go

@@ -0,0 +1,106 @@
+/*
+
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1
+
+import (
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	networking "k8s.io/api/networking/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
+)
+
+func TestPolicySpec_Validate(t *testing.T) {
+	tcp := corev1.ProtocolTCP
+	udp := corev1.ProtocolUDP
+	port1 := intstr.FromInt(8080)
+	port2 := intstr.FromInt(8081)
+	invalid := intstr.FromString("invalid")
+	tests := []struct {
+		name    string
+		Ingress []IngressRule
+		Egress  []EgressRule
+		wantErr bool
+	}{
+		{
+			name: "simple test",
+			Ingress: []IngressRule{
+				{
+					From: []networking.IPBlock{
+						{
+							CIDR:   "1.1.0.0/16",
+							Except: []string{"1.1.1.0/24"},
+						},
+						{
+							CIDR:   "192.168.0.1/32",
+							Except: []string{"192.168.0.1/32"},
+						},
+					},
+					Ports: []networking.NetworkPolicyPort{
+						{
+							Protocol: nil,
+							Port:     &port1,
+						},
+						{
+							Protocol: &tcp,
+							Port:     &port2,
+						},
+						{
+							Protocol: &udp,
+							Port:     &port2,
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "invalid test",
+			Ingress: []IngressRule{
+				{
+					From: []networking.IPBlock{
+						{
+							CIDR:   "1.1.0.0/24",
+							Except: []string{"1.1.1.0/16"},
+						},
+						{
+							CIDR:   "192.168.0.1",
+							Except: []string{"192.168.0.2"},
+						},
+					},
+					Ports: []networking.NetworkPolicyPort{
+						{
+							Protocol: nil,
+							Port:     &invalid,
+						},
+					},
+				},
+			},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			p := &PolicySpec{
+				Ingress: tt.Ingress,
+				Egress:  tt.Egress,
+			}
+			if err := p.Validate(); (err != nil) != tt.wantErr {
+				t.Errorf("PolicySpec.Validate() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}

controllers/clusterwidenetworkpolicy_controller.go

@@ -45,18 +45,24 @@ const clusterwideNPNamespace = "firewall"
 func (r *ClusterwideNetworkPolicyReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 	ctx := context.Background()
 
+	var clusterNP firewallv1.ClusterwideNetworkPolicy
+	if err := r.Get(ctx, req.NamespacedName, &clusterNP); err != nil {
+		return ctrl.Result{}, client.IgnoreNotFound(err)
+	}
+
 	// if network policy does not belong to the namespace where clusterwide network policies are stored:
 	// update status with error message
 	if req.Namespace != clusterwideNPNamespace {
-		var clusterNP firewallv1.ClusterwideNetworkPolicy
-		if err := r.Get(ctx, req.NamespacedName, &clusterNP); err != nil {
-			return ctrl.Result{}, client.IgnoreNotFound(err)
-		}
-
 		r.recorder.Event(&clusterNP, "Warning", "Unapplicable", fmt.Sprintf("cluster wide network policies must be defined in namespace %s otherwise they won't take effect", clusterwideNPNamespace))
 		return ctrl.Result{}, nil
 	}
 
+	err := clusterNP.Spec.Validate()
+	if err != nil {
+		r.recorder.Event(&clusterNP, "Warning", "Unapplicable", fmt.Sprintf("cluster wide network policy is not valid: %v", err))
+		return ctrl.Result{}, nil
+	}
+
 	return ctrl.Result{}, nil
 }
 

controllers/firewall_controller.go

@@ -383,19 +383,30 @@ func (r *FirewallReconciler) reconcileFirewallService(ctx context.Context, s fir
 
 // updateStatus updates the status field for this firewall
 func (r *FirewallReconciler) updateStatus(ctx context.Context, f firewallv1.Firewall, log logr.Logger) error {
-	if !f.Spec.DryRun {
-		c := collector.NewNFTablesCollector(&r.Log)
-		ruleStats := c.CollectRuleStats()
-
+	if f.Spec.DryRun {
 		f.Status.FirewallStats = firewallv1.FirewallStats{
-			RuleStats: ruleStats,
+			RuleStats:   firewallv1.RuleStatsByAction{},
+			DeviceStats: firewallv1.DeviceStatsByDevice{},
+			IDSStats:    firewallv1.IDSStatsByDevice{},
 		}
-		deviceStats, err := c.CollectDeviceStats()
-		if err != nil {
-			return err
+		f.Status.Updated.Time = time.Now()
+		if err := r.Status().Update(ctx, &f); err != nil {
+			return fmt.Errorf("unable to update firewall status, err: %w", err)
 		}
-		f.Status.FirewallStats.DeviceStats = deviceStats
+		return nil
+	}
+
+	c := collector.NewNFTablesCollector(&r.Log)
+	ruleStats := c.CollectRuleStats()
+
+	f.Status.FirewallStats = firewallv1.FirewallStats{
+		RuleStats: ruleStats,
+	}
+	deviceStats, err := c.CollectDeviceStats()
+	if err != nil {
+		return err
 	}
+	f.Status.FirewallStats.DeviceStats = deviceStats
 
 	idsStats := firewallv1.IDSStatsByDevice{}
 	if r.EnableIDS { // checks the CLI-flag

deploy/clusterwidenetworkpolicy.yaml

@@ -12,15 +12,14 @@ metadata:
 spec:
   egress:
   - to:
-    - cidr: 1.1.0.0/24
+    - cidr: 1.1.0.0/16
       except:
-      - 1.1.1.0/16
+      - 1.1.1.0/24
     - cidr: 8.8.8.8/32
     ports:
     - protocol: UDP
       port: 53
-    - protocol: TCP
-      port: 53
+    - port: 53
 ---
 apiVersion: v1
 kind: Namespace

deploy/firewall.yaml

@@ -16,7 +16,7 @@ spec:
   egressRules:
   - networkid: internet
     ips:
-    - 212.34.80.1
+    - 185.1.2.4
   firewallNetworks:
   - asn: 4200003073
     destinationprefixes: []
@@ -26,8 +26,6 @@ spec:
     networkid: bc830818-2df1-4904-8c40-4322296d393d
     prefixes:
       - 10.0.16.0/22
-    private: true
-    underlay: false
     networktype: privateprimaryunshared
     vrf: 3981
   - asn: 4200003073
@@ -40,8 +38,6 @@ spec:
     prefixes:
       - 185.1.2.0/24
       - 185.27.0.0/22
-    private: false
-    underlay: false
     networktype: external
     vrf: 104009
   - asn: 4200003073
@@ -52,8 +48,6 @@ spec:
     networkid: underlay-vagrant-lab
     prefixes:
       - 10.0.12.0/22
-    private: false
-    underlay: true
     networktype: external
     vrf: 0
   signature: ""

pkg/nftables/rendering.go

@@ -25,6 +25,10 @@ type firewallRenderingData struct {
 func newFirewallRenderingData(f *Firewall) (*firewallRenderingData, error) {
 	ingress, egress := nftablesRules{}, nftablesRules{}
 	for _, np := range f.clusterwideNetworkPolicies.Items {
+		err := np.Spec.Validate()
+		if err != nil {
+			continue
+		}
 		i, e := clusterwideNetworkPolicyRules(np)
 		ingress = append(ingress, i...)
 		egress = append(egress, e...)

pkg/nftables/service.go

@@ -2,26 +2,47 @@ package nftables
 
 import (
 	"fmt"
+	"net"
 	"strings"
 
 	corev1 "k8s.io/api/core/v1"
 )
 
+func isCIDR(cidr string) bool {
+	_, _, err := net.ParseCIDR(cidr)
+	return err != nil
+}
+
+func isIP(ip string) bool {
+	i := net.ParseIP(ip)
+	return i != nil
+}
+
 // serviceRules generates nftables rules base on a k8s service definition
 func serviceRules(svc corev1.Service) nftablesRules {
 	if svc.Spec.Type != corev1.ServiceTypeLoadBalancer && svc.Spec.Type != corev1.ServiceTypeNodePort {
 		return nil
 	}
 
 	from := []string{}
+	for _, lbsr := range svc.Spec.LoadBalancerSourceRanges {
+		if !isCIDR(lbsr) && !isIP(lbsr) {
+			continue
+		}
+	}
+
 	from = append(from, svc.Spec.LoadBalancerSourceRanges...)
 	to := []string{}
 	if svc.Spec.Type == corev1.ServiceTypeLoadBalancer {
 		if svc.Spec.LoadBalancerIP != "" {
-			to = append(to, svc.Spec.LoadBalancerIP)
+			if isIP(svc.Spec.LoadBalancerIP) {
+				to = append(to, svc.Spec.LoadBalancerIP)
+			}
 		}
 		for _, e := range svc.Status.LoadBalancer.Ingress {
-			to = append(to, e.IP)
+			if isIP(e.IP) {
+				to = append(to, e.IP)
+			}
 		}
 	}