Skip to content

Commit

Permalink
chore(iast): fix propagation for platformdirs (#9593)
Browse files Browse the repository at this point in the history
IAST: Removes the detection and later patching skipping of loaded
third-party modules. This change increases the coverage of propagation,
so only Python standard libraries (Python batteries) and the ones
included in the deny list are not patched.

## Checklist

- [x] Change(s) are motivated and described in the PR description
- [x] Testing strategy is described if automated tests are not included
in the PR
- [x] Risks are described (performance impact, potential for breakage,
maintainability)
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed or label `changelog/no-changelog` is set
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/))
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))
- [x] If this PR changes the public interface, I've notified
`@DataDog/apm-tees`.

## Reviewer Checklist

- [x] Title is accurate
- [x] All changes are related to the pull request's stated goal
- [ ] Description motivates each change
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [x] Testing strategy adequately addresses listed risks
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] Release note makes sense to a user of the library
- [x] Author has acknowledged and discussed the performance implications
of this PR as reported in the benchmarks PR comment
- [x] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

---------

Co-authored-by: Juanjo Alvarez Martinez <[email protected]>
  • Loading branch information
gnufede and juanjux authored Jun 25, 2024
1 parent 9ef9b74 commit 7527e61
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 47 deletions.
76 changes: 50 additions & 26 deletions ddtrace/appsec/_iast/_ast/ast_patching.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
from sys import builtin_module_names
from types import ModuleType
from typing import Optional
from typing import Set
from typing import Text
from typing import Tuple

Expand All @@ -25,19 +24,58 @@
# Prefixes for modules where IAST patching is allowed
IAST_ALLOWLIST: Tuple[Text, ...] = ("tests.appsec.iast",)
IAST_DENYLIST: Tuple[Text, ...] = (
"flask",
"werkzeug",
"crypto", # This module is patched by the IAST patch methods, propagation is not needed
"deprecated",
"api_pb2", # Patching crashes with these auto-generated modules, propagation is not needed
"api_pb2_grpc", # ditto
"asyncpg.pgproto",
"blinker",
"bytecode",
"cattrs",
"click",
"ddsketch",
"ddtrace",
"pkg_resources",
"encodings", # this package is used to load encodings when a module is imported, propagation is not needed
"envier",
"exceptiongroup",
"freezegun", # Testing utilities for time manipulation
"hypothesis",
"importlib_metadata",
"inspect", # this package is used to get the stack frames, propagation is not needed
"itsdangerous",
"opentelemetry-api",
"packaging",
"pip",
"pkg_resources",
"pluggy",
"protobuf",
"pycparser", # this package is called when a module is imported, propagation is not needed
"Crypto", # This module is patched by the IAST patch methods, propagation is not needed
"api_pb2", # Patching crashes with these auto-generated modules, propagation is not needed
"api_pb2_grpc", # ditto
"unittest.mock",
"pytest", # Testing framework
"freezegun", # Testing utilities for time manipulation
"setuptools",
"sklearn", # Machine learning library
"tomli",
"typing_extensions",
"unittest.mock",
"uvloop",
"urlpatterns_reverse.tests", # assertRaises eat exceptions in native code, so we don't call the original function
"wrapt",
"zipp",
## This is a workaround for Sanic failures:
"websocket",
"h11",
"aioquic",
"httptools",
"sniffio",
"py",
"sanic",
"rich",
"httpx",
"websockets",
"uvicorn",
"anyio",
"httpcore",
)


Expand Down Expand Up @@ -67,24 +105,10 @@ def get_encoding(module_path: Text) -> Text:
return ENCODING


try:
import importlib.metadata as il_md
except ImportError:
import importlib_metadata as il_md # type: ignore[no-redef]


def _build_installed_package_names_list() -> Set[Text]:
return {
ilmd_d.metadata["name"] for ilmd_d in il_md.distributions() if ilmd_d is not None and ilmd_d.files is not None
}


_NOT_PATCH_MODULE_NAMES = (
_build_installed_package_names_list() | _stdlib_for_python_version() | set(builtin_module_names)
)
_NOT_PATCH_MODULE_NAMES = _stdlib_for_python_version() | set(builtin_module_names)


def _in_python_stdlib_or_third_party(module_name: str) -> bool:
def _in_python_stdlib(module_name: str) -> bool:
return module_name.split(".")[0].lower() in [x.lower() for x in _NOT_PATCH_MODULE_NAMES]


Expand All @@ -98,11 +122,11 @@ def _should_iast_patch(module_name: Text) -> bool:
# max_deny = max((len(prefix) for prefix in IAST_DENYLIST if module_name.startswith(prefix)), default=-1)
# diff = max_allow - max_deny
# return diff > 0 or (diff == 0 and not _in_python_stdlib_or_third_party(module_name))
if module_name.startswith(IAST_ALLOWLIST):
if module_name.lower().startswith(IAST_ALLOWLIST):
return True
if module_name.startswith(IAST_DENYLIST):
if module_name.lower().startswith(IAST_DENYLIST):
return False
return not _in_python_stdlib_or_third_party(module_name)
return not _in_python_stdlib(module_name)


def visit_ast(
Expand Down
12 changes: 6 additions & 6 deletions tests/appsec/iast/_ast/test_ast_patching.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import mock
import pytest

from ddtrace.appsec._iast._ast.ast_patching import _in_python_stdlib_or_third_party
from ddtrace.appsec._iast._ast.ast_patching import _in_python_stdlib
from ddtrace.appsec._iast._ast.ast_patching import _should_iast_patch
from ddtrace.appsec._iast._ast.ast_patching import astpatch_module
from ddtrace.appsec._iast._ast.ast_patching import visit_ast
Expand Down Expand Up @@ -136,19 +136,19 @@ def test_module_should_iast_patch():
@pytest.mark.parametrize(
"module_name, result",
[
("Envier", True),
("Envier", False),
("iterTools", True),
("functooLs", True),
("astunparse", True),
("pytest.warns", True),
("astunparse", False),
("pytest.warns", False),
("datetime", True),
("posiX", True),
("app", False),
("my_app", False),
],
)
def test_module_in_python_stdlib_or_third_party(module_name, result):
assert _in_python_stdlib_or_third_party(module_name) == result
def test_module_in_python_stdlib(module_name, result):
assert _in_python_stdlib(module_name) == result


def test_module_path_none(caplog):
Expand Down
15 changes: 5 additions & 10 deletions tests/appsec/iast_packages/packages/pkg_iniconfig.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,18 +67,13 @@ def pkg_iniconfig_propagation_view():
ini_path = "example.ini"

try:
with open(ini_path, "w") as f:
f.write(ini_content)

config = iniconfig.IniConfig(ini_path)
parsed_data = {section.name: list(section.items()) for section in config}
value = parsed_data["section"][0][1]
config = iniconfig.IniConfig(ini_path, data=ini_content)
read_value = config["section"]["key"]
result_output = (
"OK" if is_pyobject_tainted(value) else f"Error: value from parsed_data is not tainted: {value}"
"OK"
if is_pyobject_tainted(read_value)
else f"Error: read_value from parsed_data is not tainted: {read_value}"
)

if os.path.exists(ini_path):
os.remove(ini_path)
except Exception as e:
result_output = f"Error: {str(e)}"
except Exception as e:
Expand Down
8 changes: 3 additions & 5 deletions tests/appsec/iast_packages/test_packages.py
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,6 @@ def uninstall(self, python_cmd):
"",
import_module_to_validate="platformdirs.unix",
test_propagation=True,
fixme_propagation_fails=True,
),
PackageForTesting(
"pluggy",
Expand Down Expand Up @@ -710,7 +709,6 @@ def uninstall(self, python_cmd):
"Parsed INI data: {'section': [('key', 'test1234')]}",
"",
test_propagation=True,
fixme_propagation_fails=True,
),
PackageForTesting("psutil", "5.9.8", "cpu", "CPU Usage: replaced_usage", ""),
PackageForTesting(
Expand Down Expand Up @@ -857,10 +855,10 @@ def _assert_propagation_results(response, package):
result_ok = content["result1"] == "OK"
if package.fixme_propagation_fails:
if result_ok:
print("FIXME: remove fixme_propagation_fails from package %s" % package.name)
pytest.fail("FIXME: remove fixme_propagation_fails from package %s" % package.name)
else:
print("FIXME: propagation test (expectedly) failed for package %s" % package.name)
return
# Add pytest xfail marker to skip the test
pytest.xfail("FIXME: remove fixme_propagation_fails from package %s" % package.name)

if not result_ok:
print(f"Error: incorrect result from propagation endpoint for package {package.name}: {content}")
Expand Down

0 comments on commit 7527e61

Please sign in to comment.