DevFW-CICD/ingress-nginx-helm
6
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Switch logic on path type validation and setting it to false (#9543)

Browse source 
* update path type validation to be false and update e2e test scripts

Signed-off-by: James Strong <strong.james.e@gmail.com>

* update to make tests clear

Signed-off-by: James Strong <strong.james.e@gmail.com>

* update test params

Signed-off-by: James Strong <strong.james.e@gmail.com>

* Adding else per pr comments

Signed-off-by: James Strong <james.strong@chainguard.dev>

---------

Signed-off-by: James Strong <strong.james.e@gmail.com>
Signed-off-by: James Strong <james.strong@chainguard.dev>
This commit is contained in:
James Strong 2023-01-31 20:09:06 -05:00 • committed by GitHub
parent f90f37bed6
commit 7d1c47ab54
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
14 changed files with 354 additions and 319 deletions
Download patch file Download diff file Expand all files Collapse all files

43
pkg/util/ingress/ingress.go
View file

@ -246,44 +246,65 @@ func BuildRedirects(servers []*ingress.Server) []*redirect {
return redirectServers
}
func ValidateIngressPath(copyIng *networkingv1.Ingress, disablePathTypeValidation bool, additionalChars string) error {
func ValidateIngressPath(copyIng *networkingv1.Ingress, enablePathTypeValidation bool, pathAdditionalAllowedChars string) error {
if copyIng == nil {
return nil
}
escapedAdditionalChars := regexp.QuoteMeta(additionalChars)
regexPath, err := regexp.Compile("^[" + alphaNumericChars + escapedAdditionalChars + "]*$")
escapedPathAdditionalAllowedChars := regexp.QuoteMeta(pathAdditionalAllowedChars)
regexPath, err := regexp.Compile("^[" + alphaNumericChars + escapedPathAdditionalAllowedChars + "]*$")
if err != nil {
return fmt.Errorf("ingress has misconfigured validation regex on configmap: %s - %w", additionalChars, err)
return fmt.Errorf("ingress has misconfigured validation regex on configmap: %s - %w", pathAdditionalAllowedChars, err)
}
for _, rule := range copyIng.Spec.Rules {
if rule.HTTP == nil {
continue
}
if err := checkPath(rule.HTTP.Paths, disablePathTypeValidation, regexPath); err != nil {
if err := checkPath(rule.HTTP.Paths, enablePathTypeValidation, regexPath); err != nil {
return fmt.Errorf("error validating ingressPath: %w", err)
}
}
return nil
}
func checkPath(paths []networkingv1.HTTPIngressPath, disablePathTypeValidation bool, regexSpecificChars *regexp.Regexp) error {
func checkPath(paths []networkingv1.HTTPIngressPath, enablePathTypeValidation bool, regexSpecificChars *regexp.Regexp) error {
for _, path := range paths {
if path.PathType == nil {
path.PathType = &defaultPathType
}
if disablePathTypeValidation || *path.PathType == networkingv1.PathTypeImplementationSpecific {
klog.V(9).InfoS("PathType Validation", "enablePathTypeValidation", enablePathTypeValidation, "regexSpecificChars", regexSpecificChars.String(), "Path", path.Path)
switch pathType := *path.PathType; pathType {
case networkingv1.PathTypeImplementationSpecific:
//only match on regex chars per Ingress spec when path is implementation specific
if !regexSpecificChars.MatchString(path.Path) {
return fmt.Errorf("path %s of type %s contains invalid characters", path.Path, *path.PathType)
}
continue
}
if !pathAlphaNumericRegex(path.Path) {
return fmt.Errorf("path %s of type %s contains invalid characters", path.Path, *path.PathType)
case networkingv1.PathTypeExact, networkingv1.PathTypePrefix:
//enforce path type validation
if enablePathTypeValidation {
//only allow alphanumeric chars, no regex chars
if !pathAlphaNumericRegex(path.Path) {
return fmt.Errorf("path %s of type %s contains invalid characters", path.Path, *path.PathType)
}
continue
} else {
//path validation is disabled, so we check what regex chars are allowed by user
if !regexSpecificChars.MatchString(path.Path) {
return fmt.Errorf("path %s of type %s contains invalid characters", path.Path, *path.PathType)
}
continue
}
default:
return fmt.Errorf("unknown path type %v on path %v", *path.PathType, path.Path)
}
}
return nil

66
pkg/util/ingress/ingress_test.go
View file

@ -212,11 +212,11 @@ const (
func TestValidateIngressPath(t *testing.T) {
tests := []struct {
name string
copyIng *networkingv1.Ingress
disablePathTypeValidation bool
additionalChars string
wantErr bool
name string
copyIng *networkingv1.Ingress
EnablePathTypeValidation bool
additionalChars string
wantErr bool
}{
{
name: "should return nil when ingress = nil",
@ -251,49 +251,53 @@ func TestValidateIngressPath(t *testing.T) {
{
name: "should deny path with bad characters and pathType not implementationSpecific",
wantErr: true,
additionalChars: defaultAdditionalChars,
additionalChars: "()",
copyIng: generateDumbIngressforPathTest(&pathTypeExact, "/foo/bar/(.+)"),
},
{
name: "should accept path with regex characters and pathType implementationSpecific",
wantErr: false,
additionalChars: defaultAdditionalChars,
copyIng: generateDumbIngressforPathTest(&pathTypeImplSpecific, "/foo/bar/(.+)"),
name: "should accept path with regex characters and pathType implementationSpecific",
wantErr: false,
additionalChars: defaultAdditionalChars,
EnablePathTypeValidation: false,
copyIng: generateDumbIngressforPathTest(&pathTypeImplSpecific, "/foo/bar/(.+)"),
},
{
name: "should accept path with regex characters and pathType exact, but pathType validation disabled",
wantErr: false,
additionalChars: defaultAdditionalChars,
disablePathTypeValidation: true,
copyIng: generateDumbIngressforPathTest(&pathTypeImplSpecific, "/foo/bar/(.+)"),
name: "should accept path with regex characters and pathType exact, but pathType validation disabled",
wantErr: false,
additionalChars: defaultAdditionalChars,
EnablePathTypeValidation: false,
copyIng: generateDumbIngressforPathTest(&pathTypeExact, "/foo/bar/(.+)"),
},
{
name: "should reject path when the allowed additional set does not match",
wantErr: true,
additionalChars: "().?",
disablePathTypeValidation: false,
copyIng: generateDumbIngressforPathTest(&pathTypeImplSpecific, "/foo/bar/(.+)"),
name: "should reject path when the allowed additional set does not match",
wantErr: true,
additionalChars: "().?",
EnablePathTypeValidation: false,
copyIng: generateDumbIngressforPathTest(&pathTypeImplSpecific, "/foo/bar/(.+)"),
},
{
name: "should accept path when the allowed additional set does match",
wantErr: false,
additionalChars: "().?",
copyIng: generateDumbIngressforPathTest(&pathTypeImplSpecific, "/foo/bar/(.?)"),
name: "should accept path when the allowed additional set does match",
wantErr: false,
additionalChars: "().?",
EnablePathTypeValidation: false,
copyIng: generateDumbIngressforPathTest(&pathTypeImplSpecific, "/foo/bar/(.?)"),
},
{
name: "should block if at least one path is bad",
wantErr: true,
copyIng: generateComplexIngress(generateDumbIngressforPathTest(&pathTypeExact, "/foo/bar/(.?)")),
name: "should block if at least one path is bad",
wantErr: true,
EnablePathTypeValidation: false,
copyIng: generateComplexIngress(generateDumbIngressforPathTest(&pathTypeExact, "/foo/bar/(.?)")),
},
{
name: "should block if at least one path is bad",
wantErr: true,
copyIng: generateComplexIngress(generateDumbIngressforPathTest(&pathTypeImplSpecific, "/foo/bar/(.?)")),
name: "should block if at least one path is bad",
wantErr: true,
EnablePathTypeValidation: false,
copyIng: generateComplexIngress(generateDumbIngressforPathTest(&pathTypeImplSpecific, "/foo/bar/(.?)")),
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if err := ValidateIngressPath(tt.copyIng, tt.disablePathTypeValidation, tt.additionalChars); (err != nil) != tt.wantErr {
if err := ValidateIngressPath(tt.copyIng, tt.EnablePathTypeValidation, tt.additionalChars); (err != nil) != tt.wantErr {
t.Errorf("ValidateIngressPath() error = %v, wantErr %v", err, tt.wantErr)
}
})