From aec4d3aec75473f6b667a1b7a83d40f354714072 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 5 Sep 2026 17:14:45 +0200 Subject: [PATCH] Keep the client feature constants in the source file and trim comments --- esphome/__main__.py | 3 +-- esphome/components/esphome/ota/__init__.py | 23 ++++++---------- .../components/esphome/ota/ota_esphome.cpp | 20 ++++++++++---- esphome/components/esphome/ota/ota_esphome.h | 21 +++------------ .../esphome/ota/ota_esphome_noise.cpp | 9 +++---- esphome/components/noise/__init__.py | 5 ++-- esphome/components/noise/noise.h | 8 +++--- esphome/espota2.py | 27 +++++++------------ esphome/wizard.py | 3 +-- 9 files changed, 45 insertions(+), 74 deletions(-) diff --git a/esphome/__main__.py b/esphome/__main__.py index 120422680e..30e97f55eb 100644 --- a/esphome/__main__.py +++ b/esphome/__main__.py @@ -1354,8 +1354,7 @@ def _upload_via_native_api( # It arrives here as a SensitiveStr which aioesphomeapi rejects. noise_psk = str(noise_psk) elif api_key := static_encryption_key(config.get(CONF_API) or {}): - # Remove before 2027.3.0: without an ota block the api key is tried - # when the device offers encryption, falling back to plaintext + # Remove before 2027.3.0: the api key is tried, falling back to plaintext noise_psk = str(api_key) plaintext_fallback = True diff --git a/esphome/components/esphome/ota/__init__.py b/esphome/components/esphome/ota/__init__.py index 8a3b79a5a6..35d65423f6 100644 --- a/esphome/components/esphome/ota/__init__.py +++ b/esphome/components/esphome/ota/__init__.py @@ -40,12 +40,10 @@ DEPENDENCIES = ["network"] def AUTO_LOAD(config: ConfigType) -> list[str]: - """Auto-load noise only when encryption is configured. The api key offer - path inherits noise from the api component's own AUTO_LOAD.""" + """Auto-load noise only when encryption is configured; the api key offer + inherits it from the api component.""" base = ["sha256", "socket"] - # A falsy config is a tooling probe for the maximal set (None from - # dependency resolution, {} from the components-graph platform probe); - # a validated config always carries defaults, never empty + # A falsy config is a tooling probe for the maximal set if not config or CONF_ENCRYPTION in config: return base + ["noise"] return base @@ -154,9 +152,7 @@ def ota_esphome_final_validate(config: ConfigType) -> None: and CONF_ENCRYPTION in api_conf and not api_conf[CONF_ENCRYPTION].get(CONF_KEY) ): - # A runtime provisioned key: the CLI still needs the password, but - # whoever holds the key skips it, and without a provisioning window - # anyone on the network can be the one to provision it + # The CLI still needs the password; whoever provisions the key skips it _LOGGER.warning( "The '%s' %s %s provisioned at runtime also authenticates OTA " "uploads once provisioned; '%s' %s then only guards plaintext " @@ -169,9 +165,8 @@ def ota_esphome_final_validate(config: ConfigType) -> None: CONF_OTA, CONF_PASSWORD, ) - # Only the web_server component starts the shared listener permanently; - # the captive portal brings it up just for the fallback AP, which is the - # intended recovery path, so only the component plus the platform warns + # Only the web_server component keeps the listener up; the captive + # portal's copy is the recovery path if ( CONF_WEB_SERVER in full_conf and any(conf.get(CONF_PLATFORM) == CONF_WEB_SERVER for conf in full_ota_conf) @@ -310,10 +305,8 @@ async def to_code(config: ConfigType) -> None: if config.get(CONF_ALLOW_PARTITION_ACCESS): cg.add_define("USE_OTA_PARTITIONS") - # One key per device: with an api encryption block the device uses the api - # server's key (static, or provisioned at runtime) and offers encryption - # while still accepting plaintext; the ota block is what requires it. - # An ota key of its own only exists without api encryption. + # One key per device: an api encryption block supplies it (static or + # runtime) and offers; the ota block only adds the requirement api_conf = CORE.config.get(CONF_API) or {} encryption_conf = config.get(CONF_ENCRYPTION) own_key = None diff --git a/esphome/components/esphome/ota/ota_esphome.cpp b/esphome/components/esphome/ota/ota_esphome.cpp index 67344d2ecd..1005ed214b 100644 --- a/esphome/components/esphome/ota/ota_esphome.cpp +++ b/esphome/components/esphome/ota/ota_esphome.cpp @@ -177,10 +177,24 @@ void ESPHomeOTAComponent::loop() { static constexpr uint8_t CLIENT_FEATURE_SUPPORTS_COMPRESSION = 0x01; static constexpr uint8_t CLIENT_FEATURE_SUPPORTS_SHA256_AUTH = 0x02; +static constexpr uint8_t CLIENT_FEATURE_SUPPORTS_EXTENDED_PROTOCOL = 0x04; +static constexpr uint8_t CLIENT_FEATURE_SUPPORTS_NOISE = 0x08; +// Noise needs the extended protocol: the prologue binds the 2-byte feature ack +static constexpr uint8_t CLIENT_NOISE_FEATURES = + CLIENT_FEATURE_SUPPORTS_NOISE | CLIENT_FEATURE_SUPPORTS_EXTENDED_PROTOCOL; static constexpr uint8_t SERVER_FEATURE_SUPPORTS_COMPRESSION = 0x01; static constexpr uint8_t SERVER_FEATURE_SUPPORTS_PARTITION_ACCESS = 0x02; static constexpr uint8_t SERVER_FEATURE_SUPPORTS_NOISE = 0x04; +inline bool ESPHomeOTAComponent::extended_proto_() const { +#ifdef USE_OTA_ENCRYPTION_REQUIRED + // FEATURE_READ already refused every client without the extended protocol + return true; +#else + return (this->ota_features_ & CLIENT_FEATURE_SUPPORTS_EXTENDED_PROTOCOL) != 0; +#endif +} + void ESPHomeOTAComponent::handle_handshake_() { /// Handle the OTA handshake and authentication. /// @@ -265,8 +279,7 @@ void ESPHomeOTAComponent::handle_handshake_() { ESP_LOGV(TAG, "Features: 0x%02X", this->ota_features_); #ifdef USE_OTA_ENCRYPTION_REQUIRED - // Fail closed: an explicit `ota: encryption:` block means the client must - // negotiate encryption; refuse plaintext uploads + // `ota: encryption:` requires the client to negotiate encryption 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); @@ -313,9 +326,6 @@ void ESPHomeOTAComponent::handle_handshake_() { return; } #ifdef USE_OTA_ENCRYPTION - // 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. // Latch the offer actually sent: a key activating between the two // states must not start a session the client never expects if ((this->handshake_buf_[1] & SERVER_FEATURE_SUPPORTS_NOISE) != 0 && diff --git a/esphome/components/esphome/ota/ota_esphome.h b/esphome/components/esphome/ota/ota_esphome.h index d37b65b772..c6f710b3fc 100644 --- a/esphome/components/esphome/ota/ota_esphome.h +++ b/esphome/components/esphome/ota/ota_esphome.h @@ -86,8 +86,7 @@ class ESPHomeOTAComponent final : public ota::OTAComponent { bool writing{false}; // a produced handshake frame is still being flushed uint8_t frame_buf[noise::FRAME_HEADER_SIZE + 1 + noise::MAX_HANDSHAKE_SIZE]; }; - // The key the offer and the handshake use: the api server's live context - // when the api has encryption, otherwise the component's own + // The api server's live context when the api has encryption, else our own const noise::NoiseContext &noise_context_() const; bool noise_start_session_(uint8_t server_feature_flags); bool handle_noise_handshake_(); @@ -173,22 +172,8 @@ class ESPHomeOTAComponent final : public ota::OTAComponent { "OTA_BUFFER_SIZE must fit a full encrypted data frame"); #endif static constexpr uint8_t MAGIC_BYTES[5] = {0x6C, 0x26, 0xF7, 0x5C, 0x45}; - static constexpr uint8_t CLIENT_FEATURE_SUPPORTS_EXTENDED_PROTOCOL = 0x04; - static constexpr uint8_t CLIENT_FEATURE_SUPPORTS_NOISE = 0x08; - // Noise needs the extended protocol: the prologue binds the 2-byte feature ack - static constexpr uint8_t CLIENT_NOISE_FEATURES = - CLIENT_FEATURE_SUPPORTS_NOISE | CLIENT_FEATURE_SUPPORTS_EXTENDED_PROTOCOL; - // Derived from the feature byte rather than stored, which keeps the - // trailing byte members at a multiple of four - inline bool extended_proto_() const { -#ifdef USE_OTA_ENCRYPTION_REQUIRED - // Encryption needs the extended protocol and FEATURE_READ has already - // refused every client without it, so the legacy paths cannot be reached - return true; -#else - return (this->ota_features_ & CLIENT_FEATURE_SUPPORTS_EXTENDED_PROTOCOL) != 0; -#endif - } + // Derived from the feature byte; storing it would pad the trailing bytes + bool extended_proto_() const; #ifdef USE_OTA_PARTITIONS uint32_t running_app_offset_{0}; size_t running_app_size_{0}; diff --git a/esphome/components/esphome/ota/ota_esphome_noise.cpp b/esphome/components/esphome/ota/ota_esphome_noise.cpp index fe0a322c4d..9375d94426 100644 --- a/esphome/components/esphome/ota/ota_esphome_noise.cpp +++ b/esphome/components/esphome/ota/ota_esphome_noise.cpp @@ -41,12 +41,9 @@ ESPHomeOTAComponent::NoiseSession::~NoiseSession() { * "NoiseOTAInit" | magic(5) | OK,version | client_features | FEATURE_FLAGS,server_flags */ bool ESPHomeOTAComponent::noise_start_session_(uint8_t server_feature_flags) { - // Not guarded against a provisioned key being cleared between the offer and - // here: the clear is deferred 100 ms and the ack write rarely blocks, and a - // session on the zero key that load_psk then fills in fails the real - // client's MAC. Yaml keys cannot change, so no check is needed there. - // Default-init: the frame buffer is always written before it is read, so - // skip zeroing its 132 bytes + // A provisioned key cleared between the offer and here is not guarded: the + // session runs on the zero key load_psk fills in and fails the client's MAC. + // Default-init: the frame buffer is written before it is read // NOLINTNEXTLINE(clang-analyzer-cplusplus.NewDeleteLeaks) this->noise_ = std::unique_ptr(new (std::nothrow) NoiseSession); static constexpr size_t PROLOGUE_ACK_LEN = 2; // OTA_RESPONSE_OK + version diff --git a/esphome/components/noise/__init__.py b/esphome/components/noise/__init__.py index d569ba78ed..a1d9444fc0 100644 --- a/esphome/components/noise/__init__.py +++ b/esphome/components/noise/__init__.py @@ -63,9 +63,8 @@ ENCRYPTION_SCHEMA = cv.Schema( def static_encryption_key(conf: ConfigType) -> str | None: - """The encryption key of a component config when it is fixed at build - time; None when there is no encryption block or the key is provisioned at - runtime (validation already rejects the all-zeros key).""" + """The build time key of a component config; None without one or when + the key is provisioned at runtime.""" return (conf.get(CONF_ENCRYPTION) or {}).get(CONF_KEY) or None diff --git a/esphome/components/noise/noise.h b/esphome/components/noise/noise.h index 75d6d2a42e..1033d5423c 100644 --- a/esphome/components/noise/noise.h +++ b/esphome/components/noise/noise.h @@ -23,11 +23,9 @@ class NoiseContext { } return acc == 0; } - /// psk points at 32 bytes that outlive the context: a PROGMEM array from - /// codegen, or RAM owned by the caller for a runtime provisioned key; - /// nullptr means no key. A key from yaml is never the reserved all-zeros - /// key (validation rejects it); a caller loading a runtime key must map - /// that key to nullptr itself. + /// psk points at 32 bytes that outlive the context (PROGMEM or caller owned + /// RAM); nullptr means no key. Runtime callers map the all-zeros key to + /// nullptr themselves; validation keeps it out of yaml. void set_psk(const uint8_t *psk) { this->psk_ = psk; } /// Copy the key out (flash-aware on ESP8266); all zeros when none is set. void load_psk(psk_t &out) const; diff --git a/esphome/espota2.py b/esphome/espota2.py index 55c5388f2b..68e5d50cab 100644 --- a/esphome/espota2.py +++ b/esphome/espota2.py @@ -217,11 +217,8 @@ PLAINTEXT_FALLBACK_NOTICE = ( # Remove before 2027.3.0 class _EncryptionAttempt: - """The key an upload tries and whether it may fall back to plaintext. - - A rejected handshake falls back at once; a transport fault inside the - handshake is retried encrypted first and only a repeat falls back. - """ + """The key an upload tries and whether it may fall back to plaintext; + a rejected handshake falls back at once, a transport fault only on repeat.""" def __init__(self, noise_psk: str | None, plaintext_fallback: bool) -> None: self.noise_psk = noise_psk @@ -576,19 +573,16 @@ def perform_ota( if noise_psk and not (extended_proto and features & SERVER_FEATURE_SUPPORTS_NOISE): if plaintext_fallback: - # Remove before 2027.3.0: the api key is tried opportunistically - # without an 'ota: encryption:' block, so an older firmware that - # cannot encrypt still gets its update on this connection + # Remove before 2027.3.0: older firmware that cannot encrypt still + # gets its update on this connection _LOGGER.warning( "The device did not offer OTA encryption; continuing in plaintext. %s", PLAINTEXT_FALLBACK_NOTICE, ) noise_psk = None else: - # Fail closed: never fall back to a plaintext upload when an - # encryption key is configured, an active attacker could otherwise - # strip the feature flag and capture the image (it contains the wifi - # credentials and the api encryption key). + # Fail closed: an attacker could otherwise strip the offer and + # capture the image (wifi credentials, api key) raise OTAError( "An OTA encryption key is configured but the device did not " "offer encryption; refusing to send the image in plaintext. " @@ -607,14 +601,12 @@ def perform_ota( + bytes([RESPONSE_OK, version, features_to_send]) + bytes([RESPONSE_FEATURE_FLAGS, features]) ) - # Built outside the try: a missing noise library or a malformed key - # is a local problem and must never downgrade the upload + # Built outside the try: a local failure must never downgrade the upload sock = NoiseSocketWrapper(sock, noise_psk, prologue) try: sock.do_handshake() except OTANetworkError as err: - # A transport fault, not a key problem: the retry loop tries - # encrypted again before it considers plaintext + # A transport fault: retry encrypted before considering plaintext raise OTAHandshakeNetworkError(str(err)) from err except OTAError as err: # Remove before 2027.3.0 @@ -905,8 +897,7 @@ def run_ota_impl_( encryption.plaintext_fallback, ) except OTAEncryptionFallback as err: - # Same address, same attempt budget: the plaintext retry does - # not count as a network retry + # Same address and attempt budget: not a network retry last_error = str(err) encryption.downgrade(last_error) continue diff --git a/esphome/wizard.py b/esphome/wizard.py index 8e0a350a92..897d5f60a1 100644 --- a/esphome/wizard.py +++ b/esphome/wizard.py @@ -148,8 +148,7 @@ def wizard_file(**kwargs: Unpack[WizardFileKwargs]) -> str: if "api_encryption_key" in kwargs: config += f' encryption:\n key: "{kwargs["api_encryption_key"]}"\n' - # Configure OTA: the api key also secures OTA updates, a password is only - # for uploaders that do not support encryption + # The api key also secures OTA; a password only serves older uploaders config += "\nota:\n" config += " - platform: esphome\n" if "ota_password" in kwargs: