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

Issue 16610043: code review 16610043: go.crypto/ssh: ensure {Server,Client}Conn do not expose... (Closed)

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

Description

go.crypto/ssh: ensure {Server,Client}Conn do not expose io.ReadWriter Transport should not be a ReadWriter. It can only write packets, i.e. no partial reads or writes. Furthermore, you can currently do ClientConn.Write() while the connection is live, which sends raw bytes over the connection. Doing so will confuse the transports because the data is not encrypted. As a consequence, ClientConn and ServerConn stop being a net.Conn Finally, ensure that {Server,Client}Conn implement LocalAddr and RemoteAddr methods that previously were exposed by an embedded net.Conn field.

Patch Set 1 #

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

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

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

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

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

Total comments: 1

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

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

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

Unified diffs Side-by-side diffs Delta from patch set Stats (+88 lines, -39 lines) Patch
M ssh/client.go View 1 2 3 4 5 6 13 chunks +25 lines, -16 lines 0 comments Download
M ssh/client_auth.go View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M ssh/common_test.go View 1 2 3 4 5 6 2 chunks +31 lines, -0 lines 0 comments Download
M ssh/server.go View 1 2 3 4 5 6 16 chunks +28 lines, -19 lines 0 comments Download
M ssh/session.go View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M ssh/tcpip.go View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 16
hanwen-google
I'm not thrilled by this proposal, to be honest, because: * lots of casts: ugh. ...
10 years, 5 months ago (2013-10-24 16:33:16 UTC) #1
dfc
On Fri, Oct 25, 2013 at 3:33 AM, <hanwen@google.com> wrote: > I'm not thrilled by ...
10 years, 5 months ago (2013-10-24 16:42:52 UTC) #2
dfc
Hello hanwen@google.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go.crypto
10 years, 5 months ago (2013-10-24 16:48:19 UTC) #3
hanwen-google
On Thu, Oct 24, 2013 at 9:42 AM, Dave Cheney <dave@cheney.net> wrote: >> * for ...
10 years, 5 months ago (2013-10-24 17:01:04 UTC) #4
dfc
You are correct. PTAL.
10 years, 5 months ago (2013-10-24 17:45:30 UTC) #5
hanwen-google
Why can't we go with my original CL? I've tried this option in my code ...
10 years, 5 months ago (2013-10-24 18:01:58 UTC) #6
dfc
> Why can't we go with my original CL? I've tried this option in my ...
10 years, 5 months ago (2013-10-24 18:09:46 UTC) #7
dfc
Hello hanwen@google.com (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 5 months ago (2013-10-24 18:18:06 UTC) #8
hanwen-google
On Thu, Oct 24, 2013 at 11:09 AM, Dave Cheney <dave@cheney.net> wrote: >> Why can't ...
10 years, 5 months ago (2013-10-24 18:20:28 UTC) #9
hanwen-google
On Thu, Oct 24, 2013 at 11:20 AM, Han-Wen Nienhuys <hanwen@google.com> wrote: >> I don't ...
10 years, 5 months ago (2013-10-24 18:22:45 UTC) #10
dfc
> Transport will be hidden one layer deeper when the rekeying moves into > a ...
10 years, 5 months ago (2013-10-24 18:23:12 UTC) #11
dfc
Hello hanwen@google.com (cc: golang-dev@googlegroups.com), Please take another look.
10 years, 5 months ago (2013-10-24 18:39:28 UTC) #12
dfc
On 2013/10/24 18:39:28, dfc wrote: > Hello mailto:hanwen@google.com (cc: mailto:golang-dev@googlegroups.com), > > Please take another ...
10 years, 5 months ago (2013-10-24 18:44:31 UTC) #13
hanwen-google
LGTM (sorry, had to run out for a bit)
10 years, 5 months ago (2013-10-24 19:15:48 UTC) #14
hanwen-google
if you haven't submitted yet, can you copy bits of the explanation from my CL ...
10 years, 5 months ago (2013-10-24 19:18:50 UTC) #15
dfc
10 years, 5 months ago (2013-10-24 19:30:05 UTC) #16
*** Submitted as
https://code.google.com/p/go/source/detail?r=213a06a7ce81&repo=crypto ***

go.crypto/ssh: ensure {Server,Client}Conn do not expose io.ReadWriter

Transport should not be a ReadWriter. It can only write packets, i.e. no partial
reads or writes. Furthermore, you can currently do ClientConn.Write() while the
connection is live, which sends raw bytes over the connection. Doing so will
confuse the transports because the data is not encrypted.

As a consequence, ClientConn and ServerConn stop being a net.Conn

Finally, ensure that {Server,Client}Conn implement LocalAddr and RemoteAddr
methods that previously were exposed by an embedded net.Conn field.

R=hanwen
CC=golang-dev
https://codereview.appspot.com/16610043
Sign in to reply to this message.

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