code review 161700043: crypto/tls: don't echo NPN/ALPN or tickets with SSLv3.

crypto/tls: don't echo NPN/ALPN or tickets with SSLv3.

NSS doesn't like connecting to a Go server with MaxVersion of SSLv3
because it doesn't expect SSLv3 servers to implement these extensions.
This isn't terribly important because usually when using NSS with SSLv3
it's because of protocol fallback and the client won't advertise those
extensions in the first place.

(No tests updated because tests are done against OpenSSL and this is
the result of an NSS behaviour.)

[agl1, 2014-10-28 22:30:23]

Hello bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),

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

[agl1, 2014-10-28 22:30:41]

This is probably /not/ needed for Go 1.4. But I'll lose the change unless I
upload it.

[bradfitz, 2014-10-28 22:32:12]

LGTM

To Russ for 1.4-worthiness.


On Tue, Oct 28, 2014 at 3:30 PM, <agl@golang.org> wrote:

> Reviewers: bradfitz,
>
> Message:
> Hello bradfitz@golang.org (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> crypto/tls: don't echo NPN/ALPN or tickets with SSLv3.
>
> NSS doesn't like connecting to a Go server with MaxVersion of SSLv3
> because it doesn't expect SSLv3 servers to implement these extensions.
> This isn't terribly important because usually when using NSS with SSLv3
> it's because of protocol fallback and the client won't advertise those
> extensions in the first place.
>
> (No tests updated because tests are done against OpenSSL and this is
> the result of an NSS behaviour.)
>
> Please review this at https://codereview.appspot.com/161700043/
>
> Affected files (+2, -2 lines):
>   M src/crypto/tls/handshake_server.go
>
>
> Index: src/crypto/tls/handshake_server.go
> ===================================================================
> --- a/src/crypto/tls/handshake_server.go
> +++ b/src/crypto/tls/handshake_server.go
> @@ -174,7 +174,7 @@
>                 // had a bug around this. Best to send nothing at all if
>                 // config.NextProtos is empty. See
>                 // https://code.google.com/p/go/issues/detail?id=5445.
> -               if hs.clientHello.nextProtoNeg && len(config.NextProtos) >
> 0 {
> +               if hs.clientHello.nextProtoNeg && len(config.NextProtos) >
> 0 && c.vers
>
>> = VersionTLS10 {
>>
>                         hs.hello.nextProtoNeg = true
>                         hs.hello.nextProtos = config.NextProtos
>                 }
> @@ -318,7 +318,7 @@
>                 hs.hello.ocspStapling = true
>         }
>
> -       hs.hello.ticketSupported = hs.clientHello.ticketSupported &&
> !config.SessionTicketsDisabled
> +       hs.hello.ticketSupported = hs.clientHello.ticketSupported &&
> !config.SessionTicketsDisabled && c.vers >= VersionTLS10
>         hs.hello.cipherSuite = hs.suite.id
>         hs.finishedHash.Write(hs.hello.marshal())
>         c.writeRecord(recordTypeHandshake, hs.hello.marshal())
>
>
>

[rsc, 2014-10-29 01:38:06]

LGTM

filed issue 9014 to remind us to submit this at the beginning of 1.5

[gobot, 2014-12-19 05:14:43]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/161700043 is best) in the description in your
new CL.

Thanks very much.