race condition bug in minio http server

[aead]

The minio http server contains a race condition bug caused by a shared pointer of tls.Config.
Take a look at the following line. The comment says: "Take a copy" but we don't copy - we share the pointer. This can lead to race conditions when the tls.Config is accessed from the code which spans the server and the server itself.

Expected Behavior

The tls.Config should be copied (cloned)

Current Behavior

The tls.Config is shared

Possible Solution

Replace https://github.com/minio/minio/blob/master/pkg/http/server.go#L67 with
lsConfig := srv.TLSConfig.Clone()

Steps to Reproduce (for bugs)

Run the following test with the race detector go test -race -v:

func TestTLSCiphers(t *testing.T) {
	certificate, err := getTLSCert()
	if err != nil {
		t.Fatalf("Unable to parse private/certificate data. %v\n", err)
	}
	port := getNextPort()
	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		fmt.Fprintf(w, "Hello, world")
	})
	server := NewServer([]string{"127.0.0.1:" + port}, handler, &certificate)
	go func() { panic(server.Start()) }()
	defer server.Shutdown()
	for i, test := range tlsCipherSuitesTests {
		client := http.Client{
			Transport: &http.Transport{
				TLSClientConfig: &tls.Config{
					InsecureSkipVerify: true,
					CipherSuites:       test.ciphers,
				},
			},
		}
		_, err := client.Get("https://127.0.0.1:" + port + "/")
		if err != nil && !test.shouldFail {
			t.Fatalf("Test %d: Failed to execute GET request: %s", i, err)
		}
		if err == nil && test.shouldFail {
			t.Fatalf("Test %d: Successfully connected to server but expected connection failure", i)
		}
	}
}

Context

Detected while fixing #5245

Your Environment

master

[fwessels]

Nice catch -- are there any other places in the code where the same mistake could be made?

[aead]

Probably... For example 4 lines further down: https://github.com/minio/minio/blob/master/pkg/http/server.go#L72 - We don't copy the slice... However it's very unlikely that somebody changes the addresses - nevertheless we should fix it, too.

[balamurugana]

The idea of taking copy of server fields is to safely start the server irrespective of field value changes. As addrs argument is to NewServer(), we would need to safely store. Basically Addrs should be a set.StringSet and NewServer() could create the set from the inputted addrs

[aead]

The idea of taking copy of server fields is to safely start the server irrespective of field value changes.

Correct. The straight-farword solution (IMO) to this would be:

addrs := make([]string, len(srv.Addrs))
copy(addrs, srv.Addrs)

[fwessels]

Yes, please fix

[balamurugana]

Correct. The straight-farword solution (IMO) to this would be:

addrs := make([]string, len(srv.Addrs))
copy(addrs, srv.Addrs)

I won't prefer this patching. As I mentioned above, we should fix properly by using set.StringSet which fixes completely i.e. no more duplicate addr where we error out later.

[aead]

While I agree that we should fix duplicate addresses I still would not do this in Start.
It looks odd from an API point of view if I can create a server with duplicate addresses but cannot start it:

server := NewServer(addresses, ...)
server.Start() // fails because of duplicate addresses

or do you want to just ignore duplicates and not error out?

[balamurugana]

While I agree that we should fix duplicate addresses I still would not do this in Start. It looks odd from an API point of view if I can create a server with duplicate addresses but cannot start it:

You are right. The exact way of fixing this problem is like below

1. Change Addrs field type to set.StringSet from []string currently.
2. In NewServer(), assign addrs arg to Addrs by set.CreateStringSet(addrs...)
3. In Start(), have addrs = srv.Addrs.ToSlice()

Here, duplicate addrs are ignored i.e. no error is thrown. As Addrs is a set.StringSet there is no way user adds/modifies Addrs with duplicates.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.