diff --git a/appinfo/routes.php b/appinfo/routes.php index e9478b355..61555cc94 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -280,7 +280,10 @@ // The `doriath` segment survives the doriath -> keepiq rename on purpose: // it is a published contract URL, not an app id. See the class docblock // on DiscoveryController for the full reasoning. - ['name' => 'discovery#document', 'url' => '/api/v1/app/.well-known/doriath', 'verb' => 'GET'], + // Canonical discovery path. The pre-rename path below is still served + // and is retired before the first stable release — see legacyDocument(). + ['name' => 'discovery#document', 'url' => '/api/v1/app/.well-known/keepiq', 'verb' => 'GET'], + ['name' => 'discovery#legacyDocument', 'url' => '/api/v1/app/.well-known/doriath', 'verb' => 'GET'], // JWT-Bearer token exchange (public; signature-verified). ['name' => 'applicationToken#exchange', 'url' => '/api/v1/token', 'verb' => 'POST'], diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 5a26cb58a..8577679ab 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -49,6 +49,27 @@ class Application extends App implements IBootstrap { public const APP_ID = 'keepiq'; + /** + * The app version by which every pre-rename compatibility shim is gone. + * + * The doriath -> keepiq rename left a handful of published identifiers + * carrying the old codename: the assertion audience, the discovery path + * and the envelope format name. Each is accepted or announced in parallel + * with its replacement so no consumer needs a flag day — but only while + * the app is pre-stable. This app has never shipped a stable release, so + * there is no released contract to preserve and no reason to carry a dead + * codename past 1.0.0; the shims are removed before the first stable + * release, not deferred to a future apiVersion. + * + * `apiVersion` therefore stays at 1 throughout. The "breaking changes + * MUST ship as a new apiVersion" rule in the secret-store-api spec binds + * from the first stable release onward, which is exactly the point these + * shims stop existing. + * + * @var string + */ + public const PRE_STABLE_COMPAT_REMOVED_IN = '1.0.0'; + /** * Constructor for the Application class. * diff --git a/lib/Controller/DiscoveryController.php b/lib/Controller/DiscoveryController.php index 6f2044614..0980634e7 100644 --- a/lib/Controller/DiscoveryController.php +++ b/lib/Controller/DiscoveryController.php @@ -4,21 +4,27 @@ * Keepiq Machine API Discovery Controller * * Serves the unauthenticated, machine-readable discovery document at - * `GET /api/v1/app/.well-known/doriath`. A consumer configures one base + * `GET /api/v1/app/.well-known/keepiq`, and at the pre-rename + * `.well-known/doriath` until that path is retired. A consumer configures one base * URL plus its application id and private key, fetches this document, and * derives every contract URL (token endpoint, grant type, assertion * requirements, secret endpoints, envelope formats) without reading * Keepiq source. The document carries no instance-private data. * - * THE PATH SEGMENT STILL SAYS `doriath` AFTER THE doriath -> keepiq RENAME, - * on purpose. It is the one URL a machine consumer is configured with by - * hand; everything else it uses is derived from the document this endpoint - * returns. Renaming the segment would break every configured consumer at the - * same moment as, and independently of, the `/apps//` prefix change — - * two breaking changes where the contract (openspec/specs/secret-store-api/ - * spec.md) allows none in place. Moving it belongs to the coordinated - * apiVersion bump that also retires the `doriath-machine-secret-v1` envelope - * name and the `aud=doriath` claim, not to an app-id rename. + * BOTH PATHS ARE SERVED, and the pre-rename one is not going away yet. This + * is the one URL a machine consumer is configured with by hand — everything + * else it uses is derived from the document this endpoint returns — so moving + * it would break every configured consumer at once. Serving both instead is + * additive: the document names the canonical path in `discoveryPath`, so a + * consumer re-points itself without anyone coordinating a change window, and + * `deprecatedDiscoveryPaths[].removedInAppVersion` says when the old one + * stops. Every hit on it is logged so the migration is observable. + * + * The old path retires at Application::PRE_STABLE_COMPAT_REMOVED_IN, together + * with the `doriath-machine-secret-v1` envelope name and the `aud=doriath` + * claim — not at a future apiVersion. Nothing stable has shipped, so there is + * no released contract a version bump would protect; PreStableCompatDeadlineTest + * fails the build if any of the three outlives that version. * * @category Controller * @package OCA\Keepiq\Controller @@ -37,6 +43,7 @@ namespace OCA\Keepiq\Controller; use OCA\Keepiq\AppInfo\Application as KeepiqApp; +use OCA\Keepiq\Service\AudiencePolicy; use OCA\Keepiq\Service\JwtAuthService; use OCA\Keepiq\Service\MachineSecretEnvelopeService; use OCP\AppFramework\Controller; @@ -46,6 +53,7 @@ use OCP\AppFramework\Http\JSONResponse; use OCP\IAppConfig; use OCP\IRequest; +use Psr\Log\LoggerInterface; use OCP\IURLGenerator; /** @@ -62,12 +70,34 @@ class DiscoveryController extends Controller { */ public const API_VERSION = 1; + /** + * The discovery path this document advertises as canonical. + * + * @var string + */ + public const CANONICAL_DISCOVERY_PATH = '/api/v1/app/.well-known/keepiq'; + + /** + * The pre-rename discovery path, still served and now deprecated. + * + * @var string + */ + public const DEPRECATED_DISCOVERY_PATH = '/api/v1/app/.well-known/doriath'; + + /** + * The app version in which DEPRECATED_DISCOVERY_PATH stops being served. + * + * @var string + */ + public const DEPRECATED_PATH_REMOVED_IN = KeepiqApp::PRE_STABLE_COMPAT_REMOVED_IN; + /** * Constructor for DiscoveryController. * * @param IRequest $request The HTTP request * @param IURLGenerator $urlGenerator The URL generator * @param IAppConfig|null $appConfig The app config (lease policy advert) + * @param LoggerInterface|null $logger Logger for the deprecated-path warning * * @return void */ @@ -75,6 +105,7 @@ public function __construct( IRequest $request, private IURLGenerator $urlGenerator, private ?IAppConfig $appConfig = null, + private ?LoggerInterface $logger = null, ) { parent::__construct(appName: KeepiqApp::APP_ID, request: $request); }//end __construct() @@ -107,12 +138,35 @@ public function document(): JSONResponse { return new JSONResponse( data: [ 'apiVersion' => self::API_VERSION, + // The path to fetch this document from. A consumer configured + // with the pre-rename path can re-point itself from here + // before that path is retired. + 'discoveryPath' => self::CANONICAL_DISCOVERY_PATH, + 'deprecatedDiscoveryPaths' => [ + [ + 'value' => self::DEPRECATED_DISCOVERY_PATH, + 'removedInAppVersion' => self::DEPRECATED_PATH_REMOVED_IN, + ], + ], 'tokenEndpoint' => $tokenEndpoint, 'grantType' => 'urn:ietf:params:oauth:grant-type:jwt-bearer', 'assertion' => [ 'alg' => 'RS256', 'maxLifetime' => JwtAuthService::ACCESS_TOKEN_TTL, - 'audience' => JwtAuthService::EXPECTED_AUDIENCE, + // `audience` is the value to SEND; `acceptedAudiences` is + // what this instance will honour. Both are additive within + // the current apiVersion: a consumer reading `audience` + // converges on the canonical name, and one still sending a + // deprecated value keeps working until the version named in + // `deprecatedAudiences[].removedInAppVersion`. + 'audience' => AudiencePolicy::CANONICAL_AUDIENCE, + 'acceptedAudiences' => AudiencePolicy::ACCEPTED_AUDIENCES, + 'deprecatedAudiences' => [ + [ + 'value' => AudiencePolicy::DEPRECATED_AUDIENCE, + 'removedInAppVersion' => AudiencePolicy::DEPRECATED_AUDIENCE_REMOVED_IN, + ], + ], 'audienceUrl' => $tokenAbsolute, ], 'secrets' => [ @@ -122,7 +176,18 @@ public function document(): JSONResponse { 'create' => $this->urlGenerator->linkToRoute('keepiq.applicationSecrets.index'), 'update' => $this->urlGenerator->linkToRoute('keepiq.applicationSecrets.index') . '/{id}', ], + // What this instance actually emits today. The successor is + // announced separately rather than listed here, because + // listing a format nothing writes would be a lie a consumer + // could reasonably act on. 'envelopeFormats' => [MachineSecretEnvelopeService::FORMAT], + 'upcomingEnvelopeFormats' => [ + [ + 'value' => MachineSecretEnvelopeService::UPCOMING_FORMAT, + 'replaces' => MachineSecretEnvelopeService::FORMAT, + 'emittedFromAppVersion' => MachineSecretEnvelopeService::UPCOMING_FORMAT_APP_VERSION, + ], + ], // Machine leases (machine-secret-leases §3.3): additive // advert of the instance lease policy — no envelope or // addressing change. @@ -135,4 +200,37 @@ public function document(): JSONResponse { ] ); }//end document() + /** + * Serve the same document on the pre-rename discovery path. + * + * This is THE one URL a machine consumer is configured with by hand, so + * moving it is the single most disruptive rename available: everything + * else a consumer uses is derived from the document this returns. Serving + * both paths is additive and costs nothing, and it hands the consumer the + * canonical path in `discoveryPath` so it can re-point itself without + * anyone coordinating a change window. + * + * Each hit is logged so the set of consumers still on the old path is + * observable before the shim is removed. + * + * @return JSONResponse The discovery document. + * + * @spec openspec/specs/secret-store-api/spec.md + */ + #[PublicPage] + #[NoCSRFRequired] + #[AnonRateLimit(limit: 120, period: 60)] + public function legacyDocument(): JSONResponse { + $this->logger?->warning( + 'Discovery fetched on the deprecated path "{deprecated}", which is removed in ' + . 'app version {version}. Re-point the consumer at "{canonical}".', + [ + 'deprecated' => self::DEPRECATED_DISCOVERY_PATH, + 'canonical' => self::CANONICAL_DISCOVERY_PATH, + 'version' => self::DEPRECATED_PATH_REMOVED_IN, + ] + ); + + return $this->document(); + }//end legacyDocument() }//end class diff --git a/lib/Service/AudiencePolicy.php b/lib/Service/AudiencePolicy.php new file mode 100644 index 000000000..71d2d8b5e --- /dev/null +++ b/lib/Service/AudiencePolicy.php @@ -0,0 +1,212 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + */ + +declare(strict_types=1); + +namespace OCA\Keepiq\Service; + +use OCA\Keepiq\AppInfo\Application as KeepiqApp; +use Psr\Log\LoggerInterface; +use RuntimeException; + +/** + * The set of audience values this instance accepts, and their deprecation. + */ +class AudiencePolicy { + + /** + * The audience claim this instance advertises and prefers. + * + * Assertions are ACCEPTED on any value in ACCEPTED_AUDIENCES; this is the + * one published in the `.well-known` discovery document, so a + * self-configuring consumer converges on it without being told. + * + * @var string + */ + public const CANONICAL_AUDIENCE = 'keepiq'; + + /** + * The pre-rename audience, still accepted and now deprecated. + * + * Not an app id but a published authentication parameter: every + * application registered before the rename signs `aud=doriath` into its + * RS256 assertion with a private key this server does not hold and cannot + * re-sign. Rejecting it outright would be a fleet-wide credential outage + * that no repair step can heal, because the fix lives in each consumer's + * configuration. + * + * Accepting both instead is ADDITIVE, so it costs nothing and no consumer + * needs a change window. The shim is removed before the first stable + * release — see Application::PRE_STABLE_COMPAT_REMOVED_IN — not deferred + * to a future apiVersion: nothing stable has shipped, so there is no + * released contract a version bump would protect. + * + * @var string + */ + public const DEPRECATED_AUDIENCE = 'doriath'; + + /** + * The app version in which DEPRECATED_AUDIENCE stops being accepted. + * + * @var string + */ + public const DEPRECATED_AUDIENCE_REMOVED_IN = KeepiqApp::PRE_STABLE_COMPAT_REMOVED_IN; + + /** + * Every audience value an assertion may carry to reach this instance. + * + * RFC 7519 section 4.1.3 requires the recipient to identify itself with a + * value in the claim — both of these name this app, so accepting the pair + * does not widen the confused-deputy guard. + * + * @var string[] + */ + public const ACCEPTED_AUDIENCES = [ + self::CANONICAL_AUDIENCE, + self::DEPRECATED_AUDIENCE, + ]; + + /** + * Constructor. + * + * @param LoggerInterface $logger Reports use of the deprecated value. + */ + public function __construct( + private readonly LoggerInterface $logger, + ) { + }//end __construct() + + /** + * Assert the claim set names this instance, and flag the deprecated name. + * + * RFC 7519 section 4.1.3 makes `aud` either a single string or an array of + * them, and requires only that the recipient identify itself with ONE of + * the values. Both accepted values name this app, so honouring the pair is + * the claim's own semantics rather than a relaxation of it. + * + * REPORTING IS NOT DONE HERE. This runs before replay detection, issuer + * lookup and signature verification, so at this point the assertion is + * merely well-addressed, not authentic — and its `iss` is a string an + * unauthenticated caller chose. Warning here would let anyone forge + * migration traffic for any issuer they name, and the resulting log could + * not be used to decide when the deprecated value is safe to remove, which + * is the only reason the log exists. The caller reports separately, after + * authentication succeeds. + * + * @param array $claims The decoded claim set. + * + * @return bool True when ONLY the deprecated value named this instance. + * + * @throws RuntimeException When no presented value names this instance. + * + * @spec openspec/specs/secret-store-api/spec.md#requirement-assertion-audience + */ + public function assertNamesThisInstance(array $claims): bool { + $presented = $this->presentedValues(claim: ($claims['aud'] ?? null)); + $matched = array_values(array_intersect($presented, self::ACCEPTED_AUDIENCES)); + + if ($matched === []) { + throw new RuntimeException(message: 'Wrong audience'); + } + + return (in_array(self::CANONICAL_AUDIENCE, $matched, true) === false); + }//end assertNamesThisInstance() + + /** + * Report that an AUTHENTICATED assertion used the deprecated audience. + * + * Called only once the signature, issuer and replay checks have passed, so + * the issuer named here is one this instance verified rather than one the + * caller asserted. That is what makes the resulting set of issuers usable + * as the migration checklist it is meant to be. + * + * @param array $claims The decoded claim set. + * + * @return void + * + * @spec openspec/specs/secret-store-api/spec.md#requirement-assertion-audience + */ + public function reportDeprecatedUse(array $claims): void { + $this->logger->warning( + 'Assertion accepted on the deprecated audience "{deprecated}", which is ' + . 'removed in app version {version}. Update issuer "{iss}" to send "{canonical}".', + [ + 'deprecated' => self::DEPRECATED_AUDIENCE, + 'canonical' => self::CANONICAL_AUDIENCE, + 'version' => self::DEPRECATED_AUDIENCE_REMOVED_IN, + 'iss' => (string)($claims['iss'] ?? 'unknown'), + ] + ); + }//end reportDeprecatedUse() + + /** + * Read the `aud` claim as the list of strings it presents. + * + * RFC 7519 section 4.1.3 defines `aud` as a StringOrURI or an array of + * them, so anything else is a malformed assertion and presents no audience + * at all — which the caller turns into a rejection. + * + * NOTHING IS COERCED. `aud: 123` and `aud: true` are not audiences that + * happen to be written oddly; they are malformed, and casting them to + * "123" and "1" would launder a type error into a value that then gets + * compared against the accepted set. Nothing in that set is numeric today, + * so the coercion changed no verdict — but "no accepted audience is + * currently a number" is a fact about configuration, not a property of the + * check, and it is not one an auth path should lean on. + * + * A MIXED ARRAY IS REJECTED WHOLE, rather than filtered down to its string + * members. `["keepiq", {...}]` is not a well-formed claim, and quietly + * discarding the part that does not parse would authenticate on the + * remainder — the lenient reading is the one that lets a malformed + * assertion through. + * + * @param mixed $claim The raw `aud` claim. + * + * @return string[] The presented audience values, empty when malformed. + * + * @spec openspec/specs/secret-store-api/spec.md#requirement-assertion-audience + */ + private function presentedValues(mixed $claim): array { + if (is_array($claim) === true) { + $values = []; + foreach ($claim as $value) { + if (is_string($value) === false || $value === '') { + // One malformed member makes the whole claim malformed. + return []; + } + + $values[] = $value; + } + + return $values; + } + + if (is_string($claim) === true && $claim !== '') { + return [$claim]; + } + + return []; + }//end presentedValues() +}//end class diff --git a/lib/Service/JwtAssertionVerifier.php b/lib/Service/JwtAssertionVerifier.php index e46d0b58b..543d3bc0d 100644 --- a/lib/Service/JwtAssertionVerifier.php +++ b/lib/Service/JwtAssertionVerifier.php @@ -64,8 +64,9 @@ public function __construct( * Deserialize an assertion and return its claim set, having asserted * that every required claim is present and acceptable. * - * Required claims: iss (application id), aud="doriath", exp (>now), - * iat (<=now+CLOCK_SKEW), jti. + * Required claims: iss (application id), aud (PRESENCE only — which values + * name this instance is AudiencePolicy's decision, asserted by + * JwtAuthService), exp (>now), iat (<=now+CLOCK_SKEW), jti. * * @param string $assertion The JWS compact serialization * @@ -145,11 +146,20 @@ private function readAssertionClaims(JWS $jws): array { throw new RuntimeException(message: 'Assertion has no payload'); } - $claims = json_decode($payloadRaw, true); - if (is_array($claims) === false) { + // Decoded WITHOUT assoc, then cast at the top level only. `json_decode` + // with $associative=true erases the difference between a JSON array and + // a JSON object, so `"aud": {"target": "keepiq"}` would arrive as a PHP + // array indistinguishable from `["keepiq"]` and match on its values. + // array_is_list() does not recover it either: `{"0": "keepiq"}` decodes + // to a list. Keeping nested objects as stdClass is what lets a claim + // check reject one. + $decoded = json_decode($payloadRaw); + if (is_object($decoded) === false) { throw new RuntimeException(message: 'Assertion payload is not a JSON object'); } + $claims = (array)$decoded; + // Required claims. foreach (['iss', 'aud', 'exp', 'iat', 'jti'] as $required) { if (array_key_exists($required, $claims) === false) { @@ -172,9 +182,9 @@ private function readAssertionClaims(JWS $jws): array { private function assertClaimsAcceptable(array $claims): void { $now = time(); - if ((string)$claims['aud'] !== JwtAuthService::EXPECTED_AUDIENCE) { - throw new RuntimeException(message: 'Wrong audience'); - } + // The audience is asserted by AudiencePolicy, from JwtAuthService: which + // values this deployment answers to is a published contract decision, + // not a property of a well-formed JWS. if ((int)$claims['exp'] <= $now) { throw new RuntimeException(message: 'Assertion expired'); diff --git a/lib/Service/JwtAuthService.php b/lib/Service/JwtAuthService.php index 2b155bea5..b420288be 100644 --- a/lib/Service/JwtAuthService.php +++ b/lib/Service/JwtAuthService.php @@ -25,6 +25,7 @@ namespace OCA\Keepiq\Service; +use OCA\Keepiq\AppInfo\Application as KeepiqApp; use OCA\Keepiq\Db\Application; use OCA\Keepiq\Db\ApplicationMapper; use OCA\Keepiq\Event\Audit\AuditEvent; @@ -33,6 +34,7 @@ use OCP\AppFramework\Db\DoesNotExistException; use OCP\EventDispatcher\IEventDispatcher; use OCP\ICacheFactory; +use Psr\Log\NullLogger; use RuntimeException; /** @@ -87,28 +89,6 @@ class JwtAuthService { */ public const CLOCK_SKEW_SECONDS = 60; - /** - * The expected audience claim ("aud") for assertions targeted at - * this Keepiq instance. - * - * DELIBERATELY STILL `doriath` AFTER THE RENAME. This value is not an - * app id, it is a published authentication parameter: every registered - * application signs `aud=doriath` into its RS256 assertion with a - * private key this server does not hold and cannot re-sign. Changing - * the expected audience would reject every existing application's - * assertion with an opaque 400 — a fleet-wide credential outage that - * no repair step can heal, because the fix lives in each consumer's - * configuration. - * - * The value is advertised in the `.well-known` discovery document, so a - * self-configuring consumer reads it rather than hardcoding it; rolling - * it to `keepiq` is a coordinated cross-app change (a new `apiVersion` - * per openspec/specs/secret-store-api/spec.md), not part of an app-id - * rename. - * - * @var string - */ - public const EXPECTED_AUDIENCE = 'doriath'; /** * Constructor for JwtAuthService. @@ -117,6 +97,7 @@ class JwtAuthService { * @param ICacheFactory $cacheFactory The cache factory * @param JwtAssertionVerifier $verifier The JOSE assertion verifier * @param ApplicationJwkResolver $keyResolver The issuer key resolver + * @param AudiencePolicy $audiencePolicy Which `aud` values name this instance * @param IEventDispatcher|null $eventDispatcher The event dispatcher * @param AuditEventFactory $auditEvents The audit-event factory * @@ -127,6 +108,7 @@ public function __construct( private ICacheFactory $cacheFactory, private JwtAssertionVerifier $verifier, private ApplicationJwkResolver $keyResolver, + private AudiencePolicy $audiencePolicy = new AudiencePolicy(new NullLogger()), private ?IEventDispatcher $eventDispatcher = null, private AuditEventFactory $auditEvents = new AuditEventFactory(), ) { @@ -209,6 +191,11 @@ public function verifyAssertion(string $assertion): Application { $claims = $this->verifier->readAcceptableClaims(assertion: $assertion); + // Audience is asserted here rather than inside the verifier: which + // values this deployment answers to, and for how much longer, is a + // published contract decision, not a property of a well-formed JWS. + $usesDeprecated = $this->audiencePolicy->assertNamesThisInstance(claims: $claims); + $jtiCache = $this->cacheFactory->createDistributed(self::JTI_CACHE_NS); $jti = (string)$claims['jti']; if ($jtiCache->hasKey($jti) === true) { @@ -225,6 +212,14 @@ public function verifyAssertion(string $assertion): Application { // Store jti to prevent replay during max assertion lifetime. $jtiCache->set($jti, true, self::ACCESS_TOKEN_TTL); + // Reported only now: everything above can reject, and an issuer named + // by a rejected assertion is unverified. Warning earlier would let a + // forged or replayed assertion manufacture migration traffic for any + // issuer it cared to name. + if ($usesDeprecated === true) { + $this->audiencePolicy->reportDeprecatedUse(claims: $claims); + } + return $application; }//end verifyAssertion() diff --git a/lib/Service/MachineSecretEnvelopeService.php b/lib/Service/MachineSecretEnvelopeService.php index 433fc9e95..38345b784 100644 --- a/lib/Service/MachineSecretEnvelopeService.php +++ b/lib/Service/MachineSecretEnvelopeService.php @@ -27,6 +27,7 @@ namespace OCA\Keepiq\Service; +use OCA\Keepiq\AppInfo\Application as KeepiqApp; use OCA\Keepiq\Db\EncryptionSuiteMapper; use OCA\Keepiq\Db\FolderMapper; use OCA\Keepiq\Db\Secret; @@ -44,21 +45,48 @@ class MachineSecretEnvelopeService { /** * The current envelope format identifier. * - * DELIBERATELY STILL `doriath-` AFTER THE doriath -> keepiq RENAME. This - * string is a version tag on a published wire format, not an app id: it - * is advertised in the discovery document's `envelopeFormats`, and every - * machine consumer asserts on it before attempting decryption. Changing - * it in place is precisely what the paragraph above — and - * openspec/specs/secret-store-api/spec.md — forbid: a breaking change to - * the envelope ships as a NEW format identifier under a NEW apiVersion, - * so that a consumer pinned to v1 keeps working instead of silently - * refusing every secret. The v1 envelope's BYTES did not change here, so - * neither may its name. + * STILL `doriath-` AFTER THE doriath -> keepiq RENAME, and it stays that + * way until the first stable release. This string is a version tag on a published + * wire format, not an app id: it is advertised in the discovery + * document's `envelopeFormats`, and every machine consumer asserts on it + * before attempting decryption. + * + * IT CANNOT BE DUAL-VALUED THE WAY THE AUDIENCE AND THE DISCOVERY PATH + * CAN. Those are inbound — the consumer offers a value and this server + * decides whether to honour it, so accepting a second one costs nothing + * and no consumer notices. This is outbound: exactly one string goes into + * the envelope's `format` field, and whichever one it is, every consumer + * pinned to the other rejects the secret. There is no server-side change + * that makes a flip safe. + * + * So the compatibility work is on the consumer, and the only thing this + * server can usefully do is say what is coming. UPCOMING_FORMAT is + * published in discovery so consumers can be updated to accept BOTH names + * ahead of time; once they do, the switch is a non-event. + * The v1 envelope's BYTES do not change with the name — the successor + * differs in identifier only. * * @var string */ public const FORMAT = 'doriath-machine-secret-v1'; + /** + * The format identifier that replaces FORMAT at the first stable release. + * + * Published, not emitted. Nothing writes this value yet; it exists so a + * consumer can be taught to accept it before it starts arriving. + * + * @var string + */ + public const UPCOMING_FORMAT = 'keepiq-machine-secret-v1'; + + /** + * The app version in which UPCOMING_FORMAT starts being emitted. + * + * @var string + */ + public const UPCOMING_FORMAT_APP_VERSION = KeepiqApp::PRE_STABLE_COMPAT_REMOVED_IN; + /** * The encryption scheme identifier naming the existing ADR-003 path * (RSA-OAEP-SHA256 with 512-byte block chunking — see EncryptService). diff --git a/openspec/specs/application-mgmt/spec.md b/openspec/specs/application-mgmt/spec.md index 4113a4dbc..1c7a2d2f7 100644 --- a/openspec/specs/application-mgmt/spec.md +++ b/openspec/specs/application-mgmt/spec.md @@ -155,7 +155,7 @@ The listing MUST NOT render a request's full token, and MUST NOT expose any subm - **THEN** only requests they created MUST be returned, exactly as before #### Scenario: An administrator revokes a circulating fill link -@e2e exclude Driven by SecretRequestServiceTest::testAdminRevokeDeletesTheUnfilledApplicationPlaceholder, ::testAdminRevokeNeverDeletesAFilledApplicationSecret, ::testAdminRevokeWillNotDeleteAnotherApplicationsSecret and ::testRevokeForApplicationRefusesARequestOfAnotherActor (which fails when the created_by check is removed), plus the vitest "asks before revoking, and revokes through the application endpoint". NOT verified live: doing so would hard-delete a seeded placeholder Secret on the development instance, and the request row cannot be restored. +@e2e exclude Driven by ApplicationRequestAdminServiceTest::testAdminRevokeDeletesTheUnfilledApplicationPlaceholder, ::testAdminRevokeNeverDeletesAFilledApplicationSecret, ::testAdminRevokeWillNotDeleteAnotherApplicationsSecret and ::testRevokeForApplicationRefusesARequestOfAnotherActor (which fails when the created_by check is removed), plus the vitest "asks before revoking, and revokes through the application endpoint". NOT verified live: doing so would hard-delete a seeded placeholder Secret on the development instance, and the request row cannot be restored. - **GIVEN** an application has a pending request whose link is in circulation - **WHEN** an administrator revokes it - **THEN** the token MUST stop being fillable diff --git a/openspec/specs/secret-store-api/spec.md b/openspec/specs/secret-store-api/spec.md index 5259bd0db..a9a93bad0 100644 --- a/openspec/specs/secret-store-api/spec.md +++ b/openspec/specs/secret-store-api/spec.md @@ -4,7 +4,7 @@ TBD - created by archiving change openconnector-secret-store-api. Update Purpose after archive. ## Requirements ### Requirement: Machine API Discovery Document -The system MUST serve an unauthenticated, machine-readable discovery document at `GET /api/v1/app/.well-known/doriath` declaring the API version, token endpoint, supported grant type (`urn:ietf:params:oauth:grant-type:jwt-bearer`), assertion requirements (algorithm, maximum lifetime, audience), the secret endpoint paths (list, by-id, by-name), and the supported envelope formats. The document MUST contain no instance-private data. Breaking changes to addressing or envelope shape MUST be published as a new API version in this document, never as an in-place mutation of an existing version. +The system MUST serve an unauthenticated, machine-readable discovery document declaring the API version, token endpoint, supported grant type (`urn:ietf:params:oauth:grant-type:jwt-bearer`), assertion requirements (algorithm, maximum lifetime, audience), the secret endpoint paths (list, by-id, by-name), and the supported envelope formats. The canonical location is `GET /api/v1/app/.well-known/keepiq`. The identical document MUST also be served at the pre-rename path `GET /api/v1/app/.well-known/doriath`, until that path is removed before the first stable release. The document MUST contain no instance-private data. Breaking changes to addressing or envelope shape MUST be published as a new API version in this document, never as an in-place mutation of an existing version. #### Scenario: Consumer bootstraps from the base URL alone @e2e exclude Machine-to-machine API contract with no UI surface; covered by DiscoveryControllerTest (document shape, no instance-private data) and the machine-secret-api Newman collection's discovery group. @@ -76,6 +76,121 @@ The system MUST allow an authenticated application to create and update secrets ### Requirement: Token Endpoint Hardening The token endpoint MUST verify the JWT assertion's signature against the application's registered certificate, reject assertions with a lifetime over 300 seconds or an expired/future validity window, and reject any reuse of a `jti` within the assertion's lifetime (replay protection). Failed exchanges MUST be subject to Nextcloud brute-force throttling. Applications that are pending, rejected, deleted, or whose EncryptionSuite is revoked or compromised MUST be refused a token. Issued bearer tokens MUST be opaque, expire within 5 minutes, and grant access to exactly one application's vault. +### Requirement: Discovery Path +The discovery document MUST be served at both the canonical path +`/api/v1/app/.well-known/keepiq` and the pre-rename path +`/api/v1/app/.well-known/doriath`, returning identical content from each. The +document MUST publish the canonical path as `discoveryPath` and each deprecated +path with the apiVersion retiring it as +`deprecatedDiscoveryPaths[].removedInAppVersion`, so a consumer configured with +the old path can re-point itself without coordination. This is the one URL a +consumer holds by hand; everything else it uses is derived from the document. + +Serving the deprecated path MUST be removed before the first stable release (app version 1.0.0). Until then, every +fetch on it MUST be logged, so the set of consumers still to migrate is +observable rather than assumed. + +#### Scenario: Both discovery paths serve the same document +@e2e exclude Machine-to-machine API contract with no UI surface; covered by DiscoveryControllerTest. +- **WHEN** the document is fetched from the canonical and the deprecated path +- **THEN** both MUST return 200 with identical content + +### Requirement: Envelope Format Succession +The envelope `format` identifier is written by the server and asserted on by the +consumer, so unlike an inbound value it cannot be dual-valued: exactly one string +is emitted and any consumer pinned to a different one rejects the secret. The +system MUST therefore keep emitting `doriath-machine-secret-v1` until the first stable +release, and MUST publish its successor in the discovery document as +`upcomingEnvelopeFormats[]` with `value`, `replaces` and `emittedFromAppVersion`, +so consumers can be taught to accept both names before the switch. The successor +MUST NOT be listed in `envelopeFormats`, which declares only what is actually +emitted. The envelope bytes MUST NOT change with the identifier. + +#### Scenario: Successor announced but not emitted +@e2e exclude Machine-to-machine API contract with no UI surface; covered by DiscoveryControllerTest and MachineSecretEnvelopeServiceTest. +- **WHEN** the discovery document is fetched before the first stable release +- **THEN** `envelopeFormats` MUST contain only `doriath-machine-secret-v1` +- **AND** `upcomingEnvelopeFormats` MUST announce `keepiq-machine-secret-v1` for app version 1.0.0 + +### Requirement: Assertion Audience +The token endpoint MUST accept an assertion whose `aud` claim names this +instance, honouring RFC 7519 §4.1.3: `aud` MAY be a single string or an array +of strings, and the assertion is acceptable when ANY presented value is one the +instance accepts. + +The claim MUST be read strictly and MUST NOT be coerced. A value that is not a +string, and an array containing any member that is not a non-empty string, are +malformed and MUST be rejected — an array MUST NOT be filtered down to its +well-formed members, because authenticating on the remainder is exactly what a +malformed claim must not achieve. + +An OBJECT-valued claim MUST be rejected, and the distinction between a JSON +array and a JSON object MUST survive decoding for that to be possible: decoding +an object into a keyed map makes `{"target": "keepiq"}` indistinguishable from +`["keepiq"]`, and a list check does not recover it, since `{"0": "keepiq"}` +decodes to a list. + +Use of a deprecated audience MUST be reported only after the assertion is fully +authenticated — signature, issuer and replay checks all passed. The report +names an issuer, and before authentication that issuer is a string the caller +chose; reporting earlier would let an unauthenticated or replayed assertion +manufacture migration traffic for any issuer it named, and the log exists +precisely to decide when the deprecated value can be withdrawn. The instance MUST accept both `keepiq` (canonical) and +`doriath` (deprecated, the pre-rename name), and MUST reject any other value. + +The discovery document MUST publish the canonical value as `audience`, the full +accepted set as `acceptedAudiences`, and each deprecated value together with the +apiVersion that retires it as `deprecatedAudiences[].removedInAppVersion`. This +is additive and therefore valid within the current apiVersion: an existing +consumer is unaffected, and a self-configuring consumer converges on the +canonical value without coordination. + +Accepting `doriath` MUST be removed before the first stable release (app version +1.0.0), alongside the `doriath-machine-secret-v1` envelope name and the +`.well-known/doriath` path. These are pre-stable shims, not a released contract: +the rule below that breaking changes ship as a new apiVersion binds from the +first stable release onward, and `apiVersion` stays at 1 through their removal. +Until then, every assertion accepted on a deprecated value MUST be logged with +its `iss`, so the set of consumers still to migrate is observable rather than +assumed. + +#### Scenario: Canonical audience accepted +@e2e exclude Machine-to-machine API contract with no UI surface; covered by JwtAuthServiceTest. +- **WHEN** an assertion presents `aud: "keepiq"` +- **THEN** the audience check MUST pass and nothing MUST be logged as deprecated + +#### Scenario: Deprecated audience accepted and reported +@e2e exclude Machine-to-machine API contract with no UI surface; covered by JwtAuthServiceTest. +- **WHEN** an assertion presents `aud: "doriath"` +- **THEN** the audience check MUST pass +- **AND** a warning naming the assertion's `iss` and the removing app version MUST be logged + +#### Scenario: Array-valued audience accepted +@e2e exclude Machine-to-machine API contract with no UI surface; covered by JwtAuthServiceTest. +- **WHEN** an assertion presents `aud: ["something-else", "keepiq"]` +- **THEN** the audience check MUST pass + +#### Scenario: A malformed audience is rejected, not coerced +@e2e exclude Machine-to-machine API contract with no UI surface; covered by JwtAuthServiceTest. +- **WHEN** an assertion presents `aud: 123`, `aud: true` or `aud: ["keepiq", 123]` +- **THEN** the exchange MUST be rejected +- **AND** the well-formed members of a mixed array MUST NOT be matched against the accepted set + +#### Scenario: An object-valued audience is rejected +@e2e exclude Machine-to-machine API contract with no UI surface; covered by JwtAuthServiceTest. +- **WHEN** an assertion presents `aud: {"target": "keepiq"}` or `aud: {"0": "keepiq"}` +- **THEN** the exchange MUST be rejected, even though an accepted value appears among the object's values + +#### Scenario: A rejected assertion is never reported as a migration +@e2e exclude Machine-to-machine API contract with no UI surface; covered by JwtAuthServiceTest. +- **WHEN** an assertion carrying a deprecated audience fails signature verification or replays a `jti` +- **THEN** no deprecation warning MUST be emitted for its issuer + +#### Scenario: Foreign audience rejected +@e2e exclude Machine-to-machine API contract with no UI surface; covered by JwtAuthServiceTest. +- **WHEN** an assertion presents an `aud` naming neither accepted value +- **THEN** the exchange MUST be rejected + #### Scenario: Replayed assertion rejected @e2e exclude Machine-to-machine API contract with no UI surface; covered by JwtAuthServiceTest (jti replay) and the Newman token negative cases. - **WHEN** the same signed assertion (same `jti`) is presented twice within its lifetime diff --git a/tests/Unit/AppInfo/PreStableCompatDeadlineTest.php b/tests/Unit/AppInfo/PreStableCompatDeadlineTest.php new file mode 100644 index 000000000..9fc17e0e8 --- /dev/null +++ b/tests/Unit/AppInfo/PreStableCompatDeadlineTest.php @@ -0,0 +1,176 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + */ + +declare(strict_types=1); + +namespace OCA\Keepiq\Tests\Unit\AppInfo; + +use OCA\Keepiq\AppInfo\Application; +use OCA\Keepiq\Controller\DiscoveryController; +use OCA\Keepiq\Service\AudiencePolicy; +use OCA\Keepiq\Service\MachineSecretEnvelopeService; +use PHPUnit\Framework\TestCase; + +/** + * Fails the build once the shims outlive their stated deadline. + */ +class PreStableCompatDeadlineTest extends TestCase { + + /** + * The app version declared in appinfo/info.xml. + * + * @return string The version. + */ + private function appVersion(): string { + // Read as text rather than through simplexml: the parser behaves + // differently under the suite's error handling, and one element does + // not justify depending on that. + $path = dirname(__DIR__, 3) . '/appinfo/info.xml'; + $xml = file_get_contents($path); + + $this->assertIsString($xml, 'appinfo/info.xml must be readable at ' . $path); + $this->assertSame( + 1, + preg_match('#([^<]+)#', $xml, $matches), + 'appinfo/info.xml must declare a ' + ); + + return trim($matches[1]); + }//end appVersion() + + /** + * The shims still present in the codebase, named for a failure message. + * + * Each entry is a thing that must not exist once the deadline is reached. + * + * @return string[] The surviving shims. + */ + private function survivingShims(): array { + $surviving = []; + + if (defined(AudiencePolicy::class . '::DEPRECATED_AUDIENCE') === true) { + $surviving[] = sprintf( + 'AudiencePolicy::DEPRECATED_AUDIENCE (%s is still accepted on the token endpoint)', + AudiencePolicy::DEPRECATED_AUDIENCE + ); + } + + if (defined(DiscoveryController::class . '::DEPRECATED_DISCOVERY_PATH') === true) { + $surviving[] = sprintf( + 'DiscoveryController::DEPRECATED_DISCOVERY_PATH (%s is still served)', + DiscoveryController::DEPRECATED_DISCOVERY_PATH + ); + } + + if (str_starts_with(MachineSecretEnvelopeService::FORMAT, 'doriath-') === true) { + $surviving[] = sprintf( + 'MachineSecretEnvelopeService::FORMAT (still emitting %s instead of %s)', + MachineSecretEnvelopeService::FORMAT, + MachineSecretEnvelopeService::UPCOMING_FORMAT + ); + } + + return $surviving; + }//end survivingShims() + + /** + * The shims are gone by the version that says they will be. + * + * Below the deadline this asserts the opposite — that the shims are still + * findable — so the test cannot quietly stop guarding anything if a + * constant is renamed out from under it. A deadline test that silently + * matches nothing is worse than no deadline test. + * + * @return void + */ + public function testTheShimsAreGoneByTheirStatedDeadline(): void { + $version = $this->appVersion(); + $deadline = Application::PRE_STABLE_COMPAT_REMOVED_IN; + $surviving = $this->survivingShims(); + + if (version_compare($version, $deadline, '>=') === true) { + $this->assertSame( + [], + $surviving, + sprintf( + "App version %s has reached the declared removal version %s, so these must go:\n - %s\n\n" + . 'Each is published to consumers as removed at %s. Shipping past it makes the discovery ' + . 'document untrue and leaves a deprecated audience accepted on the token endpoint.', + $version, + $deadline, + implode("\n - ", $surviving), + $deadline + ) + ); + + return; + } + + $this->assertNotSame( + [], + $surviving, + sprintf( + 'This test guards three shims and can no longer find any of them, while the app is still at ' + . '%s (below %s). Either they were removed early - in which case delete this test and the ' + . 'deadline constant - or they were renamed and this guard now watches nothing.', + $version, + $deadline + ) + ); + }//end testTheShimsAreGoneByTheirStatedDeadline() + + /** + * Everything published as removed at the deadline names the same version. + * + * Three constants feed three separate fields of the discovery document. If + * they drift, consumers are told different removal dates for shims that go + * together. + * + * @return void + */ + public function testEveryPublishedDeadlineNamesTheSameVersion(): void { + $this->assertSame( + Application::PRE_STABLE_COMPAT_REMOVED_IN, + AudiencePolicy::DEPRECATED_AUDIENCE_REMOVED_IN, + 'the audience deadline must not drift from the shared one' + ); + $this->assertSame( + Application::PRE_STABLE_COMPAT_REMOVED_IN, + DiscoveryController::DEPRECATED_PATH_REMOVED_IN, + 'the discovery-path deadline must not drift from the shared one' + ); + $this->assertSame( + Application::PRE_STABLE_COMPAT_REMOVED_IN, + MachineSecretEnvelopeService::UPCOMING_FORMAT_APP_VERSION, + 'the envelope-format deadline must not drift from the shared one' + ); + }//end testEveryPublishedDeadlineNamesTheSameVersion() +}//end class diff --git a/tests/Unit/Controller/DiscoveryControllerTest.php b/tests/Unit/Controller/DiscoveryControllerTest.php index 2b2be1ae1..40441dc00 100644 --- a/tests/Unit/Controller/DiscoveryControllerTest.php +++ b/tests/Unit/Controller/DiscoveryControllerTest.php @@ -32,6 +32,13 @@ class DiscoveryControllerTest extends TestCase { private DiscoveryController $controller; + /** + * Mocked logger, for the deprecated-path warning. + * + * @var \Psr\Log\LoggerInterface&\PHPUnit\Framework\MockObject\MockObject + */ + private $logger; + /** * Wire the controller with a URL generator that echoes route names. * @@ -53,7 +60,13 @@ static function (string $route): string { static fn (string $p) => 'https://nc.test' . $p ); - $this->controller = new DiscoveryController(request: $request, urlGenerator: $url); + $this->logger = $this->createMock(\Psr\Log\LoggerInterface::class); + $this->controller = new DiscoveryController( + request: $request, + urlGenerator: $url, + appConfig: null, + logger: $this->logger, + ); }//end setUp() /** @@ -88,4 +101,117 @@ public function testNoInstancePrivateData(): void { $this->assertStringNotContainsString($needle, $flat); } }//end testNoInstancePrivateData() + /** + * Both discovery paths return the identical document. + * + * The deprecated path is the one URL consumers hold by hand, so it must + * keep working byte-for-byte until it is removed before the first stable release. + * + * @return void + */ + public function testDeprecatedPathServesTheIdenticalDocument(): void { + $canonical = $this->controller->document()->getData(); + $legacy = $this->controller->legacyDocument()->getData(); + + $this->assertSame($canonical, $legacy); + }//end testDeprecatedPathServesTheIdenticalDocument() + + /** + * The document names the canonical path and the deprecated one. + * + * @return void + */ + public function testDocumentAdvertisesDiscoveryPaths(): void { + $data = $this->controller->document()->getData(); + + $this->assertSame('/api/v1/app/.well-known/keepiq', $data['discoveryPath']); + $this->assertSame( + [['value' => '/api/v1/app/.well-known/doriath', 'removedInAppVersion' => '1.0.0']], + $data['deprecatedDiscoveryPaths'] + ); + }//end testDocumentAdvertisesDiscoveryPaths() + + /** + * The document publishes the audience contract the spec requires. + * + * Token ACCEPTANCE is tested thoroughly in JwtAuthServiceTest, but what a + * consumer is TOLD to send is a separate surface: a swapped constant or a + * dropped field would break self-configuring clients while every + * acceptance test stayed green. + * + * @return void + */ + public function testDocumentPublishesTheAudienceContract(): void { + $assertion = $this->controller->document()->getData()['assertion']; + + $this->assertSame('keepiq', $assertion['audience'], 'the value a consumer should send'); + $this->assertSame( + ['keepiq', 'doriath'], + $assertion['acceptedAudiences'], + 'both values are honoured until the deprecated one is removed' + ); + $this->assertSame( + [['value' => 'doriath', 'removedInAppVersion' => '1.0.0']], + $assertion['deprecatedAudiences'], + 'the deprecated value must be published with the version that retires it' + ); + }//end testDocumentPublishesTheAudienceContract() + + /** + * The canonical audience is not also listed as deprecated. + * + * A copy-paste swapping the two would tell every consumer to migrate away + * from the value they should be adopting, and read as plausible. + * + * @return void + */ + public function testTheCanonicalAudienceIsNotAlsoDeprecated(): void { + $assertion = $this->controller->document()->getData()['assertion']; + $deprecated = array_column($assertion['deprecatedAudiences'], 'value'); + + $this->assertContains($assertion['audience'], $assertion['acceptedAudiences']); + $this->assertNotContains($assertion['audience'], $deprecated); + }//end testTheCanonicalAudienceIsNotAlsoDeprecated() + + /** + * Fetching the deprecated path reports it, naming the retiring version. + * + * @return void + */ + public function testDeprecatedPathIsReported(): void { + $context = []; + $this->logger->method('warning')->willReturnCallback( + static function (string $message, array $ctx = []) use (&$context): void { + $context = $ctx; + } + ); + + $this->controller->legacyDocument(); + + $this->assertSame('/api/v1/app/.well-known/doriath', $context['deprecated'] ?? null); + $this->assertSame('/api/v1/app/.well-known/keepiq', $context['canonical'] ?? null); + $this->assertSame('1.0.0', $context['version'] ?? null); + }//end testDeprecatedPathIsReported() + + /** + * The successor envelope format is announced but not yet emitted. + * + * `envelopeFormats` declares what actually goes on the wire; listing a + * format nothing writes would be a claim a consumer could act on. + * + * @return void + */ + public function testUpcomingEnvelopeFormatIsAnnouncedNotEmitted(): void { + $data = $this->controller->document()->getData(); + + $this->assertSame(['doriath-machine-secret-v1'], $data['envelopeFormats']); + $this->assertSame( + [[ + 'value' => 'keepiq-machine-secret-v1', + 'replaces' => 'doriath-machine-secret-v1', + 'emittedFromAppVersion' => '1.0.0', + ]], + $data['upcomingEnvelopeFormats'] + ); + }//end testUpcomingEnvelopeFormatIsAnnouncedNotEmitted() }//end class diff --git a/tests/Unit/Service/JwtAuthServiceTest.php b/tests/Unit/Service/JwtAuthServiceTest.php index 9131522c4..03eacd1a2 100644 --- a/tests/Unit/Service/JwtAuthServiceTest.php +++ b/tests/Unit/Service/JwtAuthServiceTest.php @@ -30,6 +30,7 @@ use OCA\Keepiq\Db\EncryptionSuite; use OCA\Keepiq\Db\EncryptionSuiteMapper; use OCA\Keepiq\Service\ApplicationJwkResolver; +use OCA\Keepiq\Service\AudiencePolicy; use OCA\Keepiq\Service\JwtAssertionVerifier; use OCA\Keepiq\Service\JwtAuthService; use OCP\AppFramework\Db\DoesNotExistException; @@ -114,6 +115,7 @@ protected function setUp(): void { cacheFactory: $this->cacheFactory, verifier: new JwtAssertionVerifier(logger: $this->logger), keyResolver: new ApplicationJwkResolver(suiteMapper: $this->suiteMapper), + audiencePolicy: new AudiencePolicy(logger: $this->logger), ); }//end setUp() @@ -339,6 +341,292 @@ public function testWrongAudienceRejected(): void { $this->service->exchangeAssertion($assertion); }//end testWrongAudienceRejected() + /** + * The canonical audience is accepted and raises no deprecation warning. + * + * @return void + */ + public function testCanonicalAudienceAccepted(): void { + $this->stubActiveApp('app-1'); + + $warnings = []; + $this->logger->method('warning')->willReturnCallback( + static function (string $message) use (&$warnings): void { + $warnings[] = $message; + } + ); + + $now = time(); + $assertion = $this->buildAssertion( + [ + 'iss' => 'app-1', + 'aud' => 'keepiq', + 'iat' => $now, + 'exp' => ($now + 60), + 'jti' => 'jti-aud-canonical', + ] + ); + + $result = $this->service->exchangeAssertion($assertion); + + $this->assertSame('Bearer', $result['token_type']); + $this->assertSame([], $warnings, 'the canonical audience must not be reported as deprecated'); + }//end testCanonicalAudienceAccepted() + + /** + * The pre-rename audience still works, and is reported with its issuer. + * + * Rejecting it would be a fleet-wide credential outage; accepting it + * silently would leave nobody knowing who still has to migrate before + * the value is retired. + * + * @return void + */ + public function testDeprecatedAudienceAcceptedAndReported(): void { + $this->stubActiveApp('app-1'); + + $context = []; + $this->logger->method('warning')->willReturnCallback( + static function (string $message, array $ctx = []) use (&$context): void { + $context = $ctx; + } + ); + + $now = time(); + $assertion = $this->buildAssertion( + [ + 'iss' => 'app-1', + 'aud' => 'doriath', + 'iat' => $now, + 'exp' => ($now + 60), + 'jti' => 'jti-aud-deprecated', + ] + ); + + $result = $this->service->exchangeAssertion($assertion); + + $this->assertSame('Bearer', $result['token_type'], 'the deprecated audience must still exchange'); + $this->assertSame('app-1', $context['iss'] ?? null, 'the warning must name the issuer still to migrate'); + $this->assertSame('doriath', $context['deprecated'] ?? null); + $this->assertSame('1.0.0', $context['version'] ?? null, 'the warning must name the removing app version'); + }//end testDeprecatedAudienceAcceptedAndReported() + + /** + * An array-valued `aud` is accepted when any member names this instance. + * + * RFC 7519 §4.1.3 permits `aud` to be an array; a conformant client + * sending one used to be rejected outright. + * + * @return void + */ + public function testArrayValuedAudienceAccepted(): void { + $this->stubActiveApp('app-1'); + + $now = time(); + $assertion = $this->buildAssertion( + [ + 'iss' => 'app-1', + 'aud' => ['someoneelse', 'keepiq'], + 'iat' => $now, + 'exp' => ($now + 60), + 'jti' => 'jti-aud-array', + ] + ); + + $result = $this->service->exchangeAssertion($assertion); + + $this->assertSame('Bearer', $result['token_type']); + }//end testArrayValuedAudienceAccepted() + + /** + * An array-valued `aud` naming only foreign audiences is rejected. + * + * @return void + */ + public function testArrayValuedForeignAudienceRejected(): void { + $this->stubActiveApp('app-1'); + + $now = time(); + $assertion = $this->buildAssertion( + [ + 'iss' => 'app-1', + 'aud' => ['someoneelse', 'anotherapp'], + 'iat' => $now, + 'exp' => ($now + 60), + 'jti' => 'jti-aud-array-foreign', + ] + ); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('Wrong audience'); + $this->service->exchangeAssertion($assertion); + }//end testArrayValuedForeignAudienceRejected() + + /** + * A non-string audience is malformed, not an audience written oddly. + * + * Casting would turn `123` into "123" and `true` into "1" before the + * comparison. No accepted audience is numeric today, so nothing would have + * slipped through — but that is a fact about configuration, not a property + * of the check. + * + * @param mixed $aud The malformed claim value. + * + * @dataProvider malformedAudiences + * + * @return void + */ + public function testNonStringAudienceIsRejected(mixed $aud): void { + $this->stubActiveApp('app-1'); + + $now = time(); + $assertion = $this->buildAssertion( + [ + 'iss' => 'app-1', + 'aud' => $aud, + 'iat' => $now, + 'exp' => ($now + 60), + 'jti' => 'jti-aud-' . md5(serialize($aud)), + ] + ); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('Wrong audience'); + $this->service->exchangeAssertion($assertion); + }//end testNonStringAudienceIsRejected() + + /** + * A rejected assertion never produces a migration warning. + * + * The deprecation log is the checklist for deciding when `doriath` can stop + * being accepted, so it has to contain only issuers that actually + * authenticated. Warning before signature, issuer and replay checks let any + * unauthenticated caller manufacture traffic for any issuer it named. + * + * @return void + */ + public function testAReplayedDeprecatedAudienceAssertionIsNotReported(): void { + $this->stubActiveApp('app-1'); + + $warnings = []; + $this->logger->method('warning')->willReturnCallback( + static function (string $message) use (&$warnings): void { + $warnings[] = $message; + } + ); + + $now = time(); + $claims = [ + 'iss' => 'app-1', + 'aud' => 'doriath', + 'iat' => $now, + 'exp' => ($now + 60), + 'jti' => 'jti-replay-deprecated', + ]; + + // First exchange succeeds and legitimately reports. + $this->service->exchangeAssertion($this->buildAssertion($claims)); + $this->assertCount(1, $warnings, 'an authenticated deprecated-audience exchange is reported'); + + $warnings = []; + + try { + $this->service->exchangeAssertion($this->buildAssertion($claims)); + $this->fail('the replayed assertion should have been rejected'); + } catch (RuntimeException $e) { + $this->assertSame('Assertion jti replayed', $e->getMessage()); + } + + $this->assertSame([], $warnings, 'a replayed assertion must not be reported as a migration'); + }//end testAReplayedDeprecatedAudienceAssertionIsNotReported() + + /** + * A badly signed assertion never produces a migration warning. + * + * Same reasoning as the replay case: the `iss` in a failed exchange is a + * string the caller chose, not one this instance verified. + * + * @return void + */ + public function testABadlySignedDeprecatedAudienceAssertionIsNotReported(): void { + $this->stubActiveApp('app-1'); + + $warnings = []; + $this->logger->method('warning')->willReturnCallback( + static function (string $message) use (&$warnings): void { + $warnings[] = $message; + } + ); + + $now = time(); + $assertion = $this->buildAssertionWithForeignKey( + [ + 'iss' => 'app-1', + 'aud' => 'doriath', + 'iat' => $now, + 'exp' => ($now + 60), + 'jti' => 'jti-badsig-deprecated', + ] + ); + + try { + $this->service->exchangeAssertion($assertion); + $this->fail('the badly signed assertion should have been rejected'); + } catch (RuntimeException) { + // Expected. + } + + $this->assertSame([], $warnings, 'an unverified issuer must not appear in the migration log'); + }//end testABadlySignedDeprecatedAudienceAssertionIsNotReported() + + /** + * Build an assertion signed with a key the application does not hold. + * + * Mirrors what testInvalidSignatureRejected() does inline, so a test that + * needs a well-formed but unauthentic assertion does not have to restate + * the whole builder. + * + * @param array $claims The claim set to sign. + * + * @return string The compact serialization. + */ + private function buildAssertionWithForeignKey(array $claims): string { + $foreignPkey = openssl_pkey_new( + ['private_key_type' => OPENSSL_KEYTYPE_RSA, 'private_key_bits' => 2048] + ); + openssl_pkey_export($foreignPkey, $foreignPem); + + $jws = (new JWSBuilder(new AlgorithmManager([new RS256()])))->create() + ->withPayload((string)json_encode($claims)) + ->addSignature(JWKFactory::createFromKey($foreignPem), ['alg' => 'RS256', 'typ' => 'JWT']) + ->build(); + + return (new CompactSerializer())->serialize($jws, 0); + }//end buildAssertionWithForeignKey() + + /** + * Claim shapes RFC 7519 section 4.1.3 does not permit. + * + * The mixed array is the one that matters: filtering it down to its string + * members would authenticate on the part that happens to parse. + * + * @return array> + */ + public static function malformedAudiences(): array { + return [ + 'integer' => [123], + 'boolean' => [true], + 'float' => [1.5], + 'array with an integer member' => [['keepiq', 123]], + 'array with a nested array' => [['keepiq', ['keepiq']]], + 'array with an empty string' => [['keepiq', '']], + // Objects: json_decode(..., true) would flatten these into arrays + // whose VALUES contain an accepted audience. + 'object naming the audience in a field' => [(object)['target' => 'keepiq']], + 'object with a numeric key' => [(object)['0' => 'keepiq']], + ]; + }//end malformedAudiences() + /** * Replayed jti is rejected on second use. *