code review 37730043: gosshnew/ssh: introduce ssh.Conn, representing the conn...

gosshnew/ssh: introduce ssh.Conn, representing the connection protocol.

This adds NewServerConn and NewClientConn returning a ssh.Conn.

[hanwen-google, 2013-12-10 10:19:39]

Hello agl1, dave@cheney.net, jpsugar@google.com (cc: agl@golang.org,
dave@cheney.net, golang-dev@googlegroups.com, hanwen@google.com,
jpsugar@google.com),

I'd like you to review this change to
https://hanwen%40google.com@code.google.com/p/gosshnew/

[agl, 2013-12-11 20:52:47]

https://codereview.appspot.com/37730043/diff/160001/ssh/client.go
File ssh/client.go (right):

https://codereview.appspot.com/37730043/diff/160001/ssh/client.go#newcode34
ssh/client.go:34: // Client returns a new SSH client connection using c as the
underlying transport.
This comment is above the wrong function now. Also, after the move,
NewClientConn will need a comment. It should explain the when a user might want
Client(), when NewClientConn() and when Dial().

https://codereview.appspot.com/37730043/diff/160001/ssh/connection.go
File ssh/connection.go (right):

https://codereview.appspot.com/37730043/diff/160001/ssh/connection.go#newcode1
ssh/connection.go:1: package ssh
Copyright header.

https://codereview.appspot.com/37730043/diff/160001/ssh/connection.go#newcode8
ssh/connection.go:8: // OpenChannelError is returned if the other side rejects
our
s/our/an/

https://codereview.appspot.com/37730043/diff/160001/ssh/connection.go#newcode19
ssh/connection.go:19: // Conn represents an SSH connection. It is equal for
server and
// Conn represents an SSH connection for both client and server roles.

https://codereview.appspot.com/37730043/diff/160001/ssh/connection.go#newcode116
ssh/connection.go:116: type ConnMetadata interface {
Unclear why this exists. Why not put the contents directly into Conn?

[hanwen-google, 2013-12-12 10:38:19]

https://codereview.appspot.com/37730043/diff/160001/ssh/client.go
File ssh/client.go (right):

https://codereview.appspot.com/37730043/diff/160001/ssh/client.go#newcode34
ssh/client.go:34: // Client returns a new SSH client connection using c as the
underlying transport.
On 2013/12/11 20:52:47, agl wrote:
> This comment is above the wrong function now. Also, after the move,
> NewClientConn will need a comment. It should explain the when a user might
want
> Client(), when NewClientConn() and when Dial().

Done. 

I tried clarifying, not sure if I succeeded.

https://codereview.appspot.com/37730043/diff/160001/ssh/connection.go
File ssh/connection.go (right):

https://codereview.appspot.com/37730043/diff/160001/ssh/connection.go#newcode1
ssh/connection.go:1: package ssh
On 2013/12/11 20:52:47, agl wrote:
> Copyright header.

Done.

https://codereview.appspot.com/37730043/diff/160001/ssh/connection.go#newcode8
ssh/connection.go:8: // OpenChannelError is returned if the other side rejects
our
On 2013/12/11 20:52:47, agl wrote:
> s/our/an/

Done.

https://codereview.appspot.com/37730043/diff/160001/ssh/connection.go#newcode19
ssh/connection.go:19: // Conn represents an SSH connection. It is equal for
server and
On 2013/12/11 20:52:47, agl wrote:
> // Conn represents an SSH connection for both client and server roles.

Done.

https://codereview.appspot.com/37730043/diff/160001/ssh/connection.go#newcode116
ssh/connection.go:116: type ConnMetadata interface {
On 2013/12/11 20:52:47, agl wrote:
> Unclear why this exists. Why not put the contents directly into Conn?

It's passed into the server auth callbacks so they can enforce
per-user/per-address restrictions. Should I document that here?

[hanwen-google, 2013-12-12 11:19:03]

https://codereview.appspot.com/37730043/diff/180001/ssh/connection.go
File ssh/connection.go (right):

https://codereview.appspot.com/37730043/diff/180001/ssh/connection.go#newcode72
ssh/connection.go:72: //   Disconnect
any opinion on these?

[agl1, 2013-12-12 16:18:01]

https://codereview.appspot.com/37730043/diff/160001/ssh/client.go
File ssh/client.go (right):

https://codereview.appspot.com/37730043/diff/160001/ssh/client.go#newcode34
ssh/client.go:34: // Client returns a new SSH client connection using c as the
underlying transport.
On 2013/12/12 10:38:19, hanwen-google wrote:
> I tried clarifying, not sure if I succeeded.

I think some thought is probably still needed here, but this CL need not be
delayed if you don't want to.

There are three layers in play (I think):

net.Conn --(NewClientConn)--> ssh.Conn --(Client)--> *ssh.ClientConn

I seems odd that NewClientConn *doesn't* return a *ClientConn.

On the other side, there's no Server() function any longer, one deals directly
with a ServerConn, but NewServerConn *does* create that?

It seems that ssh.Conn is common between clients and servers, except for the
handshake. Perhaps:

NewClientConn(net.Conn, addr, config) (Conn, <-chan NewChannel, <-chan *Request,
error)

and, likewise, NewServerConn() (differing only in the flavor of handshake)

Then ClientConn could be renamed to Client, and the current Client() become
NewClient().

https://codereview.appspot.com/37730043/diff/180001/ssh/connection.go
File ssh/connection.go (right):

https://codereview.appspot.com/37730043/diff/180001/ssh/connection.go#newcode72
ssh/connection.go:72: //   Disconnect
On 2013/12/12 11:19:03, hanwen-google wrote:
> any opinion on these?

I think being cautious with what's exposed is fine.

[hanwen-google, 2013-12-12 16:21:36]

Google Germany GmbH - ABC-Str. 19 - 20354 Hamburg
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg - Geschäftsführer: Graham Law,
Christine Elizabeth Flores


On Thu, Dec 12, 2013 at 5:18 PM,  <agl@golang.org> wrote:
>
> https://codereview.appspot.com/37730043/diff/160001/ssh/client.go
> File ssh/client.go (right):
>
> https://codereview.appspot.com/37730043/diff/160001/ssh/client.go#newcode34
> ssh/client.go:34: // Client returns a new SSH client connection using c
> as the underlying transport.
> On 2013/12/12 10:38:19, hanwen-google wrote:
>>
>> I tried clarifying, not sure if I succeeded.
>
>
> I think some thought is probably still needed here, but this CL need not
> be delayed if you don't want to.
>
> There are three layers in play (I think):
>
> net.Conn --(NewClientConn)--> ssh.Conn --(Client)--> *ssh.ClientConn
>
> I seems odd that NewClientConn *doesn't* return a *ClientConn.
>
> On the other side, there's no Server() function any longer, one deals
> directly with a ServerConn, but NewServerConn *does* create that?
>
> It seems that ssh.Conn is common between clients and servers, except for
> the handshake. Perhaps:
>
> NewClientConn(net.Conn, addr, config) (Conn, <-chan NewChannel, <-chan
> *Request, error)
>
> and, likewise, NewServerConn() (differing only in the flavor of
> handshake)
>
> Then ClientConn could be renamed to Client, and the current Client()
> become NewClient().

SGTM .  I'll do this, probably tomorrow morning.


-- 
Han-Wen Nienhuys
Google Munich
hanwen@google.com

[agl, 2013-12-12 16:23:08]

Not sure if you intended to do it in a separate CL but LGTM for this one if you
did.

[hanwen-google, 2013-12-12 20:48:59]

*** Submitted as https://code.google.com/p/gosshnew/source/detail?r=be9d186b49f4
***

gosshnew/ssh: introduce ssh.Conn, representing the connection protocol.

This adds NewServerConn and NewClientConn returning a ssh.Conn.

R=agl, dave, jpsugar, agl
CC=agl, dave, golang-dev, hanwen, jpsugar
https://codereview.appspot.com/37730043