From 833948e8b2c5728587748dc8da7dbbe6294dfaca Mon Sep 17 00:00:00 2001 From: Jasmine Hegman Date: Thu, 11 Oct 2018 09:20:10 -0700 Subject: [PATCH] Add CLI option `-nginx-status-allow-cidrs` to allow easily restricting access to sensitive endpoints via CLI argument. --- cmd/nginx-ingress/main.go | 44 +++++++++++++- cmd/nginx-ingress/main_test.go | 75 ++++++++++++++++++++++++ docs/cli-arguments.md | 3 + internal/controller/controller_test.go | 2 +- internal/nginx/configurator_test.go | 4 +- internal/nginx/nginx.go | 1 + internal/nginx/template_executor.go | 25 ++++---- internal/nginx/templates/nginx-plus.tmpl | 6 +- internal/nginx/templates/nginx.tmpl | 6 +- 9 files changed, 144 insertions(+), 22 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index b978a8ce42..c05607291f 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -8,6 +8,7 @@ import ( "net/http" "os" "os/signal" + "strings" "syscall" "time" @@ -83,6 +84,8 @@ The external address of the service is used when reporting the status of Ingress leaderElectionEnabled = flag.Bool("enable-leader-election", false, "Enable Leader election to avoid multiple replicas of the controller reporting the status of Ingress resources -- only one replica will report status. See -report-ingress-status flag.") + nginxStatusAllowCIDRs = flag.String("nginx-status-allow-cidrs", "127.0.0.1", `Whitelist IPv4 IP/CIDR blocks to allow access to NGINX stub_status or the NGINX Plus API. Separate multiple IP/CIDR by commas.`) + nginxStatusPort = flag.Int("nginx-status-port", 8080, "Set the port where the NGINX stub_status or the NGINX Plus API is exposed. [1023 - 65535]") @@ -104,9 +107,14 @@ func main() { glog.Fatalf("Invalid value for nginx-status-port: %v", portValidationError) } + var err error + allowedCIDRs, err := parseNginxStatusAllowCIDRs(*nginxStatusAllowCIDRs) + if err != nil { + glog.Fatalf(`Invalid value for nginx-status-allow-cidrs: %v`, err) + } + glog.Infof("Starting NGINX Ingress controller Version=%v GitCommit=%v\n", version, gitCommit) - var err error var config *rest.Config if *proxyURL != "" { config, err = clientcmd.NewNonInteractiveDeferredLoadingClientConfig( @@ -146,7 +154,7 @@ func main() { nginxIngressTemplatePath = *ingressTemplatePath } - templateExecutor, err := nginx.NewTemplateExecutor(nginxConfTemplatePath, nginxIngressTemplatePath, *healthStatus, *nginxStatus, *nginxStatusPort) + templateExecutor, err := nginx.NewTemplateExecutor(nginxConfTemplatePath, nginxIngressTemplatePath, *healthStatus, *nginxStatus, allowedCIDRs, *nginxStatusPort) if err != nil { glog.Fatalf("Error creating TemplateExecutor: %v", err) } @@ -335,3 +343,35 @@ func validateStatusPort(nginxStatusPort int) error { } return nil } + +// parseNginxStatusAllowCIDRs converts a comma separated CIDR/IP address string into an array of CIDR/IP addresses. +// It returns an array of the valid CIDR/IP addresses or an error if given an invalid address. +func parseNginxStatusAllowCIDRs(input string) (cidrs []string, err error) { + cidrsArray := strings.Split(input, ",") + for _, cidr := range cidrsArray { + trimmedCidr := strings.TrimSpace(cidr) + err := validateCIDRorIP(trimmedCidr) + if err != nil { + return cidrs, err + } + cidrs = append(cidrs, trimmedCidr) + } + return cidrs, nil +} + +// validateCIDRorIP makes sure a given string is either a valid CIDR block or IP address. +// It an error if it is not valid. +func validateCIDRorIP(cidr string) error { + if cidr == "" { + return fmt.Errorf("invalid CIDR address: an empty string is an invalid CIDR block or IP address") + } + _, _, err := net.ParseCIDR(cidr) + if err == nil { + return nil + } + ip := net.ParseIP(cidr) + if ip == nil { + return fmt.Errorf("invalid IP address: %v", cidr) + } + return nil +} diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index 6d863964da..6c0050aaef 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -1,6 +1,7 @@ package main import ( + "errors" "testing" ) @@ -22,3 +23,77 @@ func TestValidateStatusPort(t *testing.T) { } } + +func TestParseNginxStatusAllowCIDRs(t *testing.T) { + + var tests = []struct { + input string + expected []string + expectedError error + }{ + {"earth, ,,furball", + []string{}, + errors.New("invalid IP address: earth")}, + {"127.0.0.1", + []string{"127.0.0.1"}, + nil}, + {"10.0.1.0/24", + []string{"10.0.1.0/24"}, + nil}, + {"127.0.0.1,10.0.1.0/24,68.121.233.214 , 24.24.24.24/32", + []string{"127.0.0.1", "10.0.1.0/24", "68.121.233.214", "24.24.24.24/32"}, nil}, + {"127.0.0.1,10.0.1.0/24, ,,furball", + []string{"127.0.0.1", "10.0.1.0/24"}, + errors.New("invalid CIDR address: an empty string is an invalid CIDR block or IP address")}, + {"false", + []string{}, + errors.New("invalid IP address: false")}, + } + + for _, test := range tests { + splitArray, err := parseNginxStatusAllowCIDRs(test.input) + if err != test.expectedError { + if test.expectedError == nil { + t.Errorf("parseNginxStatusAllowCIDRs(%q) returned an error %q when it should not have returned an error.", test.input, err.Error()) + } else if err == nil { + t.Errorf("parseNginxStatusAllowCIDRs(%q) returned no error when it should have returned error %q", test.input, test.expectedError) + } else if err.Error() != test.expectedError.Error() { + t.Errorf("parseNginxStatusAllowCIDRs(%q) returned error %q when it should have returned error %q", test.input, err.Error(), test.expectedError) + } + } + + for _, expectedEntry := range test.expected { + if !contains(splitArray, expectedEntry) { + t.Errorf("parseNginxStatusAllowCIDRs(%q) did not include %q but returned %q", test.input, expectedEntry, splitArray) + } + } + } + +} + +func TestValidateCIDRorIP(t *testing.T) { + badCIDRs := []string{"localhost", "thing", "~", "!!!", "", " ", "-1"} + for _, badCIDR := range badCIDRs { + err := validateCIDRorIP(badCIDR) + if err == nil { + t.Errorf(`Expected error for invalid CIDR "%v"\n`, badCIDR) + } + } + + goodCIDRs := []string{"0.0.0.0/32", "0.0.0.0/0", "127.0.0.1/32", "127.0.0.0/24", "23.232.65.42"} + for _, goodCIDR := range goodCIDRs { + err := validateCIDRorIP(goodCIDR) + if err != nil { + t.Errorf("Error for valid CIDR: %v err: %v\n", goodCIDR, err) + } + } +} + +func contains(s []string, e string) bool { + for _, a := range s { + if a == e { + return true + } + } + return false +} diff --git a/docs/cli-arguments.md b/docs/cli-arguments.md index 6a6d26f748..d3c7a7cec0 100644 --- a/docs/cli-arguments.md +++ b/docs/cli-arguments.md @@ -41,6 +41,9 @@ Usage of ./nginx-ingress: Enable support for NGINX Plus -nginx-status Enable the NGINX stub_status, or the NGINX Plus API. (default true) + -nginx-status-allow-cidrs + Whitelist IPv4 IP/CIDR blocks to allow access to NGINX stub_status or the NGINX Plus API. + Separate multiple IP/CIDR by commas. -nginx-status-port int Set the port where the NGINX stub_status or the NGINX Plus API is exposed. [1023 - 65535] (default 8080) -proxy string diff --git a/internal/controller/controller_test.go b/internal/controller/controller_test.go index e406af7a19..f69cd75aa6 100644 --- a/internal/controller/controller_test.go +++ b/internal/controller/controller_test.go @@ -926,7 +926,7 @@ func TestFindIngressesForSecret(t *testing.T) { t.Run(test.desc, func(t *testing.T) { fakeClient := fake.NewSimpleClientset() - templateExecutor, err := nginx.NewTemplateExecutor("../nginx/templates/nginx-plus.tmpl", "../nginx/templates/nginx-plus.ingress.tmpl", true, true, 8080) + templateExecutor, err := nginx.NewTemplateExecutor("../nginx/templates/nginx-plus.tmpl", "../nginx/templates/nginx-plus.ingress.tmpl", true, true, []string{"127.0.0.1"}, 8080) if err != nil { t.Fatalf("templateExecuter could not start: %v", err) } diff --git a/internal/nginx/configurator_test.go b/internal/nginx/configurator_test.go index 1b7b1e1e91..7f4e0eea59 100644 --- a/internal/nginx/configurator_test.go +++ b/internal/nginx/configurator_test.go @@ -524,7 +524,7 @@ func createExpectedConfigForMergeableCafeIngress() IngressNginxConfig { } func createTestConfigurator() (*Configurator, error) { - templateExecutor, err := NewTemplateExecutor("templates/nginx-plus.tmpl", "templates/nginx-plus.ingress.tmpl", true, true, 8080) + templateExecutor, err := NewTemplateExecutor("templates/nginx-plus.tmpl", "templates/nginx-plus.ingress.tmpl", true, true, []string{"127.0.0.1"}, 8080) if err != nil { return nil, err } @@ -537,7 +537,7 @@ func createTestConfigurator() (*Configurator, error) { } func createTestConfiguratorInvalidIngressTemplate() (*Configurator, error) { - templateExecutor, err := NewTemplateExecutor("templates/nginx-plus.tmpl", "templates/nginx-plus.ingress.tmpl", true, true, 8080) + templateExecutor, err := NewTemplateExecutor("templates/nginx-plus.tmpl", "templates/nginx-plus.ingress.tmpl", true, true, []string{"127.0.0.1"}, 8080) if err != nil { return nil, err } diff --git a/internal/nginx/nginx.go b/internal/nginx/nginx.go index d8b734d097..428ae0444d 100644 --- a/internal/nginx/nginx.go +++ b/internal/nginx/nginx.go @@ -154,6 +154,7 @@ type MainConfig struct { StreamLogFormat string HealthStatus bool NginxStatus bool + NginxStatusAllowCIDRs []string NginxStatusPort int MainSnippets []string HTTPSnippets []string diff --git a/internal/nginx/template_executor.go b/internal/nginx/template_executor.go index 306f5a05a1..abafd77d29 100644 --- a/internal/nginx/template_executor.go +++ b/internal/nginx/template_executor.go @@ -8,15 +8,16 @@ import ( // TemplateExecutor executes NGINX configuration templates type TemplateExecutor struct { - HealthStatus bool - NginxStatus bool - NginxStatusPort int - mainTemplate *template.Template - ingressTemplate *template.Template + HealthStatus bool + NginxStatus bool + NginxStatusAllowCIDRs []string + NginxStatusPort int + mainTemplate *template.Template + ingressTemplate *template.Template } // NewTemplateExecutor creates a TemplateExecutor -func NewTemplateExecutor(mainTemplatePath string, ingressTemplatePath string, healthStatus bool, nginxStatus bool, nginxStatusPort int) (*TemplateExecutor, error) { +func NewTemplateExecutor(mainTemplatePath string, ingressTemplatePath string, healthStatus bool, nginxStatus bool, nginxStatusAllowCIDRs []string, nginxStatusPort int) (*TemplateExecutor, error) { // template name must be the base name of the template file https://golang.org/pkg/text/template/#Template.ParseFiles nginxTemplate, err := template.New(path.Base(mainTemplatePath)).ParseFiles(mainTemplatePath) if err != nil { @@ -29,11 +30,12 @@ func NewTemplateExecutor(mainTemplatePath string, ingressTemplatePath string, he } return &TemplateExecutor{ - mainTemplate: nginxTemplate, - ingressTemplate: ingressTemplate, - HealthStatus: healthStatus, - NginxStatus: nginxStatus, - NginxStatusPort: nginxStatusPort, + mainTemplate: nginxTemplate, + ingressTemplate: ingressTemplate, + HealthStatus: healthStatus, + NginxStatus: nginxStatus, + NginxStatusAllowCIDRs: nginxStatusAllowCIDRs, + NginxStatusPort: nginxStatusPort, }, nil } @@ -63,6 +65,7 @@ func (te *TemplateExecutor) UpdateIngressTemplate(templateString *string) error func (te *TemplateExecutor) ExecuteMainConfigTemplate(cfg *MainConfig) ([]byte, error) { cfg.HealthStatus = te.HealthStatus cfg.NginxStatus = te.NginxStatus + cfg.NginxStatusAllowCIDRs = te.NginxStatusAllowCIDRs cfg.NginxStatusPort = te.NginxStatusPort var configBuffer bytes.Buffer diff --git a/internal/nginx/templates/nginx-plus.tmpl b/internal/nginx/templates/nginx-plus.tmpl index 3788d5976c..5ffbc361ee 100644 --- a/internal/nginx/templates/nginx-plus.tmpl +++ b/internal/nginx/templates/nginx-plus.tmpl @@ -96,10 +96,10 @@ http { location = /dashboard.html { } - - allow 127.0.0.1; + {{ range $value := .NginxStatusAllowCIDRs }}{{ if ne $value "" }} + allow {{$value}};{{ end }} + {{end}} deny all; - location /api { api write=off; } diff --git a/internal/nginx/templates/nginx.tmpl b/internal/nginx/templates/nginx.tmpl index de239b0250..b0d790db57 100644 --- a/internal/nginx/templates/nginx.tmpl +++ b/internal/nginx/templates/nginx.tmpl @@ -86,10 +86,10 @@ http { # stub_status server { listen {{.NginxStatusPort}}; - - allow 127.0.0.1; + {{ range $value := .NginxStatusAllowCIDRs }}{{ if ne $value "" }} + allow {{$value}};{{ end }} + {{end}} deny all; - location /stub_status { stub_status; }