diff --git a/THREAT_MODEL.md b/THREAT_MODEL.md index 3d820761e21..b716fe9ca24 100644 --- a/THREAT_MODEL.md +++ b/THREAT_MODEL.md @@ -147,11 +147,10 @@ The following are **not** vulnerabilities, by design: that offers encryption because it has a static api key. That is the documented default, authenticated (if at all) by the OTA password. An uploader that takes the offer skips the password; the key authenticates it. -- The enablement window: requiring encryption needs a running firmware that - offers it, which every build with a static api key does. Older firmware - takes one last upload of the offering firmware over the existing plaintext - channel, with the pre-existing plaintext exposure; the upload that turns on - `ota: encryption:` is then already encrypted. +- The enablement window: firmware built with a static api key already offers + encryption, so turning on `ota: encryption:` is itself an encrypted upload. + Older firmware needs one last plaintext upload of an offering build, with + the pre-existing plaintext exposure. - The web OTA `/update` endpoint alongside encryption. The `web_server` component keeps it always reachable, and `captive_portal:` auto-loads it for the fallback AP window; validation warns about both combinations, and diff --git a/esphome/components/esphome/ota/__init__.py b/esphome/components/esphome/ota/__init__.py index 20d9734c6c0..dd084853c32 100644 --- a/esphome/components/esphome/ota/__init__.py +++ b/esphome/components/esphome/ota/__init__.py @@ -7,7 +7,7 @@ from esphome.components.noise import ( is_reserved_key, ) from esphome.components.ota import BASE_OTA_SCHEMA, OTAComponent, ota_to_code -from esphome.config_helpers import merge_config +from esphome.config_helpers import filter_source_files_from_defines, merge_config import esphome.config_validation as cv from esphome.const import ( CONF_API, @@ -174,12 +174,8 @@ def _warn_web_server_ota(full_conf: ConfigType) -> None: def _api_static_key(api_conf: ConfigType) -> str | None: - """The api encryption key when it is fixed at build time. - - A keyless api block provisions its key at runtime and the all-zeros key is - the unprovisioned sentinel (the device treats it as no key); neither can - seed the OTA encryption offer. - """ + """The api key when fixed at build time; None for a runtime provisioned + or all-zeros key, neither can seed the encryption offer.""" key = api_conf.get(CONF_ENCRYPTION, {}).get(CONF_KEY) if not key or is_reserved_key(key): return None @@ -280,18 +276,9 @@ CONFIG_SCHEMA = cv.All( FINAL_VALIDATE_SCHEMA = ota_esphome_final_validate -def FILTER_SOURCE_FILES() -> list[str]: - """Filter out the noise transport unless an ota entry configures - encryption or the api has a static key to offer.""" - if _api_static_key(CORE.config.get(CONF_API) or {}) is not None: - return [] - for ota_conf in CORE.config.get(CONF_OTA, []): - if ( - ota_conf.get(CONF_PLATFORM) == CONF_ESPHOME - and ota_conf.get(CONF_ENCRYPTION) is not None - ): - return [] - return ["ota_esphome_noise.cpp"] +FILTER_SOURCE_FILES = filter_source_files_from_defines( + {"ota_esphome_noise.cpp": "USE_OTA_ENCRYPTION"} +) @coroutine_with_priority(CoroPriority.OTA_UPDATES) @@ -317,9 +304,8 @@ async def to_code(config: ConfigType) -> None: key = encryption_conf[CONF_KEY] cg.add_define("USE_OTA_ENCRYPTION_REQUIRED") else: - # With only an api key the device offers encryption but still accepts - # plaintext, so a firmware that predates the ota block can be replaced - # over OTA and the next upload with the block is encrypted. + # An api key alone makes the device offer encryption while still + # accepting plaintext, so the upload that adds the block is encrypted key = _api_static_key(CORE.config.get(CONF_API) or {}) if key is not None: cg.add_define("USE_OTA_ENCRYPTION") diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index b7ea0fa0af9..daf5f9cd245 100644 --- a/esphome/components/esphome/ota/ota_esphome.cpp +++ b/esphome/components/esphome/ota/ota_esphome.cpp @@ -97,22 +97,19 @@ void ESPHomeOTAComponent::dump_config() { ESP_LOGCONFIG(TAG, "Over-The-Air updates:\n" " Address: %s:%u\n" - " Version: %d", + " Version: %d" +#ifdef USE_OTA_ENCRYPTION_REQUIRED + "\n Encryption: required" +#elif defined(USE_OTA_ENCRYPTION) + "\n Encryption: offered, plaintext accepted" +#endif + , network::get_use_address_to(addr_buf), this->port_, USE_OTA_VERSION); #ifdef USE_OTA_PASSWORD if (!this->password_.empty()) { ESP_LOGCONFIG(TAG, " Password configured"); } #endif -#ifdef USE_OTA_ENCRYPTION - if (this->noise_ctx_.has_psk()) { -#ifdef USE_OTA_ENCRYPTION_REQUIRED - ESP_LOGCONFIG(TAG, " Encryption: required"); -#else - ESP_LOGCONFIG(TAG, " Encryption: offered (api key), plaintext accepted"); -#endif - } -#endif #ifdef USE_OTA_PARTITIONS ESP_LOGCONFIG(TAG, " Partition access allowed\n" @@ -252,9 +249,8 @@ void ESPHomeOTAComponent::handle_handshake_() { #ifdef USE_OTA_ENCRYPTION_REQUIRED // Fail closed: an explicit `ota: encryption:` block means the client must - // negotiate encryption; refuse plaintext uploads. A build with only an api - // key offers encryption but lets plaintext through. - if (this->noise_ctx_.has_psk() && (this->ota_features_ & CLIENT_NOISE_FEATURES) != CLIENT_NOISE_FEATURES) { + // negotiate encryption; refuse plaintext uploads + if ((this->ota_features_ & CLIENT_NOISE_FEATURES) != CLIENT_NOISE_FEATURES) { ESP_LOGW(TAG, "Client does not support encryption"); this->send_error_and_cleanup_(ota::OTA_RESPONSE_ERROR_ENCRYPTION_REQUIRED); return; @@ -278,9 +274,7 @@ void ESPHomeOTAComponent::handle_handshake_() { this->handshake_buf_[1] |= SERVER_FEATURE_SUPPORTS_PARTITION_ACCESS; #endif #ifdef USE_OTA_ENCRYPTION - if (this->noise_ctx_.has_psk()) { - this->handshake_buf_[1] |= SERVER_FEATURE_SUPPORTS_NOISE; - } + this->handshake_buf_[1] |= SERVER_FEATURE_SUPPORTS_NOISE; #endif } else { this->handshake_buf_[0] = @@ -297,12 +291,10 @@ void ESPHomeOTAComponent::handle_handshake_() { return; } #ifdef USE_OTA_ENCRYPTION - // When the client took the encryption offer the rest of the session runs - // inside the noise transport, which also authenticates it; the client - // sends the first handshake frame next, so there is nothing to do until - // data arrives. A client without both noise bits continues in plaintext - // (already rejected above when encryption is required). - if (this->noise_ctx_.has_psk() && (this->ota_features_ & CLIENT_NOISE_FEATURES) == CLIENT_NOISE_FEATURES) { + // The client took the encryption offer: the rest of the session runs + // inside the noise transport, which also authenticates it. Nothing to + // do until its first handshake frame arrives. + if ((this->ota_features_ & CLIENT_NOISE_FEATURES) == CLIENT_NOISE_FEATURES) { // handshake_buf_ still holds the feature ack composed above; a // would-block re-entry lands here without rebuilding it if (!this->noise_start_session_(this->handshake_buf_[1])) { diff --git a/esphome/components/esphome/ota/ota_esphome_noise.cpp b/esphome/components/esphome/ota/ota_esphome_noise.cpp index 7f8331cf961..847b3e95b78 100644 --- a/esphome/components/esphome/ota/ota_esphome_noise.cpp +++ b/esphome/components/esphome/ota/ota_esphome_noise.cpp @@ -73,7 +73,7 @@ bool ESPHomeOTAComponent::noise_start_session_(uint8_t server_feature_flags) { int err = this->noise_->handshake.init(this->noise_ctx_.get_psk(), prologue, sizeof(prologue)); if (err != 0) { - ESP_LOGW(TAG, "Handshake init: %s", LOG_STR_ARG(noise::noise_err_to_logstr(err))); + ESP_LOGW(TAG, "Handshake init: %d", err); this->cleanup_connection_(); return false; } @@ -105,14 +105,16 @@ bool ESPHomeOTAComponent::handle_noise_handshake_() { s.frame_pos = 0; s.frame_len = 0; if (s.frame_buf[noise::FRAME_HEADER_SIZE] != noise::HANDSHAKE_STATUS_OK) { - ESP_LOGW(TAG, "Bad handshake error byte: %u", s.frame_buf[noise::FRAME_HEADER_SIZE]); + ESP_LOGV(TAG, "Bad handshake error byte: %u", s.frame_buf[noise::FRAME_HEADER_SIZE]); this->cleanup_connection_(); return false; } int err = s.handshake.read_message(s.frame_buf + noise::FRAME_HEADER_SIZE + 1, payload_len - 1); if (err != 0) { - ESP_LOGW(TAG, "Handshake read: %s", LOG_STR_ARG(noise::noise_err_to_logstr(err))); - this->noise_send_reject_(noise::reject_reason_for(err)); + // A MAC failure here almost always means the uploader has a different key + const LogString *reason = noise::reject_reason_for(err); + ESP_LOGW(TAG, "Handshake read: %s (%d)", LOG_STR_ARG(reason), err); + this->noise_send_reject_(reason); this->cleanup_connection_(); return false; } @@ -123,7 +125,7 @@ bool ESPHomeOTAComponent::handle_noise_handshake_() { int err = s.handshake.write_message(s.frame_buf + noise::FRAME_HEADER_SIZE + 1, noise::MAX_HANDSHAKE_SIZE, msg_len); if (err != 0) { - ESP_LOGW(TAG, "Handshake write: %s", LOG_STR_ARG(noise::noise_err_to_logstr(err))); + ESP_LOGW(TAG, "Handshake write: %d", err); this->cleanup_connection_(); return false; } @@ -138,7 +140,7 @@ bool ESPHomeOTAComponent::handle_noise_handshake_() { case noise::NoiseResponderHandshake::Action::ACTION_SPLIT: { int err = s.handshake.split(s.send_cipher, s.recv_cipher); if (err != 0) { - ESP_LOGW(TAG, "Handshake split: %s", LOG_STR_ARG(noise::noise_err_to_logstr(err))); + ESP_LOGW(TAG, "Handshake split: %d", err); this->cleanup_connection_(); return false; } @@ -146,7 +148,7 @@ bool ESPHomeOTAComponent::handle_noise_handshake_() { return true; } default: { - ESP_LOGW(TAG, "Bad handshake state"); + ESP_LOGV(TAG, "Bad handshake state"); this->cleanup_connection_(); return false; } @@ -159,7 +161,7 @@ bool ESPHomeOTAComponent::noise_try_read_frame_() { NoiseSession &s = *this->noise_; while (s.frame_pos < noise::FRAME_HEADER_SIZE) { ssize_t read = this->client_->read(s.frame_buf + s.frame_pos, noise::FRAME_HEADER_SIZE - s.frame_pos); - if (!this->handle_read_error_(read, LOG_STR("read noise header"))) { + if (!this->handle_read_error_(read, LOG_STR("read noise frame"))) { return false; } s.frame_pos += read; @@ -214,7 +216,7 @@ ssize_t ESPHomeOTAComponent::noise_decrypt_(uint8_t *buf, size_t len) { noise_buffer_set_inout(mbuf, buf, len, len); int err = noise_cipherstate_decrypt(this->noise_->recv_cipher, &mbuf); if (err != 0) { - ESP_LOGW(TAG, "Decrypt: %s", LOG_STR_ARG(noise::noise_err_to_logstr(err))); + ESP_LOGW(TAG, "Decrypt: %d", err); return -1; } return mbuf.size; @@ -267,7 +269,7 @@ bool ESPHomeOTAComponent::noise_write_byte_(uint8_t byte) { noise_buffer_set_inout(mbuf, frame + noise::FRAME_HEADER_SIZE, 1, 1 + noise::MAC_SIZE); int err = noise_cipherstate_encrypt(this->noise_->send_cipher, &mbuf); if (err != 0) { - ESP_LOGW(TAG, "Encrypt: %s", LOG_STR_ARG(noise::noise_err_to_logstr(err))); + ESP_LOGW(TAG, "Encrypt: %d", err); return false; } noise::write_frame_header(frame, mbuf.size); diff --git a/esphome/espota2.py b/esphome/espota2.py index bfd257e2eb8..40adb4527aa 100644 --- a/esphome/espota2.py +++ b/esphome/espota2.py @@ -539,11 +539,10 @@ def perform_ota( "offer encryption; refusing to send the image in plaintext. " "The running firmware was built before OTA encryption " "(ESPHome 2026.9.0) or without an 'api: encryption: key'. " - "Install once with the 'ota: encryption:' block removed and " - "the api key kept, that firmware offers encryption, then " - "restore the block and install again; every install after " - "that is encrypted. Use the web_server OTA platform or a " - "serial flash if the device cannot be reached that way." + "Firmware built with an api key offers encryption: install " + "once with the 'ota: encryption:' block removed, then restore " + "the block and install again. Otherwise flash by serial or " + "the web_server OTA platform." ) # The prologue binds every negotiation byte both sides saw, so any # tampering with the plaintext preamble breaks the handshake. diff --git a/tests/component_tests/ota/test_esphome_ota.py b/tests/component_tests/ota/test_esphome_ota.py index 141ce17b059..b3c01636c66 100644 --- a/tests/component_tests/ota/test_esphome_ota.py +++ b/tests/component_tests/ota/test_esphome_ota.py @@ -372,44 +372,6 @@ def test_auto_load_pulls_noise_only_for_encryption() -> None: assert "noise" in AUTO_LOAD({}) -def test_filter_source_files_excludes_noise_without_encryption() -> None: - """The noise transport source compiles only for encrypted builds.""" - old_config = CORE.config - try: - CORE.config = {CONF_OTA: [_make_ota_config(port=3232)]} - assert FILTER_SOURCE_FILES() == ["ota_esphome_noise.cpp"] - CORE.config = { - CONF_OTA: [ - _make_ota_config(port=3232, **{CONF_ENCRYPTION: {CONF_KEY: API_KEY}}) - ] - } - assert FILTER_SOURCE_FILES() == [] - finally: - CORE.config = old_config - - -def test_filter_source_files_keeps_noise_for_static_api_key() -> None: - """A static api key makes the device offer encryption, so the transport - compiles even without an ota encryption block.""" - old_config = CORE.config - ota = [_make_ota_config(port=3232)] - try: - CORE.config = {CONF_API: {CONF_ENCRYPTION: {CONF_KEY: API_KEY}}, CONF_OTA: ota} - assert FILTER_SOURCE_FILES() == [] - # A runtime provisioned or all-zeros api key has nothing to offer - CORE.config = {CONF_API: {CONF_ENCRYPTION: {}}, CONF_OTA: ota} - assert FILTER_SOURCE_FILES() == ["ota_esphome_noise.cpp"] - CORE.config = { - CONF_API: {CONF_ENCRYPTION: {CONF_KEY: ZEROS_KEY}}, - CONF_OTA: ota, - } - assert FILTER_SOURCE_FILES() == ["ota_esphome_noise.cpp"] - CORE.config = {CONF_API: {}, CONF_OTA: ota} - assert FILTER_SOURCE_FILES() == ["ota_esphome_noise.cpp"] - finally: - CORE.config = old_config - - def test_api_static_key() -> None: """Only a real build-time api key can seed the encryption offer.""" assert _api_static_key({}) is None @@ -418,52 +380,44 @@ def test_api_static_key() -> None: assert _api_static_key({CONF_ENCRYPTION: {CONF_KEY: API_KEY}}) == API_KEY -def _defines() -> set[str]: - return {define.name for define in CORE.defines} - - -def test_api_key_offers_encryption_without_requiring_it( +@pytest.mark.parametrize( + ("yaml_name", "defines_present", "defines_absent"), + [ + # An api key alone compiles the transport in without requiring it + ("api_key_offer", {"USE_OTA_ENCRYPTION"}, {"USE_OTA_ENCRYPTION_REQUIRED"}), + # A password still guards plaintext uploads on an offering device + ( + "api_key_offer_password", + {"USE_OTA_ENCRYPTION", "USE_OTA_PASSWORD"}, + {"USE_OTA_ENCRYPTION_REQUIRED"}, + ), + # The ota encryption block is what makes the device refuse plaintext + ( + "encryption_required", + {"USE_OTA_ENCRYPTION", "USE_OTA_ENCRYPTION_REQUIRED"}, + set(), + ), + # A key provisioned at runtime is unknown at build time, so no offer + ("runtime_api_key", set(), {"USE_OTA_ENCRYPTION"}), + ], +) +def test_encryption_offer_codegen( generate_main: Callable[[str], str], + yaml_name: str, + defines_present: set[str], + defines_absent: set[str], ) -> None: - """An api key alone compiles the transport in and sets the psk, but the - device keeps accepting plaintext uploads.""" main_cpp = generate_main( - "tests/component_tests/ota/test_esphome_ota_api_key_offer.yaml" + f"tests/component_tests/ota/test_esphome_ota_{yaml_name}.yaml" ) - assert "USE_OTA_ENCRYPTION" in _defines() - assert "USE_OTA_ENCRYPTION_REQUIRED" not in _defines() - assert "set_noise_psk(" in main_cpp - - -def test_api_key_offer_keeps_password(generate_main: Callable[[str], str]) -> None: - """A password still guards plaintext uploads on an offering device.""" - main_cpp = generate_main( - "tests/component_tests/ota/test_esphome_ota_api_key_offer_password.yaml" - ) - assert {"USE_OTA_ENCRYPTION", "USE_OTA_PASSWORD"} <= _defines() - assert "USE_OTA_ENCRYPTION_REQUIRED" not in _defines() - assert "set_noise_psk(" in main_cpp - assert "set_auth_password(" in main_cpp - - -def test_encryption_block_requires_encryption( - generate_main: Callable[[str], str], -) -> None: - """The ota encryption block is what makes the device refuse plaintext.""" - main_cpp = generate_main( - "tests/component_tests/ota/test_esphome_ota_encryption_required.yaml" - ) - assert {"USE_OTA_ENCRYPTION", "USE_OTA_ENCRYPTION_REQUIRED"} <= _defines() - assert "set_noise_psk(" in main_cpp - - -def test_runtime_api_key_offers_nothing(generate_main: Callable[[str], str]) -> None: - """A key provisioned at runtime is unknown at build time, so no offer.""" - main_cpp = generate_main( - "tests/component_tests/ota/test_esphome_ota_runtime_api_key.yaml" - ) - assert "USE_OTA_ENCRYPTION" not in _defines() - assert "set_noise_psk(" not in main_cpp + defines = {define.name for define in CORE.defines} + assert defines_present <= defines + assert not (defines_absent & defines) + encrypted = "USE_OTA_ENCRYPTION" in defines_present + assert ("set_noise_psk(" in main_cpp) is encrypted + assert ("set_auth_password(" in main_cpp) is ("USE_OTA_PASSWORD" in defines_present) + # The noise transport source compiles only when the define is set + assert FILTER_SOURCE_FILES() == ([] if encrypted else ["ota_esphome_noise.cpp"]) def test_password_with_encryption_rejected() -> None: diff --git a/tests/integration/fixtures/host_ota_api_key_offers_encryption.yaml b/tests/integration/fixtures/host_ota_api_key_offers_encryption.yaml deleted file mode 100644 index e846e4ea569..00000000000 --- a/tests/integration/fixtures/host_ota_api_key_offers_encryption.yaml +++ /dev/null @@ -1,11 +0,0 @@ -esphome: - name: host-ota-test -host: -api: - encryption: - key: "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=" -ota: - - platform: esphome - port: __OTA_PORT__ -logger: - level: DEBUG diff --git a/tests/integration/test_host_ota.py b/tests/integration/test_host_ota.py index feaa97bdfee..1f44c2253d5 100644 --- a/tests/integration/test_host_ota.py +++ b/tests/integration/test_host_ota.py @@ -11,6 +11,7 @@ import asyncio from collections.abc import Generator from contextlib import contextmanager import functools +from pathlib import Path import socket import pytest @@ -22,6 +23,7 @@ from .const import LOCALHOST, PORT_POLL_INTERVAL, PORT_WAIT_TIMEOUT from .types import CompileFunction, ConfigWriter DEVICE_NAME = "host-ota-test" +API_KEY = "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=" @contextmanager @@ -121,7 +123,6 @@ async def test_host_ota_encrypted( ) -> None: """Encrypted self-OTA succeeds; a plaintext upload to the same device fails.""" pytest.importorskip("aioesphomeapi.noise") - noise_psk = "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=" api_port, api_socket = reserved_tcp_port with _reserve_port() as (ota_port, ota_socket): yaml_config = yaml_config.replace("__OTA_PORT__", str(ota_port)) @@ -158,7 +159,7 @@ async def test_host_ota_encrypted( ota_port, None, binary_path, - noise_psk=noise_psk, + noise_psk=API_KEY, ), ) assert rc == 0, "encrypted OTA reported failure" @@ -187,61 +188,22 @@ class _RebootCounter: await self._seen.wait() -@pytest.mark.asyncio -async def test_host_ota_api_key_offers_encryption( - yaml_config: str, - write_yaml_config: ConfigWriter, - compile_esphome: CompileFunction, - reserved_tcp_port: tuple[int, socket.socket], -) -> None: - """With only an api key the device takes both a plaintext upload and an - encrypted one using that key, which is the enablement path for - `ota: encryption:`.""" - pytest.importorskip("aioesphomeapi.noise") - api_key = "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=" - api_port, api_socket = reserved_tcp_port - with _reserve_port() as (ota_port, ota_socket): - yaml_config = yaml_config.replace("__OTA_PORT__", str(ota_port)) - config_path = await write_yaml_config(yaml_config) - binary_path = await compile_esphome(config_path) - api_socket.close() - ota_socket.close() - - loop = asyncio.get_running_loop() - reboots = _RebootCounter() - - async with run_binary(binary_path, line_callback=reboots.on_log) as ( - proc, - lines, - ): - await _wait_for_port(LOCALHOST, api_port, PORT_WAIT_TIMEOUT) - pid_before = proc.pid - - rc, _ = await loop.run_in_executor( - None, espota2.run_ota, LOCALHOST, ota_port, None, binary_path - ) - assert rc == 0, "plaintext upload to an offering device must succeed" - await reboots.wait(1) - await _wait_for_port(LOCALHOST, api_port, PORT_WAIT_TIMEOUT) - assert proc.pid == pid_before - - rc, _ = await loop.run_in_executor( - None, - functools.partial( - espota2.run_ota, - LOCALHOST, - ota_port, - None, - binary_path, - noise_psk=api_key, - ), - ) - assert rc == 0, "encrypted upload with the api key must succeed" - await reboots.wait(2) - await _wait_for_port(LOCALHOST, api_port, PORT_WAIT_TIMEOUT) - assert proc.returncode is None, "process exited instead of execing" - assert proc.pid == pid_before - assert any("Encryption: offered" in line for line in lines) +async def _run_ota( + ota_port: int, password: str | None, binary_path: Path, noise_psk: str | None +) -> int: + """espota2 is blocking; run it in the executor and return its exit code.""" + rc, _ = await asyncio.get_running_loop().run_in_executor( + None, + functools.partial( + espota2.run_ota, + LOCALHOST, + ota_port, + password, + binary_path, + noise_psk=noise_psk, + ), + ) + return rc @pytest.mark.asyncio @@ -251,10 +213,11 @@ async def test_host_ota_api_key_offer_with_password( compile_esphome: CompileFunction, reserved_tcp_port: tuple[int, socket.socket], ) -> None: - """The OTA password still guards plaintext uploads on an offering device - while the api key alone authenticates an encrypted one.""" + """With only an api key the device offers encryption without requiring + it: the password still guards plaintext uploads, and the key alone + authenticates an encrypted one, which is the enablement path for + `ota: encryption:`.""" pytest.importorskip("aioesphomeapi.noise") - api_key = "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=" api_port, api_socket = reserved_tcp_port with _reserve_port() as (ota_port, ota_socket): yaml_config = yaml_config.replace("__OTA_PORT__", str(ota_port)) @@ -263,46 +226,32 @@ async def test_host_ota_api_key_offer_with_password( api_socket.close() ota_socket.close() - loop = asyncio.get_running_loop() reboots = _RebootCounter() - async with run_binary(binary_path, line_callback=reboots.on_log) as ( proc, - _lines, + lines, ): await _wait_for_port(LOCALHOST, api_port, PORT_WAIT_TIMEOUT) pid_before = proc.pid - rc, _ = await loop.run_in_executor( - None, espota2.run_ota, LOCALHOST, ota_port, None, binary_path - ) + rc = await _run_ota(ota_port, None, binary_path, None) assert rc == 1, "plaintext upload without the password must fail" await asyncio.sleep(0.5) assert proc.returncode is None, "process died on rejected upload" - rc, _ = await loop.run_in_executor( - None, espota2.run_ota, LOCALHOST, ota_port, "hunter2", binary_path - ) + rc = await _run_ota(ota_port, "hunter2", binary_path, None) assert rc == 0, "plaintext upload with the password must succeed" await reboots.wait(1) await _wait_for_port(LOCALHOST, api_port, PORT_WAIT_TIMEOUT) assert proc.pid == pid_before - rc, _ = await loop.run_in_executor( - None, - functools.partial( - espota2.run_ota, - LOCALHOST, - ota_port, - None, - binary_path, - noise_psk=api_key, - ), - ) + rc = await _run_ota(ota_port, None, binary_path, API_KEY) assert rc == 0, "encrypted upload with the api key must succeed" await reboots.wait(2) await _wait_for_port(LOCALHOST, api_port, PORT_WAIT_TIMEOUT) + assert proc.returncode is None, "process exited instead of execing" assert proc.pid == pid_before + assert any("Encryption: offered" in line for line in lines) @pytest.mark.asyncio diff --git a/tests/unit_tests/test_espota2_noise.py b/tests/unit_tests/test_espota2_noise.py index 68cb2b7d3a7..61bc87fcfec 100644 --- a/tests/unit_tests/test_espota2_noise.py +++ b/tests/unit_tests/test_espota2_noise.py @@ -110,17 +110,19 @@ class FakeEncryptedDevice(threading.Thread): return server_flags = espota2.SERVER_FEATURE_SUPPORTS_NOISE if self.offer_noise else 0 sock.sendall(bytes([espota2.RESPONSE_FEATURE_FLAGS, server_flags])) - if noise_negotiated and not self.offer_noise: - return # the client fails closed; nothing further arrives if not noise_negotiated: # A device that does not require encryption lets a plaintext # client through self._transfer( lambda byte: sock.sendall(bytes([byte])), lambda length: _recv_exact(sock, length), - lambda remaining: sock.recv(min(remaining, 4096)), + lambda remaining: _recv_exact( + sock, min(remaining, espota2.UPLOAD_BLOCK_SIZE) + ), ) return + if not self.offer_noise: + return # the client fails closed; nothing further arrives from cryptography.exceptions import InvalidTag from noise.connection import NoiseConnection @@ -159,7 +161,7 @@ class FakeEncryptedDevice(threading.Thread): assert len(plaintext) == length, "control units must be one per frame" return plaintext - def recv_data(remaining: int) -> bytes: + def recv_data(_remaining: int) -> bytes: plaintext = proto.decrypt(_recv_frame(sock)) assert 0 < len(plaintext) <= espota2.NOISE_MAX_PLAINTEXT return plaintext @@ -183,9 +185,7 @@ class FakeEncryptedDevice(threading.Thread): received = b"" acked = 0 while len(received) < size: - chunk = recv_data(size - len(received)) - assert chunk, "client closed mid-transfer" - received += chunk + received += recv_data(size - len(received)) if self.version >= espota2.OTA_VERSION_2_0: while acked + espota2.UPLOAD_BLOCK_SIZE <= len(received) or ( len(received) == size and acked < size @@ -264,25 +264,16 @@ def test_client_fails_closed_when_device_lacks_encryption() -> None: device.join_and_check() -def test_plaintext_client_accepted_by_offering_device() -> None: - """A device that offers but does not require encryption still takes a - plaintext upload from a client with no key configured.""" +@pytest.mark.parametrize("noise_psk", [None, PSK], ids=["plaintext", "encrypted"]) +def test_offering_device_accepts_either_transport(noise_psk: str | None) -> None: + """A device that offers but does not require encryption takes a plaintext + upload from a keyless client and an encrypted one from a keyed client.""" + if noise_psk: + pytest.importorskip("aioesphomeapi.noise") firmware = bytes(range(256)) * 40 device = FakeEncryptedDevice(offer_noise=True, require_noise=False) with patch("time.sleep"): - _upload(device, firmware, None) - device.join_and_check() - assert device.received == firmware - - -def test_keyed_client_encrypts_with_offering_device() -> None: - """The upload that turns on `ota: encryption:` is already encrypted when - the running firmware offers it.""" - pytest.importorskip("aioesphomeapi.noise") - firmware = bytes(range(256)) * 40 - device = FakeEncryptedDevice(offer_noise=True, require_noise=False) - with patch("time.sleep"): - _upload(device, firmware, PSK) + _upload(device, firmware, noise_psk) device.join_and_check() assert device.received == firmware