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

ioigoume · tvdijen · commit ba044395697f · 2026-07-06T19:08:17.000+02:00
diff --git a/.github/workflows/php.yml b/.github/workflows/php.yml
@@ -74,6 +74,15 @@ jobs:
       - name: Get composer cache directory
         run: echo COMPOSER_CACHE="$(composer config cache-files-dir)" >> "$GITHUB_ENV"
 
+      - name: Get COMPOSER_ROOT_VERSION from composer.json branch alias
+        run: |
+            ROOT_VERSION=$(composer config extra.branch-alias.dev-master)
+            if [ -z "$ROOT_VERSION" ]; then
+              echo "Could not read extra.branch-alias.dev-master from composer.json" >&2
+              exit 1
+            fi
+            echo "COMPOSER_ROOT_VERSION=$ROOT_VERSION" >> "$GITHUB_ENV"
+
       - name: Cache composer dependencies
         uses: actions/cache@v4
         with:
@@ -136,6 +145,15 @@ jobs:
       - name: Get composer cache directory
         run: echo COMPOSER_CACHE="$(composer config cache-files-dir)" >> "$env:GITHUB_ENV"
 
+      - name: Get COMPOSER_ROOT_VERSION from composer.json branch alias
+        run: |
+            ROOT_VERSION=$(composer config extra.branch-alias.dev-master)
+            if [ -z "$ROOT_VERSION" ]; then
+              echo "Could not read extra.branch-alias.dev-master from composer.json" >&2
+              exit 1
+            fi
+            echo "COMPOSER_ROOT_VERSION=$ROOT_VERSION" >> "$GITHUB_ENV"
+
       - name: Cache composer dependencies
         uses: actions/cache@v4
         with:
@@ -173,6 +191,15 @@ jobs:
       - name: Get composer cache directory
         run: echo COMPOSER_CACHE="$(composer config cache-files-dir)" >> "$GITHUB_ENV"
 
+      - name: Get COMPOSER_ROOT_VERSION from composer.json branch alias
+        run: |
+            ROOT_VERSION=$(composer config extra.branch-alias.dev-master)
+            if [ -z "$ROOT_VERSION" ]; then
+              echo "Could not read extra.branch-alias.dev-master from composer.json" >&2
+              exit 1
+            fi
+            echo "COMPOSER_ROOT_VERSION=$ROOT_VERSION" >> "$GITHUB_ENV"
+
       - name: Cache composer dependencies
         uses: actions/cache@v4
         with:
@@ -226,6 +253,15 @@ jobs:
       - name: Get composer cache directory
         run: echo COMPOSER_CACHE="$(composer config cache-files-dir)" >> "$GITHUB_ENV"
 
+      - name: Get COMPOSER_ROOT_VERSION from composer.json branch alias
+        run: |
+            ROOT_VERSION=$(composer config extra.branch-alias.dev-master)
+            if [ -z "$ROOT_VERSION" ]; then
+              echo "Could not read extra.branch-alias.dev-master from composer.json" >&2
+              exit 1
+            fi
+            echo "COMPOSER_ROOT_VERSION=$ROOT_VERSION" >> "$GITHUB_ENV"
+
       - name: Cache composer dependencies
         uses: actions/cache@v4
         with:
diff --git a/composer.json b/composer.json
@@ -33,6 +33,7 @@
         "beste/clock": "~3.0.0",
         "mockery/mockery": "~1.6.12",
         "simplesamlphp/composer-xmlprovider-installer": "~1.0.2",
+        "simplesamlphp/simplesamlphp": "~2.3.0 | ~2.4.0",
         "simplesamlphp/simplesamlphp-test-framework": "~1.9.2"
     },
     "suggest": {
@@ -50,14 +51,15 @@
     },
     "extra": {
         "branch-alias": {
-            "dev-master": "v5.0.x-dev"
+            "dev-master": "v5.1.x-dev"
         }
     },
     "config": {
         "allow-plugins": {
             "composer/package-versions-deprecated": true,
             "dealerdirect/phpcodesniffer-composer-installer": true,
             "phpstan/extension-installer": true,
+            "simplesamlphp/composer-module-installer": true,
             "simplesamlphp/composer-xmlprovider-installer": true
         }
     }
diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -26,7 +26,12 @@ parameters:
 			path: src/Binding/HTTPArtifact.php
 
 		-
-			message: "#^Parameter \\$key of method SimpleSAML\\\\SAML2\\\\Binding\\\\HTTPArtifact\\:\\:validateSignature\\(\\) has invalid type SimpleSAML\\\\XMLSecurity\\\\XMLSecurityKey\\.$#"
+			message: "#^Parameter \\$idpMetadata of method SimpleSAML\\\\SAML2\\\\Binding\\\\HTTPArtifact\\:\\:verifyArtifactResponseSignature\\(\\) has invalid type SimpleSAML\\\\Configuration\\.$#"
+			count: 1
+			path: src/Binding/HTTPArtifact.php
+
+		-
+			message: "#^Parameter \\$idpMetadata of method SimpleSAML\\\\SAML2\\\\Binding\\\\HTTPArtifact\\:\\:verifyMessageSignature\\(\\) has invalid type SimpleSAML\\\\Configuration\\.$#"
 			count: 1
 			path: src/Binding/HTTPArtifact.php
 
diff --git a/src/Binding/HTTPArtifact.php b/src/Binding/HTTPArtifact.php
@@ -15,6 +15,7 @@
 use SimpleSAML\Module\saml\Message as MSG;
 use SimpleSAML\SAML2\Binding;
 use SimpleSAML\SAML2\Binding\RelayStateTrait;
+use SimpleSAML\SAML2\Compat\ContainerSingleton;
 use SimpleSAML\SAML2\SOAPClient;
 use SimpleSAML\SAML2\Utils;
 use SimpleSAML\SAML2\XML\saml\Issuer;
@@ -24,7 +25,8 @@
 use SimpleSAML\SAML2\XML\samlp\ArtifactResponse;
 use SimpleSAML\Store\StoreFactory;
 use SimpleSAML\Utils\HTTP;
-use SimpleSAML\XMLSecurity\XMLSecurityKey;
+use SimpleSAML\XMLSecurity\Alg\Signature\SignatureAlgorithmFactory;
+use SimpleSAML\XMLSecurity\Key\PublicKey;
 
 use function array_key_exists;
 use function base64_decode;
@@ -204,7 +206,11 @@ public function receive(ServerRequestInterface $request): AbstractMessage
             $this->setRelayState($query['RelayState']);
         }
 
-        return $samlResponse;
+        if (!$samlResponse->isSigned()) {
+            return $samlResponse;
+        }
+
+        return $this->verifyMessageSignature($samlResponse, $idpMetadata);
     }
 
 
@@ -220,15 +226,139 @@ public function setSPMetadata(Configuration $sp): void
 
 
     /**
-     * A validator which returns true if the ArtifactResponse was signed with the given key
+     * 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.
      *
      * @param \SimpleSAML\SAML2\XML\samlp\ArtifactResponse $message
      * @param \SimpleSAML\XMLSecurity\XMLSecurityKey $key
-     * @return bool
+     *
+     * Returns the verified ArtifactResponse instance.
+     *
+     * @throws \Exception When unsigned, when metadata has no signing keys, or when verification fails.
      */
-    public static function validateSignature(ArtifactResponse $message, XMLSecurityKey $key): bool
-    {
-        // @todo verify if this works and/or needs to do anything more. Ref. HTTPRedirect binding
-        return $message->validate($key);
+    private function verifyArtifactResponseSignature(
+        ArtifactResponse $artifactResponse,
+        Configuration $idpMetadata,
+    ): ArtifactResponse {
+        if ($artifactResponse->isSigned() !== true) {
+            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);
+
+        $signatureMethod = $artifactResponse
+            ->getSignature()
+            ->getSignedInfo()
+            ->getSignatureMethod()
+            ->getAlgorithm()
+            ->getValue();
+
+        $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
@@ -203,14 +203,14 @@ protected function createSoapClient(array $options): BuiltinSoapClient
      *
      * @param \SoapClient $client
      * @param string|null $request
-     * @param \SimpleSAML\SAML2\Type\SAMLAnyURIValue $destination
+     * @param string $destination
      * @param string $action
      * @return string
      */
     protected function doSoapRequest(
         BuiltinSoapClient $client,
         ?string $request,
-        SAMLAnyURIValue $destination,
+        string $destination,
         string $action,
     ): string {
         return (string) $client->__doRequest(
diff --git a/tests/SAML2/Binding/HTTPArtifactTest.php b/tests/SAML2/Binding/HTTPArtifactTest.php
