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

Issue 14641044: code review 14641044: go.crypto/ssh: put version exchange in function (Closed)

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

Description

go.crypto/ssh: put version exchange in function

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 #

Total comments: 17

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

Total comments: 4

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

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

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

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

Total comments: 1

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

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

Total comments: 8

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

Total comments: 2

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

Total comments: 3

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

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+107 lines, -77 lines) Patch
M ssh/client.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +5 lines, -18 lines 0 comments Download
M ssh/client_test.go View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M ssh/server.go View 1 2 3 4 5 6 7 8 9 10 4 chunks +19 lines, -24 lines 0 comments Download
M ssh/transport.go View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +38 lines, -8 lines 0 comments Download
M ssh/transport_test.go View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +44 lines, -26 lines 0 comments Download

Messages

Total messages: 23
hanwen-google
Hello golang-dev@googlegroups.com (cc: agl@golang.org, dave@cheney.net), I'd like you to review this change to https://code.google.com/p/go.crypto
5 years, 11 months ago (2013-10-13 12:46:00 UTC) #1
dfc
Thanks for this, nice cleanly. My comments are mainly around who adds the "SSH-2.0-" prefix, ...
5 years, 11 months ago (2013-10-14 00:32:28 UTC) #2
hanwen-google
https://codereview.appspot.com/14641044/diff/6001/ssh/client_test.go File ssh/client_test.go (right): https://codereview.appspot.com/14641044/diff/6001/ssh/client_test.go#newcode29 ssh/client_test.go:29: testClientVersion(t, &ClientConfig{ClientVersion: version}, "SSH-2.0-"+version) On 2013/10/14 00:32:28, dfc wrote: ...
5 years, 11 months ago (2013-10-14 06:41:44 UTC) #3
dfc
Thank you. I would like to see the logic in exchangeVersion made more strict. If ...
5 years, 11 months ago (2013-10-14 06:55:15 UTC) #4
hanwen-google
I'm always adding the SSH-2.0- line (I see no value in letting the user supply ...
5 years, 11 months ago (2013-10-14 13:01:48 UTC) #5
jpsugar
This changes the semantics of ClientVersion. I have a server that expects an "XSH-" prefix ...
5 years, 11 months ago (2013-10-14 18:33:58 UTC) #6
hanwen-google
On Mon, Oct 14, 2013 at 8:33 PM, <jpsugar@google.com> wrote: > This changes the semantics ...
5 years, 11 months ago (2013-10-14 19:54:49 UTC) #7
jpsugar
On Mon, Oct 14, 2013 at 12:54 PM, Han-Wen Nienhuys <hanwen@google.com> wrote: > Ugh. What ...
5 years, 11 months ago (2013-10-14 19:59:00 UTC) #8
hanwen-google
The RFC says to ignore everything before the SSH-2.0- line, for the following reason. The ...
5 years, 11 months ago (2013-10-14 20:14:17 UTC) #9
jpsugar
On Mon, Oct 14, 2013 at 1:13 PM, Han-Wen Nienhuys <hanwen@google.com> wrote: > We could ...
5 years, 11 months ago (2013-10-14 20:17:12 UTC) #10
dfc
not lgtm. If the client supplies a version that should be passed through unmolested to ...
5 years, 11 months ago (2013-10-15 04:46:21 UTC) #11
hanwen-google
On 2nd thought JP's problem means that we have to accept any and all version ...
5 years, 11 months ago (2013-10-15 05:13:26 UTC) #12
hanwen-google
PTAL On Mon, Oct 14, 2013 at 9:46 PM, <dave@cheney.net> wrote: > not lgtm. > ...
5 years, 11 months ago (2013-10-15 14:29:38 UTC) #13
dfc
Thank you. Some small nits. agl/jp, I'd like you to review this. https://codereview.appspot.com/14641044/diff/48002/ssh/transport.go File ssh/transport.go ...
5 years, 11 months ago (2013-10-16 01:44:20 UTC) #14
hanwen-google
https://codereview.appspot.com/14641044/diff/48002/ssh/transport.go File ssh/transport.go (right): https://codereview.appspot.com/14641044/diff/48002/ssh/transport.go#newcode361 ssh/transport.go:361: var packageVersion = []byte("SSH-2.0-Go") On 2013/10/16 01:44:21, dfc wrote: ...
5 years, 11 months ago (2013-10-16 03:00:17 UTC) #15
dfc
I'm fine with this CL as it stands now, if like to wait for agl ...
5 years, 11 months ago (2013-10-16 03:03:22 UTC) #16
agl1
No objections. https://codereview.appspot.com/14641044/diff/53001/ssh/transport.go File ssh/transport.go (right): https://codereview.appspot.com/14641044/diff/53001/ssh/transport.go#newcode412 ssh/transport.go:412: // all of it (version and comments) ...
5 years, 11 months ago (2013-10-16 13:46:22 UTC) #17
hanwen-google
https://codereview.appspot.com/14641044/diff/53001/ssh/transport.go File ssh/transport.go (right): https://codereview.appspot.com/14641044/diff/53001/ssh/transport.go#newcode412 ssh/transport.go:412: // all of it (version and comments) go into ...
5 years, 11 months ago (2013-10-16 16:40:18 UTC) #18
jpsugar
LGTM https://codereview.appspot.com/14641044/diff/8001/ssh/client.go File ssh/client.go (right): https://codereview.appspot.com/14641044/diff/8001/ssh/client.go#newcode67 ssh/client.go:67: serverVersion, err := exchangeVersions(c.transport.Conn, []byte(clientVersion)) No need for ...
5 years, 11 months ago (2013-10-16 17:59:58 UTC) #19
hanwen-google
https://codereview.appspot.com/14641044/diff/8001/ssh/client.go File ssh/client.go (right): https://codereview.appspot.com/14641044/diff/8001/ssh/client.go#newcode67 ssh/client.go:67: serverVersion, err := exchangeVersions(c.transport.Conn, []byte(clientVersion)) On 2013/10/16 17:59:58, jpsugar ...
5 years, 11 months ago (2013-10-16 18:38:25 UTC) #20
hanwen-google
https://codereview.appspot.com/14641044/diff/8001/ssh/client.go File ssh/client.go (right): https://codereview.appspot.com/14641044/diff/8001/ssh/client.go#newcode67 ssh/client.go:67: serverVersion, err := exchangeVersions(c.transport.Conn, []byte(clientVersion)) On 2013/10/16 17:59:58, jpsugar ...
5 years, 11 months ago (2013-10-16 18:44:18 UTC) #21
dfc
On 2013/10/16 18:44:18, hanwen-google wrote: > https://codereview.appspot.com/14641044/diff/8001/ssh/client.go > File ssh/client.go (right): > > https://codereview.appspot.com/14641044/diff/8001/ssh/client.go#newcode67 > ...
5 years, 11 months ago (2013-10-16 21:51:12 UTC) #22
dfc
5 years, 11 months ago (2013-10-16 21:57:16 UTC) #23
*** Submitted as
https://code.google.com/p/go/source/detail?r=5ff5636e18c9&repo=crypto ***

go.crypto/ssh: put version exchange in function

R=golang-dev, dave, jpsugar, agl
CC=golang-dev
https://codereview.appspot.com/14641044

Committer: Dave Cheney <dave@cheney.net>
Sign in to reply to this message.

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