From 78d3bd03930156146ee98e5b0ef5eb1ceeb45238 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 9 Jul 2026 08:33:26 -1000 Subject: [PATCH] Apply review cleanups: hot path branch placement, shared zero sentinel, provisioning gates that survive plaintext removal --- esphome/components/api/api_connection.cpp | 38 +++++----- esphome/components/api/api_frame_helper.cpp | 7 +- esphome/components/api/api_noise_context.h | 17 +++-- esphome/components/mdns/mdns_component.cpp | 17 +++-- .../api_zero_psk_provisioning_rejects.yaml | 6 -- .../test_api_zero_psk_provisioning.py | 73 ++++++++----------- 6 files changed, 75 insertions(+), 83 deletions(-) delete mode 100644 tests/integration/fixtures/api_zero_psk_provisioning_rejects.yaml diff --git a/esphome/components/api/api_connection.cpp b/esphome/components/api/api_connection.cpp index 9f52651468..2efdf0bc03 100644 --- a/esphome/components/api/api_connection.cpp +++ b/esphome/components/api/api_connection.cpp @@ -210,10 +210,10 @@ void APIConnection::upgrade_helper_to_noise_() { uint8_t header[3]; uint8_t header_len = plaintext->get_consumed_header(header); auto *noise = new APINoiseFrameHelper(plaintext->release_socket_for_switch(), this->parent_->get_noise_ctx()); + // Carry over the peername-based client name (Hello has not arrived yet) + const char *name = plaintext->get_client_name(); + noise->set_client_name(name, strlen(name)); this->helper_.reset(noise); // destroys the plaintext helper - // Restore the peername-based client name (Hello has not arrived yet) - char peername[socket::SOCKADDR_STR_LEN]; - noise->set_client_name(noise->get_peername_to(peername), strlen(peername)); APIError err = noise->init_from_handoff(header, header_len); if (err != APIError::OK) { this->fatal_error_with_log_(LOG_STR("Noise handoff failed"), err); @@ -278,12 +278,16 @@ void APIConnection::loop() { if (err == APIError::WOULD_BLOCK) { // No more data available break; -#if defined(USE_API_NOISE) && defined(USE_API_PLAINTEXT) - } else if (err == APIError::PROTOCOL_SWITCH_TO_NOISE) { - this->upgrade_helper_to_noise_(); - return; -#endif } else if (err != APIError::OK) { +#if defined(USE_API_NOISE) && defined(USE_API_PLAINTEXT) + // Checked inside the error branch to keep the hot err == OK path + // free of it; this can only fire on the first bytes of a plaintext + // helper on an unprovisioned device + if (err == APIError::PROTOCOL_SWITCH_TO_NOISE) { + this->upgrade_helper_to_noise_(); + return; + } +#endif this->fatal_error_with_log_(LOG_STR("Reading failed"), err); return; } else { @@ -1888,10 +1892,10 @@ bool APIConnection::send_device_info_response_() { #endif #ifdef USE_API_NOISE resp.api_encryption_supported = true; -#ifdef USE_API_PLAINTEXT - // Dual build (encryption compiled in, no key from YAML): while no key is - // set, the key can be provisioned over a zero-PSK Noise connection instead - // of plaintext +#ifndef USE_API_NOISE_PSK_FROM_YAML + // No key from YAML: while no key is set, the key can be provisioned over a + // zero-PSK Noise connection. Gated on the YAML define (not the plaintext + // one) so this advertisement survives the plaintext removal in 2027.2.0. resp.api_encryption_provisionable = !this->parent_->get_noise_ctx().has_psk(); #endif #endif @@ -2071,10 +2075,9 @@ bool APIConnection::send_noise_encryption_set_key_response_(const NoiseEncryptio } } else if (base64_decode(msg.key, msg.key_len, psk.data(), psk.size()) != psk.size()) { ESP_LOGW(TAG, "Invalid encryption key length"); - } else if (psk == psk_t{}) { - // The all-zeros key is reserved: it marks the device as unprovisioned and - // serves as the well-known provisioning PSK. Accepting it would report - // success without enabling encryption (or silently clear an existing key). + } else if (APINoiseContext::is_all_zeros(psk)) { + // Accepting the reserved provisioning PSK would report success without + // enabling encryption (or silently clear an existing key) ESP_LOGW(TAG, "Rejecting all-zero encryption key"); } else if (!this->parent_->save_noise_psk(psk, true)) { ESP_LOGW(TAG, "Failed to save encryption key"); @@ -2084,8 +2087,7 @@ bool APIConnection::send_noise_encryption_set_key_response_(const NoiseEncryptio if (this->helper_->frame_footer_size() == 0) { // Plaintext transport has no frame footer; Noise always has the MAC footer. // Remove after 2027.2.0 together with plaintext support on keyless devices. - ESP_LOGW(TAG, "Key was received on an unencrypted connection; this is deprecated and will stop " - "working in 2027.2.0. Update Home Assistant to provision the key encrypted"); + ESP_LOGW(TAG, "Key received over plaintext; deprecated, will be removed in 2027.2.0"); } #endif } diff --git a/esphome/components/api/api_frame_helper.cpp b/esphome/components/api/api_frame_helper.cpp index cc59717782..7425304766 100644 --- a/esphome/components/api/api_frame_helper.cpp +++ b/esphome/components/api/api_frame_helper.cpp @@ -97,11 +97,8 @@ const LogString *api_error_to_logstr(APIError err) { return LOG_STR("BAD_HANDSHAKE_ERROR_BYTE"); } #endif -#if defined(USE_API_NOISE) && defined(USE_API_PLAINTEXT) - else if (err == APIError::PROTOCOL_SWITCH_TO_NOISE) { - return LOG_STR("PROTOCOL_SWITCH_TO_NOISE"); - } -#endif + // PROTOCOL_SWITCH_TO_NOISE is intercepted in APIConnection::loop() before + // any logging can happen, so it intentionally has no entry here. return LOG_STR("UNKNOWN"); } diff --git a/esphome/components/api/api_noise_context.h b/esphome/components/api/api_noise_context.h index b5f7016689..44484ffa2c 100644 --- a/esphome/components/api/api_noise_context.h +++ b/esphome/components/api/api_noise_context.h @@ -10,13 +10,20 @@ using psk_t = std::array; class APINoiseContext { public: + // The all-zeros PSK is reserved: it marks the device as unprovisioned and + // doubles as the well-known provisioning PSK that unprovisioned devices + // accept for Noise handshakes (passive-sniffing protection only, no + // authentication). It is never a valid real key. + static bool is_all_zeros(const psk_t &psk) { + uint8_t acc = 0; + for (uint8_t b : psk) { + acc |= b; + } + return acc == 0; + } void set_psk(psk_t psk) { this->psk_ = psk; - bool has_psk = false; - for (auto i : psk) { - has_psk |= i; - } - this->has_psk_ = has_psk; + this->has_psk_ = !is_all_zeros(psk); } const psk_t &get_psk() const { return this->psk_; } bool has_psk() const { return this->has_psk_; } diff --git a/esphome/components/mdns/mdns_component.cpp b/esphome/components/mdns/mdns_component.cpp index d34a43bd20..fbfedee19a 100644 --- a/esphome/components/mdns/mdns_component.cpp +++ b/esphome/components/mdns/mdns_component.cpp @@ -107,9 +107,10 @@ void MDNSComponent::compile_records_(StaticVectorget_noise_ctx().has_psk(); txt_count++; // api_encryption or api_encryption_supported -#ifdef USE_API_PLAINTEXT - if (!api::global_api_server->get_noise_ctx().has_psk()) { +#ifndef USE_API_NOISE_PSK_FROM_YAML + if (!api_has_psk) { txt_count++; // api_provisioning } #endif @@ -168,13 +169,13 @@ void MDNSComponent::compile_records_(StaticVectorget_noise_ctx().has_psk(); - const char *encryption_key = has_psk ? TXT_API_ENCRYPTION : TXT_API_ENCRYPTION_SUPPORTED; + const char *encryption_key = api_has_psk ? TXT_API_ENCRYPTION : TXT_API_ENCRYPTION_SUPPORTED; txt_records.push_back({MDNS_STR(encryption_key), MDNS_STR(NOISE_ENCRYPTION)}); -#ifdef USE_API_PLAINTEXT - if (!has_psk) { - // Unprovisioned dual-build device: advertise that the encryption key can - // be provisioned over a zero-PSK Noise connection (no plaintext needed) +#ifndef USE_API_NOISE_PSK_FROM_YAML + if (!api_has_psk) { + // Unprovisioned device without a YAML key: advertise that the encryption + // key can be provisioned over a zero-PSK Noise connection. Gated on the + // YAML define so this survives the plaintext removal in 2027.2.0. MDNS_STATIC_CONST_CHAR(TXT_API_PROVISIONING, "api_provisioning"); MDNS_STATIC_CONST_CHAR(VALUE_ZERO_PSK, "zero-psk"); txt_records.push_back({MDNS_STR(TXT_API_PROVISIONING), MDNS_STR(VALUE_ZERO_PSK)}); diff --git a/tests/integration/fixtures/api_zero_psk_provisioning_rejects.yaml b/tests/integration/fixtures/api_zero_psk_provisioning_rejects.yaml deleted file mode 100644 index 600fc8cacf..0000000000 --- a/tests/integration/fixtures/api_zero_psk_provisioning_rejects.yaml +++ /dev/null @@ -1,6 +0,0 @@ -esphome: - name: zero-psk-rejects-test -host: -api: - encryption: -logger: diff --git a/tests/integration/test_api_zero_psk_provisioning.py b/tests/integration/test_api_zero_psk_provisioning.py index d751f023eb..bcea2a2471 100644 --- a/tests/integration/test_api_zero_psk_provisioning.py +++ b/tests/integration/test_api_zero_psk_provisioning.py @@ -36,8 +36,38 @@ async def test_api_zero_psk_provisioning( run_compiled: RunCompiledFunction, api_client_connected: APIClientConnectedFactory, ) -> None: - """Provision a key over the zero-PSK channel and verify the switch to it.""" + """Exercise the reject paths, then provision a key over the zero-PSK channel.""" async with run_compiled(yaml_config): + # --- Pre-provisioning reject paths (device state is unchanged) --- + + # A wrong (non-zero) PSK fails against the zero provisioning PSK + with pytest.raises(InvalidEncryptionKeyAPIError): + async with api_client_connected( + noise_psk=base64.b64encode(b"w" * 32).decode(), timeout=5 + ) as client: + await client.device_info() + + # A plaintext client and a zero-PSK client can be connected at the + # same time while the device is unprovisioned + async with ( + api_client_connected() as plaintext_client, + api_client_connected(noise_psk=ZERO_PSK) as noise_client, + ): + plaintext_info = await plaintext_client.device_info() + noise_info = await noise_client.device_info() + # Both transports advertise provisioning support so old and new + # clients can decide how to provision + assert plaintext_info.api_encryption_provisionable is True + assert noise_info.api_encryption_provisionable is True + + # The all-zeros key is reserved as the provisioning PSK and is + # rejected on both transports + zero_key = base64.b64encode(bytes(32)) + assert await noise_client.noise_encryption_set_key(zero_key) is False + assert await plaintext_client.noise_encryption_set_key(zero_key) is False + + # --- Provision over the zero-PSK channel --- + # The unprovisioned device accepts the all-zeros PSK; the handshake's # ephemeral-ephemeral DH encrypts everything that follows async with api_client_connected(noise_psk=ZERO_PSK) as client: @@ -68,45 +98,6 @@ async def test_api_zero_psk_provisioning( await client.device_info() -@pytest.mark.asyncio -async def test_api_zero_psk_provisioning_rejects( - yaml_config: str, - run_compiled: RunCompiledFunction, - api_client_connected: APIClientConnectedFactory, -) -> None: - """Exercise the paths that must not provision a key.""" - async with run_compiled(yaml_config): - # A wrong (non-zero) PSK fails against the zero provisioning PSK - with pytest.raises(InvalidEncryptionKeyAPIError): - async with api_client_connected( - noise_psk=base64.b64encode(b"w" * 32).decode(), timeout=5 - ) as client: - await client.device_info() - - # A plaintext client and a zero-PSK client can be connected at the - # same time while the device is unprovisioned - async with ( - api_client_connected() as plaintext_client, - api_client_connected(noise_psk=ZERO_PSK) as noise_client, - ): - plaintext_info = await plaintext_client.device_info() - noise_info = await noise_client.device_info() - # Both transports advertise provisioning support so old and new - # clients can decide how to provision - assert plaintext_info.api_encryption_provisionable is True - assert noise_info.api_encryption_provisionable is True - - # The all-zeros key is reserved as the provisioning PSK and is - # rejected on both transports - zero_key = base64.b64encode(bytes(32)) - assert await noise_client.noise_encryption_set_key(zero_key) is False - assert await plaintext_client.noise_encryption_set_key(zero_key) is False - - # Nothing was provisioned: the zero PSK still works - async with api_client_connected(noise_psk=ZERO_PSK) as client: - assert (await client.device_info()).api_encryption_provisionable is True - - @pytest.mark.asyncio async def test_api_zero_psk_provisioning_plaintext( yaml_config: str, @@ -125,7 +116,7 @@ async def test_api_zero_psk_provisioning_plaintext( await asyncio.sleep(KEY_ACTIVATION_DELAY) # The deprecation warning was logged - assert any("unencrypted connection" in line for line in log_lines) + assert any("deprecated" in line for line in log_lines) # The new key works; the zero PSK does not async with api_client_connected(noise_psk=NEW_KEY.decode()) as client: