From 08a42254371fc74b65ac4783b3a70b33fe37f07d Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 6 Apr 2017 05:53:51 -0700 Subject: [PATCH] Provide "bin/files integrity" for debugging, maintaining and backfilling integrity hashes Summary: Ref T12470. Provides an "integrity" utility which runs in these modes: - Verify: check that hashes match. - Compute: backfill missing hashes. - Strip: remove hashes. Useful for upgrading across a hash change. - Corrupt: intentionally corrupt hashes. Useful for debugging. - Overwrite: force hash recomputation. Users normally shouldn't need to run any of this stuff, but this provides a reasonable toolkit for managing integrity hashes. I'll recommend existing installs use `bin/files integrity --compute all` in the upgrade guidance to backfill hashes for existing files. Test Plan: - Ran the script in many modes against various files, saw expected operation, including: - Verified a file, corrupted it, saw it fail. - Verified a file, stripped it, saw it have no hash. - Stripped a file, computed it, got a clean verify. - Stripped a file, overwrote it, got a clean verify. - Corrupted a file, overwrote it, got a clean verify. - Overwrote a file, overwrote again, got a no-op. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12470 Differential Revision: https://secure.phabricator.com/D17629 --- src/__phutil_library_map__.php | 2 + .../engine/PhabricatorFileStorageEngine.php | 2 +- ...icatorFilesManagementIntegrityWorkflow.php | 325 ++++++++++++++++++ .../files/storage/PhabricatorFile.php | 33 +- .../PhabricatorManagementWorkflow.php | 36 ++ 5 files changed, 391 insertions(+), 7 deletions(-) create mode 100644 src/applications/files/management/PhabricatorFilesManagementIntegrityWorkflow.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 28e808026a..89401d56eb 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2810,6 +2810,7 @@ phutil_register_library_map(array( 'PhabricatorFilesManagementEncodeWorkflow' => 'applications/files/management/PhabricatorFilesManagementEncodeWorkflow.php', 'PhabricatorFilesManagementEnginesWorkflow' => 'applications/files/management/PhabricatorFilesManagementEnginesWorkflow.php', 'PhabricatorFilesManagementGenerateKeyWorkflow' => 'applications/files/management/PhabricatorFilesManagementGenerateKeyWorkflow.php', + 'PhabricatorFilesManagementIntegrityWorkflow' => 'applications/files/management/PhabricatorFilesManagementIntegrityWorkflow.php', 'PhabricatorFilesManagementMigrateWorkflow' => 'applications/files/management/PhabricatorFilesManagementMigrateWorkflow.php', 'PhabricatorFilesManagementPurgeWorkflow' => 'applications/files/management/PhabricatorFilesManagementPurgeWorkflow.php', 'PhabricatorFilesManagementRebuildWorkflow' => 'applications/files/management/PhabricatorFilesManagementRebuildWorkflow.php', @@ -7941,6 +7942,7 @@ phutil_register_library_map(array( 'PhabricatorFilesManagementEncodeWorkflow' => 'PhabricatorFilesManagementWorkflow', 'PhabricatorFilesManagementEnginesWorkflow' => 'PhabricatorFilesManagementWorkflow', 'PhabricatorFilesManagementGenerateKeyWorkflow' => 'PhabricatorFilesManagementWorkflow', + 'PhabricatorFilesManagementIntegrityWorkflow' => 'PhabricatorFilesManagementWorkflow', 'PhabricatorFilesManagementMigrateWorkflow' => 'PhabricatorFilesManagementWorkflow', 'PhabricatorFilesManagementPurgeWorkflow' => 'PhabricatorFilesManagementWorkflow', 'PhabricatorFilesManagementRebuildWorkflow' => 'PhabricatorFilesManagementWorkflow', diff --git a/src/applications/files/engine/PhabricatorFileStorageEngine.php b/src/applications/files/engine/PhabricatorFileStorageEngine.php index 9a15460ec8..527d39ff9a 100644 --- a/src/applications/files/engine/PhabricatorFileStorageEngine.php +++ b/src/applications/files/engine/PhabricatorFileStorageEngine.php @@ -336,7 +336,7 @@ abstract class PhabricatorFileStorageEngine extends Phobject { $known_integrity = $file->getIntegrityHash(); if ($known_integrity !== null) { $new_integrity = $this->newIntegrityHash($formatted_data, $format); - if ($known_integrity !== $new_integrity) { + if (!phutil_hashes_are_identical($known_integrity, $new_integrity)) { throw new PhabricatorFileIntegrityException( pht( 'File data integrity check failed. Dark forces have corrupted '. diff --git a/src/applications/files/management/PhabricatorFilesManagementIntegrityWorkflow.php b/src/applications/files/management/PhabricatorFilesManagementIntegrityWorkflow.php new file mode 100644 index 0000000000..344df71460 --- /dev/null +++ b/src/applications/files/management/PhabricatorFilesManagementIntegrityWorkflow.php @@ -0,0 +1,325 @@ +setName('integrity') + ->setSynopsis(pht('Verify or recalculate file integrity hashes.')) + ->setArguments( + array( + array( + 'name' => 'all', + 'help' => pht('Affect all files.'), + ), + array( + 'name' => 'strip', + 'help' => pht( + 'DANGEROUS. Strip integrity hashes from files. This makes '. + 'files vulnerable to corruption or tampering.'), + ), + array( + 'name' => 'corrupt', + 'help' => pht( + 'Corrupt integrity hashes for given files. This is intended '. + 'for debugging.'), + ), + array( + 'name' => 'compute', + 'help' => pht( + 'Compute and update integrity hashes for files which do not '. + 'yet have them.'), + ), + array( + 'name' => 'overwrite', + 'help' => pht( + 'DANGEROUS. Recompute and update integrity hashes, overwriting '. + 'invalid hashes. This may mark corrupt or dangerous files as '. + 'valid.'), + ), + array( + 'name' => 'force', + 'short' => 'f', + 'help' => pht( + 'Execute dangerous operations without prompting for '. + 'confirmation.'), + ), + array( + 'name' => 'names', + 'wildcard' => true, + ), + )); + } + + public function execute(PhutilArgumentParser $args) { + $modes = array(); + + $is_strip = $args->getArg('strip'); + if ($is_strip) { + $modes[] = 'strip'; + } + + $is_corrupt = $args->getArg('corrupt'); + if ($is_corrupt) { + $modes[] = 'corrupt'; + } + + $is_compute = $args->getArg('compute'); + if ($is_compute) { + $modes[] = 'compute'; + } + + $is_overwrite = $args->getArg('overwrite'); + if ($is_overwrite) { + $modes[] = 'overwrite'; + } + + $is_verify = !$modes; + if ($is_verify) { + $modes[] = 'verify'; + } + + if (count($modes) > 1) { + throw new PhutilArgumentUsageException( + pht( + 'You have selected multiple operation modes (%s). Choose a '. + 'single mode to operate in.', + implode(', ', $modes))); + } + + $is_force = $args->getArg('force'); + if (!$is_force) { + $prompt = null; + if ($is_strip) { + $prompt = pht( + 'Stripping integrity hashes is dangerous and makes files '. + 'vulnerable to corruption or tampering.'); + } + + if ($is_corrupt) { + $prompt = pht( + 'Corrupting integrity hashes will prevent files from being '. + 'accessed. This mode is intended only for development and '. + 'debugging.'); + } + + if ($is_overwrite) { + $prompt = pht( + 'Overwriting integrity hashes is dangerous and may mark files '. + 'which have been corrupted or tampered with as safe.'); + } + + if ($prompt) { + $this->logWarn(pht('DANGEROUS'), $prompt); + + if (!phutil_console_confirm(pht('Continue anyway?'))) { + throw new PhutilArgumentUsageException(pht('Aborted workflow.')); + } + } + } + + $iterator = $this->buildIterator($args); + if (!$iterator) { + throw new PhutilArgumentUsageException( + pht( + 'Either specify a list of files to affect, or use "--all" to '. + 'affect all files.')); + } + + $failure_count = 0; + $total_count = 0; + + foreach ($iterator as $file) { + $total_count++; + $display_name = $file->getMonogram(); + + $old_hash = $file->getIntegrityHash(); + + if ($is_strip) { + if ($old_hash === null) { + $this->logInfo( + pht('SKIPPED'), + pht( + 'File "%s" does not have an integrity hash to strip.', + $display_name)); + } else { + $file + ->setIntegrityHash(null) + ->save(); + + $this->logWarn( + pht('STRIPPED'), + pht( + 'Stripped integrity hash for "%s".', + $display_name)); + } + + continue; + } + + $need_hash = ($is_verify && $old_hash) || + ($is_compute && ($old_hash === null)) || + ($is_corrupt) || + ($is_overwrite); + if ($need_hash) { + try { + $new_hash = $file->newIntegrityHash(); + } catch (Exception $ex) { + $failure_count++; + + $this->logFail( + pht('ERROR'), + pht( + 'Unable to compute integrity hash for file "%s": %s', + $display_name, + $ex->getMessage())); + + continue; + } + } else { + $new_hash = null; + } + + // NOTE: When running in "corrupt" mode, we only corrupt the hash if + // we're able to compute a valid hash. Some files, like chunked files, + // do not support integrity hashing so corrupting them would create an + // unusual state. + + if ($is_corrupt) { + if ($new_hash === null) { + $this->logInfo( + pht('IGNORED'), + pht( + 'Storage for file "%s" does not support integrity hashing.', + $display_name)); + } else { + $file + ->setIntegrityHash('') + ->save(); + + $this->logWarn( + pht('CORRUPTED'), + pht( + 'Corrupted integrity hash for file "%s".', + $display_name)); + } + + continue; + } + + if ($is_verify) { + if ($old_hash === null) { + $this->logInfo( + pht('NONE'), + pht( + 'File "%s" has no stored integrity hash.', + $display_name)); + } else if ($new_hash === null) { + $failure_count++; + + $this->logWarn( + pht('UNEXPECTED'), + pht( + 'Storage for file "%s" does not support integrity hashing, '. + 'but the file has an integrity hash.', + $display_name)); + } else if (phutil_hashes_are_identical($old_hash, $new_hash)) { + $this->logOkay( + pht('VALID'), + pht( + 'File "%s" has a valid integrity hash.', + $display_name)); + } else { + $failure_count++; + + $this->logFail( + pht('MISMATCH'), + pht( + 'File "%s" has an invalid integrity hash!', + $display_name)); + } + + continue; + } + + if ($is_compute) { + if ($old_hash !== null) { + $this->logInfo( + pht('SKIP'), + pht( + 'File "%s" already has an integrity hash.', + $display_name)); + } else if ($new_hash === null) { + $this->logInfo( + pht('IGNORED'), + pht( + 'Storage for file "%s" does not support integrity hashing.', + $display_name)); + } else { + $file + ->setIntegrityHash($new_hash) + ->save(); + + $this->logOkay( + pht('COMPUTE'), + pht( + 'Computed and stored integrity hash for file "%s".', + $display_name)); + } + + continue; + } + + if ($is_overwrite) { + $same_hash = ($old_hash !== null) && + ($new_hash !== null) && + phutil_hashes_are_identical($old_hash, $new_hash); + + if ($new_hash === null) { + $this->logInfo( + pht('IGNORED'), + pht( + 'Storage for file "%s" does not support integrity hashing.', + $display_name)); + } else if ($same_hash) { + $this->logInfo( + pht('UNCHANGED'), + pht( + 'File "%s" already has the correct integrity hash.', + $display_name)); + } else { + $file + ->setIntegrityHash($new_hash) + ->save(); + + $this->logOkay( + pht('OVERWRITE'), + pht( + 'Overwrote integrity hash for file "%s".', + $display_name)); + } + + continue; + } + } + + if ($failure_count) { + $this->logFail( + pht('FAIL'), + pht( + 'Processed %s file(s), encountered %s error(s).', + new PhutilNumber($total_count), + new PhutilNumber($failure_count))); + } else { + $this->logOkay( + pht('DONE'), + pht( + 'Processed %s file(s) with no errors.', + new PhutilNumber($total_count))); + } + + return 0; + } + +} diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index dd474d9aa6..4faecbd557 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -493,9 +493,7 @@ final class PhabricatorFile extends PhabricatorFileDAO $engine_class = get_class($engine); - $key = $this->getStorageFormat(); - $format = id(clone PhabricatorFileStorageFormat::requireFormat($key)) - ->setFile($this); + $format = $this->newStorageFormat(); $data_iterator = array($data); $formatted_iterator = $format->newWriteIterator($data_iterator); @@ -756,9 +754,7 @@ final class PhabricatorFile extends PhabricatorFileDAO public function getFileDataIterator($begin = null, $end = null) { $engine = $this->instantiateStorageEngine(); - $key = $this->getStorageFormat(); - $format = id(clone PhabricatorFileStorageFormat::requireFormat($key)) - ->setFile($this); + $format = $this->newStorageFormat(); $iterator = $engine->getRawFileDataIterator( $this, @@ -1238,6 +1234,21 @@ final class PhabricatorFile extends PhabricatorFileDAO return idx($this->metadata, self::METADATA_INTEGRITY); } + public function newIntegrityHash() { + $engine = $this->instantiateStorageEngine(); + + if ($engine->isChunkEngine()) { + return null; + } + + $format = $this->newStorageFormat(); + + $storage_handle = $this->getStorageHandle(); + $data = $engine->readFile($storage_handle); + + return $engine->newIntegrityHash($data, $format); + } + /** * Write the policy edge between this file and some object. * @@ -1406,6 +1417,16 @@ final class PhabricatorFile extends PhabricatorFileDAO return $this->assertAttachedKey($this->transforms, $key); } + public function newStorageFormat() { + $key = $this->getStorageFormat(); + $template = PhabricatorFileStorageFormat::requireFormat($key); + + $format = id(clone $template) + ->setFile($this); + + return $format; + } + /* -( PhabricatorApplicationTransactionInterface )------------------------- */ diff --git a/src/infrastructure/management/PhabricatorManagementWorkflow.php b/src/infrastructure/management/PhabricatorManagementWorkflow.php index 0cd8c0230e..ae33f5d6cc 100644 --- a/src/infrastructure/management/PhabricatorManagementWorkflow.php +++ b/src/infrastructure/management/PhabricatorManagementWorkflow.php @@ -31,4 +31,40 @@ abstract class PhabricatorManagementWorkflow extends PhutilArgumentWorkflow { PhabricatorConsoleContentSource::SOURCECONST); } + protected function logInfo($label, $message) { + $this->logRaw( + tsprintf( + "** %s ** %s\n", + $label, + $message)); + } + + protected function logOkay($label, $message) { + $this->logRaw( + tsprintf( + "** %s ** %s\n", + $label, + $message)); + } + + protected function logWarn($label, $message) { + $this->logRaw( + tsprintf( + "** %s ** %s\n", + $label, + $message)); + } + + protected function logFail($label, $message) { + $this->logRaw( + tsprintf( + "** %s ** %s\n", + $label, + $message)); + } + + private function logRaw($message) { + fprintf(STDERR, '%s', $message); + } + }