Validate ingress path fields (#9309)

* Validate characters in path fields

* Add e2e tests for path validation

* Fix review comments
This commit is contained in:
Ricardo Katz 2022-11-17 09:24:40 -03:00 committed by GitHub
parent b6c6305523
commit c1413e6079
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 249 additions and 0 deletions

View file

@ -18,15 +18,30 @@ package ingress
import (
"fmt"
"regexp"
"strings"
networkingv1 "k8s.io/api/networking/v1"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
"k8s.io/ingress-nginx/internal/k8s"
"k8s.io/ingress-nginx/internal/net/ssl"
"k8s.io/ingress-nginx/pkg/apis/ingress"
"k8s.io/klog/v2"
)
const (
alphaNumericChars = `\-\.\_\~a-zA-Z0-9/`
regexEnabledChars = `\^\$\[\]\(\)\{\}\*\+`
)
var (
// pathAlphaNumeric is a regex validation of something like "^/[a-zA-Z]+$" on path
pathAlphaNumeric = regexp.MustCompile("^/[" + alphaNumericChars + "]*$").MatchString
// pathRegexEnabled is a regex validation of paths that may contain regex.
pathRegexEnabled = regexp.MustCompile("^/[" + alphaNumericChars + regexEnabledChars + "]*$").MatchString
)
func GetRemovedHosts(rucfg, newcfg *ingress.Configuration) []string {
oldSet := sets.NewString()
newSet := sets.NewString()
@ -231,3 +246,13 @@ func BuildRedirects(servers []*ingress.Server) []*redirect {
return redirectServers
}
// IsSafePath verifies if the path used in ingress object contains only valid characters.
// It will behave differently if regex is enabled or not
func IsSafePath(copyIng *networkingv1.Ingress, path string) bool {
isRegex, _ := parser.GetBoolAnnotation("use-regex", copyIng)
if isRegex {
return pathRegexEnabled(path)
}
return pathAlphaNumeric(path)
}

View file

@ -17,8 +17,13 @@ limitations under the License.
package ingress
import (
"fmt"
"testing"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
networkingv1 "k8s.io/api/networking/v1"
"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
"k8s.io/ingress-nginx/pkg/apis/ingress"
)
@ -130,3 +135,83 @@ func TestIsDynamicConfigurationEnough(t *testing.T) {
t.Errorf("Expected new config to not change")
}
}
func generateDumbIngressforPathTest(regexEnabled bool) *networkingv1.Ingress {
var annotations = make(map[string]string)
regexAnnotation := fmt.Sprintf("%s/use-regex", parser.AnnotationsPrefix)
if regexEnabled {
annotations[regexAnnotation] = "true"
}
return &networkingv1.Ingress{
ObjectMeta: metav1.ObjectMeta{
Name: "dumb",
Namespace: "default",
Annotations: annotations,
},
}
}
func TestIsSafePath(t *testing.T) {
tests := []struct {
name string
copyIng *networkingv1.Ingress
path string
want bool
}{
{
name: "should accept valid path with regex disabled",
want: true,
copyIng: generateDumbIngressforPathTest(false),
path: "/xpto/~user/t-e_st.exe",
},
{
name: "should accept valid path / with regex disabled",
want: true,
copyIng: generateDumbIngressforPathTest(false),
path: "/",
},
{
name: "should reject invalid path with invalid chars",
want: false,
copyIng: generateDumbIngressforPathTest(false),
path: "/foo/bar/;xpto",
},
{
name: "should reject regex path when regex is disabled",
want: false,
copyIng: generateDumbIngressforPathTest(false),
path: "/foo/bar/(.+)",
},
{
name: "should accept valid path / with regex enabled",
want: true,
copyIng: generateDumbIngressforPathTest(true),
path: "/",
},
{
name: "should accept regex path when regex is enabled",
want: true,
copyIng: generateDumbIngressforPathTest(true),
path: "/foo/bar/(.+)",
},
{
name: "should reject regex path when regex is enabled but the path is invalid",
want: false,
copyIng: generateDumbIngressforPathTest(true),
path: "/foo/bar/;xpto",
},
{
name: "should reject regex path when regex is enabled but the path is invalid",
want: false,
copyIng: generateDumbIngressforPathTest(true),
path: ";xpto",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := IsSafePath(tt.copyIng, tt.path); got != tt.want {
t.Errorf("IsSafePath() = %v, want %v", got, tt.want)
}
})
}
}