From 0cea6cccf41771420e409564a2a7dbd653e01ee9 Mon Sep 17 00:00:00 2001 From: Martin Sawczynski Date: Thu, 30 Apr 2026 23:04:00 +0100 Subject: [PATCH] fix(workflow): security + maintainability fixes from T0 review HIGH: narrow transport fail-open to 404-only (endpoint not deployed); all other errors (5xx, 401, timeout, parse) now fail closed in WorkflowAccessValidator.validate() via re-raise + BLOCKED_RESULT guard HIGH: strict-deny enforcement for empty enterprise boolean lists in is_pam_action_allowed_by_enforcement (helpers.py) and _is_rotation_allowed_by_enforcement (discoveryrotation.py); booleans=[] in enterprise context now falls through to key-absent deny instead of returning allow HIGH: redact credentials/private-keys/tokens from debug logs in terminal_connection.py (turn_password, callback_token, guacd_params password/private-key/passphrase/client-key) MEDIUM: strict boolean arg parser for pam workflow update flags; rejects non-true/false with argparse.ArgumentTypeError MEDIUM: HHMM table decode fix divmod(100) instead of divmod(60) MEDIUM: backward-compat migration for legacy minutes-since-midnight workflow configs; values >= 1440 converted to HHMM in _check_allowed_times MEDIUM: warn on auto check-in failure with pam workflow end hint MEDIUM: fix --auto-checkout help text in both launch and tunnel to state that the lease is NOT released automatically MEDIUM: pam workflow delete pre-check now surfaces read failures as a warning instead of silently treating them as "nothing to delete" LOW: validate --wait-timeout > 0 before polling loop in both launch and tunnel paths; raises CommandError if invalid LOW: remove dead _print_transport_error helper (unreachable post HIGH-1) Made-with: Cursor --- PAM_WORKFLOW_DOCUMENTATION.md | 876 ++++++++++++++++++ WORKFLOW_BRANCH_REVIEW_T0.md | 94 ++ keepercommander/commands/discoveryrotation.py | 10 +- keepercommander/commands/pam_launch/launch.py | 12 +- .../pam_launch/terminal_connection.py | 21 +- .../commands/tunnel_and_connections.py | 10 +- .../commands/workflow/config_commands.py | 29 +- keepercommander/commands/workflow/helpers.py | 11 +- keepercommander/commands/workflow/mfa.py | 77 +- 9 files changed, 1103 insertions(+), 37 deletions(-) create mode 100644 PAM_WORKFLOW_DOCUMENTATION.md create mode 100644 WORKFLOW_BRANCH_REVIEW_T0.md diff --git a/PAM_WORKFLOW_DOCUMENTATION.md b/PAM_WORKFLOW_DOCUMENTATION.md new file mode 100644 index 000000000..a16c3c504 --- /dev/null +++ b/PAM_WORKFLOW_DOCUMENTATION.md @@ -0,0 +1,876 @@ +# PAM Workflow Command Documentation + +This document describes **PAM Workflow** support in Keeper Commander: access approval, optional check-in/check-out, multi-factor authentication, justification fields, time-bounded leases, and integration with `pam launch`, `pam tunnel start`, and `pam action rotate`. + +> **Note**: Keeper Commander supports line continuation using backslash (`\`) at the end of lines. Do not put spaces after the backslash. + +## Examples and placeholders + +Throughout this document, replace placeholders with values from your vault: + +| Placeholder | Meaning | +|-------------|---------| +| `` | Base64-url record UID for the PAM resource | +| `` | Workflow flow UID (from `pam workflow pending` or `pam workflow state`) | +| `` | Keeper account email | + +--- + +## 1. Overview — PAM Workflow (Access Approval + Time-Bounded Lease) + +**What it is** + +PAM Workflow is a **per-record policy** that can gate privileged access behind: + +- One or more **approvals** (with optional escalation approvers) +- **Time windows** (allowed days, time-of-day ranges, timezone) +- **Multi-factor authentication** (2FA through the router workflow gate) +- **Reason** and/or **ticket** requirements +- **Check-in/check-out** when configured +- A **time-bounded lease** (`accessLength`) that limits how long an approved session remains valid + +**Where it runs** + +- Commander **17.2.16** and newer expects a **workflow-aware Keeper Router** so workflow configuration and state endpoints respond normally. +- Older routers **without** workflow REST endpoints are handled via **transport fallback** (see Section 3 and Section 11): the client does not hard-block `pam launch` / `pam tunnel start` solely because workflow reads failed. + +**Scope** + +Workflow applies to **PAM resources** managed through Commander (`pamMachine`, `pamDirectory`, `pamDatabase`, and Remote Browser Isolation flows where workflow is configured on the underlying PAM record types your tenant uses). Commander’s CLI surfaces workflow on commands that touch those records. + +--- + +## 2. Prerequisites + +### Enterprise enforcement booleans + +Enterprise admins configure enforcement keys that Commander evaluates **before** workflow-specific logic on supported paths: + +| Concept | Enforcement key (exact string) | Used by | +|---------|-------------------------------|---------| +| Launch / cloud connections | `allow_launch_pam_on_cloud_connection` | `pam launch` | +| Tunnels / port forwarding | `allow_launch_pam_tunnels` | `pam tunnel start` | +| Credential rotation | `allow_rotate_credentials` | `pam action rotate` | +| Workflow administration exemption | `allow_configure_workflow_settings` | Clients use this with record ACL to determine users who **manage** workflow settings (see `is_workflow_exempt` — users with edit access **and** this permission may bypass workflow for that record). | + +**Strict-deny semantics when the key is missing** + +When an **enterprise enforcement context** exists (non-empty `booleans` list on `params.enforcements`), keys such as `allow_launch_pam_on_cloud_connection`, `allow_launch_pam_tunnels`, and workflow-related checks follow web-vault parity: if the relevant boolean **is absent** from the list, access is treated as **denied** (missing key behaves like false). See `is_pam_action_allowed_by_enforcement` in `keepercommander/commands/workflow/helpers.py`. + +Rotation uses dedicated logic in `_is_rotation_allowed_by_enforcement`: absent `allow_rotate_credentials` in an enterprise context → deny. + +### PAM Configuration `allowedSettings` + +Each PAM resource links to a **PAM Configuration** record whose DAG exposes `allowedSettings`. Commander maps DAG fields to JSON keys including: + +| JSON key | Meaning | +|----------|---------| +| `connections` | Launch / connect allowed | +| `tunneling` | Tunnel / port-forward allowed (DAG may expose `portForwards`; CLI helper maps to `tunneling`) | +| `rotation` | Manual rotation allowed | +| Remote browser isolation | Where exposed on your configuration | + +If `allowedSettings.` is **explicitly false**, Commander blocks the matching action **before** workflow auto-checkout. + +### Operational readiness + +- **Gateway online** and reachable through the router for the resource’s configuration. +- Valid **session** (`keeper login` or equivalent). + +--- + +## 3. Two-Gate Enforcement Model + +**Gate 1 — Per-user enterprise enforcement** + +Evaluated first on launch and tunnel paths via `is_pam_action_allowed_by_enforcement`: + +- Launch: key `allow_launch_pam_on_cloud_connection` +- Tunnel: key `allow_launch_pam_tunnels` + +If this gate denies, Commander prints an error and returns without contacting workflow services. + +**Gate 2 — PAM Configuration `allowedSettings`** + +Evaluated via `is_pam_config_action_allowed_for_record`: + +- Launch: action key `'connections'` +- Tunnel: action key `'tunneling'` +- Rotation: `'rotation'` checked inside `record_rotate` using `PAMConfigurationListCommand._pam_config_allowed_settings_json` + +Explicit `False` on the configuration denies the operation before workflow checkout. + +**Transport fallback (workflow router compatibility)** + +`WorkflowAccessValidator._read_workflow_config` (and related workflow state reads) catches transport failures and returns an internal sentinel so **`check_workflow_for_launch`** treats “workflow API unavailable” as **allow legacy path** — gateway remains authoritative. This avoids blocking production launches on routers that do not yet expose workflow endpoints. + +**Rotation** + +`pam action rotate` applies: + +1. `_is_rotation_allowed_by_enforcement` → must find `allow_rotate_credentials: true`; missing key in enterprise context → deny. +2. PAM config → `allowed.get('rotation') is False` → deny with configuration-scoped message. + +The legacy umbrella **`allow_pam_rotation` is not consulted** for the client gate; disabling rotation via `allow_rotate_credentials: false` is honored even if older umbrella defaults exist elsewhere. + +--- + +## 4. Command Reference — `pam workflow *` + +### 4.1 `pam workflow create` + +**Purpose**: Create workflow configuration for a PAM record. + +#### Command Syntax + +```bash +pam workflow create [OPTIONS] +``` + +#### Arguments + +| Argument | Required | Default | Description | +|----------|----------|---------|-------------| +| `record` | Yes | — | Record UID or name to configure workflow for | +| `-n`, `--approvals-needed` | No | `1` | Number of approvals required | +| `-co`, `--checkout` | No | off | Enable single-user check-in/check-out mode | +| `-sa`, `--start-on-approval` | No | off | Start access timer when approved (vs when checked out) | +| `-rr`, `--require-reason` | No | off | Require user to provide reason for access | +| `-rt`, `--require-ticket` | No | off | Require user to provide ticket number | +| `-rm`, `--require-mfa` | No | off | Require MFA verification for access | +| `-d`, `--duration` | No | `1d` | Access duration (e.g., `"2h"`, `"30m"`, `"1d"`). Default: 1d | +| `--allowed-days` | No | — | Comma-separated allowed days (e.g., `"mon,tue,wed,thu,fri"`) | +| `--time-range` | No | — | Allowed time range in HH:MM-HH:MM format (e.g., `"09:00-17:00"`) | +| `--timezone` | No | — | Timezone for allowed times (e.g., `"America/New_York"`) | +| `-u`, `--approver` | Conditional | — | User email to add as an approver. Pass multiple times to add several. Required when `--approvals-needed > 0`. Duplicates are removed automatically. | +| `--format` | No | `table` | Output format: `table` or `json` | + +#### Examples + +```bash +# Two approvers, business hours only (replace placeholders) +pam workflow create \ + --approvals-needed 2 \ + --approver alice.admin@example.com \ + --approver bob.manager@example.com \ + --require-reason \ + --duration 4h \ + --allowed-days mon,tue,wed,thu,fri \ + --time-range 09:00-17:00 \ + --timezone America/New_York +``` + +```bash +# Approvals disabled — checkout-only workflow +pam workflow create --approvals-needed 0 --checkout --duration 2h +``` + +```bash +# JSON output for automation +pam workflow create -n 1 -u approver@example.com --format json +``` + +#### Notes + +- If workflow already exists, Commander raises **`Workflow already configured for "" (<uid>).`** with hints to `pam workflow update`, `pam workflow read`, or delete-and-recreate. +- **`At least one --approver is required when --approvals-needed > 0.`** — the creator is **not** auto-added as an approver. +- If `--approvals-needed 0` but approvers are supplied, a **warning** is logged that approvers will never be required. +- Server failures after `create_workflow_config` may leave configuration without approvers; the command prints recovery **`pam workflow add-approver`** guidance. + +--- + +### 4.2 `pam workflow read` + +**Purpose**: Read and display workflow configuration. + +#### Command Syntax + +```bash +pam workflow read <record> [--format table|json] +``` + +#### Arguments + +| Argument | Required | Default | Description | +|----------|----------|---------|-------------| +| `record` | Yes | — | Record UID or name | +| `--format` | No | `table` | Output format: `table` or `json` | + +#### Examples + +```bash +pam workflow read <RECORD_UID> +pam workflow read <RECORD_UID> --format json +``` + +#### Notes + +- When no workflow exists, table mode prints **`No workflow configured for this record`** and suggests **`pam workflow create <record>`**. +- JSON mode emits `{"status": "no_workflow", "message": "No workflow configured"}`. + +--- + +### 4.3 `pam workflow update` + +**Purpose**: Update existing workflow configuration. Only specified fields change. + +#### Command Syntax + +```bash +pam workflow update <record> [OPTIONS] +``` + +#### Arguments + +| Argument | Required | Default | Description | +|----------|----------|---------|-------------| +| `record` | Yes | — | Record UID or name with workflow to update | +| `-n`, `--approvals-needed` | No | — | Number of approvals required | +| `-co`, `--checkout` | No | — | Enable/disable check-in/check-out (`true`/`false`) | +| `-sa`, `--start-on-approval` | No | — | Start timer on approval vs check-out (`true`/`false`) | +| `-rr`, `--require-reason` | No | — | Require reason (`true`/`false`) | +| `-rt`, `--require-ticket` | No | — | Require ticket (`true`/`false`) | +| `-rm`, `--require-mfa` | No | — | Require MFA (`true`/`false`) | +| `-d`, `--duration` | No | — | Access duration (e.g., `"2h"`, `"30m"`, `"1d"`) | +| `--allowed-days` | No | — | Comma-separated allowed days | +| `--time-range` | No | — | Allowed time range HH:MM-HH:MM | +| `--timezone` | No | — | Timezone for allowed times | +| `--format` | No | `table` | Output format: `table` or `json` | + +#### Examples + +```bash +pam workflow update <RECORD_UID> --duration 8h --require-mfa true +pam workflow update <RECORD_UID> -n 2 --checkout false --format json +``` + +#### Notes + +- **`No workflow found for record. Create one first with "pam workflow create"`** when nothing exists. +- **`No updates provided. Specify at least one option to update (e.g., --approvals-needed, --duration)`** if no updatable flags were passed. +- **`Approvals needed must be 0 or greater`** if `-n` is negative. + +--- + +### 4.4 `pam workflow delete` + +**Purpose**: Delete workflow configuration from a record. + +#### Command Syntax + +```bash +pam workflow delete <record> [--format table|json] +``` + +#### Arguments + +| Argument | Required | Default | Description | +|----------|----------|---------|-------------| +| `record` | Yes | — | Record UID or name to remove workflow from | +| `--format` | No | `table` | Output format: `table` or `json` | + +#### Examples + +```bash +pam workflow delete <RECORD_UID> +pam workflow delete <RECORD_UID> --format json +``` + +#### Notes + +- Pre-check prevents confusing repeat deletes: **`No workflow configured for "<title>" (<uid>). Nothing to delete.`** + +--- + +### 4.5 `pam workflow add-approver` + +**Purpose**: Add approvers to a workflow. + +#### Command Syntax + +```bash +pam workflow add-approver <record> [OPTIONS] +``` + +#### Arguments + +| Argument | Required | Default | Description | +|----------|----------|---------|-------------| +| `record` | Yes | — | Record UID or name | +| `-u`, `--user` | Conditional | — | User email to add as approver (can specify multiple times) | +| `-t`, `--team` | Conditional | — | Team name or UID to add as approver (can specify multiple times) | +| `-e`, `--escalation` | No | off | Mark as escalation approver | +| `-ea`, `--escalation-after` | No | — | Time before escalating (e.g., `"30m"`, `"1h"`). Only meaningful with `--escalation` | +| `--format` | No | `table` | Output format: `table` or `json` | + +#### Examples + +```bash +pam workflow add-approver <RECORD_UID> --user alice.admin@example.com --user bob.manager@example.com +pam workflow add-approver <RECORD_UID> --team "Platform Ops" --escalation --escalation-after 1h +``` + +#### Notes + +- **`Must specify at least one --user or --team`** +- **`--escalation-after requires --escalation flag`** +- Users and teams are **de-duplicated** (first-seen order preserved). + +--- + +### 4.6 `pam workflow remove-approver` + +**Purpose**: Remove approvers from a workflow. + +#### Command Syntax + +```bash +pam workflow remove-approver <record> [OPTIONS] +``` + +#### Arguments + +| Argument | Required | Default | Description | +|----------|----------|---------|-------------| +| `record` | Yes | — | Record UID or name | +| `-u`, `--user` | Conditional | — | User email to remove as approver | +| `-t`, `--team` | Conditional | — | Team name or UID to remove as approver | +| `--format` | No | `table` | Output format: `table` or `json` | + +#### Examples + +```bash +pam workflow remove-approver <RECORD_UID> --user former.approver@example.com +pam workflow remove-approver <RECORD_UID> --team abcTeamUidExample00001 +``` + +#### Notes + +- **`Must specify at least one --user or --team`** + +--- + +### 4.7 `pam workflow request` + +**Purpose**: Request access to a PAM resource, escalate, or cancel a pending request. + +#### Command Syntax + +```bash +pam workflow request <record> [OPTIONS] +``` + +#### Arguments + +| Argument | Required | Default | Description | +|----------|----------|---------|-------------| +| `record` | Yes | — | Record UID or name | +| `-r`, `--reason` | No | — | Reason for access request | +| `-t`, `--ticket` | No | — | External ticket/reference number | +| `-e`, `--escalate` | No | off | Escalate a pending request to escalation approvers | +| `-c`, `--cancel` | No | off | Cancel a pending or active workflow request | +| `--format` | No | `table` | Output format: `table` or `json` | + +#### Examples + +```bash +pam workflow request <RECORD_UID> --reason "INC123 investigation" --ticket INC123 +pam workflow request <RECORD_UID> --escalate +pam workflow request <RECORD_UID> --cancel +``` + +#### Notes + +- **`--cancel and --escalate cannot be used together`** +- **`--cancel cannot be used with --reason or --ticket`** +- **`No active workflow request found for this record.`** on cancel when nothing active. + +--- + +### 4.8 `pam workflow start` + +**Purpose**: Start a workflow (**check-out**). Accepts record UID/name or Flow UID. + +#### Command Syntax + +```bash +pam workflow start <uid> [--format table|json] +``` + +#### Arguments + +| Argument | Required | Default | Description | +|----------|----------|---------|-------------| +| `uid` | Yes | — | Record UID, record name, or Flow UID | +| `--format` | No | `table` | Output format: `table` or `json` | + +#### Examples + +```bash +pam workflow start <RECORD_UID> +pam workflow start <FLOW_UID> +``` + +#### Notes + +- Invalid UID: **`"<uid>" is not a valid record UID/name or flow UID`** + +--- + +### 4.9 `pam workflow end` + +**Purpose**: End a workflow (**check-in**). + +#### Command Syntax + +```bash +pam workflow end <uid> [OPTIONS] +``` + +#### Arguments + +| Argument | Required | Default | Description | +|----------|----------|---------|-------------| +| `uid` | Yes | — | Record UID, record name, or Flow UID | +| `-f`, `--force` | No | off | Force check-in: approvers can terminate another user’s active session when single-user checkout is enabled | +| `--format` | No | `table` | Output format: `table` or `json` | + +#### Examples + +```bash +pam workflow end <RECORD_UID> +pam workflow end <FLOW_UID> --force +``` + +#### Notes + +- Normal path without `--force`: **`No active workflow found for this record. The workflow may have already ended or never started.`** + +--- + +### 4.10 `pam workflow state` + +**Purpose**: Get workflow state for a record or flow. + +#### Command Syntax + +```bash +pam workflow state (--record <record> | --flow-uid <flow_uid>) [--format table|json] +``` + +#### Arguments + +| Argument | Required | Default | Description | +|----------|----------|---------|-------------| +| `-r`, `--record` | One of group | — | Record UID or name | +| `-f`, `--flow-uid` | One of group | — | Flow UID of active workflow | +| `--format` | No | `table` | Output format: `table` or `json` | + +Exactly one of `--record` or `--flow-uid` is required (mutually exclusive group). + +#### Examples + +```bash +pam workflow state --record <RECORD_UID> +pam workflow state --flow-uid <FLOW_UID> --format json +``` + +#### Notes + +- **`Invalid flow UID: "<flow_uid>"`** when `--flow-uid` is malformed. + +--- + +### 4.11 `pam workflow my-access` + +**Purpose**: List all workflow states for the **current user**. + +#### Command Syntax + +```bash +pam workflow my-access [--format table|json] +``` + +#### Arguments + +| Argument | Required | Default | Description | +|----------|----------|---------|-------------| +| `--format` | No | `table` | Output format: `table` or `json` | + +#### Examples + +```bash +pam workflow my-access +pam workflow my-access --format json +``` + +--- + +### 4.12 `pam workflow pending` + +**Purpose**: List pending approval requests for the current approver session. + +#### Command Syntax + +```bash +pam workflow pending [--format table|json] +``` + +#### Arguments + +| Argument | Required | Default | Description | +|----------|----------|---------|-------------| +| `--format` | No | `table` | Output format: `table` or `json` | + +#### Examples + +```bash +pam workflow pending +pam workflow pending --format json +``` + +#### Notes + +- Duplicate flows from the router response are collapsed; flows already approved by the current user are filtered out. + +--- + +### 4.13 `pam workflow approve` + +**Purpose**: Approve a workflow access request. + +#### Command Syntax + +```bash +pam workflow approve <flow_uid> [--format table|json] +``` + +#### Arguments + +| Argument | Required | Default | Description | +|----------|----------|---------|-------------| +| `flow_uid` | Yes | — | Flow UID of the workflow to approve | +| `--format` | No | `table` | Output format: `table` or `json` | + +#### Examples + +```bash +pam workflow approve <FLOW_UID> +pam workflow approve <FLOW_UID> --format json +``` + +#### Notes + +- **`Invalid flow UID: "<flow_uid>"`** when decoding fails. + +--- + +### 4.14 `pam workflow deny` + +**Purpose**: Deny a workflow access request. + +#### Command Syntax + +```bash +pam workflow deny <flow_uid> [-r REASON] [--format table|json] +``` + +#### Arguments + +| Argument | Required | Default | Description | +|----------|----------|---------|-------------| +| `flow_uid` | Yes | — | Flow UID of the workflow to deny | +| `-r`, `--reason` | No | — | Reason for denial | +| `--format` | No | `table` | Output format: `table` or `json` | + +#### Examples + +```bash +pam workflow deny <FLOW_UID> --reason "Does not meet change-window policy" +pam workflow deny <FLOW_UID> +``` + +#### Notes + +- **`Invalid flow UID: "<flow_uid>"`** when decoding fails. + +--- + +## 5. Time and Duration Encoding + +### `--duration` + +`WorkflowFormatter.parse_duration` accepts: + +- Suffix forms: **`Nd`**, **`Nh`**, **`Nm`** where `N` is a positive integer (e.g., `"1d"`, `"2h"`, `"30m"`). +- A bare positive integer string → interpreted as **minutes**. + +Invalid input raises: + +```text +Invalid duration format: <value>. Use a positive value like "2h", "30m", or "1d" +``` + +### `--time-range` + +Format **`HH:MM-HH:MM`**. Parsed into protobuf **`TimeOfDayRange`** with **`startTime` / `endTime` stored as HHMM integers** (for example `09:00` → `900`, `17:30` → `1730`), matching server validation (hours 0–23, minutes 0–59). + +**Known formatting caveat (`pam workflow read`, table mode)** + +The table renderer still uses **`divmod(value, 60)`** on `startTime`/`endTime` in `WorkflowReadCommand._print_table`. That arithmetic assumes **minutes-since-midnight**, not HHMM. Values remain correct on the wire and in JSON (`WorkflowFormatter.format_temporal_filter` uses HHMM-aware formatting). Operators should prefer **`--format json`** or interpret ranges via JSON export until the table path is aligned. + +### `--allowed-days` + +Comma-separated tokens; accepted aliases include short (`mon`) and long (`monday`) forms per `WorkflowFormatter.DAY_PARSE_MAP`. Unknown tokens raise **`Invalid day: "<token>". Valid: ...`**. + +### `--timezone` + +IANA name (for example **`America/New_York`**). Unknown zones fall back to local wall-clock for enforcement with debug logging. + +### Enforcement at launch + +`WorkflowAccessValidator._check_allowed_times` enforces configured **`allowedTimes`** during **`check_workflow_for_launch`** before approving interactive access. + +--- + +## 6. `pam launch` Integration + +**Program name**: `pam launch` + +### Workflow-related flags + +| Flag | Short | Default | Help text (summary) | +|------|-------|---------|---------------------| +| `--reason` | `-r` | — | Justification for workflow access when required | +| `--ticket` | `-tk` | — | External ticket/reference when required | +| `--auto-checkout` | `-aco` | off | Auto-confirm checkout when approved but not checked out | +| `--wait` | `-w` | off | Poll until approval or timeout | +| `--wait-timeout` | `-wt` | `600` | Max seconds to poll when `--wait` is set | + +### Behavior + +1. **Enterprise gate**: `allow_launch_pam_on_cloud_connection`. +2. **PAM config gate**: `connections` in `allowedSettings`. +3. **`check_workflow_for_launch`** orchestrates workflow states: + - **`no_workflow` / `needs_start`**: inline **`prompt_for_reason_ticket`** when required; submits **`request_workflow_access`** via **`submit_access_request`** (matches web vault first-launch flow). + - **`needs_action`**: same prompting when **`AC_REASON` / `AC_TICKET`** pending. + - **`ready_to_start`**: interactive **`Workflow approved. Check out '<record_uid>' now? [Y/n]:`** unless **`--auto-checkout`**. + - **`waiting`**: with **`--wait`**, polls every **8 seconds** until state changes or **`--wait-timeout`**; timeout prints **`Approval not received within <n>s.`** +4. **MFA**: When **`requireMFA`** and gateway online probe **`is_gateway_online_for_record` is `False`**, the MFA prompt is **skipped** (debug log); launch proceeds to gateway errors if applicable. +5. **Gateway resolution fallback**: If **`get_all_gateways`** misses the controller, Commander calls **`pam/get_configuration_controller`** (`configuration_controller_get`) so non-owner KSM visibility cases still resolve the gateway (matches web vault). +6. **Lease expiry**: A threading timer fires at **`expires_on_ms`** → **`shutdown_requested`**, ending the CLI session; after terminal reset the user sees **`Access expired — session terminated by workflow lease.`** +7. **Auto check-in**: On normal exit, if **`workflow_started_by_launch`** and lease did not expire, **`end_workflow`** is posted with **`ProtobufRefBuilder.workflow_ref`** — failures **`logging.debug("Auto check-in failed: ...")`** only (see Troubleshooting). +8. After **`WorkflowGate`** denies, **`pam launch`** returns **silently** (no redundant catch-all error), mirroring **`pam tunnel start`**. + +--- + +## 7. `pam tunnel start` Integration + +**Program name**: `pam tunnel start` + +### Workflow-related flags + +Same set as **`pam launch`**: **`--reason` / `-r`**, **`--ticket` / `-tk`**, **`--auto-checkout` / `-aco`**, **`--wait` / `-w`**, **`--wait-timeout` / `-wt`** (default **600**). + +### Behavior + +- Gates: **`allow_launch_pam_tunnels`** then **`tunneling`** in PAM config **`allowedSettings`**. +- **`check_workflow_for_launch`** identical orchestration path as launch (shared implementation). +- **Lease-expiry timer dedup**: **`_LEASE_EXPIRY_TIMERS_BY_RECORD`** — starting a tunnel cancels any prior timer for the same **`record_uid`** so duplicate **`Tunnel access lease expired`** messages do not stack. +- At expiry, Commander prints **`Tunnel access lease expired for <record_uid>. Server will refuse new auth requests; any in-flight SSH session will continue until you disconnect it.`** — reflects **soft-close** limits (Python cannot hard-kill existing forwarded channels). + +### Known limitation — `pam tunnel stop` does not release workflow lease + +Stopping the tunnel **does not** call **`end_workflow`**. The lease remains until server **`expiresOn`** or explicit: + +```bash +pam workflow end <RECORD_UID> +``` + +--- + +## 8. `pam action rotate` Integration + +**Program name**: `pam action rotate` + +### Enforcement + +1. **`_is_rotation_allowed_by_enforcement`** — requires explicit **`allow_rotate_credentials: true`** in enterprise **`booleans`**; absent → deny with **`Rotation is not allowed for this account by enterprise enforcement (allow_rotate_credentials).`** +2. **`PAMConfigurationListCommand._pam_config_allowed_settings_json`** → **`rotation`** must not be **`False`**; otherwise **`Rotation is disabled by the PAM Configuration [<config_uid>] for record [<record_uid>].`** + +Legacy **`allow_pam_rotation`** fallback is **not** used in this client gate. + +--- + +## 9. Approver Workflow (End-to-End Example) + +Replace placeholders before running. + +**1. Administrator creates workflow with two approvers** + +```bash +pam workflow create <RECORD_UID> \ + --approvals-needed 2 \ + --approver alice.admin@example.com \ + --approver bob.manager@example.com \ + --require-reason \ + --require-ticket \ + --checkout \ + --duration 2h \ + --timezone UTC +``` + +Expected (table): + +```text +Workflow created successfully + +Record: Prod SSH Bastion (<RECORD_UID>) +Approvals needed: 2 +Check-in/out: Yes +Duration: 2 hours +Requires reason: Yes +Requires ticket: Yes +Approvers: alice.admin@example.com, bob.manager@example.com +``` + +**2. Requester launches — prompted inline, blocked pending approval** + +```bash +pam launch <RECORD_UID> +``` + +Typical intermediate messages: + +```text +Workflow requires a justification. +... +Access request submitted. + +Workflow access is pending: waiting for Approval Required. +Your request is being processed. Please wait for approval. +``` + +**3. Each approver reviews and approves** + +Approver Alice: + +```bash +pam workflow pending +pam workflow approve <FLOW_UID_FROM_PENDING_TABLE> +``` + +Approver Bob repeats **`pam workflow approve`** with the same **`FLOW_UID`** (until approvals satisfied). + +**4. Requester retries with wait** + +```bash +pam launch <RECORD_UID> --wait --wait-timeout 900 --auto-checkout +``` + +Typical: + +```text +Waiting for approval... (timeout: 900s; press Ctrl+C to cancel) +Checked out. +Launching connection to Prod SSH Bastion... +``` + +**5. Session ends — auto check-in** + +Exit the terminal session normally (Ctrl+C double-tap per CLI guidance). When **`pam launch`** performed checkout, **`end_workflow`** runs automatically unless lease expired first. + +If checkout must be released manually: + +```bash +pam workflow end <RECORD_UID> +``` + +--- + +## 10. Troubleshooting + +| Symptom | Explanation / Next step | +|---------|-------------------------| +| **`Workflow already configured for "<title>" (<uid>).`** | Use **`pam workflow update`** or **`pam workflow delete`** then recreate. | +| **`At least one --approver is required when --approvals-needed > 0.`** | Pass **`-u`** for each approver or set **`--approvals-needed 0`**. | +| **`This record is protected by a workflow.`** / must **`pam workflow request`** | No active flow yet — **`pam launch`** may submit automatically when prompted; or run **`pam workflow request`** explicitly. | +| Launch succeeds but workflow reads previously failed | Transport fallback — router may lack workflow REST endpoints; verify router/gateway deployment supports workflow APIs if policy requires client-side enforcement. | +| **`Auto check-in failed`** | Only **`logging.debug`** today — check **`DEBUG`** logs or run **`pam workflow end <RECORD_UID>`**. | +| Tunnel stopped but lease active | Expected — run **`pam workflow end <RECORD_UID>`**. | +| **`Access expired — session terminated by workflow lease.`** | Lease **`expiresOn`** reached — **`pam launch`** hard-disconnect from CLI perspective. | +| **`Tunnel access lease expired...`** | Timer fired — server rejects new tunnel auth; disconnect existing SSH clients manually if needed. | +| **`Workflow access is outside the allowed time window.`** | Outside **`allowedTimes`** — retry inside policy window or update configuration. | +| **`PAM tunnels are not allowed... (allow_launch_pam_tunnels)`** | Enterprise enforcement — admin must grant tunnel entitlement. | +| **`pam launch aborted: ... allow_launch_pam_on_cloud_connection`** | Enterprise enforcement denies launch. | +| **`Rotation is not allowed... (allow_rotate_credentials)`** | Enterprise enforcement denies rotation. | + +--- + +## 11. Backward Compatibility + +| Commander version | Behavior | +|-------------------|----------| +| **17.2.15 and earlier** | No **`pam workflow *`** commands; **`pam launch`**, **`pam tunnel start`**, **`pam action rotate`** use legacy gates only where implemented. | +| Router **without** workflow REST API | Transport fallback → **`WorkflowAccessValidator`** allows legacy launch/tunnel path; gateway/server policy still applies. | +| **17.2.16** | Full workflow CLI surface shipped (merge **`2d911529`** — PR **#1997** squash — plus related release commits). | + +--- + +## 12. Changelog — Commits in This Shipment + +The **first 23** commits below were delivered as **PR #1997** (“Workflow pam launch compat”), squashed into upstream Commander at **`2d911529`** and released as **Commander 17.2.16**. + +The **final two** rows (**`a19d6e88`**, **`79a709a5`**) were independent upstream fixes bundled into the same release for completeness. + +| Commit | Area | Summary | +|--------|------|---------| +| `8f4aaa37` | Workflow gate | Allow on transport error so prod routers without workflow API don't hard-block legacy launch/tunnel | +| `b8dd42ea` | PAM launch | Drop redundant catch-all error after workflow gate, mirror tunnel | +| `3711e014` | Rotation enforcement | Drop legacy allow_pam_rotation fallback to honor allow_rotate_credentials:false | +| `43c2d83b` | Enforcement helpers | Strict-deny PAM enforcement helpers when key absent in enterprise context | +| `03e30176` | PAM workflow | Handle no_workflow / needs_start with inline prompt + initial request submission | +| `790eef62` | PAM workflow delete | Pre-check existing config and bail with clear message | +| `e3b17dc2` | PAM tunnel | Dedup lease-expiry timer per record, document soft-close limitation | +| `828a2257` | PAM launch | Fall back to pam/get_configuration_controller when get_controllers misses | +| `c0a7638e` | PAM workflow create | Pre-check existing config and fail with an actionable message | +| `f8d51dba` | PAM workflow create | Drop creator auto-add; require --approver when approvalsNeeded > 0 | +| `7a18ac69` | PAM workflow create | Skip auto-add of creator as approver when approvalsNeeded=0 | +| `3d238705` | PAM workflow | Encode TimeOfDayRange as HHMM (server format), not minutes-since-midnight | +| `44d2e3fd` | PAM launch / tunnel | Per-user enforcement gate before PAM-config gate | +| `9bacc795` | PAM launch / tunnel | Gate on PAM config allowedSettings before lease auto-checkout | +| `38c9d182` | PAM tunnel | Document that stop does not release the workflow lease | +| `1f408ed2` | PAM action rotate | Enforce the same two gates as the web vault | +| `98a68cca` | PAM workflow | Skip MFA prompt when gateway is offline | +| `7b0ca7d4` | PAM workflow | --wait polls for approval before launching | +| `fb9f08be` | PAM workflow | Auto check-in pam launch session when launch owns the checkout | +| `a72b6093` | PAM workflow | Prompt to check out when launch hits WS_READY_TO_START | +| `a3c859a8` | PAM workflow | Prompt for reason/ticket inline at launch instead of bailing | +| `5e484793` | PAM workflow | Hard-disconnect pam launch and pam tunnel at lease expiry | +| `6d21c6ae` | PAM workflow | Enforce allowedTimes window in launch gate | +| `a19d6e88` | pam project import / extend | Fix --sample-data + load enterprise data lazily (PR #1996) | +| `79a709a5` | Share folder | Fix ShareFolderCommand | + +--- + +## 13. See Also + +- [Keeper Documentation — PAM Overview](https://docs.keeper.io/privileged-access-manager) +- [Keeper Documentation — PAM Configuration](https://docs.keeper.io/privileged-access-manager) +- Commander **`--syntax-help`** on record commands — cross-reference **`RECORD_ADD_DOCUMENTATION.md`** for typed field syntax when editing PAM records. + +--- + +## Appendix — Implementation references (for maintainers) + +| Component | Module | +|-----------|--------| +| Workflow CLI — create/read/update/delete/approvers | `keepercommander/commands/workflow/config_commands.py` | +| Workflow CLI — request/start/end | `keepercommander/commands/workflow/requester_commands.py` | +| Workflow CLI — pending/approve/deny | `keepercommander/commands/workflow/approver_commands.py` | +| Workflow CLI — state/my-access | `keepercommander/commands/workflow/state_commands.py` | +| Formatters, resolver, enforcement helpers | `keepercommander/commands/workflow/helpers.py` | +| **`WorkflowAccessValidator`**, **`check_workflow_for_launch`**, MFA | `keepercommander/commands/workflow/mfa.py` | +| **`pam launch`** | `keepercommander/commands/pam_launch/launch.py` | +| **`pam tunnel start`** | `keepercommander/commands/tunnel_and_connections.py` | +| **`pam action rotate`** gates | `keepercommander/commands/discoveryrotation.py` | diff --git a/WORKFLOW_BRANCH_REVIEW_T0.md b/WORKFLOW_BRANCH_REVIEW_T0.md new file mode 100644 index 000000000..e0195bd5d --- /dev/null +++ b/WORKFLOW_BRANCH_REVIEW_T0.md @@ -0,0 +1,94 @@ +I’ll review the branch from the changed code and the smoke-test doc, then map findings back to the review axes and commits. I’m starting with the branch diff and the three primary Python files so the report is grounded in actual behavior. +`rg` is not installed in this workspace, so I’m falling back to standard shell search. One referenced file path does not exist at the exact location given, so I’m locating the actual registry module before assessing the lease logic. +The branch has deleted `keepercommander/commands/tunnel_registry.py`; the current lease expiry/dedup implementation appears embedded in `tunnel_and_connections.py`. I’m expanding the review to the workflow helper modules because the validator, transport fallback, approval polling, and command validation live there rather than in the launch terminal module. +The validator’s transport-error branch does allow launch/tunnel when workflow API reads fail. I’m checking whether that is constrained elsewhere by server-side checks, config gates, or action enforcement, and whether command paths can distinguish intentional denials from unavailable workflow endpoints. +I’ve found one concrete regression in the workflow read display path: create/update encode time as HHMM, but the table formatter still decodes with `/60`, so table output lies for values with minutes. I’m now mapping the smoke document against the actual commit list to separate code issues from coverage gaps. +**Scope Notes** + +[OK] I reviewed the changed Python paths plus workflow helper modules where the actual gate logic lives; `keepercommander/commands/tunnel_registry.py` is deleted on this branch, with lease timer logic now in `tunnel_and_connections.py`. +Recommendation: Update the review/smoke references to the current file layout. + +[LOW] The branch is 24 commits above `f4c35150`, while the smoke doc says “all 11 commits” and uses stale commit hashes. +Recommendation: Refresh the smoke doc commit map before treating it as merge evidence. + +**Security Review** + +[HIGH] Workflow transport fallback is too broad: `WorkflowAccessValidator.validate()` allows access on any exception from `read_workflow_config`, `get_user_access_state`, or `get_workflow_state` after a config may exist ([mfa.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/workflow/mfa.py:89)). +Recommendation: Fail open only for explicit “endpoint unsupported/not deployed” capability errors; fail closed for authz, 5xx, timeout, malformed response, and any state-read failure after a workflow config is known. + +[HIGH] Strict-deny enforcement is not actually strict when `params.enforcements["booleans"]` is empty; launch/tunnel and rotation return allow for an empty list ([helpers.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/workflow/helpers.py:124), [discoveryrotation.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/discoveryrotation.py:3045)). +Recommendation: Treat an enterprise enforcement context with an empty/missing specific key as deny; reserve allow only for truly non-enterprise/no-enforcement context. + +[HIGH] Debug logging can leak secrets: `guacd_params` includes username/password/private key/passphrase and is copied into `offer_data`, then logged wholesale; TURN password and callback token are also logged ([terminal_connection.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/pam_launch/terminal_connection.py:1123), [terminal_connection.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/pam_launch/terminal_connection.py:1348), [terminal_connection.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/pam_launch/terminal_connection.py:1742)). +Recommendation: Redact secret-bearing keys before logging, or log only structural fields and lengths. + +[OK] The workflow exemption enforces the intended AND condition: it first requires `allow_configure_workflow_settings`, then requires owner/edit access from owner/meta/shared-folder caches ([helpers.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/workflow/helpers.py:49)). +Recommendation: Add a regression test for edit-only, enforcement-only, and both-together cases. + +[OK] Approval-to-checkout race handling is mostly delegated to `start_workflow`; client code does not assume approval is sufficient and catches checkout failure ([mfa.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/workflow/mfa.py:724)). +Recommendation: Preserve the ready flow UID across checkout so auto check-in still works if the post-checkout revalidation has a transient failure. + +[OK] Rotation now mirrors the launch/tunnel two-gate pattern for enforcement and PAM config, and intentionally does not workflow-gate rotation ([discoveryrotation.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/discoveryrotation.py:3288)). +Recommendation: Fix the empty-booleans strict-deny issue there too. + +**Code Quality Review** + +[MEDIUM] `pam workflow update` boolean parsers silently turn any non-`true` value into `False`, so typos like `--require-mfa maybe` disable MFA ([config_commands.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/workflow/config_commands.py:371)). +Recommendation: Use a strict boolean parser accepting only `true/false` with a `CommandError` for invalid values. + +[MEDIUM] HHMM encoding is only partly fixed: create/update and JSON formatting use HHMM, but table read output still decodes with `divmod(..., 60)` ([config_commands.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/workflow/config_commands.py:330)). +Recommendation: Change table formatting to `divmod(..., 100)` and add tests for `09:30-17:45`. + +[MEDIUM] There is no backward-compat handling for workflow configs previously written as minutes-since-midnight; the new validator compares as HHMM ([mfa.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/workflow/mfa.py:206)). +Recommendation: Detect impossible HHMM values and document/migrate old branch-created configs where feasible. + +[LOW] `--wait-timeout` accepts zero/negative values; the poller then waits at least 8 seconds while printing the invalid timeout ([mfa.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/workflow/mfa.py:556)). +Recommendation: Validate `--wait-timeout > 0` in both launch and tunnel command paths. + +[LOW] `_print_transport_error()` and the `transport_error` comment path are now unreachable after fail-open transport handling ([mfa.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/workflow/mfa.py:345)). +Recommendation: Remove dead code or reinstate explicit fail-closed transport states. + +[LOW] Tunnel lease timer dedup is per-record and cancels prior timers, so it avoids duplicate timer accumulation; stopped tunnels can still leave one timer alive until lease expiry ([tunnel_and_connections.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/tunnel_and_connections.py:775)). +Recommendation: Keep the dedup, but consider canceling or marking timers when workflows are manually ended to avoid stale notices. + +**Usability / Ergonomics** + +[MEDIUM] `pam tunnel start --auto-checkout` help says the lease is released when the tunnel ends, but the implementation intentionally does not release tunnel-owned workflow leases ([tunnel_and_connections.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/tunnel_and_connections.py:529)). +Recommendation: Fix the help text to say the lease remains until expiry or `pam workflow end`. + +[MEDIUM] Auto check-in failure after `pam launch` is debug-only, leaving users unaware that a launch-owned lease stayed checked out ([launch.py](/Users/martin/Downloads/Cursor%20tests/Commander/keepercommander/commands/pam_launch/launch.py:1859)). +Recommendation: Emit a warning with `pam workflow end <record-or-flow>` guidance when auto check-in fails. + +[LOW] Inline prompts are usable interactively, but CI safety depends on callers remembering `--reason`, `--ticket`, and `--auto-checkout`; there is no strict non-interactive mode. +Recommendation: Add `--non-interactive` or fail fast when required workflow inputs are missing and stdin is not a TTY. + +[OK] Create/delete pre-check messages are actionable when the read succeeds. +Recommendation: For delete, distinguish “no config” from “could not verify config” instead of treating read failure as nothing to delete. + +**Test Coverage Review** + +[HIGH] The smoke doc has no transport-error fallback test and does not verify that transport failures cannot bypass intentional denies. +Recommendation: Add router/API fault tests for unsupported endpoint vs timeout/5xx/access-denied, including an approved-denied workflow state. + +[MEDIUM] Auto check-in failure is not tested; section 8 covers success and preserving manual checkout only. +Recommendation: Mock `end_workflow` failure after launch exit and assert user-visible warning plus lease remains. + +[OK] `--wait` timeout is documented as a manual variation at smoke lines 550-552. +Recommendation: Add invalid timeout coverage for `0` and negative values. + +[MEDIUM] No explicit smoke coverage for: `8f4aaa37`, `828a2257`, `790eef62`, `c0a7638e`, `43c2d83b`, `3711e014`, `7a18ac69`, `f8d51dba`, and the unrelated `79a709a5` / `a19d6e88`. +Recommendation: Add focused sections or remove “covers all commits” language. + +[MEDIUM] Partial-only coverage for `3d238705` and `e3b17dc2`: the doc exercises time ranges and lease expiry, but not HHMM read/table compatibility or duplicate timer suppression. +Recommendation: Add exact expected read output and repeat-start-before-expiry checks. + +**Overall** + +Quality score: 3/5. + +Top 3 action items before merge: +1. Replace broad workflow transport fail-open with explicit unsupported-endpoint compatibility handling. +2. Fix strict-deny enforcement for empty enterprise boolean lists across launch/tunnel/rotation. +3. Redact debug logs that currently include credentials, private keys, TURN passwords, and callback tokens. + +<!-- REVIEW DONE --> diff --git a/keepercommander/commands/discoveryrotation.py b/keepercommander/commands/discoveryrotation.py index 4d53a9d9a..6f9819ba5 100644 --- a/keepercommander/commands/discoveryrotation.py +++ b/keepercommander/commands/discoveryrotation.py @@ -3085,9 +3085,15 @@ def _is_rotation_allowed_by_enforcement(params): enforcements = getattr(params, 'enforcements', None) if not enforcements or not isinstance(enforcements, dict): return True - booleans = enforcements.get('booleans') or [] - if not isinstance(booleans, list) or not booleans: + booleans = enforcements.get('booleans') + if booleans is None: + # Key fully absent — not yet configured; allow defensively. return True + if not isinstance(booleans, list): + # Malformed payload — allow defensively. + return True + # Empty list: enterprise context, no boolean enforcements set. + # Fall through to key lookup; missing key → deny (matches WV). for b in booleans: if isinstance(b, dict) and b.get('key') == 'allow_rotate_credentials': return bool(b.get('value')) diff --git a/keepercommander/commands/pam_launch/launch.py b/keepercommander/commands/pam_launch/launch.py index 6d64cf993..158320630 100644 --- a/keepercommander/commands/pam_launch/launch.py +++ b/keepercommander/commands/pam_launch/launch.py @@ -365,8 +365,9 @@ class PAMLaunchCommand(Command): parser.add_argument('--auto-checkout', '-aco', required=False, dest='workflow_auto_checkout', action='store_true', help='Auto-confirm workflow check-out when the record is approved but not yet ' - 'checked out (skips the interactive Y/n prompt). The lease is released ' - 'automatically when the launch session ends.') + 'checked out (skips the interactive Y/n prompt). ' + 'Note: the lease is NOT released when the launch session ends ' + '— use `pam workflow end` to release it manually.') parser.add_argument('--wait', '-w', required=False, dest='workflow_wait', action='store_true', help='When the workflow is waiting on approval, poll until approved ' @@ -853,13 +854,16 @@ def execute(self, params: KeeperParams, **kwargs): workflow_started_by_launch = False try: from ..workflow import check_workflow_for_launch + _wait_timeout_val = int(kwargs.get('workflow_wait_timeout') or 600) + if _wait_timeout_val <= 0: + raise CommandError('', '--wait-timeout must be a positive integer (seconds)') gate = check_workflow_for_launch( params, record_uid, reason=kwargs.get('workflow_reason'), ticket=kwargs.get('workflow_ticket'), auto_checkout=bool(kwargs.get('workflow_auto_checkout')), wait=bool(kwargs.get('workflow_wait')), - wait_timeout=int(kwargs.get('workflow_wait_timeout') or 600), + wait_timeout=_wait_timeout_val, ) if not gate.allowed: # Orchestrator (`check_workflow_for_launch`) already prints @@ -2046,7 +2050,7 @@ def _remote_key_ctrl_c() -> None: _post_request_to_router(params, 'end_workflow', rq_proto=flow_ref) logging.debug("Auto check-in: released workflow lease.") except Exception as e: - logging.debug("Auto check-in failed: %s", e) + logging.warning("Auto check-in failed: %s. Release manually with: pam workflow end <flow_uid>", e) exit_pam_launch_terminal_rust_logging(rust_log_token) signal.signal(signal.SIGINT, original_handler) diff --git a/keepercommander/commands/pam_launch/terminal_connection.py b/keepercommander/commands/pam_launch/terminal_connection.py index bf27de0be..dfc158899 100644 --- a/keepercommander/commands/pam_launch/terminal_connection.py +++ b/keepercommander/commands/pam_launch/terminal_connection.py @@ -1353,7 +1353,9 @@ def _open_terminal_webrtc_tunnel(params: KeeperParams, } # Debug: Log settings to verify control_channel_label is present - logging.debug(f"WebRTC settings before create_tube: {json.dumps(webrtc_settings, default=str)}") + _safe_webrtc = {k: ('***redacted***' if k in ('turn_password', 'callback_token') else v) + for k, v in webrtc_settings.items()} + logging.debug("WebRTC settings before create_tube: %s", json.dumps(_safe_webrtc, default=str)) # Register the encryption key in the global conversation store register_conversation_key(conversation_id, symmetric_key) @@ -1735,7 +1737,22 @@ def _open_terminal_webrtc_tunnel(params: KeeperParams, # if 'protocol_specific' in settings and settings['protocol_specific']: # offer_data["protocolSettings"] = settings['protocol_specific'] - logging.debug(f"Sending initial offer with connection parameters: {json.dumps(offer_data, indent=2)}") + _SECRET_GUACD_FIELDS = frozenset({ + 'password', 'private-key', 'passphrase', 'client-key', + }) + def _redact_offer(d): + if not isinstance(d, dict): + return d + out = {} + for k, v in d.items(): + if k == 'guacd_params' and isinstance(v, dict): + out[k] = {kk: ('***redacted***' if kk in _SECRET_GUACD_FIELDS else vv) + for kk, vv in v.items()} + else: + out[k] = v + return out + logging.debug("Sending initial offer with connection parameters: %s", + json.dumps(_redact_offer(offer_data), indent=2, default=str)) data_bytes = string_to_bytes(json.dumps(offer_data)) encrypted_data = tunnel_encrypt(symmetric_key, data_bytes) diff --git a/keepercommander/commands/tunnel_and_connections.py b/keepercommander/commands/tunnel_and_connections.py index 0117a4382..b9098e0d3 100644 --- a/keepercommander/commands/tunnel_and_connections.py +++ b/keepercommander/commands/tunnel_and_connections.py @@ -562,8 +562,9 @@ class PAMTunnelStartCommand(Command): pam_cmd_parser.add_argument('--auto-checkout', '-aco', required=False, dest='workflow_auto_checkout', action='store_true', help='Auto-confirm workflow check-out when the record is approved but not yet ' - 'checked out (skips the interactive Y/n prompt). The lease is released ' - 'automatically when the tunnel ends.') + 'checked out (skips the interactive Y/n prompt). ' + 'Note: the lease is NOT released when the tunnel stops ' + '— use `pam workflow end` to release it manually.') pam_cmd_parser.add_argument('--wait', '-w', required=False, dest='workflow_wait', action='store_true', help='When the workflow is waiting on approval, poll until approved ' @@ -680,13 +681,16 @@ def execute(self, params, **kwargs): workflow_expires_on_ms = 0 try: from .workflow import check_workflow_for_launch + _wt_tunnel = int(kwargs.get('workflow_wait_timeout') or 600) + if _wt_tunnel <= 0: + raise CommandError('', '--wait-timeout must be a positive integer (seconds)') gate = check_workflow_for_launch( params, record_uid, reason=kwargs.get('workflow_reason'), ticket=kwargs.get('workflow_ticket'), auto_checkout=bool(kwargs.get('workflow_auto_checkout')), wait=bool(kwargs.get('workflow_wait')), - wait_timeout=int(kwargs.get('workflow_wait_timeout') or 600), + wait_timeout=_wt_tunnel, ) if not gate.allowed: return diff --git a/keepercommander/commands/workflow/config_commands.py b/keepercommander/commands/workflow/config_commands.py index 28ca9c71f..ed3bdc98d 100644 --- a/keepercommander/commands/workflow/config_commands.py +++ b/keepercommander/commands/workflow/config_commands.py @@ -25,6 +25,17 @@ from .helpers import RecordResolver, ProtobufRefBuilder, WorkflowFormatter, sanitize_router_error +def _strict_bool_arg(val): + """Strict argparse type: accepts only 'true' or 'false' (case-insensitive).""" + if val.lower() == 'true': + return True + if val.lower() == 'false': + return False + raise argparse.ArgumentTypeError( + f"Invalid boolean value {val!r}: expected 'true' or 'false'" + ) + + def _add_approvers_to_workflow(params, record_uid, record_name, users=None, teams=None, is_escalation=False, escalation_after_ms=0): @@ -371,15 +382,15 @@ class WorkflowUpdateCommand(Command): ) parser.add_argument('record', help='Record UID or name with workflow to update') parser.add_argument('-n', '--approvals-needed', type=int, help='Number of approvals required') - parser.add_argument('-co', '--checkout', type=lambda x: x.lower() == 'true', + parser.add_argument('-co', '--checkout', type=_strict_bool_arg, help='Enable/disable check-in/check-out (true/false)') - parser.add_argument('-sa', '--start-on-approval', type=lambda x: x.lower() == 'true', + parser.add_argument('-sa', '--start-on-approval', type=_strict_bool_arg, help='Start timer on approval vs check-out (true/false)') - parser.add_argument('-rr', '--require-reason', type=lambda x: x.lower() == 'true', + parser.add_argument('-rr', '--require-reason', type=_strict_bool_arg, help='Require reason (true/false)') - parser.add_argument('-rt', '--require-ticket', type=lambda x: x.lower() == 'true', + parser.add_argument('-rt', '--require-ticket', type=_strict_bool_arg, help='Require ticket (true/false)') - parser.add_argument('-rm', '--require-mfa', type=lambda x: x.lower() == 'true', + parser.add_argument('-rm', '--require-mfa', type=_strict_bool_arg, help='Require MFA (true/false)') parser.add_argument('-d', '--duration', type=str, help='Access duration (e.g., "2h", "30m", "1d")') parser.add_argument('--allowed-days', type=str, @@ -487,8 +498,12 @@ def execute(self, params: KeeperParams, **kwargs): rq_proto=ref, rs_type=workflow_pb2.WorkflowConfig, ) except Exception as e: - logging.debug('Pre-check read_workflow_config failed: %s', e) - existing = None + raise CommandError( + '', + f'Could not verify workflow config for "{record.title}" ({record_uid}): ' + f'{sanitize_router_error(e)}. Cannot determine if a workflow exists — ' + f'check connectivity and try again.' + ) if not existing: raise CommandError( '', diff --git a/keepercommander/commands/workflow/helpers.py b/keepercommander/commands/workflow/helpers.py index 21a3ac4dd..51677bd05 100644 --- a/keepercommander/commands/workflow/helpers.py +++ b/keepercommander/commands/workflow/helpers.py @@ -121,11 +121,16 @@ def is_pam_action_allowed_by_enforcement(params: KeeperParams, enforcement_key: enforcements = getattr(params, 'enforcements', None) if not enforcements or not isinstance(enforcements, dict): return True - booleans = enforcements.get('booleans') or [] - if not isinstance(booleans, list) or not booleans: - # Enforcement context but no booleans at all — treat as + booleans = enforcements.get('booleans') + if booleans is None: + # Key fully absent from the enforcement dict — treat as # "not yet configured" and allow; gateway will gate. return True + if not isinstance(booleans, list): + # Malformed payload shape — allow defensively. + return True + # Empty list: enterprise context but no boolean enforcements set. + # Fall through to key lookup; if key is absent, deny (matches WV). for b in booleans: if isinstance(b, dict) and b.get('key') == enforcement_key: return bool(b.get('value')) diff --git a/keepercommander/commands/workflow/mfa.py b/keepercommander/commands/workflow/mfa.py index d9a977eac..8309be0c1 100644 --- a/keepercommander/commands/workflow/mfa.py +++ b/keepercommander/commands/workflow/mfa.py @@ -15,6 +15,7 @@ from typing import NamedTuple, Optional from ..pam.router_helper import _post_request_to_router +from ...error import KeeperApiError from ...display import bcolors from ...params import KeeperParams from ...proto import workflow_pb2, router_pb2 @@ -86,7 +87,11 @@ def validate(self, silent_actionable: bool = False) -> dict: # gate without opening a real security gap. Old behavior (block # with a banner) was correct for QA but a hard regression on prod # legacy launches that have never seen workflow. - config = self._read_workflow_config() + try: + config = self._read_workflow_config() + except Exception as _e: + # Non-404 error from the workflow API — fail closed (deny). + return dict(self._BLOCKED_RESULT) if config is _TRANSPORT_ERROR: logging.debug( 'read_workflow_config unavailable for %s; falling through to ' @@ -103,7 +108,10 @@ def validate(self, silent_actionable: bool = False) -> dict: return self._blocked('outside_time_window') no_approvals = config.parameters and config.parameters.approvalsNeeded == 0 - workflow = self._find_active_workflow() + try: + workflow = self._find_active_workflow() + except Exception as _e: + return dict(self._BLOCKED_RESULT) if workflow is _TRANSPORT_ERROR: logging.debug( 'get_user_access_state unavailable for %s; treating as no ' @@ -111,7 +119,10 @@ def validate(self, silent_actionable: bool = False) -> dict: ) return dict(self._DEFAULT_RESULT) if workflow is None and no_approvals: - workflow = self._get_workflow_state_by_record() + try: + workflow = self._get_workflow_state_by_record() + except Exception as _e: + return dict(self._BLOCKED_RESULT) if workflow is _TRANSPORT_ERROR: logging.debug( 'get_workflow_state unavailable for %s; allowing. Gateway ' @@ -159,9 +170,19 @@ def _read_workflow_config(self): self.params, 'read_workflow_config', rq_proto=ref, rs_type=workflow_pb2.WorkflowConfig, ) + except KeeperApiError as e: + if e.result_code == 404: + # HTTP 404 = endpoint not deployed on this router; fail open so + # legacy routers without the workflow API don't block launches. + logging.debug('read_workflow_config: endpoint not deployed (HTTP 404) for %s; failing open', self.record_uid) + return _TRANSPORT_ERROR + # Any other API error (5xx, 401, timeout/connection) — fail closed. + logging.warning('read_workflow_config: unexpected error for %s (%s); blocking access', self.record_uid, e) + raise except Exception as e: - logging.debug('Failed to read workflow config for %s: %s', self.record_uid, e) - return _TRANSPORT_ERROR + # Non-API exception (parse error, protobuf decode, etc.) — fail closed. + logging.warning('read_workflow_config: unexpected exception for %s: %s; blocking access', self.record_uid, e) + raise def _find_active_workflow(self): try: @@ -169,9 +190,15 @@ def _find_active_workflow(self): self.params, 'get_user_access_state', rs_type=workflow_pb2.UserAccessState, ) + except KeeperApiError as e: + if e.result_code == 404: + logging.debug('get_user_access_state: endpoint not deployed (HTTP 404); failing open') + return _TRANSPORT_ERROR + logging.warning('get_user_access_state: unexpected error (%s); blocking access', e) + raise except Exception as e: - logging.debug('Failed to get user access state: %s', e) - return _TRANSPORT_ERROR + logging.warning('get_user_access_state: unexpected exception: %s; blocking access', e) + raise if user_state and user_state.workflows: for wf in user_state.workflows: @@ -207,16 +234,32 @@ def _check_allowed_times(self, config) -> bool: # TimeOfDayRange.startTime / .endTime are HHMM-encoded integers # (server-validated: HH in 0-23, MM in 0-59). e.g. 03:00 -> 300, # 17:30 -> 1730. Compare current wall-clock in the same encoding. + # + # Backward-compat: older workflow configs stored times as + # minutes-since-midnight (range 0–1439). Values >= 1440 are + # unambiguously legacy HHMM (valid HHMM max is 2359). Values + # < 1440 are already valid HHMM (0000-1439 covers 00:00-14:39). + # We convert only values >= 1440 to avoid double-converting + # current valid HHMM values. + def _to_hhmm(t): + if t >= 1440: + # Legacy minutes-since-midnight → HHMM + h, m = divmod(t, 60) + return h * 100 + m + return t # Already HHMM + current_hhmm = now.hour * 100 + now.minute in_range = False for r in at.timeRanges: - if r.startTime <= r.endTime: - if r.startTime <= current_hhmm <= r.endTime: + start = _to_hhmm(r.startTime) + end = _to_hhmm(r.endTime) + if start <= end: + if start <= current_hhmm <= end: in_range = True break else: # range crosses midnight (e.g. 22:00-06:00) - if current_hhmm >= r.startTime or current_hhmm <= r.endTime: + if current_hhmm >= start or current_hhmm <= end: in_range = True break if not in_range: @@ -338,13 +381,15 @@ def _get_workflow_state_by_record(self): self.params, 'get_workflow_state', rq_proto=state_query, rs_type=workflow_pb2.WorkflowState, ) + except KeeperApiError as e: + if e.result_code == 404: + logging.debug('get_workflow_state: endpoint not deployed (HTTP 404) for %s; failing open', self.record_uid) + return _TRANSPORT_ERROR + logging.warning('get_workflow_state: unexpected error for %s (%s); blocking access', self.record_uid, e) + raise except Exception as e: - logging.debug('Failed to get workflow state for %s: %s', self.record_uid, e) - return _TRANSPORT_ERROR - - def _print_transport_error(self, action: str): - print(f"\n{bcolors.FAIL}Unable to {action} — the server may be unavailable.{bcolors.ENDC}") - print("Access is blocked until workflow status can be verified. Please try again later.\n") + logging.warning('get_workflow_state: unexpected exception for %s: %s; blocking access', self.record_uid, e) + raise def _print_no_workflow(self): print(f"\n{bcolors.WARNING}This record is protected by a workflow.{bcolors.ENDC}")