code review 1743053: Websocket: fix authentication mistake caused by misuse ...

Websocket: fix authentication mistake caused by misuse of bytes.Buffer

byteBuffer := bytes.NewBuffer(buf) copies the contents of the buf into
the byteBuffer.  Subsequent writes will append to byteBuffer, not
overwrite data from buf.  The websocket code was trying to initialize
byteBuffer with the array from buf, not write buf into byteBuffer.

This mistake was not caught because both the server and client code
made the identical mistake so the tests did not catch it.  Also at the
time this code was committed, Chrome did not support draft-76 so there
was little external verification.

The latest Chrome does support draft-76 and this change will allow
websockets to work with the latest Chrome from the dev channel.
Without this change, the md5 sum returned by websocket/server.go will
be incorrect (according to the spec) and the browser will close the
websocket.

[tarm, 2010-08-01 20:53:56]

Hello golang-dev@googlegroups.com (cc: fumitoshi ukai <ukai@google.com>,
golang-dev@googlegroups.com),

I'd like you to review this change.

[ukai, 2010-08-02 04:14:31]

LGTM.
Thanks for fixing!

[rsc1, 2010-08-02 23:21:04]

Looks pretty good.  A few small things.  
Please fix and re-run hg mail.

Also, please complete a CLA as described at 
http://golang.org/doc/contribute.html#copyright .

Thanks.

http://codereview.appspot.com/1743053/diff/2001/3001
File src/pkg/websocket/client.go (right):

http://codereview.appspot.com/1743053/diff/2001/3001#newcode165
src/pkg/websocket/client.go:165: func getExpectedForChallenge(number1, number2
uint32, key3 []byte) (expected []byte, err os.Error) {
getChallengeResponse would be a clearer name.

While you're fixing things up, this code shortens to:

var buf [8]byte
binary.BigEndian.PutUint32(buf[0:], number1)
binary.BigEndian.PutUint32(buf[4:], number2)
h := md5.New()
h.Write(buf[0:])
h.Write(key3)
return h.Sum()

which avoids the reflection of binary.Write.

http://codereview.appspot.com/1743053/diff/2001/3002
File src/pkg/websocket/server.go (right):

http://codereview.appspot.com/1743053/diff/2001/3002#newcode126
src/pkg/websocket/server.go:126: challengeBuf := new(bytes.Buffer)
Can this code call getExpectedForChallenge (getChallengeResponse) instead?
Seems odd to have two copies of the code.

[tarm, 2010-08-03 04:04:58]

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

Please take another look.

[ukai, 2010-08-03 04:11:42]

http://codereview.appspot.com/1743053/diff/12001/13001
File src/pkg/websocket/client.go (right):

http://codereview.appspot.com/1743053/diff/12001/13001#newcode165
src/pkg/websocket/client.go:165: func getChallengeResponse(number1, number2
uint32, key3 []byte) (expected []byte, err os.Error) {
might be better to move this in websocket.go, because it is used in both client
and server.

[tarm, 2010-08-03 04:23:16]

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

Please take another look.

[ukai, 2010-08-03 04:24:40]

On 2010/08/03 04:23:16, tarm wrote:
> Hello ukai, rsc (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

need to add websocket.go in this change?

[tarm, 2010-08-03 04:26:35]

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

Please take another look.

[tarm, 2010-08-03 04:29:59]

Sorry, how do I do that?  I tried "hg add" and then "hg mail" but that
didn't seem to work.  I'm much more familiar with git and am a little
confused by the hg+codereview thing.

Thanks,
Tarmigan

On Mon, Aug 2, 2010 at 10:24 PM,  <ukai@google.com> wrote:
> On 2010/08/03 04:23:16, tarm wrote:
>>
>> Hello ukai, rsc (cc: mailto:golang-dev@googlegroups.com),
>
>> Please take another look.
>
> need to add websocket.go in this change?
>
>
> http://codereview.appspot.com/1743053/show
>

[rsc, 2010-08-03 05:28:40]

On Mon, Aug 2, 2010 at 21:29, Tarmigan Casebolt <tarmigan@gmail.com> wrote:
> Sorry, how do I do that?  I tried "hg add" and then "hg mail" but that
> didn't seem to work.  I'm much more familiar with git and am a little
> confused by the hg+codereview thing.

no worries.  after hg add you need to run hg change to
assign it to the CL you're working on.  so in this case

hg change 1743053

and then add it to the Files list

[tarm, 2010-08-03 05:51:49]

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

Please take another look.

[ukai, 2010-08-03 05:55:08]

LGTM
Thanks!

http://codereview.appspot.com/1743053/diff/28001/29003
File src/pkg/websocket/websocket.go (right):

http://codereview.appspot.com/1743053/diff/28001/29003#newcode13
src/pkg/websocket/websocket.go:13: "encoding/binary"
sort alphabetically?
bufio, crypto/md5, encoding/binary, ..

[tarm, 2010-08-03 06:05:39]

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

Please take another look.

[rsc1, 2010-08-03 20:00:31]

http://codereview.appspot.com/1743053/diff/33001/34003
File src/pkg/websocket/websocket.go (right):

http://codereview.appspot.com/1743053/diff/33001/34003#newcode141
src/pkg/websocket/websocket.go:141: // SeWritetTimeout sets the connection's
network write timeout in nanoseconds.
while you're here, please fix this typo: SetWriteTimeout

http://codereview.appspot.com/1743053/diff/33001/34003#newcode149
src/pkg/websocket/websocket.go:149: /*
one final nit.  should be in the same form as the other
doc comments: // and complete sentences.

// getChallengeResponse computes the expected response
// as described in section 4.1 steps 42 and 43 of 
// http://www.whatwg.org/specs/web-socket-protocol/.

[tarm, 2010-08-03 21:30:29]

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

Please take another look.

[rsc, 2010-08-03 21:33:47]

LGTM

thanks again

[rsc, 2010-08-03 21:34:47]

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

websocket: correct challenge response

Tested against latest Chrome.

R=ukai, rsc
CC=golang-dev
http://codereview.appspot.com/1743053

Committer: Russ Cox <rsc@golang.org>