* fix: prevent panic in `httputils.Forwarder` on client cancellation
Kubernetes Watchers are usually a long-lived HTTP request where the
server pushes new updates to clients. When clients are no longer
interested in the stream because they already received the information
they were looking for, the client cancels the request. The request
cancellation is propagated via context cancelation.
Go's `httputils.ReverseProxy` has a special handling for cases where the
read or write body operations fail. This happens when the client
initially started the request, receive the contents and then cancelled
it. The reverse proxy sees the error as `context.Canceled` error - i.e.
the client canceled it - but
[`copyBuffer`](https://cs.opensource.google/go/go/+/refs/tags/go1.24.4:src/net/http/httputil/reverseproxy.go;l=664-669;drc=e64f7ef03fdfa1c0d847c21b16c9302cc824e79b)
only has a special case for `io.EOF`. This means that `context.Canceled`
error is propagated to [`ServeHTTP`](https://cs.opensource.google/go/go/+/refs/tags/go1.24.4:src/net/http/httputil/reverseproxy.go;l=520-530;drc=e64f7ef03fdfa1c0d847c21b16c9302cc824e79b)
and hits the `shouldPanicOnCopyError` function. This function checks if
the function should panic or not. Since we are running inside
`http.Server`, the decision is always to panic which caused all
watcher resources to never cleanup and accumulate.
This PR changes the logic of our `httputils.Forwarder` to remove the
`http.ServerContextKey` to avoid any panic. It also fixes a possible
deadlock caused by incorrect condition.
Changes:
- Remove http.ServerContextKey from httputils.Forwarder to prevent panic
- Fix potential deadlock caused by incorrect condition
This ensures graceful handling of client cancellations and proper
resource cleanup for Kubernetes Watchers.
Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
* Update lib/httplib/reverseproxy/reverse_proxy.go
Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
---------
Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
Co-authored-by: rosstimothy <39066650+rosstimothy@users.noreply.github.com>
* use httputil Rewrite instead of Director
Director will remove HTTP headers if they are
deemed to be hop-by-hop headers which could result
in a security vulnerability. Use httputil Rewrite
instead to ensure that HTTP headers added by our
reverse proxy are not unintentionally removed.
* ensure removed forwarded headers are added back
* use a separate request object in TestHeaderRewriter
---------
Co-authored-by: Andrew LeFevre <Andrew LeFevre>
Adds the new linter and configures it to prevent direct error
comparisions and type assertions in favor of errors.Is and errors.As.
All current violations caught by the linter have been updated
accordingly.
When creating a reverse proxy the ErrorLog was being created
with the writer returned from logrus.WriterLevel. This writer
uses an io.Pipe and a goroutine to get any log messages written
from a standard log.Logger to our custom formated logrus.Entry.
The ErrorLog only ever omits output in a few very rare (hopefully)
error cases, which means we were creating an extra goroutine per
reverse proxy which in most cases would never be used to produce
any added output. Additionally, now that we are starting to support
log/slog, any output from a log.Logger will now be formatted via
our registered default slog handler. Meaning that we can still
get the benefits of producing output from the ErrorLog in the
desired format without the penalty incurred by using logrus.WriterLevel.
Partially addresses #36541.
* Fix duplicate entry for `X-Forwarded-For` header
PR #27761 replaced `oxy.Forwarder` with `httputil.ReverseProxy`.
The new forwarder based on `httputil.ReverseProxy` is appending the
`X-Forwarder-For` header values instead of replacing them.
This PR fixes that behavior and forces the XFF header to be a single
value.
Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
* Update lib/httplib/reverseproxy/rewriter_test.go
Co-authored-by: Reed Loden <reed@goteleport.com>
---------
Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
Co-authored-by: Reed Loden <reed@goteleport.com>
GKE Autopilot clusters deny impersonating `system:masters` kubernetes
group due to security reasons ( https://cloud.google.com/kubernetes-engine/docs/concepts/autopilot-security#built-in-security ).
Since it is the only product that enforces this and most administrators impersonate `system:masters`, this limitation has created too many difficulties in being understood.
First because the error is not clear enough and second because it doesn't explain what's the real problem and what part of the Teleport fails.
This PR tries to solve the problem by changing the original error message to a more user-friendly, one that better explains the problem and its origin.
Example of response body that is rewritten:
> Error from server (Forbidden): groups "system:masters" is forbidden:
> User "<user>" cannot impersonate resource "groups" in API group "" at the cluster
> scope: GKE Warden authz [denied by user-impersonation-limitation]: impersonating
> system identities are not allowed
The rewritten response body will look like:
> Error from server (Forbidden): "GKE Autopilot denied the request because it impersonates the "system:masters" group.
> Your Teleport Roles [role1,role2] have given access to the "system:masters" group for the cluster "<cluster>".
> For additional information and resolution, please visit https://goteleport.com/docs/kubernetes-access/troubleshooting/#unable-to-connect-to-gke-autopilot-clustersFixes#28506
This PR replaces the usage of `gravitational/oxy.Forwarder` in our codebase with `httputil.ReverseProxy`.
`httputil.ReverseProxy` is part of Go's standard library and thus it is
maintained by the Go team. Converting everything to use the std lib
allows us to drop our `oxy` fork.
There are some operational changes between the two versions:
1. `httputil.ReverseProxy` handles WebSockets upgrades internally and
reuses the `Transport` provided for the initial request. Thus, it does
not require any special dialer or rewriter mechanisms for WebSockets only.
2. `httputil.ReverseProxy` calls the `ErrorHandler` with the request
after it was rewritten and after roundtrip was called. This was a
problem because our `web.transport` deletes Teleport associated
cookies and when the request failed, the request received by the
`ErrorHandler` missed the cookies to generate a new session. This was
fixed by cloning the request at `web.transport` round tripper.
3. `httputil.ReverseProxy` uses std's `log.Logger`. `logrus` exposes a
method `Writer` that allows it to be used with standard lib logger.
The API is similar to what our `oxy` fork has available.
Fixes#27412
Zac Bergquist