Merge pull request #8 from nnemirovsky/fix/dns-and-channel-gate

fix(proxy): forward DNS for all non-denied domains, remove ChannelGateMiddleware

Author: nnemirovsky

cmd/sluice/main.go

@@ -861,11 +861,10 @@ func startAPIServer(addr string, apiSrv *api.Server, st *store.Store, mcpHandler
 	}
 	// oapi-codegen wraps handlers bottom-up: last middleware in the slice
 	// becomes the outermost layer. List channel gate first, then auth, so
-	// the request hits auth before the channel gate. This ensures bad tokens
-	// get 401 before the channel gate reveals whether HTTP channel is enabled.
+	// Bearer token auth protects all /api/* routes. The API is accessible
+	// whenever SLUICE_API_TOKEN is set, regardless of which channels are enabled.
 	apiHandler := api.HandlerWithOptions(apiSrv, api.ChiServerOptions{
 		Middlewares: []api.MiddlewareFunc{
-			api.ChannelGateMiddleware(st),
 			api.BearerAuthMiddleware,
 		},
 	})

internal/api/server_test.go

@@ -45,11 +45,10 @@ func enableHTTPChannel(t *testing.T, st *store.Store) {
 // oapi-codegen wraps handlers bottom-up: last middleware in the slice becomes
 // outermost. Channel gate goes first (innermost), auth second (outermost),
 // so auth rejects before channel gate reveals channel state.
-func newTestHandler(t *testing.T, srv *api.Server, st *store.Store) http.Handler {
+func newTestHandler(t *testing.T, srv *api.Server, _ *store.Store) http.Handler {
 	t.Helper()
 	return api.HandlerWithOptions(srv, api.ChiServerOptions{
 		Middlewares: []api.MiddlewareFunc{
-			api.ChannelGateMiddleware(st),
 			api.BearerAuthMiddleware,
 		},
 	})
@@ -210,53 +209,6 @@ func TestAuth_BadTokenBeforeChannelCheck(t *testing.T) {
 
 // --- Channel gate middleware tests ---
 
-func TestChannelGate_Disabled(t *testing.T) {
-	st := newTestStore(t)
-	// Default store has only Telegram channel (type=0), no HTTP channel
-	srv := api.NewServer(st, nil, nil, "")
-
-	t.Setenv("SLUICE_API_TOKEN", "secret")
-	handler := newTestHandler(t, srv, st)
-
-	req := httptest.NewRequest("GET", "/api/status", nil)
-	req.Header.Set("Authorization", "Bearer secret")
-	rec := httptest.NewRecorder()
-	handler.ServeHTTP(rec, req)
-
-	if rec.Code != http.StatusForbidden {
-		t.Errorf("expected 403, got %d", rec.Code)
-	}
-
-	var resp api.ErrorResponse
-	if err := json.NewDecoder(rec.Body).Decode(&resp); err != nil {
-		t.Fatalf("decode: %v", err)
-	}
-	if resp.Error != "HTTP channel is not enabled" {
-		t.Errorf("unexpected error: %q", resp.Error)
-	}
-	if resp.Code == nil || *resp.Code != "channel_disabled" {
-		t.Errorf("unexpected code: %v", resp.Code)
-	}
-}
-
-func TestChannelGate_Enabled(t *testing.T) {
-	st := newTestStore(t)
-	enableHTTPChannel(t, st)
-	srv := api.NewServer(st, nil, nil, "")
-
-	t.Setenv("SLUICE_API_TOKEN", "secret")
-	handler := newTestHandler(t, srv, st)
-
-	req := httptest.NewRequest("GET", "/api/status", nil)
-	req.Header.Set("Authorization", "Bearer secret")
-	rec := httptest.NewRecorder()
-	handler.ServeHTTP(rec, req)
-
-	if rec.Code != http.StatusOK {
-		t.Errorf("expected 200, got %d", rec.Code)
-	}
-}
-
 // --- Approval endpoint tests ---
 
 func TestGetApiApprovals_Empty(t *testing.T) {

internal/policy/engine.go

@@ -381,6 +381,25 @@ func (e *Engine) IsDenied(dest string, port int) bool {
 	return matchRules(e.compiled.denyRules, dest, port)
 }
 
+// IsDeniedDomain checks if a domain matches any deny rule regardless of port
+// or protocol. Used by the DNS interceptor to block resolution for explicitly
+// denied domains while allowing all others through (policy enforcement happens
+// at the SOCKS5 CONNECT level).
+func (e *Engine) IsDeniedDomain(dest string) bool {
+	dest = normalizeDestination(dest)
+	e.mu.RLock()
+	defer e.mu.RUnlock()
+	if e.compiled == nil {
+		return false
+	}
+	for _, r := range e.compiled.denyRules {
+		if r.glob.Match(dest) {
+			return true
+		}
+	}
+	return false
+}
+
 // IsRestricted checks whether a destination and port match any explicit deny
 // or ask rule. Unlike Evaluate, this does not fall back to the default verdict.
 // Used for DNS rebinding checks where the original FQDN was already allowed

internal/proxy/dns.go

@@ -239,11 +239,18 @@ func (d *DNSInterceptor) HandleQuery(query []byte) ([]byte, error) {
 	}
 
 	domain := questions[0].Name
-	verdict := d.evaluate(domain)
+
+	// Only block DNS for explicitly denied domains. All other verdicts
+	// (allow, ask, default) are forwarded. Policy enforcement for the
+	// actual connection happens at the SOCKS5 CONNECT level, not DNS.
+	// Blocking DNS for "ask" or default-deny domains would prevent the
+	// connection from ever reaching the approval flow.
+	eng := d.engine.Load()
+	denied := eng.IsDeniedDomain(domain)
 
 	if d.audit != nil {
 		verdictStr := "allow"
-		if verdict != policy.Allow {
+		if denied {
 			verdictStr = "deny"
 		}
 		if logErr := d.audit.Log(audit.Event{
@@ -257,7 +264,7 @@ func (d *DNSInterceptor) HandleQuery(query []byte) ([]byte, error) {
 		}
 	}
 
-	if verdict != policy.Allow {
+	if denied {
 		return BuildNXDOMAIN(query)
 	}
 
@@ -275,17 +282,10 @@ func (d *DNSInterceptor) HandleQuery(query []byte) ([]byte, error) {
 	return resp, nil
 }
 
-// evaluate checks the DNS domain against the policy engine. Uses
-// EvaluateWithProtocol with protocol "dns" so dns-specific rules match.
-func (d *DNSInterceptor) evaluate(domain string) policy.Verdict {
-	eng := d.engine.Load()
-	// Use EvaluateWithProtocol with "dns" so protocol-scoped rules match.
-	// DNS follows the same deny-then-allow-then-default semantics as
-	// regular evaluation, not the UDP default-deny semantics, because
-	// DNS queries are a known protocol with meaningful domain-level policy.
-	v := eng.EvaluateWithProtocol(domain, 53, ProtoDNS.String())
-	return v
-}
+// NOTE: the old evaluate() method has been removed. DNS resolution is now
+// allowed for all domains except those with explicit deny rules. Policy
+// enforcement (allow/ask/deny) happens at the SOCKS5 CONNECT level where
+// the approval broker can send Telegram notifications for "ask" verdicts.
 
 // forwardToResolver sends the query to the upstream DNS resolver and returns
 // the response.

internal/proxy/dns_test.go

@@ -447,6 +447,9 @@ func TestDNSInterceptor_DeniedDomain(t *testing.T) {
 	eng, err := policy.LoadFromBytes([]byte(`
 [policy]
 default = "deny"
+
+[[deny]]
+destination = "denied.example.com"
 `))
 	if err != nil {
 		t.Fatal(err)
@@ -464,7 +467,7 @@ default = "deny"
 		t.Fatalf("HandleQuery: %v", err)
 	}
 
-	// Response should be NXDOMAIN.
+	// Response should be NXDOMAIN (explicit deny rule).
 	respID := binary.BigEndian.Uint16(resp[0:2])
 	if respID != 0x2222 {
 		t.Errorf("response ID = 0x%04x, want 0x2222", respID)
@@ -543,16 +546,18 @@ protocols = ["dns"]
 		t.Errorf("evil.google.com RCODE = %d, want %d (NXDOMAIN)", rcode, dnsRcodeNXDomain)
 	}
 
-	// Unmatched domain (default deny).
+	// Unmatched domain (default deny). DNS should still resolve so the
+	// connection reaches the SOCKS5 layer where the ask/deny flow runs.
+	// Only explicitly denied domains get NXDOMAIN at the DNS level.
 	query = buildDNSQuery(0x5555, "other.com", dnsTypeA)
 	resp, err = interceptor.HandleQuery(query)
 	if err != nil {
 		t.Fatalf("HandleQuery(other.com): %v", err)
 	}
 	flags = binary.BigEndian.Uint16(resp[2:4])
 	rcode = flags & 0x000F
-	if rcode != dnsRcodeNXDomain {
-		t.Errorf("other.com RCODE = %d, want %d (NXDOMAIN)", rcode, dnsRcodeNXDomain)
+	if rcode == dnsRcodeNXDomain {
+		t.Errorf("other.com RCODE = %d (NXDOMAIN), want forwarded (non-denied domains resolve via DNS)", rcode)
 	}
 }
 
@@ -877,10 +882,13 @@ func TestParseDNSNameEdgeCases(t *testing.T) {
 	})
 }
 
-// TestHandleQueryDenyRule tests that a deny rule causes NXDOMAIN.
+// TestHandleQueryDenyRule tests that an explicit deny rule causes NXDOMAIN.
 func TestHandleQueryDenyRule(t *testing.T) {
 	eng, _ := policy.LoadFromBytes([]byte(`[policy]
 default = "deny"
+
+[[deny]]
+destination = "blocked.example.com"
 `))
 
 	var enginePtr atomic.Pointer[policy.Engine]

internal/proxy/server_test.go

@@ -1203,8 +1203,8 @@ protocols = ["dns"]
 }
 
 func TestUDPAssociateDNSInterceptionNXDOMAIN(t *testing.T) {
-	// DNS interceptor should return NXDOMAIN for denied domains without
-	// contacting the upstream resolver.
+	// DNS interceptor should return NXDOMAIN for explicitly denied domains
+	// without contacting the upstream resolver.
 	eng, err := policy.LoadFromBytes([]byte(`
 [policy]
 default = "deny"
@@ -1213,6 +1213,9 @@ default = "deny"
 destination = "allowed.example.com"
 ports = [53]
 protocols = ["dns"]
+
+[[deny]]
+destination = "denied.example.com"
 `))
 	if err != nil {
 		t.Fatal(err)