code review 5342057: exp/ssh: Add support for (most) of the ciphers from RFC...

exp/ssh: Add support for (most) of the ciphers from RFC4253, RFC4344 and RFC4345.

[huin-google, 2011-11-09 02:43:08]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[huin-google, 2011-11-09 02:45:06]

http://codereview.appspot.com/5342057/diff/4001/src/pkg/Makefile
File src/pkg/Makefile (right):

http://codereview.appspot.com/5342057/diff/4001/src/pkg/Makefile#newcode89
src/pkg/Makefile:89: exp/ssh\
I'm unsure if I should add this or not as part of this change. Was there a
reason it was not added already?

[dave_cheney.net, 2011-11-09 08:52:58]

This is really nice. Thank you for this. I can't comment on the crypto changes
with any great authority, so I've looped in agl.

http://codereview.appspot.com/5342057/diff/4001/src/pkg/Makefile
File src/pkg/Makefile (right):

http://codereview.appspot.com/5342057/diff/4001/src/pkg/Makefile#newcode89
src/pkg/Makefile:89: exp/ssh\
I think you're probably right, but it's probably a call for the core commiters.

On 2011/11/09 02:45:06, huin-google wrote:
> I'm unsure if I should add this or not as part of this change. Was there a
> reason it was not added already?

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/Makefile
File src/pkg/exp/ssh/Makefile (right):

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/Makefile#newc...
src/pkg/exp/ssh/Makefile:15: transport.go\
Could you please move transport to it's correct place in the sequence.

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/cipher.go
File src/pkg/exp/ssh/cipher.go (right):

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/cipher.go#new...
src/pkg/exp/ssh/cipher.go:52: return fmt.Sprintf("bad crypt direction: %d",
cryptDirection(err))
Maybe add a String method on cryptDirection?

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/cipher.go#new...
src/pkg/exp/ssh/cipher.go:236: // minus algorithms that are not implemented.
for consistency with the rest of the package, include a ref. to the RFC?

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/client.go
File src/pkg/exp/ssh/client.go (right):

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/client.go#new...
src/pkg/exp/ssh/client.go:306: // The allowed cipher algorithms. If unspecified
then a default order is used.
Maybe mention where the default order is listed in the RFC.

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/server.go
File src/pkg/exp/ssh/server.go (right):

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/server.go#new...
src/pkg/exp/ssh/server.go:44: // The allowed cipher algorithms. If unspecified
then a default order is used.
Same as client.go

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/transport.go
File src/pkg/exp/ssh/transport.go (right):

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/transport.go#...
src/pkg/exp/ssh/transport.go:21: minPacketSizeMultiple = 8 // TODO(dfc) does
this need to be configurable?
I guess it doesn't need to be configurable, can you please remove the TODO.

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/transport.go#...
src/pkg/exp/ssh/transport.go:257: cipher: noneCipher{},
This is great. I had hoped it would be possible to remove the conditional logic
in writePacket with an empty cipher. Thank you.

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/transport.go#...
src/pkg/exp/ssh/transport.go:287: func (c *common) setupKeys(d direction, cd
cryptDirection, K, H, sessionId []byte, hashFunc crypto.Hash) error {
This function declaration is getting to be a whopper. Is there anything that can
be done to simplify it ?

[huin-google, 2011-11-09 12:12:15]

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

Please take another look.

[huin-google, 2011-11-09 12:12:32]

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/Makefile
File src/pkg/exp/ssh/Makefile (right):

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/Makefile#newc...
src/pkg/exp/ssh/Makefile:15: transport.go\
On 2011/11/09 08:52:58, dfc wrote:
> Could you please move transport to it's correct place in the sequence. 

Done.

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/cipher.go
File src/pkg/exp/ssh/cipher.go (right):

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/cipher.go#new...
src/pkg/exp/ssh/cipher.go:52: return fmt.Sprintf("bad crypt direction: %d",
cryptDirection(err))
On 2011/11/09 08:52:58, dfc wrote:
> Maybe add a String method on cryptDirection?

Done.

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/cipher.go#new...
src/pkg/exp/ssh/cipher.go:236: // minus algorithms that are not implemented.
On 2011/11/09 08:52:58, dfc wrote:
> for consistency with the rest of the package, include a ref. to the RFC?

The set and order of algorithms here is from OpenSSH rather than an RFC. Is
there an RFC with a suggested set/order in that I could use and cite instead?

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/client.go
File src/pkg/exp/ssh/client.go (right):

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/client.go#new...
src/pkg/exp/ssh/client.go:306: // The allowed cipher algorithms. If unspecified
then a default order is used.
On 2011/11/09 08:52:58, dfc wrote:
> Maybe mention where the default order is listed in the RFC.

It isn't... that I know of. Maybe I should expose or otherwise advertise the
contents of the default order variable in the docs, as well as citing OpenSSH?

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/server.go
File src/pkg/exp/ssh/server.go (right):

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/server.go#new...
src/pkg/exp/ssh/server.go:44: // The allowed cipher algorithms. If unspecified
then a default order is used.
On 2011/11/09 08:52:58, dfc wrote:
> Same as client.go

I'll do some refactoring to make it common, putting this in a new,
commonly-used-struct, that will specify crypto algorithms to be used. I intend
to be filling in some others, later (such as MAC algorithm).

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/transport.go
File src/pkg/exp/ssh/transport.go (right):

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/transport.go#...
src/pkg/exp/ssh/transport.go:21: minPacketSizeMultiple = 8 // TODO(dfc) does
this need to be configurable?
On 2011/11/09 08:52:58, dfc wrote:
> I guess it doesn't need to be configurable, can you please remove the TODO.

Done.

http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/transport.go#...
src/pkg/exp/ssh/transport.go:287: func (c *common) setupKeys(d direction, cd
cryptDirection, K, H, sessionId []byte, hashFunc crypto.Hash) error {
On 2011/11/09 08:52:58, dfc wrote:
> This function declaration is getting to be a whopper. Is there anything that
can
> be done to simplify it ?

I have made an attempt at this, moving K, H and hashFunc into a "kexResult"
struct type that is returned from the kexDH methods, as they are all determined
by the key-exchange algorithm, ultimately.

sessionId doesn't fit in there too logically, as it comes from the first H
value, but stays constant across subsequent key-re-exchanges (not that those are
implemented yet, although they should be for longer-lived connections).

Not sure that I'm 100% happy with the change, but maybe it's an improvement.

[dave_cheney.net, 2011-11-09 12:46:22]

Re: the default algo list, after I made that comment I went looking and realized
that there isn't one specified in the rfc. I think it's fine to say we follow
the default OpenSSH list, or possibly put the list in a comment at the top of
cipher.go. I think godoc will pick it up if you follow the same format as
doc.go.

Re: the size of setupkeys, if it makes the intent of the code less clear then
feel free to ignore m y suggestion, I was just overreacting to the size of the
method prototype. 

Sent from my iPad

On 09/11/2011, at 23:12, huin@google.com wrote:

> 
> http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/Makefile
> File src/pkg/exp/ssh/Makefile (right):
> 
>
http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/Makefile#newc...
> src/pkg/exp/ssh/Makefile:15: transport.go\
> On 2011/11/09 08:52:58, dfc wrote:
>> Could you please move transport to it's correct place in the sequence.
> 
> 
> Done.
> 
> http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/cipher.go
> File src/pkg/exp/ssh/cipher.go (right):
> 
>
http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/cipher.go#new...
> src/pkg/exp/ssh/cipher.go:52: return fmt.Sprintf("bad crypt direction:
> %d", cryptDirection(err))
> On 2011/11/09 08:52:58, dfc wrote:
>> Maybe add a String method on cryptDirection?
> 
> Done.
> 
>
http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/cipher.go#new...
> src/pkg/exp/ssh/cipher.go:236: // minus algorithms that are not
> implemented.
> On 2011/11/09 08:52:58, dfc wrote:
>> for consistency with the rest of the package, include a ref. to the
> RFC?
> 
> The set and order of algorithms here is from OpenSSH rather than an RFC.
> Is there an RFC with a suggested set/order in that I could use and cite
> instead?
> 
> http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/client.go
> File src/pkg/exp/ssh/client.go (right):
> 
>
http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/client.go#new...
> src/pkg/exp/ssh/client.go:306: // The allowed cipher algorithms. If
> unspecified then a default order is used.
> On 2011/11/09 08:52:58, dfc wrote:
>> Maybe mention where the default order is listed in the RFC.
> 
> It isn't... that I know of. Maybe I should expose or otherwise advertise
> the contents of the default order variable in the docs, as well as
> citing OpenSSH?
> 
> http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/server.go
> File src/pkg/exp/ssh/server.go (right):
> 
>
http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/server.go#new...
> src/pkg/exp/ssh/server.go:44: // The allowed cipher algorithms. If
> unspecified then a default order is used.
> On 2011/11/09 08:52:58, dfc wrote:
>> Same as client.go
> 
> I'll do some refactoring to make it common, putting this in a new,
> commonly-used-struct, that will specify crypto algorithms to be used. I
> intend to be filling in some others, later (such as MAC algorithm).
> 
> http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/transport.go
> File src/pkg/exp/ssh/transport.go (right):
> 
>
http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/transport.go#...
> src/pkg/exp/ssh/transport.go:21: minPacketSizeMultiple = 8 // TODO(dfc)
> does this need to be configurable?
> On 2011/11/09 08:52:58, dfc wrote:
>> I guess it doesn't need to be configurable, can you please remove the
> TODO.
> 
> Done.
> 
>
http://codereview.appspot.com/5342057/diff/4001/src/pkg/exp/ssh/transport.go#...
> src/pkg/exp/ssh/transport.go:287: func (c *common) setupKeys(d
> direction, cd cryptDirection, K, H, sessionId []byte, hashFunc
> crypto.Hash) error {
> On 2011/11/09 08:52:58, dfc wrote:
>> This function declaration is getting to be a whopper. Is there
> anything that can
>> be done to simplify it ?
> 
> I have made an attempt at this, moving K, H and hashFunc into a
> "kexResult" struct type that is returned from the kexDH methods, as they
> are all determined by the key-exchange algorithm, ultimately.
> 
> sessionId doesn't fit in there too logically, as it comes from the first
> H value, but stays constant across subsequent key-re-exchanges (not that
> those are implemented yet, although they should be for longer-lived
> connections).
> 
> Not sure that I'm 100% happy with the change, but maybe it's an
> improvement.
> 
> http://codereview.appspot.com/5342057/

[huin-google, 2011-11-09 13:36:52]

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/server.go
File src/pkg/exp/ssh/server.go (right):

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/server.go#ne...
src/pkg/exp/ssh/server.go:44: // Cryptographic-related conifguration.
TODO: Correct spelling.

[huin-google, 2011-11-09 14:32:29]

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go
File src/pkg/exp/ssh/cipher.go (right):

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:334: "aes128-ctr": &blockCipherFactory{
It turns out that the aes*-ctr ciphers still retain the output block size of 16
bytes for packetization, despite the effective streaming of CTR mode. Similar
for twofish and blowfish.

[agl1, 2011-11-09 14:51:08]

My suggestion for this CL is to try and drastically simplify it and make it do
less.

If there's a significant population of SSH servers/clients that we can't talk to
without some of these ciphersuites, then that's a good argument for including
them. But we shouldn't try to support anything. CAST5 will probably be removed
from the standard library soon and, otherwise, I think anything outside of
AES-CTR and RC4 is probably too much.


Cheers

AGL

http://codereview.appspot.com/5342057/diff/11002/src/pkg/Makefile
File src/pkg/Makefile (right):

http://codereview.appspot.com/5342057/diff/11002/src/pkg/Makefile#newcode89
src/pkg/Makefile:89: exp/ssh\
Not in this change please.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go
File src/pkg/exp/ssh/cipher.go (right):

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:27: cryptDirectionEncrypt = cryptDirection(iota)
can these just be named "encrypt" and "decrypt"?

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:31: func (cd cryptDirection) String() string {
I feel that this is excessive, although I haven't reached any uses of it yet.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:42: Expected int
since the struct is private, any reason why the members are public?

expected, actual int

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:66: type cipherFactory interface {
Might this just be a struct so that keySize and ivSize were just int members?

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:75: CreateCipher(key []byte, iv []byte, direction
cryptDirection) (blk cipher.BlockMode, err error)
I don't think that naming the return values adds anything.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:75: CreateCipher(key []byte, iv []byte, direction
cryptDirection) (blk cipher.BlockMode, err error)
key, iv []byte

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:90: func (c noneCipher) CreateCipher(key []byte, iv
[]byte, direction cryptDirection) (cipher.BlockMode, error) {
key, iv []byte

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:122: return nil, keySizeError{factory.keySize,
len(key)}
These errors indicate a bug in this package, no? It's up to use to generate
enough key material for the ciphersuite. Thus a panic would seem to be more
suitable.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:138: // The keystream bits written to streamDump are
sensitive, and should be
They're not really that sensitive when you consider that the cipher's internal
state is kept in the same address space.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:211: return nil, badCryptDirectionError(direction)
again, this is an internal error.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/config.go
File src/pkg/exp/ssh/config.go (right):

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/config.go#ne...
src/pkg/exp/ssh/config.go:1: // Copyright 2011 Google Inc. All Rights Reserved.
It doesn't seem that this file justifies itself yet. Maybe in a later change
several config structures could be moved but just this one doesn't seem to be
enough.

[huin-google, 2011-11-10 02:04:36]

I've removed 3DES ciphers (as that's well known as broken, pretty much only put
it in as it was marked as required in RFC4253), and also removed CAST5 based
ciphers, as its removal from the standard library is a good case to remove it
here, also.

As for the rest, are there strong arguments against supporting CBC mode and
Blowfish/Twofish based ciphers? At this point their implementations are
essentially just items in a table in cipher.go.

There are probably arguments for their inclusion for both compatibility, and
people who like to use slightly more obscure/less-used algorithms, which would
seem to be a matter of taste rather than something I'd want to force.

I'd be happier about removing more of these algorithms if I also make provision
for people to supply their own CryptoConfig cipherMap to augment/override those
in cipher.go - which is another change I have in mind, but didn't want to
include in the public interface in a first pass.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/Makefile
File src/pkg/Makefile (right):

http://codereview.appspot.com/5342057/diff/11002/src/pkg/Makefile#newcode89
src/pkg/Makefile:89: exp/ssh\
On 2011/11/09 14:51:08, agl1 wrote:
> Not in this change please.

Done.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go
File src/pkg/exp/ssh/cipher.go (right):

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:27: cryptDirectionEncrypt = cryptDirection(iota)
On 2011/11/09 14:51:08, agl1 wrote:
> can these just be named "encrypt" and "decrypt"?

Done.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:31: func (cd cryptDirection) String() string {
On 2011/11/09 14:51:08, agl1 wrote:
> I feel that this is excessive, although I haven't reached any uses of it yet.

There aren't any. It was suggested for addition, although I'll happily remove it
as dead code.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:42: Expected int
On 2011/11/09 14:51:08, agl1 wrote:
> since the struct is private, any reason why the members are public?
> 
> expected, actual int

Done. Same for ivSizeError.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:66: type cipherFactory interface {
On 2011/11/09 14:51:08, agl1 wrote:
> Might this just be a struct so that keySize and ivSize were just int members?

No, there are two implementations of cipherFactory: *streamCipherFactory and
*blockCipherFactory.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:75: CreateCipher(key []byte, iv []byte, direction
cryptDirection) (blk cipher.BlockMode, err error)
On 2011/11/09 14:51:08, agl1 wrote:
> I don't think that naming the return values adds anything.

Done.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:75: CreateCipher(key []byte, iv []byte, direction
cryptDirection) (blk cipher.BlockMode, err error)
On 2011/11/09 14:51:08, agl1 wrote:
> key, iv []byte

Done.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:90: func (c noneCipher) CreateCipher(key []byte, iv
[]byte, direction cryptDirection) (cipher.BlockMode, error) {
On 2011/11/09 14:51:08, agl1 wrote:
> key, iv []byte

Done, and elsewhere.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:122: return nil, keySizeError{factory.keySize,
len(key)}
On 2011/11/09 14:51:08, agl1 wrote:
> These errors indicate a bug in this package, no? It's up to use to generate
> enough key material for the ciphersuite. Thus a panic would seem to be more
> suitable.

Changed to a panic. Is it reasonable to panic with an error?

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:138: // The keystream bits written to streamDump are
sensitive, and should be
On 2011/11/09 14:51:08, agl1 wrote:
> They're not really that sensitive when you consider that the cipher's internal
> state is kept in the same address space.

True. I was thinking that key material should be wiped (.Reset methods on
various cipher types) at some point, but given how much this data is passed
around it hardly seems worth it.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:211: return nil, badCryptDirectionError(direction)
On 2011/11/09 14:51:08, agl1 wrote:
> again, this is an internal error.

Changed to a panic.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:334: "aes128-ctr": &blockCipherFactory{
On 2011/11/09 14:32:29, huin-google wrote:
> It turns out that the aes*-ctr ciphers still retain the output block size of
16
> bytes for packetization, despite the effective streaming of CTR mode. Similar
> for twofish and blowfish.

Done.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/config.go
File src/pkg/exp/ssh/config.go (right):

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/config.go#ne...
src/pkg/exp/ssh/config.go:1: // Copyright 2011 Google Inc. All Rights Reserved.
On 2011/11/09 14:51:08, agl1 wrote:
> It doesn't seem that this file justifies itself yet. Maybe in a later change
> several config structures could be moved but just this one doesn't seem to be
> enough.

Indeed not. I actually wanted to move the Rand io.Reader in here as well, but
ran into some subtle issues that seemed to break. I'll move this type into
common.go for now, for want of a better place.

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/server.go
File src/pkg/exp/ssh/server.go (right):

http://codereview.appspot.com/5342057/diff/11002/src/pkg/exp/ssh/server.go#ne...
src/pkg/exp/ssh/server.go:44: // Cryptographic-related conifguration.
On 2011/11/09 13:36:52, huin-google wrote:
> TODO: Correct spelling.

Done.

[huin-google, 2011-11-10 02:05:44]

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

Please take another look.

[taruti, 2011-11-10 10:06:19]

On Thu, 10 Nov 2011 02:04:37 +0000, huin@google.com wrote:
> I've removed 3DES ciphers (as that's well known as broken, pretty much
> only put it in as it was marked as required in RFC4253), and also
> removed CAST5 based ciphers, as its removal from the standard library is
> a good case to remove it here, also.
> 
> As for the rest, are there strong arguments against supporting CBC mode
> and Blowfish/Twofish based ciphers? At this point their implementations
> are essentially just items in a table in cipher.go.

iirc CBC-mode is broken for ssh for all ciphers.

http://www.kb.cert.org/vuls/id/958563

Currently the practise is to use CTR-mode.

- Taru Karttunen

[huin-google, 2011-11-10 13:00:38]

On 2011/11/10 10:06:19, taruti wrote:
> On Thu, 10 Nov 2011 02:04:37 +0000, mailto:huin@google.com wrote:
> > I've removed 3DES ciphers (as that's well known as broken, pretty much
> > only put it in as it was marked as required in RFC4253), and also
> > removed CAST5 based ciphers, as its removal from the standard library is
> > a good case to remove it here, also.
> > 
> > As for the rest, are there strong arguments against supporting CBC mode
> > and Blowfish/Twofish based ciphers? At this point their implementations
> > are essentially just items in a table in cipher.go.
> 
> iirc CBC-mode is broken for ssh for all ciphers.
> 
> http://www.kb.cert.org/vuls/id/958563
> 
> Currently the practise is to use CTR-mode.
> 
> - Taru Karttunen

Okay, that seems like a good reason to discard the CBC modes (at least in the
default cipher suite). Although it's interesting that OpenSSH still includes CBC
ciphers in its default cipher suite.

[rsc, 2011-11-10 15:40:23]

Please delete twofish and blowfish.
The implementations are more than a table item.
They introduce dependencies on those libraries
for anyone using exp/ssh.

Russ

[huin-google, 2011-11-14 11:03:00]

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

Please take another look.

[huin-google, 2011-11-14 11:06:41]

On 2011/11/14 11:03:00, huin-google wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:dave@cheney.net,
mailto:agl@golang.org,
> mailto:taruti@taruti.net, mailto:rsc@golang.org (cc:
mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

This latest change strips down to simply aes*-ctr and arcfour* ciphers. I've
removed the crypt direction enumeration for now, as it's only strictly needed by
CBC cipher mode.

I plan to write another CL to open up the API to allow people to implement their
own ciphers which will re-introduce the crypt direction.

[agl1, 2011-11-14 17:34:27]

This patch still has too much code: the concept of a blocksize appears
repeatedly, but everything supported is a stream cipher and there's a lot of
non-idiomatic code.

I think that starting with a smaller change would be very beneficial.

Cheers

AGL

http://codereview.appspot.com/5342057/diff/10024/src/pkg/exp/ssh/cipher.go
File src/pkg/exp/ssh/cipher.go (right):

http://codereview.appspot.com/5342057/diff/10024/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:19: type keySizeError struct {
panic takes a string, these Error structs aren't needed.

[huin-google, 2011-11-14 17:54:07]

On 2011/11/14 17:34:27, agl1 wrote:
> This patch still has too much code: the concept of a blocksize appears
> repeatedly, but everything supported is a stream cipher and there's a lot of
> non-idiomatic code.
> 
> I think that starting with a smaller change would be very beneficial.
> 
> Cheers
> 
> AGL
> 
> http://codereview.appspot.com/5342057/diff/10024/src/pkg/exp/ssh/cipher.go
> File src/pkg/exp/ssh/cipher.go (right):
> 
>
http://codereview.appspot.com/5342057/diff/10024/src/pkg/exp/ssh/cipher.go#ne...
> src/pkg/exp/ssh/cipher.go:19: type keySizeError struct {
> panic takes a string, these Error structs aren't needed.

I can see about stripping this down some more - I can certainly see that this
can be done now that the set of ciphers is so specific compared to the original
patch.

I would appreciate specifics on what is non-idiomatic, otherwise I cannot
address that point.

[huin-google, 2011-11-15 15:26:41]

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

Please take another look.

[rsc, 2011-11-16 21:53:17]

http://codereview.appspot.com/5342057/diff/9022/src/pkg/exp/ssh/cipher.go
File src/pkg/exp/ssh/cipher.go (right):

http://codereview.appspot.com/5342057/diff/9022/src/pkg/exp/ssh/cipher.go#new...
src/pkg/exp/ssh/cipher.go:14: // Used to dump the initial keystream for stream
ciphers, and otherwise act as
Full sentences, please.
http://golang.org/doc/effective_go.html#commentary

http://codereview.appspot.com/5342057/diff/9022/src/pkg/exp/ssh/cipher.go#new...
src/pkg/exp/ssh/cipher.go:19: // A cipherFactory creates instances of ciphers
that implement cipher.BlockMode.
I think this code is far more general than it needs to be.

type cipherMode struct {
    key int
    iv int
    skip int
    new func(key, iv []byte) (cipher.Stream, error)
}

func newAESCTR(key, iv []byte) (cipher.Stream, error) {
    c, err := aes.NewCipher(key)
    if err != nil {
        return nil, err
    }
    return cipher.NewCTR(c, iv)
}

func newRC4(key, iv []byte) (cipher.Stream, error) {
    return rc4.NewCipher(key)
}

var cipherModes = map[string]*cipherMode{
    "aes128-ctr": &cipherMode{16, aes.BlockSize, 0, newAESCTR},
    "arcfour256": &cipherMode{32, 0, 1536, newRC4},
}

http://codereview.appspot.com/5342057/diff/9022/src/pkg/exp/ssh/cipher.go#new...
src/pkg/exp/ssh/cipher.go:32: // noneCipher implements cipher.BlockMode.
This appears to be unused.

http://codereview.appspot.com/5342057/diff/9022/src/pkg/exp/ssh/cipher.go#new...
src/pkg/exp/ssh/cipher.go:128: return streamBlock{
return &streamBlock

[rsc, 2011-11-16 21:58:33]

Also, is anyone running servers that refuse AES but accept RC4?
My understanding was that, as it says at the bottom of godoc
crypto/rc4:

[rsc, 2011-11-16 21:58:47]

On Wed, Nov 16, 2011 at 16:58, Russ Cox <rsc@golang.org> wrote:
> Also, is anyone running servers that refuse AES but accept RC4?
> My understanding was that, as it says at the bottom of godoc
> crypto/rc4:

RC4 is in common use but has design weaknesses that make
it a poor choice for new protocols.

[huin-google, 2011-11-17 10:32:44]

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

Please take another look.

[huin-google, 2011-11-17 10:39:48]

On 2011/11/16 21:58:47, rsc wrote:
> On Wed, Nov 16, 2011 at 16:58, Russ Cox <mailto:rsc@golang.org> wrote:
> > Also, is anyone running servers that refuse AES but accept RC4?
> > My understanding was that, as it says at the bottom of godoc
> > crypto/rc4:
> 
> RC4 is in common use but has design weaknesses that make
> it a poor choice for new protocols.

My intent for this patch was twofold:

1. Compatibility. As you observe, it's most likely that most servers support
aes*-ctr, so the addition of arcfour-based ciphers are unlikely to be of great
benefit in this regard.

2. Choice. It's up to the user to decide which ciphers are applicable for their
use case. As one example: I believe that arcfour is desireable in cases where
privacy of the stream via the cipher is not paramount, and the greater concern
is to achieve greater throughput by using a more CPU-efficient cipher where the
transport layer is CPU-bound instead of network-bound. RFC4345 addresses some of
the concerns of arcfour weaknesses.

I do not believe it is our place to decide what ciphers a user can and cannot
use, but rather only to provide sensible recommendations/defaults.

[huin-google, 2011-11-17 10:40:11]

http://codereview.appspot.com/5342057/diff/9022/src/pkg/exp/ssh/cipher.go
File src/pkg/exp/ssh/cipher.go (right):

http://codereview.appspot.com/5342057/diff/9022/src/pkg/exp/ssh/cipher.go#new...
src/pkg/exp/ssh/cipher.go:14: // Used to dump the initial keystream for stream
ciphers, and otherwise act as
On 2011/11/16 21:53:17, rsc wrote:
> Full sentences, please.
> http://golang.org/doc/effective_go.html#commentary

Done.

http://codereview.appspot.com/5342057/diff/9022/src/pkg/exp/ssh/cipher.go#new...
src/pkg/exp/ssh/cipher.go:19: // A cipherFactory creates instances of ciphers
that implement cipher.BlockMode.
On 2011/11/16 21:53:17, rsc wrote:
> I think this code is far more general than it needs to be.
> 
> type cipherMode struct {
>     key int
>     iv int
>     skip int
>     new func(key, iv []byte) (cipher.Stream, error)
> }
> 
> func newAESCTR(key, iv []byte) (cipher.Stream, error) {
>     c, err := aes.NewCipher(key)
>     if err != nil {
>         return nil, err
>     }
>     return cipher.NewCTR(c, iv)
> }
> 
> func newRC4(key, iv []byte) (cipher.Stream, error) {
>     return rc4.NewCipher(key)
> }
> 
> var cipherModes = map[string]*cipherMode{
>     "aes128-ctr": &cipherMode{16, aes.BlockSize, 0, newAESCTR},
>     "arcfour256": &cipherMode{32, 0, 1536, newRC4},
> }

I will go along with this, but:

This code does not report the cipher's effective block size which is required by
the transport layer. The transport can assume an effective block size of 16
always, but this is wasteful of entropy for padding (and minimally for
bandwidth) in the case of the arcfour cipher which only needs an effective block
size of 8 (rfc4253 section 6). The block size could be stored in the cipherMode
structure, but it has then become separated from the cipher in use, and the
transport will have to keep track of it separately. For this change I think
things will have to be kept simple and for the transport to assume an effective
block size of 16.

My next intended change was to open up the cipher factory API to allow users of
exp/ssh to supply their own ciphers. Maybe I'm thinking too far ahead with this
generalised code (which is the remnants of a much fuller cipher suite). The
assumptions that can be made with a stream cipher cannot be made with block
ciphers, but the assumptions can be made in the opposite direction. I will roll
the generalization of block ciphers out of this patch, but they'd have to be
rolled back in for the next intended one.

http://codereview.appspot.com/5342057/diff/9022/src/pkg/exp/ssh/cipher.go#new...
src/pkg/exp/ssh/cipher.go:32: // noneCipher implements cipher.BlockMode.
On 2011/11/16 21:53:17, rsc wrote:
> This appears to be unused.

It is used in the transport as the initial cipher, which discards special-case
handling in the transport layer in initial key-exchange.

http://codereview.appspot.com/5342057/diff/9022/src/pkg/exp/ssh/cipher.go#new...
src/pkg/exp/ssh/cipher.go:128: return streamBlock{
On 2011/11/16 21:53:17, rsc wrote:
> return &streamBlock

Done, and the receiver argument for streamBlock is now a pointer to match here
and in arcfour.

[rsc, 2011-11-17 21:54:53]

Thanks.  I'm much more comfortable with this change now.
I'd like Adam to look at the crypto.

Russ

[agl1, 2011-11-17 22:10:59]

On Thu, Nov 17, 2011 at 2:09 PM, Russ Cox <rsc@golang.org> wrote:
> Thanks.  I'm much more comfortable with this change now.
> I'd like Adam to look at the crypto.

The codereview site is read-only for maintenance at the moment, but
I'll reply when it's back.


Cheers

AGL

[agl1, 2011-11-18 16:10:43]

LGTM with these small changes.

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher.go
File src/pkg/exp/ssh/cipher.go (right):

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:47: panic(fmt.Sprintf("cipher requires %d bytes for
key but got %d bytes", c.keySize, len(key)))
panic("ssh: key length too small for cipher")

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:50: panic(fmt.Sprintf("cipher requires %d bytes for IV
but got %d bytes", c.ivSize, len(iv)))
panic("ssh: iv too small for cipher")

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher_test.go
File src/pkg/exp/ssh/cipher_test.go (right):

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher_test....
src/pkg/exp/ssh/cipher_test.go:22: t.Logf("Test cipher %q...", name)
We don't generally use t.Logf for context. (`grep t.Logf -r .` in src/pkg to get
a feel for it.)

Which means...

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher_test....
src/pkg/exp/ssh/cipher_test.go:26: t.Error("  creating encryptor: ",
err.Error())
That the Error calls should be something like:

t.Errorf("failed to create encryptor for %q: %s", name, err)

[r, 2011-11-18 16:25:59]

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher_test.go
File src/pkg/exp/ssh/cipher_test.go (right):

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher_test....
src/pkg/exp/ssh/cipher_test.go:24: encryptor, err :=
cipherMode.createCipher(testKey, testIv)
encrypter not encryptor

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher_test....
src/pkg/exp/ssh/cipher_test.go:29: decryptor, err :=
cipherMode.createCipher(testKey, testIv)
decrypter not decryptor

[huin-google, 2011-11-18 16:37:00]

Hello dave@cheney.net, agl@golang.org, taruti@taruti.net, rsc@golang.org,
r@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[huin-google, 2011-11-18 16:37:31]

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher.go
File src/pkg/exp/ssh/cipher.go (right):

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:47: panic(fmt.Sprintf("cipher requires %d bytes for
key but got %d bytes", c.keySize, len(key)))
On 2011/11/18 16:10:43, agl1 wrote:
> panic("ssh: key length too small for cipher")

Done.

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher.go#ne...
src/pkg/exp/ssh/cipher.go:50: panic(fmt.Sprintf("cipher requires %d bytes for IV
but got %d bytes", c.ivSize, len(iv)))
On 2011/11/18 16:10:43, agl1 wrote:
> panic("ssh: iv too small for cipher")

Done.

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher_test.go
File src/pkg/exp/ssh/cipher_test.go (right):

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher_test....
src/pkg/exp/ssh/cipher_test.go:22: t.Logf("Test cipher %q...", name)
On 2011/11/18 16:10:43, agl1 wrote:
> We don't generally use t.Logf for context. (`grep t.Logf -r .` in src/pkg to
get
> a feel for it.)
> 
> Which means...

Done.

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher_test....
src/pkg/exp/ssh/cipher_test.go:24: encryptor, err :=
cipherMode.createCipher(testKey, testIv)
On 2011/11/18 16:25:59, r wrote:
> encrypter not encryptor

Done.

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher_test....
src/pkg/exp/ssh/cipher_test.go:26: t.Error("  creating encryptor: ",
err.Error())
On 2011/11/18 16:10:43, agl1 wrote:
> That the Error calls should be something like:
> 
> t.Errorf("failed to create encryptor for %q: %s", name, err)

Done.

http://codereview.appspot.com/5342057/diff/18001/src/pkg/exp/ssh/cipher_test....
src/pkg/exp/ssh/cipher_test.go:29: decryptor, err :=
cipherMode.createCipher(testKey, testIv)
On 2011/11/18 16:25:59, r wrote:
> decrypter not decryptor

Done.

[agl1, 2011-11-18 17:57:03]

*** Submitted as http://code.google.com/p/go/source/detail?r=76e734d187df ***

exp/ssh: Add support for (most) of the ciphers from RFC4253, RFC4344 and
RFC4345.

R=dave, agl, taruti, rsc, r
CC=golang-dev
http://codereview.appspot.com/5342057

Committer: Adam Langley <agl@golang.org>