amadan

carlos / platform Public

edge: gate the tunnel upgrade headers, and give the token-binding test teeth

Review findings on the TunnelHub:

1. The "a token may only claim its own channel" property had no test that
   could detect its failure — the hub under test had a single client, so an
   implementation looping over h.clients would have passed unchanged. The
   table now configures two clients and asserts the cross-name case in both
   directions; verified by mutation that it catches exactly that loop.

2. Connection/Upgrade were published wire contract but never checked, so we
   answered "101 carlos-tunnel/1" to clients that never asked and a future
   carlos-tunnel/2 host would have hit a mystery h2-preface failure. Now a
   400 naming the expected protocol, gated AFTER auth so an unauthenticated
   prober still learns nothing (pinned by its own test).

3. Documented the ALPN trap beside tunnelUpgradeProto: the dialer must pin
   NextProtos to http/1.1, because the 443 listener advertises h2 and an h2
   response writer is not an http.Hijacker — auth would succeed and the
   handshake then die on "hijack unsupported".
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell f74a6a8c9d3b75f903a34fd907e27e946f381bda parent 5acaed3
2 files changed, +117 −9
  • internal/edge/tunnel.go +28 −0
  • internal/edge/tunnel_test.go +89 −9
diff --git a/internal/edge/tunnel.go b/internal/edge/tunnel.go
index 10566dd25..8cd1659e2 100644
--- a/internal/edge/tunnel.go
+++ b/internal/edge/tunnel.go
@@ -19,11 +19,26 @@ import (
"sync"
"time"
+ "golang.org/x/net/http/httpguts"
"golang.org/x/net/http2"
)
const (
tunnelRegisterPath = "/register"
+ // tunnelUpgradeProto is the wire contract the host dialer codes against:
+ //
+ // GET /register
+ // Authorization: Bearer <token>
+ // Carlos-Tunnel-Name: <name>
+ // Connection: Upgrade
+ // Upgrade: carlos-tunnel/1
+ //
+ // The dialer MUST also pin NextProtos to {"http/1.1"} on its TLS config.
+ // The edge's 443 listener advertises h2, and a dialer that lets ALPN pick
+ // it authenticates fine and then dies on "hijack unsupported": HTTP/2
+ // response writers are not http.Hijacker, and hijacking is how this
+ // handshake gets the raw conn. The h2 session begins only AFTER the 101,
+ // on that raw conn, with the roles inverted.
tunnelUpgradeProto = "carlos-tunnel/1"
// tunnelPingEvery / tunnelPingTimeout bound how long a half-dead channel
// can look connected: a failed or timed-out ping tears it down, the host
@@ -88,6 +103,19 @@ func (h *TunnelHub) ServeHTTP(w http.ResponseWriter, r *http.Request) {
http.Error(w, "unauthorized", http.StatusUnauthorized)
return
}
+ // Wire-contract gate, deliberately AFTER auth: an unauthenticated caller
+ // gets 401 whatever it sends, so probing cannot discover which protocols
+ // this endpoint speaks. Checking it at all is what keeps a version skew
+ // diagnosable — without it we would answer "101 carlos-tunnel/1" to a
+ // client that never asked, and a future carlos-tunnel/2 host would see a
+ // mystery h2-preface failure instead of a refusal naming the version.
+ // Connection is matched token-wise and Upgrade case-insensitively, both
+ // per RFC 7230; the version suffix is what must match exactly.
+ if !httpguts.HeaderValuesContainsToken(r.Header["Connection"], "Upgrade") ||
+ !strings.EqualFold(strings.TrimSpace(r.Header.Get("Upgrade")), tunnelUpgradeProto) {
+ http.Error(w, "expected Connection: Upgrade with Upgrade: "+tunnelUpgradeProto, http.StatusBadRequest)
+ return
+ }
hj, ok := w.(http.Hijacker)
if !ok {
http.Error(w, "hijack unsupported", http.StatusInternalServerError)
diff --git a/internal/edge/tunnel_test.go b/internal/edge/tunnel_test.go
index 554dfe36b..d75e97c31 100644
--- a/internal/edge/tunnel_test.go
+++ b/internal/edge/tunnel_test.go
@@ -3,28 +3,108 @@ package edge
import (
"net/http"
"net/http/httptest"
+ "strings"
"testing"
)
+// tunnelReq builds a request that is well-formed EXCEPT for the credentials
+// under test: valid upgrade headers, so the auth cases below can only fail on
+// auth. Pass an empty token to send no Authorization header at all.
+func tunnelReq(name, token string) *http.Request {
+ req := httptest.NewRequest("GET", "/register", nil)
+ req.Header.Set("Connection", "Upgrade")
+ req.Header.Set("Upgrade", tunnelUpgradeProto)
+ req.Header.Set("Carlos-Tunnel-Name", name)
+ if token != "" {
+ req.Header.Set("Authorization", "Bearer "+token)
+ }
+ return req
+}
+
func TestTunnelRegisterRejectsBadToken(t *testing.T) {
+ // TWO clients on purpose: with only one configured, no test here can tell
+ // "every token is bound to its own channel" apart from "any known token
+ // admits any known channel" — an implementation looping over h.clients
+ // would pass a single-client table unchanged.
+ hub := NewTunnelHub("tunnel.test", map[string]string{"pi-box": "sekrit", "hq": "other"})
+ for _, tc := range []struct{ desc, name, token string }{
+ {"no auth", "", ""},
+ {"wrong token", "pi-box", "nope"},
+ {"unknown name", "nosuch", "sekrit"},
+ {"empty name", "", "sekrit"},
+ // The binding property itself: a valid token may only ever claim ITS
+ // OWN channel name, in either direction.
+ {"cross-name", "hq", "sekrit"},
+ {"cross-name reversed", "pi-box", "other"},
+ } {
+ rec := httptest.NewRecorder()
+ hub.ServeHTTP(rec, tunnelReq(tc.name, tc.token))
+ if rec.Code != http.StatusUnauthorized {
+ t.Errorf("%s (name=%q token=%q): got %d, want 401", tc.desc, tc.name, tc.token, rec.Code)
+ }
+ }
+}
+
+func TestTunnelRegisterAuthPrecedesUpgradeCheck(t *testing.T) {
+ hub := NewTunnelHub("tunnel.test", map[string]string{"pi-box": "sekrit"})
+ req := httptest.NewRequest("GET", "/register", nil) // no upgrade headers at all
+ req.Header.Set("Authorization", "Bearer nope")
+ req.Header.Set("Carlos-Tunnel-Name", "pi-box")
+ rec := httptest.NewRecorder()
+ hub.ServeHTTP(rec, req)
+ if rec.Code != http.StatusUnauthorized {
+ t.Fatalf("got %d, want 401 — an unauthenticated caller must not learn which upgrade protocols we speak", rec.Code)
+ }
+}
+
+func TestTunnelRegisterRejectsWrongUpgrade(t *testing.T) {
hub := NewTunnelHub("tunnel.test", map[string]string{"pi-box": "sekrit"})
- for name, hdr := range map[string]http.Header{
- "no auth": {},
- "wrong token": {"Authorization": {"Bearer nope"}, "Carlos-Tunnel-Name": {"pi-box"}},
- "unknown name": {"Authorization": {"Bearer sekrit"}, "Carlos-Tunnel-Name": {"other"}},
- // A valid token may only claim ITS OWN channel name.
- "cross-name": {"Authorization": {"Bearer sekrit"}, "Carlos-Tunnel-Name": {""}},
+ for _, tc := range []struct{ desc, conn, upgrade string }{
+ {"no upgrade headers", "", ""},
+ {"missing Upgrade", "Upgrade", ""},
+ {"missing Connection", "", tunnelUpgradeProto},
+ {"wrong version", "Upgrade", "carlos-tunnel/2"},
+ {"other protocol", "Upgrade", "websocket"},
} {
req := httptest.NewRequest("GET", "/register", nil)
- req.Header = hdr
+ req.Header.Set("Authorization", "Bearer sekrit")
+ req.Header.Set("Carlos-Tunnel-Name", "pi-box")
+ if tc.conn != "" {
+ req.Header.Set("Connection", tc.conn)
+ }
+ if tc.upgrade != "" {
+ req.Header.Set("Upgrade", tc.upgrade)
+ }
rec := httptest.NewRecorder()
hub.ServeHTTP(rec, req)
- if rec.Code != http.StatusUnauthorized {
- t.Errorf("%s: got %d, want 401", name, rec.Code)
+ if rec.Code != http.StatusBadRequest {
+ t.Errorf("%s: got %d, want 400", tc.desc, rec.Code)
+ continue
+ }
+ // A future carlos-tunnel/2 host must get a diagnosable refusal.
+ if !strings.Contains(rec.Body.String(), tunnelUpgradeProto) {
+ t.Errorf("%s: body %q should name the expected protocol", tc.desc, rec.Body.String())
}
}
}
+func TestTunnelRegisterAcceptsTokenWiseConnectionHeader(t *testing.T) {
+ hub := NewTunnelHub("tunnel.test", map[string]string{"pi-box": "sekrit"})
+ req := httptest.NewRequest("GET", "/register", nil)
+ req.Header.Set("Authorization", "Bearer sekrit")
+ req.Header.Set("Carlos-Tunnel-Name", "pi-box")
+ req.Header.Set("Connection", "keep-alive, Upgrade") // token-wise, per RFC 7230
+ req.Header.Set("Upgrade", "Carlos-Tunnel/1") // protocol names are case-insensitive
+ rec := httptest.NewRecorder()
+ hub.ServeHTTP(rec, req)
+ // This request passes auth AND the upgrade gate, then dies at the hijack,
+ // which httptest.NewRecorder cannot do — so 500 (not 400) is how we know
+ // the gate let it through. The real 101 is Task 10's e2e.
+ if rec.Code != http.StatusInternalServerError {
+ t.Fatalf("got %d, want 500 (past the upgrade gate, stopped at the hijack)", rec.Code)
+ }
+}
+
func TestTunnelRegisterRejectsUnknownPath(t *testing.T) {
hub := NewTunnelHub("tunnel.test", map[string]string{"pi-box": "sekrit"})
req := httptest.NewRequest("GET", "/anything-else", nil)