Merge pull request #87 from tft7000/pkce

feat: support PKCE

README.md

@@ -411,7 +411,7 @@ docker run --name ssp-oauth2-dev \
   -e SSP_ENABLED_MODULES="authoauth2" \
   --mount type=bind,source="$(pwd)/docker/config/authsources.php",target=/var/simplesamlphp/config/authsources.php,readonly \
   --mount type=bind,source="$(pwd)/docker/config/config-override.php",target=/var/simplesamlphp/config/config-override.php,readonly \
-  -p 443:443 cirrusid/simplesamlphp:v2.0.0
+  -p 443:443 cirrusid/simplesamlphp:v2.0.7
 ```
 
 and visit (which resolves to localhost, and the docker container) the [test authsource page](https://oauth2-validation.local.stack-dev.cirrusidentity.com/simplesaml/module.php/admin/test)

composer.json

@@ -11,7 +11,7 @@
   "require": {
     "php": ">=7.4 || ^8.0",
     "simplesamlphp/composer-module-installer": "^1.1",
-    "league/oauth2-client": "^2.6",
+    "league/oauth2-client": "^2.7",
     "simplesamlphp/simplesamlphp": "^v2.0.0",
     "firebase/php-jwt": "^5.5|^6",
     "kevinrob/guzzle-cache-middleware": "^4.1.1",

docker/config/authsources.php

@@ -57,6 +57,18 @@ $config = array(
         'clientSecret' => 'GXc8Q~mgI7kTBllrvpBthUEioeARdjrRYORSyda4',
     ],
 
+    'microsoftOIDCPkceSource' => [
+        'authoauth2:OpenIDConnect',
+        'issuer' => 'https://sts.windows.net/{tenantid}/',
+        // When using the 'common' discovery endpoint it allows any Azure user to authenticate, however
+        // the token issuer is tenant specific and will not match what is in the common discovery document.
+        'validateIssuer' => false,  // issuer is just used to confirm correct discovery endpoint loaded
+        'discoveryUrl' => 'https://login.microsoftonline.com/common/.well-known/openid-configuration',
+        'clientId' => 'f579dc6e-58f5-41a8-8bbf-96d54eacfe8d',
+        'clientSecret' => 'GXc8Q~mgI7kTBllrvpBthUEioeARdjrRYORSyda4',
+        'pkceMethod' => 'S256',
+    ],
+
 
     // This is a authentication source which handles admin authentication.
     'admin' => array(

docs/PKCE.md

+# PKCE support
+
+PKCE (Proof Key for Code Exchange) is an extension to the OAuth2 protocol that is used to secure the
+authorization code flow against CSRF (cross site request forgery) and code injection attacks.
+PKCE is recommended in almost all OAuth use cases. Some servers or operators require the clients to use PKCE.
+
+## Usage
+
+Enable PKCE by setting the `pkceMethod` configuration key to a valid method (only `S256` is recommended).
+Note: `plain` is also a valid method, but not recommended, see the link to 'thephpleague/oauth2-client' below for details.
+
+### Example configuration
+
+Below is an example demonstrating how to configure the `authoauth2` module for PKCE:
+
+```php
+// config/authsources.php
+
+$config = [
+    'my-oidc-auth-source' => [
+        'authoauth2:OpenIDConnect',
+
+        'issuer' => 'https://my-issuer',
+        'clientId' => 'client-id',
+        'clientSecret' => 'client-secret',
+        
+        // activate PKCE with the S256 method
+        'pkceMethod' => 'S256',
+    ]
+];
+```
+
+## Links
+
+- See https://github.com/thephpleague/oauth2-client/blob/master/docs/usage.md#authorization-code-grant-with-pkce
+  for implementation notes of the underlying library.
+- RFC 7636 for PKCE: https://datatracker.ietf.org/doc/html/rfc7636

src/Auth/Source/OAuth2.php

@@ -12,7 +12,6 @@ use InvalidArgumentException;
 use League\OAuth2\Client\Provider\AbstractProvider;
 use League\OAuth2\Client\Provider\ResourceOwnerInterface;
 use League\OAuth2\Client\Token\AccessToken;
-use League\OAuth2\Client\Token\AccessTokenInterface;
 use Psr\Http\Message\RequestInterface;
 use SimpleSAML\Auth\Source;
 use SimpleSAML\Auth\State;
@@ -23,7 +22,7 @@ use SimpleSAML\Module\authoauth2\AttributeManipulator;
 use SimpleSAML\Module\authoauth2\ConfigTemplate;
 use SimpleSAML\Module\authoauth2\locators\HTTPLocator;
 use SimpleSAML\Module\authoauth2\Providers\AdjustableGenericProvider;
-use SimpleSAML\Utils\HTTP;
+use SimpleSAML\Session;
 
 /**
  * Authenticate using Oauth2.
@@ -49,6 +48,9 @@ class OAuth2 extends Source
     // phpcs:ignore
     public const DEBUG_LOG_FORMAT = "{method} {uri} {code} {req_headers_Authorization} >>>>'{req_body}' <<<<'{res_body}'";
 
+    private const PKCE_SESSION_NAMESPACE = 'authoauth2_pkce';
+    private const PKCE_SESSION_KEY = 'pkceCode';
+
     protected static string $defaultProviderClass = AdjustableGenericProvider::class;
 
     protected Configuration $config;
@@ -124,6 +126,9 @@ class OAuth2 extends Source
         $authorizeURL = $provider->getAuthorizationUrl($options);
         Logger::debug("authoauth2: $providerLabel redirecting to authorizeURL=$authorizeURL");
 
+        // save the pkce code in the current session, so it can be retrieved later for verification
+        $this->saveCodeChallengeFromProvider($provider);
+
         $this->getHttp()->redirectTrustedURL($authorizeURL);
     }
 
@@ -206,6 +211,10 @@ class OAuth2 extends Source
 
         $provider = $this->getProvider($this->config);
 
+        // load the pkce code from the session, so the server can validate it
+        // when exchanging the authorization code for an access token.
+        $this->loadCodeChallengeIntoProvider($provider);
+
         /**
          * @var AccessToken $accessToken
          */
@@ -359,6 +368,50 @@ class OAuth2 extends Source
         return $decoded;
     }
 
+    protected function isPkceEnabled(): bool
+    {
+        return (bool)$this->config->getOptionalValueValidate('pkceMethod', [
+            AbstractProvider::PKCE_METHOD_PLAIN,
+            AbstractProvider::PKCE_METHOD_S256,
+            ''
+        ], null);
+    }
+
+    /**
+     * support saving the providers PKCE code in the session for later verification.
+     * We store in the session rather in the $state since the $provider generates
+     * the pkce after it has been configured with the $state id, which we get after
+     * saving the $state.
+     */
+    protected function saveCodeChallengeFromProvider(AbstractProvider $provider): void
+    {
+        if ($this->isPkceEnabled()) {
+            Session::getSessionFromRequest()
+                ->setData(
+                    self::PKCE_SESSION_NAMESPACE,
+                    self::PKCE_SESSION_KEY,
+                    $provider->getPkceCode()
+                );
+        }
+    }
+
+    /**
+     * support retrieving the PKCE code from the session for verification.
+     */
+    protected function loadCodeChallengeIntoProvider(AbstractProvider $provider): void
+    {
+        if ($this->isPkceEnabled()) {
+            $pkceCode = (string)Session::getSessionFromRequest()
+                ->getData(
+                    self::PKCE_SESSION_NAMESPACE,
+                    self::PKCE_SESSION_KEY
+                );
+            if ($pkceCode) {
+                $provider->setPkceCode($pkceCode);
+            }
+        }
+    }
+
     /**
      * Allow subclasses to invoked any additional methods, such as extra API calls
      * @param AccessToken $accessToken The user's access token

src/Providers/OpenIDConnectProvider.php

@@ -26,6 +26,8 @@ class OpenIDConnectProvider extends AbstractProvider
 
     protected string $discoveryUrl;
 
+    protected ?string $pkceMethod = null;
+
     /**
      * @var ?Configuration
      */
@@ -258,4 +260,9 @@ class OpenIDConnectProvider extends AbstractProvider
         $config = $this->getOpenIDConfiguration();
         return $config->getOptionalString("end_session_endpoint", null);
     }
+
+    protected function getPkceMethod(): ?string
+    {
+        return $this->pkceMethod ?: parent::getPkceMethod();
+    }
 }

tests/lib/Auth/Source/OAuth2Test.php

@@ -12,6 +12,7 @@ use PHPUnit\Framework\TestCase;
 use Psr\Http\Message\RequestInterface;
 use SimpleSAML\Auth\State;
 use SimpleSAML\Module\authoauth2\Auth\Source\OAuth2;
+use SimpleSAML\Session;
 use SimpleSAML\Utils\HTTP;
 use Test\SimpleSAML\MockOAuth2Provider;
 use Test\SimpleSAML\RedirectException;
@@ -416,4 +417,106 @@ class OAuth2Test extends TestCase
         $strHandler = (string)$handlerStack;
         $this->assertStringContainsString('logHttpTraffic', $strHandler);
     }
+
+    /**
+     * test that the authenticate with save the pkce code challenge in the session and
+     * finalStep methods will load the pkce code challenge from the session.
+     *
+     * @psalm-param class-string<\Throwable>|null $expectedException
+     *
+     * @dataProvider dataAuthenticateAndFinalStepWillCallSaveAndRetrieveTheCode
+     */
+    public function testAuthenticateAndFinalStepWillCallSaveAndRetrieveTheCode(
+        ?string $method,
+        int $count,
+        ?string $expectedException
+    ): void {
+        if ($expectedException !== null) {
+            $this->expectException($expectedException);
+        }
+
+        $config = [
+            'providerClass' => MockOAuth2Provider::class,
+        ];
+        if ($method !== 'unset') {
+            $config['pkceMethod'] = $method;
+        }
+
+        $authOAuth2 = $this->getInstance($config);
+
+        $mock = $this->getMockBuilder(AbstractProvider::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        $accessToken = new AccessToken(['access_token' => 'stubAccessToken', 'id_token' => 'stubIdToken']);
+        $mock->method('getAccessToken')
+            ->willReturn($accessToken);
+
+        $attributes = ['name' => 'Bob'];
+        $user = new GenericResourceOwner($attributes, 'userId');
+        $mock->method('getResourceOwner')
+            ->with($accessToken)
+            ->willReturn($user);
+
+        // use different codes for loading and saving to test both cases
+        $pkceCodeOnAuthenticate = 'thePkceCodeOnAuthenticate';
+        $pkceCodeOnFinalStep = 'thePkceCodeOnFinalStep';
+        $mock
+            ->expects($this->exactly($count))
+            ->method('getPkceCode')
+            ->willReturn($pkceCodeOnAuthenticate);
+
+        $mock
+            ->expects($this->exactly($count))
+            ->method('setPkceCode')
+            ->with($pkceCodeOnFinalStep);
+
+        MockOAuth2Provider::setDelegate($mock);
+
+        // Override redirect behavior of authenticate
+        $http = $this->createMock(HTTP::class);
+        $http->method('redirectTrustedURL')
+            ->willThrowException(
+                new RedirectException('redirectTrustedURL', 'https://mock.com/')
+            );
+        // set the http mock
+        $authOAuth2->setHttp($http);
+
+        // empty the session and check it is empty
+        Session::getSessionFromRequest()->deleteData('authoauth2_pkce', 'pkceCode');
+        static::assertEmpty(Session::getSessionFromRequest()->getDataOfType('authoauth2_pkce'));
+
+        // perform the test
+        $state = [State::ID => 'stateId'];
+        try {
+            $authOAuth2->authenticate($state);
+            $this->fail("Redirect expected");
+        } catch (RedirectException $e) {
+            $sessionData = Session::getSessionFromRequest()->getDataOfType('authoauth2_pkce');
+            if ($count === 0) {
+                static::assertEmpty($sessionData);
+            } else {
+                static::assertEquals($pkceCodeOnAuthenticate, $sessionData['pkceCode']);
+            }
+        }
+
+        // prepare the session for the final step (different code to test, that the session will get used)
+        Session::getSessionFromRequest()->setData('authoauth2_pkce', 'pkceCode', $pkceCodeOnFinalStep);
+        $authOAuth2->finalStep($state, 'theOAuth2Code');
+    }
+
+    /**
+     * @return array<string, array{0: string|null, 1: int, 2: class-string<\Throwable>|null}>
+     */
+    public function dataAuthenticateAndFinalStepWillCallSaveAndRetrieveTheCode(): array
+    {
+        return [
+            'pkceMethod=S256' => ['S256', 1, null],
+            'pkceMethod=plain' => ['plain', 1, null],
+            // exception from underlying AbstractProvider
+            'invalid pkceMethod throws exception' => ['invalid', 0, \InvalidArgumentException::class],
+            'pkceMethod="" means pkce is disabled' => ['', 0, null],
+            'pkceMethod=null means pkce is disabled' => [null, 0, null],
+            'pkceMethod not set means pkce is disabled' => ['unset', 0, null],
+        ];
+    }
 }

tests/lib/MockOAuth2Provider.php

@@ -54,6 +54,16 @@ class MockOAuth2Provider extends GenericProvider implements ClearableState
         return self::$delegate->getParsedResponse($request);
     }
 
+    public function setPkceCode($pkceCode): void
+    {
+        self::$delegate->setPkceCode($pkceCode);
+    }
+
+    public function getPkceCode()
+    {
+        return self::$delegate->getPkceCode();
+    }
+
     /**
      * Clear any cached internal state.
      */

tests/lib/Providers/OpenIDConnectProviderTest.php

@@ -96,4 +96,26 @@ class OpenIDConnectProviderTest extends TestCase
             $provider->getDiscoveryUrl()
         );
     }
+
+    public function testGetPkceMethodGetsSetFromConfig(): void
+    {
+        $provider = new OpenIDConnectProvider(
+            ['issuer' => 'https://accounts.example.com']
+        );
+        // make the protected getPkceMethod available
+        $reflection = new \ReflectionClass($provider);
+        $method = $reflection->getMethod('getPkceMethod');
+        $method->setAccessible(true);
+        $this->assertNull($method->invoke($provider));
+
+        $provider = new OpenIDConnectProvider([
+            'issuer' => 'https://accounts.example.com',
+            'pkceMethod' => 'S256'
+        ]);
+        // make the protected getPkceMethod available
+        $reflection = new \ReflectionClass($provider);
+        $method = $reflection->getMethod('getPkceMethod');
+        $method->setAccessible(true);
+        $this->assertEquals('S256', $method->invoke($provider));
+    }
 }