diff --git a/docs/adr/0327-hold-store-schema-and-hold-id-plumbing.md b/docs/adr/0327-hold-store-schema-and-hold-id-plumbing.md index c5f69fe76..43ccf6f0e 100644 --- a/docs/adr/0327-hold-store-schema-and-hold-id-plumbing.md +++ b/docs/adr/0327-hold-store-schema-and-hold-id-plumbing.md @@ -2,6 +2,9 @@ ## Status +> **Decision 5 superseded by [0343](0343-system-write-hold-fence.md)** (2026-11-04), +> for the six system-wide partition-write tools only. + Accepted (2026-11-03), issue #1851 (piece 1 of #1231). Implements ADR 0193 Decisions 1, 2, 5 and 7 on the ADR 0190 store; ADR 0193's prepare/release tool is #1852. diff --git a/docs/adr/0343-system-write-hold-fence.md b/docs/adr/0343-system-write-hold-fence.md new file mode 100644 index 000000000..6898e63e6 --- /dev/null +++ b/docs/adr/0343-system-write-hold-fence.md @@ -0,0 +1,49 @@ +# 0343 — System-wide partition writes refuse while a system has holds + +## Status + +Accepted (2026-11-04), issue #1887. Supersedes ADR 0327 Decision 5 only for the +six system-wide tools enumerated in the accompanying design. + +## Context + +ADR 0327 leaves system-scoped writes outside the partition-resolution hook. A held +partition can therefore be stopped or reconfigured through its managed system. The +operator selected "Refuse while any system hold exists" and accepted releasing holds +before these writes, without an override or multi-token authorization. + +## Decision + +Mark `hmc_modify_system`, `hmc_power_on_system`, `hmc_power_off_system`, +`hmc_activate_system_profile`, `hmc_restore_lpar_profiles` and `hmc_update_firmware` +as system-wide partition writes in the registry. Their existing managed-system UUID +resolution checks the local hold store through the front-end-installed guard. One hold +on that system refuses the call with `system_held`, regardless of caller or hold ID; +unknown store state also refuses. Holds on another system do not block it. + +MCP dispatch, including invoke, and existing CLI mirrors carry the marker centrally. +This is a whole-tool rule, including rename-only or validate-only argument variants. +No system hold token or override is introduced. Keep partition-token behavior unchanged. +Update handoff disclosure and consumer docs. Other ADR 0327 decisions remain in force. + +## Consequences + +Holders must release all holds on the system before these operations, accepting the +loss of their hold protection while doing so. Existing ownership/access-policy checks +still apply. The check has ADR 0193's resolver-time semantics: it does not lock the HMC +job against a hold placed after admission, and does not fence library/raw/other-host +writers. No store migration or dependency is needed. Resource-only and creation tools +remain outside the six-tool set; the design records the complete current inventory. + +## Considered & rejected + +- **Keep system writes unchecked.** judgment: fit. It leaves the held-partition hazard + in issue #1887 despite the operator's chosen refusal policy. +- **Accept every hold ID or an override.** judgment: complexity and fit. It adds an + authorization interface the operator expressly declined. +- **Check separately in each handler and CLI command.** judgment: complexity. One + registry marker and shared resolver keep policy and mirror coverage in the existing owners. +- **Fence every non-read managed-system tool.** judgment: fit. That broadens the rule + to new-partition creation and unrelated resource records beyond the chosen boundary. +- **Condition the check on individual arguments.** judgment: complexity. It creates + exceptions within tools whose safe whole-tool refusal the operator accepted. diff --git a/docs/kdive-tier-a-contract.md b/docs/kdive-tier-a-contract.md index 27048e41b..e6e1bc5e0 100644 --- a/docs/kdive-tier-a-contract.md +++ b/docs/kdive-tier-a-contract.md @@ -122,6 +122,12 @@ declared, and what kdive's doctor/adopt still has to check with `partition_held` unless the call presents the matching `hold_id`, whatever `authorize_power_operations` is set to. Console capture (`hmc_capture_lpar_console`) and reads are not checked. +- **System-wide partition writes require release first.** System modification, power on/off, + system-profile activation, profile restore and firmware update refuse `system_held` while + any partition on that system is held ([ADR 0343](adr/0343-system-write-hold-fence.md)). + These whole-tool checks have no token or ownership override, including validate-only + activation and rename-only modification. Existing CLI mirrors enforce the same rule. + Coordinate handoff with already-admitted system operations; this check does not lock a job. - **Library calls are not checked.** The hold binds the MCP server and CLI on the host that placed it. The library operations above, hmcpctl on other hosts, the HMC GUI and direct HMC commands are not fenced, nor is any tool the hold hook does not check; the document's diff --git a/docs/mcp-server.md b/docs/mcp-server.md index 0cf825566..944958414 100644 --- a/docs/mcp-server.md +++ b/docs/mcp-server.md @@ -567,12 +567,20 @@ hold ends `failed`, and a repeat of its `request_id` returns that record, so ret `request_id`. Reads, `hmc_capture_lpar_console` and the handoff tool are not refused. A store that cannot be read refuses these calls too. -A hold binds only this host's front ends. The library API, other hosts, the HMC GUI and direct -HMC commands are not fenced, and neither are tools that never resolve a partition: system-scoped -writes such as `hmc_restore_lpar_profiles`, system power and system profile activation, and -`hmc_run_command` and `hmcpctl raw` (ADR 0327). While any hold exists, a mutation that names its -partition only by name over SSH resolves the name through REST first; when REST cannot identify -it, the call is refused with `partition_hold_unverified`. +System-wide partition writes also consult the hold store (ADR 0343): `hmc_modify_system`, +`hmc_power_on_system`, `hmc_power_off_system`, `hmc_activate_system_profile`, +`hmc_restore_lpar_profiles` and `hmc_update_firmware` refuse with `system_held` when any +partition on the selected system is held. Their existing CLI mirrors enforce the same rule. +Release all holds on that system first; neither a matching `hold_id` nor ownership override +bypasses it. The entire tool is covered, including rename-only modification and validate-only +profile activation. This is a check at system resolution, not a lock spanning an HMC job: +coordinate handoff with operations already admitted before the hold was placed. + +A hold binds only this host's front ends. The library API, other hosts, the HMC GUI, direct +HMC commands, `hmc_run_command` and `hmcpctl raw` remain unfenced. Other unchecked tools are +listed in the handoff document. While any hold exists, a mutation that names its partition +only by name over SSH resolves the name through REST first; when REST cannot identify it, +the call is refused with `partition_hold_unverified`. `hmc_prepare_host_handoff` places and releases holds (ADR 0329). `prepare` with `hold` true and a `consumer_label` places the partition's hold and returns its `hold_id` only to the placing agent diff --git a/docs/workflow/specs/2026-11-04-system-write-hold-fence-design.md b/docs/workflow/specs/2026-11-04-system-write-hold-fence-design.md new file mode 100644 index 000000000..4f912903a --- /dev/null +++ b/docs/workflow/specs/2026-11-04-system-write-hold-fence-design.md @@ -0,0 +1,151 @@ +# System-wide writes respect external-management holds + +Issue #1887; scope token `q1887-92586ba1`; full-spec, M/250, iterating. +Decision: [ADR 0343](../../adr/0343-system-write-hold-fence.md). +Source baseline: `0dcb93630a66efcae7aed85d3265ba85f8289c58`. + +## Problem and scope + +The partition hook cannot see writes that resolve only a managed system. The operator chose +"Refuse while any system hold exists": holders release first, without an override or a set +of tokens. This extends the existing front-end resolver-time check to the six tools below. +It does not turn the local store into a distributed lock or fence library/raw HMC writers. + +### Inventory and affected callers + +The non-read `TOOL_SECURITY` records with `target_kind == "managed_system"` contain 23 tools. +Names in this table have the `hmc_` prefix. The checked set is exactly these six: + +| Tool | Existing operation owner and effect | CLI mirror | +| --- | --- | --- | +| modify_system | operations/systems/core.py: modify_system writes platform configuration, including LPAR start policy | none | +| power_on_system | operations/systems/core.py: power_system may auto-start partitions | systems power-on | +| power_off_system | operations/systems/core.py: power_system stops the platform and its partitions | systems power-off | +| activate_system_profile | operations/systems/profiles.py: activate_system_profile starts contained members | systems profile-activate | +| restore_lpar_profiles | operations/lpar/configuration.py: restore_system_lpar_profiles rewrites partition profiles | none | +| update_firmware | operations/updates/service.py: update_firmware submits platform/VIOS/I/O update work | none | + +These operation paths call `resource_identity.resolve_system_uuid` before their first +mutation. Restore reaches it through `operations/lpar/ownership.py` before ownership override. +Firmware's version read precedes it; profile activation's argument validation precedes it. +The marker applies to the whole tool: even rename-only modify and validate-only activation +refuse on a held system. This deliberately avoids argument-dependent exceptions. + +The other 17 records are classified by the resources their current code writes: + +- create_system_profile, update_system_profile, delete_system_profile change system-profile + container membership/metadata, not the contained partition profiles or running state + (`operations/systems/profiles.py`, `_metadata`). +- create_lpar, create_vios, deploy_partition_template create new partitions; their creation + paths do not rewrite an existing held partition (`operations/lpar/workflows.py`, + `operations/vios/core.py`, `operations/templates/core.py`). Resource competition remains outside + the requested existing-partition fence. +- set_vios_fc_port_label, remove_vios_fc_port_label, create_vios_vfc_group_label, + update_vios_vfc_group_label, remove_vios_vfc_group_label edit label resources, not partition + configuration (`server_tools/vios/labels.py`). +- create_virtual_network, delete_virtual_network change virtual-network resources + (`server_tools/virtualization/network.py`); remove_memory_pool removes an unused pool only + after dedicated-memory checks (`ssh/memory.py`). Shared-resource protection is not added. +- set_sriov_adapter_mode currently verifies already-selected mode and refuses transitions + (`server_tools/virtualization/pcie.py`); it submits no admitted mode change. +- backup_lpar_profiles and delete_backup_lpar_profile change backup artifacts, not current + partition profiles (`server_tools/lpar/profiles.py`). + +The existing partition fence remains unchanged. Direct HMC/raw commands and library fencing +are excluded under ADR 0193 D6; protection beyond system tools affecting partitions belongs +to the operator. The inventory explains the boundary instead of calling all system tools safe. + +## Architecture and behavior + +`ToolSecurity.system_partition_write: bool = False` records this classification explicitly +in the decorator. Reject a true marker on a read tool, a non-managed-system target, or a +record lacking the required managed-system selector. Add the marker to the six declarations. +Keep `hold_guarded` as the partition-token predicate: the system marker does not inject +`hold_id` into schemas or `--hold-id` into CLI help. + +Copy the marker into `CallContext` in `authorized()` and `CLICommand.invoke`, using the existing +mirror table. Nested `hmc_invoke_tool` dispatch gets the selected tool's marker. Existing +context restoration and worker propagation continue to apply. No handler signature changes. + +Extend the installed `PartitionGuard` with an optional system-check callback; the existing +server/CLI `HOLD_GUARD` supplies it and library code installs none. After resolving a name or +accepting a UUID, `resolve_system_uuid` calls that callback before returning. There is one +check path for name, lowercase UUID and uppercase UUID. Existing two-argument guard fixtures +remain valid because the new callback defaults to absent. + +The callback returns immediately unless the current call carries the system marker. For a +marked call it verifies canonical UUID shape, lowercases the lookup key, and uses the existing +non-creating `store.read_session` lifecycle to select one hold on that system. Holds on other +systems and an absent store permit the call. An unreadable/corrupt/lost store propagates its +existing refusal; it never becomes an empty hold result. No schema or state migration is needed. + +A matching hold raises `HoldRefused` with reason `system_held`, naming the system and one held +partition, advising `hmcpctl holds list` and release before retry. It includes no hold ID. +The placing agent, matching partition token, ownership override and power-operation setting +cannot waive this refusal. Hold release remains the existing explicit operation. + +`server_tools/lpar/handoff.unchecked_tools` excludes both partition-guarded tools and marked +system tools. Update `operations/lpar/handoff.UNFENCED`, MCP docs and the kdive contract to +name the system rule and retain the library/raw/other-host exclusions. No handoff field is +added or removed. ADR 0327 gets only a Status banner identifying D5's partial supersession. + +## Success + +- C1: the six-tool checked set and the other 17 system records are classified above; ADR 0343 + records the policy and tradeoffs and ADR 0327's body remains unchanged. +- C2: each of the six tools refuses `system_held` before its HMC mutation when its system has + at least one hold at the resolver check, including a hold placed by the calling agent. + The same tools can pass this check for an unheld system; all holds must be released first. +- C3: direct MCP and invoke dispatch and the three existing CLI mirrors agree; no system + `hold_id` interface appears; handoff disclosure removes exactly the newly checked tools. +- C4: unknown local store state fails closed for the checked set, reads and library calls keep + their prior behavior, and focused tests plus final guardrails pass without live HMC access. + +## Failure model + +1. **Actors and deployments** + - MCP callers and CLI operators on one host sharing its configured ADR 0190 store. + - Concurrent local processes may read/place/release holds; existing resolver-time admission. +2. **Invariants and assets at stake** + - A hold observed on the selected system blocks the six declared system tools before write. + - Other systems, reads, library callers and existing partition-token semantics retain behavior. + - Unknown store state cannot authorize a marked write; refusal text reveals no hold ID. +3. **Accepted failure classes** + - A hold placed after the resolver snapshot does not cancel an already-admitted operation; + the existing ADR 0193 hook is a pre-write check, not a lock spanning an HMC job. Operators + coordinate handoff with in-flight system operations; no new transaction/job lock is added. + - Conservative lockout of non-mutating argument variants within the six marked tools is + bounded to those tools and ends after explicit hold release, matching the operator choice. +4. **Covered elsewhere** + - Library, other hosts, GUI and raw HMC writers: ADR 0193 D6, consumer/operator coordination. + - Shared-resource competition and tools outside the six-tool set: operator responsibility. + - Store permissions/corruption/schema handling: ADR 0190; partition hold IDs: ADR 0193/0327. + +## Threat model + +- **Boundary inventory:** existing MCP/CLI arguments reach registry context, then resolved + system UUID reaches the local hold store. No new remote input, credential or database schema. +- **Actors:** callers can choose selectors, tokens and ownership flags; they cannot set internal + registry markers through public arguments. Local credential holders control store/release. +- **Controls:** registry shape validation and six-tool coverage test; existing access-policy + authorization before handler; authoritative UUID resolution and canonical shape checking; + parameterized system lookup and existing store validation; no token override or token leakage. +- **Out of scope:** hostile local store owner and independent HMC writers already control the + excluded surfaces. This does not provide protection against them or span HMC job execution. + +## Global constraints + +Python 3.11 through 3.14; CI target amd64; local host x86_64. No new dependency or facade export. +Use `just setup`; never bare `uv sync` or `uv run`. Use private TMPDIR outside the worktree. +No live HMC access. Final candidate requires `just verify` and `uv run --no-sync prek run --all-files`. + +## Validation + +Use real temporary hold stores and mocked HMC/SSH boundaries. Add registry-driven composed +MCP tests for the six tools by name/lowercase UUID/uppercase UUID, directly and via invoke; +assert `system_held` and zero HMC writes/SSH mutations. Exercise the three CLI mirrors. +Store tests cover multiple holds, release of one versus last, cross-system isolation, matching +hold ID/agent non-bypass, missing store, corrupt/lost store and malformed resolved UUID. +Keep positive paths for reads, unheld systems, library with no installed guard, and unchanged +partition-token schema behavior. Test marker validation and handoff registry coverage. +No synthetic prose tests: doc changes are reviewed; regenerate generated tool docs if needed. diff --git a/src/hmcpctl/call_context.py b/src/hmcpctl/call_context.py index a4f1977d5..a65ba9648 100644 --- a/src/hmcpctl/call_context.py +++ b/src/hmcpctl/call_context.py @@ -1,7 +1,8 @@ """The front-end call a partition resolution belongs to (ADR 0193 Decision 2, ADR 0327). The MCP tool wrapper and each CLI command record which registry tool is running, whether it -is a read, and any presented ``hold_id``. The hold hook reads it; nothing else does. A +is a read, any presented ``hold_id``, and the system-wide partition-write marker (ADR 0339). +The hold hook reads it; nothing else does. A resolution with no call context is treated as a mutation presenting no ``hold_id``. """ @@ -23,6 +24,7 @@ class CallContext: tool: str | None read: bool hold_id: str | None + system_partition_write: bool = False _CURRENT: ContextVar[CallContext | None] = ContextVar("hmcpctl_call", default=None) diff --git a/src/hmcpctl/cli_commands/app.py b/src/hmcpctl/cli_commands/app.py index 1ed664c64..168715878 100644 --- a/src/hmcpctl/cli_commands/app.py +++ b/src/hmcpctl/cli_commands/app.py @@ -70,7 +70,15 @@ def invoke(self, ctx: _click.Context) -> Any: tool = _mirrored_tool(ctx) hold_id = ctx.params.pop(_HOLD_ID_OPTION, None) read = tool is not None and TOOL_SECURITY[tool].effect == "read" - with calling(CallContext(tool=tool, read=read, hold_id=hold_id)): + with calling( + CallContext( + tool=tool, + read=read, + hold_id=hold_id, + system_partition_write=tool is not None + and TOOL_SECURITY[tool].system_partition_write, + ) + ): return super().invoke(ctx) def parse_args(self, ctx: _click.Context, args: list[str]) -> list[str]: diff --git a/src/hmcpctl/operations/logical/holds.py b/src/hmcpctl/operations/logical/holds.py index 7b37f8f70..dadad5949 100644 --- a/src/hmcpctl/operations/logical/holds.py +++ b/src/hmcpctl/operations/logical/holds.py @@ -16,7 +16,7 @@ from dataclasses import dataclass from ...call_context import HOLD_EXEMPT_TOOLS, current_call -from ...resource_identity import PartitionGuard +from ...resource_identity import PartitionGuard, is_uuid from . import store from .store import OperationRefused @@ -272,6 +272,37 @@ def enforce_hold(partition_uuid: str) -> None: ) +def enforce_system_hold(system_uuid: str) -> None: + """Refuse marked system writes while a partition on that system is held (ADR 0339).""" + call = current_call() + if call is None or not call.system_partition_write: + return + if not is_uuid(system_uuid): + raise HoldRefused( + "system_hold_unverified", + "managed-system identity is not a UUID; retry after inventory recovers", + ) + if not _locatable(): + return + with store.read_session() as conn: + row = ( + None + if conn is None + else conn.execute( + "SELECT partition_uuid FROM holds WHERE system_uuid = ? ORDER BY partition_uuid LIMIT 1", + (system_uuid.lower(),), + ).fetchone() + ) + if row is not None: + raise HoldRefused( + "system_held", + f"system {system_uuid} has a hold on partition {row['partition_uuid']}; " + "use hmcpctl holds list and hmcpctl holds release to release its holds before retrying", + ) + + HOLD_GUARD = PartitionGuard( - check=enforce_hold, names_need_resolution=_names_need_resolution + check=enforce_hold, + names_need_resolution=_names_need_resolution, + check_system=enforce_system_hold, ) diff --git a/src/hmcpctl/operations/lpar/handoff.py b/src/hmcpctl/operations/lpar/handoff.py index 229004b0c..abe52e32a 100644 --- a/src/hmcpctl/operations/lpar/handoff.py +++ b/src/hmcpctl/operations/lpar/handoff.py @@ -38,7 +38,7 @@ "ssh_reachability", "host_identity", ) -# ADR 0193 Decision 6 and ADR 0327 Decision 5: what a hold does not fence. +# ADR 0193 Decision 6 and ADR 0339: what the partition/system hold checks do not fence. UNFENCED = ( "the hmcpctl library API, which kdive calls", "hmcpctl on other hosts", @@ -47,7 +47,7 @@ "hmcpctl raw", ( "every non-read hmcpctl tool the hold hook does not check, listed in " - "unchecked_tools (ADR 0327 Decision 5); hmc_invoke_tool is checked as the tool " + "unchecked_tools (ADR 0327 Decision 5, ADR 0339); hmc_invoke_tool is checked as the tool " "it dispatches" ), ( diff --git a/src/hmcpctl/resource_identity.py b/src/hmcpctl/resource_identity.py index 036c28d5a..9d445d806 100644 --- a/src/hmcpctl/resource_identity.py +++ b/src/hmcpctl/resource_identity.py @@ -27,17 +27,19 @@ class PartitionGuard: *check* refuses a partition UUID's resolution by raising. *names_need_resolution* says whether a partition the caller named only by name must be resolved so *check* can see it. + *check_system* optionally checks resolved system UUIDs for system-wide writes (ADR 0339). """ check: Callable[[str], None] names_need_resolution: Callable[[], bool] + check_system: Callable[[str], None] | None = None _PARTITION_GUARD: PartitionGuard | None = None def install_partition_guard(guard: PartitionGuard | None) -> None: - """Install *guard* for every later partition resolution in this process, or remove it.""" + """Install *guard* for later partition/system resolution in this process, or remove it.""" global _PARTITION_GUARD _PARTITION_GUARD = guard @@ -108,17 +110,19 @@ def summary_matching(summaries: list[Any], value: str) -> dict[str, Any] | None: async def resolve_system_uuid(hmc: HMCClient, value: str) -> str: """Resolve a managed-system name or pass through its UUID.""" - if is_uuid(value): - return value - summary = summary_matching(await hmc.list_system_summaries(), value) - if summary is None: - raise ResourceNotFoundError( - "managed system", - value, - f"No managed system named {value!r} found. " - "Use hmc_list_systems to list available systems.", - ) - return str(summary["UUID"]) + if not is_uuid(value): + summary = summary_matching(await hmc.list_system_summaries(), value) + if summary is None: + raise ResourceNotFoundError( + "managed system", + value, + f"No managed system named {value!r} found. " + "Use hmc_list_systems to list available systems.", + ) + value = str(summary["UUID"]) + if _PARTITION_GUARD is not None and _PARTITION_GUARD.check_system is not None: + _PARTITION_GUARD.check_system(value) + return value def _named(name: object) -> str | None: diff --git a/src/hmcpctl/server_tools/lpar/handoff.py b/src/hmcpctl/server_tools/lpar/handoff.py index 2341241a7..152c76032 100644 --- a/src/hmcpctl/server_tools/lpar/handoff.py +++ b/src/hmcpctl/server_tools/lpar/handoff.py @@ -55,7 +55,7 @@ def unchecked_tools(tool_security: Mapping[str, ToolSecurity]) -> list[str]: - """The non-read tools the hold hook never checks (ADR 0327 Decision 5). + """The non-read tools the hold hook never checks (ADR 0327 Decision 5, ADR 0339). The exempt tools are named on their own in the document, and ``hmc_invoke_tool`` is left out because the tool it dispatches is checked as if called directly. @@ -65,6 +65,7 @@ def unchecked_tools(tool_security: Mapping[str, ToolSecurity]) -> list[str]: for name, security in tool_security.items() if security.effect != "read" and not hold_guarded(name, security) + and not security.system_partition_write and name not in HOLD_EXEMPT_TOOLS and name != INVOKE_TOOL_NAME ) diff --git a/src/hmcpctl/server_tools/lpar/profiles.py b/src/hmcpctl/server_tools/lpar/profiles.py index 40943cd45..49dbf4aa9 100644 --- a/src/hmcpctl/server_tools/lpar/profiles.py +++ b/src/hmcpctl/server_tools/lpar/profiles.py @@ -149,6 +149,7 @@ def hmc_backup_lpar_profiles( @tool( effect="destructive", operation="lpar_profile.restore", + system_partition_write=True, target_kind="managed_system", exhaustive_targets=False, ) diff --git a/src/hmcpctl/server_tools/systems/core.py b/src/hmcpctl/server_tools/systems/core.py index 54873bb90..80c3532b6 100644 --- a/src/hmcpctl/server_tools/systems/core.py +++ b/src/hmcpctl/server_tools/systems/core.py @@ -292,7 +292,12 @@ def hmc_get_system( ) -@tool(effect="mutate", operation="system.modify", target_kind="managed_system") +@tool( + effect="mutate", + operation="system.modify", + system_partition_write=True, + target_kind="managed_system", +) def hmc_modify_system( system_name_or_uuid: str, new_name: str | None = None, @@ -333,7 +338,12 @@ def hmc_modify_system( ) -@tool(effect="mutate", operation="system.power_on", target_kind="managed_system") +@tool( + effect="mutate", + operation="system.power_on", + system_partition_write=True, + target_kind="managed_system", +) def hmc_power_on_system( system_name_or_uuid: str, wait: bool = False, @@ -367,7 +377,12 @@ def hmc_power_on_system( ) -@tool(effect="destructive", operation="system.power_off", target_kind="managed_system") +@tool( + effect="destructive", + operation="system.power_off", + system_partition_write=True, + target_kind="managed_system", +) def hmc_power_off_system( system_name_or_uuid: str, immediate: bool = False, diff --git a/src/hmcpctl/server_tools/systems/profiles.py b/src/hmcpctl/server_tools/systems/profiles.py index 4b749b71d..747586059 100644 --- a/src/hmcpctl/server_tools/systems/profiles.py +++ b/src/hmcpctl/server_tools/systems/profiles.py @@ -188,6 +188,7 @@ def hmc_delete_system_profile( @tool( effect="destructive", operation="system_profile.activate", + system_partition_write=True, target_kind="managed_system", exhaustive_targets=False, ) diff --git a/src/hmcpctl/server_tools/updates.py b/src/hmcpctl/server_tools/updates.py index 6f47687d7..4e767af01 100644 --- a/src/hmcpctl/server_tools/updates.py +++ b/src/hmcpctl/server_tools/updates.py @@ -193,7 +193,12 @@ def hmc_vios_upgrade( ) -@tool(effect="destructive", operation="update.firmware", target_kind="managed_system") +@tool( + effect="destructive", + operation="update.firmware", + system_partition_write=True, + target_kind="managed_system", +) def hmc_update_firmware( system_name_or_uuid: str, platform_update: PlatformUpdateParameter, diff --git a/src/hmcpctl/tool_registry.py b/src/hmcpctl/tool_registry.py index 4bb7e93f1..00f57f63b 100644 --- a/src/hmcpctl/tool_registry.py +++ b/src/hmcpctl/tool_registry.py @@ -214,6 +214,7 @@ class ToolSecurity: targets: tuple[TargetSelector, ...] = () connection_argument: str | None = "profile" exhaustive_targets: bool = False + system_partition_write: bool = False @dataclass(frozen=True) @@ -362,6 +363,7 @@ def _call_context(name: str, security: ToolSecurity, hold_id: object) -> CallCon tool=name, read=security.effect == "read", hold_id=hold_id if isinstance(hold_id, str) else None, + system_partition_write=security.system_partition_write, ) @@ -585,6 +587,16 @@ def validate_security(security: ToolSecurity, handler: Callable[..., Any]) -> No "policy targets table would have nothing to bind on" ) + if security.system_partition_write and ( + security.effect == "read" + or security.target_kind != "managed_system" + or not any(t.kind == "managed_system" and t.required for t in security.targets) + ): + raise ValueError( + f"{name}: system_partition_write requires a non-read managed-system tool " + "with a required managed-system selector" + ) + _validate_arguments(security, inspect.signature(handler).parameters, name) if security.target_kind == "none": @@ -635,6 +647,7 @@ def __init__( extra_targets: Iterable[tuple[TargetKind, str]], connection_argument: str | None, exhaustive_targets: bool, + system_partition_write: bool, ) -> None: self._definitions = definitions self._effect = effect @@ -643,6 +656,7 @@ def __init__( self._extra_targets = extra_targets self._connection_argument = connection_argument self._exhaustive_targets = exhaustive_targets + self._system_partition_write = system_partition_write def __call__(self, fn: ToolHandlerT) -> ToolHandlerT: name = getattr(fn, "__name__", "") @@ -651,6 +665,7 @@ def __call__(self, fn: ToolHandlerT) -> ToolHandlerT: operation=self._operation, target_kind=self._target_kind, connection_argument=self._connection_argument, + system_partition_write=self._system_partition_write, ) try: targets = build_targets(fn, self._extra_targets) @@ -681,6 +696,7 @@ def tool( extra_targets: Iterable[tuple[TargetKind, str]] = (), connection_argument: str | None = "profile", exhaustive_targets: bool = True, + system_partition_write: bool = False, ) -> HandlerDecorator: return _ToolCollector( self.definitions, @@ -690,6 +706,7 @@ def tool( extra_targets=extra_targets, connection_argument=connection_argument, exhaustive_targets=exhaustive_targets, + system_partition_write=system_partition_write, ) def register_tools( diff --git a/tests/app/test_cli_holds.py b/tests/app/test_cli_holds.py index bb0a86853..439196e47 100644 --- a/tests/app/test_cli_holds.py +++ b/tests/app/test_cli_holds.py @@ -123,3 +123,24 @@ def test_cli_reads_of_a_held_partition_pass(hmc): _place() result = RUNNER.invoke(app, ["lpars", "show", LPAR, "--system", SYS]) assert "partition_held" not in result.output + + +@pytest.mark.parametrize( + "command,extra", + [ + ("power-on", ["--yes"]), + ("power-off", ["--yes"]), + ("profile-activate", ["profile-a", "--action", "validate"]), + ], +) +def test_system_cli_mirrors_refuse_held_system(hmc, command, extra): + from conftest import assert_no_mutating_requests + + from hmcpctl.call_context import current_call + + _place() + result = RUNNER.invoke(app, ["systems", command, "sys-R1", *extra]) + assert result.exit_code == 1, result.output + assert "system_held" in result.output + assert current_call() is None + assert_no_mutating_requests(hmc) diff --git a/tests/app/test_host_handoff_tool.py b/tests/app/test_host_handoff_tool.py index ac269d197..ee8f34450 100644 --- a/tests/app/test_host_handoff_tool.py +++ b/tests/app/test_host_handoff_tool.py @@ -145,11 +145,13 @@ def test_unchecked_tools_match_the_registry(): for name, security in TOOL_SECURITY.items() if security.effect != "read" and not hold_guarded(name, security) + and not security.system_partition_write and name not in HOLD_EXEMPT_TOOLS and name != "hmc_invoke_tool" ) assert document["unchecked_tools"] == expected - assert {"hmc_run_command", "hmc_restore_lpar_profiles"} <= set(expected) + assert "hmc_run_command" in expected + assert "hmc_restore_lpar_profiles" not in expected assert not {"hmc_power_lpar", "hmc_invoke_tool", HANDOFF} & set(expected) assert "hmc_capture_lpar_console" not in expected diff --git a/tests/app/test_kdive_tier_a_contract.py b/tests/app/test_kdive_tier_a_contract.py index 79559180f..24d7d576a 100644 --- a/tests/app/test_kdive_tier_a_contract.py +++ b/tests/app/test_kdive_tier_a_contract.py @@ -84,6 +84,7 @@ "consumer_label", "document", "partition_held", + "system_held", "action", "unfenced", "unchecked_tools", diff --git a/tests/app/test_system_hold_coverage.py b/tests/app/test_system_hold_coverage.py new file mode 100644 index 000000000..b6f9e61b4 --- /dev/null +++ b/tests/app/test_system_hold_coverage.py @@ -0,0 +1,146 @@ +"""System hold checks at composed MCP and resolver boundaries (ADR 0339).""" + +import asyncio +from unittest.mock import AsyncMock, Mock + +import httpx +import pytest +from conftest import assert_no_mutating_requests, mock_hmc, mock_system_summary # noqa: F401 +from fastmcp import Client +from fastmcp.exceptions import ToolError + +from hmcpctl.call_context import CallContext, calling, current_call +from hmcpctl.cli_commands.legacy_policy import compile_legacy_policy +from hmcpctl.client.core import HMCClient +from hmcpctl.operations.logical import holds +from hmcpctl.resource_identity import install_partition_guard, resolve_system_uuid +from hmcpctl.server import TOOL_SECURITY, create_mcp +from hmcpctl.ssh import transport + +SYS = "0b3f9a4e-1c2d-4e5f-8a9b-0c1d2e3f4a5b" +LPAR = "7d1e2f3a-4b5c-4d6e-8f70-819a2b3c4d5e" +ARGUMENTS = { + "hmc_modify_system": {"new_name": "renamed"}, + "hmc_power_on_system": {}, + "hmc_power_off_system": {"immediate": True}, + "hmc_activate_system_profile": {"name": "profile-a", "action": "validate"}, + "hmc_restore_lpar_profiles": { + "file_path": "backup", + "restore_type": 3, + "system_wide_restore_approved": True, + "ownership_override": True, + }, + "hmc_update_firmware": { + "platform_update": { + "SystemFirmwareUpdate": {"UpdateType": "Update", "UpdateOrder": 1} + } + }, +} + + +@pytest.fixture +def environment(monkeypatch, mock_hmc): # noqa: F811 + monkeypatch.setenv("HMC_HOST", "hmc.test") + monkeypatch.setenv("HMC_USER", "hscroot") + monkeypatch.setenv("HMC_PASSWORD", "abc123") + mock_system_summary(mock_hmc, SYS, "sys-R1") + monkeypatch.setattr( + HMCClient, + "get_console_info", + AsyncMock( + return_value={ + "Resource": { + "VersionInfo": { + "Version": "11", + "Release": "1", + "ServicePackName": "1111", + } + } + } + ), + ) + ssh = Mock(side_effect=AssertionError("SSH reached")) + monkeypatch.setattr(transport.asyncssh, "connect", ssh) + mock_hmc.route(method__in=["POST", "PUT", "DELETE"]).mock( + return_value=httpx.Response(500, text="write reached") + ) + app = create_mcp( + compile_legacy_policy( + TOOL_SECURITY, ("",), include_arbitrary_command=False + ), + catalog="registered", + ) + return app, mock_hmc, ssh + + +def test_system_marker_set_is_exact(): + assert { + name + for name, s in TOOL_SECURITY.items() + if getattr(s, "system_partition_write", False) + } == set(ARGUMENTS) + + +@pytest.mark.parametrize("tool", ARGUMENTS) +def test_each_system_tool_refuses_before_write_direct_and_invoke(environment, tool): + app, router, ssh = environment + placed = holds.place_hold( + SYS, LPAR, consumer_label="consumer", agent_id="hmcpctl", connection="c" + ) + + async def check(): + async with Client(app) as client: + listed = {t.name: t for t in await client.list_tools()} + assert "hold_id" not in listed[tool].input_schema["properties"] + for selector in (SYS, SYS.upper(), "sys-R1"): + arguments = {"system_name_or_uuid": selector, **ARGUMENTS[tool]} + for name, args in ( + (tool, arguments), + ("hmc_invoke_tool", {"name": tool, "arguments": arguments}), + ): + with pytest.raises(ToolError, match="system_held") as error: + await client.call_tool(name, args) + assert placed.hold_id not in str(error.value) + + asyncio.run(check()) + assert_no_mutating_requests(router) + ssh.assert_not_called() + assert current_call() is None + + +def test_unheld_system_can_reach_write_and_reads_ignore_holds(environment): + app, router, _ = environment + holds.place_hold(LPAR, SYS, consumer_label="other", agent_id="a", connection="c") + + async def check(): + async with Client(app) as client: + with pytest.raises(ToolError, match="write reached"): + await client.call_tool( + "hmc_power_on_system", {"system_name_or_uuid": SYS} + ) + holds.place_hold( + SYS, LPAR, consumer_label="held", agent_id="a", connection="c" + ) + await client.call_tool("hmc_list_systems", {"state": "unlisted-state"}) + + asyncio.run(check()) + assert any(c.request.url.path.endswith("/do/PowerOn") for c in router.calls) + + +@pytest.mark.asyncio +async def test_resolver_rejects_malformed_identity_only_with_frontend_guard(): + client = AsyncMock() + client.list_system_summaries.return_value = [ + {"UUID": "bad", "SystemName": "sys-R1"} + ] + context = CallContext("system-write", False, None, system_partition_write=True) + holds.place_hold(SYS, LPAR, consumer_label="held", agent_id="a", connection="c") + with calling(context): + assert await resolve_system_uuid(client, SYS) == SYS + assert await resolve_system_uuid(client, "sys-R1") == "bad" + install_partition_guard(holds.HOLD_GUARD) + with pytest.raises(holds.HoldRefused, match="system_hold_unverified"): + await resolve_system_uuid(client, "sys-R1") + client.list_system_summaries.return_value = [] + with pytest.raises(ValueError, match="No managed system"): + await resolve_system_uuid(client, "missing") diff --git a/tests/unit/test_holds.py b/tests/unit/test_holds.py index 8140e3d01..7e80a9dfa 100644 --- a/tests/unit/test_holds.py +++ b/tests/unit/test_holds.py @@ -326,3 +326,50 @@ def test_a_misconfigured_state_directory_refuses(monkeypatch, override): assert _reason( _enforce, CallContext("hmc_power_lpar", read=False, hold_id=None) ) == ("state_dir_unresolved") + + +def test_system_hold_requires_last_release_even_with_matching_token(): + placed = _place() + holds.place_hold( + SYS, OTHER, consumer_label="second", agent_id="agent-a", connection="c" + ) + context = CallContext( + "system-write", False, placed.hold_id, system_partition_write=True + ) + with calling(context): + holds.enforce_system_hold(OTHER) + for partition in (LPAR, OTHER): + with pytest.raises(HoldRefused, match="system_held") as error: + holds.enforce_system_hold(SYS.upper()) + assert placed.hold_id not in str(error.value) + assert "hmcpctl holds list" in str(error.value) + holds.operator_release(partition) + holds.enforce_system_hold(SYS) + assert current_call() is None + + +def test_system_check_ignores_unmarked_calls_and_does_not_create_store(): + holds.enforce_system_hold(SYS) + with calling(CallContext("system-write", False, None, system_partition_write=True)): + holds.enforce_system_hold(SYS) + with pytest.raises(HoldRefused, match="system_hold_unverified"): + holds.enforce_system_hold("invalid") + assert not store.state_dir().exists() + + +@pytest.mark.parametrize("damage", ["corrupt", "lost", "relative"]) +def test_system_write_refuses_unknown_store_but_reads_pass(damage, monkeypatch): + _place() + if damage == "corrupt": + (store.state_dir() / store.DB_NAME).write_bytes(b"garbage" * 200) + elif damage == "lost": + (store.state_dir() / store.DB_NAME).unlink() + else: + monkeypatch.setenv(store.STATE_DIR_ENV, "relative/state") + with calling(CallContext("read", True, None)): + holds.enforce_system_hold(SYS) + with ( + calling(CallContext("system-write", False, None, system_partition_write=True)), + pytest.raises(OperationRefused), + ): + holds.enforce_system_hold(SYS) diff --git a/tests/unit/test_tool_registry.py b/tests/unit/test_tool_registry.py index 2f3caa12d..b296b677b 100644 --- a/tests/unit/test_tool_registry.py +++ b/tests/unit/test_tool_registry.py @@ -629,3 +629,35 @@ def deny(*_args) -> None: with pytest.raises(RuntimeError, match="denied"): asyncio.run(guarded("lab")) assert reached == [] + + +@pytest.mark.parametrize( + "effect,kind,argument", + [ + ("read", "managed_system", "system_name_or_uuid"), + ("mutate", "lpar", "system_name_or_uuid"), + ("mutate", "managed_system", "other"), + ], +) +def test_system_write_marker_requires_mutation_and_required_system_selector( + effect, kind, argument +): + def handler(**kwargs): + pass + + handler.__signature__ = inspect.Signature( + [ + inspect.Parameter( + argument, inspect.Parameter.POSITIONAL_OR_KEYWORD, annotation=str + ) + ] + ) + tool, _, _ = tool_module() + with pytest.raises(ValueError, match="system_partition_write"): + tool( + effect=effect, + operation="system.write", + target_kind=kind, + connection_argument=None, + system_partition_write=True, + )(handler)