From ad2e9ff583d39c2180d81b3617efaf1e8b33cd58 Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Thu, 4 Jan 2024 08:59:37 -0700 Subject: [PATCH 1/4] Always attempt desktop discovery, even if LDAP is not ready If Teleport loses it's connection to the LDAP server, it will attempt to initiate a new condition when: 1. The user tries to connect to a desktop and Teleport fails to obtain the user's SID. 2. The periodic desktop discovery routine attempts to search LDAP for desktops. In some circumstances, #2 never gets the chance to apply, since discovery is skipped when LDAP is not ready. Additionally, if LDAP is not ready, then you can't connect to a desktop, so #1 can't happen either, which means Teleport won't connect again until it is restarted. --- lib/srv/desktop/discovery.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/srv/desktop/discovery.go b/lib/srv/desktop/discovery.go index c44bcc0a9cabb..6050309b35bbe 100644 --- a/lib/srv/desktop/discovery.go +++ b/lib/srv/desktop/discovery.go @@ -99,11 +99,6 @@ func (s *WindowsService) ldapSearchFilter() string { // getDesktopsFromLDAP discovers Windows hosts via LDAP func (s *WindowsService) getDesktopsFromLDAP() map[string]types.WindowsDesktop { - if !s.ldapReady() { - s.cfg.Logger.WarnContext(context.Background(), "skipping desktop discovery: LDAP not yet initialized") - return nil - } - filter := s.ldapSearchFilter() s.cfg.Logger.DebugContext(context.Background(), "searching for desktops", "filter", filter) @@ -250,7 +245,11 @@ func (s *WindowsService) lookupDesktop(ctx context.Context, hostname string) ([] // ldapEntryToWindowsDesktop generates the Windows Desktop resource // from an LDAP search result -func (s *WindowsService) ldapEntryToWindowsDesktop(ctx context.Context, entry *ldap.Entry, getHostLabels func(string) map[string]string) (types.WindowsDesktop, error) { +func (s *WindowsService) ldapEntryToWindowsDesktop( + ctx context.Context, + entry *ldap.Entry, + getHostLabels func(string) map[string]string, +) (types.WindowsDesktop, error) { hostname := entry.GetAttributeValue(windows.AttrDNSHostName) if hostname == "" { attrs := make([]string, len(entry.Attributes)) From d5a2b166084f95d643703f3b7df648a8469004da Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Thu, 4 Jan 2024 09:18:29 -0700 Subject: [PATCH 2/4] Periodically use the LDAP connection when discovery is not enabled If LDAP-based discovery is not enabled then we may go long periods of time without trying to use the LDAP connection, which prevents us from detecting disconnects (and restoring the connection) in a timely manner. When discovery is disabled, perform a read every 5 minutes and reconnect if we detect a connection problem. --- lib/srv/desktop/windows_server.go | 34 +++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/lib/srv/desktop/windows_server.go b/lib/srv/desktop/windows_server.go index 791f477861666..1ee9db1a36437 100644 --- a/lib/srv/desktop/windows_server.go +++ b/lib/srv/desktop/windows_server.go @@ -425,6 +425,34 @@ func NewWindowsService(cfg WindowsServiceConfig) (*WindowsService, error) { s.cfg.Logger.InfoContext(ctx, "desktop discovery via LDAP is disabled, set 'base_dn' to enable") } + // if LDAP-based discovery is not enabled, but we have configured LDAP + // then it's important that we periodically try to use the LDAP connection + // to detect connection closure + if s.ldapConfigured && len(s.cfg.DiscoveryBaseDN) == 0 { + s.cfg.Log.Debugln("starting LDAP connection checker") + go func() { + t := s.cfg.Clock.NewTicker(5 * time.Minute) + defer t.Stop() + + for { + select { + case <-t.Chan(): + // attempt to read CAs in the NTAuth store (we know we have permissions to do so) + ntAuthDN := "CN=NTAuthCertificates,CN=Public Key Services,CN=Services,CN=Configuration," + s.cfg.LDAPConfig.DomainDN() + _, err := s.lc.Read(ntAuthDN, "certificationAuthority", []string{"cACertificate"}) + if trace.IsConnectionProblem(err) { + s.cfg.Log.Infoln("reconnecting to LDAP server") + if err := s.initializeLDAP(); err != nil { + s.cfg.Log.Warnf("failed to reconnect to LDAP: %v", err) + } + } + case <-ctx.Done(): + return + } + } + }() + } + ok = true return s, nil } @@ -693,12 +721,6 @@ func (s *WindowsService) readyForConnections() bool { return s.ldapInitialized } -func (s *WindowsService) ldapReady() bool { - s.mu.Lock() - defer s.mu.Unlock() - return s.ldapInitialized -} - // handleConnection handles TLS connections from a Teleport proxy. // It authenticates and authorizes the connection, and then begins // translating the TDP messages from the proxy into native RDP. From d3e4b2d5b89462fb42c5064b72bb4ebd57ec097d Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Tue, 13 Feb 2024 10:51:56 -0700 Subject: [PATCH 3/4] Address review comments --- lib/srv/desktop/windows_server.go | 53 ++++++++++++++++++------------- 1 file changed, 31 insertions(+), 22 deletions(-) diff --git a/lib/srv/desktop/windows_server.go b/lib/srv/desktop/windows_server.go index 1ee9db1a36437..77b272acd0696 100644 --- a/lib/srv/desktop/windows_server.go +++ b/lib/srv/desktop/windows_server.go @@ -429,34 +429,43 @@ func NewWindowsService(cfg WindowsServiceConfig) (*WindowsService, error) { // then it's important that we periodically try to use the LDAP connection // to detect connection closure if s.ldapConfigured && len(s.cfg.DiscoveryBaseDN) == 0 { - s.cfg.Log.Debugln("starting LDAP connection checker") - go func() { - t := s.cfg.Clock.NewTicker(5 * time.Minute) - defer t.Stop() - - for { - select { - case <-t.Chan(): - // attempt to read CAs in the NTAuth store (we know we have permissions to do so) - ntAuthDN := "CN=NTAuthCertificates,CN=Public Key Services,CN=Services,CN=Configuration," + s.cfg.LDAPConfig.DomainDN() - _, err := s.lc.Read(ntAuthDN, "certificationAuthority", []string{"cACertificate"}) - if trace.IsConnectionProblem(err) { - s.cfg.Log.Infoln("reconnecting to LDAP server") - if err := s.initializeLDAP(); err != nil { - s.cfg.Log.Warnf("failed to reconnect to LDAP: %v", err) - } - } - case <-ctx.Done(): - return - } - } - }() + s.startLDAPConnectionCheck(ctx) } ok = true return s, nil } +// startLDAPConnectionCheck starts a background process that +// periodically reads from the LDAP connection in order to detect +// connection closure, and reconnects if necessary. +// This is useful when LDAP-based discovery is disabled, because without +// discovery the connection goes idle and may be closed by the server. +func (s *WindowsService) startLDAPConnectionCheck(ctx context.Context) { + s.cfg.Logger.DebugContext(ctx, "starting LDAP connection checker") + go func() { + t := s.cfg.Clock.NewTicker(5 * time.Minute) + defer t.Stop() + + for { + select { + case <-t.Chan(): + // attempt to read CAs in the NTAuth store (we know we have permissions to do so) + ntAuthDN := "CN=NTAuthCertificates,CN=Public Key Services,CN=Services,CN=Configuration," + s.cfg.LDAPConfig.DomainDN() + _, err := s.lc.Read(ntAuthDN, "certificationAuthority", []string{"cACertificate"}) + if trace.IsConnectionProblem(err) { + s.cfg.Logger.DebugContext(ctx, "detected broken LDAP connection, will reconnect") + if err := s.initializeLDAP(); err != nil { + s.cfg.Logger.WarnContext(ctx, "failed to reconnect to LDAP", "error", err) + } + } + case <-ctx.Done(): + return + } + } + }() +} + func (s *WindowsService) newSessionRecorder(recConfig types.SessionRecordingConfig, sessionID string) (libevents.SessionPreparerRecorder, error) { return recorder.New(recorder.Config{ SessionID: session.ID(sessionID), From c32308758dfbfe047fed3bce52e97a7cf9fcfc65 Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Mon, 28 Oct 2024 13:40:31 -0600 Subject: [PATCH 4/4] Fix some LDAP connection bugs In #36281 we made some improvements to the LDAP reconnect behavior. These changes considered the case where we had a connection to the LDAP server but then got disconnected. They did not consider the case where we never succesfully established a connection at all. --- lib/srv/desktop/discovery.go | 10 ++++++++++ lib/srv/desktop/windows_server.go | 15 ++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/srv/desktop/discovery.go b/lib/srv/desktop/discovery.go index 6050309b35bbe..ea65cc4fa1536 100644 --- a/lib/srv/desktop/discovery.go +++ b/lib/srv/desktop/discovery.go @@ -99,6 +99,16 @@ func (s *WindowsService) ldapSearchFilter() string { // getDesktopsFromLDAP discovers Windows hosts via LDAP func (s *WindowsService) getDesktopsFromLDAP() map[string]types.WindowsDesktop { + // Check whether we've ever successfully initialized our LDAP client. + s.mu.Lock() + if !s.ldapInitialized { + s.cfg.Logger.DebugContext(context.Background(), "LDAP not ready, skipping discovery and attempting to reconnect") + s.mu.Unlock() + s.initializeLDAP() + return nil + } + s.mu.Unlock() + filter := s.ldapSearchFilter() s.cfg.Logger.DebugContext(context.Background(), "searching for desktops", "filter", filter) diff --git a/lib/srv/desktop/windows_server.go b/lib/srv/desktop/windows_server.go index 77b272acd0696..fd75cbc89bd04 100644 --- a/lib/srv/desktop/windows_server.go +++ b/lib/srv/desktop/windows_server.go @@ -450,7 +450,20 @@ func (s *WindowsService) startLDAPConnectionCheck(ctx context.Context) { for { select { case <-t.Chan(): - // attempt to read CAs in the NTAuth store (we know we have permissions to do so) + // First check if we have successfully initialized the LDAP client. + // If not, then do that now and return. + // (This mimics the check that is performed when LDAP discovery is enabled.) + s.mu.Lock() + if !s.ldapInitialized { + s.cfg.Logger.DebugContext(context.Background(), "LDAP not ready, attempting to reconnect") + s.mu.Unlock() + s.initializeLDAP() + return + } + s.mu.Unlock() + + // If we have initizlied the LDAP client, then try to use it to make sure we're still connected + // by attempting to read CAs in the NTAuth store (we know we have permissions to do so). ntAuthDN := "CN=NTAuthCertificates,CN=Public Key Services,CN=Services,CN=Configuration," + s.cfg.LDAPConfig.DomainDN() _, err := s.lc.Read(ntAuthDN, "certificationAuthority", []string{"cACertificate"}) if trace.IsConnectionProblem(err) {