fix(ssh): handle chmod failures gracefully and simplify key management
- Log warnings instead of silently failing when chmod 0600 fails - Remove redundant refresh() call before SSH key validation - Remove storeInFileSystem() call from updatePrivateKey() transaction - Remove @unlink() of lock file after filesystem store - Refactor unit tests to use real temp disk and anonymous class stub instead of reflection-only checks
This commit is contained in:
Andras Bacsai
parent
9976645c25
commit
6325e41aec
3 changed files with 155 additions and 138 deletions
|
|
@ -209,8 +209,6 @@ private static function isMultiplexingEnabled(): bool
|
|||
|
||||
private static function validateSshKey(PrivateKey $privateKey): void
|
||||
{
|
||||
$privateKey->refresh();
|
||||
|
||||
$keyLocation = $privateKey->getKeyLocation();
|
||||
$filename = "ssh_key@{$privateKey->uuid}";
|
||||
$disk = Storage::disk('ssh-keys');
|
||||
|
|
@ -236,8 +234,11 @@ private static function validateSshKey(PrivateKey $privateKey): void
|
|||
// Ensure correct permissions (SSH requires 0600)
|
||||
if (file_exists($keyLocation)) {
|
||||
$currentPerms = fileperms($keyLocation) & 0777;
|
||||
if ($currentPerms !== 0600) {
|
||||
chmod($keyLocation, 0600);
|
||||
if ($currentPerms !== 0600 && ! chmod($keyLocation, 0600)) {
|
||||
Log::warning('Failed to set SSH key file permissions to 0600', [
|
||||
'key_uuid' => $privateKey->uuid,
|
||||
'path' => $keyLocation,
|
||||
]);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
@ -71,7 +72,7 @@ protected static function booted()
|
|||
$key->storeInFileSystem();
|
||||
refresh_server_connection($key);
|
||||
} catch (\Exception $e) {
|
||||
\Illuminate\Support\Facades\Log::error('Failed to resync SSH key after update', [
|
||||
Log::error('Failed to resync SSH key after update', [
|
||||
'key_uuid' => $key->uuid,
|
||||
'error' => $e->getMessage(),
|
||||
]);
|
||||
|
|
@ -235,15 +236,17 @@ public function storeInFileSystem()
|
|||
}
|
||||
|
||||
// Ensure correct permissions for SSH (0600 required)
|
||||
if (file_exists($keyLocation)) {
|
||||
chmod($keyLocation, 0600);
|
||||
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);
|
||||
@unlink($lockFile);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -291,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;
|
||||
});
|
||||
}
|
||||
|
|
|
|||
|
|
@ -20,126 +20,26 @@
|
|||
*/
|
||||
class SshKeyValidationTest extends TestCase
|
||||
{
|
||||
public function test_validate_ssh_key_method_exists()
|
||||
private string $diskRoot;
|
||||
|
||||
protected function setUp(): void
|
||||
{
|
||||
$reflection = new \ReflectionMethod(SshMultiplexingHelper::class, 'validateSshKey');
|
||||
$this->assertTrue($reflection->isStatic(), 'validateSshKey should be a static method');
|
||||
}
|
||||
parent::setUp();
|
||||
|
||||
public function test_validate_ssh_key_accepts_private_key_parameter()
|
||||
{
|
||||
$reflection = new \ReflectionMethod(SshMultiplexingHelper::class, 'validateSshKey');
|
||||
$parameters = $reflection->getParameters();
|
||||
|
||||
$this->assertCount(1, $parameters);
|
||||
$this->assertEquals('privateKey', $parameters[0]->getName());
|
||||
|
||||
$type = $parameters[0]->getType();
|
||||
$this->assertNotNull($type);
|
||||
$this->assertEquals(PrivateKey::class, $type->getName());
|
||||
}
|
||||
|
||||
public function test_store_in_file_system_sets_correct_permissions()
|
||||
{
|
||||
// Verify that storeInFileSystem enforces chmod 0600 via code inspection
|
||||
$reflection = new \ReflectionMethod(PrivateKey::class, 'storeInFileSystem');
|
||||
$this->assertTrue(
|
||||
$reflection->isPublic(),
|
||||
'storeInFileSystem should be public'
|
||||
);
|
||||
|
||||
// Verify the method source contains chmod for 0600
|
||||
$filename = $reflection->getFileName();
|
||||
$startLine = $reflection->getStartLine();
|
||||
$endLine = $reflection->getEndLine();
|
||||
$source = implode('', array_slice(file($filename), $startLine - 1, $endLine - $startLine + 1));
|
||||
|
||||
$this->assertStringContainsString('chmod', $source, 'storeInFileSystem should set file permissions');
|
||||
$this->assertStringContainsString('0600', $source, 'storeInFileSystem should enforce 0600 permissions');
|
||||
}
|
||||
|
||||
public function test_store_in_file_system_uses_file_locking()
|
||||
{
|
||||
// Verify the method uses flock to prevent race conditions
|
||||
$reflection = new \ReflectionMethod(PrivateKey::class, 'storeInFileSystem');
|
||||
$filename = $reflection->getFileName();
|
||||
$startLine = $reflection->getStartLine();
|
||||
$endLine = $reflection->getEndLine();
|
||||
$source = implode('', array_slice(file($filename), $startLine - 1, $endLine - $startLine + 1));
|
||||
|
||||
$this->assertStringContainsString('flock', $source, 'storeInFileSystem should use file locking');
|
||||
$this->assertStringContainsString('LOCK_EX', $source, 'storeInFileSystem should use exclusive locks');
|
||||
}
|
||||
|
||||
public function test_validate_ssh_key_checks_content_not_just_existence()
|
||||
{
|
||||
// Verify validateSshKey compares file content with DB value
|
||||
$reflection = new \ReflectionMethod(SshMultiplexingHelper::class, 'validateSshKey');
|
||||
$filename = $reflection->getFileName();
|
||||
$startLine = $reflection->getStartLine();
|
||||
$endLine = $reflection->getEndLine();
|
||||
$source = implode('', array_slice(file($filename), $startLine - 1, $endLine - $startLine + 1));
|
||||
|
||||
// Should read file content and compare, not just check existence with `ls`
|
||||
$this->assertStringNotContainsString('ls $keyLocation', $source, 'Should not use ls to check key existence');
|
||||
$this->assertStringContainsString('private_key', $source, 'Should compare against DB key content');
|
||||
$this->assertStringContainsString('refresh', $source, 'Should refresh key from database');
|
||||
}
|
||||
|
||||
public function test_server_model_detects_private_key_id_changes()
|
||||
{
|
||||
// Verify the Server model's saved event checks for 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',
|
||||
$source,
|
||||
'Server saved event should detect private_key_id changes via wasChanged()'
|
||||
);
|
||||
$this->assertStringContainsString(
|
||||
'private_key_id',
|
||||
$source,
|
||||
'Server saved event should specifically check private_key_id'
|
||||
);
|
||||
}
|
||||
|
||||
public function test_private_key_saved_event_resyncs_on_key_change()
|
||||
{
|
||||
// Verify PrivateKey model resyncs file and mux on key content change
|
||||
$reflection = new \ReflectionMethod(PrivateKey::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')",
|
||||
$source,
|
||||
'PrivateKey saved event should detect key content changes'
|
||||
);
|
||||
$this->assertStringContainsString(
|
||||
'refresh_server_connection',
|
||||
$source,
|
||||
'PrivateKey saved event should invalidate mux connections'
|
||||
);
|
||||
$this->assertStringContainsString(
|
||||
'storeInFileSystem',
|
||||
$source,
|
||||
'PrivateKey saved event should resync key file'
|
||||
);
|
||||
}
|
||||
|
||||
public function test_validate_ssh_key_rewrites_stale_file_and_fixes_permissions()
|
||||
{
|
||||
$diskRoot = sys_get_temp_dir().'/coolify-ssh-test-'.Str::uuid();
|
||||
File::ensureDirectoryExists($diskRoot);
|
||||
config(['filesystems.disks.ssh-keys.root' => $diskRoot]);
|
||||
$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;
|
||||
|
|
@ -168,22 +68,141 @@ public function storeInFileSystem()
|
|||
};
|
||||
|
||||
$privateKey->uuid = (string) Str::uuid();
|
||||
$privateKey->private_key = 'NEW_PRIVATE_KEY_CONTENT';
|
||||
$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');
|
||||
$staleKeyPath = $disk->path($filename);
|
||||
chmod($staleKeyPath, 0644);
|
||||
$keyPath = $disk->path($filename);
|
||||
chmod($keyPath, 0644);
|
||||
|
||||
$reflection = new \ReflectionMethod(SshMultiplexingHelper::class, 'validateSshKey');
|
||||
$reflection->setAccessible(true);
|
||||
$reflection->invoke(null, $privateKey);
|
||||
$this->callValidateSshKey($privateKey);
|
||||
|
||||
$this->assertSame('NEW_PRIVATE_KEY_CONTENT', $disk->get($filename));
|
||||
$this->assertSame(1, $privateKey->storeCallCount);
|
||||
$this->assertSame(0600, fileperms($staleKeyPath) & 0777);
|
||||
$this->assertSame(0600, fileperms($keyPath) & 0777);
|
||||
}
|
||||
|
||||
File::deleteDirectory($diskRoot);
|
||||
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'
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue