Skip to content

Commit

Permalink
Merge topic branch 'issue15-graceful-af_unix-degredation' into master
Browse files Browse the repository at this point in the history
* t/issue15-graceful-af_unix-degredation:
  Fail gracefully if UNIX domain socket support is unavailable

GitHub: Closes #15.
  • Loading branch information
the-13th-letter committed Oct 2, 2024
2 parents 2511d75 + 27f9bd1 commit ba27276
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 19 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
### Changed

- Fail earlier, and more gracefully/specifically, when we cannot talk to
the SSH agent because Python does not support UNIX domain sockets on
this system. In particular, this is the current situation on Windows.

This adds another failure case to the `SSHAgentClient` constructor, and
therefore constitutes a breaking API change.

19 changes: 17 additions & 2 deletions src/derivepassphrase/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,8 @@ def _get_suitable_ssh_keys(
If neither are given, then the agent's socket location is
looked up in the `SSH_AUTH_SOCK` environment variable, and
used to construct/deconstruct a one-shot client, as in the
previous case.
previous case. This requires the [`socket.AF_UNIX`][]
symbol to exist.
Yields:
Every SSH key from the SSH agent that is suitable for passphrase
Expand All @@ -506,6 +507,10 @@ def _get_suitable_ssh_keys(
KeyError:
`conn` was `None`, and the `SSH_AUTH_SOCK` environment
variable was not found.
NotImplementedError:
`conn` was `None`, and this Python does not support
[`socket.AF_UNIX`][], so the SSH agent client cannot be
automatically set up.
OSError:
`conn` was a socket or `None`, and there was an error
setting up a socket connection to the agent.
Expand Down Expand Up @@ -640,7 +645,8 @@ def _select_ssh_key(
If neither are given, then the agent's socket location is
looked up in the `SSH_AUTH_SOCK` environment variable, and
used to construct/deconstruct a one-shot client, as in the
previous case.
previous case. This requires the [`socket.AF_UNIX`][]
symbol to exist.
Returns:
The selected SSH key.
Expand All @@ -649,6 +655,10 @@ def _select_ssh_key(
KeyError:
`conn` was `None`, and the `SSH_AUTH_SOCK` environment
variable was not found.
NotImplementedError:
`conn` was `None`, and this Python does not support
[`socket.AF_UNIX`][], so the SSH agent client cannot be
automatically set up.
OSError:
`conn` was a socket or `None`, and there was an error
setting up a socket connection to the agent.
Expand Down Expand Up @@ -1477,6 +1487,11 @@ def put_config(config: _types.VaultConfig, /) -> None:
err('No valid SSH key selected')
except KeyError:
err('Cannot find running SSH agent; check SSH_AUTH_SOCK')
except NotImplementedError:
err(
'Cannot connect to SSH agent because '
'this Python version does not support UNIX domain sockets'
)
except OSError as e:
err(
f'Cannot connect to SSH agent: {e.strerror}: '
Expand Down
35 changes: 22 additions & 13 deletions src/derivepassphrase/ssh_agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
from __future__ import annotations

import collections
import errno
import os
import socket
from typing import TYPE_CHECKING, overload
Expand Down Expand Up @@ -85,9 +84,16 @@ def __init__(
Args:
socket:
An optional socket, connected to the SSH agent. If not
given, we query the `SSH_AUTH_SOCK` environment
An optional socket, already connected to the SSH agent.
If not given, we query the `SSH_AUTH_SOCK` environment
variable to auto-discover the correct socket address.
[We currently only support connecting via UNIX domain
sockets][issue13], and only on platforms with support
for [`socket.AF_UNIX`][AF_UNIX].
[issue13]: https://github.com/the-13th-letter/derivepassphrase/issues/13
[AF_UNIX]: https://docs.python.org/3/library/socket.html#socket.AF_UNIX
timeout:
A connection timeout for the SSH agent. Only used if
the socket is not yet connected. The default value
Expand All @@ -96,27 +102,30 @@ def __init__(
Raises:
KeyError:
The `SSH_AUTH_SOCK` environment was not found.
The `SSH_AUTH_SOCK` environment variable was not found.
NotImplementedError:
This Python version does not support UNIX domain
sockets, necessary to automatically connect to a running
SSH agent via the `SSH_AUTH_SOCK` environment variable.
OSError:
There was an error setting up a socket connection to the
agent.
"""
if socket is not None:
self._connection = socket
else:
self._connection = _socket.socket(family=_socket.AF_UNIX)
try:
# Test whether the socket is connected.
self._connection.getpeername()
except OSError as e:
# This condition is hard to test purposefully, so exclude
# from coverage.
if e.errno != errno.ENOTCONN: # pragma: no cover
raise
else:
if not hasattr(_socket, 'AF_UNIX'):
msg = (
'This Python version does not support UNIX domain sockets'
)
raise NotImplementedError(msg)
self._connection = _socket.socket(family=_socket.AF_UNIX)
if 'SSH_AUTH_SOCK' not in os.environ:
msg = 'SSH_AUTH_SOCK environment variable'
raise KeyError(msg) from None
raise KeyError(msg)
ssh_auth_sock = os.environ['SSH_AUTH_SOCK']
self._connection.settimeout(timeout)
self._connection.connect(ssh_auth_sock)
Expand Down
22 changes: 21 additions & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import operator
import os
import shutil
import socket
import subprocess
import sys
import textwrap
Expand Down Expand Up @@ -51,6 +52,20 @@ def term_handler() -> Iterator[None]: # pragma: no cover
signal.signal(signal.SIGTERM, orig_term)


@pytest.fixture(scope='session')
def skip_if_no_af_unix_support() -> None: # pragma: no cover
"""Skip the test if Python does not support AF_UNIX.
Implemented as a fixture instead of a mark because it has
consequences for other fixtures, and because another "autouse"
session fixture may want to force/simulate non-support of
[`socket.AF_UNIX`][].
"""
if not hasattr(socket, 'AF_UNIX'):
pytest.skip('socket module does not support AF_UNIX')


def _spawn_pageant( # pragma: no cover
executable: str | None, env: dict[str, str]
) -> tuple[subprocess.Popen[str], bool] | None:
Expand Down Expand Up @@ -217,7 +232,9 @@ def _spawn_system_agent( # pragma: no cover


@pytest.fixture
def running_ssh_agent() -> Iterator[str]: # pragma: no cover
def running_ssh_agent( # pragma: no cover
skip_if_no_af_unix_support: None,
) -> Iterator[str]:
"""Ensure a running SSH agent, if possible, as a pytest fixture.
Check for a running SSH agent, or spawn a new one if possible. We
Expand All @@ -237,6 +254,7 @@ def running_ssh_agent() -> Iterator[str]: # pragma: no cover
If no agent is running or can be spawned, skip this test.
"""
del skip_if_no_af_unix_support
exit_stack = contextlib.ExitStack()
Popen = TypeVar('Popen', bound=subprocess.Popen)

Expand Down Expand Up @@ -371,6 +389,7 @@ def _spawn_data_sink( # pragma: no cover
@pytest.fixture(params=_spawn_handlers, ids=operator.itemgetter(0))
def spawn_ssh_agent( # noqa: C901
request: pytest.FixtureRequest,
skip_if_no_af_unix_support: None,
) -> Iterator[tests.SpawnedSSHAgentInfo]:
"""Spawn an isolated SSH agent, if possible, as a pytest fixture.
Expand All @@ -390,6 +409,7 @@ def spawn_ssh_agent( # noqa: C901
If the agent cannot be spawned, skip this test.
"""
del skip_if_no_af_unix_support
agent_env = os.environ.copy()
agent_env.pop('SSH_AUTH_SOCK', None)
exit_stack = contextlib.ExitStack()
Expand Down
26 changes: 26 additions & 0 deletions tests/test_derivepassphrase_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -975,7 +975,9 @@ def raiser() -> None:
def test_225b_store_config_fail_manual_no_ssh_agent(
self,
monkeypatch: pytest.MonkeyPatch,
skip_if_no_af_unix_support: None,
) -> None:
del skip_if_no_af_unix_support
runner = click.testing.CliRunner(mix_stderr=False)
with tests.isolated_vault_config(
monkeypatch=monkeypatch,
Expand Down Expand Up @@ -1295,6 +1297,30 @@ def test_300_unicode_normalization_form_warning(
warning_message in result.stderr
), 'expected known warning message in stderr'

def test_400_missing_af_unix_support(
self,
monkeypatch: pytest.MonkeyPatch,
) -> None:
runner = click.testing.CliRunner(mix_stderr=False)
with tests.isolated_vault_config(
monkeypatch=monkeypatch,
runner=runner,
config={'global': {'phrase': 'abc'}, 'services': {}},
):
monkeypatch.setenv(
'SSH_AUTH_SOCK', "the value doesn't even matter"
)
monkeypatch.delattr(socket, 'AF_UNIX', raising=False)
_result = runner.invoke(
cli.derivepassphrase_vault,
['--key', '--config'],
catch_exceptions=False,
)
result = tests.ReadableResult.parse(_result)
assert result.error_exit(
error='does not support UNIX domain sockets'
), 'expected error exit and known error message'


class TestCLIUtils:
@pytest.mark.parametrize(
Expand Down
21 changes: 18 additions & 3 deletions tests/test_derivepassphrase_ssh_agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,14 +104,16 @@ def test_190_sh_export_line_parsing(
tests.parse_sh_export_line(line, env_name=env_name)

def test_200_constructor_no_running_agent(
self, monkeypatch: pytest.MonkeyPatch
self,
monkeypatch: pytest.MonkeyPatch,
skip_if_no_af_unix_support: None,
) -> None:
del skip_if_no_af_unix_support
monkeypatch.delenv('SSH_AUTH_SOCK', raising=False)
sock = socket.socket(family=socket.AF_UNIX)
with pytest.raises(
KeyError, match='SSH_AUTH_SOCK environment variable'
):
ssh_agent.SSHAgentClient(socket=sock)
ssh_agent.SSHAgentClient()

@pytest.mark.parametrize(
['input', 'expected'],
Expand Down Expand Up @@ -352,6 +354,19 @@ def test_300_constructor_bad_running_agent(
with pytest.raises(OSError): # noqa: PT011
ssh_agent.SSHAgentClient(socket=sock)

def test_301_constructor_no_af_unix_support(
self,
monkeypatch: pytest.MonkeyPatch,
) -> None:
with monkeypatch.context() as monkeypatch2:
monkeypatch2.setenv('SSH_AUTH_SOCK', "the value doesn't matter")
monkeypatch2.delattr(socket, 'AF_UNIX', raising=False)
with pytest.raises(
NotImplementedError,
match='UNIX domain sockets',
):
ssh_agent.SSHAgentClient()

@pytest.mark.parametrize(
'response',
[
Expand Down

0 comments on commit ba27276

Please sign in to comment.