Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression: OIDC identity tokens no longer cached when using AuthTokenMiddleware/FetchAuthTokenCache #503

Closed
Daniel-I-Am opened this issue Dec 7, 2023 · 2 comments · Fixed by #510

Comments

@Daniel-I-Am
Copy link

Daniel-I-Am commented Dec 7, 2023

In a recent PR [1] a lot of internals for the AuthTokenMiddleware were refactored. This includes a change to prefer fetcher->updateMetadata() over fetcher->fetchAuthToken() to retrieve the required credentials when the credentials fetcher supports UpdateMetadataInterface [2].

This change seems to have broken the ability to cache OIDC identity tokens. See the reproduction case.

When using a FetchAuthTokenCache credentials fetcher (with GCECredentials as underlying fetcher), the old method used to always cache the result of the underlying fetcher [3]. The method that is now called to get the credentials from the underlying fetcher, has an additional check to ensure the token was not a null-value [4]. For OIDC identity tokens, this will always return NULL, due to never reaching the code to set the lastReceivedToken when dealing with OIDC identity tokens in the underlying credentials fetcher [5].

References:

  1. chore: Refactor Auth Token Middleware's token logic #492
  2. if (!$this->fetcher instanceof UpdateMetadataInterface ||
    ($this->fetcher instanceof FetchAuthTokenCache &&
    !$this->fetcher->getFetcher() instanceof UpdateMetadataInterface)
    ) {
    $token = $this->fetcher->fetchAuthToken();
    $request = $request->withHeader(
    'authorization', 'Bearer ' . ($token['access_token'] ?? $token['id_token'])
    );
    } else {
    $headers = $this->fetcher->updateMetadata($request->getHeaders(), null, $this->httpHandler);
    $request = Utils::modifyRequest($request, ['set_headers' => $headers]);
    }
  3. $auth_token = $this->fetcher->fetchAuthToken($httpHandler);
    $this->saveAuthTokenInCache($auth_token);
  4. if (!$cached && $token = $this->fetcher->getLastReceivedToken()) {
    $this->saveAuthTokenInCache($token, $authUri);
    }
  5. if ($this->targetAudience) {
    return ['id_token' => $response];
    }
    if (null === $json = json_decode($response, true)) {
    throw new \Exception('Invalid JSON response');
    }
    $json['expires_at'] = time() + $json['expires_in'];
    // store this so we can retrieve it later
    $this->lastReceivedToken = $json;

Environment details

  • OS: Linux any distro on a GCE VM Instance
  • PHP version: 8.2.12
  • Package name and version: google/auth:1.33.0 compared to google/auth:1.32.1

Steps to reproduce

  1. Use AuthTokenMiddleware with a FetchAuthTokenCache.
  2. Call the middleware to fetch an identity token for authentication to (for instance) a Cloud Run service, which requires an OIDC identity token.
  3. Notice that the cache is not filled with the OIDC identity token in version 1.33.0, but it is a cache entry on 1.32.1

Code example

This is run on a GCE VM instance, where the metadata server is available

Install dependencies for convenience: composer require guzzlehttp/guzzle symfony/cache google/auth:1.33.0

<?php

// Uses guzzlehttp/guzzle for PSR-7 compatible request
// Uses symfony/cache for in-memory cache

use Google\Auth\Credentials\GCECredentials;
use Google\Auth\FetchAuthTokenCache;
use Google\Auth\GCECache;
use Google\Auth\Middleware\AuthTokenMiddleware;
use GuzzleHttp\Psr7\Request;
use Symfony\Component\Cache\Adapter\ArrayAdapter;

require_once __DIR__ . '/vendor/autoload.php';

$cache = new ArrayAdapter();

$oauthAudience = 'audience';

$cacheConfig = [
    'prefix' => "oidc_{$oauthAudience}_",
];

$onGceCacheConfig = [
    'prefix' => 'gce_',
];

$gce = new GCECredentials(null, null, $oauthAudience);
$gce->setIsOnGce((new GCECache($onGceCacheConfig, $cache))->onGce());
$gceCache = new FetchAuthTokenCache($gce, $cacheConfig, $cache);
$middleware = new AuthTokenMiddleware($gceCache);

$request = new Request('GET', 'https://example.com');

$stack = $middleware(function($request, $options) {
    // Mock handler
});
$stack($request, ['auth' => 'google_auth']);

var_dump($cache->getValues());

The output of this script is the contents of the cache. When running this code on a GCE instance, the outputs are as follows:

Version 1.33.0
array(2) {
  ["gce_google_auth_on_gce_cache"]=>
  string(4) "b:1;"
  ["oidc_audience_GOOGLE_AUTH_PHP_GCE"]=>
  NULL
}
Version 1.32.1

Downgrade this library: composer require google/auth:1.32.1

array(2) {
  ["gce_google_auth_on_gce_cache"]=>
  string(4) "b:1;"
  ["oidc_audience_GOOGLE_AUTH_PHP_GCE"]=>
  string(674) "a:1:{s:8:"id_token";s:644:"eyJhbGciOiJSUzI1NiIsImtpZCI6ImU0YWRmYjQzNmI5ZTE5N2UyZTExMDZhZjJjODQyMjg0ZTQ5ODZhZmYiLCJ0eXAiOiJKV1QifQ.eyJhdWQiOiJhdWRpZW5jZSIsImF6cCI6IjExNzYwOTc1MTk2MjkxMTUyNjY1MSIsImV4cCI6MTcwMTk0MDc0MCwiaWF0IjoxNzAxOTM3MTQwLCJpc3MiOiJodHRwczovL2FjY291bnRzLmdvb2dsZS5jb20iLCJzdWIiOiIxMTc2MDk3NTE5NjI5MTE1MjY2NTEifQ.<redacting signature>";}"
}

My proposal is to update the code of the GCECredentials to also store lastReceivedToken when dealing with an id_token type. If this change is acceptable, I am willing to open a PR for it.

Essentially adjust the following to also set the lastReceivedToken value:

if ($this->targetAudience) {
return ['id_token' => $response];
}

And make the updateMetadata call compatible with the id_token credential type:

// Set the access token in the `Authorization` metadata header so
// the downstream call to updateMetadata know they don't need to
// fetch another token.
if (isset($cached['access_token'])) {
$metadata[self::AUTH_METADATA_KEY] = [
'Bearer ' . $cached['access_token']
];
}

@bshaffer
Copy link
Contributor

cc @yash30201

@bshaffer
Copy link
Contributor

bshaffer commented Dec 18, 2023

@Daniel-I-Am Thank you for taking the time to hunt this down (it's a tricky one).

My proposal is to update the code of the GCECredentials to also store lastReceivedToken when dealing with an id_token type. If this change is acceptable, I am willing to open a PR for it.

This seems great, I agree this is the best course of action

And make the updateMetadata call compatible with the id_token credential type:

This also seems like the right approach.

If you'd like to submit the fix, please do so, as that would be a huge help to us! However, if you would rather us handle it (because of the complicated nature) that's understandable as well.

We will also want to add tests once we fix this, since it's such a tricky regression and we want to make sure it doesn't happen again. If you submit the fix, we can add the tests later. We'll defer to you since you found the bug, and we would of course love your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants