From bc68eb66d50845972fc34c2c96600e91cd293184 Mon Sep 17 00:00:00 2001 From: Tim van Dijen Date: Mon, 29 Jul 2024 21:53:17 +0200 Subject: [PATCH] Rationalize algorithm blacklist code --- .../Encryption/EncryptionAlgorithmFactory.php | 13 ++++++----- .../KeyTransportAlgorithmFactory.php | 11 +++++----- .../Signature/SignatureAlgorithmFactory.php | 9 ++++---- src/XML/EncryptableElementTrait.php | 6 ++--- src/XML/EncryptedElementTrait.php | 6 ++--- src/XML/SignableElementTrait.php | 4 ++-- src/XML/SignedElementTrait.php | 4 ++-- .../EncryptionAlgorithmFactoryTest.php | 4 +++- tests/XML/CustomSignable.php | 22 +++---------------- tests/XML/EncryptedCustom.php | 22 +++---------------- 10 files changed, 37 insertions(+), 64 deletions(-) diff --git a/src/Alg/Encryption/EncryptionAlgorithmFactory.php b/src/Alg/Encryption/EncryptionAlgorithmFactory.php index 7b1c9111..af92ce82 100644 --- a/src/Alg/Encryption/EncryptionAlgorithmFactory.php +++ b/src/Alg/Encryption/EncryptionAlgorithmFactory.php @@ -60,16 +60,16 @@ final class EncryptionAlgorithmFactory /** * Build a factory that creates algorithms. * - * @param string[]|null $blacklist A list of algorithms forbidden for their use. + * @param string[] $blacklist A list of algorithms forbidden for their use. */ public function __construct( - protected ?array $blacklist = self::DEFAULT_BLACKLIST, + protected array $blacklist = self::DEFAULT_BLACKLIST, ) { // initialize the cache for supported algorithms per known implementation - if (!self::$initialized && $blacklist !== null) { + if (!self::$initialized) { foreach (self::SUPPORTED_DEFAULTS as $algorithm) { foreach ($algorithm::getSupportedAlgorithms() as $algId) { - if (array_key_exists($algId, self::$cache)) { + if (array_key_exists($algId, self::$cache) && !array_key_exists($algId, $this->blacklist)) { /* * If the key existed before initialization, that means someone registered a handler for this * algorithm, so we should respect that and skip registering the default here. @@ -101,8 +101,9 @@ public function getAlgorithm( #[\SensitiveParameter] KeyInterface $key, ): EncryptionAlgorithmInterface { - Assert::false( - ($this->blacklist !== null) && in_array($algId, $this->blacklist, true), + Assert::notInArray( + $algId, + $this->blacklist, sprintf('Blacklisted algorithm: \'%s\'.', $algId), BlacklistedAlgorithmException::class, ); diff --git a/src/Alg/KeyTransport/KeyTransportAlgorithmFactory.php b/src/Alg/KeyTransport/KeyTransportAlgorithmFactory.php index 887542f7..057302db 100644 --- a/src/Alg/KeyTransport/KeyTransportAlgorithmFactory.php +++ b/src/Alg/KeyTransport/KeyTransportAlgorithmFactory.php @@ -58,13 +58,13 @@ class KeyTransportAlgorithmFactory /** * Build a factory that creates algorithms. * - * @param string[]|null $blacklist A list of algorithms forbidden for their use. + * @param string[] $blacklist A list of algorithms forbidden for their use. */ public function __construct( - protected ?array $blacklist = self::DEFAULT_BLACKLIST, + protected array $blacklist = self::DEFAULT_BLACKLIST, ) { // initialize the cache for supported algorithms per known implementation - if (!self::$initialized && $blacklist !== null) { + if (!self::$initialized) { foreach (self::SUPPORTED_DEFAULTS as $algorithm) { foreach ($algorithm::getSupportedAlgorithms() as $algId) { if (array_key_exists($algId, self::$cache)) { @@ -99,8 +99,9 @@ public function getAlgorithm( #[\SensitiveParameter] KeyInterface $key, ): KeyTransportAlgorithmInterface { - Assert::false( - ($this->blacklist !== null) && in_array($algId, $this->blacklist, true), + Assert::notInArray( + $algId, + $this->blacklist, sprintf('Blacklisted algorithm: \'%s\'.', $algId), BlacklistedAlgorithmException::class, ); diff --git a/src/Alg/Signature/SignatureAlgorithmFactory.php b/src/Alg/Signature/SignatureAlgorithmFactory.php index 6d24e70f..11a47f46 100644 --- a/src/Alg/Signature/SignatureAlgorithmFactory.php +++ b/src/Alg/Signature/SignatureAlgorithmFactory.php @@ -65,10 +65,10 @@ final class SignatureAlgorithmFactory * @param string[]|null $blacklist A list of algorithms forbidden for their use. */ public function __construct( - protected ?array $blacklist = self::DEFAULT_BLACKLIST, + protected array $blacklist = self::DEFAULT_BLACKLIST, ) { // initialize the cache for supported algorithms per known implementation - if (!self::$initialized && $blacklist !== null) { + if (!self::$initialized) { foreach (self::SUPPORTED_DEFAULTS as $algorithm) { foreach ($algorithm::getSupportedAlgorithms() as $algId) { if (array_key_exists($algId, self::$cache)) { @@ -103,8 +103,9 @@ public function getAlgorithm( #[\SensitiveParameter] KeyInterface $key, ): SignatureAlgorithmInterface { - Assert::false( - ($this->blacklist !== null) && in_array($algId, $this->blacklist, true), + Assert::notInArray( + $algId, + $this->blacklist, sprintf('Blacklisted algorithm: \'%s\'.', $algId), BlacklistedAlgorithmException::class, ); diff --git a/src/XML/EncryptableElementTrait.php b/src/XML/EncryptableElementTrait.php index 0f7bba1c..a85d4b06 100644 --- a/src/XML/EncryptableElementTrait.php +++ b/src/XML/EncryptableElementTrait.php @@ -66,7 +66,7 @@ public function encrypt(EncryptionAlgorithmInterface $encryptor): EncryptedData $keyInfo = new KeyInfo([$encryptedKey]); - $factory = new EncryptionAlgorithmFactory($this->getBlacklistedAlgorithms()); + $factory = new EncryptionAlgorithmFactory($this->getBlacklistedAlgorithms() ?? EncryptionAlgorithmFactory::DEFAULT_BLACKLIST); $encryptor = $factory->getAlgorithm($this->blockCipherAlgId, $sessionKey); $encryptor->setBackend($this->getEncryptionBackend()); } @@ -99,8 +99,8 @@ abstract public function getEncryptionBackend(): ?EncryptionBackend; /** * Get the list of algorithms that are blacklisted for any encryption operation. * - * @return string[]|null An array with all algorithm identifiers that are blacklisted, or null if we want to use the - * defaults. + * @return string[]|null An array with all algorithm identifiers that are blacklisted, or null to use this + * libraries default. */ abstract public function getBlacklistedAlgorithms(): ?array; diff --git a/src/XML/EncryptedElementTrait.php b/src/XML/EncryptedElementTrait.php index d68463bc..987f6d94 100644 --- a/src/XML/EncryptedElementTrait.php +++ b/src/XML/EncryptedElementTrait.php @@ -131,7 +131,7 @@ protected function decryptData(EncryptionAlgorithmInterface $decryptor): string $encryptedKey = $this->getEncryptedKey(); $decryptionKey = $encryptedKey->decrypt($decryptor); - $factory = new EncryptionAlgorithmFactory($this->getBlacklistedAlgorithms()); + $factory = new EncryptionAlgorithmFactory($this->getBlacklistedAlgorithms() ?? EncryptionAlgorithmFactory::DEFAULT_BLACKLIST); $decryptor = $factory->getAlgorithm($encMethod->getAlgorithm(), new SymmetricKey($decryptionKey)); $decryptor->setBackend($this->getEncryptionBackend()); } @@ -209,8 +209,8 @@ abstract public function getEncryptionBackend(): ?EncryptionBackend; /** * Get the list of algorithms that are blacklisted for any encryption operation. * - * @return string[]|null An array with all algorithm identifiers that are blacklisted, or null if we want to use the - * defaults. + * @return string[]|null An array with all algorithm identifiers that are blacklisted, or null to use this + * libraries default. */ abstract public function getBlacklistedAlgorithms(): ?array; } diff --git a/src/XML/SignableElementTrait.php b/src/XML/SignableElementTrait.php index 1bd9bc5d..4509ea9a 100644 --- a/src/XML/SignableElementTrait.php +++ b/src/XML/SignableElementTrait.php @@ -189,8 +189,8 @@ protected function doSign(DOMElement $xml): DOMElement /** * Get the list of algorithms that are blacklisted for any signing operation. * - * @return string[]|null An array with all algorithm identifiers that are blacklisted, or null if we want to use the - * defaults. + * @return string[]|null An array with all algorithm identifiers that are blacklisted, or null to use this + * libraries default. */ abstract public function getBlacklistedAlgorithms(): ?array; } diff --git a/src/XML/SignedElementTrait.php b/src/XML/SignedElementTrait.php index de6c729a..d9fc68dc 100644 --- a/src/XML/SignedElementTrait.php +++ b/src/XML/SignedElementTrait.php @@ -310,8 +310,8 @@ abstract public function getId(): ?string; /** * Get the list of algorithms that are blacklisted for any signing operation. * - * @return string[]|null An array with all algorithm identifiers that are blacklisted, or null if we want to use the - * defaults. + * @return string[]|null An array with all algorithm identifiers that are blacklisted, or null to use this + * libraries default. */ abstract public function getBlacklistedAlgorithms(): ?array; } diff --git a/tests/Alg/Encryption/EncryptionAlgorithmFactoryTest.php b/tests/Alg/Encryption/EncryptionAlgorithmFactoryTest.php index f6df73a4..4f580843 100644 --- a/tests/Alg/Encryption/EncryptionAlgorithmFactoryTest.php +++ b/tests/Alg/Encryption/EncryptionAlgorithmFactoryTest.php @@ -35,7 +35,7 @@ public static function setUpBeforeClass(): void */ public function testGetUnknownAlgorithm(): void { - $factory = new EncryptionAlgorithmFactory([]); + $factory = new EncryptionAlgorithmFactory(); $this->expectException(UnsupportedAlgorithmException::class); $factory->getAlgorithm('Unsupported algorithm identifier', self::$skey); } @@ -47,6 +47,7 @@ public function testGetUnknownAlgorithm(): void public function testDefaultBlacklistedAlgorithms(): void { $factory = new EncryptionAlgorithmFactory(); + $algorithm = $factory->getAlgorithm(C::BLOCK_ENC_AES128, self::$skey); $this->assertInstanceOf(AES::class, $algorithm); $this->assertEquals(C::BLOCK_ENC_AES128, $algorithm->getAlgorithmId()); @@ -83,6 +84,7 @@ public function testDefaultBlacklistedAlgorithms(): void public function testBlacklistedAlgorithm(): void { $factory = new EncryptionAlgorithmFactory([C::BLOCK_ENC_AES256_GCM]); + $algorithm = $factory->getAlgorithm(C::BLOCK_ENC_3DES, self::$skey); $this->assertInstanceOf(TripleDES::class, $algorithm); $this->assertEquals(C::BLOCK_ENC_3DES, $algorithm->getAlgorithmId()); diff --git a/tests/XML/CustomSignable.php b/tests/XML/CustomSignable.php index c25a8737..91dad755 100644 --- a/tests/XML/CustomSignable.php +++ b/tests/XML/CustomSignable.php @@ -47,9 +47,6 @@ class CustomSignable extends AbstractElement implements /** @var \SimpleSAML\XMLSecurity\Backend\EncryptionBackend|null */ private ?EncryptionBackend $backend = null; - /** @var string[] */ - private array $blacklistedAlgs = []; - /** * Constructor * @@ -143,25 +140,12 @@ public function setEncryptionBackend(?EncryptionBackend $backend): void * Implement a method like this if your encrypted object needs to instantiate a new decryptor, for example, to * decrypt a session key. This method is required by \SimpleSAML\XMLSecurity\XML\EncryptedElementTrait. * - * @return string[]|null An array with all algorithm identifiers that we want to blacklist, or null if we want to - * use the defaults. + * @return string[]|null An array with all algorithm identifiers that are blacklisted, or null to use this + * libraries default. */ public function getBlacklistedAlgorithms(): ?array { - return $this->blacklistedAlgs; - } - - - /** - * Implement a method like this if your encrypted object needs to instantiate a new decryptor, for example, to - * decrypt a session key. This method is required by \SimpleSAML\XMLSecurity\XML\EncryptedElementTrait. - * - * @param string[]|null $algIds An array with the identifiers of the algorithms we want to blacklist, or null if we - * want to use the defaults. - */ - public function setBlacklistedAlgorithms(?array $algIds): void - { - $this->blacklistedAlgs = $algIds; + return []; } diff --git a/tests/XML/EncryptedCustom.php b/tests/XML/EncryptedCustom.php index 4b06602b..2615894d 100644 --- a/tests/XML/EncryptedCustom.php +++ b/tests/XML/EncryptedCustom.php @@ -45,9 +45,6 @@ final class EncryptedCustom extends AbstractElement implements EncryptedElementI /** @var EncryptionBackend|null $backend */ private ?EncryptionBackend $backend = null; - /** @var string[] $blacklistedAlgs */ - private array $blacklistedAlgs = []; - /** * Construct an encrypted object. @@ -91,25 +88,12 @@ public function setEncryptionBackend(?EncryptionBackend $backend): void * Implement a method like this if your encrypted object needs to instantiate a new decryptor, for example, to * decrypt a session key. This method is required by \SimpleSAML\XMLSecurity\XML\EncryptedElementTrait. * - * @return string[]|null An array with all algorithm identifiers that we want to blacklist, or null if we want to - * use the defaults. + * @return string[]|null An array with all algorithm identifiers that are blacklisted, or null to use this + * libraries default. */ public function getBlacklistedAlgorithms(): ?array { - return $this->blacklistedAlgs; - } - - - /** - * Implement a method like this if your encrypted object needs to instantiate a new decryptor, for example, to - * decrypt a session key. This method is required by \SimpleSAML\XMLSecurity\XML\EncryptedElementTrait. - * - * @param string[]|null $algIds An array with the identifiers of the algorithms we want to blacklist, or null if we - * want to use the defaults. - */ - public function setBlacklistedAlgorithms(?array $algIds): void - { - $this->blacklistedAlgs = $algIds; + return []; }