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

Add option to sanitize annotation inputs (#7874)

Browse source 
* Add option to sanitize annotation inputs

* Fix e2e tests after string sanitization

* Add proxy_pass and serviceaccount as denied values
This commit is contained in:
Ricardo Katz 2021-11-12 16:40:30 -03:00 committed by GitHub
parent 8333c8c127
commit 67e13bf692
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 283 additions and 16 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
internal/ingress/annotations/parser/main_test.go
View file

@ -116,6 +116,12 @@ rewrite (?i)/arcgis/services/Utilities/Geometry/GeometryServer(.*)$ /arcgis/serv
}
continue
}
if !test.expErr {
if err != nil {
t.Errorf("%v: didn't expected error but error was returned: %v", test.name, err)
}
continue
}
if s != test.exp {
t.Errorf("%v: expected \"%v\" but \"%v\" was returned", test.name, test.exp, s)
}

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

@ -18,6 +18,7 @@ package config
import (
"strconv"
"strings"
"time"
"k8s.io/klog/v2"
@ -97,6 +98,11 @@ type Configuration struct {
// If disabled, only snippets added via ConfigMap are added to ingress.
AllowSnippetAnnotations bool `json:"allow-snippet-annotations"`
// AnnotationValueWordBlocklist defines words that should not be part of an user annotation value
// (can be used to run arbitrary code or configs, for example) and that should be dropped.
// This list should be separated by "," character
AnnotationValueWordBlocklist string `json:"annotation-value-word-blocklist"`
// Sets the name of the configmap that contains the headers to pass to the client
AddHeaders string `json:"add-headers,omitempty"`
@ -762,6 +768,20 @@ func NewDefault() Configuration {
defNginxStatusIpv6Whitelist := make([]string, 0)
defResponseHeaders := make([]string, 0)
defAnnotationValueWordBlocklist := []string{
"load_module",
"lua_package",
"_by_lua",
"location",
"root",
"proxy_pass",
"serviceaccount",
"{",
"}",
"'",
"\\",
}
defIPCIDR = append(defIPCIDR, "0.0.0.0/0")
defNginxStatusIpv4Whitelist = append(defNginxStatusIpv4Whitelist, "127.0.0.1")
defNginxStatusIpv6Whitelist = append(defNginxStatusIpv6Whitelist, "::1")
@ -772,6 +792,7 @@ func NewDefault() Configuration {
AllowSnippetAnnotations: true,
AllowBackendServerHeader: false,
AnnotationValueWordBlocklist: strings.Join(defAnnotationValueWordBlocklist, ","),
AccessLogPath: "/var/log/nginx/access.log",
AccessLogParams: "",
EnableAccessLogForDefaultBackend: false,

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

@ -239,12 +239,22 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
cfg := n.store.GetBackendConfiguration()
cfg.Resolver = n.resolver
for key := range ing.ObjectMeta.GetAnnotations() {
arraybadWords := strings.Split(strings.TrimSpace(cfg.AnnotationValueWordBlocklist), ",")
for key, value := range ing.ObjectMeta.GetAnnotations() {
if parser.AnnotationsPrefix != parser.DefaultAnnotationsPrefix {
if strings.HasPrefix(key, fmt.Sprintf("%s/", parser.DefaultAnnotationsPrefix)) {
return fmt.Errorf("This deployment has a custom annotation prefix defined. Use '%s' instead of '%s'", parser.AnnotationsPrefix, parser.DefaultAnnotationsPrefix)
}
}
if strings.HasPrefix(key, fmt.Sprintf("%s/", parser.AnnotationsPrefix)) {
for _, forbiddenvalue := range arraybadWords {
if strings.Contains(value, forbiddenvalue) {
return fmt.Errorf("%s annotation contains invalid word %s", key, forbiddenvalue)
}
}
}
if !cfg.AllowSnippetAnnotations && strings.HasSuffix(key, "-snippet") {
return fmt.Errorf("%s annotation cannot be used. Snippet directives are disabled by the Ingress administrator", key)

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

@ -268,6 +268,23 @@ func TestCheckIngress(t *testing.T) {
}
})
t.Run("When invalid directives are used in annotation values", func(t *testing.T) {
nginx.store = fakeIngressStore{
ingresses: []*ingress.Ingress{},
configuration: ngx_config.Configuration{
AnnotationValueWordBlocklist: "invalid_directive, another_directive",
},
}
nginx.command = testNginxTestCommand{
t: t,
err: nil,
}
ing.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/custom-headers"] = "invalid_directive"
if err := nginx.CheckIngress(ing); err == nil {
t.Errorf("with an invalid value in annotation the ingress should be rejected")
}
})
t.Run("When a new catch-all ingress is being created despite catch-alls being disabled ", func(t *testing.T) {
backendBefore := ing.Spec.DefaultBackend
disableCatchAllBefore := nginx.cfg.DisableCatchAll

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

@ -23,6 +23,7 @@ import (
"os"
"reflect"
"sort"
"strings"
"sync"
"time"
@ -734,6 +735,21 @@ func hasCatchAllIngressRule(spec networkingv1.IngressSpec) bool {
return spec.DefaultBackend != nil
}
func checkBadAnnotationValue(annotations map[string]string, badwords string) error {
arraybadWords := strings.Split(strings.TrimSpace(badwords), ",")
for annotation, value := range annotations {
if strings.HasPrefix(annotation, fmt.Sprintf("%s/", parser.AnnotationsPrefix)) {
for _, forbiddenvalue := range arraybadWords {
if strings.Contains(value, forbiddenvalue) {
return fmt.Errorf("%s annotation contains invalid word %s", annotation, forbiddenvalue)
}
}
}
}
return nil
}
// syncIngress parses ingress annotations converting the value of the
// annotation to a go struct
func (s *k8sStore) syncIngress(ing *networkingv1.Ingress) {
@ -742,6 +758,13 @@ func (s *k8sStore) syncIngress(ing *networkingv1.Ingress) {
copyIng := &networkingv1.Ingress{}
ing.ObjectMeta.DeepCopyInto(&copyIng.ObjectMeta)
klog.Errorf("Blocklist: %v", s.backendConfig.AnnotationValueWordBlocklist)
if err := checkBadAnnotationValue(copyIng.Annotations, s.backendConfig.AnnotationValueWordBlocklist); err != nil {
klog.Errorf("skipping ingress %s: %s", key, err)
return
}
ing.Spec.DeepCopyInto(&copyIng.Spec)
ing.Status.DeepCopyInto(&copyIng.Status)

22
test/e2e/admission/admission.go
View file

@ -121,6 +121,28 @@ var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() {
assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid configuration should return an error")
})
ginkgo.It("should return an error if there is an invalid value in some annotation", func() {
host := "admission-test"
annotations := map[string]string{
"nginx.ingress.kubernetes.io/connection-proxy-header": "a;}",
}
firstIngress := framework.NewSingleIngress("first-ingress", "/", host, f.Namespace, framework.EchoService, 80, annotations)
_, err := f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), firstIngress, metav1.CreateOptions{})
assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid annotation value should return an error")
})
ginkgo.It("should return an error if there is a forbidden value in some annotation", func() {
host := "admission-test"
annotations := map[string]string{
"nginx.ingress.kubernetes.io/connection-proxy-header": "set_by_lua",
}
firstIngress := framework.NewSingleIngress("first-ingress", "/", host, f.Namespace, framework.EchoService, 80, annotations)
_, err := f.KubeClientSet.NetworkingV1().Ingresses(f.Namespace).Create(context.TODO(), firstIngress, metav1.CreateOptions{})
assert.NotNil(ginkgo.GinkgoT(), err, "creating an ingress with invalid annotation value should return an error")
})
ginkgo.It("should not return an error if the Ingress V1 definition is valid with Ingress Class", func() {
err := createIngress(f.Namespace, validV1Ingress)
assert.Nil(ginkgo.GinkgoT(), err, "creating an ingress using kubectl")

5
test/e2e/annotations/globalratelimit.go
View file

@ -40,6 +40,11 @@ var _ = framework.DescribeAnnotation("annotation-global-rate-limit", func() {
annotations["nginx.ingress.kubernetes.io/global-rate-limit"] = "5"
annotations["nginx.ingress.kubernetes.io/global-rate-limit-window"] = "2m"
// We need to allow { and } characters for this annotation to work
f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "load_module, lua_package, _by_lua, location, root")
// Sleep a while just to guarantee that the configmap is applied
framework.Sleep()
ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations)
ing = f.EnsureIngress(ing)
namespace := strings.Replace(string(ing.UID), "-", "", -1)

20
test/e2e/annotations/modsecurity/modsecurity.go
View file

@ -165,7 +165,9 @@ var _ = framework.DescribeAnnotation("modsecurity owasp", func() {
"nginx.ingress.kubernetes.io/enable-modsecurity": "true",
"nginx.ingress.kubernetes.io/modsecurity-snippet": snippet,
}
f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "load_module, lua_package, _by_lua, location, root, {, }")
// Sleep a while just to guarantee that the configmap is applied
framework.Sleep()
ing := framework.NewSingleIngress(host, "/", host, nameSpace, framework.EchoService, 80, annotations)
f.EnsureIngress(ing)
@ -198,7 +200,9 @@ var _ = framework.DescribeAnnotation("modsecurity owasp", func() {
annotations := map[string]string{
"nginx.ingress.kubernetes.io/modsecurity-snippet": snippet,
}
f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "load_module, lua_package, _by_lua, location, root, {, }")
// Sleep a while just to guarantee that the configmap is applied
framework.Sleep()
ing := framework.NewSingleIngress(host, "/", host, nameSpace, framework.EchoService, 80, annotations)
f.EnsureIngress(ing)
@ -232,7 +236,9 @@ var _ = framework.DescribeAnnotation("modsecurity owasp", func() {
annotations := map[string]string{
"nginx.ingress.kubernetes.io/modsecurity-snippet": snippet,
}
f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "load_module, lua_package, _by_lua, location, root, {, }")
// Sleep a while just to guarantee that the configmap is applied
framework.Sleep()
ing := framework.NewSingleIngress(host, "/", host, nameSpace, framework.EchoService, 80, annotations)
f.EnsureIngress(ing)
@ -268,7 +274,9 @@ var _ = framework.DescribeAnnotation("modsecurity owasp", func() {
annotations := map[string]string{
"nginx.ingress.kubernetes.io/modsecurity-snippet": snippet,
}
f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "load_module, lua_package, _by_lua, location, root, {, }")
// Sleep a while just to guarantee that the configmap is applied
framework.Sleep()
ing := framework.NewSingleIngress(host, "/", host, nameSpace, framework.EchoService, 80, annotations)
f.EnsureIngress(ing)
@ -307,7 +315,9 @@ var _ = framework.DescribeAnnotation("modsecurity owasp", func() {
annotations := map[string]string{
"nginx.ingress.kubernetes.io/modsecurity-snippet": snippet,
}
f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "load_module, lua_package, _by_lua, location, root, {, }")
// Sleep a while just to guarantee that the configmap is applied
framework.Sleep()
ing := framework.NewSingleIngress(host, "/", host, nameSpace, framework.EchoService, 80, annotations)
f.EnsureIngress(ing)

2
test/e2e/framework/util.go
View file

@ -42,7 +42,7 @@ const (
Poll = 2 * time.Second
// DefaultTimeout time to wait for operations to complete
DefaultTimeout = 5 * time.Minute
DefaultTimeout = 30 * time.Second
)
func nowStamp() string {

18
test/e2e/ingress/pathtype_mixed.go
View file

@ -41,14 +41,14 @@ var _ = framework.IngressNginxDescribe("[Ingress] [PathType] mix Exact and Prefi
host := "mixed.path"
annotations := map[string]string{
"nginx.ingress.kubernetes.io/configuration-snippet": `more_set_input_headers "pathType: exact";more_set_input_headers "pathlocation: /";`,
"nginx.ingress.kubernetes.io/configuration-snippet": `more_set_input_headers "pathType: exact";more_set_input_headers "pathheader: /";`,
}
ing := framework.NewSingleIngress("exact-root", "/", host, f.Namespace, framework.EchoService, 80, annotations)
ing.Spec.Rules[0].IngressRuleValue.HTTP.Paths[0].PathType = &exactPathType
f.EnsureIngress(ing)
annotations = map[string]string{
"nginx.ingress.kubernetes.io/configuration-snippet": `more_set_input_headers "pathType: prefix";more_set_input_headers "pathlocation: /";`,
"nginx.ingress.kubernetes.io/configuration-snippet": `more_set_input_headers "pathType: prefix";more_set_input_headers "pathheader: /";`,
}
ing = framework.NewSingleIngress("prefix-root", "/", host, f.Namespace, framework.EchoService, 80, annotations)
f.EnsureIngress(ing)
@ -71,7 +71,7 @@ var _ = framework.IngressNginxDescribe("[Ingress] [PathType] mix Exact and Prefi
assert.NotContains(ginkgo.GinkgoT(), body, "pathtype=prefix")
assert.Contains(ginkgo.GinkgoT(), body, "pathtype=exact")
assert.Contains(ginkgo.GinkgoT(), body, "pathlocation=/")
assert.Contains(ginkgo.GinkgoT(), body, "pathheader=/")
ginkgo.By("Checking prefix request to /bar")
body = f.HTTPTestClient().
@ -84,17 +84,17 @@ var _ = framework.IngressNginxDescribe("[Ingress] [PathType] mix Exact and Prefi
assert.Contains(ginkgo.GinkgoT(), body, "pathtype=prefix")
assert.NotContains(ginkgo.GinkgoT(), body, "pathtype=exact")
assert.Contains(ginkgo.GinkgoT(), body, "pathlocation=/")
assert.Contains(ginkgo.GinkgoT(), body, "pathheader=/")
annotations = map[string]string{
"nginx.ingress.kubernetes.io/configuration-snippet": `more_set_input_headers "pathType: exact";more_set_input_headers "pathlocation: /foo";`,
"nginx.ingress.kubernetes.io/configuration-snippet": `more_set_input_headers "pathType: exact";more_set_input_headers "pathheader: /foo";`,
}
ing = framework.NewSingleIngress("exact-foo", "/foo", host, f.Namespace, framework.EchoService, 80, annotations)
ing.Spec.Rules[0].IngressRuleValue.HTTP.Paths[0].PathType = &exactPathType
f.EnsureIngress(ing)
annotations = map[string]string{
"nginx.ingress.kubernetes.io/configuration-snippet": `more_set_input_headers "pathType: prefix";more_set_input_headers "pathlocation: /foo";`,
"nginx.ingress.kubernetes.io/configuration-snippet": `more_set_input_headers "pathType: prefix";more_set_input_headers "pathheader: /foo";`,
}
ing = framework.NewSingleIngress("prefix-foo", "/foo", host, f.Namespace, framework.EchoService, 80, annotations)
f.EnsureIngress(ing)
@ -117,7 +117,7 @@ var _ = framework.IngressNginxDescribe("[Ingress] [PathType] mix Exact and Prefi
assert.NotContains(ginkgo.GinkgoT(), body, "pathtype=prefix")
assert.Contains(ginkgo.GinkgoT(), body, "pathtype=exact")
assert.Contains(ginkgo.GinkgoT(), body, "pathlocation=/foo")
assert.Contains(ginkgo.GinkgoT(), body, "pathheader=/foo")
ginkgo.By("Checking prefix request to /foo/bar")
body = f.HTTPTestClient().
@ -129,7 +129,7 @@ var _ = framework.IngressNginxDescribe("[Ingress] [PathType] mix Exact and Prefi
Raw()
assert.Contains(ginkgo.GinkgoT(), body, "pathtype=prefix")
assert.Contains(ginkgo.GinkgoT(), body, "pathlocation=/foo")
assert.Contains(ginkgo.GinkgoT(), body, "pathheader=/foo")
ginkgo.By("Checking prefix request to /foobar")
body = f.HTTPTestClient().
@ -141,6 +141,6 @@ var _ = framework.IngressNginxDescribe("[Ingress] [PathType] mix Exact and Prefi
Raw()
assert.Contains(ginkgo.GinkgoT(), body, "pathtype=prefix")
assert.Contains(ginkgo.GinkgoT(), body, "pathlocation=/")
assert.Contains(ginkgo.GinkgoT(), body, "pathheader=/")
})
})

153
test/e2e/settings/badannotationvalues.go Normal file
View file

@ -0,0 +1,153 @@
/*
Copyright 2021 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package settings
import (
"fmt"
"net/http"
"strings"
"github.com/onsi/ginkgo"
"k8s.io/ingress-nginx/test/e2e/framework"
)
var _ = framework.DescribeAnnotation("Bad annotation values", func() {
f := framework.NewDefaultFramework("bad-annotation")
ginkgo.BeforeEach(func() {
f.NewEchoDeployment()
})
ginkgo.It("should drop an ingress if there is an invalid character in some annotation", func() {
host := "invalid-value-test"
annotations := map[string]string{
"nginx.ingress.kubernetes.io/configuration-snippet": `
# abc { }`,
}
ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations)
f.UpdateNginxConfigMapData("allow-snippet-annotations", "true")
f.EnsureIngress(ing)
f.WaitForNginxServer(host,
func(server string) bool {
return !strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
})
f.WaitForNginxServer(host,
func(server string) bool {
return !strings.Contains(server, "# abc { }")
})
f.HTTPTestClient().
GET("/").
WithHeader("Host", host).
Expect().
Status(http.StatusNotFound)
})
ginkgo.It("should drop an ingress if there is a forbidden word in some annotation", func() {
host := "forbidden-value-test"
annotations := map[string]string{
"nginx.ingress.kubernetes.io/configuration-snippet": `
default_type text/plain;
content_by_lua_block {
ngx.say("Hello World")
}`,
}
ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations)
f.UpdateNginxConfigMapData("allow-snippet-annotations", "true")
// Sleep a while just to guarantee that the configmap is applied
framework.Sleep()
f.EnsureIngress(ing)
f.WaitForNginxServer(host,
func(server string) bool {
return !strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
})
f.WaitForNginxServer(host,
func(server string) bool {
return !strings.Contains(server, `ngx.say("Hello World")`)
})
f.HTTPTestClient().
GET("/").
WithHeader("Host", host).
Expect().
Status(http.StatusNotFound)
})
ginkgo.It("should drop an ingress if there is a custom blocklist config in place and allow others to pass", func() {
host := "custom-forbidden-value-test"
annotations := map[string]string{
"nginx.ingress.kubernetes.io/configuration-snippet": `
# something_forbidden`,
}
hostValid := "custom-allowed-value-test"
annotationsValid := map[string]string{
"nginx.ingress.kubernetes.io/configuration-snippet": `
# bla_by_lua`,
}
ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations)
ingValid := framework.NewSingleIngress(hostValid, "/", hostValid, f.Namespace, framework.EchoService, 80, annotationsValid)
f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "something_forbidden,otherthing_forbidden")
// Sleep a while just to guarantee that the configmap is applied
framework.Sleep()
f.EnsureIngress(ing)
f.EnsureIngress(ingValid)
f.WaitForNginxServer(host,
func(server string) bool {
return !strings.Contains(server, fmt.Sprintf("server_name %s ;", host))
})
f.WaitForNginxServer(hostValid,
func(server string) bool {
return strings.Contains(server, fmt.Sprintf("server_name %s ;", hostValid))
})
f.WaitForNginxServer(host,
func(server string) bool {
return !strings.Contains(server, "# something_forbidden")
})
f.WaitForNginxServer(hostValid,
func(server string) bool {
return strings.Contains(server, "# bla_by_lua")
})
f.HTTPTestClient().
GET("/").
WithHeader("Host", host).
Expect().
Status(http.StatusNotFound)
f.HTTPTestClient().
GET("/").
WithHeader("Host", hostValid).
Expect().
Status(http.StatusOK)
})
})