Add CLI option -nginx-status-allow-cidrs to allow easily restrictin…

…g access to sensitive endpoints via CLI argument.

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
+}

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
+}

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

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

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
 	}

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

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

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;
         }

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;
         }