diff --git a/README.md b/README.md
index f74bb5b8a506e0f320373c97063c19e2cf9235eb..90327b13d3d000a5760d02a76702ae3e01b063b0 100644
--- a/README.md
+++ b/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)
diff --git a/composer.json b/composer.json
index 517636c6fd71ad1fb2b4d47110f2e511a9366d0b..61c65e7a6537b3aad62a4ff5a5663069c649d8f1 100644
--- a/composer.json
+++ b/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",
diff --git a/docker/config/authsources.php b/docker/config/authsources.php
index d1e2be9965546d5e28eb0add75bd20231965004c..5e4c76542c53d5c54d868bd0875e04209e35337e 100644
--- a/docker/config/authsources.php
+++ b/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(
diff --git a/docs/PKCE.md b/docs/PKCE.md
new file mode 100644
index 0000000000000000000000000000000000000000..4d6efd58ebb2d41e3febbb04ad9a2b6d80ce8bc7
--- /dev/null
+++ b/docs/PKCE.md
@@ -0,0 +1,37 @@
+# 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
diff --git a/src/Auth/Source/OAuth2.php b/src/Auth/Source/OAuth2.php
index 3cdd32b9639815b974c8d1d05049d4e6d31bf967..c233a6c5a4fb564168be394f1759096d5bd8ee32 100644
--- a/src/Auth/Source/OAuth2.php
+++ b/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
diff --git a/src/Providers/OpenIDConnectProvider.php b/src/Providers/OpenIDConnectProvider.php
index 23d4d01565d9475395b3a0e3f43d8b5cc3740bc2..0abc2b9a8fb5710a64d9769a2aaab6526bbff84c 100644
--- a/src/Providers/OpenIDConnectProvider.php
+++ b/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();
+ }
}
diff --git a/tests/lib/Auth/Source/OAuth2Test.php b/tests/lib/Auth/Source/OAuth2Test.php
index e433638650b0fd7bfd3ff32415b6c2a51048a793..e4ae842b6bc3550169ef7deb9aa6561614da5357 100644
--- a/tests/lib/Auth/Source/OAuth2Test.php
+++ b/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],
+ ];
+ }
}
diff --git a/tests/lib/MockOAuth2Provider.php b/tests/lib/MockOAuth2Provider.php
index 5b505f20c353d91af7cb58fea8accc6634229197..781bb1844dbb09583972f77763f71b00e4f224a2 100644
--- a/tests/lib/MockOAuth2Provider.php
+++ b/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.
*/
diff --git a/tests/lib/Providers/OpenIDConnectProviderTest.php b/tests/lib/Providers/OpenIDConnectProviderTest.php
index 64e33f31e73cd745520d6bdfa1bc1eb288ebb4ec..726ba55f5318350a0a19dc6c81e393f772f716e0 100644
--- a/tests/lib/Providers/OpenIDConnectProviderTest.php
+++ b/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));
+ }
}