From 2064beb3e26125fd6026d795bddc278291a1540c Mon Sep 17 00:00:00 2001 From: Anton Miniailo Date: Fri, 21 Jun 2024 13:57:49 -0400 Subject: [PATCH 1/5] Fix wrong context usage for reissuing expired certificate for tsh proxy kube. --- lib/srv/alpnproxy/kube.go | 10 ++++++++-- lib/teleterm/gateway/kube.go | 5 +++-- tool/tsh/common/kube_proxy.go | 1 + 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/srv/alpnproxy/kube.go b/lib/srv/alpnproxy/kube.go index 20018ecfb5a5a..fac6d8e143f08 100644 --- a/lib/srv/alpnproxy/kube.go +++ b/lib/srv/alpnproxy/kube.go @@ -81,7 +81,8 @@ type KubeMiddleware struct { // headless controls whether proxy is working in headless login mode. headless bool - logger logrus.FieldLogger + logger logrus.FieldLogger + context context.Context // isCertReissuingRunning is used to only ever have one concurrent cert reissuing session requiring user input. isCertReissuingRunning atomic.Bool @@ -97,6 +98,7 @@ type KubeMiddlewareConfig struct { Headless bool Clock clockwork.Clock Logger logrus.FieldLogger + Context context.Context } // NewKubeMiddleware creates a new KubeMiddleware. @@ -107,6 +109,7 @@ func NewKubeMiddleware(cfg KubeMiddlewareConfig) LocalProxyHTTPMiddleware { headless: cfg.Headless, clock: cfg.Clock, logger: cfg.Logger, + context: cfg.Context, } } @@ -121,6 +124,9 @@ func (m *KubeMiddleware) CheckAndSetDefaults() error { if m.logger == nil { m.logger = logrus.WithField(teleport.ComponentKey, "local_proxy_kube") } + if m.context == nil { + m.context = context.Background() + } return nil } @@ -245,7 +251,7 @@ func (m *KubeMiddleware) reissueCertIfExpired(ctx context.Context, cert tls.Cert if identity.RouteToCluster != "" { cluster = identity.RouteToCluster } - newCert, err := m.certReissuer(ctx, cluster, identity.KubernetesCluster) + newCert, err := m.certReissuer(m.context, cluster, identity.KubernetesCluster) if err == nil { m.certsMu.Lock() m.certs[serverName] = newCert diff --git a/lib/teleterm/gateway/kube.go b/lib/teleterm/gateway/kube.go index 323d51903440e..7bafd1948c72a 100644 --- a/lib/teleterm/gateway/kube.go +++ b/lib/teleterm/gateway/kube.go @@ -131,8 +131,9 @@ func (k *kube) makeKubeMiddleware() (alpnproxy.LocalProxyHTTPMiddleware, error) cert, err := k.cfg.OnExpiredCert(ctx, k) return cert, trace.Wrap(err) }, - Clock: k.cfg.Clock, - Logger: k.cfg.Log, + Clock: k.cfg.Clock, + Logger: k.cfg.Log, + Context: k.closeContext, }), nil } diff --git a/tool/tsh/common/kube_proxy.go b/tool/tsh/common/kube_proxy.go index 4dc8845b4b23d..78052b3d632f5 100644 --- a/tool/tsh/common/kube_proxy.go +++ b/tool/tsh/common/kube_proxy.go @@ -342,6 +342,7 @@ func makeKubeLocalProxy(cf *CLIConf, tc *client.TeleportClient, clusters kubecon CertReissuer: kubeProxy.getCertReissuer(tc), Headless: cf.Headless, Logger: log, + Context: cf.Context, }) localProxy, err := alpnproxy.NewLocalProxy( From 9241628afbf2cb661b48a5bd8a7e1221fc447fca Mon Sep 17 00:00:00 2001 From: Anton Miniailo Date: Wed, 26 Jun 2024 12:11:02 -0400 Subject: [PATCH 2/5] Rename context to closeContext --- lib/srv/alpnproxy/kube.go | 14 +++++++------- lib/teleterm/gateway/kube.go | 6 +++--- tool/tsh/common/kube_proxy.go | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/srv/alpnproxy/kube.go b/lib/srv/alpnproxy/kube.go index fac6d8e143f08..256eb2ad79a30 100644 --- a/lib/srv/alpnproxy/kube.go +++ b/lib/srv/alpnproxy/kube.go @@ -81,8 +81,8 @@ type KubeMiddleware struct { // headless controls whether proxy is working in headless login mode. headless bool - logger logrus.FieldLogger - context context.Context + logger logrus.FieldLogger + closeContext context.Context // isCertReissuingRunning is used to only ever have one concurrent cert reissuing session requiring user input. isCertReissuingRunning atomic.Bool @@ -98,7 +98,7 @@ type KubeMiddlewareConfig struct { Headless bool Clock clockwork.Clock Logger logrus.FieldLogger - Context context.Context + CloseContext context.Context } // NewKubeMiddleware creates a new KubeMiddleware. @@ -109,7 +109,7 @@ func NewKubeMiddleware(cfg KubeMiddlewareConfig) LocalProxyHTTPMiddleware { headless: cfg.Headless, clock: cfg.Clock, logger: cfg.Logger, - context: cfg.Context, + closeContext: cfg.CloseContext, } } @@ -124,8 +124,8 @@ func (m *KubeMiddleware) CheckAndSetDefaults() error { if m.logger == nil { m.logger = logrus.WithField(teleport.ComponentKey, "local_proxy_kube") } - if m.context == nil { - m.context = context.Background() + if m.closeContext == nil { + return trace.BadParameter("missing close context") } return nil } @@ -251,7 +251,7 @@ func (m *KubeMiddleware) reissueCertIfExpired(ctx context.Context, cert tls.Cert if identity.RouteToCluster != "" { cluster = identity.RouteToCluster } - newCert, err := m.certReissuer(m.context, cluster, identity.KubernetesCluster) + newCert, err := m.certReissuer(m.closeContext, cluster, identity.KubernetesCluster) if err == nil { m.certsMu.Lock() m.certs[serverName] = newCert diff --git a/lib/teleterm/gateway/kube.go b/lib/teleterm/gateway/kube.go index 7bafd1948c72a..a15f7cf105cf9 100644 --- a/lib/teleterm/gateway/kube.go +++ b/lib/teleterm/gateway/kube.go @@ -131,9 +131,9 @@ func (k *kube) makeKubeMiddleware() (alpnproxy.LocalProxyHTTPMiddleware, error) cert, err := k.cfg.OnExpiredCert(ctx, k) return cert, trace.Wrap(err) }, - Clock: k.cfg.Clock, - Logger: k.cfg.Log, - Context: k.closeContext, + Clock: k.cfg.Clock, + Logger: k.cfg.Log, + CloseContext: k.closeContext, }), nil } diff --git a/tool/tsh/common/kube_proxy.go b/tool/tsh/common/kube_proxy.go index 78052b3d632f5..59149360903f2 100644 --- a/tool/tsh/common/kube_proxy.go +++ b/tool/tsh/common/kube_proxy.go @@ -342,7 +342,7 @@ func makeKubeLocalProxy(cf *CLIConf, tc *client.TeleportClient, clusters kubecon CertReissuer: kubeProxy.getCertReissuer(tc), Headless: cf.Headless, Logger: log, - Context: cf.Context, + CloseContext: cf.Context, }) localProxy, err := alpnproxy.NewLocalProxy( From 14af8050cd9fd6f1c7106a3627020aecefdc367e Mon Sep 17 00:00:00 2001 From: Anton Miniailo Date: Wed, 26 Jun 2024 12:12:21 -0400 Subject: [PATCH 3/5] Add test for request context expiration. --- lib/srv/alpnproxy/local_proxy_test.go | 43 ++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/lib/srv/alpnproxy/local_proxy_test.go b/lib/srv/alpnproxy/local_proxy_test.go index 8054622c5b303..dd6a64e8fd5e8 100644 --- a/lib/srv/alpnproxy/local_proxy_test.go +++ b/lib/srv/alpnproxy/local_proxy_test.go @@ -504,9 +504,49 @@ func TestKubeMiddleware(t *testing.T) { ) certReissuer = func(ctx context.Context, teleportCluster, kubeCluster string) (tls.Certificate, error) { - return newCert, nil + select { + case <-ctx.Done(): + return tls.Certificate{}, ctx.Err() + default: + return newCert, nil + } } + t.Run("expired certificate is still reissued if request context expires", func(t *testing.T) { + req := &http.Request{ + TLS: &tls.ConnectionState{ + ServerName: "kube1", + }, + } + // we set request context to a context that will expired immediately. + reqCtx, cancel := context.WithDeadline(context.Background(), time.Now()) + defer cancel() + req = req.WithContext(reqCtx) + + km := NewKubeMiddleware(KubeMiddlewareConfig{ + Certs: KubeClientCerts{"kube1": kube1Cert}, + CertReissuer: certReissuer, + Clock: clockwork.NewFakeClockAt(now.Add(time.Hour * 2)), + CloseContext: context.Background(), + }) + err := km.CheckAndSetDefaults() + require.NoError(t, err) + + rw := responsewriters.NewMemoryResponseWriter() + // HandleRequest will reissue certificate if needed. + km.HandleRequest(rw, req) + + // request timed out. + require.Equal(t, http.StatusInternalServerError, rw.Status()) + require.Contains(t, rw.Buffer().String(), "context deadline exceeded") + + // but certificate still was reissued. + certs, err := km.OverwriteClientCerts(req) + require.NoError(t, err) + require.Len(t, certs, 1) + require.Equal(t, newCert, certs[0]) + }) + testCases := []struct { name string reqClusterName string @@ -559,6 +599,7 @@ func TestKubeMiddleware(t *testing.T) { Certs: tt.startCerts, CertReissuer: certReissuer, Clock: tt.clock, + CloseContext: context.Background(), }) // HandleRequest will reissue certificate if needed From 527c22db8543a9aef0219f8b4f897329abc24dd7 Mon Sep 17 00:00:00 2001 From: Anton Miniailo Date: Wed, 26 Jun 2024 12:47:35 -0400 Subject: [PATCH 4/5] Add missing context in tests. --- integration/proxy/proxy_helpers.go | 1 + 1 file changed, 1 insertion(+) diff --git a/integration/proxy/proxy_helpers.go b/integration/proxy/proxy_helpers.go index e766bb9ae4dc8..b6402a6a8fa70 100644 --- a/integration/proxy/proxy_helpers.go +++ b/integration/proxy/proxy_helpers.go @@ -567,6 +567,7 @@ func mustCreateKubeLocalProxyMiddleware(t *testing.T, teleportCluster, kubeClust CertReissuer: func(ctx context.Context, teleportCluster, kubeCluster string) (tls.Certificate, error) { return tls.Certificate{}, nil }, + CloseContext: context.Background(), }) } From 60b5d34cf8f281cab2be50aba0dbdb302eed4d4a Mon Sep 17 00:00:00 2001 From: Anton Miniailo Date: Thu, 27 Jun 2024 14:49:01 -0400 Subject: [PATCH 5/5] Remove flakiness from the test. --- lib/srv/alpnproxy/local_proxy_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/srv/alpnproxy/local_proxy_test.go b/lib/srv/alpnproxy/local_proxy_test.go index dd6a64e8fd5e8..0f49928370a16 100644 --- a/lib/srv/alpnproxy/local_proxy_test.go +++ b/lib/srv/alpnproxy/local_proxy_test.go @@ -540,11 +540,14 @@ func TestKubeMiddleware(t *testing.T) { require.Equal(t, http.StatusInternalServerError, rw.Status()) require.Contains(t, rw.Buffer().String(), "context deadline exceeded") + // just let the reissuing goroutine some time to replace certs. + time.Sleep(10 * time.Millisecond) + // but certificate still was reissued. certs, err := km.OverwriteClientCerts(req) require.NoError(t, err) require.Len(t, certs, 1) - require.Equal(t, newCert, certs[0]) + require.Equal(t, newCert, certs[0], "certificate was not reissued") }) testCases := []struct {