diff --git a/composer.json b/composer.json index 2b9f44c97..f33820147 100644 --- a/composer.json +++ b/composer.json @@ -42,7 +42,8 @@ "mockery/mockery": "~1.6", "simplesamlphp/simplesamlphp": "^2.5", "simplesamlphp/simplesamlphp-test-framework": "~1.11", - "icanhazstring/composer-unused": "^0.9.6" + "icanhazstring/composer-unused": "^0.9.6", + "maglnet/composer-require-checker": "^4.20" }, "suggest": { "ext-soap": "*" diff --git a/src/Binding/HTTPArtifact.php b/src/Binding/HTTPArtifact.php index 9fd84dedd..b5262fa20 100644 --- a/src/Binding/HTTPArtifact.php +++ b/src/Binding/HTTPArtifact.php @@ -24,14 +24,20 @@ use SimpleSAML\SAML2\XML\samlp\ArtifactResponse; use SimpleSAML\Store\StoreFactory; use SimpleSAML\Utils\HTTP; +use SimpleSAML\XMLSchema\Type\AnyURIValue; use SimpleSAML\XMLSecurity\Alg\Signature\SignatureAlgorithmFactory; +use SimpleSAML\XMLSecurity\Key\PublicKey; use SimpleSAML\XMLSecurity\TestUtils\PEMCertificatesMock; use function array_key_exists; use function base64_decode; use function base64_encode; use function bin2hex; +use function chunk_split; +use function file_exists; use function hexdec; +use function openssl_pkey_get_details; +use function openssl_pkey_get_public; use function openssl_random_pseudo_bytes; use function pack; use function sha1; @@ -76,7 +82,9 @@ public function getRedirectURL(AbstractMessage $message): string if ($issuer === null) { throw new Exception('Cannot get redirect URL, no Issuer set in the message.'); } - $artifact = base64_encode("\x00\x04\x00\x00" . sha1($issuer->getContent(), true) . $generatedId); + $artifact = base64_encode( + "\x00\x04\x00\x00" . sha1((string)$issuer->getContent(), true) . $generatedId, + ); $artifactData = $message->toXML(); $artifactDataString = $artifactData->ownerDocument?->saveXML($artifactData); @@ -96,7 +104,7 @@ public function getRedirectURL(AbstractMessage $message): string } $httpUtils = new HTTP(); - return $httpUtils->addURLparameters($destination, $params); + return $httpUtils->addURLparameters((string)$destination, $params); } @@ -184,6 +192,8 @@ public function receive(ServerRequestInterface $request): AbstractMessage throw new Exception('Received error from ArtifactResolutionService.'); } + $artifactResponse = $this->verifyArtifactResponseSignature($artifactResponse, $idpMetadata); + $samlResponse = $artifactResponse->getMessage(); if ($samlResponse === null) { /* Empty ArtifactResponse - possibly because of Artifact replay? */ @@ -218,4 +228,70 @@ public function setSPMetadata(Configuration $sp): void { $this->spMetadata = $sp; } + + + /** + * Verify the ArtifactResponse signature using IdP metadata keys. + * + * Returns the verified ArtifactResponse instance. + * + * @throws \Exception When unsigned, when metadata has no signing keys, or when verification fails. + */ + private function verifyArtifactResponseSignature( + ArtifactResponse $artifactResponse, + Configuration $idpMetadata, + ): ArtifactResponse { + if ($artifactResponse->isSigned() !== true) { + throw new Exception('ArtifactResponse must be signed.'); + } + + $keys = $idpMetadata->getPublicKeys('signing', true); + if (empty($keys)) { + throw new Exception('No signing keys found in IdP metadata.'); + } + + $alg = $artifactResponse->getSignature()->getSignedInfo()->getSignatureMethod()->getAlgorithm(); + $signatureMethod = $alg instanceof AnyURIValue ? $alg->getValue() : (string) $alg; + + $factory = new SignatureAlgorithmFactory(); + + $lastException = null; + foreach ($keys as $k) { + if (($k['type'] ?? null) !== 'X509Certificate') { + continue; + } + + $pemCert = "-----BEGIN CERTIFICATE-----\n" . + chunk_split($k['X509Certificate'], 64) . + "-----END CERTIFICATE-----\n"; + + $opensslKey = openssl_pkey_get_public($pemCert); + if ($opensslKey === false) { + $lastException = new Exception('Unable to extract public key from X509 certificate.'); + continue; + } + + $keyInfo = openssl_pkey_get_details($opensslKey); + if ($keyInfo === false || !isset($keyInfo['key']) || !is_string($keyInfo['key'])) { + $lastException = new Exception('Unable to get public key details from X509 certificate.'); + continue; + } + + $pemPublicKey = $keyInfo['key']; + + $file = Utils::getContainer()->getTempDir() . '/' . sha1($pemPublicKey) . '.pem'; + if (!file_exists($file)) { + Utils::getContainer()->writeFile($file, $pemPublicKey); + } + + try { + $verifier = $factory->getAlgorithm($signatureMethod, PublicKey::fromFile($file)); + return $artifactResponse->verify($verifier); + } catch (\Exception $e) { + $lastException = $e; + } + } + + throw $lastException ?? new Exception('Unable to verify ArtifactResponse signature.'); + } } diff --git a/src/SOAPClient.php b/src/SOAPClient.php index 6174c05ec..6163756fb 100644 --- a/src/SOAPClient.php +++ b/src/SOAPClient.php @@ -6,7 +6,7 @@ use DOMDocument; use Exception; -use RobRichards\XMLSecLibs\XMLSecurityKey; +use OpenSSLAsymmetricKey; use SimpleSAML\Configuration; use SimpleSAML\SAML2\Compat\ContainerSingleton; use SimpleSAML\SAML2\XML\samlp\AbstractMessage; @@ -24,12 +24,17 @@ use function chunk_split; use function file_exists; +use function is_object; +use function is_string; +use function method_exists; use function openssl_pkey_get_details; use function openssl_pkey_get_public; +use function property_exists; use function sha1; use function sprintf; use function stream_context_create; use function stream_context_get_options; +use function trim; /** * Implementation of the SAML 2.0 SOAP binding. @@ -251,7 +256,7 @@ private static function addSSLValidator(AbstractMessage $msg, $context): void return; } - if (!isset($keyInfo['key'])) { + if (!isset($keyInfo['key']) || !is_string($keyInfo['key'])) { $container->getLogger()->warning('Missing key in public key details.'); return; } @@ -263,29 +268,88 @@ private static function addSSLValidator(AbstractMessage $msg, $context): void /** * Validate a SOAP message against the certificate on the SSL connection. * - * @param string $data The public key that was used on the connection. - * @param \RobRichards\XMLSecLibs\XMLSecurityKey $key The key we should validate the certificate against. + * @param string $data The public key (PEM) that was used on the connection. + * @param mixed $key The key we should validate the certificate against. * @throws \Exception */ - public static function validateSSL(string $data, XMLSecurityKey $key): void + public static function validateSSL(string $data, mixed $key): void { $container = ContainerSingleton::getInstance(); - $keyInfo = openssl_pkey_get_details($key->key); + $pem = self::extractPublicKeyPem($key); - if ($keyInfo === false) { - throw new Exception('Unable to get key details from XMLSecurityKey.'); + if (trim($pem) !== trim($data)) { + throw new Exception('Key on SSL connection did not match key we validated against.'); } - if (!isset($keyInfo['key'])) { - throw new Exception('Missing key in public key details.'); + $container->getLogger()->debug('Message validated based on SSL certificate.'); + } + + + /** + * Extract a PEM-encoded public key from different key representations. + * + * This avoids coupling to a specific XML security backend. + * + * @param mixed $key + * @throws \Exception + */ + private static function extractPublicKeyPem(mixed $key): string + { + // If the validating key is already PEM, normalize it by re-loading through OpenSSL. + if (is_string($key)) { + $opensslKey = openssl_pkey_get_public($key); + if ($opensslKey === false) { + throw new Exception('Unable to load validating public key from PEM string.'); + } + + $keyInfo = openssl_pkey_get_details($opensslKey); + if ($keyInfo === false || !isset($keyInfo['key']) || !is_string($keyInfo['key'])) { + throw new Exception('Unable to get key details from validating PEM key.'); + } + + return $keyInfo['key']; } - if (trim($keyInfo['key']) !== trim($data)) { - throw new Exception('Key on SSL connection did not match key we validated against.'); + // Some key implementations may expose PEM via a method. + if (is_object($key)) { + foreach (['getPublicKeyPem', 'getPem', 'toPEM', 'toPem'] as $method) { + if (method_exists($key, $method)) { + /** @var mixed $pem */ + $pem = $key->{$method}(); + if (is_string($pem) && $pem !== '') { + return self::extractPublicKeyPem($pem); + } + } + } + + // Common compatibility case: an object wraps an OpenSSL key or PEM in a public "key" property. + if (property_exists($key, 'key')) { + /** @var mixed $inner */ + $inner = $key->key; + + if (is_string($inner)) { + return self::extractPublicKeyPem($inner); + } + + if ($inner instanceof OpenSSLAsymmetricKey) { + $keyInfo = openssl_pkey_get_details($inner); + if ($keyInfo !== false && isset($keyInfo['key']) && is_string($keyInfo['key'])) { + return $keyInfo['key']; + } + } + } } - $container->getLogger()->debug('Message validated based on SSL certificate.'); + // Last attempt: OpenSSL might accept the value directly (OpenSSLAsymmetricKey). + if ($key instanceof OpenSSLAsymmetricKey) { + $keyInfo = openssl_pkey_get_details($key); + if ($keyInfo !== false && isset($keyInfo['key']) && is_string($keyInfo['key'])) { + return $keyInfo['key']; + } + } + + throw new Exception('Unable to extract public key PEM from validating key.'); } diff --git a/tests/SAML2/Binding/HTTPArtifactTest.php b/tests/SAML2/Binding/HTTPArtifactTest.php index 460a1cf74..30a2773a8 100644 --- a/tests/SAML2/Binding/HTTPArtifactTest.php +++ b/tests/SAML2/Binding/HTTPArtifactTest.php @@ -4,12 +4,22 @@ namespace SimpleSAML\Test\SAML2\Binding; +use DOMDocument; use Exception; use Nyholm\Psr7\ServerRequest; use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\TestCase; +use ReflectionMethod; +use SimpleSAML\Configuration; use SimpleSAML\SAML2\Binding\HTTPArtifact; +use SimpleSAML\SAML2\XML\samlp\ArtifactResponse; +use SimpleSAML\XMLSecurity\TestUtils\PEMCertificatesMock; +use SimpleSAML\XMLSecurity\XML\ds\Signature; + +use function htmlspecialchars; +use function method_exists; /** * @package simplesamlphp\saml2 @@ -34,4 +44,175 @@ public function testArtifactMissingUrlParamThrowsException(): void $this->expectExceptionMessage('Missing SAMLart parameter.'); $ha->receive($request); } + + + /** + * Distinguish the legacy and the new signature-validation APIs: + * - Legacy messages: validate(XMLSecurityKey $key) + * - New XML model: verify($verifier) returning a verified instance + */ + public function testLegacyVsNewSignatureApiIsDifferent(): void + { + $legacyClass = \SAML2\Message::class; + $newClass = \SimpleSAML\SAML2\XML\samlp\AbstractMessage::class; + + /** @phpstan-ignore-next-line */ + $this->assertTrue(method_exists($legacyClass, 'validate')); + $this->assertFalse(method_exists($legacyClass, 'verify')); + + /** @phpstan-ignore-next-line */ + $this->assertTrue(method_exists($newClass, 'verify')); + $this->assertFalse(method_exists($newClass, 'validate')); + } + + + /** + * @return array< + * string, + * array{ + * signed: bool, + * verifyThrowsMessage: ?string, + * idpMetadata: \SimpleSAML\Configuration, + * expectedExceptionMessage: ?string + * } + * > + */ + public static function provideVerifyArtifactResponseSignatureCases(): array + { + $base64Cert = PEMCertificatesMock::getPlainCertificateContents(); + + $idpMetadataWithSigningKey = Configuration::loadFromArray( + [ + 'entityid' => 'https://idp.example.test', + 'keys' => [ + [ + 'type' => 'X509Certificate', + 'signing' => true, + 'encryption' => false, + 'X509Certificate' => $base64Cert, + ], + ], + ], + '[idp]', + ); + + $idpMetadataWithoutKeys = Configuration::loadFromArray( + [ + 'entityid' => 'https://idp.example.test', + ], + '[idp]', + ); + + return [ + 'unsigned ArtifactResponse => throws must be signed' => [ + 'signed' => false, + 'verifyThrowsMessage' => null, + 'idpMetadata' => $idpMetadataWithSigningKey, + 'expectedExceptionMessage' => 'ArtifactResponse must be signed.', + ], + 'signed ArtifactResponse but metadata has no keys => throws (metadata required)' => [ + 'signed' => true, + 'verifyThrowsMessage' => null, + 'idpMetadata' => $idpMetadataWithoutKeys, + 'expectedExceptionMessage' => 'Missing certificate in metadata.', + ], + 'signed ArtifactResponse but verification fails => throws verify exception' => [ + 'signed' => true, + 'verifyThrowsMessage' => 'Unable to validate Signature', + 'idpMetadata' => $idpMetadataWithSigningKey, + 'expectedExceptionMessage' => 'Unable to validate Signature', + ], + 'signed ArtifactResponse and verification ok => returns verified instance' => [ + 'signed' => true, + 'verifyThrowsMessage' => null, + 'idpMetadata' => $idpMetadataWithSigningKey, + 'expectedExceptionMessage' => null, + ], + ]; + } + + + #[DataProvider('provideVerifyArtifactResponseSignatureCases')] + public function testVerifyArtifactResponseSignatureBasicScenarios( + bool $signed, + ?string $verifyThrowsMessage, + Configuration $idpMetadata, + ?string $expectedExceptionMessage, + ): void { + $artifactResponse = $this->buildArtifactResponseStub($signed, $verifyThrowsMessage); + + if ($expectedExceptionMessage !== null) { + $this->expectException(Exception::class); + $this->expectExceptionMessage($expectedExceptionMessage); + } + + $ha = new HTTPArtifact(); + $verified = $this->callVerifyArtifactResponseSignature($ha, $artifactResponse, $idpMetadata); + + if ($expectedExceptionMessage === null) { + $this->assertSame($artifactResponse, $verified); + } + } + + + private function callVerifyArtifactResponseSignature( + HTTPArtifact $ha, + ArtifactResponse $artifactResponse, + Configuration $idpMetadata, + ): ArtifactResponse { + $m = new ReflectionMethod(HTTPArtifact::class, 'verifyArtifactResponseSignature'); + /** @var \SimpleSAML\SAML2\XML\samlp\ArtifactResponse $result */ + $result = $m->invoke($ha, $artifactResponse, $idpMetadata); + return $result; + } + + + private function buildArtifactResponseStub(bool $signed, ?string $verifyThrowsMessage): ArtifactResponse + { + $stub = $this->createStub(ArtifactResponse::class); + + $stub->method('isSigned')->willReturn($signed); + + if ($signed) { + $stub->method('getSignature')->willReturn( + self::buildMinimalDsSignature('http://www.w3.org/2001/04/xmldsig-more#rsa-sha256'), + ); + } else { + $stub->method('getSignature')->willReturn(null); + } + + if ($verifyThrowsMessage !== null) { + $stub->method('verify')->willThrowException(new Exception($verifyThrowsMessage)); + } else { + $stub->method('verify')->willReturn($stub); + } + + return $stub; + } + + + private static function buildMinimalDsSignature(string $signatureAlgorithm): Signature + { + $xml = + '' . + '' . + '' . + '' . + '' . + '' . + '' . + '' . + '' . + '' . + 'AA==' . + '' . + '' . + 'AA==' . + ''; + + $doc = new DOMDocument('1.0', 'UTF-8'); + $doc->loadXML($xml); + + return Signature::fromXML($doc->documentElement); + } } diff --git a/tests/SAML2/SOAPClientTest.php b/tests/SAML2/SOAPClientTest.php index 4e37b8b3e..8b5f4087f 100644 --- a/tests/SAML2/SOAPClientTest.php +++ b/tests/SAML2/SOAPClientTest.php @@ -6,10 +6,11 @@ use DOMDocument; use Exception; +use OpenSSLAsymmetricKey; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; -use RobRichards\XMLSecLibs\XMLSecurityKey; +use ReflectionMethod; use SimpleSAML\Configuration; use SimpleSAML\SAML2\SOAPClient; use SimpleSAML\SAML2\Type\SAMLAnyURIValue; @@ -17,6 +18,10 @@ use SimpleSAML\XMLSchema\Type\AnyURIValue; use SimpleSAML\XMLSecurity\TestUtils\PEMCertificatesMock; +use function is_string; +use function openssl_pkey_get_public; +use function str_contains; + /** * Tests for {@see \SimpleSAML\SAML2\SOAPClient}: * - SSL peer key validation behavior ({@see \SimpleSAML\SAML2\SOAPClient::validateSSL()}) @@ -37,8 +42,8 @@ final class SOAPClientTest extends TestCase public static function provideSslKeyMatchCases(): array { return [ - 'tls key matches xml key' => [true], - 'tls key differs from xml key' => [false], + 'tls key matches validating key' => [true], + 'tls key differs from validating key' => [false], ]; } @@ -52,7 +57,7 @@ public static function provideSslKeyMatchCases(): array * * This test reuses deterministic key fixtures from simplesamlphp/xml-security via {@see PEMCertificatesMock}. * - * @param bool $shouldMatch Whether the peer key material and the XMLSecurityKey should match. + * @param bool $shouldMatch Whether the peer key material and the validating key should match. */ #[DataProvider('provideSslKeyMatchCases')] public function testValidateSslThrowsOnMismatchAndPassesOnMatch(bool $shouldMatch): void @@ -60,15 +65,15 @@ public function testValidateSslThrowsOnMismatchAndPassesOnMatch(bool $shouldMatc $tlsPublicKeyPem = PEMCertificatesMock::getPlainPublicKey(); $otherPublicKeyPem = PEMCertificatesMock::getPlainPublicKey(PEMCertificatesMock::OTHER_PUBLIC_KEY); - $xmlPublicKeyPem = $shouldMatch ? $tlsPublicKeyPem : $otherPublicKeyPem; - $key = $this->buildXmlSecurityPublicKey($xmlPublicKeyPem); + $validatingKeyPem = $shouldMatch ? $tlsPublicKeyPem : $otherPublicKeyPem; + $validatingKey = $this->buildOpenSslPublicKey($validatingKeyPem); if (!$shouldMatch) { $this->expectException(Exception::class); $this->expectExceptionMessage('Key on SSL connection did not match key we validated against.'); } - SOAPClient::validateSSL($tlsPublicKeyPem, $key); + SOAPClient::validateSSL($tlsPublicKeyPem, $validatingKey); if ($shouldMatch) { $this->addToAssertionCount(1); @@ -77,15 +82,17 @@ public function testValidateSslThrowsOnMismatchAndPassesOnMatch(bool $shouldMatc /** - * Build an {@see XMLSecurityKey} from a PEM-encoded public key, for use with - * {@see SOAPClient::validateSSL()}. + * Build an OpenSSL public key handle from a PEM-encoded public key, for use with + * {@see SOAPClient::validateSSL()} without relying on xmlseclibs types. * * @param string $publicKeyPem PEM-encoded public key (e.g. "-----BEGIN PUBLIC KEY----- ..."). */ - private function buildXmlSecurityPublicKey(string $publicKeyPem): XMLSecurityKey + private function buildOpenSslPublicKey(string $publicKeyPem): OpenSSLAsymmetricKey { - $key = new XMLSecurityKey(XMLSecurityKey::RSA_SHA256, ['type' => 'public']); - $key->loadKey($publicKeyPem, false); + $key = openssl_pkey_get_public($publicKeyPem); + if ($key === false) { + throw new Exception('Unable to load OpenSSL public key from PEM fixture.'); + } return $key; } @@ -211,4 +218,104 @@ protected function doSoapRequest( $client->send($msg, $src, null); } + + + /** + * @return array + */ + public static function provideExtractPublicKeyPemCases(): array + { + $validPem = PEMCertificatesMock::getPlainPublicKey(); + $otherPem = PEMCertificatesMock::getPlainPublicKey(PEMCertificatesMock::OTHER_PUBLIC_KEY); + + $validOpenSslKey = openssl_pkey_get_public($validPem); + + return [ + 'string: invalid pem => throws unable to load' => [ + 'key' => 'not a pem', + 'expectedExceptionMessage' => 'Unable to load validating public key from PEM string.', + ], + 'object: getPem() returns invalid => throws unable to load' => [ + 'key' => new class { + public function getPem(): string + { + return 'not a pem'; + } + }, + 'expectedExceptionMessage' => 'Unable to load validating public key from PEM string.', + ], + 'object: toPem() returns empty => falls through => throws unable to extract' => [ + 'key' => new class { + public function toPem(): string + { + return ''; + } + }, + 'expectedExceptionMessage' => 'Unable to extract public key PEM from validating key.', + ], + 'object: getPublicKeyPem() returns valid pem => ok' => [ + 'key' => new class ($validPem) { + public function __construct(private readonly string $pem) + { + } + + + public function getPublicKeyPem(): string + { + return $this->pem; + } + }, + 'expectedExceptionMessage' => null, + ], + 'object: public key property contains pem => ok (and differs from tls key is irrelevant here)' => [ + 'key' => new class ($otherPem) { + public function __construct(public string $key) + { + } + }, + 'expectedExceptionMessage' => null, + ], + 'object: public key property contains OpenSSL key => ok' => [ + 'key' => new class ($validOpenSslKey) { + public function __construct(public mixed $key) + { + } + }, + 'expectedExceptionMessage' => null, + ], + 'unsupported scalar => throws unable to extract' => [ + 'key' => 123, + 'expectedExceptionMessage' => 'Unable to extract public key PEM from validating key.', + ], + ]; + } + + + #[DataProvider('provideExtractPublicKeyPemCases')] + public function testExtractPublicKeyPemCoversThrowPathsAndSuccessCases( + mixed $key, + ?string $expectedExceptionMessage, + ): void { + if ($expectedExceptionMessage !== null) { + $this->expectException(Exception::class); + $this->expectExceptionMessage($expectedExceptionMessage); + } + + $pem = $this->callExtractPublicKeyPem($key); + + if ($expectedExceptionMessage === null) { + $this->assertTrue(str_contains($pem, 'BEGIN PUBLIC KEY') || str_contains($pem, 'BEGIN CERTIFICATE')); + } + } + + + private function callExtractPublicKeyPem(mixed $key): string + { + $m = new ReflectionMethod(SOAPClient::class, 'extractPublicKeyPem'); + + $result = $m->invoke(null, $key); + $this->assertTrue(is_string($result)); + + return $result; + } }