Skip to content

Commit 8bbb99e

Browse files
huntergregoryhuntergregory
authored
fix: [NPM] add NetworkPolicy validation for matchExpression values (#1717)
* add validation for matchExpression values * fix lint * make regex identical to kubectl validation and include more test cases * remove debugging lines
1 parent 45108ae commit 8bbb99e

File tree

4 files changed

+480
-35
lines changed

4 files changed

+480
-35
lines changed

‎npm/pkg/controlplane/translation/parseSelector.go‎

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,22 @@ package translation
33
import (
44
"fmt"
55

6+
"regexp"
7+
68
"github.com/Azure/azure-container-networking/log"
79
"github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets"
810
"github.com/Azure/azure-container-networking/npm/util"
911
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1012
)
1113

14+
// validLabelRegex is defined from the result of kubectl (this includes empty string matches):
15+
// a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with
16+
// 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])?'
17+
var validLabelRegex = regexp.MustCompile("(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?")
18+
1219
// flattenNameSpaceSelector will help flatten multiple nameSpace selector match Expressions values
1320
// into multiple label selectors helping with the OR condition.
14-
func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) []metav1.LabelSelector {
21+
func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) ([]metav1.LabelSelector, error) {
1522
/*
1623
This function helps to create multiple labelSelectors when given a single multivalue nsSelector
1724
Take below example: this nsSelector has 2 values in a matchSelector.
@@ -48,11 +55,11 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) []metav1.LabelSe
4855
// To avoid any additional length checks, just return a slice of labelSelectors
4956
// with original nsSelector
5057
if nsSelector == nil {
51-
return []metav1.LabelSelector{}
58+
return []metav1.LabelSelector{}, nil
5259
}
5360

5461
if len(nsSelector.MatchExpressions) == 0 {
55-
return []metav1.LabelSelector{*nsSelector}
62+
return []metav1.LabelSelector{*nsSelector}, nil
5663
}
5764

5865
// create a baseSelector which needs to be same across all
@@ -71,6 +78,12 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) []metav1.LabelSe
7178
// to create multiple nsSelectors to preserve OR condition across all labels and expressions
7279
switch {
7380
case (req.Operator == metav1.LabelSelectorOpIn) || (req.Operator == metav1.LabelSelectorOpNotIn):
81+
for _, v := range req.Values {
82+
if !isValidLabelValue(v) {
83+
return nil, ErrInvalidMatchExpressionValues
84+
}
85+
}
86+
7487
if len(req.Values) == 1 {
7588
// for length 1, add the matchExpr to baseSelector
7689
baseSelector.MatchExpressions = append(baseSelector.MatchExpressions, req)
@@ -89,7 +102,7 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) []metav1.LabelSe
89102
// If there are no multiValue NS selector match expressions
90103
// return the original NsSelector
91104
if !multiValuePresent {
92-
return []metav1.LabelSelector{*nsSelector}
105+
return []metav1.LabelSelector{*nsSelector}, nil
93106
}
94107

95108
// Now use the baseSelector and loop over multiValueMatchExprs to create all
@@ -101,7 +114,7 @@ func flattenNameSpaceSelector(nsSelector *metav1.LabelSelector) []metav1.LabelSe
101114
flatNsSelectors = zipMatchExprs(flatNsSelectors, req)
102115
}
103116

104-
return flatNsSelectors
117+
return flatNsSelectors, nil
105118
}
106119

107120
// zipMatchExprs helps with zipping a given matchExpr with given baseLabelSelectors
@@ -253,6 +266,12 @@ func parsePodSelector(policyKey string, selector *metav1.LabelSelector) ([]label
253266
}
254267
switch op {
255268
case metav1.LabelSelectorOpIn, metav1.LabelSelectorOpNotIn:
269+
for _, v := range req.Values {
270+
if !isValidLabelValue(v) {
271+
return nil, ErrInvalidMatchExpressionValues
272+
}
273+
}
274+
256275
// "(!) + matchKey + : + matchVal" case
257276
if len(req.Values) == 1 {
258277
setName = util.GetIpSetFromLabelKV(req.Key, req.Values[0])
@@ -284,3 +303,14 @@ func unsupportedOpsInWindows(op metav1.LabelSelectorOperator) bool {
284303
return util.IsWindowsDP() &&
285304
(op == metav1.LabelSelectorOpNotIn || op == metav1.LabelSelectorOpDoesNotExist)
286305
}
306+
307+
// isValidLabelValue ensures the string is empty or satisfies validLabelRegex.
308+
// Given that v != "", ReplaceAllString() would yield "" when v matches this regex exactly once.
309+
func isValidLabelValue(v string) bool {
310+
matches := validLabelRegex.FindAllStringIndex(v, -1)
311+
// v = "abc-123" would produce [[0 7]], which satisfies the below
312+
// v = "" will produce [[0 0]], which satisfies the below
313+
// v = "$" would produce [[0 0] [1 1]], which would fail the below
314+
// v = "abc$" would produce [[0 3] [4 4]], which would fail the below
315+
return len(matches) == 1 && matches[0][0] == 0 && matches[0][1] == len(v)
316+
}

‎npm/pkg/controlplane/translation/parseSelector_test.go‎

Lines changed: 169 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,27 @@
11
package translation
22

33
import (
4+
"fmt"
45
"reflect"
56
"testing"
67

8+
"github.com/stretchr/testify/require"
79
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
810
)
911

1012
func TestFlattenNameSpaceSelectorCases(t *testing.T) {
1113
firstSelector := &metav1.LabelSelector{}
1214

13-
testSelectors := flattenNameSpaceSelector(firstSelector)
15+
testSelectors, err := flattenNameSpaceSelector(firstSelector)
16+
require.Nil(t, err)
1417
if len(testSelectors) != 1 {
1518
t.Errorf("TestFlattenNameSpaceSelectorCases failed @ 1st selector length check %+v", testSelectors)
1619
}
1720

1821
var secondSelector *metav1.LabelSelector
1922

20-
testSelectors = flattenNameSpaceSelector(secondSelector)
23+
testSelectors, err = flattenNameSpaceSelector(secondSelector)
24+
require.Nil(t, err)
2125
if len(testSelectors) > 0 {
2226
t.Errorf("TestFlattenNameSpaceSelectorCases failed @ 1st selector length check %+v", testSelectors)
2327
}
@@ -61,7 +65,8 @@ func TestFlattenNameSpaceSelector(t *testing.T) {
6165
MatchLabels: commonMatchLabel,
6266
}
6367

64-
testSelectors := flattenNameSpaceSelector(firstSelector)
68+
testSelectors, err := flattenNameSpaceSelector(firstSelector)
69+
require.Nil(t, err)
6570
if len(testSelectors) != 1 {
6671
t.Errorf("TestFlattenNameSpaceSelector failed @ 1st selector length check %+v", testSelectors)
6772
}
@@ -105,7 +110,8 @@ func TestFlattenNameSpaceSelector(t *testing.T) {
105110
MatchLabels: commonMatchLabel,
106111
}
107112

108-
testSelectors = flattenNameSpaceSelector(secondSelector)
113+
testSelectors, err = flattenNameSpaceSelector(secondSelector)
114+
require.Nil(t, err)
109115
if len(testSelectors) != 8 {
110116
t.Errorf("TestFlattenNameSpaceSelector failed @ 2nd selector length check %+v", testSelectors)
111117
}
@@ -399,7 +405,8 @@ func TestFlattenNameSpaceSelectorWoMatchLabels(t *testing.T) {
399405
},
400406
}
401407

402-
testSelectors := flattenNameSpaceSelector(firstSelector)
408+
testSelectors, err := flattenNameSpaceSelector(firstSelector)
409+
require.Nil(t, err)
403410
if len(testSelectors) != 2 {
404411
t.Errorf("TestFlattenNameSpaceSelector failed @ 1st selector length check %+v", testSelectors)
405412
}
@@ -471,3 +478,160 @@ func TestFlattenNameSpaceSelectorWoMatchLabels(t *testing.T) {
471478
t.Errorf("TestFlattenNameSpaceSelector failed @ 1st selector deepEqual check.\n Expected: %+v \n Actual: %+v", expectedSelectors, testSelectors)
472479
}
473480
}
481+
482+
func TestFlattenNamespaceSelectorError(t *testing.T) {
483+
tests := []struct {
484+
name string
485+
selector *metav1.LabelSelector
486+
wantErr bool
487+
}{
488+
{
489+
name: "good alphanumeric with hyphen",
490+
selector: &metav1.LabelSelector{
491+
MatchExpressions: []metav1.LabelSelectorRequirement{
492+
{
493+
Key: "testIn",
494+
Operator: metav1.LabelSelectorOpIn,
495+
Values: []string{
496+
"good",
497+
"good-1",
498+
"good2-too",
499+
},
500+
},
501+
{
502+
Key: "testNotIn",
503+
Operator: metav1.LabelSelectorOpNotIn,
504+
Values: []string{
505+
"good",
506+
"good-1",
507+
"good2-too",
508+
},
509+
},
510+
},
511+
},
512+
wantErr: false,
513+
},
514+
{
515+
name: "bad in",
516+
selector: &metav1.LabelSelector{
517+
MatchExpressions: []metav1.LabelSelectorRequirement{
518+
{
519+
Key: "testIn",
520+
Operator: metav1.LabelSelectorOpIn,
521+
Values: []string{
522+
"good-1",
523+
"bad$",
524+
},
525+
},
526+
},
527+
},
528+
wantErr: true,
529+
},
530+
{
531+
name: "bad not in",
532+
selector: &metav1.LabelSelector{
533+
MatchExpressions: []metav1.LabelSelectorRequirement{
534+
{
535+
Key: "testNotIn",
536+
Operator: metav1.LabelSelectorOpIn,
537+
Values: []string{
538+
"bad$",
539+
"good-1",
540+
},
541+
},
542+
},
543+
},
544+
wantErr: true,
545+
},
546+
{
547+
name: "good and bad",
548+
selector: &metav1.LabelSelector{
549+
MatchExpressions: []metav1.LabelSelectorRequirement{
550+
{
551+
Key: "testIn",
552+
Operator: metav1.LabelSelectorOpIn,
553+
Values: []string{
554+
"good-1",
555+
},
556+
},
557+
{
558+
Key: "testNotIn",
559+
Operator: metav1.LabelSelectorOpIn,
560+
Values: []string{
561+
"bad$",
562+
"good-1",
563+
},
564+
},
565+
},
566+
},
567+
wantErr: true,
568+
},
569+
{
570+
name: "bad with space",
571+
selector: &metav1.LabelSelector{
572+
MatchExpressions: []metav1.LabelSelectorRequirement{
573+
{
574+
Key: "testIn",
575+
Operator: metav1.LabelSelectorOpIn,
576+
Values: []string{
577+
"bad space",
578+
"good-1",
579+
},
580+
},
581+
},
582+
},
583+
wantErr: true,
584+
},
585+
}
586+
587+
for i, tt := range tests {
588+
tt := tt
589+
t.Run(fmt.Sprintf("test %d", i), func(t *testing.T) {
590+
s, err := flattenNameSpaceSelector(tt.selector)
591+
if tt.wantErr {
592+
require.Error(t, err)
593+
require.Nil(t, s)
594+
} else {
595+
require.NoError(t, err)
596+
require.NotNil(t, s)
597+
}
598+
})
599+
}
600+
}
601+
602+
func TestIsValidLabel(t *testing.T) {
603+
good := []string{
604+
"",
605+
"1",
606+
"abc",
607+
"ABC",
608+
"abc1",
609+
"ABC1",
610+
"abc-1",
611+
"ABC-1",
612+
"ABC_1",
613+
"ABC_-a54--f",
614+
}
615+
616+
for _, g := range good {
617+
require.True(t, isValidLabelValue(g), "string was [%s]", g)
618+
}
619+
620+
bad := []string{
621+
"-",
622+
"_",
623+
"$",
624+
" ",
625+
"abc-",
626+
"abc$",
627+
"abc$123",
628+
"bad space",
629+
"end-with-hyphen-",
630+
"end-with-underscore_",
631+
"end-with-space ",
632+
}
633+
634+
for _, b := range bad {
635+
require.False(t, isValidLabelValue(b), "string was [%s]", b)
636+
}
637+
}

‎npm/pkg/controlplane/translation/translatePolicy.go‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ var (
2828
ErrUnsupportedExceptCIDR = errors.New("unsupported Except CIDR block translation features used on windows")
2929
// ErrUnsupportedSCTP is returned when SCTP protocol is used in windows.
3030
ErrUnsupportedSCTP = errors.New("unsupported SCTP protocol used on windows")
31+
// ErrInvalidMatchExpressionValues ensures proper matchExpression label values since k8s doesn't perform this check.
32+
ErrInvalidMatchExpressionValues = errors.New(
33+
"matchExpression label values must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character",
34+
)
3135
)
3236

3337
type podSelectorResult struct {
@@ -407,7 +411,11 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, netPolName string, dire
407411
if peer.PodSelector == nil && peer.NamespaceSelector != nil {
408412
// Before translating NamespaceSelector, flattenNameSpaceSelector function call should be called
409413
// to handle multiple values in matchExpressions spec.
410-
flattenNSSelector := flattenNameSpaceSelector(peer.NamespaceSelector)
414+
flattenNSSelector, err := flattenNameSpaceSelector(peer.NamespaceSelector)
415+
if err != nil {
416+
return err
417+
}
418+
411419
for i := range flattenNSSelector {
412420
nsSelectorIPSets, nsSelectorList := nameSpaceSelector(matchType, &flattenNSSelector[i])
413421
npmNetPol.RuleIPSets = append(npmNetPol.RuleIPSets, nsSelectorIPSets...)
@@ -444,7 +452,11 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, netPolName string, dire
444452

445453
// Before translating NamespaceSelector, flattenNameSpaceSelector function call should be called
446454
// to handle multiple values in matchExpressions spec.
447-
flattenNSSelector := flattenNameSpaceSelector(peer.NamespaceSelector)
455+
flattenNSSelector, err := flattenNameSpaceSelector(peer.NamespaceSelector)
456+
if err != nil {
457+
return err
458+
}
459+
448460
for i := range flattenNSSelector {
449461
nsSelectorIPSets, nsSelectorList := nameSpaceSelector(matchType, &flattenNSSelector[i])
450462
npmNetPol.RuleIPSets = append(npmNetPol.RuleIPSets, nsSelectorIPSets...)

0 commit comments

Comments
 (0)