From 69f8a1f63051f2ca1f8882330e92e7c184c081df Mon Sep 17 00:00:00 2001 From: Sakurapainting Date: Thu, 2 Apr 2026 18:36:50 +0800 Subject: [PATCH] fix(gateway): address fallback review feedback --- docs/configuration.md | 34 +++++++++++++++ pkg/config/gateway.go | 8 ++-- pkg/gateway/gateway.go | 15 ++++++- pkg/gateway/network_policy.go | 68 ++++++++++++++++++++---------- pkg/gateway/network_policy_test.go | 24 ++++++++++- pkg/pid/pidfile.go | 35 +++++++++++++++ pkg/pid/pidfile_test.go | 36 ++++++++++++++++ 7 files changed, 192 insertions(+), 28 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 7a5902f58..952385701 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -49,6 +49,40 @@ When omitted, the default is `warn`. Supported values: `debug`, `info`, `warn`, You can also override this with the environment variable `PICOCLAW_LOG_LEVEL`. +### Gateway Host Fallback And CIDR Allowlist + +`gateway.host` defaults to `127.0.0.1`. + +When `gateway.host` is a loopback address (`127.0.0.1`, `::1`, or `localhost`) and bind fails (for example, on boards where loopback is unavailable), PicoClaw automatically: + +1. Falls back to bind on `0.0.0.0`. +2. Enforces a CIDR allowlist for gateway HTTP endpoints. +3. Discovers local interface CIDRs only when `gateway.allowed_cidrs` is empty. + +CIDR sources in fallback mode: + +- If `gateway.allowed_cidrs` is configured, that list is used. +- If `gateway.allowed_cidrs` is empty, discovered local CIDRs are used. +- If no non-loopback CIDR can be discovered, gateway startup fails. + +Loopback clients are always allowed for local administration. + +> **Reverse proxy note:** CIDR checks use connection `RemoteAddr`. If you place the gateway behind a local reverse proxy/tunnel (so requests arrive as loopback), those requests are treated as loopback and pass CIDR filtering at this layer. + +Example: + +```json +{ + "gateway": { + "host": "127.0.0.1", + "port": 18790, + "allowed_cidrs": [ + "192.168.1.0/24", + "10.0.0.0/8" + ] + } +} +``` ### Workspace Layout PicoClaw stores data in your configured workspace (default: `~/.picoclaw/workspace`): diff --git a/pkg/config/gateway.go b/pkg/config/gateway.go index 3d22568c8..22c76cbd7 100644 --- a/pkg/config/gateway.go +++ b/pkg/config/gateway.go @@ -10,10 +10,10 @@ import ( const DefaultGatewayLogLevel = "warn" type GatewayConfig struct { - Host string `json:"host" env:"PICOCLAW_GATEWAY_HOST"` - Port int `json:"port" env:"PICOCLAW_GATEWAY_PORT"` - HotReload bool `json:"hot_reload" env:"PICOCLAW_GATEWAY_HOT_RELOAD"` - LogLevel string `json:"log_level,omitempty" env:"PICOCLAW_LOG_LEVEL"` + Host string `json:"host" env:"PICOCLAW_GATEWAY_HOST"` + Port int `json:"port" env:"PICOCLAW_GATEWAY_PORT"` + HotReload bool `json:"hot_reload" env:"PICOCLAW_GATEWAY_HOT_RELOAD"` + LogLevel string `json:"log_level,omitempty" env:"PICOCLAW_LOG_LEVEL"` AllowedCIDRs []string `json:"allowed_cidrs,omitempty"` } diff --git a/pkg/gateway/gateway.go b/pkg/gateway/gateway.go index 7e9e66cbc..0f10cf9a3 100644 --- a/pkg/gateway/gateway.go +++ b/pkg/gateway/gateway.go @@ -190,6 +190,19 @@ func Run(debug bool, homePath, configPath string, allowEmptyStartup bool) error return err } + if pidData.Host != runningServices.ListenHost || pidData.Port != cfg.Gateway.Port { + updatedPidData, updateErr := pid.UpdatePidFileEndpoint(homePath, runningServices.ListenHost, cfg.Gateway.Port) + if updateErr != nil { + logger.WarnCF("gateway", "Failed to sync pid listen endpoint", map[string]any{ + "error": updateErr.Error(), + "listen_host": runningServices.ListenHost, + "listen_port": cfg.Gateway.Port, + }) + } else { + pidData = updatedPidData + } + } + // Setup manual reload channel for /reload endpoint manualReloadChan := make(chan struct{}, 1) runningServices.manualReloadChan = manualReloadChan @@ -210,7 +223,7 @@ func Run(debug bool, homePath, configPath string, allowEmptyStartup bool) error agentLoop.SetReloadFunc(reloadTrigger) if runningServices.ListenHost == gatewayFallbackBindHost { - fmt.Printf("✓ Gateway started (all interfaces, port %d)\n", cfg.Gateway.Port) + fmt.Printf("✓ Gateway started (all interfaces, port %d)\n", pidData.Port) } else { fmt.Printf("✓ Gateway started on %s\n", runningServices.ListenAddr) } diff --git a/pkg/gateway/network_policy.go b/pkg/gateway/network_policy.go index 4adf0580d..f6441861c 100644 --- a/pkg/gateway/network_policy.go +++ b/pkg/gateway/network_policy.go @@ -19,7 +19,6 @@ const ( type gatewayListenDecision struct { BindHost string - Port int AllowedCIDRs []string AutoFallback bool FallbackReason string @@ -49,7 +48,6 @@ func resolveGatewayListenDecision( if bindErr == nil { return &gatewayListenDecision{ BindHost: host, - Port: port, AllowedCIDRs: normalizedCIDRs, }, nil } @@ -58,23 +56,27 @@ func resolveGatewayListenDecision( return nil, fmt.Errorf("bind %s:%d failed: %w", host, port, bindErr) } - discoveredCIDRs, discoverErr := discoverGatewayCIDRs() - if discoverErr != nil { - return nil, fmt.Errorf( - "loopback bind %s:%d failed: %w; interface discovery failed: %v", - host, - port, - bindErr, - discoverErr, - ) - } - if len(discoveredCIDRs) == 0 { - return nil, fmt.Errorf( - "loopback bind %s:%d failed: %w; no non-loopback interface CIDRs discovered", - host, - port, - bindErr, - ) + var discoveredCIDRs []string + if len(normalizedCIDRs) == 0 { + var discoverErr error + discoveredCIDRs, discoverErr = discoverGatewayCIDRs() + if discoverErr != nil { + return nil, fmt.Errorf( + "loopback bind %s:%d failed: %w; interface discovery failed: %v", + host, + port, + bindErr, + discoverErr, + ) + } + if len(discoveredCIDRs) == 0 { + return nil, fmt.Errorf( + "loopback bind %s:%d failed: %w; no non-loopback interface CIDRs discovered", + host, + port, + bindErr, + ) + } } fallbackCIDRs := normalizedCIDRs @@ -99,7 +101,6 @@ func resolveGatewayListenDecision( return &gatewayListenDecision{ BindHost: gatewayFallbackBindHost, - Port: port, AllowedCIDRs: fallbackCIDRs, AutoFallback: true, FallbackReason: fmt.Sprintf( @@ -174,11 +175,17 @@ func discoverLocalInterfaceCIDRs() ([]string, error) { continue } - masked := ip.Mask(ipNet.Mask) + mask := ipNet.Mask + if len(mask) == 0 { + continue + } + ip = normalizeIPForMask(ip, mask) + + masked := ip.Mask(mask) if masked == nil { continue } - canonical := (&net.IPNet{IP: masked, Mask: ipNet.Mask}).String() + canonical := (&net.IPNet{IP: masked, Mask: mask}).String() if _, ok := seen[canonical]; ok { continue } @@ -194,6 +201,20 @@ func discoverLocalInterfaceCIDRs() ([]string, error) { return out, nil } +func normalizeIPForMask(ip net.IP, mask net.IPMask) net.IP { + switch len(mask) { + case net.IPv4len: + if ip4 := ip.To4(); ip4 != nil { + return ip4 + } + case net.IPv6len: + if ip16 := ip.To16(); ip16 != nil { + return ip16 + } + } + return ip +} + func toIPNet(addr net.Addr) *net.IPNet { switch v := addr.(type) { case *net.IPNet: @@ -253,6 +274,9 @@ func newCIDRAllowlistMiddleware(allowedCIDRs []string) channels.HTTPMiddleware { http.Error(w, "Forbidden", http.StatusForbidden) return } + // Loopback is always allowed for local administration. + // When deployed behind a local reverse proxy/tunnel, forwarded + // external traffic may still appear as loopback at this layer. if ip.IsLoopback() { next.ServeHTTP(w, r) return diff --git a/pkg/gateway/network_policy_test.go b/pkg/gateway/network_policy_test.go index 7c92fc729..b8c63aa42 100644 --- a/pkg/gateway/network_policy_test.go +++ b/pkg/gateway/network_policy_test.go @@ -2,6 +2,7 @@ package gateway import ( "errors" + "net" "net/http" "net/http/httptest" "reflect" @@ -35,6 +36,8 @@ func TestResolveGatewayListenDecisionLoopbackFallbackUsesConfiguredCIDRs(t *test discoverGatewayCIDRs = origDiscover }) + discoverCalled := false + probeGatewayBind = func(host string, _ int) error { switch host { case "127.0.0.1": @@ -46,7 +49,8 @@ func TestResolveGatewayListenDecisionLoopbackFallbackUsesConfiguredCIDRs(t *test } } discoverGatewayCIDRs = func() ([]string, error) { - return []string{"192.168.50.0/24"}, nil + discoverCalled = true + return nil, errors.New("must not be called when configured CIDRs are provided") } decision, err := resolveGatewayListenDecision("127.0.0.1", 18790, []string{"10.0.0.0/8"}) @@ -63,6 +67,9 @@ func TestResolveGatewayListenDecisionLoopbackFallbackUsesConfiguredCIDRs(t *test if !reflect.DeepEqual(decision.AllowedCIDRs, wantCIDRs) { t.Fatalf("decision.AllowedCIDRs = %v, want %v", decision.AllowedCIDRs, wantCIDRs) } + if discoverCalled { + t.Fatal("discoverGatewayCIDRs() called unexpectedly") + } } func TestResolveGatewayListenDecisionLoopbackFallbackUsesDiscoveredCIDRs(t *testing.T) { @@ -198,3 +205,18 @@ func TestCIDRAllowlistMiddlewareInvalidCIDR(t *testing.T) { t.Fatal("middleware expected error for invalid CIDR") } } + +func TestNormalizeIPForMaskIPv4MappedIPv6(t *testing.T) { + ip := net.ParseIP("::ffff:192.168.10.20") + mask := net.CIDRMask(24, 32) + + normalized := normalizeIPForMask(ip, mask) + if got := normalized.String(); got != "192.168.10.20" { + t.Fatalf("normalizeIPForMask() = %q, want %q", got, "192.168.10.20") + } + + masked := normalized.Mask(mask) + if got := (&net.IPNet{IP: masked, Mask: mask}).String(); got != "192.168.10.0/24" { + t.Fatalf("masked network = %q, want %q", got, "192.168.10.0/24") + } +} diff --git a/pkg/pid/pidfile.go b/pkg/pid/pidfile.go index 69d02bc65..273b8a57e 100644 --- a/pkg/pid/pidfile.go +++ b/pkg/pid/pidfile.go @@ -99,6 +99,41 @@ func WritePidFile(homePath, host string, port int) (*PidFileData, error) { return data, nil } +// UpdatePidFileEndpoint updates host/port in the current process pid file +// without rotating the auth token. +func UpdatePidFileEndpoint(homePath, host string, port int) (*PidFileData, error) { + pidMu.Lock() + defer pidMu.Unlock() + + pidPath := pidFilePath(homePath) + data, err := readPidFileUnlocked(pidPath) + if err != nil { + return nil, fmt.Errorf("failed to read pid file: %w", err) + } + if data.PID != os.Getpid() { + return nil, fmt.Errorf("pid file belongs to another process: %d", data.PID) + } + + data.Host = host + data.Port = port + + raw, err := json.MarshalIndent(data, "", " ") + if err != nil { + return nil, fmt.Errorf("failed to marshal pid file: %w", err) + } + + tmp := pidPath + ".tmp" + if err := os.WriteFile(tmp, raw, 0o600); err != nil { + return nil, fmt.Errorf("failed to write pid file: %w", err) + } + if err := os.Rename(tmp, pidPath); err != nil { + os.Remove(tmp) + return nil, fmt.Errorf("failed to rename pid file: %w", err) + } + + return data, nil +} + // ReadPidFileWithCheck reads the PID file and additionally checks if // the recorded process is still alive. Returns nil if the file is // missing, unreadable, or the process has exited. diff --git a/pkg/pid/pidfile_test.go b/pkg/pid/pidfile_test.go index 921f590ad..1545d8a61 100644 --- a/pkg/pid/pidfile_test.go +++ b/pkg/pid/pidfile_test.go @@ -120,6 +120,42 @@ func TestWritePidFileOverwrite(t *testing.T) { } } +func TestUpdatePidFileEndpoint(t *testing.T) { + dir := tmpDir(t) + + data, err := WritePidFile(dir, "127.0.0.1", 18790) + if err != nil { + t.Fatalf("WritePidFile failed: %v", err) + } + + updated, err := UpdatePidFileEndpoint(dir, "0.0.0.0", 18888) + if err != nil { + t.Fatalf("UpdatePidFileEndpoint failed: %v", err) + } + if updated.Host != "0.0.0.0" { + t.Fatalf("updated.Host = %q, want %q", updated.Host, "0.0.0.0") + } + if updated.Port != 18888 { + t.Fatalf("updated.Port = %d, want 18888", updated.Port) + } + if updated.Token != data.Token { + t.Fatal("UpdatePidFileEndpoint must not rotate token") + } +} + +func TestUpdatePidFileEndpointDifferentPID(t *testing.T) { + dir := tmpDir(t) + + other := PidFileData{PID: 99999999, Token: "deadbeef12345678deadbeef12345678", Host: "127.0.0.1", Port: 18790} + raw, _ := json.MarshalIndent(other, "", " ") + os.WriteFile(filepath.Join(dir, pidFileName), raw, 0o600) + + _, err := UpdatePidFileEndpoint(dir, "0.0.0.0", 18888) + if err == nil { + t.Fatal("expected error when pid file belongs to another process") + } +} + // TestWritePidFileStalePID writes a PID file with a non-running PID, then // verifies WritePidFile cleans it up and writes a new one. func TestWritePidFileStalePID(t *testing.T) {