rosslh/coolify
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix: prevent sporadic SSH permission denied on key rotation (#8990)

Browse source
This commit is contained in:
Andras Bacsai 2026-03-16 21:37:29 +01:00 committed by GitHub
commit fd3fc17b2f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 293 additions and 25 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

32
app/Helpers/SshMultiplexingHelper.php
View file

@ -8,6 +8,7 @@
use Illuminate\Support\Facades\Hash;
use Illuminate\Support\Facades\Log;
use Illuminate\Support\Facades\Process;
use Illuminate\Support\Facades\Storage;
class SshMultiplexingHelper
{
@ -209,12 +210,37 @@ private static function isMultiplexingEnabled(): bool
private static function validateSshKey(PrivateKey $privateKey): void
{
$keyLocation = $privateKey->getKeyLocation();
$checkKeyCommand = "ls $keyLocation 2>/dev/null";
$keyCheckProcess = Process::run($checkKeyCommand);
$filename = "ssh_key@{$privateKey->uuid}";
$disk = Storage::disk('ssh-keys');
if ($keyCheckProcess->exitCode() !== 0) {
$needsRewrite = false;
if (! $disk->exists($filename)) {
$needsRewrite = true;
} else {
$diskContent = $disk->get($filename);
if ($diskContent !== $privateKey->private_key) {
Log::warning('SSH key file content does not match database, resyncing', [
'key_uuid' => $privateKey->uuid,
]);
$needsRewrite = true;
}
}
if ($needsRewrite) {
$privateKey->storeInFileSystem();
}
// Ensure correct permissions (SSH requires 0600)
if (file_exists($keyLocation)) {
$currentPerms = fileperms($keyLocation) & 0777;
if ($currentPerms !== 0600 && ! chmod($keyLocation, 0600)) {
Log::warning('Failed to set SSH key file permissions to 0600', [
'key_uuid' => $privateKey->uuid,
'path' => $keyLocation,
]);
}
}
}
private static function getCommonSshOptions(Server $server, string $sshKeyLocation, int $connectionTimeout, int $serverInterval, bool $isScp = false): string

76
app/Models/PrivateKey.php
View file

@ -5,6 +5,7 @@
use App\Traits\HasSafeStringAttribute;
use DanHarrin\LivewireRateLimiting\WithRateLimiting;
use Illuminate\Support\Facades\DB;
use Illuminate\Support\Facades\Log;
use Illuminate\Support\Facades\Storage;
use Illuminate\Validation\ValidationException;
use OpenApi\Attributes as OA;
@ -65,6 +66,20 @@ protected static function booted()
}
});
static::saved(function ($key) {
if ($key->wasChanged('private_key')) {
try {
$key->storeInFileSystem();
refresh_server_connection($key);
} catch (\Exception $e) {
Log::error('Failed to resync SSH key after update', [
'key_uuid' => $key->uuid,
'error' => $e->getMessage(),
]);
}
}
});
static::deleted(function ($key) {
self::deleteFromStorage($key);
});
@ -185,29 +200,54 @@ public function storeInFileSystem()
{
$filename = "ssh_key@{$this->uuid}";
$disk = Storage::disk('ssh-keys');
$keyLocation = $this->getKeyLocation();
$lockFile = $keyLocation.'.lock';
// Ensure the storage directory exists and is writable
$this->ensureStorageDirectoryExists();
// Attempt to store the private key
$success = $disk->put($filename, $this->private_key);
if (! $success) {
throw new \Exception("Failed to write SSH key to filesystem. Check disk space and permissions for: {$this->getKeyLocation()}");
// Use file locking to prevent concurrent writes from corrupting the key
$lockHandle = fopen($lockFile, 'c');
if ($lockHandle === false) {
throw new \Exception("Failed to open lock file for SSH key: {$lockFile}");
}
// Verify the file was actually created and has content
if (! $disk->exists($filename)) {
throw new \Exception("SSH key file was not created: {$this->getKeyLocation()}");
}
try {
if (! flock($lockHandle, LOCK_EX)) {
throw new \Exception("Failed to acquire lock for SSH key: {$keyLocation}");
}
$storedContent = $disk->get($filename);
if (empty($storedContent) || $storedContent !== $this->private_key) {
$disk->delete($filename); // Clean up the bad file
throw new \Exception("SSH key file content verification failed: {$this->getKeyLocation()}");
}
// Attempt to store the private key
$success = $disk->put($filename, $this->private_key);
return $this->getKeyLocation();
if (! $success) {
throw new \Exception("Failed to write SSH key to filesystem. Check disk space and permissions for: {$keyLocation}");
}
// Verify the file was actually created and has content
if (! $disk->exists($filename)) {
throw new \Exception("SSH key file was not created: {$keyLocation}");
}
$storedContent = $disk->get($filename);
if (empty($storedContent) || $storedContent !== $this->private_key) {
$disk->delete($filename); // Clean up the bad file
throw new \Exception("SSH key file content verification failed: {$keyLocation}");
}
// Ensure correct permissions for SSH (0600 required)
if (file_exists($keyLocation) && ! chmod($keyLocation, 0600)) {
Log::warning('Failed to set SSH key file permissions to 0600', [
'key_uuid' => $this->uuid,
'path' => $keyLocation,
]);
}
return $keyLocation;
} finally {
flock($lockHandle, LOCK_UN);
fclose($lockHandle);
}
}
public static function deleteFromStorage(self $privateKey)
@ -254,12 +294,6 @@ public function updatePrivateKey(array $data)
return DB::transaction(function () use ($data) {
$this->update($data);
try {
$this->storeInFileSystem();
} catch (\Exception $e) {
throw new \Exception('Failed to update SSH key: '.$e->getMessage());
}
return $this;
});
}

2
app/Models/Server.php
View file

@ -135,7 +135,7 @@ protected static function booted()
$server->forceFill($payload);
});
static::saved(function ($server) {
if ($server->privateKey?->isDirty()) {
if ($server->wasChanged('private_key_id') || $server->privateKey?->isDirty()) {
refresh_server_connection($server->privateKey);
}
});

208
tests/Unit/SshKeyValidationTest.php Normal file
View file

@ -0,0 +1,208 @@
<?php
namespace Tests\Unit;
use App\Helpers\SshMultiplexingHelper;
use App\Models\PrivateKey;
use Illuminate\Support\Facades\File;
use Illuminate\Support\Facades\Storage;
use Illuminate\Support\Str;
use Tests\TestCase;
/**
* Tests for SSH key validation to prevent sporadic "Permission denied" errors.
*
* The root cause: validateSshKey() only checked file existence, not content.
* When a key was rotated in the DB but the old file persisted on disk,
* SSH would use the stale key and fail with "Permission denied (publickey)".
*
* @see https://github.com/coollabsio/coolify/issues/7724
*/
class SshKeyValidationTest extends TestCase
{
private string $diskRoot;
protected function setUp(): void
{
parent::setUp();
$this->diskRoot = sys_get_temp_dir().'/coolify-ssh-test-'.Str::uuid();
File::ensureDirectoryExists($this->diskRoot);
config(['filesystems.disks.ssh-keys.root' => $this->diskRoot]);
app('filesystem')->forgetDisk('ssh-keys');
}
protected function tearDown(): void
{
File::deleteDirectory($this->diskRoot);
parent::tearDown();
}
private function makePrivateKey(string $keyContent = 'TEST_KEY_CONTENT'): PrivateKey
{
$privateKey = new class extends PrivateKey
{
public int $storeCallCount = 0;
public function refresh()
{
return $this;
}
public function getKeyLocation()
{
return Storage::disk('ssh-keys')->path("ssh_key@{$this->uuid}");
}
public function storeInFileSystem()
{
$this->storeCallCount++;
$filename = "ssh_key@{$this->uuid}";
$disk = Storage::disk('ssh-keys');
$disk->put($filename, $this->private_key);
$keyLocation = $disk->path($filename);
chmod($keyLocation, 0600);
return $keyLocation;
}
};
$privateKey->uuid = (string) Str::uuid();
$privateKey->private_key = $keyContent;
return $privateKey;
}
private function callValidateSshKey(PrivateKey $privateKey): void
{
$reflection = new \ReflectionMethod(SshMultiplexingHelper::class, 'validateSshKey');
$reflection->setAccessible(true);
$reflection->invoke(null, $privateKey);
}
public function test_validate_ssh_key_rewrites_stale_file_and_fixes_permissions()
{
$privateKey = $this->makePrivateKey('NEW_PRIVATE_KEY_CONTENT');
$filename = "ssh_key@{$privateKey->uuid}";
$disk = Storage::disk('ssh-keys');
$disk->put($filename, 'OLD_PRIVATE_KEY_CONTENT');
$keyPath = $disk->path($filename);
chmod($keyPath, 0644);
$this->callValidateSshKey($privateKey);
$this->assertSame('NEW_PRIVATE_KEY_CONTENT', $disk->get($filename));
$this->assertSame(1, $privateKey->storeCallCount);
$this->assertSame(0600, fileperms($keyPath) & 0777);
}
public function test_validate_ssh_key_creates_missing_file()
{
$privateKey = $this->makePrivateKey('MY_KEY_CONTENT');
$filename = "ssh_key@{$privateKey->uuid}";
$disk = Storage::disk('ssh-keys');
$this->assertFalse($disk->exists($filename));
$this->callValidateSshKey($privateKey);
$this->assertTrue($disk->exists($filename));
$this->assertSame('MY_KEY_CONTENT', $disk->get($filename));
$this->assertSame(1, $privateKey->storeCallCount);
}
public function test_validate_ssh_key_skips_rewrite_when_content_matches()
{
$privateKey = $this->makePrivateKey('SAME_KEY_CONTENT');
$filename = "ssh_key@{$privateKey->uuid}";
$disk = Storage::disk('ssh-keys');
$disk->put($filename, 'SAME_KEY_CONTENT');
$keyPath = $disk->path($filename);
chmod($keyPath, 0600);
$this->callValidateSshKey($privateKey);
$this->assertSame(0, $privateKey->storeCallCount, 'Should not rewrite when content matches');
$this->assertSame('SAME_KEY_CONTENT', $disk->get($filename));
}
public function test_validate_ssh_key_fixes_permissions_without_rewrite()
{
$privateKey = $this->makePrivateKey('KEY_CONTENT');
$filename = "ssh_key@{$privateKey->uuid}";
$disk = Storage::disk('ssh-keys');
$disk->put($filename, 'KEY_CONTENT');
$keyPath = $disk->path($filename);
chmod($keyPath, 0644);
$this->callValidateSshKey($privateKey);
$this->assertSame(0, $privateKey->storeCallCount, 'Should not rewrite when content matches');
$this->assertSame(0600, fileperms($keyPath) & 0777, 'Should fix permissions even without rewrite');
}
public function test_store_in_file_system_enforces_correct_permissions()
{
$privateKey = $this->makePrivateKey('KEY_FOR_PERM_TEST');
$privateKey->storeInFileSystem();
$filename = "ssh_key@{$privateKey->uuid}";
$keyPath = Storage::disk('ssh-keys')->path($filename);
$this->assertSame(0600, fileperms($keyPath) & 0777);
}
public function test_store_in_file_system_lock_file_persists()
{
// Use the real storeInFileSystem to verify lock file behavior
$disk = Storage::disk('ssh-keys');
$uuid = (string) Str::uuid();
$filename = "ssh_key@{$uuid}";
$keyLocation = $disk->path($filename);
$lockFile = $keyLocation.'.lock';
$privateKey = new class extends PrivateKey
{
public function refresh()
{
return $this;
}
public function getKeyLocation()
{
return Storage::disk('ssh-keys')->path("ssh_key@{$this->uuid}");
}
protected function ensureStorageDirectoryExists()
{
// No-op in test — directory already exists
}
};
$privateKey->uuid = $uuid;
$privateKey->private_key = 'LOCK_TEST_KEY';
$privateKey->storeInFileSystem();
// Lock file should persist (not be deleted) to prevent flock race conditions
$this->assertFileExists($lockFile, 'Lock file should persist after storeInFileSystem');
}
public function test_server_model_detects_private_key_id_changes()
{
$reflection = new \ReflectionMethod(\App\Models\Server::class, 'booted');
$filename = $reflection->getFileName();
$startLine = $reflection->getStartLine();
$endLine = $reflection->getEndLine();
$source = implode('', array_slice(file($filename), $startLine - 1, $endLine - $startLine + 1));
$this->assertStringContainsString(
"wasChanged('private_key_id')",
$source,
'Server saved event should detect private_key_id changes'
);
}
}