From 8d99af1872c5ec36f00e7b47f5a38f5369fc3f96 Mon Sep 17 00:00:00 2001 From: Chotiwat Chawannakul Date: Wed, 27 Sep 2023 20:14:24 -0700 Subject: [PATCH 01/10] fix corrupted hostname from partial connection read --- pkg/tcpproxy/tcp.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/pkg/tcpproxy/tcp.go b/pkg/tcpproxy/tcp.go index fba4d21be8..65923aa209 100644 --- a/pkg/tcpproxy/tcp.go +++ b/pkg/tcpproxy/tcp.go @@ -62,7 +62,15 @@ func (p *TCPProxy) Handle(conn net.Conn) { // See: https://www.ibm.com/docs/en/ztpf/1.1.0.15?topic=sessions-ssl-record-format data := make([]byte, 16384) - length, err := conn.Read(data) + // read the tls header first + _, err := io.ReadFull(conn, data[:parser.TLSHeaderLength]) + if err != nil { + klog.V(4).ErrorS(err, "Error reading TLS header from the connection") + return + } + // get the total data length then read the rest + length := int(data[3])<<8 + int(data[4]) + parser.TLSHeaderLength + _, err = io.ReadFull(conn, data[parser.TLSHeaderLength:length]) if err != nil { klog.V(4).ErrorS(err, "Error reading data from the connection") return @@ -115,7 +123,7 @@ func (p *TCPProxy) Handle(conn net.Conn) { } else { _, err = clientConn.Write(data[:length]) if err != nil { - klog.Errorf("Error writing the first 4k of proxy data: %v", err) + klog.Errorf("Error writing the first %d bytes of proxy data: %v", length, err) clientConn.Close() } } From 6473481c85accafe56bcf2ee58fd851102fb8657 Mon Sep 17 00:00:00 2001 From: Chotiwat Chawannakul Date: Thu, 15 Feb 2024 17:51:53 -0800 Subject: [PATCH 02/10] fix e2e deployment missing custom env vars --- test/e2e/framework/deployment.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/framework/deployment.go b/test/e2e/framework/deployment.go index d6e59f18a9..29b60ab276 100644 --- a/test/e2e/framework/deployment.go +++ b/test/e2e/framework/deployment.go @@ -527,7 +527,7 @@ func newDeployment(name, namespace, image string, port int32, replicas int32, co { Name: name, Image: image, - Env: []corev1.EnvVar{}, + Env: env, Ports: []corev1.ContainerPort{ { Name: "http", From 8292585bb4b9b6264a1363e908b1272e57631476 Mon Sep 17 00:00:00 2001 From: Chotiwat Chawannakul Date: Fri, 16 Feb 2024 01:38:01 -0800 Subject: [PATCH 03/10] add e2e tests --- test/e2e/framework/httpexpect/request.go | 35 ++- test/e2e/settings/ssl_passthrough.go | 294 +++++++++++++++-------- 2 files changed, 221 insertions(+), 108 deletions(-) diff --git a/test/e2e/framework/httpexpect/request.go b/test/e2e/framework/httpexpect/request.go index 4daba136e4..c3854a6eb3 100644 --- a/test/e2e/framework/httpexpect/request.go +++ b/test/e2e/framework/httpexpect/request.go @@ -80,22 +80,41 @@ func (h *HTTPRequest) ForceResolve(ip string, port uint16) *HTTPRequest { h.chain.fail(fmt.Sprintf("invalid ip address: %s", ip)) return h } - dialer := &net.Dialer{ - Timeout: h.client.Timeout, - KeepAlive: h.client.Timeout, - DualStack: true, - } resolveAddr := fmt.Sprintf("%s:%d", ip, int(port)) + return h.WithDialContextMiddleware(func(next DialContextFunc) DialContextFunc { + return func(ctx context.Context, network, _ string) (net.Conn, error) { + return next(ctx, network, resolveAddr) + } + }) +} + +// DialContextFunc is the function signature for `DialContext` +type DialContextFunc func(ctx context.Context, network, addr string) (net.Conn, error) + +// WithDialContextMiddleware sets the `DialContext` function of the client +// transport with a new function returns from `fn`. An existing `DialContext` +// is passed into `fn` so the new function can act as a middleware by calling +// the old one. +func (h *HTTPRequest) WithDialContextMiddleware(fn func(next DialContextFunc) DialContextFunc) *HTTPRequest { oldTransport, ok := h.client.Transport.(*http.Transport) if !ok { h.chain.fail("invalid old transport address") return h } - newTransport := oldTransport.Clone() - newTransport.DialContext = func(ctx context.Context, network, _ string) (net.Conn, error) { - return dialer.DialContext(ctx, network, resolveAddr) + var nextDialContext DialContextFunc + if oldTransport.DialContext != nil { + nextDialContext = oldTransport.DialContext + } else { + dialer := &net.Dialer{ + Timeout: h.client.Timeout, + KeepAlive: h.client.Timeout, + DualStack: true, + } + nextDialContext = dialer.DialContext } + newTransport := oldTransport.Clone() + newTransport.DialContext = fn(nextDialContext) h.client.Transport = newTransport return h } diff --git a/test/e2e/settings/ssl_passthrough.go b/test/e2e/settings/ssl_passthrough.go index b10511bde4..2aa11eb070 100644 --- a/test/e2e/settings/ssl_passthrough.go +++ b/test/e2e/settings/ssl_passthrough.go @@ -27,10 +27,10 @@ import ( "github.com/stretchr/testify/assert" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/ingress-nginx/test/e2e/framework" + "k8s.io/ingress-nginx/test/e2e/framework/httpexpect" ) var _ = framework.IngressNginxDescribe("[Flag] enable-ssl-passthrough", func() { @@ -75,114 +75,208 @@ var _ = framework.IngressNginxDescribe("[Flag] enable-ssl-passthrough", func() { Status(http.StatusNotFound) }) - ginkgo.It("should pass unknown traffic to default backend and handle known traffic", func() { + ginkgo.Context("when handling traffic", func() { + var tlsConfig *tls.Config host := "testpassthrough.com" echoName := "echopass" + secretName := host + + ginkgo.BeforeEach(func() { + /* Even with enable-ssl-passthrough enabled, only annotated ingresses may receive the traffic */ + annotations := map[string]string{ + "nginx.ingress.kubernetes.io/ssl-passthrough": "true", + } - /* Even with enable-ssl-passthrough enabled, only annotated ingresses may receive the traffic */ - annotations := map[string]string{ - "nginx.ingress.kubernetes.io/ssl-passthrough": "true", - } - - ingressDef := framework.NewSingleIngressWithTLS(host, - "/", - host, - []string{host}, - f.Namespace, - echoName, - 80, - annotations) - tlsConfig, err := framework.CreateIngressTLSSecret(f.KubeClientSet, - ingressDef.Spec.TLS[0].Hosts, - ingressDef.Spec.TLS[0].SecretName, - ingressDef.Namespace) - - volumeMount := []corev1.VolumeMount{ - { - Name: "certs", - ReadOnly: true, - MountPath: "/certs", - }, - } - volume := []corev1.Volume{ - { - Name: "certs", - VolumeSource: corev1.VolumeSource{ - Secret: &corev1.SecretVolumeSource{ - SecretName: ingressDef.Spec.TLS[0].SecretName, + ingressDef := framework.NewSingleIngress(host, + "/", + host, + f.Namespace, + echoName, + 80, + annotations) + var err error + tlsConfig, err = framework.CreateIngressTLSSecret(f.KubeClientSet, + []string{host}, + secretName, + ingressDef.Namespace) + + volumeMount := []corev1.VolumeMount{ + { + Name: "certs", + ReadOnly: true, + MountPath: "/certs", + }, + } + volume := []corev1.Volume{ + { + Name: "certs", + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{ + SecretName: secretName, + }, }, }, - }, - } - envs := []corev1.EnvVar{ - { - Name: "HTTPBUN_SSL_CERT", - Value: "/certs/tls.crt", - }, - { - Name: "HTTPBUN_SSL_KEY", - Value: "/certs/tls.key", - }, - } - - f.NewDeploymentWithOpts("echopass", - framework.HTTPBunImage, - 80, - 1, - nil, - nil, - envs, - volumeMount, - volume, - false) - - f.EnsureIngress(ingressDef) - - assert.Nil(ginkgo.GinkgoT(), err) - framework.WaitForTLS(f.GetURL(framework.HTTPS), tlsConfig) - - f.WaitForNginxServer(host, - func(server string) bool { - return strings.Contains(server, "listen 442") - }) + } + envs := []corev1.EnvVar{ + { + Name: "HTTPBUN_SSL_CERT", + Value: "/certs/tls.crt", + }, + { + Name: "HTTPBUN_SSL_KEY", + Value: "/certs/tls.key", + }, + } - /* This one should not receive traffic as it does not contain passthrough annotation */ - hostBad := "noannotationnopassthrough.com" - ingBad := f.EnsureIngress(framework.NewSingleIngressWithTLS(hostBad, - "/", - hostBad, - []string{hostBad}, - f.Namespace, - echoName, - 80, - nil)) - tlsConfigBad, err := framework.CreateIngressTLSSecret(f.KubeClientSet, - ingBad.Spec.TLS[0].Hosts, - ingBad.Spec.TLS[0].SecretName, - ingBad.Namespace) - assert.Nil(ginkgo.GinkgoT(), err) - framework.WaitForTLS(f.GetURL(framework.HTTPS), tlsConfigBad) - - f.WaitForNginxServer(hostBad, - func(server string) bool { - return strings.Contains(server, "listen 442") + f.NewDeploymentWithOpts("echopass", + framework.HTTPBunImage, + 80, + 1, + nil, + nil, + envs, + volumeMount, + volume, + false) + + f.EnsureIngress(ingressDef) + + assert.Nil(ginkgo.GinkgoT(), err) + framework.WaitForTLS(f.GetURL(framework.HTTPS), tlsConfig) + + f.WaitForNginxServer(host, + func(server string) bool { + return strings.Contains(server, "listen 442") + }) + }) + + ginkgo.It("should pass unknown traffic to default backend and handle known traffic", func() { + /* This one should not receive traffic as it does not contain passthrough annotation */ + hostBad := "noannotationnopassthrough.com" + ingBad := f.EnsureIngress(framework.NewSingleIngressWithTLS(hostBad, + "/", + hostBad, + []string{hostBad}, + f.Namespace, + echoName, + 80, + nil)) + tlsConfigBad, err := framework.CreateIngressTLSSecret(f.KubeClientSet, + ingBad.Spec.TLS[0].Hosts, + ingBad.Spec.TLS[0].SecretName, + ingBad.Namespace) + assert.Nil(ginkgo.GinkgoT(), err) + framework.WaitForTLS(f.GetURL(framework.HTTPS), tlsConfigBad) + + f.WaitForNginxServer(hostBad, + func(server string) bool { + return strings.Contains(server, "listen 442") + }) + + //nolint:gosec // Ignore the gosec error in testing + f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). + GET("/"). + WithURL("https://"+net.JoinHostPort(host, "443")). + ForceResolve(f.GetNginxIP(), 443). + Expect(). + Status(http.StatusOK) + + //nolint:gosec // Ignore the gosec error in testing + f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: hostBad, InsecureSkipVerify: true}). + GET("/"). + WithURL("https://"+net.JoinHostPort(hostBad, "443")). + ForceResolve(f.GetNginxIP(), 443). + Expect(). + Status(http.StatusNotFound) + + //nolint:gosec // Ignore the gosec error in testing + f.HTTPTestClientWithTLSConfig(tlsConfig). + GET("/"). + WithURL("https://"+net.JoinHostPort(host, "443")). + ForceResolve(f.GetNginxIP(), 443). + Expect(). + Status(http.StatusOK) + + //nolint:gosec // Ignore the gosec error in testing + f.HTTPTestClientWithTLSConfig(tlsConfigBad). + GET("/"). + WithURL("https://"+net.JoinHostPort(hostBad, "443")). + ForceResolve(f.GetNginxIP(), 443). + Expect(). + Status(http.StatusNotFound) + }) + + ginkgo.Context("on throttled connections", func() { + throttleMiddleware := func(next httpexpect.DialContextFunc) httpexpect.DialContextFunc { + return func(ctx context.Context, network, addr string) (net.Conn, error) { + // Wrap the connection with a throttled writer to simulate real + // world traffic where streaming data may arrive in chunks + conn, err := next(ctx, network, addr) + return &writeThrottledConn{ + Conn: conn, + chunkSize: 50, + }, err + } + } + tries := 3 + + ginkgo.It("should handle known traffic without Host header", func() { + for i := 0; i < tries; i++ { + //nolint:gosec // Ignore the gosec error in testing + f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). + GET("/"). + WithURL("https://"+net.JoinHostPort(host, "443")). + ForceResolve(f.GetNginxIP(), 443). + WithDialContextMiddleware(throttleMiddleware). + Expect(). + Status(http.StatusOK) + } }) - //nolint:gosec // Ignore the gosec error in testing - f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). - GET("/"). - WithURL("https://"+net.JoinHostPort(host, "443")). - ForceResolve(f.GetNginxIP(), 443). - Expect(). - Status(http.StatusOK) + ginkgo.It("should handle known traffic with Host header", func() { + for i := 0; i < tries; i++ { + //nolint:gosec // Ignore the gosec error in testing + f.HTTPTestClientWithTLSConfig(tlsConfig). + GET("/"). + WithURL("https://"+net.JoinHostPort(host, "443")). + WithHeader("Host", host). + ForceResolve(f.GetNginxIP(), 443). + WithDialContextMiddleware(throttleMiddleware). + Expect(). + Status(http.StatusOK) + } + }) - //nolint:gosec // Ignore the gosec error in testing - f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: hostBad, InsecureSkipVerify: true}). - GET("/"). - WithURL("https://"+net.JoinHostPort(hostBad, "443")). - ForceResolve(f.GetNginxIP(), 443). - Expect(). - Status(http.StatusNotFound) + ginkgo.It("should handle insecure traffic with Host header", func() { + for i := 0; i < tries; i++ { + //nolint:gosec // Ignore the gosec error in testing + f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). + GET("/"). + WithURL("https://"+net.JoinHostPort(host, "443")). + WithHeader("Host", host). + ForceResolve(f.GetNginxIP(), 443). + WithDialContextMiddleware(throttleMiddleware). + Expect(). + Status(http.StatusOK) + } + }) + }) }) }) }) + +type writeThrottledConn struct { + net.Conn + chunkSize int +} + +// Write writes data to the connection `chunkSize` bytes (or less) at a time. +func (c *writeThrottledConn) Write(b []byte) (n int, err error) { + for i := 0; i < len(b); i += c.chunkSize { + n, err := c.Conn.Write(b[i:min(i+c.chunkSize, len(b))]) + if err != nil { + return i + n, err + } + } + return len(b), nil +} From c97bcbb8ce4fa329785baab4c2866aafc5c9c47e Mon Sep 17 00:00:00 2001 From: Chotiwat Chawannakul Date: Fri, 16 Feb 2024 01:56:17 -0800 Subject: [PATCH 04/10] fix lint --- test/e2e/settings/ssl_passthrough.go | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/test/e2e/settings/ssl_passthrough.go b/test/e2e/settings/ssl_passthrough.go index 2aa11eb070..51a64991a0 100644 --- a/test/e2e/settings/ssl_passthrough.go +++ b/test/e2e/settings/ssl_passthrough.go @@ -78,6 +78,7 @@ var _ = framework.IngressNginxDescribe("[Flag] enable-ssl-passthrough", func() { ginkgo.Context("when handling traffic", func() { var tlsConfig *tls.Config host := "testpassthrough.com" + url := "https://" + net.JoinHostPort(host, "443") echoName := "echopass" secretName := host @@ -153,6 +154,7 @@ var _ = framework.IngressNginxDescribe("[Flag] enable-ssl-passthrough", func() { ginkgo.It("should pass unknown traffic to default backend and handle known traffic", func() { /* This one should not receive traffic as it does not contain passthrough annotation */ hostBad := "noannotationnopassthrough.com" + urlBad := "https://" + net.JoinHostPort(hostBad, "443") ingBad := f.EnsureIngress(framework.NewSingleIngressWithTLS(hostBad, "/", hostBad, @@ -173,34 +175,30 @@ var _ = framework.IngressNginxDescribe("[Flag] enable-ssl-passthrough", func() { return strings.Contains(server, "listen 442") }) - //nolint:gosec // Ignore the gosec error in testing f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). GET("/"). - WithURL("https://"+net.JoinHostPort(host, "443")). + WithURL(url). ForceResolve(f.GetNginxIP(), 443). Expect(). Status(http.StatusOK) - //nolint:gosec // Ignore the gosec error in testing f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: hostBad, InsecureSkipVerify: true}). GET("/"). - WithURL("https://"+net.JoinHostPort(hostBad, "443")). + WithURL(urlBad). ForceResolve(f.GetNginxIP(), 443). Expect(). Status(http.StatusNotFound) - //nolint:gosec // Ignore the gosec error in testing f.HTTPTestClientWithTLSConfig(tlsConfig). GET("/"). - WithURL("https://"+net.JoinHostPort(host, "443")). + WithURL(url). ForceResolve(f.GetNginxIP(), 443). Expect(). Status(http.StatusOK) - //nolint:gosec // Ignore the gosec error in testing f.HTTPTestClientWithTLSConfig(tlsConfigBad). GET("/"). - WithURL("https://"+net.JoinHostPort(hostBad, "443")). + WithURL(urlBad). ForceResolve(f.GetNginxIP(), 443). Expect(). Status(http.StatusNotFound) @@ -222,10 +220,9 @@ var _ = framework.IngressNginxDescribe("[Flag] enable-ssl-passthrough", func() { ginkgo.It("should handle known traffic without Host header", func() { for i := 0; i < tries; i++ { - //nolint:gosec // Ignore the gosec error in testing f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). GET("/"). - WithURL("https://"+net.JoinHostPort(host, "443")). + WithURL(url). ForceResolve(f.GetNginxIP(), 443). WithDialContextMiddleware(throttleMiddleware). Expect(). @@ -235,10 +232,9 @@ var _ = framework.IngressNginxDescribe("[Flag] enable-ssl-passthrough", func() { ginkgo.It("should handle known traffic with Host header", func() { for i := 0; i < tries; i++ { - //nolint:gosec // Ignore the gosec error in testing f.HTTPTestClientWithTLSConfig(tlsConfig). GET("/"). - WithURL("https://"+net.JoinHostPort(host, "443")). + WithURL(url). WithHeader("Host", host). ForceResolve(f.GetNginxIP(), 443). WithDialContextMiddleware(throttleMiddleware). @@ -249,10 +245,9 @@ var _ = framework.IngressNginxDescribe("[Flag] enable-ssl-passthrough", func() { ginkgo.It("should handle insecure traffic with Host header", func() { for i := 0; i < tries; i++ { - //nolint:gosec // Ignore the gosec error in testing f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). GET("/"). - WithURL("https://"+net.JoinHostPort(host, "443")). + WithURL(url). WithHeader("Host", host). ForceResolve(f.GetNginxIP(), 443). WithDialContextMiddleware(throttleMiddleware). From 0f4804078374c1cfd4d2ebe3cea37819ed236b0c Mon Sep 17 00:00:00 2001 From: Chotiwat Chawannakul Date: Fri, 16 Feb 2024 02:00:38 -0800 Subject: [PATCH 05/10] fix tests --- test/e2e/settings/ssl_passthrough.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/e2e/settings/ssl_passthrough.go b/test/e2e/settings/ssl_passthrough.go index 51a64991a0..bde53c557d 100644 --- a/test/e2e/settings/ssl_passthrough.go +++ b/test/e2e/settings/ssl_passthrough.go @@ -175,6 +175,7 @@ var _ = framework.IngressNginxDescribe("[Flag] enable-ssl-passthrough", func() { return strings.Contains(server, "listen 442") }) + //nolint:gosec // Ignore the gosec error in testing f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). GET("/"). WithURL(url). @@ -182,6 +183,7 @@ var _ = framework.IngressNginxDescribe("[Flag] enable-ssl-passthrough", func() { Expect(). Status(http.StatusOK) + //nolint:gosec // Ignore the gosec error in testing f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: hostBad, InsecureSkipVerify: true}). GET("/"). WithURL(urlBad). @@ -220,6 +222,19 @@ var _ = framework.IngressNginxDescribe("[Flag] enable-ssl-passthrough", func() { ginkgo.It("should handle known traffic without Host header", func() { for i := 0; i < tries; i++ { + f.HTTPTestClientWithTLSConfig(tlsConfig). + GET("/"). + WithURL(url). + ForceResolve(f.GetNginxIP(), 443). + WithDialContextMiddleware(throttleMiddleware). + Expect(). + Status(http.StatusOK) + } + }) + + ginkgo.It("should handle insecure traffic without Host header", func() { + for i := 0; i < tries; i++ { + //nolint:gosec // Ignore the gosec error in testing f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). GET("/"). WithURL(url). @@ -245,6 +260,7 @@ var _ = framework.IngressNginxDescribe("[Flag] enable-ssl-passthrough", func() { ginkgo.It("should handle insecure traffic with Host header", func() { for i := 0; i < tries; i++ { + //nolint:gosec // Ignore the gosec error in testing f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). GET("/"). WithURL(url). From bde121a429a7f1d680a89efe68eaa2359dc52168 Mon Sep 17 00:00:00 2001 From: Chotiwat Chawannakul Date: Fri, 16 Feb 2024 02:24:33 -0800 Subject: [PATCH 06/10] use MustPassRepeatedly --- test/e2e/settings/ssl_passthrough.go | 80 +++++++++++++--------------- 1 file changed, 36 insertions(+), 44 deletions(-) diff --git a/test/e2e/settings/ssl_passthrough.go b/test/e2e/settings/ssl_passthrough.go index bde53c557d..2421d4a906 100644 --- a/test/e2e/settings/ssl_passthrough.go +++ b/test/e2e/settings/ssl_passthrough.go @@ -220,56 +220,48 @@ var _ = framework.IngressNginxDescribe("[Flag] enable-ssl-passthrough", func() { } tries := 3 - ginkgo.It("should handle known traffic without Host header", func() { - for i := 0; i < tries; i++ { - f.HTTPTestClientWithTLSConfig(tlsConfig). - GET("/"). - WithURL(url). - ForceResolve(f.GetNginxIP(), 443). - WithDialContextMiddleware(throttleMiddleware). - Expect(). - Status(http.StatusOK) - } + ginkgo.It("should handle known traffic without Host header", ginkgo.MustPassRepeatedly(tries), func() { + f.HTTPTestClientWithTLSConfig(tlsConfig). + GET("/"). + WithURL(url). + ForceResolve(f.GetNginxIP(), 443). + WithDialContextMiddleware(throttleMiddleware). + Expect(). + Status(http.StatusOK) }) - ginkgo.It("should handle insecure traffic without Host header", func() { - for i := 0; i < tries; i++ { - //nolint:gosec // Ignore the gosec error in testing - f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). - GET("/"). - WithURL(url). - ForceResolve(f.GetNginxIP(), 443). - WithDialContextMiddleware(throttleMiddleware). - Expect(). - Status(http.StatusOK) - } + ginkgo.It("should handle insecure traffic without Host header", ginkgo.MustPassRepeatedly(tries), func() { + //nolint:gosec // Ignore the gosec error in testing + f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). + GET("/"). + WithURL(url). + ForceResolve(f.GetNginxIP(), 443). + WithDialContextMiddleware(throttleMiddleware). + Expect(). + Status(http.StatusOK) }) - ginkgo.It("should handle known traffic with Host header", func() { - for i := 0; i < tries; i++ { - f.HTTPTestClientWithTLSConfig(tlsConfig). - GET("/"). - WithURL(url). - WithHeader("Host", host). - ForceResolve(f.GetNginxIP(), 443). - WithDialContextMiddleware(throttleMiddleware). - Expect(). - Status(http.StatusOK) - } + ginkgo.It("should handle known traffic with Host header", ginkgo.MustPassRepeatedly(tries), func() { + f.HTTPTestClientWithTLSConfig(tlsConfig). + GET("/"). + WithURL(url). + WithHeader("Host", host). + ForceResolve(f.GetNginxIP(), 443). + WithDialContextMiddleware(throttleMiddleware). + Expect(). + Status(http.StatusOK) }) - ginkgo.It("should handle insecure traffic with Host header", func() { - for i := 0; i < tries; i++ { - //nolint:gosec // Ignore the gosec error in testing - f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). - GET("/"). - WithURL(url). - WithHeader("Host", host). - ForceResolve(f.GetNginxIP(), 443). - WithDialContextMiddleware(throttleMiddleware). - Expect(). - Status(http.StatusOK) - } + ginkgo.It("should handle insecure traffic with Host header", ginkgo.MustPassRepeatedly(tries), func() { + //nolint:gosec // Ignore the gosec error in testing + f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). + GET("/"). + WithURL(url). + WithHeader("Host", host). + ForceResolve(f.GetNginxIP(), 443). + WithDialContextMiddleware(throttleMiddleware). + Expect(). + Status(http.StatusOK) }) }) }) From 525931b437716c5b4b5c1972cd23febe3fdb3836 Mon Sep 17 00:00:00 2001 From: Chotiwat Chawannakul Date: Fri, 16 Feb 2024 15:10:01 -0800 Subject: [PATCH 07/10] Revert "use MustPassRepeatedly" This reverts commit d042a373125426296ec502461f976d4daad1dc5b. --- test/e2e/settings/ssl_passthrough.go | 80 +++++++++++++++------------- 1 file changed, 44 insertions(+), 36 deletions(-) diff --git a/test/e2e/settings/ssl_passthrough.go b/test/e2e/settings/ssl_passthrough.go index 2421d4a906..bde53c557d 100644 --- a/test/e2e/settings/ssl_passthrough.go +++ b/test/e2e/settings/ssl_passthrough.go @@ -220,48 +220,56 @@ var _ = framework.IngressNginxDescribe("[Flag] enable-ssl-passthrough", func() { } tries := 3 - ginkgo.It("should handle known traffic without Host header", ginkgo.MustPassRepeatedly(tries), func() { - f.HTTPTestClientWithTLSConfig(tlsConfig). - GET("/"). - WithURL(url). - ForceResolve(f.GetNginxIP(), 443). - WithDialContextMiddleware(throttleMiddleware). - Expect(). - Status(http.StatusOK) + ginkgo.It("should handle known traffic without Host header", func() { + for i := 0; i < tries; i++ { + f.HTTPTestClientWithTLSConfig(tlsConfig). + GET("/"). + WithURL(url). + ForceResolve(f.GetNginxIP(), 443). + WithDialContextMiddleware(throttleMiddleware). + Expect(). + Status(http.StatusOK) + } }) - ginkgo.It("should handle insecure traffic without Host header", ginkgo.MustPassRepeatedly(tries), func() { - //nolint:gosec // Ignore the gosec error in testing - f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). - GET("/"). - WithURL(url). - ForceResolve(f.GetNginxIP(), 443). - WithDialContextMiddleware(throttleMiddleware). - Expect(). - Status(http.StatusOK) + ginkgo.It("should handle insecure traffic without Host header", func() { + for i := 0; i < tries; i++ { + //nolint:gosec // Ignore the gosec error in testing + f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). + GET("/"). + WithURL(url). + ForceResolve(f.GetNginxIP(), 443). + WithDialContextMiddleware(throttleMiddleware). + Expect(). + Status(http.StatusOK) + } }) - ginkgo.It("should handle known traffic with Host header", ginkgo.MustPassRepeatedly(tries), func() { - f.HTTPTestClientWithTLSConfig(tlsConfig). - GET("/"). - WithURL(url). - WithHeader("Host", host). - ForceResolve(f.GetNginxIP(), 443). - WithDialContextMiddleware(throttleMiddleware). - Expect(). - Status(http.StatusOK) + ginkgo.It("should handle known traffic with Host header", func() { + for i := 0; i < tries; i++ { + f.HTTPTestClientWithTLSConfig(tlsConfig). + GET("/"). + WithURL(url). + WithHeader("Host", host). + ForceResolve(f.GetNginxIP(), 443). + WithDialContextMiddleware(throttleMiddleware). + Expect(). + Status(http.StatusOK) + } }) - ginkgo.It("should handle insecure traffic with Host header", ginkgo.MustPassRepeatedly(tries), func() { - //nolint:gosec // Ignore the gosec error in testing - f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). - GET("/"). - WithURL(url). - WithHeader("Host", host). - ForceResolve(f.GetNginxIP(), 443). - WithDialContextMiddleware(throttleMiddleware). - Expect(). - Status(http.StatusOK) + ginkgo.It("should handle insecure traffic with Host header", func() { + for i := 0; i < tries; i++ { + //nolint:gosec // Ignore the gosec error in testing + f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). + GET("/"). + WithURL(url). + WithHeader("Host", host). + ForceResolve(f.GetNginxIP(), 443). + WithDialContextMiddleware(throttleMiddleware). + Expect(). + Status(http.StatusOK) + } }) }) }) From 6d8a4207d55a0471a75febdf7412d036a6da6bc7 Mon Sep 17 00:00:00 2001 From: Chotiwat Chawannakul Date: Fri, 16 Feb 2024 15:36:10 -0800 Subject: [PATCH 08/10] make chunkSize < len(host) and remove retries --- test/e2e/settings/ssl_passthrough.go | 75 ++++++++++++---------------- 1 file changed, 33 insertions(+), 42 deletions(-) diff --git a/test/e2e/settings/ssl_passthrough.go b/test/e2e/settings/ssl_passthrough.go index bde53c557d..75f50e4aac 100644 --- a/test/e2e/settings/ssl_passthrough.go +++ b/test/e2e/settings/ssl_passthrough.go @@ -214,62 +214,53 @@ var _ = framework.IngressNginxDescribe("[Flag] enable-ssl-passthrough", func() { conn, err := next(ctx, network, addr) return &writeThrottledConn{ Conn: conn, - chunkSize: 50, + chunkSize: len(host) / 3, }, err } } - tries := 3 ginkgo.It("should handle known traffic without Host header", func() { - for i := 0; i < tries; i++ { - f.HTTPTestClientWithTLSConfig(tlsConfig). - GET("/"). - WithURL(url). - ForceResolve(f.GetNginxIP(), 443). - WithDialContextMiddleware(throttleMiddleware). - Expect(). - Status(http.StatusOK) - } + f.HTTPTestClientWithTLSConfig(tlsConfig). + GET("/"). + WithURL(url). + ForceResolve(f.GetNginxIP(), 443). + WithDialContextMiddleware(throttleMiddleware). + Expect(). + Status(http.StatusOK) }) ginkgo.It("should handle insecure traffic without Host header", func() { - for i := 0; i < tries; i++ { - //nolint:gosec // Ignore the gosec error in testing - f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). - GET("/"). - WithURL(url). - ForceResolve(f.GetNginxIP(), 443). - WithDialContextMiddleware(throttleMiddleware). - Expect(). - Status(http.StatusOK) - } + //nolint:gosec // Ignore the gosec error in testing + f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). + GET("/"). + WithURL(url). + ForceResolve(f.GetNginxIP(), 443). + WithDialContextMiddleware(throttleMiddleware). + Expect(). + Status(http.StatusOK) }) ginkgo.It("should handle known traffic with Host header", func() { - for i := 0; i < tries; i++ { - f.HTTPTestClientWithTLSConfig(tlsConfig). - GET("/"). - WithURL(url). - WithHeader("Host", host). - ForceResolve(f.GetNginxIP(), 443). - WithDialContextMiddleware(throttleMiddleware). - Expect(). - Status(http.StatusOK) - } + f.HTTPTestClientWithTLSConfig(tlsConfig). + GET("/"). + WithURL(url). + WithHeader("Host", host). + ForceResolve(f.GetNginxIP(), 443). + WithDialContextMiddleware(throttleMiddleware). + Expect(). + Status(http.StatusOK) }) ginkgo.It("should handle insecure traffic with Host header", func() { - for i := 0; i < tries; i++ { - //nolint:gosec // Ignore the gosec error in testing - f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). - GET("/"). - WithURL(url). - WithHeader("Host", host). - ForceResolve(f.GetNginxIP(), 443). - WithDialContextMiddleware(throttleMiddleware). - Expect(). - Status(http.StatusOK) - } + //nolint:gosec // Ignore the gosec error in testing + f.HTTPTestClientWithTLSConfig(&tls.Config{ServerName: host, InsecureSkipVerify: true}). + GET("/"). + WithURL(url). + WithHeader("Host", host). + ForceResolve(f.GetNginxIP(), 443). + WithDialContextMiddleware(throttleMiddleware). + Expect(). + Status(http.StatusOK) }) }) }) From 1069c21677ca6e58333c4190412cb3cddab5cd7c Mon Sep 17 00:00:00 2001 From: Chotiwat Chawannakul Date: Fri, 25 Oct 2024 15:49:59 -0700 Subject: [PATCH 09/10] fix and bound buffer length, add documentation from PR 11843 by maxl99 --- pkg/tcpproxy/tcp.go | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/pkg/tcpproxy/tcp.go b/pkg/tcpproxy/tcp.go index 65923aa209..6780f40585 100644 --- a/pkg/tcpproxy/tcp.go +++ b/pkg/tcpproxy/tcp.go @@ -59,8 +59,21 @@ func (p *TCPProxy) Get(host string) *TCPServer { // and open a connection to the passthrough server. func (p *TCPProxy) Handle(conn net.Conn) { defer conn.Close() - // See: https://www.ibm.com/docs/en/ztpf/1.1.0.15?topic=sessions-ssl-record-format - data := make([]byte, 16384) + // [Documentation by @maxl99](https://github.com/kubernetes/ingress-nginx/pull/11843/files#diff-aef3e187fd37c68706ad582d7b89a2d9ad11691bd929a2158b86f93362244105R67-R79) + // It appears that the ClientHello must fit into *one* TLSPlaintext message: + // When a client first connects to a server, it is REQUIRED to send the ClientHello as its first TLS message. + // Source: https://datatracker.ietf.org/doc/html/rfc8446#section-4.1.2 + // + // length: The length (in bytes) of the following TLSPlaintext.fragment. The length MUST NOT exceed 2^14 bytes. + // An endpoint that receives a record that exceeds this length MUST terminate the connection with a "record_overflow" alert. + // Source: https://datatracker.ietf.org/doc/html/rfc8446#section-5.1 + // bytes 0 : content type + // bytes 1-2: legacy version + // bytes 3-4: length + // bytes 5+ : message + // https://en.wikipedia.org/wiki/Transport_Layer_Security#TLS_record + // Thus, we need to allocate 5 + 16384 bytes + data := make([]byte, parser.TLSHeaderLength+16384) // read the tls header first _, err := io.ReadFull(conn, data[:parser.TLSHeaderLength]) @@ -69,7 +82,7 @@ func (p *TCPProxy) Handle(conn net.Conn) { return } // get the total data length then read the rest - length := int(data[3])<<8 + int(data[4]) + parser.TLSHeaderLength + length := min(int(data[3])<<8+int(data[4])+parser.TLSHeaderLength, len(data)) _, err = io.ReadFull(conn, data[parser.TLSHeaderLength:length]) if err != nil { klog.V(4).ErrorS(err, "Error reading data from the connection") From 43bc60e4d118c63a672430e7c830947310fa506a Mon Sep 17 00:00:00 2001 From: Chotiwat Chawannakul Date: Thu, 31 Oct 2024 01:32:31 -0700 Subject: [PATCH 10/10] Update pkg/tcpproxy/tcp.go Co-authored-by: Marco Ebert --- pkg/tcpproxy/tcp.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/tcpproxy/tcp.go b/pkg/tcpproxy/tcp.go index 6780f40585..ae07c0c003 100644 --- a/pkg/tcpproxy/tcp.go +++ b/pkg/tcpproxy/tcp.go @@ -59,7 +59,6 @@ func (p *TCPProxy) Get(host string) *TCPServer { // and open a connection to the passthrough server. func (p *TCPProxy) Handle(conn net.Conn) { defer conn.Close() - // [Documentation by @maxl99](https://github.com/kubernetes/ingress-nginx/pull/11843/files#diff-aef3e187fd37c68706ad582d7b89a2d9ad11691bd929a2158b86f93362244105R67-R79) // It appears that the ClientHello must fit into *one* TLSPlaintext message: // When a client first connects to a server, it is REQUIRED to send the ClientHello as its first TLS message. // Source: https://datatracker.ietf.org/doc/html/rfc8446#section-4.1.2