code review 67010043: crypto/tls: enforce that either ServerName or InsecureS...

crypto/tls: enforce that either ServerName or InsecureSkipVerify be given.

crypto/tls has two functions for creating a client connection: Dial,
which most users are expected to use, and Client, which is the
lower-level API.

Dial does what you expect: it gives you a secure connection to the host
that you specify and the majority of users of crypto/tls appear to work
fine with it.

Client gives more control but needs more care. Specifically, if it
wasn't given a server name in the tls.Config then it didn't check that
the server's certificates match any hostname - because it doesn't have
one to check against. It was assumed that users of the low-level API
call VerifyHostname on the certificate themselves if they didn't supply
a hostname.

A review of the uses of Client both within Google and in a couple of
external libraries has shown that nearly all of them got this wrong.

Thus, this change enforces that either a ServerName or
InsecureSkipVerify is given. This does not affect tls.Dial.

See discussion at https://groups.google.com/d/msg/golang-nuts/4vnt7NdLvVU/b1SJ4u0ikb0J.

Fixes issue 7342.

[agl1, 2014-02-21 16:24:43]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-02-21 18:14:03]

LGTM but update doc/go1.3.txt



On Fri, Feb 21, 2014 at 8:24 AM, <agl@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> crypto/tls: enforce that either ServerName or InsecureSkipVerify be
> given.
>
> crypto/tls has two functions for creating a client connection: Dial,
> which most users are expected to use, and Client, which is the
> lower-level API.
>
> Dial does what you expect: it gives you a secure connection to the host
> that you specify and the majority of users of crypto/tls appear to work
> fine with it.
>
> Client gives more control but needs more care. Specifically, if it
> wasn't given a server name in the tls.Config then it didn't check that
> the server's certificates match any hostname - because it doesn't have
> one to check against. It was assumed that users of the low-level API
> call VerifyHostname on the certificate themselves if they didn't supply
> a hostname.
>
> A review of the uses of Client both within Google and in a couple of
> external libraries has shown that nearly all of them got this wrong.
>
> Thus, this change enforces that either a ServerName or
> InsecureSkipVerify is given. This does not affect tls.Dial.
>
> See discussion at
> https://groups.google.com/d/msg/golang-nuts/4vnt7NdLvVU/b1SJ4u0ikb0J.
>
> Fixes issue 7342.
>
> Please review this at https://codereview.appspot.com/67010043/
>
> Affected files (+4, -0 lines):
>   M src/pkg/crypto/tls/handshake_client.go
>
>
> Index: src/pkg/crypto/tls/handshake_client.go
> ===================================================================
> --- a/src/pkg/crypto/tls/handshake_client.go
> +++ b/src/pkg/crypto/tls/handshake_client.go
> @@ -33,6 +33,10 @@
>                 c.config = defaultConfig()
>         }
>
> +       if len(c.config.ServerName) == 0 && !c.config.InsecureSkipVerify {
> +               return errors.New("tls: either ServerName or
> InsecureSkipVerify must be specified in the tls.Config")
> +       }
> +
>         hello := &clientHelloMsg{
>                 vers:                c.config.maxVersion(),
>                 compressionMethods:  []uint8{compressionNone},
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[agl1, 2014-02-21 20:56:21]

On Fri, Feb 21, 2014 at 1:14 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> LGTM but update doc/go1.3.txt

Done.

[agl1, 2014-02-21 20:56:50]

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

crypto/tls: enforce that either ServerName or InsecureSkipVerify be given.

crypto/tls has two functions for creating a client connection: Dial,
which most users are expected to use, and Client, which is the
lower-level API.

Dial does what you expect: it gives you a secure connection to the host
that you specify and the majority of users of crypto/tls appear to work
fine with it.

Client gives more control but needs more care. Specifically, if it
wasn't given a server name in the tls.Config then it didn't check that
the server's certificates match any hostname - because it doesn't have
one to check against. It was assumed that users of the low-level API
call VerifyHostname on the certificate themselves if they didn't supply
a hostname.

A review of the uses of Client both within Google and in a couple of
external libraries has shown that nearly all of them got this wrong.

Thus, this change enforces that either a ServerName or
InsecureSkipVerify is given. This does not affect tls.Dial.

See discussion at
https://groups.google.com/d/msg/golang-nuts/4vnt7NdLvVU/b1SJ4u0ikb0J.

Fixes issue 7342.

LGTM=bradfitz
R=golang-codereviews, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/67010043

[gobot, 2014-02-21 21:14:47]

This CL appears to have broken the darwin-386-cheney builder.

[josharian, 2014-02-21 21:46:40]

That's https://code.google.com/p/go/issues/detail?id=7370.

On Fri, Feb 21, 2014 at 1:14 PM,  <gobot@golang.org> wrote:
> This CL appears to have broken the darwin-386-cheney builder.
>
>
> https://codereview.appspot.com/67010043/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.