From c753aad9e37791f049a88780ebe87505de46fd48 Mon Sep 17 00:00:00 2001 From: Micke Nordin Date: Sun, 17 May 2026 18:41:31 +0200 Subject: [PATCH] refactor(ocm): expose confirmRequestOrigin as a function on ocmDiscoveryService Apps implementing OCM endpoints via OCMEndpointRequestEvent (e.g. SUNET/nextcloud-ocm_request_share for request-share, nextcloud/contacts for invite-accepted) need to apply the same identity check that the built-in addShare and receiveNotification handlers apply, so it makes sense to make it publicly accessible. It also allows us to refactor RequestHandlerController::confirmSignedOrigin to use the new public method and drop the confirmNotificationIdentity helper. Signed-off-by: Micke Nordin --- .../Controller/RequestHandlerController.php | 72 +++---------------- .../tests/RequestHandlerControllerTest.php | 8 --- lib/private/OCM/OCMDiscoveryService.php | 45 ++++++++++++ lib/public/OCM/IOCMDiscoveryService.php | 15 ++++ 4 files changed, 69 insertions(+), 71 deletions(-) diff --git a/apps/cloud_federation_api/lib/Controller/RequestHandlerController.php b/apps/cloud_federation_api/lib/Controller/RequestHandlerController.php index 06480df1ad5..d2df7da8e92 100644 --- a/apps/cloud_federation_api/lib/Controller/RequestHandlerController.php +++ b/apps/cloud_federation_api/lib/Controller/RequestHandlerController.php @@ -12,7 +12,6 @@ use OCA\CloudFederationAPI\Config; use OCA\CloudFederationAPI\Db\FederatedInviteMapper; use OCA\CloudFederationAPI\Events\FederatedInviteAcceptedEvent; use OCA\CloudFederationAPI\ResponseDefinitions; -use OCA\FederatedFileSharing\AddressHandler; use OCP\AppFramework\Controller; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Http; @@ -38,11 +37,8 @@ use OCP\IRequest; use OCP\IURLGenerator; use OCP\IUserManager; use OCP\OCM\IOCMDiscoveryService; -use OCP\Security\Signature\Exceptions\IdentityNotFoundException; use OCP\Security\Signature\Exceptions\IncomingRequestException; -use OCP\Security\Signature\Exceptions\SignatoryNotFoundException; use OCP\Security\Signature\IIncomingSignedRequest; -use OCP\Security\Signature\ISignatureManager; use OCP\Share\Exceptions\ShareNotFound; use OCP\Util; use Psr\Log\LoggerInterface; @@ -69,12 +65,10 @@ class RequestHandlerController extends Controller { private Config $config, private IEventDispatcher $dispatcher, private FederatedInviteMapper $federatedInviteMapper, - private readonly AddressHandler $addressHandler, private readonly IAppConfig $appConfig, private ICloudFederationFactory $factory, private ICloudIdManager $cloudIdManager, private readonly IOCMDiscoveryService $ocmDiscoveryService, - private readonly ISignatureManager $signatureManager, private ITimeFactory $timeFactory, ) { parent::__construct($appName, $request); @@ -440,6 +434,8 @@ class RequestHandlerController extends Controller { * If request is not signed, we still verify that the hostname from the extracted value does, * actually, not support signed request * + * Delegates to {@see IOCMDiscoveryService::confirmRequestOrigin()}. + * * @param IIncomingSignedRequest|null $signedRequest * @param string $key entry from data available in data * @param string $value value itself used in case request is not signed @@ -447,21 +443,13 @@ class RequestHandlerController extends Controller { * @throws IncomingRequestException */ private function confirmSignedOrigin(?IIncomingSignedRequest $signedRequest, string $key, string $value): void { - if ($signedRequest === null) { - $instance = $this->getHostFromFederationId($value); - try { - $this->signatureManager->getSignatory($instance); - throw new IncomingRequestException('instance is supposed to sign its request'); - } catch (SignatoryNotFoundException) { - return; - } - } - - $body = json_decode($signedRequest->getBody(), true) ?? []; - $entry = trim($body[$key] ?? '', '@'); - if ($this->getHostFromFederationId($entry) !== $signedRequest->getOrigin()) { - throw new IncomingRequestException('share initiation (' . $signedRequest->getOrigin() . ') from different instance (' . $entry . ') [key=' . $key . ']'); + if ($signedRequest !== null) { + $body = json_decode($signedRequest->getBody(), true) ?? []; + $entry = trim(($body[$key] ?? ''), '@'); + } else { + $entry = trim($value, '@'); } + $this->ocmDiscoveryService->confirmRequestOrigin($signedRequest?->getOrigin(), $entry); } /** @@ -498,48 +486,6 @@ class RequestHandlerController extends Controller { throw new IncomingRequestException($e->getMessage(), previous: $e); } - $this->confirmNotificationEntry($signedRequest, $identity); - } - - - /** - * @param IIncomingSignedRequest|null $signedRequest - * @param string $entry - * - * @return void - * @throws IncomingRequestException - */ - private function confirmNotificationEntry(?IIncomingSignedRequest $signedRequest, string $entry): void { - $instance = $this->getHostFromFederationId($entry); - if ($signedRequest === null) { - try { - $this->signatureManager->getSignatory($instance); - throw new IncomingRequestException('instance is supposed to sign its request'); - } catch (SignatoryNotFoundException) { - return; - } - } elseif ($instance !== $signedRequest->getOrigin()) { - throw new IncomingRequestException('remote instance ' . $instance . ' not linked to origin ' . $signedRequest->getOrigin()); - } - } - - /** - * @param string $entry - * @return string - * @throws IncomingRequestException - */ - private function getHostFromFederationId(string $entry): string { - if (!str_contains($entry, '@')) { - throw new IncomingRequestException('entry ' . $entry . ' does not contain @'); - } - $rightPart = substr($entry, strrpos($entry, '@') + 1); - - // in case the full scheme is sent; getting rid of it - $rightPart = $this->addressHandler->removeProtocolFromUrl($rightPart); - try { - return $this->signatureManager->extractIdentityFromUri('https://' . $rightPart); - } catch (IdentityNotFoundException) { - throw new IncomingRequestException('invalid host within federation id: ' . $entry); - } + $this->ocmDiscoveryService->confirmRequestOrigin($signedRequest?->getOrigin(), $identity); } } diff --git a/apps/cloud_federation_api/tests/RequestHandlerControllerTest.php b/apps/cloud_federation_api/tests/RequestHandlerControllerTest.php index 04cabbd234c..326da930d9c 100644 --- a/apps/cloud_federation_api/tests/RequestHandlerControllerTest.php +++ b/apps/cloud_federation_api/tests/RequestHandlerControllerTest.php @@ -13,7 +13,6 @@ use OCA\CloudFederationAPI\Config; use OCA\CloudFederationAPI\Controller\RequestHandlerController; use OCA\CloudFederationAPI\Db\FederatedInvite; use OCA\CloudFederationAPI\Db\FederatedInviteMapper; -use OCA\FederatedFileSharing\AddressHandler; use OCP\AppFramework\Http; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Utility\ITimeFactory; @@ -28,7 +27,6 @@ use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserManager; use OCP\OCM\IOCMDiscoveryService; -use OCP\Security\Signature\ISignatureManager; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -43,13 +41,11 @@ class RequestHandlerControllerTest extends TestCase { private Config&MockObject $config; private IEventDispatcher&MockObject $eventDispatcher; private FederatedInviteMapper&MockObject $federatedInviteMapper; - private AddressHandler&MockObject $addressHandler; private IAppConfig&MockObject $appConfig; private ICloudFederationFactory&MockObject $cloudFederationFactory; private ICloudIdManager&MockObject $cloudIdManager; private IOCMDiscoveryService&MockObject $discoveryService; - private ISignatureManager&MockObject $signatureManager; private ITimeFactory&MockObject $timeFactory; private RequestHandlerController $requestHandlerController; @@ -66,12 +62,10 @@ class RequestHandlerControllerTest extends TestCase { $this->config = $this->createMock(Config::class); $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->federatedInviteMapper = $this->createMock(FederatedInviteMapper::class); - $this->addressHandler = $this->createMock(AddressHandler::class); $this->appConfig = $this->createMock(IAppConfig::class); $this->cloudFederationFactory = $this->createMock(ICloudFederationFactory::class); $this->cloudIdManager = $this->createMock(ICloudIdManager::class); $this->discoveryService = $this->createMock(IOCMDiscoveryService::class); - $this->signatureManager = $this->createMock(ISignatureManager::class); $this->timeFactory = $this->createMock(ITimeFactory::class); $this->requestHandlerController = new RequestHandlerController( @@ -85,12 +79,10 @@ class RequestHandlerControllerTest extends TestCase { $this->config, $this->eventDispatcher, $this->federatedInviteMapper, - $this->addressHandler, $this->appConfig, $this->cloudFederationFactory, $this->cloudIdManager, $this->discoveryService, - $this->signatureManager, $this->timeFactory, ); } diff --git a/lib/private/OCM/OCMDiscoveryService.php b/lib/private/OCM/OCMDiscoveryService.php index 77b7d63ec0d..7aca4ab3e48 100644 --- a/lib/private/OCM/OCMDiscoveryService.php +++ b/lib/private/OCM/OCMDiscoveryService.php @@ -18,6 +18,7 @@ use OC\OCM\Model\OCMProvider; use OCP\AppFramework\Attribute\Consumable; use OCP\AppFramework\Http; use OCP\EventDispatcher\IEventDispatcher; +use OCP\Federation\ICloudIdManager; use OCP\Http\Client\IClient; use OCP\Http\Client\IClientService; use OCP\Http\Client\IResponse; @@ -63,6 +64,7 @@ final class OCMDiscoveryService implements IOCMDiscoveryService { private IURLGenerator $urlGenerator, private readonly ISignatureManager $signatureManager, private readonly OCMSignatoryManager $signatoryManager, + private readonly ICloudIdManager $cloudIdManager, private LoggerInterface $logger, ) { $this->cache = $cacheFactory->createDistributed('ocm-discovery'); @@ -277,6 +279,49 @@ final class OCMDiscoveryService implements IOCMDiscoveryService { return null; } + /** + * @inheritDoc + * + * @since 34.0.0 + */ + #[\Override] + public function confirmRequestOrigin(?string $signedOrigin, string $ocmAddress): void { + if ($this->appConfig->getValueBool('core', OCMSignatoryManager::APPCONFIG_SIGN_DISABLED, lazy: true)) { + return; + } + + $instance = $this->getHostFromOcmAddress($ocmAddress); + + if ($signedOrigin === null) { + try { + $this->signatureManager->getSignatory($instance); + } catch (SignatoryNotFoundException) { + return; + } + throw new IncomingRequestException('instance is supposed to sign its request'); + } + + if ($instance !== $signedOrigin) { + throw new IncomingRequestException( + 'claimed origin ' . $instance . ' does not match signed origin ' . $signedOrigin + ); + } + } + + /** + * @throws IncomingRequestException on malformed address or unresolvable host + */ + private function getHostFromOcmAddress(string $entry): string { + try { + $cloudId = $this->cloudIdManager->resolveCloudId(trim($entry, '@')); + return $this->signatureManager->extractIdentityFromUri($cloudId->getRemote()); + } catch (\InvalidArgumentException $e) { + throw new IncomingRequestException('invalid OCM address: ' . $entry, previous: $e); + } catch (IdentityNotFoundException $e) { + throw new IncomingRequestException('invalid host within OCM address: ' . $entry, previous: $e); + } + } + /** * @inheritDoc * diff --git a/lib/public/OCM/IOCMDiscoveryService.php b/lib/public/OCM/IOCMDiscoveryService.php index 3f521224442..674c907bb15 100644 --- a/lib/public/OCM/IOCMDiscoveryService.php +++ b/lib/public/OCM/IOCMDiscoveryService.php @@ -65,6 +65,21 @@ interface IOCMDiscoveryService { */ public function getIncomingSignedRequest(): ?IIncomingSignedRequest; + /** + * Confirm that the host portion of $ocmAddress matches $signedOrigin + * under the current local signing policy. + * + * @param string|null $signedOrigin verified origin of the signed request, + * typically taken from {@see IIncomingSignedRequest::getOrigin()} or + * from {@see \OCP\OCM\Events\OCMEndpointRequestEvent::getRemote()}. + * NULL if the request was not signed. + * @param string $ocmAddress in `user@host` or `user@https://host` form + * + * @throws IncomingRequestException on mismatch or malformed address + * @since 34.0.0 + */ + public function confirmRequestOrigin(?string $signedOrigin, string $ocmAddress): void; + /** * Request a remote OCM endpoint. *