From 306269af5de842d2674de72348f9af745bbe3ed5 Mon Sep 17 00:00:00 2001 From: Yogesh Mahajan Date: Mon, 2 Sep 2024 19:32:25 +0530 Subject: [PATCH] Fix issues found while testing keyring changes. #7076 --- web/config.py | 9 +++ web/pgadmin/browser/__init__.py | 64 ++++++++++--------- .../browser/server_groups/servers/utils.py | 7 +- web/pgadmin/utils/master_password.py | 55 +++++----------- 4 files changed, 64 insertions(+), 71 deletions(-) diff --git a/web/config.py b/web/config.py index b4c25c079..53145d159 100644 --- a/web/config.py +++ b/web/config.py @@ -583,6 +583,15 @@ ALLOW_SAVE_TUNNEL_PASSWORD = False # Applicable for desktop mode only ########################################################################## MASTER_PASSWORD_REQUIRED = True +########################################################################## + +########################################################################## +# Allow to save master password which is used to encrypt/decrypt saved +# passwords in the os level secret like Keychain, password store etc. +# Disabling this will require user to enter master password +# if MASTER_PASSWORD_REQUIRED is set to True. Note: this is applicable only +# in case of Desktop mode. +########################################################################## USE_OS_SECRET_STORAGE = True ########################################################################## diff --git a/web/pgadmin/browser/__init__.py b/web/pgadmin/browser/__init__.py index 4cd8d1271..33f9478bc 100644 --- a/web/pgadmin/browser/__init__.py +++ b/web/pgadmin/browser/__init__.py @@ -19,7 +19,7 @@ from smtplib import SMTPConnectError, SMTPResponseException, \ from socket import error as SOCKETErrorException import keyring -from keyring.errors import KeyringLocked +from keyring.errors import KeyringLocked, NoKeyringError from pgadmin.utils.constants import KEY_RING_SERVICE_NAME, \ KEY_RING_USER_NAME,MessageType @@ -632,7 +632,7 @@ def get_nodes(): def form_master_password_response(existing=True, present=False, errmsg=None, - keyring_name='', master_password_hook=''): + keyring_name='', master_password_hook=False): return make_json_response(data={ 'present': present, 'reset': existing, @@ -680,7 +680,7 @@ def reset_master_password(): # Set masterpass_check if MASTER_PASSWORD_HOOK is set which provides # encryption key - if config.MASTER_PASSWORD_REQUIRED and config.MASTER_PASSWORD_HOOK: + if config.SERVER_MODE and config.MASTER_PASSWORD_HOOK: set_masterpass_check_text(crypt_key) return make_json_response(data=status) @@ -705,6 +705,7 @@ def set_master_password(): keyring_name = '' errmsg = '' + master_password_hook = False if not config.SERVER_MODE: if config.USE_OS_SECRET_STORAGE: try: @@ -715,7 +716,8 @@ def set_master_password(): if not master_key: # Generate new one and migration required master_key = secrets.token_urlsafe(12) - + keyring.delete_password(KEY_RING_SERVICE_NAME, + 'entry_to_check_keychain_access') # migrate existing server passwords from pgadmin.browser.server_groups.servers.utils \ import migrate_saved_passwords @@ -751,40 +753,39 @@ def set_master_password(): errmsg=errmsg, keyring_name=keyring_name) else: - current_app.logger.warning( - ' Master key was already present in the keyring,' - ' hence not doing any migration') + current_app.logger.debug( + 'Master key was already present in the keyring,' + 'hence not doing any migration') # Key is already generated and set, no migration required # set crypt key set_crypt_key(master_key) return form_master_password_response( present=True) - except KeyringLocked as e: - current_app.logger.warning( - 'Failed to set because Access Denied.' - ' Error: {0}'.format(e)) - config.USE_OS_SECRET_STORAGE = False except Exception as e: - current_app.logger.warning( - 'Failed to set encryption key using OS password manager' - ', fallback to master password. Error: {0}'.format(e)) - # Also if masterpass_check is none it means previously - # passwords were migrated using keyring crypt key. - # Reset all passwords because we are going to master password - # again and while setting master password, all server - # passwords are decrypted using old key before re-encryption - if current_user.masterpass_check is None: + error = 'Failed to get/set encryption key using OS password ' \ + 'manager because of exception.' \ + ' Error: {0}'.format(e) + current_app.logger.warning(error) + # Disable local os storage if any exception other than + # access denied + if not isinstance(e, KeyringLocked): + config.USE_OS_SECRET_STORAGE = False + # delete key if exception other than no keyring backend + # error + if not isinstance(e, NoKeyringError): + delete_local_storage_master_key() + + # Delete saved password encrypted with kecyhain master key from pgadmin.browser.server_groups.servers.utils \ import remove_saved_passwords, update_session_manager remove_saved_passwords(current_user.id) update_session_manager(current_user.id) - # Disable local os storage if any exception while creation - config.USE_OS_SECRET_STORAGE = False - delete_local_storage_master_key() - else: - # if os secret storage disabled now, but was used once then - # remove all the saved passwords - delete_local_storage_master_key() + + return form_master_password_response( + existing=False, + present=True, + errmsg=errmsg, + keyring_name=keyring_name) else: # If the master password is required and the master password hook # is specified then try to retrieve the encryption key and update data. @@ -817,11 +818,16 @@ def set_master_password(): if current_user.masterpass_check is not None and \ data.get('submit_password', False) and \ not validate_master_password(data.get('password')): + + if config.SERVER_MODE and config.MASTER_PASSWORD_HOOK: + master_password_hook = True + else: + errmsg = gettext("Incorrect master password") return form_master_password_response( existing=True, present=False, errmsg=errmsg, - master_password_hook=config.MASTER_PASSWORD_HOOK, + master_password_hook=master_password_hook, keyring_name=keyring_name ) diff --git a/web/pgadmin/browser/server_groups/servers/utils.py b/web/pgadmin/browser/server_groups/servers/utils.py index 6511ea286..44ae1b47d 100644 --- a/web/pgadmin/browser/server_groups/servers/utils.py +++ b/web/pgadmin/browser/server_groups/servers/utils.py @@ -355,9 +355,9 @@ def migrate_saved_passwords(master_key, master_password): elif master_password: old_key = master_password else: - current_app.logger.warning( - 'Saved password were already migrated once. ' - 'Hence not migrating again. ' + current_app.logger.info( + 'Passwords saved with Master password were already' + ' migrated once. Hence not migrating again. ' 'May be the old master key was deleted.') else: old_key = current_user.password @@ -432,7 +432,6 @@ def remove_saved_passwords(user_id): """ This function will remove all the saved passwords for the server """ - try: db.session.query(Server) \ .filter(Server.user_id == user_id) \ diff --git a/web/pgadmin/utils/master_password.py b/web/pgadmin/utils/master_password.py index 74685d0d3..b25734cd3 100644 --- a/web/pgadmin/utils/master_password.py +++ b/web/pgadmin/utils/master_password.py @@ -44,21 +44,15 @@ def get_crypt_key(): def get_master_password_key_from_os_secret(): - master_key = None - try: - # Try to get master key is from local os storage - master_key = keyring.get_password( - KEY_RING_SERVICE_NAME, KEY_RING_USER_NAME) - except KeyringLocked as e: - current_app.logger.warning( - 'Failed to retrieve master key because Access Denied.' - ' Error: {0}'.format(e)) - config.USE_OS_SECRET_STORAGE = False - except Exception as e: - current_app.logger.warning( - 'Failed to set encryption key using OS password manager' - ', fallback to master password. Error: {0}'.format(e)) - config.USE_OS_SECRET_STORAGE = False + # Try to get master key is from local os storage + master_key = keyring.get_password(KEY_RING_SERVICE_NAME, + KEY_RING_USER_NAME) + if not master_key: + # If master password does not exist, keychain does not ask for + # permission. This will forces to ask for permission + keyring.set_password(KEY_RING_SERVICE_NAME, + 'entry_to_check_keychain_access', + 'dummy_password') return master_key @@ -147,32 +141,17 @@ def delete_local_storage_master_key(): if master_key: keyring.delete_password(KEY_RING_SERVICE_NAME, KEY_RING_USER_NAME) - from pgadmin.browser.server_groups.servers.utils \ - import remove_saved_passwords - remove_saved_passwords(current_user.id) - - from pgadmin.utils.driver import get_driver - driver = get_driver(config.PG_DEFAULT_DRIVER) - for server in Server.query.filter_by( - user_id=current_user.id).all(): - manager = driver.connection_manager(server.id) - manager.update(server) current_app.logger.warning( 'Deleted master key stored in OS password manager.') - except NoKeyringError as e: - current_app.logger.warning( - ' Failed to delete master key stored in OS password manager' - ' because Keyring backend not found. Error: {0}'.format(e)) - config.USE_OS_SECRET_STORAGE = False - except KeyringLocked as e: - current_app.logger.warning( - ' Failed to delete master key stored in OS password manager' - ' because of Access Denied. Error: {0}'.format(e)) - config.USE_OS_SECRET_STORAGE = False + except (NoKeyringError, KeyringLocked) as e: + error = 'Failed to delete master key stored in OS password ' \ + 'manager because Keyring backend not found or ' \ + 'access denied. Error: {0}'.format(e) + current_app.logger.warning(error) except Exception as e: - current_app.logger.warning( - 'Failed to delete master key stored in OS password manager.') - config.USE_OS_SECRET_STORAGE = False + error = 'Failed to delete master key stored in OS password ' \ + 'manager. Error: {0}'.format(e) + current_app.logger.warning(error) def process_masterpass_disabled():