From 3f5ee12018dafe90d698327176f7c4a2a1c1f2cc Mon Sep 17 00:00:00 2001 From: Tim Ross Date: Thu, 14 Apr 2022 16:03:53 -0400 Subject: [PATCH 1/3] Prevent goroutine leak in oidc client Adds the stop channel returned from client.SyncProviderConfig to the oidcClient and closes the channel when the client is cleaned up. This causes the sync goroutine being run by go-oidc to end. --- lib/auth/auth.go | 1 + lib/auth/oidc.go | 11 +++++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/auth/auth.go b/lib/auth/auth.go index be9a7b8de7f60..809246c1e25fd 100644 --- a/lib/auth/auth.go +++ b/lib/auth/auth.go @@ -3664,6 +3664,7 @@ const ( type oidcClient struct { client *oidc.Client config oidc.ClientConfig + stop chan struct{} } // samlProvider is internal structure that stores SAML client and its config diff --git a/lib/auth/oidc.go b/lib/auth/oidc.go index db8f0dcf33288..67a4a5611097e 100644 --- a/lib/auth/oidc.go +++ b/lib/auth/oidc.go @@ -67,6 +67,7 @@ func (a *Server) getOIDCClient(conn types.OIDCConnector) (*oidc.Client, error) { return clientPack.client, nil } + close(clientPack.stop) delete(a.oidcClients, conn.GetName()) return nil, trace.NotFound("connector %v has updated the configuration and is invalidated", conn.GetName()) @@ -79,14 +80,16 @@ func (a *Server) createOIDCClient(conn types.OIDCConnector) (*oidc.Client, error return nil, trace.Wrap(err) } - doneSyncing := make(chan struct{}) + doneSyncing := make(chan chan struct{}) go func() { defer close(doneSyncing) - client.SyncProviderConfig(conn.GetIssuerURL()) + doneSyncing <- client.SyncProviderConfig(conn.GetIssuerURL()) }() + oidcClient := &oidcClient{client: client, config: config} select { - case <-doneSyncing: + case stop := <-doneSyncing: + oidcClient.stop = stop case <-time.After(defaults.WebHeadersTimeout): return nil, trace.ConnectionProblem(nil, "timed out syncing oidc connector %v, ensure URL %q is valid and accessible and check configuration", @@ -98,7 +101,7 @@ func (a *Server) createOIDCClient(conn types.OIDCConnector) (*oidc.Client, error a.lock.Lock() defer a.lock.Unlock() - a.oidcClients[conn.GetName()] = &oidcClient{client: client, config: config} + a.oidcClients[conn.GetName()] = oidcClient return client, nil } From 1b74998b8426d4c0fa65534ff81f1fff79c646a5 Mon Sep 17 00:00:00 2001 From: Tim Ross Date: Fri, 15 Apr 2022 09:08:20 -0400 Subject: [PATCH 2/3] prevent leak on server close too --- lib/auth/auth.go | 2 +- lib/auth/oidc.go | 25 +++++++++++++++++-------- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/lib/auth/auth.go b/lib/auth/auth.go index 809246c1e25fd..d1204ceac421f 100644 --- a/lib/auth/auth.go +++ b/lib/auth/auth.go @@ -3664,7 +3664,7 @@ const ( type oidcClient struct { client *oidc.Client config oidc.ClientConfig - stop chan struct{} + cancel context.CancelFunc } // samlProvider is internal structure that stores SAML client and its config diff --git a/lib/auth/oidc.go b/lib/auth/oidc.go index 67a4a5611097e..66abb3ebae431 100644 --- a/lib/auth/oidc.go +++ b/lib/auth/oidc.go @@ -67,7 +67,7 @@ func (a *Server) getOIDCClient(conn types.OIDCConnector) (*oidc.Client, error) { return clientPack.client, nil } - close(clientPack.stop) + clientPack.cancel() delete(a.oidcClients, conn.GetName()) return nil, trace.NotFound("connector %v has updated the configuration and is invalidated", conn.GetName()) @@ -80,28 +80,37 @@ func (a *Server) createOIDCClient(conn types.OIDCConnector) (*oidc.Client, error return nil, trace.Wrap(err) } - doneSyncing := make(chan chan struct{}) + // SyncProviderConfig doesn't take a context for cancellation, instead it + // returns a channel that has to be closed to stop the sync. So what we + // can do until it is changed to take a context, is to wait for the syncContext + // we create below to be done and then close the channel. This allows the sync + // to be stopped in the event that the oidcClient is removed, or if the Server + // is Closed. + firstSync := make(chan struct{}) + syncCtx, syncCancel := context.WithCancel(a.closeCtx) go func() { - defer close(doneSyncing) - doneSyncing <- client.SyncProviderConfig(conn.GetIssuerURL()) + stop := client.SyncProviderConfig(conn.GetIssuerURL()) + close(firstSync) + <-syncCtx.Done() + close(stop) }() - oidcClient := &oidcClient{client: client, config: config} select { - case stop := <-doneSyncing: - oidcClient.stop = stop + case <-firstSync: case <-time.After(defaults.WebHeadersTimeout): + syncCancel() return nil, trace.ConnectionProblem(nil, "timed out syncing oidc connector %v, ensure URL %q is valid and accessible and check configuration", conn.GetName(), conn.GetIssuerURL()) case <-a.closeCtx.Done(): + syncCancel() return nil, trace.ConnectionProblem(nil, "auth server is shutting down") } a.lock.Lock() defer a.lock.Unlock() - a.oidcClients[conn.GetName()] = oidcClient + a.oidcClients[conn.GetName()] = &oidcClient{client: client, config: config, cancel: syncCancel} return client, nil } From 1c67bea32c23aaffd2d0cf301892eb675a62fdbd Mon Sep 17 00:00:00 2001 From: Tim Ross Date: Tue, 19 Apr 2022 09:29:58 -0400 Subject: [PATCH 3/3] focus sync comment on present day behavior --- lib/auth/oidc.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/auth/oidc.go b/lib/auth/oidc.go index 66abb3ebae431..1e8c12b2f2550 100644 --- a/lib/auth/oidc.go +++ b/lib/auth/oidc.go @@ -81,11 +81,10 @@ func (a *Server) createOIDCClient(conn types.OIDCConnector) (*oidc.Client, error } // SyncProviderConfig doesn't take a context for cancellation, instead it - // returns a channel that has to be closed to stop the sync. So what we - // can do until it is changed to take a context, is to wait for the syncContext - // we create below to be done and then close the channel. This allows the sync - // to be stopped in the event that the oidcClient is removed, or if the Server - // is Closed. + // returns a channel that has to be closed to stop the sync. To ensure that + // the sync is eventually stopped we create a child context of the server context, which + // is cancelled either on deletion of the connector or shutdown of the server. + // This will cause syncCtx.Done() to unblock, at which point we can close the stop channel. firstSync := make(chan struct{}) syncCtx, syncCancel := context.WithCancel(a.closeCtx) go func() {