Skip to content
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.

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

feat: add ability to check for multiple default ssh key locations #406

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

a-dubs
Copy link
Collaborator

@a-dubs a-dubs commented Aug 26, 2024

Since Ubuntu 24.04 LTS and newer, the default location for ssh keys is
~/.ssh/id_ed25519 instead of ~/.ssh/id_rsa. Thus, when no ssh key
path is specified, this commit changes pycloudlib to check for an
existing ssh key at either ~/.ssh/id_ed25519 or ~/.ssh/id_rsa.

Fixes GH #341


Testing of changes

Testing id_ed25519 can be found

  1. made sure no key exists at ~/.ssh/id_rsa
  2. created new key at ~/.ssh/id_ed25519
  3. removed public_key_path and private_key_path from [oci] config in my pycloudlib.toml config file
  4. ran the oracle example script: python3 examples/oracle.py
  5. made sure the following line showed up in the log (was one of the first few lines logged):
DEBUG:pycloudlib.cloud.OCI:using SSH pubkey: /home/a-dubs/.ssh/id_ed25519.pub

Testing error is raised when neither default key is found

  1. Removed keys at ~/.ssh/id_ed25519
  2. ran the oracle example script: python3 examples/oracle.py
  3. Ensured error stating the following was thrown:
pycloudlib.errors.PycloudlibError: No public key path provided and no key found in default locations: '~/.ssh/id_rsa.pub' or '~/.ssh/id_ed25519.pub'

Testing error is raised when key from config does not exist:

  1. set public_key_path in [oci] config in my pycloudlib.toml config file to be ~/.ssh/nonexistant-key.pub
  2. ran the oracle example script: python3 examples/oracle.py
  3. Ensured error stating the following was thrown:
pycloudlib.errors.PycloudlibError: Provided public key path '/home/a-dubs/.ssh/oracle-ipv6-testing.pub1' does not exist

@a-dubs a-dubs force-pushed the GH-341-check-multiple-pubkeys branch 6 times, most recently from 6d98b02 to 2e1190a Compare August 28, 2024 22:19
@a-dubs
Copy link
Collaborator Author

a-dubs commented Aug 29, 2024

putting this into draft until i fix tests.

@a-dubs a-dubs marked this pull request as draft August 29, 2024 00:08
@a-dubs
Copy link
Collaborator Author

a-dubs commented Aug 29, 2024

Also, should i add new unit tests that verify the functionality of the added changes? @holmanb (tag you're it)

@holmanb holmanb self-assigned this Aug 30, 2024
Copy link
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

Also, should i add new unit tests that verify the functionality of the added changes? @holmanb (tag you're it)

Pycloudlib's test coverage is unfortunately very light on tests, but in this case the feature being tested is something that a user would immediately notice if it isn't working.

I'm honestly not sure whether it would be worth the effort in this case to write and maintain a test given the infrequency of changes to this part of the code and the immediate feedback that we would receive.

What do you think?

@@ -62,13 +66,39 @@ def __init__(
self._check_and_set_config(config_file, required_values)

user = getpass.getuser()
# check if id_rsa or id_ed25519 keys exist in the user's .ssh directory
Copy link
Member

Choose a reason for hiding this comment

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

This turns __init__() mostly into a key-handling function. I would prefer a helper method _get_ssh_keys()[1] to contain this logic and get called from __init__()

[1] maybe a better name, I didn't put any thought into that one

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Haha agreed! Way too much going on there. And would allow for actually unit testing that (if so desired)

@a-dubs
Copy link
Collaborator Author

a-dubs commented Sep 9, 2024

Also, should i add new unit tests that verify the functionality of the added changes? @holmanb (tag you're it)

Pycloudlib's test coverage is unfortunately very light on tests, but in this case the feature being tested is something that a user would immediately notice if it isn't working.

I'm honestly not sure whether it would be worth the effort in this case to write and maintain a test given the infrequency of changes to this part of the code and the immediate feedback that we would receive.

What do you think?

Let me go fix the existing unit tests that this change broke, and see if this change gets inherently tested that way. If not, it should be sufficient to just test the helper function that I'll create encapsulating this functionality.

@a-dubs a-dubs force-pushed the GH-341-check-multiple-pubkeys branch from 2e1190a to e540971 Compare September 24, 2024 14:08
Since Ubuntu 24.04 LTS and newer, the default location for ssh keys is
`~/.ssh/id_ed25519` instead of `~/.ssh/id_rsa`. Thus, when no ssh key
path is specified, this commit changes pycloudlib to check for an
existing ssh key at either  `~/.ssh/id_ed25519` or `~/.ssh/id_rsa`.

Fixes GH canonical#341
@a-dubs a-dubs force-pushed the GH-341-check-multiple-pubkeys branch from e540971 to 92b30f6 Compare September 24, 2024 14:54
@a-dubs a-dubs marked this pull request as ready for review September 24, 2024 14:57
@a-dubs
Copy link
Collaborator Author

a-dubs commented Sep 24, 2024

@holmanb ready for re-review!

@a-dubs a-dubs requested a review from holmanb September 24, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants