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

Migrates the legacy PHP CLI's SSH key handling from the old Accounts API (via the platformsh-client library) to the new Auth API SSH keys endpoints. A new Platformsh\Cli\Model\SshKey value object replaces the client library model (string ULID IDs, label instead of title, OpenSSH SHA-256 fingerprints instead of MD5), and Api gains list/get/add/delete methods with pagination and caching. The ssh-key:add, :delete and :list commands and the SshKey service are updated accordingly, with new unit tests for fingerprint generation.

Changes

Layer / File(s)	Summary
SSH key model
legacy/src/Model/SshKey.php	New readonly value object representing an Auth API SSH key (id, sha256, value, label, active, userId, timestamps) with a fromData() factory.
API client
legacy/src/Service/Api.php	Replaces getLegacyAccountInfo()/client-library SSH key access with direct Auth API calls: getSshKeys() (paginated, cached), getSshKey(), addSshKey(), deleteSshKey() and clearSshKeysCache(), plus URL and cache-key helpers; getMyUserId() moved.
SSH key service
legacy/src/Service/SshKey.php	Uses the new CLI model and computes OpenSSH-style SHA256:<base64> fingerprints instead of MD5 hashes.
Commands
legacy/src/Command/SshKey/SshKeyAddCommand.php	Adds keys through Api::addSshKey(), handles a 409 response with a clear "already registered" error, and matches existing keys by SHA-256.
legacy/src/Command/SshKey/SshKeyDeleteCommand.php	Works with string key IDs (drops the numeric-ID check), uses label/sha256 in the chooser, and deletes via Api::deleteSshKey(); example updated to a ULID.
legacy/src/Command/SshKey/SshKeyListCommand.php	Table columns renamed from title/fingerprint to label/sha256 ("Fingerprint (SHA-256)") and rows built from the new model fields.
Tests and static analysis
legacy/tests/Service/SshKeyTest.php	New tests asserting the SHA-256 fingerprint matches ssh-keygen output and that an invalid key throws.
legacy/phpstan-baseline.neon	Removes the now-obsolete baseline entry for the SshKeyDeleteCommand choose() array type.
legacy/src/Event/LoginRequiredEvent.php	Adds an explicit string cast when escaping login option values for the command line.

[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