diff --git a/npm/pkg/controlplane/translation/parseSelector.go b/npm/pkg/controlplane/translation/parseSelector.go index ded4ffcc47..e3fce8056c 100644 --- a/npm/pkg/controlplane/translation/parseSelector.go +++ b/npm/pkg/controlplane/translation/parseSelector.go @@ -3,15 +3,22 @@ package translation import ( "fmt" + "regexp" + "github.com/Azure/azure-container-networking/log" "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets" "github.com/Azure/azure-container-networking/npm/util" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// validLabelRegex is defined from the result of kubectl (this includes empty string matches): +// a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with +// an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?' +var validLabelRegex = regexp.MustCompile("(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?") + // flattenNameSpaceSelector will help flatten multiple nameSpace selector match Expressions values // into multiple label selectors helping with the OR condition. -func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) []metav1.LabelSelector { +func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelSelector, error) { /* This function helps to create multiple labelSelectors when given a single multivalue nsSelector Take below example: this nsSelector has 2 values in a matchSelector. @@ -48,11 +55,11 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) []metav1.LabelSe // To avoid any additional length checks, just return a slice of labelSelectors // with original nsSelector if nsSelector == nil { - return []metav1.LabelSelector{} + return []metav1.LabelSelector{}, nil } if len(nsSelector.MatchExpressions) == 0 { - return []metav1.LabelSelector{*nsSelector} + return []metav1.LabelSelector{*nsSelector}, nil } // create a baseSelector which needs to be same across all @@ -71,6 +78,12 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) []metav1.LabelSe // to create multiple nsSelectors to preserve OR condition across all labels and expressions switch { case (req.Operator == metav1.LabelSelectorOpIn) || (req.Operator == metav1.LabelSelectorOpNotIn): + for _, v := range req.Values { + if !isValidLabelValue(v) { + return nil, ErrInvalidMatchExpressionValues + } + } + if len(req.Values) == 1 { // for length 1, add the matchExpr to baseSelector baseSelector.MatchExpressions = append(baseSelector.MatchExpressions, req) @@ -89,7 +102,7 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) []metav1.LabelSe // If there are no multiValue NS selector match expressions // return the original NsSelector if !multiValuePresent { - return []metav1.LabelSelector{*nsSelector} + return []metav1.LabelSelector{*nsSelector}, nil } // Now use the baseSelector and loop over multiValueMatchExprs to create all @@ -101,7 +114,7 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) []metav1.LabelSe flatNsSelectors = zipMatchExprs(flatNsSelectors, req) } - return flatNsSelectors + return flatNsSelectors, nil } // zipMatchExprs helps with zipping a given matchExpr with given baseLabelSelectors @@ -253,6 +266,12 @@ func parsePodSelector(policyKey string, selector *metav1.LabelSelector) ([]label } switch op { case metav1.LabelSelectorOpIn, metav1.LabelSelectorOpNotIn: + for _, v := range req.Values { + if !isValidLabelValue(v) { + return nil, ErrInvalidMatchExpressionValues + } + } + // "(!) + matchKey + : + matchVal" case if len(req.Values) == 1 { setName = util.GetIpSetFromLabelKV(req.Key, req.Values[0]) @@ -284,3 +303,14 @@ func unsupportedOpsInWindows(op metav1.LabelSelectorOperator) bool { return util.IsWindowsDP() && (op == metav1.LabelSelectorOpNotIn || op == metav1.LabelSelectorOpDoesNotExist) } + +// isValidLabelValue ensures the string is empty or satisfies validLabelRegex. +// Given that v != "", ReplaceAllString() would yield "" when v matches this regex exactly once. +func isValidLabelValue(v string) bool { + matches := validLabelRegex.FindAllStringIndex(v, -1) + // v = "abc-123" would produce [[0 7]], which satisfies the below + // v = "" will produce [[0 0]], which satisfies the below + // v = "$" would produce [[0 0] [1 1]], which would fail the below + // v = "abc$" would produce [[0 3] [4 4]], which would fail the below + return len(matches) == 1 && matches[0][0] == 0 && matches[0][1] == len(v) +} diff --git a/npm/pkg/controlplane/translation/parseSelector_test.go b/npm/pkg/controlplane/translation/parseSelector_test.go index 6285051e6c..e93f99500a 100644 --- a/npm/pkg/controlplane/translation/parseSelector_test.go +++ b/npm/pkg/controlplane/translation/parseSelector_test.go @@ -1,23 +1,27 @@ package translation import ( + "fmt" "reflect" "testing" + "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestFlattenNameSpaceSelectorCases(t *testing.T) { firstSelector := &metav1.LabelSelector{} - testSelectors := flattenNameSpaceSelector(firstSelector) + testSelectors, err := flattenNameSpaceSelector(firstSelector) + require.Nil(t, err) if len(testSelectors) != 1 { t.Errorf("TestFlattenNameSpaceSelectorCases failed @ 1st selector length check %+v", testSelectors) } var secondSelector *metav1.LabelSelector - testSelectors = flattenNameSpaceSelector(secondSelector) + testSelectors, err = flattenNameSpaceSelector(secondSelector) + require.Nil(t, err) if len(testSelectors) > 0 { t.Errorf("TestFlattenNameSpaceSelectorCases failed @ 1st selector length check %+v", testSelectors) } @@ -61,7 +65,8 @@ func TestFlattenNameSpaceSelector(t *testing.T) { MatchLabels: commonMatchLabel, } - testSelectors := flattenNameSpaceSelector(firstSelector) + testSelectors, err := flattenNameSpaceSelector(firstSelector) + require.Nil(t, err) if len(testSelectors) != 1 { t.Errorf("TestFlattenNameSpaceSelector failed @ 1st selector length check %+v", testSelectors) } @@ -105,7 +110,8 @@ func TestFlattenNameSpaceSelector(t *testing.T) { MatchLabels: commonMatchLabel, } - testSelectors = flattenNameSpaceSelector(secondSelector) + testSelectors, err = flattenNameSpaceSelector(secondSelector) + require.Nil(t, err) if len(testSelectors) != 8 { t.Errorf("TestFlattenNameSpaceSelector failed @ 2nd selector length check %+v", testSelectors) } @@ -399,7 +405,8 @@ func TestFlattenNameSpaceSelectorWoMatchLabels(t *testing.T) { }, } - testSelectors := flattenNameSpaceSelector(firstSelector) + testSelectors, err := flattenNameSpaceSelector(firstSelector) + require.Nil(t, err) if len(testSelectors) != 2 { t.Errorf("TestFlattenNameSpaceSelector failed @ 1st selector length check %+v", testSelectors) } @@ -471,3 +478,160 @@ func TestFlattenNameSpaceSelectorWoMatchLabels(t *testing.T) { t.Errorf("TestFlattenNameSpaceSelector failed @ 1st selector deepEqual check.\n Expected: %+v \n Actual: %+v", expectedSelectors, testSelectors) } } + +func TestFlattenNamespaceSelectorError(t *testing.T) { + tests := []struct { + name string + selector *metav1.LabelSelector + wantErr bool + }{ + { + name: "good alphanumeric with hyphen", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "testIn", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "good", + "good-1", + "good2-too", + }, + }, + { + Key: "testNotIn", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{ + "good", + "good-1", + "good2-too", + }, + }, + }, + }, + wantErr: false, + }, + { + name: "bad in", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "testIn", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "good-1", + "bad$", + }, + }, + }, + }, + wantErr: true, + }, + { + name: "bad not in", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "testNotIn", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad$", + "good-1", + }, + }, + }, + }, + wantErr: true, + }, + { + name: "good and bad", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "testIn", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "good-1", + }, + }, + { + Key: "testNotIn", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad$", + "good-1", + }, + }, + }, + }, + wantErr: true, + }, + { + name: "bad with space", + selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "testIn", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad space", + "good-1", + }, + }, + }, + }, + wantErr: true, + }, + } + + for i, tt := range tests { + tt := tt + t.Run(fmt.Sprintf("test %d", i), func(t *testing.T) { + s, err := flattenNameSpaceSelector(tt.selector) + if tt.wantErr { + require.Error(t, err) + require.Nil(t, s) + } else { + require.NoError(t, err) + require.NotNil(t, s) + } + }) + } +} + +func TestIsValidLabel(t *testing.T) { + good := []string{ + "", + "1", + "abc", + "ABC", + "abc1", + "ABC1", + "abc-1", + "ABC-1", + "ABC_1", + "ABC_-a54--f", + } + + for _, g := range good { + require.True(t, isValidLabelValue(g), "string was [%s]", g) + } + + bad := []string{ + "-", + "_", + "$", + " ", + "abc-", + "abc$", + "abc$123", + "bad space", + "end-with-hyphen-", + "end-with-underscore_", + "end-with-space ", + } + + for _, b := range bad { + require.False(t, isValidLabelValue(b), "string was [%s]", b) + } +} diff --git a/npm/pkg/controlplane/translation/translatePolicy.go b/npm/pkg/controlplane/translation/translatePolicy.go index c1dbe00730..0ede73986f 100644 --- a/npm/pkg/controlplane/translation/translatePolicy.go +++ b/npm/pkg/controlplane/translation/translatePolicy.go @@ -28,6 +28,10 @@ var ( 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") + // ErrInvalidMatchExpressionValues ensures proper matchExpression label values since k8s doesn't perform this check. + 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", + ) ) type podSelectorResult struct { @@ -407,7 +411,11 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, netPolName string, dire if peer.PodSelector == nil && peer.NamespaceSelector != nil { // Before translating NamespaceSelector, flattenNameSpaceSelector function call should be called // to handle multiple values in matchExpressions spec. - flattenNSSelector := flattenNameSpaceSelector(peer.NamespaceSelector) + flattenNSSelector, err := flattenNameSpaceSelector(peer.NamespaceSelector) + if err != nil { + return err + } + for i := range flattenNSSelector { nsSelectorIPSets, nsSelectorList := nameSpaceSelector(matchType, &flattenNSSelector[i]) npmNetPol.RuleIPSets = append(npmNetPol.RuleIPSets, nsSelectorIPSets...) @@ -444,7 +452,11 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, netPolName string, dire // Before translating NamespaceSelector, flattenNameSpaceSelector function call should be called // to handle multiple values in matchExpressions spec. - flattenNSSelector := flattenNameSpaceSelector(peer.NamespaceSelector) + flattenNSSelector, err := flattenNameSpaceSelector(peer.NamespaceSelector) + if err != nil { + return err + } + for i := range flattenNSSelector { nsSelectorIPSets, nsSelectorList := nameSpaceSelector(matchType, &flattenNSSelector[i]) npmNetPol.RuleIPSets = append(npmNetPol.RuleIPSets, nsSelectorIPSets...) diff --git a/npm/pkg/controlplane/translation/translatePolicy_test.go b/npm/pkg/controlplane/translation/translatePolicy_test.go index a4fbe25c01..671d08407e 100644 --- a/npm/pkg/controlplane/translation/translatePolicy_test.go +++ b/npm/pkg/controlplane/translation/translatePolicy_test.go @@ -563,14 +563,15 @@ func TestPodSelector(t *testing.T) { policyKey := "test-ns/test-policy" policyKeyWithDash := policyKey + "-" tests := []struct { - name string - namespace string - matchType policies.MatchType - labelSelector *metav1.LabelSelector - podSelectorIPSets []*ipsets.TranslatedIPSet - childPodSelectorIPSets []*ipsets.TranslatedIPSet - podSelectorList []policies.SetInfo - skipWindows bool + name string + namespace string + matchType policies.MatchType + labelSelector *metav1.LabelSelector + podSelectorIPSets []*ipsets.TranslatedIPSet + childPodSelectorIPSets []*ipsets.TranslatedIPSet + podSelectorList []policies.SetInfo + skipWindows bool + wantExpressionValuesErr bool }{ { name: "all pods selector in default namespace in ingress", @@ -778,6 +779,47 @@ func TestPodSelector(t *testing.T) { }, skipWindows: true, }, + { + name: "bad in expression", + namespace: defaultNS, + matchType: matchType, + labelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "k0": "v0", + }, + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "k1", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad space", + "v11", + }, + }, + }, + }, + wantExpressionValuesErr: true, + }, + { + name: "bad notIn expression", + namespace: defaultNS, + matchType: matchType, + labelSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "k0": "v0", + }, + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "k1", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{ + "bad-char$", + }, + }, + }, + }, + wantExpressionValuesErr: true, + }, } for _, tt := range tests { @@ -795,7 +837,7 @@ func TestPodSelector(t *testing.T) { if psResult == nil { psResult = &podSelectorResult{} } - if tt.skipWindows && util.IsWindowsDP() { + if tt.wantExpressionValuesErr || (tt.skipWindows && util.IsWindowsDP()) { require.Error(t, err) } else { require.NoError(t, err) @@ -815,6 +857,7 @@ func TestNameSpaceSelector(t *testing.T) { labelSelector *metav1.LabelSelector nsSelectorIPSets []*ipsets.TranslatedIPSet nsSelectorList []policies.SetInfo + wantErr bool }{ { name: "namespaceSelector for all namespaces in ingress", @@ -1394,13 +1437,14 @@ func TestIngressPolicy(t *testing.T) { emptyString := intstr.FromString("") // TODO(jungukcho): add test cases with more complex rules tests := []struct { - name string - targetSelector *metav1.LabelSelector - rules []networkingv1.NetworkPolicyIngressRule - npmNetPol *policies.NPMNetworkPolicy - wantErr bool - skipWindows bool - windowsNil bool + name string + targetSelector *metav1.LabelSelector + rules []networkingv1.NetworkPolicyIngressRule + npmNetPol *policies.NPMNetworkPolicy + wantErr bool + skipWindows bool + windowsNil bool + wantTargetSelectorErr bool }{ { name: "only port in ingress rules", @@ -1949,6 +1993,98 @@ func TestIngressPolicy(t *testing.T) { }, }, }, + { + name: "bad target selector expression", + targetSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "k0": "v0", + }, + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "k1", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad space", + }, + }, + }, + }, + rules: []networkingv1.NetworkPolicyIngressRule{ + {}, + }, + npmNetPol: &policies.NPMNetworkPolicy{ + Namespace: "default", + PolicyKey: "default/serve-tcp", + ACLPolicyID: "azure-acl-default-serve-tcp", + }, + wantTargetSelectorErr: true, + }, + { + name: "bad pod selector expression", + targetSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "k0": "v0", + }, + }, + rules: []networkingv1.NetworkPolicyIngressRule{ + { + From: []networkingv1.NetworkPolicyPeer{ + { + PodSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "k1", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad space", + }, + }, + }, + }, + }, + }, + }, + }, + npmNetPol: &policies.NPMNetworkPolicy{ + Namespace: "default", + PolicyKey: "default/serve-tcp", + ACLPolicyID: "azure-acl-default-serve-tcp", + }, + wantErr: true, + }, + { + name: "bad ns selector expression", + targetSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "k0": "v0", + }, + }, + rules: []networkingv1.NetworkPolicyIngressRule{ + { + From: []networkingv1.NetworkPolicyPeer{ + { + NamespaceSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "k1", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad space", + }, + }, + }, + }, + }, + }, + }, + }, + npmNetPol: &policies.NPMNetworkPolicy{ + Namespace: "default", + PolicyKey: "default/serve-tcp", + ACLPolicyID: "azure-acl-default-serve-tcp", + }, + wantErr: true, + }, } for _, tt := range tests { @@ -1964,6 +2100,11 @@ func TestIngressPolicy(t *testing.T) { require.Error(t, err) return } + if tt.wantTargetSelectorErr { + require.Error(t, err) + return + } + require.NoError(t, err) npmNetPol.PodSelectorIPSets = psResult.psSets npmNetPol.ChildPodSelectorIPSets = psResult.childPSSets @@ -1987,13 +2128,14 @@ func TestEgressPolicy(t *testing.T) { targetPodMatchType := policies.EitherMatch peerMatchType := policies.DstMatch tests := []struct { - name string - targetSelector *metav1.LabelSelector - rules []networkingv1.NetworkPolicyEgressRule - npmNetPol *policies.NPMNetworkPolicy - wantErr bool - skipWindows bool - windowsNil bool + name string + targetSelector *metav1.LabelSelector + rules []networkingv1.NetworkPolicyEgressRule + npmNetPol *policies.NPMNetworkPolicy + wantErr bool + skipWindows bool + windowsNil bool + wantTargetSelectorErr bool }{ { name: "only port in egress rules", @@ -2542,6 +2684,98 @@ func TestEgressPolicy(t *testing.T) { }, }, }, + { + name: "bad target selector expression", + targetSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "k0": "v0", + }, + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "k1", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad space", + }, + }, + }, + }, + rules: []networkingv1.NetworkPolicyEgressRule{ + {}, + }, + npmNetPol: &policies.NPMNetworkPolicy{ + Namespace: "default", + PolicyKey: "default/serve-tcp", + ACLPolicyID: "azure-acl-default-serve-tcp", + }, + wantTargetSelectorErr: true, + }, + { + name: "bad pod selector expression", + targetSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "k0": "v0", + }, + }, + rules: []networkingv1.NetworkPolicyEgressRule{ + { + To: []networkingv1.NetworkPolicyPeer{ + { + PodSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "k1", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad space", + }, + }, + }, + }, + }, + }, + }, + }, + npmNetPol: &policies.NPMNetworkPolicy{ + Namespace: "default", + PolicyKey: "default/serve-tcp", + ACLPolicyID: "azure-acl-default-serve-tcp", + }, + wantErr: true, + }, + { + name: "bad ns selector expression", + targetSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "k0": "v0", + }, + }, + rules: []networkingv1.NetworkPolicyEgressRule{ + { + To: []networkingv1.NetworkPolicyPeer{ + { + NamespaceSelector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "k1", + Operator: metav1.LabelSelectorOpIn, + Values: []string{ + "bad space", + }, + }, + }, + }, + }, + }, + }, + }, + npmNetPol: &policies.NPMNetworkPolicy{ + Namespace: "default", + PolicyKey: "default/serve-tcp", + ACLPolicyID: "azure-acl-default-serve-tcp", + }, + wantErr: true, + }, } for _, tt := range tests { @@ -2557,6 +2791,11 @@ func TestEgressPolicy(t *testing.T) { require.Error(t, err) return } + if tt.wantTargetSelectorErr { + require.Error(t, err) + return + } + require.NoError(t, err) npmNetPol.PodSelectorIPSets = psResult.psSets npmNetPol.ChildPodSelectorIPSets = psResult.childPSSets