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

Legacy cherrypick (#7965)

Browse source 
* update default block list,docs, tests (#7942)

* update default block list,docs, tests

* fix config for admin test

* gofmt

* remove the err return

* Change sanitization message from error to warning (#7963)

Signed-off-by: Ricardo Pchevuzinske Katz <ricardo.katz@gmail.com>

* duplicate test

Co-authored-by: Ricardo Katz <rikatz@users.noreply.github.com>
This commit is contained in:
James Strong 2021-11-24 12:34:21 -05:00 committed by GitHub
parent b159577c23
commit 18e6eb0a31
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 89 additions and 74 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

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

@ -30,7 +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","{","}","'","\"
|[annotation-value-word-blocklist](#annotation-value-word-blocklist)|string array|""|
|[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"|
@ -221,20 +221,17 @@ 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.
and must be blocked. Related to [CVE-2021-25742](https://github.com/kubernetes/ingress-nginx/issues/7837)
When an annotation is detected with a value that matches one of the blocked badwords, the whole Ingress wont be configured.
When an annotation is detected with a value that matches one of the blocked bad words, the whole Ingress won't be configured.
_**default:**_ `"load_module,lua_package,_by_lua,location,root,proxy_pass,serviceaccount,{,},',\"`
_**default:**_ `""`
When doing this, the default blocklist is override, which means that the Ingress admin should add all the words
that should be blocked, here is a suggested block list.
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.
_**suggested:**_ `"load_module,lua_package,_by_lua,location,root,proxy_pass,serviceaccount,{,},',\"`
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.

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

@ -18,7 +18,6 @@ package config
import (
"strconv"
"strings"
"time"
"k8s.io/klog/v2"
@ -759,21 +758,6 @@ func NewDefault() Configuration {
defNginxStatusIpv4Whitelist := make([]string, 0)
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")
@ -784,7 +768,7 @@ func NewDefault() Configuration {
AllowSnippetAnnotations: true,
AllowBackendServerHeader: false,
AnnotationValueWordBlocklist: strings.Join(defAnnotationValueWordBlocklist, ","),
AnnotationValueWordBlocklist: "",
AccessLogPath: "/var/log/nginx/access.log",
AccessLogParams: "",
EnableAccessLogForDefaultBackend: false,

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

@ -238,7 +238,11 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
cfg := n.store.GetBackendConfiguration()
cfg.Resolver = n.resolver
arraybadWords := strings.Split(strings.TrimSpace(cfg.AnnotationValueWordBlocklist), ",")
var arrayBadWords []string
if cfg.AnnotationValueWordBlocklist != "" {
arrayBadWords = strings.Split(strings.TrimSpace(cfg.AnnotationValueWordBlocklist), ",")
}
for key, value := range ing.ObjectMeta.GetAnnotations() {
@ -247,8 +251,9 @@ func (n *NGINXController) CheckIngress(ing *networking.Ingress) error {
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.HasPrefix(key, fmt.Sprintf("%s/", parser.AnnotationsPrefix)) && len(arrayBadWords) != 0 {
for _, forbiddenvalue := range arrayBadWords {
if strings.Contains(value, strings.TrimSpace(forbiddenvalue)) {
return fmt.Errorf("%s annotation contains invalid word %s", key, forbiddenvalue)
}

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

@ -655,10 +655,11 @@ 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
if s.backendConfig.AnnotationValueWordBlocklist != "" {
if err := checkBadAnnotationValue(copyIng.Annotations, s.backendConfig.AnnotationValueWordBlocklist); err != nil {
klog.Warningf("skipping ingress %s: %s", key, err)
return
}
}
ing.Spec.DeepCopyInto(&copyIng.Spec)

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

@ -146,6 +146,34 @@ var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() {
}
})
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;}",
}
f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "}")
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",
}
f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "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")
})
ginkgo.It("should not return an error if the Ingress V1 definition is valid", func() {
if !f.IsIngressV1Ready {
ginkgo.Skip("Test requires Kubernetes v1.19 or higher")
@ -189,17 +217,6 @@ var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() {
_, 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 {

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 = 30 * time.Second
DefaultTimeout = 90 * time.Second
)
func nowStamp() string {

67
test/e2e/settings/badannotationvalues.go
View file

@ -33,7 +33,7 @@ var _ = framework.DescribeAnnotation("Bad annotation values", func() {
f.NewEchoDeployment()
})
ginkgo.It("should drop an ingress if there is an invalid character in some annotation", func() {
ginkgo.It("[BAD_ANNOTATIONS] should drop an ingress if there is an invalid character in some annotation", func() {
host := "invalid-value-test"
annotations := map[string]string{
@ -43,6 +43,8 @@ var _ = framework.DescribeAnnotation("Bad annotation values", func() {
ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations)
f.UpdateNginxConfigMapData("allow-snippet-annotations", "true")
f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "something_forbidden,otherthing_forbidden,{")
f.EnsureIngress(ing)
f.WaitForNginxServer(host,
@ -62,7 +64,7 @@ var _ = framework.DescribeAnnotation("Bad annotation values", func() {
Status(http.StatusNotFound)
})
ginkgo.It("should drop an ingress if there is a forbidden word in some annotation", func() {
ginkgo.It("[BAD_ANNOTATIONS] should drop an ingress if there is a forbidden word in some annotation", func() {
host := "forbidden-value-test"
annotations := map[string]string{
@ -75,6 +77,7 @@ var _ = framework.DescribeAnnotation("Bad annotation values", func() {
ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations)
f.UpdateNginxConfigMapData("allow-snippet-annotations", "true")
f.UpdateNginxConfigMapData("annotation-value-word-blocklist", "something_forbidden,otherthing_forbidden,content_by_lua_block")
// Sleep a while just to guarantee that the configmap is applied
framework.Sleep()
f.EnsureIngress(ing)
@ -96,13 +99,7 @@ var _ = framework.DescribeAnnotation("Bad annotation values", func() {
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`,
}
ginkgo.It("[BAD_ANNOTATIONS] should allow an ingress if there is a default blocklist config in place", func() {
hostValid := "custom-allowed-value-test"
annotationsValid := map[string]string{
@ -110,44 +107,58 @@ var _ = framework.DescribeAnnotation("Bad annotation values", func() {
# 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)
})
ginkgo.It("[BAD_ANNOTATIONS] 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`,
}
ing := framework.NewSingleIngress(host, "/", host, f.Namespace, framework.EchoService, 80, annotations)
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.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, "# something_forbidden")
})
f.HTTPTestClient().
GET("/").
WithHeader("Host", host).
Expect().
Status(http.StatusNotFound)
})
})