refactor(http-sig): drop Jwk wrapper, use Firebase\JWT\Key

Addresses review feedback from @CarlSchwan.

Signed-off-by: Micke Nordin <kano@sunet.se>

Author: mickenordin

core/Command/OCM/ListKeys.php

@@ -39,7 +39,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
 		}
 
 		if ($keys === []) {
-			$output->writeln('<comment>No Ed25519 keys yet — one will be generated on first OCM request.</comment>');
+			$output->writeln('<comment>No Ed25519 keys yet; one will be generated on first OCM request.</comment>');
 			return 0;
 		}
 

lib/composer/composer/autoload_classmap.php

@@ -2147,7 +2147,6 @@
     'OC\\Security\\Ip\\Factory' => $baseDir . '/lib/private/Security/Ip/Factory.php',
     'OC\\Security\\Ip\\Range' => $baseDir . '/lib/private/Security/Ip/Range.php',
     'OC\\Security\\Ip\\RemoteAddress' => $baseDir . '/lib/private/Security/Ip/RemoteAddress.php',
-    'OC\\Security\\Jwks\\Jwk' => $baseDir . '/lib/private/Security/Jwks/Jwk.php',
     'OC\\Security\\Normalizer\\IpAddress' => $baseDir . '/lib/private/Security/Normalizer/IpAddress.php',
     'OC\\Security\\RateLimiting\\Backend\\DatabaseBackend' => $baseDir . '/lib/private/Security/RateLimiting/Backend/DatabaseBackend.php',
     'OC\\Security\\RateLimiting\\Backend\\IBackend' => $baseDir . '/lib/private/Security/RateLimiting/Backend/IBackend.php',

lib/composer/composer/autoload_static.php

@@ -2188,7 +2188,6 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
         'OC\\Security\\Ip\\Factory' => __DIR__ . '/../../..' . '/lib/private/Security/Ip/Factory.php',
         'OC\\Security\\Ip\\Range' => __DIR__ . '/../../..' . '/lib/private/Security/Ip/Range.php',
         'OC\\Security\\Ip\\RemoteAddress' => __DIR__ . '/../../..' . '/lib/private/Security/Ip/RemoteAddress.php',
-        'OC\\Security\\Jwks\\Jwk' => __DIR__ . '/../../..' . '/lib/private/Security/Jwks/Jwk.php',
         'OC\\Security\\Normalizer\\IpAddress' => __DIR__ . '/../../..' . '/lib/private/Security/Normalizer/IpAddress.php',
         'OC\\Security\\RateLimiting\\Backend\\DatabaseBackend' => __DIR__ . '/../../..' . '/lib/private/Security/RateLimiting/Backend/DatabaseBackend.php',
         'OC\\Security\\RateLimiting\\Backend\\IBackend' => __DIR__ . '/../../..' . '/lib/private/Security/RateLimiting/Backend/IBackend.php',

lib/private/OCM/OCMJwksHandler.php

@@ -43,7 +43,7 @@ public function handle(string $service, IRequestContext $context, ?IResponse $pr
 		if (!$this->appConfig->getValueBool('core', OCMSignatoryManager::APPCONFIG_SIGN_DISABLED, lazy: true)) {
 			try {
 				foreach ($this->signatoryManager->getLocalEd25519Jwks() as $jwk) {
-					$keys[] = $jwk->toArray();
+					$keys[] = $jwk;
 				}
 			} catch (Throwable $e) {
 				$this->logger->warning('failed to build local Ed25519 JWKs', ['exception' => $e]);

lib/private/OCM/OCMSignatoryManager.php

@@ -9,9 +9,12 @@
 
 namespace OC\OCM;
 
+use Firebase\JWT\JWK;
+use Firebase\JWT\JWT;
+use Firebase\JWT\Key;
 use JsonException;
 use OC\Security\IdentityProof\Manager;
-use OC\Security\Jwks\Jwk;
+use OC\Security\Signature\Rfc9421\Algorithm;
 use OC\Security\Signature\Rfc9421\IJwkResolvingSignatoryManager;
 use OCP\Http\Client\IClientService;
 use OCP\IAppConfig;
@@ -51,7 +54,7 @@ class OCMSignatoryManager implements IJwkResolvingSignatoryManager {
 	/**
 	 * Each Ed25519 keypair lives in a numbered "pool" appkey. Three slot
 	 * pointers (active/pending/retiring) reference pools by id, so rotation
-	 * is just pointer reshuffling — no copying of keypair bytes.
+	 * is just pointer reshuffling, no copying of keypair bytes.
 	 */
 	private const APPKEY_ED25519_POOL_PREFIX = 'ocm_ed25519_pool_';
 	private const APPCONFIG_ED25519_POOL_COUNTER = 'ocm_ed25519_pool_counter';
@@ -173,10 +176,10 @@ public function getLocalEd25519Signatory(): ?Signatory {
 	 * for their JWKS cache to expire.
 	 *
 	 * The active key is provisioned on demand if it does not yet exist, so
-	 * the first request to JWKS always returns at least one key — peers
+	 * the first request to JWKS always returns at least one key. Peers
 	 * must be able to discover us before we have ever signed anything.
 	 *
-	 * @return list<Jwk>
+	 * @return list<array<string, string>>
 	 */
 	public function getLocalEd25519Jwks(): array {
 		if ($this->getSlotPool(self::SLOT_ACTIVE) === null) {
@@ -191,7 +194,7 @@ public function getLocalEd25519Jwks(): array {
 			}
 			$signatory = $this->signatoryFromPool($poolId);
 			if ($signatory !== null) {
-				$jwks[] = Jwk::fromEd25519PublicKey($signatory->getPublicKey(), $signatory->getKeyId());
+				$jwks[] = self::buildEd25519JwkArray($signatory->getPublicKey(), $signatory->getKeyId());
 			}
 		}
 		return $jwks;
@@ -209,7 +212,7 @@ public function stageEd25519Key(): Signatory {
 		if ($this->getSlotPool(self::SLOT_PENDING) !== null) {
 			throw new \RuntimeException('a pending Ed25519 key already exists; activate or retire it first');
 		}
-		// Make sure we have an active key to begin with — otherwise staging
+		// Make sure we have an active key to begin with, otherwise staging
 		// a "next" key with nothing to switch from would be odd.
 		if ($this->getSlotPool(self::SLOT_ACTIVE) === null) {
 			$this->getLocalEd25519Signatory();
@@ -303,7 +306,7 @@ public function listEd25519Keys(): array {
 	 * Generate a fresh Ed25519 keypair into a new pool, recording the kid
 	 * alongside. The kid is run through {@see Signatory::setKeyId} first so
 	 * the stored value matches the canonical form (no `/index.php/`, https
-	 * scheme) used on the wire — otherwise admin output from
+	 * scheme) used on the wire, otherwise admin output from
 	 * `occ ocm:keys:list` would diverge from the kid actually published in
 	 * JWKS and used to sign requests.
 	 *
@@ -332,8 +335,8 @@ private function canonicalKid(string $kid): string {
 	/**
 	 * Build a kid for a newly-generated key. The identity portion is derived
 	 * once (from the first request that creates a key) and persisted, so
-	 * later rotations — including those triggered from CLI where there is
-	 * no Host header — produce kids on the same hostname instead of falling
+	 * later rotations (including those triggered from CLI where there is
+	 * no Host header) produce kids on the same hostname instead of falling
 	 * back to `overwrite.cli.url`.
 	 *
 	 * @throws \RuntimeException if no instance identity can be derived
@@ -349,7 +352,7 @@ private function nextEd25519PoolKid(): string {
 	 * every Ed25519 kid this instance has ever published. Resolution order:
 	 *
 	 *   1. {@see APPCONFIG_ED25519_KID_BASE} if previously stored.
-	 *   2. The active pool's kid (with the `-N` suffix stripped) — handles
+	 *   2. The active pool's kid (with the `-N` suffix stripped); handles
 	 *      instances upgraded from a single-key world without an explicit
 	 *      base appconfig.
 	 *   3. {@see buildLocalKeyId} as a fresh derivation from the request
@@ -492,13 +495,13 @@ public function getRemoteSignatory(string $remote): ?Signatory {
 	 * Results are cached per-origin for {@see JWKS_CACHE_TTL} seconds so the
 	 * common case of repeated inbound requests from the same peer doesn't
 	 * trigger a fresh HTTPS round-trip each time. On a cache hit where the
-	 * requested kid is missing, we refetch once — that lets a remote key
+	 * requested kid is missing, we refetch once. That lets a remote key
 	 * rotation propagate without waiting out the TTL.
 	 *
-	 * @return Jwk|null null when the fetch fails or no key with that kid is published
+	 * @return Key|null null when the fetch fails or no key with that kid is published
 	 */
 	#[\Override]
-	public function getRemoteJwk(string $origin, string $keyId): ?Jwk {
+	public function getRemoteKey(string $origin, string $keyId): ?Key {
 		$keys = $this->readCachedJwks($origin);
 		$fromCache = $keys !== null;
 		if (!$fromCache) {
@@ -508,12 +511,12 @@ public function getRemoteJwk(string $origin, string $keyId): ?Jwk {
 			}
 		}
 
-		$jwk = $this->findKid($keys, $keyId);
-		if ($jwk !== null) {
-			return $jwk;
+		$key = $this->findKid($keys, $keyId);
+		if ($key !== null) {
+			return $key;
 		}
 		// Only refetch if the answer came from the cache. A fresh fetch is
-		// already authoritative — refetching it just hammers the peer.
+		// already authoritative; refetching it just hammers the peer.
 		if (!$fromCache) {
 			return null;
 		}
@@ -584,15 +587,35 @@ private function fetchJwks(string $origin): ?array {
 	/**
 	 * @param list<array<string, mixed>>|null $keys
 	 */
-	private function findKid(?array $keys, string $keyId): ?Jwk {
+	private function findKid(?array $keys, string $keyId): ?Key {
 		if ($keys === null) {
 			return null;
 		}
 		foreach ($keys as $entry) {
-			if (($entry['kid'] ?? null) === $keyId) {
-				return Jwk::fromArray($entry);
+			if (($entry['kid'] ?? null) !== $keyId) {
+				continue;
+			}
+			try {
+				return JWK::parseKey($entry, Algorithm::deriveJoseAlgFromJwk($entry));
+			} catch (Throwable $e) {
+				$this->logger->warning('failed to parse remote JWK', ['exception' => $e, 'kid' => $keyId]);
+				return null;
 			}
 		}
 		return null;
 	}
+
+	/**
+	 * @return array<string, string>
+	 */
+	private static function buildEd25519JwkArray(string $rawPublicKey, string $kid): array {
+		return [
+			'kty' => 'OKP',
+			'crv' => 'Ed25519',
+			'kid' => $kid,
+			'alg' => 'EdDSA',
+			'use' => 'sig',
+			'x' => JWT::urlsafeB64Encode($rawPublicKey),
+		];
+	}
 }

lib/private/OCM/Rfc9421SignatoryManager.php

@@ -9,7 +9,7 @@
 
 namespace OC\OCM;
 
-use OC\Security\Jwks\Jwk;
+use Firebase\JWT\Key;
 use OC\Security\Signature\Rfc9421\IJwkResolvingSignatoryManager;
 use OCP\Security\Signature\Exceptions\IdentityNotFoundException;
 use OCP\Security\Signature\Model\Signatory;
@@ -21,7 +21,7 @@
  * advertised the OCM `http-sig` capability.
  *
  * Wrapping rather than mutating OCMSignatoryManager keeps the underlying
- * service stateless — important because it lives in the DI container and
+ * service stateless. That matters because it lives in the DI container and
  * may be reused across requests.
  */
 final class Rfc9421SignatoryManager implements IJwkResolvingSignatoryManager {
@@ -55,7 +55,7 @@ public function getRemoteSignatory(string $remote): ?Signatory {
 	}
 
 	#[\Override]
-	public function getRemoteJwk(string $origin, string $keyId): ?Jwk {
-		return $this->delegate->getRemoteJwk($origin, $keyId);
+	public function getRemoteKey(string $origin, string $keyId): ?Key {
+		return $this->delegate->getRemoteKey($origin, $keyId);
 	}
 }

lib/private/Security/IdentityProof/Manager.php

@@ -181,7 +181,7 @@ public function generateAppKey(string $app, string $name, array $options = []):
 	/**
 	 * Generate an Ed25519 keypair via libsodium and persist it under the given
 	 * app/name. The Key returned holds the raw 32-byte public key and the raw
-	 * 64-byte secret key (libsodium format: seed||publickey) — there is no
+	 * 64-byte secret key (libsodium format: seed||publickey); there is no
 	 * PEM wrapping. ext-sodium is a hard requirement of the server.
 	 *
 	 * Note: if a key already exists at this path it will be overwritten.

lib/private/Security/Jwks/Jwk.php

@@ -9,8 +9,8 @@
 
 namespace OC\Security\Signature\Model;
 
+use Firebase\JWT\Key;
 use JsonSerializable;
-use OC\Security\Jwks\Jwk;
 use OC\Security\Signature\Rfc9421\Algorithm;
 use OC\Security\Signature\Rfc9421\ContentDigest;
 use OC\Security\Signature\Rfc9421\OcmProfile;
@@ -42,7 +42,7 @@
  * specification fixes that policy to the `ocm` label.
  *
  * The cryptographic check is deferred to {@see verify()} which requires that
- * a {@see Jwk} has been attached via {@see setJwk()}.
+ * a {@see Key} has been attached via {@see setKey()}.
  *
  * Body integrity is enforced separately: if `content-digest` is present in the
  * covered components, the header value MUST hash to the body bytes per
@@ -55,7 +55,7 @@ class Rfc9421IncomingSignedRequest extends SignedRequest implements
 	 * Components a signature MUST cover to be considered authentic for OCM.
 	 * Missing any of these means the signer left a body or freshness window
 	 * unprotected. Callers can override via the `rfc9421.requiredComponents`
-	 * option — but tightening below this baseline is on them.
+	 * option, but tightening below this baseline is on them.
 	 */
 	private const DEFAULT_REQUIRED_COMPONENTS = [
 		'@method',
@@ -79,7 +79,7 @@ class Rfc9421IncomingSignedRequest extends SignedRequest implements
 	private array $signatureParams;
 	private string $signatureBaseString;
 	private string $rawSignature;
-	private ?Jwk $jwk = null;
+	private ?Key $key = null;
 
 	/**
 	 * @throws IncomingRequestException if anything looks wrong with the request structure
@@ -185,16 +185,14 @@ public function getKeyId(): string {
 		return $this->getSigningElement('keyId');
 	}
 
-	/**
-	 * Attach the verification key. Required before {@see verify()} is called.
-	 */
-	public function setJwk(Jwk $jwk): self {
-		$this->jwk = $jwk;
+	/** Required before {@see verify()} is called. */
+	public function setKey(Key $key): self {
+		$this->key = $key;
 		return $this;
 	}
 
-	public function getJwk(): ?Jwk {
-		return $this->jwk;
+	public function getKey(): ?Key {
+		return $this->key;
 	}
 
 	/**
@@ -226,14 +224,14 @@ public function getSignatureBaseString(): string {
 
 	#[\Override]
 	public function verify(): void {
-		if ($this->jwk === null) {
+		if ($this->key === null) {
 			throw new SignatoryNotFoundException('no JWK set for verification');
 		}
 		try {
 			$ok = Algorithm::verify(
 				$this->signatureBaseString,
 				$this->rawSignature,
-				$this->jwk,
+				$this->key,
 				$this->getAlgorithm(),
 			);
 		} catch (SignatureException $e) {