Data race in UpdateCAs

[aaronlehmann]

Seen here: https://circleci.com/gh/docker/swarmkit/6414

==================
WARNING: DATA RACE
Read at 0x00c422a46d05 by goroutine 66:
  github.com/docker/swarmkit/vendor/github.com/docker/docker/pkg/tlsconfig.Clone()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/vendor/github.com/docker/docker/pkg/tlsconfig/tlsconfig_clone_go17.go:25 +0x51e
  github.com/docker/swarmkit/ca.(*MutableTLSCreds).UpdateCAs()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/ca/transport.go:143 +0x94
  github.com/docker/swarmkit/ca.(*SecurityConfig).UpdateRootCA()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/ca/config.go:146 +0x57c
  github.com/docker/swarmkit/ca.(*Server).UpdateRootCA()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/ca/server.go:551 +0x516
  github.com/docker/swarmkit/ca.(*Server).Run.func1()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/ca/server.go:395 +0x1d6
  github.com/docker/swarmkit/manager/state/store.ViewAndWatch.func1()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/manager/state/store/memory.go:717 +0xc0
  github.com/docker/swarmkit/manager/state/store.(*MemoryStore).update()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/manager/state/store/memory.go:235 +0x1c6
  github.com/docker/swarmkit/manager/state/store.(*MemoryStore).Update()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/manager/state/store/memory.go:277 +0x6a
  github.com/docker/swarmkit/manager/state/store.ViewAndWatch()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/manager/state/store/memory.go:722 +0x137
  github.com/docker/swarmkit/ca.(*Server).Run()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/ca/server.go:401 +0x40d
  github.com/docker/swarmkit/manager.(*Manager).becomeLeader.func5()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/manager/manager.go:945 +0x61

Previous write at 0x00c422a46d05 by goroutine 42:
  runtime.slicecopy()
      /usr/local/go/src/runtime/slice.go:144 +0x0
  bufio.(*Reader).Read()
      /usr/local/go/src/bufio/bufio.go:216 +0x21a
  crypto/rand.(*devReader).Read()
      /usr/local/go/src/crypto/rand/rand_unix.go:64 +0x159
  io.ReadAtLeast()
      /usr/local/go/src/io/io.go:307 +0xb1
  io.ReadFull()
      /usr/local/go/src/io/io.go:325 +0x72
  crypto/tls.(*Config).serverInit()
      /usr/local/go/src/crypto/tls/common.go:466 +0x2c6
  crypto/tls.(*Config).(crypto/tls.serverInit)-fm()
      /usr/local/go/src/crypto/tls/handshake_server.go:44 +0x41
  sync.(*Once).Do()
      /usr/local/go/src/sync/once.go:44 +0xf2
  crypto/tls.(*Conn).serverHandshake()
      /usr/local/go/src/crypto/tls/handshake_server.go:44 +0xca
  crypto/tls.(*Conn).Handshake()
      /usr/local/go/src/crypto/tls/conn.go:1234 +0x238
  github.com/docker/swarmkit/ca.(*MutableTLSCreds).ServerHandshake()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/ca/transport.go:116 +0x172
  github.com/docker/swarmkit/vendor/google.golang.org/grpc.(*Server).useTransportAuthenticator()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/vendor/google.golang.org/grpc/server.go:324 +0x9b
  github.com/docker/swarmkit/vendor/google.golang.org/grpc.(*Server).handleRawConn()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/vendor/google.golang.org/grpc/server.go:391 +0x6a

Goroutine 66 (running) created at:
  github.com/docker/swarmkit/manager.(*Manager).becomeLeader()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/manager/manager.go:948 +0x1b72
  github.com/docker/swarmkit/manager.(*Manager).handleLeadershipEvents()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/manager/manager.go:821 +0x1db

Goroutine 42 (running) created at:
  github.com/docker/swarmkit/vendor/google.golang.org/grpc.(*Server).Serve()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/vendor/google.golang.org/grpc/server.go:384 +0x6b3
  github.com/docker/swarmkit/manager.(*Manager).serveListener()
      /home/ubuntu/.go_workspace/src/github.com/docker/swarmkit/manager/manager.go:850 +0x740
==================

cc @cyli

[cyli]

@aaronlehmann Thanks! Hmm... I guess tlsconfig.Clone() may not be safe to use while the tls config is being used?

[cyli]

Looks like SessionTicketKey that's not safe to copy, since the data race seems to indicate https://github.com/docker/docker/blob/master/pkg/tlsconfig/tlsconfig_clone_go17.go#L25 and https://github.com/golang/go/blob/release-branch.go1.7/src/crypto/tls/common.go#L466 that's the problem. I wonder if this is an issue in the go 1.8 version of Clone too, since it also just copies the SessionTicketKey.

[cyli]

Looks like it doesn't affect go 1.8, although I'm not sure why. They added some extra stuff with duplicating the original config, but they still do write to c.SessionTicketKey and read from it.

But:

package main_test

import (
	"crypto/tls"
	"net/http"
	"net/http/httptest"
	"testing"

	"github.com/docker/docker/pkg/tlsconfig"
	"github.com/stretchr/testify/require"
)

// TestDockerTLSConfigClone attempts to trigger a data race when calling tlsconfig.Clone while a server is running
func TestDockerTLSConfigClone(t *testing.T) {
	client := http.Client{
		Transport: &http.Transport{
			TLSClientConfig: &tls.Config{
				InsecureSkipVerify: true,
			},
		},
	}

	s := httptest.NewTLSServer(http.DefaultServeMux)
	defer s.Close()
	go tlsconfig.Clone(s.TLS)

	done := make(chan struct{})
	go func() {
		defer close(done)
		_, err := client.Get(s.URL)
		require.NoError(t, err)
	}()

	<-done
}

Fails pretty immediately with the same data race in go 1.7, and not go 1.8

[aaronlehmann]

Could we work around this by cloning the TLS config when it's first created, and storing that as a template for future clones?

[cyli]

Sure - I was actually just going to recreate the client and server creds, since we don't actually set all that many parameters, but this is probably cleaner.

…

On Mon, Mar 6, 2017 at 4:05 PM Aaron Lehmann ***@***.***> wrote: Could we work around this by cloning the TLS config when it's first created, and storing that as a template for future clones? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2012 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANn4NcgCYWUHcwccVrZrxfgGyv6fqxpks5rjJ9egaJpZM4MUm4M> .

[aaronlehmann]

Recreating it also sounds good. That would avoid having to deal with Clone at all, right?

[cyli]

Yes

…

On Mon, Mar 6, 2017 at 4:09 PM Aaron Lehmann ***@***.***> wrote: Recreating it also sounds good. That would avoid having to deal with Clone at all, right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2012 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANn4HfDYz_DVF3sh32jh5drjm87e7LXks5rjKA7gaJpZM4MUm4M> .