diff --git a/src/Executor/Logs.php b/src/Executor/Logs.php index 1845359..ce3c73b 100644 --- a/src/Executor/Logs.php +++ b/src/Executor/Logs.php @@ -19,13 +19,13 @@ public static function get(string $containerId): array $logsFile = $dir . "/logs.txt"; $timingsFile = $dir . "/timings.txt"; - if (!\file_exists($logsFile) || !\file_exists($timingsFile)) { + // The build container writes both files, so a symlink or anything else reads as empty. + $logs = RegularFile::read($logsFile); + $timings = RegularFile::read($timingsFile); + if ($logs === '' || $timings === '') { return []; } - $logs = \file_get_contents($logsFile) ?: ''; - $timings = \file_get_contents($timingsFile) ?: ''; - $offset = 0; // Current offset from timing for reading logs content $introOffset = self::getLogOffset($logs); diff --git a/src/Executor/RegularFile.php b/src/Executor/RegularFile.php new file mode 100644 index 0000000..83f8a76 --- /dev/null +++ b/src/Executor/RegularFile.php @@ -0,0 +1,99 @@ +exists($tmpBuild)) { - throw new \Exception('Something went wrong when starting runtime.'); - } + // The build container can write this path. Following a symlink there would upload the + // executor's own files (its environment) as the build, for the owner to download. + // The upload reads a copy in /tmp/, which no container mounts. + $snapshot = \dirname($tmpBuild, 2) . '/output-' . \basename($tmpBuild); + try { + $size = RegularFile::copy($tmpBuild, $snapshot); + if ($size === null) { + throw new \Exception('Something went wrong when starting runtime.'); + } - $size = $localDevice->getFileSize($tmpBuild); - $container['size'] = $size; + $container['size'] = $size; - $destinationDevice = StorageFactory::getDevice($destination, System::getEnv('OPR_EXECUTOR_CONNECTION_STORAGE')); - $path = $destinationDevice->getPath(\uniqid() . '.' . \pathinfo($tmpBuild, PATHINFO_EXTENSION)); + $destinationDevice = StorageFactory::getDevice($destination, System::getEnv('OPR_EXECUTOR_CONNECTION_STORAGE')); + $path = $destinationDevice->getPath(\uniqid() . '.' . \pathinfo($tmpBuild, PATHINFO_EXTENSION)); - if (!$localDevice->copy($tmpBuild, $path, $destinationDevice)) { - throw new \Exception('Failed to move built code to storage'); - }; + if (!$localDevice->copy($snapshot, $path, $destinationDevice)) { + throw new \Exception('Failed to move built code to storage'); + }; + } finally { + // A runtime kept after its build keeps /tmp/, so the copy is removed here. + @\unlink($snapshot); + } $container['path'] = $path; } @@ -631,15 +641,22 @@ private function storeBuildCacheArtifact(string $cacheKey, string $tmpCache, Loc { $artifact = $tmpCache . '/stores.sqfs'; - if (!$localDevice->exists($artifact)) { - return; - } + // /cache is mounted into the build container, so anything but a regular file is skipped, and + // the upload reads a copy in /tmp/, which no container mounts. + $snapshot = \dirname($tmpCache) . '/cache-stores.sqfs'; + try { + if (RegularFile::copy($artifact, $snapshot) === null) { + return; + } - $cacheDevice = $this->getBuildCacheDevice(); - $destinationArtifact = $this->getBuildCacheArtifactPath($cacheDevice, $cacheKey); + $cacheDevice = $this->getBuildCacheDevice(); + $destinationArtifact = $this->getBuildCacheArtifactPath($cacheDevice, $cacheKey); - if (!$localDevice->copy($artifact, $destinationArtifact, $cacheDevice)) { - throw new \Exception('Failed to store build cache artifact'); + if (!$localDevice->copy($snapshot, $destinationArtifact, $cacheDevice)) { + throw new \Exception('Failed to store build cache artifact'); + } + } finally { + @\unlink($snapshot); } } @@ -1023,34 +1040,23 @@ public function createExecution( $logs = ''; $errors = ''; - if ($fileId !== '' && $fileId !== '0') { + // The runtime sends this id, so it may only name a file directly in its own logs directory. + if ($fileId !== '' && $fileId !== '0' && \ctype_alnum($fileId)) { $logFile = '/tmp/' . $runtimeName . '/logs/' . $fileId . '_logs.log'; $errorFile = '/tmp/' . $runtimeName . '/logs/' . $fileId . '_errors.log'; - $logDevice = new Local(); - - if ($logDevice->exists($logFile)) { - if ($logDevice->getFileSize($logFile) > MAX_LOG_SIZE) { - $maxToRead = MAX_LOG_SIZE; - $logs = (string) $logDevice->read($logFile, 0, $maxToRead); - $logs .= "\nLog file has been truncated to " . number_format(MAX_LOG_SIZE / 1048576, 2) . "MB."; - } else { - $logs = (string) $logDevice->read($logFile); - } - - $logDevice->delete($logFile); + // The runtime writes these files, so a symlink is refused, never followed. unlink + // removes a symlink itself, never its target. + $logs = RegularFile::read($logFile, 0, MAX_LOG_SIZE + 1); + @\unlink($logFile); + if (\strlen($logs) > MAX_LOG_SIZE) { + $logs = \substr($logs, 0, MAX_LOG_SIZE) . "\nLog file has been truncated to " . number_format(MAX_LOG_SIZE / 1048576, 2) . "MB."; } - if ($logDevice->exists($errorFile)) { - if ($logDevice->getFileSize($errorFile) > MAX_LOG_SIZE) { - $maxToRead = MAX_LOG_SIZE; - $errors = (string) $logDevice->read($errorFile, 0, $maxToRead); - $errors .= "\nError file has been truncated to " . number_format(MAX_LOG_SIZE / 1048576, 2) . "MB."; - } else { - $errors = (string) $logDevice->read($errorFile); - } - - $logDevice->delete($errorFile); + $errors = RegularFile::read($errorFile, 0, MAX_LOG_SIZE + 1); + @\unlink($errorFile); + if (\strlen($errors) > MAX_LOG_SIZE) { + $errors = \substr($errors, 0, MAX_LOG_SIZE) . "\nError file has been truncated to " . number_format(MAX_LOG_SIZE / 1048576, 2) . "MB."; } } diff --git a/tests/e2e/ExecutorTest.php b/tests/e2e/ExecutorTest.php index 54b205c..4b297c9 100644 --- a/tests/e2e/ExecutorTest.php +++ b/tests/e2e/ExecutorTest.php @@ -425,6 +425,68 @@ public function testBuildOutputDirectory(): void $this->assertEquals(200, $response['headers']['status-code']); } + public function testBuildOutputSymlinkIsNotUploaded(): void + { + $output = ''; + $stderr = ''; + Console::execute('cd /app/tests/resources/functions/node && tar --exclude code.tar.gz -czf code.tar.gz .', '', $output, $stderr); + + $runtimeId = \bin2hex(\random_bytes(4)); + + // The build leaves a link to /proc/self/environ as its output. Followed, the executor + // uploads its own environment, secret included, as a build the owner can download. + $response = $this->client->call(Client::METHOD_POST, '/runtimes', [], [ + 'runtimeId' => 'test-build-symlink-' . $runtimeId, + 'source' => '/storage/functions/node/code.tar.gz', + 'destination' => '/storage/builds/test', + 'entrypoint' => 'index.js', + 'image' => 'openruntimes/node:v5-22', + 'command' => 'ln -sf /proc/self/environ /mnt/code/code.tar.gz', + 'remove' => true + ]); + + $this->assertNotEquals(201, $response['headers']['status-code']); + $this->assertArrayNotHasKey('path', (array) $response['body']); + $this->assertStringNotContainsString('OPR_EXECUTOR_SECRET', (string) \json_encode($response['body'])); + } + + public function testExecutionLogSymlinkIsNotFollowed(): void + { + $output = ''; + $stderr = ''; + Console::execute('cd /app/tests/resources/functions/node-log-symlink && tar --exclude code.tar.gz -czf code.tar.gz .', '', $output, $stderr); + + $runtimeId = \bin2hex(\random_bytes(4)); + + $response = $this->client->call(Client::METHOD_POST, '/runtimes', [], [ + 'runtimeId' => 'test-build-log-symlink-' . $runtimeId, + 'source' => '/storage/functions/node-log-symlink/code.tar.gz', + 'destination' => '/storage/builds/test', + 'entrypoint' => 'index.js', + 'image' => 'openruntimes/node:v5-22', + 'command' => 'tar -zxf /tmp/code.tar.gz -C /mnt/code && bash helpers/build.sh "npm i"', + 'remove' => true + ]); + $this->assertEquals(201, $response['headers']['status-code']); + + // The function swaps its log files for links to /proc/self/environ. Followed, they return + // the executor's own environment, secret included, as the execution's logs. + $response = $this->client->call(Client::METHOD_POST, '/runtimes/test-exec-log-symlink-' . $runtimeId . '/executions', [], [ + 'source' => $response['body']['path'], + 'entrypoint' => 'index.js', + 'image' => 'openruntimes/node:v5-22', + 'runtimeEntrypoint' => 'cp /tmp/code.tar.gz /mnt/code/code.tar.gz && nohup helpers/start.sh "bash helpers/server.sh"', + ]); + + $this->assertEquals(200, $response['headers']['status-code']); + $this->assertEquals('OK', $response['body']['body']); + $this->assertStringNotContainsString('OPR_EXECUTOR_SECRET', (string) $response['body']['logs']); + $this->assertStringNotContainsString('OPR_EXECUTOR_SECRET', (string) $response['body']['errors']); + + $response = $this->client->call(Client::METHOD_DELETE, '/runtimes/test-exec-log-symlink-' . $runtimeId, [], []); + $this->assertEquals(200, $response['headers']['status-code']); + } + public function testBuildUncompressed(): void { $output = ''; @@ -826,6 +888,44 @@ public function testBuildCache(): void $this->assertEquals(200, $response['headers']['status-code']); } + public function testBuildCacheSymlinkIsNotStored(): void + { + $output = ''; + $stderr = ''; + Console::execute('cd /app/tests/resources/functions/node && tar --exclude code.tar.gz -czf code.tar.gz .', '', $output, $stderr); + + $runtimeId = \bin2hex(\random_bytes(4)); + $cacheKey = 'test-build-cache-symlink-' . $runtimeId; + + // The first build swaps its cache artifact for a link to /proc/self/environ. Followed, the + // executor stores its own environment, secret included, as the project's build cache. + $response = $this->client->call(Client::METHOD_POST, '/runtimes', [], [ + 'runtimeId' => 'test-build-cache-plant-' . $runtimeId, + 'source' => '/storage/functions/node/code.tar.gz', + 'destination' => '/storage/builds/test-cache-plant', + 'entrypoint' => 'index.js', + 'image' => 'openruntimes/node:v5-18.0', + 'command' => 'tar -zxf /tmp/code.tar.gz -C /mnt/code && bash helpers/build.sh "npm install" && ln -sf /proc/self/environ /cache/stores.sqfs', + 'cacheKey' => $cacheKey, + 'remove' => true, + ]); + $this->assertEquals(201, $response['headers']['status-code']); + + // The second build prints whatever the executor restored as its cache. + $response = $this->client->call(Client::METHOD_POST, '/runtimes', [], [ + 'runtimeId' => 'test-build-cache-read-' . $runtimeId, + 'source' => '/storage/functions/node/code.tar.gz', + 'destination' => '/storage/builds/test-cache-read', + 'entrypoint' => 'index.js', + 'image' => 'openruntimes/node:v5-18.0', + 'command' => 'tar -zxf /tmp/code.tar.gz -C /mnt/code && (cat /cache/stores.sqfs || true) && bash helpers/build.sh "npm install"', + 'cacheKey' => $cacheKey, + 'remove' => true, + ]); + $this->assertEquals(201, $response['headers']['status-code']); + $this->assertStringNotContainsString('OPR_EXECUTOR_SECRET', (string) \json_encode($response['body'])); + } + // We also test SSR two Set-cookie here public function testSSRLogs(): void { diff --git a/tests/resources/functions/node-log-symlink/index.js b/tests/resources/functions/node-log-symlink/index.js new file mode 100644 index 0000000..2e1957f --- /dev/null +++ b/tests/resources/functions/node-log-symlink/index.js @@ -0,0 +1,14 @@ +const fs = require('fs'); + +// Replaces this execution's log files with symlinks to /proc/self/environ. The runtime keeps +// writing to the files it already opened, but the executor reads them by path afterwards. +module.exports = async (context) => { + context.log('planted'); + + for (const file of fs.readdirSync('/mnt/logs')) { + fs.rmSync(`/mnt/logs/${file}`); + fs.symlinkSync('/proc/self/environ', `/mnt/logs/${file}`); + } + + return context.res.send('OK'); +} diff --git a/tests/resources/functions/node-log-symlink/package.json b/tests/resources/functions/node-log-symlink/package.json new file mode 100644 index 0000000..e019f53 --- /dev/null +++ b/tests/resources/functions/node-log-symlink/package.json @@ -0,0 +1,12 @@ +{ + "name": "log-symlink", + "version": "1.0.0", + "description": "", + "main": "index.js", + "scripts": { + "test": "echo \"Error: no test specified\" && exit 1" + }, + "keywords": [], + "author": "", + "license": "ISC" +} diff --git a/tests/unit/Executor/RegularFileTest.php b/tests/unit/Executor/RegularFileTest.php new file mode 100644 index 0000000..ed0dd3e --- /dev/null +++ b/tests/unit/Executor/RegularFileTest.php @@ -0,0 +1,140 @@ +dir = \sys_get_temp_dir() . '/regular-file-' . \bin2hex(\random_bytes(6)); + \mkdir($this->dir); + + // Stands in for the executor's own files, such as /proc/self/environ. Two lines, because + // build logs drop the first line as the script banner. + $this->secret = $this->dir . '/secret'; + \file_put_contents($this->secret, "first line\nOPR_EXECUTOR_SECRET=leaked"); + } + + protected function tearDown(): void + { + $this->remove($this->dir); + + parent::tearDown(); + } + + public function testReadsARegularFile(): void + { + \file_put_contents($this->dir . '/logs.log', 'hello world'); + + $this->assertSame('hello world', RegularFile::read($this->dir . '/logs.log')); + $this->assertSame('world', RegularFile::read($this->dir . '/logs.log', 6)); + $this->assertSame('hello', RegularFile::read($this->dir . '/logs.log', 0, 5)); + } + + public function testDoesNotFollowASymlinkTheContainerPlanted(): void + { + \symlink($this->secret, $this->dir . '/logs.log'); + + $this->assertSame('', RegularFile::read($this->dir . '/logs.log')); + $this->assertStringContainsString('leaked', (string) \file_get_contents($this->secret)); + } + + public function testRefusesAFifoWithoutBlocking(): void + { + if (!\function_exists('posix_mkfifo')) { + $this->markTestSkipped('posix_mkfifo is not available'); + } + + \posix_mkfifo($this->dir . '/logs.log', 0600); + + $this->assertSame('', RegularFile::read($this->dir . '/logs.log')); + } + + public function testMissingPathReadsAsEmpty(): void + { + $this->assertSame('', RegularFile::read($this->dir . '/missing.log')); + } + + public function testCopiesARegularFile(): void + { + \file_put_contents($this->dir . '/code.tar.gz', 'build'); + + $this->assertSame(5, RegularFile::copy($this->dir . '/code.tar.gz', $this->dir . '/output')); + $this->assertSame('build', \file_get_contents($this->dir . '/output')); + } + + public function testDoesNotCopyASymlinkTheBuildPlanted(): void + { + \symlink($this->secret, $this->dir . '/code.tar.gz'); + + $this->assertNull(RegularFile::copy($this->dir . '/code.tar.gz', $this->dir . '/output')); + $this->assertFileDoesNotExist($this->dir . '/output'); + } + + public function testDoesNotCopyOverAnExistingEntry(): void + { + \file_put_contents($this->dir . '/code.tar.gz', 'build'); + \symlink($this->secret, $this->dir . '/output'); + + try { + RegularFile::copy($this->dir . '/code.tar.gz', $this->dir . '/output'); + $this->fail('Copied over an existing entry'); + } catch (\Exception) { + $this->assertStringContainsString('leaked', (string) \file_get_contents($this->secret)); + $this->assertStringNotContainsString('build', (string) \file_get_contents($this->secret)); + } + } + + public function testBuildLogsDoNotFollowASymlinkTheBuildPlanted(): void + { + // Logs::get reads /tmp//logging, the directory the build mounts as /tmp/logging. + // app/init.php defines it in the executor; the unit suite doesn't load that. + if (!\defined('MAX_BUILD_LOG_SIZE')) { + \define('MAX_BUILD_LOG_SIZE', 1000 * 1000); + } + + $container = 'regular-file-test-' . \bin2hex(\random_bytes(6)); + $logging = '/tmp/' . $container . '/logging'; + \mkdir($logging, 0777, true); + + try { + \symlink($this->secret, $logging . '/logs.txt'); + \file_put_contents($logging . '/timings.txt', "0.1 30\n"); + + $output = \json_encode(Logs::get($container)); + + $this->assertIsString($output); + $this->assertStringNotContainsString('leaked', $output); + } finally { + $this->remove('/tmp/' . $container); + } + } + + private function remove(string $path): void + { + if (\is_link($path) || \is_file($path) || \filetype($path) === 'fifo') { + \unlink($path); + return; + } + + foreach (\scandir($path) ?: [] as $entry) { + if ($entry !== '.' && $entry !== '..') { + $this->remove($path . '/' . $entry); + } + } + + \rmdir($path); + } +}