From c1a9d6c031c206cb5b9792a30fd737f0327560dd Mon Sep 17 00:00:00 2001 From: dustinface <35775977+xdustinface@users.noreply.github.com> Date: Thu, 11 Aug 2022 22:40:46 +0200 Subject: [PATCH] cmds|daemon: Improve legacy keyring migration enforcement (#12911) * Use the daemon for partial migration checks if its running * Make `migrate_legacy_keyring_interactive` async and drop redundant calls So that we can await it since ``` asyncio.run(async_update_daemon_migration_completed_if_running()) ``` fails because we now here already have an event loop running. * Drop redundant `unlocks_keyring` decoration * Update some comments * Move the `asyncio` import to make LGTM happy --- chia/cmds/keys.py | 11 ++++--- chia/cmds/keys_funcs.py | 57 ++++++++++++++++++++++++++++-------- chia/cmds/passphrase.py | 4 +-- chia/cmds/start.py | 6 +--- chia/cmds/start_funcs.py | 10 ++++++- chia/cmds/wallet.py | 5 ++-- chia/daemon/server.py | 1 - chia/util/keychain.py | 32 -------------------- chia/util/keyring_wrapper.py | 30 +++++-------------- tests/core/cmds/test_keys.py | 14 ++++----- 10 files changed, 81 insertions(+), 89 deletions(-) diff --git a/chia/cmds/keys.py b/chia/cmds/keys.py index eaad0d121534..9ae1c6cedb8d 100644 --- a/chia/cmds/keys.py +++ b/chia/cmds/keys.py @@ -1,4 +1,7 @@ +import asyncio + import click +import sys from typing import Optional, Tuple @@ -10,13 +13,13 @@ def keys_cmd(ctx: click.Context): from pathlib import Path from .keys_funcs import migrate_keys - if ctx.obj["force_legacy_keyring_migration"]: - migrate_keys(True) - root_path: Path = ctx.obj["root_path"] if not root_path.is_dir(): raise RuntimeError("Please initialize (or migrate) your config directory with chia init") + if ctx.obj["force_legacy_keyring_migration"] and not asyncio.run(migrate_keys(root_path, True)): + sys.exit(1) + @keys_cmd.command("generate", short_help="Generates and adds a key to keychain") @click.pass_context @@ -156,7 +159,7 @@ def verify_cmd(message: str, public_key: str, signature: str): def migrate_cmd(ctx: click.Context): from .keys_funcs import migrate_keys - migrate_keys() + asyncio.run(migrate_keys(ctx.obj["root_path"])) @keys_cmd.group("derive", short_help="Derive child keys or wallet addresses") diff --git a/chia/cmds/keys_funcs.py b/chia/cmds/keys_funcs.py index 162311740ba0..9f36a9d21d71 100644 --- a/chia/cmds/keys_funcs.py +++ b/chia/cmds/keys_funcs.py @@ -1,3 +1,4 @@ +import logging import os import sys @@ -7,7 +8,10 @@ from pathlib import Path from typing import Any, Dict, List, Optional, Tuple, Union from chia.consensus.coinbase import create_puzzlehash_for_pk +from chia.daemon.client import connect_to_daemon_and_validate +from chia.daemon.keychain_proxy import KeychainProxy, connect_to_keychain_and_validate, wrap_local_keychain from chia.util.bech32m import encode_puzzle_hash +from chia.util.errors import KeychainNotSet from chia.util.config import load_config from chia.util.default_root import DEFAULT_ROOT_PATH from chia.util.ints import uint32 @@ -183,22 +187,44 @@ def verify(message: str, public_key: str, signature: str): print(AugSchemeMPL.verify(public_key, messageBytes, signature)) -def migrate_keys(forced: bool = False): +async def migrate_keys(root_path: Path, forced: bool = False) -> bool: from chia.util.keyring_wrapper import KeyringWrapper from chia.util.misc import prompt_yes_no deprecation_message = ( "\nLegacy keyring support is deprecated and will be removed in version 1.5.2. " - "You need to migrate your keyring to continue using Chia." + "You need to migrate your keyring to continue using Chia.\n" ) # Check if the keyring needs a full migration (i.e. if it's using the old keyring) if Keychain.needs_migration(): print(deprecation_message) - KeyringWrapper.get_shared_instance().migrate_legacy_keyring_interactive() + await KeyringWrapper.get_shared_instance().migrate_legacy_keyring_interactive() else: - keys_to_migrate, legacy_keyring = Keychain.get_keys_needing_migration() - if len(keys_to_migrate) > 0 and legacy_keyring is not None: + log = logging.getLogger("migrate_keys") + config = load_config(root_path, "config.yaml") + # Connect to the daemon here first to see if ts running since `connect_to_keychain_and_validate` just tries to + # connect forever if it's not up. + keychain_proxy: Optional[KeychainProxy] = None + daemon = await connect_to_daemon_and_validate(root_path, config, quiet=True) + if daemon is not None: + await daemon.close() + keychain_proxy = await connect_to_keychain_and_validate(root_path, log) + if keychain_proxy is None: + keychain_proxy = wrap_local_keychain(Keychain(), log=log) + + try: + legacy_keyring = Keychain(force_legacy=True) + all_sks = await keychain_proxy.get_all_private_keys() + all_legacy_sks = legacy_keyring.get_all_private_keys() + set_legacy_sks = {str(x[0]) for x in all_legacy_sks} + set_sks = {str(x[0]) for x in all_sks} + missing_legacy_keys = set_legacy_sks - set_sks + keys_to_migrate = [x for x in all_legacy_sks if str(x[0]) in missing_legacy_keys] + except KeychainNotSet: + keys_to_migrate = [] + + if len(keys_to_migrate) > 0: print(deprecation_message) print(f"Found {len(keys_to_migrate)} key(s) that need migration:") for key, _ in keys_to_migrate: @@ -206,32 +232,39 @@ def migrate_keys(forced: bool = False): print() if not prompt_yes_no("Migrate these keys?"): - sys.exit("Migration aborted, can't run any chia commands.") + await keychain_proxy.close() + print("Migration aborted, can't run any chia commands.") + return False - keychain = Keychain() for sk, seed_bytes in keys_to_migrate: mnemonic = bytes_to_mnemonic(seed_bytes) - keychain.add_private_key(mnemonic, "") + await keychain_proxy.add_private_key(mnemonic, "") fingerprint = sk.get_g1().get_fingerprint() print(f"Added private key with public key fingerprint {fingerprint}") print(f"Migrated {len(keys_to_migrate)} key(s)") print("Verifying migration results...", end="") - if Keychain.verify_keys_present(keys_to_migrate): + all_sks = await keychain_proxy.get_all_private_keys() + await keychain_proxy.close() + set_sks = {str(x[0]) for x in all_sks} + keys_present = set_sks.issuperset(set(map(lambda x: str(x[0]), keys_to_migrate))) + if keys_present: print(" Verified") print() - response = prompt_yes_no("Remove key(s) from old keyring?") + response = prompt_yes_no("Remove key(s) from old keyring (recommended)?") if response: legacy_keyring.delete_keys(keys_to_migrate) print(f"Removed {len(keys_to_migrate)} key(s) from old keyring") print("Migration complete") else: print(" Failed") - sys.exit(1) - sys.exit(0) + return False + return True elif not forced: print("No keys need migration") + await keychain_proxy.close() + return True def _clear_line_part(n: int): diff --git a/chia/cmds/passphrase.py b/chia/cmds/passphrase.py index 71d42dade9c0..0f6934a831d5 100644 --- a/chia/cmds/passphrase.py +++ b/chia/cmds/passphrase.py @@ -12,8 +12,8 @@ from chia.util.config import load_config def passphrase_cmd(ctx: click.Context): from .keys_funcs import migrate_keys - if ctx.obj["force_legacy_keyring_migration"]: - migrate_keys(True) + if ctx.obj["force_legacy_keyring_migration"] and not asyncio.run(migrate_keys(ctx.obj["root_path"], True)): + sys.exit(1) @passphrase_cmd.command( diff --git a/chia/cmds/start.py b/chia/cmds/start.py index 97ac30fed52e..7397c6b6d920 100644 --- a/chia/cmds/start.py +++ b/chia/cmds/start.py @@ -11,11 +11,7 @@ from chia.util.service_groups import all_groups def start_cmd(ctx: click.Context, restart: bool, group: str) -> None: import asyncio from .start_funcs import async_start - from .keys_funcs import migrate_keys - - if ctx.obj["force_legacy_keyring_migration"]: - migrate_keys(True) root_path = ctx.obj["root_path"] config = load_config(root_path, "config.yaml") - asyncio.run(async_start(root_path, config, group, restart)) + asyncio.run(async_start(root_path, config, group, restart, ctx.obj["force_legacy_keyring_migration"])) diff --git a/chia/cmds/start_funcs.py b/chia/cmds/start_funcs.py index 9990d7a4a82d..d4dacc7e516c 100644 --- a/chia/cmds/start_funcs.py +++ b/chia/cmds/start_funcs.py @@ -7,6 +7,7 @@ import sys from pathlib import Path from typing import Any, Dict, Optional +from chia.cmds.keys_funcs import migrate_keys from chia.cmds.passphrase_funcs import get_current_passphrase from chia.daemon.client import DaemonProxy, connect_to_daemon_and_validate from chia.util.errors import KeychainMaxUnlockAttempts @@ -50,7 +51,9 @@ async def create_start_daemon_connection(root_path: Path, config: Dict[str, Any] return None -async def async_start(root_path: Path, config: Dict[str, Any], group: str, restart: bool) -> None: +async def async_start( + root_path: Path, config: Dict[str, Any], group: str, restart: bool, force_keyring_migration: bool +) -> None: try: daemon = await create_start_daemon_connection(root_path, config) except KeychainMaxUnlockAttempts: @@ -61,6 +64,11 @@ async def async_start(root_path: Path, config: Dict[str, Any], group: str, resta print("Failed to create the chia daemon") return None + if force_keyring_migration: + if not await migrate_keys(root_path, True): + await daemon.close() + sys.exit(1) + for service in services_for_groups(group): if await daemon.is_running(service_name=service): print(f"{service}: ", end="", flush=True) diff --git a/chia/cmds/wallet.py b/chia/cmds/wallet.py index 9e1fe9cd5a0e..d57898f1d7f4 100644 --- a/chia/cmds/wallet.py +++ b/chia/cmds/wallet.py @@ -11,10 +11,11 @@ from chia.wallet.util.wallet_types import WalletType @click.group("wallet", short_help="Manage your wallet") @click.pass_context def wallet_cmd(ctx: click.Context) -> None: + import asyncio from .keys_funcs import migrate_keys - if ctx.obj["force_legacy_keyring_migration"]: - migrate_keys(True) + if ctx.obj["force_legacy_keyring_migration"] and not asyncio.run(migrate_keys(ctx.obj["root_path"], True)): + sys.exit(1) @wallet_cmd.command("get_transaction", short_help="Get a transaction") diff --git a/chia/daemon/server.py b/chia/daemon/server.py index c5b801b918d6..05992ed9a8f9 100644 --- a/chia/daemon/server.py +++ b/chia/daemon/server.py @@ -514,7 +514,6 @@ class WebSocketServer: Keychain.set_master_passphrase( current_passphrase, new_passphrase, - allow_migration=False, passphrase_hint=passphrase_hint, save_passphrase=save_passphrase, ) diff --git a/chia/util/keychain.py b/chia/util/keychain.py index a3730cc8c07c..ddb34852ba09 100644 --- a/chia/util/keychain.py +++ b/chia/util/keychain.py @@ -224,7 +224,6 @@ class Keychain: self.keyring_wrapper = keyring_wrapper - @unlocks_keyring(use_passphrase_cache=True) def _get_pk_and_entropy(self, user: str) -> Optional[Tuple[G1Element, bytes]]: """ Returns the keychain contents for a specific 'user' (key index). The contents @@ -491,41 +490,12 @@ class Keychain: current_passphrase=None, new_passphrase=passphrase, write_to_keyring=False, - allow_migration=False, passphrase_hint=passphrase_hint, save_passphrase=save_passphrase, ) KeyringWrapper.get_shared_instance().migrate_legacy_keyring(cleanup_legacy_keyring=cleanup_legacy_keyring) - @staticmethod - def get_keys_needing_migration() -> Tuple[List[Tuple[PrivateKey, bytes]], Optional["Keychain"]]: - try: - legacy_keyring: Keychain = Keychain(force_legacy=True) - except KeychainNotSet: - # No legacy keyring available, so no keys need to be migrated - return [], None - keychain = Keychain() - all_legacy_sks = legacy_keyring.get_all_private_keys() - all_sks = keychain.get_all_private_keys() - set_legacy_sks = {str(x[0]) for x in all_legacy_sks} - set_sks = {str(x[0]) for x in all_sks} - missing_legacy_keys = set_legacy_sks - set_sks - keys_needing_migration = [x for x in all_legacy_sks if str(x[0]) in missing_legacy_keys] - - return keys_needing_migration, legacy_keyring - - @staticmethod - def verify_keys_present(keys_to_verify: List[Tuple[PrivateKey, bytes]]) -> bool: - """ - Verifies that the given keys are present in the keychain. - """ - keychain = Keychain() - all_sks = keychain.get_all_private_keys() - set_sks = {str(x[0]) for x in all_sks} - keys_present = set_sks.issuperset(set(map(lambda x: str(x[0]), keys_to_verify))) - return keys_present - @staticmethod def passphrase_is_optional() -> bool: """ @@ -599,7 +569,6 @@ class Keychain: current_passphrase: Optional[str], new_passphrase: str, *, - allow_migration: bool = True, passphrase_hint: Optional[str] = None, save_passphrase: bool = False, ) -> None: @@ -610,7 +579,6 @@ class Keychain: KeyringWrapper.get_shared_instance().set_master_passphrase( current_passphrase, new_passphrase, - allow_migration=allow_migration, passphrase_hint=passphrase_hint, save_passphrase=save_passphrase, ) diff --git a/chia/util/keyring_wrapper.py b/chia/util/keyring_wrapper.py index e6a686056160..402bda04816a 100644 --- a/chia/util/keyring_wrapper.py +++ b/chia/util/keyring_wrapper.py @@ -1,5 +1,3 @@ -import asyncio - from blspy import PrivateKey # pyright: reportMissingImports=false from chia.util.default_root import DEFAULT_KEYS_ROOT_PATH from chia.util.file_keyring import FileKeyring @@ -237,7 +235,6 @@ class KeyringWrapper: new_passphrase: str, *, write_to_keyring: bool = True, - allow_migration: bool = True, passphrase_hint: Optional[str] = None, save_passphrase: bool = False, ) -> None: @@ -260,17 +257,12 @@ class KeyringWrapper: self.keyring.set_passphrase_hint(passphrase_hint) if write_to_keyring: - # We'll migrate the legacy contents to the new keyring at this point if self.using_legacy_keyring(): - if not allow_migration: - raise KeychainRequiresMigration() - - self.migrate_legacy_keyring_interactive() - else: - # We're reencrypting the keyring contents using the new passphrase. Ensure that the - # payload has been decrypted by calling load_keyring with the current passphrase. - self.keyring.load_keyring(passphrase=current_passphrase) - self.keyring.write_keyring(fresh_salt=True) # Create a new salt since we're changing the passphrase + raise KeychainRequiresMigration() + # We're reencrypting the keyring contents using the new passphrase. Ensure that the + # payload has been decrypted by calling load_keyring with the current passphrase. + self.keyring.load_keyring(passphrase=current_passphrase) + self.keyring.write_keyring(fresh_salt=True) # Create a new salt since we're changing the passphrase if supports_os_passphrase_storage(): if save_passphrase: @@ -489,7 +481,7 @@ class KeyringWrapper: if success and cleanup_legacy_keyring: self.cleanup_legacy_keyring(results) - def migrate_legacy_keyring_interactive(self): + async def migrate_legacy_keyring_interactive(self): """ Handle importing keys from the legacy keyring into the new keyring. @@ -523,7 +515,7 @@ class KeyringWrapper: print("Keys in old keyring left intact") # Notify the daemon (if running) that migration has completed - asyncio.run(async_update_daemon_migration_completed_if_running()) + await async_update_daemon_migration_completed_if_running() exit(0) # Keyring interface @@ -537,15 +529,7 @@ class KeyringWrapper: return self.get_keyring().get_password(service, user) def set_passphrase(self, service: str, user: str, passphrase: str): - # On the first write while using the legacy keyring, we'll start migration - if self.using_legacy_keyring() and self.has_cached_master_passphrase(): - self.migrate_legacy_keyring_interactive() - self.get_keyring().set_password(service, user, passphrase) def delete_passphrase(self, service: str, user: str): - # On the first write while using the legacy keyring, we'll start migration - if self.using_legacy_keyring() and self.has_cached_master_passphrase(): - self.migrate_legacy_keyring_interactive() - self.get_keyring().delete_password(service, user) diff --git a/tests/core/cmds/test_keys.py b/tests/core/cmds/test_keys.py index 5ad26a3faa7c..835c2e47244d 100644 --- a/tests/core/cmds/test_keys.py +++ b/tests/core/cmds/test_keys.py @@ -2,7 +2,6 @@ import os import pytest import re -from blspy import PrivateKey from chia.cmds.chia import cli from chia.cmds.keys import delete_all_cmd, generate_and_print_cmd, show_cmd, sign_cmd, verify_cmd from chia.util.config import load_config @@ -13,7 +12,7 @@ from click.testing import CliRunner, Result from keyring.backend import KeyringBackend from pathlib import Path from tests.util.keyring import TempKeyring -from typing import Dict, List, Optional, Tuple +from typing import Dict, Optional TEST_MNEMONIC_SEED = ( @@ -991,17 +990,18 @@ class TestKeysCommands: """ Test the `chia keys migrate` command when no migration is necessary """ + keys_root_path = KeyringWrapper.get_shared_instance().keys_root_path + runner = CliRunner() + init_result = runner.invoke( + cli, ["--root-path", os.fspath(tmp_path), "--keys-root-path", os.fspath(keys_root_path), "init"] + ) + assert init_result.exit_code == 0 def mock_keychain_needs_migration() -> bool: return False monkeypatch.setattr(Keychain, "needs_migration", mock_keychain_needs_migration) - def mock_keychain_get_keys_needing_migration() -> Tuple[List[Tuple[PrivateKey, bytes]], Optional[Keychain]]: - return [], None - - monkeypatch.setattr(Keychain, "get_keys_needing_migration", mock_keychain_get_keys_needing_migration) - runner = CliRunner() result: Result = runner.invoke( cli,