From: Anthony Date: Thu, 24 Mar 2022 04:29:44 +0000 (+0800) Subject: dev/core#3133 - Extend Authx functionality to support validation of externally genera... X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=292527ef2d0ab37e18c989ff8e0ae59a363a95d0;p=civicrm-core.git dev/core#3133 - Extend Authx functionality to support validation of externally generated JWTs Migrate legacy rest to appropriate place Remove legacy auth as not needed due to it self registering now --- diff --git a/ext/authx/Civi/Authx/Authenticator.php b/ext/authx/Civi/Authx/Authenticator.php index b9c7d8a3b3..d38bc985dd 100644 --- a/ext/authx/Civi/Authx/Authenticator.php +++ b/ext/authx/Civi/Authx/Authenticator.php @@ -14,7 +14,6 @@ namespace Civi\Authx; use Civi\Core\Event\GenericHookEvent; use Civi\Core\HookInterface; use Civi\Core\Service\AutoService; -use Civi\Crypto\Exception\CryptoException; use GuzzleHttp\Psr7\Response; /** @@ -154,44 +153,18 @@ class Authenticator extends AutoService implements HookInterface { * @see \Civi\Authx\AuthenticatorTarget::setPrincipal() */ protected function checkCredential($tgt) { - [$credFmt, $credValue] = explode(' ', $tgt->cred, 2); - - switch ($credFmt) { - case 'Basic': - [$user, $pass] = explode(':', base64_decode($credValue), 2); - if ($userId = $this->authxUf->checkPassword($user, $pass)) { - return ['userId' => $userId, 'credType' => 'pass']; - } - break; - - case 'Bearer': - $c = \CRM_Core_DAO::singleValueQuery('SELECT id FROM civicrm_contact WHERE api_key = %1', [ - 1 => [$credValue, 'String'], - ]); - if ($c) { - return ['contactId' => $c, 'credType' => 'api_key']; - } - - try { - $claims = \Civi::service('crypto.jwt')->decode($credValue); - $scopes = isset($claims['scope']) ? explode(' ', $claims['scope']) : []; - if (!in_array('authx', $scopes)) { - $this->reject('JWT does not permit general authentication'); - } - if (empty($claims['sub']) || substr($claims['sub'], 0, 4) !== 'cid:') { - $this->reject('JWT does not specify the contact ID (sub)'); - } - $contactId = substr($claims['sub'], 4); - return ['contactId' => $contactId, 'credType' => 'jwt', 'jwt' => $claims]; - } - catch (CryptoException $e) { - // Invalid JWT. Proceed to check any other token sources. - } - - break; + // In order of priority, each subscriber will either: + // 1. Accept the cred, which stops event propagation and further checks; + // 2. Reject the cred, which stops event propagation and further checks; + // 3. Neither accept nor reject, letting the event continue on to the next. + $checkEvent = new CheckCredentialEvent($tgt->cred); + \Civi::dispatcher()->dispatch('civi.authx.checkCredential', $checkEvent); + + if ($checkEvent->getRejection()) { + $this->reject($checkEvent->getRejection()); } - return NULL; + return $checkEvent->getPrincipal(); } /** diff --git a/ext/authx/Civi/Authx/CheckCredential.php b/ext/authx/Civi/Authx/CheckCredential.php new file mode 100644 index 0000000000..61368f84e3 --- /dev/null +++ b/ext/authx/Civi/Authx/CheckCredential.php @@ -0,0 +1,118 @@ +credFormat === 'Basic') { + [$user, $pass] = explode(':', base64_decode($check->credValue), 2); + if ($userId = _authx_uf()->checkPassword($user, $pass)) { + $check->accept(['userId' => $userId, 'credType' => 'pass']); + } + } + } + + /** + * Interpret the HTTP `Bearer` credential as a traditional Civi API key + * (`civicrm_contact.api_key`). + * + * @param \Civi\Authx\CheckCredentialEvent $check + */ + public function bearerApiKey(CheckCredentialEvent $check): void { + if ($check->credFormat === 'Bearer') { + $c = \CRM_Core_DAO::singleValueQuery('SELECT id FROM civicrm_contact WHERE api_key = %1', [ + 1 => [$check->credValue, 'String'], + ]); + if ($c) { + $check->accept(['contactId' => $c, 'credType' => 'api_key']); + } + } + } + + /** + * Interpret the HTTP `Bearer` credential as an Authx-style JSON Web Token. + * + * @param \Civi\Authx\CheckCredentialEvent $check + */ + public function bearerJwt(CheckCredentialEvent $check): void { + if ($check->credFormat === 'Bearer') { + try { + $claims = \Civi::service('crypto.jwt')->decode($check->credValue); + $scopes = isset($claims['scope']) ? explode(' ', $claims['scope']) : []; + if (!in_array('authx', $scopes)) { + // This is not an authx JWT. Proceed to check any other token sources. + return; + } + if (empty($claims['sub']) || substr($claims['sub'], 0, 4) !== 'cid:') { + $check->reject('Malformed JWT. Must specify the contact ID.'); + } + else { + $contactId = substr($claims['sub'], 4); + $check->accept(['contactId' => $contactId, 'credType' => 'jwt', 'jwt' => $claims]); + } + } + catch (CryptoException $e) { + // TODO: Is responding that its expired a security risk? + if (strpos($e->getMessage(), 'Expired token') !== FALSE) { + $check->reject('Expired token'); + } + + // Not a valid AuthX JWT. Proceed to check any other token sources. + } + } + } + +} diff --git a/ext/authx/Civi/Authx/CheckCredentialEvent.php b/ext/authx/Civi/Authx/CheckCredentialEvent.php new file mode 100644 index 0000000000..87379f7e81 --- /dev/null +++ b/ext/authx/Civi/Authx/CheckCredentialEvent.php @@ -0,0 +1,126 @@ +credFormat, $this->credValue] = explode(' ', $cred, 2); + } + + /** + * Emphatically reject the credential. + * + * If you know that the credential is targeted at your provider, and if there + * is an error in it, then you may set a rejection message. This will can + * provide more detailed debug information. However, it will preclude other + * listeners from accepting the credential. + * + * @param string $message + */ + public function reject(string $message): void { + $this->rejection = $message; + } + + /** + * Accept the sub claim, matching the credentials to a specific user by + * civicrm contact id ('contactId'), CRM user id ('userId') or CRM username + * ('user'). This will cause authx to log in that user for the purposes of the + * current request. + * + * The $principal must a mix of of 'user', 'userId', 'contactId' and + * 'credType': + * + * - 'credType': (string) type of credential used to identify the principal. + * ('pass', 'api_key', 'jwt') + * + * - 'contactId': (Authenticated) CiviCRM contact ID. If not specified, will + * be obtained from 'userId'. + * + * - 'userId': (Authenticated) UF user ID. If not specified, will be obtained + * from 'user' or 'contactId'. + * + * - 'user': (string). The username of the CMS user. Can be used instead of + * 'userId'. + * + * - 'jwt': (Authenticated, Decoded) JWT claims (if applicable) + * + * Note: Event propogation will stop after this, so subscribers with lower + * priorities will not be able to reject it. + * + * @param array $principal Must include credType and (contactId or (userId + * xor user)) + * + */ + public function accept(array $principal): void { + if (empty($principal['credType'])) { + throw new AuthxException("Principal must specify credType"); + } + + if (empty($principal['contactId']) && empty($principal['userId']) && empty($principal['user'])) { + throw new AuthxException("Principal must specify at least one of contactId, userId or user"); + } + + if (!empty($principal['userId']) && !empty($principal['user'])) { + throw new AuthxException("Only userId or user can be specified in principal, not both"); + } + + $this->principal = $principal; + $this->stopPropagation(); + } + + public function getPrincipal(): ?array { + return $this->principal; + } + + public function getRejection(): ?string { + return $this->rejection; + } + +} diff --git a/ext/authx/README.md b/ext/authx/README.md index d5404a88bf..d355388080 100644 --- a/ext/authx/README.md +++ b/ext/authx/README.md @@ -34,6 +34,8 @@ CiviCRM `Contact` records are often linked to CMS `User` records -- but not alwa * __Optional__: If there is a correlated CMS `User`, then load it. If there isn't, leave the CMS user as anonymous. * __Require__: Only allow authentication if proceed if there is a correlated user account. +Handling of these credentials, or new ones can be modified by custom extensions that subscribe to the `civi.authx.checkCredential` event. See [Extending credential handling](#extending-credential-handling). + ## Configuration For each authentication flow, one may toggle support for different credentials and user-links. Here is the default configuration: @@ -127,3 +129,83 @@ $ curl 'https://demouser:demopass@example.org/civicrm/authx/id' The "AuthX: Authenticate to services with password" CiviCRM permission must also be granted for the role associated to the user. + +## Extending credential handling + +To determine if a set of credentials is accepted or rejected, authx dispatches a +`civi.authx.checkCredential` symfony event of class `Civi\Authx\CheckCredentialEvent`. + +Once the event has been processed by any subscribers, the event's state is examined to determine if +it has been rejected. If not, the principal containing user identification is +extracted and the request is processed as that user. + +Authx its has 3 built-in subscribers to this events to handle. One to handle Basic `pass` +credentials, one for Bearer `jwt` credentials and one for Bearer `api_key` credentials. These have +priorties of -200, -300 and -400 respectively. Each checks to see if the credentials are relevant to +its processing capabilities and if so either calls `accept()` or `reject()`; irrelevant events are +just ignored. Once the event has been accepted or rejected, it will no longer propogate to +subscribers with a lower priority. + +This behaviour can be replicated in a separate extension subscribing to the +`civi.authx.checkCredential` event in the same manner. If a higher priority is used, then it can +also be used to override authx's built-in credential handling by always accepting or rejecting the +event. + +### Example + +Say you wanted to handle JWT credentials in a different way, to handle tokens signed by an external +external provider, rather than only tokens generated by your civicrm install. Assuming you have used +[Civix](https://docs.civicrm.org/dev/en/latest/extensions/civix/) to generate the boilerplate for +a new extension called "myextension", you would add the following hook to `myextension.php`: + + +```php +function myextension_civicrm_container(\Symfony\Component\DependencyInjection\ContainerBuilder $container) { + $container->register('myextension_credentials', '\Civi\MyExtension\MyCheckCredential') + ->addTag('kernel.event_subscriber') + ->setPublic(TRUE); +} +``` + +Lets say the identifier for the user is stored in the `sub` claim of the JWT. Your +`MyCheckCredential` could then be defined along the lines of: + +```php +class MyCheckCredential implements Symfony\Component\EventDispatcher\EventSubscriberInterface { + const PRIORITY_BEARER_SPECIAL_JWT = 200; + + public static function getSubscribedEvents(): array { + $events = []; + $events['civi.authx.checkCredential'][] = ['bearerSpecialJwt', self::PRIORITY_BEARER_SPECIAL_JWT]; + return $events; + } + + public function bearerSpecialJwt(Civi\Authx\CheckCredentialEvent $checkEvent): void { + if ($checkEvent->credFormat === 'Bearer') { + try { + $claims = \Civi::service('crypto.jwt')->decode($checkEvent->credValue); + + // Perhaps we require the scope claim to contain something specific + $scopes = isset($claims['scope']) ? explode(' ', $claims['scope']) : []; + if (!in_array('somespecialthing', $scopes)) { + // Not our responsibility. Proceed to check any other token sources. + return; + } + + // Maybe a table links external ids to user ids, or they are encoded in the sub somehow + $userId = someFunctionToGetCmsUserId($claims['sub']) + + if ($userId) { + $checkEvent->accept(['userId' => $userId, 'credType' => 'jwt', 'jwt' => $claims]); + return; + } else { + // Alternatively, could return, so other token sources can be checked + $checkEvent->reject('User not found'); + } + } catch (Civi\Crypto\Exception\CryptoException $e) { + // Not a valid JWT. Proceed to check any other token sources. + } + } + } +} +``` diff --git a/ext/authx/authx.php b/ext/authx/authx.php index 432afe1d89..c3ebc11746 100644 --- a/ext/authx/authx.php +++ b/ext/authx/authx.php @@ -5,6 +5,13 @@ require_once 'authx.civix.php'; use CRM_Authx_ExtensionUtil as E; // phpcs:enable + +function authx_civicrm_container(\Symfony\Component\DependencyInjection\ContainerBuilder $container) { + $container->register('authx_credentials', '\Civi\Authx\CheckCredential') + ->addTag('kernel.event_subscriber') + ->setPublic(TRUE); +} + /** * Perform a system login. * diff --git a/ext/authx/tests/phpunit/Civi/Authx/AllFlowsTest.php b/ext/authx/tests/phpunit/Civi/Authx/AllFlowsTest.php index e4972fe16f..0afa38b433 100644 --- a/ext/authx/tests/phpunit/Civi/Authx/AllFlowsTest.php +++ b/ext/authx/tests/phpunit/Civi/Authx/AllFlowsTest.php @@ -95,6 +95,14 @@ class AllFlowsTest extends \PHPUnit\Framework\TestCase implements EndToEndInterf return $exs; } + public function getFlowTypes() { + $exs = []; + $exs[] = ['param']; + $exs[] = ['header']; + $exs[] = ['xheader']; + return $exs; + } + public function testAnonymous(): void { $http = $this->createGuzzle(['http_errors' => FALSE]); @@ -170,6 +178,49 @@ class AllFlowsTest extends \PHPUnit\Framework\TestCase implements EndToEndInterf } } + /** + * Send a request using a jwt that can't be decoded at all. Assert that it fails + * + * @param string $flowType + * The "flow" determines how the credential is added on top of the base-request (e.g. adding a parameter or header). + * + * @dataProvider getFlowTypes + */ + public function testInvalidJwt($flowType): void { + $http = $this->createGuzzle(['http_errors' => FALSE]); + + $cred = $this->credJwt('Bearer thisisnotavalidjwt'); + + $flowFunc = 'auth' . ucfirst(preg_replace(';[^a-zA-Z0-9];', '', $flowType)); + /** @var \Psr\Http\Message\RequestInterface $request */ + $request = $this->$flowFunc($this->requestMyContact(), $cred); + + \Civi::settings()->set("authx_{$flowType}_cred", ['jwt']); + $response = $http->send($request); + $this->assertNotAuthenticated('prohibit', $response); + } + + /** + * Send a request using a jwt that has expired. Assert that it fails + * + * @param string $flowType + * The "flow" determines how the credential is added on top of the base-request (e.g. adding a parameter or header). + * + * @dataProvider getFlowTypes + */ + public function testExpiredJwt($flowType): void { + $http = $this->createGuzzle(['http_errors' => FALSE]); + + $cred = $this->credJwt($this->getDemoCID(), TRUE); + $flowFunc = 'auth' . ucfirst(preg_replace(';[^a-zA-Z0-9];', '', $flowType)); + /** @var \Psr\Http\Message\RequestInterface $request */ + $request = $this->$flowFunc($this->requestMyContact(), $cred); + + \Civi::settings()->set("authx_{$flowType}_cred", ['jwt']); + $response = $http->send($request); + $this->assertNotAuthenticated('prohibit', $response); + } + /** * The setting "authx_guard" may be used to require (or not require) the site_key. * @@ -780,12 +831,12 @@ class AllFlowsTest extends \PHPUnit\Framework\TestCase implements EndToEndInterf return 'Bearer ' . $api_key; } - public function credJwt($cid) { + public function credJwt($cid, $expired = FALSE) { if (empty(\Civi::service('crypto.registry')->findKeysByTag('SIGN'))) { $this->markTestIncomplete('Cannot test JWT. No CIVICRM_SIGN_KEYS are defined.'); } $token = \Civi::service('crypto.jwt')->encode([ - 'exp' => time() + 60 * 60, + 'exp' => $expired ? time() - 60 * 60 : time() + 60 * 60, 'sub' => "cid:$cid", 'scope' => 'authx', ]);