From 5d87eb97be5f8820c17324859ad9ac6a0b0f5595 Mon Sep 17 00:00:00 2001
From: Marc 'risson' Schmitt <marc.schmitt@risson.space>
Date: Fri, 2 Jun 2023 12:52:20 +0200
Subject: [PATCH] outposts/ldap: fix race condition when refreshing the
 provider

Fixes the race condition causing the crash found in #4138, which doesn't
actually have anything to do with the issue itself.

As far as I can work out, when the outpost refreshes its list of
providers, it copies over its `boundUsers`, probably to avoid having to
fetch them all again, and does so by making a shallow copy of that
`map`, but not the mutex associated with it. It now has multiple
references to the same map, each protected by a different mutex, which
under certain conditions can cause a `concurrent map read and map write`
error.

This fix copies the map contents instead of make a shallow copy.

Signed-off-by: Marc 'risson' Schmitt <marc.schmitt@risson.space>
---
 internal/outpost/ldap/instance.go | 3 ++-
 internal/outpost/ldap/refresh.go  | 8 +++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/internal/outpost/ldap/instance.go b/internal/outpost/ldap/instance.go
index b39dc8c51..646fa7f7b 100644
--- a/internal/outpost/ldap/instance.go
+++ b/internal/outpost/ldap/instance.go
@@ -9,6 +9,7 @@ import (
 	"github.com/go-openapi/strfmt"
 	"github.com/nmcclain/ldap"
 	log "github.com/sirupsen/logrus"
+
 	"goauthentik.io/api/v3"
 	"goauthentik.io/internal/constants"
 	"goauthentik.io/internal/outpost/ldap/bind"
@@ -39,7 +40,7 @@ type ProviderInstance struct {
 	outpostName         string
 	outpostPk           int32
 	searchAllowedGroups []*strfmt.UUID
-	boundUsersMutex     sync.RWMutex
+	boundUsersMutex     *sync.RWMutex
 	boundUsers          map[string]*flags.UserFlags
 
 	uidStartNumber int32
diff --git a/internal/outpost/ldap/refresh.go b/internal/outpost/ldap/refresh.go
index 40b757c92..0d121ad46 100644
--- a/internal/outpost/ldap/refresh.go
+++ b/internal/outpost/ldap/refresh.go
@@ -9,6 +9,7 @@ import (
 
 	"github.com/go-openapi/strfmt"
 	log "github.com/sirupsen/logrus"
+
 	"goauthentik.io/api/v3"
 	"goauthentik.io/internal/outpost/ldap/bind"
 	directbind "goauthentik.io/internal/outpost/ldap/bind/direct"
@@ -56,11 +57,12 @@ func (ls *LDAPServer) Refresh() error {
 
 		// Get existing instance so we can transfer boundUsers
 		existing := ls.getCurrentProvider(provider.Pk)
+		usersMutex := &sync.RWMutex{}
 		users := make(map[string]*flags.UserFlags)
 		if existing != nil {
-			existing.boundUsersMutex.RLock()
+			usersMutex = existing.boundUsersMutex
+			// Shallow copy, no need to lock
 			users = existing.boundUsers
-			existing.boundUsersMutex.RUnlock()
 		}
 
 		providers[idx] = &ProviderInstance{
@@ -72,7 +74,7 @@ func (ls *LDAPServer) Refresh() error {
 			authenticationFlowSlug: provider.BindFlowSlug,
 			invalidationFlowSlug:   invalidationFlow,
 			searchAllowedGroups:    []*strfmt.UUID{(*strfmt.UUID)(provider.SearchGroup.Get())},
-			boundUsersMutex:        sync.RWMutex{},
+			boundUsersMutex:        usersMutex,
 			boundUsers:             users,
 			s:                      ls,
 			log:                    logger,