TCP Proxy: Fix corrupted hostname from partial connection read. #10454

chotiwat · 2023-09-28T03:47:16Z

What this PR does / why we need it:

We have run into intermittent "HTTP request sent to an HTTPS server" errors from our SSL passthrough ingresses. We found that the passthrough proxy would sometimes read incomplete data from the connection and cause corrupted hostname.

When this happens, the connection handler would fall back to the default server at 127.0.0.1:442 and cause the error if the nginx.ingress.kubernetes.io/backend-protocol: HTTPS annotation is not specified.

This PR fixes the issue by making sure that we fully read the Client Hello data from the connection by getting the total length from the TLS header.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
CVE Report (Scanner found CVE and adding report)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation only

Which issue/s this PR fixes

fixes #11491
fixes #11424

How Has This Been Tested?

We built a custom image to debug the issue and added some debug logs. For example, we updated

ingress-nginx/pkg/tcpproxy/tcp.go

Line 74 in 4bac120

klog.V(4).InfoS("TLS Client Hello", "host", hostname)

to

klog.V(4).InfoS("TLS Client Hello", "host", hostname, "conn", fmt.Sprintf("%p", conn), "read-length", length, "total-length", int(data[3])<<8+int(data[4]))

Example log of corrupted data before the fix:

I0928 01:42:51.621260       7 tcp.go:74] "TLS Client Hello" host="\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" conn="0xc002db0500" read-length=244 total-length=330

We can reproduce the issue by running curl in a loop. I've seen around 5% of errors from my machine with the number of requests as low as 10 enough to trigger the error. This fix eliminates the errors completely for us.

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added unit and/or e2e tests to cover my changes.
All new and existing tests passed.

netlify · 2023-09-28T03:47:22Z

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	`43bc60e`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/672340a25e7c6400087f246f

k8s-ci-robot · 2023-09-28T03:47:25Z

Hi @chotiwat. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

chotiwat · 2023-09-29T19:29:17Z

/retest

k8s-ci-robot · 2023-09-29T19:29:31Z

@chotiwat: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

chotiwat · 2023-10-05T01:29:05Z

Hi @cpanato @tao12345666333, is there anything I need to do on my end to get this PR reviewed?

cpanato

i'm not familiar with this part of the code

maybe @rikatz can help

chotiwat · 2023-10-24T20:35:43Z

Hi @rikatz, would you be able to take a look at this PR when you get a chance?

chotiwat · 2024-02-05T20:26:19Z

Hi @rikatz, ping on this issue 🙏

strongjz · 2024-02-09T02:20:23Z

/retest

longwuyuan · 2024-02-09T02:55:58Z

Which issue number shows the corrupted hostname
Can the corrupted hostname problem be reproduced
Why does the problem NOT occur for multiple users of ssl-passthrough
What are details of why a backend-protocol is in play when ssl traffic is directly passed through to backend
Is there any tests written for this change
Is any e2e test suite going to be added or modified to check this change's impact

chotiwat · 2024-02-12T20:38:18Z

Hi @longwuyuan, I've answered your questions below:

Which issue number shows the corrupted hostname

None. I couldn't find any issue describing the same problem.

Can the corrupted hostname problem be reproduced

Yes, as described in the PR body.

Why does the problem NOT occur for multiple users of ssl-passthrough

I wonder that myself. I can hazard a few guesses:

SSL passthrough might not be a widely used feature
The errors might have been disregarded by others if they aren't violating their SLAs
Retry mechanisms might have masked the errors
People might not care enough to report the issue

This doesn't mean the problem doesn't occur for other users of SSL passthrough though.

What are details of why a backend-protocol is in play when ssl traffic is directly passed through to backend

The connection handler for the proxy falls back to the default NGINX backend (p.Default) when the Client Hello hostname is corrupted, because it cannot be found in p.ServerList.

When this happens, the default backend will terminate the TLS and send a new request to the upstream per its generated NGINX configuration block, hence the HTTP-sent-to-HTTPS error.

Setting backend-protocol to HTTPS is just a hack to at least avoid the error, but it's not a real passthrough since the TLS gets terminated then re-initiated.

ingress-nginx/pkg/tcpproxy/tcp.go

Lines 71 to 76 in 86f3af8

    
           proxy := p.Default 
        
           hostname, err := parser.GetHostname(data) 
        
           if err == nil { 
        
           	klog.V(4).InfoS("TLS Client Hello", "host", hostname) 
        
           	proxy = p.Get(hostname) 
        
           }

ingress-nginx/pkg/tcpproxy/tcp.go

Lines 43 to 56 in 86f3af8

    
           // Get returns the TCPServer to use for a given host. 
        
           func (p *TCPProxy) Get(host string) *TCPServer { 
        
           	if p.ServerList == nil { 
        
           		return p.Default 
        
           	} 
        
           	for _, s := range p.ServerList { 
        
           		if s.Hostname == host { 
        
           			return s 
        
           		} 
        
           	} 
        
           	return p.Default 
        
           }

Is there any tests written for this change

No, there isn't currently. I could add some though (see below).

Is any e2e test suite going to be added or modified to check this change's impact

I could modify the existing test suite if that sounds good to you. I didn't see it initially. It took some digging around the docs.

longwuyuan · 2024-02-12T22:41:00Z

Can you create a issue for this and link it here. If you do, then ensuring that the issue description helps a developer of the ingress-nginx controller to reduce the work on reproducing the issue, will go a long way. I am still unclear on how I would reproduce the issue, if i wanted to
The hope against hope is that there is at least a few other users who face the same issue because the fact is that there is a large number of users of ssl-passthrough feature and why nobody else has reported this until now is extremely odd
I am not a developer so I am lost at the "connection handler" info you provided. So waiting for a developer to comment on that. My limit is failing to understand why, a new backend HTTPS connection, from the controller to backend pod, is even being stated as occuring, when the ssl-passthrough annotation implies that the termination of thc onnection from client, is not on the controller but instead is directly passed-through-and-through to the backend pod

longwuyuan · 2024-02-12T22:43:18Z

And the e2e tests are a absolute requirement I think, based on the latest information you provided
thanks for the contribution

chotiwat · 2024-02-12T23:12:41Z

Ok, I'll go ahead and try to add an e2e test for this.

Can you create a issue for this and link it here. If you do, then ensuring that the issue description helps a developer of the ingress-nginx controller to reduce the work on reproducing the issue, will go a long way. I am still unclear on how I would reproduce the issue, if i wanted to

I believe I've explained as clearly as I could in this PR. If you insist, I can create an issue, which would be a copy of this PR body and my previous answers to your questions, but I personally don't see any point in doing so.

The mistake in the code should also be pretty clear from the developer's point of view as well. Hopefully, I'd be able to come up with an e2e test that would fail against the current main branch but would pass on this branch. If that's still not enough for the developers, then I can try to set up a minimum reproduction steps.

I am not a developer so I am lost at the "connection handler" info you provided. So waiting for a developer to comment on that.

Yes, let's do that.

chotiwat

I've added the e2e tests that would have failed without this fix, and cherry-picked just the tests to #10988.

Unfortunately, it seems that this issue is harder to reproduce on a kind cluster, perhaps because the traffic doesn't go through many hops and there are no other workloads contending for I/O, so I had to introduce virtual throttling to the tests.

The throttling is done by wrapping the connection returned from the client's DialContext with a net.Conn that writes to the connection at most chunkSize bytes.

These tests would sometimes pass without the fix from this PR so I added retries ~~with ginkgo.MustPassRepeatedly decorator~~ as well. Alternatively, we could make the chunkSize less than len(host) but I don't know if it would make the tests significantly slower.

chotiwat · 2024-02-16T21:26:22Z

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,


There was a bug in the e2e framework as well. This was preventing httpbun from getting the environment variables so it always ran as an HTTP server even though HTTPBUN_SSL_CERT and HTTPBUN_SSL_KEY are specified.

chotiwat · 2024-02-16T21:27:54Z