code review 6061052: ssh: cosmetic cleanups

ssh: cosmetic cleanups

These are the cosmetic cleanups from the bits of code that I
rereviewed.

1) stringLength now takes a int; the length of the string. Too many
   callers were allocating with stringLength([]byte(s)) and
   stringLength only needs to call len().

2) agent.go now has sendAndReceive to remove logic that was
   duplicated.

3) We now reject negative DH values

4) We now reject empty packets rather than crashing.

[agl1, 2012-04-19 17:04:58]

Hello dave@cheney.net (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://agl%40golang.org@code.google.com/p/go.crypto/

[dfc, 2012-04-20 10:01:10]

Thank you for this CL, especially taking a pass through the error messages.

I only have one comment regarding the change to stringLength, please see my
comment in messages.go

http://codereview.appspot.com/6061052/diff/5001/ssh/agent.go
File ssh/agent.go (right):

http://codereview.appspot.com/6061052/diff/5001/ssh/agent.go#newcode47
ssh/agent.go:47: const maxAgentResponseBytes = 16 << 20
Is this an RFC constant, or a sane default?

http://codereview.appspot.com/6061052/diff/5001/ssh/channel.go
File ssh/channel.go (left):

http://codereview.appspot.com/6061052/diff/5001/ssh/channel.go#oldcode388
ssh/channel.go:388: panic("unreachable")
Thank you for removing the panic.

http://codereview.appspot.com/6061052/diff/5001/ssh/messages.go
File ssh/messages.go (right):

http://codereview.appspot.com/6061052/diff/5001/ssh/messages.go#newcode546
ssh/messages.go:546: func stringLength(n int) int {
If stringLength() is being called almost exclusively with strings, not []byte's,
why not change it to accept a string, and add bytesLength() for the rare cases?

http://codereview.appspot.com/6061052/diff/5001/ssh/server.go
File ssh/server.go (right):

http://codereview.appspot.com/6061052/diff/5001/ssh/server.go#newcode146
ssh/server.go:146: }
This code is in two places, both wrong (not surpassing, client.go spawned from
the server implementation), is it possible to consolidate this function?

http://codereview.appspot.com/6061052/diff/5001/ssh/transport.go
File ssh/transport.go (right):

http://codereview.appspot.com/6061052/diff/5001/ssh/transport.go#newcode140
ssh/transport.go:140: return nil, errors.New("ssh: zero length packet")
I couldn't find anything in the RFC about a minimum packet length, but this
check sounds reasonable.

[jmp, 2012-04-20 12:44:24]

Just chiming in!

http://codereview.appspot.com/6061052/diff/5001/ssh/agent.go
File ssh/agent.go (right):

http://codereview.appspot.com/6061052/diff/5001/ssh/agent.go#newcode47
ssh/agent.go:47: const maxAgentResponseBytes = 16 << 20
On 2012/04/20 10:01:10, dfc wrote:
> Is this an RFC constant, or a sane default?

AFAIK, there is no RFC for SSH agent.  And PROTOCOL.agent does not mention this.
 However, every implementation (even by the author of PROTOCOL.agent) I have
seen establishes a constant like this for the purposes illustrated below.  So,
it is definitely a sane default.

http://codereview.appspot.com/6061052/diff/5001/ssh/agent.go#newcode131
ssh/agent.go:131: io.ReadWriter
In addition to the locking mechanism in my comment below, I changed this from
embedded to assigning a private variable name...

type AgentClient struct {
        conn io.ReadWriter
        mu sync.Mutex
}

...in order to prevent direct access to the Read and Write methods.

http://codereview.appspot.com/6061052/diff/5001/ssh/agent.go#newcode137
ssh/agent.go:137: func (ac *AgentClient) sendAndReceive(req []byte) (reply
interface{}, replyType uint8, err error) {
Thanks for doing this.  I had been meaning to consolidate the two functions. 
BTW, in my testing, this is also going to need a locking mechanism of some sort
to prevent concurrent uses of this function with the same AgentClient.  Since
the AgentClient has only a single connection to the ssh-agent, concurrent reads
and writes will trip over each other (causing parse errors) as the ssh-agent
will not distinguish between them.  It will only distinguish if there are
separate connections for each concurrent request.

I was using a sync.Mutex to lock before the Write and then defer the unlock, but
there may be a better way.

[agl1, 2012-04-20 19:19:29]

*** Submitted as
http://code.google.com/p/go/source/detail?r=06ef434db227&repo=crypto ***

ssh: cosmetic cleanups

These are the cosmetic cleanups from the bits of code that I
rereviewed.

1) stringLength now takes a int; the length of the string. Too many
   callers were allocating with stringLength([]byte(s)) and
   stringLength only needs to call len().

2) agent.go now has sendAndReceive to remove logic that was
   duplicated.

3) We now reject negative DH values

4) We now reject empty packets rather than crashing.

R=dave, jonathan.mark.pittman
CC=golang-dev
http://codereview.appspot.com/6061052

http://codereview.appspot.com/6061052/diff/5001/ssh/agent.go
File ssh/agent.go (right):

http://codereview.appspot.com/6061052/diff/5001/ssh/agent.go#newcode47
ssh/agent.go:47: const maxAgentResponseBytes = 16 << 20
On 2012/04/20 10:01:10, dfc wrote:
> Is this an RFC constant, or a sane default?

I made it up. Have amended the comment to make that clear.

http://codereview.appspot.com/6061052/diff/5001/ssh/agent.go#newcode137
ssh/agent.go:137: func (ac *AgentClient) sendAndReceive(req []byte) (reply
interface{}, replyType uint8, err error) {
On 2012/04/20 12:44:24, jmp wrote:
> I was using a sync.Mutex to lock before the Write and then defer the unlock,
but
> there may be a better way.

I think that adding locking is out of scope for this CL. But thanks for the
heads up.

What's the situation where you're using it concurrently? I guess you have a
client process that is making many SSH connections?

http://codereview.appspot.com/6061052/diff/5001/ssh/messages.go
File ssh/messages.go (right):

http://codereview.appspot.com/6061052/diff/5001/ssh/messages.go#newcode546
ssh/messages.go:546: func stringLength(n int) int {
On 2012/04/20 10:01:10, dfc wrote:
> If stringLength() is being called almost exclusively with strings, not
[]byte's,
> why not change it to accept a string, and add bytesLength() for the rare
cases?

I thought about it. But everything else here is named based on the data type
from the RFC. i.e. it's the length of the SSH `string'. So bytesLength seemed
like a greater anomaly than taking the length.

http://codereview.appspot.com/6061052/diff/5001/ssh/server.go
File ssh/server.go (right):

http://codereview.appspot.com/6061052/diff/5001/ssh/server.go#newcode146
ssh/server.go:146: }
On 2012/04/20 10:01:10, dfc wrote:
> This code is in two places, both wrong (not surpassing, client.go spawned from
> the server implementation), is it possible to consolidate this function?

Done.

[dfc, 2012-04-20 23:43:22]

> I thought about it. But everything else here is named based on the data type
> from the RFC. i.e. it's the length of the SSH `string'. So bytesLength seemed
> like a greater anomaly than taking the length.

As sound a reason as any.