diff --git a/.changeset/modbus-write-owner.md b/.changeset/modbus-write-owner.md new file mode 100644 index 000000000..3de4703cb --- /dev/null +++ b/.changeset/modbus-write-owner.md @@ -0,0 +1,7 @@ +--- +"ftw": patch +--- + +Keep one Modbus writer per device. The proxy defaults to read only. When external writes are enabled, FTW keeps reading but stops all device writes until the owner changes the setting and restarts Core. Restrict proxy requests to configured unit IDs, give driver traffic priority, and close idle clients on shutdown. + +Rename the Home Assistant settings tab to Integrations. Home Assistant MQTT and the Modbus proxy each keep their own section. diff --git a/config.example.yaml b/config.example.yaml index 11b44b812..a2d2b6437 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -157,7 +157,12 @@ homeassistant: # modbus_proxy: # enabled: true # listen: ":1502" # used when the site has one unique Modbus TCP endpoint -# allow_write: false # keep off unless you trust every host on the LAN +# allow_write: false # read-only proxy; FTW owns control +# # true gives LAN clients write ownership; FTW only reads these endpoints. +# # All configured unit IDs on each endpoint share this choice. Unknown unit +# # IDs (including broadcast 0) are blocked. The external controller owns +# # device safety. A disconnect does not return control to FTW; change the +# # setting and restart. Ownership stays in force if the listener fails. # # Two inverters (different host:port) each need their own listen address: # capabilities.modbus.proxy_listen: ":1502" / ":1503" diff --git a/go/cmd/ftw/control_state.go b/go/cmd/ftw/control_state.go index ab8eb85fc..f97581f66 100644 --- a/go/cmd/ftw/control_state.go +++ b/go/cmd/ftw/control_state.go @@ -24,7 +24,7 @@ func newControlStateFromConfig(cfg *config.Config) *control.State { ctrl.InverterGroups = inverterGroupsFrom(cfg.Drivers) ctrl.SupportsPVCurtail = supportsPVCurtailFrom(cfg.Drivers) ctrl.SolarFeedDrivers = solarFeedDriversFrom(cfg.Drivers) - ctrl.DriverLimits = driverLimitsFrom(cfg.Drivers, cfg.Batteries) + ctrl.DriverLimits = driverLimitsFrom(cfg.ModbusControlDrivers(), cfg.Batteries) // Per-phase fuse params for the per-phase clamp inside applyFuseGuard // + forceFuseDischarge. Reads l1_a/l2_a/l3_a from the meter driver // when SiteFuseAmps > 0; otherwise the per-phase clamp is disabled. diff --git a/go/cmd/ftw/main.go b/go/cmd/ftw/main.go index c3f88a0b7..6ee39c978 100644 --- a/go/cmd/ftw/main.go +++ b/go/cmd/ftw/main.go @@ -647,7 +647,8 @@ func main() { // ---- Driver capacities (site, for control + fuse guard) ---- // Loadpoint drivers are filtered out — their battery_capacity_wh // is vehicle capacity, not site-battery capacity. - capacities := driverCapacitiesFrom(cfg.Drivers, cfg.Loadpoints, driverCatalog, true) + cfg.PinModbusProxyOwnership() + capacities := driverCapacitiesFrom(cfg.ModbusControlDrivers(), cfg.Loadpoints, driverCatalog, true) telemetryCapacities := driverCapacitiesFrom(cfg.Drivers, cfg.Loadpoints, driverCatalog, false) observeOnly := config.ObserveOnlyDriverSet(cfg) warnIfEVHasBatteryCapacity(cfg.Drivers, cfg.Loadpoints, driverCatalog) @@ -699,6 +700,7 @@ func main() { return mqttcli.DialWithOptions(c.Host, c.Port, c.Username, c.Password, "ftw-"+name, c.AllowUnverifiedLocal) } modbusEngine := modbuscli.NewEngine() + modbusEngine.SetWriteOwners(cfg.ModbusProxyWriteOwners) reg.ModbusFactory = func(name string, c *config.ModbusConfig) (drivers.ModbusCap, error) { return modbusEngine.Open(c.Host, c.Port, c.UnitID, c.AllowUnverifiedLocal) } @@ -716,6 +718,7 @@ func main() { Host: b.Host, Port: b.Port, AllowUnverifiedLocal: b.AllowUnverifiedLocal, + UnitIDs: b.UnitIDs, }) } proxy, err := modbusEngine.Listen(mbBinds, cfg.ModbusProxy.AllowWrite) @@ -735,7 +738,7 @@ func main() { // WithBatterySoCBounds routes each battery's soc_min/soc_max into the // matching driver's config (discharge_floor_soc/charge_ceil_soc) so the // operator's SoC window reaches the driver, not just the planner. - for _, d := range config.WithBatterySoCBounds(cfg.Drivers, cfg.Batteries) { + for _, d := range config.WithBatterySoCBounds(cfg.ModbusControlDrivers(), cfg.Batteries) { if d.Disabled { slog.Info("driver skipped (disabled)", "name", d.Name) continue @@ -992,13 +995,14 @@ func main() { // ---- Apply saved configuration ---- // Settings commit to SQLite before this callback applies them. applyConfigChange := func(newCfg, oldCfg *config.Config) { + newCfg.ModbusProxyWriteOwners = oldCfg.ModbusProxyWriteOwners forecastConfigMu.Lock() defer forecastConfigMu.Unlock() // Driver paths are already resolved by config.Load; no extra // work needed here. Re-apply the battery SoC-window → driver // config mapping so a hot-edited soc_max reaches the driver too. reg.Reload(ctx, - config.WithBatterySoCBounds(newCfg.Drivers, newCfg.Batteries), + config.WithBatterySoCBounds(newCfg.ModbusControlDrivers(), newCfg.Batteries), newCfg.Site.TroubleshootingMode) // Refresh capacities — mutate the existing map in place so // Deps.Capacities (a map header captured at init) sees the @@ -1022,7 +1026,7 @@ func main() { } else { driverCatalog = reloadCatalog } - for k, v := range driverCapacitiesFrom(newCfg.Drivers, newCfg.Loadpoints, reloadCatalog, true) { + for k, v := range driverCapacitiesFrom(newCfg.ModbusControlDrivers(), newCfg.Loadpoints, reloadCatalog, true) { capacities[k] = v } for k, v := range driverCapacitiesFrom(newCfg.Drivers, newCfg.Loadpoints, reloadCatalog, false) { @@ -1040,7 +1044,7 @@ func main() { ctrl.InverterGroups = inverterGroupsFrom(newCfg.Drivers) ctrl.SupportsPVCurtail = supportsPVCurtailFrom(newCfg.Drivers) ctrl.SolarFeedDrivers = solarFeedDriversFrom(newCfg.Drivers) - ctrl.DriverLimits = driverLimitsFrom(newCfg.Drivers, newCfg.Batteries) + ctrl.DriverLimits = driverLimitsFrom(newCfg.ModbusControlDrivers(), newCfg.Batteries) // Fuse params + safety margin: previously startup-only. // Hot-reload them so operators can tune the per-phase margin // from the UI without restarting (e.g. raising it after the diff --git a/go/cmd/ftw/modbus_owner_test.go b/go/cmd/ftw/modbus_owner_test.go new file mode 100644 index 000000000..1675e7934 --- /dev/null +++ b/go/cmd/ftw/modbus_owner_test.go @@ -0,0 +1,28 @@ +package main + +import ( + "testing" + + "github.com/srcfl/ftw/go/internal/config" +) + +func TestExternalModbusOwnerKeepsTelemetryButLeavesPlannerAndDispatch(t *testing.T) { + cfg := &config.Config{ + ModbusProxy: &config.ModbusProxy{Enabled: true, AllowWrite: true}, + Drivers: []config.Driver{{Name: "sungrow", Lua: "drivers/sungrow.lua", BatteryCapacityWh: 10000, + Capabilities: config.Capabilities{Modbus: &config.ModbusConfig{Host: "127.0.0.1", Port: 502, UnitID: 1}}}}, + } + cfg.PinModbusProxyOwnership() + if len(driverCapacitiesFrom(cfg.ModbusControlDrivers(), nil, evCatalog(), true)) != 0 { + t.Fatal("external battery remains in planner pool") + } + if got := driverCapacitiesFrom(cfg.Drivers, nil, evCatalog(), false); got["sungrow"] != 10000 { + t.Fatalf("telemetry capacity lost: %v", got) + } + if len(driverLimitsFrom(cfg.ModbusControlDrivers(), cfg.Batteries)) != 0 { + t.Fatal("external battery remains in dispatch limits") + } + if !config.ObserveOnlyDriverSet(cfg)["sungrow"] { + t.Fatal("watchdog does not see read-only owner") + } +} diff --git a/go/internal/api/api_drivers_debug.go b/go/internal/api/api_drivers_debug.go index 52e75e2e3..a36eddacd 100644 --- a/go/internal/api/api_drivers_debug.go +++ b/go/internal/api/api_drivers_debug.go @@ -197,6 +197,7 @@ func (s *Server) configuredDriver(name string) (config.Driver, bool) { } for _, d := range s.deps.Cfg.Drivers { if d.Name == name { + d.ObserveOnly = d.ObserveOnly || s.deps.Cfg.ModbusProxyOwnsWrites(d) return d, true } } diff --git a/go/internal/api/api_modbus_owner_test.go b/go/internal/api/api_modbus_owner_test.go new file mode 100644 index 000000000..1f42d79a0 --- /dev/null +++ b/go/internal/api/api_modbus_owner_test.go @@ -0,0 +1,29 @@ +package api + +import ( + "net/http" + "testing" + + "github.com/srcfl/ftw/go/internal/config" +) + +func TestDriverControlReportsExternalModbusOwnership(t *testing.T) { + srv, _ := controlServerWithLuaConfig(t, controlProbeLua, config.Driver{ + Name: "heat", ObserveOnly: true, + Capabilities: config.Capabilities{Modbus: &config.ModbusConfig{Host: "127.0.0.1", Port: 502, UnitID: 1}}, + }) + // The setting belongs to the endpoint; it is not a saved observe_only + // flag on one driver that another driver could omit. + srv.deps.Cfg.Drivers[0].ObserveOnly = false + srv.deps.Cfg.ModbusProxy = &config.ModbusProxy{Enabled: true, AllowWrite: true} + srv.deps.Cfg.PinModbusProxyOwnership() + rec := post(t, srv, "/api/drivers/heat/control", `{"control":"set_offset","value":2,"duration_s":600}`) + if rec.Code != http.StatusForbidden { + t.Fatalf("external owner POST = %d: %s", rec.Code, rec.Body.String()) + } + srv.deps.Cfg.ModbusProxy.AllowWrite = false + rec = post(t, srv, "/api/drivers/heat/control", `{"control":"set_offset","value":2,"duration_s":600}`) + if rec.Code != http.StatusForbidden { + t.Fatalf("save returned ownership before restart: %d", rec.Code) + } +} diff --git a/go/internal/config/config.go b/go/internal/config/config.go index 6eb5f2e5f..a215a7c1b 100644 --- a/go/internal/config/config.go +++ b/go/internal/config/config.go @@ -59,6 +59,9 @@ type Config struct { ConfigDatabase string `yaml:"config_database,omitempty" json:"-"` Revision int64 `yaml:"-" json:"-"` LANPasswordHash string `yaml:"-" json:"-"` + // Startup write ownership stays in force until restart, including after + // saving a different proxy mode or a failed listener start. + ModbusProxyWriteOwners map[string]bool `yaml:"-" json:"-"` } // OCPP configures the built-in OCPP 1.6J and 2.0.1 Central System. Chargers connect to @@ -1352,6 +1355,7 @@ func (c Config) MaskSecrets() Config { // wherever the incoming value is empty (the UI sends "" for masked fields). // Call this before saving a config received from the API. func (incoming *Config) PreserveMaskedSecrets(existing *Config) { + incoming.ModbusProxyWriteOwners = existing.ModbusProxyWriteOwners incoming.ConfigDatabase = existing.ConfigDatabase incoming.Revision = existing.Revision incoming.LANPasswordHash = existing.LANPasswordHash diff --git a/go/internal/config/modbus_ownership_test.go b/go/internal/config/modbus_ownership_test.go new file mode 100644 index 000000000..8a7c0896b --- /dev/null +++ b/go/internal/config/modbus_ownership_test.go @@ -0,0 +1,85 @@ +package config + +import ( + "reflect" + "testing" +) + +func proxyOwnershipConfig() *Config { + return &Config{ + ModbusProxy: &ModbusProxy{Enabled: true, AllowWrite: true}, + Drivers: []Driver{ + {Name: "battery", BatteryCapacityWh: 10000, Capabilities: Capabilities{Modbus: &ModbusConfig{Host: "10.0.0.1", UnitID: 1}}}, + {Name: "meter", Capabilities: Capabilities{Modbus: &ModbusConfig{Host: "10.0.0.1", Port: 502, UnitID: 2}}}, + {Name: "mqtt", Capabilities: Capabilities{MQTT: &MQTTConfig{Host: "10.0.0.2"}}}, + }, + } +} + +func TestModbusProxyOwnershipSurvivesSavedModeChanges(t *testing.T) { + cfg := proxyOwnershipConfig() + cfg.PinModbusProxyOwnership() + cfg.ModbusProxy.AllowWrite = false + if got := ObserveOnlyDriverSet(cfg); !got["battery"] || !got["meter"] || got["mqtt"] { + t.Fatalf("active ownership changed before restart: %v", got) + } + runtime := cfg.ModbusControlDrivers() + if !runtime[0].ObserveOnly || !runtime[1].ObserveOnly || runtime[2].ObserveOnly { + t.Fatalf("runtime drivers: %+v", runtime) + } + if cfg.Drivers[0].ObserveOnly || cfg.Drivers[1].ObserveOnly { + t.Fatal("derived flags changed saved settings") + } + incoming := proxyOwnershipConfig() + incoming.ModbusProxy.Enabled = false + incoming.PreserveMaskedSecrets(cfg) + if !incoming.ModbusProxyOwnsWrites(incoming.Drivers[0]) { + t.Fatal("settings save lost active ownership") + } + incoming.PinModbusProxyOwnership() + if len(ObserveOnlyDriverSet(incoming)) != 0 { + t.Fatal("restart did not return ownership to FTW") + } +} + +func TestModbusProxyReadOnlyDoesNotTakeControlOnSave(t *testing.T) { + cfg := proxyOwnershipConfig() + cfg.ModbusProxy.AllowWrite = false + cfg.PinModbusProxyOwnership() + cfg.ModbusProxy.AllowWrite = true + if len(ObserveOnlyDriverSet(cfg)) != 0 { + t.Fatal("write ownership changed before restart") + } + cfg.PinModbusProxyOwnership() + if !cfg.ModbusProxyOwnsWrites(cfg.Drivers[0]) { + t.Fatal("restart did not apply external ownership") + } +} + +func TestModbusProxyAllowlistAndRestartFollowEffectiveEndpoints(t *testing.T) { + cfg := proxyOwnershipConfig() + binds, err := cfg.ModbusProxyBinds() + if err != nil { + t.Fatal(err) + } + if len(binds) != 1 || !reflect.DeepEqual(binds[0].UnitIDs, []uint8{1, 2}) { + t.Fatalf("binds: %+v", binds) + } + other := proxyOwnershipConfig() + other.Drivers[0], other.Drivers[1] = other.Drivers[1], other.Drivers[0] + if got := RestartRequiredFor(cfg, other); len(got) != 0 { + t.Fatalf("driver order changed effective listener: %v", got) + } + other.Drivers[0].Capabilities.Modbus.UnitID = 3 + if got := RestartRequiredFor(cfg, other); len(got) == 0 { + t.Fatal("unit allowlist change needs restart") + } +} + +func TestUnusedModbusProxySettingsDoNotRequireRestart(t *testing.T) { + old := &Config{} + newCfg := &Config{ModbusProxy: &ModbusProxy{Listen: ":1502"}} + if got := RestartRequiredFor(old, newCfg); len(got) != 0 { + t.Fatalf("disabled defaults require restart: %v", got) + } +} diff --git a/go/internal/config/modbus_proxy.go b/go/internal/config/modbus_proxy.go index 207245dbb..d49fb1eb8 100644 --- a/go/internal/config/modbus_proxy.go +++ b/go/internal/config/modbus_proxy.go @@ -4,6 +4,7 @@ import ( "fmt" "net" "reflect" + "sort" "strconv" "strings" ) @@ -16,9 +17,11 @@ const DefaultModbusProxyListen = ":1502" // integrations can share the socket FTW already holds. Off by default: // Modbus TCP has no authentication, and writes bypass the control loop. type ModbusProxy struct { - Enabled bool `yaml:"enabled" json:"enabled"` - Listen string `yaml:"listen,omitempty" json:"listen,omitempty"` - AllowWrite bool `yaml:"allow_write,omitempty" json:"allow_write,omitempty"` + Enabled bool `yaml:"enabled" json:"enabled"` + Listen string `yaml:"listen,omitempty" json:"listen,omitempty"` + // AllowWrite gives external clients write ownership. FTW only reads + // the exposed endpoints until this setting changes and Core restarts. + AllowWrite bool `yaml:"allow_write,omitempty" json:"allow_write,omitempty"` } // ModbusProxyBind is one listener attached to a driver Modbus TCP endpoint. @@ -27,6 +30,50 @@ type ModbusProxyBind struct { Host string Port int AllowUnverifiedLocal bool + UnitIDs []uint8 +} + +func modbusEndpointKey(mb *ModbusConfig) string { + port := mb.Port + if port == 0 { + port = 502 + } + return net.JoinHostPort(mb.Host, strconv.Itoa(port)) +} + +// PinModbusProxyOwnership records the selected owner before any driver starts. +// A missing listener or client does not hand control back to FTW. +func (c *Config) PinModbusProxyOwnership() { + c.ModbusProxyWriteOwners = make(map[string]bool) + if !c.ModbusProxy.On() || !c.ModbusProxy.AllowWrite { + return + } + for _, d := range c.Drivers { + if mb := d.EffectiveModbus(); mb != nil && !d.Disabled { + c.ModbusProxyWriteOwners[modbusEndpointKey(mb)] = true + } + } +} + +func (c *Config) ModbusProxyOwnsWrites(d Driver) bool { + mb := d.EffectiveModbus() + if c == nil || mb == nil { + return false + } + if c.ModbusProxyWriteOwners != nil { + return c.ModbusProxyWriteOwners[modbusEndpointKey(mb)] + } + return !d.Disabled && c.ModbusProxy.On() && c.ModbusProxy.AllowWrite +} + +// ModbusControlDrivers makes a runtime copy without saving derived observe_only +// flags into the user's settings. Telemetry remains available in either mode. +func (c *Config) ModbusControlDrivers() []Driver { + out := append([]Driver(nil), c.Drivers...) + for i := range out { + out[i].ObserveOnly = out[i].ObserveOnly || c.ModbusProxyOwnsWrites(out[i]) + } + return out } // On reports whether the proxy should bind. Nil-safe. @@ -65,6 +112,7 @@ func (c *Config) ModbusProxyBinds() ([]ModbusProxyBind, error) { port int allowUnverified bool drivers []string + units map[uint8]bool } byKey := map[string]*ep{} order := []string{} @@ -83,7 +131,7 @@ func (c *Config) ModbusProxyBinds() ([]ModbusProxyBind, error) { key := net.JoinHostPort(mb.Host, strconv.Itoa(port)) e := byKey[key] if e == nil { - e = &ep{host: mb.Host, port: port, allowUnverified: d.Capabilities.AllowUnverifiedLocal} + e = &ep{host: mb.Host, port: port, allowUnverified: d.Capabilities.AllowUnverifiedLocal, units: make(map[uint8]bool)} byKey[key] = e order = append(order, key) } @@ -91,6 +139,14 @@ func (c *Config) ModbusProxyBinds() ([]ModbusProxyBind, error) { e.allowUnverified = true } e.drivers = append(e.drivers, d.Name) + unit := mb.UnitID + if unit == 0 { + unit = 1 + } + if unit < 1 || unit > 247 { + return nil, fmt.Errorf("driver %q: modbus unit_id must be 1..247", d.Name) + } + e.units[uint8(unit)] = true pl := strings.TrimSpace(mb.ProxyListen) if pl == "" { continue @@ -121,6 +177,7 @@ func (c *Config) ModbusProxyBinds() ([]ModbusProxyBind, error) { } used := map[string]string{} + sort.Strings(order) out := make([]ModbusProxyBind, 0, len(order)) for _, key := range order { e := byKey[key] @@ -132,11 +189,17 @@ func (c *Config) ModbusProxyBinds() ([]ModbusProxyBind, error) { return nil, fmt.Errorf("modbus_proxy: listen %s used by both %s and %s", listen, other, key) } used[listen] = key + units := make([]uint8, 0, len(e.units)) + for unit := range e.units { + units = append(units, unit) + } + sort.Slice(units, func(i, j int) bool { return units[i] < units[j] }) out = append(out, ModbusProxyBind{ Listen: listen, Host: e.host, Port: e.port, AllowUnverifiedLocal: e.allowUnverified, + UnitIDs: units, }) } return out, nil @@ -164,7 +227,12 @@ func NormalizeListenAddr(s string) (string, error) { func modbusProxyRestartReasons(oldCfg, newCfg *Config) []string { var reasons []string - if !reflect.DeepEqual(oldCfg.ModbusProxy, newCfg.ModbusProxy) { + if !oldCfg.ModbusProxy.On() && !newCfg.ModbusProxy.On() { + return reasons + } + if oldCfg.ModbusProxy.On() != newCfg.ModbusProxy.On() || + oldCfg.ModbusProxy.ListenAddr() != newCfg.ModbusProxy.ListenAddr() || + oldCfg.ModbusProxy.AllowWrite != newCfg.ModbusProxy.AllowWrite { reasons = append(reasons, "modbus_proxy — TCP listener binds at startup") } if oldCfg.ModbusProxy.On() || newCfg.ModbusProxy.On() { @@ -175,31 +243,7 @@ func modbusProxyRestartReasons(oldCfg, newCfg *Config) []string { return reasons } -type proxySig struct { - Host, Listen string - Port int - AllowUnverified bool -} - -func modbusProxySignature(c *Config) []proxySig { - if c == nil { - return nil - } - var out []proxySig - for _, d := range c.Drivers { - if d.Disabled { - continue - } - mb := d.EffectiveModbus() - if mb == nil { - continue - } - out = append(out, proxySig{ - Host: mb.Host, - Port: mb.Port, - Listen: mb.ProxyListen, - AllowUnverified: d.Capabilities.AllowUnverifiedLocal, - }) - } - return out +func modbusProxySignature(c *Config) []ModbusProxyBind { + binds, _ := c.ModbusProxyBinds() + return binds } diff --git a/go/internal/config/observe_only.go b/go/internal/config/observe_only.go index 9acc79a1c..2d396ed37 100644 --- a/go/internal/config/observe_only.go +++ b/go/internal/config/observe_only.go @@ -9,7 +9,7 @@ func ObserveOnlyDriverSet(cfg *Config) map[string]bool { return out } for _, d := range cfg.Drivers { - if d.ObserveOnly { + if d.ObserveOnly || cfg.ModbusProxyOwnsWrites(d) { out[d.Name] = true } } diff --git a/go/internal/drivers/host.go b/go/internal/drivers/host.go index 0af09602d..4b8636a31 100644 --- a/go/internal/drivers/host.go +++ b/go/internal/drivers/host.go @@ -125,6 +125,9 @@ type HostEnv struct { // this so a buggy driver_fingerprint cannot reconfigure hardware: bundled // drivers otherwise have allowWrite as a no-op. ProbeReadOnly bool + // DeviceReadOnly blocks device writes in every Lua phase when an + // external Modbus controller owns the endpoint. + DeviceReadOnly bool // BatteryCapacityWh mirrors the operator's `battery_capacity_wh` // declaration for this driver. Zero means "no physical battery @@ -261,6 +264,9 @@ func (h *HostEnv) allowAuthPost(rawURL string) bool { } func (h *HostEnv) allowWrite(permission string) error { + if h.DeviceReadOnly { + return fmt.Errorf("%s: external Modbus client owns control; FTW only reads", permission) + } if h.ProbeReadOnly { return fmt.Errorf("%s: fingerprint probe cannot write", permission) } diff --git a/go/internal/drivers/modbus_owner_test.go b/go/internal/drivers/modbus_owner_test.go new file mode 100644 index 000000000..c4e81ca29 --- /dev/null +++ b/go/internal/drivers/modbus_owner_test.go @@ -0,0 +1,80 @@ +package drivers + +import ( + "context" + "errors" + "sync/atomic" + "testing" + "time" + + "github.com/srcfl/ftw/go/internal/config" + "github.com/srcfl/ftw/go/internal/telemetry" +) + +type externallyOwnedModbus struct { + writes atomic.Int32 + reads chan struct{} +} + +func (m *externallyOwnedModbus) Read(uint16, uint16, int32) ([]uint16, error) { + select { + case m.reads <- struct{}{}: + default: + } + return []uint16{42}, nil +} +func (m *externallyOwnedModbus) WriteSingle(uint16, uint16) error { m.writes.Add(1); return nil } +func (m *externallyOwnedModbus) WriteMulti(uint16, []uint16) error { m.writes.Add(1); return nil } +func (m *externallyOwnedModbus) Close() error { return nil } +func (m *externallyOwnedModbus) ReadOnly() bool { return true } + +func TestExternalModbusOwnerBlocksInitPollCommandAndRecoveryWrites(t *testing.T) { + path := writeTestDriver(t, ` +function driver_init(config) + host.set_poll_interval(100) + local err = host.modbus_write(10, 1) + if not err then error("init was allowed to write") end +end +function driver_poll() + local err = host.modbus_write_multi(10, {1, 2}) + if not err then error("poll was allowed to write") end + local regs = host.modbus_read(1, 1, "holding") + host.emit("meter", {w=regs[1]}) + return 3600 +end +function driver_command(cmd) host.emit_metric("command_invoked", 1) end +function driver_default_mode() host.emit_metric("default_invoked", 1) end +function driver_cleanup() host.emit_metric("cleanup_invoked", 1) end +`) + tel := telemetry.NewStore() + reg := NewRegistry(tel) + mb := &externallyOwnedModbus{reads: make(chan struct{}, 1)} + reg.ModbusFactory = func(string, *config.ModbusConfig) (ModbusCap, error) { return mb, nil } + ctx := context.Background() + cfg := config.Driver{Name: "external", Lua: path, Capabilities: config.Capabilities{Modbus: &config.ModbusConfig{Host: "127.0.0.1", Port: 502, UnitID: 1}}} + if err := reg.Add(ctx, cfg); err != nil { + t.Fatal(err) + } + defer reg.Remove("external") + select { + case <-mb.reads: + case <-time.After(time.Second): + t.Fatal("read-only driver stopped polling") + } + if err := reg.Send(ctx, "external", []byte(`{"action":"set"}`)); !errors.Is(err, ErrObserveOnly) { + t.Fatalf("command: %v", err) + } + if err := reg.SendDefault(ctx, "external"); !errors.Is(err, ErrObserveOnly) { + t.Fatalf("default: %v", err) + } + reg.Remove("external") + if mb.writes.Load() != 0 { + t.Fatal("FTW wrote while external clients owned control") + } + metrics := emittedHooks(tel) + for _, key := range []string{"command_invoked", "default_invoked", "cleanup_invoked"} { + if metrics["external:"+key] { + t.Fatalf("external owner ran %s", key) + } + } +} diff --git a/go/internal/drivers/registry.go b/go/internal/drivers/registry.go index 504e2a5e1..962ef00b2 100644 --- a/go/internal/drivers/registry.go +++ b/go/internal/drivers/registry.go @@ -579,6 +579,10 @@ func (r *Registry) add(ctx context.Context, cfg config.Driver, startupDefault bo return fmt.Errorf("modbus capability: %w", err) } env.WithModbus(cap) + if policy, ok := cap.(interface{ ReadOnly() bool }); ok && policy.ReadOnly() { + cfg.ObserveOnly = true + env.DeviceReadOnly = true + } env.SetEndpoint(fmt.Sprintf("modbus://%s:%d", dialCfg.Host, dialCfg.Port)) if r.ARPLookup != nil { if mac, ok := r.ARPLookup(dialCfg.Host); ok { diff --git a/go/internal/modbus/client.go b/go/internal/modbus/client.go index 573dbbb34..60fcbf0b2 100644 --- a/go/internal/modbus/client.go +++ b/go/internal/modbus/client.go @@ -10,6 +10,7 @@ import ( "strconv" "strings" "sync" + "sync/atomic" "syscall" "time" @@ -51,6 +52,8 @@ type sharedConn struct { addr string allowUnverifiedLocal bool requestTimeout time.Duration + externalWriteOwner bool // guarded by mu; only changes at process startup + localWaiters atomic.Int32 // activeUnitID mirrors what the live client was last programmed // with, so capabilities with different unit ids can share the @@ -249,7 +252,9 @@ func releaseConn(c *Capability) bool { // Read — implements drivers.ModbusCap. Reconnects once on transport error. func (c *Capability) Read(addr, count uint16, kind int32) ([]uint16, error) { conn := c.conn + conn.localWaiters.Add(1) conn.mu.Lock() + conn.localWaiters.Add(-1) defer conn.mu.Unlock() if err := conn.ensureClient(); err != nil { return nil, err @@ -299,8 +304,13 @@ func markTransport(err error) error { // WriteSingle — implements drivers.ModbusCap. Reconnects once on transport error. func (c *Capability) WriteSingle(addr, value uint16) error { conn := c.conn + conn.localWaiters.Add(1) conn.mu.Lock() + conn.localWaiters.Add(-1) defer conn.mu.Unlock() + if conn.externalWriteOwner { + return drivers.ErrObserveOnly + } if err := conn.ensureClient(); err != nil { return err } @@ -326,8 +336,13 @@ func (c *Capability) WriteSingle(addr, value uint16) error { // WriteMulti — implements drivers.ModbusCap. Reconnects once on transport error. func (c *Capability) WriteMulti(addr uint16, values []uint16) error { conn := c.conn + conn.localWaiters.Add(1) conn.mu.Lock() + conn.localWaiters.Add(-1) defer conn.mu.Unlock() + if conn.externalWriteOwner { + return drivers.ErrObserveOnly + } if err := conn.ensureClient(); err != nil { return err } @@ -351,35 +366,47 @@ func (c *Capability) WriteMulti(addr uint16, values []uint16) error { } // executePDU runs one raw PDU on the shared session. Exception PDUs are -// returned as data so a proxy can forward them. Transport errors follow -// the same reconnect-once policy as Read. unitID is taken from the -// request so one TCP session can serve several slaves. +// returned as data so a proxy can forward them. Local requests have priority; +// proxy traffic gets one bounded attempt and leaves reconnect to drivers. func (c *Capability) executePDU(unitID uint8, pdu []byte) ([]byte, error) { conn := c.conn - conn.mu.Lock() - defer conn.mu.Unlock() - if err := conn.ensureClient(); err != nil { - return nil, err - } - conn.applyUnit(int(unitID)) - res, err := conn.client.roundTrip(unitID, pdu) - if err == nil { - conn.noteLiveResponse() - return res, nil + // Proxy work never queues ahead of driver reads or writes. A pending + // local request wins, and a busy client gets a Modbus busy exception. + if conn.localWaiters.Load() > 0 || !conn.mu.TryLock() { + return exceptionPDU(pdu[0], modbusExcBusy), nil } - if !isTransportError(err) { - conn.noteLiveResponse() - return res, err + defer conn.mu.Unlock() + if conn.localWaiters.Load() > 0 { + return exceptionPDU(pdu[0], modbusExcBusy), nil } - if rerr := conn.prepareTransportRetry(); rerr != nil { - return nil, fmt.Errorf("pdu after reconnect: %w (original: %v)", rerr, err) + // Drivers own reconnect. Proxy requests cannot add a second retry or + // repeatedly dial an unavailable device while polls are cooling down. + if conn.client == nil { + return exceptionPDU(pdu[0], modbusExcGWTarget), nil } conn.applyUnit(int(unitID)) - res, err = conn.client.roundTrip(unitID, pdu) + oldTimeout := conn.client.timeout + if oldTimeout > proxyRequestTimeout { + conn.client.timeout = proxyRequestTimeout + } + defer func() { + if conn.client != nil { + conn.client.timeout = oldTimeout + } + }() + res, err := conn.client.roundTrip(unitID, pdu) conn.finishRequest(err) return res, markTransport(err) } +// ReadOnly tells the registry that external clients own this device. The +// transport still checks every write, including tests and lifecycle hooks. +func (c *Capability) ReadOnly() bool { + c.conn.mu.Lock() + defer c.conn.mu.Unlock() + return c.conn.externalWriteOwner +} + // applyUnit programs the handle's unit id into the live client when it // differs from what the session was last set to. Caller holds c.mu and // has ensured the client exists. Unit id 0 keeps whatever the session @@ -550,7 +577,8 @@ func isTransportError(err error) bool { if err == nil { return false } - if errors.Is(err, io.EOF) || errors.Is(err, io.ErrClosedPipe) { + if errors.Is(err, io.EOF) || errors.Is(err, io.ErrClosedPipe) || + errors.Is(err, io.ErrUnexpectedEOF) || errors.Is(err, drivers.ErrModbusTransport) { return true } if errors.Is(err, syscall.ECONNRESET) || errors.Is(err, syscall.EPIPE) || diff --git a/go/internal/modbus/engine.go b/go/internal/modbus/engine.go index b2b3dfc77..b7e6ab594 100644 --- a/go/internal/modbus/engine.go +++ b/go/internal/modbus/engine.go @@ -4,6 +4,7 @@ import ( "errors" "net" "strconv" + "sync" "github.com/srcfl/ftw/go/internal/drivers" ) @@ -12,7 +13,10 @@ import ( // and the optional LAN proxy all Dial through it. The actual one-socket- // per-endpoint pool lives in Dial — master already shares sessions there // (#987) — so Open is a named entry point, not a second pool. -type Engine struct{} +type Engine struct { + mu sync.Mutex + writeOwners map[string]bool +} // NewEngine returns the process Modbus wiring. Session sharing is in Dial. func NewEngine() *Engine { @@ -27,7 +31,43 @@ func (e *Engine) Open(host string, port, unitID int, allowUnverifiedLocal bool) if e == nil { return nil, errors.New("modbus engine is nil") } - return DialWithOptions(host, port, unitID, allowUnverifiedLocal) + cap, err := DialWithOptions(host, port, unitID, allowUnverifiedLocal) + if err != nil { + return nil, err + } + e.mu.Lock() + owned := e.writeOwners[sessionKey(host, port)] + e.mu.Unlock() + if owned { + cap.conn.mu.Lock() + cap.conn.externalWriteOwner = true + cap.conn.mu.Unlock() + } + return cap, nil +} + +// SetWriteOwners installs startup policy even when a listener cannot start. +// Existing handles share the same gate; new probes cannot acquire a writer. +func (e *Engine) SetWriteOwners(owners map[string]bool) { + e.mu.Lock() + e.writeOwners = make(map[string]bool, len(owners)) + for key, owned := range owners { + e.writeOwners[key] = owned + } + e.mu.Unlock() + registryMu.Lock() + var conns []*sharedConn + for key, owned := range owners { + if conn := endpointConns[key]; owned && conn != nil { + conns = append(conns, conn) + } + } + registryMu.Unlock() + for _, conn := range conns { + conn.mu.Lock() + conn.externalWriteOwner = true + conn.mu.Unlock() + } } func (e *Engine) sessionCount() int { diff --git a/go/internal/modbus/proxy.go b/go/internal/modbus/proxy.go index 819218daa..f0096fe23 100644 --- a/go/internal/modbus/proxy.go +++ b/go/internal/modbus/proxy.go @@ -15,13 +15,16 @@ import ( ) const ( - proxyIdleTimeout = 90 * time.Second - proxyMaxPDUSize = 253 - proxyMaxClients = 16 - proxyMaxADULength = 1 + proxyMaxPDUSize // unit ID + PDU - modbusExcIllegalFn = 0x01 - modbusExcGWPath = 0x0A - modbusExcGWTarget = 0x0B + proxyIdleTimeout = 90 * time.Second + proxyRequestTimeout = time.Second + proxyMaxPDUSize = 253 + proxyMaxClients = 16 + proxyMaxADULength = 1 + proxyMaxPDUSize // unit ID + PDU + modbusExcIllegalFn = 0x01 + modbusExcIllegalValue = 0x03 + modbusExcBusy = 0x06 + modbusExcGWPath = 0x0A + modbusExcGWTarget = 0x0B ) // Bind is one proxy listener attached to a backend already in the Engine. @@ -30,11 +33,12 @@ type Bind struct { Host string Port int AllowUnverifiedLocal bool + UnitIDs []uint8 } // Proxy accepts Modbus TCP clients and multiplexes their PDUs onto the // Engine session for that backend. Writes are denied unless allowWrite is -// set: a LAN client writing registers would bypass FTW's control loop. +// set. External write ownership makes FTW read-only on that endpoint. type Proxy struct { engine *Engine allowWrite bool @@ -46,6 +50,10 @@ type Proxy struct { wg sync.WaitGroup closing atomic.Bool sem chan struct{} + clients map[net.Conn]bool + stop chan struct{} + closeOnce sync.Once + closeErr error } // Listen pins each backend session and serves Modbus TCP on Bind.Listen. @@ -58,6 +66,15 @@ func (e *Engine) Listen(binds []Bind, allowWrite bool) (*Proxy, error) { allowWrite: allowWrite, byAddr: make(map[string]*Capability), sem: make(chan struct{}, proxyMaxClients), + clients: make(map[net.Conn]bool), + stop: make(chan struct{}), + } + if allowWrite { + owners := make(map[string]bool, len(binds)) + for _, b := range binds { + owners[sessionKey(b.Host, b.Port)] = true + } + e.SetWriteOwners(owners) } for _, b := range binds { if err := p.addBind(b); err != nil { @@ -72,10 +89,20 @@ func (p *Proxy) addBind(b Bind) error { if b.Listen == "" || b.Host == "" || b.Port < 1 { return fmt.Errorf("modbus proxy bind incomplete: listen=%q host=%q port=%d", b.Listen, b.Host, b.Port) } - pin, err := DialWithOptions(b.Host, b.Port, 1, b.AllowUnverifiedLocal) + if len(b.UnitIDs) == 0 { + return errors.New("modbus proxy bind needs configured unit IDs") + } + for _, unit := range b.UnitIDs { + if unit == 0 || unit > 247 { + return fmt.Errorf("modbus proxy unit ID %d must be 1..247", unit) + } + } + b.UnitIDs = append([]uint8(nil), b.UnitIDs...) + opened, err := p.engine.Open(b.Host, b.Port, int(b.UnitIDs[0]), b.AllowUnverifiedLocal) if err != nil { return fmt.Errorf("modbus proxy pin %s:%d: %w", b.Host, b.Port, err) } + pin := opened.(*Capability) ln, err := net.Listen("tcp", b.Listen) if err != nil { _ = pin.Close() @@ -90,7 +117,8 @@ func (p *Proxy) addBind(b Bind) error { p.listeners = append(p.listeners, ln) p.mu.Unlock() - backend := Bind{Host: b.Host, Port: b.Port, Listen: ln.Addr().String()} + backend := b + backend.Listen = ln.Addr().String() p.wg.Add(1) go p.serve(ln, backend) slog.Info("modbus proxy listening", @@ -102,6 +130,7 @@ func (p *Proxy) addBind(b Bind) error { func (p *Proxy) serve(ln net.Listener, backend Bind) { defer p.wg.Done() + retryDelay := 50 * time.Millisecond for { conn, err := ln.Accept() if err != nil { @@ -109,8 +138,19 @@ func (p *Proxy) serve(ln net.Listener, backend Bind) { return } slog.Warn("modbus proxy accept", "listen", ln.Addr().String(), "err", err) - return + timer := time.NewTimer(retryDelay) + select { + case <-p.stop: + timer.Stop() + return + case <-timer.C: + } + if retryDelay < time.Second { + retryDelay *= 2 + } + continue } + retryDelay = 50 * time.Millisecond select { case p.sem <- struct{}{}: default: @@ -118,10 +158,24 @@ func (p *Proxy) serve(ln net.Listener, backend Bind) { _ = conn.Close() continue } + p.mu.Lock() + if p.closing.Load() { + p.mu.Unlock() + _ = conn.Close() + <-p.sem + return + } + p.clients[conn] = true p.wg.Add(1) + p.mu.Unlock() go func(c net.Conn) { defer p.wg.Done() defer func() { <-p.sem }() + defer func() { + p.mu.Lock() + delete(p.clients, c) + p.mu.Unlock() + }() p.handleClient(c, backend) }(conn) } @@ -148,12 +202,22 @@ func (p *Proxy) handleClient(conn net.Conn, backend Bind) { func (p *Proxy) forward(backend Bind, unitID uint8, pdu []byte) []byte { fc := pdu[0] + allowed := false + for _, unit := range backend.UnitIDs { + allowed = allowed || unit == unitID + } + if !allowed { + return exceptionPDU(fc, modbusExcGWPath) + } if isModbusWrite(fc) && !p.allowWrite { return exceptionPDU(fc, modbusExcIllegalFn) } if !isModbusRead(fc) && !isModbusWrite(fc) { return exceptionPDU(fc, modbusExcIllegalFn) } + if !validProxyPDU(pdu) { + return exceptionPDU(fc, modbusExcIllegalValue) + } p.mu.Lock() cap := p.byAddr[sessionKey(backend.Host, backend.Port)] p.mu.Unlock() @@ -194,27 +258,34 @@ func (p *Proxy) Close() error { if p == nil { return nil } - p.closing.Store(true) - p.mu.Lock() - listeners := p.listeners - p.listeners = nil - pins := p.pins - p.pins = nil - p.byAddr = nil - p.mu.Unlock() - var first error - for _, ln := range listeners { - if err := ln.Close(); err != nil && first == nil { - first = err + p.closeOnce.Do(func() { + p.closing.Store(true) + close(p.stop) + p.mu.Lock() + listeners := p.listeners + p.listeners = nil + pins := p.pins + p.pins = nil + p.byAddr = nil + for conn := range p.clients { + _ = conn.Close() } - } - p.wg.Wait() - for _, pin := range pins { - if err := pin.Close(); err != nil && first == nil { - first = err + p.mu.Unlock() + var first error + for _, ln := range listeners { + if err := ln.Close(); err != nil && first == nil { + first = err + } } - } - return first + p.wg.Wait() + for _, pin := range pins { + if err := pin.Close(); err != nil && first == nil { + first = err + } + } + p.closeErr = first + }) + return p.closeErr } func isModbusRead(fc byte) bool { @@ -228,13 +299,57 @@ func isModbusRead(fc byte) bool { func isModbusWrite(fc byte) bool { switch fc { - case 0x05, 0x06, 0x0F, 0x10, 0x15, 0x16, 0x17: + case 0x05, 0x06, 0x0F, 0x10, 0x16, 0x17: return true default: return false } } +// Check shape and protocol limits before an unauthenticated request can take +// the shared socket. Malformed requests must not interrupt driver polling. +func validProxyPDU(pdu []byte) bool { + word := func(offset int) int { return int(binary.BigEndian.Uint16(pdu[offset:])) } + rangeOK := func(offset, count, max int) bool { + return count > 0 && count <= max && word(offset)+count <= 65536 + } + switch pdu[0] { + case 0x01, 0x02, 0x03, 0x04: + if len(pdu) != 5 { + return false + } + max := 125 + if pdu[0] <= 2 { + max = 2000 + } + return rangeOK(1, word(3), max) + case 0x05: + return len(pdu) == 5 && (word(3) == 0 || word(3) == 0xff00) + case 0x06: + return len(pdu) == 5 + case 0x0f, 0x10: + if len(pdu) < 7 { + return false + } + count, max, bytes := word(3), 123, word(3)*2 + if pdu[0] == 0x0f { + max, bytes = 1968, (count+7)/8 + } + return rangeOK(1, count, max) && int(pdu[5]) == bytes && len(pdu) == 6+bytes + case 0x16: + return len(pdu) == 7 + case 0x17: + if len(pdu) < 12 { + return false + } + bytes := word(7) * 2 + return rangeOK(1, word(3), 125) && rangeOK(5, word(7), 121) && + int(pdu[9]) == bytes && len(pdu) == 10+bytes + default: + return false + } +} + func exceptionPDU(fc, code byte) []byte { return []byte{fc | 0x80, code} } diff --git a/go/internal/modbus/proxy_ownership_test.go b/go/internal/modbus/proxy_ownership_test.go new file mode 100644 index 000000000..41705e51c --- /dev/null +++ b/go/internal/modbus/proxy_ownership_test.go @@ -0,0 +1,340 @@ +package modbus + +import ( + "errors" + "net" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/srcfl/ftw/go/internal/drivers" +) + +func TestProxyWriteOwnershipBlocksAllLocalHandles(t *testing.T) { + slave := startTestSlave(t) + host, port := slave.Addr() + engine := NewEngine() + driver, err := engine.Open(host, port, 1, false) + if err != nil { + t.Fatal(err) + } + defer driver.Close() + proxy, err := engine.Listen([]Bind{{Listen: "127.0.0.1:0", Host: host, Port: port, UnitIDs: []uint8{1, 2}}}, true) + if err != nil { + t.Fatal(err) + } + defer proxy.Close() + probe, err := Dial(host, port, 2) + if err != nil { + t.Fatal(err) + } + defer probe.Close() + for _, cap := range []drivers.ModbusCap{driver, probe} { + if _, err := cap.Read(1, 1, drivers.ModbusHolding); err != nil { + t.Fatal(err) + } + if err := cap.WriteSingle(10, 99); !errors.Is(err, drivers.ErrObserveOnly) { + t.Fatalf("single write: %v", err) + } + if err := cap.WriteMulti(10, []uint16{99, 100}); !errors.Is(err, drivers.ErrObserveOnly) { + t.Fatalf("multi write: %v", err) + } + } + if slave.Writes() != 0 { + t.Fatal("a local write reached the device") + } + pHost, pPort := mustAddr(t, proxy.ListenAddrs()[0]) + client, err := Dial(pHost, pPort, 1) + if err != nil { + t.Fatal(err) + } + defer client.Close() + if err := client.WriteSingle(10, 0xbeef); err != nil { + t.Fatal(err) + } + if slave.Holding(10) != 0xbeef || slave.Accepts() != 1 { + t.Fatal("external write did not use the one backend session") + } + if err := proxy.Close(); err != nil { + t.Fatal(err) + } + if err := driver.WriteSingle(10, 7); !errors.Is(err, drivers.ErrObserveOnly) { + t.Fatalf("closing proxy returned write ownership: %v", err) + } + if _, err := driver.Read(1, 1, drivers.ModbusHolding); err != nil { + t.Fatal(err) + } +} + +func TestProxyReadOnlyKeepsLocalWriter(t *testing.T) { + slave := startTestSlave(t) + host, port := slave.Addr() + engine := NewEngine() + proxy, err := engine.Listen([]Bind{{Listen: "127.0.0.1:0", Host: host, Port: port, UnitIDs: []uint8{1}}}, false) + if err != nil { + t.Fatal(err) + } + defer proxy.Close() + driver, err := engine.Open(host, port, 1, false) + if err != nil { + t.Fatal(err) + } + defer driver.Close() + if err := driver.WriteSingle(10, 7); err != nil { + t.Fatal(err) + } + if slave.Holding(10) != 7 { + t.Fatal("FTW lost write access in read-only proxy mode") + } +} + +func TestProxyFailedListenKeepsExternalOwnership(t *testing.T) { + slave := startTestSlave(t) + host, port := slave.Addr() + occupied, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer occupied.Close() + engine := NewEngine() + if _, err := engine.Listen([]Bind{{Listen: occupied.Addr().String(), Host: host, Port: port, UnitIDs: []uint8{1}}}, true); err == nil { + t.Fatal("expected bind failure") + } + driver, err := engine.Open(host, port, 1, false) + if err != nil { + t.Fatal(err) + } + defer driver.Close() + if err := driver.WriteSingle(10, 7); !errors.Is(err, drivers.ErrObserveOnly) { + t.Fatalf("failed listener returned control: %v", err) + } + if _, err := driver.Read(1, 1, drivers.ModbusHolding); err != nil { + t.Fatal(err) + } +} + +func TestProxyRejectsUnconfiguredUnitsAndMalformedRequests(t *testing.T) { + slave := startTestSlave(t) + host, port := slave.Addr() + engine := NewEngine() + bind := Bind{Listen: "127.0.0.1:0", Host: host, Port: port, UnitIDs: []uint8{1, 2}} + proxy, err := engine.Listen([]Bind{bind}, true) + if err != nil { + t.Fatal(err) + } + defer proxy.Close() + for _, unit := range []uint8{0, 3, 248, 255} { + res := proxy.forward(bind, unit, []byte{6, 0, 10, 0, 7}) + if len(res) != 2 || res[1] != modbusExcGWPath { + t.Fatalf("unit %d: %x", unit, res) + } + } + for _, pdu := range [][]byte{{3}, {3, 0, 1, 0, 0}, {4, 0xff, 0xff, 0, 2}, {6, 0, 1}, {0x10, 0, 1, 0, 2, 2, 0, 1}} { + res := proxy.forward(bind, 1, pdu) + if res[1] != modbusExcIllegalValue { + t.Fatalf("malformed %x: %x", pdu, res) + } + } + if len(slave.Units()) != 0 { + t.Fatal("denied request reached backend") + } + res := proxy.forward(bind, 2, []byte{3, 0, 1, 0, 1}) + if res[0] != 3 { + t.Fatalf("configured unit denied: %x", res) + } + if got := slave.Units(); len(got) != 1 || got[0] != 2 { + t.Fatalf("backend units %v", got) + } +} + +func TestProxyYieldsToLocalRequests(t *testing.T) { + slave := startTestSlave(t) + host, port := slave.Addr() + engine := NewEngine() + driver, err := engine.Open(host, port, 1, false) + if err != nil { + t.Fatal(err) + } + defer driver.Close() + bind := Bind{Listen: "127.0.0.1:0", Host: host, Port: port, UnitIDs: []uint8{1}} + proxy, err := engine.Listen([]Bind{bind}, false) + if err != nil { + t.Fatal(err) + } + defer proxy.Close() + conn := driver.(*Capability).conn + request := []byte{3, 0, 1, 0, 1} + conn.mu.Lock() + res := proxy.forward(bind, 1, request) + conn.mu.Unlock() + if res[1] != modbusExcBusy { + t.Fatalf("occupied socket response: %x", res) + } + conn.mu.Lock() + done := make(chan error, 1) + go func() { + _, err := driver.Read(1, 1, drivers.ModbusHolding) + done <- err + }() + deadline := time.Now().Add(time.Second) + for conn.localWaiters.Load() == 0 { + if time.Now().After(deadline) { + conn.mu.Unlock() + t.Fatal("driver did not queue a read") + } + time.Sleep(time.Millisecond) + } + res = proxy.forward(bind, 1, request) + conn.mu.Unlock() + if res[1] != modbusExcBusy { + t.Fatalf("queued driver response: %x", res) + } + select { + case err := <-done: + if err != nil { + t.Fatal(err) + } + case <-time.After(time.Second): + t.Fatal("driver's read did not finish") + } + if len(slave.Units()) != 1 { + t.Fatal("proxy took a local request's turn") + } +} + +func TestProxyCloseUnblocksIdleClients(t *testing.T) { + slave := startTestSlave(t) + host, port := slave.Addr() + proxy, err := NewEngine().Listen([]Bind{{Listen: "127.0.0.1:0", Host: host, Port: port, UnitIDs: []uint8{1}}}, false) + if err != nil { + t.Fatal(err) + } + defer proxy.Close() + client, err := net.Dial("tcp", proxy.ListenAddrs()[0]) + if err != nil { + t.Fatal(err) + } + defer client.Close() + deadline := time.Now().Add(time.Second) + for { + proxy.mu.Lock() + accepted := len(proxy.clients) == 1 + proxy.mu.Unlock() + if accepted { + break + } + if time.Now().After(deadline) { + t.Fatal("client was not accepted") + } + time.Sleep(time.Millisecond) + } + done := make(chan error, 1) + go func() { done <- proxy.Close() }() + select { + case err := <-done: + if err != nil { + t.Fatal(err) + } + case <-time.After(time.Second): + t.Fatal("idle client stalled shutdown") + } +} + +func TestProxySlowBackendDoesNotRetryOrHoldDriverForFiveSeconds(t *testing.T) { + slave := startTestSlave(t) + host, port := slave.Addr() + engine := NewEngine() + driver, err := engine.Open(host, port, 1, false) + if err != nil { + t.Fatal(err) + } + defer driver.Close() + bind := Bind{Listen: "127.0.0.1:0", Host: host, Port: port, UnitIDs: []uint8{1}} + proxy, err := engine.Listen([]Bind{bind}, false) + if err != nil { + t.Fatal(err) + } + defer proxy.Close() + entered, release := make(chan struct{}), make(chan struct{}) + var blocked atomic.Bool + var releaseOnce sync.Once + t.Cleanup(func() { releaseOnce.Do(func() { close(release) }) }) + slave.mu.Lock() + slave.beforeReply = func() { + if blocked.CompareAndSwap(false, true) { + close(entered) + <-release + } + } + slave.mu.Unlock() + result := make(chan []byte, 1) + go func() { result <- proxy.forward(bind, 1, []byte{3, 0, 1, 0, 1}) }() + select { + case <-entered: + case <-time.After(time.Second): + t.Fatal("proxy did not send the request") + } + select { + case res := <-result: + if res[1] != modbusExcGWTarget { + t.Fatalf("timeout response: %x", res) + } + case <-time.After(3 * time.Second): + t.Fatal("proxy held the shared session for a driver timeout/retry") + } + if slave.Accepts() != 1 { + t.Fatal("proxy reconnected to retry the timed-out read") + } + releaseOnce.Do(func() { close(release) }) + if _, err := driver.Read(1, 1, drivers.ModbusHolding); err != nil { + t.Fatalf("driver could not restore its session: %v", err) + } +} + +type interruptedAcceptListener struct { + net.Listener + once sync.Once +} + +func (l *interruptedAcceptListener) Accept() (net.Conn, error) { + interrupted := false + l.once.Do(func() { interrupted = true }) + if interrupted { + return nil, errors.New("temporary accept failure") + } + return l.Listener.Accept() +} + +func TestProxyAcceptFailureDoesNotStopServing(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + wrapper := &interruptedAcceptListener{Listener: ln} + proxy := &Proxy{ + listeners: []net.Listener{wrapper}, + clients: make(map[net.Conn]bool), + sem: make(chan struct{}, proxyMaxClients), + stop: make(chan struct{}), + } + proxy.wg.Add(1) + go proxy.serve(wrapper, Bind{UnitIDs: []uint8{1}}) + defer proxy.Close() + client, err := net.Dial("tcp", ln.Addr().String()) + if err != nil { + t.Fatal(err) + } + defer client.Close() + client.SetDeadline(time.Now().Add(time.Second)) + if err := writeMBAP(client, 1, 1, []byte{3, 0, 1, 0, 1}); err != nil { + t.Fatal(err) + } + _, _, res, err := readMBAP(client) + if err != nil { + t.Fatalf("listener did not recover: %v", err) + } + if res[1] != modbusExcGWPath { + t.Fatalf("unexpected response: %x", res) + } +} diff --git a/go/internal/modbus/proxy_test.go b/go/internal/modbus/proxy_test.go index a28df1aed..ba4557a49 100644 --- a/go/internal/modbus/proxy_test.go +++ b/go/internal/modbus/proxy_test.go @@ -21,9 +21,10 @@ func TestProxyMultiplexesOntoSharedSession(t *testing.T) { defer driver.Close() proxy, err := engine.Listen([]Bind{{ - Listen: "127.0.0.1:0", - Host: host, - Port: port, + Listen: "127.0.0.1:0", + Host: host, + Port: port, + UnitIDs: []uint8{1}, }}, false) if err != nil { t.Fatalf("listen: %v", err) @@ -76,9 +77,10 @@ func TestProxyAllowWriteForwardsRegisterWrites(t *testing.T) { host, port := slave.Addr() engine := NewEngine() proxy, err := engine.Listen([]Bind{{ - Listen: "127.0.0.1:0", - Host: host, - Port: port, + Listen: "127.0.0.1:0", + Host: host, + Port: port, + UnitIDs: []uint8{1}, }}, true) if err != nil { t.Fatalf("listen: %v", err) @@ -104,9 +106,10 @@ func TestProxyUnknownFunctionIsIllegal(t *testing.T) { host, port := slave.Addr() engine := NewEngine() proxy, err := engine.Listen([]Bind{{ - Listen: "127.0.0.1:0", - Host: host, - Port: port, + Listen: "127.0.0.1:0", + Host: host, + Port: port, + UnitIDs: []uint8{1}, }}, false) if err != nil { t.Fatalf("listen: %v", err) diff --git a/go/internal/modbus/slave_test.go b/go/internal/modbus/slave_test.go index adfdac53d..f4f0085c3 100644 --- a/go/internal/modbus/slave_test.go +++ b/go/internal/modbus/slave_test.go @@ -12,14 +12,15 @@ import ( // testSlave is a tiny Modbus TCP server with holding/input maps. It // records Accept count and unit IDs so sharing tests can see one socket. type testSlave struct { - ln net.Listener - accepts atomic.Int32 - mu sync.Mutex - holding map[uint16]uint16 - input map[uint16]uint16 - unitIDs []uint8 - writes int - closing atomic.Bool + ln net.Listener + accepts atomic.Int32 + mu sync.Mutex + holding map[uint16]uint16 + input map[uint16]uint16 + unitIDs []uint8 + writes int + closing atomic.Bool + beforeReply func() } func startTestSlave(t *testing.T) *testSlave { @@ -102,7 +103,11 @@ func (s *testSlave) serve(c net.Conn) { s.mu.Lock() s.unitIDs = append(s.unitIDs, unit) respPDU := s.handlePDU(pdu) + beforeReply := s.beforeReply s.mu.Unlock() + if beforeReply != nil { + beforeReply() + } resp := make([]byte, 7+len(respPDU)) copy(resp[0:2], hdr[0:2]) diff --git a/go/internal/modbus/tcp_client.go b/go/internal/modbus/tcp_client.go index 1414090d5..5860cfcdc 100644 --- a/go/internal/modbus/tcp_client.go +++ b/go/internal/modbus/tcp_client.go @@ -8,6 +8,7 @@ import ( "net" "time" + "github.com/srcfl/ftw/go/internal/drivers" "github.com/srcfl/ftw/go/internal/mdnsresolve" ) @@ -181,7 +182,7 @@ func (c *tcpClient) roundTrip(unitID uint8, pdu []byte) ([]byte, error) { if c.conn == nil { return nil, io.ErrClosedPipe } - if len(pdu) == 0 { + if len(pdu) == 0 || len(pdu) > proxyMaxPDUSize { return nil, errors.New("modbus empty request pdu") } c.txID++ @@ -203,19 +204,28 @@ func (c *tcpClient) roundTrip(unitID uint8, pdu []byte) ([]byte, error) { return nil, err } if got := binary.BigEndian.Uint16(hdr[0:2]); got != txID { - return nil, fmt.Errorf("modbus transaction id mismatch: got %d want %d", got, txID) + return nil, fmt.Errorf("%w: modbus transaction id mismatch: got %d want %d", drivers.ErrModbusTransport, got, txID) } if proto := binary.BigEndian.Uint16(hdr[2:4]); proto != 0 { - return nil, fmt.Errorf("modbus protocol id mismatch: got %d", proto) + return nil, fmt.Errorf("%w: modbus protocol id mismatch: got %d", drivers.ErrModbusTransport, proto) + } + if hdr[6] != unitID { + return nil, fmt.Errorf("%w: modbus unit id mismatch: got %d want %d", drivers.ErrModbusTransport, hdr[6], unitID) } length := int(binary.BigEndian.Uint16(hdr[4:6])) - if length < 2 { - return nil, fmt.Errorf("modbus invalid response length %d", length) + if length < 2 || length > proxyMaxADULength { + return nil, fmt.Errorf("%w: modbus invalid response length %d", drivers.ErrModbusTransport, length) } res := make([]byte, length-1) if _, err := io.ReadFull(c.conn, res); err != nil { return nil, err } + if res[0] != pdu[0] && res[0] != pdu[0]|0x80 { + return nil, fmt.Errorf("%w: modbus response function mismatch", drivers.ErrModbusTransport) + } + if res[0]&0x80 != 0 && len(res) != 2 { + return nil, fmt.Errorf("%w: modbus invalid exception response", drivers.ErrModbusTransport) + } return res, nil } diff --git a/web/index.html b/web/index.html index 7c52f39d3..3cd2177f3 100644 --- a/web/index.html +++ b/web/index.html @@ -100,7 +100,7 @@

Settings

- + diff --git a/web/settings/tabs/ha.js b/web/settings/tabs/ha.js index 38112c1a6..4290aa712 100644 --- a/web/settings/tabs/ha.js +++ b/web/settings/tabs/ha.js @@ -1,4 +1,4 @@ -// Settings → Home Assistant tab: MQTT bridge config + live status. +// Settings → Integrations tab: Home Assistant MQTT and Modbus proxy. (function () { var S = (window.FTWSettings = window.FTWSettings || { tabs: {} }); S.tabs = S.tabs || {}; @@ -39,7 +39,6 @@ render: function (ctx) { var field = ctx.field, config = ctx.config; if (!config.homeassistant) config.homeassistant = {}; - if (!config.modbus_proxy) config.modbus_proxy = {}; return '
checking…
' + '
Home Assistant MQTT' + '' + @@ -58,11 +57,12 @@ "How often state topics are pushed to HA. 5 s is a good default.") + '
' + '
Modbus TCP proxy' + - '

FTW keeps the inverter\'s one Modbus socket and lets other tools on the LAN share it. Point Home Assistant at this box and the listen port, with the same unit id as the device. Reads are multiplexed; writes stay blocked unless you opt in — they bypass FTW\'s control loop.

' + + '

Point Home Assistant or another local tool at this box and port, using the device\'s unit ID. The proxy shares FTW\'s connection. By default, clients can only read and FTW keeps control.

' + '' + field("Listen", "modbus_proxy.listen", "text", ":1502", "Local bind. Default :1502 when the site has one Modbus device. Several inverters need capabilities.modbus.proxy_listen on each driver.") + - '' + + '' + + '

With external control, FTW keeps measuring but sends no commands to the exposed devices. Your external controller handles device safety. Control stays external until you turn this off and restart FTW. Proxy settings take effect after a restart.

' + '
'; }, after: function () { diff --git a/web/settings/tabs/ha.test.mjs b/web/settings/tabs/ha.test.mjs index 54024865f..3f1c5761d 100644 --- a/web/settings/tabs/ha.test.mjs +++ b/web/settings/tabs/ha.test.mjs @@ -49,7 +49,15 @@ describe("Home Assistant status", () => { }); describe("Modbus proxy settings", () => { - it("renders the proxy fieldset on the Home Assistant tab", () => { + it("keeps an unused proxy absent and explains the write owner", () => { + const config = {}; + const html = haTab.render({field: () => "", config}); + assert.equal(config.modbus_proxy, undefined); + assert.match(html, /only read and FTW keeps control/); + assert.match(html, /External control — FTW only reads/); + assert.match(html, /Control stays external until you turn this off and restart/); + }); + it("renders the proxy fieldset on the Integrations tab", () => { const html = haTab.render({ field: function (label, path) { return "" + label + ""; }, config: {},