Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to sanitize annotation inputs #7874

Merged
merged 3 commits into from
Nov 12, 2021
Merged

Add option to sanitize annotation inputs #7874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f490a4a
Add option to sanitize annotation inputs
rikatz Oct 23, 2021
c5dd4f1
Fix e2e tests after string sanitization
rikatz Nov 3, 2021
cfa573a
Add proxy_pass and serviceaccount as denied values
rikatz Nov 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions internal/ingress/annotations/parser/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
21 changes: 21 additions & 0 deletions internal/ingress/controller/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package config

import (
"strconv"
"strings"
"time"

"k8s.io/klog/v2"
Expand Down Expand Up @@ -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"`

Expand Down Expand Up @@ -754,6 +760,20 @@ func NewDefault() Configuration {
defNginxStatusIpv6Whitelist := make([]string, 0)
defResponseHeaders := make([]string, 0)

defAnnotationValueWordBlocklist := []string{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we allow users to override the default value? Or should it be appended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can override. Appending IMO is bad, as the idea of overriding is actually removing something that may be problematic for them (like, if you use mod_security directives you want to remove {} from the list)

"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")
Expand All @@ -764,6 +784,7 @@ func NewDefault() Configuration {

AllowSnippetAnnotations: true,
AllowBackendServerHeader: false,
AnnotationValueWordBlocklist: strings.Join(defAnnotationValueWordBlocklist, ","),
AccessLogPath: "/var/log/nginx/access.log",
AccessLogParams: "",
EnableAccessLogForDefaultBackend: false,
Expand Down
12 changes: 11 additions & 1 deletion internal/ingress/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,12 +237,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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only thought is if we need to run strings.TrimSpace every element as well, I’m afraid Contains may fail otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you give me one example? not following here (rainy day, a bit sleepy... :) )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here’s what I mean:
https://play.golang.org/p/mqWmLjoz63N

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theunrealgeek GOOD CATCH!!! Thanks! I will quickly fix this and also add some regression / e2e test before releasing!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But now I see that I do trimming a bit upper:

arraybadWords := strings.Split(strings.TrimSpace(cfg.AnnotationValueWordBlocklist), ",")

Don't this solve the problem?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rikatz

arraybadWords := strings.Split(strings.TrimSpace(cfg.AnnotationValueWordBlocklist), ",")

will only trim space at the beginning and end of the annotation value, but not in between elements that may exist around the commas which is what my example in the Go playground is pointing out.

In the default value this won’t be a problem since you are coming the CSV with a Join on the list, but for user overridden ones it will be hard to enforce that they don’t put spaces between the commas to make things more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I see :) I was in my head thinking that trimSpace works for all the array, not only beginning and ending and you are right :)

Weirdly, when adding this to unit_test it passes as well, will have to check it better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, PTAL here: #7921

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)
Expand Down
17 changes: 17 additions & 0 deletions internal/ingress/controller/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With context of my previous comment, checking for another_directive would also perhaps be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/kubernetes/ingress-nginx/pull/7921/files

Added here just to make sure it's working fine :)

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
Expand Down
23 changes: 23 additions & 0 deletions internal/ingress/controller/store/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"os"
"reflect"
"sort"
"strings"
"sync"
"time"

Expand Down Expand Up @@ -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) {
Expand All @@ -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)

Expand Down
22 changes: 22 additions & 0 deletions test/e2e/admission/admission.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to check if the correct err (message) is thrown?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hum, I think we can do that yes. I would leave this anyway as a followup for some issue (feel free to open it!) to improve e2e tests in a sense that we not only verify if the error is not nil, but if error is what we expect (in all tests).

This can become a good first issue, wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good Idea, I will open one :)

})

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")
Expand Down
5 changes: 5 additions & 0 deletions test/e2e/annotations/globalratelimit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
20 changes: 15 additions & 5 deletions test/e2e/annotations/modsecurity/modsecurity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion test/e2e/framework/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
18 changes: 9 additions & 9 deletions test/e2e/ingress/pathtype_mixed.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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().
Expand All @@ -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)
Expand All @@ -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().
Expand All @@ -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().
Expand All @@ -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=/")
})
})
Loading