From 628046b34cb66f5df832c22150983d7c38e731e8 Mon Sep 17 00:00:00 2001 From: bolkedebruin Date: Thu, 30 Apr 2026 14:07:50 +0200 Subject: [PATCH] Bind cached tunnel reuse to the original session identity (#185) HandleGatewayProtocol caches a *Tunnel keyed on the client-supplied Rdg-Connection-Id header so the two halves of a session (RDG_OUT_DATA and RDG_IN_DATA) can rendezvous on a single record. The cache hit path previously reused the tunnel without checking who was making the follow-up request. Add tunnelOwnerMatches(t, id) to compare the cached tunnel's UserName() and AttrClientIp against the request identity. On mismatch, refuse with 401 instead of attaching the new request to the existing tunnel. The helper is conservative: nil tunnel/user/identity, empty username, or missing client-IP attribute all fail closed. The legitimate case (the same client returns to attach its second half-channel to its own first half) is unchanged. Adds TestTunnelOwnershipEnforced. --- cmd/rdpgw/protocol/gateway.go | 24 ++++++++++++ cmd/rdpgw/protocol/gateway_test.go | 61 ++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/cmd/rdpgw/protocol/gateway.go b/cmd/rdpgw/protocol/gateway.go index c1f6869..e38b18e 100644 --- a/cmd/rdpgw/protocol/gateway.go +++ b/cmd/rdpgw/protocol/gateway.go @@ -75,6 +75,11 @@ func (g *Gateway) HandleGatewayProtocol(w http.ResponseWriter, r *http.Request) } } else { t = x.(*Tunnel) + if !tunnelOwnerMatches(t, id) { + log.Printf("rejecting reuse of Rdg-Connection-Id %q from a different identity", connId) + http.Error(w, "Tunnel is owned by a different session", http.StatusUnauthorized) + return + } } ctx = context.WithValue(ctx, CtxTunnel, t) @@ -106,6 +111,25 @@ func (g *Gateway) HandleGatewayProtocol(w http.ResponseWriter, r *http.Request) } } +// tunnelOwnerMatches reports whether the cached tunnel was opened by the same +// identity making the current request. The Rdg-Connection-Id is a client- +// chosen header that pairs the two halves of a session; without this check +// any caller who learns or guesses one can attach to another user's tunnel. +func tunnelOwnerMatches(t *Tunnel, id identity.Identity) bool { + if t == nil || t.User == nil || id == nil { + return false + } + if t.User.UserName() == "" || t.User.UserName() != id.UserName() { + return false + } + cachedIp, _ := t.User.GetAttribute(identity.AttrClientIp).(string) + reqIp, _ := id.GetAttribute(identity.AttrClientIp).(string) + if cachedIp == "" { + return false + } + return cachedIp == reqIp +} + // headerHasToken reports whether the HTTP header named by name contains // token, matched case-insensitively against comma-separated tokens. // Fields like Connection carry a list (e.g. "keep-alive, Upgrade") so a diff --git a/cmd/rdpgw/protocol/gateway_test.go b/cmd/rdpgw/protocol/gateway_test.go index fd288de..779966f 100644 --- a/cmd/rdpgw/protocol/gateway_test.go +++ b/cmd/rdpgw/protocol/gateway_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/bolkedebruin/rdpgw/cmd/rdpgw/identity" + "github.com/patrickmn/go-cache" ) func TestHeaderHasToken(t *testing.T) { @@ -144,3 +145,63 @@ func TestHandleGatewayProtocolRouting(t *testing.T) { }) } } + +// TestTunnelOwnershipEnforced asserts that an Rdg-Connection-Id which already +// has a cached tunnel cannot be reused by a different identity. The connection +// id travels in plain HTTP headers, so a client that learns or guesses one +// must not be able to attach to the original tunnel — the cache is for +// matching the two halves of the same client's session, not for authorizing +// access. +func TestTunnelOwnershipEnforced(t *testing.T) { + connId := "shared-connid-ownership" + + aliceID := identity.NewUser() + aliceID.SetUserName("alice") + aliceID.SetAttribute(identity.AttrRemoteAddr, "10.1.1.1:1234") + aliceID.SetAttribute(identity.AttrClientIp, "10.1.1.1") + c.Set(connId, &Tunnel{ + RDGId: connId, + RemoteAddr: "10.1.1.1:1234", + User: aliceID, + }, cache.DefaultExpiration) + defer c.Delete(connId) + + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + bob := identity.NewUser() + bob.SetUserName("bob") + bob.SetAttribute(identity.AttrRemoteAddr, "10.2.2.2:5678") + bob.SetAttribute(identity.AttrClientIp, "10.2.2.2") + r = identity.AddToRequestCtx(bob, r) + (&Gateway{}).HandleGatewayProtocol(w, r) + }) + srv := httptest.NewServer(handler) + defer srv.Close() + + addr := strings.TrimPrefix(srv.URL, "http://") + conn, err := net.DialTimeout("tcp", addr, 2*time.Second) + if err != nil { + t.Fatalf("dial: %v", err) + } + defer conn.Close() + if err := conn.SetDeadline(time.Now().Add(3 * time.Second)); err != nil { + t.Fatalf("set deadline: %v", err) + } + + req := "RDG_OUT_DATA /remoteDesktopGateway/ HTTP/1.1\r\n" + + "Host: " + addr + "\r\n" + + "Rdg-Connection-Id: " + connId + "\r\n" + + "\r\n" + if _, err := conn.Write([]byte(req)); err != nil { + t.Fatalf("write: %v", err) + } + + line, err := bufio.NewReader(conn).ReadString('\n') + if err != nil { + t.Fatalf("read status line: %v", err) + } + line = strings.TrimRight(line, "\r\n") + + if strings.HasPrefix(line, "HTTP/1.1 2") { + t.Fatalf("a different identity attached to a cached tunnel via Rdg-Connection-Id (status %q); the header was treated as authorization", line) + } +}