From d2840f240cb233509d1734e85ff6061dbe859794 Mon Sep 17 00:00:00 2001 From: Cristina Kovacs Date: Thu, 29 Dec 2022 13:06:20 -0600 Subject: [PATCH 1/6] add check for ipv6 addresses in translatePolicy --- .../controllers/v2/networkPolicyController.go | 2 +- .../translation/translatePolicy.go | 23 +++- .../translation/translatePolicy_test.go | 120 ++++++++++++++++++ 3 files changed, 142 insertions(+), 3 deletions(-) diff --git a/npm/pkg/controlplane/controllers/v2/networkPolicyController.go b/npm/pkg/controlplane/controllers/v2/networkPolicyController.go index 0d33aef7d4..6d89176455 100644 --- a/npm/pkg/controlplane/controllers/v2/networkPolicyController.go +++ b/npm/pkg/controlplane/controllers/v2/networkPolicyController.go @@ -296,7 +296,7 @@ func (c *NetworkPolicyController) syncAddAndUpdateNetPol(netPolObj *networkingv1 klog.Errorf("Failed to translate podSelector in NetworkPolicy %s in namespace %s: %s", netPolObj.ObjectMeta.Name, netPolObj.ObjectMeta.Namespace, err.Error()) // The exec time isn't relevant here, so consider a no-op. - return metrics.NoOp, errNetPolTranslationFailure + return metrics.NoOp, nil } _, policyExisted := c.rawNpSpecMap[netpolKey] diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index c1dbe00730..e26f160871 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -551,12 +551,31 @@ func egressPolicy(npmNetPol *policies.NPMNetworkPolicy, netPolName string, egres return nil } -// TranslatePolicy traslates networkpolicy object to NPMNetworkPolicy object -// and return the NPMNetworkPolicy object. +// TranslatePolicy translates networkpolicy object to NPMNetworkPolicy object +// and returns the NPMNetworkPolicy object. func TranslatePolicy(npObj *networkingv1.NetworkPolicy) (*policies.NPMNetworkPolicy, error) { netPolName := npObj.Name npmNetPol := policies.NewNPMNetworkPolicy(netPolName, npObj.Namespace) + // check if IPs are valid IPV4 addresses + for _, egress := range npObj.Spec.Egress { + for _, value := range egress.To { + if value.IPBlock != nil { + if !util.IsIPV4(value.IPBlock.CIDR) { + return nil, fmt.Errorf("IP %s is not a valid IPV4 address", value.IPBlock.CIDR) + } + } + } + } + for _, ingress := range npObj.Spec.Ingress { + for _, value := range ingress.From { + if value.IPBlock != nil { + if !util.IsIPV4(value.IPBlock.CIDR) { + return nil, fmt.Errorf("IP %s is not a valid IPV4 address", value.IPBlock.CIDR) + } + } + } + } // podSelector in spec.PodSelector is common for ingress and egress. // Process this podSelector first. psResult, err := podSelectorWithNS(npmNetPol.PolicyKey, npmNetPol.Namespace, policies.EitherMatch, &npObj.Spec.PodSelector) diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index a4fbe25c01..d47080abd5 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -2573,3 +2573,123 @@ func TestEgressPolicy(t *testing.T) { }) } } + +func TestValidateTranslatePolicyIPV4Check(t *testing.T) { + tests := []struct { + name string + netPolicy *networkingv1.NetworkPolicy + wantErr bool + }{ + { + name: "bad egress", + netPolicy: &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-egress", + }, + Spec: networkingv1.NetworkPolicySpec{ + Ingress: []networkingv1.NetworkPolicyIngressRule{ + { + From: []networkingv1.NetworkPolicyPeer{ + { + IPBlock: &networkingv1.IPBlock{ + CIDR: "10.0.0.0/16", + }, + }, + }, + }, + }, + Egress: []networkingv1.NetworkPolicyEgressRule{ + { + To: []networkingv1.NetworkPolicyPeer{ + { + IPBlock: &networkingv1.IPBlock{ + CIDR: "2001:db9::/64", + }, + }, + }, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "bad ingress", + netPolicy: &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-ingress", + }, + Spec: networkingv1.NetworkPolicySpec{ + Ingress: []networkingv1.NetworkPolicyIngressRule{ + { + From: []networkingv1.NetworkPolicyPeer{ + { + IPBlock: &networkingv1.IPBlock{ + CIDR: "2345:0425:2CA1:0000:0000:0567:5673:23b5/24", + }, + }, + }, + }, + }, + Egress: []networkingv1.NetworkPolicyEgressRule{ + { + To: []networkingv1.NetworkPolicyPeer{ + { + IPBlock: &networkingv1.IPBlock{ + CIDR: "12.4.1.5/32", + }, + }, + }, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "both valid", + netPolicy: &networkingv1.NetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "both valid", + }, + Spec: networkingv1.NetworkPolicySpec{ + Ingress: []networkingv1.NetworkPolicyIngressRule{ + { + From: []networkingv1.NetworkPolicyPeer{ + { + IPBlock: &networkingv1.IPBlock{ + CIDR: "10.0.0.0/16", + }, + }, + }, + }, + }, + Egress: []networkingv1.NetworkPolicyEgressRule{ + { + To: []networkingv1.NetworkPolicyPeer{ + { + IPBlock: &networkingv1.IPBlock{ + CIDR: "10.0.0.0/32", + }, + }, + }, + }, + }, + }, + }, + wantErr: false, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + _, err := TranslatePolicy(tt.netPolicy) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} From 558f8c54d04aeaf20c5459289e3ad8f1084cda21 Mon Sep 17 00:00:00 2001 From: Cristina Kovacs Date: Thu, 29 Dec 2022 13:32:33 -0600 Subject: [PATCH 2/6] fix static error lint issue --- npm/pkg/controlplane/translation/translatePolicy.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index e26f160871..b7ba2a1575 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -27,7 +27,8 @@ var ( // ErrUnsupportedExceptCIDR is returned when Except CIDR block translation feature is used in windows. ErrUnsupportedExceptCIDR = errors.New("unsupported Except CIDR block translation features used on windows") // ErrUnsupportedSCTP is returned when SCTP protocol is used in windows. - ErrUnsupportedSCTP = errors.New("unsupported SCTP protocol used on windows") + ErrUnsupportedSCTP = errors.New("unsupported SCTP protocol used on windows") + ErrInvalidIPV6Address = errors.New("invalid IPV6 address") ) type podSelectorResult struct { @@ -562,7 +563,7 @@ func TranslatePolicy(npObj *networkingv1.NetworkPolicy) (*policies.NPMNetworkPol for _, value := range egress.To { if value.IPBlock != nil { if !util.IsIPV4(value.IPBlock.CIDR) { - return nil, fmt.Errorf("IP %s is not a valid IPV4 address", value.IPBlock.CIDR) + return nil, ErrInvalidIPV6Address } } } @@ -571,7 +572,7 @@ func TranslatePolicy(npObj *networkingv1.NetworkPolicy) (*policies.NPMNetworkPol for _, value := range ingress.From { if value.IPBlock != nil { if !util.IsIPV4(value.IPBlock.CIDR) { - return nil, fmt.Errorf("IP %s is not a valid IPV4 address", value.IPBlock.CIDR) + return nil, ErrInvalidIPV6Address } } } From 1dcaf9b03dd1b9ba0262aba41bcc93c8ac91889e Mon Sep 17 00:00:00 2001 From: Cristina Kovacs Date: Tue, 3 Jan 2023 19:06:03 -0600 Subject: [PATCH 3/6] updated static error name and moved IPV4 logic --- .../controllers/v2/networkPolicyController.go | 2 +- .../translation/translatePolicy.go | 27 +++++-------------- 2 files changed, 7 insertions(+), 22 deletions(-) diff --git a/npm/pkg/controlplane/controllers/v2/networkPolicyController.go b/npm/pkg/controlplane/controllers/v2/networkPolicyController.go index 6d89176455..c8baf98efd 100644 --- a/npm/pkg/controlplane/controllers/v2/networkPolicyController.go +++ b/npm/pkg/controlplane/controllers/v2/networkPolicyController.go @@ -295,7 +295,7 @@ func (c *NetworkPolicyController) syncAddAndUpdateNetPol(netPolObj *networkingv1 } klog.Errorf("Failed to translate podSelector in NetworkPolicy %s in namespace %s: %s", netPolObj.ObjectMeta.Name, netPolObj.ObjectMeta.Namespace, err.Error()) - // The exec time isn't relevant here, so consider a no-op. + // The exec time isn't relevant here, so consider a no-op. Returning nil to prevent re-queuing since this is not a transient error. return metrics.NoOp, nil } diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index b7ba2a1575..6259dda068 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -27,8 +27,8 @@ var ( // ErrUnsupportedExceptCIDR is returned when Except CIDR block translation feature is used in windows. ErrUnsupportedExceptCIDR = errors.New("unsupported Except CIDR block translation features used on windows") // ErrUnsupportedSCTP is returned when SCTP protocol is used in windows. - ErrUnsupportedSCTP = errors.New("unsupported SCTP protocol used on windows") - ErrInvalidIPV6Address = errors.New("invalid IPV6 address") + ErrUnsupportedSCTP = errors.New("unsupported SCTP protocol used on windows") + ErrUnsupportedIPAddress = errors.New("unsupported IP address") ) type podSelectorResult struct { @@ -383,6 +383,10 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, netPolName string, dire // #2.1 Handle IPBlock and port if exist if peer.IPBlock != nil { if len(peer.IPBlock.CIDR) > 0 { + if !util.IsIPV4(peer.IPBlock.CIDR) { + return ErrUnsupportedIPAddress + } + ipBlockIPSet, ipBlockSetInfo, err := ipBlockRule(netPolName, npmNetPol.Namespace, direction, matchType, ruleIndex, peerIdx, peer.IPBlock) if err != nil { return err @@ -558,25 +562,6 @@ func TranslatePolicy(npObj *networkingv1.NetworkPolicy) (*policies.NPMNetworkPol netPolName := npObj.Name npmNetPol := policies.NewNPMNetworkPolicy(netPolName, npObj.Namespace) - // check if IPs are valid IPV4 addresses - for _, egress := range npObj.Spec.Egress { - for _, value := range egress.To { - if value.IPBlock != nil { - if !util.IsIPV4(value.IPBlock.CIDR) { - return nil, ErrInvalidIPV6Address - } - } - } - } - for _, ingress := range npObj.Spec.Ingress { - for _, value := range ingress.From { - if value.IPBlock != nil { - if !util.IsIPV4(value.IPBlock.CIDR) { - return nil, ErrInvalidIPV6Address - } - } - } - } // podSelector in spec.PodSelector is common for ingress and egress. // Process this podSelector first. psResult, err := podSelectorWithNS(npmNetPol.PolicyKey, npmNetPol.Namespace, policies.EitherMatch, &npObj.Spec.PodSelector) From c07c9a3552a696c9e6add1b2bcbd977e0f4efc2a Mon Sep 17 00:00:00 2001 From: Cristina Kovacs Date: Wed, 4 Jan 2023 16:23:04 -0600 Subject: [PATCH 4/6] moved check to ipBlockRule --- .../translation/translatePolicy.go | 8 +- .../translation/translatePolicy_test.go | 120 ------------------ 2 files changed, 4 insertions(+), 124 deletions(-) diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index 6259dda068..32b9de794c 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -222,6 +222,10 @@ func ipBlockRule(policyName, ns string, direction policies.Direction, matchType return nil, policies.SetInfo{}, nil } + if !util.IsIPV4(ipBlockRule.CIDR) { + return nil, policies.SetInfo{}, ErrUnsupportedIPAddress + } + ipBlockIPSet, err := ipBlockIPSet(policyName, ns, direction, ipBlockSetIndex, ipBlockPeerIndex, ipBlockRule) if err != nil { return nil, policies.SetInfo{}, err @@ -383,10 +387,6 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, netPolName string, dire // #2.1 Handle IPBlock and port if exist if peer.IPBlock != nil { if len(peer.IPBlock.CIDR) > 0 { - if !util.IsIPV4(peer.IPBlock.CIDR) { - return ErrUnsupportedIPAddress - } - ipBlockIPSet, ipBlockSetInfo, err := ipBlockRule(netPolName, npmNetPol.Namespace, direction, matchType, ruleIndex, peerIdx, peer.IPBlock) if err != nil { return err diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index d47080abd5..a4fbe25c01 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -2573,123 +2573,3 @@ func TestEgressPolicy(t *testing.T) { }) } } - -func TestValidateTranslatePolicyIPV4Check(t *testing.T) { - tests := []struct { - name string - netPolicy *networkingv1.NetworkPolicy - wantErr bool - }{ - { - name: "bad egress", - netPolicy: &networkingv1.NetworkPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "bad-egress", - }, - Spec: networkingv1.NetworkPolicySpec{ - Ingress: []networkingv1.NetworkPolicyIngressRule{ - { - From: []networkingv1.NetworkPolicyPeer{ - { - IPBlock: &networkingv1.IPBlock{ - CIDR: "10.0.0.0/16", - }, - }, - }, - }, - }, - Egress: []networkingv1.NetworkPolicyEgressRule{ - { - To: []networkingv1.NetworkPolicyPeer{ - { - IPBlock: &networkingv1.IPBlock{ - CIDR: "2001:db9::/64", - }, - }, - }, - }, - }, - }, - }, - wantErr: true, - }, - { - name: "bad ingress", - netPolicy: &networkingv1.NetworkPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "bad-ingress", - }, - Spec: networkingv1.NetworkPolicySpec{ - Ingress: []networkingv1.NetworkPolicyIngressRule{ - { - From: []networkingv1.NetworkPolicyPeer{ - { - IPBlock: &networkingv1.IPBlock{ - CIDR: "2345:0425:2CA1:0000:0000:0567:5673:23b5/24", - }, - }, - }, - }, - }, - Egress: []networkingv1.NetworkPolicyEgressRule{ - { - To: []networkingv1.NetworkPolicyPeer{ - { - IPBlock: &networkingv1.IPBlock{ - CIDR: "12.4.1.5/32", - }, - }, - }, - }, - }, - }, - }, - wantErr: true, - }, - { - name: "both valid", - netPolicy: &networkingv1.NetworkPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "both valid", - }, - Spec: networkingv1.NetworkPolicySpec{ - Ingress: []networkingv1.NetworkPolicyIngressRule{ - { - From: []networkingv1.NetworkPolicyPeer{ - { - IPBlock: &networkingv1.IPBlock{ - CIDR: "10.0.0.0/16", - }, - }, - }, - }, - }, - Egress: []networkingv1.NetworkPolicyEgressRule{ - { - To: []networkingv1.NetworkPolicyPeer{ - { - IPBlock: &networkingv1.IPBlock{ - CIDR: "10.0.0.0/32", - }, - }, - }, - }, - }, - }, - }, - wantErr: false, - }, - } - - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - _, err := TranslatePolicy(tt.netPolicy) - if tt.wantErr { - require.Error(t, err) - } else { - require.NoError(t, err) - } - }) - } -} From 294fd96a3cde9b33abfb3b609d2d4d5716984332 Mon Sep 17 00:00:00 2001 From: Cristina Kovacs Date: Thu, 5 Jan 2023 17:24:23 -0600 Subject: [PATCH 5/6] updated UT --- .../translation/translatePolicy_test.go | 33 +++++++++++++++++-- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index a4fbe25c01..210807376a 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -491,6 +491,7 @@ func TestIPBlockRule(t *testing.T) { translatedIPSet *ipsets.TranslatedIPSet setInfo policies.SetInfo skipWindows bool + wantErr bool }{ { name: "empty ipblock rule ", @@ -540,6 +541,26 @@ func TestIPBlockRule(t *testing.T) { setInfo: policies.NewSetInfo("test-network-policy-in-ns-default-0-0IN", ipsets.CIDRBlocks, included, policies.SrcMatch), skipWindows: true, }, + { + name: "invalid ipv6", + ipBlockInfo: createIPBlockInfo("test", defaultNS, policies.Ingress, policies.SrcMatch, 0, 0), + ipBlockRule: &networkingv1.IPBlock{ + CIDR: "2002::1234:abcd:ffff:c0a8:101/64", + }, + translatedIPSet: nil, + setInfo: policies.SetInfo{}, + wantErr: true, + }, + { + name: "invalid cidr", + ipBlockInfo: createIPBlockInfo("test", defaultNS, policies.Ingress, policies.SrcMatch, 0, 0), + ipBlockRule: &networkingv1.IPBlock{ + CIDR: "10.0.0.1/33", + }, + translatedIPSet: nil, + setInfo: policies.SetInfo{}, + wantErr: true, + }, } for _, tt := range tests { @@ -550,9 +571,15 @@ func TestIPBlockRule(t *testing.T) { if tt.skipWindows && util.IsWindowsDP() { require.Error(t, err) } else { - require.NoError(t, err) - require.Equal(t, tt.translatedIPSet, translatedIPSet) - require.Equal(t, tt.setInfo, setInfo) + if tt.wantErr { + require.Error(t, err) + require.Equal(t, tt.translatedIPSet, translatedIPSet) + require.Equal(t, tt.setInfo, setInfo) + } else { + require.NoError(t, err) + require.Equal(t, tt.translatedIPSet, translatedIPSet) + require.Equal(t, tt.setInfo, setInfo) + } } }) } From f28105aa7ff3bc6d01f4bf0911e85443fb671185 Mon Sep 17 00:00:00 2001 From: Cristina Kovacs Date: Mon, 6 Feb 2023 14:35:52 -0600 Subject: [PATCH 6/6] fix lint issue --- npm/pkg/controlplane/translation/translatePolicy.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index 7e0eed9853..12ac42b52e 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -32,8 +32,8 @@ var ( ErrInvalidMatchExpressionValues = errors.New( "matchExpression label values must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character", ) - // ErrUnsupportedIPAddress is returned when an unsupported IP address, such as IPV6, is used - ErrUnsupportedIPAddress = errors.New("unsupported IP address") + // ErrUnsupportedIPAddress is returned when an unsupported IP address, such as IPV6, is used + ErrUnsupportedIPAddress = errors.New("unsupported IP address") ) type podSelectorResult struct {