From 5472c750cb78242e97d50d32dbb6a6c2c297ae2a Mon Sep 17 00:00:00 2001 From: Zuriel Olu-Silas <163940298+ZCute1@users.noreply.github.com> Date: Thu, 2 Jul 2026 23:28:07 -0400 Subject: [PATCH 1/2] Fix roles list-keys when no role is provided and add CLI coverage --- taf/api/roles.py | 58 ++++++++++++++----- taf/tests/test_api/roles/api/test_roles.py | 43 ++++++++++++-- .../test_api/roles/cli/test_roles_cmds.py | 32 +++++++++- taf/tools/roles/__init__.py | 5 +- 4 files changed, 114 insertions(+), 24 deletions(-) diff --git a/taf/api/roles.py b/taf/api/roles.py index b719abdf4..0040c577e 100644 --- a/taf/api/roles.py +++ b/taf/api/roles.py @@ -917,38 +917,68 @@ def _get_roles_key_size(role: str, keystore: str, keys_num: int) -> int: return get_key_size(key_path) +def _format_role_keys_block(role_label: str, key_ids: List[str], certs_dir: str) -> str: + key_lines = [ + str(get_metadata_key_info(certs_dir, key_id)) for key_id in key_ids + ] + return f"Role: {role_label}\n" + "\n".join(key_lines) + + +def _roles_for_keys(auth_repo: AuthenticationRepository) -> Dict[str, set]: + roles_for_key: Dict[str, set] = defaultdict(set) + for role_name in auth_repo.get_all_roles(): + role_key_ids = auth_repo.get_role_keys(role=role_name) + if role_key_ids: + for key_id in role_key_ids: + roles_for_key[key_id].add(role_name) + return roles_for_key + + @log_on_error( ERROR, - "Could not list keys of {role:s}: {e}", + "Could not list keys: {e}", logger=taf_logger, on_exceptions=TAFError, reraise=True, ) def list_keys_of_role( path: str, - role: str, + role: Optional[str] = None, ) -> List[str]: """ - Print information about signing keys of role. If a certificate whose name matches - a key's id exists, include information contained by that certificate (like name and valid to/from dates) + Return formatted information about signing keys. If a certificate whose name matches + a key's id exists, include information contained by that certificate (like name and + valid to/from dates). - Arguments: - path: Path to the authentication repository. - role: Name of the role which is to be removed. + Arguments: + path: Path to the authentication repository. + role (optional): Name of the role whose keys should be listed. If omitted, each + unique key is listed once with all roles that reference it. Side Effects: - None + None - Returns: - None + Returns: + A list of formatted key blocks, each starting with a Role line. """ auth_repo = AuthenticationRepository(path=path) - key_ids = auth_repo.get_role_keys(role=role) - if key_ids is None: - raise TAFError(f"Role {role} does not exist") + if role is not None: + key_ids = auth_repo.get_role_keys(role=role) + if key_ids is None: + raise TAFError(f"Role {role} does not exist") + if not key_ids: + return [] + return [_format_role_keys_block(role, key_ids, auth_repo.certs_dir)] + + roles_for_key = _roles_for_keys(auth_repo) return [ - str(get_metadata_key_info(auth_repo.certs_dir, key_id)) for key_id in key_ids + _format_role_keys_block( + ", ".join(sorted(roles_for_key[key_id])), + [key_id], + auth_repo.certs_dir, + ) + for key_id in sorted(roles_for_key.keys()) ] diff --git a/taf/tests/test_api/roles/api/test_roles.py b/taf/tests/test_api/roles/api/test_roles.py index 6adb539e6..773bb8c0b 100644 --- a/taf/tests/test_api/roles/api/test_roles.py +++ b/taf/tests/test_api/roles/api/test_roles.py @@ -243,13 +243,35 @@ def test_remove_role_paths( def test_list_keys(auth_repo: AuthenticationRepository): root_keys_infos = list_keys_of_role(str(auth_repo.path), "root") - assert len(root_keys_infos) == 3 + assert len(root_keys_infos) == 1 + assert root_keys_infos[0].startswith("Role: root\n") + assert root_keys_infos[0].count("Key ID:") == 3 + targets_keys_infos = list_keys_of_role(str(auth_repo.path), "targets") - assert len(targets_keys_infos) == 2 + assert len(targets_keys_infos) == 1 + assert targets_keys_infos[0].startswith("Role: targets\n") + assert targets_keys_infos[0].count("Key ID:") == 2 + snapshot_keys_infos = list_keys_of_role(str(auth_repo.path), "snapshot") assert len(snapshot_keys_infos) == 1 + assert snapshot_keys_infos[0].startswith("Role: snapshot\n") + assert snapshot_keys_infos[0].count("Key ID:") == 1 + timestamp_keys_infos = list_keys_of_role(str(auth_repo.path), "timestamp") assert len(timestamp_keys_infos) == 1 + assert timestamp_keys_infos[0].startswith("Role: timestamp\n") + assert timestamp_keys_infos[0].count("Key ID:") == 1 + + +def test_list_all_keys(auth_repo: AuthenticationRepository): + all_keys_infos = list_keys_of_role(str(auth_repo.path)) + unique_keys = set() + for role_name in auth_repo.get_all_roles(): + for key_id in auth_repo.get_role_keys(role=role_name) or []: + unique_keys.add(key_id) + assert len(all_keys_infos) == len(unique_keys) + assert all(info.startswith("Role:") for info in all_keys_infos) + assert all("Key ID:" in info for info in all_keys_infos) def test_add_signing_key( @@ -272,10 +294,20 @@ def test_add_signing_key( commits = auth_repo.list_pygit_commits() assert len(commits) == initial_commits_num + 1 assert commits[0].message.strip() == COMMIT_MSG + shared_key_info = next( + info + for info in list_keys_of_role(str(auth_repo.path)) + if "Role: snapshot, timestamp" in info or "Role: timestamp, snapshot" in info + ) + assert shared_key_info.count("Key ID:") == 1 timestamp_keys_infos = list_keys_of_role(str(auth_repo.path), "timestamp") - assert len(timestamp_keys_infos) == 2 + assert len(timestamp_keys_infos) == 1 + assert timestamp_keys_infos[0].startswith("Role: timestamp\n") + assert timestamp_keys_infos[0].count("Key ID:") == 2 snapshot_keys_infos = list_keys_of_role(str(auth_repo.path), "snapshot") - assert len(snapshot_keys_infos) == 2 + assert len(snapshot_keys_infos) == 1 + assert snapshot_keys_infos[0].startswith("Role: snapshot\n") + assert snapshot_keys_infos[0].count("Key ID:") == 2 def test_revoke_signing_key( @@ -286,7 +318,8 @@ def test_revoke_signing_key( key_to_remove = targest_keyids[-1] initial_commits_num = len(auth_repo.list_pygit_commits()) targets_keys_infos = list_keys_of_role(str(auth_repo.path), "targets") - assert len(targets_keys_infos) == 2 + assert len(targets_keys_infos) == 1 + assert targets_keys_infos[0].count("Key ID:") == 2 COMMIT_MSG = "Revoke a targets key" revoke_signing_key( path=str(auth_repo.path), diff --git a/taf/tests/test_api/roles/cli/test_roles_cmds.py b/taf/tests/test_api/roles/cli/test_roles_cmds.py index d8e22d7fa..3d5e10618 100644 --- a/taf/tests/test_api/roles/cli/test_roles_cmds.py +++ b/taf/tests/test_api/roles/cli/test_roles_cmds.py @@ -90,16 +90,42 @@ def test_roles_add_signing_key_cmd_expect_success(auth_repo, roles_keystore): ], ) timestamp_keys_infos = list_keys_of_role(str(auth_repo.path), "timestamp") - assert len(timestamp_keys_infos) == 2 + # assert len(timestamp_keys_infos) == 2 + assert len(timestamp_keys_infos) == 1 + assert timestamp_keys_infos[0].count("Key ID:") == 2 snapshot_keys_infos = list_keys_of_role(str(auth_repo.path), "snapshot") - assert len(snapshot_keys_infos) == 2 + # assert len(snapshot_keys_infos) == 2 + assert len(snapshot_keys_infos) == 1 + assert snapshot_keys_infos[0].count("Key ID:") == 2 + + +def test_roles_list_keys_no_role_arg_expect_success(auth_repo, roles_keystore): + runner = CliRunner() + + with runner.isolated_filesystem(): + result = runner.invoke( + taf, + [ + "roles", + "list-keys", + "--path", + f"{str(auth_repo.path)}", + ], + ) + + assert result.exit_code == 0 + assert "Role:" in result.output + assert "Key ID:" in result.output + assert result.output.strip().startswith("Role:") def test_revoke_key_cmd_expect_success(auth_repo, roles_keystore): runner = CliRunner() targets_keys_infos = list_keys_of_role(str(auth_repo.path), "targets") - assert len(targets_keys_infos) == 2 + # assert len(targets_keys_infos) == 2 + assert len(targets_keys_infos) == 1 + assert targets_keys_infos[0].count("Key ID:") == 2 with runner.isolated_filesystem(): targest_keyids = auth_repo.get_keyids_of_role("targets") diff --git a/taf/tools/roles/__init__.py b/taf/tools/roles/__init__.py index 375a7af69..eeeee9150 100644 --- a/taf/tools/roles/__init__.py +++ b/taf/tools/roles/__init__.py @@ -494,15 +494,16 @@ def rotate_key( return rotate_key - +# Zuriel: made the Role optional def list_keys_command(): @click.command(help=""" List all keys of the specified role. If certs directory exists and contains certificates exported from YubiKeys, include additional information read from these certificates, like name or organization. + If role isn't specified, list all keys with their roles. """) @find_repository @catch_cli_exception(handle=TAFError) - @click.argument("role") + @click.argument("role", required=False, default=None) @click.option( "--path", default=".", From 6d23749eecdcf7d1e0c9bf1b64e5c54af449480f Mon Sep 17 00:00:00 2001 From: Zuriel Olu-Silas <163940298+ZCute1@users.noreply.github.com> Date: Thu, 2 Jul 2026 23:55:27 -0400 Subject: [PATCH 2/2] Fix roles list-keys when no role is provided and add CLI coverage --- taf/tools/roles/__init__.py | 1 - 1 file changed, 1 deletion(-) diff --git a/taf/tools/roles/__init__.py b/taf/tools/roles/__init__.py index eeeee9150..1988240a5 100644 --- a/taf/tools/roles/__init__.py +++ b/taf/tools/roles/__init__.py @@ -494,7 +494,6 @@ def rotate_key( return rotate_key -# Zuriel: made the Role optional def list_keys_command(): @click.command(help=""" List all keys of the specified role. If certs directory exists and contains certificates exported from YubiKeys,