code review 943043: crypto/tls: simpler implementation of record layer

crypto/tls: simpler implementation of record layer

Depends on CL 957045, 980043, 1004043.
Fixes issue 715.

[rsc, 2010-04-25 01:08:53]

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

I'd like you to review this change.

[rsc, 2010-04-26 02:07:16]

Changed to R=agl1.
Added handshake_server_test.go to the CL.

I regenerated the transcript and to do so had
to use a new key and certificate.  The one pair
does not seem to work with gnutls or openssl:
I believe that the Go server sends either a
bad MAC alert once the cipher spec changes.
This may be a bug in any number of packages:
big, crypto/rc4, crypto/rsa, crypto/sha1, crypto/tls.
I suspect big, but I am leaving the debugging for
a future CL.

[rsc, 2010-04-26 05:18:01]

Ready for review.

Found and fixed the bug that was keeping the old key+cert
from working: I didn't realize that the unmarshallers kept
references to the data passed to them, and the new record
layer code was doing too good a job of reusing buffers.
The new key+cert was bigger and must have triggered a
reallocation between the clientHello and what was overwriting it.
Fixed by passing in a copy to unmarshal.

Still using the new key+cert because the new transcript
is based on it.

Russ

[agl, 2010-04-26 15:07:49]

LGTM

http://codereview.appspot.com/943043/diff/10001/11002
File src/pkg/crypto/tls/alert.go (right):

http://codereview.appspot.com/943043/diff/10001/11002#newcode72
src/pkg/crypto/tls/alert.go:72: return "alert" + strconv.Itoa(int(e))
maybe "alert(" + ... + ")", or "alert " or "alert#"?

http://codereview.appspot.com/943043/diff/10001/11004
File src/pkg/crypto/tls/conn.go (right):

http://codereview.appspot.com/943043/diff/10001/11004#newcode382
src/pkg/crypto/tls/conn.go:382: // RFC 4346 says we should just ignore the
message?
In general, no. The only way that either side should send anything that the
other side doesn't expect is if it has been negotiated with an extension.

http://codereview.appspot.com/943043/diff/10001/11004#newcode426
src/pkg/crypto/tls/conn.go:426: return c.sendAlertLocked(alertNoRenegotiation)
alertNoRenegotiation is always a warning level alert. Also, I must admit that
I'm missing why this is sendAlertLocked not sendAlert.

http://codereview.appspot.com/943043/diff/10001/11004#newcode490
src/pkg/crypto/tls/conn.go:490: c.tmp[0] = alertLevelError
Can sendAlertLocked not be called here?

http://codereview.appspot.com/943043/diff/10001/11005
File src/pkg/crypto/tls/handshake_client.go (right):

http://codereview.appspot.com/943043/diff/10001/11005#newcode32
src/pkg/crypto/tls/handshake_client.go:32: t := uint32(time.Seconds())
(aside: having the time function in the config was to aid testing.)

http://codereview.appspot.com/943043/diff/10001/11005#newcode37
src/pkg/crypto/tls/handshake_client.go:37: _, err := io.ReadFull(rand.Reader,
hello.random[4:])
(Likewise with the random source.)

[rsc1, 2010-04-26 17:21:07]

Thanks for the review.

http://codereview.appspot.com/943043/diff/10001/11002
File src/pkg/crypto/tls/alert.go (right):

http://codereview.appspot.com/943043/diff/10001/11002#newcode72
src/pkg/crypto/tls/alert.go:72: return "alert" + strconv.Itoa(int(e))
On 2010/04/26 15:07:50, agl wrote:
> maybe "alert(" + ... + ")", or "alert " or "alert#"?

Done.

http://codereview.appspot.com/943043/diff/10001/11004
File src/pkg/crypto/tls/conn.go (right):

http://codereview.appspot.com/943043/diff/10001/11004#newcode382
src/pkg/crypto/tls/conn.go:382: // RFC 4346 says we should just ignore the
message?
On 2010/04/26 15:07:50, agl wrote:
> In general, no. The only way that either side should send anything that the
> other side doesn't expect is if it has been negotiated with an extension.

Comment deleted (and I agree with you) but it really does say that:

pp.13-14:

   If a TLS implementation receives a record type it does not
   understand, it SHOULD just ignore it.  Any protocol designed for use
   over TLS MUST be carefully designed to deal with all possible attacks
   against it.  Note that because the type and length of a record are
   not protected by encryption, care SHOULD be taken to minimize the
   value of traffic analysis of these values.

http://codereview.appspot.com/943043/diff/10001/11004#newcode426
src/pkg/crypto/tls/conn.go:426: return c.sendAlertLocked(alertNoRenegotiation)
On 2010/04/26 15:07:50, agl wrote:
> alertNoRenegotiation is always a warning level alert.

Changed sendAlert to set the warning level for this one.

> Also, I must admit that
> I'm missing why this is sendAlertLocked not sendAlert.

Bug.  Fixed.

http://codereview.appspot.com/943043/diff/10001/11005
File src/pkg/crypto/tls/handshake_client.go (right):

http://codereview.appspot.com/943043/diff/10001/11005#newcode32
src/pkg/crypto/tls/handshake_client.go:32: t := uint32(time.Seconds())
On 2010/04/26 15:07:50, agl wrote:
> (aside: having the time function in the config was to aid testing.)

Sorry, dregs of debugging.
Fixed.

http://codereview.appspot.com/943043/diff/10001/11005#newcode37
src/pkg/crypto/tls/handshake_client.go:37: _, err := io.ReadFull(rand.Reader,
hello.random[4:])
On 2010/04/26 15:07:50, agl wrote:
> (Likewise with the random source.)

Likewise.

[rsc, 2010-04-27 05:19:08]

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

crypto/tls: simpler implementation of record layer

Depends on CL 957045, 980043, 1004043.
Fixes issue 715.

R=agl1, agl
CC=golang-dev
http://codereview.appspot.com/943043