Fix wrong context usage for reissuing expired certificate for tsh proxy kube.

[AntonAM]

We should not use request's context for the procedure of reissuing certs, those requests are supposed to fail early in case of MFA requirement to bring user's attention to the Connect/tsh, so users can finish the procedure. Additional discussion here: https://gravitational.slack.com/archives/C03FJA391M3/p1718188745510909 . This PR introduces context for the kube proxy middleware so we have a cancelable context (important for Connect) and don't have to rely on request's context.

Changelog: Wait for user MFA input when reissuing expired certificates for a kube proxy.

[marcoandredinis]

Left a few comments.
How hard would it be to add test coverage?

[ravicious]

I've learned that storing a context in a struct should be avoided. But I also know that often it's hard to refactor code out of that pattern.

However, in the case of alpnproxy.LocalProxy, the TCP version of the proxy already utilizes contexts passed as arguments pretty well. Instead of storing a context on KubeMiddleware, what do you think about passing the context from LocalProxy.Start? This is the same thing that the TCP proxy does. It'd make it so that the context works the same between TCP middlewares and HTTP middlewares.

It'd probably warrant adding some comments so that people implementing middlewares understand what kind of context is passed to HandleRequest.

Patch

diff --git a/lib/srv/alpnproxy/auth_checker_middleware.go b/lib/srv/alpnproxy/auth_checker_middleware.go
index d61ad24f6dd..372d1218813 100644
--- a/lib/srv/alpnproxy/auth_checker_middleware.go
+++ b/lib/srv/alpnproxy/auth_checker_middleware.go
@@ -19,6 +19,7 @@
 package alpnproxy
 
 import (
+	"context"
 	"crypto/subtle"
 	"net/http"
 
@@ -55,7 +56,7 @@ func (m *AuthorizationCheckerMiddleware) CheckAndSetDefaults() error {
 }
 
 // HandleRequest checks Authorization header, which must be either missing or set to the secret value of a bearer token.
-func (m *AuthorizationCheckerMiddleware) HandleRequest(rw http.ResponseWriter, req *http.Request) bool {
+func (m *AuthorizationCheckerMiddleware) HandleRequest(ctx context.Context, rw http.ResponseWriter, req *http.Request) bool {
 	auth := req.Header.Get("Authorization")
 	if auth == "" {
 		m.Log.Debugf("No Authorization header present, ignoring request.")
diff --git a/lib/srv/alpnproxy/aws_local_proxy.go b/lib/srv/alpnproxy/aws_local_proxy.go
index 794fc6b9b78..6e10806eb79 100644
--- a/lib/srv/alpnproxy/aws_local_proxy.go
+++ b/lib/srv/alpnproxy/aws_local_proxy.go
@@ -19,6 +19,7 @@
 package alpnproxy
 
 import (
+	"context"
 	"net/http"
 	"strings"
 
@@ -109,7 +110,7 @@ func (m *AWSAccessMiddleware) CheckAndSetDefaults() error {
 // Note that the first sts:AssumeRole should be signed with the
 // local-proxy-generated credentials by the AWS client, while the second
 // request is signed with real credentials of the assumed role.
-func (m *AWSAccessMiddleware) HandleRequest(rw http.ResponseWriter, req *http.Request) bool {
+func (m *AWSAccessMiddleware) HandleRequest(ctx context.Context, rw http.ResponseWriter, req *http.Request) bool {
 	sigV4, err := awsutils.ParseSigV4(req.Header.Get(awsutils.AuthorizationHeader))
 	if err != nil {
 		m.Log.WithError(err).Error("Failed to parse AWS request authorization header.")
diff --git a/lib/srv/alpnproxy/azure_msi_middleware.go b/lib/srv/alpnproxy/azure_msi_middleware.go
index f612c325ca6..c64948282ff 100644
--- a/lib/srv/alpnproxy/azure_msi_middleware.go
+++ b/lib/srv/alpnproxy/azure_msi_middleware.go
@@ -19,6 +19,7 @@
 package alpnproxy
 
 import (
+	"context"
 	"crypto"
 	"encoding/json"
 	"fmt"
@@ -85,7 +86,7 @@ func (m *AzureMSIMiddleware) CheckAndSetDefaults() error {
 	return nil
 }
 
-func (m *AzureMSIMiddleware) HandleRequest(rw http.ResponseWriter, req *http.Request) bool {
+func (m *AzureMSIMiddleware) HandleRequest(ctx context.Context, rw http.ResponseWriter, req *http.Request) bool {
 	if req.Host == types.TeleportAzureMSIEndpoint {
 		if err := m.msiEndpoint(rw, req); err != nil {
 			m.Log.Warnf("Bad MSI request: %v", err)
diff --git a/lib/srv/alpnproxy/kube.go b/lib/srv/alpnproxy/kube.go
index fac6d8e143f..c51e75c078c 100644
--- a/lib/srv/alpnproxy/kube.go
+++ b/lib/srv/alpnproxy/kube.go
@@ -160,13 +160,13 @@ func writeKubeError(rw http.ResponseWriter, kubeError *apierrors.StatusError, lo
 // HandleRequest checks if middleware has valid certificate for this request and
 // reissues it if needed. In case of reissuing error we write directly to the response and return true,
 // so caller won't continue processing the request.
-func (m *KubeMiddleware) HandleRequest(rw http.ResponseWriter, req *http.Request) bool {
+func (m *KubeMiddleware) HandleRequest(ctx context.Context, rw http.ResponseWriter, req *http.Request) bool {
 	cert, err := m.getCertForRequest(req)
 	if err != nil {
 		return false
 	}
 
-	err = m.reissueCertIfExpired(req.Context(), cert, req.TLS.ServerName)
+	err = m.reissueCertIfExpired(ctx, cert, req.TLS.ServerName)
 	if err != nil {
 		// If user input is required we return an error that will try to get user attention to the local proxy
 		if errors.Is(err, ErrUserInputRequired) {
@@ -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(ctx, cluster, identity.KubernetesCluster)
 			if err == nil {
 				m.certsMu.Lock()
 				m.certs[serverName] = newCert
diff --git a/lib/srv/alpnproxy/local_proxy.go b/lib/srv/alpnproxy/local_proxy.go
index 0aa7e75a193..053ab7a4b43 100644
--- a/lib/srv/alpnproxy/local_proxy.go
+++ b/lib/srv/alpnproxy/local_proxy.go
@@ -363,7 +363,7 @@ func (l *LocalProxy) startHTTPAccessProxy(ctx context.Context) error {
 				}
 			}
 
-			if l.cfg.HTTPMiddleware.HandleRequest(rw, req) {
+			if l.cfg.HTTPMiddleware.HandleRequest(ctx, rw, req) {
 				return
 			}
 
diff --git a/lib/srv/alpnproxy/local_proxy_http_middleware.go b/lib/srv/alpnproxy/local_proxy_http_middleware.go
index a2d619f6bd3..74a5ac08eae 100644
--- a/lib/srv/alpnproxy/local_proxy_http_middleware.go
+++ b/lib/srv/alpnproxy/local_proxy_http_middleware.go
@@ -19,6 +19,7 @@
 package alpnproxy
 
 import (
+	"context"
 	"crypto/tls"
 	"net/http"
 
@@ -31,7 +32,7 @@ type LocalProxyHTTPMiddleware interface {
 	CheckAndSetDefaults() error
 
 	// HandleRequest returns true if requests has been handled and must not be processed further, false otherwise.
-	HandleRequest(rw http.ResponseWriter, req *http.Request) bool
+	HandleRequest(ctx context.Context, rw http.ResponseWriter, req *http.Request) bool
 
 	// HandleResponse processes the server response before sending it to the client.
 	HandleResponse(resp *http.Response) error

[AntonAM]

I think that article is one sided, it mostly talks about per-request context usage. And we of course have this pattern in the code and we use it a lot. But there's a also pattern with closeContext, that isn't limited to a single request, but controls lifecycle on a broader scope. And we use this pattern a lot as well.

Within HandleRequest we already have a request-scoped context, which is stored in the request.Context() (that backward compatibility case described in the article) and passing second context I think just muddies the waters, and indeed you have a wrong usage of a context in your patch - context provided to m.reissueCertIfExpired should really be req.Context(), because it processes cancellation of the request itself inside. If we want to provide the closeContext in the call, we would need to provide 2 contexts to that function, which again I don't think makes it more clear. TCP proxy doesn't have HTTP request, hence no object containing per-request context automatically provided to it.

As Marco correctly pointed out we should rename introduced context to closeContext to make its intention clearer, but otherwise I think that works fine for this case.

[ravicious]

I'm still not sold on the closeContext pattern, but that's mostly because I'm not a Go expert and I haven't seen an authoritative source speak in favor of it (like that blog post 😏). As such, I'm not going to push you on it. Assuming that the closeContext pattern is valid, your argumentation makes sense to me.

[marcoandredinis]

Go's stdlib has examples of context coming out of structs.
It's not exactly the same but, as an example, in the http server:
https://cs.opensource.google/go/go/+/refs/tags/go1.22.4:src/net/http/server.go;l=2898-2904

On the other hand, this is a pattern used in a lot of places in teleport's codebase.
That's why it felt strange to not name it closeContext

[ravicious]

I'm still not sold on the closeContext pattern, but that's mostly because I'm not a Go expert and I haven't seen an authoritative source speak in favor of it (like that blog post 😏). As such, I'm not going to push you on it. Assuming that the closeContext pattern is valid, your argumentation makes sense to me.

[public-teleport-github-review-bot]

@AntonAM See the table below for backport results.

Branch	Result
branch/v14	Failed
branch/v15	Create PR
branch/v16	Create PR