HTTP-Artifact: verify signatures using IdP metadata signing keys (fix TODO in verifier setup) (#419)

Author: ioigoume

src/Binding/HTTPArtifact.php

@@ -26,7 +26,6 @@
 use SimpleSAML\Utils\HTTP;
 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;
@@ -208,15 +207,7 @@ public function receive(ServerRequestInterface $request): AbstractMessage
             return $samlResponse;
         }
 
-        $container = ContainerSingleton::getInstance();
-        $blacklist = $container->getBlacklistedEncryptionAlgorithms();
-        $verifier = (new SignatureAlgorithmFactory($blacklist))->getAlgorithm(
-            $samlResponse->getSignature()->getSignedInfo()->getSignatureMethod()->getAlgorithm(),
-            // TODO:  Need to use the key from the metadata
-            PEMCertificatesMock::getPublicKey(PEMCertificatesMock::SELFSIGNED_PUBLIC_KEY),
-        );
-
-        return $samlResponse->verify($verifier);
+        return $this->verifyMessageSignature($samlResponse, $idpMetadata);
     }
 
 
@@ -229,6 +220,72 @@ public function setSPMetadata(Configuration $sp): void
     }
 
 
+    /**
+     * Verify the signature on a signed SAML message using IdP metadata keys.
+     *
+     * Returns the verified message instance.
+     *
+     * @throws \Exception When metadata has no signing keys, or when verification fails.
+     */
+    private function verifyMessageSignature(AbstractMessage $message, Configuration $idpMetadata): AbstractMessage
+    {
+        $container = ContainerSingleton::getInstance();
+        $blacklist = $container->getBlacklistedEncryptionAlgorithms();
+
+        // getPublicKeys(..., $required = true) throws if no signing cert/key material is present in metadata,
+        // so $keys is guaranteed non-empty here (no additional empty($keys) guard needed).
+        $keys = $idpMetadata->getPublicKeys('signing', true);
+
+        $signatureMethod = $message
+            ->getSignature()
+            ->getSignedInfo()
+            ->getSignatureMethod()
+            ->getAlgorithm()
+            ->getValue();
+
+        $factory = new SignatureAlgorithmFactory($blacklist);
+
+        $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 $message->verify($verifier);
+            } catch (Exception $e) {
+                $lastException = $e;
+            }
+        }
+
+        throw $lastException ?? new Exception('Unable to verify message signature.');
+    }
+
+
     /**
      * Verify the ArtifactResponse signature using IdP metadata keys.
      *
@@ -244,10 +301,9 @@ private function verifyArtifactResponseSignature(
             throw new Exception('ArtifactResponse must be signed.');
         }
 
+        // getPublicKeys(..., $required = true) throws if no signing cert/key material is present in metadata,
+        // so $keys is guaranteed non-empty here (no additional empty($keys) guard needed).
         $keys = $idpMetadata->getPublicKeys('signing', true);
-        if (empty($keys)) {
-            throw new Exception('No signing keys found in IdP metadata.');
-        }
 
         $signatureMethod = $artifactResponse
             ->getSignature()

tests/SAML2/Binding/HTTPArtifactTest.php

@@ -14,6 +14,7 @@
 use ReflectionMethod;
 use SimpleSAML\Configuration;
 use SimpleSAML\SAML2\Binding\HTTPArtifact;
+use SimpleSAML\SAML2\XML\samlp\AbstractMessage;
 use SimpleSAML\SAML2\XML\samlp\ArtifactResponse;
 use SimpleSAML\XMLSecurity\TestUtils\PEMCertificatesMock;
 use SimpleSAML\XMLSecurity\XML\ds\Signature;
@@ -155,6 +156,84 @@ public function testVerifyArtifactResponseSignatureBasicScenarios(
     }
 
 
+    /**
+     * @return array<
+     *     string,
+     *     array{
+     *         verifyThrowsMessage: ?string,
+     *         idpMetadata: \SimpleSAML\Configuration,
+     *         expectedExceptionMessage: ?string
+     *     }
+     * >
+     */
+    public static function provideVerifyMessageSignatureCases(): 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 [
+            'signed message but metadata has no keys => throws (metadata required)' => [
+                'verifyThrowsMessage' => null,
+                'idpMetadata' => $idpMetadataWithoutKeys,
+                'expectedExceptionMessage' => 'Missing certificate in metadata.',
+            ],
+            'signed message but verification fails => throws verify exception' => [
+                'verifyThrowsMessage' => 'Unable to validate Signature',
+                'idpMetadata' => $idpMetadataWithSigningKey,
+                'expectedExceptionMessage' => 'Unable to validate Signature',
+            ],
+            'signed message and verification ok => returns verified instance' => [
+                'verifyThrowsMessage' => null,
+                'idpMetadata' => $idpMetadataWithSigningKey,
+                'expectedExceptionMessage' => null,
+            ],
+        ];
+    }
+
+
+    #[DataProvider('provideVerifyMessageSignatureCases')]
+    public function testVerifyMessageSignatureBasicScenarios(
+        ?string $verifyThrowsMessage,
+        Configuration $idpMetadata,
+        ?string $expectedExceptionMessage,
+    ): void {
+        $message = $this->buildSignedMessageStub($verifyThrowsMessage);
+
+        if ($expectedExceptionMessage !== null) {
+            $this->expectException(Exception::class);
+            $this->expectExceptionMessage($expectedExceptionMessage);
+        }
+
+        $ha = new HTTPArtifact();
+        $verified = $this->callVerifyMessageSignature($ha, $message, $idpMetadata);
+
+        if ($expectedExceptionMessage === null) {
+            $this->assertSame($message, $verified);
+        }
+    }
+
+
     private function callVerifyArtifactResponseSignature(
         HTTPArtifact $ha,
         ArtifactResponse $artifactResponse,
@@ -167,6 +246,18 @@ private function callVerifyArtifactResponseSignature(
     }
 
 
+    private function callVerifyMessageSignature(
+        HTTPArtifact $ha,
+        AbstractMessage $message,
+        Configuration $idpMetadata,
+    ): AbstractMessage {
+        $m = new ReflectionMethod(HTTPArtifact::class, 'verifyMessageSignature');
+        /** @var \SimpleSAML\SAML2\XML\samlp\AbstractMessage $result */
+        $result = $m->invoke($ha, $message, $idpMetadata);
+        return $result;
+    }
+
+
     private function buildArtifactResponseStub(bool $signed, ?string $verifyThrowsMessage): ArtifactResponse
     {
         $stub = $this->createStub(ArtifactResponse::class);
@@ -191,6 +282,24 @@ private function buildArtifactResponseStub(bool $signed, ?string $verifyThrowsMe
     }
 
 
+    private function buildSignedMessageStub(?string $verifyThrowsMessage): AbstractMessage
+    {
+        $stub = $this->createStub(AbstractMessage::class);
+
+        $stub->method('getSignature')->willReturn(
+            self::buildMinimalDsSignature('http://www.w3.org/2001/04/xmldsig-more#rsa-sha256'),
+        );
+
+        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 =