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.

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

fix(ssh): don't raise exception when default ssh keys can't be found #452

Merged
merged 1 commit into from
Nov 27, 2024
Merged

fix(ssh): don't raise exception when default ssh keys can't be found #452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion VERSION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
1!10.5.0
1!10.6.0
52 changes: 42 additions & 10 deletions pycloudlib/cloud.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,11 @@ def __init__(
self.tag = get_timestamped_tag(tag) if timestamp_suffix else tag
self._validate_tag(self.tag)

self.key_pair = self._get_ssh_keys()
self.key_pair = self._get_ssh_keys(
public_key_path=self.config.get("public_key_path", ""),
private_key_path=self.config.get("private_key_path", ""),
name=self.config.get("key_name", getpass.getuser()),
)

def __enter__(self):
"""Enter context manager for this class."""
Expand Down Expand Up @@ -251,7 +255,11 @@ def use_key(self, public_key_path, private_key_path=None, name=None):
name: name to reference key by
"""
self._log.debug("using SSH key from %s", public_key_path)
self.key_pair = KeyPair(public_key_path, private_key_path, name)
self.key_pair = self._get_ssh_keys(
public_key_path=public_key_path,
private_key_path=private_key_path,
name=name,
)

def _check_and_set_config(
self,
Expand Down Expand Up @@ -310,33 +318,57 @@ def _validate_tag(tag: str):
if rules_failed:
raise InvalidTagNameError(tag=tag, rules_failed=rules_failed)

def _get_ssh_keys(self) -> KeyPair:
user = getpass.getuser()
# check if id_rsa or id_ed25519 keys exist in the user's .ssh directory
def _get_ssh_keys(
self,
public_key_path: Optional[str] = None,
private_key_path: Optional[str] = None,
name: Optional[str] = None,
) -> KeyPair:
"""Retrieve SSH key pair paths.

This method attempts to retrieve the paths to the public and private SSH keys.
If no public key path is provided, it will look for default keys in the user's
`~/.ssh` directory. If no keys are found, it logs a warning and returns a KeyPair
with None values.

Args:
public_key_path (Optional[str]): The path to the public SSH key. If not provided,
the method will search for default keys.
private_key_path (Optional[str]): The path to the private SSH key. Defaults to None.
name (Optional[str]): An optional name for the key pair. Defaults to None.

Returns:
KeyPair: An instance of KeyPair containing the paths to the public and private keys,
and the optional name.

Raises:
PycloudlibError: If the provided public key path does not exist.
"""
possible_default_keys = [
os.path.expanduser("~/.ssh/id_rsa.pub"),
os.path.expanduser("~/.ssh/id_ed25519.pub"),
]
public_key_path: Optional[str] = os.path.expanduser(self.config.get("public_key_path", ""))
public_key_path = os.path.expanduser(public_key_path or "")
if not public_key_path:
for pubkey in possible_default_keys:
if os.path.exists(pubkey):
self._log.info("No public key path provided, using: %s", pubkey)
public_key_path = pubkey
break
if not public_key_path:
raise PycloudlibError(
self._log.warning(
"No public key path provided and no key found in default locations: "
"'~/.ssh/id_rsa.pub' or '~/.ssh/id_ed25519.pub'"
"'~/.ssh/id_rsa.pub' or '~/.ssh/id_ed25519.pub'. SSH key authentication will "
"not be possible unless a key is later provided with the 'use_key' method."
)
return KeyPair(None, None, None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can be more explicit by returning None and adjust _get_ssh_keys to return Optional[KeyPair] instead of a KeyPair with null attributes. Otherwise _get_ssh_keys is returning KeyPairs which are effectively invalid objects (prefer to construct complete and correct data structures) and nullity checks against the object would need to inspect each attribute.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we return None from _get_ssh_keys, this makes for absolutely nuclear splash radius. why not add the null check in KeyPair class in one place instead of needing to add checks across dozens of call sites that rely on the ssh key being a KeyPair and not potentially none?

Copy link
Member

Choose a reason for hiding this comment

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

I initially had the same thought as @jrtknauer , but you're right @a-dubs in that this'll introduce a ton of None checks across the code base. We already allow a keypair with no private key set (why???), so this doesn't seem to be all that much worse to me.

Do we know if there are any use cases of anybody needing to set a key after initializing a cloud object? I don't just mean that use_key() currently has callers, but is there a reason we couldn't rely on the class always being initialized with the key pair? If not, I think I'd prefer we nuke use_key() altogether and turn it into a constructor argument. There's no reason to carry around uninitialized or half-initialized state if it's not needed. I realize that'd be a breaking change and require a major version bump, so I'm not asking for it here, but it might be better end state to get to.

Copy link
Collaborator

@jrtknauer jrtknauer Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we know if there are any use cases of anybody needing to set a key after initializing a cloud object? I don't just mean that use_key() currently has callers, but is there a reason we couldn't rely on the class always being initialized with the key pair? If not, I think I'd prefer we nuke use_key() altogether and turn it into a constructor argument. There's no reason to carry around uninitialized or half-initialized state if it's not needed. I realize that'd be a breaking change and require a major version bump, so I'm not asking for it here, but it might be better end state to get to

Given I encountered the issue with the current implementation in the wild and motivated this pull request:

There is no interface in the current implementation for the IBM cloud to set SSH keys on instantiation:

class IBM(BaseCloud):
    """IBM Virtual Private Cloud Class."""

    _type = "ibm"

    def __init__(
        self,
        tag: str,
        timestamp_suffix: bool = True,
        config_file: Optional[ConfigFile] = None,
        *,
        resource_group: Optional[str] = None,
        vpc: Optional[str] = None,
        api_key: Optional[str] = None,
        region: Optional[str] = None,
        zone: Optional[str] = None,
    ):
        """Initialize the connection to IBM VPC.

        Args:
            tag: string used to name and tag resources with
            timestamp_suffix: Append a timestamped suffix to the tag string.
            config_file: path to pycloudlib configuration file
        """
        super().__init__(
            tag,
            timestamp_suffix,
            config_file,
            required_values=[resource_group, api_key, region],
        )
        ...

In fact, this is an issue with all clouds. Removing use_key is a non-starter until this is addressed. But that's not the point of this pull request. All I wanted was for pycloudlib to not raise exceptions if an SSH key is not set on initialization, because the introduction of that behavior in 335c4a4d9525d78120cc127905ccbd1928046e2d now crashes applications using pycloudlib on systems which do not have the hard-coded default public SSH keys (~/.ssh/id_rsa.pub or ~/.ssh/id_ed25519.pub).

My would-like-to-haves span from:

  • Not constructing objects with nothing but None attributes. However, as it's been established the existing pattern has infiltrated the entirety of the code base making it a high impact change on its own.
  • Consistency in specification of SSH keys from Cloud initialization. However, this would require updating the initialization interfaces for the base Cloud class and all derived clouds.

Ultimately, my requirement is that pycloudlib is not crashing from a recoverable state (use_key exists).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks for the feedback @jrtknauer. I agree that we should implement a way to pass ssh key configuration to the base cloud class at runtime (enabling this across all clouds simultaneously) and then possibly explore the removal of the use_key() function. But for the time being, the changes put forward in this PR will alleviate the issue you are describing and that other members of CPC have been running into. So I will open an issue to capture this future work, which will also address both items in your nice-to-have list.

if not os.path.exists(os.path.expanduser(public_key_path)):
raise PycloudlibError(f"Provided public key path '{public_key_path}' does not exist")
if public_key_path not in possible_default_keys:
self._log.info("Using provided public key path: '%s'", public_key_path)
private_key_path = self.config.get("private_key_path", "")

return KeyPair(
public_key_path=public_key_path,
private_key_path=private_key_path,
name=self.config.get("key_name", user),
name=name,
)
15 changes: 15 additions & 0 deletions pycloudlib/errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,3 +156,18 @@ def __init__(self, tag: str, rules_failed: List[str]):
def __str__(self) -> str:
"""Return string representation of the error."""
return f"Tag '{self.tag}' failed the following rules: {', '.join(self.rules_failed)}"


class UnsetSSHKeyError(PycloudlibException):
"""Raised when a SSH key is unset and no default key can be found."""

def __str__(self) -> str:
"""Return string representation of the error."""
return (
"No public key content available for unset key pair. This error occurs when no SSH "
"key is provided in the pycloudlib.toml file and no default keys can be found on "
"the system. If you wish to provide custom SSH keys at runtime, you can do so by "
"calling the `use_key` method on the `Cloud` class. If you wish to use default SSH "
"keys, make sure they are present on the system and that they are located in the "
"default locations."
)
27 changes: 20 additions & 7 deletions pycloudlib/key.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,20 @@
"""Base Key Class."""

import os
from typing import Optional

from pycloudlib.errors import UnsetSSHKeyError


class KeyPair:
"""Key Class."""

def __init__(self, public_key_path, private_key_path=None, name=None):
def __init__(
self,
public_key_path: Optional[str],
private_key_path: Optional[str] = None,
name: Optional[str] = None,
):
"""Initialize key class to generate key or reuse existing key.

The public key path is given then the key is stored and the
Expand All @@ -21,11 +29,15 @@ def __init__(self, public_key_path, private_key_path=None, name=None):
"""
self.name = name
self.public_key_path = public_key_path
if private_key_path:
self.private_key_path = private_key_path
else:
self.private_key_path = self.public_key_path.replace(".pub", "")

# don't set private key path if public key path is None (ssh key is unset)
if self.public_key_path is None:
self.private_key_path = None
return

self.private_key_path = private_key_path or self.public_key_path.replace(".pub", "")

# Expand user paths after setting private key path
self.public_key_path = os.path.expanduser(self.public_key_path)
self.private_key_path = os.path.expanduser(self.private_key_path)

Expand All @@ -40,7 +52,8 @@ def public_key_content(self):
"""Read the contents of the public key.

Returns:
output of public key

str: The public key content
"""
if self.public_key_path is None:
raise UnsetSSHKeyError()
return open(self.public_key_path, encoding="utf-8").read()
29 changes: 27 additions & 2 deletions tests/unit_tests/test_cloud.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
"""Tests related to pycloudlib.cloud module."""

from io import StringIO
import logging
from textwrap import dedent
from typing import List
from typing import List, Optional

import mock
import pytest

from pycloudlib.cloud import BaseCloud
from pycloudlib.errors import InvalidTagNameError
from pycloudlib.errors import InvalidTagNameError, UnsetSSHKeyError

# mock module path
MPATH = "pycloudlib.cloud."
Expand Down Expand Up @@ -181,6 +182,30 @@ def test_missing_private_key_in_ssh_config(self, _m_expanduser, _m_exists):
assert mycloud.key_pair.public_key_path == "/home/asdf/.ssh/id_rsa.pub"
assert mycloud.key_pair.private_key_path == "/home/asdf/.ssh/id_rsa"

@pytest.mark.dont_mock_ssh_keys
@mock.patch("os.path.expanduser", side_effect=lambda x: x.replace("~", "/root"))
@mock.patch("os.path.exists", return_value=False)
def test_init_raises_error_when_no_ssh_keys_found(
self,
_m_expanduser,
_m_exists,
caplog,
):
"""
Test that an error is raised when no SSH keys can be found.

This test verifies that an error is raised when no SSH keys can be found in the default
locations and no public key path is provided in the config file.
"""
# set log level to Warning to ensure warning gets logged
caplog.set_level(logging.WARNING)
with pytest.raises(UnsetSSHKeyError) as exc_info:
cloud = CloudSubclass(tag="tag", timestamp_suffix=False, config_file=StringIO(CONFIG))
# now we try to access the public key content to trigger the exception
cloud.key_pair.public_key_content
assert "No public key path provided and no key found in default locations" in caplog.text
assert "No public key content available for unset key pair." in str(exc_info.value)

rule1 = "All letters must be lowercase"
rule2 = "Must be between 1 and 63 characters long"
rule3 = "Must not start or end with a hyphen"
Expand Down