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.

Sign up for GitHub

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

fix(signed-request): trigger metadata insert with default value manually #49646

Merged
merged 5 commits into from
Dec 5, 2024
Merged

fix(signed-request): trigger metadata insert with default value manually #49646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a6e8d41
fix(signed-request): trigger metadata insert with default value manually
ArtificialOwl Dec 4, 2024
ac47018
fix(ocm): get details from sharedSecret from provider
ArtificialOwl Dec 5, 2024
bf89735
fix(typo): Fix message of exception
nickvergessen Dec 5, 2024
abd24f9
fix(signed-request): use share owner on reshare
ArtificialOwl Dec 5, 2024
b61a266
Merge branch 'fix/noid/trigger-field-insert' of https://github.com/ne…
ArtificialOwl Dec 5, 2024
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
69 changes: 35 additions & 34 deletions apps/cloud_federation_api/lib/Controller/RequestHandlerController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
namespace OCA\CloudFederationAPI\Controller;

use NCU\Federation\ISignedCloudFederationProvider;
use NCU\Security\Signature\Exceptions\IdentityNotFoundException;
use NCU\Security\Signature\Exceptions\IncomingRequestException;
use NCU\Security\Signature\Exceptions\SignatoryNotFoundException;
Expand Down Expand Up @@ -37,8 +38,6 @@
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 Down Expand Up @@ -68,7 +67,6 @@ public function __construct(
private ICloudIdManager $cloudIdManager,
private readonly ISignatureManager $signatureManager,
private readonly OCMSignatoryManager $signatoryManager,
private readonly IProviderFactory $shareProviderFactory,
) {
parent::__construct($appName, $request);
}
Expand Down Expand Up @@ -234,16 +232,6 @@ 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'] ?? '');
} 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 All @@ -259,6 +247,16 @@ public function receiveNotification($notificationType, $resourceType, $providerI
);
}

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->confirmNotificationIdentity($signedRequest, $resourceType, $notification);
} catch (IncomingRequestException $e) {
$this->logger->warning('incoming request exception', ['exception' => $e]);
return new JSONResponse(['message' => $e->getMessage(), 'validationErrors' => []], Http::STATUS_BAD_REQUEST);
}

try {
$provider = $this->cloudFederationProviderManager->getCloudFederationProvider($resourceType);
$result = $provider->notificationReceived($notificationType, $providerId, $notification);
Expand Down Expand Up @@ -387,50 +385,53 @@ private function confirmSignedOrigin(?IIncomingSignedRequest $signedRequest, str
}
}


/**
* confirm that the value related to share token is in format userid@hostname
* and compare hostname with the origin of the signed request.
* confirm identity of the remote instance on notification, based on the share token.
*
* 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
* @param string $resourceType
* @param string $sharedSecret
*
* @throws IncomingRequestException
* @throws BadRequestException
*/
private function confirmShareOrigin(?IIncomingSignedRequest $signedRequest, string $token): void {
if ($token === '') {
private function confirmNotificationIdentity(
?IIncomingSignedRequest $signedRequest,
string $resourceType,
array $notification,
): void {
$sharedSecret = $notification['sharedSecret'] ?? '';
if ($sharedSecret === '') {
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;
$provider = $this->cloudFederationProviderManager->getCloudFederationProvider($resourceType);
if ($provider instanceof ISignedCloudFederationProvider) {
$identity = $provider->getFederationIdFromSharedSecret($sharedSecret, $notification);
} else {
$this->logger->debug('cloud federation provider {provider} does not implements ISignedCloudFederationProvider', ['provider' => $provider::class]);
return;
}
} catch (\Exception $e) {
throw new IncomingRequestException($e->getMessage());
}

$this->confirmNotificationEntry($signedRequest, $identity);
}


/**
* @param IIncomingSignedRequest|null $signedRequest
* @param string $entry
*
* @return void
* @throws IncomingRequestException
*/
private function confirmShareEntry(?IIncomingSignedRequest $signedRequest, string $entry): void {
private function confirmNotificationEntry(?IIncomingSignedRequest $signedRequest, string $entry): void {
$instance = $this->getHostFromFederationId($entry);
if ($signedRequest === null) {
try {
Expand All @@ -440,7 +441,7 @@ private function confirmShareEntry(?IIncomingSignedRequest $signedRequest, strin
return;
}
} elseif ($instance !== $signedRequest->getOrigin()) {
throw new IncomingRequestException('token sharedWith (' . $instance . ') not linked to origin (' . $signedRequest->getOrigin() . ')');
throw new IncomingRequestException('remote instance ' . $instance . ' not linked to origin ' . $signedRequest->getOrigin());
}
}

Expand Down
35 changes: 33 additions & 2 deletions apps/federatedfilesharing/lib/OCM/CloudFederationProviderFiles.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
namespace OCA\FederatedFileSharing\OCM;

use NCU\Federation\ISignedCloudFederationProvider;
use OC\AppFramework\Http;
use OC\Files\Filesystem;
use OCA\FederatedFileSharing\AddressHandler;
Expand All @@ -21,7 +22,6 @@
use OCP\Federation\Exceptions\BadRequestException;
use OCP\Federation\Exceptions\ProviderCouldNotAddShareException;
use OCP\Federation\ICloudFederationFactory;
use OCP\Federation\ICloudFederationProvider;
use OCP\Federation\ICloudFederationProviderManager;
use OCP\Federation\ICloudFederationShare;
use OCP\Federation\ICloudIdManager;
Expand All @@ -37,11 +37,13 @@
use OCP\Server;
use OCP\Share\Exceptions\ShareNotFound;
use OCP\Share\IManager;
use OCP\Share\IProviderFactory;
use OCP\Share\IShare;
use OCP\Util;
use Psr\Log\LoggerInterface;
use SensitiveParameter;

class CloudFederationProviderFiles implements ICloudFederationProvider {
class CloudFederationProviderFiles implements ISignedCloudFederationProvider {
/**
* CloudFederationProvider constructor.
*/
Expand All @@ -63,6 +65,7 @@ public function __construct(
private Manager $externalShareManager,
private LoggerInterface $logger,
private IFilenameValidator $filenameValidator,
private readonly IProviderFactory $shareProviderFactory,
) {
}

Expand Down Expand Up @@ -747,4 +750,32 @@ public function getUserDisplayName(string $userId): string {

return $slaveService->getUserDisplayName($this->cloudIdManager->removeProtocolFromUrl($userId), false);
}

/**
* @inheritDoc
*
* @param string $sharedSecret
* @param array $payload
* @return string
*/
public function getFederationIdFromSharedSecret(
#[SensitiveParameter]
string $sharedSecret,
array $payload,
): string {
$provider = $this->shareProviderFactory->getProviderForType(IShare::TYPE_REMOTE);
try {
$share = $provider->getShareByToken($sharedSecret);
Copy link
Member

Choose a reason for hiding this comment

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

If we're in the recipient side, this will return null as the share is in share_external and not share

{
  "reqId": "t4wJRkGxiWXx6g1uJn4Q",
  "level": 2,
  "time": "2024-12-26T12:34:55+00:00",
  "remoteAddr": "172.21.0.2",
  "user": false,
  "app": "cloud_federation_api",
  "method": "POST",
  "url": "/index.php/ocm/notifications",
  "message": "incoming request exception",
  "userAgent": "Nextcloud Server Crawler",
  "version": "31.0.0.6",
  "exception": {
    "Exception": "NCU\\Security\\Signature\\Exceptions\\IncomingRequestException",
    "Message": "entry  does not contains @",
    "Code": 0,
    "Trace": [
      {
        "file": "/var/www/html/apps/cloud_federation_api/lib/Controller/RequestHandlerController.php",
        "line": 435,
        "function": "getHostFromFederationId",
        "class": "OCA\\CloudFederationAPI\\Controller\\RequestHandlerController",
        "type": "->",
        "args": [
          ""
        ]
      },
      {
        "file": "/var/www/html/apps/cloud_federation_api/lib/Controller/RequestHandlerController.php",
        "line": 423,
        "function": "confirmNotificationEntry",
        "class": "OCA\\CloudFederationAPI\\Controller\\RequestHandlerController",
        "type": "->",
        "args": [
          {
            "__class__": "OC\\Security\\Signature\\Model\\IncomingSignedRequest"
          },
          ""
        ]
      },
      {
        "file": "/var/www/html/apps/cloud_federation_api/lib/Controller/RequestHandlerController.php",
        "line": 254,
        "function": "confirmNotificationIdentity",
        "class": "OCA\\CloudFederationAPI\\Controller\\RequestHandlerController",
        "type": "->",
        "args": [
          {
            "__class__": "OC\\Security\\Signature\\Model\\IncomingSignedRequest"
          },
          "file",
          {
            "sharedSecret": "la0KxWqDj08lrUZ",
            "message": "file is no longer shared with you"
          }
        ]
      },
      {
        "file": "/var/www/html/lib/private/AppFramework/Http/Dispatcher.php",
        "line": 200,
        "function": "receiveNotification",
        "class": "OCA\\CloudFederationAPI\\Controller\\RequestHandlerController",
        "type": "->",
        "args": [
          "SHARE_UNSHARED",
          "file",
          "30",
          {
            "sharedSecret": "la0KxWqDj08lrUZ",
            "message": "file is no longer shared with you"
          }
        ]
      },
      {
        "file": "/var/www/html/lib/private/AppFramework/Http/Dispatcher.php",
        "line": 114,
        "function": "executeController",
        "class": "OC\\AppFramework\\Http\\Dispatcher",
        "type": "->",
        "args": [
          {
            "__class__": "OCA\\CloudFederationAPI\\Controller\\RequestHandlerController"
          },
          "receiveNotification"
        ]
      },
      {
        "file": "/var/www/html/lib/private/AppFramework/App.php",
        "line": 161,
        "function": "dispatch",
        "class": "OC\\AppFramework\\Http\\Dispatcher",
        "type": "->",
        "args": [
          {
            "__class__": "OCA\\CloudFederationAPI\\Controller\\RequestHandlerController"
          },
          "receiveNotification"
        ]
      },
      {
        "file": "/var/www/html/lib/private/Route/Router.php",
        "line": 306,
        "function": "main",
        "class": "OC\\AppFramework\\App",
        "type": "::",
        "args": [
          "OCA\\CloudFederationAPI\\Controller\\RequestHandlerController",
          "receiveNotification",
          {
            "__class__": "OC\\AppFramework\\DependencyInjection\\DIContainer"
          },
          {
            "_route": "cloud_federation_api.requesthandler.receivenotification"
          }
        ]
      },
      {
        "file": "/var/www/html/lib/base.php",
        "line": 1019,
        "function": "match",
        "class": "OC\\Route\\Router",
        "type": "->",
        "args": [
          "/ocm/notifications"
        ]
      },
      {
        "file": "/var/www/html/index.php",
        "line": 24,
        "function": "handleRequest",
        "class": "OC",
        "type": "::",
        "args": []
      }
    ],
    "File": "/var/www/html/apps/cloud_federation_api/lib/Controller/RequestHandlerController.php",
    "Line": 455,
    "message": "incoming request exception",
    "exception": {},
    "CustomMessage": "incoming request exception"
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

Fix incoming

} catch (ShareNotFound) {
return '';
}

// if uid_owner is a local account, the request comes from the recipient
// if not, request comes from the instance that owns the share and recipient is the re-sharer
if ($this->userManager->get($share->getShareOwner()) !== null) {
return $share->getSharedWith();
} else {
return $share->getShareOwner();
}
}
}
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_classmap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
'NCU\\Config\\Exceptions\\UnknownKeyException' => $baseDir . '/lib/unstable/Config/Exceptions/UnknownKeyException.php',
'NCU\\Config\\IUserConfig' => $baseDir . '/lib/unstable/Config/IUserConfig.php',
'NCU\\Config\\ValueType' => $baseDir . '/lib/unstable/Config/ValueType.php',
'NCU\\Federation\\ISignedCloudFederationProvider' => $baseDir . '/lib/unstable/Federation/ISignedCloudFederationProvider.php',
'NCU\\Security\\Signature\\Enum\\DigestAlgorithm' => $baseDir . '/lib/unstable/Security/Signature/Enum/DigestAlgorithm.php',
'NCU\\Security\\Signature\\Enum\\SignatoryStatus' => $baseDir . '/lib/unstable/Security/Signature/Enum/SignatoryStatus.php',
'NCU\\Security\\Signature\\Enum\\SignatoryType' => $baseDir . '/lib/unstable/Security/Signature/Enum/SignatoryType.php',
Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_static.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'NCU\\Config\\Exceptions\\UnknownKeyException' => __DIR__ . '/../../..' . '/lib/unstable/Config/Exceptions/UnknownKeyException.php',
'NCU\\Config\\IUserConfig' => __DIR__ . '/../../..' . '/lib/unstable/Config/IUserConfig.php',
'NCU\\Config\\ValueType' => __DIR__ . '/../../..' . '/lib/unstable/Config/ValueType.php',
'NCU\\Federation\\ISignedCloudFederationProvider' => __DIR__ . '/../../..' . '/lib/unstable/Federation/ISignedCloudFederationProvider.php',
'NCU\\Security\\Signature\\Enum\\DigestAlgorithm' => __DIR__ . '/../../..' . '/lib/unstable/Security/Signature/Enum/DigestAlgorithm.php',
'NCU\\Security\\Signature\\Enum\\SignatoryStatus' => __DIR__ . '/../../..' . '/lib/unstable/Security/Signature/Enum/SignatoryStatus.php',
'NCU\\Security\\Signature\\Enum\\SignatoryType' => __DIR__ . '/../../..' . '/lib/unstable/Security/Signature/Enum/SignatoryType.php',
Expand Down
3 changes: 2 additions & 1 deletion lib/private/OCM/OCMDiscoveryService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

namespace OC\OCM;

use GuzzleHttp\Exception\ConnectException;
use JsonException;
use OCP\AppFramework\Http;
use OCP\Http\Client\IClientService;
Expand Down Expand Up @@ -50,7 +51,7 @@ public function discover(string $remote, bool $skipCache = false): IOCMProvider
// if scheme not specified, we test both;
try {
return $this->discover('https://' . $remote, $skipCache);
} catch (OCMProviderException) {
} catch (OCMProviderException|ConnectException) {
return $this->discover('http://' . $remote, $skipCache);
}
}
Expand Down
3 changes: 2 additions & 1 deletion lib/private/Security/Signature/SignatureManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ private function confirmIncomingRequestSignature(
if ($ttlSignatory > 0 && $knownSignatory->getLastUpdated() < (time() - $ttlSignatory)) {
$signatory = $this->getSaneRemoteSignatory($signatoryManager, $signedRequest);
$this->updateSignatoryMetadata($signatory);
$knownSignatory->setMetadata($signatory->getMetadata());
$knownSignatory->setMetadata($signatory->getMetadata() ?? []);
}

$signedRequest->setSignatory($knownSignatory);
Expand Down Expand Up @@ -353,6 +353,7 @@ private function insertSignatory(Signatory $signatory): void {
$time = time();
$signatory->setCreation($time);
$signatory->setLastUpdated($time);
$signatory->setMetadata($signatory->getMetadata() ?? []); // trigger insert on field metadata using current or default value
$this->mapper->insert($signatory);
}

Expand Down
33 changes: 33 additions & 0 deletions lib/unstable/Federation/ISignedCloudFederationProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?php

declare(strict_types=1);
/**
* SPDX-FileCopyrightText: 2018 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace NCU\Federation;

use OCP\Federation\ICloudFederationProvider;

/**
* Interface ICloudFederationProvider
*
* Enable apps to create their own cloud federation provider
*
* @experimental 31.0.0
*/
interface ISignedCloudFederationProvider extends ICloudFederationProvider {

/**
* returns federationId in direct relation (as recipient or as author) of a sharedSecret
* the federationId must be the one at the remote end
*
* @param string $sharedSecret
* @param array $payload
*
* @experimental 31.0.0
* @return string
*/
public function getFederationIdFromSharedSecret(string $sharedSecret, array $payload): string;
}
4 changes: 2 additions & 2 deletions lib/unstable/Security/Signature/Model/Signatory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
* @method void setAccount(string $account)
* @method string getAccount()
* @method void setMetadata(array $metadata)
* @method array getMetadata()
* @method ?array getMetadata()
* @method void setCreation(int $creation)
* @method int getCreation()
* @method void setLastUpdated(int $creation)
Expand All @@ -59,7 +59,7 @@ class Signatory extends Entity implements JsonSerializable {
protected string $account = '';
Copy link
Member

Choose a reason for hiding this comment

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

Account is a nullable column which seems to cause issues on oracle:

#45979 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Not blocking so talk tests can go green again, but this is probably something to follow up

Copy link
Member

Choose a reason for hiding this comment

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

It was actually breaking Oracle as it turned out :D
#49750

protected int $type = 9;
protected int $status = 1;
protected array $metadata = [];
protected ?array $metadata = null;
protected int $creation = 0;
protected int $lastUpdated = 0;

Expand Down
Loading