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

Legacy cherrypick (#7925)

Browse source 
* fix: fix thread synchronization issue #6245 (#7800)

* Add option to sanitize annotation inputs (#7874)

* Add option to sanitize annotation inputs

* Fix e2e tests after string sanitization

* Add proxy_pass and serviceaccount as denied values

* Trim spaces from badword items (#7921)

* Fix tests from cherrypick

Co-authored-by: Jens Reimann <ctron@dentrassi.de>
This commit is contained in:
Ricardo Katz 2021-11-16 10:57:27 -03:00 committed by GitHub
parent 3673519a73
commit b159577c23
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
13 changed files with 314 additions and 17 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

18
docs/user-guide/nginx-configuration/configmap.md
View file

@ -30,6 +30,7 @@ The following table shows a configuration option's name, type, and the default v
|[add-headers](#add-headers)|string|""|
|[allow-backend-server-header](#allow-backend-server-header)|bool|"false"|
|[allow-snippet-annotations](#allow-snippet-annotations)|bool|true|
|[annotation-value-word-blocklist](#annotation-value-word-blocklist)|string array|"load_module","lua_package","_by_lua","location","root","proxy_pass","serviceaccount","{","}","'","\"
|[hide-headers](#hide-headers)|string array|empty|
|[access-log-params](#access-log-params)|string|""|
|[access-log-path](#access-log-path)|string|"/var/log/nginx/access.log"|
@ -217,6 +218,23 @@ Enables Ingress to parse and add *-snippet annotations/directives created by the
Warning: We recommend enabling this option only if you TRUST users with permission to create Ingress objects, as this
may allow a user to add restricted configurations to the final nginx.conf file
## annotation-value-word-blocklist
Contains a comma-separated value of chars/words that are well known of being used to abuse Ingress configuration
and must be blocked.
When an annotation is detected with a value that matches one of the blocked badwords, the whole Ingress wont be configured.
_**default:**_ `"load_module,lua_package,_by_lua,location,root,proxy_pass,serviceaccount,{,},',\"`
Warning: The default value already contains a sane set of badwords. Some features like mod_security needs characters that are blocked, and it's up to the Ingress admin to remove this characters from the blocklist.
When doing this, the default blocklist is overrided, which means that the Ingress admin should add all the words
that should be blocked.
If you find some word should not be on the default list, or if you think that we should add more badwords, please
feel free to open an issue with your case!
## hide-headers
Sets additional header that will not be passed from the upstream server to the client response.

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"`
@ -754,6 +760,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")
@ -764,6 +784,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

@ -238,12 +238,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, strings.TrimSpace(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)

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

@ -279,6 +279,27 @@ 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")
}
ing.ObjectMeta.Annotations["nginx.ingress.kubernetes.io/custom-headers"] = "another_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.Backend
disableCatchAllBefore := nginx.cfg.DisableCatchAll

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

@ -23,6 +23,7 @@ import (
"os"
"reflect"
"sort"
"strings"
"sync"
"time"
@ -630,6 +631,21 @@ func hasCatchAllIngressRule(spec networkingv1beta1.IngressSpec) bool {
return spec.Backend != 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 *networkingv1beta1.Ingress) {
@ -638,6 +654,13 @@ func (s *k8sStore) syncIngress(ing *networkingv1beta1.Ingress) {
copyIng := &networkingv1beta1.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)

10
internal/ingress/controller/template/template.go
View file

@ -62,6 +62,9 @@ const (
// Writer is the interface to render a template
type Writer interface {
// Write renders the template.
// NOTE: Implementors must ensure that the content of the returned slice is not modified by the implementation
// after the return of this function.
Write(conf config.TemplateConfig) ([]byte, error)
}
@ -201,7 +204,12 @@ func (t *Template) Write(conf config.TemplateConfig) ([]byte, error) {
return nil, err
}
return outCmdBuf.Bytes(), nil
// make a copy to ensure that we are no longer modifying the content of the buffer
out := outCmdBuf.Bytes()
res := make([]byte, len(out))
copy(res, out)
return res, nil
}
var (

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

@ -178,6 +178,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.NetworkingV1beta1().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.NetworkingV1beta1().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")
})
})
func uninstallChart(f *framework.Framework) error {

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.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

@ -38,7 +38,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

@ -44,14 +44,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)
@ -74,7 +74,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().
@ -87,17 +87,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)
@ -120,7 +120,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().
@ -132,7 +132,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().
@ -144,6 +144,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)
})
})