Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[downstream] Report failures for SignApp and SignCore #2724

Merged
merged 3 commits into from
Dec 21, 2016
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions core/Command/Integrity/SignApp.php
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,13 @@ protected function execute(InputInterface $input, OutputInterface $output) {
$x509 = new X509();
$x509->loadX509($keyBundle);
$x509->setPrivateKey($rsa);
$this->checker->writeAppSignature($path, $x509, $rsa);

$output->writeln('Successfully signed "'.$path.'"');
try {
$this->checker->writeAppSignature($path, $x509, $rsa);
$output->writeln('Successfully signed "'.$path.'"');
} catch (\Exception $e){
$output->writeln('Error: ' . $e->getMessage());
return 1;
}
return 0;
}
}
12 changes: 8 additions & 4 deletions core/Command/Integrity/SignCore.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,10 @@
namespace OC\Core\Command\Integrity;

use OC\IntegrityCheck\Checker;
use OC\IntegrityCheck\Helpers\EnvironmentHelper;
use OC\IntegrityCheck\Helpers\FileAccessHelper;
use phpseclib\Crypt\RSA;
use phpseclib\File\X509;
use Symfony\Component\Console\Command\Command;
use OCP\IConfig;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;
Expand Down Expand Up @@ -94,8 +92,14 @@ protected function execute(InputInterface $input, OutputInterface $output) {
$x509 = new X509();
$x509->loadX509($keyBundle);
$x509->setPrivateKey($rsa);
$this->checker->writeCoreSignature($x509, $rsa, $path);

$output->writeln('Successfully signed "core"');
try {
$this->checker->writeCoreSignature($x509, $rsa, $path);
$output->writeln('Successfully signed "core"');
} catch (\Exception $e){
$output->writeln('Error: ' . $e->getMessage());
return 1;
}
return 0;
}
}
38 changes: 29 additions & 9 deletions lib/private/IntegrityCheck/Checker.php
Original file line number Diff line number Diff line change
Expand Up @@ -271,16 +271,24 @@ private function createSignatureData(array $hashes,
public function writeAppSignature($path,
X509 $certificate,
RSA $privateKey) {
if(!is_dir($path)) {
throw new \Exception('Directory does not exist.');
}
$appInfoDir = $path . '/appinfo';
$this->fileAccessHelper->assertDirectoryExists($path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_dir does not fail, when one of the parents doesn't exist, so this can be removed in favor of the next line

$this->fileAccessHelper->assertDirectoryExists($appInfoDir);

$iterator = $this->getFolderIterator($path);
$hashes = $this->generateHashes($iterator, $path);
$signature = $this->createSignatureData($hashes, $certificate, $privateKey);
$this->fileAccessHelper->file_put_contents(
$path . '/appinfo/signature.json',
try {
$this->fileAccessHelper->file_put_contents(
$appInfoDir . '/signature.json',
json_encode($signature, JSON_PRETTY_PRINT)
);
);
} catch (\Exception $e){
if (!$this->fileAccessHelper->is_writeable($appInfoDir)){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check if signature.json is writeable instead of the folder?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_writable will return false is the file doesn't exist yet. So I think checking for the folder is right here.

throw new \Exception($appInfoDir . ' is not writable');
}
throw $e;
}
}

/**
Expand All @@ -289,17 +297,29 @@ public function writeAppSignature($path,
* @param X509 $certificate
* @param RSA $rsa
* @param string $path
* @throws \Exception
*/
public function writeCoreSignature(X509 $certificate,
RSA $rsa,
$path) {
$coreDir = $path . '/core';
$this->fileAccessHelper->assertDirectoryExists($path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_dir does not fail, when one of the parents doesn't exist, so this can be removed in favor of the next line

$this->fileAccessHelper->assertDirectoryExists($coreDir);

$iterator = $this->getFolderIterator($path, $path);
$hashes = $this->generateHashes($iterator, $path);
$signatureData = $this->createSignatureData($hashes, $certificate, $rsa);
$this->fileAccessHelper->file_put_contents(
$path . '/core/signature.json',
try {
$this->fileAccessHelper->file_put_contents(
$coreDir . '/signature.json',
json_encode($signatureData, JSON_PRETTY_PRINT)
);
);
} catch (\Exception $e){
if (!$this->fileAccessHelper->is_writeable($coreDir)){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, check that signature.json is writable

throw new \Exception($coreDir . ' is not writable');
}
throw $e;
}
}

/**
Expand Down
29 changes: 26 additions & 3 deletions lib/private/IntegrityCheck/Helpers/FileAccessHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,33 @@ public function file_exists($filename) {
* Wrapper around file_put_contents($filename, $data)
*
* @param string $filename
* @param $data
* @return int|false
* @param string $data
* @return int
* @throws \Exception
*/
public function file_put_contents($filename, $data) {
return file_put_contents($filename, $data);
$bytesWritten = file_put_contents($filename, $data);
if ($bytesWritten === false || $bytesWritten !== strlen($data)){
throw new \Exception('Failed to write into ' . $filename);
}
return $bytesWritten;
}

/**
* @param string $path
* @return bool
*/
public function is_writeable($path){
return is_writeable($path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bildschirmfoto vom 2016-12-19 11-44-19

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this has been done to allow mocking here. Let me actually write tests for this and then that makes some more sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well no problem with the wrapper name, but the actual function in php is:

function is_writeable($path) {
    return is_writable($path);
}

so killing the e will reduce nesting by 1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Gotcha :)

}

/**
* @param string $path
* @throws \Exception
*/
public function assertDirectoryExists($path){
if (!is_dir($path)) {
throw new \Exception('Directory ' . $path . ' does not exist.');
}
}
}
19 changes: 18 additions & 1 deletion tests/lib/IntegrityCheck/CheckerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ public function setUp() {

/**
* @expectedException \Exception
* @expectedExceptionMessage Directory does not exist.
*/
public function testWriteAppSignatureOfNotExistingApp() {
$keyBundle = file_get_contents(__DIR__ .'/../../data/integritycheck/SomeApp.crt');
Expand All @@ -89,6 +88,24 @@ public function testWriteAppSignatureOfNotExistingApp() {
$this->checker->writeAppSignature('NotExistingApp', $x509, $rsa);
}

/**
* @expectedException \Exception
*/
public function testWriteAppSignatureWrongPermissions(){
$this->fileAccessHelper
->expects($this->once())
->method('file_put_contents')
->will($this->throwException(new \Exception))
;
$keyBundle = file_get_contents(__DIR__ .'/../../data/integritycheck/SomeApp.crt');
$rsaPrivateKey = file_get_contents(__DIR__ .'/../../data/integritycheck/SomeApp.key');
$rsa = new RSA();
$rsa->loadKey($rsaPrivateKey);
$x509 = new X509();
$x509->loadX509($keyBundle);
$this->checker->writeAppSignature(\OC::$SERVERROOT . '/tests/data/integritycheck/app/', $x509, $rsa);
}

public function testWriteAppSignature() {
$expectedSignatureFileData = '{
"hashes": {
Expand Down