Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/tls: allow access to net.Conn underneath tls.Conn #29257

Closed
ja-nixi opened this issue Dec 14, 2018 · 45 comments
Closed

crypto/tls: allow access to net.Conn underneath tls.Conn #29257

ja-nixi opened this issue Dec 14, 2018 · 45 comments
Labels
FeatureRequest FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues Proposal-FinalCommentPeriod
Milestone

Comments

@ja-nixi
Copy link

ja-nixi commented Dec 14, 2018

Currently it is not possible to access FD in tls.Conn
in tls.Conn the underlying net.Conn is not accessible except in ClientHelloInfo

Senario:
in net/http we can't access the FD for https connections but we can access it for http connections
in a WebSocket connection after Hijack, when the connection is http we can use netpoll but when its https we can't

the only workaround right now is using a https to http proxy but it's not efficient as exposing the net.Conn on tls.Conn

/cc @FiloSottile

@ja-nixi
Copy link
Author

ja-nixi commented Dec 17, 2018

/cc @bradfitz

@bradfitz bradfitz added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest labels Dec 17, 2018
@bradfitz bradfitz added this to the Go1.13 milestone Dec 17, 2018
@bradfitz
Copy link
Contributor

/cc @ianlancetaylor

@odeke-em odeke-em changed the title crypto/tls: allow access to file descriptor for tls.Conn proposal: crypto/tls: allow access to file descriptor for tls.Conn Mar 7, 2019
@odeke-em
Copy link
Member

odeke-em commented Mar 7, 2019

I have marked this as a proposal for it to get the proposal treatment.

@ianlancetaylor
Copy link
Member

What do you actually want to do with the file descriptor?

@ianlancetaylor ianlancetaylor added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Mar 20, 2019
@andybons andybons added the Proposal-Crypto Proposal related to crypto packages or other security issues label Mar 27, 2019
@gopherbot
Copy link
Contributor

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@blakerouse
Copy link

It is useful to get the underlying connections file descriptor so that it can be passed into a syscall for epoll. This removes the needed to start a go routine for every websocket connection.

In my concurrent-websocket module I need to do just that so multiple websocket connections can be multiplexed through a pool of goroutines. I am currently working around this issue by using reflect and unsafe, which is really bad!

L26 below shows how I am getting the connection, and you can see on L45 how I pass that connection to netpoll.HandleReadOnce. It still uses the tls.Conn for reading and writing, but needs to pass the underlying connection for epoll to work.

https://github.com/blakerouse/concurrent-websocket/blob/master/channel.go#L26

@BernhardValenti
Copy link

This is also an issue when using TLS with the new http.Server ConnContext callback, in my case I want to get syscall.TCPInfo for each connection.

http.Server{
	ConnContext: func(ctx context.Context, c net.Conn) context.Context {
		// c is a tls.Conn, with apparently no way to access the actual connection
		return
	},
}

@odeke-em odeke-em reopened this Feb 5, 2020
@odeke-em
Copy link
Member

odeke-em commented Feb 5, 2020

Gopherbot, this is is still an issue.

@odeke-em odeke-em reopened this Feb 5, 2020
@odeke-em odeke-em modified the milestones: Go1.13, Backlog Feb 5, 2020
@andyx719

This comment has been minimized.

@tomerBZ
Copy link

tomerBZ commented May 25, 2020

Same issue here +1
The file descriptor is needed when working with epoll WebSocket

@jtorvald

This comment has been minimized.

@odeke-em odeke-em removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label May 26, 2020
@davecheney
Copy link
Contributor

Friends, rather than saying “I need this”, I’m sure the maintainers would find it vastly more useful to say why you need this. Be as specific and concrete as possible. Try to answer the question if this feature were added these are the specific ways it would enable me to do X which I currently cannot do. Be specific about the X, not just X the feature, but how you would change your code to use X.

Thank you

@lordspace
Copy link

hey, I need this because I am building a static file server and wanted to restart it gracefully without interruption

@davecheney
Copy link
Contributor

@lordspace have you investigated https://godoc.org/crypto/tls#Server?

@lordspace
Copy link

@davecheney thanks for the link. I think I've seen it but it's still not clear to me how to get the descriptor so I can pass it to the program again when it restarts.

@davecheney
Copy link
Contributor

To use tls.Server you would already hold the net.Conn for the listening socket.

@rsc
Copy link
Contributor

rsc commented Mar 31, 2021

Looking at the other rationales put forward for getting the underlying fd from the tls.Conn,

@FMLS:

I need to access Fd too, In public cloud environment, we need this fd to parse VPC infomation

Can you say more about this use case?

@LeGEC:

I have some code where I would like to call (*TCPConn).CloseWrite() from a TLS connection,

Can you say more about this use case? It's unclear to me whether half-closing a fd out from under TLS is well defined. Perhaps it is?

@bradfitz:

If you want to find stuck connections it can be useful to take the TLS fd and look it up in the kernel's socket debugging tables.

@jtorvald:

It's just that with a lot of connections the limiting of go routines is really helpful to keep memory consumption to a minimum (but yeah, I almost feel uncomfortable to write that in a reply to you ;-)).

I asked above: "What are the memory constraints you have where a goroutine with minimal stack is too much overhead? It seems like we'd be talking about something like 2-4 kB per connection."

It sounds like we need more information about use cases before we can figure out what the right API is.

@jtorvald
Copy link

@rsc I rest my case. If needed I will re-open it in the future when I have some real numbers.

To me it seemed as legit as any other optimization issue raised here. I appreciate all the work on making go faster, smaller and better. I thought a tiny change here could help safe memory and fulfill other people's use cases. If it's considered to impactful or not worth it then it's fine by me. Thanks for your time.

@rsc
Copy link
Contributor

rsc commented Apr 7, 2021

Ping @FMLS and @LeGEC if you have anything to add in response to the questions in #29257 (comment).

@rsc
Copy link
Contributor

rsc commented Apr 7, 2021

Not retitling (yet), but worth pointing out that not every tls.Conn has a file descriptor, so really this would be about allowing a tls.Conn to return the underlying net.Conn.

@LeGEC
Copy link

LeGEC commented Apr 8, 2021

Our use case :

We wrote a proxy, which connects to our server, and allows us to multiplex individual connections (conn1, conn2 in the diagram) to external service(s) (service1, service2) over a single real connection (realConn).

  Server (ours)                       Proxy (ours)     |  Service (not ours)
+--------------+                    +--------------+   |
|              |                    |              |   |
|   conn1 -*   |      realConn      |   *- prox1 -------- service1
|    (tls)  \  +--------------------+  /   (blind) |   |   (tls)
|            --------------------------            |   |
|            --------------------------            |   |
|           /  +--------------------+  \           |   |
|   conn2 -*   |                    |   *- prox2 -------- service2
|    (tls)     |                    |      (blind) |   |    (tls)
+--------------+                    +--------------+   |

We wrote a simple (simplish ?) protocol where each packet of data gets sent over realConn along with an id that identifies the connection, and also allows to exchange "out of band" messages.
For example :

  • the Proxy can send a message stating "when I tried to write on connection prox2, I got the following error : ...",
  • the Server can send a message "close connection prox1, I won't use it anymore".

Our proxy is generic, and doesn't understand the protocol that transits over the connection ; when connections to the Service require TLS, our proxy is completely "blind" regarding the state of the TLS connection.

Our "out of band" messages are important for bookkeeping : that's what allows us to control that connections and resources are actually released at some point (on the Server side as well as on the Proxy side).

Connection to this issue :

  • I first wanted to test what happened if I called .CloseWrite() on the TCP connection of a regular TLS connection, and realized it wasn't as straightforward as I thought ;
  • in our actual use case, the underlying connection is not a net.TCPConn, it is a custom ourpkg.ProxiedConn ;
  • our more generic need is to be able to send "out of band" messages to our Proxy on this connection.

A sketch of implementation I had in mind was : in some use cases, call something like .CloseWrite(), to indicate that the Server will not send any more information on that connection, but keep the reading end open to see if more information come downstream.

After further thoughts, and reading the other comments in this thread, I realize calling shutdown(fd, SHUT_WR) at the TCP level of a TLS connection is perhaps not a great idea ;

but to fulfill our need to send "out of band" messages to our proxy, we have to keep together in a struct both the "clear" connection and the tls connection, in places where accessing the underlying net.Conn (exactly as @rsc pointed out in his last comment) would seem pretty natural.

one extra point : this is not a show stopper, since we can implement what we want, in our case it would be more of a nice to have.

@rsc rsc changed the title proposal: crypto/tls: allow access to file descriptor for tls.Conn proposal: crypto/tls: allow access to net.Conn underneath tls.Conn Apr 14, 2021
@rsc
Copy link
Contributor

rsc commented Apr 14, 2021

Thanks for the details @LeGEC. If we're to commit to exposing the underlying net.Conn, it would be best to have a setting where that's strictly required, as opposed to just "a little nicer".

@FMLS, can you say more about the Cloud VPC usage?

@rsc
Copy link
Contributor

rsc commented Apr 21, 2021

In November, @FMLS wrote:

I need to access Fd too, In public cloud environment, we need this fd to parse VPC infomation

Does anyone know what public cloud APIs work in terms of file descriptors (which would potentially justify accepting this proposal)? Thanks.

@agnivade
Copy link
Contributor

agnivade commented Apr 22, 2021

I just came across this and wanted to share my experience. Forgive me for reiterating on something which has already been discussed, but I want to go back to the websockets and fd use case.

My use case was similar to OP. I wanted to revamp the websocket architecture for Mattermost using a single epoll instance and get rid of all the reader goroutines. I actually don't require full access to net.Conn, just being able to get the fd so that I can stick it into the epoll instance should be good enough for me.

Epoll is not a good enough reason to expose the file descriptor.

Personally, it is a matter of API consistency to me. TCPConn, UDPConn, IPConn all of them have the File method. I would expect tls.Conn to have it too.

worth pointing out that not every tls.Conn has a file descriptor, so really this would be about allowing a tls.Conn to return the underlying net.Conn.

Fair point. In that case, I would expect it to return a nil.

What are the memory constraints you have where a goroutine with minimal stack is too much overhead? It seems like we'd be talking about something like 2-4 kB per connection.

The argument is similar to the fact that while goroutines are cheap, they are not free. Having 100,000 connections (which is not that implausible) costs 400MB, which starts to matter in a multi-tenant Cloud environment.

But more importantly, it feels a bit weird to me that we can have epoll instances for HTTP servers, but not HTTPS servers (without resorting to hacks). If the argument is that goroutines are cheap, and we should let the runtime do epoll, it should apply to TCPConn as well.

EDIT: Just realized another use-case that I have. I want to call SetNoDelay on a TLS websocket connection. That's also not possible in the current state of things.

@rsc
Copy link
Contributor

rsc commented Apr 28, 2021

SetNoDelay and maybe SetKeepAlive seem like the strongest arguments so far.
If we just provide access to the net.Conn then people can use the net.Conn to get an fd if they really need it.
This seems OK.

Does anyone object to adding

package tls
func (c *Conn) NetConn() net.Conn

?

/cc @FiloSottile

@rsc
Copy link
Contributor

rsc commented May 5, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented May 12, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: crypto/tls: allow access to net.Conn underneath tls.Conn crypto/tls: allow access to net.Conn underneath tls.Conn May 12, 2021
@rsc rsc modified the milestones: Proposal, Backlog May 12, 2021
@jarrettyeo
Copy link

@rsc Thank you for fixing this, when will this issue be resolved?

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/325250 mentions this issue: crypto/tls: add NetConn method

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues Proposal-FinalCommentPeriod
Projects
None yet
Development

No branches or pull requests