Skip to content

Commit eb45821

Browse files
Merge branch 'master' into fix/air-hub-lazy-clients-6542
2 parents b12a514 + bd53b10 commit eb45821

11 files changed

Lines changed: 906 additions & 49 deletions

File tree

‎sagemaker-core/src/sagemaker/core/common_utils.py‎

Lines changed: 141 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,15 @@
9292
"/root",
9393
"/var/lib",
9494
"/opt/ml/metadata",
95+
"/proc",
96+
"/sys",
97+
"/dev",
98+
]
99+
100+
# Locations under _SENSITIVE_SYSTEM_PATHS that are allowed, such as shared memory used
101+
# as scratch space.
102+
_SENSITIVE_SYSTEM_PATH_EXCEPTIONS = [
103+
"/dev/shm",
95104
]
96105

97106
logger = logging.getLogger(__name__)
@@ -668,13 +677,11 @@ def _validate_source_directory(source_directory):
668677
# Resolve symlinks to get the actual path
669678
abs_source = abspath(realpath(source_directory))
670679

671-
# Check if the source path is under any sensitive directory
672-
for sensitive_path in _SENSITIVE_SYSTEM_PATHS:
673-
if abs_source != "/" and abs_source.startswith(sensitive_path):
674-
raise ValueError(
675-
f"source_directory cannot access sensitive system paths. "
676-
f"Got: {source_directory} (resolved to {abs_source})"
677-
)
680+
if _is_sensitive_system_path(abs_source):
681+
raise ValueError(
682+
f"source_directory cannot access sensitive system paths. "
683+
f"Got: {source_directory} (resolved to {abs_source})"
684+
)
678685

679686

680687
def _validate_dependency_path(dependency):
@@ -694,13 +701,126 @@ def _validate_dependency_path(dependency):
694701
# Resolve symlinks to get the actual path
695702
abs_dependency = abspath(realpath(dependency))
696703

697-
# Check if the dependency path is under any sensitive directory
698-
for sensitive_path in _SENSITIVE_SYSTEM_PATHS:
699-
if abs_dependency != "/" and abs_dependency.startswith(sensitive_path):
700-
raise ValueError(
701-
f"dependency path cannot access sensitive system paths. "
702-
f"Got: {dependency} (resolved to {abs_dependency})"
703-
)
704+
if _is_sensitive_system_path(abs_dependency):
705+
raise ValueError(
706+
f"dependency path cannot access sensitive system paths. "
707+
f"Got: {dependency} (resolved to {abs_dependency})"
708+
)
709+
710+
711+
def _validate_inference_script_path(inference_script):
712+
"""Validate that an inference script path is safe to use.
713+
714+
Ensures the inference script path does not access restricted system locations.
715+
716+
Args:
717+
inference_script (str): The inference script path to validate.
718+
719+
Raises:
720+
ValueError: If the path is not allowed.
721+
"""
722+
if not inference_script:
723+
return
724+
725+
# Resolve symlinks to get the actual path
726+
abs_script = abspath(realpath(inference_script))
727+
728+
if _is_sensitive_system_path(abs_script):
729+
raise ValueError(
730+
f"inference_script cannot access sensitive system paths. "
731+
f"Got: {inference_script} (resolved to {abs_script})"
732+
)
733+
734+
735+
def _resolved_sensitive_system_paths():
736+
"""Return the sensitive system paths, both as configured and with symlinks resolved.
737+
738+
Paths such as ``/etc`` or a symlinked home directory resolve to a different location
739+
(for example ``/private/etc`` on macOS), so both forms are checked.
740+
741+
Returns:
742+
tuple[str, ...]: Normalized absolute sensitive paths.
743+
"""
744+
return _resolve_path_variants(tuple(_SENSITIVE_SYSTEM_PATHS))
745+
746+
747+
@lru_cache(maxsize=8)
748+
def _resolve_path_variants(paths):
749+
"""Return each path both as configured and with symlinks resolved, without duplicates.
750+
751+
Args:
752+
paths (tuple[str, ...]): Paths to resolve.
753+
754+
Returns:
755+
tuple[str, ...]: Normalized absolute paths.
756+
"""
757+
resolved = []
758+
for path in paths:
759+
for candidate in (normpath(abspath(path)), normpath(realpath(path))):
760+
if candidate not in resolved:
761+
resolved.append(candidate)
762+
return tuple(resolved)
763+
764+
765+
def _is_sensitive_system_path(resolved_path):
766+
"""Check whether an already resolved absolute path is under a sensitive system path.
767+
768+
Matching is done on whole path components, so ``~/.aws`` does not match ``~/.awsfoo``.
769+
Paths under ``_SENSITIVE_SYSTEM_PATH_EXCEPTIONS`` (for example ``/dev/shm``) are allowed.
770+
771+
Args:
772+
resolved_path (str): An absolute path with symlinks resolved.
773+
774+
Returns:
775+
bool: True if resolved_path is a sensitive path or nested under one.
776+
"""
777+
resolved_path = normpath(resolved_path)
778+
if resolved_path == os.path.sep:
779+
return False
780+
if any(
781+
_is_within_base(resolved_path, allowed_path)
782+
for allowed_path in _resolve_path_variants(tuple(_SENSITIVE_SYSTEM_PATH_EXCEPTIONS))
783+
):
784+
return False
785+
return any(
786+
_is_within_base(resolved_path, sensitive_path)
787+
for sensitive_path in _resolved_sensitive_system_paths()
788+
)
789+
790+
791+
def _validate_symlinks_within_root(path, description):
792+
"""Validate that symlinks nested under a directory resolve to locations inside it.
793+
794+
Copying or archiving a directory follows nested symlinks, so a symlink that points
795+
outside the directory would pull in content from elsewhere on the file system.
796+
Symlinks that resolve to a location inside the directory are allowed. A path that
797+
is not a directory has no nested entries and is not checked here.
798+
799+
Args:
800+
path (str): The directory to check.
801+
description (str): Name of the input, used in the error message.
802+
803+
Raises:
804+
ValueError: If a nested symlink resolves outside of ``path``.
805+
"""
806+
if not path or not os.path.isdir(path):
807+
return
808+
809+
root = normpath(realpath(abspath(path)))
810+
for dirpath, dirnames, filenames in os.walk(path):
811+
for name in dirnames + filenames:
812+
entry = joinpath(dirpath, name)
813+
if not os.path.islink(entry):
814+
continue
815+
target = normpath(realpath(entry))
816+
if not _is_within_base(target, root):
817+
raise ValueError(
818+
f"{description} contains a symlink that resolves outside of {path}: "
819+
f"{entry} -> {target}. Symlinks under {description} must point to files "
820+
f"or directories inside it. Replace the symlink with a copy of its "
821+
f"target, or remove it from {path} (for example, move a virtual "
822+
f"environment or other tooling directory out of {description})."
823+
)
704824

705825

706826
def _create_or_update_code_dir(
@@ -711,11 +831,10 @@ def _create_or_update_code_dir(
711831
resolved_code_dir = _get_resolved_path(code_dir)
712832

713833
# Validate that code_dir does not resolve to a sensitive system path
714-
for sensitive_path in _SENSITIVE_SYSTEM_PATHS:
715-
if resolved_code_dir != "/" and resolved_code_dir.startswith(sensitive_path):
716-
raise ValueError(
717-
f"Invalid code_dir path: {code_dir} resolves to sensitive system path {resolved_code_dir}"
718-
)
834+
if _is_sensitive_system_path(resolved_code_dir):
835+
raise ValueError(
836+
f"Invalid code_dir path: {code_dir} resolves to sensitive system path {resolved_code_dir}"
837+
)
719838

720839
if source_directory and source_directory.lower().startswith("s3://"):
721840
local_code_path = os.path.join(tmp, "local_code.tar.gz")
@@ -725,12 +844,13 @@ def _create_or_update_code_dir(
725844
custom_extractall_tarfile(t, code_dir)
726845

727846
elif source_directory:
728-
# Validate source_directory for security
729847
_validate_source_directory(source_directory)
848+
_validate_symlinks_within_root(source_directory, "source_directory")
730849
if os.path.exists(code_dir):
731850
shutil.rmtree(code_dir)
732851
shutil.copytree(source_directory, code_dir)
733852
else:
853+
_validate_inference_script_path(inference_script)
734854
if not os.path.exists(code_dir):
735855
os.mkdir(code_dir)
736856
try:
@@ -742,8 +862,8 @@ def _create_or_update_code_dir(
742862
raise
743863

744864
for dependency in dependencies:
745-
# Validate dependency path for security
746865
_validate_dependency_path(dependency)
866+
_validate_symlinks_within_root(dependency, "dependency")
747867
lib_dir = os.path.join(code_dir, "lib")
748868
if os.path.isdir(dependency):
749869
shutil.copytree(dependency, os.path.join(lib_dir, os.path.basename(dependency)))

‎sagemaker-core/src/sagemaker/core/fw_utils.py‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -460,6 +460,14 @@ def tar_and_upload_dir(
460460
"Inputted directory for storing newly generated temporary directory does "
461461
f"not exist: '{settings.local_download_dir}'"
462462
)
463+
# create_tar_file follows symlinks, so ensure nested symlinks stay inside each input.
464+
if directory:
465+
utils._validate_symlinks_within_root(directory, "source_dir")
466+
else:
467+
utils._validate_symlinks_within_root(script, "entry_point")
468+
for dependency in dependencies:
469+
utils._validate_symlinks_within_root(dependency, "dependency")
470+
463471
local_download_dir = None if settings is None else settings.local_download_dir
464472
tmp = tempfile.mkdtemp(dir=local_download_dir)
465473
encrypt_artifact = True if settings is None else settings.encrypt_repacked_artifacts

‎sagemaker-core/src/sagemaker/core/git_utils.py‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -337,15 +337,17 @@ def _redact_credentials_from_url(url):
337337
338338
Replaces any username, password, or token embedded before the '@' in an
339339
HTTPS URL with a placeholder so that credentials are never exposed in
340-
logs or exception messages.
340+
logs or exception messages. Only the authority (the part before the first
341+
'/', '?' or '#') is considered, up to its last '@'. URLs without userinfo,
342+
including ones with an '@' later in the path, are returned unchanged.
341343
342344
Args:
343345
url (str): The Git repository URL that may contain embedded credentials.
344346
345347
Returns:
346348
str: The URL with credentials replaced by '<credentials-redacted>'.
347349
"""
348-
return re.sub(r"(https://)([^@]+)@", r"\1<credentials-redacted>@", url)
350+
return re.sub(r"^(https://)[^/?#]*@", r"\1<credentials-redacted>@", url)
349351

350352

351353
def _run_clone_command(repo_url, dest_dir):

‎sagemaker-core/src/sagemaker/core/modules/utils.py‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
from datetime import datetime
2424
from typing import Literal, Any
2525

26+
from sagemaker.core.git_utils import _redact_credentials_from_url
2627
from sagemaker.core.shapes import Unassigned
2728
from sagemaker.core.modules import logger
2829

@@ -170,9 +171,16 @@ def _run_clone_command_silent(repo_url, dest_dir):
170171
stderr=subprocess.DEVNULL,
171172
)
172173
except subprocess.CalledProcessError as e:
173-
logger.error(f"Failed to clone repository: {repo_url}")
174-
logger.error(f"Error output:\n{e}")
175-
raise
174+
safe_url = _redact_credentials_from_url(repo_url)
175+
error = subprocess.CalledProcessError(
176+
e.returncode,
177+
["git", "clone", safe_url, dest_dir],
178+
output=e.output,
179+
stderr=e.stderr,
180+
)
181+
logger.error(f"Failed to clone repository: {safe_url}")
182+
logger.error(f"Error output:\n{error}")
183+
raise error from None
176184
elif repo_url.startswith("git@") or repo_url.startswith("ssh://"):
177185
try:
178186
with tempfile.TemporaryDirectory() as tmp_dir:
Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License"). You
4+
# may not use this file except in compliance with the License. A copy of
5+
# the License is located at
6+
#
7+
# http://aws.amazon.com/apache2.0/
8+
#
9+
# or in the "license" file accompanying this file. This file is
10+
# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
11+
# ANY KIND, either express or implied. See the License for the specific
12+
# language governing permissions and limitations under the License.
13+
"""Unit tests for _run_clone_command_silent in sagemaker.core.modules.utils."""
14+
15+
import logging
16+
import subprocess
17+
from unittest.mock import patch
18+
19+
import pytest
20+
21+
from sagemaker.core.modules.utils import _run_clone_command_silent
22+
23+
MODULE = "sagemaker.core.modules.utils"
24+
DEST = "/tmp/dest"
25+
26+
27+
@pytest.mark.parametrize(
28+
"repo_url,userinfo",
29+
[
30+
("https://oauth2:abc123@example.com/org/repo.git", "oauth2:abc123"),
31+
("https://user:p%40ss@example.com/org/repo.git", "user:p%40ss"),
32+
("https://user:p@ss@example.com/org/repo.git", "user:p@ss"),
33+
],
34+
)
35+
def test_clone_failure_uses_sanitized_url(repo_url, userinfo, caplog):
36+
with patch(
37+
f"{MODULE}.subprocess.check_call",
38+
side_effect=subprocess.CalledProcessError(128, ["git", "clone", repo_url, DEST]),
39+
):
40+
with caplog.at_level(logging.ERROR):
41+
with pytest.raises(subprocess.CalledProcessError) as exc_info:
42+
_run_clone_command_silent(repo_url, DEST)
43+
44+
error = exc_info.value
45+
expected_url = "https://<credentials-redacted>@example.com/org/repo.git"
46+
assert error.returncode == 128
47+
assert error.cmd == ["git", "clone", expected_url, DEST]
48+
assert error.__suppress_context__
49+
assert userinfo not in str(error)
50+
assert userinfo not in caplog.text
51+
assert expected_url in caplog.text
52+
53+
54+
def test_clone_failure_without_userinfo_keeps_url(caplog):
55+
repo_url = "https://example.com/org/repo.git"
56+
with patch(
57+
f"{MODULE}.subprocess.check_call",
58+
side_effect=subprocess.CalledProcessError(128, ["git", "clone", repo_url, DEST]),
59+
):
60+
with caplog.at_level(logging.ERROR):
61+
with pytest.raises(subprocess.CalledProcessError) as exc_info:
62+
_run_clone_command_silent(repo_url, DEST)
63+
64+
assert exc_info.value.cmd == ["git", "clone", repo_url, DEST]
65+
assert repo_url in caplog.text
66+
67+
68+
def test_clone_success_uses_original_url():
69+
repo_url = "https://oauth2:abc123@example.com/org/repo.git"
70+
with patch(f"{MODULE}.subprocess.check_call") as mock_check_call:
71+
_run_clone_command_silent(repo_url, DEST)
72+
73+
args, kwargs = mock_check_call.call_args
74+
assert args[0] == ["git", "clone", repo_url, DEST]
75+
assert kwargs["env"]["GIT_TERMINAL_PROMPT"] == "0"

0 commit comments

Comments
 (0)