mirror of
https://github.com/pgadmin-org/pgadmin4.git
synced 2026-08-17 16:34:44 -05:00
fix: unbound keyring probe at config-import time can hang desktop startup
evaluate_and_patch_config() called keyring.get_password() synchronously at `import config` time to detect a selected-but-unusable OS keyring backend. On Debian 13 (and similar headless/RDP sessions with no live D-Bus/GNOME-Keyring session), that call - and even the keyring import itself - can block forever, freezing the whole desktop app before it ever starts. Revert evaluate_config.py to the pre-9.17 synchronous check (backend name only, no get_password call). Move the usability probe into pgadmin.utils.keyring_probe, run from create_app() in a background daemon thread so it never delays startup, isolated in its own process via subprocess.Popen(sys.executable, '-c', ...) so a hang can actually be killed - a thread-only timeout can't do this, it would leave CPython's per-module import lock held forever and wedge any later `import keyring` in the parent process too. subprocess.Popen (plain fork+exec), not multiprocessing.Process, is required here: create_app() runs at the top level of pgAdmin4.py while it's still being imported, and multiprocessing's spawn start method refuses to start a child before the current process finishes bootstrapping its __main__ module. An earlier version of this fix used multiprocessing and crashed the probe thread with RuntimeError on every single test run, which corrupted the SQLAlchemy/sqlite session for the rest of app init and surfaced as an unrelated-looking "attempt to write a readonly database" error on module_preference inserts. config.USE_OS_SECRET_STORAGE is only ever read from request handlers requiring an authenticated session, never at import time or inside create_app() itself, so the async resolution race is safe in practice. Tests mock subprocess.Popen for the timeout/kill/config-fallback orchestration (deterministic, no real 3s wait or backend dependency), plus one test class that runs the real probe script in a real subprocess against a fake keyring module injected via PYTHONPATH, so the script body itself has real coverage. Verified with the full regression suite (--exclude feature_tests) in both desktop mode (2134 passed, 0 failed) and server mode (2251 passed, 0 failed); remaining skips are pre-existing pgAgent-dependent job tests.
This commit is contained in:
Ashesh Vashi
@@ -210,6 +210,12 @@ def create_app(app_name=None):
|
||||
# we don't want it to redirect to main page after password
|
||||
# change operation so we will open the same password change page again.
|
||||
config.SECURITY_POST_CHANGE_VIEW = 'browser.change_password'
|
||||
else:
|
||||
# Desktop mode: re-check the keyring backend picked at config-load
|
||||
# time is actually usable, without blocking startup on a possible
|
||||
# hang (see pgadmin/keyring_probe.py for why).
|
||||
from pgadmin.utils.keyring_probe import start_async_probe
|
||||
start_async_probe(config.__dict__)
|
||||
|
||||
"""Create the Flask application, startup logging and dynamically load
|
||||
additional modules (blueprints) that are found in this directory."""
|
||||
|
||||
@@ -9,7 +9,6 @@
|
||||
|
||||
import os
|
||||
import sys
|
||||
import keyring
|
||||
import importlib.util
|
||||
|
||||
from pgadmin.utils.db_utils import normalize_database_uri
|
||||
@@ -137,27 +136,20 @@ def evaluate_and_patch_config(config: dict) -> dict:
|
||||
config.setdefault('USE_OS_SECRET_STORAGE', False)
|
||||
config.setdefault('KEYRING_NAME', '')
|
||||
else:
|
||||
k_name = keyring.get_keyring().name
|
||||
# A keyring backend may be selected (e.g. SecretService on Linux)
|
||||
# yet be completely unusable at runtime - for example in a headless
|
||||
# or RDP session where there is no running D-Bus / GNOME Keyring.
|
||||
# In that case every keyring call raises and the crypt key is never
|
||||
# set, surfacing as a bare CryptKeyMissing on every operation.
|
||||
# Probe the backend with a harmless read of an entry that never
|
||||
# exists; if the backend is healthy this returns None without
|
||||
# prompting, and if it is unusable it raises. When it is not usable,
|
||||
# disable OS secret storage so pgAdmin falls back to the
|
||||
# master-password / in-app crypt key mechanism.
|
||||
keyring_usable = k_name != 'fail Keyring'
|
||||
if keyring_usable:
|
||||
try:
|
||||
keyring.get_password(
|
||||
'pgAdmin4', 'entry_to_check_keyring_access')
|
||||
except Exception:
|
||||
keyring_usable = False
|
||||
# NOTE: whether the selected keyring backend is actually *usable*
|
||||
# (e.g. SecretService with no live D-Bus/GNOME-Keyring session) is
|
||||
# probed separately, asynchronously, from create_app() via
|
||||
# keyring_probe.start_async_probe() - see that module for why this
|
||||
# can't be done safely here at config-import time.
|
||||
config.setdefault('USE_OS_SECRET_STORAGE', True)
|
||||
k_name = ''
|
||||
try:
|
||||
import keyring
|
||||
k_name = keyring.get_keyring().name
|
||||
except Exception:
|
||||
k_name = 'fail Keyring'
|
||||
|
||||
if not keyring_usable:
|
||||
# Setup USE_OS_SECRET_STORAGE false as no usable keyring backend
|
||||
if k_name == 'fail Keyring':
|
||||
config['USE_OS_SECRET_STORAGE'] = False
|
||||
config['KEYRING_NAME'] = ''
|
||||
else:
|
||||
|
||||
@@ -0,0 +1,99 @@
|
||||
##########################################################################
|
||||
#
|
||||
# pgAdmin 4 - PostgreSQL Tools
|
||||
#
|
||||
# Copyright (C) 2013 - 2026, The pgAdmin Development Team
|
||||
# This software is released under the PostgreSQL Licence
|
||||
#
|
||||
##########################################################################
|
||||
|
||||
"""Background, hang-safe check of whether the selected OS keyring backend
|
||||
is actually usable (desktop mode only).
|
||||
|
||||
A keyring backend may be *selected* (e.g. SecretService on Linux) yet be
|
||||
completely unusable at runtime - for example in a headless or RDP session
|
||||
where there is no live D-Bus / GNOME Keyring session. In that case
|
||||
`import keyring`, `keyring.get_keyring()`, and any keyring call can block
|
||||
forever on a D-Bus call, instead of raising.
|
||||
|
||||
The probe therefore runs in its own OS process (subprocess.Popen, plain
|
||||
fork+exec) so a hang can be killed outright from the parent - a
|
||||
thread-based timeout can only stop *waiting*, it can't stop the
|
||||
import/call, and would leave CPython's per-module import lock held
|
||||
forever, wedging every later `import keyring` in the parent too.
|
||||
|
||||
NOTE: this must be `subprocess.Popen`, not `multiprocessing.Process`.
|
||||
multiprocessing's 'spawn' start method refuses to start a child while the
|
||||
current process hasn't finished bootstrapping its __main__ module yet
|
||||
(`_check_not_importing_main`) - and that's exactly the situation here,
|
||||
since this runs from create_app(), which itself executes at the top
|
||||
level of pgAdmin4.py while it's still being imported. Popen has no such
|
||||
restriction; it's an independent process from the start.
|
||||
|
||||
It's started from create_app() in a background daemon thread so it never
|
||||
delays application startup; config.USE_OS_SECRET_STORAGE / KEYRING_NAME
|
||||
are updated in place once the probe resolves (or times out).
|
||||
"""
|
||||
|
||||
import subprocess
|
||||
import sys
|
||||
import threading
|
||||
|
||||
KEYRING_PROBE_TIMEOUT = 3
|
||||
|
||||
# Kept minimal and dependency-free: runs in a fresh interpreter, so it
|
||||
# only has stdlib + keyring available, and reports its result over
|
||||
# stdout rather than needing to be pickled back to the parent.
|
||||
_PROBE_SCRIPT = (
|
||||
"try:\n"
|
||||
" import keyring\n"
|
||||
" name = keyring.get_keyring().name\n"
|
||||
" if name == 'fail Keyring':\n"
|
||||
" print('0')\n"
|
||||
" else:\n"
|
||||
" keyring.get_password(\n"
|
||||
" 'pgAdmin4', 'entry_to_check_keyring_access')\n"
|
||||
" print('1')\n"
|
||||
" print(name)\n"
|
||||
"except Exception:\n"
|
||||
" print('0')\n"
|
||||
)
|
||||
|
||||
|
||||
def _run_probe(config):
|
||||
try:
|
||||
proc = subprocess.Popen(
|
||||
[sys.executable, '-c', _PROBE_SCRIPT],
|
||||
stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, text=True)
|
||||
except Exception:
|
||||
# Can't probe - leave config as evaluate_config.py set it.
|
||||
return
|
||||
|
||||
try:
|
||||
stdout, _ = proc.communicate(timeout=KEYRING_PROBE_TIMEOUT)
|
||||
except subprocess.TimeoutExpired:
|
||||
proc.kill()
|
||||
proc.communicate()
|
||||
stdout = ''
|
||||
|
||||
lines = stdout.splitlines()
|
||||
keyring_usable = bool(lines) and lines[0] == '1'
|
||||
|
||||
if not keyring_usable:
|
||||
config['USE_OS_SECRET_STORAGE'] = False
|
||||
config['KEYRING_NAME'] = ''
|
||||
else:
|
||||
config['KEYRING_NAME'] = lines[1] if len(lines) > 1 else ''
|
||||
|
||||
|
||||
def start_async_probe(config):
|
||||
"""Kick off the keyring usability probe in the background.
|
||||
|
||||
Desktop mode only. Non-blocking - returns immediately; config is
|
||||
updated in place whenever the probe resolves.
|
||||
"""
|
||||
if config.get('SERVER_MODE'):
|
||||
return
|
||||
|
||||
threading.Thread(
|
||||
target=_run_probe, args=(config,), daemon=True).start()
|
||||
@@ -0,0 +1,234 @@
|
||||
##########################################################################
|
||||
#
|
||||
# pgAdmin 4 - PostgreSQL Tools
|
||||
#
|
||||
# Copyright (C) 2013 - 2026, The pgAdmin Development Team
|
||||
# This software is released under the PostgreSQL Licence
|
||||
#
|
||||
##########################################################################
|
||||
|
||||
"""Regression tests for the keyring-hang fix (Debian 13 / RDP D-Bus hang).
|
||||
|
||||
9.17 added a synchronous, unbounded keyring.get_password() probe at
|
||||
config-import time. When the selected backend (e.g. SecretService) has no
|
||||
live D-Bus/GNOME-Keyring session, that call - and even `import keyring`
|
||||
itself - can block forever, freezing application startup outright.
|
||||
|
||||
The fix moves the probe into pgadmin.utils.keyring_probe, run from
|
||||
create_app() in a background thread, isolated in a killable subprocess
|
||||
(subprocess.Popen, not multiprocessing - see that module's docstring for
|
||||
why) so a hang can be terminated instead of wedging the parent process.
|
||||
|
||||
Most tests below mock subprocess.Popen to verify _run_probe()'s
|
||||
orchestration (timeout -> kill(), stdout parsing, config fallback)
|
||||
quickly and deterministically - a real hang would have to actually wait
|
||||
out the timeout, and a real keyring backend's behaviour differs per
|
||||
machine/CI runner. TestProbeScriptRunsForReal is the exception: it
|
||||
executes the real _PROBE_SCRIPT in a real subprocess (via
|
||||
subprocess.Popen, unmocked) against a fake `keyring` module injected
|
||||
over PYTHONPATH, so the script's actual body - never exercised by the
|
||||
mocked tests - gets real coverage without depending on any real OS
|
||||
keyring backend."""
|
||||
|
||||
import os
|
||||
import sys
|
||||
import tempfile
|
||||
from unittest.mock import patch, MagicMock
|
||||
|
||||
from pgadmin.utils.route import BaseTestGenerator
|
||||
from pgadmin.utils import keyring_probe
|
||||
|
||||
|
||||
class _FakeProcess:
|
||||
"""Stand-in for subprocess.Popen. `communicate_result` is either a
|
||||
(stdout, stderr) tuple, or a subprocess.TimeoutExpired instance to be
|
||||
raised by the first communicate() call (mimicking a hung process)."""
|
||||
|
||||
def __init__(self, communicate_result):
|
||||
self._communicate_result = communicate_result
|
||||
self._communicate_calls = 0
|
||||
self.killed = False
|
||||
|
||||
def communicate(self, timeout=None):
|
||||
self._communicate_calls += 1
|
||||
if self._communicate_calls == 1 and isinstance(
|
||||
self._communicate_result, BaseException):
|
||||
raise self._communicate_result
|
||||
return ('', '')
|
||||
|
||||
def kill(self):
|
||||
self.killed = True
|
||||
|
||||
|
||||
class TestKeyringProbeRunsInKillableSubprocess(BaseTestGenerator):
|
||||
"""_run_probe() must resolve USE_OS_SECRET_STORAGE / KEYRING_NAME from
|
||||
whatever the isolated subprocess reports on stdout, and must kill()
|
||||
it rather than wait forever when it hangs past the timeout."""
|
||||
|
||||
scenarios = [
|
||||
('healthy backend keeps OS secret storage enabled', dict(
|
||||
stdout='1\nSecretService Keyring\n',
|
||||
expect_use_os_secret_storage=True,
|
||||
expect_keyring_name='SecretService Keyring',
|
||||
expect_killed=False,
|
||||
)),
|
||||
('backend raises disables OS secret storage', dict(
|
||||
stdout='0\n',
|
||||
expect_use_os_secret_storage=False,
|
||||
expect_keyring_name='',
|
||||
expect_killed=False,
|
||||
)),
|
||||
]
|
||||
|
||||
def runTest(self):
|
||||
fake_process = _FakeProcess((self.stdout, ''))
|
||||
fake_process.communicate = MagicMock(
|
||||
return_value=(self.stdout, ''))
|
||||
|
||||
config = {'USE_OS_SECRET_STORAGE': True}
|
||||
|
||||
with patch.object(keyring_probe.subprocess, 'Popen',
|
||||
return_value=fake_process):
|
||||
keyring_probe._run_probe(config)
|
||||
|
||||
self.assertEqual(config['USE_OS_SECRET_STORAGE'],
|
||||
self.expect_use_os_secret_storage)
|
||||
self.assertEqual(config.get('KEYRING_NAME'),
|
||||
self.expect_keyring_name)
|
||||
self.assertEqual(fake_process.killed, self.expect_killed)
|
||||
|
||||
|
||||
class TestKeyringProbeTimeoutIsKilled(BaseTestGenerator):
|
||||
"""When the subprocess hangs past the timeout, _run_probe() must
|
||||
kill() it and fall back to disabling OS secret storage, rather than
|
||||
waiting on communicate() forever."""
|
||||
|
||||
def runTest(self):
|
||||
import subprocess as subprocess_module
|
||||
|
||||
fake_process = _FakeProcess(
|
||||
subprocess_module.TimeoutExpired(cmd='probe', timeout=3))
|
||||
|
||||
config = {'USE_OS_SECRET_STORAGE': True}
|
||||
|
||||
with patch.object(keyring_probe.subprocess, 'Popen',
|
||||
return_value=fake_process):
|
||||
keyring_probe._run_probe(config)
|
||||
|
||||
self.assertTrue(fake_process.killed)
|
||||
self.assertFalse(config['USE_OS_SECRET_STORAGE'])
|
||||
self.assertEqual(config.get('KEYRING_NAME'), '')
|
||||
|
||||
|
||||
class TestStartAsyncProbeIsNonBlockingAndServerModeAware(BaseTestGenerator):
|
||||
"""start_async_probe() must never run the probe inline (it would
|
||||
reintroduce the startup hang), and must be a no-op in server mode,
|
||||
where OS secret storage doesn't apply."""
|
||||
|
||||
scenarios = [
|
||||
('server mode skips the probe entirely', dict(
|
||||
server_mode=True, expect_thread_started=False)),
|
||||
('desktop mode backgrounds the probe', dict(
|
||||
server_mode=False, expect_thread_started=True)),
|
||||
]
|
||||
|
||||
def runTest(self):
|
||||
config = {'SERVER_MODE': self.server_mode}
|
||||
|
||||
with patch.object(keyring_probe, 'threading') as mock_threading:
|
||||
mock_thread = MagicMock()
|
||||
mock_threading.Thread.return_value = mock_thread
|
||||
|
||||
keyring_probe.start_async_probe(config)
|
||||
|
||||
if self.expect_thread_started:
|
||||
mock_threading.Thread.assert_called_once()
|
||||
_, kwargs = mock_threading.Thread.call_args
|
||||
self.assertEqual(kwargs.get('target'),
|
||||
keyring_probe._run_probe)
|
||||
self.assertEqual(kwargs.get('args'), (config,))
|
||||
self.assertTrue(kwargs.get('daemon'))
|
||||
mock_thread.start.assert_called_once()
|
||||
else:
|
||||
mock_threading.Thread.assert_not_called()
|
||||
|
||||
|
||||
class TestProbeScriptRunsForReal(BaseTestGenerator):
|
||||
"""Runs the real _PROBE_SCRIPT in a real subprocess - the only test
|
||||
here that isn't mocked - against a fake `keyring` module so the
|
||||
script's actual body (never executed by the mocked tests above) is
|
||||
verified for real, without depending on this machine's OS keyring
|
||||
backend."""
|
||||
|
||||
scenarios = [
|
||||
('healthy backend', dict(
|
||||
fake_keyring="""
|
||||
class _KR:
|
||||
name = 'FakeKeyring'
|
||||
|
||||
|
||||
def get_keyring():
|
||||
return _KR()
|
||||
|
||||
|
||||
def get_password(service, key):
|
||||
return None
|
||||
""",
|
||||
expect_stdout='1\nFakeKeyring\n')),
|
||||
('fail Keyring name disables storage', dict(
|
||||
fake_keyring="""
|
||||
class _KR:
|
||||
name = 'fail Keyring'
|
||||
|
||||
|
||||
def get_keyring():
|
||||
return _KR()
|
||||
|
||||
|
||||
def get_password(service, key):
|
||||
return None
|
||||
""",
|
||||
expect_stdout='0\n')),
|
||||
('get_password raising disables storage', dict(
|
||||
fake_keyring="""
|
||||
class _KR:
|
||||
name = 'FakeKeyring'
|
||||
|
||||
|
||||
def get_keyring():
|
||||
return _KR()
|
||||
|
||||
|
||||
def get_password(service, key):
|
||||
raise RuntimeError('backend unusable')
|
||||
""",
|
||||
expect_stdout='0\n')),
|
||||
('get_keyring raising disables storage', dict(
|
||||
fake_keyring="""
|
||||
def get_keyring():
|
||||
raise RuntimeError('no backend available')
|
||||
|
||||
|
||||
def get_password(service, key):
|
||||
return None
|
||||
""",
|
||||
expect_stdout='0\n')),
|
||||
]
|
||||
|
||||
def runTest(self):
|
||||
with tempfile.TemporaryDirectory() as fake_site:
|
||||
with open(os.path.join(fake_site, 'keyring.py'), 'w') as f:
|
||||
f.write(self.fake_keyring)
|
||||
|
||||
env = dict(os.environ)
|
||||
env['PYTHONPATH'] = fake_site + os.pathsep + \
|
||||
env.get('PYTHONPATH', '')
|
||||
|
||||
proc = keyring_probe.subprocess.Popen(
|
||||
[sys.executable, '-c', keyring_probe._PROBE_SCRIPT],
|
||||
stdout=keyring_probe.subprocess.PIPE,
|
||||
stderr=keyring_probe.subprocess.PIPE, text=True, env=env)
|
||||
stdout, stderr = proc.communicate(
|
||||
timeout=keyring_probe.KEYRING_PROBE_TIMEOUT)
|
||||
|
||||
self.assertEqual(stdout, self.expect_stdout, stderr)
|
||||
Reference in New Issue
Block a user