Marco Ricci commited on 2024-12-09 11:30:59
Zeige 1 geänderte Dateien mit 35 Einfügungen und 146 Löschungen.
Pageant 0.82, released two weeks ago, supports both proper output buffer flushing and a "foreground" mode. Foreground mode is used to correctly isolate a spawned Pageant instance in the test suite. The test for Pageant's suitability can now be simplified to whether it is at least version 0.82, or not. Furthermore, because no published version of Pageant supports only proper output buffer flushing but not foreground mode, it is no longer necessary to arrange for debug output to be silently discarded. This further simplifies SSH agent handling considerably. Finally, we fix various mistakes in the documentation of the testing machinery.
... | ... |
@@ -11,8 +11,6 @@ import os |
11 | 11 |
import shutil |
12 | 12 |
import socket |
13 | 13 |
import subprocess |
14 |
-import sys |
|
15 |
-import textwrap |
|
16 | 14 |
from typing import TYPE_CHECKING, TypeVar |
17 | 15 |
|
18 | 16 |
import hypothesis |
... | ... |
@@ -24,7 +22,6 @@ from derivepassphrase import _types, ssh_agent |
24 | 22 |
|
25 | 23 |
if TYPE_CHECKING: |
26 | 24 |
from collections.abc import Iterator |
27 |
- from typing import Literal |
|
28 | 25 |
|
29 | 26 |
startup_ssh_auth_sock = os.environ.get('SSH_AUTH_SOCK', None) |
30 | 27 |
|
... | ... |
@@ -68,12 +65,12 @@ def skip_if_no_af_unix_support() -> None: # pragma: no cover |
68 | 65 |
|
69 | 66 |
def _spawn_pageant( # pragma: no cover |
70 | 67 |
executable: str | None, env: dict[str, str] |
71 |
-) -> tuple[subprocess.Popen[str], bool] | None: |
|
68 |
+) -> subprocess.Popen[str] | None: |
|
72 | 69 |
"""Spawn an isolated Pageant, if possible. |
73 | 70 |
|
74 | 71 |
We attempt to detect whether Pageant is usable, i.e. whether Pageant |
75 | 72 |
has output buffering problems when announcing its authentication |
76 |
- socket. |
|
73 |
+ socket. This is the case for Pageant 0.81 and earlier. |
|
77 | 74 |
|
78 | 75 |
Args: |
79 | 76 |
executable: |
... | ... |
@@ -83,36 +80,29 @@ def _spawn_pageant( # pragma: no cover |
83 | 80 |
include an SSH_AUTH_SOCK variable. |
84 | 81 |
|
85 | 82 |
Returns: |
86 |
- (tuple[subprocess.Popen, bool] | None): |
|
87 |
- A 2-tuple `(proc, debug_output)`, where `proc` is the spawned |
|
88 |
- Pageant subprocess, and `debug_output` indicates whether Pageant |
|
89 |
- will continue to emit debug output (that needs to be actively |
|
90 |
- read) or not. |
|
83 |
+ The spawned Pageant subprocess. If the executable is `None`, or |
|
84 |
+ if we detect that Pageant cannot be sensibly controlled as |
|
85 |
+ a subprocess, then return `None` directly. |
|
91 | 86 |
|
92 | 87 |
It is the caller's responsibility to clean up the spawned |
93 | 88 |
subprocess. |
94 | 89 |
|
95 |
- If the executable is `None`, or if we detect that Pageant is too |
|
96 |
- old to properly flush its output, which prevents readers from |
|
97 |
- learning the SSH_AUTH_SOCK setting needed to connect to Pageant |
|
98 |
- in the first place, then return `None` directly. |
|
99 |
- |
|
100 | 90 |
""" |
101 | 91 |
if executable is None: # pragma: no cover |
102 | 92 |
return None |
103 | 93 |
|
104 |
- pageant_features = {'flush': False, 'foreground': False} |
|
105 |
- |
|
106 | 94 |
# Apparently, Pageant 0.81 and lower running in debug mode does |
107 | 95 |
# not actively flush its output. As a result, the first two |
108 | 96 |
# lines, which set the SSH_AUTH_SOCK and the SSH_AGENT_PID, only |
109 | 97 |
# print once the output buffer is flushed, whenever that is. |
110 | 98 |
# |
111 |
- # This has been reported to the PuTTY developers. |
|
112 |
- # |
|
113 |
- # For testing purposes, I currently build a version of Pageant with |
|
114 |
- # output flushing fixed and with a `--foreground` option. This is |
|
115 |
- # detected here. |
|
99 |
+ # This has been reported to the PuTTY developers. It is fixed in |
|
100 |
+ # version 0.82, though the PuTTY developers consider this to be an |
|
101 |
+ # abuse of debug mode. A new foreground mode (`--foreground`), also |
|
102 |
+ # introduced in 0.82, provides the desired behavior: no forking, and |
|
103 |
+ # immediately parsable instructions for SSH_AUTH_SOCK and |
|
104 |
+ # SSH_AGENT_PID. |
|
105 |
+ |
|
116 | 106 |
help_output = subprocess.run( |
117 | 107 |
['pageant', '--help'], |
118 | 108 |
executable=executable, |
... | ... |
@@ -127,44 +117,14 @@ def _spawn_pageant( # pragma: no cover |
127 | 117 |
if len(help_lines) >= 2 |
128 | 118 |
else '' |
129 | 119 |
) |
130 |
- v0_81 = packaging.version.Version('0.81') |
|
131 |
- if pageant_version_string not in {'', 'Unidentified build'}: |
|
132 |
- # TODO(the-13th-letter): Once a fixed Pageant is released, |
|
133 |
- # remove the check for build information in the version string. |
|
134 |
- # https://github.com/the-13th-letter/derivepassphrase/issues/14 |
|
135 |
- pageant_version_string_numeric, local_segment_list = ( |
|
136 |
- pageant_version_string.split('+', 1) |
|
137 |
- if '+' in pageant_version_string |
|
138 |
- else (pageant_version_string, '') |
|
139 |
- ) |
|
140 |
- local_segments = frozenset(local_segment_list.split('+')) |
|
141 |
- pageant_version = packaging.version.Version( |
|
142 |
- pageant_version_string_numeric |
|
143 |
- ) |
|
144 |
- for key in pageant_features: |
|
145 |
- pageant_features[key] = pageant_version > v0_81 or ( |
|
146 |
- pageant_version == v0_81 and key in local_segments |
|
147 |
- ) |
|
120 |
+ v0_82 = packaging.version.Version('0.82') |
|
121 |
+ pageant_version = packaging.version.Version(pageant_version_string) |
|
148 | 122 |
|
149 |
- if not pageant_features['flush']: # pragma: no cover |
|
123 |
+ if pageant_version < v0_82: # pragma: no cover |
|
150 | 124 |
return None |
151 | 125 |
|
152 |
- # Because Pageant's debug mode prints debugging information on |
|
153 |
- # standard output, and because we yield control to a different |
|
154 |
- # thread of execution, we cannot read-and-discard Pageant's output |
|
155 |
- # here. Instead, spawn a consumer process and connect it to |
|
156 |
- # Pageant's standard output; see _spawn_data_sink. |
|
157 |
- # |
|
158 |
- # This will hopefully not be necessary with newer Pageants: |
|
159 |
- # a feature request for a `--foreground` option that just avoids the |
|
160 |
- # forking behavior has been submitted. |
|
161 |
- |
|
162 | 126 |
return subprocess.Popen( |
163 |
- [ |
|
164 |
- 'pageant', |
|
165 |
- '--foreground' if pageant_features['foreground'] else '--debug', |
|
166 |
- '-s', |
|
167 |
- ], |
|
127 |
+ ['pageant', '--foreground', '-s'], |
|
168 | 128 |
executable=executable, |
169 | 129 |
stdin=subprocess.DEVNULL, |
170 | 130 |
stdout=subprocess.PIPE, |
... | ... |
@@ -172,18 +132,14 @@ def _spawn_pageant( # pragma: no cover |
172 | 132 |
env=env, |
173 | 133 |
text=True, |
174 | 134 |
bufsize=1, |
175 |
- ), not pageant_features['foreground'] |
|
135 |
+ ) |
|
176 | 136 |
|
177 | 137 |
|
178 | 138 |
def _spawn_openssh_agent( # pragma: no cover |
179 | 139 |
executable: str | None, env: dict[str, str] |
180 |
-) -> tuple[subprocess.Popen[str], Literal[False]] | None: |
|
140 |
+) -> subprocess.Popen[str] | None: |
|
181 | 141 |
"""Spawn an isolated OpenSSH agent, if possible. |
182 | 142 |
|
183 |
- We attempt to detect whether Pageant is usable, i.e. whether Pageant |
|
184 |
- has output buffering problems when announcing its authentication |
|
185 |
- socket. |
|
186 |
- |
|
187 | 143 |
Args: |
188 | 144 |
executable: |
189 | 145 |
The path to the OpenSSH agent executable. |
... | ... |
@@ -192,17 +148,12 @@ def _spawn_openssh_agent( # pragma: no cover |
192 | 148 |
not include an SSH_AUTH_SOCK variable. |
193 | 149 |
|
194 | 150 |
Returns: |
195 |
- (tuple[subprocess.Popen, Literal[False]] | None): |
|
196 |
- A 2-tuple `(proc, debug_output)`, where `proc` is the spawned |
|
197 |
- OpenSSH agent subprocess, and `debug_output` indicates whether |
|
198 |
- the OpenSSH agent will continue to emit debug output that needs |
|
199 |
- to be actively read (which it doesn't, so this is always false). |
|
151 |
+ The spawned OpenSSH agent subprocess. If the executable is |
|
152 |
+ `None`, then return `None` directly. |
|
200 | 153 |
|
201 | 154 |
It is the caller's responsibility to clean up the spawned |
202 | 155 |
subprocess. |
203 | 156 |
|
204 |
- If the executable is `None`, then return `None` directly. |
|
205 |
- |
|
206 | 157 |
""" |
207 | 158 |
if executable is None: |
208 | 159 |
return None |
... | ... |
@@ -215,7 +166,7 @@ def _spawn_openssh_agent( # pragma: no cover |
215 | 166 |
env=env, |
216 | 167 |
text=True, |
217 | 168 |
bufsize=1, |
218 |
- ), False |
|
169 |
+ ) |
|
219 | 170 |
|
220 | 171 |
|
221 | 172 |
def _spawn_system_agent( # pragma: no cover |
... | ... |
@@ -245,11 +196,10 @@ def running_ssh_agent( # pragma: no cover |
245 | 196 |
can it guarantee a particular set of loaded keys. |
246 | 197 |
|
247 | 198 |
Yields: |
248 |
- : |
|
249 |
- A 2-tuple `(ssh_auth_sock, agent_type)`, where |
|
250 |
- `ssh_auth_sock` is the value of the `SSH_AUTH_SOCK` |
|
251 |
- environment variable, to be used to connect to the running |
|
252 |
- agent, and `agent_type` is the agent type. |
|
199 |
+ A 2-tuple `(ssh_auth_sock, agent_type)`, where `ssh_auth_sock` |
|
200 |
+ is the value of the `SSH_AUTH_SOCK` environment variable, to be |
|
201 |
+ used to connect to the running agent, and `agent_type` is the |
|
202 |
+ agent type. |
|
253 | 203 |
|
254 | 204 |
Raises: |
255 | 205 |
pytest.skip.Exception: |
... | ... |
@@ -305,14 +255,9 @@ def running_ssh_agent( # pragma: no cover |
305 | 255 |
os.environ.get('SSH_AUTH_SOCK', None) |
306 | 256 |
== startup_ssh_auth_sock |
307 | 257 |
), f'SSH_AUTH_SOCK mismatch when checking for spawnable {exec_name}' # noqa: E501 |
308 |
- spawn_data = spawn_func( # type: ignore[operator] |
|
309 |
- executable=shutil.which(exec_name), env={} |
|
310 |
- ) |
|
311 |
- if spawn_data is None: |
|
258 |
+ proc = spawn_func(executable=shutil.which(exec_name), env={}) |
|
259 |
+ if proc is None: |
|
312 | 260 |
continue |
313 |
- proc: subprocess.Popen[str] |
|
314 |
- emits_debug_output: bool |
|
315 |
- proc, emits_debug_output = spawn_data |
|
316 | 261 |
with exit_stack: |
317 | 262 |
exit_stack.enter_context(terminate_on_exit(proc)) |
318 | 263 |
assert ( |
... | ... |
@@ -333,15 +278,6 @@ def running_ssh_agent( # pragma: no cover |
333 | 278 |
and '_pid' not in pid_line.lower() |
334 | 279 |
): # pragma: no cover |
335 | 280 |
pytest.skip(f'Cannot parse agent output: {pid_line!r}') |
336 |
- proc2 = _spawn_data_sink( |
|
337 |
- emits_debug_output=emits_debug_output, proc=proc |
|
338 |
- ) |
|
339 |
- if proc2 is not None: # pragma: no cover |
|
340 |
- exit_stack.enter_context(terminate_on_exit(proc2)) |
|
341 |
- assert ( |
|
342 |
- os.environ.get('SSH_AUTH_SOCK', None) |
|
343 |
- == startup_ssh_auth_sock |
|
344 |
- ), f'SSH_AUTH_SOCK mismatch after spawning {exec_name} helper' # noqa: E501 |
|
345 | 281 |
monkeypatch2 = exit_stack.enter_context( |
346 | 282 |
pytest.MonkeyPatch.context() |
347 | 283 |
) |
... | ... |
@@ -355,43 +291,8 @@ def running_ssh_agent( # pragma: no cover |
355 | 291 |
pytest.skip('No SSH agent running or spawnable') |
356 | 292 |
|
357 | 293 |
|
358 |
-def _spawn_data_sink( # pragma: no cover |
|
359 |
- emits_debug_output: bool, *, proc: subprocess.Popen[str] |
|
360 |
-) -> subprocess.Popen[str] | None: |
|
361 |
- """Spawn a data sink to read and discard standard input. |
|
362 |
- |
|
363 |
- Necessary for certain SSH agents that emit copious debugging output. |
|
364 |
- |
|
365 |
- On UNIX, we can use `cat`, redirected to `/dev/null`. Otherwise, |
|
366 |
- the most robust thing to do is to spawn Python and repeatedly call |
|
367 |
- `.read()` on `sys.stdin.buffer`. |
|
368 |
- |
|
369 |
- """ |
|
370 |
- if not emits_debug_output: |
|
371 |
- return None |
|
372 |
- if proc.stdout is None: |
|
373 |
- return None |
|
374 |
- sink_script = textwrap.dedent(""" |
|
375 |
- import sys |
|
376 |
- while sys.stdin.buffer.read(4096): |
|
377 |
- pass |
|
378 |
- """) |
|
379 |
- return subprocess.Popen( |
|
380 |
- ( |
|
381 |
- ['cat'] |
|
382 |
- if os.name == 'posix' |
|
383 |
- else [sys.executable or 'python3', '-c', sink_script] |
|
384 |
- ), |
|
385 |
- executable=sys.executable or None, |
|
386 |
- stdin=proc.stdout.fileno(), |
|
387 |
- stdout=subprocess.DEVNULL, |
|
388 |
- shell=False, |
|
389 |
- text=True, |
|
390 |
- ) |
|
391 |
- |
|
392 |
- |
|
393 | 294 |
@pytest.fixture(params=_spawn_handlers, ids=operator.itemgetter(0)) |
394 |
-def spawn_ssh_agent( # noqa: C901 |
|
295 |
+def spawn_ssh_agent( |
|
395 | 296 |
request: pytest.FixtureRequest, |
396 | 297 |
skip_if_no_af_unix_support: None, |
397 | 298 |
) -> Iterator[tests.SpawnedSSHAgentInfo]: |
... | ... |
@@ -402,11 +303,10 @@ def spawn_ssh_agent( # noqa: C901 |
402 | 303 |
PuTTY's Pageant, and the "(system)" fallback agent. |
403 | 304 |
|
404 | 305 |
Yields: |
405 |
- (tests.SpawnedSSHAgentInfo): |
|
406 |
- A [named tuple][collections.namedtuple] containing |
|
407 |
- information about the spawned agent, e.g. the software |
|
408 |
- product, a client connected to the agent, and whether the |
|
409 |
- agent is isolated from other clients. |
|
306 |
+ A [named tuple][collections.namedtuple] containing information |
|
307 |
+ about the spawned agent, e.g. the software product, a client |
|
308 |
+ connected to the agent, and whether the agent is isolated from |
|
309 |
+ other clients. |
|
410 | 310 |
|
411 | 311 |
Raises: |
412 | 312 |
pytest.skip.Exception: |
... | ... |
@@ -471,15 +371,14 @@ def spawn_ssh_agent( # noqa: C901 |
471 | 371 |
assert ( |
472 | 372 |
os.environ.get('SSH_AUTH_SOCK', None) == startup_ssh_auth_sock |
473 | 373 |
), f'SSH_AUTH_SOCK mismatch when checking for spawnable {exec_name}' # noqa: E501 |
474 |
- spawn_data = spawn_func( |
|
374 |
+ proc = spawn_func( |
|
475 | 375 |
executable=shutil.which(exec_name), env=agent_env |
476 | 376 |
) |
477 | 377 |
assert ( |
478 | 378 |
os.environ.get('SSH_AUTH_SOCK', None) == startup_ssh_auth_sock |
479 | 379 |
), f'SSH_AUTH_SOCK mismatch after spawning {exec_name}' |
480 |
- if spawn_data is None: # pragma: no cover |
|
380 |
+ if proc is None: # pragma: no cover |
|
481 | 381 |
pytest.skip(f'Cannot spawn usable {exec_name}') |
482 |
- proc, emits_debug_output = spawn_data |
|
483 | 382 |
with exit_stack: |
484 | 383 |
exit_stack.enter_context(terminate_on_exit(proc)) |
485 | 384 |
assert proc.stdout is not None |
... | ... |
@@ -502,15 +401,6 @@ def spawn_ssh_agent( # noqa: C901 |
502 | 401 |
os.environ.get('SSH_AUTH_SOCK', None) |
503 | 402 |
== startup_ssh_auth_sock |
504 | 403 |
), f'SSH_AUTH_SOCK mismatch before spawning {exec_name} helper' |
505 |
- proc2 = _spawn_data_sink( |
|
506 |
- emits_debug_output=emits_debug_output, proc=proc |
|
507 |
- ) |
|
508 |
- if proc2 is not None: # pragma: no cover |
|
509 |
- exit_stack.enter_context(terminate_on_exit(proc2)) |
|
510 |
- assert ( |
|
511 |
- os.environ.get('SSH_AUTH_SOCK', None) |
|
512 |
- == startup_ssh_auth_sock |
|
513 |
- ), f'SSH_AUTH_SOCK mismatch after spawning {exec_name} helper' |
|
514 | 404 |
monkeypatch2 = exit_stack.enter_context( |
515 | 405 |
pytest.MonkeyPatch.context() |
516 | 406 |
) |
... | ... |
@@ -546,7 +436,6 @@ def ssh_agent_client_with_test_keys_loaded( # noqa: C901 |
546 | 436 |
automatically assume any particular key is present in the agent. |
547 | 437 |
|
548 | 438 |
Yields: |
549 |
- (ssh_agent.SSHAgentClient): |
|
550 | 439 |
A [named tuple][collections.namedtuple] containing |
551 | 440 |
information about the spawned agent, e.g. the software |
552 | 441 |
product, a client connected to the agent, and whether the |
... | ... |
@@ -610,7 +499,7 @@ def ssh_agent_client_with_test_keys_loaded( # noqa: C901 |
610 | 499 |
# reasons: |
611 | 500 |
# |
612 | 501 |
# - Pageant refuses to accept a key it already holds |
613 |
- # in memory. Verify this by listing key |
|
502 |
+ # in memory. Verify this by listing keys. |
|
614 | 503 |
# - Pageant does not support key constraints (see |
615 | 504 |
# references below). |
616 | 505 |
# |
617 | 506 |