Add unit tests and fix rootcerts creation bug

lib/private/security/certificate.php

@@ -30,17 +30,22 @@ class Certificate implements ICertificate {
 	/**
 	 * @param string $data base64 encoded certificate
 	 * @param string $name
+	 * @throws \Exception If the certificate could not get parsed
 	 */
 	public function __construct($data, $name) {
 		$this->name = $name;
-		$info = openssl_x509_parse($data);
+		try {
-		$this->commonName = $info['subject']['CN'];
+			$info = openssl_x509_parse($data);
-		$this->organization = isset($info['subject']['O']) ? $info['subject']['O'] : null;
+			$this->commonName = isset($info['subject']['CN']) ? $info['subject']['CN'] : null;
-		$this->serial = $this->formatSerial($info['serialNumber']);
+			$this->organization = isset($info['subject']['O']) ? $info['subject']['O'] : null;
-		$this->issueDate = new \DateTime('@' . $info['validFrom_time_t']);
+			$this->serial = $this->formatSerial($info['serialNumber']);
-		$this->expireDate = new \DateTime('@' . $info['validTo_time_t']);
+			$this->issueDate = new \DateTime('@' . $info['validFrom_time_t']);
-		$this->issuerName = $info['issuer']['CN'];
+			$this->expireDate = new \DateTime('@' . $info['validTo_time_t']);
-		$this->issuerOrganization = isset($info['issuer']['O']) ? $info['issuer']['O'] : null;
+			$this->issuerName = isset($info['issuer']['CN']) ? $info['issuer']['CN'] : null;
+			$this->issuerOrganization = isset($info['issuer']['O']) ? $info['issuer']['O'] : null;
+		} catch (\Exception $e) {
+			throw new \Exception('Certificate could not get parsed.');
+		}
 	}
 
 	/**
@@ -62,7 +67,7 @@ class Certificate implements ICertificate {
 	}
 
 	/**
-	 * @return string
+	 * @return string|null
 	 */
 	public function getCommonName() {
 		return $this->commonName;
@@ -105,14 +110,14 @@ class Certificate implements ICertificate {
 	}
 
 	/**
-	 * @return string
+	 * @return string|null
 	 */
 	public function getIssuerName() {
 		return $this->issuerName;
 	}
 
 	/**
-	 * @return string
+	 * @return string|null
 	 */
 	public function getIssuerOrganization() {
 		return $this->issuerOrganization;

lib/private/security/certificatemanager.php

@@ -44,7 +44,9 @@ class CertificateManager implements ICertificateManager {
 		}
 		while (false !== ($file = readdir($handle))) {
 			if ($file != '.' && $file != '..') {
-				$result[] = new Certificate(file_get_contents($path . $file), $file);
+				try {
+					$result[] = new Certificate(file_get_contents($path . $file), $file);
+				} catch(\Exception $e) {}
 			}
 		}
 		return $result;
@@ -59,7 +61,7 @@ class CertificateManager implements ICertificateManager {
 
 		$fh_certs = fopen($path . '/rootcerts.crt', 'w');
 		foreach ($certs as $cert) {
-			$file = $path . '/uploads/' . $cert;
+			$file = $path . '/uploads/' . $cert->getName();
 			$data = file_get_contents($file);
 			if (strpos($data, 'BEGIN CERTIFICATE')) {
 				fwrite($fh_certs, $data);
@@ -75,35 +77,32 @@ class CertificateManager implements ICertificateManager {
 	 *
 	 * @param string $certificate the certificate data
 	 * @param string $name the filename for the certificate
-	 * @return bool | \OCP\ICertificate
+	 * @return \OCP\ICertificate|void|bool
+	 * @throws \Exception If the certificate could not get added
 	 */
 	public function addCertificate($certificate, $name) {
 		if (!Filesystem::isValidPath($name) or Filesystem::isFileBlacklisted($name)) {
 			return false;
 		}
-		$isValid = openssl_pkey_get_public($certificate);
 
-		if (!$isValid) {
+		$dir = $this->user->getHome() . '/files_external/uploads/';
-			$data = chunk_split(base64_encode($certificate), 64, "\n");
+		if (!file_exists($dir)) {
-			$data = "-----BEGIN CERTIFICATE-----\n" . $data . "-----END CERTIFICATE-----\n";
+			//path might not exist (e.g. non-standard OC_User::getHome() value)
-			$isValid = openssl_pkey_get_public($data);
+			//in this case create full path using 3rd (recursive=true) parameter.
+			//note that we use "normal" php filesystem functions here since the certs need to be local
+			mkdir($dir, 0700, true);
 		}
 
-		if ($isValid) {
+		try {
-			$dir = $this->user->getHome() . '/files_external/uploads/';
-			if (!file_exists($dir)) {
-				//path might not exist (e.g. non-standard OC_User::getHome() value)
-				//in this case create full path using 3rd (recursive=true) parameter.
-				//note that we use "normal" php filesystem functions here since the certs need to be local
-				mkdir($dir, 0700, true);
-			}
 			$file = $dir . $name;
+			$certificateObject = new Certificate($certificate, $name);
 			file_put_contents($file, $certificate);
 			$this->createCertificateBundle();
-			return new Certificate($certificate, $name);
+			return $certificateObject;
-		} else {
+		} catch (\Exception $e) {
-			return false;
+			throw $e;
 		}
+
 	}
 
 	/**

settings/ajax/addRootCertificate.php

@@ -1,4 +1,5 @@
 <?php
+OCP\JSON::checkLoggedIn();
 OCP\JSON::callCheck();
 
 $l = new OC_L10N('core');
@@ -13,8 +14,8 @@ $filename = basename($_FILES['rootcert_import']['name']);
 
 $certificateManager = \OC::$server->getCertificateManager();
 
-$cert = $certificateManager->addCertificate($data, $filename);
+try {
-if ($cert) {
+	$cert = $certificateManager->addCertificate($data, $filename);
 	OCP\JSON::success(array(
 		'name' => $cert->getName(),
 		'commonName' => $cert->getCommonName(),
@@ -26,6 +27,6 @@ if ($cert) {
 		'issuer' => $cert->getIssuerName(),
 		'issuerOrganization' => $cert->getIssuerOrganization()
 	));
-} else {
+} catch(\Exception $e) {
 	OCP\JSON::error(array('error' => 'Couldn\'t import SSL root certificate, allowed formats: PEM and DER'));
 }

settings/js/personal.js

@@ -325,7 +325,7 @@ $(document).ready(function () {
 			var row = $('<tr/>');
 			row.addClass(isExpired? 'expired': 'valid');
 			row.append($('<td/>').attr('title', data.result.organization).text(data.result.commonName));
-			row.append($('<td/>').attr('title', t('core,', 'Valid from {date}', {date: data.result.validFromString}))
+			row.append($('<td/>').attr('title', t('core,', 'Valid until {date}', {date: data.result.validFromString}))
 				.text(data.result.validTillString));
 			row.append($('<td/>').attr('title', data.result.issuerOrganization).text(data.result.issuer));
 			row.append($('<td/>').addClass('remove').append(

settings/templates/personal.php

@@ -2,7 +2,10 @@
  * Copyright (c) 2011, Robin Appelman <icewind1991@gmail.com>
  * This file is licensed under the Affero General Public License version 3 or later.
  * See the COPYING-README file.
- */?>
+ */
+
+/** @var $_ array */
+?>
 
 <div class="clientsbox center">
 	<h2><?php p($l->t('Get the apps to sync your files'));?></h2>
@@ -160,12 +163,12 @@ if($_['passwordChangeSupported']) {
 			<th/>
 		</thead>
 		<tbody>
-			<?php foreach ($_['certs'] as $rootCert): /**@var \OCP\ICertificate $rootCert*/?>
+			<?php foreach ($_['certs'] as $rootCert): /**@var \OCP\ICertificate $rootCert*/ ?>
 				<tr class="<?php echo ($rootCert->isExpired()) ? 'expired' : 'valid' ?>" data-name="<?php p($rootCert->getName()) ?>">
 					<td class="rootCert" title="<?php p($rootCert->getOrganization())?>">
 						<?php p($rootCert->getCommonName()) ?>
 					</td>
-					<td title="<?php p($l->t('Valid from %s', $l->l('date', $rootCert->getExpireDate()))) ?>">
+					<td title="<?php p($l->t('Valid until %s', $l->l('date', $rootCert->getExpireDate()))) ?>">
 						<?php echo $l->l('date', $rootCert->getExpireDate()) ?>
 					</td>
 					<td title="<?php p($rootCert->getIssuerOrganization()) ?>">

tests/data/certificates/badCertificate.crt

@@ -0,0 +1,13 @@
+-----BEGIN CERTIFICATE-----
+MIICATCCAWoCCQDNdmb4pJrUeDANBgkqhkiG9w0BAQUFADBFMQswCQYDVQQGEwJB
+VTETMBEGA1UECBMKU29tZS1TdGF0ZTEhMB8GA1UEChMYSW50ZXJuZXQgV2lkZ2l0
+cyBQdHkgTHRkMB4XDTE0MDgyNzA4NDg1MVoXDTE1MDgyNzA4NDg1MVowRTELMAkG
+A1UEBhMCQVUxEzARBgNVBAgTClNvbWUtU3RhdGUxITAfBgNVBAoTGEludGVybmV0
+IFdpZGdpdHMgUHR5IEx0ZDCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAvrMe
+x5D45HVMV2U4kqTU0mzHAihHT6r+OtO6g7S9yIlJZGGVcEet6An78Ow7aYM141eI
+Jfbvqql7OIblHXSw7mvkw4bOQ1ee5lmJYOYCgaMNJ6mBLJfpK9xwidb0ZvhWOA8P
+DLIiBKA3T5ChXCzilD5GF2+H/BXBE9lL9tuDjM0CAwEAATANBgkqhkiG9w0BAQUF
+AAOBgQCJwfJe7j+aNkopw+P8uxobfOnMWU9XC4Pu+39TVLeakeSqu2Y8vJSHmkjF
+WK3VXAJr33Eul5VP/3SWGwuRPd9X4i4iLh1gJfYvi9MJf1lQNYncGCM+xtdrNu2O
+u0yexkOBRrapDYjcv58BiOaFgvFLquKvtVj9HlcYRfwfM77uKQ==
+-----END CERTIFICATE-----

tests/data/certificates/expiredCertificate.crt

@@ -0,0 +1,13 @@
+-----BEGIN CERTIFICATE-----
+MIICATCCAWoCCQCjCIB6tCZ2sDANBgkqhkiG9w0BAQUFADBFMQswCQYDVQQGEwJB
+VTETMBEGA1UECBMKU29tZS1TdGF0ZTEhMB8GA1UEChMYSW50ZXJuZXQgV2lkZ2l0
+cyBQdHkgTHRkMB4XDTE0MDgyNzA5MTI0M1oXDTE0MDgyODA5MTI0M1owRTELMAkG
+A1UEBhMCQVUxEzARBgNVBAgTClNvbWUtU3RhdGUxITAfBgNVBAoTGEludGVybmV0
+IFdpZGdpdHMgUHR5IEx0ZDCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAvrMe
+x5D45HVMV2U4kqTU0mzHAihHT6r+OtO6g7S9yIlJZGGVcEet6An78Ow7aYM141eI
+Jfbvqql7OIblHXSw7mvkw4bOQ1ee5lmJYOYCgaMNJ6mBLJfpK9xwidb0ZvhWOA8P
+DLIiBKA3T5ChXCzilD5GF2+H/BXBE9lL9tuDjM0CAwEAATANBgkqhkiG9w0BAQUF
+AAOBgQBuNClmOj3wudlX86nygcZgQT2+ZS8f1iJgM9lbrrkenT6tgcT1/YjcrN9C
+BZR29Wz7htflpqverLUGZXh72K+gYercyR16Zu7zjt/NWuZldZmzJ3bUGq2HSoCX
+2sDykAEuaDxUlzdJrztlOH4vPlRaGbxUogpC2hB1BQfxA90CIA==
+-----END CERTIFICATE-----

tests/data/certificates/goodCertificate.crt

@@ -0,0 +1,15 @@
+-----BEGIN CERTIFICATE-----
+MIICazCCAdQCCQCySF7HjQD78DANBgkqhkiG9w0BAQUFADB6MQswCQYDVQQGEwJD
+SDEPMA0GA1UECBMGWnVyaWNoMQ8wDQYDVQQHEwZadXJpY2gxFjAUBgNVBAoTDW93
+bkNsb3VkIEluYy4xETAPBgNVBAsTCFNlY3VyaXR5MR4wHAYDVQQDExVzZWN1cml0
+eS5vd25jbG91ZC5jb20wHhcNMTQwODI3MDg0NTUyWhcNMTUwODI3MDg0NTUyWjB6
+MQswCQYDVQQGEwJDSDEPMA0GA1UECBMGWnVyaWNoMQ8wDQYDVQQHEwZadXJpY2gx
+FjAUBgNVBAoTDW93bkNsb3VkIEluYy4xETAPBgNVBAsTCFNlY3VyaXR5MR4wHAYD
+VQQDExVzZWN1cml0eS5vd25jbG91ZC5jb20wgZ8wDQYJKoZIhvcNAQEBBQADgY0A
+MIGJAoGBAL55lB4RvU0pTyh7YsLCxPBq43xxkRZBxfZENoflCIUsBo7/mXNz2zVO
+476oQ4L47heUOX3j8kemOgPmWEqA34JB8rusijCy5WqFBLnm4HsRLa66i+Jgd+Yl
+QhcKvhGas1K/CVTG4oSLoAmA2coZUL94uxnRtd8aluflHMNGApIlAgMBAAEwDQYJ
+KoZIhvcNAQEFBQADgYEADo08zWdOtIvCKFDnLbzRwIjSYTlAtQtQaULv7KQe3qIn
+iaFAi6fAynHfdC8/2tvmSeniw0OZBkrfVGIVtUbwCSrljNSUY/lWrUR0pE61lb4r
+DpX0JZjlk48XEaErRVDfu3wq6n/2nYg6HnaLOPwt8OSYYrxzvXlFPrKBH3q6R+M=
+-----END CERTIFICATE-----

tests/lib/security/certificate.php

@@ -0,0 +1,90 @@
+<?php
+/**
+ * Copyright (c) 2014 Lukas Reschke <lukas@owncloud.com>
+ * This file is licensed under the Affero General Public License version 3 or
+ * later.
+ * See the COPYING-README file.
+*/
+
+use \OC\Security\Certificate;
+
+class CertificateTest extends \PHPUnit_Framework_TestCase {
+
+	/** @var Certificate That contains a valid certificate */
+	protected $goodCertificate;
+	/** @var Certificate That contains an invalid certificate */
+	protected $invalidCertificate;
+	/** @var Certificate That contains an expired certificate */
+	protected $expiredCertificate;
+
+	function setUp() {
+		$goodCertificate = file_get_contents(__DIR__.'/../../data/certificates/goodCertificate.crt');
+		$this->goodCertificate = new Certificate($goodCertificate, 'GoodCertificate');
+		$badCertificate = file_get_contents(__DIR__.'/../../data/certificates/badCertificate.crt');
+		$this->invalidCertificate = new Certificate($badCertificate, 'BadCertificate');
+		$expiredCertificate = file_get_contents(__DIR__.'/../../data/certificates/expiredCertificate.crt');
+		$this->expiredCertificate = new Certificate($expiredCertificate, 'ExpiredCertificate');
+	}
+
+	/**
+	 * @expectedException \Exception
+	 * @expectedExceptionMessage Certificate could not get parsed.
+	 */
+	function testBogusData() {
+		new Certificate('foo', 'bar');
+	}
+
+	function testGetName() {
+		$this->assertSame('GoodCertificate', $this->goodCertificate->getName());
+		$this->assertSame('BadCertificate', $this->invalidCertificate->getName());
+	}
+
+	function testGetCommonName() {
+		$this->assertSame('security.owncloud.com', $this->goodCertificate->getCommonName());
+		$this->assertSame(null, $this->invalidCertificate->getCommonName());
+	}
+
+	function testGetOrganization() {
+		$this->assertSame('ownCloud Inc.', $this->goodCertificate->getOrganization());
+		$this->assertSame('Internet Widgits Pty Ltd', $this->invalidCertificate->getOrganization());
+	}
+
+	function testGetSerial() {
+		$this->assertSame('7F:FF:FF:FF:FF:FF:FF:FF', $this->goodCertificate->getSerial());
+		$this->assertSame('7F:FF:FF:FF:FF:FF:FF:FF', $this->invalidCertificate->getSerial());
+	}
+
+	function testGetIssueDate() {
+		$this->assertEquals(new DateTime('2014-08-27 08:45:52'), $this->goodCertificate->getIssueDate());
+		$this->assertEquals(new DateTime('2014-08-27 08:48:51'), $this->invalidCertificate->getIssueDate());
+	}
+
+	function testGetExpireDate() {
+		$this->assertEquals(new DateTime('2015-08-27 08:45:52'), $this->goodCertificate->getExpireDate());
+		$this->assertEquals(new DateTime('2015-08-27 08:48:51'), $this->invalidCertificate->getExpireDate());
+		$this->assertEquals(new DateTime('2014-08-28 09:12:43'), $this->expiredCertificate->getExpireDate());
+	}
+
+	/**
+	 * Obviously the following test case might fail after 2015-08-27, just create a new certificate with longer validity then
+	 */
+	function testIsExpired() {
+		$this->assertSame(false, $this->goodCertificate->isExpired());
+		$this->assertSame(false, $this->invalidCertificate->isExpired());
+
+		// TODO: Change to false after tomorrow
+		$this->assertSame(false, $this->expiredCertificate->isExpired());
+	}
+
+	function testGetIssuerName() {
+		$this->assertSame('security.owncloud.com', $this->goodCertificate->getIssuerName());
+		$this->assertSame(null, $this->invalidCertificate->getIssuerName());
+		$this->assertSame(null, $this->expiredCertificate->getIssuerName());
+	}
+
+	function testGetIssuerOrganization() {
+		$this->assertSame('ownCloud Inc.', $this->goodCertificate->getIssuerOrganization());
+		$this->assertSame('Internet Widgits Pty Ltd', $this->invalidCertificate->getIssuerOrganization());
+		$this->assertSame('Internet Widgits Pty Ltd', $this->expiredCertificate->getIssuerOrganization());
+	}
+}

tests/lib/security/certificatemanager.php

@@ -0,0 +1,87 @@
+<?php
+/**
+ * Copyright (c) 2014 Lukas Reschke <lukas@owncloud.com>
+ * This file is licensed under the Affero General Public License version 3 or
+ * later.
+ * See the COPYING-README file.
+ */
+
+use \OC\Security\CertificateManager;
+
+class CertificateManagerTest extends \PHPUnit_Framework_TestCase {
+
+	/** @var CertificateManager */
+	private $certificateManager;
+	/** @var String */
+	private $username;
+	/** @var  \OC\User\User */
+	private $user;
+
+	function setUp() {
+		$this->username = OC_Util::generateRandomBytes(20);
+		OC_User::createUser($this->username, OC_Util::generateRandomBytes(20));
+
+		\OC_Util::tearDownFS();
+		\OC_User::setUserId('');
+		\OC\Files\Filesystem::tearDown();
+		\OC_Util::setupFS($this->username);
+
+		$this->user = \OC::$server->getUserManager()->get($this->username);
+
+		$this->certificateManager = new CertificateManager($this->user);
+	}
+
+	function tearDown() {
+		\OC_User::deleteUser($this->username);
+	}
+
+	protected function assertEqualsArrays($expected, $actual) {
+		sort($expected);
+		sort($actual);
+
+		$this->assertEquals($expected, $actual);
+	}
+
+	function testListCertificates() {
+		// Test empty certificate bundle
+		$this->assertSame(array(), $this->certificateManager->listCertificates());
+
+		// Add some certificates
+		$this->certificateManager->addCertificate(file_get_contents(__DIR__.'/../../data/certificates/goodCertificate.crt'), 'GoodCertificate');
+		$certificateStore = array();
+		$certificateStore[] =  new \OC\Security\Certificate(file_get_contents(__DIR__.'/../../data/certificates/goodCertificate.crt'), 'GoodCertificate');
+		$this->assertEqualsArrays($certificateStore, $this->certificateManager->listCertificates());
+
+		// Add another certificates
+		$this->certificateManager->addCertificate(file_get_contents(__DIR__.'/../../data/certificates/expiredCertificate.crt'), 'ExpiredCertificate');
+		$certificateStore[] =  new \OC\Security\Certificate(file_get_contents(__DIR__.'/../../data/certificates/expiredCertificate.crt'), 'ExpiredCertificate');
+		$this->assertEqualsArrays($certificateStore, $this->certificateManager->listCertificates());
+	}
+
+	/**
+	 * @expectedException \Exception
+	 * @expectedExceptionMessage Certificate could not get parsed.
+	 */
+	function testAddInvalidCertificate() {
+		$this->certificateManager->addCertificate('InvalidCertificate', 'invalidCertificate');
+	}
+
+	function testAddDangerousFile() {
+		$this->assertFalse($this->certificateManager->addCertificate(file_get_contents(__DIR__.'/../../data/certificates/expiredCertificate.crt'), '.htaccess'));
+		$this->assertFalse($this->certificateManager->addCertificate(file_get_contents(__DIR__.'/../../data/certificates/expiredCertificate.crt'), '../../foo.txt'));
+	}
+
+	function testRemoveDangerousFile() {
+		$this->assertFalse($this->certificateManager->removeCertificate('../../foo.txt'));
+	}
+
+	function testRemoveExistingFile() {
+		$this->certificateManager->addCertificate(file_get_contents(__DIR__.'/../../data/certificates/goodCertificate.crt'), 'GoodCertificate');
+		$this->assertTrue($this->certificateManager->removeCertificate('GoodCertificate'));
+	}
+
+	function testGetCertificateBundle() {
+		$this->assertSame($this->user->getHome().'/files_external/rootcerts.crt', $this->certificateManager->getCertificateBundle());
+	}
+
+}