From 88879d28934b8b6a4b80257eb568ad9b6ba5dd6c Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Fri, 20 Dec 2024 10:00:53 +0000 Subject: [PATCH 1/2] Add unprivileged port validation --- cmd/nginx-ingress/flags.go | 17 +++++----------- cmd/nginx-ingress/flags_test.go | 18 ----------------- internal/validation/validation.go | 22 +++++++++++++------- internal/validation/validation_test.go | 28 +++++++++++++++++++------- pkg/apis/dos/validation/dos.go | 7 ++++++- 5 files changed, 47 insertions(+), 45 deletions(-) diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index 26ba224e0f..0804ac796c 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -9,6 +9,7 @@ import ( "regexp" "strings" + internalValidation "github.com/nginxinc/kubernetes-ingress/internal/validation" api_v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/validation" @@ -345,22 +346,22 @@ func mustValidateFlags(ctx context.Context) { nl.Fatalf(l, "Invalid value for leader-election-lock-name: %v", statusLockNameValidationError) } - statusPortValidationError := validatePort(*nginxStatusPort) + statusPortValidationError := internalValidation.ValidateUnprivilegedPort(*nginxStatusPort) if statusPortValidationError != nil { nl.Fatalf(l, "Invalid value for nginx-status-port: %v", statusPortValidationError) } - metricsPortValidationError := validatePort(*prometheusMetricsListenPort) + metricsPortValidationError := internalValidation.ValidateUnprivilegedPort(*prometheusMetricsListenPort) if metricsPortValidationError != nil { nl.Fatalf(l, "Invalid value for prometheus-metrics-listen-port: %v", metricsPortValidationError) } - readyStatusPortValidationError := validatePort(*readyStatusPort) + readyStatusPortValidationError := internalValidation.ValidateUnprivilegedPort(*readyStatusPort) if readyStatusPortValidationError != nil { nl.Fatalf(l, "Invalid value for ready-status-port: %v", readyStatusPortValidationError) } - healthProbePortValidationError := validatePort(*serviceInsightListenPort) + healthProbePortValidationError := internalValidation.ValidateUnprivilegedPort(*serviceInsightListenPort) if healthProbePortValidationError != nil { nl.Fatalf(l, "Invalid value for service-insight-listen-port: %v", metricsPortValidationError) } @@ -464,14 +465,6 @@ func validateResourceName(name string) error { return nil } -// validatePort makes sure a given port is inside the valid port range for its usage -func validatePort(port int) error { - if port < 1024 || port > 65535 { - return fmt.Errorf("port outside of valid port range [1024 - 65535]: %v", port) - } - return nil -} - // validateLogLevel makes sure a given logLevel is one of the allowed values func validateLogLevel(logLevel string) error { switch strings.ToLower(logLevel) { diff --git a/cmd/nginx-ingress/flags_test.go b/cmd/nginx-ingress/flags_test.go index eeb6c2b597..6021cea8b0 100644 --- a/cmd/nginx-ingress/flags_test.go +++ b/cmd/nginx-ingress/flags_test.go @@ -7,24 +7,6 @@ import ( "testing" ) -func TestValidatePort(t *testing.T) { - badPorts := []int{80, 443, 1, 1023, 65536} - for _, badPort := range badPorts { - err := validatePort(badPort) - if err == nil { - t.Errorf("Expected error for port %v\n", badPort) - } - } - - goodPorts := []int{8080, 8081, 8082, 1024, 65535} - for _, goodPort := range goodPorts { - err := validatePort(goodPort) - if err != nil { - t.Errorf("Error for valid port: %v err: %v\n", goodPort, err) - } - } -} - func TestParseNginxStatusAllowCIDRs(t *testing.T) { badCIDRs := []struct { input string diff --git a/internal/validation/validation.go b/internal/validation/validation.go index bfeaaef452..10a6935eb3 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -14,13 +14,17 @@ var ( ) // ValidatePort ensure port matches rfc6335 https://www.rfc-editor.org/rfc/rfc6335.html -func ValidatePort(value string) error { - port, err := strconv.Atoi(value) - if err != nil { - return fmt.Errorf("error parsing port number: %w", err) +func ValidatePort(value int) error { + if value > 65535 || value < 1 { + return fmt.Errorf("error parsing port: %d not a valid port number", value) } - if port > 65535 || port < 1 { - return fmt.Errorf("error parsing port: %v not a valid port number", port) + return nil +} + +// ValidateUnprivilegedPort ensure port is in the 1024-65535 range +func ValidateUnprivilegedPort(value int) error { + if value > 65535 || value < 1023 { + return fmt.Errorf("error parsing port: %d not a valid unprivileged port number", value) } return nil } @@ -34,7 +38,11 @@ func ValidateHost(host string) error { if validIPRegex.MatchString(host) || validDNSRegex.MatchString(host) || validHostnameRegex.MatchString(host) { chunks := strings.Split(host, ":") if len(chunks) > 1 { - err := ValidatePort(chunks[1]) + port, err := strconv.Atoi(chunks[1]) + if err != nil { + return err + } + err = ValidatePort(port) if err != nil { return fmt.Errorf("invalid port: %w", err) } diff --git a/internal/validation/validation_test.go b/internal/validation/validation_test.go index 9ed4cf6b2c..4322aece61 100644 --- a/internal/validation/validation_test.go +++ b/internal/validation/validation_test.go @@ -8,7 +8,7 @@ import ( func TestValidatePort_IsValidOnValidInput(t *testing.T) { t.Parallel() - ports := []string{"1", "65535"} + ports := []int{1, 65535} for _, p := range ports { if err := ValidatePort(p); err != nil { t.Error(err) @@ -16,20 +16,34 @@ func TestValidatePort_IsValidOnValidInput(t *testing.T) { } } -func TestValidatePort_ErrorsOnInvalidString(t *testing.T) { +func TestValidatePort_ErrorsOnInvalidRange(t *testing.T) { t.Parallel() - if err := ValidatePort(""); err == nil { - t.Error("want error, got nil") + ports := []int{0, -1, 65536} + for _, p := range ports { + if err := ValidatePort(p); err == nil { + t.Error("want error, got nil") + } } } -func TestValidatePort_ErrorsOnInvalidRange(t *testing.T) { +func TestValidateUnprivilegedPort_IsValidOnValidInput(t *testing.T) { t.Parallel() - ports := []string{"0", "-1", "65536"} + ports := []int{1024, 65535} for _, p := range ports { - if err := ValidatePort(p); err == nil { + if err := ValidateUnprivilegedPort(p); err != nil { + t.Error(err) + } + } +} + +func TestValidateUnprivilegedPort_ErrorsOnInvalidRange(t *testing.T) { + t.Parallel() + + ports := []int{0, -1, 80, 443, 65536} + for _, p := range ports { + if err := ValidateUnprivilegedPort(p); err == nil { t.Error("want error, got nil") } } diff --git a/pkg/apis/dos/validation/dos.go b/pkg/apis/dos/validation/dos.go index 9f3970fc3e..523bc5e264 100644 --- a/pkg/apis/dos/validation/dos.go +++ b/pkg/apis/dos/validation/dos.go @@ -5,6 +5,7 @@ import ( "net" "net/url" "regexp" + "strconv" "strings" internalValidation "github.com/nginxinc/kubernetes-ingress/internal/validation" @@ -128,7 +129,11 @@ func validateAppProtectDosLogDest(dstAntn string) error { } if validIPRegex.MatchString(dstAntn) || validDNSRegex.MatchString(dstAntn) || validLocalhostRegex.MatchString(dstAntn) { chunks := strings.Split(dstAntn, ":") - err := internalValidation.ValidatePort(chunks[1]) + port, err := strconv.Atoi(chunks[1]) + if err != nil { + return err + } + err = internalValidation.ValidatePort(port) if err != nil { return fmt.Errorf("invalid log destination: %w", err) } From fa28d0c49b9ad387e84fbba17740284660d83ab7 Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Fri, 20 Dec 2024 10:05:39 +0000 Subject: [PATCH 2/2] switch previous error message for unprivileged ports --- internal/validation/validation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/validation/validation.go b/internal/validation/validation.go index 10a6935eb3..612b8d4ee9 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -24,7 +24,7 @@ func ValidatePort(value int) error { // ValidateUnprivilegedPort ensure port is in the 1024-65535 range func ValidateUnprivilegedPort(value int) error { if value > 65535 || value < 1023 { - return fmt.Errorf("error parsing port: %d not a valid unprivileged port number", value) + return fmt.Errorf("port outside of valid port range [1024 - 65535]: %d", value) } return nil }