[
https://issues.apache.org/jira/browse/THRIFT-5996?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18080977#comment-18080977
]
Marin Nozhchev commented on THRIFT-5996:
----------------------------------------
Thanks for taking a look! Peeking without consuming data should be safe. If
peeking weren't safe, the existing connection check on plain connection could
corrupt the Thrift protocol.
Also MySQL has been calling syscall.Read on the TCP connection under a TLS
connection since 2019 - [https://github.com/go-sql-driver/mysql/pull/934] . I
don't see a difference in risk in syscall.Read and syscall.Recvfrom .
> go: connection check should work for TLS sockets
> ------------------------------------------------
>
> Key: THRIFT-5996
> URL: https://issues.apache.org/jira/browse/THRIFT-5996
> Project: Thrift
> Issue Type: Improvement
> Components: Go - Library
> Affects Versions: 0.23.0
> Reporter: Marin Nozhchev
> Priority: Major
>
> The connection check, implemented for THRIFT-5214 , works only for plain
> sockets, not TLS. This is a significant deficiency.
> Fortunately, the fix is simple and I happy to create a PR.
> github.com/go-sql-driver/mysql that initially inspired THRIFT-5214 had this
> working from the start - [https://github.com/go-sql-driver/mysql/pull/934] .
> Essentially, in case if TLS, the conn check must access the underlying TCP
> connection. In
> [https://github.com/apache/thrift/blob/master/lib/go/thrift/socket_unix_conn.go#L40]
> , checkConn() instead of
>
> {code:java}
> syscallConn, ok := sc.Conn.(syscall.Conn)
> if !ok {
> // No way to check, return nil
> return nil
> }{code}
> we should have
> {code:java}
> conn := sc.Conn
> if tlsConn, ok := conn.(*tls.Conn); ok {
> conn = tlsConn.NetConn()
> }
> syscallConn, ok := conn.(syscall.Conn)
> if !ok {
> ...{code}
> *tls.Conn itself doesn't implement syscall.Conn .
> The original implementation of THRIFT-5214 could not do this because
> NetConn() wasn't available at the time - it was added in Go 1.18. The mysql
> implementation managed without it because uses lower level API to open a TLS
> connection.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
