New ssh key api

[camsmith]

No description provided.

[Copilot]

🟡 Changes recommended

The new SSH key API methods introduce inconsistent error handling (raw BadResponseException) compared to established ApiResponseException wrapping, which should be aligned before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR migrates the legacy CLI’s SSH key management from the older account/SSH-key representation to a newer SSH keys API, updating the CLI to use SHA-256 (OpenSSH-style) fingerprints and a new SSH key model.

Changes:

• Added a dedicated Platformsh\Cli\Model\SshKey model and updated commands/services to use new fields (id, label, sha256).
• Implemented new SSH key API methods in Api (list/get/add/delete) including pagination handling and cache invalidation.
• Updated fingerprint calculation to return OpenSSH-style SHA256: fingerprints and added PHPUnit coverage for the fingerprint behavior.

File summaries

File	Description
legacy/tests/Service/SshKeyTest.php	Adds tests for SHA-256/OpenSSH fingerprint generation and invalid key handling.
legacy/src/Service/SshKey.php	Switches account-key matching from MD5 to SHA-256/OpenSSH fingerprint format.
legacy/src/Service/Api.php	Adds SSH keys API endpoints (list/get/add/delete), pagination, and dedicated caching.
legacy/src/Model/SshKey.php	Introduces a CLI-level SSH key model for the new API representation.
legacy/src/Event/LoginRequiredEvent.php	Ensures login option values are consistently stringified for shell escaping.
legacy/src/Command/SshKey/SshKeyListCommand.php	Updates displayed/output columns to match new key fields (label, sha256).
legacy/src/Command/SshKey/SshKeyDeleteCommand.php	Updates deletion flow to use new key IDs and new API delete operation.
legacy/src/Command/SshKey/SshKeyAddCommand.php	Updates add flow to use new API and handles “already registered” (409) response.
legacy/phpstan-baseline.neon	Removes a baseline entry that is no longer applicable after key-ID typing changes.

Review details

Suppressed comments (2)

legacy/src/Service/Api.php:994

• In getSshKey(), non-404 failures are rethrown as BadResponseException. For consistency with the rest of the API layer (and to preserve the richer error details formatting), it should rethrow ApiResponseException::create(...) instead.

            if ($e->getResponse()->getStatusCode() === 404) {
                return null;
            }
            throw $e;
        }

legacy/src/Service/Api.php:1027

• deleteSshKey() currently lets BadResponseException bubble up directly. To keep error handling consistent with other direct HTTP calls, catch BadResponseException and rethrow ApiResponseException::create(...).

        $this->getHttpClient()->request('DELETE', $this->sshKeysUrl() . '/' . rawurlencode($id));
        $this->clearSshKeysCache();

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[upsun-dispatch]

📋 PR Summary

This PR migrates the legacy CLI's SSH key handling from the platform client library's account-based endpoints to the new SSH key API. A dedicated Platformsh\Cli\Model\SshKey value object is introduced, and Api gains methods to list (with pagination and caching), fetch, add and delete keys, with the ssh-key:add, ssh-key:delete and ssh-key:list commands and the SshKey service updated to the new model fields (id, label, sha256, active). New PHPUnit tests cover pagination/caching, error handling, cache invalidation, fingerprint matching and the list command's output; the latest push fixes the test config env keys to use the PLATFORMSH_CLI_ prefix and always looks up a local identity in ssh-key:list, including for inactive keys.

Changes

Layer / File(s)	Summary
SSH key model and API
legacy/src/Model/SshKey.php	New value object representing an SSH key from the new API (id, sha256, value, label, active, user_id, timestamps).
legacy/src/Service/Api.php	Adds SSH key API methods: current user id lookup, paginated and cached key listing with a circular-pagination guard, plus get/add/delete that invalidate the cached list.
Commands
legacy/src/Command/SshKey/SshKeyAddCommand.php	Uses the new Api::addSshKey call and the new key model fields.
legacy/src/Command/SshKey/SshKeyDeleteCommand.php	Deletes via the new API by string key id, dropping the previous numeric-id handling.
legacy/src/Command/SshKey/SshKeyListCommand.php	Renders the new fields (id, label, SHA-256 fingerprint, active) with active in the default columns, and always resolves the matching local identity path.
Services and supporting code
legacy/src/Service/SshKey.php	Reads account key fingerprints from the new model and filters to active keys.
legacy/src/Event/LoginRequiredEvent.php	Small adjustment following the API changes.
legacy/phpstan-baseline.neon	Removes baseline entries that no longer apply after the migration.
Tests
legacy/tests/Service/ApiSshKeyTest.php	Tests listing across pages with caching, circular-pagination rejection, 404 handling, error detail surfacing and cache invalidation on mutations; env overrides now use the PLATFORMSH_CLI_ prefix and are asserted.
legacy/tests/Service/SshKeyTest.php	Tests SHA-256 fingerprint computation, invalid key failure, and that inactive account keys are not matched locally; the home directory override now uses the prefixed env key and is asserted.
legacy/tests/Command/SshKey/SshKeyListCommandTest.php	New command test asserting that an inactive key shows 'No' in the active column and still reports its local key path.

[upsun-dispatch]

Note

Reviewed — No blocking findings · 🔵 3 minor points

🔍 Full review · 9 files reviewed

🔍 What this review checked

• The test vector's expected fingerprint SHA256:Zc8rf0C3ZFAVs8mnWl4r6jKmJN8kutsoBK2h4UkXPp8 is the correct unpadded base64 SHA-256 of that key blob (recomputed independently).
• No references to the old fields ($key->title, ->fingerprint, ->key_id) or to Platformsh\Client\Model\SshKey remain, and getLegacyAccountInfo has no leftover callers.
• addSshKey() and deleteSshKey() both invalidate the new session-scoped 'ssh-keys' cache key, and both commands re-warm it with getSshKeys(true).
• The removed phpstan-baseline entry is the only baseline entry referencing SshKey code, so no baseline entry is left unmatched.
• The is_array() cache check makes a cached empty key list a hit rather than a miss, unlike the old truthiness check.

Verification. The diff adds legacy/tests/Service/SshKeyTest.php covering only getPublicKeyFingerprint (valid and invalid key); nothing covers the new Api::getSshKeys pagination/caching, getSshKey 404 handling, addSshKey 409 handling or deleteSshKey. The legacy-php job in .github/workflows/ci.yml runs php-cs-fixer, phpstan (level 8 + baseline) and ./scripts/test/unit.sh over these files.

Review details

• Commit: 8672834
• Model: claude-opus-5

🔵 Minor points

• legacy/src/Service/Api.php:988 — The new SSH key methods let raw Guzzle exceptions escape, unlike every other direct HTTP call in this codebase (e.g. Api::getTasks() at line ~1914, TaskRunCommand, TeamUserAddCommand, which all do throw ApiResponseException::create($e->getRequest(), $e->getResponse(), $e)). getSshKey() maps only 404 to null; combined with the removal of the is_numeric($id) guard in SshKeyDeleteCommand, ssh-key:delete <malformed-id> now sends the ID to the API and a 400/422 response surfaces as an unhandled GuzzleHttp\Exception\ClientException with a truncated-body message instead of "SSH key not found" or a formatted API error. The same applies to getSshKeys(), addSshKey() (non-409 errors) and deleteSshKey().
• legacy/src/Service/Api.php:959 — The pagination loop in getSshKeys() has no page limit and no guard against a repeated URL: it follows _links.next.href unconditionally, so if the API returns a next link on the last page that resolves to the URL just requested (a common pattern for APIs that always emit next), the loop issues the same GET forever while appending duplicate items to $items, hanging the command and growing memory without bound.
• legacy/src/Model/SshKey.php:22 — SshKey::$active is parsed from the API but never read anywhere: Api::getSshKeys() returns inactive keys, SshKeyListCommand has no active column, and Service\SshKey::listAccountKeyFingerprints() feeds inactive keys' fingerprints into findIdentityMatchingPublicKeys(). A user whose only account key is inactive gets it auto-selected as the SSH IdentityFile by selectIdentity() and is told by WelcomeCommand/SshDiagnostics that a local key matching the account exists, while SSH authentication fails with no explanation.

---

Review 1 of 10 for this pull request · View the full run

[upsun-dispatch]

Warning

Changes suggested — 🟡 1 warning · 🔵 2 minor points

🔁 Incremental · 5 files reviewed

🔍 What this review checked

• ApiResponseException::wrapGuzzleException() exists in platformsh/client at the locked revision and recreates the exception with get_class($e), so SshKeyAddCommand's catch (BadResponseException) still matches wrapped client errors.
• addSshKey() deliberately rethrows the raw 409 so the command's duplicate-key message still fires, while all other statuses get API error details appended.
• The circular-pagination guard keys $visitedUrls on the resolved absolute URL, so a relative next href pointing back at the first page is detected.
• The anonymous test subclass matches Api::__construct(config, cache, output, io, ...) and the getHttpClient(): ClientInterface / getMyUserId(bool): string signatures it overrides.
• listAccountKeyFingerprints()' array_filter result keeps string values only, so in_array() in findIdentityMatchingPublicKeys() is unaffected by the non-list keys.

Verification. The diff adds legacy/tests/Service/ApiSshKeyTest.php (pagination, 404→null, error details, cache invalidation) and one SshKeyTest case, run by the legacy-php CI job via ./scripts/test/unit.sh alongside make lint-phpstan/make lint-php-cs-fixer; note the new SshKeyTest case does not actually exercise the inactive-key filter (see finding), and no test covers SshKeyListCommand's new active column.

Review details

• Commits: 8672834..488f1d4
• Model: claude-opus-5

---

Review 2 of 10 for this pull request · View the full run

[upsun-dispatch]

Note

Reviewed — No blocking findings · 🔵 2 minor points

🔁 Incremental · 4 files reviewed

🔍 What this review checked

• Removing the $key->active ? guard in SshKeyListCommand makes the local-path lookup run for inactive keys, so a matching ~/.ssh key is now shown instead of 'Not found'.
• active was added to both $tableHeader and $defaultColumns, so the new column renders and is selectable via --columns.
• The circular-pagination test's next href /api/users/user-id/ssh-keys resolves to exactly the first URL built by sshKeysUrl() from base https://api.example.test/api, so the RuntimeException is really triggered.
• PLATFORMSH_CLI_HOME/PLATFORMSH_CLI_API_URL/PLATFORMSH_CLI_SESSION_ID match application.env_prefix in legacy/config.yaml, and the added assertions fail loudly if the overrides stop being honoured.
• SshKeyListCommandTest can construct the command without a container: CommandBase::run() never touches the unset private $config, and readonly services like SshKey are already doubled elsewhere in the suite.

Verification. This increment adds legacy/tests/Command/SshKey/SshKeyListCommandTest.php covering the inactive-key row and path, and tightens the env overrides in ApiSshKeyTest/SshKeyTest; all run in the legacy-php CI job (./scripts/test/unit.sh, plus phpstan and php-cs-fixer lint steps) on ubuntu-latest only.

Review details

• Commits: 488f1d4..a9e4892
• Model: claude-opus-5

🔵 Minor points

• legacy/tests/Service/SshKeyTest.php:84 — assertSame($this->tempDir, $config->getHomeDirectory()) compares the raw tempnam() path against Config::getHomeDirectory(), which returns realpath($value). HasTempDirTrait::tempDirSetUp() builds the directory under sys_get_temp_dir(), which on macOS is /var/folders/... — a symlink to /private/var/folders/.... The assertion therefore fails on any platform where the temp dir path contains a symlink (macOS make test locally), even though the underlying behaviour is correct; the GitHub Actions legacy-php job runs on ubuntu-latest and does not exercise this. Comparing realpath($this->tempDir) would be portable.
• legacy/tests/Service/SshKeyTest.php:87 — testInactiveAccountKeysAreNotMatched depends on SshKey::listPublicKeys(), whose result is held in a method-level static $publicKeyList shared by every SshKey instance in the PHP process. The test only proves the active filter because no earlier test in the suite calls a real SshKey::findIdentityMatchingPublicKeys()/hasLocalKey(); as soon as one does (PHPUnit runs all test classes in one process, without process isolation), the glob of $tempDir/.ssh is skipped, hasLocalKey() returns false for the wrong reason and the test passes even if the array_filter(..., fn($k) => $k->active) is deleted.

---

Review 3 of 10 for this pull request · View the full run