code review 17600043: go.crypto/ssh: implement the connection protocol modularly.

go.crypto/ssh: implement the connection protocol modularly.

This change adds new types and codepaths which are not used
yet.

[hanwen-google, 2013-10-26 06:54:37]

Hello agl1, dfc, jpsugar@google.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.crypto

[jpsugar, 2013-10-28 17:02:40]

https://codereview.appspot.com/17600043/diff/20001/ssh/newchannel.go
File ssh/newchannel.go (right):

https://codereview.appspot.com/17600043/diff/20001/ssh/newchannel.go#newcode16
ssh/newchannel.go:16: // channel implements the Channel
Maybe this?
// nChannel is the standard implementation of the Channel interface.

[hanwen-google, 2013-10-28 17:06:52]

https://codereview.appspot.com/17600043/diff/20001/ssh/newchannel.go
File ssh/newchannel.go (right):

https://codereview.appspot.com/17600043/diff/20001/ssh/newchannel.go#newcode16
ssh/newchannel.go:16: // channel implements the Channel
On 2013/10/28 17:02:41, jpsugar wrote:
> Maybe this?
> // nChannel is the standard implementation of the Channel interface.

Done.

[agl1, 2013-10-28 17:25:00]

Generally LGTM.

https://codereview.appspot.com/17600043/diff/20001/ssh/mux.go
File ssh/mux.go (right):

https://codereview.appspot.com/17600043/diff/20001/ssh/mux.go#newcode20
ssh/mux.go:20: type nChanList struct {
What's the prefix 'n' for? Shouldn't this just be "chanList"?

https://codereview.appspot.com/17600043/diff/20001/ssh/mux.go#newcode69
ssh/mux.go:69: // dropAll drops all remaining channels
// dropAll removes all channels from c and returns them in a slice.

https://codereview.appspot.com/17600043/diff/20001/ssh/mux.go#newcode120
ssh/mux.go:120: // return includes success status and extra data. See also
RFC4254 section 4
comma after RFC4254

https://codereview.appspot.com/17600043/diff/20001/ssh/mux.go#newcode176
ssh/mux.go:176: // Loop runs the connection machine. It will process packets
until an
This file varies between one and two spaces after the period. One space is best.

https://codereview.appspot.com/17600043/diff/20001/ssh/mux.go#newcode233
ssh/mux.go:233: ID := binary.BigEndian.Uint32(packet[1:])
s/ID/id/

https://codereview.appspot.com/17600043/diff/20001/ssh/mux.go#newcode236
ssh/mux.go:236: return fmt.Errorf("invalid channel %d", ID)
"ssh: " at start of error.

https://codereview.appspot.com/17600043/diff/20001/ssh/mux.go#newcode272
ssh/mux.go:272: panic(fmt.Sprintf("not a global message %#v", msg))
return error?

https://codereview.appspot.com/17600043/diff/20001/ssh/newchannel.go
File ssh/newchannel.go (right):

https://codereview.appspot.com/17600043/diff/20001/ssh/newchannel.go#newcode17
ssh/newchannel.go:17: type nChannel struct {
Again, not sure about the 'n' prefix. Is it just for 'new' with an intent to
switch over later?

https://codereview.appspot.com/17600043/diff/20001/ssh/newchannel.go#newcode182
ssh/newchannel.go:182: return 0, fmt.Errorf("ssh: extended code %d implemented",
extended)
unimplemented?

[hanwen-google, 2013-10-28 17:40:06]

https://codereview.appspot.com/17600043/diff/20001/ssh/mux.go
File ssh/mux.go (right):

https://codereview.appspot.com/17600043/diff/20001/ssh/mux.go#newcode20
ssh/mux.go:20: type nChanList struct {
On 2013/10/28 17:25:00, agl1 wrote:
> What's the prefix 'n' for? Shouldn't this just be "chanList"?

added more comments.

https://codereview.appspot.com/17600043/diff/20001/ssh/mux.go#newcode69
ssh/mux.go:69: // dropAll drops all remaining channels
On 2013/10/28 17:25:00, agl1 wrote:
> // dropAll removes all channels from c and returns them in a slice.

done.

https://codereview.appspot.com/17600043/diff/20001/ssh/mux.go#newcode120
ssh/mux.go:120: // return includes success status and extra data. See also
RFC4254 section 4
On 2013/10/28 17:25:00, agl1 wrote:
> comma after RFC4254

Done.

https://codereview.appspot.com/17600043/diff/20001/ssh/mux.go#newcode176
ssh/mux.go:176: // Loop runs the connection machine. It will process packets
until an
On 2013/10/28 17:25:00, agl1 wrote:
> This file varies between one and two spaces after the period. One space is
best.

fixed throughout. I'll do a cosmetic fix for the rest of the code one of these
days.

https://codereview.appspot.com/17600043/diff/20001/ssh/mux.go#newcode233
ssh/mux.go:233: ID := binary.BigEndian.Uint32(packet[1:])
On 2013/10/28 17:25:00, agl1 wrote:
> s/ID/id/

Done.

https://codereview.appspot.com/17600043/diff/20001/ssh/mux.go#newcode236
ssh/mux.go:236: return fmt.Errorf("invalid channel %d", ID)
On 2013/10/28 17:25:00, agl1 wrote:
> "ssh: " at start of error.

Done.

https://codereview.appspot.com/17600043/diff/20001/ssh/mux.go#newcode272
ssh/mux.go:272: panic(fmt.Sprintf("not a global message %#v", msg))
On 2013/10/28 17:25:00, agl1 wrote:
> return error?

it would mean a programming error in line 225.

https://codereview.appspot.com/17600043/diff/20001/ssh/newchannel.go
File ssh/newchannel.go (right):

https://codereview.appspot.com/17600043/diff/20001/ssh/newchannel.go#newcode17
ssh/newchannel.go:17: type nChannel struct {
On 2013/10/28 17:25:00, agl1 wrote:
> Again, not sure about the 'n' prefix. Is it just for 'new' with an intent to
> switch over later?

it's new, with intent of switching over. There already is a newChannel function
(which creates a new channel).

https://codereview.appspot.com/17600043/diff/20001/ssh/newchannel.go#newcode182
ssh/newchannel.go:182: return 0, fmt.Errorf("ssh: extended code %d implemented",
extended)
On 2013/10/28 17:25:00, agl1 wrote:
> unimplemented?

done. :)

[agl1, 2013-10-28 17:56:58]

You should be able to land in gosshnew, so hopefully that's all I need to do
here. Let me know if hg submit doesn't work for you. (See
https://code.google.com/hosting/settings for the password.)

[hanwen-google, 2013-10-28 19:23:28]

On 2013/10/28 17:56:58, agl1 wrote:
> You should be able to land in gosshnew, so hopefully that's all I need to do
> here. Let me know if hg submit doesn't work for you. (See
> https://code.google.com/hosting/settings for the password.)

I know, but I reworked this to be API compatible so it can go into regular
go.crypto/ssh. gosshnew was API changes, right?

[agl, 2013-10-28 19:27:03]

On 2013/10/28 19:23:28, hanwen-google wrote:
> I know, but I reworked this to be API compatible so it can go into regular
> go.crypto/ssh. gosshnew was API changes, right?

gosshnew was to insulate a whole series of changes. Reworking this wasn't a bad
idea: it means that it's easier to review.

[hanwen-google, 2013-10-28 19:28:45]

OK.

Dave, any comments ?

On Mon, Oct 28, 2013 at 8:27 PM,  <agl@chromium.org> wrote:
> On 2013/10/28 19:23:28, hanwen-google wrote:
>>
>> I know, but I reworked this to be API compatible so it can go into
>
> regular
>>
>> go.crypto/ssh. gosshnew was API changes, right?
>
>
> gosshnew was to insulate a whole series of changes. Reworking this
> wasn't a bad idea: it means that it's easier to review.
>
> https://codereview.appspot.com/17600043/



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

[dfc, 2013-10-29 03:03:58]

> Dave, any comments ?

I'm still reviewing, there is still too much code for me to hold in my head.

Is it possible to avoid duplicating the nChanList, nChannel etc types ?

[dfc, 2013-10-29 03:34:49]

A lot of small nits. I'm sorry I keep asking for smaller changes, is it possible
to break this up even smaller, I want to see the mux type as a change alone. If
this needs supporting changes to chanList and channel, then can they be proposed
beforehand ?

https://codereview.appspot.com/17600043/diff/50005/ssh/messages.go
File ssh/messages.go (right):

https://codereview.appspot.com/17600043/diff/50005/ssh/messages.go#newcode206
ssh/messages.go:206: Data      []byte `ssh:"rest"`
This looks harmless, I think it can land in whatever CL lands next.

https://codereview.appspot.com/17600043/diff/50005/ssh/mux.go
File ssh/mux.go (right):

https://codereview.appspot.com/17600043/diff/50005/ssh/mux.go#newcode32
ssh/mux.go:32: // server/client muxes
if we avoid adding this offset logic for now, is it possible to avoid
introducing this temporary nChanList type ?

https://codereview.appspot.com/17600043/diff/50005/ssh/mux.go#newcode33
ssh/mux.go:33: offset uint32
or, alternatively, can you add the offset logic to the existing chanList type ?

https://codereview.appspot.com/17600043/diff/50005/ssh/mux.go#newcode57
ssh/mux.go:57: return c.chans[id]
this is the same logic as chanList.getChan, just with the condition reversed.

https://codereview.appspot.com/17600043/diff/50005/ssh/mux.go#newcode67
ssh/mux.go:67: }
This is a good check, it should be rolled into chanList.remove()

https://codereview.appspot.com/17600043/diff/50005/ssh/mux.go#newcode72
ssh/mux.go:72: func (c *nChanList) dropAll() []*nChannel {
add this dropAll method to chanList.

https://codereview.appspot.com/17600043/diff/50005/ssh/mux.go#newcode322
ssh/mux.go:322: ch := newChannel(chanType, extra)
why isn't mux passed into newChannel ?

https://codereview.appspot.com/17600043/diff/50005/ssh/newchannel.go
File ssh/newchannel.go (right):

https://codereview.appspot.com/17600043/diff/50005/ssh/newchannel.go#newcode281
ssh/newchannel.go:281: return e.ch.WriteExtended(data, e.code)
Why do we need this type ? why can't the stdout and stderr types just be the
same ?

https://codereview.appspot.com/17600043/diff/50005/ssh/newchannel.go#newcode308
ssh/newchannel.go:308: return errDecidedAlready
I think there needs to be a mutex around ch.decided

[hanwen-google, 2013-10-29 14:06:16]

I've split out the chanlist into a separate CL 19130043.

It will be a lot of work to rework the channel into clienChan, because I'd have
to make ClientConn work like mux without being able to import the mux code. I'd
also have to understand their interactions, which includes understanding all the
bugs lurking in there.

Could I, for this one time, plead you to bear with me and work through the code?
It may be large, but it is internally consistent and self-contained.

Also, https://codereview.appspot.com/14494058/ still is awaiting review, and it
must go in some time soon.

https://codereview.appspot.com/17600043/diff/50005/ssh/mux.go
File ssh/mux.go (right):

https://codereview.appspot.com/17600043/diff/50005/ssh/mux.go#newcode322
ssh/mux.go:322: ch := newChannel(chanType, extra)
On 2013/10/29 03:34:50, dfc wrote:
> why isn't mux passed into newChannel ?

Done.

https://codereview.appspot.com/17600043/diff/50005/ssh/newchannel.go
File ssh/newchannel.go (right):

https://codereview.appspot.com/17600043/diff/50005/ssh/newchannel.go#newcode281
ssh/newchannel.go:281: return e.ch.WriteExtended(data, e.code)
On 2013/10/29 03:34:50, dfc wrote:
> Why do we need this type ? why can't the stdout and stderr types just be the
> same ?

Which stdout type? 

We can create a stdout/stdin io.ReadWriter by returning extChannel{ch, 0}, but
the channel itself fulfills the same role.

https://codereview.appspot.com/17600043/diff/50005/ssh/newchannel.go#newcode308
ssh/newchannel.go:308: return errDecidedAlready
On 2013/10/29 03:34:50, dfc wrote:
> I think there needs to be a mutex around ch.decided

why?

the API says to first call either Reject/Accept, and then the other methods.
When following the API, the reads and writer are properly ordered.

[hanwen-google, 2013-11-04 18:22:38]

now in CL 21690043.

Note that I had to maintain nChanList, as I can't share chanList and nChanList
except by using interface{}.

[hanwen-google, 2013-11-04 18:24:26]

*** Abandoned ***