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>
This commit is contained in:
parent
48ba1af481
commit
5d87eb97be
|
@ -9,6 +9,7 @@ import (
|
||||||
"github.com/go-openapi/strfmt"
|
"github.com/go-openapi/strfmt"
|
||||||
"github.com/nmcclain/ldap"
|
"github.com/nmcclain/ldap"
|
||||||
log "github.com/sirupsen/logrus"
|
log "github.com/sirupsen/logrus"
|
||||||
|
|
||||||
"goauthentik.io/api/v3"
|
"goauthentik.io/api/v3"
|
||||||
"goauthentik.io/internal/constants"
|
"goauthentik.io/internal/constants"
|
||||||
"goauthentik.io/internal/outpost/ldap/bind"
|
"goauthentik.io/internal/outpost/ldap/bind"
|
||||||
|
@ -39,7 +40,7 @@ type ProviderInstance struct {
|
||||||
outpostName string
|
outpostName string
|
||||||
outpostPk int32
|
outpostPk int32
|
||||||
searchAllowedGroups []*strfmt.UUID
|
searchAllowedGroups []*strfmt.UUID
|
||||||
boundUsersMutex sync.RWMutex
|
boundUsersMutex *sync.RWMutex
|
||||||
boundUsers map[string]*flags.UserFlags
|
boundUsers map[string]*flags.UserFlags
|
||||||
|
|
||||||
uidStartNumber int32
|
uidStartNumber int32
|
||||||
|
|
|
@ -9,6 +9,7 @@ import (
|
||||||
|
|
||||||
"github.com/go-openapi/strfmt"
|
"github.com/go-openapi/strfmt"
|
||||||
log "github.com/sirupsen/logrus"
|
log "github.com/sirupsen/logrus"
|
||||||
|
|
||||||
"goauthentik.io/api/v3"
|
"goauthentik.io/api/v3"
|
||||||
"goauthentik.io/internal/outpost/ldap/bind"
|
"goauthentik.io/internal/outpost/ldap/bind"
|
||||||
directbind "goauthentik.io/internal/outpost/ldap/bind/direct"
|
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
|
// Get existing instance so we can transfer boundUsers
|
||||||
existing := ls.getCurrentProvider(provider.Pk)
|
existing := ls.getCurrentProvider(provider.Pk)
|
||||||
|
usersMutex := &sync.RWMutex{}
|
||||||
users := make(map[string]*flags.UserFlags)
|
users := make(map[string]*flags.UserFlags)
|
||||||
if existing != nil {
|
if existing != nil {
|
||||||
existing.boundUsersMutex.RLock()
|
usersMutex = existing.boundUsersMutex
|
||||||
|
// Shallow copy, no need to lock
|
||||||
users = existing.boundUsers
|
users = existing.boundUsers
|
||||||
existing.boundUsersMutex.RUnlock()
|
|
||||||
}
|
}
|
||||||
|
|
||||||
providers[idx] = &ProviderInstance{
|
providers[idx] = &ProviderInstance{
|
||||||
|
@ -72,7 +74,7 @@ func (ls *LDAPServer) Refresh() error {
|
||||||
authenticationFlowSlug: provider.BindFlowSlug,
|
authenticationFlowSlug: provider.BindFlowSlug,
|
||||||
invalidationFlowSlug: invalidationFlow,
|
invalidationFlowSlug: invalidationFlow,
|
||||||
searchAllowedGroups: []*strfmt.UUID{(*strfmt.UUID)(provider.SearchGroup.Get())},
|
searchAllowedGroups: []*strfmt.UUID{(*strfmt.UUID)(provider.SearchGroup.Get())},
|
||||||
boundUsersMutex: sync.RWMutex{},
|
boundUsersMutex: usersMutex,
|
||||||
boundUsers: users,
|
boundUsers: users,
|
||||||
s: ls,
|
s: ls,
|
||||||
log: logger,
|
log: logger,
|
||||||
|
|
Reference in New Issue