Refuse privileged HA commands on unencrypted connections (B2) - #162
Merged
Conversation
Firmware Install, Factory Reset and Reboot were reachable by anything on the LAN while the device is keyless. The ESPHome protocol answers AuthenticationRequest unconditionally, so a Noise session is the only real credential, and espex only enforces encryption once a PSK exists. That keyless window is how Home Assistant adopts the device, so it can't simply be closed — but it shouldn't extend to commands that reboot, wipe or reflash. Uses espex's new handle_command/2 to see the connection's security context and refuse just those three; diagnostic entities keep working keyless. Refusals return :ok because the protocol has no denied reply for a command — the warning is the audit trail. NOTE: espex is temporarily pinned to a branch here. It must return to a hex constraint before merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bbangert
force-pushed
the
feat/refuse-privileged-commands-unencrypted
branch
from
August 15, 2026 22:18
f7c9a4e to
e6a66f8
Compare
bbangert
marked this pull request as ready for review
August 15, 2026 22:18
There was a problem hiding this comment.
Pull request overview
Adds connection-aware authorization to protect destructive ESPHome commands from plaintext clients.
Changes:
- Rejects firmware install, factory reset, and reboot commands on unencrypted connections.
- Adds tests covering privileged-command identification and encrypted/plaintext behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/universal_proxy/esphome/entity_provider.ex |
Adds security-context command filtering and audit logging. |
test/universal_proxy/esphome/entity_provider_test.exs |
Tests privileged-command filtering. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The gate matches on entity key, so the update entity's CHECK is refused alongside UPDATE. That is deliberate — CHECK reaches FirmwareUpdate.check/0, which hits GitHub with no debounce, so leaving it open would let an unauthenticated client drive the API into its secondary rate limits — but the docstring said "Factory Reset, Reboot or Firmware Update", which reads as install-only and never said so. Says it plainly now, with the reasoning, and adds a test pinning the behaviour so it isn't quietly reversed later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes B2 from the
/phx:reviewof #161.The problem
Firmware Install, Factory Reset and Reboot are reachable by anything on the LAN while the device is keyless. Verified in the dependency source rather than inferred:
esphome/supervisor.ex:66-70—psk = PskStore.load_psk()isniluntil Home Assistant provisions, so a fresh device runs keyless.deps/espex/lib/espex/dispatch.ex:105-107— espex answers everyAuthenticationRequestwithinvalid_password: false. There is no password check to fail.deps/espex/lib/espex/dispatch.ex:425-431— entity commands route on adapter presence alone. No auth, no encryption, no hello gate.Once a PSK exists espex enforces encryption properly (a plaintext client gets a handshake rejection), so the exposure is confined to the keyless window — but that window is open indefinitely on a device nobody has adopted yet.
Scoped honestly: this is not arbitrary-firmware execution.
:repois only settable over SSH/IEx, so the reachable outcome is "force-install the legitimate release, then reboot" — availability, not RCE. Factory Reset and Reboot were already on this surface before #161; that PR added flash to it.Why not just close the window
The keyless window is how HA adopts the device — it connects plaintext and provisions the PSK. Refusing all commands keyless would break onboarding, and refusing none leaves reboot/wipe/reflash exposed. The distinction is per-entity, which is app knowledge, so espex#23 passes the security context and this PR makes the policy decision:
Those three are refused unless the connection is encrypted; every diagnostic entity keeps working keyless. Refusals return
:ok— the protocol has no "denied" reply for a command, so the warning is the audit trail.What this does not fix
Only the app's own entities. Any other privileged adapter surface is out of scope, and the underlying "keyless means anonymous" property of the protocol is unchanged — this narrows the blast radius rather than removing it. The real close is HA provisioning a PSK, which the app already invites via
accepts_key_provisioning.Pairs with nerves_github_updater#3 (B3), which removes the replay loop independently — so even a caller that is allowed to install can't loop the device.
Verification
mix compile --warnings-as-errors— PASSmise run test— 958 passed, 3 excluded (5 new)mix dialyzer— PASS (7 errors, 7 skipped, 0 unnecessary skips)MIX_TARGET=rpi3 mix compile— PASSMutation-tested: dropping
firmware_updatefrom the privileged list fails 2 tests.Before merge
mix deps.getand the suite🤖 Generated with Claude Code