fix(pyats): refactor pyats config file creation (incl disable EnvironmentDebugPlugin) by oboehmer · Pull Request #697 · netascode/nac-test

oboehmer · 2026-03-22T12:35:20Z

Description

Security fix: PyATS EnvironmentDebugPlugin was writing environment variables (including passwords) to env.txt files within archive artifacts. This change disables the plugin and refactors config file management for cleaner architecture.
Also refactor pyats config file creation and remove dead code

Closes

Fixes Security: Disable pyATS EnvironmentDebugPlugin to prevent credential exposure in env.txt #689
Fixes Tech Debt: Consolidate PyATS config file creation in SubprocessRunner #570

Related Issue(s)

Remove merged data model artifact after test execution to prevent potential credential exposure #677 (planned robust cleanup mechanism; __del__ in this PR is a stopgap)

Type of Change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactoring / Technical debt (internal improvements with no user-facing changes)
Documentation update
Chore (build process, CI, tooling, dependencies)
Other (please describe):

Test Framework Affected

PyATS
Robot Framework
Both
N/A (not test-framework specific)

Network as Code (NaC) Architecture Affected

Platform Tested

macOS (version tested: macOS with Python 3.12)
Linux (distro/version tested: )

Key Changes

Security (#689)

Add EnvironmentDebugPlugin: enabled: False to plugin configuration
Add E2E regression test scanning ALL output artifacts for credential sentinels
Test covers both zip contents and all non-zip files

Architecture Refactoring (#570)

Move config file creation from execute_job() into SubprocessRunner.__init__()
Add explicit cleanup() method to SubprocessRunner for config file removal
Add __del__ for opportunistic cleanup on unexpected exits (best-effort; see Remove merged data model artifact after test execution to prevent potential credential exposure #677)
Extract _cleanup_subprocess_runner() helper in orchestrator to DRY up two identical call sites
Orchestrator calls cleanup() conditionally based on keep_artifacts policy
Config file attributes only set after successful file creation (safer error handling)
Extract config filenames as constants in constants.py
Write config files to output_dir with well-known names instead of temp files

Dead Code Removal

Remove unused _build_reporter_config() and _generate_plugin_config() from orchestrator
Remove unnecessary tempfile.TemporaryDirectory wrapper in _run_tests_async
Remove unused plugin_config_path parameter

Testing Done

Unit tests added/updated (26 subprocess_runner unit tests)
Integration tests performed (16 subprocess_runner integration tests)
Manual testing performed:
- PyATS tests executed successfully
- Robot Framework tests executed successfully
- D2D/SSH tests executed successfully (if applicable)
- HTML reports generated correctly
All existing tests pass (pytest / pre-commit run -a)

Test Commands Used

# Unit tests (26 subprocess_runner tests, 219 pyats_core tests total)
pytest tests/unit/pyats_core/execution/test_subprocess_runner.py -v
pytest tests/unit/pyats_core/ tests/pyats_core/ -v

# E2E credential exposure tests
pytest tests/e2e/test_e2e_scenarios.py -v -k "credential" -n auto --dist loadscope

# Pre-commit hooks
pre-commit run --all-files

Checklist

Code follows project style guidelines (pre-commit run -a passes)
Self-review of code completed
Code is commented where necessary (especially complex logic)
Documentation updated (if applicable)
No new warnings introduced
Changes work on both macOS and Linux
CHANGELOG.md updated (if applicable)

Screenshots (if applicable)

N/A

Additional Notes

Design decisions:

SubprocessRunner now owns both creation AND cleanup of its config files (cleaner SRP)
Orchestrator retains policy control (decides when to call cleanup based on debug flags)
__del__ cleanup is a safe no-op if files are already gone or were never created
Config file cleanup is best-effort: files are small and non-sensitive; a robust cleanup mechanism covering all exit paths will be added as part of Remove merged data model artifact after test execution to prevent potential credential exposure #677

…andling (#689, #570) Security fix: PyATS EnvironmentDebugPlugin was writing environment variables (including passwords) to env.txt files within archive artifacts. This change disables the plugin and refactors config file management for cleaner architecture. **Security (#689)** - Add `EnvironmentDebugPlugin: enabled: False` to plugin configuration - Add E2E regression test scanning ALL output artifacts for credential sentinels - Test covers both zip contents and all non-zip files **Architecture Refactoring (#570)** - Move config file creation from `execute_job()` into `SubprocessRunner.__init__()` - Add `cleanup()` method to SubprocessRunner for config file removal - Orchestrator calls `cleanup()` conditionally based on keep_artifacts policy - Config file attributes only set after successful file creation (safer error handling) - Extract config filenames as constants in `constants.py` - Write config files to output_dir with well-known names instead of temp files **Dead Code Removal** - Remove unused `_build_reporter_config()` and `_generate_plugin_config()` from orchestrator - Remove unnecessary `tempfile.TemporaryDirectory` wrapper in `_run_tests_async` - Remove unused `plugin_config_path` parameter Closes #689, #570 Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

… credential check - Remove 3 moot unit tests that checked config strings (fragile, not behavioral) - Remove duplicate integration test for config creation failure - Move credential sentinel test imports to module level - Run credential check for all scenarios, not just PyATS - Improve error message for PyATS config creation failure

…chestrator Verify that when SubprocessRunner fails to create config files (e.g., disk full), PyATSOrchestrator returns PyATSResults with from_error() for discovered test types.

…nv-debug-plugin

…ix cleanup leak - Delete ConfigFileCreationError; _create_config_files now raises RuntimeError, consistent with the pyats-not-found failure in __init__ - Catch RuntimeError in orchestrator so both init failures populate api/d2d results correctly instead of falling through to the broad except-Exception handler - Call subprocess_runner.cleanup() in the report-generation failure branch so config files are removed regardless of outcome - Assert EnvironmentDebugPlugin disabled in plugin config integration test using regex; convert all config-content checks to re.search - Remove leftover try/finally unlink() blocks in TestConfigFileContent (files live in tmp_path, cleaned up by pytest)

…asserts Review feedback from PR #697: - Fix bug: if first config file writes successfully but second fails, clean up the first file to avoid orphaned files on disk - Replace assert statements with RuntimeError in _build_command() since asserts are stripped in optimized mode (-O) - Add clarifying comments explaining why cleanup isn't needed on SubprocessRunner init failure (attributes only set after success) - Add docstring note about planned nac-test-wide cleanup mechanism - Add test for partial write failure cleanup behavior

- Add SubprocessRunner.__del__ for opportunistic cleanup on unexpected exits (best-effort: CPython GC only, not guaranteed on SIGKILL; ref #677) - Mark unreachable None-guard with # pragma: no cover and explanatory comment - Update cleanup() docstring to reflect dual call sites (__del__ + orchestrator) - Extract _cleanup_subprocess_runner() helper in orchestrator to DRY up two identical call sites; documents that __del__ covers unexpected exit paths - Cast keep_artifacts to bool() to avoid bool | str | None union type - Remove stale comment in except RuntimeError block (made obsolete by __del__) - Strengthen test_cleanup_is_idempotent with file-existence assertions - Add test_del_calls_cleanup to verify __del__ removes config files

…nv-debug-plugin

CleanupManager.register() gains a skip_if_debug parameter. Files registered with skip_if_debug=True are deleted on normal exit but retained when NAC_TEST_DEBUG is set, letting developers inspect intermediate artifacts without any extra flags or code changes. The merged data model YAML is the first consumer: it now registers with skip_if_debug=True so it survives for inspection in debug runs. The same mechanism is available for job scripts, testbed YAMLs, and any other artifacts tracked in PR #697. Internally, _files changes from set[Path] to dict[Path, bool], keeping the flag co-located with the path and avoiding two collections to sync. Remove the exit_with_cleanup() closure from main() — typer.Exit is caught by Click which calls sys.exit(), reliably triggering atexit handlers on every exit path. The closure and all cleanup_now() pre-exit calls were redundant with the already-registered atexit handler.

oboehmer force-pushed the fix/689-570-disable-env-debug-plugin branch from 6a826c2 to 92ca3a8 Compare March 22, 2026 17:36

oboehmer force-pushed the fix/689-570-disable-env-debug-plugin branch from 92ca3a8 to 9214b78 Compare March 22, 2026 17:42

oboehmer mentioned this pull request Mar 22, 2026

fix(pyats): prevent credential exposure in PyATS archives (#689) #698

Merged

42 tasks

oboehmer added 9 commits March 22, 2026 23:16

test(pyats): add test for ConfigFileCreationError handling in PyATSOr…

208f60d

…chestrator Verify that when SubprocessRunner fails to create config files (e.g., disk full), PyATSOrchestrator returns PyATSResults with from_error() for discovered test types.

Merge remote-tracking branch 'origin/main' into fix/689-570-disable-e…

cb7ea7d

…nv-debug-plugin

use textwrap.dedent() also for PYATS_CONFIG

16211cc

address review feedback from #698

23481f2

don't re-export new constants

f42ae1c

Merge remote-tracking branch 'origin/main' into fix/689-570-disable-e…

13e1ecd

…nv-debug-plugin

oboehmer marked this pull request as ready for review March 24, 2026 20:55

oboehmer changed the title ~~fix(pyats): disable EnvironmentDebugPlugin to prevent credential exposure~~ fix(pyats): refactor pyats config file creation - disable EnvironmentDebugPlugin to prevent credential exposure Mar 24, 2026

oboehmer changed the title ~~fix(pyats): refactor pyats config file creation - disable EnvironmentDebugPlugin to prevent credential exposure~~ fix(pyats): refactor pyats config file creation (incl disable EnvironmentDebugPlugin) Mar 24, 2026

oboehmer requested a review from aitestino March 24, 2026 20:59

Merge remote-tracking branch 'origin/main' into fix/689-570-disable-e…

0e78e76

…nv-debug-plugin

oboehmer self-assigned this Mar 25, 2026

oboehmer added the prio: high label Mar 31, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(pyats): refactor pyats config file creation (incl disable EnvironmentDebugPlugin)#697

fix(pyats): refactor pyats config file creation (incl disable EnvironmentDebugPlugin)#697
oboehmer wants to merge 12 commits intomainfrom
fix/689-570-disable-env-debug-plugin

oboehmer commented Mar 22, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

oboehmer commented Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Closes

Related Issue(s)

Type of Change

Test Framework Affected

Network as Code (NaC) Architecture Affected

Platform Tested

Key Changes

Security (#689)

Architecture Refactoring (#570)

Dead Code Removal

Testing Done

Test Commands Used

Checklist

Screenshots (if applicable)

Additional Notes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

oboehmer commented Mar 22, 2026 •

edited

Loading