Fix a potential race: StaticOIDCClientsStore concurrent access (#45)

This PR introduces an RWMutex to protect StaticOIDCClientsStore access.

I read the StaticOIDCClientsStore implementation and think that a race
condition is possible under some circumstances, in particular when a new
OIDCClientConfig is added (just after the auth agent startup) and at the
same time HAProxy causes the auth agent to lookup for another
OIDCClientConfig for another domain.

I tried to write a test case (in the commit) to check how it behaved and
before I added the sync.RWMutex, I managed to get, sometimes (once every
5-8 test runs) an error:

```
fatal error: concurrent map read and map write

goroutine 9 [running]:
github.com/criteo/haproxy-spoe-auth/internal/auth.(*StaticOIDCClientsStore).GetClient(...)
        /home/demonihin/projects/haproxy-spoe-auth/internal/auth/oidc_clients_store.go:38
github.com/criteo/haproxy-spoe-auth/internal/auth.TestStaticOIDCClientsStoreRace.func2()
        /home/demonihin/projects/haproxy-spoe-auth/internal/auth/oidc_clients_store_test.go:38 +0x78
created by github.com/criteo/haproxy-spoe-auth/internal/auth.TestStaticOIDCClientsStoreRace in goroutine 6
        /home/demonihin/projects/haproxy-spoe-auth/internal/auth/oidc_clients_store_test.go:36 +0x114

goroutine 1 [runnable]:
internal/poll.(*FD).Write(0x400014bc18, {0x222380, 0x400000e348, 0x100004000103450})
        /opt/go/src/internal/poll/fd_unix.go:366
os.(*File).write(...)
        /opt/go/src/os/file_posix.go:46
os.(*File).Write(0x4000052030, {0x40000108b8?, 0x5, 0xf202c?})
        /opt/go/src/os/file.go:183 +0x5c
fmt.Fprint({0x293048, 0x4000052030}, {0x400014bdb8, 0x2, 0x2})
        /opt/go/src/fmt/print.go:263 +0x74
fmt.Print(...)
        /opt/go/src/fmt/print.go:272
testing.(*M).Run(0x4000104dc0)
        /opt/go/src/testing/testing.go:1961 +0x8bc
main.main()
        _testmain.go:59 +0x1a8

goroutine 7 [runnable]:
strings.Clone(...)
        /opt/go/src/strings/clone.go:25
github.com/criteo/haproxy-spoe-auth/internal/auth.(*StaticOIDCClientsStore).AddClient(0x400006e2e0, {0x400004f790, 0xe}, {0x22d5fe, 0x9}, {0x22eab3, 0xd}, {0x4000200340, 0x1f})
        /home/demonihin/projects/haproxy-spoe-auth/internal/auth/oidc_clients_store.go:49 +0xac
github.com/criteo/haproxy-spoe-auth/internal/auth.TestStaticOIDCClientsStoreRace.func1()
        /home/demonihin/projects/haproxy-spoe-auth/internal/auth/oidc_clients_store_test.go:26 +0xa4
created by github.com/criteo/haproxy-spoe-auth/internal/auth.TestStaticOIDCClientsStoreRace in goroutine 6
        /home/demonihin/projects/haproxy-spoe-auth/internal/auth/oidc_clients_store_test.go:17 +0xec
exit status 2
FAIL    github.com/criteo/haproxy-spoe-auth/internal/auth       0.040s
```

Co-authored-by: Dmitrii Ermakov <[email protected]>

internal/auth/oidc_clients_store.go

@@ -2,6 +2,7 @@ package auth
 
 import (
 	"strings"
+	"sync"
 )
 
 type OIDCClientConfig struct {
@@ -18,6 +19,8 @@ type OIDCClientsStore interface {
 
 type StaticOIDCClientsStore struct {
 	clients map[string]OIDCClientConfig
+
+	mtx sync.RWMutex
 }
 
 func NewStaticOIDCClientStore(config map[string]OIDCClientConfig) *StaticOIDCClientsStore {
@@ -29,18 +32,24 @@ func NewEmptyStaticOIDCClientStore() *StaticOIDCClientsStore {
 }
 
 func (ocf *StaticOIDCClientsStore) GetClient(domain string) (*OIDCClientConfig, error) {
+	ocf.mtx.RLock()
+	defer ocf.mtx.RUnlock()
+
 	if config, ok := ocf.clients[domain]; ok {
 		return &config, nil
 	}
 	return nil, ErrOIDCClientConfigNotFound
 }
 
 func (ocf *StaticOIDCClientsStore) AddClient(domain string, clientid string, clientsecret string, redirecturl string) {
+	ocf.mtx.Lock()
+	defer ocf.mtx.Unlock()
+
 	if _, ok := ocf.clients[domain]; !ok {
-		ocf.clients[strings.Clone(domain)] = OIDCClientConfig {
-			ClientID: strings.Clone(clientid),
+		ocf.clients[strings.Clone(domain)] = OIDCClientConfig{
+			ClientID:     strings.Clone(clientid),
 			ClientSecret: strings.Clone(clientsecret),
-			RedirectURL: strings.Clone(redirecturl),
+			RedirectURL:  strings.Clone(redirecturl),
 		}
 	}
 }

internal/auth/oidc_clients_store_test.go

@@ -0,0 +1,52 @@
+package auth
+
+import (
+	"strconv"
+	"sync"
+	"testing"
+)
+
+func TestStaticOIDCClientsStoreRace(t *testing.T) {
+	var wg = &sync.WaitGroup{}
+	var expectedValue OIDCClientConfig
+	var store = NewEmptyStaticOIDCClientStore()
+	const steps = 10000
+
+	// One Goroutine changes the store state while 2 other try to read from it.
+	wg.Add(1)
+	go func() {
+		for i := 0; i < steps; i++ {
+			// Something comes with requests for a new and a valid domain,
+			// so it is being added to the store.
+			expectedValue.ClientID = "client-id"
+			expectedValue.ClientSecret = "client-secret"
+			expectedValue.RedirectURL = "https://" + strconv.Itoa(i) + ".example.com/redirect"
+			domain := strconv.Itoa(i) + ".example.com"
+
+			store.AddClient(domain, expectedValue.ClientID, expectedValue.ClientSecret, expectedValue.RedirectURL)
+		}
+
+		wg.Done()
+	}()
+
+	// Read and compare.
+	var found bool
+	for i := 0; i < 2; i++ {
+		wg.Add(1)
+		go func() {
+			for i := 0; i < steps; i++ {
+				_, err := store.GetClient("100000.example.com")
+				if err == nil {
+					found = true
+					break
+				}
+			}
+
+			wg.Done()
+		}()
+	}
+
+	if found {
+		t.Fatal("Received a value while should get ErrOIDCClientConfigNotFound")
+	}
+}