Code review - Issue 14225043: code review 14225043: go.crypto/ssh: reimplement SSH connection protocol modu...https://codereview.appspot.com/2014-04-15T10:22:54+00:00rietveld
Message from unknown
2013-10-01T17:37:31+00:00hanwen-googleurn:md5:41be00ea618dbb27bac453c90db85dbf
Message from unknown
2013-10-01T17:41:03+00:00hanwen-googleurn:md5:dfeb1307fcb8936cf82a2fa87994612b
Message from unknown
2013-10-01T18:21:05+00:00hanwen-googleurn:md5:e303479498ca104ba323b5e5677f6c94
Message from unknown
2013-10-01T20:08:46+00:00hanwen-googleurn:md5:a701e04ceb2681fa3d601cd138e3d17a
Message from unknown
2013-10-01T20:17:31+00:00hanwen-googleurn:md5:4e5d99943489c7579ac273fbde965b29
Message from unknown
2013-10-01T20:18:38+00:00hanwen-googleurn:md5:3c94c4115127ad45d43e459ae1fded64
Message from unknown
2013-10-05T18:56:09+00:00hanwen-googleurn:md5:d2d7d8e67d25c7c2112719905ff3ee9d
Message from unknown
2013-10-05T20:03:12+00:00hanwen-googleurn:md5:6ab68a37861aa811c2a90696f9025e05
Message from hanwen@google.com
2013-10-05T20:03:21+00:00hanwen-googleurn:md5:dd689ae363c950a20b8fe9f689b18e43
Hello golang-dev@googlegroups.com (cc: agl@golang.org, dave@cheney.net, jpsugar@google.com),
I'd like you to review this change to
https://code.google.com/p/go.crypto
Message from hanwenn@gmail.com
2013-10-05T20:07:28+00:00hanwennurn:md5:82f35f202d4c514911890cc6ab0cf85c
hi guys,
this is still a bit rough, but I think it warrants a first look. The API change which will cause the most pain is that Read() will no longer return ChannelRequest errors. I think the added type safety offsets the pain, and the requests weren't serialized with the read data anyway.
Things to consider:
* In the mux API, should we use functions, eg. Accept(), or channels, eg. ReceivedRequests()?
* key change is not handled (I think it should be at transport level.) Did it work before? It was only server-side, not tested, and channel writes didn't seem to be synchronized with the key change messages.
* should the mux be a public class? It could help people implement multiplexed transport between peers.
Message from unknown
2013-10-05T20:13:04+00:00hanwen-googleurn:md5:1dc0000fdb81f71eaad122f9fc63e1df
Message from unknown
2013-10-05T22:11:45+00:00hanwen-googleurn:md5:839aee88884cc99371131107dd1b935e
Message from unknown
2013-10-05T22:45:40+00:00hanwen-googleurn:md5:3163bd6d39657d76dec0ed4374502adf
Message from unknown
2013-10-05T22:55:10+00:00hanwen-googleurn:md5:cae1b3adf67dd2095503d19203cbdf54
Message from kardianos@gmail.com
2013-10-06T03:09:54+00:00kardiaurn:md5:3fc08f4c6e615205d019e0b90fa1fb20
The key change on the server side was handled when the client initiated,
though the server lacked the ability to initiate.
-Daniel
On Saturday, October 5, 2013 1:07:28 PM UTC-7, Han-Wen Nienhuys wrote:
>
> hi guys,
>
> this is still a bit rough, but I think it warrants a first look. The API
> change which will cause the most pain is that Read() will no longer
> return ChannelRequest errors. I think the added type safety offsets the
> pain, and the requests weren't serialized with the read data anyway.
>
> Things to consider:
>
> * In the mux API, should we use functions, eg. Accept(), or channels,
> eg. ReceivedRequests()?
>
> * key change is not handled (I think it should be at transport level.)
> Did it work before? It was only server-side, not tested, and channel
> writes didn't seem to be synchronized with the key change messages.
>
> * should the mux be a public class? It could help people implement
> multiplexed transport between peers.
>
>
> https://codereview.appspot.com/14225043/
>
Message from hanwenn@gmail.com
2013-10-06T07:56:36+00:00hanwennurn:md5:b1fc4c0502e5a547eed759df5f8b1e77
How can I test if this really works, ie. how do I make OpenSSH issue a rekeying?
My concern specifically is this part of the server kex routine:
if err = s.writePacket([]byte{msgNewKeys}); err != nil {
return
}
// *
if err = s.transport.writer.setupKeys(serverKeys, result.K,
result.H, s.sessionId, kex.Hash()); err != nil {
return
}
if another serverChan writes a packet at the point // * , it will use
the old keys but the remote side will decrypt with the new keys.
On Sun, Oct 6, 2013 at 5:09 AM, Daniel Theophanes <kardianos@gmail.com> wrote:
> The key change on the server side was handled when the client initiated,
> though the server lacked the ability to initiate.
>
> -Daniel
>
> On Saturday, October 5, 2013 1:07:28 PM UTC-7, Han-Wen Nienhuys wrote:
>>
>> hi guys,
>>
>> this is still a bit rough, but I think it warrants a first look. The API
>> change which will cause the most pain is that Read() will no longer
>> return ChannelRequest errors. I think the added type safety offsets the
>> pain, and the requests weren't serialized with the read data anyway.
>>
>> Things to consider:
>>
>> * In the mux API, should we use functions, eg. Accept(), or channels,
>> eg. ReceivedRequests()?
>>
>> * key change is not handled (I think it should be at transport level.)
>> Did it work before? It was only server-side, not tested, and channel
>> writes didn't seem to be synchronized with the key change messages.
>>
>> * should the mux be a public class? It could help people implement
>> multiplexed transport between peers.
>>
>>
>> https://codereview.appspot.com/14225043/
--
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
Message from kardianos@gmail.com
2013-10-06T21:13:41+00:00kardiaurn:md5:b101d3d95fb3ffa7599d1c525f950ebb
When implementing, I tested with putty which you can set the criteria
when it re-keys, either with time or data transfers.
On Sun, Oct 6, 2013 at 12:56 AM, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
> How can I test if this really works, ie. how do I make OpenSSH issue a rekeying?
>
> My concern specifically is this part of the server kex routine:
>
> if err = s.writePacket([]byte{msgNewKeys}); err != nil {
> return
> }
> // *
> if err = s.transport.writer.setupKeys(serverKeys, result.K,
> result.H, s.sessionId, kex.Hash()); err != nil {
> return
> }
>
> if another serverChan writes a packet at the point // * , it will use
> the old keys but the remote side will decrypt with the new keys.
>
>
> On Sun, Oct 6, 2013 at 5:09 AM, Daniel Theophanes <kardianos@gmail.com> wrote:
>> The key change on the server side was handled when the client initiated,
>> though the server lacked the ability to initiate.
>>
>> -Daniel
>>
>> On Saturday, October 5, 2013 1:07:28 PM UTC-7, Han-Wen Nienhuys wrote:
>>>
>>> hi guys,
>>>
>>> this is still a bit rough, but I think it warrants a first look. The API
>>> change which will cause the most pain is that Read() will no longer
>>> return ChannelRequest errors. I think the added type safety offsets the
>>> pain, and the requests weren't serialized with the read data anyway.
>>>
>>> Things to consider:
>>>
>>> * In the mux API, should we use functions, eg. Accept(), or channels,
>>> eg. ReceivedRequests()?
>>>
>>> * key change is not handled (I think it should be at transport level.)
>>> Did it work before? It was only server-side, not tested, and channel
>>> writes didn't seem to be synchronized with the key change messages.
>>>
>>> * should the mux be a public class? It could help people implement
>>> multiplexed transport between peers.
>>>
>>>
>>> https://codereview.appspot.com/14225043/
>
>
>
> --
> Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen
Message from michael.hudson@linaro.org
2013-10-06T21:26:25+00:00mwhudsonurn:md5:be19ab438c72bbf137d32d7e685d756f
On 2013/10/06 21:13:41, kardia wrote:
> When implementing, I tested with putty which you can set the criteria
> when it re-keys, either with time or data transfers.
You can also type "~R" in an interactive session with openssh.
Message from dave@cheney.net
2013-10-09T04:40:31+00:00dfcurn:md5:75c274b7513962b07eb359ff554c0525
On 2013/10/06 21:26:25, mwhudson wrote:
> On 2013/10/06 21:13:41, kardia wrote:
> > When implementing, I tested with putty which you can set the criteria
> > when it re-keys, either with time or data transfers.
>
> You can also type "~R" in an interactive session with openssh.
I like this. I haven't had a change to review it in full yet and the diff is very large. Would you be open to breaking the change into smaller parts ?
Message from hanwen@google.com
2013-10-09T10:03:12+00:00hanwen-googleurn:md5:b9e477845a4f461d864797c1ece6580d
There a couple of bits I can break out easily (which I will do), but
it does rewrite a large portion of the code, so the diff is large.
On Wed, Oct 9, 2013 at 6:40 AM, <dave@cheney.net> wrote:
> On 2013/10/06 21:26:25, mwhudson wrote:
>>
>> On 2013/10/06 21:13:41, kardia wrote:
>> > When implementing, I tested with putty which you can set the
>
> criteria
>>
>> > when it re-keys, either with time or data transfers.
>
>
>> You can also type "~R" in an interactive session with openssh.
>
>
> I like this. I haven't had a change to review it in full yet and the
> diff is very large. Would you be open to breaking the change into
> smaller parts ?
>
> https://codereview.appspot.com/14225043/
--
Han-Wen Nienhuys
Google Munich
hanwen@google.com
Message from dave@cheney.net
2013-10-09T10:05:19+00:00dfcurn:md5:ddf49610c61e27c3974365e2d1000cad
Thanks. If there are any bits of the change which are moving code
around that would help make the diff easier to consume.
On Wed, Oct 9, 2013 at 9:02 PM, Han-Wen Nienhuys <hanwen@google.com> wrote:
> There a couple of bits I can break out easily (which I will do), but
> it does rewrite a large portion of the code, so the diff is large.
>
> On Wed, Oct 9, 2013 at 6:40 AM, <dave@cheney.net> wrote:
>> On 2013/10/06 21:26:25, mwhudson wrote:
>>>
>>> On 2013/10/06 21:13:41, kardia wrote:
>>> > When implementing, I tested with putty which you can set the
>>
>> criteria
>>>
>>> > when it re-keys, either with time or data transfers.
>>
>>
>>> You can also type "~R" in an interactive session with openssh.
>>
>>
>> I like this. I haven't had a change to review it in full yet and the
>> diff is very large. Would you be open to breaking the change into
>> smaller parts ?
>>
>> https://codereview.appspot.com/14225043/
>
>
>
> --
> Han-Wen Nienhuys
> Google Munich
> hanwen@google.com
Message from unknown
2013-10-13T11:50:46+00:00hanwen-googleurn:md5:5e505787b1f547483fa65065b36e41ed
Message from hanwen@google.com
2013-10-13T11:52:34+00:00hanwen-googleurn:md5:156cec2a3c3dd0ae20da85bd4830c5be
I've synced this, to include the mempipe change.
I've started another change which rewrites the (re)keying handshake, which I'll have to submit first to not break rekeying support.
I'll the first bit of that in another CL.
Message from hanwen@google.com
2013-10-13T11:55:18+00:00hanwen-googleurn:md5:5262ea24f7c61a29306dc7892950797f
also, if you want to start reviewing this, mux and mux_test are self-contained. I could split that out, but I'd have to replace either the client or the server to make that a useful change.
Message from dave@cheney.net
2013-10-14T00:59:02+00:00dfcurn:md5:a3cd7447e6fbfb0012a812f8ef637200
On 2013/10/13 11:55:18, hanwen-google wrote:
> also, if you want to start reviewing this, mux and mux_test are self-contained.
> I could split that out, but I'd have to replace either the client or the server
> to make that a useful change.
Hi Hanwen,
Here are some unsorted review items. I really like where this CL is going, and if it stands a chance of unifying the mux logic in the client and the server, it will be a massive win.
I've noted some other areas which can probably be split off into an easy to review CL to reduce the size of the diff down to it's absolute minimum.
Cheers
Dave
Message from dave@cheney.net
2013-10-14T00:59:10+00:00dfcurn:md5:3f7a0f5d9a952b3f453347682e2a7136
https://codereview.appspot.com/14225043/diff/77001/ssh/channel.go
File ssh/channel.go (right):
https://codereview.appspot.com/14225043/diff/77001/ssh/channel.go#newcode40
ssh/channel.go:40: Stderr() io.ReadWriter
Why did Stderr grow the ability to both read and write ?
https://codereview.appspot.com/14225043/diff/77001/ssh/channel.go#newcode83
ssh/channel.go:83: func (r RejectionReason) String() string {
I think this can move to its own CL. It would help to make the diff on this file smaller
https://codereview.appspot.com/14225043/diff/77001/ssh/messages.go
File ssh/messages.go (right):
https://codereview.appspot.com/14225043/diff/77001/ssh/messages.go#newcode324
ssh/messages.go:324: // If msgType is zero, the output is not prefixed with a type byte.
Could all these changes be moved to their own CL ? What uses the msgType == 0 logic now ? I'm guessing nothing.
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go
File ssh/mux.go (right):
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go#newcode50
ssh/mux.go:50: id -= c.offset
the race detector won't like this
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go#newcode61
ssh/mux.go:61: id -= c.offset
same
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go#newcode86
ssh/mux.go:86: // multiplexes many channels onto a single packet transport.
// mux represents the state of an SSH connection.
// mux multiplexes many channels onto a single packet transport.
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go#newcode89
ssh/mux.go:89: chanList chanList
s/chanList chanList/chanList/
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go#newcode98
ssh/mux.go:98: func (m *mux) writePacket(p []byte) error {
if you embed packetConn into mux, then you won't need this forwarding method.
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go#newcode158
ssh/mux.go:158: // mux may hang.
s/may/will/
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go#newcode189
ssh/mux.go:189: var err error
for err != nil {
err = m.onePacket()
if debug && err != nil {
// println
}
}
https://codereview.appspot.com/14225043/diff/77001/ssh/server.go
File ssh/server.go (right):
https://codereview.appspot.com/14225043/diff/77001/ssh/server.go#newcode119
ssh/server.go:119: mux *mux
what about embedding *mux here ?
https://codereview.appspot.com/14225043/diff/77001/ssh/server.go#newcode521
ssh/server.go:521: return s.mux.Accept()
then you don't need this forwarding method.
https://codereview.appspot.com/14225043/diff/77001/ssh/session.go
File ssh/session.go (right):
https://codereview.appspot.com/14225043/diff/77001/ssh/session.go#newcode20
ssh/session.go:20: var _ = log.Println
really ?
https://codereview.appspot.com/14225043/diff/77001/ssh/tcpip.go
File ssh/tcpip.go (right):
https://codereview.appspot.com/14225043/diff/77001/ssh/tcpip.go#newcode83
ssh/tcpip.go:83: }
Please move this cleanup to a new CL to reduce the size of the diff
Message from hanwen@google.com
2013-10-14T06:30:26+00:00hanwen-googleurn:md5:0912a01c63cb58c3a28e4c68f239de81
https://codereview.appspot.com/14225043/diff/77001/ssh/channel.go
File ssh/channel.go (right):
https://codereview.appspot.com/14225043/diff/77001/ssh/channel.go#newcode40
ssh/channel.go:40: Stderr() io.ReadWriter
On 2013/10/14 00:59:11, dfc wrote:
> Why did Stderr grow the ability to both read and write ?
the server needs to write stderr for the client to read it.
https://codereview.appspot.com/14225043/diff/77001/ssh/channel.go#newcode83
ssh/channel.go:83: func (r RejectionReason) String() string {
On 2013/10/14 00:59:11, dfc wrote:
> I think this can move to its own CL. It would help to make the diff on this file
> smaller
will do.
https://codereview.appspot.com/14225043/diff/77001/ssh/messages.go
File ssh/messages.go (right):
https://codereview.appspot.com/14225043/diff/77001/ssh/messages.go#newcode324
ssh/messages.go:324: // If msgType is zero, the output is not prefixed with a type byte.
On 2013/10/14 00:59:11, dfc wrote:
> Could all these changes be moved to their own CL ? What uses the msgType == 0
> logic now ? I'm guessing nothing.
I guess so.
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go
File ssh/mux.go (right):
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go#newcode50
ssh/mux.go:50: id -= c.offset
On 2013/10/14 00:59:11, dfc wrote:
> the race detector won't like this
id is thread-local, while offset is constant. I don't see the problem?
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go#newcode61
ssh/mux.go:61: id -= c.offset
On 2013/10/14 00:59:11, dfc wrote:
> same
same.
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go#newcode86
ssh/mux.go:86: // multiplexes many channels onto a single packet transport.
On 2013/10/14 00:59:11, dfc wrote:
> // mux represents the state of an SSH connection.
> // mux multiplexes many channels onto a single packet transport.
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go#newcode89
ssh/mux.go:89: chanList chanList
On 2013/10/14 00:59:11, dfc wrote:
> s/chanList chanList/chanList/
one of the problems I had with the old code was actually it's liberal use of embedding. When you embed, it's not obvious what p.method() will run and where it is defined. I would rather err on the other side, which is to only embed when there is a strong reason for it (eg. when the result must obey some interface that contains these methods.)
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go#newcode98
ssh/mux.go:98: func (m *mux) writePacket(p []byte) error {
On 2013/10/14 00:59:11, dfc wrote:
> if you embed packetConn into mux, then you won't need this forwarding method.
Done.
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go#newcode158
ssh/mux.go:158: // mux may hang.
On 2013/10/14 00:59:11, dfc wrote:
> s/may/will/
Done.
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go#newcode189
ssh/mux.go:189: var err error
On 2013/10/14 00:59:11, dfc wrote:
> for err != nil {
> err = m.onePacket()
> if debug && err != nil {
> // println
> }
> }
Done.
https://codereview.appspot.com/14225043/diff/77001/ssh/server.go
File ssh/server.go (right):
https://codereview.appspot.com/14225043/diff/77001/ssh/server.go#newcode119
ssh/server.go:119: mux *mux
On 2013/10/14 00:59:11, dfc wrote:
> what about embedding *mux here ?
that would export all the mux methods. I don't want to do that on a whim.
https://codereview.appspot.com/14225043/diff/77001/ssh/server.go#newcode521
ssh/server.go:521: return s.mux.Accept()
On 2013/10/14 00:59:11, dfc wrote:
> then you don't need this forwarding method.
ack.
https://codereview.appspot.com/14225043/diff/77001/ssh/session.go
File ssh/session.go (right):
https://codereview.appspot.com/14225043/diff/77001/ssh/session.go#newcode20
ssh/session.go:20: var _ = log.Println
On 2013/10/14 00:59:11, dfc wrote:
> really ?
debugging.
removed.
https://codereview.appspot.com/14225043/diff/77001/ssh/tcpip.go
File ssh/tcpip.go (right):
https://codereview.appspot.com/14225043/diff/77001/ssh/tcpip.go#newcode83
ssh/tcpip.go:83: }
On 2013/10/14 00:59:11, dfc wrote:
> Please move this cleanup to a new CL to reduce the size of the diff
just the move? (did you see that the struct layout changes?)
Message from hanwen@google.com
2013-10-14T06:32:41+00:00hanwen-googleurn:md5:b6902867c09e857566829ecd3046a820
Message from dave@cheney.net
2013-10-14T06:36:40+00:00dfcurn:md5:2830a01a63db157ae3289249dbbc7caa
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go
File ssh/mux.go (right):
https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go#newcode50
ssh/mux.go:50: id -= c.offset
On 2013/10/14 06:30:26, hanwen-google wrote:
> On 2013/10/14 00:59:11, dfc wrote:
> > the race detector won't like this
>
> id is thread-local, while offset is constant. I don't see the problem?
You are correct, I was mistaken.
https://codereview.appspot.com/14225043/diff/77001/ssh/server.go
File ssh/server.go (right):
https://codereview.appspot.com/14225043/diff/77001/ssh/server.go#newcode119
ssh/server.go:119: mux *mux
On 2013/10/14 06:30:26, hanwen-google wrote:
> On 2013/10/14 00:59:11, dfc wrote:
> > what about embedding *mux here ?
>
> that would export all the mux methods. I don't want to do that on a whim.
fair enough
https://codereview.appspot.com/14225043/diff/77001/ssh/tcpip.go
File ssh/tcpip.go (right):
https://codereview.appspot.com/14225043/diff/77001/ssh/tcpip.go#newcode83
ssh/tcpip.go:83: }
On 2013/10/14 06:30:26, hanwen-google wrote:
> On 2013/10/14 00:59:11, dfc wrote:
> > Please move this cleanup to a new CL to reduce the size of the diff
>
> just the move? (did you see that the struct layout changes?)
anything that you can do to reduce the size of this change would be appreciated.
Message from unknown
2013-10-14T21:28:31+00:00hanwen-googleurn:md5:644f0a5f5f8907d33dfb1099b95c16c5
Message from unknown
2013-10-15T14:47:14+00:00hanwen-googleurn:md5:b59c812c923b47f6750de037fe38d7de
Message from unknown
2013-10-16T03:15:41+00:00hanwen-googleurn:md5:8780385e5204670b71cb06253410efeb
Message from unknown
2013-10-16T03:27:17+00:00hanwen-googleurn:md5:cf96a6850aeb4ce0b04496cfdf9a373e
Message from dave@cheney.net
2013-10-16T22:49:12+00:00dfcurn:md5:4a49cf984e17a103dd639b44108c49e2
On 2013/10/14 06:36:40, dfc wrote:
> https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go
> File ssh/mux.go (right):
>
> https://codereview.appspot.com/14225043/diff/77001/ssh/mux.go#newcode50
> ssh/mux.go:50: id -= c.offset
> On 2013/10/14 06:30:26, hanwen-google wrote:
> > On 2013/10/14 00:59:11, dfc wrote:
> > > the race detector won't like this
> >
> > id is thread-local, while offset is constant. I don't see the problem?
>
> You are correct, I was mistaken.
>
> https://codereview.appspot.com/14225043/diff/77001/ssh/server.go
> File ssh/server.go (right):
>
> https://codereview.appspot.com/14225043/diff/77001/ssh/server.go#newcode119
> ssh/server.go:119: mux *mux
> On 2013/10/14 06:30:26, hanwen-google wrote:
> > On 2013/10/14 00:59:11, dfc wrote:
> > > what about embedding *mux here ?
> >
> > that would export all the mux methods. I don't want to do that on a whim.
>
> fair enough
>
> https://codereview.appspot.com/14225043/diff/77001/ssh/tcpip.go
> File ssh/tcpip.go (right):
>
> https://codereview.appspot.com/14225043/diff/77001/ssh/tcpip.go#newcode83
> ssh/tcpip.go:83: }
> On 2013/10/14 06:30:26, hanwen-google wrote:
> > On 2013/10/14 00:59:11, dfc wrote:
> > > Please move this cleanup to a new CL to reduce the size of the diff
> >
> > just the move? (did you see that the struct layout changes?)
>
> anything that you can do to reduce the size of this change would be appreciated.
Can you please remail this to incorporate the changes that have landed recently.
Message from unknown
2013-10-17T00:43:41+00:00hanwen-googleurn:md5:5e053367b5843b053cd5235776c87c65
Message from hanwen@google.com
2013-10-17T00:43:48+00:00hanwen-googleurn:md5:e0ce30d18c82bf734c5c5e98559e8a13
Hello golang-dev@googlegroups.com, hanwenn@gmail.com, kardianos@gmail.com, michael.hudson@linaro.org, dave@cheney.net (cc: agl@golang.org, golang-dev@googlegroups.com, jpsugar@google.com),
Please take another look.
Message from hanwen@google.com
2013-10-17T00:45:25+00:00hanwen-googleurn:md5:18a820f0cb51889289f1999a88576238
I synced in the changes, but you should probably first look at
https://codereview.appspot.com/14494058/ - it implements rekeying properly, so this change does not including regress.
Message from jpsugar@google.com
2013-10-17T00:51:01+00:00jpsugarurn:md5:d026273577c066bcc92f83a0d3b499bb
https://codereview.appspot.com/14225043/diff/122001/ssh/channel.go
File ssh/channel.go (right):
https://codereview.appspot.com/14225043/diff/122001/ssh/channel.go#newcode18
ssh/channel.go:18: type Channel interface {
I'm not sure I'm a fan of this model where both nascent (undecided) and actual channels are the same type. I would rather have
type ChannelRequest interface {
Accept() (Channel, error)
Reject(...) error
etc.
https://codereview.appspot.com/14225043/diff/122001/ssh/channel.go#newcode38
ssh/channel.go:38: // Stderr returns an io.ReadWriter that writes to this channel with the
This comment only talks about writing. It would be clearer if it described what reading did too.
https://codereview.appspot.com/14225043/diff/122001/ssh/client.go
File ssh/client.go (right):
https://codereview.appspot.com/14225043/diff/122001/ssh/client.go#newcode48
ssh/client.go:48: go conn.handleGlobalRequests()
It might be cleaner if these two new goroutines were launched from loop().
https://codereview.appspot.com/14225043/diff/122001/ssh/mux.go
File ssh/mux.go (right):
https://codereview.appspot.com/14225043/diff/122001/ssh/mux.go#newcode368
ssh/mux.go:368: func (m *mux) Accept() (Channel, error) {
Why not just expose openedChans to the user?
Message from hanwen@google.com
2013-10-17T14:59:07+00:00hanwen-googleurn:md5:e9d292d9f4d2418f63db10a407aba1e3
https://codereview.appspot.com/14225043/diff/122001/ssh/channel.go
File ssh/channel.go (right):
https://codereview.appspot.com/14225043/diff/122001/ssh/channel.go#newcode18
ssh/channel.go:18: type Channel interface {
On 2013/10/17 00:51:02, jpsugar wrote:
> I'm not sure I'm a fan of this model where both nascent (undecided) and actual
> channels are the same type. I would rather have
>
> type ChannelRequest interface {
> Accept() (Channel, error)
> Reject(...) error
>
> etc.
It minimizes API breakage, but I agree your suggestion is better.
https://codereview.appspot.com/14225043/diff/122001/ssh/channel.go#newcode38
ssh/channel.go:38: // Stderr returns an io.ReadWriter that writes to this channel with the
On 2013/10/17 00:51:02, jpsugar wrote:
> This comment only talks about writing. It would be clearer if it described what
> reading did too.
Done.
https://codereview.appspot.com/14225043/diff/122001/ssh/client.go
File ssh/client.go (right):
https://codereview.appspot.com/14225043/diff/122001/ssh/client.go#newcode48
ssh/client.go:48: go conn.handleGlobalRequests()
On 2013/10/17 00:51:02, jpsugar wrote:
> It might be cleaner if these two new goroutines were launched from loop().
Done.
https://codereview.appspot.com/14225043/diff/122001/ssh/mux.go
File ssh/mux.go (right):
https://codereview.appspot.com/14225043/diff/122001/ssh/mux.go#newcode368
ssh/mux.go:368: func (m *mux) Accept() (Channel, error) {
On 2013/10/17 00:51:02, jpsugar wrote:
> Why not just expose openedChans to the user?
Done.
Message from dave@cheney.net
2013-10-19T11:27:04+00:00dfcurn:md5:ac51e7ada50ca5f8d5d2f33f40615a23
On 2013/10/17 14:59:07, hanwen-google wrote:
> https://codereview.appspot.com/14225043/diff/122001/ssh/channel.go
> File ssh/channel.go (right):
>
> https://codereview.appspot.com/14225043/diff/122001/ssh/channel.go#newcode18
> ssh/channel.go:18: type Channel interface {
> On 2013/10/17 00:51:02, jpsugar wrote:
> > I'm not sure I'm a fan of this model where both nascent (undecided) and actual
> > channels are the same type. I would rather have
> >
> > type ChannelRequest interface {
> > Accept() (Channel, error)
> > Reject(...) error
> >
> > etc.
>
> It minimizes API breakage, but I agree your suggestion is better.
>
> https://codereview.appspot.com/14225043/diff/122001/ssh/channel.go#newcode38
> ssh/channel.go:38: // Stderr returns an io.ReadWriter that writes to this
> channel with the
> On 2013/10/17 00:51:02, jpsugar wrote:
> > This comment only talks about writing. It would be clearer if it described
> what
> > reading did too.
>
> Done.
>
> https://codereview.appspot.com/14225043/diff/122001/ssh/client.go
> File ssh/client.go (right):
>
> https://codereview.appspot.com/14225043/diff/122001/ssh/client.go#newcode48
> ssh/client.go:48: go conn.handleGlobalRequests()
> On 2013/10/17 00:51:02, jpsugar wrote:
> > It might be cleaner if these two new goroutines were launched from loop().
>
> Done.
>
> https://codereview.appspot.com/14225043/diff/122001/ssh/mux.go
> File ssh/mux.go (right):
>
> https://codereview.appspot.com/14225043/diff/122001/ssh/mux.go#newcode368
> ssh/mux.go:368: func (m *mux) Accept() (Channel, error) {
> On 2013/10/17 00:51:02, jpsugar wrote:
> > Why not just expose openedChans to the user?
>
> Done.
Ping. Rietveld says there are many comments open for review. Do you need to hg mail this ?
Message from unknown
2013-10-21T00:48:49+00:00hanwen-googleurn:md5:67a0ad28f12870791bc16519639471be
Message from hanwen@google.com
2013-10-21T00:48:58+00:00hanwen-googleurn:md5:79af92774b395c1223b4b7bb8f8d3408
Hello golang-dev@googlegroups.com, hanwenn@gmail.com, kardianos@gmail.com, michael.hudson@linaro.org, dave@cheney.net, jpsugar@google.com (cc: agl@golang.org, golang-dev@googlegroups.com),
Please take another look.
Message from hanwen@google.com
2013-10-21T00:49:58+00:00hanwen-googleurn:md5:4d60d12551143af2294ec29d065f6286
* remailed (I was not familiar with needing to do that in rietveld.)
* we'll have to commit https://codereview.appspot.com/14494058/ first, and I think that change would also benefit from your scrutiny.
Message from unknown
2013-10-21T05:12:27+00:00hanwen-googleurn:md5:e7620358918d46b26ebac659ef2f3659
Message from unknown
2013-10-21T05:15:34+00:00hanwen-googleurn:md5:bbd73bff3449a533ea5aba69d482c246
Message from dave@cheney.net
2013-10-22T22:12:17+00:00dfcurn:md5:3f2605954d26c372d08bb79f42eacf4e
On 2013/10/21 00:49:58, hanwen-google wrote:
> * remailed (I was not familiar with needing to do that in rietveld.)
>
> * we'll have to commit https://codereview.appspot.com/14494058/ first, and I
> think that change would also benefit from your scrutiny.
2800 lines of change is too large. Is it possible to split this CL into changes that add code, and changes that intergrate those additions ?
Message from unknown
2013-10-24T23:35:45+00:00hanwen-googleurn:md5:fe964b3f062c926d4235799c103dc823
Message from unknown
2013-10-26T06:33:38+00:00hanwen-googleurn:md5:85588195b061f5b7ba11b7df5a4057a2
Message from unknown
2013-10-26T06:34:32+00:00hanwen-googleurn:md5:bc19893835260692bf0933dcd5cb6b18
Message from hanwen@google.com
2013-10-26T06:59:38+00:00hanwen-googleurn:md5:a94d59640990a19bdfdc2a05ef5488f0
I've reworked this change to be backward compatible, so we don't have to wait for API changes for fixing the code.
The new code is split into https://codereview.appspot.com/17600043. Hopefully we can make some faster progress with this split.
Message from hanwen@google.com
2013-11-07T11:58:14+00:00hanwen-googleurn:md5:651d4184efb1ee1d176f0edf52112146
On 2013/10/26 06:59:38, hanwen-google wrote:
> I've reworked this change to be backward compatible, so we don't have to wait
> for API changes for fixing the code.
>
> The new code is split into https://codereview.appspot.com/17600043. Hopefully
> we can make some faster progress with this split.
This code has now been folded into gosshnew.
Dropping this CL.
Message from hanwen@google.com
2013-11-07T12:53:03+00:00hanwen-googleurn:md5:cdd1b04affc61daf4ff194a43de0b0cf
*** Abandoned ***
Message from hanwen@google.com
2014-04-15T10:22:54+00:00hanwen-googleurn:md5:ab2c818b14006a1e66cee42c0c7620bc
*** Abandoned ***