code review 2119042: websocket: Add support for secure WebSockets

websocket: Add support for secure WebSockets

Fixes issue 842.
Fixes issue 1061.

[madari, 2010-09-02 17:52:19]

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

I'd like you to review this change.

[madari, 2010-09-03 06:18:42]

You could do the same thing without touching the http-package at all, but I
think the
Secure() method I added could be useful somewhere else too. If you think it's
not cool,
then one could check the type of the hijacked rwc directly in the server.go. I'm
really not sure about this...

On 2010/09/02 17:52:19, madari wrote:
> Hello mailto:golang-dev@googlegroups.com (cc:
mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change.

[adg1, 2010-09-07 12:44:46]

This is an active feature request. Please add "Fixes issue 1061." to
the CL description.

See: http://code.google.com/p/go/issues/detail?id=1061

On 3 September 2010 03:52,  <karatepekka@gmail.com> wrote:
> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> websocket: Add support for secure WebSockets
>
> Please review this at http://codereview.appspot.com/2119042/
>
> Affected files:
>  M src/pkg/http/server.go
>  M src/pkg/websocket/client.go
>  M src/pkg/websocket/server.go
>
>
> Index: src/pkg/http/server.go
> ===================================================================
> --- a/src/pkg/http/server.go
> +++ b/src/pkg/http/server.go
> @@ -63,6 +63,7 @@
>        header          map[string]string // reply header parameters
>        written         int64             // number of bytes written in body
>        status          int               // status code passed to
> WriteHeader
> +       secure          bool              // a flag indicating connection
> over TLS
>  }
>
>  // Create new connection from rwc.
> @@ -73,6 +74,7 @@
>        }
>        c.handler = handler
>        c.rwc = rwc
> +       _, c.secure = rwc.(*tls.Conn)
>        br := bufio.NewReader(rwc)
>        bw := bufio.NewWriter(rwc)
>        c.buf = bufio.NewReadWriter(br, bw)
> @@ -144,6 +146,12 @@
>        return req, nil
>  }
>
> +// Secure returns true if the connection is establised over
> +// TLS.
> +func (c *Conn) Secure() bool {
> +       return c.secure
> +}
> +
>  // SetHeader sets a header line in the eventual reply.
>  // For example, SetHeader("Content-Type", "text/html; charset=utf-8")
>  // will result in the header line
> Index: src/pkg/websocket/client.go
> ===================================================================
> --- a/src/pkg/websocket/client.go
> +++ b/src/pkg/websocket/client.go
> @@ -8,6 +8,7 @@
>        "bufio"
>        "bytes"
>        "container/vector"
> +       "crypto/tls"
>        "fmt"
>        "http"
>        "io"
> @@ -97,6 +98,19 @@
>        return newClient(parsedUrl.RawPath, parsedUrl.Host, origin, url,
> protocol, client, handshake)
>  }
>
> +// DialTLS opens a new secure WebSocket client connection
> +func DialTLS(url, protocol, origin string) (ws *Conn, err os.Error) {
> +       parsedUrl, err := http.ParseURL(url)
> +       if err != nil {
> +               return
> +       }
> +       client, err := tls.Dial("tcp", "", parsedUrl.Host)
> +       if err != nil {
> +               return
> +       }
> +       return newClient(parsedUrl.RawPath, parsedUrl.Host, origin, url,
> protocol, client, handshake)
> +}
> +
>  /*
>  Generates handshake key as described in 4.1 Opening handshake step 16 to
> 22.
>  cf. http://www.whatwg.org/specs/web-socket-protocol/
> Index: src/pkg/websocket/server.go
> ===================================================================
> --- a/src/pkg/websocket/server.go
> +++ b/src/pkg/websocket/server.go
> @@ -97,7 +97,12 @@
>                return
>        }
>
> -       location := "ws://" + req.Host + req.URL.RawPath
> +       var location string
> +       if c.Secure() {
> +               location = "wss://" + req.Host + req.URL.RawPath
> +       } else {
> +               location = "ws://" + req.Host + req.URL.RawPath
> +       }
>
>        // Step 4. get key number in Sec-WebSocket-Key<n> fields.
>        keyNumber1 := getKeyNumber(key1)
>
>
>

[rsc1, 2010-09-07 13:37:08]

Fixes issue 842.
Fixes issue 1061.

(842 is asking for server support, 1061 client support.)

[rsc1, 2010-09-07 13:45:26]

The fixes notes need to be two separate lines
so that the issue auto-closer will recognize them.

http://codereview.appspot.com/2119042/diff/6001/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/2119042/diff/6001/src/pkg/http/server.go#newcode66
src/pkg/http/server.go:66: secure          bool              // a flag
indicating connection over TLS
usingTLS

http://codereview.appspot.com/2119042/diff/6001/src/pkg/http/server.go#newcode77
src/pkg/http/server.go:77: _, c.secure = rwc.(*tls.Conn)
This information should be passed in, not inferred.
There might be an alternate tls implementation some day.

http://codereview.appspot.com/2119042/diff/6001/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:149: // Secure returns true if the connection is
establised over
Secure has many meanings to many people.
Better to be precise about what's being checked.

// UsingTLS returns true if the connection uses transport layer security (TLS).
func (c *Conn) UsingTLS() bool {

http://codereview.appspot.com/2119042/diff/6001/src/pkg/websocket/client.go
File src/pkg/websocket/client.go (right):

http://codereview.appspot.com/2119042/diff/6001/src/pkg/websocket/client.go#n...
src/pkg/websocket/client.go:101: // DialTLS opens a new secure WebSocket client
connection
Missing period at end of sentence.

Not clear to me that this belongs here anyway.
Dial already takes a URL, and I believe that wss:// is the
standard prefix for TLS-based websockets.
Dial should already be checking that parsedUrl.Scheme
is "ws" (it does not; please add that), and then it can 
use tls.Dial when parsedUrl.Scheme is "wss".

[madari, 2010-09-07 15:53:52]

Please have another look:

On 2010/09/07 13:45:26, rsc1 wrote:
> The fixes notes need to be two separate lines
> so that the issue auto-closer will recognize them.

Fixed, thanks.

>
http://codereview.appspot.com/2119042/diff/6001/src/pkg/http/server.go#newcode66
> src/pkg/http/server.go:66: secure          bool              // a flag
> indicating connection over TLS
> usingTLS

Fixed.

>
http://codereview.appspot.com/2119042/diff/6001/src/pkg/http/server.go#newcode77
> src/pkg/http/server.go:77: _, c.secure = rwc.(*tls.Conn)
> This information should be passed in, not inferred.
> There might be an alternate tls implementation some day.

The field name was changed to usingTLS - can we now infer the value? If we
really want
to pass the information in, this would mean a signature change on the exported
"Serve"-function.

>
http://codereview.appspot.com/2119042/diff/6001/src/pkg/http/server.go#newcod...
> src/pkg/http/server.go:149: // Secure returns true if the connection is
> establised over
> Secure has many meanings to many people.
> Better to be precise about what's being checked.
> 
> // UsingTLS returns true if the connection uses transport layer security
(TLS).
> func (c *Conn) UsingTLS() bool {

Fixed.

>
http://codereview.appspot.com/2119042/diff/6001/src/pkg/websocket/client.go#n...
> src/pkg/websocket/client.go:101: // DialTLS opens a new secure WebSocket
client
> connection
> Missing period at end of sentence.
> 
> Not clear to me that this belongs here anyway.
> Dial already takes a URL, and I believe that wss:// is the
> standard prefix for TLS-based websockets.
> Dial should already be checking that parsedUrl.Scheme
> is "ws" (it does not; please add that), and then it can 
> use tls.Dial when parsedUrl.Scheme is "wss".

Fixed. I used a generic os.ErrorString-type for the bad scheme error. Would it
be better to introduce a new
error type, say websocket.URIError - or is this ok?

[rsc1, 2010-09-08 14:31:01]

looking pretty good.  let's make the errors a little more useful.

http://codereview.appspot.com/2119042/diff/14001/src/pkg/websocket/client.go
File src/pkg/websocket/client.go (right):

http://codereview.appspot.com/2119042/diff/14001/src/pkg/websocket/client.go#...
src/pkg/websocket/client.go:35: 
please add


type DialError struct {
    URL string
    Protocol string
    Origin string
    Error os.Error
}

func (e *DialError) String() {
    return "websocket.Dial " + e.URL + ": " + e.Error.String()
}

http://codereview.appspot.com/2119042/diff/14001/src/pkg/websocket/client.go#...
src/pkg/websocket/client.go:95: return
goto Error

http://codereview.appspot.com/2119042/diff/14001/src/pkg/websocket/client.go#...
src/pkg/websocket/client.go:109: return
goto Error

http://codereview.appspot.com/2119042/diff/14001/src/pkg/websocket/client.go#...
src/pkg/websocket/client.go:112: return newClient(parsedUrl.RawPath,
parsedUrl.Host, origin, url, protocol, client, handshake)
    ws, err = newClient...
    if err != nil {
        goto Error
    }
    return ws

Error:
    return &DialError{url, protocol, origin, err}

this gives the errors useful context
that they're missing, both for bad scheme
and the other errors.

[madari, 2010-09-08 16:01:46]

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

Please take another look.

[rsc, 2010-09-11 04:27:19]

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

websocket: Add support for secure WebSockets

Fixes issue 842.
Fixes issue 1061.

R=rsc
CC=golang-dev
http://codereview.appspot.com/2119042

Committer: Russ Cox <rsc@golang.org>