From 7706dbc4cde54b55b7aa3695816416161c485583 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Wed, 10 Apr 2024 22:44:19 +0200 Subject: [PATCH] refactor: `SslContext` and its instantiation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This refactors some of the `SslContext` implementation to have better readability, less complexity (regarding camel case handling, etc.) and explicit context serialization. Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- src/RedisClusterOptions.php | 42 +++- src/RedisClusterResourceManager.php | 4 +- src/SslContext.php | 336 ++++++++++++++------------ test/unit/RedisClusterOptionsTest.php | 14 +- test/unit/SslContextTest.php | 109 ++++----- 5 files changed, 267 insertions(+), 238 deletions(-) diff --git a/src/RedisClusterOptions.php b/src/RedisClusterOptions.php index 66e9c5c..79bbaee 100644 --- a/src/RedisClusterOptions.php +++ b/src/RedisClusterOptions.php @@ -6,8 +6,11 @@ use Laminas\Cache\Exception\RuntimeException; use Laminas\Cache\Storage\Adapter\Exception\InvalidRedisClusterConfigurationException; +use Laminas\Stdlib\AbstractOptions; +use Traversable; use function is_array; +use function iterator_to_array; final class RedisClusterOptions extends AdapterOptions { @@ -55,7 +58,6 @@ final class RedisClusterOptions extends AdapterOptions private string $password = ''; - /** @psalm-var SslContext|null */ private ?SslContext $sslContext = null; /** @@ -81,6 +83,31 @@ public function __construct($options = null) } } + /** + * {@inheritDoc} + */ + public function setFromArray($options) + { + if ($options instanceof AbstractOptions) { + $options = $options->toArray(); + } elseif ($options instanceof Traversable) { + $options = iterator_to_array($options); + } + + $sslContext = $options['sslContext'] ?? $options['ssl_context'] ?? null; + unset($options['sslContext'], $options['ssl_context']); + if (is_array($sslContext)) { + /** @psalm-suppress MixedArgumentTypeCoercion Trust upstream that they verify the array beforehand. */ + $sslContext = SslContext::fromSslContextArray($sslContext); + } + + if ($sslContext instanceof SslContext) { + $options['ssl_context'] = $sslContext; + } + + return parent::setFromArray($options); + } + public function setTimeout(float $timeout): void { $this->timeout = $timeout; @@ -228,24 +255,13 @@ public function setPassword(string $password): void $this->password = $password; } - /** - * @psalm-return SslContext|null - */ public function getSslContext(): ?SslContext { return $this->sslContext; } - /** - * @psalm-param array|SslContext|null $sslContext - */ - public function setSslContext(array|SslContext|null $sslContext): void + public function setSslContext(SslContext|null $sslContext): void { - if (is_array($sslContext)) { - $data = $sslContext; - $sslContext = new SslContext(); - $sslContext->exchangeArray($data); - } $this->sslContext = $sslContext; } } diff --git a/src/RedisClusterResourceManager.php b/src/RedisClusterResourceManager.php index 382a498..a9609f4 100644 --- a/src/RedisClusterResourceManager.php +++ b/src/RedisClusterResourceManager.php @@ -104,7 +104,7 @@ private function createRedisResource(RedisClusterOptions $options): RedisCluster $options->getReadTimeout(), $options->isPersistent(), $password, - $options->getSslContext()?->getArrayCopy() + $options->getSslContext()?->toSslContextArray() ); } @@ -138,7 +138,7 @@ private function createRedisResourceFromName( $readTimeout, $persistent, $password, - $sslContext?->getArrayCopy() + $sslContext?->toSslContextArray() ); } diff --git a/src/SslContext.php b/src/SslContext.php index 059e15c..d9209a2 100644 --- a/src/SslContext.php +++ b/src/SslContext.php @@ -4,197 +4,217 @@ namespace Laminas\Cache\Storage\Adapter; -use InvalidArgumentException; -use Laminas\Stdlib\ArraySerializableInterface; -use Symfony\Component\Serializer\NameConverter\CamelCaseToSnakeCaseNameConverter; - -use function boolval; -use function get_object_vars; -use function property_exists; -use function sprintf; - -use const OPENSSL_DEFAULT_STREAM_CIPHERS; -use const OPENSSL_TLSEXT_SERVER_NAME; +use SensitiveParameter; /** * Class containing the SSL context options in its fields. * * @link https://www.php.net/manual/en/context.ssl.php + * + * @psalm-type SSLContextArrayShape = array{ + * peer_name?: non-empty-string, + * verify_peer?: bool, + * verify_peer_name?: bool, + * allow_self_signed?: bool, + * cafile?: non-empty-string, + * capath?: non-empty-string, + * local_cert?: non-empty-string, + * local_pk?: non-empty-string, + * passphrase?: non-empty-string, + * verify_depth?: non-negative-int, + * ciphers?: non-empty-string, + * SNI_enabled?: bool, + * disable_compression?: bool, + * peer_fingerprint?: non-empty-string|array, + * security_level?: non-negative-int + * } */ -final class SslContext implements ArraySerializableInterface +final class SslContext { /** - * Peer name to be used. - * If this value is not set, then the name is guessed based on the hostname used when opening the stream. - */ - private ?string $peerName; - - /** - * Require verification of SSL certificate used. + * @param non-empty-string|null $expectedPeerName + * @param non-empty-string|null $certificateAuthorityFile + * @param non-empty-string|null $certificateAuthorityPath + * @param non-empty-string|null $localCertificatePath + * @param non-empty-string|null $localPrivateKeyPath + * @param non-empty-string|null $passphrase + * @param non-negative-int|null $verifyDepth + * @param non-empty-string|null $ciphers + * @param non-empty-string|array|null $peerFingerprint + * @param non-negative-int|null $securityLevel */ - private bool $verifyPeer; - - /** - * Require verification of peer name. - */ - private bool $verifyPeerName; - - /** - * Allow self-signed certificates. Requires verifyPeer. - */ - private bool $allowSelfSigned; + public function __construct( + /** + * Peer name to be used. + * If this value is not set, then the name is guessed based on the hostname used when opening the stream. + */ + public readonly ?string $expectedPeerName = null, + /** + * Require verification of SSL certificate used. + */ + public readonly ?bool $verifyPeer = null, + /** + * Require verification of peer name. + */ + public readonly ?bool $verifyPeerName = null, + /** + * Allow self-signed certificates. Requires verifyPeer. + */ + public readonly ?bool $allowSelfSignedCertificates = null, + /** + * Location of Certificate Authority file on local filesystem which should be used with the verifyPeer + * context option to authenticate the identity of the remote peer. + */ + public readonly ?string $certificateAuthorityFile = null, + /** + * If cafile is not specified or if the certificate is not found there, the directory pointed to by capath is + * searched for a suitable certificate. capath must be a correctly hashed certificate directory. + */ + public readonly ?string $certificateAuthorityPath = null, + /** + * Path to local certificate file on filesystem. It must be a PEM encoded file which contains your certificate + * and private key. It can optionally contain the certificate chain of issuers. + * The private key also may be contained in a separate file specified by localPk. + */ + public readonly ?string $localCertificatePath = null, + /** + * Path to local private key file on filesystem in case of separate files for certificate (localCert) + * and private key. + */ + public readonly ?string $localPrivateKeyPath = null, + /** + * Passphrase with which your localCert file was encoded. + */ + #[SensitiveParameter] + public readonly ?string $passphrase = null, + /** + * Abort if the certificate chain is too deep. + * If not set, defaults to no verification. + */ + public readonly ?int $verifyDepth = null, + /** + * Sets the list of available ciphers. The format of the string is described in + * https://www.openssl.org/docs/manmaster/man1/ciphers.html#CIPHER-LIST-FORMAT + */ + public readonly ?string $ciphers = null, + /** + * If set to true server name indication will be enabled. Enabling SNI allows multiple certificates on the same + * IP address. + * If not set, will automatically be enabled if SNI support is available. + */ + public readonly ?bool $serverNameIndicationEnabled = null, + /** + * If set, disable TLS compression. This can help mitigate the CRIME attack vector. + */ + public readonly ?bool $disableCompression = null, + /** + * Aborts when the remote certificate digest doesn't match the specified hash. + * + * When a string is used, the length will determine which hashing algorithm is applied, + * either "md5" (32) or "sha1" (40). + * + * When an array is used, the keys indicate the hashing algorithm name and each corresponding + * value is the expected digest. + */ + public readonly array|string|null $peerFingerprint = null, + /** + * Sets the security level. If not specified the library default security level is used. The security levels are + * described in https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_get_security_level.html. + */ + public readonly ?int $securityLevel = null, + ) { + } /** - * Location of Certificate Authority file on local filesystem which should be used with the verifyPeer - * context option to authenticate the identity of the remote peer. + * @param SSLContextArrayShape $context */ - private ?string $cafile; + public static function fromSslContextArray(array $context): self + { + return new self( + $context['peer_name'] ?? null, + $context['verify_peer'] ?? null, + $context['verify_peer_name'] ?? null, + $context['allow_self_signed'] ?? null, + $context['cafile'] ?? null, + $context['capath'] ?? null, + $context['local_cert'] ?? null, + $context['local_pk'] ?? null, + $context['passphrase'] ?? null, + $context['verify_depth'] ?? null, + $context['ciphers'] ?? null, + $context['SNI_enabled'] ?? null, + $context['disable_compression'] ?? null, + $context['peer_fingerprint'] ?? null, + $context['security_level'] ?? null, + ); + } /** - * If cafile is not specified or if the certificate is not found there, the directory pointed to by capath is - * searched for a suitable certificate. capath must be a correctly hashed certificate directory. + * @return SSLContextArrayShape */ - private ?string $capath; + public function toSslContextArray(): array + { + $context = []; + if ($this->expectedPeerName !== null) { + $context['peer_name'] = $this->expectedPeerName; + } - /** - * Path to local certificate file on filesystem. It must be a PEM encoded file which contains your certificate and - * private key. It can optionally contain the certificate chain of issuers. - * The private key also may be contained in a separate file specified by localPk. - */ - private ?string $localCert; + if ($this->verifyPeer !== null) { + $context['verify_peer'] = $this->verifyPeer; + } - /** - * Path to local private key file on filesystem in case of separate files for certificate (localCert) - * and private key. - */ - private ?string $localPk; + if ($this->verifyPeerName !== null) { + $context['verify_peer_name'] = $this->verifyPeerName; + } - /** - * Passphrase with which your localCert file was encoded. - */ - private ?string $passphrase; + if ($this->allowSelfSignedCertificates !== null) { + $context['allow_self_signed'] = $this->allowSelfSignedCertificates; + } - /** - * Abort if the certificate chain is too deep. - * If not set, defaults to no verification. - */ - private ?int $verifyDepth; + if ($this->certificateAuthorityFile !== null) { + $context['cafile'] = $this->certificateAuthorityFile; + } - /** - * Sets the list of available ciphers. The format of the string is described in - * https://www.openssl.org/docs/manmaster/man1/ciphers.html#CIPHER-LIST-FORMAT - */ - private string $ciphers; + if ($this->certificateAuthorityPath !== null) { + $context['capath'] = $this->certificateAuthorityPath; + } - /** - * If set to true server name indication will be enabled. Enabling SNI allows multiple certificates on the same - * IP address. - * If not set, will automatically be enabled if SNI support is available. - */ - private ?bool $sniEnabled; + if ($this->localCertificatePath !== null) { + $context['local_cert'] = $this->localCertificatePath; + } - /** - * If set, disable TLS compression. This can help mitigate the CRIME attack vector. - */ - private bool $disableCompression; + if ($this->localPrivateKeyPath !== null) { + $context['local_pk'] = $this->localPrivateKeyPath; + } - /** - * Aborts when the remote certificate digest doesn't match the specified hash. - * - * When a string is used, the length will determine which hashing algorithm is applied, - * either "md5" (32) or "sha1" (40). - * - * When an array is used, the keys indicate the hashing algorithm name and each corresponding - * value is the expected digest. - */ - private string|array|null $peerFingerprint; + if ($this->passphrase !== null) { + $context['passphrase'] = $this->passphrase; + } - /** - * Sets the security level. If not specified the library default security level is used. The security levels are - * described in https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_get_security_level.html. - */ - private ?int $securityLevel; + if ($this->verifyDepth !== null) { + $context['verify_depth'] = $this->verifyDepth; + } - public function __construct( - ?string $peerName = null, - bool $verifyPeer = true, - bool $verifyPeerName = true, - bool $allowSelfSigned = false, - ?string $cafile = null, - ?string $capath = null, - ?string $localCert = null, - ?string $localPk = null, - ?string $passphrase = null, - ?int $verifyDepth = null, - string $ciphers = OPENSSL_DEFAULT_STREAM_CIPHERS, - ?bool $sniEnabled = null, - bool $disableCompression = true, - array|string|null $peerFingerprint = null, - ?int $securityLevel = null - ) { - $this->peerName = $peerName; - $this->verifyPeer = $verifyPeer; - $this->verifyPeerName = $verifyPeerName; - $this->allowSelfSigned = $allowSelfSigned; - $this->cafile = $cafile; - $this->capath = $capath; - $this->localCert = $localCert; - $this->localPk = $localPk; - $this->passphrase = $passphrase; - $this->verifyDepth = $verifyDepth; - $this->ciphers = $ciphers; - $this->sniEnabled = $sniEnabled; - $this->disableCompression = $disableCompression; - $this->peerFingerprint = $peerFingerprint; - $this->securityLevel = $securityLevel; - if ($sniEnabled === null) { - $this->sniEnabled = boolval(OPENSSL_TLSEXT_SERVER_NAME); + if ($this->ciphers !== null) { + $context['ciphers'] = $this->ciphers; } - } - public function exchangeArray(array $array): void - { - foreach ($array as $key => $value) { - $property = $this->mapArrayKeyToPropertyName($key); - if (! property_exists($this, $property)) { - throw new InvalidArgumentException( - sprintf( - '%s does not contain the property "%s" corresponding to the array key "%s"', - self::class, - $property, - $key - ) - ); - } - $this->$property = $value; + if ($this->serverNameIndicationEnabled !== null) { + $context['SNI_enabled'] = $this->serverNameIndicationEnabled; } - } - public function getArrayCopy(): array - { - $array = []; - foreach (get_object_vars($this) as $property => $value) { - if ($value !== null) { - $key = $this->mapPropertyNameToArrayKey($property); - $array[$key] = $value; - } + if ($this->disableCompression !== null) { + $context['disable_compression'] = $this->disableCompression; } - return $array; - } - private function mapArrayKeyToPropertyName(string $key): string - { - if ($key === 'SNI_enabled') { - return 'sniEnabled'; + if ($this->peerFingerprint !== null) { + $context['peer_fingerprint'] = $this->peerFingerprint; } - return (new CamelCaseToSnakeCaseNameConverter())->denormalize($key); - } - private function mapPropertyNameToArrayKey(string $property): string - { - if ($property === 'sniEnabled') { - return 'SNI_enabled'; + if ($this->securityLevel !== null) { + $context['security_level'] = $this->securityLevel; } - return (new CamelCaseToSnakeCaseNameConverter())->normalize($property); + + return $context; } } diff --git a/test/unit/RedisClusterOptionsTest.php b/test/unit/RedisClusterOptionsTest.php index fb8c2a7..93ae56e 100644 --- a/test/unit/RedisClusterOptionsTest.php +++ b/test/unit/RedisClusterOptionsTest.php @@ -34,11 +34,13 @@ public function testCanHandleOptionsWithSslContextObject(): void { $options = new RedisClusterOptions([ 'name' => 'foo', - 'ssl_context' => new SslContext(localCert: '/path/to/localcert'), + 'ssl_context' => new SslContext(localCertificatePath: '/path/to/localcert'), ]); - $this->assertEquals('foo', $options->getName()); - $this->assertEquals(new SslContext(localCert: '/path/to/localcert'), $options->getSslContext()); + self::assertEquals('foo', $options->getName()); + $sslContext = $options->getSslContext(); + self::assertNotNull($sslContext); + self::assertSame('/path/to/localcert', $sslContext->localCertificatePath); } public function testCanHandleOptionsWithSslContextArray(): void @@ -48,8 +50,10 @@ public function testCanHandleOptionsWithSslContextArray(): void 'ssl_context' => ['local_cert' => '/path/to/localcert'], ]); - $this->assertEquals('foo', $options->getName()); - $this->assertEquals(new SslContext(localCert: '/path/to/localcert'), $options->getSslContext()); + self::assertEquals('foo', $options->getName()); + $sslContext = $options->getSslContext(); + self::assertNotNull($sslContext); + self::assertSame('/path/to/localcert', $sslContext->localCertificatePath); } public function testCanHandleOptionsWithNodename(): void diff --git a/test/unit/SslContextTest.php b/test/unit/SslContextTest.php index c75f7a0..bd6ed42 100644 --- a/test/unit/SslContextTest.php +++ b/test/unit/SslContextTest.php @@ -4,80 +4,69 @@ namespace LaminasTest\Cache\Storage\Adapter; -use InvalidArgumentException; use Laminas\Cache\Storage\Adapter\SslContext; use PHPUnit\Framework\TestCase; use TypeError; -use function boolval; - -use const OPENSSL_DEFAULT_STREAM_CIPHERS; -use const OPENSSL_TLSEXT_SERVER_NAME; - final class SslContextTest extends TestCase { - private SslContext $correspondingSslContextObject; - private array $correspondingSslContextArray; - - protected function setUp(): void + private const SSL_CONTEXT = [ + 'peer_name' => 'some peer name', + 'verify_peer' => true, + 'verify_peer_name' => true, + 'allow_self_signed' => true, + 'cafile' => '/some/path/to/cafile.pem', + 'capath' => '/some/path/to/ca', + 'local_cert' => '/some/path/to/local.certificate.pem', + 'local_pk' => '/some/path/to/local.key', + 'passphrase' => 'secret', + 'verify_depth' => 10, + 'ciphers' => 'ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-GCM-SHA256:" . +"ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:" . +"DHE-DSS-AES128-GCM-SHA256:kEDH+AESGCM:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA256:" . +"ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA384:" . +"ECDHE-RSA-AES256-SHA:ECDHE-ECDSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:" . +"DHE-DSS-AES128-SHA256:DHE-RSA-AES256-SHA256:DHE-DSS-AES256-SHA:DHE-RSA-AES256-SHA:AES128-GCM-SHA256:" . +"AES256-GCM-SHA384:AES128:AES256:HIGH:!SSLv2:!aNULL:!eNULL:!EXPORT:!DES:!MD5:!RC4:!ADH', + 'SNI_enabled' => true, + 'disable_compression' => true, + 'peer_fingerprint' => ['md5' => 'some fingerprint'], + 'security_level' => 5, + ]; + + public function testWillNotGenerateContextIfNoneProvided(): void { - $this->correspondingSslContextObject = new SslContext( - peerName: 'some peer name', - allowSelfSigned: true, - verifyDepth: 10, - peerFingerprint: ['md5' => 'some fingerprint'] - ); - - $this->correspondingSslContextArray = [ - 'peer_name' => 'some peer name', - 'verify_peer' => true, - 'verify_peer_name' => true, - 'allow_self_signed' => true, - 'verify_depth' => 10, - 'ciphers' => OPENSSL_DEFAULT_STREAM_CIPHERS, - 'SNI_enabled' => boolval(OPENSSL_TLSEXT_SERVER_NAME), - 'disable_compression' => true, - 'peer_fingerprint' => ['md5' => 'some fingerprint'], - ]; + $context = new SslContext(); + self::assertSame([], $context->toSslContextArray()); } - public function testExchangeArraySetsPropertiesCorrectly(): void + public function testSetFromArraySetsPropertiesCorrectly(): void { - $sslContextObject = new SslContext(); - $sslContextObject->exchangeArray($this->correspondingSslContextArray); - - $this->assertEquals( - $this->correspondingSslContextObject, - $sslContextObject - ); - } - - public function testGetArrayCopyReturnsAnArrayWithPropertyValues() - { - $sslContextArray = $this->correspondingSslContextObject->getArrayCopy(); - - $this->assertEquals($this->correspondingSslContextArray, $sslContextArray); - } - - public function testExchangeArrayThrowsExceptionWhenProvidingInvalidKeyName(): void - { - $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessageMatches( - '/does not contain the property "someInvalidKey" corresponding to the array key "some_invalid_key"/' - ); - - $sslContextObject = new SslContext(); - $sslContextObject->exchangeArray(['some_invalid_key' => true]); + $context = SslContext::fromSslContextArray(self::SSL_CONTEXT); + + self::assertSame(self::SSL_CONTEXT['peer_name'], $context->expectedPeerName); + self::assertSame(self::SSL_CONTEXT['verify_peer'], $context->verifyPeer); + self::assertSame(self::SSL_CONTEXT['verify_peer_name'], $context->verifyPeerName); + self::assertSame(self::SSL_CONTEXT['allow_self_signed'], $context->allowSelfSignedCertificates); + self::assertSame(self::SSL_CONTEXT['cafile'], $context->certificateAuthorityFile); + self::assertSame(self::SSL_CONTEXT['capath'], $context->certificateAuthorityPath); + self::assertSame(self::SSL_CONTEXT['local_cert'], $context->localCertificatePath); + self::assertSame(self::SSL_CONTEXT['local_pk'], $context->localPrivateKeyPath); + self::assertSame(self::SSL_CONTEXT['passphrase'], $context->passphrase); + self::assertSame(self::SSL_CONTEXT['verify_depth'], $context->verifyDepth); + self::assertSame(self::SSL_CONTEXT['ciphers'], $context->ciphers); + self::assertSame(self::SSL_CONTEXT['SNI_enabled'], $context->serverNameIndicationEnabled); + self::assertSame(self::SSL_CONTEXT['disable_compression'], $context->disableCompression); + self::assertSame(self::SSL_CONTEXT['peer_fingerprint'], $context->peerFingerprint); + self::assertSame(self::SSL_CONTEXT['security_level'], $context->securityLevel); } - public function testExchangeArrayThrowsExceptionWhenProvidingInvalidValueType(): void + public function testSetFromArrayThrowsTypeErrorWhenProvidingInvalidValueType(): void { $this->expectException(TypeError::class); - $this->expectExceptionMessageMatches( - '/\$verifyPeer of type bool/' - ); + $this->expectExceptionMessageMatches('/\(\$verifyPeer\) must be of type \?bool, string given/'); - $sslContextObject = new SslContext(); - $sslContextObject->exchangeArray(['verify_peer' => 'invalid type']); + /** @psalm-suppress InvalidArgument We do want to verify what happens when invalid types are passed. */ + SslContext::fromSslContextArray(['verify_peer' => 'invalid type']); } }