code review 9539045: crypto/tls: don't send NPN extension if NextProtos is n...

crypto/tls: don't send NPN extension if NextProtos is not set.

This isn't clearly a bug on Go's part, but it triggers a bug in Firefox
which means that crypto/tls and net/http cannot be wired up together
unless NextProtos includes "http/1.1". When net/http sets up the
tls.Config, it does this and so works fine. But anyone setting up the
tls.Config themselves will hit the Firefox bug.

Fixes issue 5445.

[agl1, 2013-05-20 18:26:24]

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

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2013-05-20 18:35:39]

LGTM



On Mon, May 20, 2013 at 11:26 AM, <agl@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> crypto/tls: don't send NPN extension if NextProtos is not set.
>
> This isn't clearly a bug on Go's part, but it triggers a bug in Firefox
> which means that crypto/tls and net/http cannot be wired up together
> unless NextProtos includes "http/1.1". When net/http sets up the
> tls.Config, it does this and so works fine. But anyone setting up the
> tls.Config themselves will hit the Firefox bug.
>
> Fixes issue 5445.
>
> Please review this at
https://codereview.appspot.**com/9539045/<https://codereview.appspot.com/9539...
>
> Affected files:
>   M src/pkg/crypto/tls/handshake_**server.go
>
>
> Index: src/pkg/crypto/tls/handshake_**server.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/tls/**handshake_server.go
> +++ b/src/pkg/crypto/tls/**handshake_server.go
> @@ -156,7 +156,7 @@
>         if len(hs.clientHello.serverName) > 0 {
>                 c.serverName = hs.clientHello.serverName
>         }
> -       if hs.clientHello.nextProtoNeg {
> +       if hs.clientHello.nextProtoNeg && len(config.NextProtos) > 0 {
>                 hs.hello.nextProtoNeg = true
>                 hs.hello.nextProtos = config.NextProtos
>         }
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[r, 2013-05-20 18:42:23]

https://codereview.appspot.com/9539045/diff/5001/src/pkg/crypto/tls/handshake...
File src/pkg/crypto/tls/handshake_server.go (right):

https://codereview.appspot.com/9539045/diff/5001/src/pkg/crypto/tls/handshake...
src/pkg/crypto/tls/handshake_server.go:159: if hs.clientHello.nextProtoNeg &&
len(config.NextProtos) > 0 {
does this deserve a comment?

[agl1, 2013-05-20 21:12:02]

https://codereview.appspot.com/9539045/diff/5001/src/pkg/crypto/tls/handshake...
File src/pkg/crypto/tls/handshake_server.go (right):

https://codereview.appspot.com/9539045/diff/5001/src/pkg/crypto/tls/handshake...
src/pkg/crypto/tls/handshake_server.go:159: if hs.clientHello.nextProtoNeg &&
len(config.NextProtos) > 0 {
On 2013/05/20 18:42:24, r wrote:
> does this deserve a comment?

Done.

[r, 2013-05-20 21:32:49]

LGTM

[agl1, 2013-05-21 14:47:39]

*** Submitted as https://code.google.com/p/go/source/detail?r=08896028782c ***

crypto/tls: don't send NPN extension if NextProtos is not set.

This isn't clearly a bug on Go's part, but it triggers a bug in Firefox
which means that crypto/tls and net/http cannot be wired up together
unless NextProtos includes "http/1.1". When net/http sets up the
tls.Config, it does this and so works fine. But anyone setting up the
tls.Config themselves will hit the Firefox bug.

Fixes issue 5445.

R=golang-dev, bradfitz, r
CC=golang-dev
https://codereview.appspot.com/9539045