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

Revert Implement pathType validation (#9511) (#9607)

Browse source 
Signed-off-by: James Strong <strong.james.e@gmail.com>
This commit is contained in:
James Strong 2023-02-13 07:57:29 +01:00 • committed by GitHub
parent 59d247dd74
commit 01c9a2bf25
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
16 changed files with 211 additions and 426 deletions
Download patch file Download diff file Expand all files Collapse all files

14
internal/ingress/controller/config/config.go
View file

@ -782,18 +782,6 @@ type Configuration struct {
// http://nginx.org/en/docs/ngx_core_module.html#debug_connection
// Default: ""
DebugConnections []string `json:"debug-connections"`
// EnablePathTypeValidation allows the admin to enable the pathType validation.
// If EnablePathTypeValidation is enabled, the Controller will only allow alphanumeric
// characters on path (0-9, a-z, A-Z, "-", ".", "_", "~", "/")
// to control what characters are allowed set them with PathAdditionalAllowedChars
EnablePathTypeValidation bool `json:"enable-pathtype-validation"`
// PathAdditionalAllowedChars allows the admin to specify what are the additional
// characters allowed in case of pathType=ImplementationSpecific.
// Case enable-pathtype-validation=true, this characters will be only allowed on ImplementationSpecific.
// Defaults to: "^%$[](){}*+?"
PathAdditionalAllowedChars string `json:"path-additional-allowed-chars"`
}
// NewDefault returns the default nginx configuration
@ -829,8 +817,6 @@ func NewDefault() Configuration {
ClientHeaderTimeout: 60,
ClientBodyBufferSize: "8k",
ClientBodyTimeout: 60,
EnablePathTypeValidation: false,
PathAdditionalAllowedChars: "^%$[](){}*+?|",
EnableUnderscoresInHeaders: false,
ErrorLogLevel: errorLevel,
UseForwardedHeaders: false,

4
internal/ingress/controller/controller.go
View file

@ -325,10 +325,6 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
k8s.SetDefaultNGINXPathType(ing)
if err := utilingress.ValidateIngressPath(ing, cfg.EnablePathTypeValidation, cfg.PathAdditionalAllowedChars); err != nil {
return fmt.Errorf("ingress contains invalid characters: %s", err)
}
allIngresses := n.store.ListIngresses()
filter := func(toCheck *ingress.Ingress) bool {

91
internal/ingress/controller/controller_test.go
View file

@ -201,97 +201,6 @@ func TestCheckIngress(t *testing.T) {
},
},
}
t.Run("when validating pathType", func(t *testing.T) {
t.Run("When ingress contains invalid path and pathType validation is enabled", func(t *testing.T) {
nginx.store = fakeIngressStore{
ingresses: []*ingress.Ingress{},
configuration: ngx_config.Configuration{
EnablePathTypeValidation: true,
},
}
nginx.command = testNginxTestCommand{
t: t,
err: nil,
expected: "",
}
nginx.cfg.IngressClassConfiguration = &ingressclass.IngressClassConfiguration{
WatchWithoutClass: true,
}
ingPath := &networking.Ingress{
ObjectMeta: metav1.ObjectMeta{
Name: "test-ingress1",
Namespace: "user-namespace1",
Annotations: map[string]string{
"kubernetes.io/ingress.class": "nginx",
},
},
Spec: networking.IngressSpec{
Rules: []networking.IngressRule{
{
Host: "example.com",
IngressRuleValue: networking.IngressRuleValue{
HTTP: &networking.HTTPIngressRuleValue{
Paths: []networking.HTTPIngressPath{
{
Path: "/xpto/(a+)",
PathType: &pathTypePrefix,
},
},
},
},
},
},
},
}
if nginx.CheckIngress(ingPath) == nil {
t.Errorf("invalid path on pathTypePrefix and validation enabled should return an error")
}
})
t.Run("When ingress contains invalid path and pathType validation is disabled", func(t *testing.T) {
nginx.store = fakeIngressStore{
ingresses: []*ingress.Ingress{},
configuration: ngx_config.Configuration{
EnablePathTypeValidation: false,
PathAdditionalAllowedChars: "^%$[](){}*+?|",
},
}
nginx.command = testNginxTestCommand{
t: t,
err: nil,
expected: "_,example.com",
}
ingPath := &networking.Ingress{
ObjectMeta: metav1.ObjectMeta{
Name: "test-ingress2",
Namespace: "user-namespace2",
},
Spec: networking.IngressSpec{
Rules: []networking.IngressRule{
{
Host: "example.com",
IngressRuleValue: networking.IngressRuleValue{
HTTP: &networking.HTTPIngressRuleValue{
Paths: []networking.HTTPIngressPath{
{
Path: "/example(/|$)(.*)",
PathType: &pathTypePrefix,
},
},
},
},
},
},
},
}
if nginx.CheckIngress(ingPath) != nil {
t.Errorf("invalid path on pathTypePrefix and validation disabled should not return an error: %s", nginx.CheckIngress(ingPath))
}
})
})
t.Run("when the class is the nginx one", func(t *testing.T) {
ing.ObjectMeta.Annotations["kubernetes.io/ingress.class"] = "nginx"
nginx.command = testNginxTestCommand{

9
internal/ingress/controller/store/store.go
View file

@ -846,11 +846,6 @@ func (s *k8sStore) syncIngress(ing *networkingv1.Ingress) {
copyIng := &networkingv1.Ingress{}
ing.ObjectMeta.DeepCopyInto(&copyIng.ObjectMeta)
if err := ingressutils.ValidateIngressPath(ing, s.backendConfig.EnablePathTypeValidation, s.backendConfig.PathAdditionalAllowedChars); err != nil {
klog.Errorf("ingress %s contains invalid path and will be skipped: %s", key, err)
return
}
if s.backendConfig.AnnotationValueWordBlocklist != "" {
if err := checkBadAnnotationValue(copyIng.Annotations, s.backendConfig.AnnotationValueWordBlocklist); err != nil {
klog.Warningf("skipping ingress %s: %s", key, err)
@ -870,6 +865,10 @@ func (s *k8sStore) syncIngress(ing *networkingv1.Ingress) {
if path.Path == "" {
copyIng.Spec.Rules[ri].HTTP.Paths[pi].Path = "/"
}
if !ingressutils.IsSafePath(copyIng, path.Path) {
klog.Warningf("ingress %s contains invalid path %s", key, path.Path)
return
}
}
}

3
internal/k8s/main.go
View file

@ -180,6 +180,9 @@ func SetDefaultNGINXPathType(ing *networkingv1.Ingress) {
p.PathType = &defaultPathType
}
if *p.PathType == networkingv1.PathTypeImplementationSpecific {
p.PathType = &defaultPathType
}
}
}
}