Skip to content

Commit bf71ccb

Browse files
agnersclaude
andauthored
Enforce panel_admin for ingress sessions (#7308)
* Enforce panel_admin for ingress sessions Core allows non-admin users to create ingress sessions and read add-on info for any app slug, since the frontend needs both to open apps that set panel_admin: false. The Supervisor ingress proxy only validated the session cookie, so a non-admin user could open the ingress UI of any app, including admin-only ones such as the Terminal or VS Code app. Resolve the user's role in Supervisor from the Core user record that session creation already fetches: a user is admin when they are the owner or an active member of the system-admin group, mirroring Core's definition. The role is cached on the session and the ingress proxy returns 403 when a non-admin session targets an app with panel_admin set. This protects every installation as soon as Supervisor updates, independent of the Core version. A user ID is now required to create a session, which Core has sent since 2023.9. Sessions without user data are dropped on load. Cached roles are cleared on load and resolved again on first use, so sessions that survive a restart pick up role changes. While Core is unreachable a session is treated as non-admin for that request without caching the result. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Refresh the ingress session user on resolve and drop removed users The user name and username stored with an ingress session were looked up once at creation and forwarded to the app unchanged for the lifetime of the session, so a renamed user kept sending the old name and a removed user kept sending their identity. When a session is resolved on first use, store the fresh user record along with the role instead of only the admin flag. Invalidate the session when the user no longer exists, and reject session creation for a user ID Core does not know. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Invalidate ingress sessions of deactivated users A deactivated user cannot log in to Core anymore, but an existing ingress session kept working for apps without an admin-only panel. Treat inactive users like removed ones: reject session creation and invalidate the session when it is resolved. Also parametrize the user list mock explicitly in the tests instead of branching on the parameter type. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Cache the ingress role per user instead of per session The admin role is runtime state derived from Core, not part of the session. Storing it on the session added a field to the on-disk session format that was written on unload only to be dropped again on load, and used None on the session data to mark unresolved sessions. Keep the role in an in-memory dict keyed by user ID on the Ingress manager instead. The session format is unchanged, and a restart starts with an empty cache so roles are resolved again on first use. The cache only ever receives IDs of users Core knows, since session creation rejects unknown users and resolving drops their sessions, so it is bounded by the user list. All sessions of a user share the entry and every new session refreshes it, so a demoted user's older sessions lose admin access as soon as the user creates a new session instead of only after a restart or expiry. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Cache the ingress user record instead of only the role A session created while Core was unreachable only stores the user ID. Once the user's role got cached through another session, the handler no longer resolved that session, so the app never received the user name headers for it. Cache the whole user record per user ID instead and build the forwarded headers from it. All sessions of a user then share one up-to-date name, and the record stored with the session is only the fallback while Core is unreachable, so it no longer needs to be rewritten on resolve. Also split the admin check so the denial log names the user. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
1 parent 44f16a5 commit bf71ccb

5 files changed

Lines changed: 504 additions & 44 deletions

File tree

‎supervisor/api/ingress.py‎

Lines changed: 67 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
from aiohttp import ClientTimeout, ClientWSTimeout, WSCloseCode, hdrs, web
1010
from aiohttp.web_exceptions import (
1111
HTTPBadGateway,
12+
HTTPForbidden,
1213
HTTPServiceUnavailable,
1314
HTTPUnauthorized,
1415
)
@@ -35,7 +36,7 @@
3536
IngressSessionData,
3637
)
3738
from ..coresys import CoreSysAttributes
38-
from ..exceptions import HomeAssistantAPIError
39+
from ..exceptions import APIError, HomeAssistantAPIError
3940
from .const import COOKIE_INGRESS, WEBSOCKETS
4041
from .utils import api_process, api_validate, require_home_assistant, stop_on_disconnect
4142

@@ -48,7 +49,7 @@
4849
"""Expected optional payload of create session request"""
4950
SCHEMA_INGRESS_CREATE_SESSION_DATA = vol.Schema(
5051
{
51-
vol.Optional(ATTR_SESSION_DATA_USER_ID): str,
52+
vol.Required(ATTR_SESSION_DATA_USER_ID): str,
5253
}
5354
)
5455

@@ -113,19 +114,21 @@ async def panels(self, request: web.Request) -> dict[str, Any]:
113114
@require_home_assistant
114115
async def create_session(self, request: web.Request) -> dict[str, Any]:
115116
"""Create a new session."""
116-
schema_ingress_config_session_data = await api_validate(
117-
SCHEMA_INGRESS_CREATE_SESSION_DATA, request
118-
)
119-
data: IngressSessionData | None = None
117+
body = await api_validate(SCHEMA_INGRESS_CREATE_SESSION_DATA, request)
118+
user_id = body[ATTR_SESSION_DATA_USER_ID]
120119

121-
if ATTR_SESSION_DATA_USER_ID in schema_ingress_config_session_data:
122-
user = await self._find_user_by_id(
123-
schema_ingress_config_session_data[ATTR_SESSION_DATA_USER_ID]
124-
)
125-
if user:
126-
data = IngressSessionData(user)
120+
try:
121+
user = await self._find_user_by_id(user_id)
122+
except HomeAssistantAPIError as err:
123+
_LOGGER.warning("Could not fetch list of users: %s", err)
124+
# Keep the ID so the user can be resolved on first use
125+
user = HomeAssistantUser(user_id)
126+
else:
127+
if user is None or not user.is_active:
128+
raise APIError(f"User {user_id} does not exist or is inactive")
129+
self.sys_ingress.users[user.id] = user
127130

128-
session = self.sys_ingress.create_session(data)
131+
session = self.sys_ingress.create_session(IngressSessionData(user))
129132
return {ATTR_SESSION: session}
130133

131134
@api_process
@@ -154,13 +157,21 @@ async def handler(
154157
app = self._extract_app(request)
155158
path = request.match_info.get("path", "")
156159
session_data = self.sys_ingress.get_session_data(session)
160+
user = await self._get_user(session, session_data) if session_data else None
161+
if app.panel_admin and (user is None or not user.is_admin):
162+
_LOGGER.warning(
163+
"Ingress access to %s denied for non-admin user %s",
164+
app.slug,
165+
user.id if user else None,
166+
)
167+
raise HTTPForbidden
157168
try:
158169
# Websocket
159170
if _is_websocket(request):
160-
return await self._handle_websocket(request, app, path, session_data)
171+
return await self._handle_websocket(request, app, path, user)
161172

162173
# Request
163-
return await self._handle_request(request, app, path, session_data)
174+
return await self._handle_request(request, app, path, user)
164175

165176
except aiohttp.ClientError as err:
166177
_LOGGER.error("Ingress error: %s", err)
@@ -172,7 +183,7 @@ async def _handle_websocket(
172183
request: web.Request,
173184
app: App,
174185
path: str,
175-
session_data: IngressSessionData | None,
186+
user: HomeAssistantUser | None,
176187
) -> web.WebSocketResponse:
177188
"""Ingress route for websocket."""
178189
if hdrs.SEC_WEBSOCKET_PROTOCOL in request.headers:
@@ -198,7 +209,7 @@ async def _handle_websocket(
198209

199210
# Preparing
200211
url = self._create_url(app, path)
201-
source_header = _init_header(request, app, session_data)
212+
source_header = _init_header(request, app, user)
202213

203214
# Support GET query
204215
if request.query_string:
@@ -238,11 +249,11 @@ async def _handle_request(
238249
request: web.Request,
239250
app: App,
240251
path: str,
241-
session_data: IngressSessionData | None,
252+
user: HomeAssistantUser | None,
242253
) -> web.Response | web.StreamResponse:
243254
"""Ingress route for request."""
244255
url = self._create_url(app, path)
245-
source_header = _init_header(request, app, session_data)
256+
source_header = _init_header(request, app, user)
246257

247258
# Passing the raw stream breaks requests for some webservers
248259
# since we just need it for POST requests really, for all other methods
@@ -320,28 +331,54 @@ async def _handle_request(
320331
return response
321332

322333
async def _find_user_by_id(self, user_id: str) -> HomeAssistantUser | None:
323-
"""Find user object by the user's ID."""
334+
"""Find user object by the user's ID.
335+
336+
Raises HomeAssistantAPIError if the user list cannot be fetched.
337+
"""
338+
users = await self.sys_homeassistant.list_users()
339+
return next((user for user in users if user.id == user_id), None)
340+
341+
async def _get_user(
342+
self, session: str, session_data: IngressSessionData
343+
) -> HomeAssistantUser:
344+
"""Return the user record of a session, resolved from Core once.
345+
346+
While Core is unreachable the record stored with the session is
347+
returned, which denies admin access without caching. Sessions of
348+
removed or deactivated users are invalidated.
349+
"""
350+
if user := self.sys_ingress.users.get(session_data.user.id):
351+
return user
352+
324353
try:
325-
users = await self.sys_homeassistant.list_users()
354+
user = await self._find_user_by_id(session_data.user.id)
326355
except HomeAssistantAPIError as err:
327356
_LOGGER.warning("Could not fetch list of users: %s", err)
328-
return None
357+
return session_data.user
329358

330-
return next((user for user in users if user.id == user_id), None)
359+
if user is None or not user.is_active:
360+
_LOGGER.warning(
361+
"Ingress session of removed or inactive user %s", session_data.user.id
362+
)
363+
self.sys_ingress.remove_session(session)
364+
raise HTTPUnauthorized
365+
366+
self.sys_ingress.users[user.id] = user
367+
return user
331368

332369

333370
def _init_header(
334-
request: web.Request, app: App, session_data: IngressSessionData | None
371+
request: web.Request, app: App, user: HomeAssistantUser | None
335372
) -> CIMultiDict[str]:
336373
"""Create initial header."""
337374
headers = CIMultiDict[str]()
338375

339-
if session_data is not None:
340-
headers[HEADER_REMOTE_USER_ID] = session_data.user.id
341-
if session_data.user.username is not None:
342-
headers[HEADER_REMOTE_USER_NAME] = session_data.user.username
343-
if session_data.user.name is not None:
344-
headers[HEADER_REMOTE_USER_DISPLAY_NAME] = session_data.user.name
376+
if user is not None:
377+
headers[HEADER_REMOTE_USER_ID] = user.id
378+
if user.username is not None:
379+
headers[HEADER_REMOTE_USER_NAME] = user.username
380+
if user.name is not None:
381+
headers[HEADER_REMOTE_USER_DISPLAY_NAME] = user.name
345382

346383
# filter flags
347384
for name, value in request.headers.items():

‎supervisor/const.py‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -609,6 +609,9 @@ class FeatureFlag(StrEnum):
609609
SUPERVISOR_WEBSOCKET_V2_API = "supervisor_websocket_v2_api"
610610

611611

612+
HOMEASSISTANT_GROUP_ID_ADMIN = "system-admin"
613+
614+
612615
@dataclass
613616
class HomeAssistantUser:
614617
"""A Home Assistant Core user.
@@ -627,6 +630,13 @@ class HomeAssistantUser:
627630
system_generated: bool = False
628631
group_ids: list[str] | None = None
629632

633+
@property
634+
def is_admin(self) -> bool:
635+
"""Return if the user is an admin, mirroring Core's User.is_admin."""
636+
return self.is_owner or (
637+
self.is_active and HOMEASSISTANT_GROUP_ID_ADMIN in (self.group_ids or [])
638+
)
639+
630640
@classmethod
631641
def from_dict(cls, data: Mapping[str, Any]) -> Self:
632642
"""Return object from dictionary representation."""

‎supervisor/ingress.py‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
FILE_HASSIO_INGRESS,
1414
INGRESS_DYNAMIC_PORT_MAX,
1515
INGRESS_DYNAMIC_PORT_MIN,
16+
HomeAssistantUser,
1617
IngressSessionData,
1718
IngressSessionDataDict,
1819
)
@@ -34,6 +35,9 @@ def __init__(self, coresys: CoreSys):
3435
super().__init__(FILE_HASSIO_INGRESS, SCHEMA_INGRESS_CONFIG)
3536
self.coresys: CoreSys = coresys
3637
self.tokens: dict[str, str] = {}
38+
# User records by ID, resolved from Core and not persisted so a
39+
# restart picks up user changes. Only known users are ever added.
40+
self.users: dict[str, HomeAssistantUser] = {}
3741

3842
def get(self, token: str) -> App | None:
3943
"""Return app they have this ingress token."""
@@ -105,10 +109,12 @@ def _cleanup_sessions(self) -> None:
105109
if valid_dt < now:
106110
continue
107111

108-
# Is valid
112+
# Sessions without user data predate Core 2023.9 and cannot be authorized
113+
if not (session_data := self.sessions_data.get(session)):
114+
continue
115+
109116
sessions[session] = valid
110-
if session_data := self.sessions_data.get(session):
111-
sessions_data[session] = session_data
117+
sessions_data[session] = session_data
112118

113119
# Write back
114120
self.sessions.clear()
@@ -136,6 +142,11 @@ def create_session(self, data: IngressSessionData | None = None) -> str:
136142

137143
return session
138144

145+
def remove_session(self, session: str) -> None:
146+
"""Invalidate a session."""
147+
self.sessions.pop(session, None)
148+
self.sessions_data.pop(session, None)
149+
139150
def validate_session(self, session: str) -> bool:
140151
"""Return True if session valid and make it longer valid."""
141152
if session not in self.sessions:

0 commit comments

Comments
 (0)