Skip to content

feat: automatically find existing IdentityFile #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
FanBB2333 wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

feat: automatically find existing IdentityFile #39

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
098d5cb
feat: automatically find existing `IdentityFile`
FanBB2333 Nov 23, 2024
3f933cb
refactor: use click.echo instead of print for consistency
FanBB2333 Dec 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion ssh_config/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,22 @@ def posix_shell(chan):
finally:
termios.tcsetattr(sys.stdin, termios.TCSADRAIN, oldtty)

def get_identity_files(path: str = "~/.ssh"):
fullpath = os.path.expanduser(path)
keys = []
for file in os.listdir(fullpath):
# if not ends with .pub and starts with id_
if file.startswith("id_") and not file.endswith(".pub") and os.path.isfile(os.path.join(fullpath, file)):
keys.append(os.path.join(path, file))
Comment on lines +74 to +77
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function does not handle potential permission errors when accessing the .ssh directory. If the user lacks read permissions for the directory, os.listdir() will raise a PermissionError. Consider adding error handling to gracefully fall back to the default identity file in such cases.

Suggested change
for file in os.listdir(fullpath):
# if not ends with .pub and starts with id_
if file.startswith("id_") and not file.endswith(".pub") and os.path.isfile(os.path.join(fullpath, file)):
keys.append(os.path.join(path, file))
try:
for file in os.listdir(fullpath):
# if not ends with .pub and starts with id_
if file.startswith("id_") and not file.endswith(".pub") and os.path.isfile(os.path.join(fullpath, file)):
keys.append(os.path.join(path, file))
except (PermissionError, FileNotFoundError):
# Cannot access the directory or it does not exist; fall back to default key
click.echo(f"[Warning] unable to access {fullpath}, using default identity file")
return os.path.join(path, "id_rsa")

Copilot uses AI. Check for mistakes.
Comment on lines +74 to +77
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function does not handle the case where the .ssh directory does not exist. This will cause an unhandled FileNotFoundError when os.listdir(fullpath) is called. Consider wrapping the directory listing in a try-except block or checking if the directory exists before attempting to list its contents.

Copilot uses AI. Check for mistakes.
if len(keys) == 0:
# keys not found, return default key
click.echo(f"[Warning] identity file not found, using default")
return os.path.join(path, "id_rsa")
Comment on lines +77 to +81
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an inconsistency in path handling. On line 72, fullpath is created using os.path.expanduser(path), but on line 77, keys are appended using the unexpanded path variable instead of fullpath. This means that if the function is called with the default "/.ssh", the returned paths will contain the literal "" instead of the expanded home directory path. Either use fullpath throughout, or ensure the returned paths use the same format as the input.

Suggested change
keys.append(os.path.join(path, file))
if len(keys) == 0:
# keys not found, return default key
click.echo(f"[Warning] identity file not found, using default")
return os.path.join(path, "id_rsa")
keys.append(os.path.join(fullpath, file))
if len(keys) == 0:
# keys not found, return default key
click.echo(f"[Warning] identity file not found, using default")
return os.path.join(fullpath, "id_rsa")

Copilot uses AI. Check for mistakes.
elif len(keys) == 1:
return keys[0]
else:
print(f"[Warning] multiple identity files found: {keys}")
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function uses click.echo for the warning on line 80 but uses print for the warning on line 85. This is inconsistent. Since this is a Click-based CLI application, click.echo should be used consistently for all output.

Suggested change
print(f"[Warning] multiple identity files found: {keys}")
click.echo(f"[Warning] multiple identity files found: {keys}")

Copilot uses AI. Check for mistakes.
return keys[0]

@click.group()
@click.option(
Expand Down Expand Up @@ -215,7 +231,7 @@ def add_config(ctx, name: str, attributes: list[str]):
type=int, default=22, show_default=True)
if 'IdentityFile' not in attrs:
attrs['IdentityFile'] = click.prompt("IdentityFile",
type=str, default="~/.ssh/id_rsa",
type=str, default=get_identity_files(),
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new get_identity_files() function is called during the add command flow, but there are no tests covering this new behavior. Since the repository has comprehensive test coverage for CLI commands (as seen in test_cli.py), tests should be added to verify the identity file discovery functionality, including edge cases like missing .ssh directory, multiple identity files, and no identity files found.

Copilot uses AI. Check for mistakes.
show_default=True)
if not attributes:
while click.confirm("Do you have additonal attribute?"):
Expand Down