Skip to content

Commit c0f7417

Browse files
ioigoumeioigoume
committed
Remove xmlseclibs coupling by switching SOAP/Artifact validators to OpenSSL-based key extraction and updating tests
1 parent e4fa6ea commit c0f7417

6 files changed

Lines changed: 453 additions & 51 deletions

File tree

‎composer.json‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,7 @@
3333
"simplesamlphp/assert": "~2.0",
3434
"simplesamlphp/xml-common": "~2.8",
3535
"simplesamlphp/xml-security": "~2.3",
36-
"simplesamlphp/xml-soap": "~2.3",
37-
"robrichards/xmlseclibs": "^3.1"
36+
"simplesamlphp/xml-soap": "~2.3"
3837
},
3938
"require-dev": {
4039
"ext-intl": "*",
@@ -43,7 +42,8 @@
4342
"mockery/mockery": "~1.6",
4443
"simplesamlphp/simplesamlphp": "^2.5",
4544
"simplesamlphp/simplesamlphp-test-framework": "~1.11",
46-
"icanhazstring/composer-unused": "^0.9.6"
45+
"icanhazstring/composer-unused": "^0.9.6",
46+
"maglnet/composer-require-checker": "^4.20"
4747
},
4848
"suggest": {
4949
"ext-soap": "*"

‎phpstan-baseline.neon‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,5 @@
11
parameters:
22
ignoreErrors:
3-
-
4-
message: '#^Call to an undefined method SimpleSAML\\SAML2\\XML\\samlp\\AbstractMessage\:\:addValidator\(\)\.$#'
5-
identifier: method.notFound
6-
path: src/Binding/HTTPArtifact.php
7-
8-
-
9-
message: '#^Call to an undefined method SimpleSAML\\SAML2\\XML\\samlp\\ArtifactResponse\:\:validate\(\)\.$#'
10-
identifier: method.notFound
11-
path: src/Binding/HTTPArtifact.php
12-
133
-
144
message: '#^Call to an undefined method SimpleSAML\\SAML2\\XML\\samlp\\AbstractMessage\:\:addValidator\(\)\.$#'
155
identifier: method.notFound

‎src/Binding/HTTPArtifact.php‎

Lines changed: 73 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
use Nyholm\Psr7\Response;
1010
use Psr\Http\Message\ResponseInterface;
1111
use Psr\Http\Message\ServerRequestInterface;
12-
use RobRichards\XMLSecLibs\XMLSecurityKey;
1312
use SimpleSAML\Configuration;
1413
use SimpleSAML\Metadata\MetaDataStorageHandler;
1514
use SimpleSAML\Module\saml\Message as MSG;
@@ -24,12 +23,19 @@
2423
use SimpleSAML\SAML2\XML\samlp\ArtifactResponse;
2524
use SimpleSAML\Store\StoreFactory;
2625
use SimpleSAML\Utils\HTTP;
26+
use SimpleSAML\XMLSchema\Type\AnyURIValue;
27+
use SimpleSAML\XMLSecurity\Alg\Signature\SignatureAlgorithmFactory;
28+
use SimpleSAML\XMLSecurity\Key\PublicKey;
2729

2830
use function array_key_exists;
2931
use function base64_decode;
3032
use function base64_encode;
3133
use function bin2hex;
34+
use function chunk_split;
35+
use function file_exists;
3236
use function hexdec;
37+
use function openssl_pkey_get_details;
38+
use function openssl_pkey_get_public;
3339
use function openssl_random_pseudo_bytes;
3440
use function pack;
3541
use function sha1;
@@ -74,7 +80,9 @@ public function getRedirectURL(AbstractMessage $message): string
7480
if ($issuer === null) {
7581
throw new Exception('Cannot get redirect URL, no Issuer set in the message.');
7682
}
77-
$artifact = base64_encode("\x00\x04\x00\x00" . sha1($issuer->getContent(), true) . $generatedId);
83+
$artifact = base64_encode(
84+
"\x00\x04\x00\x00" . sha1((string)$issuer->getContent(), true) . $generatedId,
85+
);
7886
$artifactData = $message->toXML();
7987
$artifactDataString = $artifactData->ownerDocument?->saveXML($artifactData);
8088

@@ -94,7 +102,7 @@ public function getRedirectURL(AbstractMessage $message): string
94102
}
95103

96104
$httpUtils = new HTTP();
97-
return $httpUtils->addURLparameters($destination, $params);
105+
return $httpUtils->addURLparameters((string)$destination, $params);
98106
}
99107

100108

@@ -180,15 +188,14 @@ public function receive(ServerRequestInterface $request): AbstractMessage
180188
throw new Exception('Received error from ArtifactResolutionService.');
181189
}
182190

191+
$artifactResponse = $this->verifyArtifactResponseSignature($artifactResponse, $idpMetadata);
192+
183193
$samlResponse = $artifactResponse->getMessage();
184194
if ($samlResponse === null) {
185195
/* Empty ArtifactResponse - possibly because of Artifact replay? */
186-
187196
throw new Exception('Empty ArtifactResponse received, maybe a replay?');
188197
}
189198

190-
$samlResponse->addValidator([get_class($this), 'validateSignature'], $artifactResponse);
191-
192199
$query = $request->getQueryParams();
193200
if (isset($query['RelayState'])) {
194201
$this->setRelayState($query['RelayState']);
@@ -208,14 +215,67 @@ public function setSPMetadata(Configuration $sp): void
208215

209216

210217
/**
211-
* A validator which returns true if the ArtifactResponse was signed with the given key
218+
* Verify the ArtifactResponse signature using IdP metadata keys.
212219
*
213-
* @param \SimpleSAML\SAML2\XML\samlp\ArtifactResponse $message
214-
* @param \RobRichards\XMLSecLibs\XMLSecurityKey $key
220+
* Returns the verified ArtifactResponse instance.
221+
*
222+
* @throws \Exception When unsigned, when metadata has no signing keys, or when verification fails.
215223
*/
216-
public static function validateSignature(ArtifactResponse $message, XMLSecurityKey $key): bool
217-
{
218-
// @todo verify if this works and/or needs to do anything more. Ref. HTTPRedirect binding
219-
return $message->validate($key);
224+
private function verifyArtifactResponseSignature(
225+
ArtifactResponse $artifactResponse,
226+
Configuration $idpMetadata,
227+
): ArtifactResponse {
228+
if ($artifactResponse->isSigned() !== true) {
229+
throw new Exception('ArtifactResponse must be signed.');
230+
}
231+
232+
$keys = $idpMetadata->getPublicKeys('signing', true);
233+
if (empty($keys)) {
234+
throw new Exception('No signing keys found in IdP metadata.');
235+
}
236+
237+
$alg = $artifactResponse->getSignature()->getSignedInfo()->getSignatureMethod()->getAlgorithm();
238+
$signatureMethod = $alg instanceof AnyURIValue ? $alg->getValue() : (string) $alg;
239+
240+
$factory = new SignatureAlgorithmFactory();
241+
242+
$lastException = null;
243+
foreach ($keys as $k) {
244+
if (($k['type'] ?? null) !== 'X509Certificate') {
245+
continue;
246+
}
247+
248+
$pemCert = "-----BEGIN CERTIFICATE-----\n" .
249+
chunk_split($k['X509Certificate'], 64) .
250+
"-----END CERTIFICATE-----\n";
251+
252+
$opensslKey = openssl_pkey_get_public($pemCert);
253+
if ($opensslKey === false) {
254+
$lastException = new Exception('Unable to extract public key from X509 certificate.');
255+
continue;
256+
}
257+
258+
$keyInfo = openssl_pkey_get_details($opensslKey);
259+
if ($keyInfo === false || !isset($keyInfo['key']) || !is_string($keyInfo['key'])) {
260+
$lastException = new Exception('Unable to get public key details from X509 certificate.');
261+
continue;
262+
}
263+
264+
$pemPublicKey = $keyInfo['key'];
265+
266+
$file = Utils::getContainer()->getTempDir() . '/' . sha1($pemPublicKey) . '.pem';
267+
if (!file_exists($file)) {
268+
Utils::getContainer()->writeFile($file, $pemPublicKey);
269+
}
270+
271+
try {
272+
$verifier = $factory->getAlgorithm($signatureMethod, PublicKey::fromFile($file));
273+
return $artifactResponse->verify($verifier);
274+
} catch (\Exception $e) {
275+
$lastException = $e;
276+
}
277+
}
278+
279+
throw $lastException ?? new Exception('Unable to verify ArtifactResponse signature.');
220280
}
221281
}

‎src/SOAPClient.php‎

Lines changed: 77 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
use DOMDocument;
88
use Exception;
9-
use RobRichards\XMLSecLibs\XMLSecurityKey;
9+
use OpenSSLAsymmetricKey;
1010
use SimpleSAML\Configuration;
1111
use SimpleSAML\SAML2\Compat\ContainerSingleton;
1212
use SimpleSAML\SAML2\XML\samlp\AbstractMessage;
@@ -24,12 +24,17 @@
2424

2525
use function chunk_split;
2626
use function file_exists;
27+
use function is_object;
28+
use function is_string;
29+
use function method_exists;
2730
use function openssl_pkey_get_details;
2831
use function openssl_pkey_get_public;
32+
use function property_exists;
2933
use function sha1;
3034
use function sprintf;
3135
use function stream_context_create;
3236
use function stream_context_get_options;
37+
use function trim;
3338

3439
/**
3540
* Implementation of the SAML 2.0 SOAP binding.
@@ -251,7 +256,7 @@ private static function addSSLValidator(AbstractMessage $msg, $context): void
251256
return;
252257
}
253258

254-
if (!isset($keyInfo['key'])) {
259+
if (!isset($keyInfo['key']) || !is_string($keyInfo['key'])) {
255260
$container->getLogger()->warning('Missing key in public key details.');
256261
return;
257262
}
@@ -263,29 +268,88 @@ private static function addSSLValidator(AbstractMessage $msg, $context): void
263268
/**
264269
* Validate a SOAP message against the certificate on the SSL connection.
265270
*
266-
* @param string $data The public key that was used on the connection.
267-
* @param \RobRichards\XMLSecLibs\XMLSecurityKey $key The key we should validate the certificate against.
271+
* @param string $data The public key (PEM) that was used on the connection.
272+
* @param mixed $key The key we should validate the certificate against.
268273
* @throws \Exception
269274
*/
270-
public static function validateSSL(string $data, XMLSecurityKey $key): void
275+
public static function validateSSL(string $data, mixed $key): void
271276
{
272277
$container = ContainerSingleton::getInstance();
273278

274-
$keyInfo = openssl_pkey_get_details($key->key);
279+
$pem = self::extractPublicKeyPem($key);
275280

276-
if ($keyInfo === false) {
277-
throw new Exception('Unable to get key details from XMLSecurityKey.');
281+
if (trim($pem) !== trim($data)) {
282+
throw new Exception('Key on SSL connection did not match key we validated against.');
278283
}
279284

280-
if (!isset($keyInfo['key'])) {
281-
throw new Exception('Missing key in public key details.');
285+
$container->getLogger()->debug('Message validated based on SSL certificate.');
286+
}
287+
288+
289+
/**
290+
* Extract a PEM-encoded public key from different key representations.
291+
*
292+
* This avoids coupling to a specific XML security backend.
293+
*
294+
* @param mixed $key
295+
* @throws \Exception
296+
*/
297+
private static function extractPublicKeyPem(mixed $key): string
298+
{
299+
// If the validating key is already PEM, normalize it by re-loading through OpenSSL.
300+
if (is_string($key)) {
301+
$opensslKey = openssl_pkey_get_public($key);
302+
if ($opensslKey === false) {
303+
throw new Exception('Unable to load validating public key from PEM string.');
304+
}
305+
306+
$keyInfo = openssl_pkey_get_details($opensslKey);
307+
if ($keyInfo === false || !isset($keyInfo['key']) || !is_string($keyInfo['key'])) {
308+
throw new Exception('Unable to get key details from validating PEM key.');
309+
}
310+
311+
return $keyInfo['key'];
282312
}
283313

284-
if (trim($keyInfo['key']) !== trim($data)) {
285-
throw new Exception('Key on SSL connection did not match key we validated against.');
314+
// Some key implementations may expose PEM via a method.
315+
if (is_object($key)) {
316+
foreach (['getPublicKeyPem', 'getPem', 'toPEM', 'toPem'] as $method) {
317+
if (method_exists($key, $method)) {
318+
/** @var mixed $pem */
319+
$pem = $key->{$method}();
320+
if (is_string($pem) && $pem !== '') {
321+
return self::extractPublicKeyPem($pem);
322+
}
323+
}
324+
}
325+
326+
// Common compatibility case: an object wraps an OpenSSL key or PEM in a public "key" property.
327+
if (property_exists($key, 'key')) {
328+
/** @var mixed $inner */
329+
$inner = $key->key;
330+
331+
if (is_string($inner)) {
332+
return self::extractPublicKeyPem($inner);
333+
}
334+
335+
if ($inner instanceof OpenSSLAsymmetricKey) {
336+
$keyInfo = openssl_pkey_get_details($inner);
337+
if ($keyInfo !== false && isset($keyInfo['key']) && is_string($keyInfo['key'])) {
338+
return $keyInfo['key'];
339+
}
340+
}
341+
}
286342
}
287343

288-
$container->getLogger()->debug('Message validated based on SSL certificate.');
344+
// Last attempt: OpenSSL might accept the value directly (OpenSSLAsymmetricKey).
345+
if ($key instanceof OpenSSLAsymmetricKey) {
346+
$keyInfo = openssl_pkey_get_details($key);
347+
if ($keyInfo !== false && isset($keyInfo['key']) && is_string($keyInfo['key'])) {
348+
return $keyInfo['key'];
349+
}
350+
}
351+
352+
throw new Exception('Unable to extract public key PEM from validating key.');
289353
}
290354

291355

0 commit comments

Comments
 (0)