Merge pull request #20357 from ramr/fix-haproxy-whitelist

HAProxy router fix for ip whitelist exceeding max config arguments that haproxy allows.

images/router/haproxy/Dockerfile

@@ -9,7 +9,7 @@ RUN INSTALL_PKGS="haproxy18 rsyslog" && \
     yum install -y $INSTALL_PKGS && \
     rpm -V $INSTALL_PKGS && \
     yum clean all && \
-    mkdir -p /var/lib/haproxy/router/{certs,cacerts} && \
+    mkdir -p /var/lib/haproxy/router/{certs,cacerts,whitelists} && \
     mkdir -p /var/lib/haproxy/{conf/.tmp,run,bin,log} && \
     touch /var/lib/haproxy/conf/{{os_http_be,os_edge_reencrypt_be,os_tcp_be,os_sni_passthrough,os_route_http_redirect,cert_config,os_wildcard_domain}.map,haproxy.config} && \
     setcap 'cap_net_bind_service=ep' /usr/sbin/haproxy && \

images/router/haproxy/conf/haproxy-config.template

@@ -388,7 +388,13 @@ backend {{genBackendNamePrefix $cfg.TLSTermination}}:{{$cfgIdx}}
   balance {{ if gt $cfg.ActiveServiceUnits 1 }}roundrobin{{ else }}leastconn{{ end }}
     {{- end }}
     {{- with $ip_whiteList := firstMatch $cidrListPattern (index $cfg.Annotations "haproxy.router.openshift.io/ip_whitelist") }}
-  acl whitelist src {{ $ip_whiteList }}
+      {{- if validateHAProxyWhiteList $ip_whiteList }}
+  acl whitelist src {{$ip_whiteList}}
+      {{- else }}
+	{{- with $whiteListFileName := generateHAProxyWhiteListFile $workingDir $cfgIdx $ip_whiteList}}
+  acl whitelist src -f {{$whiteListFileName}}
+        {{- end }}
+      {{- end }}
   tcp-request content reject if !whitelist
     {{- end }}
     {{- with $value := firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout")}}
@@ -510,7 +516,13 @@ backend {{genBackendNamePrefix $cfg.TLSTermination}}:{{$cfgIdx}}
   balance {{ if gt $cfg.ActiveServiceUnits 1 }}roundrobin{{ else }}source{{ end }}
     {{- end }}
     {{- with $ip_whiteList := firstMatch $cidrListPattern (index $cfg.Annotations "haproxy.router.openshift.io/ip_whitelist") }}
+      {{- if validateHAProxyWhiteList $ip_whiteList }}
   acl whitelist src {{$ip_whiteList}}
+      {{- else }}
+	{{- with $whiteListFileName := generateHAProxyWhiteListFile $workingDir $cfgIdx $ip_whiteList}}
+  acl whitelist src -f {{$whiteListFileName}}
+        {{- end }}
+      {{- end }}
   tcp-request content reject if !whitelist
     {{- end }}
     {{- with $value := firstMatch $timeSpecPattern (index $cfg.Annotations "haproxy.router.openshift.io/timeout")}}

pkg/router/template/template_helper.go

@@ -2,6 +2,7 @@ package templaterouter
 
 import (
 	"fmt"
+	"io/ioutil"
 	"math/rand"
 	"os"
 	"path"
@@ -182,6 +183,25 @@ func generateHAProxyCertConfigMap(td templateData) []string {
 	return lines
 }
 
+// validateHAProxyWhiteList validates a whitelist for use with an haproxy acl.
+func validateHAProxyWhiteList(value string) bool {
+	_, valid := haproxyutil.ValidateWhiteList(value)
+	return valid
+}
+
+// generateHAProxyWhiteListFile generates a whitelist file for use with an haproxy acl.
+func generateHAProxyWhiteListFile(workingDir, id, value string) string {
+	name := path.Join(workingDir, "whitelists", fmt.Sprintf("%s.txt", id))
+	cidrs, _ := haproxyutil.ValidateWhiteList(value)
+	data := []byte(strings.Join(cidrs, "\n") + "\n")
+	if err := ioutil.WriteFile(name, data, 0644); err != nil {
+		glog.Errorf("Error writing haproxy whitelist contents: %v", err)
+		return ""
+	}
+
+	return name
+}
+
 // getHTTPAliasesGroupedByHost returns HTTP(S) aliases grouped by their host.
 func getHTTPAliasesGroupedByHost(aliases map[string]ServiceAliasConfig) map[string]map[string]ServiceAliasConfig {
 	result := make(map[string]map[string]ServiceAliasConfig)
@@ -270,5 +290,7 @@ var helperFunctions = template.FuncMap{
 	"getHTTPAliasesGroupedByHost": getHTTPAliasesGroupedByHost, //returns HTTP(S) aliases grouped by their host
 	"getPrimaryAliasKey":          getPrimaryAliasKey,          //returns the key of the primary alias for a group of aliases
 
-	"generateHAProxyMap": generateHAProxyMap, //generates a haproxy map content
+	"generateHAProxyMap":           generateHAProxyMap,           //generates a haproxy map content
+	"validateHAProxyWhiteList":     validateHAProxyWhiteList,     //validates a haproxy whitelist (acl) content
+	"generateHAProxyWhiteListFile": generateHAProxyWhiteListFile, //generates a haproxy whitelist file for use in an acl
 }

pkg/router/template/util/haproxy/whitelist.go

@@ -0,0 +1,30 @@
+package haproxy
+
+import (
+	"strings"
+)
+
+const (
+	// HAPROXY_MAX_LINE_ARGS is the maximum number of arguments that haproxy
+	// supports on a configuration line.
+	// Ref: https://github.com/haproxy/haproxy/blob/master/include/common/defaults.h#L75
+	HAPROXY_MAX_LINE_ARGS = 64
+
+	// HAPROXY_MAX_WHITELIST_LENGTH is the maximum number of CIDRs allowed
+	// for an "acl whitelist src [<cidr>]*" config line.
+	HAPROXY_MAX_WHITELIST_LENGTH = HAPROXY_MAX_LINE_ARGS - 3
+)
+
+// ValidateWhiteList validates a haproxy acl whitelist from an annotation value.
+func ValidateWhiteList(value string) ([]string, bool) {
+	values := strings.Split(value, " ")
+
+	cidrs := make([]string, 0)
+	for _, v := range values {
+		if len(v) > 0 {
+			cidrs = append(cidrs, v)
+		}
+	}
+
+	return cidrs, len(cidrs) <= HAPROXY_MAX_WHITELIST_LENGTH
+}

pkg/router/template/util/haproxy/whitelist_test.go

@@ -0,0 +1,92 @@
+package haproxy
+
+import (
+	"fmt"
+	"reflect"
+	"strings"
+	"testing"
+
+	utilrand "k8s.io/apimachinery/pkg/util/rand"
+)
+
+func generateTestData(n int) []string {
+	cidrs := make([]string, 0)
+	prefix := fmt.Sprintf("%d.%d.%d", utilrand.IntnRange(1, 254), utilrand.IntnRange(1, 254), utilrand.IntnRange(1, 254))
+	for i := 1; i <= n; i++ {
+		if i%254 == 0 {
+			prefix = fmt.Sprintf("%d.%d.%d", utilrand.IntnRange(1, 254), utilrand.IntnRange(1, 254), utilrand.IntnRange(1, 254))
+		}
+
+		cidr := fmt.Sprintf("%s.%d", prefix, (i%254)+1)
+		if i%10 == 0 {
+			cidr = fmt.Sprintf("%s/24", cidr)
+		}
+		cidrs = append(cidrs, cidr)
+	}
+
+	return cidrs
+}
+
+func TestValidateWhiteList(t *testing.T) {
+	tests := []struct {
+		name        string
+		data        []string
+		expectation []string
+	}{
+		{
+			name:        "empty list",
+			data:        []string{},
+			expectation: []string{},
+		},
+		{
+			name:        "blanks",
+			data:        []string{"", "  ", "", "   ", " "},
+			expectation: []string{},
+		},
+		{
+			name:        "one ip",
+			data:        []string{"1.2.3.4"},
+			expectation: []string{"1.2.3.4"},
+		},
+		{
+			name:        "onesie",
+			data:        []string{"172.16.32.1/24"},
+			expectation: []string{"172.16.32.1/24"},
+		},
+		{
+			name:        "duo",
+			data:        []string{"172.16.32.1/24", "10.1.2.3"},
+			expectation: []string{"172.16.32.1/24", "10.1.2.3"},
+		},
+		{
+			name:        "interleaved blank entries",
+			data:        []string{"172.16.32.1/24", "", "1.2.3.4", "", "5.6.7.8", ""},
+			expectation: []string{"172.16.32.1/24", "1.2.3.4", "5.6.7.8"},
+		},
+	}
+
+	for _, tc := range tests {
+		values, ok := ValidateWhiteList(strings.Join(tc.data, " "))
+		if !reflect.DeepEqual(tc.expectation, values) {
+			t.Errorf("%s: expected validated data %+v, got %+v", tc.name, tc.expectation, values)
+		}
+		flagExpectation := len(tc.expectation) <= 61
+		if ok != flagExpectation {
+			t.Errorf("%s: expected flag %+v, got %+v", tc.name, flagExpectation, ok)
+		}
+	}
+
+	limitsTest := []int{9, 10, 16, 32, 60, 61, 62, 63, 64, 128, 253, 254, 255, 256, 512, 1024}
+	for _, v := range limitsTest {
+		name := fmt.Sprintf("limits-test-%d", v)
+		data := generateTestData(v)
+		values, ok := ValidateWhiteList(strings.Join(data, " "))
+		if !reflect.DeepEqual(data, values) {
+			t.Errorf("%s: expected validated data %+v, got %+v", name, data, values)
+		}
+		expectation := len(data) <= 61
+		if ok != expectation {
+			t.Errorf("%s: expected flag %+v, got %+v", name, expectation, ok)
+		}
+	}
+}