From 0c2a05aae0bcd2696dc03d8d8b6e5446af132b00 Mon Sep 17 00:00:00 2001 From: Andrew Walker Date: Tue, 23 Jul 2024 11:43:29 -0700 Subject: [PATCH] Remove check for whether localhost connection is root We don't use this functionality internally and it's a potential security liability if someone decides to set up their own internal proxy to middlewared socket that's running as root. (cherry picked from commit 0673f0de2b7c17e00181a84fb9343cfed3696b59) --- src/middlewared/middlewared/auth.py | 4 --- src/middlewared/middlewared/plugins/auth.py | 27 +++---------------- src/middlewared/middlewared/utils/nginx.py | 9 ------- .../middlewared/utils/privilege.py | 8 ++---- tests/api2/test_ip_auth.py | 24 ----------------- tests/api2/test_localhost_ws_auth.py | 11 ++++++++ 6 files changed, 16 insertions(+), 67 deletions(-) delete mode 100644 tests/api2/test_ip_auth.py create mode 100644 tests/api2/test_localhost_ws_auth.py diff --git a/src/middlewared/middlewared/auth.py b/src/middlewared/middlewared/auth.py index 5100b020389c4..7dc92f4398144 100644 --- a/src/middlewared/middlewared/auth.py +++ b/src/middlewared/middlewared/auth.py @@ -59,10 +59,6 @@ class UnixSocketSessionManagerCredentials(UserSessionManagerCredentials): pass -class RootTcpSocketSessionManagerCredentials(UserSessionManagerCredentials): - pass - - class LoginPasswordSessionManagerCredentials(UserSessionManagerCredentials): pass diff --git a/src/middlewared/middlewared/plugins/auth.py b/src/middlewared/middlewared/plugins/auth.py index 9202be3433466..09a956dcf0e77 100644 --- a/src/middlewared/middlewared/plugins/auth.py +++ b/src/middlewared/middlewared/plugins/auth.py @@ -8,9 +8,8 @@ import psutil from middlewared.auth import (SessionManagerCredentials, UserSessionManagerCredentials, - UnixSocketSessionManagerCredentials, RootTcpSocketSessionManagerCredentials, - LoginPasswordSessionManagerCredentials, ApiKeySessionManagerCredentials, - TrueNasNodeSessionManagerCredentials) + UnixSocketSessionManagerCredentials, LoginPasswordSessionManagerCredentials, + ApiKeySessionManagerCredentials, TrueNasNodeSessionManagerCredentials) from middlewared.schema import accepts, Any, Bool, Datetime, Dict, Int, List, Password, Patch, Ref, returns, Str from middlewared.service import ( Service, filterable, filterable_returns, filter_list, no_auth_required, no_authz_required, @@ -18,7 +17,6 @@ ) from middlewared.service_exception import MatchNotFound import middlewared.sqlalchemy as sa -from middlewared.utils.nginx import get_peer_process from middlewared.utils.origin import UnixSocketOrigin, TCPIPOrigin from middlewared.utils.crypto import generate_token @@ -210,10 +208,7 @@ def is_internal_session(session): if isinstance(session.app.origin, UnixSocketOrigin) and session.app.origin.uid == 0: return True - if isinstance(session.app.authenticated_credentials, ( - RootTcpSocketSessionManagerCredentials, - TrueNasNodeSessionManagerCredentials, - )): + if isinstance(session.app.authenticated_credentials, TrueNasNodeSessionManagerCredentials): return True return False @@ -673,22 +668,6 @@ async def check_permission(middleware, app): await AuthService.session_manager.login(app, UnixSocketSessionManagerCredentials(user)) return - if isinstance(origin, TCPIPOrigin): - if not (origin.addr.startswith('127.') or origin.addr == '::1'): - return - - # This is an expensive operation, but it is only performed for localhost TCP connections which are rare - if process := await middleware.run_in_thread(get_peer_process, origin.addr, origin.port): - try: - euid = (await middleware.run_in_thread(process.uids)).effective - except psutil.NoSuchProcess: - pass - else: - if euid == 0: - user = await middleware.call('auth.authenticate_root') - await AuthService.session_manager.login(app, RootTcpSocketSessionManagerCredentials(user)) - return - def setup(middleware): middleware.event_register('auth.sessions', 'Notification of new and removed sessions.') diff --git a/src/middlewared/middlewared/utils/nginx.py b/src/middlewared/middlewared/utils/nginx.py index 7c81036e89fa1..a4223cd919ec9 100644 --- a/src/middlewared/middlewared/utils/nginx.py +++ b/src/middlewared/middlewared/utils/nginx.py @@ -3,15 +3,6 @@ from psutil._common import addr -def get_peer_process(remote_addr, remote_port): - for connection in psutil.net_connections(kind='tcp'): - if connection.laddr == addr(remote_addr, remote_port): - try: - return psutil.Process(connection.pid) - except psutil.ProcessNotFound: - return None - - def get_remote_addr_port(request): try: remote_addr, remote_port = request.transport.get_extra_info("peername") diff --git a/src/middlewared/middlewared/utils/privilege.py b/src/middlewared/middlewared/utils/privilege.py index f350af98005ea..b5fd8c315f64b 100644 --- a/src/middlewared/middlewared/utils/privilege.py +++ b/src/middlewared/middlewared/utils/privilege.py @@ -1,6 +1,5 @@ import enum -from middlewared.auth import (RootTcpSocketSessionManagerCredentials, - TrueNasNodeSessionManagerCredentials) +from middlewared.auth import TrueNasNodeSessionManagerCredentials from middlewared.role import ROLES @@ -29,10 +28,7 @@ def credential_has_full_admin(credential: object) -> bool: if credential.is_user_session and 'FULL_ADMIN' in credential.user['privilege']['roles']: return True - if isinstance(credential, ( - RootTcpSocketSessionManagerCredentials, - TrueNasNodeSessionManagerCredentials - )): + if isinstance(credential, TrueNasNodeSessionManagerCredentials): return True if credential.allowlist is None: diff --git a/tests/api2/test_ip_auth.py b/tests/api2/test_ip_auth.py deleted file mode 100644 index a249f634aec74..0000000000000 --- a/tests/api2/test_ip_auth.py +++ /dev/null @@ -1,24 +0,0 @@ -import json -import shlex - -import pytest - -from middlewared.test.integration.assets.account import user -from middlewared.test.integration.utils import ssh - - -@pytest.mark.parametrize("url", ["127.0.0.1", "127.0.0.1:6000"]) -@pytest.mark.parametrize("root", [True, False]) -def test_tcp_connection_from_localhost(url, root): - cmd = f"midclt -u ws://{url}/websocket call auth.sessions '[[\"current\", \"=\", true]]' '{{\"get\": true}}'" - if root: - assert json.loads(ssh(cmd))["credentials"] == "ROOT_TCP_SOCKET" - else: - with user({ - "username": "unprivileged", - "full_name": "Unprivileged User", - "group_create": True, - "password": "test1234", - }): - result = ssh(f"sudo -u unprivileged {cmd}", check=False, complete_response=True) - assert "Not authenticated" in result["stderr"] diff --git a/tests/api2/test_localhost_ws_auth.py b/tests/api2/test_localhost_ws_auth.py new file mode 100644 index 0000000000000..36cb392fef9a9 --- /dev/null +++ b/tests/api2/test_localhost_ws_auth.py @@ -0,0 +1,11 @@ +from middlewared.test.integration.utils import ssh + + +def test__authentication_required_localhost(): + cmd = 'midclt -u ws://localhost/websocket call user.query' + resp = ssh(cmd, check=False, complete_response=True) + + assert not resp['result'] + + assert 'Not authenticated' in resp['stderr'] +