code review 2747042: crypto/tls: changes to support adding more cipher suites

crypto/tls: changes to support adding more cipher suites

[ality, 2010-10-27 08:45:04]

Hello agl, rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[ality, 2010-10-27 08:50:47]

Hello agl, rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[ality, 2010-10-27 09:03:03]

The changes to generate_cert.go snuck in by accident. One
makes it use crypto/rand instead of opening /dev/urandom
itself and the other simply makes it compile.

The changes aren't necessary for the rest of the patch set.

[ality, 2010-10-27 09:22:50]

Also, this is just the first step in supporting
more cipher suites. I'm almost done with the code
to actually *use* the other ciphers/HMACs.

I wanted to get some feedback to see if I'm on the
right track.

Thanks.

[ality, 2010-10-27 14:05:08]

Hello agl, rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[agl, 2010-10-27 14:23:09]

Note: I intend to do significant damage to crypto/block and I guess this is as
good a time as ever to get moving on that. Firstly, the order of src, dst in
Encrypt is backwards and needs to be swapped.

I don't see that it needs to block this CL, but you might want to be aware.

http://codereview.appspot.com/2747042/diff/11001/src/pkg/crypto/tls/cipher_su...
File src/pkg/crypto/tls/cipher_suites.go (right):

http://codereview.appspot.com/2747042/diff/11001/src/pkg/crypto/tls/cipher_su...
src/pkg/crypto/tls/cipher_suites.go:10: cblock "crypto/block"
I would just omit 'cblock'. It's even a longer name!

http://codereview.appspot.com/2747042/diff/11001/src/pkg/crypto/tls/cipher_su...
src/pkg/crypto/tls/cipher_suites.go:19: id              uint16 // The number of
this suite on the wire.
I would just say 'cipher suite number'. 'On the wire' suggests endianness issues
(although I'm probably guilty of using that phrase.)

http://codereview.appspot.com/2747042/diff/11001/src/pkg/crypto/tls/cipher_su...
src/pkg/crypto/tls/cipher_suites.go:43: var iv []byte // TODO(ality): Securely
generate an IV
The IV comes from the master secret key material.

http://codereview.appspot.com/2747042/diff/11001/src/pkg/crypto/tls/cipher_su...
src/pkg/crypto/tls/cipher_suites.go:71: func supportedCipherSuite(id uint16)
(suite *cipherSuite, ok bool) {
it would seem that this should be called 'getCipherSuite' and that it can return
a single pointer. Failure can be indicated via a nil return.

http://codereview.appspot.com/2747042/diff/11001/src/pkg/crypto/tls/cipher_su...
src/pkg/crypto/tls/cipher_suites.go:75: ok = true
break

http://codereview.appspot.com/2747042/diff/11001/src/pkg/crypto/tls/cipher_su...
src/pkg/crypto/tls/cipher_suites.go:81: func (s *cipherSuite) Setup(key, mac
[]byte) (c crypter, h hash.Hash) {
need to pass the IV in here also

http://codereview.appspot.com/2747042/diff/11001/src/pkg/crypto/tls/cipher_su...
src/pkg/crypto/tls/cipher_suites.go:103: const (
You should have a comment referencing
http://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-param...

http://codereview.appspot.com/2747042/diff/11001/src/pkg/crypto/tls/common.go
File src/pkg/crypto/tls/common.go (right):

http://codereview.appspot.com/2747042/diff/11001/src/pkg/crypto/tls/common.go...
src/pkg/crypto/tls/common.go:103: // CipherSuites is a list of supported cipher
specifications.
s/specifications/suites/

http://codereview.appspot.com/2747042/diff/11001/src/pkg/crypto/tls/generate_...
File src/pkg/crypto/tls/generate_cert.go (right):

http://codereview.appspot.com/2747042/diff/11001/src/pkg/crypto/tls/generate_...
src/pkg/crypto/tls/generate_cert.go:12: "crypto/rand"
Please remove this from this CL and submit as a separate CL.

[ality, 2010-10-28 22:53:04]

Do you have plans to add a stream.Cipher interface?

I'm currently reworking conn.go to support both
stream and block cipher modes (with an eye towards
a future AEAD mode).

http://codereview.appspot.com/2747042/diff/11001/src/pkg/crypto/tls/cipher_su...
File src/pkg/crypto/tls/cipher_suites.go (right):

http://codereview.appspot.com/2747042/diff/11001/src/pkg/crypto/tls/cipher_su...
src/pkg/crypto/tls/cipher_suites.go:10: cblock "crypto/block"
On 2010/10/27 14:23:09, agl wrote:
> I would just omit 'cblock'. It's even a longer name!

This was less intrusive than changing conn.go
to use a different name for its block type.

It's your call.

http://codereview.appspot.com/2747042/diff/11001/src/pkg/crypto/tls/cipher_su...
src/pkg/crypto/tls/cipher_suites.go:43: var iv []byte // TODO(ality): Securely
generate an IV
On 2010/10/27 14:23:09, agl wrote:
> The IV comes from the master secret key material.

Is this true? I thought it was only required to
be a cryptographically secture random number.

[ality, 2010-10-31 11:15:47]

Hello agl, rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[ality, 2010-10-31 11:20:49]

Hello agl, rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[ality, 2010-10-31 15:31:56]

Hello agl, rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc1, 2010-11-01 19:33:14]

looks reasonable to me.
deferring to agl

http://codereview.appspot.com/2747042/diff/28001/src/pkg/crypto/tls/cipher_su...
File src/pkg/crypto/tls/cipher_suites.go (right):

http://codereview.appspot.com/2747042/diff/28001/src/pkg/crypto/tls/cipher_su...
src/pkg/crypto/tls/cipher_suites.go:26: hmacNull = iota
are the enums necessary?
can you use func variables in the table instead,
so that instead of hmacSHA1 you'd use hmac.NewSHA1?

[agl, 2010-11-01 20:05:47]

On vacation. Back tomorrow.
On Nov 1, 2010 12:34 PM, <rsc@google.com> wrote:
> looks reasonable to me.
> deferring to agl
>
>
>
>
http://codereview.appspot.com/2747042/diff/28001/src/pkg/crypto/tls/cipher_su...
> File src/pkg/crypto/tls/cipher_suites.go (right):
>
>
http://codereview.appspot.com/2747042/diff/28001/src/pkg/crypto/tls/cipher_su...
> src/pkg/crypto/tls/cipher_suites.go:26: hmacNull = iota
> are the enums necessary?
> can you use func variables in the table instead,
> so that instead of hmacSHA1 you'd use hmac.NewSHA1?
>
> http://codereview.appspot.com/2747042/

[agl, 2010-11-04 16:25:46]

On Mon, Nov 1, 2010 at 4:05 PM, Adam Langley <agl@chromium.org> wrote:
> On vacation. Back tomorrow.

I'm working on a crypto/cipher package to replace crypto/block. Let me
get it out this weekend and then we can get it into shape for TLS.


AGL

[ality, 2010-12-02 12:37:09]

Adam Langley <agl@chromium.org> once said:
> On Mon, Nov 1, 2010 at 4:05 PM, Adam Langley <agl@chromium.org> wrote:
> > On vacation. Back tomorrow.
> 
> I'm working on a crypto/cipher package to replace crypto/block. Let me
> get it out this weekend and then we can get it into shape for TLS.

Ping.

[rsc, 2010-12-07 17:50:13]

mostly leaving for agl but you did not
reply to my last set of comments

http://codereview.appspot.com/2747042/diff/28001/src/pkg/crypto/tls/cipher_su...
File src/pkg/crypto/tls/cipher_suites.go (right):

http://codereview.appspot.com/2747042/diff/28001/src/pkg/crypto/tls/cipher_su...
src/pkg/crypto/tls/cipher_suites.go:68: type blockCipher interface {
why is this here instead of importing crypto/block?

[agl1, 2010-12-07 18:07:46]

On Tue, Dec 7, 2010 at 12:50 PM,  <rsc@golang.org> wrote:
> mostly leaving for agl but you did not
> reply to my last set of comments

Yes, I need to do work on this. I'm afraid that it's behind a lot of
other things at the moment.


AGL

[ality, 2010-12-07 19:44:26]

rsc:
Sorry. I didn't notice your previous comments.

agl:
Could you say a bit about your plans for the
tls package? I'm working on some code that
relies on this CL and other changes I've yet
to submit for review (i.e. support for DH
w/ephemeral keys and a des package). If you
don't have the time, I'd be willing to
volunteer.

http://codereview.appspot.com/2747042/diff/28001/src/pkg/crypto/tls/cipher_su...
File src/pkg/crypto/tls/cipher_suites.go (right):

http://codereview.appspot.com/2747042/diff/28001/src/pkg/crypto/tls/cipher_su...
src/pkg/crypto/tls/cipher_suites.go:26: hmacNull = iota
On 2010/11/01 19:33:14, rsc1 wrote:
> are the enums necessary?
> can you use func variables in the table instead,
> so that instead of hmacSHA1 you'd use hmac.NewSHA1?

Yes, I can use funcs for the hmac algorithms
but not for the ciphers since they can return
either a stream or block interface.

http://codereview.appspot.com/2747042/diff/28001/src/pkg/crypto/tls/cipher_su...
src/pkg/crypto/tls/cipher_suites.go:68: type blockCipher interface {
On 2010/12/07 17:50:13, rsc wrote:
> why is this here instead of importing crypto/block?

I put it here so the code that checks the
interface type in conn.go would look
symmetrical. Not a solid reason, I admit.

It should probably just use the interfaces
in agl's new crypto/cipher package.

[agl, 2010-12-15 17:47:27]

On Tue, Dec 7, 2010 at 1:07 PM, Adam Langley <agl@golang.org> wrote:
> Yes, I need to do work on this. I'm afraid that it's behind a lot of
> other things at the moment.

Support submitted as:

https://code.google.com/p/go/source/detail?r=062a54b8130444a14a2f70c771a410c0...


AGL

[ality, 2010-12-15 18:07:27]

*** Abandoned ***

[agl, 2010-12-15 18:08:36]

On Wed, Dec 15, 2010 at 1:07 PM,  <ality@pbrane.org> wrote:
> *** Abandoned ***
>
> http://codereview.appspot.com/2747042/

p.s. if anything sucks for you with the current code, please let me know!


AGL

[ality, 2010-12-15 18:20:06]

Adam Langley <agl@chromium.org> once said:
> p.s. if anything sucks for you with the current
> code, please let me know!

I'm going through it right now. Thanks for doing
this.

  Anthony