koding / tunnel

Tunnel proxy package in Go
BSD 3-Clause "New" or "Revised" License
314 stars 70 forks source link

client: allow secure transport #20

Closed mmatczuk closed 8 years ago

mmatczuk commented 8 years ago

Adds ability for client to connect to server via HTTPS. Among other things this allows server for client authentication using a certificate validation.

rjeczalik commented 8 years ago

@mmatczuk You'll hate me, but could I ask you to try a different design please? What about adding Dial field to client and Listen field to server:

type ClientConfig struct {
    ...
    Dial func(network, addr string) (net.Conn, error)
    ...
}
type ServerConfig struct {
    ...
    Listen func(network, addr string) (net.Listener, error)
    ...
}

This way (tunnel.Client).Dial field can be used both to allow the user to configure dial timeout (net.Dialer) or use tls.Dial for secure transport. Or even replace it with in-mem conn pipe for tests. Flexibility.

The same with (tunnel.Server).Listen. If you have time, you could add NewServerTLS and NewClientTLS helper methods which would reduce the necessary boilerplate to get TLS working, something like:

func NewServerTLS(cfg *ServerConfig, tlsCfg *tls.Config) (*Server, error) {
   cfg.Listen = func(network, addr string) (net.Listener, error) {
       return tls.Listen(network, addr, tlsCfg)
   }

   return NewServer(cfg)
}

The only tricky part here would be using proper scheme for control connections here.

I guess it would be ok for now to guess the scheme basing on the net.Conn underlying type, if the connection comes from tls.Dial, then "https" should be used - something like:

...

var remoteURL string
if _, ok := conn.(*tls.Conn); ok {
    remoteURL = "https://" + conn.RemoteAddr().String() + controlPath
} else {
    remoteURL = "http://" + conn.RemoteAddr().String() + controlPath
}

...

What do you think?

mmatczuk commented 8 years ago

@rjeczalik I very much appreciate your proposals.

Client: the tunnel library lacks Hooks in general, I believe there should be a struct in ClientConfig holding all OnThis OnThat functions and possibly Dial could go there as well. On the other hand passing TLSConfig is a common practice in go stdlib.

Server: while it's true that Listen is analogous to Dial I think introducing proposed changes adds little value to the overall solution as tunnel.Server is in fact implementation of http.Handler and can be run using any http.Server you want.

If you want I could implement client part by introducing Hooks struct to ClientConfig and putting Dial there but I'm not convinced it's a best possible option.

rjeczalik commented 8 years ago

Client: the tunnel library lacks Hooks in general, I believe there should be a struct in ClientConfig holding all OnThis OnThat functions and possibly Dial could go there as well.

The client connection is in fact stateful - you can pass your own chan to listen on events, and in your code you can handle events you want. What more would you need here?

example test for event listening

Server: while it's true that Listen is analogous to Dial I think introducing proposed changes adds little value to the overall solution as tunnel.Server is in fact implementation of http.Handler and can be run using any http.Server you want.

Yup, you're right, forgot about that. Listen for ServerConfig makes no sense.

What about just adding Dial field then to ClientConfig?

mmatczuk commented 8 years ago

I must have missed this channel. I'll rewrite this to Dial no problem.

mmatczuk commented 8 years ago

@rjeczalik done

rjeczalik commented 8 years ago

@mmatczuk Thanks!