Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signed requests #45979

Merged
merged 7 commits into from
Dec 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 33 additions & 6 deletions apps/cloud_federation_api/lib/Capabilities.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,27 @@
* SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\CloudFederationAPI;

use NCU\Security\Signature\Exceptions\IdentityNotFoundException;
use NCU\Security\Signature\Exceptions\SignatoryException;
use OC\OCM\OCMSignatoryManager;
use OCP\Capabilities\ICapability;
use OCP\IAppConfig;
use OCP\IURLGenerator;
use OCP\OCM\Exceptions\OCMArgumentException;
use OCP\OCM\IOCMProvider;
use Psr\Log\LoggerInterface;

class Capabilities implements ICapability {
public const API_VERSION = '1.0-proposal1';
public const API_VERSION = '1.1'; // informative, real version.

public function __construct(
private IURLGenerator $urlGenerator,
private IAppConfig $appConfig,
private IOCMProvider $provider,
private readonly OCMSignatoryManager $ocmSignatoryManager,
private readonly LoggerInterface $logger,
) {
}

Expand All @@ -28,15 +35,20 @@ public function __construct(
*
* @return array{
* ocm: array{
* apiVersion: '1.0-proposal1',
* enabled: bool,
* apiVersion: string,
* endPoint: string,
* publicKey: array{
* keyId: string,
* publicKeyPem: string,
* },
* resourceTypes: list<array{
* name: string,
* shareTypes: list<string>,
* protocols: array<string, string>
* }>,
* },
* }>,
* version: string
* }
* }
* @throws OCMArgumentException
*/
Expand All @@ -60,6 +72,21 @@ public function getCapabilities() {

$this->provider->addResourceType($resource);

return ['ocm' => $this->provider->jsonSerialize()];
// Adding a public key to the ocm discovery
try {
if (!$this->appConfig->getValueBool('core', OCMSignatoryManager::APPCONFIG_SIGN_DISABLED, lazy: true)) {
/**
* @experimental 31.0.0
* @psalm-suppress UndefinedInterfaceMethod
*/
$this->provider->setSignatory($this->ocmSignatoryManager->getLocalSignatory());
} else {
$this->logger->debug('ocm public key feature disabled');
}
} catch (SignatoryException|IdentityNotFoundException $e) {
$this->logger->warning('cannot generate local signatory', ['exception' => $e]);
}

return ['ocm' => json_decode(json_encode($this->provider->jsonSerialize()), true)];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the json_decode(json_encode( really needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is only requested for psalm as jsonSerialize returns serializable objects

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@provokateurin Any idea how to avoid it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/nextcloud/server/pull/45979/files#diff-16c946a83c0045d121ec926b69acadd4ccad303eb46144b3dfd087664522b8b2R212

publicKey: ISignatory|null,

I think this is the problem, instead you need to jsonSerialize that already and make it an array

}
}
179 changes: 178 additions & 1 deletion apps/cloud_federation_api/lib/Controller/RequestHandlerController.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,17 @@
*/
namespace OCA\CloudFederationAPI\Controller;

use NCU\Security\Signature\Exceptions\IdentityNotFoundException;
use NCU\Security\Signature\Exceptions\IncomingRequestException;
use NCU\Security\Signature\Exceptions\SignatoryNotFoundException;
use NCU\Security\Signature\Exceptions\SignatureException;
use NCU\Security\Signature\Exceptions\SignatureNotFoundException;
use NCU\Security\Signature\IIncomingSignedRequest;
use NCU\Security\Signature\ISignatureManager;
use OC\OCM\OCMSignatoryManager;
use OCA\CloudFederationAPI\Config;
use OCA\CloudFederationAPI\ResponseDefinitions;
use OCA\FederatedFileSharing\AddressHandler;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\Attribute\BruteForceProtection;
Expand All @@ -22,11 +31,14 @@
use OCP\Federation\ICloudFederationFactory;
use OCP\Federation\ICloudFederationProviderManager;
use OCP\Federation\ICloudIdManager;
use OCP\IAppConfig;
use OCP\IGroupManager;
use OCP\IRequest;
use OCP\IURLGenerator;
use OCP\IUserManager;
use OCP\Share\Exceptions\ShareNotFound;
use OCP\Share\IProviderFactory;
use OCP\Share\IShare;
use OCP\Util;
use Psr\Log\LoggerInterface;

Expand All @@ -50,8 +62,13 @@ public function __construct(
private IURLGenerator $urlGenerator,
private ICloudFederationProviderManager $cloudFederationProviderManager,
private Config $config,
private readonly AddressHandler $addressHandler,
private readonly IAppConfig $appConfig,
private ICloudFederationFactory $factory,
private ICloudIdManager $cloudIdManager,
private readonly ISignatureManager $signatureManager,
private readonly OCMSignatoryManager $signatoryManager,
private readonly IProviderFactory $shareProviderFactory,
) {
parent::__construct($appName, $request);
}
Expand Down Expand Up @@ -81,11 +98,20 @@ public function __construct(
#[NoCSRFRequired]
#[BruteForceProtection(action: 'receiveFederatedShare')]
public function addShare($shareWith, $name, $description, $providerId, $owner, $ownerDisplayName, $sharedBy, $sharedByDisplayName, $protocol, $shareType, $resourceType) {
try {
// if request is signed and well signed, no exception are thrown
// if request is not signed and host is known for not supporting signed request, no exception are thrown
$signedRequest = $this->getSignedRequest();
$this->confirmSignedOrigin($signedRequest, 'owner', $owner);
} catch (IncomingRequestException $e) {
$this->logger->warning('incoming request exception', ['exception' => $e]);
return new JSONResponse(['message' => $e->getMessage(), 'validationErrors' => []], Http::STATUS_BAD_REQUEST);
}

// check if all required parameters are set
if ($shareWith === null ||
$name === null ||
$providerId === null ||
$owner === null ||
$resourceType === null ||
$shareType === null ||
!is_array($protocol) ||
Expand Down Expand Up @@ -208,6 +234,16 @@ public function addShare($shareWith, $name, $description, $providerId, $owner, $
#[PublicPage]
#[BruteForceProtection(action: 'receiveFederatedShareNotification')]
public function receiveNotification($notificationType, $resourceType, $providerId, ?array $notification) {
try {
// if request is signed and well signed, no exception are thrown
// if request is not signed and host is known for not supporting signed request, no exception are thrown
$signedRequest = $this->getSignedRequest();
$this->confirmShareOrigin($signedRequest, $notification['sharedSecret'] ?? '');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sharedSecret inside the notification is already used by some OCM messages. So this would break it. Can you take another key? Or maybe you prefix with '$#$' or something alike?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would not break it as I send the exception for the exact same reason of a missing 'sharedSecret' entry in the notifications request. The only thing is that I do this check earlier than others but I can add a prefix if you want (while I dont feel like necessary myself)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would not break it as I send the exception for the exact same reason of a missing 'sharedSecret' entry in the notifications request.

Nextcloud Talk is sending a data field 'sharedSecret' and you either overwrite that and it breaks Talk Federation with older servers or you need to use a different key

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said before, I only use this entry because it exists in the OCM protocol and doing so to compare that the origin of the reshare request, based on the token (and the linked recipient stored in the database), confirm the identity used to sign the request.

If Talk is using this endpoint to initiate anything, signature are to be required

} catch (IncomingRequestException $e) {
$this->logger->warning('incoming request exception', ['exception' => $e]);
return new JSONResponse(['message' => $e->getMessage(), 'validationErrors' => []], Http::STATUS_BAD_REQUEST);
}

// check if all required parameters are set
if ($notificationType === null ||
$resourceType === null ||
Expand Down Expand Up @@ -256,6 +292,7 @@ public function receiveNotification($notificationType, $resourceType, $providerI
$response->throttle();
return $response;
} catch (\Exception $e) {
$this->logger->warning('incoming notification exception', ['exception' => $e]);
return new JSONResponse(
[
'message' => 'Internal error at ' . $this->urlGenerator->getBaseUrl(),
Expand Down Expand Up @@ -286,4 +323,144 @@ private function mapUid($uid) {

return $uid;
}


/**
* returns signed request if available.
* throw an exception:
* - if request is signed, but wrongly signed
* - if request is not signed but instance is configured to only accept signed ocm request
*
* @return IIncomingSignedRequest|null null if remote does not (and never did) support signed request
* @throws IncomingRequestException
*/
private function getSignedRequest(): ?IIncomingSignedRequest {
try {
$signedRequest = $this->signatureManager->getIncomingSignedRequest($this->signatoryManager);
$this->logger->debug('signed request available', ['signedRequest' => $signedRequest]);
return $signedRequest;
} catch (SignatureNotFoundException|SignatoryNotFoundException $e) {
$this->logger->debug('remote does not support signed request', ['exception' => $e]);
// remote does not support signed request.
// currently we still accept unsigned request until lazy appconfig
// core.enforce_signed_ocm_request is set to true (default: false)
if ($this->appConfig->getValueBool('core', OCMSignatoryManager::APPCONFIG_SIGN_ENFORCED, lazy: true)) {
$this->logger->notice('ignored unsigned request', ['exception' => $e]);
throw new IncomingRequestException('Unsigned request');
}
} catch (SignatureException $e) {
$this->logger->warning('wrongly signed request', ['exception' => $e]);
throw new IncomingRequestException('Invalid signature');
}
return null;
}


/**
* confirm that the value related to $key entry from the payload is in format userid@hostname
* and compare hostname with the origin of the signed request.
*
* If request is not signed, we still verify that the hostname from the extracted value does,
* actually, not support signed request
*
* @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
*
* @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 . ']');
}
}


/**
* confirm that the value related to share token is in format userid@hostname
* and compare hostname with the origin of the signed request.
*
* If request is not signed, we still verify that the hostname from the extracted value does,
* actually, not support signed request
*
* @param IIncomingSignedRequest|null $signedRequest
* @param string $token
*
* @throws IncomingRequestException
*/
private function confirmShareOrigin(?IIncomingSignedRequest $signedRequest, string $token): void {
if ($token === '') {
throw new BadRequestException(['sharedSecret']);
}

$provider = $this->shareProviderFactory->getProviderForType(IShare::TYPE_REMOTE);
$share = $provider->getShareByToken($token);
try {
$this->confirmShareEntry($signedRequest, $share->getSharedWith());
} catch (IncomingRequestException $e) {
// notification might come from the instance that owns the share
$this->logger->debug('could not confirm origin on sharedWith (' . $share->getSharedWIth() . '); going with shareOwner (' . $share->getShareOwner() . ')', ['exception' => $e]);
try {
$this->confirmShareEntry($signedRequest, $share->getShareOwner());
} catch (IncomingRequestException $f) {
// if both entry are failing, we log first exception as warning and second exception
// will be logged as warning by the controller
$this->logger->warning('could not confirm origin on sharedWith (' . $share->getSharedWIth() . '); going with shareOwner (' . $share->getShareOwner() . ')', ['exception' => $e]);
throw $f;
}
}
}

/**
* @param IIncomingSignedRequest|null $signedRequest
* @param string $entry
*
* @return void
* @throws IncomingRequestException
*/
private function confirmShareEntry(?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('token sharedWith (' . $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 contains @');
}
$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);
}
}
}
33 changes: 28 additions & 5 deletions apps/cloud_federation_api/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,21 +43,41 @@
"ocm": {
"type": "object",
"required": [
"enabled",
"apiVersion",
"enabled",
"endPoint",
"resourceTypes"
"publicKey",
"resourceTypes",
"version"
],
"properties": {
"apiVersion": {
"type": "string",
"enum": [
"1.0-proposal1"
]
},
"enabled": {
"type": "boolean"
},
"apiVersion": {
"type": "string"
},
"endPoint": {
"type": "string"
},
"publicKey": {
"type": "object",
"required": [
"keyId",
"publicKeyPem"
],
"properties": {
"keyId": {
"type": "string"
},
"publicKeyPem": {
"type": "string"
}
}
},
"resourceTypes": {
"type": "array",
"items": {
Expand Down Expand Up @@ -85,6 +105,9 @@
}
}
}
},
"version": {
"type": "string"
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion apps/federatedfilesharing/lib/FederatedShareProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,8 @@ protected function askOwnerToReShare($shareWith, IShare $share, $shareId) {
$remote,
$shareWith,
$share->getPermissions(),
$share->getNode()->getName()
$share->getNode()->getName(),
$share->getShareType(),
);

return [$token, $remoteId];
Expand Down
3 changes: 2 additions & 1 deletion apps/federatedfilesharing/lib/Notifications.php
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,13 @@ public function sendRemoteShare($token, $shareWith, $name, $remoteId, $owner, $o
* @throws HintException
* @throws \OC\ServerNotAvailableException
*/
public function requestReShare($token, $id, $shareId, $remote, $shareWith, $permission, $filename) {
public function requestReShare($token, $id, $shareId, $remote, $shareWith, $permission, $filename, $shareType) {
$fields = [
'shareWith' => $shareWith,
'token' => $token,
'permission' => $permission,
'remoteId' => $shareId,
'shareType' => $shareType,
];

$ocmFields = $fields;
Expand Down
Loading
Loading