Fix: MQTT - TLS/SSL Implementation (#397)

MaStr · claude · Copilot · web-flow · commit b662776663ae · 2026-07-21T11:16:03.000+02:00
* fix: repair TLS support for MQTT and evcc plugin Both MqttApi and EvccApi had broken TLS code that could never work: the condition checked config['tls'] is True but then tried to index the same boolean as a dict (config['tls']['ca_certs']). The config format uses flat keys cafile/certfile/keyfile at the same level as tls: true. Fix reads from those keys and drops the now-unused tls_version/cert_reqs fields (paho picks sensible defaults). Also updated the example config to remove the defunct tls_version key and add clarifying comments. Verified end-to-end: Mosquitto 2 with mutual TLS on port 8883, both MqttApi and EvccApi connect successfully with tls: true. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: add CLI section to CLAUDE.md with --one-shot flag Documents the correct flag name to avoid confusion with --once. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: validate TLS config and add unit tests for mqtt_api and evcc_api Addresses Copilot review comments on PR #397: - Raise ValueError with a clear message if tls: true but cafile is missing, or if certfile/keyfile are only partially provided. - Add TestTlsSetup in test_mqtt_api.py covering: TLS disabled, CA-only, mutual TLS, missing cafile, and unpaired cert/key. - Add tests/batcontrol/test_evcc_api.py with the same coverage for EvccApi; both use unittest.mock.patch so no broker is needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * docs: update mqtt-api.md to document working TLS configuration * docs: update evcc-connection.md to document working TLS configuration --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
diff --git a/CLAUDE.md b/CLAUDE.md
@@ -72,6 +72,14 @@ draft GitHub release + version-bump PR; the pushed tag triggers the Docker build
 `main` is bumped to the next `dev` version and the release is promoted into the HA add-on repo
 (`release-addon` skill there).
 
+## CLI
+
+```
+python -m batcontrol [--config PATH] [--one-shot]
+```
+
+- `--one-shot` — fetch data, run the control loop once, then exit. Useful for testing. Not `--once`.
+
 ## Known Pitfalls
 
 - ASCII-only in source code — no umlauts, special chars, emoji, even in log messages. Does not
diff --git a/config/batcontrol_config_dummy.yaml b/config/batcontrol_config_dummy.yaml
@@ -160,11 +160,12 @@ mqtt:
   password: password
   retry_attempts: 5 # optional, default: 5
   retry_delay: 10 # seconds, optional, default: 10
+  # TLS: set tls to true and provide paths to CA cert and (optional) client cert/key
+  # for mutual TLS. Use port 8883 for TLS-secured brokers.
   tls: false
-  cafile: /etc/ssl/certs/ca-certificates.crt
-  certfile: /etc/ssl/certs/client.crt
-  keyfile: /etc/ssl/certs/client.key
-  tls_version: tlsv1.2
+  cafile: /etc/ssl/certs/ca-certificates.crt  # path to CA certificate (required when tls: true)
+  certfile: /etc/ssl/certs/client.crt          # path to client certificate (optional, for mutual TLS)
+  keyfile: /etc/ssl/certs/client.key           # path to client private key (optional, for mutual TLS)
   auto_discover_enable: true # enables mqtt auto discover => https://www.home-assistant.io/integrations/mqtt/#mqtt-discovery
   auto_discover_topic: homeassistant # base topic path for auto discover config messages - default 'homeassistant' -> https://www.home-assistant.io/integrations/mqtt/#discovery-options
 
@@ -259,11 +260,12 @@ evcc:
   block_battery_while_charging: true
   username: user
   password: password
+  # TLS: set tls to true and provide paths to CA cert and (optional) client cert/key
+  # for mutual TLS. Use port 8883 for TLS-secured brokers.
   tls: false
-  cafile: /etc/ssl/certs/ca-certificates.crt
-  certfile: /etc/ssl/certs/client.crt
-  keyfile: /etc/ssl/certs/client.key
-  tls_version: tlsv1.2
+  cafile: /etc/ssl/certs/ca-certificates.crt  # path to CA certificate (required when tls: true)
+  certfile: /etc/ssl/certs/client.crt          # path to client certificate (optional, for mutual TLS)
+  keyfile: /etc/ssl/certs/client.key           # path to client private key (optional, for mutual TLS)
   # Optional:
   # Choose which topic should deliver the limit for the battery
   # below this limit the battery will be locked
diff --git a/docs/integrations/evcc-connection.md b/docs/integrations/evcc-connection.md
@@ -55,7 +55,26 @@ evcc:
 
 ### TLS/SSL Support
 
-> ⚠️ **Note**: TLS/SSL support is currently **untested and known to be non-functional** (same limitation as in the [MQTT API](mqtt-api.md)). Keep MQTT traffic on a trusted local network.
+Batcontrol supports TLS-encrypted MQTT connections to the evcc broker. Configure TLS with flat keys at the same level as other evcc connection parameters:
+
+```yaml
+evcc:
+  broker: mqtt.home.local
+  port: 8883
+  tls: true
+  cafile: /etc/ssl/certs/ca-certificates.crt   # path to CA certificate (required when tls: true)
+  certfile: /etc/ssl/certs/client.crt           # path to client certificate (optional, for mutual TLS)
+  keyfile: /etc/ssl/certs/client.key            # path to client private key (optional, for mutual TLS)
+```
+
+| Parameter | Type | Description |
+|-----------|------|-------------|
+| `tls` | boolean | Enable TLS encryption (default: `false`). Use port 8883 for TLS-secured brokers |
+| `cafile` | string | Path to the CA certificate file. **Required** when `tls: true` |
+| `certfile` | string | Path to the client certificate file. Optional; required for mutual TLS |
+| `keyfile` | string | Path to the client private key file. Optional; required for mutual TLS (must be set together with `certfile`) |
+
+> **Note**: `certfile` and `keyfile` must both be set or both be absent — partial mutual TLS configuration raises an error at startup.
 
 ### Battery Management Options
 
@@ -237,6 +256,6 @@ When using both batcontrol and evcc with Home Assistant:
 ## Security Considerations
 
 - Use authentication for production MQTT brokers
-- TLS encryption is currently not functional (see above) — keep MQTT traffic on a trusted local network
+- Use TLS encryption (`tls: true` with a valid `cafile`) when the MQTT broker is not on a fully trusted local network
 - Ensure MQTT user has appropriate topic permissions
 - Keep MQTT credentials secure and unique
diff --git a/docs/integrations/mqtt-api.md b/docs/integrations/mqtt-api.md
@@ -44,7 +44,26 @@ mqtt:
 
 ### TLS/SSL Configuration
 
-> ⚠️ **Note**: TLS/SSL support is currently **untested and known to be non-functional**: the implementation expects the certificate options nested below `tls`, while the enable check expects a boolean — these requirements contradict each other. Track progress or report your use case in the project issues before relying on TLS.
+To connect to a TLS-secured MQTT broker, set `tls: true` and provide the path to your CA certificate. For mutual TLS, also supply a client certificate and key.
+
+```yaml
+mqtt:
+  enabled: true
+  broker: mqtt.example.com
+  port: 8883
+  topic: house/batcontrol
+  tls: true
+  cafile: /etc/ssl/certs/ca-certificates.crt   # required when tls: true
+  certfile: /etc/ssl/certs/client.crt           # optional, for mutual TLS
+  keyfile: /etc/ssl/certs/client.key            # optional, for mutual TLS
+```
+
+| Parameter | Type | Default | Description |
+|-----------|------|---------|-------------|
+| `tls` | boolean | `false` | Enable TLS/SSL for the broker connection |
+| `cafile` | string | — | Path to the CA certificate file (required when `tls: true`) |
+| `certfile` | string | — | Path to the client certificate (optional, for mutual TLS) |
+| `keyfile` | string | — | Path to the client private key (optional, for mutual TLS) |
 
 ## Home Assistant Auto-Discovery
 
@@ -279,6 +298,6 @@ This will provide detailed information about MQTT connections, published message
 ## Security Considerations
 
 - Always use authentication (`username`/`password`) in production
-- TLS encryption is currently not functional (see above) — keep MQTT traffic on a trusted local network
+- Use TLS encryption (`tls: true`) when the MQTT broker is reachable over an untrusted network
 - Limit MQTT user permissions to only necessary topics
 - Use strong, unique passwords for MQTT authentication
diff --git a/src/batcontrol/evcc_api.py b/src/batcontrol/evcc_api.py
@@ -115,15 +115,23 @@ def __init__(self, config: dict):
         if 'username' in config and 'password' in config:
             self.client.username_pw_set(config['username'], config['password'])
 
-        # TLS , not tested yet
-        if config['tls'] is True:
+        if config.get('tls') is True:
+            cafile = config.get('cafile')
+            if not cafile:
+                raise ValueError(
+                    'evcc: tls is enabled but cafile is missing or empty'
+                )
+            certfile = config.get('certfile') or None
+            keyfile = config.get('keyfile') or None
+            if bool(certfile) != bool(keyfile):
+                raise ValueError(
+                    'evcc: certfile and keyfile must both be set or both be absent'
+                )
             self.client.tls_set(
-                config['tls']['ca_certs'],
-                config['tls']['certfile'],
-                config['tls']['keyfile'],
-                cert_reqs=config['tls']['cert_reqs'],
-                tls_version=config['tls']['tls_version'],
-                ciphers=config['tls']['ciphers']
+                ca_certs=cafile,
+                certfile=certfile,
+                keyfile=keyfile,
+                ciphers=config.get('ciphers') or None,
             )
 
         # Register callback functions, survives reconnects
diff --git a/src/batcontrol/mqtt_api.py b/src/batcontrol/mqtt_api.py
@@ -105,15 +105,23 @@ def __init__(self, config: dict, interval_minutes: int = 60):
             'offline',
             retain=True)
 
-        # TLS , not tested yet
-        if config['tls'] is True:
+        if config.get('tls') is True:
+            cafile = config.get('cafile')
+            if not cafile:
+                raise ValueError(
+                    'mqtt: tls is enabled but cafile is missing or empty'
+                )
+            certfile = config.get('certfile') or None
+            keyfile = config.get('keyfile') or None
+            if bool(certfile) != bool(keyfile):
+                raise ValueError(
+                    'mqtt: certfile and keyfile must both be set or both be absent'
+                )
             self.client.tls_set(
-                config['tls']['ca_certs'],
-                config['tls']['certfile'],
-                config['tls']['keyfile'],
-                cert_reqs=config['tls']['cert_reqs'],
-                tls_version=config['tls']['tls_version'],
-                ciphers=config['tls']['ciphers']
+                ca_certs=cafile,
+                certfile=certfile,
+                keyfile=keyfile,
+                ciphers=config.get('ciphers') or None,
             )
 
         self.client.on_connect = self.on_connect
diff --git a/tests/batcontrol/test_evcc_api.py b/tests/batcontrol/test_evcc_api.py
@@ -0,0 +1,85 @@
+"""Tests for EvccApi TLS configuration."""
+from unittest.mock import patch
+
+import pytest
+
+from batcontrol.evcc_api import EvccApi
+
+
+BASE_CONFIG = {
+    'broker': 'localhost',
+    'port': 8883,
+    'status_topic': 'evcc/status',
+    'loadpoint_topic': ['evcc/loadpoints/1/charging'],
+    'block_battery_while_charging': True,
+}
+
+
+def _make_config(**overrides):
+    return {**BASE_CONFIG, **overrides}
+
+
+class TestTlsSetup:
+    """EvccApi.__init__ must configure TLS correctly and reject bad configs."""
+
+    def test_tls_disabled_does_not_call_tls_set(self):
+        with patch('batcontrol.evcc_api.mqtt.Client') as MockClient:
+            client = MockClient.return_value
+            cfg = _make_config(tls=False)
+            EvccApi(cfg)
+            client.tls_set.assert_not_called()
+
+    def test_tls_true_calls_tls_set_with_cafile(self):
+        with patch('batcontrol.evcc_api.mqtt.Client') as MockClient:
+            client = MockClient.return_value
+            cfg = _make_config(tls=True, cafile='/etc/ssl/ca.crt')
+            EvccApi(cfg)
+            client.tls_set.assert_called_once_with(
+                ca_certs='/etc/ssl/ca.crt',
+                certfile=None,
+                keyfile=None,
+                ciphers=None,
+            )
+
+    def test_tls_true_passes_mutual_tls_params(self):
+        with patch('batcontrol.evcc_api.mqtt.Client') as MockClient:
+            client = MockClient.return_value
+            cfg = _make_config(
+                tls=True,
+                cafile='/etc/ssl/ca.crt',
+                certfile='/etc/ssl/client.crt',
+                keyfile='/etc/ssl/client.key',
+            )
+            EvccApi(cfg)
+            client.tls_set.assert_called_once_with(
+                ca_certs='/etc/ssl/ca.crt',
+                certfile='/etc/ssl/client.crt',
+                keyfile='/etc/ssl/client.key',
+                ciphers=None,
+            )
+
+    def test_tls_missing_cafile_raises_valueerror(self):
+        with patch('batcontrol.evcc_api.mqtt.Client'):
+            cfg = _make_config(tls=True)
+            with pytest.raises(ValueError, match='cafile'):
+                EvccApi(cfg)
+
+    def test_tls_certfile_without_keyfile_raises_valueerror(self):
+        with patch('batcontrol.evcc_api.mqtt.Client'):
+            cfg = _make_config(
+                tls=True,
+                cafile='/etc/ssl/ca.crt',
+                certfile='/etc/ssl/client.crt',
+            )
+            with pytest.raises(ValueError, match='certfile and keyfile'):
+                EvccApi(cfg)
+
+    def test_tls_keyfile_without_certfile_raises_valueerror(self):
+        with patch('batcontrol.evcc_api.mqtt.Client'):
+            cfg = _make_config(
+                tls=True,
+                cafile='/etc/ssl/ca.crt',
+                keyfile='/etc/ssl/client.key',
+            )
+            with pytest.raises(ValueError, match='certfile and keyfile'):
+                EvccApi(cfg)
diff --git a/tests/batcontrol/test_mqtt_api.py b/tests/batcontrol/test_mqtt_api.py
@@ -1,5 +1,7 @@
 """Tests for MqttApi._handle_message, focusing on bytes payload decoding."""
-from unittest.mock import MagicMock, call
+from unittest.mock import MagicMock, call, patch
+
+import pytest
 
 from batcontrol.core import Batcontrol
 from batcontrol.logic import PeakShavingConfig
@@ -498,6 +500,89 @@ def test_skips_publish_when_disconnected(self):
         api.client.publish.assert_not_called()
 
 
+class TestTlsSetup:
+    """MqttApi.__init__ must configure TLS correctly and reject bad configs."""
+
+    BASE_CONFIG = {
+        'topic': 'house/batcontrol',
+        'broker': 'localhost',
+        'port': 8883,
+        'retry_attempts': 1,
+        'retry_delay': 0,
+    }
+
+    def _make_config(self, **overrides):
+        return {**self.BASE_CONFIG, **overrides}
+
+    def test_tls_disabled_does_not_call_tls_set(self):
+        with patch('batcontrol.mqtt_api.mqtt.Client') as MockClient:
+            client = MockClient.return_value
+            client.is_connected.return_value = True
+            cfg = self._make_config(tls=False)
+            MqttApi(cfg)
+            client.tls_set.assert_not_called()
+
+    def test_tls_true_calls_tls_set_with_cafile(self):
+        with patch('batcontrol.mqtt_api.mqtt.Client') as MockClient:
+            client = MockClient.return_value
+            client.is_connected.return_value = True
+            cfg = self._make_config(
+                tls=True,
+                cafile='/etc/ssl/ca.crt',
+            )
+            MqttApi(cfg)
+            client.tls_set.assert_called_once_with(
+                ca_certs='/etc/ssl/ca.crt',
+                certfile=None,
+                keyfile=None,
+                ciphers=None,
+            )
+
+    def test_tls_true_passes_mutual_tls_params(self):
+        with patch('batcontrol.mqtt_api.mqtt.Client') as MockClient:
+            client = MockClient.return_value
+            client.is_connected.return_value = True
+            cfg = self._make_config(
+                tls=True,
+                cafile='/etc/ssl/ca.crt',
+                certfile='/etc/ssl/client.crt',
+                keyfile='/etc/ssl/client.key',
+            )
+            MqttApi(cfg)
+            client.tls_set.assert_called_once_with(
+                ca_certs='/etc/ssl/ca.crt',
+                certfile='/etc/ssl/client.crt',
+                keyfile='/etc/ssl/client.key',
+                ciphers=None,
+            )
+
+    def test_tls_missing_cafile_raises_valueerror(self):
+        with patch('batcontrol.mqtt_api.mqtt.Client'):
+            cfg = self._make_config(tls=True)
+            with pytest.raises(ValueError, match='cafile'):
+                MqttApi(cfg)
+
+    def test_tls_certfile_without_keyfile_raises_valueerror(self):
+        with patch('batcontrol.mqtt_api.mqtt.Client'):
+            cfg = self._make_config(
+                tls=True,
+                cafile='/etc/ssl/ca.crt',
+                certfile='/etc/ssl/client.crt',
+            )
+            with pytest.raises(ValueError, match='certfile and keyfile'):
+                MqttApi(cfg)
+
+    def test_tls_keyfile_without_certfile_raises_valueerror(self):
+        with patch('batcontrol.mqtt_api.mqtt.Client'):
+            cfg = self._make_config(
+                tls=True,
+                cafile='/etc/ssl/ca.crt',
+                keyfile='/etc/ssl/client.key',
+            )
+            with pytest.raises(ValueError, match='certfile and keyfile'):
+                MqttApi(cfg)
+
+
 class TestPublishSolarActive:
     def test_publishes_true_to_correct_topic(self):
         api = _make_solar_publish_stub()
