From 740083a3954160846bff818a7459db81053fc62b Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Tue, 16 May 2023 09:39:49 -0600 Subject: [PATCH 1/3] Change the filters used to query a Windows user's SID The sAMAccount type is always indexed, so this is a more efficient query than using the object class attribute. --- lib/auth/windows/ldap.go | 10 ++++++---- lib/srv/desktop/windows_server.go | 17 ++++++++--------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/lib/auth/windows/ldap.go b/lib/auth/windows/ldap.go index f896e188130f2..1e0984329a94c 100644 --- a/lib/auth/windows/ldap.go +++ b/lib/auth/windows/ldap.go @@ -91,16 +91,18 @@ const ( ClassContainer = "container" // ClassGMSA is the object class for group managed service accounts in Active Directory. ClassGMSA = "msDS-GroupManagedServiceAccount" - // ClassUser is the object class for users in Active Directory - ClassUser = "user" - // CategoryPerson is object category for persons in Active Directory - CategoryPerson = "person" + // AccountTypeUser is the SAM account type for user accounts. + // See https://learn.microsoft.com/en-us/windows/win32/adschema/a-samaccounttype + // (SAM_USER_OBJECT) + AccountTypeUser = "805306368" // AttrName is the name of an LDAP object AttrName = "name" // AttrSAMAccountName is the SAM Account name of an LDAP object AttrSAMAccountName = "sAMAccountName" + // AttrSAMAccountType is the SAM Account type for an LDAP object + AttrSAMAccountType = "sAMAccountType" // AttrCommonName is the common name of an LDAP object, or "CN" AttrCommonName = "cn" // AttrDistinguishedName is the distinguished name of an LDAP object, or "DN" diff --git a/lib/srv/desktop/windows_server.go b/lib/srv/desktop/windows_server.go index 429f788916a7b..4cb5a279b7eb3 100644 --- a/lib/srv/desktop/windows_server.go +++ b/lib/srv/desktop/windows_server.go @@ -1112,28 +1112,27 @@ func (s *WindowsService) generateUserCert(ctx context.Context, username string, var activeDirectorySID string if !desktop.NonAD() { // Find the user's SID - s.cfg.Log.Debugf("querying LDAP for objectSid of Windows username: %v", username) - filters := []string{ - fmt.Sprintf("(%s=%s)", windows.AttrObjectCategory, windows.CategoryPerson), - fmt.Sprintf("(%s=%s)", windows.AttrObjectClass, windows.ClassUser), + filter := windows.CombineLDAPFilters([]string{ + fmt.Sprintf("(%s=%s)", windows.AttrSAMAccountType, windows.AccountTypeUser), fmt.Sprintf("(%s=%s)", windows.AttrSAMAccountName, username), - } + }) + s.cfg.Log.Debugf("querying LDAP for objectSid of Windows username %q with filter %v", username, filter) - entries, err := s.lc.ReadWithFilter(s.cfg.LDAPConfig.DomainDN(), windows.CombineLDAPFilters(filters), []string{windows.AttrObjectSid}) + entries, err := s.lc.ReadWithFilter(s.cfg.LDAPConfig.DomainDN(), filter, []string{windows.AttrObjectSid}) // if LDAP-based desktop discovery is not enabled, there may not be enough // traffic to keep the connection open. Attempt to open a new LDAP connection // in this case. if trace.IsConnectionProblem(err) { s.initializeLDAP() // ignore error, this is a best effort attempt - entries, err = s.lc.ReadWithFilter(s.cfg.LDAPConfig.DomainDN(), windows.CombineLDAPFilters(filters), []string{windows.AttrObjectSid}) + entries, err = s.lc.ReadWithFilter(s.cfg.LDAPConfig.DomainDN(), filter, []string{windows.AttrObjectSid}) } if err != nil { return nil, nil, trace.Wrap(err) } if len(entries) == 0 { - return nil, nil, trace.NotFound("LDAP failed to return objectSid for Windows username: %v", username) + return nil, nil, trace.NotFound("could not find Windows account %q", username) } else if len(entries) > 1 { - s.cfg.Log.Warnf("LDAP unexpectedly returned multiple entries for objectSid for username: %v, taking the first", username) + s.cfg.Log.Warnf("found multiple entries for username %q, taking the first", username) } activeDirectorySID, err = windows.ADSIDStringFromLDAPEntry(entries[0]) if err != nil { From db667edef772ca846caf6cd2574ab3f7f052b18a Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Tue, 16 May 2023 09:40:58 -0600 Subject: [PATCH 2/3] Don't attempt DNS resolution for an empty hostname If we discover LDAP entries with a mising hostname, just skip over them. This generates less noise in the logs. --- lib/srv/desktop/discovery.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/srv/desktop/discovery.go b/lib/srv/desktop/discovery.go index 6d26dcfd3fead..c2bf630524b78 100644 --- a/lib/srv/desktop/discovery.go +++ b/lib/srv/desktop/discovery.go @@ -194,9 +194,9 @@ func (s *WindowsService) lookupDesktop(ctx context.Context, hostname string) (ad return addrs, nil } if s.dnsResolver == nil { - return nil, trace.NewAggregate(err, trace.Errorf("DNS lookup for %v failed and there's no LDAP server to fallback to", hostname)) + return nil, trace.NewAggregate(err, trace.Errorf("DNS lookup for %q failed and there's no LDAP server to fallback to", hostname)) } - s.cfg.Log.WithError(err).Debugf("DNS lookup for %v failed, falling back to LDAP server", hostname) + s.cfg.Log.WithError(err).Debugf("DNS lookup for %q failed, falling back to LDAP server", hostname) return s.dnsResolver.LookupHost(ctx, hostname) } @@ -204,6 +204,9 @@ func (s *WindowsService) lookupDesktop(ctx context.Context, hostname string) (ad // from an LDAP search result func (s *WindowsService) ldapEntryToWindowsDesktop(ctx context.Context, entry *ldap.Entry, getHostLabels func(string) map[string]string) (types.ResourceWithLabels, error) { hostname := entry.GetAttributeValue(windows.AttrDNSHostName) + if hostname == "" { + return nil, trace.BadParameter("LDAP entry missing hostname, has attributes: %v", entry.Attributes) + } labels := getHostLabels(hostname) labels[types.TeleportNamespace+"/windows_domain"] = s.cfg.Domain s.applyLabelsFromLDAP(entry, labels) From 53243b81ce264154c9221cb74a6e1ccaac0fc377 Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Tue, 16 May 2023 09:41:36 -0600 Subject: [PATCH 3/3] Improve desktop error messaging Prefer a user-friendly message over "RDP Connection Failed" where possible. --- lib/srv/desktop/windows_server.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/srv/desktop/windows_server.go b/lib/srv/desktop/windows_server.go index 4cb5a279b7eb3..d3a9ef01ee699 100644 --- a/lib/srv/desktop/windows_server.go +++ b/lib/srv/desktop/windows_server.go @@ -783,7 +783,11 @@ func (s *WindowsService) handleConnection(proxyConn *tls.Conn) { if err := s.connectRDP(ctx, log, tdpConn, desktop, authContext); err != nil { log.Errorf("RDP connection failed: %v", err) - sendTDPError("RDP connection failed.") + msg := "RDP connection failed." + if um, ok := err.(trace.UserMessager); ok { + msg = um.UserMessage() + } + sendTDPError(msg) return } }