Race condition during SNI extraction when TLS ClientHello is fragmented leading to ssl-passthrough being "ignored"

[alshain]

I believe I stumbled over a problem related to tldr.fail, where SNI extraction might fail with large TLS ClientHellos and SSL-passthrough.

Due to a race condition when reading the buffer used for the SNI extraction, the extraction fails but the failure is ignored and we default to the default proxy target.

https://github.com/kubernetes/ingress-nginx/blob/44e550ea72f673fadeae0559a773feb9cbf3eec6/pkg/tcpproxy/tcp.go#L65

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)

    // vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
        // suppose the ClientHello is fragmented over multiple packets, 
        // data might only contain a partial ClientHello where the SNI data isn't present yet
    length, err := conn.Read(data)  
    if err != nil {
        klog.V(4).ErrorS(err, "Error reading data from the connection")
        return
    }

    proxy := p.Default  
    hostname, err := parser.GetHostname(data)
    // vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
    if err == nil {  // <--- err will not be nill, so we skip this, proxy remains p.Default
        klog.V(4).InfoS("TLS Client Hello", "host", hostname)
        proxy = p.Get(hostname)
    }

    // vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
    if proxy == nil {   // because we have p.Default, we skip this
        klog.V(4).InfoS("There is no configured proxy for SSL connections.")
        return
    }
    // vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
        // now we forward traffic to NGINX instead of the proper passthrough target

    hostPort := net.JoinHostPort(proxy.IP, fmt.Sprintf("%v", proxy.Port))

[k8s-ci-robot]

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.

[alshain]

Oh well, I had searched but couldn't find anything, already reported: https://github.com/kubernetes/ingress-nginx/issues/11424

Sorry about that.

[longwuyuan]

This is a complicated one so I think info coming from 2 is helpful