diff --git a/config/packages/ci/parameters.yml b/config/packages/ci/parameters.yml new file mode 100644 index 0000000000..a874aea6d2 --- /dev/null +++ b/config/packages/ci/parameters.yml @@ -0,0 +1,8 @@ +parameters: + encryption_keys: + default: + publicFile: /config/engine/engineblock.crt + privateFile: /config/engine/engineblock.pem + rollover: + publicFile: '%kernel.project_dir%/src/OpenConext/EngineBlockFunctionalTestingBundle/Resources/keys/rolled-over.crt' + privateFile: '%kernel.project_dir%/src/OpenConext/EngineBlockFunctionalTestingBundle/Resources/keys/rolled-over.key' diff --git a/config/packages/dev/parameters.yml b/config/packages/dev/parameters.yml new file mode 100644 index 0000000000..a874aea6d2 --- /dev/null +++ b/config/packages/dev/parameters.yml @@ -0,0 +1,8 @@ +parameters: + encryption_keys: + default: + publicFile: /config/engine/engineblock.crt + privateFile: /config/engine/engineblock.pem + rollover: + publicFile: '%kernel.project_dir%/src/OpenConext/EngineBlockFunctionalTestingBundle/Resources/keys/rolled-over.crt' + privateFile: '%kernel.project_dir%/src/OpenConext/EngineBlockFunctionalTestingBundle/Resources/keys/rolled-over.key' diff --git a/config/packages/parameters.yml.dist b/config/packages/parameters.yml.dist index f8fe5741e8..58f66a6fca 100644 --- a/config/packages/parameters.yml.dist +++ b/config/packages/parameters.yml.dist @@ -36,6 +36,9 @@ parameters: ## The Signing / Encryption keys used for the SAML2 authentication and metadata ## When EngineBlock signs responses (when it acts as an Idp) ## or requests (when it acts as an SP) it uses these X.509 certs. + ## During a key rollover, add the new key as 'rollover'. The default metadata will then + ## contain both keys, allowing IdPs to accept responses signed with either key. + ## Once all IdPs have updated their metadata, remove the old key or swap default/rollover. encryption_keys: default: publicFile: /config/engine/engineblock.crt diff --git a/src/OpenConext/EngineBlock/Metadata/Factory/Decorator/EngineBlockIdentityProvider.php b/src/OpenConext/EngineBlock/Metadata/Factory/Decorator/EngineBlockIdentityProvider.php index a4f9fe4d3f..b0aa14a032 100644 --- a/src/OpenConext/EngineBlock/Metadata/Factory/Decorator/EngineBlockIdentityProvider.php +++ b/src/OpenConext/EngineBlock/Metadata/Factory/Decorator/EngineBlockIdentityProvider.php @@ -30,9 +30,10 @@ class EngineBlockIdentityProvider extends AbstractIdentityProvider { /** - * @var X509KeyPair + * @var array */ - private $keyPair; + private array $keyPairs; + /** * @var UrlProvider */ @@ -46,20 +47,23 @@ class EngineBlockIdentityProvider extends AbstractIdentityProvider public function __construct( IdentityProviderEntityInterface $entity, ?string $keyId, - X509KeyPair $keyPair, + array $keyPairs, UrlProvider $urlProvider ) { parent::__construct($entity); $this->keyId = $keyId; - $this->keyPair = $keyPair; + $this->keyPairs = $keyPairs; $this->urlProvider = $urlProvider; } public function getCertificates(): array { - return [ - $this->keyPair->getCertificate(), - ]; + $certificates = []; + foreach ($this->keyPairs as $keyPair) { + $certificates[] = $keyPair->getCertificate(); + } + + return $certificates; } public function getSupportedNameIdFormats(): array diff --git a/src/OpenConext/EngineBlock/Metadata/Factory/Decorator/EngineBlockServiceProvider.php b/src/OpenConext/EngineBlock/Metadata/Factory/Decorator/EngineBlockServiceProvider.php index 5659726823..33e0280176 100644 --- a/src/OpenConext/EngineBlock/Metadata/Factory/Decorator/EngineBlockServiceProvider.php +++ b/src/OpenConext/EngineBlock/Metadata/Factory/Decorator/EngineBlockServiceProvider.php @@ -32,13 +32,15 @@ class EngineBlockServiceProvider extends AbstractServiceProvider { /** - * @var X509KeyPair + * @var array */ - private $keyPair; + private array $keyPairs; + /** * @var AttributesMetadata */ private $attributes; + /** * @var UrlProvider */ @@ -46,13 +48,13 @@ class EngineBlockServiceProvider extends AbstractServiceProvider public function __construct( ServiceProviderEntityInterface $entity, - X509KeyPair $keyPair, + array $keyPairs, AttributesMetadata $attributes, UrlProvider $urlProvider ) { parent::__construct($entity); - $this->keyPair = $keyPair; + $this->keyPairs = $keyPairs; $this->attributes = $attributes; $this->urlProvider = $urlProvider; } @@ -60,7 +62,12 @@ public function __construct( public function getCertificates(): array { - return [$this->keyPair->getCertificate()]; + $certificates = []; + foreach ($this->keyPairs as $keyPair) { + $certificates[] = $keyPair->getCertificate(); + } + + return $certificates; } /** diff --git a/src/OpenConext/EngineBlock/Metadata/Factory/Factory/IdentityProviderFactory.php b/src/OpenConext/EngineBlock/Metadata/Factory/Factory/IdentityProviderFactory.php index 324f3fc7e4..1c7c352ac1 100644 --- a/src/OpenConext/EngineBlock/Metadata/Factory/Factory/IdentityProviderFactory.php +++ b/src/OpenConext/EngineBlock/Metadata/Factory/Factory/IdentityProviderFactory.php @@ -100,7 +100,9 @@ private function buildEngineBlockEntityFromEntity(IdentityProvider $entity, ?str $this->engineBlockConfiguration ), $keyId, - $this->keyPairFactory->buildFromIdentifier($keyId), + ($keyId === KeyPairFactory::DEFAULT_KEY_PAIR_IDENTIFIER || $keyId === null) + ? $this->keyPairFactory->buildAll() + : [$this->keyPairFactory->buildFromIdentifier($keyId)], $this->urlProvider ); } diff --git a/src/OpenConext/EngineBlock/Metadata/Factory/Factory/ServiceProviderFactory.php b/src/OpenConext/EngineBlock/Metadata/Factory/Factory/ServiceProviderFactory.php index d49b21759a..216a7aa656 100644 --- a/src/OpenConext/EngineBlock/Metadata/Factory/Factory/ServiceProviderFactory.php +++ b/src/OpenConext/EngineBlock/Metadata/Factory/Factory/ServiceProviderFactory.php @@ -53,6 +53,7 @@ class ServiceProviderFactory * @var EngineBlockConfiguration */ private $engineBlockConfiguration; + /** * @var UrlProvider */ @@ -95,7 +96,9 @@ public function createEngineBlockEntityFrom(string $keyId): ServiceProviderEntit new ServiceProviderEntity($entity), $this->engineBlockConfiguration ), - $this->keyPairFactory->buildFromIdentifier($keyId), + ($keyId === KeyPairFactory::DEFAULT_KEY_PAIR_IDENTIFIER || $keyId === null) + ? $this->keyPairFactory->buildAll() + : [$this->keyPairFactory->buildFromIdentifier($keyId)], $this->attributes, $this->urlProvider ); diff --git a/src/OpenConext/EngineBlock/Metadata/X509/KeyPairFactory.php b/src/OpenConext/EngineBlock/Metadata/X509/KeyPairFactory.php index 2fe1be4bf7..bfe84272ca 100644 --- a/src/OpenConext/EngineBlock/Metadata/X509/KeyPairFactory.php +++ b/src/OpenConext/EngineBlock/Metadata/X509/KeyPairFactory.php @@ -26,7 +26,7 @@ class KeyPairFactory { const DEFAULT_KEY_PAIR_IDENTIFIER = 'default'; - private $keyPairConfiguration = []; + private array $keyPairConfiguration = []; /** * @param array $keyPairConfiguration @@ -42,7 +42,7 @@ public function __construct(array $keyPairConfiguration) * * @throws RuntimeException */ - public function buildFromIdentifier(?string $identifier) : X509KeyPair + public function buildFromIdentifier(?string $identifier): X509KeyPair { if ($identifier === null) { $identifier = self::DEFAULT_KEY_PAIR_IDENTIFIER; @@ -57,4 +57,20 @@ public function buildFromIdentifier(?string $identifier) : X509KeyPair } throw new UnknownKeyIdException($identifier); } + + /** + * @return array + * + * @throws RuntimeException + */ + public function buildAll(): array + { + $pairs = []; + + foreach (array_keys($this->keyPairConfiguration) as $keyId) { + $pairs[] = $this->buildFromIdentifier((string)$keyId); + } + + return $pairs; + } } diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/FrontPage.feature b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/FrontPage.feature index 2558443b36..78dd621404 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/FrontPage.feature +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/FrontPage.feature @@ -10,20 +10,25 @@ Feature: Scenario: The user can see all available metadata links When I go to Engineblock URL "/" - Then I should see 8 links on the front page + Then I should see 12 links on the front page And I should see text matching "The Public SAML Signing certificate of the OpenConext IdP" And I should see URL "/authentication/idp/certificate" # The default key should not get a separate URL next to the key-less url And I should not see URL "/authentication/idp/certificate/key:default" + # The rollover key should get its own URL + And I should see URL "/authentication/idp/certificate/key:rollover" And I should see text matching "The Public SAML metadata \(the entity descriptor\) of the OpenConext IdP Proxy" And I should see URL "/authentication/idp/metadata" And I should not see URL "/authentication/idp/metadata/key:default" + And I should see URL "/authentication/idp/metadata/key:rollover" And I should see text matching "The Public SAML metadata \(the entities descriptor\) for all the OpenConext IdPs" And I should see URL "/authentication/proxy/idps-metadata" And I should not see URL "/authentication/proxy/idps-metadata/key:default" + And I should see URL "/authentication/proxy/idps-metadata/key:rollover" And I should see text matching "The Public SAML metadata \(the entities descriptor\) of the OpenConext IdPs which" And I should see URL "/authentication/proxy/idps-metadata?sp-entity-id=urn:example.org" And I should not see URL "/authentication/proxy/idps-metadata/key:default?sp-entity-id=urn:example.org" + And I should see URL "/authentication/proxy/idps-metadata/key:rollover?sp-entity-id=urn:example.org" # The test debug link is present on the page And I should see text matching "Test authentication with an identity provider." And I should see URL "/authentication/sp/debug" diff --git a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Metadata.feature b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Metadata.feature index 2213580389..d0d08089a1 100644 --- a/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Metadata.feature +++ b/src/OpenConext/EngineBlockFunctionalTestingBundle/Features/Metadata.feature @@ -200,6 +200,27 @@ Feature: # Verify the used signing key is EB key And the response should match xpath '//ds:Signature//ds:X509Certificate[starts-with(.,"MIIDuDCCAqCgAwIBAgIJAPdqJ9JQKN6vMA0GCSqGSIb3DQEBBQUAMEYxDzANBgNVBAMT")]' + Scenario: The default SP metadata contains all configured keys (rollover support) + When I go to Engineblock URL "/authentication/sp/metadata" + # Verify the default (current) signing key is present + Then the response should match xpath '//md:KeyDescriptor[@use="signing"]//ds:X509Certificate[starts-with(.,"MIIDuDCCAqCgAwIBAgIJAPdqJ9JQKN6vMA0GCSqGSIb3DQEBBQUAMEYxDzANBgNVBAMT")]' + # Verify the rollover signing key is also present + And the response should match xpath '//md:KeyDescriptor[@use="signing"]//ds:X509Certificate[starts-with(.,"MIIDhTCCAm2gAwIBAgIJALJlbT5u9cXzMA0GCSqGSIb3DQEBBQUAMFkxCzAJ")]' + + Scenario: The default IdP metadata contains all configured keys (rollover support) + When I go to Engineblock URL "/authentication/idp/metadata" + # Verify the default (current) signing key is present + Then the response should match xpath '//md:KeyDescriptor[@use="signing"]//ds:X509Certificate[starts-with(.,"MIIDuDCCAqCgAwIBAgIJAPdqJ9JQKN6vMA0GCSqGSIb3DQEBBQUAMEYxDzANBgNVBAMT")]' + # Verify the rollover signing key is also present + And the response should match xpath '//md:KeyDescriptor[@use="signing"]//ds:X509Certificate[starts-with(.,"MIIDhTCCAm2gAwIBAgIJALJlbT5u9cXzMA0GCSqGSIb3DQEBBQUAMFkxCzAJ")]' + + Scenario: Requesting metadata with a specific key id returns only that key + When I go to Engineblock URL "/authentication/sp/metadata/key:rollover" + # Verify only the rollover key is present + Then the response should match xpath '//md:KeyDescriptor[@use="signing"]//ds:X509Certificate[starts-with(.,"MIIDhTCCAm2gAwIBAgIJALJlbT5u9cXzMA0GCSqGSIb3DQEBBQUAMFkxCzAJ")]' + # Verify the default key is not present + And the response should not match xpath '//md:KeyDescriptor[@use="signing"]//ds:X509Certificate[starts-with(.,"MIIDuDCCAqCgAwIBAgIJAPdqJ9JQKN6vMA0GCSqGSIb3DQEBBQUAMEYxDzANBgNVBAMT")]' + Scenario: A user can request the EngineBlock SP public certificate When I go to Engineblock URL "/authentication/sp/certificate" Then the response should contain '-----BEGIN CERTIFICATE-----' diff --git a/tests/unit/OpenConext/EngineBlock/Metadata/Factory/Decorator/EngineBlockServiceProvider.php b/tests/unit/OpenConext/EngineBlock/Metadata/Factory/Decorator/EngineBlockServiceProvider.php index 5a2b7b32a1..454a1543db 100644 --- a/tests/unit/OpenConext/EngineBlock/Metadata/Factory/Decorator/EngineBlockServiceProvider.php +++ b/tests/unit/OpenConext/EngineBlock/Metadata/Factory/Decorator/EngineBlockServiceProvider.php @@ -74,7 +74,7 @@ function (...$parameters) use ($matcher) { $attributesMock->method('getRequestedAttributes') ->willReturn($attributes); - $decorator = new EngineBlockServiceProvider($adapter, $keyPairMock, $attributesMock, $this->urlProvider); + $decorator = new EngineBlockServiceProvider($adapter, [$keyPairMock], $attributesMock, $this->urlProvider); $supportedNameIdFormats = [ Constants::NAMEID_PERSISTENT, diff --git a/tests/unit/OpenConext/EngineBlock/Metadata/Factory/Decorator/EngineblockIdentityProvider.php b/tests/unit/OpenConext/EngineBlock/Metadata/Factory/Decorator/EngineblockIdentityProvider.php index 7d14a91fd1..09c2a09087 100644 --- a/tests/unit/OpenConext/EngineBlock/Metadata/Factory/Decorator/EngineblockIdentityProvider.php +++ b/tests/unit/OpenConext/EngineBlock/Metadata/Factory/Decorator/EngineblockIdentityProvider.php @@ -85,7 +85,7 @@ public function test_methods() } }); - $decorator = new EngineBlockIdentityProvider($this->adapter, 'default', $this->keyPairMock, $this->urlProvider); + $decorator = new EngineBlockIdentityProvider($this->adapter, 'default', [$this->keyPairMock], $this->urlProvider); $supportedNameIdFormats = [ Constants::NAMEID_PERSISTENT, @@ -110,7 +110,7 @@ public function test_override_slo_service_if_child_slo_service_set() ->with('authentication_logout', false, null, null) ->willReturn('sloLocation'); - $decorator = new EngineBlockIdentityProvider($this->adapter, 'default', $this->keyPairMock, $this->urlProvider); + $decorator = new EngineBlockIdentityProvider($this->adapter, 'default', [$this->keyPairMock], $this->urlProvider); $this->assertEquals($decorator->getSingleLogoutService(), new Service('sloLocation', Constants::BINDING_HTTP_REDIRECT)); } @@ -121,7 +121,7 @@ public function test_return_null_for_slo_service_if_child_has_no_slo_service_set 'singleLogoutService' => null, ]); - $decorator = new EngineBlockIdentityProvider($this->adapter, 'default', $this->keyPairMock, $this->urlProvider); + $decorator = new EngineBlockIdentityProvider($this->adapter, 'default', [$this->keyPairMock], $this->urlProvider); $this->assertEquals($decorator->getSingleLogoutService(), null); } @@ -145,7 +145,7 @@ function (...$parameters) use ($matcher) { } // we would expect the fourth paremeter to be null and not 'entity-id' becasue we are EB ); - $decorator = new EngineBlockIdentityProvider($this->adapter, 'default', $this->keyPairMock, $this->urlProvider); + $decorator = new EngineBlockIdentityProvider($this->adapter, 'default', [$this->keyPairMock], $this->urlProvider); $this->assertEquals([new Service('ssoLocation', Constants::BINDING_HTTP_REDIRECT)], $decorator->getSingleSignOnServices()); } diff --git a/tests/unit/OpenConext/EngineBlock/Metadata/X509/KeyPairFactoryTest.php b/tests/unit/OpenConext/EngineBlock/Metadata/X509/KeyPairFactoryTest.php index e028ead98b..f447690db4 100644 --- a/tests/unit/OpenConext/EngineBlock/Metadata/X509/KeyPairFactoryTest.php +++ b/tests/unit/OpenConext/EngineBlock/Metadata/X509/KeyPairFactoryTest.php @@ -55,6 +55,16 @@ public function test_builds_key_pair_from_specified_identifier() $this->assertEquals('file://' . __DIR__ . '/test.pem.key', $defaultKeyPair->getPrivateKey()->getFilePath()); } + public function test_build_all_returns_all_configured_key_pairs() + { + $keyPairs = $this->factory->buildAll(); + + $this->assertCount(2, $keyPairs); + $this->assertContainsOnlyInstancesOf(X509KeyPair::class, $keyPairs); + $this->assertEquals(file_get_contents('file://' . __DIR__ . '/test.pem.crt'), $keyPairs[0]->getCertificate()->toPem()); + $this->assertEquals(file_get_contents('file://' . __DIR__ . '/test2.pem.crt'), $keyPairs[1]->getCertificate()->toPem()); + } + public function test_it_raises_exception_when_requesting_empty_key_pair_identifier() { $this->expectException(InvalidArgumentException::class);