From ad2e46aca365e95e142c013024b688110f143df7 Mon Sep 17 00:00:00 2001 From: mwindower Date: Tue, 8 Dec 2020 11:19:01 +0100 Subject: [PATCH] Validations and Fixes (#68) --- api/v1/clusterwidenetworkpolicy_types.go | 67 +++++++++++ api/v1/clusterwidenetworkpolicy_types_test.go | 106 ++++++++++++++++++ .../clusterwidenetworkpolicy_controller.go | 16 ++- controllers/firewall_controller.go | 29 +++-- deploy/clusterwidenetworkpolicy.yaml | 7 +- deploy/firewall.yaml | 8 +- pkg/nftables/rendering.go | 4 + pkg/nftables/service.go | 25 ++++- 8 files changed, 235 insertions(+), 27 deletions(-) create mode 100644 api/v1/clusterwidenetworkpolicy_types_test.go diff --git a/api/v1/clusterwidenetworkpolicy_types.go b/api/v1/clusterwidenetworkpolicy_types.go index b6915f6f..5ce1150d 100644 --- a/api/v1/clusterwidenetworkpolicy_types.go +++ b/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{}) } diff --git a/api/v1/clusterwidenetworkpolicy_types_test.go b/api/v1/clusterwidenetworkpolicy_types_test.go new file mode 100644 index 00000000..1f75e097 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/controllers/clusterwidenetworkpolicy_controller.go b/controllers/clusterwidenetworkpolicy_controller.go index 13e0cf5f..9e71ca1e 100644 --- a/controllers/clusterwidenetworkpolicy_controller.go +++ b/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 } diff --git a/controllers/firewall_controller.go b/controllers/firewall_controller.go index 89f272f7..44a0908c 100644 --- a/controllers/firewall_controller.go +++ b/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 diff --git a/deploy/clusterwidenetworkpolicy.yaml b/deploy/clusterwidenetworkpolicy.yaml index 2ad6ed33..635ddc79 100644 --- a/deploy/clusterwidenetworkpolicy.yaml +++ b/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 diff --git a/deploy/firewall.yaml b/deploy/firewall.yaml index 97a4b97a..57ba41ad 100644 --- a/deploy/firewall.yaml +++ b/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: "" \ No newline at end of file diff --git a/pkg/nftables/rendering.go b/pkg/nftables/rendering.go index ec554722..9b32f5aa 100644 --- a/pkg/nftables/rendering.go +++ b/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...) diff --git a/pkg/nftables/service.go b/pkg/nftables/service.go index 839e000f..3bc39054 100644 --- a/pkg/nftables/service.go +++ b/pkg/nftables/service.go @@ -2,11 +2,22 @@ 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 { @@ -14,14 +25,24 @@ func serviceRules(svc corev1.Service) nftablesRules { } 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) + } } }