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

update default block list,docs, tests (#7942)

Browse source 
* update default block list,docs, tests

* fix config for admin test

* gofmt

* remove the err return
This commit is contained in:
James Strong 2021-11-23 12:06:17 -05:00 committed by GitHub
parent e57d2f63fa
commit d4a6ade65f
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 69 additions and 63 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"|
@ -226,20 +226,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"
@ -767,21 +766,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")
@ -792,7 +776,7 @@ func NewDefault() Configuration {
AllowSnippetAnnotations: true,
AllowBackendServerHeader: false,
AnnotationValueWordBlocklist: strings.Join(defAnnotationValueWordBlocklist, ","),
AnnotationValueWordBlocklist: "",
AccessLogPath: "/var/log/nginx/access.log",
AccessLogParams: "",
EnableAccessLogForDefaultBackend: false,

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

@ -242,7 +242,12 @@ 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), ",")
klog.Warningf("Blocklist is %s", cfg.AnnotationValueWordBlocklist)
}
for key, value := range ing.ObjectMeta.GetAnnotations() {
@ -251,9 +256,11 @@ 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)) {
klog.Errorf("%s annotation contains invalid word %s", key, forbiddenvalue)
return fmt.Errorf("%s annotation contains invalid word %s", key, forbiddenvalue)
}
}

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

@ -823,11 +823,12 @@ func (s *k8sStore) syncIngress(ing *networkingv1.Ingress) {
copyIng := &networkingv1.Ingress{}
ing.ObjectMeta.DeepCopyInto(&copyIng.ObjectMeta)
klog.Errorf("Blocklist: %v", s.backendConfig.AnnotationValueWordBlocklist)
if 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)

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

@ -127,6 +127,9 @@ var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() {
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.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")
@ -138,6 +141,9 @@ var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() {
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.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")

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 = 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)
})
})