Marco Ricci commited on 2024-10-02 19:26:30
Zeige 6 geänderte Dateien mit 113 Einfügungen und 19 Löschungen.
To talk to the SSH agent, we currently require UNIX domain socket support, but not every Python on every system supports this (notably: Windows). If we detect such missing support, fail gracefully and with a useful error message, instead of a technical reason such as `AttributeError`. Besides the new failure modes that API consumers will need to handle, this results in one more observable change: socket objects passed to the `SSHAgentClient` constructor are now required to be already connected. The constructor will no longer prepare sockets it didn't create itself in any way. This new failure behavior also has consequences for the tests, which so far have naively assumed UNIX semantics and UNIX domain socket support. So change the testing machinery to automatically skip any test that involves constructing a custom SSH agent client *and* expecting that step to go well. Furthermore, since the "no support" constellation can be reasonably well simulated even on systems that *do* have UNIX domain socket support (via pytest's monkeypatching fixture), include explicit tests on the API and the CLI level for the "no support" constellation, in any case.
... | ... |
@@ -0,0 +1,9 @@ |
1 |
+### Changed |
|
2 |
+ |
|
3 |
+ - Fail earlier, and more gracefully/specifically, when we cannot talk to |
|
4 |
+ the SSH agent because Python does not support UNIX domain sockets on |
|
5 |
+ this system. In particular, this is the current situation on Windows. |
|
6 |
+ |
|
7 |
+ This adds another failure case to the `SSHAgentClient` constructor, and |
|
8 |
+ therefore constitutes a breaking API change. |
|
9 |
+ |
... | ... |
@@ -496,7 +496,8 @@ def _get_suitable_ssh_keys( |
496 | 496 |
If neither are given, then the agent's socket location is |
497 | 497 |
looked up in the `SSH_AUTH_SOCK` environment variable, and |
498 | 498 |
used to construct/deconstruct a one-shot client, as in the |
499 |
- previous case. |
|
499 |
+ previous case. This requires the [`socket.AF_UNIX`][] |
|
500 |
+ symbol to exist. |
|
500 | 501 |
|
501 | 502 |
Yields: |
502 | 503 |
Every SSH key from the SSH agent that is suitable for passphrase |
... | ... |
@@ -506,6 +507,10 @@ def _get_suitable_ssh_keys( |
506 | 507 |
KeyError: |
507 | 508 |
`conn` was `None`, and the `SSH_AUTH_SOCK` environment |
508 | 509 |
variable was not found. |
510 |
+ NotImplementedError: |
|
511 |
+ `conn` was `None`, and this Python does not support |
|
512 |
+ [`socket.AF_UNIX`][], so the SSH agent client cannot be |
|
513 |
+ automatically set up. |
|
509 | 514 |
OSError: |
510 | 515 |
`conn` was a socket or `None`, and there was an error |
511 | 516 |
setting up a socket connection to the agent. |
... | ... |
@@ -640,7 +645,8 @@ def _select_ssh_key( |
640 | 645 |
If neither are given, then the agent's socket location is |
641 | 646 |
looked up in the `SSH_AUTH_SOCK` environment variable, and |
642 | 647 |
used to construct/deconstruct a one-shot client, as in the |
643 |
- previous case. |
|
648 |
+ previous case. This requires the [`socket.AF_UNIX`][] |
|
649 |
+ symbol to exist. |
|
644 | 650 |
|
645 | 651 |
Returns: |
646 | 652 |
The selected SSH key. |
... | ... |
@@ -649,6 +655,10 @@ def _select_ssh_key( |
649 | 655 |
KeyError: |
650 | 656 |
`conn` was `None`, and the `SSH_AUTH_SOCK` environment |
651 | 657 |
variable was not found. |
658 |
+ NotImplementedError: |
|
659 |
+ `conn` was `None`, and this Python does not support |
|
660 |
+ [`socket.AF_UNIX`][], so the SSH agent client cannot be |
|
661 |
+ automatically set up. |
|
652 | 662 |
OSError: |
653 | 663 |
`conn` was a socket or `None`, and there was an error |
654 | 664 |
setting up a socket connection to the agent. |
... | ... |
@@ -1477,6 +1487,11 @@ def derivepassphrase_vault( # noqa: C901,PLR0912,PLR0913,PLR0914,PLR0915 |
1477 | 1487 |
err('No valid SSH key selected') |
1478 | 1488 |
except KeyError: |
1479 | 1489 |
err('Cannot find running SSH agent; check SSH_AUTH_SOCK') |
1490 |
+ except NotImplementedError: |
|
1491 |
+ err( |
|
1492 |
+ 'Cannot connect to SSH agent because ' |
|
1493 |
+ 'this Python version does not support UNIX domain sockets' |
|
1494 |
+ ) |
|
1480 | 1495 |
except OSError as e: |
1481 | 1496 |
err( |
1482 | 1497 |
f'Cannot connect to SSH agent: {e.strerror}: ' |
... | ... |
@@ -7,7 +7,6 @@ |
7 | 7 |
from __future__ import annotations |
8 | 8 |
|
9 | 9 |
import collections |
10 |
-import errno |
|
11 | 10 |
import os |
12 | 11 |
import socket |
13 | 12 |
from typing import TYPE_CHECKING, overload |
... | ... |
@@ -85,9 +84,16 @@ class SSHAgentClient: |
85 | 84 |
|
86 | 85 |
Args: |
87 | 86 |
socket: |
88 |
- An optional socket, connected to the SSH agent. If not |
|
89 |
- given, we query the `SSH_AUTH_SOCK` environment |
|
87 |
+ An optional socket, already connected to the SSH agent. |
|
88 |
+ If not given, we query the `SSH_AUTH_SOCK` environment |
|
90 | 89 |
variable to auto-discover the correct socket address. |
90 |
+ |
|
91 |
+ [We currently only support connecting via UNIX domain |
|
92 |
+ sockets][issue13], and only on platforms with support |
|
93 |
+ for [`socket.AF_UNIX`][AF_UNIX]. |
|
94 |
+ |
|
95 |
+ [issue13]: https://github.com/the-13th-letter/derivepassphrase/issues/13 |
|
96 |
+ [AF_UNIX]: https://docs.python.org/3/library/socket.html#socket.AF_UNIX |
|
91 | 97 |
timeout: |
92 | 98 |
A connection timeout for the SSH agent. Only used if |
93 | 99 |
the socket is not yet connected. The default value |
... | ... |
@@ -96,7 +102,11 @@ class SSHAgentClient: |
96 | 102 |
|
97 | 103 |
Raises: |
98 | 104 |
KeyError: |
99 |
- The `SSH_AUTH_SOCK` environment was not found. |
|
105 |
+ The `SSH_AUTH_SOCK` environment variable was not found. |
|
106 |
+ NotImplementedError: |
|
107 |
+ This Python version does not support UNIX domain |
|
108 |
+ sockets, necessary to automatically connect to a running |
|
109 |
+ SSH agent via the `SSH_AUTH_SOCK` environment variable. |
|
100 | 110 |
OSError: |
101 | 111 |
There was an error setting up a socket connection to the |
102 | 112 |
agent. |
... | ... |
@@ -104,19 +114,18 @@ class SSHAgentClient: |
104 | 114 |
""" |
105 | 115 |
if socket is not None: |
106 | 116 |
self._connection = socket |
107 |
- else: |
|
108 |
- self._connection = _socket.socket(family=_socket.AF_UNIX) |
|
109 |
- try: |
|
110 | 117 |
# Test whether the socket is connected. |
111 | 118 |
self._connection.getpeername() |
112 |
- except OSError as e: |
|
113 |
- # This condition is hard to test purposefully, so exclude |
|
114 |
- # from coverage. |
|
115 |
- if e.errno != errno.ENOTCONN: # pragma: no cover |
|
116 |
- raise |
|
119 |
+ else: |
|
120 |
+ if not hasattr(_socket, 'AF_UNIX'): |
|
121 |
+ msg = ( |
|
122 |
+ 'This Python version does not support UNIX domain sockets' |
|
123 |
+ ) |
|
124 |
+ raise NotImplementedError(msg) |
|
125 |
+ self._connection = _socket.socket(family=_socket.AF_UNIX) |
|
117 | 126 |
if 'SSH_AUTH_SOCK' not in os.environ: |
118 | 127 |
msg = 'SSH_AUTH_SOCK environment variable' |
119 |
- raise KeyError(msg) from None |
|
128 |
+ raise KeyError(msg) |
|
120 | 129 |
ssh_auth_sock = os.environ['SSH_AUTH_SOCK'] |
121 | 130 |
self._connection.settimeout(timeout) |
122 | 131 |
self._connection.connect(ssh_auth_sock) |
... | ... |
@@ -9,6 +9,7 @@ import contextlib |
9 | 9 |
import operator |
10 | 10 |
import os |
11 | 11 |
import shutil |
12 |
+import socket |
|
12 | 13 |
import subprocess |
13 | 14 |
import sys |
14 | 15 |
import textwrap |
... | ... |
@@ -51,6 +52,20 @@ def term_handler() -> Iterator[None]: # pragma: no cover |
51 | 52 |
signal.signal(signal.SIGTERM, orig_term) |
52 | 53 |
|
53 | 54 |
|
55 |
+@pytest.fixture(scope='session') |
|
56 |
+def skip_if_no_af_unix_support() -> None: # pragma: no cover |
|
57 |
+ """Skip the test if Python does not support AF_UNIX. |
|
58 |
+ |
|
59 |
+ Implemented as a fixture instead of a mark because it has |
|
60 |
+ consequences for other fixtures, and because another "autouse" |
|
61 |
+ session fixture may want to force/simulate non-support of |
|
62 |
+ [`socket.AF_UNIX`][]. |
|
63 |
+ |
|
64 |
+ """ |
|
65 |
+ if not hasattr(socket, 'AF_UNIX'): |
|
66 |
+ pytest.skip('socket module does not support AF_UNIX') |
|
67 |
+ |
|
68 |
+ |
|
54 | 69 |
def _spawn_pageant( # pragma: no cover |
55 | 70 |
executable: str | None, env: dict[str, str] |
56 | 71 |
) -> tuple[subprocess.Popen[str], bool] | None: |
... | ... |
@@ -217,7 +232,9 @@ _spawn_handlers = [ |
217 | 232 |
|
218 | 233 |
|
219 | 234 |
@pytest.fixture |
220 |
-def running_ssh_agent() -> Iterator[str]: # pragma: no cover |
|
235 |
+def running_ssh_agent( # pragma: no cover |
|
236 |
+ skip_if_no_af_unix_support: None, |
|
237 |
+) -> Iterator[str]: |
|
221 | 238 |
"""Ensure a running SSH agent, if possible, as a pytest fixture. |
222 | 239 |
|
223 | 240 |
Check for a running SSH agent, or spawn a new one if possible. We |
... | ... |
@@ -237,6 +254,7 @@ def running_ssh_agent() -> Iterator[str]: # pragma: no cover |
237 | 254 |
If no agent is running or can be spawned, skip this test. |
238 | 255 |
|
239 | 256 |
""" |
257 |
+ del skip_if_no_af_unix_support |
|
240 | 258 |
exit_stack = contextlib.ExitStack() |
241 | 259 |
Popen = TypeVar('Popen', bound=subprocess.Popen) |
242 | 260 |
|
... | ... |
@@ -371,6 +389,7 @@ def _spawn_data_sink( # pragma: no cover |
371 | 389 |
@pytest.fixture(params=_spawn_handlers, ids=operator.itemgetter(0)) |
372 | 390 |
def spawn_ssh_agent( # noqa: C901 |
373 | 391 |
request: pytest.FixtureRequest, |
392 |
+ skip_if_no_af_unix_support: None, |
|
374 | 393 |
) -> Iterator[tests.SpawnedSSHAgentInfo]: |
375 | 394 |
"""Spawn an isolated SSH agent, if possible, as a pytest fixture. |
376 | 395 |
|
... | ... |
@@ -390,6 +409,7 @@ def spawn_ssh_agent( # noqa: C901 |
390 | 409 |
If the agent cannot be spawned, skip this test. |
391 | 410 |
|
392 | 411 |
""" |
412 |
+ del skip_if_no_af_unix_support |
|
393 | 413 |
agent_env = os.environ.copy() |
394 | 414 |
agent_env.pop('SSH_AUTH_SOCK', None) |
395 | 415 |
exit_stack = contextlib.ExitStack() |
... | ... |
@@ -975,7 +975,9 @@ contents go here |
975 | 975 |
def test_225b_store_config_fail_manual_no_ssh_agent( |
976 | 976 |
self, |
977 | 977 |
monkeypatch: pytest.MonkeyPatch, |
978 |
+ skip_if_no_af_unix_support: None, |
|
978 | 979 |
) -> None: |
980 |
+ del skip_if_no_af_unix_support |
|
979 | 981 |
runner = click.testing.CliRunner(mix_stderr=False) |
980 | 982 |
with tests.isolated_vault_config( |
981 | 983 |
monkeypatch=monkeypatch, |
... | ... |
@@ -1295,6 +1297,30 @@ contents go here |
1295 | 1297 |
warning_message in result.stderr |
1296 | 1298 |
), 'expected known warning message in stderr' |
1297 | 1299 |
|
1300 |
+ def test_400_missing_af_unix_support( |
|
1301 |
+ self, |
|
1302 |
+ monkeypatch: pytest.MonkeyPatch, |
|
1303 |
+ ) -> None: |
|
1304 |
+ runner = click.testing.CliRunner(mix_stderr=False) |
|
1305 |
+ with tests.isolated_vault_config( |
|
1306 |
+ monkeypatch=monkeypatch, |
|
1307 |
+ runner=runner, |
|
1308 |
+ config={'global': {'phrase': 'abc'}, 'services': {}}, |
|
1309 |
+ ): |
|
1310 |
+ monkeypatch.setenv( |
|
1311 |
+ 'SSH_AUTH_SOCK', "the value doesn't even matter" |
|
1312 |
+ ) |
|
1313 |
+ monkeypatch.delattr(socket, 'AF_UNIX', raising=False) |
|
1314 |
+ _result = runner.invoke( |
|
1315 |
+ cli.derivepassphrase_vault, |
|
1316 |
+ ['--key', '--config'], |
|
1317 |
+ catch_exceptions=False, |
|
1318 |
+ ) |
|
1319 |
+ result = tests.ReadableResult.parse(_result) |
|
1320 |
+ assert result.error_exit( |
|
1321 |
+ error='does not support UNIX domain sockets' |
|
1322 |
+ ), 'expected error exit and known error message' |
|
1323 |
+ |
|
1298 | 1324 |
|
1299 | 1325 |
class TestCLIUtils: |
1300 | 1326 |
@pytest.mark.parametrize( |
... | ... |
@@ -104,14 +104,16 @@ class TestStaticFunctionality: |
104 | 104 |
tests.parse_sh_export_line(line, env_name=env_name) |
105 | 105 |
|
106 | 106 |
def test_200_constructor_no_running_agent( |
107 |
- self, monkeypatch: pytest.MonkeyPatch |
|
107 |
+ self, |
|
108 |
+ monkeypatch: pytest.MonkeyPatch, |
|
109 |
+ skip_if_no_af_unix_support: None, |
|
108 | 110 |
) -> None: |
111 |
+ del skip_if_no_af_unix_support |
|
109 | 112 |
monkeypatch.delenv('SSH_AUTH_SOCK', raising=False) |
110 |
- sock = socket.socket(family=socket.AF_UNIX) |
|
111 | 113 |
with pytest.raises( |
112 | 114 |
KeyError, match='SSH_AUTH_SOCK environment variable' |
113 | 115 |
): |
114 |
- ssh_agent.SSHAgentClient(socket=sock) |
|
116 |
+ ssh_agent.SSHAgentClient() |
|
115 | 117 |
|
116 | 118 |
@pytest.mark.parametrize( |
117 | 119 |
['input', 'expected'], |
... | ... |
@@ -352,6 +354,19 @@ class TestAgentInteraction: |
352 | 354 |
with pytest.raises(OSError): # noqa: PT011 |
353 | 355 |
ssh_agent.SSHAgentClient(socket=sock) |
354 | 356 |
|
357 |
+ def test_301_constructor_no_af_unix_support( |
|
358 |
+ self, |
|
359 |
+ monkeypatch: pytest.MonkeyPatch, |
|
360 |
+ ) -> None: |
|
361 |
+ with monkeypatch.context() as monkeypatch2: |
|
362 |
+ monkeypatch2.setenv('SSH_AUTH_SOCK', "the value doesn't matter") |
|
363 |
+ monkeypatch2.delattr(socket, 'AF_UNIX', raising=False) |
|
364 |
+ with pytest.raises( |
|
365 |
+ NotImplementedError, |
|
366 |
+ match='UNIX domain sockets', |
|
367 |
+ ): |
|
368 |
+ ssh_agent.SSHAgentClient() |
|
369 |
+ |
|
355 | 370 |
@pytest.mark.parametrize( |
356 | 371 |
'response', |
357 | 372 |
[ |
358 | 373 |