From 652661149812d71f39a8f4795265ddfdb501f24d Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Thu, 14 Nov 2024 15:04:48 -0700 Subject: [PATCH] Improve troubleshooting for LDAP authentication errors (#48947) This introduces two small changes: 1. Use an aggregate error to make sure the original error is included along with our attempt at providing a better error message. This change should help distinguish between bad authn/invalid cert and valid authentication but insufficient user permissions. 2. Make the CRL Distribution Point in Windows certs optional. This metadata is required in the certs we issue for users to RDP, but it doesn't need to be present in the certs we use to authenticate our service account. The problem with including it when it is not needed is it causes windows to perform a revocation check and log a failure, which can lead to wasted time when troubleshooting. --- lib/auth/desktop.go | 9 ++++++-- lib/auth/windows/ldap.go | 6 ++--- lib/auth/windows/windows.go | 38 ++++++++++++++++++++----------- lib/srv/desktop/windows_server.go | 5 +++- tool/tctl/common/auth_command.go | 3 +++ 5 files changed, 42 insertions(+), 19 deletions(-) diff --git a/lib/auth/desktop.go b/lib/auth/desktop.go index f730c3e7c3610..664a055eb7846 100644 --- a/lib/auth/desktop.go +++ b/lib/auth/desktop.go @@ -71,8 +71,13 @@ func (a *Server) GenerateWindowsDesktopCert(ctx context.Context, req *proto.Wind NotAfter: a.clock.Now().UTC().Add(req.TTL.Get()), ExtraExtensions: csr.Extensions, KeyUsage: x509.KeyUsageDigitalSignature, - // CRL is required for Windows smartcard certs. - CRLDistributionPoints: []string{req.CRLEndpoint}, + } + + // CRL Distribution Points (CDP) are required for Windows smartcard certs + // for users wanting to RDP. They are not required for the service account + // cert that Teleport itself uses to authenticate for LDAP. + if req.CRLEndpoint != "" { + certReq.CRLDistributionPoints = []string{req.CRLEndpoint} } limitExceeded, err := a.desktopsLimitExceeded(ctx) diff --git a/lib/auth/windows/ldap.go b/lib/auth/windows/ldap.go index 4970500c34628..195dd14f67606 100644 --- a/lib/auth/windows/ldap.go +++ b/lib/auth/windows/ldap.go @@ -184,9 +184,9 @@ func convertLDAPError(err error) error { return trace.ConnectionProblem(err, "network error") case ldap.LDAPResultOperationsError: if strings.Contains(err.Error(), "successful bind must be completed") { - return trace.AccessDenied( - "the LDAP server did not accept Teleport's client certificate, " + - "has the Teleport CA been imported correctly?") + return trace.NewAggregate(trace.AccessDenied( + "the LDAP server did not accept Teleport's client certificate, "+ + "has the Teleport CA been imported correctly?"), err) } case ldap.LDAPResultEntryAlreadyExists: return trace.AlreadyExists("LDAP object already exists: %v", err) diff --git a/lib/auth/windows/windows.go b/lib/auth/windows/windows.go index 5b198833410b5..dcf20188469bd 100644 --- a/lib/auth/windows/windows.go +++ b/lib/auth/windows/windows.go @@ -128,19 +128,26 @@ func getCertRequest(req *GenerateCredentialsRequest) (*certRequest, error) { return nil, trace.Wrap(err) } csrPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csrBytes}) - // Note: this CRL DN may or may not be the same DN published in updateCRL. - // - // There can be multiple AD domains connected to Teleport. Each - // windows_desktop_service is connected to a single AD domain and publishes - // CRLs in it. Each service can also handle RDP connections for a different - // domain, with the assumption that some other windows_desktop_service - // published a CRL there. - crlDN := crlDN(req.ClusterName, req.LDAPConfig, req.CAType) - return &certRequest{ - csrPEM: csrPEM, - crlEndpoint: fmt.Sprintf("ldap:///%s?certificateRevocationList?base?objectClass=cRLDistributionPoint", crlDN), - keyDER: keyDER, - }, nil + cr := &certRequest{ + csrPEM: csrPEM, + keyDER: keyDER, + } + + if !req.OmitCDP { + // Note: this CRL DN may or may not be the same DN published in updateCRL. + // + // There can be multiple AD domains connected to Teleport. Each + // windows_desktop_service is connected to a single AD domain and publishes + // CRLs in it. Each service can also handle RDP connections for a different + // domain, with the assumption that some other windows_desktop_service + // published a CRL there. + crlDN := crlDN(req.ClusterName, req.LDAPConfig, req.CAType) + + // TODO(zmb3) consider making Teleport itself the CDP (via HTTP) instead of LDAP + cr.crlEndpoint = fmt.Sprintf("ldap:///%s?certificateRevocationList?base?objectClass=cRLDistributionPoint", crlDN) + } + + return cr, nil } // AuthInterface is a subset of auth.ClientI @@ -181,6 +188,11 @@ type GenerateCredentialsRequest struct { CreateUser bool // Groups are groups that user should be member of Groups []string + + // OmitCDP can be used to prevent Teleport from issuing certs with a + // CRL Distribution Point (CDP). CDPs are required in user certificates + // for RDP, but they can be omitted for certs that are used for LDAP binds. + OmitCDP bool } // GenerateWindowsDesktopCredentials generates a private key / certificate pair for the given diff --git a/lib/srv/desktop/windows_server.go b/lib/srv/desktop/windows_server.go index d80c0dd2470b6..44e2ebd148d9a 100644 --- a/lib/srv/desktop/windows_server.go +++ b/lib/srv/desktop/windows_server.go @@ -443,6 +443,7 @@ func (s *WindowsService) tlsConfigForLDAP() (*tls.Config, error) { domain: s.cfg.Domain, ttl: windowsDesktopServiceCertTTL, activeDirectorySID: s.cfg.SID, + omitCDP: true, }) if err != nil { return nil, trace.Wrap(err) @@ -1226,7 +1227,8 @@ type generateCredentialsRequest struct { // createUser specifies if Windows user should be created if missing createUser bool // groups are groups that user should be member of - groups []string + groups []string + omitCDP bool } // generateCredentials generates a private key / certificate pair for the given @@ -1254,6 +1256,7 @@ func (s *WindowsService) generateCredentials(ctx context.Context, request genera AuthClient: s.cfg.AuthClient, CreateUser: request.createUser, Groups: request.groups, + OmitCDP: request.omitCDP, }) } diff --git a/tool/tctl/common/auth_command.go b/tool/tctl/common/auth_command.go index e02632d118f02..cb7d47bcb27a3 100644 --- a/tool/tctl/common/auth_command.go +++ b/tool/tctl/common/auth_command.go @@ -79,6 +79,7 @@ type AuthCommand struct { windowsDomain string windowsPKIDomain string windowsSID string + omitCDP bool signOverwrite bool password string caType string @@ -150,6 +151,7 @@ func (a *AuthCommand) Initialize(app *kingpin.Application, config *servicecfg.Co a.authSign.Flag("windows-domain", `Active Directory domain for which this cert is valid. Only used when --format is set to "windows"`).StringVar(&a.windowsDomain) a.authSign.Flag("windows-pki-domain", `Active Directory domain where CRLs will be located. Only used when --format is set to "windows"`).StringVar(&a.windowsPKIDomain) a.authSign.Flag("windows-sid", `Optional Security Identifier to embed in the certificate. Only used when --format is set to "windows"`).StringVar(&a.windowsSID) + a.authSign.Flag("omit-cdp", `Omit CRL Distribution Points from the cert. Only used when --format is set to "windows"`).BoolVar(&a.omitCDP) a.authRotate = auth.Command("rotate", "Rotate certificate authorities in the cluster.") a.authRotate.Flag("grace-period", "Grace period keeps previous certificate authorities signatures valid, if set to 0 will force users to re-login and nodes to re-register."). @@ -363,6 +365,7 @@ func (a *AuthCommand) generateWindowsCert(ctx context.Context, clusterAPI certif TTL: a.genTTL, ClusterName: cn.GetClusterName(), LDAPConfig: windows.LDAPConfig{Domain: domain}, + OmitCDP: a.omitCDP, AuthClient: clusterAPI, }) if err != nil {