From f9107996478c144bc2f4efd0b73bd58b6f9acda3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Renan=20Gon=C3=A7alves?= Date: Wed, 9 Mar 2022 15:13:10 +0100 Subject: [PATCH] Check real client IP for access when behind a proxy The module ngx_http_access_module allows limiting access to certain *connecting* client addresses. When using it behind a proxy it would then use the proxy IP instead of the real client IP, which is ultimately the desired outcome. By using the module ngx_http_geo_module we can explicitly specify what are the proxy IPs thus allowing us to check the real client IP directly. This implementation also creates unique geo variables to be used by locations having the same set of access list IPs. This can result in a much smaller configuration file. The nginx documentation also recommends the usage of geo variables when having a lot of rules: > In case of a lot of rules, the use of the ngx_http_geo_module module variables is preferable. See: - http://nginx.org/en/docs/http/ngx_http_access_module.html - http://nginx.org/en/docs/http/ngx_http_geo_module.html --- .../ingress/controller/template/template.go | 102 ++++++++++++++ .../controller/template/template_test.go | 126 ++++++++++++++++++ pkg/apis/ingress/types.go | 6 + rootfs/etc/nginx/template/nginx.tmpl | 43 +++++- test/e2e/annotations/ipdenylist.go | 48 +++++-- test/e2e/annotations/ipwhitelist.go | 19 ++- test/e2e/settings/configmap_change.go | 2 +- 7 files changed, 322 insertions(+), 24 deletions(-) diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index 6aadab48e..60fe66fab 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -45,6 +45,7 @@ import ( "k8s.io/ingress-nginx/internal/ingress/controller/config" ing_net "k8s.io/ingress-nginx/internal/net" "k8s.io/ingress-nginx/pkg/apis/ingress" + iSets "k8s.io/ingress-nginx/pkg/util/sets" ) const ( @@ -234,6 +235,8 @@ var ( "extractHostPort": extractHostPort, "changeHostPort": changeHostPort, "buildProxyPass": buildProxyPass, + "buildDenylists": buildDenylists, + "buildWhitelists": buildWhitelists, "filterRateLimits": filterRateLimits, "buildRateLimitZones": buildRateLimitZones, "buildRateLimit": buildRateLimit, @@ -790,6 +793,105 @@ rewrite "(?i)%s" %s break; return defProxyPass } +type accessListVariable struct { + ID string `json:"id"` + AccessList []string `json:"accessList"` +} + +func (gv1 *accessListVariable) Equal(gv2 *accessListVariable) bool { + if gv1 == gv2 { + return true + } + if gv1 == nil || gv2 == nil { + return false + } + + if len(gv1.AccessList) != len(gv2.AccessList) { + return false + } + return iSets.StringElementsMatch(gv1.AccessList, gv2.AccessList) +} + +func buildDenylists(input interface{}) []accessListVariable { + var variables []accessListVariable + + servers, ok := input.([]*ingress.Server) + if !ok { + klog.Errorf("expected a '[]*ingress.Server' type but %T was returned", input) + return variables + } + + for _, server := range servers { + for _, loc := range server.Locations { + if len(loc.Denylist.CIDR) == 0 { + continue + } + + list := accessListVariable{ + ID: fmt.Sprintf("%d", len(variables)), + AccessList: loc.Denylist.CIDR, + } + + foundID := "" + for _, list2 := range variables { + if list.Equal(&list2) { + foundID = list2.ID + continue + } + } + + if foundID == "" { + foundID = list.ID + variables = append(variables, list) + } + + loc.DenylistID = foundID + } + } + + return variables +} + +func buildWhitelists(input interface{}) []accessListVariable { + var variables []accessListVariable + + servers, ok := input.([]*ingress.Server) + if !ok { + klog.Errorf("expected a '[]*ingress.Server' type but %T was returned", input) + return variables + } + + for _, server := range servers { + for _, loc := range server.Locations { + if len(loc.Whitelist.CIDR) == 0 { + continue + } + + list := accessListVariable{ + ID: fmt.Sprintf("%d", len(variables)), + AccessList: loc.Whitelist.CIDR, + } + + foundID := "" + for _, list2 := range variables { + if list.Equal(&list2) { + foundID = list2.ID + continue + } + } + + if foundID == "" { + foundID = list.ID + variables = append(variables, list) + } + + loc.WhitelistID = foundID + } + } + + return variables +} + func filterRateLimits(input interface{}) []ratelimit.Config { ratelimits := []ratelimit.Config{} found := sets.Set[string]{} diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index 1980d7e52..568785f75 100644 --- a/internal/ingress/controller/template/template_test.go +++ b/internal/ingress/controller/template/template_test.go @@ -20,6 +20,8 @@ import ( "bytes" "encoding/base64" "fmt" + "k8s.io/ingress-nginx/internal/ingress/annotations/ipdenylist" + "k8s.io/ingress-nginx/internal/ingress/annotations/ipwhitelist" "net" "os" "path" @@ -1006,6 +1008,130 @@ func TestFilterRateLimits(t *testing.T) { } } +func TestBuildDenylists(t *testing.T) { + servers := []*ingress.Server{ + { + Locations: []*ingress.Location{ + { + Denylist: ipdenylist.SourceRange{ + CIDR: []string{"8.8.8.8/32"}, + }, + }, + { + Denylist: ipdenylist.SourceRange{ + CIDR: []string{"4.4.4.4/32"}, + }, + }, + { + Denylist: ipdenylist.SourceRange{ + CIDR: []string{"8.8.8.8/32"}, + }, + }, + { + Denylist: ipdenylist.SourceRange{ + CIDR: []string{"4.4.4.4/32", "8.8.8.8/32"}, + }, + }, + }, + }, + } + + expected := []accessListVariable{ + { + ID: "0", + AccessList: []string{"8.8.8.8/32"}, + }, + { + ID: "1", + AccessList: []string{"4.4.4.4/32"}, + }, + { + ID: "2", + AccessList: []string{"4.4.4.4/32", "8.8.8.8/32"}, + }, + } + actual := buildDenylists(servers) + + if !reflect.DeepEqual(expected, actual) { + t.Errorf("Expected '%v' but returned '%v'", expected, actual) + } + + if servers[0].Locations[0].DenylistID != "0" { + t.Errorf("Expected '%v' but returned '%v'", "0", servers[0].Locations[0].DenylistID) + } + if servers[0].Locations[1].DenylistID != "1" { + t.Errorf("Expected '%v' but returned '%v'", "1", servers[0].Locations[1].DenylistID) + } + if servers[0].Locations[2].DenylistID != "0" { + t.Errorf("Expected '%v' but returned '%v'", "0", servers[0].Locations[2].DenylistID) + } + if servers[0].Locations[3].DenylistID != "2" { + t.Errorf("Expected '%v' but returned '%v'", "2", servers[0].Locations[3].DenylistID) + } +} + +func TestBuildWhitelists(t *testing.T) { + servers := []*ingress.Server{ + { + Locations: []*ingress.Location{ + { + Whitelist: ipwhitelist.SourceRange{ + CIDR: []string{"8.8.8.8/32"}, + }, + }, + { + Whitelist: ipwhitelist.SourceRange{ + CIDR: []string{"4.4.4.4/32"}, + }, + }, + { + Whitelist: ipwhitelist.SourceRange{ + CIDR: []string{"8.8.8.8/32"}, + }, + }, + { + Whitelist: ipwhitelist.SourceRange{ + CIDR: []string{"4.4.4.4/32", "8.8.8.8/32"}, + }, + }, + }, + }, + } + + expected := []accessListVariable{ + { + ID: "0", + AccessList: []string{"8.8.8.8/32"}, + }, + { + ID: "1", + AccessList: []string{"4.4.4.4/32"}, + }, + { + ID: "2", + AccessList: []string{"4.4.4.4/32", "8.8.8.8/32"}, + }, + } + actual := buildWhitelists(servers) + + if !reflect.DeepEqual(expected, actual) { + t.Errorf("Expected '%v' but returned '%v'", expected, actual) + } + + if servers[0].Locations[0].WhitelistID != "0" { + t.Errorf("Expected '%v' but returned '%v'", "0", servers[0].Locations[0].WhitelistID) + } + if servers[0].Locations[1].WhitelistID != "1" { + t.Errorf("Expected '%v' but returned '%v'", "1", servers[0].Locations[1].WhitelistID) + } + if servers[0].Locations[2].WhitelistID != "0" { + t.Errorf("Expected '%v' but returned '%v'", "0", servers[0].Locations[2].WhitelistID) + } + if servers[0].Locations[3].WhitelistID != "2" { + t.Errorf("Expected '%v' but returned '%v'", "2", servers[0].Locations[3].WhitelistID) + } +} + func TestBuildAuthSignURL(t *testing.T) { cases := map[string]struct { Input, RedirectParam, Output string diff --git a/pkg/apis/ingress/types.go b/pkg/apis/ingress/types.go index e50666c18..eeef7a6b8 100644 --- a/pkg/apis/ingress/types.go +++ b/pkg/apis/ingress/types.go @@ -298,10 +298,16 @@ type Location struct { // addresses or networks are allowed. // +optional Denylist ipdenylist.SourceRange `json:"denylist,omitempty"` + // Denylist ID is a unique variable index + // +optional + DenylistID string // Whitelist indicates only connections from certain client // addresses or networks are allowed. // +optional Whitelist ipwhitelist.SourceRange `json:"whitelist,omitempty"` + // Whitelist ID is a unique variable index + // +optional + WhitelistID string // Proxy contains information about timeouts and buffer sizes // to be used in connections against endpoints // +optional diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index a49989f30..b6d2801e7 100644 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -540,6 +540,34 @@ http { } {{ end }} + {{ range $variable := (buildDenylists $servers) }} + geo $denied_{{ $variable.ID }} { + default "false"; + + {{- range $trustedIP := $cfg.ProxyRealIPCIDR }} + proxy {{ $trustedIP }}; + {{- end }} + + {{- range $denylistIP := $variable.AccessList }} + {{ $denylistIP }} "true"; + {{- end }} + } + {{ end }} + + {{ range $variable := (buildWhitelists $servers) }} + geo $allowed_{{ $variable.ID }} { + default "false"; + + {{- range $trustedIP := $cfg.ProxyRealIPCIDR }} + proxy {{ $trustedIP }}; + {{- end }} + + {{- range $whitelistIP := $variable.AccessList }} + {{ $whitelistIP }} "true"; + {{- end }} + } + {{ end }} + {{/* build all the required rate limit zones. Each annotation requires a dedicated zone */}} {{/* 1MB -> 16 thousand 64-byte states or about 8 thousand 128-byte states */}} {{ range $zone := (buildRateLimitZones $servers) }} @@ -1287,14 +1315,15 @@ stream { {{ buildModSecurityForLocation $all.Cfg $location }} {{ if isLocationAllowed $location }} - {{ if gt (len $location.Denylist.CIDR) 0 }} - {{ range $ip := $location.Denylist.CIDR }} - deny {{ $ip }};{{ end }} + {{ if gt (len $location.DenylistID) 0 }} + if ($denied_{{ $location.DenylistID }} = "true") { + return 403; + } {{ end }} - {{ if gt (len $location.Whitelist.CIDR) 0 }} - {{ range $ip := $location.Whitelist.CIDR }} - allow {{ $ip }};{{ end }} - deny all; + {{ if gt (len $location.WhitelistID) 0 }} + if ($allowed_{{ $location.WhitelistID }} = "false") { + return 403; + } {{ end }} {{ if $location.CorsConfig.CorsEnabled }} diff --git a/test/e2e/annotations/ipdenylist.go b/test/e2e/annotations/ipdenylist.go index 9c1d45cf5..938090ff9 100644 --- a/test/e2e/annotations/ipdenylist.go +++ b/test/e2e/annotations/ipdenylist.go @@ -17,10 +17,9 @@ limitations under the License. package annotations import ( - "net/http" - "strings" - "github.com/onsi/ginkgo/v2" + "net/http" + "regexp" "k8s.io/ingress-nginx/test/e2e/framework" ) @@ -51,11 +50,21 @@ var _ = framework.DescribeAnnotation("denylist-source-range", func() { f.EnsureIngress(ing) + f.WaitForNginxConfiguration( + func(cfg string) bool { + return regexp.MustCompile( + `geo \$denied_0 \{\s+` + + `default "false";\s+` + + `proxy 0.0.0.0/0;\s+` + + `18.0.0.0/8 "true";\s+` + + `56.0.0.1 "true";\s+` + + `}`, + ).MatchString(cfg) + }) + f.WaitForNginxServer(host, func(server string) bool { - return strings.Contains(server, "deny 18.0.0.0/8;") && - strings.Contains(server, "deny 56.0.0.1;") && - !strings.Contains(server, "deny all;") + return regexp.MustCompile(`if \(\$denied_0 = "true"\) \{\s+return 403;\s+}`).MatchString(server) }) ginkgo.By("sending request from an explicitly denied IP range") @@ -103,13 +112,30 @@ var _ = framework.DescribeAnnotation("denylist-source-range", func() { f.EnsureIngress(ing) + f.WaitForNginxConfiguration( + func(cfg string) bool { + return regexp.MustCompile( + `geo \$denied_0 \{\s+`+ + `default "false";\s+`+ + `proxy 0.0.0.0/0;\s+`+ + `18.1.0.0/16 "true";\s+`+ + `56.0.0.0/8 "true";\s+`+ + `}`, + ).MatchString(cfg) && + regexp.MustCompile( + `geo \$allowed_0 \{\s+`+ + `default "false";\s+`+ + `proxy 0.0.0.0/0;\s+`+ + `18.0.0.0/8 "true";\s+`+ + `55.0.0.0/8 "true";\s+`+ + `}`, + ).MatchString(cfg) + }) + f.WaitForNginxServer(host, func(server string) bool { - return strings.Contains(server, "deny 18.1.0.0/16;") && - strings.Contains(server, "deny 56.0.0.0/8;") && - strings.Contains(server, "allow 18.0.0.0/8;") && - strings.Contains(server, "allow 55.0.0.0/8;") && - strings.Contains(server, "deny all;") + return regexp.MustCompile(`if \(\$denied_0 = "true"\) \{\s+return 403;\s+}`).MatchString(server) && + regexp.MustCompile(`if \(\$allowed_0 = "false"\) \{\s+return 403;\s+}`).MatchString(server) }) ginkgo.By("sending request from an explicitly denied IP range") diff --git a/test/e2e/annotations/ipwhitelist.go b/test/e2e/annotations/ipwhitelist.go index 71f026c7f..d9df8fe49 100644 --- a/test/e2e/annotations/ipwhitelist.go +++ b/test/e2e/annotations/ipwhitelist.go @@ -17,9 +17,8 @@ limitations under the License. package annotations import ( - "strings" - "github.com/onsi/ginkgo/v2" + "regexp" "k8s.io/ingress-nginx/test/e2e/framework" ) @@ -42,11 +41,21 @@ var _ = framework.DescribeAnnotation("whitelist-source-range", func() { ing := framework.NewSingleIngress(host, "/", host, nameSpace, framework.EchoService, 80, annotations) f.EnsureIngress(ing) + f.WaitForNginxConfiguration( + func(cfg string) bool { + return regexp.MustCompile( + `geo \$allowed_0 \{\s+` + + `default "false";\s+` + + `proxy 0.0.0.0/0;\s+` + + `18.0.0.0/8 "true";\s+` + + `56.0.0.0/8 "true";\s+` + + `}`, + ).MatchString(cfg) + }) + f.WaitForNginxServer(host, func(server string) bool { - return strings.Contains(server, "allow 18.0.0.0/8;") && - strings.Contains(server, "allow 56.0.0.0/8;") && - strings.Contains(server, "deny all;") + return regexp.MustCompile(`if \(\$allowed_0 = "false"\) \{\s+return 403;\s+}`).MatchString(server) }) }) }) diff --git a/test/e2e/settings/configmap_change.go b/test/e2e/settings/configmap_change.go index 3e37b62cd..07f55b25d 100644 --- a/test/e2e/settings/configmap_change.go +++ b/test/e2e/settings/configmap_change.go @@ -54,7 +54,7 @@ var _ = framework.DescribeSetting("Configmap change", func() { checksum = match[1] } - return strings.Contains(cfg, "allow 1.1.1.1;") + return strings.Contains(cfg, `1.1.1.1 "true";`) }) assert.NotEmpty(ginkgo.GinkgoT(), checksum)