-
Notifications
You must be signed in to change notification settings - Fork 803
Add API to manage SSH authorized keys on Home Assistant OS #7039
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 4 commits
f82109d
8ab0fe2
bd04e62
a39f211
9e567b1
dbe711c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -146,7 +146,7 @@ class _AppSecurityPatterns: | |
| r"|/multicast/.+" | ||
| r"|/network/.+" | ||
| r"|/observer/.+" | ||
| r"|/os/(?!datadisk/wipe).+" | ||
| r"|/os/(?!datadisk/wipe|ssh/authorized_keys).+" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we make this a core only endpoint rather then a manager one? Putting it in manager would give many apps the ability to call it. I know its hardly the only attack avenue if you consider the idea of a malicious app but it still just doesn't seem like capability we want to allow apps to do. The downside would be the SSH app also can't call it which is probably the one app we'd prefer to allow. But as long as we make the proposed UI this seems like an acceptable situation to only allow host ssh key management from HA UI and the host shell itself. |
||
| r"|/refresh_updates" | ||
| r"|/resolution/.+" | ||
| r"|/security/.+" | ||
|
|
@@ -226,7 +226,7 @@ class _AppSecurityPatterns: | |
| r"|/multicast/.+" | ||
| r"|/network/.+" | ||
| r"|/observer/.+" | ||
| r"|/os/(?!datadisk/wipe).+" | ||
| r"|/os/(?!datadisk/wipe|ssh/authorized_keys).+" | ||
| r"|/reload_updates" | ||
| r"|/resolution/.+" | ||
| r"|/security/.+" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,10 +15,12 @@ | |
| from ..exceptions import ( | ||
| DBusError, | ||
| DBusNotConnectedError, | ||
| HassOSError, | ||
| HassOSJobError, | ||
| HassOSSlotNotFound, | ||
| HassOSSlotUpdateError, | ||
| HassOSUpdateError, | ||
| HostError, | ||
| ) | ||
| from ..jobs.const import JobConcurrency, JobCondition | ||
| from ..jobs.decorator import Job | ||
|
|
@@ -27,6 +29,16 @@ | |
|
|
||
| _LOGGER: logging.Logger = logging.getLogger(__name__) | ||
|
|
||
| # SSH service on Home Assistant OS consuming /root/.ssh/authorized_keys | ||
| DROPBEAR_SERVICE = "dropbear.service" | ||
|
|
||
| # OS Agent releases before this return the os.Remove error when clearing an | ||
| # already absent authorized_keys file (inverted error check) | ||
| CLEAR_SSH_AUTH_KEYS_FIXED_VERSION = AwesomeVersion("1.10.0") | ||
| CLEAR_SSH_AUTH_KEYS_MISSING_FILE_ERROR = ( | ||
| "remove /root/.ssh/authorized_keys: no such file or directory" | ||
| ) | ||
|
|
||
|
|
||
| @dataclass(slots=True, frozen=True) | ||
| class SlotStatus: | ||
|
|
@@ -501,3 +513,55 @@ async def set_boot_slot(self, boot_name: str) -> None: | |
|
|
||
| _LOGGER.info("Rebooting into new boot slot now") | ||
| await self.sys_host.control.reboot() | ||
|
|
||
| @Job( | ||
| name="os_manager_set_ssh_authorized_keys", | ||
| conditions=[JobCondition.HAOS], | ||
| on_condition=HassOSJobError, | ||
| concurrency=JobConcurrency.REJECT, | ||
| internal=True, | ||
| ) | ||
| async def set_ssh_authorized_keys(self, keys: list[str]) -> None: | ||
| """Replace root's SSH authorized keys on the host and start dropbear. | ||
|
|
||
| OS Agent validates each key since 1.10.0 and only offers clear and | ||
| append operations, so the replacement is not atomic: if an append is | ||
| rejected or fails, keys added before it remain in place. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like a great reason to lay out the API like this:
This makes the API easy to use, easy to translate to the CLI and ensures that no API call can partially succeed by having some OS Agent operations succeed before one fails. Since each API maps 1:1 with an OS Agent DBus API. |
||
| """ | ||
| _LOGGER.info("Replacing SSH authorized keys on host (%d keys)", len(keys)) | ||
| try: | ||
| await self.sys_dbus.agent.system.clear_ssh_auth_keys() | ||
| except DBusError as err: | ||
| # On affected OS Agent releases the missing-file error is the | ||
| # empty state clearing aims for, so treat it as success there. | ||
| if ( | ||
| self.sys_dbus.agent.version >= CLEAR_SSH_AUTH_KEYS_FIXED_VERSION | ||
| or CLEAR_SSH_AUTH_KEYS_MISSING_FILE_ERROR not in str(err) | ||
| ): | ||
| raise HassOSError( | ||
| f"Can't clear SSH authorized keys: {err!s}", _LOGGER.error | ||
| ) from err | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have to require users pass in every key every time? Can we offer a way to add to existing keys instead of replacing the full list each time? Since OSAgent offers a designated clear authorized keys API it seems unnecessary frustrating to make one API that does an upsert. Wouldn't it be easier to have a POST API that adds one or more keys and a DELETE API that clears the file?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah agreed mirroring the OS Agent API makes more sense. Maybe we should have a get anyways at one point. I think I was concerned about unnecessary information leak when initially created the OS Agent implementation, but maybe that was a bit overly cautious. |
||
|
|
||
| for key in keys: | ||
| try: | ||
| await self.sys_dbus.agent.system.add_ssh_auth_key(key) | ||
| except DBusError as err: | ||
| raise HassOSError( | ||
| f"Can't add SSH authorized key: {err!s}", _LOGGER.error | ||
| ) from err | ||
|
|
||
| if not keys: | ||
| return | ||
|
|
||
| # dropbear on Home Assistant OS is gated by | ||
| # ConditionFileNotEmpty=/root/.ssh/authorized_keys, which systemd only | ||
| # evaluates when the unit starts. A running dropbear re-reads the file | ||
| # on every authentication attempt and starting an active unit is a | ||
| # no-op, so only the stopped service needs this. | ||
| try: | ||
| await self.sys_host.services.start(DROPBEAR_SERVICE) | ||
| except (HostError, DBusError) as err: | ||
| raise HassOSError( | ||
| f"SSH authorized keys written, but can't start dropbear: {err!s}", | ||
| _LOGGER.error, | ||
| ) from err | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we truly want this endpoint to replace the entire authorized keys file with this new set of keys and not add a key to the set we should use
PUThere notPOSTimo. Per mozilla guidelines:As defined this is idempotent and replaces the resource (authorized key file in this case) so PUT is the better fit. But personally I would prefer this use
POST, not be idempotent, and just append one or more keys to the existing file.