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

Authentication using ssh-agent #248

Merged
merged 3 commits into from
Oct 2, 2023

Conversation

dosas
Copy link
Contributor

@dosas dosas commented Sep 26, 2023

This PR adds authentication using ssh-agent, see #246

Also exception handling for authentication is unified.

Copy link
Member

@JacobCallahan JacobCallahan left a comment

Choose a reason for hiding this comment

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

Also, please try installing pre-commit in your virtual environment and run pre-commit when you stage changes. You can also run it against specific files or --all-files.
This will get your checks passing!

Comment on lines 62 to 80
if key_filename:
if not Path(key_filename).exists():
raise FileNotFoundError(f"Key not found in '{key_filename}'")
self.session.userauth_publickey_fromfile(user, key_filename)
elif kwargs.get("password"):
self.session.userauth_password(user, kwargs["password"])
try:
self.session.userauth_publickey_fromfile(user, key_filename)
except Exception as err:
raise exceptions.AuthenticationError("Key-based authentication failed.") from err
elif password:
try:
self.session.userauth_password(user, password)
except Exception as err:
raise exceptions.AuthenticationError("Password-based authentication failed.") from err
elif user:
try:
self.session.agent_auth(user)
except Exception as err:
raise exceptions.AuthenticationError("Agent-based authentication failed.") from err
else:
raise exceptions.AuthenticationError("No password or key file provided.")
Copy link
Member

Choose a reason for hiding this comment

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

I like where you're going with, but it looks a bit more repetitive than I'd prefer. Perhaps we could wrap the entire section in a try/except and raise a formatted message like this.

        try:
            if key_filename:
                auth_type = "Key"
                if not Path(key_filename).exists():
                    raise FileNotFoundError(f"Key not found in '{key_filename}'")
                self.session.userauth_publickey_fromfile(user, key_filename)
            elif password:
                auth_type = "Password"
                self.session.userauth_password(user, password)
            elif user:
                auth_type = "Agent"
                self.session.agent_auth(user)
            else:
                raise exceptions.AuthenticationError("No password or key file provided.")
        except Exception as err:
            raise exceptions.AuthenticationError(f"{auth_type}-based authentication failed.") from err

Copy link
Contributor Author

@dosas dosas Sep 27, 2023

Choose a reason for hiding this comment

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

This will lead to an error when the else clause is called since in this case auth_type is not defined for the broad Exception and the error message will also be misleading.

Since ruff is complaining about blind exceptions, would it be okay to just omit the outer except block?

Copy link
Member

Choose a reason for hiding this comment

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

auth_type isn't used in the else clause, so shouldn't cause an issue.
as for ruff, you can add this to the right of the except statement # noqa: BLE001 there are likely a few different exceptions that could be raised here and I'd rather not waste your time chasing them all down.

Copy link
Member

@JacobCallahan JacobCallahan left a comment

Choose a reason for hiding this comment

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

Can you provide a demonstration of agent/session-based authentication working for you locally?

@dosas
Copy link
Contributor Author

dosas commented Oct 2, 2023

I am not exactly sure what I should put here but I have written some tests and ran them locally:

def test_key():
    test_host = Host(hostname=hostname, key_filename="id_ed25519")
    assert "host1" in test_host.execute("hostname").stdout, "path"


def test_wrong_key():
    test_host = Host(hostname=hostname, key_filename="wrong_key")
    with pytest.raises(BrokerAuthenticationError):
        test_host.execute("hostname").stdout

def test_password():
    test_host = Host(hostname=hostname, username="root", password="bar")
    assert "host1" in test_host.execute("hostname").stdout, "password"


def test_wrong_password():
    test_host = Host(hostname=hostname, username="root", password="foo")
    with pytest.raises(BrokerAuthenticationError):
        test_host.execute("hostname").stdout


def test_agent():
    test_host = Host(hostname=hostname, username="root")
    assert "host1" in test_host.execute("hostname").stdout, "agent"

with the wollowing results:

test_manual.py::test_key PASSED                                                                                                                                                        [ 20%]
test_manual.py::test_wrong_key PASSED                                                                                                                                                  [ 40%]
test_manual.py::test_password PASSED                                                                                                                                                   [ 60%]
test_manual.py::test_wrong_password PASSED                                                                                                                                             [ 80%]
test_manual.py::test_agent PASSED

@JacobCallahan
Copy link
Member

Awesome, thanks for the contribution @dosas!

@JacobCallahan JacobCallahan merged commit 7f0040d into SatelliteQE:master Oct 2, 2023
3 checks passed
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