Skip to content

Commit 41b32a8

Browse files
mickenordinmickenordin
committed
fix(ocm): allow plain-http jwksUri for http-only peers
The spec mandates https for jwksUri but allows an HTTP fallback in testing setups. Advertise the scheme the instance actually serves, and accept an http jwksUri only from a peer whose own endpoint is http: integration tests and http intranets work without configuration, while an https peer advertising an http jwksUri is still rejected, since that downgrade would let a path attacker swap the key material. An https jwksUri is always accepted, also from http-only peers, and it may live on a different host than the peer. Signed-off-by: Micke Nordin <kano@sunet.se>
1 parent 537a38d commit 41b32a8

5 files changed

Lines changed: 110 additions & 42 deletions

File tree

‎lib/private/OCM/OCMDiscoveryService.php‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -210,8 +210,7 @@ public function getLocalOCMProvider(bool $fullDetails = true): IOCMProvider {
210210
$provider->setTokenEndPoint($tokenUrl);
211211
if ($signingEnabled) {
212212
try {
213-
// advertising `http-sig` requires publishing the location of
214-
// the local JWK Set in `jwksUri` (and it must be https)
213+
// http-sig advertisement requires a jwksUri
215214
$provider->setJwksUri($this->signatoryManager->getLocalJwksUri());
216215
$provider->setCapabilities(['http-sig']);
217216
} catch (IdentityNotFoundException $e) {

‎lib/private/OCM/OCMSignatoryManager.php‎

Lines changed: 15 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -376,9 +376,8 @@ private function signatoryFromPool(int $poolId): ?Signatory {
376376
}
377377

378378
/**
379-
* Absolute https URL of the local JWK Set document, advertised as
380-
* `jwksUri` in the OCM discovery response. The spec mandates https and
381-
* requires the field whenever the `http-sig` capability is exposed.
379+
* Absolute URL of the local JWK Set, advertised as `jwksUri` in the
380+
* discovery response.
382381
*
383382
* @throws IdentityNotFoundException
384383
*/
@@ -396,33 +395,21 @@ private function buildLocalKeyId(string $fragment): string {
396395
}
397396

398397
/**
399-
* Prefix $path with 'https://' and the signing identity of this instance,
400-
* including a possible subfolder.
398+
* Absolute local URL for a signing path (keyId fragment or jwksUri),
399+
* built via {@see IURLGenerator::getAbsoluteURL()} so the advertised
400+
* signing origin matches the instance URL used for federated shares.
401+
* keyId callers re-canonicalize to https through {@see Signatory::setKeyId}.
401402
*
402403
* @param string $path absolute path, starting with a slash
403404
* @return string
404-
* @throws IdentityNotFoundException
405405
*/
406406
private function buildLocalUrl(string $path): string {
407407
if ($this->appConfig->hasKey('core', self::APPCONFIG_SIGN_IDENTITY_EXTERNAL, true)) {
408-
$identity = $this->appConfig->getValueString('core', self::APPCONFIG_SIGN_IDENTITY_EXTERNAL, lazy: true);
408+
$identity = $this->appConfig->getValueString('core', self::APPCONFIG_SIGN_ID_EXTERNAL, lazy: true);
409409
return 'https://' . $identity . $path;
410410
}
411411

412-
try {
413-
return $this->signatureManager->generateKeyIdFromConfig($path);
414-
} catch (IdentityNotFoundException) {
415-
}
416-
417-
$url = $this->urlGenerator->linkToRouteAbsolute('cloud_federation_api.requesthandlercontroller.addShare');
418-
$identity = $this->signatureManager->extractIdentityFromUri($url);
419-
420-
// catching possible subfolder to create a URL like 'https://hostname/subfolder/ocm#<fragment>'
421-
$routePath = parse_url($url, PHP_URL_PATH);
422-
$pos = strpos($routePath, '/ocm/shares');
423-
$sub = ($pos) ? substr($routePath, 0, $pos) : '';
424-
425-
return 'https://' . $identity . $sub . $path;
412+
return $this->urlGenerator->getAbsoluteURL($path);
426413
}
427414

428415
/**
@@ -540,10 +527,9 @@ private function fetchJwks(string $origin): ?array {
540527
}
541528

542529
/**
543-
* Location of the peer's JWK Set, read from the `jwksUri` field of its
544-
* discovery response. A peer that advertises `http-sig` without a https
545-
* `jwksUri` is non-conformant: no keys can be obtained from it, so its
546-
* signed requests will fail verification.
530+
* The peer's `jwksUri` from its discovery response. Must be https, or
531+
* http from an http-only peer (the spec's testing fallback): an https
532+
* peer pointing at an http jwksUri would downgrade the key fetch.
547533
*/
548534
private function resolveJwksUri(string $origin): ?string {
549535
try {
@@ -560,8 +546,10 @@ private function resolveJwksUri(string $origin): ?string {
560546
}
561547
return null;
562548
}
563-
if (!str_starts_with($jwksUri, 'https://')) {
564-
$this->logger->warning('remote jwksUri does not use https, ignoring', ['origin' => $origin, 'jwksUri' => $jwksUri]);
549+
$httpFromHttpPeer = str_starts_with($jwksUri, 'http://')
550+
&& str_starts_with($provider->getEndPoint(), 'http://');
551+
if (!str_starts_with($jwksUri, 'https://') && !$httpFromHttpPeer) {
552+
$this->logger->warning('refusing jwksUri: https is required unless the peer itself is http-only', ['origin' => $origin, 'jwksUri' => $jwksUri]);
565553
return null;
566554
}
567555
return $jwksUri;

‎tests/lib/OCM/DiscoveryServiceTest.php‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use OCA\CloudFederationAPI\Controller\OCMRequestController;
1515
use OCP\EventDispatcher\IEventDispatcher;
1616
use OCP\IConfig;
17+
use OCP\IURLGenerator;
1718
use OCP\OCM\Events\LocalOCMDiscoveryEvent;
1819
use OCP\OCM\Events\OCMEndpointRequestEvent;
1920
use OCP\Server;
@@ -137,11 +138,12 @@ public function testLocalCapabilitiesAdvertiseHttpSigByDefault(): void {
137138
}
138139

139140
public function testLocalDiscoveryAdvertisesJwksUri(): void {
140-
// implementations advertising `http-sig` MUST provide a https
141-
// `jwksUri` as well
141+
// scheme follows the instance base URL
142142
$local = $this->discoveryService->getLocalOCMProvider();
143143
$jwksUri = $local->getJwksUri();
144-
$this->assertStringStartsWith('https://', $jwksUri);
144+
$baseUrl = Server::get(IURLGenerator::class)->getBaseUrl();
145+
$expectedScheme = str_starts_with($baseUrl, 'http://') ? 'http://' : 'https://';
146+
$this->assertStringStartsWith($expectedScheme, $jwksUri);
145147
$this->assertStringEndsWith('/.well-known/jwks.json', $jwksUri);
146148
}
147149

‎tests/lib/OCM/OCMSignatoryManagerJwksTest.php‎

Lines changed: 82 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,10 +86,15 @@ protected function tearDown(): void {
8686
}
8787

8888
/** Remote discovery response advertising http-sig and $jwksUri. */
89-
private function primeDiscovery(string $jwksUri = self::JWKS_URI, array $capabilities = ['http-sig']): void {
89+
private function primeDiscovery(
90+
string $jwksUri = self::JWKS_URI,
91+
array $capabilities = ['http-sig'],
92+
string $endPoint = 'https://sender.example.org/ocm',
93+
): void {
9094
$provider = new OCMProvider();
9195
$provider->setCapabilities($capabilities);
9296
$provider->setJwksUri($jwksUri);
97+
$provider->setEndPoint($endPoint);
9398
$this->discoveryService->method('discover')->willReturn($provider);
9499
}
95100

@@ -166,13 +171,32 @@ public function testGetRemoteKeyRejectsMissingJwksUriWhenHttpSigAdvertised(): vo
166171
$this->assertNull($this->signatoryManager->getRemoteKey('sender.example.org', 'kid'));
167172
}
168173

169-
public function testGetRemoteKeyRejectsNonHttpsJwksUri(): void {
174+
public function testGetRemoteKeyRejectsHttpJwksUriFromHttpsPeer(): void {
175+
// downgrade guard: http jwksUri from an https peer
170176
$this->primeDiscovery(jwksUri: 'http://sender.example.org/ocm/jwks');
171177
$this->client->expects($this->never())->method('get');
172178
$this->logger->expects($this->once())->method('warning');
173179
$this->assertNull($this->signatoryManager->getRemoteKey('sender.example.org', 'kid'));
174180
}
175181

182+
public function testGetRemoteKeyAcceptsHttpJwksUriFromHttpPeer(): void {
183+
// the spec's http fallback for testing setups
184+
$this->primeDiscovery(
185+
jwksUri: 'http://sender.example.org/ocm/jwks',
186+
endPoint: 'http://sender.example.org/ocm',
187+
);
188+
$kid = 'sender.example.org#key1';
189+
$this->client->expects($this->once())
190+
->method('get')
191+
->with(
192+
$this->equalTo('http://sender.example.org/ocm/jwks'),
193+
$this->isType('array'),
194+
)
195+
->willReturn($this->jsonResponse(['keys' => [$this->ecJwk($kid)]]));
196+
197+
$this->assertNotNull($this->signatoryManager->getRemoteKey('sender.example.org', $kid));
198+
}
199+
176200
public function testGetRemoteKeyReturnsNullWhenDiscoveryFails(): void {
177201
$this->discoveryService->method('discover')
178202
->willThrowException(new OCMProviderException('no discovery'));
@@ -269,6 +293,62 @@ public function testCacheMissOnNewKidTriggersRefetchOnce(): void {
269293
$this->assertNotNull($this->signatoryManager->getRemoteKey('sender.example.org', 'new'));
270294
}
271295

296+
public function testGetRemoteKeyAcceptsHttpsJwksUriFromHttpPeer(): void {
297+
// upgrade from an http-only peer is fine
298+
$this->primeDiscovery(
299+
endPoint: 'http://sender.example.org/ocm',
300+
);
301+
$kid = 'sender.example.org#key1';
302+
$this->client->expects($this->once())
303+
->method('get')
304+
->with(
305+
$this->equalTo(self::JWKS_URI),
306+
$this->isType('array'),
307+
)
308+
->willReturn($this->jsonResponse(['keys' => [$this->ecJwk($kid)]]));
309+
310+
$this->assertNotNull($this->signatoryManager->getRemoteKey('sender.example.org', $kid));
311+
}
312+
313+
public function testGetRemoteKeyAcceptsJwksUriOnDifferentHost(): void {
314+
// the JWK Set may live on a different host than the peer
315+
$this->primeDiscovery(
316+
jwksUri: 'https://keys.example.net/ocm/jwks',
317+
endPoint: 'http://sender.example.org/ocm',
318+
);
319+
$kid = 'sender.example.org#key1';
320+
$this->client->expects($this->once())
321+
->method('get')
322+
->with(
323+
$this->equalTo('https://keys.example.net/ocm/jwks'),
324+
$this->isType('array'),
325+
)
326+
->willReturn($this->jsonResponse(['keys' => [$this->ecJwk($kid)]]));
327+
328+
$this->assertNotNull($this->signatoryManager->getRemoteKey('sender.example.org', $kid));
329+
}
330+
331+
public function testGetLocalJwksUriUsesHttpsByDefault(): void {
332+
$this->urlGenerator->method('getAbsoluteURL')
333+
->willReturnCallback(fn (string $path) => 'https://sender.example.org' . $path);
334+
335+
$this->assertSame(
336+
'https://sender.example.org/.well-known/jwks.json',
337+
$this->signatoryManager->getLocalJwksUri(),
338+
);
339+
}
340+
341+
public function testGetLocalJwksUriFollowsHttpInstanceScheme(): void {
342+
// http-only deployments must advertise a fetchable jwksUri
343+
$this->urlGenerator->method('getAbsoluteURL')
344+
->willReturnCallback(fn (string $path) => 'http://localhost:8180' . $path);
345+
346+
$this->assertSame(
347+
'http://localhost:8180/.well-known/jwks.json',
348+
$this->signatoryManager->getLocalJwksUri(),
349+
);
350+
}
351+
272352
private function respondWith(array $body): void {
273353
$this->client->method('get')->willReturn($this->jsonResponse($body));
274354
}

‎tests/lib/OCM/OCMSignatoryManagerRotationTest.php‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
use OCP\ICacheFactory;
2020
use OCP\IConfig;
2121
use OCP\IURLGenerator;
22-
use OCP\Security\Signature\Exceptions\IdentityNotFoundException;
2322
use OCP\Security\Signature\ISignatureManager;
2423
use PHPUnit\Framework\MockObject\MockObject;
2524
use Psr\Log\LoggerInterface;
@@ -47,16 +46,17 @@ protected function setUp(): void {
4746
$this->wireIdentityProofManager();
4847

4948
$signatureManager = $this->createMock(ISignatureManager::class);
50-
$signatureManager->method('generateKeyIdFromConfig')
51-
->willReturnCallback(static fn (string $suffix): string => 'https://alice.example/' . ltrim($suffix, '/'));
49+
$urlGenerator = $this->createMock(IURLGenerator::class);
50+
$urlGenerator->method('getAbsoluteURL')
51+
->willReturnCallback(static fn (string $path): string => 'https://alice.example' . $path);
5252

5353
$cacheFactory = $this->createMock(ICacheFactory::class);
5454
$cacheFactory->method('createDistributed')->willReturn(new ArrayCache(''));
5555

5656
$this->signatoryManager = new OCMSignatoryManager(
5757
$this->appConfig,
5858
$signatureManager,
59-
$this->createMock(IURLGenerator::class),
59+
$urlGenerator,
6060
$this->identityProofManager,
6161
$this->stubClientService(),
6262
$this->createMock(IConfig::class),
@@ -188,11 +188,10 @@ public function testSignerReturnsNullWhenIdentityCannotBeDerived(): void {
188188
// identity at all; provisioning the first key should fail loudly so
189189
// the admin gets a clear message instead of a corrupt half-state.
190190
$signatureManager = $this->createMock(ISignatureManager::class);
191-
$signatureManager->method('generateKeyIdFromConfig')
192-
->willThrowException(new IdentityNotFoundException('no identity'));
193191
$urlGenerator = $this->createMock(IURLGenerator::class);
194-
$urlGenerator->method('linkToRouteAbsolute')
195-
->willThrowException(new IdentityNotFoundException('no url either'));
192+
// getAbsoluteURL() yields no host, so the kid's identity cannot be
193+
// resolved; provisioning must fail loudly rather than corrupt state.
194+
$urlGenerator->method('getAbsoluteURL')->willReturn('');
196195

197196
$cacheFactory = $this->createMock(ICacheFactory::class);
198197
$cacheFactory->method('createDistributed')->willReturn(new ArrayCache(''));

0 commit comments

Comments
 (0)