Make use of PKCE verification

[rvmourik]

Hi There,

I am trying to implement the bundle with https://logto.io/, which is an OpenID provider. But when I redirect it through the $oidcClient->generateAuthorizationRedirect(); I instantly get an error while being redirected back to my application with the following error: "Authorization Server policy requires PKCE to be used for this request".

For now it is not an option to disable PKCE on the Logto side, is it possible to use PKCE with this bundle?

Thanks in advance.

[bobvandevijver]

No, this bundle does not support PKCE. It was based on a version of https://github.com/jumbojett/OpenID-Connect-PHP that did not have support for PKCE, if you're wondering why.

I'm open for a pull request to backport PKCE support from that library to this bundle, but I'm afraid I currently do not have the time to work on it myself. Even better would be to refactor both to benefit from each other (basically, see https://github.com/jumbojett/OpenID-Connect-PHP/issues/147)...

[rvmourik]

Hi, I can try if I can make a PR for the backport. I don’t know if I am capable of rewriting the two as one though.

[rvmourik]

Hi @bobvandevijver

I am working on the implementation and have a proof-of-concept working, while working on this I noticed that there is no support for the end_session_endpoint configuration. Would you also accept a PR for this?

The way I want to implement this is by adding the following changes:

OidcClientInterface.php

  /**
   * Create the redirect that should be followed in order to end the current session.
   *
   * @param array<string, string> $additionalQueryParams additional query parameters which will be added to the generated redirect request
   *
   * @throws OidcConfigurationException
   * @throws OidcConfigurationResolveException
   */
  public function generateEndSessionEndpointRedirect(
      OidcTokens $tokens,
      array $additionalQueryParams = [],
  ): RedirectResponse;

The following changes to:

OidcClient.php

  /** {@inheritDoc} */
  public function generateEndSessionEndpointRedirect(
    OidcTokens $tokens,
    array $additionalQueryParams = []
  ): RedirectResponse
  {
      // @todo: read logout_target_url from firewall or override by config
      $data = array_merge($additionalQueryParams, [
          'client_id'     => $this->clientId,
          'id_token_hint' => $tokens->getIdToken(),
          'post_logout_redirect_uri' => '/'
      ]);

      $endpointHasQuery = parse_url($this->getEndSessionEndpoint(), PHP_URL_QUERY);

      return new RedirectResponse(sprintf('%s%s%s', $this->getEndSessionEndpoint(), $endpointHasQuery ? '&' : '?', http_build_query($data)));
  }

  /**
   * @throws OidcConfigurationException
   * @throws OidcConfigurationResolveException
   */
  protected function getEndSessionEndpoint(): string
  {
    return $this->getConfigurationValue('end_session_endpoint');
  }

I will add a new file, which will hook into the Symfony LogoutEvent. That way when Symfony clears the session it will then pass the Token to the LogoutEvent which can be used to pass the recommended idToken to the endSessionEndpoint.

EventListener\LogoutSubscriber.php

class LogoutSubscriber implements EventSubscriberInterface
{
    public function __construct(
        private readonly OidcClientInterface $oidcClient
    ) {
    }

    /**
     * @throws OidcConfigurationException
     * @throws OidcConfigurationResolveException
     * @throws OidcException
     */
    public function onLogout(LogoutEvent $event): void
    {
        $token = $event->getToken();

        if (!$token instanceof TokenInterface) {
            throw new TokenNotFoundException();
        }

        $oidcTokens = $token->getAttribute('auth_data');

        if (!$oidcTokens instanceof OidcTokens) {
            throw new OidcException('Missing token object.');
        }

        $event->setResponse($this->oidcClient->generateEndSessionEndpointRedirect($oidcTokens));
    }

    public static function getSubscribedEvents(): array
    {
        return [LogoutEvent::class => 'onLogout'];
    }
}

I will make the logout functions opt-out through the firewall config with the oidc parameter and the subscriber will not be registered when the logout property is missing from the firewall config. If the user then wants to logout it can access the client directly for getting the RedirectResponse.

If you give the go ahead for implementing this, I will create two separate PR's if that is preferred. If it doesn't matter I will create one PR that contains both.

Let me know.

Robbert

[bobvandevijver]

I noticed that there is no support for the end_session_endpoint configuration. Would you also accept a PR for this?

We might.... We never added it as our IdP doesn't support single sign-out, and we actually consider the implementation broken. See also this article (https://communities.surf.nl/trust-en-identity/artikel/hoe-relevant-is-uitloggen-in-2020), which is in Dutch but that might not be a problem for you :)

I will make the logout functions opt-out through the firewall config with the oidc parameter and the subscriber will not be registered when the logout property is missing from the firewall config. If the user then wants to logout it can access the client directly for getting the RedirectResponse.

Lets not connect it with the logout firewall configuration. Using a parameter on the oidc key would be nice, but lets make it opt-in (because single-signout is fundamentally broken). We will also need a warning on why you should not trust it.

Anyways, please go ahead and open both PRs so we can collect relevant feedback over there :) Also, lets keep them separated, as they are for different functionalities.

[rvmourik]

I noticed that there is no support for the end_session_endpoint configuration. Would you also accept a PR for this?

We might.... We never added it as our IdP doesn't support single sign-out, and we actually consider the implementation broken. See also this article (https://communities.surf.nl/trust-en-identity/artikel/hoe-relevant-is-uitloggen-in-2020), which is in Dutch but that might not be a problem for you :)

I will make the logout functions opt-out through the firewall config with the oidc parameter and the subscriber will not be registered when the logout property is missing from the firewall config. If the user then wants to logout it can access the client directly for getting the RedirectResponse.

Lets not connect it with the logout firewall configuration. Using a parameter on the oidc key would be nice, but lets make it opt-in (because single-signout is fundamentally broken). We will also need a warning on why you should not trust it.

Anyways, please go ahead and open both PRs so we can collect relevant feedback over there :) Also, lets keep them separated, as they are for different functionalities.

Yes, Dutch is not going to be a problem. The article is clarifying regarding the single sing-out. The difference between my implementation and yours is that you provide the login screen your self I guess? I am using a service which provides the login screen (so it depends if they implement the "trust this computer" functionality).

I will start with the PKCE PR and will later add the sign-out PR.

Regarding the sign-out; I will make it opt-in and add the configuration values as a child option of oidc. I do however think that connecting to the LogoutEvent (through a setting) improves de DX for those who want to implement it.

[bobvandevijver]

Single sign out is broken as it cannot be guaranteed you will be signed out with every application you signed onto using the SSO-provider. As example, you use Google to login on service A and service B. Now service A requests the logout and forwards you to logout at Google as well. However, you will still be signed in with service C, unless this service authenticates every request with the SSO provider. But that is not a requirement and also something that is not done by this bundle.

[rvmourik]

What do you think about this; Only adding the getter method for retrieving the end_session_endpoint from the .well-known and the method for generating the URL if the configuration property is present. This way we follow the specification but let the usage of this information to the one implementing it. We can add some documentation with an example of how to do it with a Security Event Listener but do no not provide the actual code from within the bundle?

[bobvandevijver]

I do not oppose adding support for it using the LogoutEvent, I just want a fair readme warning and it to be disabled by default :)

[rvmourik]

I do not oppose adding support for it using the LogoutEvent, I just want a fair readme warning and it to be disabled by default :)

Will do 👍

[rvmourik]

Anyways, please go ahead and open both PRs so we can collect relevant feedback over there :) Also, lets keep them separated, as they are for different functionalities.

First PR is created

1. 24

[bobvandevijver]

Closed with #24