Skip to content

Commit ec4ed86

Browse files
ArtificialOwlArtificialOwl
committed
feat(ocm): signing ocm requests
Signed-off-by: Maxence Lange <[email protected]>
1 parent de1c175 commit ec4ed86

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

47 files changed

+3527
-97
lines changed

‎apps/cloud_federation_api/lib/Capabilities.php

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,20 +6,27 @@
66
* SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
77
* SPDX-License-Identifier: AGPL-3.0-or-later
88
*/
9-
109
namespace OCA\CloudFederationAPI;
1110

11+
use NCU\Security\PublicPrivateKeyPairs\Exceptions\KeyPairException;
12+
use NCU\Security\Signature\Exceptions\SignatoryException;
13+
use OC\OCM\OCMSignatoryManager;
1214
use OCP\Capabilities\ICapability;
15+
use OCP\IAppConfig;
1316
use OCP\IURLGenerator;
1417
use OCP\OCM\Exceptions\OCMArgumentException;
1518
use OCP\OCM\IOCMProvider;
19+
use Psr\Log\LoggerInterface;
1620

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

2024
public function __construct(
2125
private IURLGenerator $urlGenerator,
26+
private IAppConfig $appConfig,
2227
private IOCMProvider $provider,
28+
private readonly OCMSignatoryManager $ocmSignatoryManager,
29+
private readonly LoggerInterface $logger,
2330
) {
2431
}
2532

@@ -28,15 +35,20 @@ public function __construct(
2835
*
2936
* @return array{
3037
* ocm: array{
38+
* apiVersion: '1.0-proposal1',
3139
* enabled: bool,
32-
* apiVersion: string,
3340
* endPoint: string,
41+
* publicKey: array{
42+
* keyId: string,
43+
* publicKeyPem: string,
44+
* },
3445
* resourceTypes: list<array{
3546
* name: string,
3647
* shareTypes: list<string>,
3748
* protocols: array<string, string>
38-
* }>,
39-
* },
49+
* }>,
50+
* version: string
51+
* }
4052
* }
4153
* @throws OCMArgumentException
4254
*/
@@ -60,6 +72,17 @@ public function getCapabilities() {
6072

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

63-
return ['ocm' => $this->provider->jsonSerialize()];
75+
// Adding a public key to the ocm discovery
76+
try {
77+
if (!$this->appConfig->getValueBool('core', OCMSignatoryManager::APPCONFIG_SIGN_DISABLED, lazy: true)) {
78+
$this->provider->setSignatory($this->ocmSignatoryManager->getLocalSignatory());
79+
} else {
80+
$this->logger->debug('ocm public key feature disabled');
81+
}
82+
} catch (SignatoryException|KeyPairException $e) {
83+
$this->logger->warning('cannot generate local signatory', ['exception' => $e]);
84+
}
85+
86+
return ['ocm' => json_decode(json_encode($this->provider->jsonSerialize()), true)];
6487
}
6588
}

‎apps/cloud_federation_api/lib/Controller/RequestHandlerController.php

Lines changed: 154 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,13 @@
55
*/
66
namespace OCA\CloudFederationAPI\Controller;
77

8+
use NCU\Security\Signature\Exceptions\IncomingRequestException;
9+
use NCU\Security\Signature\Exceptions\SignatoryNotFoundException;
10+
use NCU\Security\Signature\Exceptions\SignatureException;
11+
use NCU\Security\Signature\Exceptions\SignatureNotFoundException;
12+
use NCU\Security\Signature\ISignatureManager;
13+
use NCU\Security\Signature\Model\IIncomingSignedRequest;
14+
use OC\OCM\OCMSignatoryManager;
815
use OCA\CloudFederationAPI\Config;
916
use OCA\CloudFederationAPI\ResponseDefinitions;
1017
use OCP\AppFramework\Controller;
@@ -22,11 +29,14 @@
2229
use OCP\Federation\ICloudFederationFactory;
2330
use OCP\Federation\ICloudFederationProviderManager;
2431
use OCP\Federation\ICloudIdManager;
32+
use OCP\IAppConfig;
2533
use OCP\IGroupManager;
2634
use OCP\IRequest;
2735
use OCP\IURLGenerator;
2836
use OCP\IUserManager;
2937
use OCP\Share\Exceptions\ShareNotFound;
38+
use OCP\Share\IProviderFactory;
39+
use OCP\Share\IShare;
3040
use OCP\Util;
3141
use Psr\Log\LoggerInterface;
3242

@@ -50,8 +60,12 @@ public function __construct(
5060
private IURLGenerator $urlGenerator,
5161
private ICloudFederationProviderManager $cloudFederationProviderManager,
5262
private Config $config,
63+
private readonly IAppConfig $appConfig,
5364
private ICloudFederationFactory $factory,
5465
private ICloudIdManager $cloudIdManager,
66+
private readonly ISignatureManager $signatureManager,
67+
private readonly OCMSignatoryManager $signatoryManager,
68+
private readonly IProviderFactory $shareProviderFactory,
5569
) {
5670
parent::__construct($appName, $request);
5771
}
@@ -81,11 +95,20 @@ public function __construct(
8195
#[NoCSRFRequired]
8296
#[BruteForceProtection(action: 'receiveFederatedShare')]
8397
public function addShare($shareWith, $name, $description, $providerId, $owner, $ownerDisplayName, $sharedBy, $sharedByDisplayName, $protocol, $shareType, $resourceType) {
98+
try {
99+
// if request is signed and well signed, no exception are thrown
100+
// if request is not signed and host is known for not supporting signed request, no exception are thrown
101+
$signedRequest = $this->getSignedRequest();
102+
$this->confirmSignedOrigin($signedRequest, 'owner', $owner);
103+
} catch (IncomingRequestException $e) {
104+
$this->logger->warning('incoming request exception', ['exception' => $e]);
105+
return new JSONResponse(['message' => $e->getMessage(), 'validationErrors' => []], Http::STATUS_BAD_REQUEST);
106+
}
107+
84108
// check if all required parameters are set
85109
if ($shareWith === null ||
86110
$name === null ||
87111
$providerId === null ||
88-
$owner === null ||
89112
$resourceType === null ||
90113
$shareType === null ||
91114
!is_array($protocol) ||
@@ -208,6 +231,16 @@ public function addShare($shareWith, $name, $description, $providerId, $owner, $
208231
#[PublicPage]
209232
#[BruteForceProtection(action: 'receiveFederatedShareNotification')]
210233
public function receiveNotification($notificationType, $resourceType, $providerId, ?array $notification) {
234+
try {
235+
// if request is signed and well signed, no exception are thrown
236+
// if request is not signed and host is known for not supporting signed request, no exception are thrown
237+
$signedRequest = $this->getSignedRequest();
238+
$this->confirmShareOrigin($signedRequest, $notification['sharedSecret'] ?? '');
239+
} catch (IncomingRequestException $e) {
240+
$this->logger->warning('incoming request exception', ['exception' => $e]);
241+
return new JSONResponse(['message' => $e->getMessage(), 'validationErrors' => []], Http::STATUS_BAD_REQUEST);
242+
}
243+
211244
// check if all required parameters are set
212245
if ($notificationType === null ||
213246
$resourceType === null ||
@@ -286,4 +319,124 @@ private function mapUid($uid) {
286319

287320
return $uid;
288321
}
322+
323+
324+
/**
325+
* returns signed request if available.
326+
* throw an exception:
327+
* - if request is signed, but wrongly signed
328+
* - if request is not signed but instance is configured to only accept signed ocm request
329+
*
330+
* @return IIncomingSignedRequest|null null if remote does not (and never did) support signed request
331+
* @throws IncomingRequestException
332+
*/
333+
private function getSignedRequest(): ?IIncomingSignedRequest {
334+
try {
335+
return $this->signatureManager->getIncomingSignedRequest($this->signatoryManager);
336+
} catch (SignatureNotFoundException|SignatoryNotFoundException $e) {
337+
// remote does not support signed request.
338+
// currently we still accept unsigned request until lazy appconfig
339+
// core.enforce_signed_ocm_request is set to true (default: false)
340+
if ($this->appConfig->getValueBool('core', OCMSignatoryManager::APPCONFIG_SIGN_ENFORCED, lazy: true)) {
341+
$this->logger->notice('ignored unsigned request', ['exception' => $e]);
342+
throw new IncomingRequestException('Unsigned request');
343+
}
344+
} catch (SignatureException $e) {
345+
$this->logger->notice('wrongly signed request', ['exception' => $e]);
346+
throw new IncomingRequestException('Invalid signature');
347+
}
348+
return null;
349+
}
350+
351+
352+
/**
353+
* confirm that the value related to $key entry from the payload is in format userid@hostname
354+
* and compare hostname with the origin of the signed request.
355+
*
356+
* If request is not signed, we still verify that the hostname from the extracted value does,
357+
* actually, not support signed request
358+
*
359+
* @param IIncomingSignedRequest|null $signedRequest
360+
* @param string $key entry from data available in data
361+
* @param string $value value itself used in case request is not signed
362+
*
363+
* @throws IncomingRequestException
364+
*/
365+
private function confirmSignedOrigin(?IIncomingSignedRequest $signedRequest, string $key, string $value): void {
366+
if ($signedRequest === null) {
367+
$instance = $this->getHostFromFederationId($value);
368+
try {
369+
$this->signatureManager->searchSignatory($instance);
370+
throw new IncomingRequestException('instance is supposed to sign its request');
371+
} catch (SignatoryNotFoundException) {
372+
return;
373+
}
374+
}
375+
376+
$body = json_decode($signedRequest->getBody(), true) ?? [];
377+
$entry = trim($body[$key] ?? '', '@');
378+
if ($this->getHostFromFederationId($entry) !== $signedRequest->getOrigin()) {
379+
throw new IncomingRequestException('share initiation from different instance');
380+
}
381+
}
382+
383+
384+
/**
385+
* confirm that the value related to share token is in format userid@hostname
386+
* and compare hostname with the origin of the signed request.
387+
*
388+
* If request is not signed, we still verify that the hostname from the extracted value does,
389+
* actually, not support signed request
390+
*
391+
* @param IIncomingSignedRequest|null $signedRequest
392+
* @param string $token
393+
*
394+
* @return void
395+
* @throws IncomingRequestException
396+
*/
397+
private function confirmShareOrigin(?IIncomingSignedRequest $signedRequest, string $token): void {
398+
if ($token === '') {
399+
throw new BadRequestException(['sharedSecret']);
400+
}
401+
402+
$provider = $this->shareProviderFactory->getProviderForType(IShare::TYPE_REMOTE);
403+
$share = $provider->getShareByToken($token);
404+
$entry = $share->getSharedWith();
405+
406+
$instance = $this->getHostFromFederationId($entry);
407+
if ($signedRequest === null) {
408+
try {
409+
$this->signatureManager->searchSignatory($instance);
410+
throw new IncomingRequestException('instance is supposed to sign its request');
411+
} catch (SignatoryNotFoundException) {
412+
return;
413+
}
414+
} elseif ($instance !== $signedRequest->getOrigin()) {
415+
throw new IncomingRequestException('token sharedWith from different instance');
416+
}
417+
}
418+
419+
/**
420+
* @param string $entry
421+
* @return string
422+
* @throws IncomingRequestException
423+
*/
424+
private function getHostFromFederationId(string $entry): string {
425+
if (!str_contains($entry, '@')) {
426+
throw new IncomingRequestException('entry does not contains @');
427+
}
428+
[, $rightPart] = explode('@', $entry, 2);
429+
430+
$host = parse_url($rightPart, PHP_URL_HOST);
431+
$port = parse_url($rightPart, PHP_URL_PORT);
432+
if ($port !== null && $port !== false) {
433+
$host .= ':' . $port;
434+
}
435+
436+
if (is_string($host) && $host !== '') {
437+
return $host;
438+
}
439+
440+
throw new IncomingRequestException('host is empty');
441+
}
289442
}

‎apps/cloud_federation_api/openapi.json

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,21 +43,41 @@
4343
"ocm": {
4444
"type": "object",
4545
"required": [
46-
"enabled",
4746
"apiVersion",
47+
"enabled",
4848
"endPoint",
49-
"resourceTypes"
49+
"publicKey",
50+
"resourceTypes",
51+
"version"
5052
],
5153
"properties": {
54+
"apiVersion": {
55+
"type": "string",
56+
"enum": [
57+
"1.0-proposal1"
58+
]
59+
},
5260
"enabled": {
5361
"type": "boolean"
5462
},
55-
"apiVersion": {
56-
"type": "string"
57-
},
5863
"endPoint": {
5964
"type": "string"
6065
},
66+
"publicKey": {
67+
"type": "object",
68+
"required": [
69+
"keyId",
70+
"publicKeyPem"
71+
],
72+
"properties": {
73+
"keyId": {
74+
"type": "string"
75+
},
76+
"publicKeyPem": {
77+
"type": "string"
78+
}
79+
}
80+
},
6181
"resourceTypes": {
6282
"type": "array",
6383
"items": {
@@ -85,6 +105,9 @@
85105
}
86106
}
87107
}
108+
},
109+
"version": {
110+
"type": "string"
88111
}
89112
}
90113
}

‎apps/files_sharing/lib/External/Storage.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ public function __construct($options) {
8888
parent::__construct(
8989
[
9090
'secure' => ((parse_url($remote, PHP_URL_SCHEME) ?? 'https') === 'https'),
91+
'verify' => !$this->config->getSystemValueBool('sharing.federation.allowSelfSignedCertificates', false),
9192
'host' => $host,
9293
'root' => $webDavEndpoint,
9394
'user' => $options['token'],

‎build/integration/federation_features/cleanup-remote-storage.feature

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,9 @@ Feature: cleanup-remote-storage
3535
# server may have its own /textfile0.txt" file)
3636
And User "user1" copies file "/textfile0.txt" to "/remote-share.txt"
3737
And User "user1" from server "REMOTE" shares "/remote-share.txt" with user "user0" from server "LOCAL"
38+
And As an "user1"
39+
And sending "GET" to "/apps/files_sharing/api/v1/shares"
40+
And the list of returned shares has 1 shares
3841
And Using server "LOCAL"
3942
# Accept and download the file to ensure that a storage is created for the
4043
# federated share

‎core/Controller/OCMController.php

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
use OCP\AppFramework\Http\Attribute\PublicPage;
1818
use OCP\AppFramework\Http\DataResponse;
1919
use OCP\Capabilities\ICapability;
20-
use OCP\IConfig;
20+
use OCP\IAppConfig;
2121
use OCP\IRequest;
2222
use OCP\Server;
2323
use Psr\Container\ContainerExceptionInterface;
@@ -31,7 +31,7 @@
3131
class OCMController extends Controller {
3232
public function __construct(
3333
IRequest $request,
34-
private IConfig $config,
34+
private readonly IAppConfig $appConfig,
3535
private LoggerInterface $logger,
3636
) {
3737
parent::__construct('core', $request);
@@ -54,10 +54,10 @@ public function __construct(
5454
public function discovery(): DataResponse {
5555
try {
5656
$cap = Server::get(
57-
$this->config->getAppValue(
58-
'core',
59-
'ocm_providers',
60-
'\OCA\CloudFederationAPI\Capabilities'
57+
$this->appConfig->getValueString(
58+
'core', 'ocm_providers',
59+
\OCA\CloudFederationAPI\Capabilities::class,
60+
lazy: true
6161
)
6262
);
6363

0 commit comments

Comments
 (0)