From 493877f03ff18b560d86bdc224e50a7aef3fe3f6 Mon Sep 17 00:00:00 2001 From: Kevin Petremann Date: Mon, 21 Aug 2023 10:19:29 +0200 Subject: [PATCH 1/8] fix: concurrent calls not authorized and LDAP timeout --- Makefile | 4 +- go.mod | 1 + go.sum | 2 + internal/api/auth/basic_auth.go | 16 ++++- internal/api/auth/ldap.go | 117 +++++++++++++++++++++++++++----- internal/api/router/manager.go | 2 +- 6 files changed, 121 insertions(+), 21 deletions(-) diff --git a/Makefile b/Makefile index b3af870..4e594a3 100644 --- a/Makefile +++ b/Makefile @@ -6,13 +6,13 @@ build: go build -ldflags="-X 'github.com/criteo/data-aggregation-api/internal/version.version=$(VER)' \ -X 'github.com/criteo/data-aggregation-api/internal/version.buildUser=$(USER)' \ -X 'github.com/criteo/data-aggregation-api/internal/version.buildTime=$(BUILDDATE)'" \ - -o .build/data_aggregation_api ./cmd/data_aggregation_api + -o .build/data-aggregation-api ./cmd/data-aggregation-api run: go run -ldflags="-X 'github.com/criteo/data-aggregation-api/internal/version.version=$(VER)' \ -X 'github.com/criteo/data-aggregation-api/internal/version.buildUser=$(USER)' \ -X 'github.com/criteo/data-aggregation-api/internal/version.buildTime=$(BUILDDATE)'" \ - ./cmd/data_aggregation_api + ./cmd/data-aggregation-api update_openconfig: ./update_openconfig.sh \ No newline at end of file diff --git a/go.mod b/go.mod index 5a025b4..30a61f0 100644 --- a/go.mod +++ b/go.mod @@ -12,6 +12,7 @@ require ( github.com/prometheus/client_golang v1.16.0 github.com/rs/zerolog v1.30.0 github.com/spf13/viper v1.16.0 + golang.org/x/sync v0.3.0 ) require ( diff --git a/go.sum b/go.sum index b4018ed..a0e998a 100644 --- a/go.sum +++ b/go.sum @@ -359,6 +359,8 @@ golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20201207232520-09787c993a3a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.3.0 h1:ftCYgMx6zT/asHUrPw8BLLscYtGznsLAnjq5RH9P66E= +golang.org/x/sync v0.3.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190312061237-fead79001313/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= diff --git a/internal/api/auth/basic_auth.go b/internal/api/auth/basic_auth.go index f83d995..0111e3c 100644 --- a/internal/api/auth/basic_auth.go +++ b/internal/api/auth/basic_auth.go @@ -1,6 +1,7 @@ package auth import ( + "context" "crypto/tls" "errors" "fmt" @@ -19,6 +20,8 @@ const ( type authMode string +const maxLDAPWorkers = 1 + const ( noAuth authMode = "None" ldapMode authMode = "LDAP" @@ -29,7 +32,7 @@ type BasicAuth struct { mode authMode } -func NewBasicAuth(cfg config.AuthConfig) (BasicAuth, error) { +func NewBasicAuth(ctx context.Context, cfg config.AuthConfig) (BasicAuth, error) { b := BasicAuth{mode: noAuth} if cfg.LDAP == nil { @@ -45,6 +48,17 @@ func NewBasicAuth(cfg config.AuthConfig) (BasicAuth, error) { } b.mode = ldapMode + // trying a connection to LDAP to check the configuration + conn, err := ldap.connect() + if err != nil { + return b, fmt.Errorf("test LDAP connection: %w", err) + } + if err := conn.Close(); err != nil { + log.Warn().Err(err).Msg("failed to close LDAP test connection") + } + + ldap.StartWorkers(ctx, maxLDAPWorkers) + return b, nil } diff --git a/internal/api/auth/ldap.go b/internal/api/auth/ldap.go index 0100ba8..ecb7a7d 100644 --- a/internal/api/auth/ldap.go +++ b/internal/api/auth/ldap.go @@ -1,41 +1,125 @@ package auth import ( + "context" "crypto/tls" "fmt" + "time" "github.com/rs/zerolog/log" "github.com/go-ldap/ldap/v3" ) +type authRequest struct { + authResp chan bool + username string + password string +} + type LDAPAuth struct { - ldapClient *ldap.Conn - bindDN string - password string - baseDN string + tlsConfig *tls.Config + reqCh chan authRequest + ldapURL string + bindDN string + password string + baseDN string } func NewLDAPAuth(ldapURL string, bindDN string, password string, baseDN string, tlsConfig *tls.Config) *LDAPAuth { - conn, err := ldap.DialURL(ldapURL, ldap.DialWithTLSConfig(tlsConfig)) - if err != nil { - log.Error().Err(err).Str("ldapURL", ldapURL).Msg("failed to connect to the LDAP server") - return nil - } return &LDAPAuth{ - ldapClient: conn, - bindDN: bindDN, - password: password, - baseDN: baseDN, + tlsConfig: tlsConfig, + ldapURL: ldapURL, + bindDN: bindDN, + password: password, + baseDN: baseDN, + reqCh: make(chan authRequest), + } +} + +func (l *LDAPAuth) StartWorkers(ctx context.Context, maxWorker int) { + for i := 0; i < maxWorker; i++ { + go l.spawnWorker(ctx) } } func (l *LDAPAuth) AuthenticateUser(username string, password string) bool { - if err := l.ldapClient.Bind(l.bindDN, l.password); err != nil { + req := authRequest{ + username: username, + password: password, + authResp: make(chan bool), + } + l.reqCh <- req + return <-req.authResp +} + +func (l *LDAPAuth) spawnWorker(ctx context.Context) { + var conn *ldap.Conn + var err error + tick := time.NewTicker(time.Minute) + + for { + select { + case req := <-l.reqCh: + // (re)connect if needed + if conn == nil || conn.IsClosing() { + log.Debug().Msg("LDAP connection is closed, reconnecting") + conn, err = l.connect() + if err != nil { + log.Error().Err(err).Msg("worker LDAP reconnection failed") + req.authResp <- false + return + } + } + + // bind with the user credentials + req.authResp <- l.authenticate(conn, req.username, req.password) + + tick.Reset(time.Minute) + + case <-tick.C: + // close connection if no request has been made for a minute + log.Debug().Msg("timer reached, closing connection") + if conn != nil { + if err := conn.Close(); err != nil { + log.Error().Err(err).Msg("unable to close the LDAP connection") + } + } + return + + case <-ctx.Done(): + // gracefully close connection if context is done + log.Debug().Msg("context is closed, closing connection") + if conn != nil { + _ = conn.Close() + } + return + } + } +} + +func (l *LDAPAuth) connect() (*ldap.Conn, error) { + conn, err := ldap.DialURL(l.ldapURL, ldap.DialWithTLSConfig(l.tlsConfig)) + if err != nil { + return nil, fmt.Errorf("failed to dial to the LDAP server: %w", err) + } + + return conn, nil +} + +func (l *LDAPAuth) authenticate(conn *ldap.Conn, username string, password string) bool { + if err := conn.Bind(l.bindDN, l.password); err != nil { log.Error().Err(err).Str("bindDN", l.bindDN).Msg("failed to bind to LDAP") + + // detect TCP connection closed or any network errors + if ldap.IsErrorWithCode(err, ldap.ErrorNetwork) { + if err := conn.Close(); err != nil { + log.Error().Err(err).Msg("connection was closed by the server but failed to close on client side") + } + } return false } - search, err := l.ldapClient.Search(ldap.NewSearchRequest( + search, err := conn.Search(ldap.NewSearchRequest( l.baseDN, ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, @@ -56,10 +140,9 @@ func (l *LDAPAuth) AuthenticateUser(username string, password string) bool { log.Error().Str(userKey, username).Msg("no result or more than 1 result found for user") return false } - if err := l.ldapClient.Bind(search.Entries[0].DN, password); err != nil { + if err := conn.Bind(search.Entries[0].DN, password); err != nil { log.Error().Err(err).Str(userKey, username).Msg("failed to bind with user") return false } - log.Debug().Str(userKey, username).Msg("succesfully authenticated user") return true } diff --git a/internal/api/router/manager.go b/internal/api/router/manager.go index b3d32b0..8757c29 100644 --- a/internal/api/router/manager.go +++ b/internal/api/router/manager.go @@ -39,7 +39,7 @@ func (m *Manager) ListenAndServe(ctx context.Context, address string, port int) log.Warn().Msg("Shutdown.") }() - withAuth, err := auth.NewBasicAuth(config.Cfg.Authentication) + withAuth, err := auth.NewBasicAuth(ctx, config.Cfg.Authentication) if err != nil { return err } From be15a4b390b3c8c523d5b2396c3f8bd2bfeab65e Mon Sep 17 00:00:00 2001 From: Kevin Petremann Date: Mon, 21 Aug 2023 15:00:20 +0200 Subject: [PATCH 2/8] feat: number of LDAP worker configurable --- internal/api/auth/basic_auth.go | 6 +++--- internal/api/auth/ldap.go | 6 +++++- internal/config/settings.go | 4 ++++ settings.example.yaml | 1 + 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/internal/api/auth/basic_auth.go b/internal/api/auth/basic_auth.go index 0111e3c..87f6e34 100644 --- a/internal/api/auth/basic_auth.go +++ b/internal/api/auth/basic_auth.go @@ -20,8 +20,6 @@ const ( type authMode string -const maxLDAPWorkers = 1 - const ( noAuth authMode = "None" ldapMode authMode = "LDAP" @@ -57,7 +55,9 @@ func NewBasicAuth(ctx context.Context, cfg config.AuthConfig) (BasicAuth, error) log.Warn().Err(err).Msg("failed to close LDAP test connection") } - ldap.StartWorkers(ctx, maxLDAPWorkers) + if err := ldap.StartWorkers(ctx, cfg.LDAP.MaxWorkers); err != nil { + return b, fmt.Errorf("failed to start LDAP workers: %w", err) + } return b, nil } diff --git a/internal/api/auth/ldap.go b/internal/api/auth/ldap.go index ecb7a7d..8f1ab0f 100644 --- a/internal/api/auth/ldap.go +++ b/internal/api/auth/ldap.go @@ -37,10 +37,14 @@ func NewLDAPAuth(ldapURL string, bindDN string, password string, baseDN string, } } -func (l *LDAPAuth) StartWorkers(ctx context.Context, maxWorker int) { +func (l *LDAPAuth) StartWorkers(ctx context.Context, maxWorker int) error { + if maxWorker <= 0 { + return fmt.Errorf("maxWorker must be greater than 0") + } for i := 0; i < maxWorker; i++ { go l.spawnWorker(ctx) } + return nil } func (l *LDAPAuth) AuthenticateUser(username string, password string) bool { diff --git a/internal/config/settings.go b/internal/config/settings.go index 9b0376a..783ae28 100644 --- a/internal/config/settings.go +++ b/internal/config/settings.go @@ -15,6 +15,8 @@ const ( defaultListenPort = 8080 localPath = "." + defaultMaxLDAPWorkers = 10 + SiteFilter Filter = "site" SiteGroupFilter Filter = "site_group" SiteRegionFilter Filter = "region" @@ -56,6 +58,7 @@ type LDAPConfig struct { BindDN string Password string InsecureSkipVerify bool + MaxWorkers int } func setDefaults() { @@ -78,6 +81,7 @@ func setDefaults() { viper.SetDefault("Authentication.LDAP.BindDN", "") viper.SetDefault("Authentication.LDAP.Password", "") viper.SetDefault("Authentication.LDAP.InsecureSkipVerify", false) + viper.SetDefault("Authentication.LDAP.MaxWorkers", defaultMaxLDAPWorkers) } func LoadConfig() error { diff --git a/settings.example.yaml b/settings.example.yaml index 947ee56..f5bb9ee 100644 --- a/settings.example.yaml +++ b/settings.example.yaml @@ -16,6 +16,7 @@ Authentication: BindDN: "cn=,OU=,DC=" BaseDN: "DC=" Password: "" + MaxWorkers: 10 NetBox: URL: "https://netbox.local" From ca29c805cb260edf6ea995f9933af19880d9540a Mon Sep 17 00:00:00 2001 From: Kevin Petremann Date: Tue, 22 Aug 2023 12:15:48 +0200 Subject: [PATCH 3/8] fix: prevent http shutdown to be blocked --- internal/api/router/manager.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/internal/api/router/manager.go b/internal/api/router/manager.go index 8757c29..1d60791 100644 --- a/internal/api/router/manager.go +++ b/internal/api/router/manager.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/http" + "time" "github.com/prometheus/client_golang/prometheus/promhttp" "github.com/rs/zerolog/log" @@ -15,6 +16,8 @@ import ( "github.com/julienschmidt/httprouter" ) +const shutdownTimeout = 5 * time.Second + type DevicesRepository interface { Set(devices map[string]*device.Device) ListAFKEnabledDevicesJSON() ([]byte, error) @@ -63,12 +66,15 @@ func (m *Manager) ListenAndServe(ctx context.Context, address string, port int) // TODO: handle http failure! with a channel go func() { if err := httpServer.ListenAndServe(); err != nil { - log.Error().Err(err).Send() + log.Error().Err(err).Msg("stopped to listen and serve") } }() <-ctx.Done() - if err := httpServer.Shutdown(context.Background()); err != nil { + ctxCancel, cancel := context.WithTimeout(context.Background(), shutdownTimeout) + defer cancel() + + if err := httpServer.Shutdown(ctxCancel); err != nil { log.Error().Err(err).Send() } From 12b4b193cb63338ecbd0252f1d91abffe845c9d9 Mon Sep 17 00:00:00 2001 From: Kevin Petremann Date: Tue, 22 Aug 2023 12:17:02 +0200 Subject: [PATCH 4/8] fix: handle timeout for LDAP requests In some cases like network connectivity lost, the LDAP bind request is waiting indefinetely. To avoid that, we to a timeout on the LDAP request by closing the LDAP connection. --- cmd/data-aggregation-api/main.go | 9 +++ go.mod | 1 - go.sum | 2 - internal/api/auth/ldap.go | 116 +++++++++++++++++++++++++------ internal/config/settings.go | 3 + 5 files changed, 105 insertions(+), 26 deletions(-) diff --git a/cmd/data-aggregation-api/main.go b/cmd/data-aggregation-api/main.go index c0c9b36..ba38607 100644 --- a/cmd/data-aggregation-api/main.go +++ b/cmd/data-aggregation-api/main.go @@ -10,6 +10,7 @@ import ( "github.com/rs/zerolog" "github.com/rs/zerolog/log" + "github.com/criteo/data-aggregation-api/internal/api/auth" "github.com/criteo/data-aggregation-api/internal/api/router" "github.com/criteo/data-aggregation-api/internal/app" "github.com/criteo/data-aggregation-api/internal/config" @@ -55,6 +56,14 @@ func run() error { log.Info().Str("build-time", date).Send() log.Info().Str("build-user", builtBy).Send() + // Configure LDAP timeout + if config.Cfg.Authentication.LDAP != nil { + if config.Cfg.Authentication.LDAP.Timeout <= 0 { + return fmt.Errorf("LDAP timeout must be greater than 0") + } + auth.SetDefaultTimeout(config.Cfg.Authentication.LDAP.Timeout) + } + deviceRepo := device.NewSafeRepository() reports := report.NewRepository() diff --git a/go.mod b/go.mod index 30a61f0..5a025b4 100644 --- a/go.mod +++ b/go.mod @@ -12,7 +12,6 @@ require ( github.com/prometheus/client_golang v1.16.0 github.com/rs/zerolog v1.30.0 github.com/spf13/viper v1.16.0 - golang.org/x/sync v0.3.0 ) require ( diff --git a/go.sum b/go.sum index a0e998a..b4018ed 100644 --- a/go.sum +++ b/go.sum @@ -359,8 +359,6 @@ golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sync v0.0.0-20201207232520-09787c993a3a/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.3.0 h1:ftCYgMx6zT/asHUrPw8BLLscYtGznsLAnjq5RH9P66E= -golang.org/x/sync v0.3.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190312061237-fead79001313/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= diff --git a/internal/api/auth/ldap.go b/internal/api/auth/ldap.go index 8f1ab0f..024a743 100644 --- a/internal/api/auth/ldap.go +++ b/internal/api/auth/ldap.go @@ -11,12 +11,24 @@ import ( "github.com/go-ldap/ldap/v3" ) +type connectionStatus bool + +const ( + connectionClosed connectionStatus = false + connectionUp connectionStatus = true +) + type authRequest struct { authResp chan bool username string password string } +type result struct { + auth bool + conn connectionStatus +} + type LDAPAuth struct { tlsConfig *tls.Config reqCh chan authRequest @@ -37,6 +49,10 @@ func NewLDAPAuth(ldapURL string, bindDN string, password string, baseDN string, } } +func SetDefaultTimeout(timeout time.Duration) { + ldap.DefaultTimeout = timeout //nolint:reassign // we want to customize the default timeout +} + func (l *LDAPAuth) StartWorkers(ctx context.Context, maxWorker int) error { if maxWorker <= 0 { return fmt.Errorf("maxWorker must be greater than 0") @@ -46,7 +62,6 @@ func (l *LDAPAuth) StartWorkers(ctx context.Context, maxWorker int) error { } return nil } - func (l *LDAPAuth) AuthenticateUser(username string, password string) bool { req := authRequest{ username: username, @@ -58,6 +73,7 @@ func (l *LDAPAuth) AuthenticateUser(username string, password string) bool { } func (l *LDAPAuth) spawnWorker(ctx context.Context) { + const maxRetry = 1 var conn *ldap.Conn var err error tick := time.NewTicker(time.Minute) @@ -65,20 +81,41 @@ func (l *LDAPAuth) spawnWorker(ctx context.Context) { for { select { case req := <-l.reqCh: - // (re)connect if needed - if conn == nil || conn.IsClosing() { - log.Debug().Msg("LDAP connection is closed, reconnecting") - conn, err = l.connect() - if err != nil { - log.Error().Err(err).Msg("worker LDAP reconnection failed") - req.authResp <- false - return + auth := false + retry := 0 + for retry <= maxRetry+1 { + retry++ + log.Debug().Msgf("worker LDAP authentication attempt number %d", retry) + // (re)connect if needed + if conn == nil || conn.IsClosing() { + log.Debug().Msg("LDAP connection is closed, reconnecting") + conn, err = l.connect() + if err != nil { + log.Error().Err(err).Msg("worker LDAP reconnection failed") + req.authResp <- false + break + } + } + + // bind with the user credentials + var connState connectionStatus + auth, connState = l.authenticateWithTimeout(ctx, conn, req.username, req.password, ldap.DefaultTimeout) + + if connState == connectionClosed { + log.Debug().Msg("LDAP connection was closed by the server, closing on client side") + if err := conn.Close(); err != nil { + log.Error().Err(err).Msg("connection was closed by the server but failed to close on client side") + } + } else { + // LDAP connection is still up, we accept the authentication result + log.Debug().Msg("auth response valid") + break } } - // bind with the user credentials - req.authResp <- l.authenticate(conn, req.username, req.password) + log.Debug().Msgf("worker LDAP authentication attempt number %d, result: %t", retry, auth) + req.authResp <- auth tick.Reset(time.Minute) case <-tick.C: @@ -94,14 +131,46 @@ func (l *LDAPAuth) spawnWorker(ctx context.Context) { case <-ctx.Done(): // gracefully close connection if context is done log.Debug().Msg("context is closed, closing connection") - if conn != nil { - _ = conn.Close() - } + closeLDAPConnection(conn) return } } } +func closeLDAPConnection(conn *ldap.Conn) { + if conn != nil && !conn.IsClosing() { + _ = conn.Close() + } +} + +// authenticateWithTimeout performs the authentication against LDAP with a timeout. +func (l *LDAPAuth) authenticateWithTimeout(ctx context.Context, conn *ldap.Conn, username, password string, timeout time.Duration) (bool, connectionStatus) { + // request the authentication + res := make(chan result) + go func() { + a, c := l.authenticate(conn, username, password) + res <- result{auth: a, conn: c} + }() + + var connState connectionStatus + var auth bool + + // handle timeout and context closing + select { + case r := <-res: + auth = r.auth + connState = r.conn + case <-time.After(timeout): + log.Error().Msg("LDAP authentication timeout") + closeLDAPConnection(conn) + auth = false + connState = connectionClosed + case <-ctx.Done(): + auth = false + } + return auth, connState +} + func (l *LDAPAuth) connect() (*ldap.Conn, error) { conn, err := ldap.DialURL(l.ldapURL, ldap.DialWithTLSConfig(l.tlsConfig)) if err != nil { @@ -111,17 +180,18 @@ func (l *LDAPAuth) connect() (*ldap.Conn, error) { return conn, nil } -func (l *LDAPAuth) authenticate(conn *ldap.Conn, username string, password string) bool { +// authenticate performs the authentication against LDAP. +// The first returned boolean is true if the authentication is successful, false otherwise. +// The second returned boolean is true if the connection is closed, false otherwise. +func (l *LDAPAuth) authenticate(conn *ldap.Conn, username string, password string) (bool, connectionStatus) { if err := conn.Bind(l.bindDN, l.password); err != nil { log.Error().Err(err).Str("bindDN", l.bindDN).Msg("failed to bind to LDAP") // detect TCP connection closed or any network errors if ldap.IsErrorWithCode(err, ldap.ErrorNetwork) { - if err := conn.Close(); err != nil { - log.Error().Err(err).Msg("connection was closed by the server but failed to close on client side") - } + return false, connectionClosed } - return false + return false, connectionUp } search, err := conn.Search(ldap.NewSearchRequest( l.baseDN, @@ -138,15 +208,15 @@ func (l *LDAPAuth) authenticate(conn *ldap.Conn, username string, password strin const userKey = "user" if err != nil { log.Error().Err(err).Str(userKey, username).Msg("failed to perform LDAP search to find user") - return false + return false, connectionUp } if len(search.Entries) != 1 { log.Error().Str(userKey, username).Msg("no result or more than 1 result found for user") - return false + return false, connectionUp } if err := conn.Bind(search.Entries[0].DN, password); err != nil { log.Error().Err(err).Str(userKey, username).Msg("failed to bind with user") - return false + return false, connectionUp } - return true + return true, connectionUp } diff --git a/internal/config/settings.go b/internal/config/settings.go index 783ae28..b63b347 100644 --- a/internal/config/settings.go +++ b/internal/config/settings.go @@ -16,6 +16,7 @@ const ( localPath = "." defaultMaxLDAPWorkers = 10 + defaultLDAPTimeout = 10 * time.Second SiteFilter Filter = "site" SiteGroupFilter Filter = "site_group" @@ -57,6 +58,7 @@ type LDAPConfig struct { BaseDN string BindDN string Password string + Timeout time.Duration InsecureSkipVerify bool MaxWorkers int } @@ -82,6 +84,7 @@ func setDefaults() { viper.SetDefault("Authentication.LDAP.Password", "") viper.SetDefault("Authentication.LDAP.InsecureSkipVerify", false) viper.SetDefault("Authentication.LDAP.MaxWorkers", defaultMaxLDAPWorkers) + viper.SetDefault("Authentication.LDAP.Timeout", defaultLDAPTimeout) } func LoadConfig() error { From 765900104fdd6139136536eaa737483a95279544 Mon Sep 17 00:00:00 2001 From: Kevin Petremann Date: Thu, 24 Aug 2023 17:36:44 +0200 Subject: [PATCH 5/8] refactor: rework ldap implementation --- cmd/data-aggregation-api/main.go | 4 ++-- internal/api/auth/basic_auth.go | 10 ++++++---- internal/api/auth/ldap.go | 34 ++++++++++++++++---------------- internal/config/settings.go | 8 ++++---- 4 files changed, 29 insertions(+), 27 deletions(-) diff --git a/cmd/data-aggregation-api/main.go b/cmd/data-aggregation-api/main.go index ba38607..3075382 100644 --- a/cmd/data-aggregation-api/main.go +++ b/cmd/data-aggregation-api/main.go @@ -59,9 +59,9 @@ func run() error { // Configure LDAP timeout if config.Cfg.Authentication.LDAP != nil { if config.Cfg.Authentication.LDAP.Timeout <= 0 { - return fmt.Errorf("LDAP timeout must be greater than 0") + return fmt.Errorf("LDAP timeout must be greater than 0: %d", config.Cfg.Authentication.LDAP.Timeout) } - auth.SetDefaultTimeout(config.Cfg.Authentication.LDAP.Timeout) + auth.SetLDAPDefaultTimeout(config.Cfg.Authentication.LDAP.Timeout) } deviceRepo := device.NewSafeRepository() diff --git a/internal/api/auth/basic_auth.go b/internal/api/auth/basic_auth.go index 87f6e34..1f8554f 100644 --- a/internal/api/auth/basic_auth.go +++ b/internal/api/auth/basic_auth.go @@ -49,13 +49,15 @@ func NewBasicAuth(ctx context.Context, cfg config.AuthConfig) (BasicAuth, error) // trying a connection to LDAP to check the configuration conn, err := ldap.connect() if err != nil { - return b, fmt.Errorf("test LDAP connection: %w", err) + log.Warn().Err(err).Msg("test connection to LDAP failed") } - if err := conn.Close(); err != nil { - log.Warn().Err(err).Msg("failed to close LDAP test connection") + if conn != nil { + if err := conn.Close(); err != nil { + log.Warn().Err(err).Msg("failed to close LDAP test connection") + } } - if err := ldap.StartWorkers(ctx, cfg.LDAP.MaxWorkers); err != nil { + if err := ldap.StartAuthenticationWorkers(ctx, cfg.LDAP.WorkersCount); err != nil { return b, fmt.Errorf("failed to start LDAP workers: %w", err) } diff --git a/internal/api/auth/ldap.go b/internal/api/auth/ldap.go index 024a743..ee1977e 100644 --- a/internal/api/auth/ldap.go +++ b/internal/api/auth/ldap.go @@ -49,16 +49,17 @@ func NewLDAPAuth(ldapURL string, bindDN string, password string, baseDN string, } } -func SetDefaultTimeout(timeout time.Duration) { +func SetLDAPDefaultTimeout(timeout time.Duration) { ldap.DefaultTimeout = timeout //nolint:reassign // we want to customize the default timeout } -func (l *LDAPAuth) StartWorkers(ctx context.Context, maxWorker int) error { - if maxWorker <= 0 { - return fmt.Errorf("maxWorker must be greater than 0") +// StartAuthenticationWorkers starts a pool of workers that will handle the authentication requests. +func (l *LDAPAuth) StartAuthenticationWorkers(ctx context.Context, workersCount int) error { + if workersCount <= 0 { + return fmt.Errorf("'WorkersCount' must be greater than 0: %d", workersCount) } - for i := 0; i < maxWorker; i++ { - go l.spawnWorker(ctx) + for i := 0; i < workersCount; i++ { + go l.spawnConnectionWorker(ctx) } return nil } @@ -72,11 +73,12 @@ func (l *LDAPAuth) AuthenticateUser(username string, password string) bool { return <-req.authResp } -func (l *LDAPAuth) spawnWorker(ctx context.Context) { +func (l *LDAPAuth) spawnConnectionWorker(ctx context.Context) { const maxRetry = 1 + const maxReusageDuration = time.Minute var conn *ldap.Conn var err error - tick := time.NewTicker(time.Minute) + tick := time.NewTicker(maxReusageDuration) for { select { @@ -99,6 +101,7 @@ func (l *LDAPAuth) spawnWorker(ctx context.Context) { // bind with the user credentials var connState connectionStatus + // ctxTimeout, cancel := context.WithDeadline(ctx, time.Now().Add(l.maxConnectionLifetime)) auth, connState = l.authenticateWithTimeout(ctx, conn, req.username, req.password, ldap.DefaultTimeout) if connState == connectionClosed { @@ -116,17 +119,12 @@ func (l *LDAPAuth) spawnWorker(ctx context.Context) { log.Debug().Msgf("worker LDAP authentication attempt number %d, result: %t", retry, auth) req.authResp <- auth - tick.Reset(time.Minute) + tick.Reset(maxReusageDuration) case <-tick.C: // close connection if no request has been made for a minute log.Debug().Msg("timer reached, closing connection") - if conn != nil { - if err := conn.Close(); err != nil { - log.Error().Err(err).Msg("unable to close the LDAP connection") - } - } - return + closeLDAPConnection(conn) case <-ctx.Done(): // gracefully close connection if context is done @@ -139,14 +137,16 @@ func (l *LDAPAuth) spawnWorker(ctx context.Context) { func closeLDAPConnection(conn *ldap.Conn) { if conn != nil && !conn.IsClosing() { - _ = conn.Close() + if err := conn.Close(); err != nil { + log.Error().Err(err).Msg("unable to close the LDAP connection") + } } } // authenticateWithTimeout performs the authentication against LDAP with a timeout. func (l *LDAPAuth) authenticateWithTimeout(ctx context.Context, conn *ldap.Conn, username, password string, timeout time.Duration) (bool, connectionStatus) { // request the authentication - res := make(chan result) + res := make(chan result, 1) go func() { a, c := l.authenticate(conn, username, password) res <- result{auth: a, conn: c} diff --git a/internal/config/settings.go b/internal/config/settings.go index b63b347..b108d95 100644 --- a/internal/config/settings.go +++ b/internal/config/settings.go @@ -15,8 +15,8 @@ const ( defaultListenPort = 8080 localPath = "." - defaultMaxLDAPWorkers = 10 - defaultLDAPTimeout = 10 * time.Second + defaultLDAPWorkersCount = 10 + defaultLDAPTimeout = 10 * time.Second SiteFilter Filter = "site" SiteGroupFilter Filter = "site_group" @@ -60,7 +60,7 @@ type LDAPConfig struct { Password string Timeout time.Duration InsecureSkipVerify bool - MaxWorkers int + WorkersCount int } func setDefaults() { @@ -83,7 +83,7 @@ func setDefaults() { viper.SetDefault("Authentication.LDAP.BindDN", "") viper.SetDefault("Authentication.LDAP.Password", "") viper.SetDefault("Authentication.LDAP.InsecureSkipVerify", false) - viper.SetDefault("Authentication.LDAP.MaxWorkers", defaultMaxLDAPWorkers) + viper.SetDefault("Authentication.LDAP.WorkersCount", defaultLDAPWorkersCount) viper.SetDefault("Authentication.LDAP.Timeout", defaultLDAPTimeout) } From d78ab1fdf5ef8a08b7869bd63a621bd0cda9bf60 Mon Sep 17 00:00:00 2001 From: Kevin Petremann Date: Wed, 30 Aug 2023 12:18:05 +0200 Subject: [PATCH 6/8] feat(ldap): configurable max connection lifetime Max connection lifetime is the maximum time a LDAP connection will be re-used for authentication requests. Default is 1 minute. This is not a guarantee: - the LDAP could close the connection - the TCP connection could be closed (timeout if the max connection lifetime is too high, issues etc...) --- internal/api/auth/basic_auth.go | 1 + internal/api/auth/ldap.go | 26 +++++++++++++++++--------- internal/config/settings.go | 16 +++++++++------- 3 files changed, 27 insertions(+), 16 deletions(-) diff --git a/internal/api/auth/basic_auth.go b/internal/api/auth/basic_auth.go index 1f8554f..57e5f37 100644 --- a/internal/api/auth/basic_auth.go +++ b/internal/api/auth/basic_auth.go @@ -44,6 +44,7 @@ func NewBasicAuth(ctx context.Context, cfg config.AuthConfig) (BasicAuth, error) if err := b.configureLdap(ldap); err != nil { return b, fmt.Errorf("failed to configure the request authenticator: %w", err) } + ldap.SetMaxConnectionLifetime(cfg.LDAP.MaxConnectionLifetime) b.mode = ldapMode // trying a connection to LDAP to check the configuration diff --git a/internal/api/auth/ldap.go b/internal/api/auth/ldap.go index ee1977e..791d2e8 100644 --- a/internal/api/auth/ldap.go +++ b/internal/api/auth/ldap.go @@ -30,12 +30,13 @@ type result struct { } type LDAPAuth struct { - tlsConfig *tls.Config - reqCh chan authRequest - ldapURL string - bindDN string - password string - baseDN string + tlsConfig *tls.Config + reqCh chan authRequest + ldapURL string + bindDN string + password string + baseDN string + maxConnectionLifetime time.Duration } func NewLDAPAuth(ldapURL string, bindDN string, password string, baseDN string, tlsConfig *tls.Config) *LDAPAuth { @@ -53,6 +54,14 @@ func SetLDAPDefaultTimeout(timeout time.Duration) { ldap.DefaultTimeout = timeout //nolint:reassign // we want to customize the default timeout } +// SetMaxConnectionLifetime sets the maximum lifetime of a connection. +// +// The maximum lifetime is the maximum amount of time a connection may be reused for. +// This is not a guarantee, as the connection may have been closed by the server before reaching that timer. +func (l *LDAPAuth) SetMaxConnectionLifetime(maxConnectionLifetime time.Duration) { + l.maxConnectionLifetime = maxConnectionLifetime +} + // StartAuthenticationWorkers starts a pool of workers that will handle the authentication requests. func (l *LDAPAuth) StartAuthenticationWorkers(ctx context.Context, workersCount int) error { if workersCount <= 0 { @@ -75,10 +84,9 @@ func (l *LDAPAuth) AuthenticateUser(username string, password string) bool { func (l *LDAPAuth) spawnConnectionWorker(ctx context.Context) { const maxRetry = 1 - const maxReusageDuration = time.Minute var conn *ldap.Conn var err error - tick := time.NewTicker(maxReusageDuration) + tick := time.NewTicker(l.maxConnectionLifetime) for { select { @@ -119,7 +127,7 @@ func (l *LDAPAuth) spawnConnectionWorker(ctx context.Context) { log.Debug().Msgf("worker LDAP authentication attempt number %d, result: %t", retry, auth) req.authResp <- auth - tick.Reset(maxReusageDuration) + tick.Reset(l.maxConnectionLifetime) case <-tick.C: // close connection if no request has been made for a minute diff --git a/internal/config/settings.go b/internal/config/settings.go index b108d95..368dc9b 100644 --- a/internal/config/settings.go +++ b/internal/config/settings.go @@ -54,13 +54,14 @@ type AuthConfig struct { } type LDAPConfig struct { - URL string - BaseDN string - BindDN string - Password string - Timeout time.Duration - InsecureSkipVerify bool - WorkersCount int + URL string + BaseDN string + BindDN string + Password string + Timeout time.Duration + MaxConnectionLifetime time.Duration + InsecureSkipVerify bool + WorkersCount int } func setDefaults() { @@ -85,6 +86,7 @@ func setDefaults() { viper.SetDefault("Authentication.LDAP.InsecureSkipVerify", false) viper.SetDefault("Authentication.LDAP.WorkersCount", defaultLDAPWorkersCount) viper.SetDefault("Authentication.LDAP.Timeout", defaultLDAPTimeout) + viper.SetDefault("Authentication.LDAP.MaxConnectionLifetime", time.Minute) } func LoadConfig() error { From c8f487b5390921f3b4fd3189ae5feb1dc4c20b15 Mon Sep 17 00:00:00 2001 From: Kevin Petremann Date: Fri, 1 Sep 2023 12:06:42 +0200 Subject: [PATCH 7/8] refactor(ldap): timeout via context --- internal/api/auth/ldap.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/internal/api/auth/ldap.go b/internal/api/auth/ldap.go index 791d2e8..8e3b10a 100644 --- a/internal/api/auth/ldap.go +++ b/internal/api/auth/ldap.go @@ -3,6 +3,7 @@ package auth import ( "context" "crypto/tls" + "errors" "fmt" "time" @@ -13,6 +14,8 @@ import ( type connectionStatus bool +var ErrLDAPTimeout = errors.New("LDAP timeout") + const ( connectionClosed connectionStatus = false connectionUp connectionStatus = true @@ -109,8 +112,9 @@ func (l *LDAPAuth) spawnConnectionWorker(ctx context.Context) { // bind with the user credentials var connState connectionStatus - // ctxTimeout, cancel := context.WithDeadline(ctx, time.Now().Add(l.maxConnectionLifetime)) - auth, connState = l.authenticateWithTimeout(ctx, conn, req.username, req.password, ldap.DefaultTimeout) + ctxTimeout, cancel := context.WithTimeoutCause(ctx, ldap.DefaultTimeout, ErrLDAPTimeout) + auth, connState = l.authenticateWithTimeout(ctxTimeout, conn, req.username, req.password) + cancel() if connState == connectionClosed { log.Debug().Msg("LDAP connection was closed by the server, closing on client side") @@ -152,7 +156,7 @@ func closeLDAPConnection(conn *ldap.Conn) { } // authenticateWithTimeout performs the authentication against LDAP with a timeout. -func (l *LDAPAuth) authenticateWithTimeout(ctx context.Context, conn *ldap.Conn, username, password string, timeout time.Duration) (bool, connectionStatus) { +func (l *LDAPAuth) authenticateWithTimeout(ctx context.Context, conn *ldap.Conn, username, password string) (bool, connectionStatus) { // request the authentication res := make(chan result, 1) go func() { @@ -168,13 +172,14 @@ func (l *LDAPAuth) authenticateWithTimeout(ctx context.Context, conn *ldap.Conn, case r := <-res: auth = r.auth connState = r.conn - case <-time.After(timeout): - log.Error().Msg("LDAP authentication timeout") - closeLDAPConnection(conn) - auth = false - connState = connectionClosed case <-ctx.Done(): auth = false + if errors.Is(context.Cause(ctx), ErrLDAPTimeout) { + log.Error().Msg("LDAP authentication timeout") + closeLDAPConnection(conn) + } else { + connState = connectionClosed + } } return auth, connState } From 2c8153ae7f550859084db1062d14d41932b8a0c6 Mon Sep 17 00:00:00 2001 From: Kevin Petremann Date: Thu, 21 Sep 2023 10:09:41 +0200 Subject: [PATCH 8/8] refactor(ldap): some improvements --- internal/api/auth/ldap.go | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/internal/api/auth/ldap.go b/internal/api/auth/ldap.go index 8e3b10a..8276aa2 100644 --- a/internal/api/auth/ldap.go +++ b/internal/api/auth/ldap.go @@ -86,7 +86,7 @@ func (l *LDAPAuth) AuthenticateUser(username string, password string) bool { } func (l *LDAPAuth) spawnConnectionWorker(ctx context.Context) { - const maxRetry = 1 + const maxAttempts = 3 var conn *ldap.Conn var err error tick := time.NewTicker(l.maxConnectionLifetime) @@ -95,10 +95,10 @@ func (l *LDAPAuth) spawnConnectionWorker(ctx context.Context) { select { case req := <-l.reqCh: auth := false - retry := 0 - for retry <= maxRetry+1 { - retry++ - log.Debug().Msgf("worker LDAP authentication attempt number %d", retry) + attempt := 1 + for attempt <= maxAttempts { + attempt++ + log.Debug().Msgf("worker LDAP authentication attempt number %d", attempt) // (re)connect if needed if conn == nil || conn.IsClosing() { log.Debug().Msg("LDAP connection is closed, reconnecting") @@ -112,9 +112,12 @@ func (l *LDAPAuth) spawnConnectionWorker(ctx context.Context) { // bind with the user credentials var connState connectionStatus - ctxTimeout, cancel := context.WithTimeoutCause(ctx, ldap.DefaultTimeout, ErrLDAPTimeout) - auth, connState = l.authenticateWithTimeout(ctxTimeout, conn, req.username, req.password) - cancel() + func() { + // this anonymous function ensures the context is released as soon as possible (because of the for loop) + ctxTimeout, cancel := context.WithTimeoutCause(ctx, ldap.DefaultTimeout, ErrLDAPTimeout) + defer cancel() + auth, connState = l.authenticateWithTimeout(ctxTimeout, conn, req.username, req.password) + }() if connState == connectionClosed { log.Debug().Msg("LDAP connection was closed by the server, closing on client side") @@ -128,13 +131,13 @@ func (l *LDAPAuth) spawnConnectionWorker(ctx context.Context) { } } - log.Debug().Msgf("worker LDAP authentication attempt number %d, result: %t", retry, auth) + log.Debug().Msgf("worker LDAP authentication attempt number %d, result: %t", attempt, auth) req.authResp <- auth tick.Reset(l.maxConnectionLifetime) case <-tick.C: - // close connection if no request has been made for a minute + // close connection if no request has been made log.Debug().Msg("timer reached, closing connection") closeLDAPConnection(conn) @@ -195,7 +198,7 @@ func (l *LDAPAuth) connect() (*ldap.Conn, error) { // authenticate performs the authentication against LDAP. // The first returned boolean is true if the authentication is successful, false otherwise. -// The second returned boolean is true if the connection is closed, false otherwise. +// The second returned boolean is false if the connection is closed, true otherwise. func (l *LDAPAuth) authenticate(conn *ldap.Conn, username string, password string) (bool, connectionStatus) { if err := conn.Bind(l.bindDN, l.password); err != nil { log.Error().Err(err).Str("bindDN", l.bindDN).Msg("failed to bind to LDAP")