Skip to content

Commit 13cbd0a

Browse files
mickenordinmickenordin
committed
fix(ocm): treat RFC 9421 keyid as opaque, verify by sender origin
RFC 7517 §4.5 leaves the keyid structure unspecified, so the receiver must not parse it. The signer origin now comes from the trusted OCM share/sender identity; the JWK Set is resolved against that origin and the keyid is matched opaquely to the JWKS kid. Aligns with the OCM verification procedure and fixes the origin-mismatch / flaky-kid failures in the two-port integration rig. Reverts the per-request host-based kid back to a stable persisted kid, drops keyid->host parsing, threads a sender-origin parameter through verification, and resolves that origin in every OCM inbound entry point (notifications, shares, token exchange, OCM requests) and the federation rate limiter. Cavage is unchanged. Assisted-by: ClaudeCode:glm-5.2 Signed-off-by: Micke Nordin <kano@sunet.se>
1 parent 51c9537 commit 13cbd0a

16 files changed

Lines changed: 250 additions & 104 deletions

File tree

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

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99

1010
namespace OCA\CloudFederationAPI\Controller;
1111

12-
use JsonException;
1312
use OCP\AppFramework\Controller;
1413
use OCP\AppFramework\Http;
1514
use OCP\AppFramework\Http\Attribute\BruteForceProtection;
@@ -18,6 +17,7 @@
1817
use OCP\AppFramework\Http\JSONResponse;
1918
use OCP\AppFramework\Http\Response;
2019
use OCP\EventDispatcher\IEventDispatcher;
20+
use OCP\Federation\ICloudFederationProviderManager;
2121
use OCP\IRequest;
2222
use OCP\OCM\Events\OCMEndpointRequestEvent;
2323
use OCP\OCM\Exceptions\OCMArgumentException;
@@ -31,6 +31,7 @@ public function __construct(
3131
IRequest $request,
3232
private readonly IEventDispatcher $eventDispatcher,
3333
private readonly IOCMDiscoveryService $ocmDiscoveryService,
34+
private readonly ICloudFederationProviderManager $cloudFederationProviderManager,
3435
private readonly LoggerInterface $logger,
3536
) {
3637
parent::__construct($appName, $request);
@@ -56,26 +57,29 @@ public function manageOCMRequests(string $ocmPath): Response {
5657
throw new OCMArgumentException('path is not UTF-8');
5758
}
5859

60+
// Resolve the signer origin from the payload before verification.
61+
$payload = $this->request->getParams();
62+
$origin = null;
63+
if ($payload !== []) {
64+
$identity = $this->cloudFederationProviderManager->resolveSenderIdentity($payload);
65+
if ($identity !== null) {
66+
try {
67+
$origin = $this->ocmDiscoveryService->getHostFromOcmAddress($identity);
68+
} catch (IncomingRequestException) {
69+
// unresolvable origin; verification will fail without one
70+
}
71+
}
72+
}
73+
5974
try {
60-
// if request is signed and well signed, no exceptions are thrown
61-
// if request is not signed and host is known for not supporting signed request, no exceptions are thrown
62-
$signedRequest = $this->ocmDiscoveryService->getIncomingSignedRequest();
75+
$signedRequest = $this->ocmDiscoveryService->getIncomingSignedRequest($origin);
6376
} catch (IncomingRequestException $e) {
6477
$this->logger->warning('incoming ocm request exception', ['exception' => $e]);
6578
$response = new JSONResponse(['message' => $e->getMessage(), 'validationErrors' => []], Http::STATUS_BAD_REQUEST);
6679
$response->throttle();
6780
return $response;
6881
}
6982

70-
// assuming that ocm request contains a json array
71-
$payload = $signedRequest?->getBody() ?? file_get_contents('php://input');
72-
try {
73-
$payload = ($payload) ? json_decode($payload, true, 512, JSON_THROW_ON_ERROR) : null;
74-
} catch (JsonException $e) {
75-
$this->logger->debug('json decode error', ['exception' => $e]);
76-
$payload = null;
77-
}
78-
7983
$event = new OCMEndpointRequestEvent(
8084
$this->request->getMethod(),
8185
preg_replace('@/+@', '/', $ocmPath),

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

Lines changed: 18 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,8 @@ public function addShare($shareWith, $name, $description, $providerId, $owner, $
110110
try {
111111
// if request is signed and well signed, no exceptions are thrown
112112
// if request is not signed and host is known for not supporting signed request, no exception are thrown
113-
$signedRequest = $this->ocmDiscoveryService->getIncomingSignedRequest();
113+
$origin = $this->ocmDiscoveryService->getHostFromOcmAddress($owner);
114+
$signedRequest = $this->ocmDiscoveryService->getIncomingSignedRequest($origin);
114115
$this->confirmSignedOrigin($signedRequest, 'owner', $owner);
115116
} catch (IncomingRequestException $e) {
116117
$this->logger->warning('incoming request exception', ['exception' => $e]);
@@ -307,10 +308,15 @@ public function receiveNotification($notificationType, $resourceType, $providerI
307308

308309
if (!$this->appConfig->getValueBool('core', OCMSignatoryManager::APPCONFIG_SIGN_DISABLED, lazy: true)) {
309310
try {
310-
// if request is signed and well signed, no exception are thrown
311-
// if request is not signed and host is known for not supporting signed request, no exception are thrown
312-
$signedRequest = $this->ocmDiscoveryService->getIncomingSignedRequest();
313-
$this->confirmNotificationIdentity($signedRequest, $resourceType, $notification);
311+
$identity = $this->resolveNotificationIdentity($resourceType, $notification);
312+
$origin = null;
313+
if ($identity !== '') {
314+
$origin = $this->ocmDiscoveryService->getHostFromOcmAddress($identity);
315+
}
316+
$signedRequest = $this->ocmDiscoveryService->getIncomingSignedRequest($origin);
317+
if ($identity !== '') {
318+
$this->ocmDiscoveryService->confirmRequestOrigin($signedRequest?->getOrigin(), $identity);
319+
}
314320
} catch (IncomingRequestException $e) {
315321
$this->logger->warning('incoming request exception', ['exception' => $e]);
316322
return new JSONResponse(['message' => $e->getMessage(), 'validationErrors' => []], Http::STATUS_BAD_REQUEST);
@@ -450,22 +456,16 @@ private function confirmSignedOrigin(?IIncomingSignedRequest $signedRequest, str
450456
}
451457

452458
/**
453-
* confirm identity of the remote instance on notification, based on the share token.
459+
* Resolve the sender identity from a notification's sharedSecret.
460+
* Returns '' when the provider does not implement signed federation.
454461
*
455-
* If request is not signed, we still verify that the hostname from the extracted value does,
456-
* actually, not support signed request
457-
*
458-
* @param IIncomingSignedRequest|null $signedRequest
459462
* @param string $resourceType
463+
* @param array<string, mixed> $notification
460464
*
461465
* @throws IncomingRequestException
462466
* @throws BadRequestException
463467
*/
464-
private function confirmNotificationIdentity(
465-
?IIncomingSignedRequest $signedRequest,
466-
string $resourceType,
467-
array $notification,
468-
): void {
468+
private function resolveNotificationIdentity(string $resourceType, array $notification): string {
469469
$sharedSecret = $notification['sharedSecret'] ?? '';
470470
if ($sharedSecret === '') {
471471
throw new BadRequestException(['sharedSecret']);
@@ -481,14 +481,12 @@ private function confirmNotificationIdentity(
481481
$mapping = Server::get(OcmTokenMapMapper::class)->getByAccessTokenId($accessTokenDb->getId());
482482
$identity = $provider->getFederationIdFromSharedSecret($mapping->getRefreshToken(), $notification);
483483
}
484-
} else {
485-
$this->logger->debug('cloud federation provider {provider} does not implements ISignedCloudFederationProvider', ['provider' => $provider::class]);
486-
return;
484+
return $identity;
487485
}
486+
$this->logger->debug('cloud federation provider {provider} does not implement ISignedCloudFederationProvider', ['provider' => $provider::class]);
488487
} catch (\Exception $e) {
489488
throw new IncomingRequestException($e->getMessage(), previous: $e);
490489
}
491-
492-
$this->ocmDiscoveryService->confirmRequestOrigin($signedRequest?->getOrigin(), $identity);
490+
return '';
493491
}
494492
}

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

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
use OCP\Authentication\Token\IToken;
2525
use OCP\IAppConfig;
2626
use OCP\IRequest;
27+
use OCP\OCM\IOCMDiscoveryService;
2728
use OCP\Security\ISecureRandom;
2829
use OCP\Security\Signature\Exceptions\IncomingRequestException;
2930
use OCP\Security\Signature\Exceptions\SignatoryNotFoundException;
@@ -51,19 +52,42 @@ public function __construct(
5152
private readonly IAppConfig $appConfig,
5253
private readonly OcmTokenMapMapper $ocmTokenMapMapper,
5354
private readonly IShareManager $shareManager,
55+
private readonly IOCMDiscoveryService $ocmDiscoveryService,
5456
) {
5557
parent::__construct('cloud_federation_api', $request);
5658
}
5759

60+
/**
61+
* Resolve the signer origin from the refresh token's share, or null.
62+
*
63+
* @param string $code refresh token
64+
* @return string|null signer origin, or null if it cannot be determined
65+
*/
66+
private function resolveOriginFromRefreshToken(string $code): ?string {
67+
if ($code === '') {
68+
return null;
69+
}
70+
try {
71+
$share = $this->shareManager->getShareByToken($code);
72+
$sharedWith = $share->getSharedWith();
73+
if ($sharedWith === null || $sharedWith === '') {
74+
return null;
75+
}
76+
return $this->ocmDiscoveryService->getHostFromOcmAddress($sharedWith);
77+
} catch (\Throwable) {
78+
return null;
79+
}
80+
}
81+
5882
/**
5983
* Verify the signature of incoming request if available
6084
*
6185
* @return IIncomingSignedRequest|null null if remote does not support signed requests
6286
* @throws IncomingRequestException if signature is required but invalid
6387
*/
64-
private function verifySignedRequest(): ?IIncomingSignedRequest {
88+
private function verifySignedRequest(?string $origin): ?IIncomingSignedRequest {
6589
try {
66-
$signedRequest = $this->signatureManager->getIncomingSignedRequest($this->signatoryManager);
90+
$signedRequest = $this->signatureManager->getIncomingSignedRequest($this->signatoryManager, null, $origin);
6791
$this->logger->debug('Token request signature verified', [
6892
'origin' => $signedRequest->getOrigin()
6993
]);
@@ -126,7 +150,7 @@ private function resolveJwtSigningKey(string $privateKeyPem): array {
126150
#[FrontpageRoute(verb: 'POST', url: '/api/v1/access-token')]
127151
public function accessToken(string $grant_type = '', string $code = ''): DataResponse {
128152
try {
129-
$signedRequest = $this->verifySignedRequest();
153+
$signedRequest = $this->verifySignedRequest($this->resolveOriginFromRefreshToken($code));
130154
} catch (IncomingRequestException $e) {
131155
$this->logger->warning('Token request signature verification failed', [
132156
'exception' => $e

‎apps/cloud_federation_api/tests/Controller/TokenControllerTest.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
use OCP\Authentication\Token\IToken;
2424
use OCP\IAppConfig;
2525
use OCP\IRequest;
26+
use OCP\OCM\IOCMDiscoveryService;
2627
use OCP\Security\ISecureRandom;
2728
use OCP\Security\Signature\Exceptions\SignatoryNotFoundException;
2829
use OCP\Security\Signature\Exceptions\SignatureException;
@@ -47,6 +48,7 @@ class TokenControllerTest extends TestCase {
4748
private IAppConfig&MockObject $appConfig;
4849
private OcmTokenMapMapper&MockObject $ocmTokenMapMapper;
4950
private IShareManager&MockObject $shareManager;
51+
private IOCMDiscoveryService&MockObject $ocmDiscoveryService;
5052

5153
private TokenController $controller;
5254

@@ -67,6 +69,7 @@ protected function setUp(): void {
6769
$this->appConfig = $this->createMock(IAppConfig::class);
6870
$this->ocmTokenMapMapper = $this->createMock(OcmTokenMapMapper::class);
6971
$this->shareManager = $this->createMock(IShareManager::class);
72+
$this->ocmDiscoveryService = $this->createMock(IOCMDiscoveryService::class);
7073

7174
$this->controller = new TokenController(
7275
$this->request,
@@ -79,6 +82,7 @@ protected function setUp(): void {
7982
$this->appConfig,
8083
$this->ocmTokenMapMapper,
8184
$this->shareManager,
85+
$this->ocmDiscoveryService,
8286
);
8387
}
8488

‎lib/private/AppFramework/Http/Attributes/FederationRateLimit.php‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
use OC\OCM\OCMDiscoveryService;
1414
use OCA\Federation\TrustedServers;
1515
use OCP\AppFramework\Http\Attribute\AnonRateLimit;
16+
use OCP\Federation\ICloudFederationProviderManager;
1617
use OCP\IRequest;
1718
use OCP\Server;
1819

@@ -24,12 +25,14 @@
2425
*/
2526
#[Attribute(Attribute::TARGET_METHOD)]
2627
class FederationRateLimit extends AnonRateLimit {
28+
private readonly ICloudFederationProviderManager $federationProviderManager;
2729
private readonly OCMDiscoveryService $discoveryService;
2830
private readonly ?TrustedServers $trustedServers;
2931

3032
public function __construct(int $limit, int $period) {
3133
parent::__construct($limit, $period);
3234

35+
$this->federationProviderManager = Server::get(ICloudFederationProviderManager::class);
3336
$this->discoveryService = Server::get(OCMDiscoveryService::class);
3437
$this->trustedServers = Server::get(TrustedServers::class);
3538
}
@@ -41,14 +44,22 @@ public function shouldApply(IRequest $request): bool {
4144
}
4245

4346
try {
44-
$signedRequest = $this->discoveryService->getIncomingSignedRequest();
47+
// Resolve the signer origin from the payload so trusted servers
48+
// can be exempted.
49+
$parsed = $request->getParams();
50+
$identity = $this->federationProviderManager->resolveSenderIdentity($parsed);
51+
$origin = null;
52+
if ($identity !== null) {
53+
$origin = $this->discoveryService->getHostFromOcmAddress($identity);
54+
}
55+
56+
$signedRequest = $this->discoveryService->getIncomingSignedRequest($origin);
4557
if (!$signedRequest) {
4658
return true;
4759
}
48-
$signedRequest->verify();
4960
return !$this->trustedServers->isTrustedServer($signedRequest->getOrigin());
5061
} catch (\Exception) {
51-
// no or invalid signature
62+
// no or invalid signature, or unresolvable origin
5263
return true;
5364
}
5465
}

‎lib/private/Federation/CloudFederationProviderManager.php‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use OCP\Federation\ICloudFederationProviderManager;
1919
use OCP\Federation\ICloudFederationShare;
2020
use OCP\Federation\ICloudIdManager;
21+
use OCP\Federation\ISignedCloudFederationProvider;
2122
use OCP\Http\Client\IClient;
2223
use OCP\Http\Client\IClientService;
2324
use OCP\Http\Client\IResponse;
@@ -105,6 +106,40 @@ public function getCloudFederationProvider($resourceType) {
105106
}
106107
}
107108

109+
/**
110+
* @inheritDoc
111+
*
112+
* Notifications resolve via sharedSecret; shares via owner/sender.
113+
* No access-token exchange here (app-layer); callers keep their own.
114+
*/
115+
#[\Override]
116+
public function resolveSenderIdentity(array $body): ?string {
117+
$resourceType = $body['resourceType'] ?? '';
118+
if ($resourceType !== '') {
119+
$notification = $body['notification'] ?? null;
120+
$sharedSecret = is_array($notification) ? ($notification['sharedSecret'] ?? '') : '';
121+
if ($sharedSecret !== '') {
122+
try {
123+
$provider = $this->getCloudFederationProvider($resourceType);
124+
if ($provider instanceof ISignedCloudFederationProvider || $provider instanceof \NCU\Federation\ISignedCloudFederationProvider) {
125+
$identity = $provider->getFederationIdFromSharedSecret($sharedSecret, is_array($notification) ? $notification : []);
126+
if ($identity !== '') {
127+
return $identity;
128+
}
129+
}
130+
} catch (\Exception) {
131+
// unresolved; fall through to share-style fields
132+
}
133+
}
134+
}
135+
foreach (['owner', 'sender', 'sharedBy'] as $field) {
136+
if (isset($body[$field]) && is_string($body[$field]) && $body[$field] !== '') {
137+
return $body[$field];
138+
}
139+
}
140+
return null;
141+
}
142+
108143
/**
109144
* @deprecated 29.0.0 - Use {@see sendCloudShare()} instead and handle errors manually
110145
*/

‎lib/private/OCM/OCMDiscoveryService.php‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -259,9 +259,9 @@ public function getLocalOCMProvider(bool $fullDetails = true): IOCMProvider {
259259
* @since 33.0.0
260260
*/
261261
#[\Override]
262-
public function getIncomingSignedRequest(): ?IIncomingSignedRequest {
262+
public function getIncomingSignedRequest(?string $origin = null): ?IIncomingSignedRequest {
263263
try {
264-
$signedRequest = $this->signatureManager->getIncomingSignedRequest($this->signatoryManager);
264+
$signedRequest = $this->signatureManager->getIncomingSignedRequest($this->signatoryManager, null, $origin);
265265
$this->logger->debug('signed request available', ['signedRequest' => $signedRequest]);
266266
return $signedRequest;
267267
} catch (SignatureNotFoundException|SignatoryNotFoundException $e) {
@@ -310,9 +310,14 @@ public function confirmRequestOrigin(?string $signedOrigin, string $ocmAddress):
310310
}
311311

312312
/**
313+
* Extract the signer origin (host) from an OCM address (`user@host`).
314+
*
315+
* @param string $entry OCM address in `user@host` or `user@https://host` form
316+
* @return string the host (with port) of the OCM address
313317
* @throws IncomingRequestException on malformed address or unresolvable host
314318
*/
315-
private function getHostFromOcmAddress(string $entry): string {
319+
#[\Override]
320+
public function getHostFromOcmAddress(string $entry): string {
316321
try {
317322
$cloudId = $this->cloudIdManager->resolveCloudId(trim($entry, '@'));
318323
return $this->signatureManager->extractIdentityFromUri($cloudId->getRemote());

0 commit comments

Comments
 (0)