Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(22)

Issue 14494058: code review 14494058: go.crypto/ssh: support rekeying in both directions. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years ago by hanwen-google
Modified:
7 years, 11 months ago
Reviewers:
agl1, jpsugar, dfc
CC:
golang-dev
Visibility:
Public.

Description

go.crypto/ssh: support rekeying in both directions. Adds a largely symmetrical handshakeTransport, which can send and process kexInit messages. Automatically rekey on every on 1G of data transmitted.

Patch Set 1 #

Patch Set 2 : diff -r bb19605bfacc https://code.google.com/p/go.crypto #

Patch Set 3 : diff -r bb19605bfacc https://code.google.com/p/go.crypto #

Patch Set 4 : diff -r bb19605bfacc https://code.google.com/p/go.crypto #

Patch Set 5 : diff -r bb19605bfacc https://code.google.com/p/go.crypto #

Patch Set 6 : diff -r ef3d7138b0b5 https://code.google.com/p/go.crypto #

Patch Set 7 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #

Patch Set 8 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #

Patch Set 9 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #

Patch Set 10 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #

Patch Set 11 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #

Patch Set 12 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #

Patch Set 13 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #

Patch Set 14 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #

Total comments: 22

Patch Set 15 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #

Patch Set 16 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #

Patch Set 17 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #

Patch Set 18 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #

Patch Set 19 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #

Total comments: 44

Patch Set 20 : diff -r 5ff5636e18c9 https://code.google.com/p/go.crypto #

Patch Set 21 : diff -r 213a06a7ce81 https://code.google.com/p/go.crypto #

Patch Set 22 : diff -r cd1eea1eb828 https://code.google.com/p/go.crypto #

Patch Set 23 : diff -r cd1eea1eb828 https://code.google.com/p/go.crypto #

Patch Set 24 : diff -r cd1eea1eb828 https://code.google.com/p/go.crypto #

Total comments: 12

Patch Set 25 : diff -r cd1eea1eb828 https://code.google.com/p/go.crypto #

Patch Set 26 : diff -r cd1eea1eb828 https://code.google.com/p/go.crypto #

Patch Set 27 : diff -r cd1eea1eb828 https://code.google.com/p/go.crypto #

Patch Set 28 : diff -r cd1eea1eb828 https://code.google.com/p/go.crypto #

Patch Set 29 : diff -r cd1eea1eb828 https://code.google.com/p/go.crypto #

Patch Set 30 : diff -r cd1eea1eb828 https://code.google.com/p/go.crypto #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+845 lines, -219 lines) Patch
M ssh/client.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 4 chunks +22 lines, -91 lines 0 comments Download
M ssh/client_auth.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +2 lines, -1 line 0 comments Download
M ssh/common.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 3 chunks +18 lines, -0 lines 1 comment Download
A ssh/handshake.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +401 lines, -0 lines 0 comments Download
A ssh/handshake_test.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +313 lines, -0 lines 0 comments Download
M ssh/server.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 9 chunks +26 lines, -108 lines 0 comments Download
M ssh/test/session_test.go View 1 2 3 4 5 6 1 chunk +39 lines, -0 lines 1 comment Download
M ssh/test/test_unix_test.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +5 lines, -0 lines 0 comments Download
M ssh/transport.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 4 chunks +19 lines, -19 lines 0 comments Download

Messages

Total messages: 29
hanwen-google
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
8 years ago (2013-10-14 21:48:26 UTC) #1
hanwen-google
this goes on top of CL 14641044. We must submit this one before the mux ...
8 years ago (2013-10-14 21:59:18 UTC) #2
jpsugar
So for "rekey after N bytes", I think it's important to do that at a ...
8 years ago (2013-10-14 22:27:31 UTC) #3
hanwen-google
Hello agl@golang.org, dave@cheney.net, jpsugar@google.com (cc: golang-dev@googlegroups.com), Please take another look.
8 years ago (2013-10-17 05:56:12 UTC) #4
jpsugar
Can we just eat the newKeys message instead of passing it up and having it ...
8 years ago (2013-10-17 17:56:54 UTC) #5
hanwen-google
Let me think for a bit about eating newkeys altogether, to make sure it doesnt ...
8 years ago (2013-10-20 20:55:25 UTC) #6
hanwen-google
I tried eating newkeys (and managed to make it work), but decided against it. It ...
8 years ago (2013-10-21 00:46:50 UTC) #7
hanwen-google
Hello agl@golang.org, dave@cheney.net, jpsugar@google.com (cc: golang-dev@googlegroups.com), Please take another look.
8 years ago (2013-10-21 00:47:21 UTC) #8
jpsugar
https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go File ssh/handshake.go (right): https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode221 ssh/handshake.go:221: // TODO(hanwen): add random bits to kexInit.Cookie. Um, this ...
8 years ago (2013-10-21 17:03:50 UTC) #9
hanwen-google
https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go File ssh/handshake.go (right): https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode221 ssh/handshake.go:221: // TODO(hanwen): add random bits to kexInit.Cookie. On 2013/10/21 ...
8 years ago (2013-10-21 20:00:20 UTC) #10
jpsugar
LGTM provided agl ok's the no-random-cookie thing.
8 years ago (2013-10-21 20:00:55 UTC) #11
hanwen-google
Adam, can you have a look? On Mon, Oct 21, 2013 at 1:00 PM, <jpsugar@google.com> ...
8 years ago (2013-10-22 21:15:52 UTC) #12
dfc
I would really like to see this CL broken into smaller chunks. 1000 lines changed ...
8 years ago (2013-10-22 22:07:01 UTC) #13
agl1
https://codereview.appspot.com/14494058/diff/180001/ssh/common.go File ssh/common.go (right): https://codereview.appspot.com/14494058/diff/180001/ssh/common.go#newcode360 ssh/common.go:360: // sshConn provides net.Conn metadata, but but disallows direct ...
8 years ago (2013-10-22 22:12:56 UTC) #14
hanwen-google
https://codereview.appspot.com/14494058/diff/180001/ssh/client.go File ssh/client.go (right): https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode19 ssh/client.go:19: *handshakeTransport On 2013/10/22 22:07:02, dfc wrote: > This exposes ...
8 years ago (2013-10-23 00:15:17 UTC) #15
dfc
> On 22 Oct 2013, at 17:15, hanwen@google.com wrote: > > > https://codereview.appspot.com/14494058/diff/180001/ssh/client.go > File ...
8 years ago (2013-10-23 00:22:47 UTC) #16
hanwen-google
On Tue, Oct 22, 2013 at 3:07 PM, <dave@cheney.net> wrote: > I would really like ...
8 years ago (2013-10-23 00:56:29 UTC) #17
hanwen-google
I suspect I can factor out the sshConn{} change. On Tue, Oct 22, 2013 at ...
8 years ago (2013-10-23 01:22:48 UTC) #18
hanwen-google
Hello agl@golang.org, dave@cheney.net, jpsugar@google.com (cc: golang-dev@googlegroups.com), Please take another look.
8 years ago (2013-10-25 20:02:57 UTC) #19
dfc
On 2013/10/25 20:02:57, hanwen-google wrote: > Hello mailto:agl@golang.org, mailto:dave@cheney.net, mailto:jpsugar@google.com (cc: > mailto:golang-dev@googlegroups.com), > > ...
8 years ago (2013-10-25 23:40:45 UTC) #20
hanwen-google
On 2013/10/25 23:40:45, dfc wrote: > On 2013/10/25 20:02:57, hanwen-google wrote: > > Hello mailto:agl@golang.org, ...
8 years ago (2013-10-25 23:51:40 UTC) #21
agl1
LGTM. (For landing in gosshnew.) https://codereview.appspot.com/14494058/diff/460001/ssh/common.go File ssh/common.go (right): https://codereview.appspot.com/14494058/diff/460001/ssh/common.go#newcode153 ssh/common.go:153: // The maximum amount ...
7 years, 12 months ago (2013-10-29 14:33:47 UTC) #22
hanwen-google
If I remove the 1G rekeying and RekeyThreshold, can we land it in go.crypto? It ...
7 years, 12 months ago (2013-10-29 14:45:22 UTC) #23
hanwen-google
Hello agl@golang.org, dave@cheney.net, jpsugar@google.com (cc: golang-dev@googlegroups.com), Please take another look.
7 years, 12 months ago (2013-10-31 12:53:07 UTC) #24
hanwen-google
Also, RekeyThreshold is backward compatible, assuming people don't use unnamed inits of structs. Leaving it ...
7 years, 12 months ago (2013-10-31 12:54:57 UTC) #25
agl1
LGTM for the diffs. (Note: If I've said LGTM, but have included a few comments, ...
7 years, 12 months ago (2013-10-31 14:36:34 UTC) #26
dfc
LGTM with minor comments. https://codereview.appspot.com/14494058/diff/580001/ssh/common.go File ssh/common.go (right): https://codereview.appspot.com/14494058/diff/580001/ssh/common.go#newcode154 ssh/common.go:154: // new key is negotiated. ...
7 years, 11 months ago (2013-11-03 08:23:03 UTC) #27
hanwen-google
this has been submitted to gosshnew in https://code.google.com/p/gosshnew/source/detail?r=2e99c4d7cb96a1285281daad7fd23c86e36cb952
7 years, 11 months ago (2013-11-04 18:04:02 UTC) #28
hanwen-google
7 years, 11 months ago (2013-11-04 18:04:17 UTC) #29
*** Abandoned ***
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b