Skip to content

Commit

Permalink
Migrate Randomness secure check to SetupCheck API
Browse files Browse the repository at this point in the history
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
  • Loading branch information
come-nc committed Nov 7, 2023
1 parent 14c5162 commit 903462a
Show file tree
Hide file tree
Showing 8 changed files with 67 additions and 123 deletions.
1 change: 1 addition & 0 deletions apps/settings/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
'OCA\\Settings\\SetupChecks\\PhpModules' => $baseDir . '/../lib/SetupChecks/PhpModules.php',
'OCA\\Settings\\SetupChecks\\PhpOutdated' => $baseDir . '/../lib/SetupChecks/PhpOutdated.php',
'OCA\\Settings\\SetupChecks\\PhpOutputBuffering' => $baseDir . '/../lib/SetupChecks/PhpOutputBuffering.php',
'OCA\\Settings\\SetupChecks\\RandomnessSecure' => $baseDir . '/../lib/SetupChecks/RandomnessSecure.php',
'OCA\\Settings\\SetupChecks\\ReadOnlyConfig' => $baseDir . '/../lib/SetupChecks/ReadOnlyConfig.php',
'OCA\\Settings\\SetupChecks\\SupportedDatabase' => $baseDir . '/../lib/SetupChecks/SupportedDatabase.php',
'OCA\\Settings\\SetupChecks\\TransactionIsolation' => $baseDir . '/../lib/SetupChecks/TransactionIsolation.php',
Expand Down
1 change: 1 addition & 0 deletions apps/settings/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ class ComposerStaticInitSettings
'OCA\\Settings\\SetupChecks\\PhpModules' => __DIR__ . '/..' . '/../lib/SetupChecks/PhpModules.php',
'OCA\\Settings\\SetupChecks\\PhpOutdated' => __DIR__ . '/..' . '/../lib/SetupChecks/PhpOutdated.php',
'OCA\\Settings\\SetupChecks\\PhpOutputBuffering' => __DIR__ . '/..' . '/../lib/SetupChecks/PhpOutputBuffering.php',
'OCA\\Settings\\SetupChecks\\RandomnessSecure' => __DIR__ . '/..' . '/../lib/SetupChecks/RandomnessSecure.php',
'OCA\\Settings\\SetupChecks\\ReadOnlyConfig' => __DIR__ . '/..' . '/../lib/SetupChecks/ReadOnlyConfig.php',
'OCA\\Settings\\SetupChecks\\SupportedDatabase' => __DIR__ . '/..' . '/../lib/SetupChecks/SupportedDatabase.php',
'OCA\\Settings\\SetupChecks\\TransactionIsolation' => __DIR__ . '/..' . '/../lib/SetupChecks/TransactionIsolation.php',
Expand Down
2 changes: 2 additions & 0 deletions apps/settings/lib/AppInfo/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
use OCA\Settings\SetupChecks\PhpGetEnv;
use OCA\Settings\SetupChecks\PhpOutdated;
use OCA\Settings\SetupChecks\PhpOutputBuffering;
use OCA\Settings\SetupChecks\RandomnessSecure;
use OCA\Settings\SetupChecks\ReadOnlyConfig;
use OCA\Settings\SetupChecks\SupportedDatabase;
use OCA\Settings\SetupChecks\TransactionIsolation;
Expand Down Expand Up @@ -164,6 +165,7 @@ public function register(IRegistrationContext $context): void {
$context->registerSetupCheck(PhpGetEnv::class);
$context->registerSetupCheck(PhpOutdated::class);
$context->registerSetupCheck(PhpOutputBuffering::class);
$context->registerSetupCheck(RandomnessSecure::class);
$context->registerSetupCheck(ReadOnlyConfig::class);
$context->registerSetupCheck(SupportedDatabase::class);
$context->registerSetupCheck(TransactionIsolation::class);
Expand Down
21 changes: 0 additions & 21 deletions apps/settings/lib/Controller/CheckSetupController.php
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@
use OCP\Lock\ILockingProvider;
use OCP\Notification\IManager;
use OCP\Security\Bruteforce\IThrottler;
use OCP\Security\ISecureRandom;
use OCP\SetupCheck\ISetupCheckManager;
use Psr\Log\LoggerInterface;

Expand Down Expand Up @@ -108,8 +107,6 @@ class CheckSetupController extends Controller {
private $dateTimeFormatter;
/** @var MemoryInfo */
private $memoryInfo;
/** @var ISecureRandom */
private $secureRandom;
/** @var IniGetWrapper */
private $iniGetWrapper;
/** @var IDBConnection */
Expand Down Expand Up @@ -139,7 +136,6 @@ public function __construct($AppName,
ILockingProvider $lockingProvider,
IDateTimeFormatter $dateTimeFormatter,
MemoryInfo $memoryInfo,
ISecureRandom $secureRandom,
IniGetWrapper $iniGetWrapper,
IDBConnection $connection,
IThrottler $throttler,
Expand All @@ -162,7 +158,6 @@ public function __construct($AppName,
$this->lockingProvider = $lockingProvider;
$this->dateTimeFormatter = $dateTimeFormatter;
$this->memoryInfo = $memoryInfo;
$this->secureRandom = $secureRandom;
$this->iniGetWrapper = $iniGetWrapper;
$this->connection = $connection;
$this->tempManager = $tempManager;
Expand Down Expand Up @@ -194,20 +189,6 @@ private function isFairUseOfFreePushService(): bool {
return $this->manager->isFairUseOfFreePushService();
}

/**
* Whether PHP can generate "secure" pseudorandom integers
*
* @return bool
*/
private function isRandomnessSecure() {
try {
$this->secureRandom->generate(1);
} catch (\Exception $ex) {
return false;
}
return true;
}

/**
* Public for the sake of unit-testing
*
Expand Down Expand Up @@ -767,8 +748,6 @@ public function check() {
'isFairUseOfFreePushService' => $this->isFairUseOfFreePushService(),
'isBruteforceThrottled' => $this->throttler->getAttempts($this->request->getRemoteAddress()) !== 0,
'bruteforceRemoteAddress' => $this->request->getRemoteAddress(),
'isRandomnessSecure' => $this->isRandomnessSecure(),
'securityDocs' => $this->urlGenerator->linkToDocs('admin-security'),
'isUsedTlsLibOutdated' => $this->isUsedTlsLibOutdated(),
'forwardedForHeadersWorking' => $this->forwardedForHeadersWorking(),
'reverseProxyDocs' => $this->urlGenerator->linkToDocs('admin-reverse-proxy'),
Expand Down
63 changes: 63 additions & 0 deletions apps/settings/lib/SetupChecks/RandomnessSecure.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2023 Côme Chilliet <come.chilliet@nextcloud.com>
*
* @author Côme Chilliet <come.chilliet@nextcloud.com>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/
namespace OCA\Settings\SetupChecks;

use OCP\IConfig;
use OCP\IL10N;
use OCP\IURLGenerator;
use OCP\Security\ISecureRandom;
use OCP\SetupCheck\ISetupCheck;
use OCP\SetupCheck\SetupResult;

class RandomnessSecure implements ISetupCheck {
public function __construct(
private IL10N $l10n,
private IConfig $config,
private IURLGenerator $urlGenerator,
private ISecureRandom $secureRandom,
) {
}

public function getName(): string {
return $this->l10n->t('Random generator');
}

public function getCategory(): string {
return 'security';
}

public function run(): SetupResult {
try {
$this->secureRandom->generate(1);
} catch (\Exception $ex) {
return SetupResult::error(
$this->l10n->t('No suitable source for randomness found by PHP which is highly discouraged for security reasons.'),
$this->urlGenerator->linkToDocs('admin-security')
);
}
return SetupResult::success($this->l10n->t('Secure'));
}
}
2 changes: 0 additions & 2 deletions apps/settings/tests/Controller/CheckSetupControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -443,8 +443,6 @@ public function testCheck() {
'backgroundJobsUrl' => 'https://example.org',
],
'cronErrors' => [],
'isRandomnessSecure' => self::invokePrivate($this->checkSetupController, 'isRandomnessSecure'),
'securityDocs' => 'https://docs.example.org/server/8.1/admin_manual/configuration_server/hardening.html',
'isUsedTlsLibOutdated' => '',
'forwardedForHeadersWorking' => false,
'reverseProxyDocs' => 'reverse-proxy-doc-link',
Expand Down
8 changes: 0 additions & 8 deletions core/js/setupchecks.js
Original file line number Diff line number Diff line change
Expand Up @@ -224,14 +224,6 @@
type: OC.SetupChecks.MESSAGE_TYPE_ERROR
});
}
if(!data.isRandomnessSecure) {
messages.push({
msg: t('core', 'No suitable source for randomness found by PHP which is highly discouraged for security reasons. Further information can be found in the {linkstart}documentation ↗{linkend}.')
.replace('{linkstart}', '<a target="_blank" rel="noreferrer noopener" class="external" href="' + data.securityDocs + '">')
.replace('{linkend}', '</a>'),
type: OC.SetupChecks.MESSAGE_TYPE_ERROR
});
}
if(data.isUsedTlsLibOutdated) {
messages.push({
msg: data.isUsedTlsLibOutdated,
Expand Down
Loading

0 comments on commit 903462a

Please sign in to comment.