Skip to content

fix: [NPM] add NetworkPolicy validation for matchExpression values #1717

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 19, 2023
Merged

fix: [NPM] add NetworkPolicy validation for matchExpression values #1717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ec3a219
add validation for matchExpression values
huntergregory Dec 12, 2022
0a664c7
fix lint
huntergregory Dec 12, 2022
572257f
make regex identical to kubectl validation and include more test cases
huntergregory Dec 13, 2022
c7869d4
remove debugging lines
huntergregory Dec 14, 2022
0c437d7
Merge branch 'master' into hgregory/bad-iptables-arg
huntergregory Jan 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 35 additions & 5 deletions npm/pkg/controlplane/translation/parseSelector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand 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)
Expand All @@ -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
Expand 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
Expand Down Expand Up @@ -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])
Expand Down Expand Up @@ -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)
}
174 changes: 169 additions & 5 deletions npm/pkg/controlplane/translation/parseSelector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
}
16 changes: 14 additions & 2 deletions npm/pkg/controlplane/translation/translatePolicy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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...)
Expand Down Expand Up @@ -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...)
Expand Down
Loading