From 906424c1b6fd884bf2081bfe6dd0b1f9651c2801 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Sun, 11 Jun 2017 23:05:19 +0200 Subject: [PATCH] Crypt site credential password --- .gitignore | 1 + .../Version20170501115751.php | 2 +- app/config/wallabag.yml | 1 + composer.json | 5 +- .../CoreBundle/Command/InstallCommand.php | 2 + .../Controller/SiteCredentialController.php | 2 + .../DependencyInjection/Configuration.php | 2 + .../WallabagCoreExtension.php | 1 + .../CoreBundle/Entity/SiteCredential.php | 3 +- .../CoreBundle/Helper/CryptoProxy.php | 86 +++++++++++++++++++ .../Repository/SiteCredentialRepository.php | 20 ++++- .../CoreBundle/Resources/config/services.yml | 8 ++ .../Controller/EntryControllerTest.php | 2 +- .../GrabySiteConfigBuilderTest.php | 3 +- .../CoreBundle/Helper/CryptoProxyTest.php | 40 +++++++++ 15 files changed, 169 insertions(+), 9 deletions(-) create mode 100644 src/Wallabag/CoreBundle/Helper/CryptoProxy.php create mode 100644 tests/Wallabag/CoreBundle/Helper/CryptoProxyTest.php diff --git a/.gitignore b/.gitignore index 709f78cf2..8fd762fa8 100644 --- a/.gitignore +++ b/.gitignore @@ -56,3 +56,4 @@ app/Resources/build/ # Test-generated files admin-export.json specialexport.json +/data/site-credentials-secret-key.txt diff --git a/app/DoctrineMigrations/Version20170501115751.php b/app/DoctrineMigrations/Version20170501115751.php index 846a87b5c..2597f1ffd 100644 --- a/app/DoctrineMigrations/Version20170501115751.php +++ b/app/DoctrineMigrations/Version20170501115751.php @@ -39,7 +39,7 @@ class Version20170501115751 extends AbstractMigration implements ContainerAwareI $table->addColumn('user_id', 'integer'); $table->addColumn('host', 'string', ['length' => 255]); $table->addColumn('username', 'string', ['length' => 255]); - $table->addColumn('password', 'string', ['length' => 255]); + $table->addColumn('password', 'text'); $table->addColumn('createdAt', 'datetime'); $table->addIndex(['user_id'], 'idx_user'); $table->setPrimaryKey(['id']); diff --git a/app/config/wallabag.yml b/app/config/wallabag.yml index 51b7e4e30..b45934e4b 100644 --- a/app/config/wallabag.yml +++ b/app/config/wallabag.yml @@ -26,6 +26,7 @@ wallabag_core: fetching_error_message: | wallabag can't retrieve contents for this article. Please troubleshoot this issue. api_limit_mass_actions: 10 + encryption_key_path: "%kernel.root_dir%/../data/site-credentials-secret-key.txt" default_internal_settings: - name: share_public diff --git a/composer.json b/composer.json index 0a170c168..7428f688c 100644 --- a/composer.json +++ b/composer.json @@ -73,7 +73,7 @@ "kphoen/rulerz-bundle": "~0.13", "guzzlehttp/guzzle": "^5.3.1", "doctrine/doctrine-migrations-bundle": "^1.0", - "paragonie/random_compat": "~1.0", + "paragonie/random_compat": "~2.0", "craue/config-bundle": "~2.0", "mnapoli/piwik-twig-extension": "^1.0", "ocramius/proxy-manager": "1.*", @@ -83,7 +83,8 @@ "javibravo/simpleue": "^1.0", "symfony/dom-crawler": "^3.1", "friendsofsymfony/jsrouting-bundle": "^1.6", - "bdunogier/guzzle-site-authenticator": "^1.0.0@dev" + "bdunogier/guzzle-site-authenticator": "^1.0.0@dev", + "defuse/php-encryption": "^2.1" }, "require-dev": { "doctrine/doctrine-fixtures-bundle": "~2.2", diff --git a/src/Wallabag/CoreBundle/Command/InstallCommand.php b/src/Wallabag/CoreBundle/Command/InstallCommand.php index 0f119377f..eb725a59a 100644 --- a/src/Wallabag/CoreBundle/Command/InstallCommand.php +++ b/src/Wallabag/CoreBundle/Command/InstallCommand.php @@ -313,6 +313,8 @@ class InstallCommand extends ContainerAwareCommand $this ->runCommand('doctrine:migrations:migrate', ['--no-interaction' => true]); + + return $this; } /** diff --git a/src/Wallabag/CoreBundle/Controller/SiteCredentialController.php b/src/Wallabag/CoreBundle/Controller/SiteCredentialController.php index dc8e723da..0bacafb79 100644 --- a/src/Wallabag/CoreBundle/Controller/SiteCredentialController.php +++ b/src/Wallabag/CoreBundle/Controller/SiteCredentialController.php @@ -45,6 +45,8 @@ class SiteCredentialController extends Controller $form->handleRequest($request); if ($form->isSubmitted() && $form->isValid()) { + $credential->setPassword($this->get('wallabag_core.helper.crypto_proxy')->crypt($credential->getPassword())); + $em = $this->getDoctrine()->getManager(); $em->persist($credential); $em->flush($credential); diff --git a/src/Wallabag/CoreBundle/DependencyInjection/Configuration.php b/src/Wallabag/CoreBundle/DependencyInjection/Configuration.php index 33df92d3c..a9791f6be 100644 --- a/src/Wallabag/CoreBundle/DependencyInjection/Configuration.php +++ b/src/Wallabag/CoreBundle/DependencyInjection/Configuration.php @@ -63,6 +63,8 @@ class Configuration implements ConfigurationInterface ->end() ->end() ->end() + ->scalarNode('encryption_key_path') + ->end() ->end() ; diff --git a/src/Wallabag/CoreBundle/DependencyInjection/WallabagCoreExtension.php b/src/Wallabag/CoreBundle/DependencyInjection/WallabagCoreExtension.php index b4d8a3866..532ce2386 100644 --- a/src/Wallabag/CoreBundle/DependencyInjection/WallabagCoreExtension.php +++ b/src/Wallabag/CoreBundle/DependencyInjection/WallabagCoreExtension.php @@ -29,6 +29,7 @@ class WallabagCoreExtension extends Extension $container->setParameter('wallabag_core.fetching_error_message_title', $config['fetching_error_message_title']); $container->setParameter('wallabag_core.api_limit_mass_actions', $config['api_limit_mass_actions']); $container->setParameter('wallabag_core.default_internal_settings', $config['default_internal_settings']); + $container->setParameter('wallabag_core.site_credentials.encryption_key_path', $config['encryption_key_path']); $loader = new Loader\YamlFileLoader($container, new FileLocator(__DIR__.'/../Resources/config')); $loader->load('services.yml'); diff --git a/src/Wallabag/CoreBundle/Entity/SiteCredential.php b/src/Wallabag/CoreBundle/Entity/SiteCredential.php index 85ee07d4a..732d95066 100644 --- a/src/Wallabag/CoreBundle/Entity/SiteCredential.php +++ b/src/Wallabag/CoreBundle/Entity/SiteCredential.php @@ -46,8 +46,7 @@ class SiteCredential * @var string * * @Assert\NotBlank() - * @Assert\Length(max=255) - * @ORM\Column(name="password", type="string", length=255) + * @ORM\Column(name="password", type="text") */ private $password; diff --git a/src/Wallabag/CoreBundle/Helper/CryptoProxy.php b/src/Wallabag/CoreBundle/Helper/CryptoProxy.php new file mode 100644 index 000000000..d0a9b85c8 --- /dev/null +++ b/src/Wallabag/CoreBundle/Helper/CryptoProxy.php @@ -0,0 +1,86 @@ +logger = $logger; + + if (!file_exists($encryptionKeyPath)) { + $key = Key::createNewRandomKey(); + + file_put_contents($encryptionKeyPath, $key->saveToAsciiSafeString()); + chmod($encryptionKeyPath, 0600); + } + + $this->encryptionKey = file_get_contents($encryptionKeyPath); + } + + /** + * Ensure the given value will be crypted. + * + * @param string $secretValue Secret valye to crypt + * + * @return string + */ + public function crypt($secretValue) + { + $this->logger->debug('Crypto: crypting value: '.$this->mask($secretValue)); + + return Crypto::encrypt($secretValue, $this->loadKey()); + } + + /** + * Ensure the given crypted value will be decrypted. + * + * @param string $cryptedValue The value to be decrypted + * + * @return string + */ + public function decrypt($cryptedValue) + { + $this->logger->debug('Crypto: decrypting value: '.$this->mask($cryptedValue)); + + try { + return Crypto::decrypt($cryptedValue, $this->loadKey()); + } catch (WrongKeyOrModifiedCiphertextException $e) { + throw new \RuntimeException('Decrypt fail: '.$e->getMessage()); + } + } + + /** + * Load the private key. + * + * @return string + */ + private function loadKey() + { + return Key::loadFromAsciiSafeString($this->encryptionKey); + } + + /** + * Keep first and last character and put some stars in between. + * + * @param string $value Value to mask + * + * @return string + */ + private function mask($value) + { + return $value[0].'*****'.$value[strlen($value) - 1]; + } +} diff --git a/src/Wallabag/CoreBundle/Repository/SiteCredentialRepository.php b/src/Wallabag/CoreBundle/Repository/SiteCredentialRepository.php index 316ecc750..6f904f0ae 100644 --- a/src/Wallabag/CoreBundle/Repository/SiteCredentialRepository.php +++ b/src/Wallabag/CoreBundle/Repository/SiteCredentialRepository.php @@ -2,11 +2,20 @@ namespace Wallabag\CoreBundle\Repository; +use Wallabag\CoreBundle\Helper\CryptoProxy; + /** * SiteCredentialRepository. */ class SiteCredentialRepository extends \Doctrine\ORM\EntityRepository { + private $cryptoProxy; + + public function setCrypto(CryptoProxy $cryptoProxy) + { + $this->cryptoProxy = $cryptoProxy; + } + /** * Retrieve one username/password for the given host and userId. * @@ -17,12 +26,21 @@ class SiteCredentialRepository extends \Doctrine\ORM\EntityRepository */ public function findOneByHostAndUser($host, $userId) { - return $this->createQueryBuilder('s') + $res = $this->createQueryBuilder('s') ->select('s.username', 's.password') ->where('s.host = :hostname')->setParameter('hostname', $host) ->andWhere('s.user = :userId')->setParameter('userId', $userId) ->setMaxResults(1) ->getQuery() ->getOneOrNullResult(); + + if (null === $res) { + return; + } + + // decrypt password before returning it + $res['password'] = $this->cryptoProxy->decrypt($res['password']); + + return $res; } } diff --git a/src/Wallabag/CoreBundle/Resources/config/services.yml b/src/Wallabag/CoreBundle/Resources/config/services.yml index 09bc77fe9..e09b0f185 100644 --- a/src/Wallabag/CoreBundle/Resources/config/services.yml +++ b/src/Wallabag/CoreBundle/Resources/config/services.yml @@ -126,6 +126,8 @@ services: factory: [ "@doctrine.orm.default_entity_manager", getRepository ] arguments: - WallabagCoreBundle:SiteCredential + calls: + - [ setCrypto, [ "@wallabag_core.helper.crypto_proxy" ] ] wallabag_core.helper.entries_export: class: Wallabag\CoreBundle\Helper\EntriesExport @@ -208,3 +210,9 @@ services: wallabag_core.entry.download_images.client: class: GuzzleHttp\Client + + wallabag_core.helper.crypto_proxy: + class: Wallabag\CoreBundle\Helper\CryptoProxy + arguments: + - "%wallabag_core.site_credentials.encryption_key_path%" + - "@logger" diff --git a/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php b/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php index 104f60f17..803806853 100644 --- a/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php +++ b/tests/Wallabag/CoreBundle/Controller/EntryControllerTest.php @@ -1341,7 +1341,7 @@ class EntryControllerTest extends WallabagCoreTestCase $credential = new SiteCredential($user); $credential->setHost('monde-diplomatique.fr'); $credential->setUsername('foo'); - $credential->setPassword('bar'); + $credential->setPassword($client->getContainer()->get('wallabag_core.helper.crypto_proxy')->crypt('bar')); $em->persist($credential); $em->flush(); diff --git a/tests/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilderTest.php b/tests/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilderTest.php index 1e1e8989b..b0c81e7b0 100644 --- a/tests/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilderTest.php +++ b/tests/Wallabag/CoreBundle/GuzzleSiteAuthenticator/GrabySiteConfigBuilderTest.php @@ -6,12 +6,11 @@ use Monolog\Handler\TestHandler; use Monolog\Logger; use BD\GuzzleSiteAuthenticator\SiteConfig\SiteConfig; use Graby\SiteConfig\SiteConfig as GrabySiteConfig; -use PHPUnit_Framework_TestCase; use Wallabag\CoreBundle\GuzzleSiteAuthenticator\GrabySiteConfigBuilder; use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken; use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorage; -class GrabySiteConfigBuilderTest extends PHPUnit_Framework_TestCase +class GrabySiteConfigBuilderTest extends \PHPUnit_Framework_TestCase { /** @var \Wallabag\CoreBundle\GuzzleSiteAuthenticator\GrabySiteConfigBuilder */ protected $builder; diff --git a/tests/Wallabag/CoreBundle/Helper/CryptoProxyTest.php b/tests/Wallabag/CoreBundle/Helper/CryptoProxyTest.php new file mode 100644 index 000000000..cede86962 --- /dev/null +++ b/tests/Wallabag/CoreBundle/Helper/CryptoProxyTest.php @@ -0,0 +1,40 @@ +crypt('test'); + $decrypted = $crypto->decrypt($crypted); + + $this->assertSame('test', $decrypted); + + $records = $logHandler->getRecords(); + $this->assertCount(2, $records); + $this->assertContains('Crypto: crypting value', $records[0]['message']); + $this->assertContains('Crypto: decrypting value', $records[1]['message']); + } + + /** + * @expectedException RuntimeException + * @expectedExceptionMessage Decrypt fail + * + * @return [type] [description] + */ + public function testDecryptBadValue() + { + $crypto = new CryptoProxy(sys_get_temp_dir().'/'.uniqid('', true).'.txt', new NullLogger()); + $crypto->decrypt('badvalue'); + } +}