Make tcp keepalive optional on client -> server and server -> proxy connections, controllable via config. #82
Conversation
notsure2
force-pushed
the
tcp-keepalive
branch
from
2fe17ce
to
22ef310
Compare
notsure2
force-pushed
the
tcp-keepalive
branch
2 times, most recently
from
dede200
to
002d6a0
Compare
|
@cbeuw I would appreciate any comments :-) |
|
Sorry I forgot about the original pull request you made quite a few months ago. But thanks for the code. My main concern is that while this definitely improves connection stability, TCP keep-alive relies on sending keep-alive packets every so often. This may be used to detect Cloak, especially when most web browsers and web servers don't use keep-alive'd connections. I suggest to make this optional. There can be a |
|
I don't believe the client actually sends a steady stream of keep alive packets. It just sets socket option that tells the OS to send a keepalive packet after the period (15 seconds) of no activity. So in practice its effect is actually small. It should be noted that go already made this the default, so cloak has always been sending it anyway. Edit: I'll implement the option. With the default actually disabling keepalive. |
Thanks I didn't know that D: |
cmd/ck-client/ck-client.go
Outdated
| time.Sleep(time.Second * 3) | ||
| goto makeconn | ||
| } | ||
| err = remoteConn.(*net.TCPConn).SetKeepAlivePeriod(time.Duration(15 * time.Second)) |
There was a problem hiding this comment.
In https://golang.org/pkg/net/#Dialer it says when Diailer.KeepAlive (defined on line 35) is zero, it defaults to 15 seconds. Is this explicit setting still necessary?
There was a problem hiding this comment.
I like to be explicit about the timing in case of compiling with different go versions.
There was a problem hiding this comment.
I think this behaviour was added in Go 1.12: golang/go#23459. That's the minimum version needed to compile Cloak. In order to be explicit it may be better to set the KeepAlive field on line 35. Errors will be caught on line 43
cmd/ck-server/ck-server.go
Outdated
| @@ -180,6 +180,18 @@ func dispatchConnection(conn net.Conn, sta *server.State) { | |||
| user.CloseSession(ci.SessionId, "Failed to connect to proxy server") | |||
| continue | |||
| } | |||
| err = localConn.(*net.TCPConn).SetKeepAlive(true) | |||
There was a problem hiding this comment.
localConn could be a UDP conn. This timeout should be dealt with in Pipe
There was a problem hiding this comment.
This is TCP keepalive, not sending of data. I think in Pipe it is too late to set any socket options. I can just check if the connect is a TCPConn instead.
…ctions. Use KeepAlive value in config (seconds).
|
@cbeuw Can you check again ? It should now default to explicit off, otherwise it will be taken from KeepAlive config key. Value is expected to be in seconds. |
notsure2
changed the title
Hello,
This pull request just makes the default tcp keepalive since GO 1.13 of 15 seconds explicit.
It is enabled on the client app, in the client cloak --> remote cloak direction
and it is enabled on the server app in the server cloak --> server proxy direction.