Skip to content

Commit

Permalink
Rationalize algorithm blacklist code
Browse files Browse the repository at this point in the history
  • Loading branch information
tvdijen committed Jul 29, 2024
1 parent ad6282d commit bc68eb6
Show file tree
Hide file tree
Showing 10 changed files with 37 additions and 64 deletions.
13 changes: 7 additions & 6 deletions src/Alg/Encryption/EncryptionAlgorithmFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
);
Expand Down
11 changes: 6 additions & 5 deletions src/Alg/KeyTransport/KeyTransportAlgorithmFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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,
);
Expand Down
9 changes: 5 additions & 4 deletions src/Alg/Signature/SignatureAlgorithmFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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,
);
Expand Down
6 changes: 3 additions & 3 deletions src/XML/EncryptableElementTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down Expand Up @@ -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;

Expand Down
6 changes: 3 additions & 3 deletions src/XML/EncryptedElementTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down Expand Up @@ -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;
}
4 changes: 2 additions & 2 deletions src/XML/SignableElementTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
4 changes: 2 additions & 2 deletions src/XML/SignedElementTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
4 changes: 3 additions & 1 deletion tests/Alg/Encryption/EncryptionAlgorithmFactoryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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());
Expand Down Expand Up @@ -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());
Expand Down
22 changes: 3 additions & 19 deletions tests/XML/CustomSignable.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down Expand Up @@ -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 [];
}


Expand Down
22 changes: 3 additions & 19 deletions tests/XML/EncryptedCustom.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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 [];
}


Expand Down

0 comments on commit bc68eb6

Please sign in to comment.