Skip to content

Commit 511c318

Browse files
un-defclaude
andauthored
Fix dstack apply --ssh-identity (#4191)
* Report a proper error instead of a traceback if the key file does not exist or cannot be parsed * Generate the public key from the private key if there is no `.pub` file, as the API docstrings already claimed * Pass the key to `Run.attach()`, so that the run SSH config no longer falls back to `~/.dstack/ssh/id_rsa` * Reject public key paths, as SSH tunneling requires a private key Refactoring and related changes: * Add the `resolve_ssh_key()` helper that resolves a private or public key path to key contents and paths, and reuse it in the fleet configurator, which had the same logic inlined * Look up `<key>.pub` instead of `<key stem>.pub`, which resolved to a wrong path for key names with dots * Add `ssh_key_pub` to `RunCollection.get_run_plan()` and `apply_configuration()` for public keys not stored on disk, mutually exclusive with `ssh_identity_file` * `dstack fleet`: raise `CLIError` instead of printing an error and exiting with a zero exit code Fixes: #4190 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 765fb31 commit 511c318

8 files changed

Lines changed: 433 additions & 31 deletions

File tree

src/dstack/_internal/cli/services/configurators/fleet.py

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838
from dstack._internal.utils.common import local_time
3939
from dstack._internal.utils.logging import get_logger
4040
from dstack._internal.utils.nested_list import NestedList, NestedListItem
41-
from dstack._internal.utils.ssh import convert_ssh_key_to_pem, generate_public_key, pkey_from_str
41+
from dstack._internal.utils.ssh import resolve_ssh_key
4242
from dstack.api.utils import load_profile
4343

4444
logger = get_logger(__name__)
@@ -354,22 +354,15 @@ def _preprocess_spec(spec: FleetSpec):
354354
def _resolve_ssh_key(ssh_key_path: Optional[str]) -> Optional[SSHKey]:
355355
if ssh_key_path is None:
356356
return None
357-
ssh_key_path_obj = Path(ssh_key_path).expanduser()
358357
try:
359-
private_key = convert_ssh_key_to_pem(ssh_key_path_obj.read_text())
360-
try:
361-
pub_key = ssh_key_path_obj.with_suffix(".pub").read_text()
362-
except FileNotFoundError:
363-
pub_key = generate_public_key(pkey_from_str(private_key))
364-
return SSHKey(public=pub_key, private=private_key)
358+
public_key, _, private_key, _ = resolve_ssh_key(ssh_key_path)
365359
except OSError as e:
366-
logger.debug("Got OSError: %s", repr(e))
367-
console.print(f"[error]Unable to read the SSH key at {ssh_key_path}[/]")
368-
exit()
360+
raise CLIError(f"Unable to read the SSH key at {ssh_key_path}") from e
369361
except ValueError as e:
370-
logger.debug("Key type is not supported", repr(e))
371-
console.print("[error]Key type is not supported[/]")
372-
exit()
362+
raise CLIError(f"Unsupported or invalid SSH key at {ssh_key_path}") from e
363+
if private_key is None:
364+
raise CLIError(f"Expected a private key at {ssh_key_path}, got a public key")
365+
return SSHKey(public=public_key, private=private_key)
373366

374367

375368
def _render_fleet_spec_diff(old_spec: FleetSpec, new_spec: FleetSpec) -> Optional[str]:

src/dstack/_internal/cli/services/configurators/run.py

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@
7575
from dstack._internal.utils.nested_list import NestedList, NestedListItem
7676
from dstack._internal.utils.nodes_interpolator import is_valid_groups_ip_ref
7777
from dstack._internal.utils.path import is_absolute_posix_path
78+
from dstack._internal.utils.ssh import resolve_ssh_key
7879
from dstack.api._public.runs import Run
7980

8081
_BIND_ADDRESS_ARG = "bind_address"
@@ -95,23 +96,27 @@ def apply_configuration(
9596
command_args: argparse.Namespace,
9697
configurator_args: argparse.Namespace,
9798
):
99+
ssh_key_pub, ssh_identity_file = self.get_ssh_key(configurator_args)
98100
run_plan, repo = self.get_plan(
99101
conf=conf,
100102
configuration_path=configuration_path,
101103
configurator_args=configurator_args,
104+
ssh_key_pub=ssh_key_pub,
102105
)
103106
return self.apply_plan(
104107
run_plan=run_plan,
105108
repo=repo,
106109
command_args=command_args,
107110
configurator_args=configurator_args,
111+
ssh_identity_file=ssh_identity_file,
108112
)
109113

110114
def get_plan(
111115
self,
112116
conf: RunConfigurationT,
113117
configuration_path: str,
114118
configurator_args: argparse.Namespace,
119+
ssh_key_pub: Optional[str],
115120
) -> tuple[RunPlan, Repo]:
116121
"""Apply CLI arguments and validation, then return the run plan and its repo."""
117122
if configurator_args.repo and configurator_args.no_repo:
@@ -132,7 +137,7 @@ def get_plan(
132137
repo=repo,
133138
configuration_path=configuration_path,
134139
profile=profile,
135-
ssh_identity_file=configurator_args.ssh_identity_file,
140+
ssh_key_pub=ssh_key_pub,
136141
max_offers=configurator_args.max_offers,
137142
full_offers=configurator_args.full_offers,
138143
unallocated_resources=configurator_args.unallocated,
@@ -145,6 +150,7 @@ def apply_plan(
145150
repo: Repo,
146151
command_args: argparse.Namespace,
147152
configurator_args: argparse.Namespace,
153+
ssh_identity_file: Optional[Path],
148154
plan_properties: Optional[Dict[str, str]] = None,
149155
):
150156
"""Apply a run plan using the standard CLI behavior."""
@@ -270,7 +276,10 @@ def apply_plan(
270276
)
271277
try:
272278
try:
273-
attached = run.attach(bind_address=bind_address)
279+
attached = run.attach(
280+
ssh_identity_file=ssh_identity_file,
281+
bind_address=bind_address,
282+
)
274283
except PortUsedError as e:
275284
console.print(
276285
f"[error]Failed to attach: port [code]{e.port}[/code] is already in use."
@@ -543,6 +552,23 @@ def get_repo(
543552

544553
return repo
545554

555+
def get_ssh_key(
556+
self, configurator_args: argparse.Namespace
557+
) -> tuple[Optional[str], Optional[Path]]:
558+
"""Resolve the `--ssh-identity` argument to a (public key, private key path) pair."""
559+
ssh_identity_file: Optional[Path] = configurator_args.ssh_identity_file
560+
if ssh_identity_file is None:
561+
return None, None
562+
try:
563+
public_key, _, _, private_key_path = resolve_ssh_key(ssh_identity_file)
564+
except OSError as e:
565+
raise CLIError(f"Unable to read the SSH key at {ssh_identity_file}") from e
566+
except ValueError as e:
567+
raise CLIError(f"Unsupported or invalid SSH key at {ssh_identity_file}") from e
568+
if private_key_path is None:
569+
raise CLIError(f"Expected a private key at {ssh_identity_file}, got a public key")
570+
return public_key, private_key_path
571+
546572

547573
class RunWithPortsConfiguratorMixin:
548574
@classmethod

src/dstack/_internal/utils/ssh.py

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import subprocess
66
import sys
77
import tempfile
8+
from contextlib import suppress
89
from pathlib import Path
910
from typing import Dict, Optional, Union
1011

@@ -268,6 +269,58 @@ def generate_public_key(private_key: PKey) -> str:
268269
return public_key
269270

270271

272+
def resolve_ssh_key(
273+
path: PathLike,
274+
) -> Union[
275+
tuple[str, Path, str, Path],
276+
tuple[str, None, str, Path],
277+
tuple[str, Path, None, None],
278+
]:
279+
"""
280+
Resolves a private or public key path to key contents and paths.
281+
282+
If a private key is given, only supported private key types are allowed. PKCS#8 keys are
283+
converted to PEM, so the returned private key may differ from the file contents. If a
284+
corresponding ".pub" file exists, its contents is used as a public key without any validation
285+
and its path is returned as the public key path, otherwise a public key is generated from the
286+
private key and the public key path is None.
287+
288+
If a public key is given, any valid public key is allowed regardless of its type, and both
289+
private key values are None. No corresponding private key (a file without ".pub" suffix) is
290+
checked.
291+
292+
Args:
293+
path: The private or public key path.
294+
295+
Returns:
296+
A (public key, public key path, private key, private key path) tuple.
297+
298+
Raises:
299+
OSError: Error reading key file(s).
300+
ValueError: Unsupported or invalid private key or invalid public key.
301+
"""
302+
path = Path(path).expanduser()
303+
content = path.read_text()
304+
private_key = convert_ssh_key_to_pem(content)
305+
pkey: Optional[PKey] = None
306+
with suppress(ValueError):
307+
pkey = pkey_from_str(private_key)
308+
if pkey is None:
309+
# unsupported private key or public key or garbage
310+
try:
311+
PublicBlob.from_string(content)
312+
except ValueError:
313+
# unsupported private key or garbage
314+
raise ValueError("Unsupported key type or invalid key")
315+
# any valid public key, including unsupported (without matching SUPPORTED_KEY_TYPES PKey)
316+
return content, path, None, None
317+
# supported private key
318+
public_key_path = path.with_name(path.name + ".pub")
319+
if public_key_path.is_file():
320+
return public_key_path.read_text(), public_key_path, private_key, path
321+
return generate_public_key(pkey), None, private_key, path
322+
323+
271324
def check_required_ssh_version() -> bool:
272325
try:
273326
result = subprocess.run(["ssh", "-V"], capture_output=True, text=True)

src/dstack/api/_public/runs.py

Lines changed: 35 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@
5050
from dstack._internal.utils.files import create_file_archive
5151
from dstack._internal.utils.logging import get_logger
5252
from dstack._internal.utils.path import PathLike
53+
from dstack._internal.utils.ssh import resolve_ssh_key
5354
from dstack.api.server import APIClient
5455

5556
logger = get_logger(__name__)
@@ -477,6 +478,7 @@ def get_run_plan(
477478
configuration_path: Optional[str] = None,
478479
repo_dir: Union[Deprecated, str, None] = Deprecated.PLACEHOLDER,
479480
ssh_identity_file: Optional[PathLike] = None,
481+
ssh_key_pub: Optional[str] = None,
480482
max_offers: Optional[int] = None,
481483
full_offers: bool = False,
482484
unallocated_resources: bool = False,
@@ -493,10 +495,15 @@ def get_run_plan(
493495
profile: The profile to use for the run.
494496
configuration_path: The path to the configuration file. Omit if the configuration
495497
is not loaded from a file.
496-
ssh_identity_file: Path to the private SSH key file. The corresponding public key
497-
(`.pub` file) is read and included in the run plan, allowing SSH access to the instances.
498-
If the `.pub` file does not exist, it is generated automatically.
499-
If ssh_identity_file is not specified, the user key is used.
498+
ssh_identity_file: Path to a private or public SSH key file. The public key is
499+
included in the run plan, allowing SSH access to the instances. If a private key
500+
is given, its public key is read from the corresponding `.pub` file or, if there
501+
is no such file, generated from the private key.
502+
Mutually exclusive with ssh_key_pub.
503+
ssh_key_pub: The public SSH key to include in the run plan, allowing SSH access to
504+
the instances. Use it instead of ssh_identity_file if the key is not stored
505+
on disk. Mutually exclusive with ssh_identity_file.
506+
If neither ssh_key_pub nor ssh_identity_file is specified, the user key is used.
500507
max_offers: Maximum number of offers returned in the run plan.
501508
full_offers: Return full offers not adjusted by requirements.
502509
unallocated_resources: Subtract allocated resources to return only unallocated
@@ -536,10 +543,20 @@ def get_run_plan(
536543
archive = self._api_client.files.upload_archive(hash=archive_hash, fp=fp)
537544
file_archives.append(FileArchiveMapping(id=archive.id, path=file_mapping.path))
538545

546+
if ssh_key_pub and ssh_identity_file:
547+
raise ConfigurationError("ssh_key_pub and ssh_identity_file are mutually exclusive")
539548
if ssh_identity_file:
540-
ssh_key_pub = Path(ssh_identity_file).with_suffix(".pub").read_text()
541-
else:
542-
ssh_key_pub = None # using the server-managed user key
549+
try:
550+
ssh_key_pub, _, _, _ = resolve_ssh_key(ssh_identity_file)
551+
except OSError as e:
552+
raise ConfigurationError(
553+
f"Unable to read the SSH key at {ssh_identity_file}"
554+
) from e
555+
except ValueError as e:
556+
raise ConfigurationError(
557+
f"Unsupported or invalid SSH key at {ssh_identity_file}"
558+
) from e
559+
# `ssh_key_pub` is None if neither is given: using the server-managed user key
543560
run_spec = RunSpec(
544561
run_name=configuration.name,
545562
repo_id=repo.repo_id,
@@ -609,6 +626,7 @@ def apply_configuration(
609626
configuration_path: Optional[str] = None,
610627
reserve_ports: bool = True,
611628
ssh_identity_file: Optional[PathLike] = None,
629+
ssh_key_pub: Optional[str] = None,
612630
) -> Run:
613631
"""
614632
Apply the run configuration.
@@ -621,10 +639,15 @@ def apply_configuration(
621639
profile: The profile to use for the run.
622640
configuration_path: The path to the configuration file. Omit if the configuration is not loaded from a file.
623641
reserve_ports: Reserve local ports before applying. Use if you'll attach to the run.
624-
ssh_identity_file: Path to the private SSH key file. The corresponding public key
625-
(`.pub` file) is read and included in the run plan, allowing SSH access to the instances.
626-
If the `.pub` file does not exist, it is generated automatically.
627-
If ssh_identity_file is not specified, the user key is used.
642+
ssh_identity_file: Path to a private or public SSH key file. The public key is
643+
included in the run plan, allowing SSH access to the instances. If a private key
644+
is given, its public key is read from the corresponding `.pub` file or, if there
645+
is no such file, generated from the private key.
646+
Mutually exclusive with ssh_key_pub.
647+
ssh_key_pub: The public SSH key to include in the run plan, allowing SSH access to
648+
the instances. Use it instead of ssh_identity_file if the key is not stored
649+
on disk. Mutually exclusive with ssh_identity_file.
650+
If neither ssh_key_pub nor ssh_identity_file is specified, the user key is used.
628651
629652
Returns:
630653
Submitted run.
@@ -635,6 +658,7 @@ def apply_configuration(
635658
profile=profile,
636659
configuration_path=configuration_path,
637660
ssh_identity_file=ssh_identity_file,
661+
ssh_key_pub=ssh_key_pub,
638662
)
639663
run = self.apply_plan(
640664
run_plan=run_plan,

src/tests/_internal/cli/services/configurators/test_fleet.py

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import argparse
22
from datetime import datetime, timezone
3+
from pathlib import Path
34
from textwrap import dedent
45
from typing import List, Optional, Tuple
56
from unittest.mock import Mock
@@ -12,8 +13,9 @@
1213
from dstack._internal.cli.services.configurators.fleet import (
1314
FleetConfigurator,
1415
_render_fleet_spec_diff,
16+
_resolve_ssh_key,
1517
)
16-
from dstack._internal.core.errors import ConfigurationError
18+
from dstack._internal.core.errors import CLIError, ConfigurationError
1719
from dstack._internal.core.models.common import ApplyAction
1820
from dstack._internal.core.models.envs import Env
1921
from dstack._internal.core.models.fleets import (
@@ -25,7 +27,9 @@
2527
FleetStatus,
2628
InstanceGroupPlacement,
2729
)
30+
from dstack._internal.core.models.instances import SSHKey
2831
from dstack._internal.core.models.profiles import Profile
32+
from tests._internal.utils.test_ssh import PRIVATE_KEY, PUBLIC_KEY, PUBLIC_KEY_NO_COMMENT
2933

3034

3135
def create_conf() -> FleetConfiguration:
@@ -245,3 +249,43 @@ def test_no_diff(self):
245249
spec = get_cloud_fleet_spec()
246250

247251
assert _render_fleet_spec_diff(spec, spec.model_copy(deep=True)) is None
252+
253+
254+
class TestResolveSSHKey:
255+
def test_returns_none_if_no_path_given(self):
256+
assert _resolve_ssh_key(None) is None
257+
258+
def test_uses_public_key_file(self, tmp_path: Path):
259+
private_key_path = tmp_path / "id_ed25519"
260+
private_key_path.write_text(PRIVATE_KEY)
261+
(tmp_path / "id_ed25519.pub").write_text(PUBLIC_KEY)
262+
263+
assert _resolve_ssh_key(str(private_key_path)) == SSHKey(
264+
public=PUBLIC_KEY, private=PRIVATE_KEY
265+
)
266+
267+
def test_generates_public_key(self, tmp_path: Path):
268+
private_key_path = tmp_path / "id_ed25519"
269+
private_key_path.write_text(PRIVATE_KEY)
270+
271+
assert _resolve_ssh_key(str(private_key_path)) == SSHKey(
272+
public=PUBLIC_KEY_NO_COMMENT, private=PRIVATE_KEY
273+
)
274+
275+
def test_raises_if_public_key_given(self, tmp_path: Path):
276+
public_key_path = tmp_path / "id_ed25519.pub"
277+
public_key_path.write_text(PUBLIC_KEY)
278+
279+
with pytest.raises(CLIError, match="Expected a private key"):
280+
_resolve_ssh_key(str(public_key_path))
281+
282+
def test_raises_if_key_does_not_exist(self, tmp_path: Path):
283+
with pytest.raises(CLIError, match="Unable to read the SSH key"):
284+
_resolve_ssh_key(str(tmp_path / "id_ed25519"))
285+
286+
def test_raises_if_key_type_is_not_supported(self, tmp_path: Path):
287+
key_path = tmp_path / "id_ed25519"
288+
key_path.write_text("garbage")
289+
290+
with pytest.raises(CLIError, match="Unsupported or invalid SSH key"):
291+
_resolve_ssh_key(str(key_path))

0 commit comments

Comments
 (0)