FAQ
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 ClientConn.SessionID(), is this intended ?
no; fixed.

https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode21
ssh/client.go:21: sshConn
On 2013/10/22 22:07:02, dfc wrote:
This is going to expose methods like ClientConn.SetDeadline() is this
intended ?

this is intended. transport embeds net.Conn. sshConn is just to hide
Write/Read.

https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode59
ssh/client.go:59: func (c *ClientConn) Close() error {
On 2013/10/22 22:07:02, dfc wrote:
Why is this needed ? how can ClientConn.sshConn.Conn ever be nil ?
Reverted (I managed to hit this debugging tests).

https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode82
ssh/client.go:82: _, _, err = c.sendKexInit()
On 2013/10/22 22:07:02, dfc wrote:
if _, _, err := c.sendKexInit(); err != nil {
return err
}
Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/client.go#newcode86
ssh/client.go:86:
On 2013/10/22 22:07:02, dfc wrote:
if packet, err := ... {
} else if { ... }
Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/client_auth.go
File ssh/client_auth.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/client_auth.go#newcode224
ssh/client_auth.go:224: sign, err := p.Sign(i, rand, data)
On 2013/10/22 22:07:02, dfc wrote:
Can you break this change out please ?
will do.

https://codereview.appspot.com/14494058/diff/180001/ssh/client_auth_test.go
File ssh/client_auth_test.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/client_auth_test.go#newcode366
ssh/client_auth_test.go:366: c.Close()
On 2013/10/22 22:07:02, dfc wrote:
this can be broken out
will do.

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#newcode361
ssh/common.go:361: // and writes.
On 2013/10/22 22:07:02, dfc wrote:
I think this can be implemented as
type sshConn interface {
Close() error
LocalAddr() Addr
RemoteAddr() Addr
SetDeadline(t time.Time) error
SetReadDeadline(t time.Time) error
SetWriteDeadline(t time.Time) error
}
maybe you want to filter out the Deadline methods.
sure. This means that ClientConn no longer is a net.Conn, though. Is
that a problem?

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#newcode17
ssh/handshake.go:17: //const debug = false
On 2013/10/22 22:12:57, agl1 wrote:
delete this line.
Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode20
ssh/handshake.go:20: // keyingTransport is packet based transport that
supports key
On 2013/10/22 22:12:57, agl1 wrote:
"a" after "is".
Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode42
ssh/handshake.go:42: // (why is Rand not part of CryptoConfig?)
On 2013/10/22 22:12:57, agl1 wrote:
// TODO: move into CryptoConfig
Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode196
ssh/handshake.go:196: // transport. This function is safe for
concurrent use by multiple
On 2013/10/22 22:12:57, agl1 wrote:
This file is inconsistent between one or two spaces after a period. (One is
better.)
Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode204
ssh/handshake.go:204: func (t *handshakeTransport) unlockedSendKexInit()
(*kexInitMsg, []byte, error) {
On 2013/10/22 22:12:57, agl1 wrote:
Isn't this lockedSendKexInit()?
Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode206
ssh/handshake.go:206: // Since send and receive of kex inits are not
On 2013/10/22 22:12:57, agl1 wrote:
This comment needs rewriting.
Done.

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/22 22:12:57, agl1 wrote:
On 2013/10/21 20:00:20, hanwen-google wrote:
On 2013/10/21 17:03:51, jpsugar wrote:
Um, this is pretty serious. No random bits is a security issue.
go.crypto only support DH and ECDH key exchanges, both of which have
randomness
of both sides. Maybe this is for GSS/Kerberos?

I wanted to address this in another CL (currently, the Cookie is not
init'd
either.) to simplify review.
Sadly the repo is Mercurial, so god knows how one checks back in time. But it's
Not Good that we're not filling in Cookie. I'm not sure when this
disappeared or
whether I screwed it up from the start.
try

    hg log -p | less

I believe it was never set.
This change doesn't have to include it, but the next one should.
understood.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake.go#newcode313
ssh/handshake.go:313: // We don't send FirstKexFollows, but we handle
receiving it
On 2013/10/22 22:12:57, agl1 wrote:
period at the end.
Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake_test.go
File ssh/handshake_test.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/handshake_test.go#newcode1
ssh/handshake_test.go:1: // Copyright 2011 The Go Authors. All rights
reserved.
On 2013/10/22 22:12:57, agl1 wrote:
2013
Done.

https://codereview.appspot.com/14494058/diff/180001/ssh/transport.go
File ssh/transport.go (right):

https://codereview.appspot.com/14494058/diff/180001/ssh/transport.go#newcode55
ssh/transport.go:55: func (t *transport) SessionID() []byte {
On 2013/10/22 22:07:02, dfc wrote:
does this need to return a copy of the sessionID ?
not necessarily, but it is safer and cost is neglible.

https://codereview.appspot.com/14494058/diff/180001/ssh/transport.go#newcode268
ssh/transport.go:268: func newTransport(conn io.ReadWriteCloser, rand
io.Reader, isClient bool) *transport {
On 2013/10/22 22:07:02, dfc wrote:
s/conn/rwc/
Done.

https://codereview.appspot.com/14494058/

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Search Discussions

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedOct 23, '13 at 12:15a
activeOct 23, '13 at 12:22a
posts2
users2
websitegolang.org

2 users in discussion

Dave Cheney: 1 post Hanwen: 1 post

People

Translate

site design / logo © 2021 Grokbase