fix(gateway): address fallback review feedback
This commit is contained in:
parent
e0db7fde0d
commit
69f8a1f630
7 changed files with 192 additions and 28 deletions
|
|
@ -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`):
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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,7 +56,10 @@ func resolveGatewayListenDecision(
|
|||
return nil, fmt.Errorf("bind %s:%d failed: %w", host, port, bindErr)
|
||||
}
|
||||
|
||||
discoveredCIDRs, discoverErr := discoverGatewayCIDRs()
|
||||
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",
|
||||
|
|
@ -76,6 +77,7 @@ func resolveGatewayListenDecision(
|
|||
bindErr,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
fallbackCIDRs := normalizedCIDRs
|
||||
if len(fallbackCIDRs) == 0 {
|
||||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue