code review 14476043: go.crypto/ssh: move interpretation of msgNewKeys into

go.crypto/ssh: move interpretation of msgNewKeys into
transport.

Sending the msgNewKeys packet and setting up the key material
now happen under a lock, preventing races with concurrent
writers.

[hanwen-google, 2013-10-06 21:51:14]

Hello kardianos@gmail.com, agl@golang.org, jpsugar@google.com (cc:
golang-dev@googlegroups.com),

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

[hanwenn, 2013-10-06 22:01:47]

This fixes a race condition, and also looks cleaner to me.

I hope to decouple the connection protocol further from encryption, so I can
make CL 14225043 be oblivious to the transport layer. This looks like a sane
first step.

[jpsugar, 2013-10-07 17:19:27]

LGTM

[hanwen-google, 2013-10-08 10:45:40]

I've been rereading the RFC for a bit.

 Once a party has sent a SSH_MSG_KEXINIT message for key exchange or
   re-exchange, until it has sent a SSH_MSG_NEWKEYS message (Section
   7.3), it MUST NOT send any messages other than:

   o  Transport layer generic messages (1 to 19) (but
      SSH_MSG_SERVICE_REQUEST and SSH_MSG_SERVICE_ACCEPT MUST NOT be
      sent);

   o  Algorithm negotiation messages (20 to 29) (but further
      SSH_MSG_KEXINIT messages MUST NOT be sent);

   o  Specific key exchange method messages (30 to 49).

ie. even during a re-keying, only transport level messages may be sent.  This
does not mean the CL is wrong, but it does not fix the problem. The solution is
to block all higher level messages until the key exchange is finalized.

[kardia, 2013-10-08 15:33:50]

What problem are you then trying to solve? As I read the RFC, it is up
to both parties to behave properly during a re-key and not send other
messages until it has finished. Are you just wanting a safety?

On Tue, Oct 8, 2013 at 3:45 AM,  <hanwen@google.com> wrote:
> I've been rereading the RFC for a bit.
>
>  Once a party has sent a SSH_MSG_KEXINIT message for key exchange or
>    re-exchange, until it has sent a SSH_MSG_NEWKEYS message (Section
>    7.3), it MUST NOT send any messages other than:
>
>    o  Transport layer generic messages (1 to 19) (but
>       SSH_MSG_SERVICE_REQUEST and SSH_MSG_SERVICE_ACCEPT MUST NOT be
>       sent);
>
>    o  Algorithm negotiation messages (20 to 29) (but further
>       SSH_MSG_KEXINIT messages MUST NOT be sent);
>
>    o  Specific key exchange method messages (30 to 49).
>
> ie. even during a re-keying, only transport level messages may be sent.
> This does not mean the CL is wrong, but it does not fix the problem. The
> solution is to block all higher level messages until the key exchange is
> finalized.
>
> https://codereview.appspot.com/14476043/

[hanwen-google, 2013-10-08 15:42:29]

On Tue, Oct 8, 2013 at 5:33 PM, Daniel Theophanes <kardianos@gmail.com> wrote:
> What problem are you then trying to solve? As I read the RFC, it is up
> to both parties to behave properly during a re-key and not send other
> messages until it has finished. Are you just wanting a safety?

I'm mainly interested in cleaning up the code duplication between
client and server (see also https://codereview.appspot.com/14225043/
). Here, the key change is a hurdle because it is asymmetric for
client and server, while the rest of connection protocol is symmetric.

Second, the server currently does not behave properly. AFAICT, it is
possible for server channels to continue sending data to the client
while a key change is happening.

> On Tue, Oct 8, 2013 at 3:45 AM,  <hanwen@google.com> wrote:
>> I've been rereading the RFC for a bit.
>>
>>  Once a party has sent a SSH_MSG_KEXINIT message for key exchange or
>>    re-exchange, until it has sent a SSH_MSG_NEWKEYS message (Section
>>    7.3), it MUST NOT send any messages other than:
>>
>>    o  Transport layer generic messages (1 to 19) (but
>>       SSH_MSG_SERVICE_REQUEST and SSH_MSG_SERVICE_ACCEPT MUST NOT be
>>       sent);
>>
>>    o  Algorithm negotiation messages (20 to 29) (but further
>>       SSH_MSG_KEXINIT messages MUST NOT be sent);
>>
>>    o  Specific key exchange method messages (30 to 49).
>>
>> ie. even during a re-keying, only transport level messages may be sent.
>> This does not mean the CL is wrong, but it does not fix the problem. The
>> solution is to block all higher level messages until the key exchange is
>> finalized.
>>
>> https://codereview.appspot.com/14476043/




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

[agl1, 2013-10-08 19:47:14]

*** Submitted as
https://code.google.com/p/go/source/detail?r=9ff358df0dad&repo=crypto ***

go.crypto/ssh: move interpretation of msgNewKeys into
transport.

Sending the msgNewKeys packet and setting up the key material
now happen under a lock, preventing races with concurrent
writers.

R=kardianos, agl, jpsugar, hanwenn
CC=golang-dev
https://codereview.appspot.com/14476043

Committer: Adam Langley <agl@golang.org>

https://codereview.appspot.com/14476043/diff/5001/ssh/transport.go
File ssh/transport.go (right):

https://codereview.appspot.com/14476043/diff/5001/ssh/transport.go#newcode71
ssh/transport.go:71: // keychanges in both directions are triggered by reading
and writing
Will add a space here when landing.

[hanwen-google, 2013-10-13 12:44:38]

*** Abandoned ***