feat: unify SCCFM interactive command and token configuration - #24
Conversation
Replace devkit with sccfm-cli-interactive and consolidate CLI and Ansible token configuration into a shared secure profile.
| "Import profiles from the former Ansible Vault token store", | ||
| _import_legacy_vault, | ||
| ), | ||
| ("run-cli", "Run an sccfm-cli command interactively", _run_cli_commands), |
There was a problem hiding this comment.
[P1] Protect sensitive CLI parameters
The run-cli path still treats api_token like ordinary text: _execute_cli_command() uses questionary.text, adds the value to argv, and prints the complete command. Because it supplies --api-token explicitly, Click's new hide_input=True prompt is bypassed. Please propagate sensitive/hidden-input metadata into CliParam, use a password prompt, and redact the rendered command.
There was a problem hiding this comment.
Fixed in 5d69315 and completed in 6ee20e2. CliParam now carries hide_input, secret values use questionary.password, rendered commands redact them, and secret-bearing CLI invocations run in-process so the raw value is not exposed through OS process arguments. Regression coverage is in test_execute_cli_command_masks_secrets_and_uses_password_prompt.
| _import_legacy_vault, | ||
| ), | ||
| ("run-cli", "Run an sccfm-cli command interactively", _run_cli_commands), | ||
| ("run-ansible", "Run an Ansible example playbook", _run_ansible_examples), |
There was a problem hiding this comment.
[P2] Make the Vault argument conditional
The new profile setup no longer creates .vault_pass, and Vault is optional for read-only examples, but _run_ansible_examples() always supplies --vault-password-file .vault_pass. Those examples fail after the documented profile-only setup. Please add the argument only when the selected playbook requires Vault and the password file exists.
There was a problem hiding this comment.
Fixed in 5d69315. _run_ansible_examples() now checks _playbook_requires_vault() and only adds --vault-password-file when the selected playbook references Vault variables and .vault_pass exists. Both the Vault and non-Vault paths have regression tests.
| def configured_sccfm_profile(monkeypatch: MonkeyPatch) -> None: | ||
| """Keep module tests isolated from the user's canonical profile file.""" | ||
| monkeypatch.setattr( | ||
| config_module.ProfileService, |
There was a problem hiding this comment.
[P2] Isolate tests from the real profile path
Patching ProfileService.load is too late to prevent ProfileService.__init__ from inspecting and chmodding the user's ~/.sccfm-cli/config.json. In a restricted environment this caused 184 module-test failures. Please redirect SCCFM_CONFIG to a tmp_path or replace service construction so tests never touch real credential state.
There was a problem hiding this comment.
Fixed in 5d69315. The autouse fixture now sets SCCFM_CONFIG to tmp_path / "config.json" before ProfileService is constructed, so module tests cannot inspect or chmod the user's real profile store.
| DOCUMENTATION = r""" | ||
| name: profile | ||
| author: Cisco SCCFM Team | ||
| version_added: "1.0.0" |
There was a problem hiding this comment.
[P3] Correct the introduction version
This lookup is introduced before collection version 1.0.0, so version_added currently reports it as a future feature. Please set this to the release version that will first contain the plugin.
There was a problem hiding this comment.
Fixed in 5d69315. The lookup plugin now declares version_added: "0.39.0", matching the release that will first contain it.
huides00
left a comment
huides00
left a comment
There was a problem hiding this comment.
Review focused on the unified interactive/profile flow. Breaking compatibility is intentionally not treated as a concern because this has not been released.
Scoombe
left a comment
Scoombe
left a comment
There was a problem hiding this comment.
The consolidation around a canonical named profile store makes sense, and the CLI/Ansible integration is internally consistent. I verified schema export, collection build/discovery, module argument resolution, mypy, and the full test suite (1,181 passing). The failing Docs check is unrelated infrastructure noise: GitHub returned HTTP 429 while downloading actions/jekyll-build-pages.
I found two issues that should be addressed before merge: the interactive runner still exposes API tokens through the child process argument list, and the Python 3.12 implementation breaks the documented native-Windows path. This is a commented review because this GitHub account owns the PR and cannot formally request changes on its own PR.
| prompt = f"{param.label}{'' if param.required else ' (leave blank to skip)'}" | ||
| single: str | None = questionary.text(prompt).unsafe_ask() | ||
| single: str | None = _prompt_param(prompt, param.hide_input) | ||
| normalized_value = (single or "").strip() |
There was a problem hiding this comment.
[P1] Keep the token out of the child process argument list
The new password prompt and rendered-command redaction protect the terminal, but argv still contains the raw token and subprocess.call(argv, ...) exposes it to process listings and process/audit tooling for the lifetime of sccfm-cli. This affects configure --api-token, precisely the credential this change is trying to store securely. Please invoke promptable secret options without placing their values in argv (or execute the Click command in-process through a secret-safe path).
There was a problem hiding this comment.
Fixed in 6ee20e2. _invoke_cli() executes any secret-bearing command through Click in the current Python process with standalone_mode=False; only non-secret commands use subprocess.call. The token therefore remains out of OS argv while terminal rendering stays redacted. Regression tests cover both invocation paths.
| ) | ||
| temporary_path = Path(temporary_name) | ||
| try: | ||
| os.fchmod(file_descriptor, _CONFIG_FILE_MODE) |
There was a problem hiding this comment.
[P1] Preserve the stated Python 3.12 Windows support
os.fchmod was only added on Windows in Python 3.13, while this project targets Python 3.12 and its CLI skill explicitly documents native Windows support. On Python 3.12/Windows every profile save reaches this line and fails before writing the config, so sccfm-cli configure cannot work. Please use a platform-compatible permission path (with an appropriate Windows security strategy) or explicitly narrow the supported platform contract.
There was a problem hiding this comment.
Fixed in 6ee20e2. POSIX mode operations are now gated by _SUPPORTS_POSIX_MODES, so Python 3.12 on Windows never calls unavailable os.fchmod or POSIX chmod paths. Windows uses the ACLs inherited from the user's profile directory, and the README/Ansible/agent guidance now documents that behavior. A regression test verifies saving without POSIX mode APIs.
Consistency CheckNo consistency issues found. Checker output |
2 participants
Replace devkit with sccfm-cli-interactive and consolidate CLI and Ansible token configuration into a shared secure profile.