Merge pull request #194 from creative-commoners/pulls/4.0/password-en…

…cryptor

API Remove BackupCodeGeneratorInterface::hash() and allow generate() to handle its hashing internally

docs/en/backup-codes.md

@@ -0,0 +1,76 @@
+# Backup codes
+
+Backup codes are random strings that are provided as recovery options in case a member loses their ability to
+verify with their registered multi-factor authentication methods, e.g. they lost their phone or security key.
+
+## Generating backup codes
+
+Backup codes are generated by the `BackupCodeGeneratorInterface` service, which has a default implementation of
+`SilverStripe\MFA\Service\BackupCodeGenerator`. This service will hash the generated backup codes using the
+default SilverStripe `PasswordEncryptor` service and algorithm, and returns an array of
+`SilverStripe\MFA\State\BackupCode` instances.
+
+You can use this service class to generate codes:
+
+```php
+$generator = Injector::inst()->get(BackupCodeGeneratorInterface::class);
+$codes = $generator->generate();
+```
+
+## Verifying backup codes
+
+To verify a backup code, you can reconstruct a `BackupCode` instance from stored data on the `RegisteredMethod`
+instance and use `BackupCode::isValid()` to verify the input. For example:
+
+```php
+$storedCodeData = json_decode($registeredMethod->Data, true);
+foreach ($storedCodeData as $codeCandidate) {
+    // Expected structure: ['hash' => 'abc123', 'algorithm' => 'blowfish', 'salt' => 'bae']
+    $candidate = json_decode($codeCandidate, true);
+
+    $backupCode = BackupCode::create($userInputCode, $candidate['hash'], $candidate['algorithm'], $candidate['salt']);
+    if ($backupCode->isValid()) {
+        // The user entered a valid backup code, do as you please now.
+    }
+}
+```
+
+The default implementation of this logic is in `VerifyHandler::verify()`.
+
+## BackupCodeGenerator
+
+This configuration documentation applies to the default `BackupCodeGenerator` implementation of
+`BackupCodeGeneratorInterface`.
+
+### Configuration
+
+You can adjust either the length of the backup code strings, or the number of them that are generated, by setting
+YAML configuration:
+
+```yaml
+SilverStripe\MFA\Service\BackupCodeGenerator:
+  # Should be 12 characters long
+  backup_code_length: 12
+  # I want 25 of them
+  backup_code_count: 25
+```
+
+### Adjusting the character sets
+
+By default, backup codes will be generated using upper and lowercase letters and numbers. If you would like to increase
+the entropy of your backup codes by adding special characters, you can do so by adding an extension:
+
+```php
+class BackupCodeGeneratorExtension extends Extension
+{
+    /**
+     * Add some special characters into the character set for generating backup codes
+     */
+    public function updateCharacterSet(&$characterSet)
+    {
+        $characterSet = array_merge($characterSet, ['!', '@', '#', '$', '%', '^']);
+    }
+}
+```
+
+This extension should be applied to `SilverStripe\MFA\Service\BackupCodeGenerator` via YAML configuration.

src/BackupCode/RegisterHandler.php

@@ -53,7 +53,7 @@ public function start(StoreInterface $store): array
             $store->getMember(),
             $method,
             array_map(function (BackupCode $backupCode) {
-                return $backupCode->getHash();
+                return json_encode($backupCode);
             }, $codes)
         );
 

src/BackupCode/VerifyHandler.php

@@ -8,6 +8,7 @@
 use SilverStripe\MFA\Method\Handler\VerifyHandlerInterface;
 use SilverStripe\MFA\Model\RegisteredMethod;
 use SilverStripe\MFA\Service\Notification;
+use SilverStripe\MFA\State\BackupCode;
 use SilverStripe\MFA\State\Result;
 use SilverStripe\MFA\Store\StoreInterface;
 
@@ -69,21 +70,31 @@ public function verify(HTTPRequest $request, StoreInterface $store, RegisteredMe
         $candidates = json_decode($registeredMethod->Data, true);
 
         foreach ($candidates as $index => $candidate) {
-            if ($this->verifyCode($code, $candidate)) {
-                // Remove the verified code from the valid list of codes
-                array_splice($candidates, $index, 1);
-                $registeredMethod->Data = json_encode($candidates);
-                $registeredMethod->write();
-                $this->notification->send(
-                    $registeredMethod->Member(),
-                    'SilverStripe/MFA/Email/Notification_backupcodeused',
-                    [
-                        'subject' => _t(self::class . '.MFAREMOVED', 'A recovery code was used to access your account'),
-                        'CodesRemaining' => count($candidates),
-                    ]
-                );
-                return Result::create();
+            $candidateData = json_decode($candidate, true) ?? [];
+            $backupCode = BackupCode::create(
+                $code,
+                $candidateData['hash'] ?? '',
+                $candidateData['algorithm'] ?? '',
+                $candidateData['salt'] ?? ''
+            );
+            if (!$backupCode->isValid()) {
+                continue;
             }
+
+
+            // Remove the verified code from the valid list of codes
+            array_splice($candidates, $index, 1);
+            $registeredMethod->Data = json_encode($candidates);
+            $registeredMethod->write();
+            $this->notification->send(
+                $registeredMethod->Member(),
+                'SilverStripe/MFA/Email/Notification_backupcodeused',
+                [
+                    'subject' => _t(self::class . '.MFAREMOVED', 'A recovery code was used to access your account'),
+                    'CodesRemaining' => count($candidates),
+                ]
+            );
+            return Result::create();
         }
 
         return Result::create(false, _t(__CLASS__ . '.INVALID_CODE', 'Invalid code'));
@@ -101,19 +112,6 @@ public function getLeadInLabel(): string
         return _t(__CLASS__ . '.LEAD_IN', 'Verify with recovery code');
     }
 
-    /**
-     * Verifies the given code (user input) against the given hash. This uses the PHP password_hash API by default but
-     * can be extended to handle a custom hash implementation
-     *
-     * @param string $code
-     * @param string $hash
-     * @return bool
-     */
-    protected function verifyCode($code, $hash): bool
-    {
-        return password_verify($code, $hash);
-    }
-
     /**
      * Get the key that a React UI component is registered under (with @silverstripe/react-injector on the front-end)
      *

src/Service/BackupCodeGenerator.php

@@ -5,8 +5,8 @@
 use SilverStripe\Core\Config\Configurable;
 use SilverStripe\Core\Extensible;
 use SilverStripe\Core\Injector\Injectable;
-use SilverStripe\MFA\Exception\HashFailedException;
 use SilverStripe\MFA\State\BackupCode;
+use SilverStripe\Security\Security;
 
 class BackupCodeGenerator implements BackupCodeGeneratorInterface
 {
@@ -46,33 +46,16 @@ public function generate(): array
         $codes = [];
         while (count($codes) < $codeCount) {
             $code = $this->generateCode($charset, $codeLength);
+
             if (!in_array($code, $codes)) {
-                $codes[] = BackupCode::create($code, $this->hash($code));
+                $hashData = Security::encrypt_password($code);
+                $codes[] = BackupCode::create($code, $hashData['password'], $hashData['algorithm'], $hashData['salt']);
             }
         }
 
         return $codes;
     }
 
-    /**
-     * Hash a back-up code for storage. This uses the native PHP password_hash API by default, but can be extended to
-     * implement a custom hash requirement callback.
-     *
-     * {@inheritDoc}
-     */
-    public function hash(string $code): string
-    {
-        $hash = (string) password_hash($code, PASSWORD_DEFAULT);
-
-        $this->extend('updateHash', $code, $hash);
-
-        if ($hash === $code) {
-            throw new HashFailedException('Hash must not equal the plaintext code!');
-        }
-
-        return $hash;
-    }
-
     public function getCharacterSet(): array
     {
         $characterSet = array_merge(

src/Service/BackupCodeGeneratorInterface.php

@@ -2,7 +2,6 @@
 
 namespace SilverStripe\MFA\Service;
 
-use SilverStripe\MFA\Exception\HashFailedException;
 use SilverStripe\MFA\State\BackupCode;
 
 /**
@@ -17,16 +16,6 @@ interface BackupCodeGeneratorInterface
      */
     public function generate(): array;
 
-    /**
-     * Hash the given backup code for storage. May throw an exception if hashing fails, or if the hash
-     * is the same as the input plaintext code.
-     *
-     * @param string $code
-     * @return string
-     * @throws HashFailedException
-     */
-    public function hash(string $code): string;
-
     /**
      * Returns a list of possible characters to use in backup codes.
      *

src/State/BackupCode.php

@@ -2,12 +2,15 @@
 
 namespace SilverStripe\MFA\State;
 
+use JsonSerializable;
 use SilverStripe\Core\Injector\Injectable;
+use SilverStripe\Security\PasswordEncryptor;
+use SilverStripe\Security\PasswordEncryptor_NotFoundException;
 
 /**
  * A container for a backup code and its hash, normally used during backup code generation
  */
-class BackupCode
+class BackupCode implements JsonSerializable
 {
     use Injectable;
 
@@ -21,14 +24,26 @@ class BackupCode
      */
     protected $hash = '';
 
+    /**
+     * @var string
+     */
+    protected $algorithm = '';
+
+    /**
+     * @var string
+     */
+    protected $salt = '';
+
     /**
      * @param string $code
      * @param string $hash
      */
-    public function __construct(string $code, string $hash)
+    public function __construct(string $code, string $hash, string $algorithm, string $salt)
     {
         $this->code = $code;
         $this->hash = $hash;
+        $this->algorithm = $algorithm;
+        $this->salt = $salt;
     }
 
     public function getCode(): string
@@ -40,4 +55,38 @@ public function getHash(): string
     {
         return $this->hash;
     }
+
+    public function getAlgorithm(): string
+    {
+        return $this->algorithm;
+    }
+
+    public function getSalt(): string
+    {
+        return $this->salt;
+    }
+
+    /**
+     * Checks whether the provided code matches a set of provided salt, hash, and algorithm, using the
+     * internal PasswordEncryptor API.
+     *
+     * @return bool
+     */
+    public function isValid(): bool
+    {
+        return PasswordEncryptor::create_for_algorithm($this->getAlgorithm())
+            ->check($this->getHash(), $this->getCode(), $this->getSalt());
+    }
+
+    /**
+     * Note: deliberately does not include "code", as this is the data that is stored in DB records
+     */
+    public function jsonSerialize(): array
+    {
+        return [
+            'hash' => $this->getHash(),
+            'algorithm' => $this->getAlgorithm(),
+            'salt' => $this->getSalt(),
+        ];
+    }
 }

tests/php/BackupCode/VerifyHandlerTest.php

@@ -3,7 +3,6 @@
 namespace SilverStripe\MFA\Tests\BackupCode;
 
 use PHPUnit_Framework_MockObject_MockObject;
-use SilverStripe\Control\Email\Email;
 use SilverStripe\Control\HTTPRequest;
 use SilverStripe\Dev\SapphireTest;
 use SilverStripe\MFA\BackupCode\VerifyHandler;

tests/php/BackupCode/VerifyHandlerTest.yml

@@ -2,7 +2,7 @@ SilverStripe\MFA\Model\RegisteredMethod:
   codes:
     MethodClassName: "SilverStripe\\MFA\\BackupCode\\Method"
     # 123456, 654321, 111111, 222222
-    Data: "[\"$2y$10$AxMi8GFGNFOZavmYVzO6ee5jH45UTvDQt0fGkk726v/HZK0wh1Kp.\",\"$2y$10$uEdK6/Otu8PY1LjQlnWGw.soDjgIhCz6vHwry.kSXLLQMRxCLR6Ii\",\"$2y$10$6b6wlbCHjNjpfemAAYnHGunPMgUBE8AHX7CjEB5yCf5NdXrNjOEN2\",\"$2y$10$0uVdYy9zgkA5bdH9b.F5ieX9A5wIFun.HWUqoU/oFNVia7kTB65VG\"]"
+    Data: '["{\"hash\":\"$2y$10$0e51c5eacb30a53f0c055uo31pASfJgWETrw8wn8hnsOT2.1ZkyAC\",\"algorithm\":\"blowfish\",\"salt\":\"10$0e51c5eacb30a53f0c0557\"}","{\"hash\":\"$2y$10$630af2e0e9c4eca2f4917e2iCJpbPUqSWLc.SNRgB7EriXbJbF5Lm\",\"algorithm\":\"blowfish\",\"salt\":\"10$630af2e0e9c4eca2f4917e\"}","{\"hash\":\"$2y$10$35138190124b6ed1948afuQf931YtHM.DRB2gGt\\\/eySPVlpVUVoRW\",\"algorithm\":\"blowfish\",\"salt\":\"10$35138190124b6ed1948af0\"}","{\"hash\":\"$2y$10$8876ae5c66ecc90742637uiA2QhTLZx1AI1SWi8e.rP02ISJBRs.m\",\"algorithm\":\"blowfish\",\"salt\":\"10$8876ae5c66ecc907426372\"}"]'
 
 SilverStripe\Security\Member:
   guy: