Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrades to Go 1.7 and fixes vet finding and TLS behavior change. #2281

Merged
merged 2 commits into from Nov 8, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
@@ -1,7 +1,7 @@
language: go

go:
- 1.6.3
- 1.7.3

branches:
only:
Expand Down
4 changes: 2 additions & 2 deletions README.md
Expand Up @@ -42,7 +42,7 @@ https://www.consul.io/docs
## Developing Consul

If you wish to work on Consul itself, you'll first need [Go](https://golang.org)
installed (version 1.6+ is _required_). Make sure you have Go properly installed,
installed (version 1.7+ is _required_). Make sure you have Go properly installed,
including setting up your [GOPATH](https://golang.org/doc/code.html#GOPATH).

Next, clone this repository into `$GOPATH/src/github.com/hashicorp/consul` and
Expand All @@ -64,7 +64,7 @@ format the code according to Go standards.

### Building Consul on Windows

Make sure Go 1.6+ is installed on your system and that the Go command is in your
Make sure Go 1.7+ is installed on your system and that the Go command is in your
%PATH%.

For building Consul on Windows, you also need to have MinGW installed.
Expand Down
2 changes: 1 addition & 1 deletion scripts/consul-builder/Dockerfile
@@ -1,6 +1,6 @@
FROM ubuntu:trusty

ENV GOVERSION 1.6.3
ENV GOVERSION 1.7.3

RUN apt-get update -y && \
apt-get install --no-install-recommends -y -q \
Expand Down
37 changes: 35 additions & 2 deletions tlsutil/config.go
Expand Up @@ -143,6 +143,39 @@ func (c *Config) OutgoingTLSConfig() (*tls.Config, error) {
return tlsConfig, nil
}

// Clone returns a copy of c. Only the exported fields are copied. This
// was copied from https://golang.org/src/crypto/tls/common.go since that
// isn't exported and Go 1.7's vet uncovered an unsafe copy of a mutex in
// here.
//
// TODO (slackpad) - This can be removed once we move to Go 1.8, see
// https://github.com/golang/go/commit/d24f446 for details.
func clone(c *tls.Config) *tls.Config {
return &tls.Config{
Rand: c.Rand,
Time: c.Time,
Certificates: c.Certificates,
NameToCertificate: c.NameToCertificate,
GetCertificate: c.GetCertificate,
RootCAs: c.RootCAs,
NextProtos: c.NextProtos,
ServerName: c.ServerName,
ClientAuth: c.ClientAuth,
ClientCAs: c.ClientCAs,
InsecureSkipVerify: c.InsecureSkipVerify,
CipherSuites: c.CipherSuites,
PreferServerCipherSuites: c.PreferServerCipherSuites,
SessionTicketsDisabled: c.SessionTicketsDisabled,
SessionTicketKey: c.SessionTicketKey,
ClientSessionCache: c.ClientSessionCache,
MinVersion: c.MinVersion,
MaxVersion: c.MaxVersion,
CurvePreferences: c.CurvePreferences,
DynamicRecordSizingDisabled: c.DynamicRecordSizingDisabled,
Renegotiation: c.Renegotiation,
}
}

// OutgoingTLSWrapper returns a a DCWrapper based on the OutgoingTLS
// configuration. If hostname verification is on, the wrapper
// will properly generate the dynamic server name for verification.
Expand All @@ -164,9 +197,9 @@ func (c *Config) OutgoingTLSWrapper() (DCWrapper, error) {
// Generate the wrapper based on hostname verification
if c.VerifyServerHostname {
wrapper := func(dc string, conn net.Conn) (net.Conn, error) {
conf := *tlsConfig
conf := clone(tlsConfig)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was required to fix a go vet finding since this was copying the state of an unexported mutex.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slackpad You might want to add a comment that in go1.8 tls.Config.Clone() will be exported.

golang/go@d24f446

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@magiconair thanks for the heads up on that!

conf.ServerName = "server." + dc + "." + domain
return WrapTLSClient(conn, &conf)
return WrapTLSClient(conn, conf)
}
return wrapper, nil
} else {
Expand Down
7 changes: 3 additions & 4 deletions tlsutil/config_test.go
Expand Up @@ -207,6 +207,7 @@ func startTLSServer(config *Config) (net.Conn, chan error) {
errc <- err
}
close(errc)

// Because net.Pipe() is unbuffered, if both sides
// Close() simultaneously, we will deadlock as they
// both send an alert and then block. So we make the
Expand Down Expand Up @@ -276,12 +277,11 @@ func TestConfig_outgoingWrapper_BadDC(t *testing.T) {
if err != nil {
t.Fatalf("wrapTLS err: %v", err)
}
defer tlsClient.Close()
err = tlsClient.(*tls.Conn).Handshake()

if _, ok := err.(x509.HostnameError); !ok {
t.Fatalf("should get hostname err: %v", err)
}
tlsClient.Close()

<-errc
}
Expand Down Expand Up @@ -309,12 +309,11 @@ func TestConfig_outgoingWrapper_BadCert(t *testing.T) {
if err != nil {
t.Fatalf("wrapTLS err: %v", err)
}
defer tlsClient.Close()
err = tlsClient.(*tls.Conn).Handshake()

if _, ok := err.(x509.HostnameError); !ok {
t.Fatalf("should get hostname err: %v", err)
}
tlsClient.Close()

<-errc
}
Expand Down