code review 6301071: go.crypto/ssh: allow server to respond to client init k...

go.crypto/ssh: allow server to respond to client init key exchange.

A windows SSH client, PuTTY, by default re-keys after every 60 minutes
or 1G of data transfer.

[kardia, 2012-06-12 09:00:50]

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

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

[agl1, 2012-06-12 14:39:28]

The CL description should include a justification. Is there some SSH client that
does renegotiations automatically that we wish to be compatible with?

The code itself involves a big lump of copy-and-paste that could probably be
better.

[kardia, 2012-06-12 15:24:22]

I'll add to the description.
A windows SSH client, PuTTY, by default re-keys after every 60 minutes 
or 1G of data transfer.

It is my thinking to add a public re-key method, maybe "ReHandshake()", 
to both the server and client in another CL.

I wasn't sure If you would like to see the changes in a simple view 
before re-factoring in a different CL or not. If the public 
ReHandshake() refactoring might be slightly different then if I do it now.

Please let me know if (1) you would be open to another CL adding 
"ReHandshake()" to the server and client, and (2) if I should redactor 
the code in this CL or not.

Thanks,
-Daniel


On 06/12/2012 07:39 AM, agl@golang.org wrote:
> The CL description should include a justification. Is there some SSH
> client that does renegotiations automatically that we wish to be
> compatible with?
>
> The code itself involves a big lump of copy-and-paste that could
> probably be better.
>
> http://codereview.appspot.com/6301071/

[agl1, 2012-06-12 17:09:33]

On Tue, Jun 12, 2012 at 11:24 AM, Daniel <kardianos@gmail.com> wrote:
> I'll add to the description.
> A windows SSH client, PuTTY, by default re-keys after every 60 minutes or 1G
> of data transfer.

Thanks. That's a good reason to support it.

> Please let me know if (1) you would be open to another CL adding
> "ReHandshake()" to the server and client, and (2) if I should redactor the
> code in this CL or not.

I think that this CL's scope is good, but the amount of duplicated
code is unfortunate. Littering code with if statements in order to try
and reduce duplicated code is also bad, but I didn't immediately see
that it was going to be a major problem here, although I could be
wrong.

We will want a way to test this functionality in the future, but I'm
not sure that we should alter the client code for that.

I question whether we should expose it publicly. I don't believe that
renegotiating the ciphers actually makes sense. If you believe that an
endpoint has been compromised then renegotiating hardly seems to help.
Possibly it could be effective against slow, secret leaks (i.e. timing
attacks), in which case we should rehandshake automatically for the
user. But I think the best course of action is to keep things simple.


Cheers

AGL

[kardia, 2012-06-12 18:03:57]

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

Please take another look.

[kardia, 2012-06-12 19:12:58]

I think the rational I read for including the rehandshake was to protect 
against leaks when the sequence counter wraps, especially for ctr 
ciphers. At this point I don't care too much if it is exposed or not, 
but if you're interested I could see how much code it affects and post a 
tentative CL (some time after this one is merged).

Thanks,
-Daniel


On 06/12/2012 10:09 AM, Adam Langley wrote:
> I question whether we should expose it publicly. I don't believe that
> renegotiating the ciphers actually makes sense. If you believe that an
> endpoint has been compromised then renegotiating hardly seems to help.
> Possibly it could be effective against slow, secret leaks (i.e. timing
> attacks), in which case we should rehandshake automatically for the
> user. But I think the best course of action is to keep things simple.

[agl1, 2012-06-12 19:29:49]

Mostly LGTM

http://codereview.appspot.com/6301071/diff/8001/ssh/server.go
File ssh/server.go (right):

http://codereview.appspot.com/6301071/diff/8001/ssh/server.go#newcode281
ssh/server.go:281: if err = unmarshal(clientKexInit, clientKexInitPacket,
msgKexInit); err != nil {
I think this means that the clientKexInit is always unmarshaled from the buffer.
I think the code should look like:

if clientKexInit == nil {
  clientKexInit = new(kexInitMsg)
  if clientKexInitPacket, err = s.readPacket(); err != nil {
    return
  }
  if err = unmarshal(clientKexInit, clientKexInitPacket, msgKexInit); err != nil
{
    return
  }
}

[kardia, 2012-06-12 19:33:34]

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

Please take another look.

[agl1, 2012-06-12 20:34:57]

*** Submitted as
http://code.google.com/p/go/source/detail?r=671a975c2b5f&repo=crypto ***

go.crypto/ssh: allow server to respond to client init key exchange.

A windows SSH client, PuTTY, by default re-keys after every 60 minutes
or 1G of data transfer.

R=dave, agl
CC=golang-dev
http://codereview.appspot.com/6301071

Committer: Adam Langley <agl@golang.org>

[dave_cheney.net, 2012-06-12 22:30:20]

LGTM. Thanks for adding this (and fixing my spelling mistakes).

On Wed, Jun 13, 2012 at 6:34 AM,  <agl@golang.org> wrote:
> *** Submitted as
> http://code.google.com/p/go/source/detail?r=671a975c2b5f&repo=crypto ***
>
>
> go.crypto/ssh: allow server to respond to client init key exchange.
>
> A windows SSH client, PuTTY, by default re-keys after every 60 minutes
> or 1G of data transfer.
>
> R=dave, agl
> CC=golang-dev
> http://codereview.appspot.com/6301071
>
> Committer: Adam Langley <agl@golang.org>
>
>
> http://codereview.appspot.com/6301071/

[kardia, 2012-06-12 23:36:18]

*** Abandoned ***