go.crypto/ssh: reimplement SSH connection protocol modularly.
Adds the "mux" type, which implements the protocol on an
arbitrary packet transport, and reimplements "channel" to be
used in both directions.
Client and server both use mux.
Fixes issue 5877, 6250.
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
10 years, 5 months ago
(2013-10-05 20:03:21 UTC)
#1
hi guys, this is still a bit rough, but I think it warrants a first ...
10 years, 5 months ago
(2013-10-05 20:07:28 UTC)
#2
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.
The key change on the server side was handled when the client initiated, though the ...
10 years, 5 months ago
(2013-10-06 03:09:54 UTC)
#3
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/
>
How can I test if this really works, ie. how do I make OpenSSH issue ...
10 years, 5 months ago
(2013-10-06 07:56:36 UTC)
#4
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
When implementing, I tested with putty which you can set the criteria when it re-keys, ...
10 years, 5 months ago
(2013-10-06 21:13:41 UTC)
#5
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
On 2013/10/06 21:13:41, kardia wrote: > When implementing, I tested with putty which you can ...
10 years, 5 months ago
(2013-10-06 21:26:25 UTC)
#6
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.
On 2013/10/06 21:26:25, mwhudson wrote: > On 2013/10/06 21:13:41, kardia wrote: > > When implementing, ...
10 years, 5 months ago
(2013-10-09 04:40:31 UTC)
#7
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 ?
There a couple of bits I can break out easily (which I will do), but ...
10 years, 5 months ago
(2013-10-09 10:03:12 UTC)
#8
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
Thanks. If there are any bits of the change which are moving code around that ...
10 years, 5 months ago
(2013-10-09 10:05:19 UTC)
#9
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
I've synced this, to include the mempipe change. I've started another change which rewrites the ...
10 years, 5 months ago
(2013-10-13 11:52:34 UTC)
#10
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.
also, if you want to start reviewing this, mux and mux_test are self-contained. I could ...
10 years, 5 months ago
(2013-10-13 11:55:18 UTC)
#11
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.
On 2013/10/13 11:55:18, hanwen-google wrote: > also, if you want to start reviewing this, mux ...
10 years, 5 months ago
(2013-10-14 00:59:02 UTC)
#12
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
I synced in the changes, but you should probably first look at https://codereview.appspot.com/14494058/ - it ...
10 years, 5 months ago
(2013-10-17 00:45:25 UTC)
#19
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.
* remailed (I was not familiar with needing to do that in rietveld.) * we'll ...
10 years, 5 months ago
(2013-10-21 00:49:58 UTC)
#24
* 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.
On 2013/10/21 00:49:58, hanwen-google wrote: > * remailed (I was not familiar with needing to ...
10 years, 5 months ago
(2013-10-22 22:12:17 UTC)
#25
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 ?
I've reworked this change to be backward compatible, so we don't have to wait for ...
10 years, 5 months ago
(2013-10-26 06:59:38 UTC)
#26
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.
On 2013/10/26 06:59:38, hanwen-google wrote: > I've reworked this change to be backward compatible, so ...
10 years, 4 months ago
(2013-11-07 11:58:14 UTC)
#27
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.