code review 65440043: crypto/tls: improve documentation for ServerName.

crypto/tls: improve documentation for ServerName.

Users of the low-level, Client function are frequenctly missing the
fact that, unless they pass a ServerName to the TLS connection then it
cannot verify the certificates against any name.

This change makes it clear that at least one of InsecureSkipVerify and
ServerName should always be set.

[agl1, 2014-02-18 18:52:30]

Hello golang-codereviews@googlegroups.com,

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

[bradfitz, 2014-02-19 00:05:45]

LGTM



On Tue, Feb 18, 2014 at 10:52 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: improve documentation for ServerName.
>
> Users of the low-level, Client function are frequenctly missing the
> fact that, unless they pass a ServerName to the TLS connection then it
> cannot verify the certificates against any name.
>
> This change makes it clear that at least one of InsecureSkipVerify and
> ServerName should always be set.
>
> Please review this at https://codereview.appspot.com/65440043/
>
> Affected files (+5, -5 lines):
>   M src/pkg/crypto/tls/common.go
>   M src/pkg/crypto/tls/tls.go
>
>
> Index: src/pkg/crypto/tls/common.go
> ===================================================================
> --- a/src/pkg/crypto/tls/common.go
> +++ b/src/pkg/crypto/tls/common.go
> @@ -231,8 +231,9 @@
>         // NextProtos is a list of supported, application level protocols.
>         NextProtos []string
>
> -       // ServerName is included in the client's handshake to support
> virtual
> -       // hosting.
> +       // ServerName is used to verify the hostname on the returned
> +       // certificates unless InsecureSkipVerify is given. It is also
> included
> +       // in the client's handshake to support virtual hosting.
>         ServerName string
>
>         // ClientAuth determines the server's policy for
> Index: src/pkg/crypto/tls/tls.go
> ===================================================================
> --- a/src/pkg/crypto/tls/tls.go
> +++ b/src/pkg/crypto/tls/tls.go
> @@ -27,9 +27,8 @@
>
>  // Client returns a new TLS client side connection
>  // using conn as the underlying transport.
> -// Client interprets a nil configuration as equivalent to
> -// the zero configuration; see the documentation of Config
> -// for the defaults.
> +// The config cannot be nil: users must set either ServerHostname or
> +// InsecureSkipVerify in the config.
>  func Client(conn net.Conn, config *Config) *Conn {
>         return &Conn{conn: conn, config: config, isClient: true}
>  }
>
>
> --
> 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-19 16:17:31]

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

crypto/tls: improve documentation for ServerName.

Users of the low-level, Client function are frequenctly missing the
fact that, unless they pass a ServerName to the TLS connection then it
cannot verify the certificates against any name.

This change makes it clear that at least one of InsecureSkipVerify and
ServerName should always be set.

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

[gobot, 2014-02-19 18:10:05]

This CL appears to have broken the windows-386-ec2 builder.