Marco Ricci commited on 2024-11-26 23:28:43
Zeige 4 geänderte Dateien mit 98 Einfügungen und 42 Löschungen.
The `has_deterministic_signatures` function internally only ever checked whether DSA signatures were known deterministic, because currently, signature schemes are either deterministic by design or they are DSA-like and can be derandomized via RFC 6979 or a similar procedure. There's no guarantee this dichotomy will stay this way in the future. Thus it is better to rename the function to match what it actually tests: Does this agent use deterministic DSA and ECDSA signatures? We do just that. In a similar vein, the `Vault._is_suitable_ssh_key` only really checks if the key type is known deterministic, not whether the key is suitable; the latter depends on the SSH agent, and requires a call to the old `has_deterministic_signatures` function. We could of course analogously rename `_is_suitable_ssh_key` into `is_known_deterministic_key_type` or similar, but this feels too much like exposing implementation details to the API user. It seems better to expose a `Vault.is_suitable_ssh_key` method that actually does what it advertises: check whether a key type is known deterministic under a given SSH agent, or under all SSH agents in general. So we do just that. Finally, we clean up some inconsistencies in the `query_extensions` docstring, and some missing SSH agent clients not passed on to the calls to the `Vault.phrase_from_key` function in the tests.
| ... | ... |
@@ -511,7 +511,6 @@ def _get_suitable_ssh_keys( |
| 511 | 511 |
|
| 512 | 512 |
""" |
| 513 | 513 |
with ssh_agent.SSHAgentClient.ensure_agent_subcontext(conn) as client: |
| 514 |
- has_deterministic_signatures = client.has_deterministic_signatures() |
|
| 515 | 514 |
try: |
| 516 | 515 |
all_key_comment_pairs = list(client.list_keys()) |
| 517 | 516 |
except EOFError as e: # pragma: no cover |
| ... | ... |
@@ -519,10 +518,7 @@ def _get_suitable_ssh_keys( |
| 519 | 518 |
suitable_keys = copy.copy(all_key_comment_pairs) |
| 520 | 519 |
for pair in all_key_comment_pairs: |
| 521 | 520 |
key, _comment = pair |
| 522 |
- if ( |
|
| 523 |
- has_deterministic_signatures |
|
| 524 |
- or vault.Vault._is_suitable_ssh_key(key) # noqa: SLF001 |
|
| 525 |
- ): |
|
| 521 |
+ if vault.Vault.is_suitable_ssh_key(key, client=client): |
|
| 526 | 522 |
yield pair |
| 527 | 523 |
if not suitable_keys: # pragma: no cover |
| 528 | 524 |
raise LookupError(_NO_USABLE_KEYS) |
| ... | ... |
@@ -351,12 +351,19 @@ class SSHAgentClient: |
| 351 | 351 |
in self.query_extensions() |
| 352 | 352 |
) |
| 353 | 353 |
|
| 354 |
- def has_deterministic_signatures(self) -> bool: |
|
| 355 |
- """Check whether the agent returns deterministic signatures. |
|
| 354 |
+ def has_deterministic_dsa_signatures(self) -> bool: |
|
| 355 |
+ """Check whether the agent returns deterministic DSA signatures. |
|
| 356 |
+ |
|
| 357 |
+ This includes ECDSA signatures. |
|
| 358 |
+ |
|
| 359 |
+ Generally, this means that the SSH agent implements [RFC 6979][] |
|
| 360 |
+ or a similar system. |
|
| 361 |
+ |
|
| 362 |
+ [RFC 6979]: https://www.rfc-editor.org/rfc/rfc6979 |
|
| 356 | 363 |
|
| 357 | 364 |
Returns: |
| 358 | 365 |
True if a known agent was detected where signatures are |
| 359 |
- deterministic for all SSH key types, false otherwise. |
|
| 366 |
+ deterministic for all DSA key types, false otherwise. |
|
| 360 | 367 |
|
| 361 | 368 |
Note: Known agents with deterministic signatures |
| 362 | 369 |
| agent | detected via | |
| ... | ... |
@@ -594,24 +601,27 @@ class SSHAgentClient: |
| 594 | 601 |
) |
| 595 | 602 |
|
| 596 | 603 |
def query_extensions(self) -> AbstractSet[bytes]: |
| 597 |
- """Request a list of extensions supported by the SSH agent. |
|
| 598 |
- |
|
| 599 |
- Args: |
|
| 600 |
- raise_if_no_extension_support: |
|
| 601 |
- If true, and if the agent does not support querying |
|
| 602 |
- extensions, then raise an error. If false, silently |
|
| 603 |
- return an empty result. |
|
| 604 |
+ """Request a listing of extensions supported by the SSH agent. |
|
| 604 | 605 |
|
| 605 | 606 |
Returns: |
| 606 |
- A read-only sequence of extension names. |
|
| 607 |
+ A read-only set of extension names the SSH agent says it |
|
| 608 |
+ supports. |
|
| 607 | 609 |
|
| 608 | 610 |
Raises: |
| 609 | 611 |
EOFError: |
| 610 | 612 |
The response from the SSH agent is truncated or missing. |
| 611 | 613 |
OSError: |
| 612 | 614 |
There was a communication error with the SSH agent. |
| 613 |
- SSHAgentFailedError: |
|
| 614 |
- The agent failed to complete the request. |
|
| 615 |
+ RuntimeError: |
|
| 616 |
+ The response from the SSH agent is malformed. |
|
| 617 |
+ |
|
| 618 |
+ Note: |
|
| 619 |
+ The set of supported extensions is queried via the `query` |
|
| 620 |
+ extension request. If the agent does not support the query |
|
| 621 |
+ extension request, or extension requests in general, then an |
|
| 622 |
+ empty set is returned. This does not however imply that the |
|
| 623 |
+ agent doesn't support *any* extension request... merely that |
|
| 624 |
+ it doesn't support extension autodiscovery. |
|
| 615 | 625 |
|
| 616 | 626 |
""" |
| 617 | 627 |
try: |
| ... | ... |
@@ -628,9 +638,19 @@ class SSHAgentClient: |
| 628 | 638 |
# This isn't necessarily true, e.g. for OpenSSH's ssh-agent. |
| 629 | 639 |
return frozenset() |
| 630 | 640 |
extensions: set[bytes] = set() |
| 641 |
+ msg = 'Malformed response from SSH agent' |
|
| 642 |
+ msg2 = 'Extension response message does not match request' |
|
| 643 |
+ try: |
|
| 631 | 644 |
_query, response_data = self.unstring_prefix(response_data) |
| 632 |
- assert bytes(_query) == b'query' |
|
| 645 |
+ except ValueError as e: |
|
| 646 |
+ raise RuntimeError(msg) from e |
|
| 647 |
+ if bytes(_query) != b'query': |
|
| 648 |
+ raise RuntimeError(msg2) |
|
| 633 | 649 |
while response_data: |
| 650 |
+ try: |
|
| 634 | 651 |
extension, response_data = self.unstring_prefix(response_data) |
| 652 |
+ except ValueError as e: |
|
| 653 |
+ raise RuntimeError(msg) from e |
|
| 654 |
+ else: |
|
| 635 | 655 |
extensions.add(bytes(extension)) |
| 636 | 656 |
return frozenset(extensions) |
| ... | ... |
@@ -447,23 +447,30 @@ class Vault: |
| 447 | 447 |
return bytes(result) |
| 448 | 448 |
|
| 449 | 449 |
@staticmethod |
| 450 |
- def _is_suitable_ssh_key(key: bytes | bytearray, /) -> bool: |
|
| 450 |
+ def is_suitable_ssh_key( |
|
| 451 |
+ key: bytes | bytearray, |
|
| 452 |
+ /, |
|
| 453 |
+ *, |
|
| 454 |
+ client: ssh_agent.SSHAgentClient | None = None, |
|
| 455 |
+ ) -> bool: |
|
| 451 | 456 |
"""Check whether the key is suitable for passphrase derivation. |
| 452 | 457 |
|
| 453 |
- Currently, this only statically checks whether signatures with |
|
| 454 |
- this key type are guaranteed to be deterministic. |
|
| 458 |
+ Some key types are guaranteed to be deterministic. Other keys |
|
| 459 |
+ types are only deterministic if the SSH agent supports this |
|
| 460 |
+ feature. |
|
| 455 | 461 |
|
| 456 | 462 |
Args: |
| 457 |
- key: SSH public key to check. |
|
| 463 |
+ key: |
|
| 464 |
+ SSH public key to check. |
|
| 465 |
+ client: |
|
| 466 |
+ An optional SSH agent client to check for additional |
|
| 467 |
+ deterministic key types. If not given, assume no such |
|
| 468 |
+ types. |
|
| 458 | 469 |
|
| 459 | 470 |
Returns: |
| 460 |
- True if and only if the key is guaranteed suitable for use in |
|
| 461 |
- deriving a passphrase deterministically. |
|
| 462 |
- |
|
| 463 |
- Note: |
|
| 464 |
- Some SSH agents additionally guarantee that all signatures |
|
| 465 |
- are deterministic and thus *all* keys are suitable; see |
|
| 466 |
- [`ssh_agent.SSHAgentClient.has_deterministic_signatures`][]. |
|
| 471 |
+ True if and only if the key is guaranteed suitable for use |
|
| 472 |
+ in deriving a passphrase deterministically (perhaps |
|
| 473 |
+ restricted to the indicated SSH agent). |
|
| 467 | 474 |
|
| 468 | 475 |
""" |
| 469 | 476 |
TestFunc: TypeAlias = 'Callable[[bytes | bytearray], bool]' |
| ... | ... |
@@ -475,7 +482,31 @@ class Vault: |
| 475 | 482 |
'ssh-ed448': lambda k: k.startswith(b'\x00\x00\x00\x09ssh-ed448'), |
| 476 | 483 |
'ssh-rsa': lambda k: k.startswith(b'\x00\x00\x00\x07ssh-rsa'), |
| 477 | 484 |
} |
| 478 |
- return any(v(key) for v in deterministic_signature_types.values()) |
|
| 485 |
+ dsa_signature_types = {
|
|
| 486 |
+ 'ssh-dss': lambda k: k.startswith(b'\x00\x00\x00\x07ssh-dss'), |
|
| 487 |
+ 'ecdsa-sha2-nistp256': lambda k: k.startswith( |
|
| 488 |
+ b'\x00\x00\x00\x13ecdsa-sha2-nistp256' |
|
| 489 |
+ ), |
|
| 490 |
+ 'ecdsa-sha2-nistp384': lambda k: k.startswith( |
|
| 491 |
+ b'\x00\x00\x00\x13ecdsa-sha2-nistp384' |
|
| 492 |
+ ), |
|
| 493 |
+ 'ecdsa-sha2-nistp521': lambda k: k.startswith( |
|
| 494 |
+ b'\x00\x00\x00\x13ecdsa-sha2-nistp521' |
|
| 495 |
+ ), |
|
| 496 |
+ } |
|
| 497 |
+ criteria = [ |
|
| 498 |
+ lambda: any( |
|
| 499 |
+ v(key) for v in deterministic_signature_types.values() |
|
| 500 |
+ ), |
|
| 501 |
+ ] |
|
| 502 |
+ if client is not None: |
|
| 503 |
+ criteria.append( |
|
| 504 |
+ lambda: ( |
|
| 505 |
+ client.has_deterministic_dsa_signatures() |
|
| 506 |
+ and any(v(key) for v in dsa_signature_types.values()) |
|
| 507 |
+ ) |
|
| 508 |
+ ) |
|
| 509 |
+ return any(crit() for crit in criteria) |
|
| 479 | 510 |
|
| 480 | 511 |
@classmethod |
| 481 | 512 |
def phrase_from_key( |
| ... | ... |
@@ -489,8 +520,8 @@ class Vault: |
| 489 | 520 |
|
| 490 | 521 |
vault allows the usage of certain SSH keys to derive a master |
| 491 | 522 |
passphrase, by signing the vault UUID with the SSH key. The key |
| 492 |
- type or the SSH agent must ensure that signatures are |
|
| 493 |
- deterministic. |
|
| 523 |
+ type must ensure that signatures are deterministic (perhaps only |
|
| 524 |
+ in conjunction with the given SSH agent). |
|
| 494 | 525 |
|
| 495 | 526 |
Args: |
| 496 | 527 |
key: |
| ... | ... |
@@ -544,10 +575,7 @@ class Vault: |
| 544 | 575 |
|
| 545 | 576 |
""" |
| 546 | 577 |
with ssh_agent.SSHAgentClient.ensure_agent_subcontext(conn) as client: |
| 547 |
- if not ( |
|
| 548 |
- client.has_deterministic_signatures() |
|
| 549 |
- or cls._is_suitable_ssh_key(key) |
|
| 550 |
- ): |
|
| 578 |
+ if not cls.is_suitable_ssh_key(key, client=client): |
|
| 551 | 579 |
msg = ( |
| 552 | 580 |
'unsuitable SSH key: bad key, or ' |
| 553 | 581 |
'signature not deterministic under this agent' |
| ... | ... |
@@ -256,7 +256,8 @@ class TestAgentInteraction: |
| 256 | 256 |
) |
| 257 | 257 |
assert signature2 == expected_signature, 'SSH signature mismatch' |
| 258 | 258 |
assert ( |
| 259 |
- vault.Vault.phrase_from_key(public_key_data) == derived_passphrase |
|
| 259 |
+ vault.Vault.phrase_from_key(public_key_data, conn=client) |
|
| 260 |
+ == derived_passphrase |
|
| 260 | 261 |
), 'SSH signature mismatch' |
| 261 | 262 |
|
| 262 | 263 |
@pytest.mark.parametrize( |
| ... | ... |
@@ -275,10 +276,13 @@ class TestAgentInteraction: |
| 275 | 276 |
_ = data_dict['expected_signature'] |
| 276 | 277 |
if public_key_data not in key_comment_pairs: # pragma: no cover |
| 277 | 278 |
pytest.skip('prerequisite SSH key not loaded')
|
| 278 |
- if client.has_deterministic_signatures(): |
|
| 279 |
- pytest.skip('agent ensures all keys are suitable')
|
|
| 279 |
+ assert not vault.Vault.is_suitable_ssh_key( |
|
| 280 |
+ public_key_data, client=None |
|
| 281 |
+ ), 'Expected key to be unsuitable in general' |
|
| 282 |
+ if vault.Vault.is_suitable_ssh_key(public_key_data, client=client): |
|
| 283 |
+ pytest.skip('agent automatically ensures key is suitable')
|
|
| 280 | 284 |
with pytest.raises(ValueError, match='unsuitable SSH key'): |
| 281 |
- vault.Vault.phrase_from_key(public_key_data) |
|
| 285 |
+ vault.Vault.phrase_from_key(public_key_data, conn=client) |
|
| 282 | 286 |
|
| 283 | 287 |
@pytest.mark.parametrize( |
| 284 | 288 |
['key', 'single'], |
| ... | ... |
@@ -298,9 +302,17 @@ class TestAgentInteraction: |
| 298 | 302 |
client = ssh_agent_client_with_test_keys_loaded |
| 299 | 303 |
|
| 300 | 304 |
def key_is_suitable(key: bytes) -> bool: |
| 301 |
- return client.has_deterministic_signatures() or key in {
|
|
| 305 |
+ always = {
|
|
| 302 | 306 |
v['public_key_data'] for v in tests.SUPPORTED_KEYS.values() |
| 303 | 307 |
} |
| 308 |
+ dsa = {
|
|
| 309 |
+ v['public_key_data'] |
|
| 310 |
+ for k, v in tests.UNSUITABLE_KEYS.items() |
|
| 311 |
+ if k.startswith(('dsa', 'ecdsa'))
|
|
| 312 |
+ } |
|
| 313 |
+ return key in always or ( |
|
| 314 |
+ client.has_deterministic_dsa_signatures() and key in dsa |
|
| 315 |
+ ) |
|
| 304 | 316 |
|
| 305 | 317 |
if single: |
| 306 | 318 |
monkeypatch.setattr( |
| 307 | 319 |