feat(ssh-cert): support RSA temporary keys for FIPS hosts

[pjcdawkins]

The temporary key pair for SSH certificates was always generated with ssh-keygen -t ed25519. Ed25519 is not FIPS-approved, so key generation fails on FIPS-mode hosts and SSH certificates can't be used there.

• New config key ssh.cert_key_algorithm (env var {PREFIX}SSH_CERT_KEY_ALGORITHM), default auto.
  • auto uses rsa if /proc/sys/crypto/fips_enabled is 1, and ed25519 otherwise.
  • rsa and ed25519 can be set explicitly (case-insensitive). Other values print a warning and are treated as auto, because the certificate parser only supports these two.
• The key filename is derived from the algorithm (id_rsa, id_ed25519), so switching algorithms never reuses a stale key.

Tests: a PHP unit test for the algorithm selection and an rsa variant of the ssh-cert:info integration test.

🤖 Generated with Claude Code

[upsun-dispatch]

Note

Reviewed — No blocking findings · 🔵 2 minor points

🔍 Full review · 4 files reviewed

🔵 Minor points

• legacy/src/SshCert/Certifier.php:86 — The key/cert filenames are now algorithm-derived, but nothing cleans up the files of the previous algorithm. When a user switches (sets {PREFIX}SSH_CERT_KEY_ALGORITHM=rsa, or moves the session dir onto a FIPS host so "auto" flips), the old id_ed25519, id_ed25519.pub and id_ed25519-cert.pub stay in the session ssh directory indefinitely. With ssh.add_to_agent: true the situation is worse: the ssh-add -d $privateKeyFilename call at the top of doGenerateCertificate now names the new path, so the previously added identity is never removed from the agent and keeps being offered (until its -t lifetime elapses), wasting authentication attempts against MaxAuthTries.
• legacy/src/SshCert/Certifier.php:269 — resolveKeyAlgorithm() throws \InvalidArgumentException for any value other than the exact lowercase strings 'rsa'/'ed25519'/'auto'/''. keyAlgorithm() is called from getExistingCertificate(), which runs on read-only paths that never generate a key — ssh-cert:info, SshDiagnostics after a failed SSH connection, Ssh::getSshArgs. So a mis-cased or padded env value (e.g. {PREFIX}SSH_CERT_KEY_ALGORITHM=RSA) aborts those commands instead of being tolerated; the sibling option ssh.windows_paths handles an invalid value with trigger_error plus a fallback (SshConfig.php:205). Trimming and lower-casing the value, or warning and falling back, would match existing behaviour.

Verification

• The upstream parser Platformsh\Client\SshCert\Metadata explicitly accepts ssh-rsa-cert-v01@openssh.com and reads the RSA e/n fields, so RSA certificates parse.
• No code outside Certifier.php still references the removed constants KEY_ALGORITHM / PRIVATE_KEY_FILENAME, and no Go or PHP code hardcodes the session key path.
• cert_key_algorithm is a second-level scalar in config-defaults.yaml, so Config::applyEnvironmentOverrides automatically maps {PREFIX}SSH_CERT_KEY_ALGORITHM to it, as the integration test relies on.
• cmdFactory.extraEnv and EnvPrefix exist and extraEnv is appended last to the child env, so the rsa subtest's override takes effect.
• The '' and 'auto' inputs both fall through to the FIPS file check, and the new unit test covers both plus the missing-file case.

The diff adds legacy/tests/SshCert/CertifierTest.php (run by the ci.yml legacy-php job via ./scripts/test/unit.sh) and an rsa variant of the ssh-cert:info integration test (ci.yml integration-test job, make integration-test). Nothing covers cleanup/agent state when the algorithm changes between runs.

Review details

• Commit: 38fcc4c
• Model: claude-opus-5

---

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

[upsun-dispatch]

📋 PR Summary

Adds support for generating the temporary SSH certificate key pair with RSA so that SSH certificates work on FIPS-mode hosts, where ed25519 key generation fails. A new ssh.cert_key_algorithm config key (env var {PREFIX}SSH_CERT_KEY_ALGORITHM) defaults to auto, which picks rsa when /proc/sys/crypto/fips_enabled is 1 and ed25519 otherwise; rsa and ed25519 may be set explicitly, and unrecognised values warn and fall back to auto. The key filename now follows the algorithm (id_rsa, id_ed25519) so switching algorithms does not reuse a stale key.

Changes

Layer / File(s)	Summary
Certificate key generation
legacy/src/SshCert/Certifier.php	Resolves the key algorithm from configuration (with FIPS auto-detection and validation of the configured value) and derives the key filename from it, instead of always using ed25519. The most recent revision drops the cleanup step that deleted and ssh-add -d'ed key files left over from a previously used algorithm.
Configuration
legacy/config-defaults.yaml	Declares the new ssh.cert_key_algorithm setting with its auto default and environment variable mapping.
Tests
legacy/tests/SshCert/CertifierTest.php	New PHP unit test covering algorithm selection for auto, explicit, and invalid configuration values.
integration-tests/ssh_cert_test.go	Adds an rsa variant of the ssh-cert:info integration test; the latest revision removes the separate algorithm-switch test that asserted stale key files were cleaned up, along with its now-unused imports.

[upsun-dispatch]

Note

Reviewed — No blocking findings · 🔵 4 minor points

🔁 Incremental · 4 files reviewed

🔵 Minor points

• legacy/src/SshCert/Certifier.php:103 — The leftover-key cleanup keys off the private key only (if (!file_exists($otherKey)) continue;). When the private key of the other algorithm is absent but its companions are not — e.g. a previous run was interrupted between the rename() of the cert and the key, or the user deleted only id_ed25519 — id_ed25519.pub and id_ed25519-cert.pub survive every subsequent run and are never removed. Checking any of the three paths (or unconditionally calling $this->fs->remove() on all three, which is a no-op for missing files) would close it. The same guard also means ssh-add -d $otherKey is skipped in that state, while in the inverse state (private key present, .pub missing) ssh-add -d cannot compute the fingerprint and the stale identity stays in the agent.
• legacy/src/SshCert/Certifier.php:109 — The old algorithm's key, public key and certificate are deleted (and dropped from the agent) before getSshCertificate() is called, and before ssh-keygen runs. If the API request fails (network error, expired token) or ssh-keygen fails — the common case on the very hosts this feature targets, where the algorithm was just switched — the user ends up with no certificate at all, where previously the ed25519 certificate was still on disk and usable after unsetting {PREFIX}SSH_CERT_KEY_ALGORITHM. Deferring the removal until after the successful rename() of the new cert/key would keep the fallback intact.
• legacy/src/SshCert/Certifier.php:283 — The invalid-value branch now reports via trigger_error(..., E_USER_WARNING) instead of throwing. Application::configureErrorReporting() sets error_reporting(E_PARSE | E_ERROR) at default verbosity (legacy/src/Application.php:483), so at normal verbosity the warning is never displayed: a typo such as cert_key_algorithm: rsa2048 silently selects ed25519 on a non-FIPS host, and the user gets no hint that their configuration was ignored. Writing the message through $this->stdErr from keyAlgorithm() (or keeping the exception) would make it visible.
• integration-tests/ssh_cert_test.go:63 — The first assertion runs with no SSH_CERT_KEY_ALGORITHM in the environment, i.e. with auto, and requires ssh-ed25519-cert-v01@openssh.com. On a host where /proc/sys/crypto/fips_enabled is 1 — exactly the environment this feature exists for — auto resolves to rsa and both this test and the default case of TestSSHCerts fail even though the code is behaving correctly. Setting EnvPrefix + "SSH_CERT_KEY_ALGORITHM=ed25519" explicitly for the first run makes the test independent of the host's FIPS state.

Verification

• resolveKeyAlgorithm only ever returns a value from KEY_ALGORITHMS, so the algorithm-derived filename cannot contain a traversal component such as ../rsa.
• ssh-add -d $otherKey is issued before fs->remove(), so the agent removal still has the key file it needs to compute the fingerprint.
• getExistingCertificate() uses the same keyAlgorithm() helper as doGenerateCertificate(), so a switched algorithm reliably reports 'no existing certificate'.
• HasTempDirTrait::tearDown() guards with empty(), which is safe for the uninitialized typed $tempDir in the test that never calls tempDirSetUp().
• Filesystem::remove() delegates to Symfony's remove(), which is a no-op for the .pub/-cert.pub paths that do not exist.

The diff adds legacy/tests/SshCert/CertifierTest.php (run by the legacy-php CI job via ./scripts/test/unit.sh) covering algorithm resolution including trimming/case and the new warning fallback, plus TestSSHCertAlgorithmSwitch and an rsa case in integration-tests/ssh_cert_test.go (the integration-test job). Nothing covers the failure path where cert generation aborts after the old algorithm's files have been deleted.

Review details

• Commits: 38fcc4c..9237676
• Model: claude-opus-5

---

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

[upsun-dispatch]

Note

Reviewed — No blocking findings · 🔵 1 minor point

🔁 Incremental · 4 files reviewed

🔵 Minor point

• legacy/src/SshCert/Certifier.php:143 — The cleanup loop deletes the other algorithm's private key, public key and certificate from the shared session ssh directory unconditionally on every successful generation, and this deletion is only serialised against other doGenerateCertificate() runs (the ssh-cert--<session> lock), not against readers. Two concurrently-running CLI processes in the same session but with different {PREFIX}SSH_CERT_KEY_ALGORITHM values (e.g. one shell exporting rsa, another left at the ed25519 default) therefore delete each other's key material: a process that has just resolved id_ed25519-cert.pub via getExistingCertificate() and is about to hand it to ssh -i finds the file gone, and the SSH connection fails with a missing identity file.

Verification

• The stale-algorithm cleanup now runs only after both rename() calls succeed, so a failed ssh-keygen or API call leaves the previous algorithm's cert usable.
• keyAlgorithm() memoises through the typed-but-uninitialised $keyAlgorithm property via isset(), so the invalid-value warning is printed at most once per process.
• The new InvalidArgumentException from resolveKeyAlgorithm() has exactly one non-test caller, keyAlgorithm(), which catches it and falls back to auto.
• Both integration tests now export SSH_CERT_KEY_ALGORITHM explicitly, so neither asserts an outcome that depends on the host's /proc/sys/crypto/fips_enabled.
• ssh-add -d for the superseded algorithm runs before fs->remove() and before the new key is added to the agent, so the agent is not left holding a deleted identity.

legacy/tests/SshCert/CertifierTest.php covers resolveKeyAlgorithm (valid, empty, whitespace/case and rejected values) and runs in the legacy-php CI job via ./scripts/test/unit.sh; the ed25519/rsa cases of TestSSHCerts and TestSSHCertAlgorithmSwitch run in the integration-test job via make integration-test. Nothing exercises the warn-and-fall-back path in keyAlgorithm() end to end, nor the agent-removal branch (ssh.add_to_agent stays false in the tests).

Review details

• Commits: 9237676..b4616f7
• Model: claude-opus-5

---

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

[upsun-dispatch]

Note

Reviewed — No blocking findings · 🔵 1 minor point

🔁 Incremental · 2 files reviewed

🔵 Minor point

• legacy/src/SshCert/Certifier.php:142 — Deleting the stale-algorithm cleanup means that when ssh.cert_key_algorithm changes (e.g. a user sets rsa after having used the default ed25519, or the auto FIPS probe flips), the previous algorithm's id_ed25519, id_ed25519.pub and id_ed25519-cert.pub stay in the session ssh directory: nothing else in the code path touches files for an algorithm other than the current one (doGenerateCertificate() and getExistingCertificate() both build the path from keyAlgorithm() only, and Api::logout() is the sole thing that wipes the directory). With ssh.add_to_agent enabled the old private key plus its certificate also remain loaded in the ssh-agent, since the startup ssh-add -d is issued only for the current algorithm's path; the agent keeps offering that identity until its -t lifetime expires. Result: usable credentials linger on disk for the rest of the session and an extra identity is offered on every SSH connection (which can eat into the server's MaxAuthTries when other keys are present).

Verification

• Each table case builds its own cmdFactory, so f.home defaults to a fresh t.TempDir() per subtest and the two algorithms cannot collide.
• The filename/keyType assertions are algorithm-specific: id_rsa-cert.pub cannot match the ed25519 run and vice versa.
• Removing the cleanup loop leaves self::KEY_ALGORITHMS still referenced by resolveKeyAlgorithm(), so no unused-constant or undefined-symbol breakage.
• The certificate returned after the rename block is unchanged by the deletion; $certificate = new Certificate(...) still runs on both the regenerate and reuse paths.

Verified by the integration-test job (make integration-test), which now runs TestSSHCerts as an ed25519/rsa table, and by the PHPUnit job (./scripts/test/unit.sh) covering CertifierTest::resolveKeyAlgorithm; this push deletes TestSSHCertAlgorithmSwitch, so nothing now covers switching algorithms within one session/home directory.

Review details

• Commits: b4616f7..930fc97
• Model: claude-opus-5

---

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

[pjcdawkins]

Keeping the previous algorithm's key files as they are, without cleanup. This is intentional:

• Deleting them breaks concurrent CLI processes in the same session that use the other algorithm: one can lose its identity file mid-connection, and the two then keep regenerating keys over each other.
• The leftover files are the user's own short-lived credentials, in the same session directory as the current key, and are removed on logout.
• With ssh.add_to_agent (off by default), the old identity stays in the agent only until its certificate's -t lifetime expires.

Switching algorithms is rare, so a bounded leftover is preferable to breaking concurrent SSH connections.

🤖 Generated with Claude Code