Skip to content

Commit e2df799

Browse files
tvdijentvdijen
committed
Fix SOAPClient SSL key validation
1 parent 8180caf commit e2df799

4 files changed

Lines changed: 261 additions & 7 deletions

File tree

‎composer.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@
3636
},
3737
"extra": {
3838
"branch-alias": {
39-
"dev-master": "v4.2.x-dev"
39+
"dev-master": "v4.20.x-dev"
4040
}
4141
},
4242
"config": {

‎phpstan-baseline-dev.neon‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,9 @@ parameters:
55
identifier: method.notFound
66
count: 3
77
path: tests/SAML2/Certificate/KeyLoaderTest.php
8+
9+
-
10+
message: '#^Call to static method loadFromArray\(\) on an unknown class SimpleSAML\\Configuration\.$#'
11+
identifier: class.notFound
12+
count: 2
13+
path: tests/SAML2/SOAPClientTest.php

‎src/SAML2/SOAPClient.php‎

Lines changed: 45 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,10 @@
1313
use SAML2\Exception\UnparseableXmlException;
1414
use SimpleSAML\Configuration;
1515
use SimpleSAML\XML\DOMDocumentFactory;
16+
use SimpleSAML\XMLSchema\Type\AnyURIValue;
1617
use SimpleSAML\Utils\Config;
1718
use SimpleSAML\Utils\Crypto;
18-
//use SoapClient as BuiltinSoapClient;
19+
use SoapClient as BuiltinSoapClient;
1920
use SOAP_1_1;
2021

2122
/**
@@ -141,7 +142,8 @@ public function send(Message $msg, Configuration $srcMetadata, ?Configuration $d
141142
}
142143

143144
/* Perform SOAP Request over HTTP */
144-
$soapresponsexml = $x->__doRequest($request, $destination, $action, SOAP_1_1);
145+
$x = $this->createSoapClient($options);
146+
$soapresponsexml = $this->doSoapRequest($x, $request, $destination, $action);
145147
if (empty($soapresponsexml)) {
146148
throw new Exception('Empty SOAP response, check peer certificate.');
147149
}
@@ -178,6 +180,45 @@ public function send(Message $msg, Configuration $srcMetadata, ?Configuration $d
178180
}
179181

180182

183+
/**
184+
* Factory method to create the built-in SoapClient. Overridable for testing.
185+
*
186+
* @param array $options
187+
* @return \SoapClient
188+
*/
189+
protected function createSoapClient(array $options): BuiltinSoapClient
190+
{
191+
return new BuiltinSoapClient(null, $options);
192+
}
193+
194+
195+
/**
196+
* Wrapper around __doRequest(), overridable for testing.
197+
*
198+
* NOTE: $destination is a generic xs:anyURI value (XMLSchema), since the SOAP endpoint URI
199+
* is transport-level and not necessarily subject to SAML-layer URI restrictions.
200+
*
201+
* @param \SoapClient $client
202+
* @param string|null $request
203+
* @param string $destination
204+
* @param string $action
205+
* @return string
206+
*/
207+
protected function doSoapRequest(
208+
BuiltinSoapClient $client,
209+
?string $request,
210+
string $destination,
211+
string $action,
212+
): string {
213+
return (string) $client->__doRequest(
214+
$request,
215+
$destination,
216+
$action,
217+
SOAP_1_1,
218+
);
219+
}
220+
221+
181222
/**
182223
* Add a signature validator based on a SSL context.
183224
*
@@ -235,10 +276,8 @@ public static function validateSSL(string $data, XMLSecurityKey $key): void
235276
throw new Exception('Missing key in public key details.');
236277
}
237278

238-
if ($keyInfo['key'] !== $data) {
239-
$container->getLogger()->debug('Key on SSL connection did not match key we validated against.');
240-
241-
return;
279+
if (trim($keyInfo['key']) !== trim($data)) {
280+
throw new Exception('Key on SSL connection did not match key we validated against.');
242281
}
243282

244283
$container->getLogger()->debug('Message validated based on SSL certificate.');

‎tests/SAML2/SOAPClientTest.php‎

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,209 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace SimpleSAML\Test\SAML2;
6+
7+
use DOMDocument;
8+
use Exception;
9+
use PHPUnit\Framework\Attributes\CoversClass;
10+
use PHPUnit\Framework\Attributes\DataProvider;
11+
use PHPUnit\Framework\TestCase;
12+
use RobRichards\XMLSecLibs\XMLSecurityKey;
13+
use SAML2\CertificatesMock;
14+
use SAML2\Message;
15+
use SAML2\Response;
16+
use SAML2\SOAPClient;
17+
use SimpleSAML\Configuration;
18+
use SimpleSAML\XMLSchema\Type\AnyURIValue;
19+
20+
/**
21+
* Tests for {@see \SimpleSAML\SAML2\SOAPClient}:
22+
* - SSL peer key validation behavior ({@see \SimpleSAML\SAML2\SOAPClient::validateSSL()})
23+
* - send() fail-fast and error handling behavior
24+
*
25+
* Notes:
26+
* - SSL validation tests use deterministic PEM fixtures from simplesamlphp/xml-security
27+
* (via {@see \SAML2\CertificatesMock}) to avoid depending on
28+
* runtime-generated keys.
29+
* - send() tests avoid network I/O by overriding {@see \SimpleSAML\SAML2\SOAPClient::doSoapRequest()}.
30+
*/
31+
#[CoversClass(SOAPClient::class)]
32+
final class SOAPClientTest extends TestCase
33+
{
34+
/**
35+
* @return array<string, array{0: bool}>
36+
*/
37+
public static function provideSslKeyMatchCases(): array
38+
{
39+
return [
40+
'tls key matches xml key' => [true],
41+
'tls key differs from xml key' => [false],
42+
];
43+
}
44+
45+
46+
/**
47+
* Use case: the SSL peer key validator must be fail-closed.
48+
*
49+
* - If the peer key material provided to {@see SOAPClient::validateSSL()} matches the key being validated,
50+
* validation succeeds (no exception).
51+
* - If it does not match, validation must throw (reject).
52+
*
53+
* This test reuses deterministic key fixtures from {@see CertificatesMock}.
54+
*
55+
* @param bool $shouldMatch Whether the peer key material and the XMLSecurityKey should match.
56+
*/
57+
#[DataProvider('provideSslKeyMatchCases')]
58+
public function testValidateSslThrowsOnMismatchAndPassesOnMatch(bool $shouldMatch): void
59+
{
60+
$tlsPublicKeyPem = CertificatesMock::PUBLIC_KEY_PEM;
61+
$otherPublicKeyPem = CertificatesMock::PUBLIC_KEY_2_PEM;
62+
63+
$xmlPublicKeyPem = $shouldMatch ? $tlsPublicKeyPem : $otherPublicKeyPem;
64+
$key = $this->buildXmlSecurityPublicKey($xmlPublicKeyPem);
65+
66+
if (!$shouldMatch) {
67+
$this->expectException(Exception::class);
68+
$this->expectExceptionMessage('Key on SSL connection did not match key we validated against.');
69+
}
70+
71+
SOAPClient::validateSSL($tlsPublicKeyPem, $key);
72+
73+
if ($shouldMatch) {
74+
$this->addToAssertionCount(1);
75+
}
76+
}
77+
78+
79+
/**
80+
* Build an {@see XMLSecurityKey} from a PEM-encoded public key, for use with
81+
* {@see SOAPClient::validateSSL()}.
82+
*
83+
* @param string $publicKeyPem PEM-encoded public key (e.g. "-----BEGIN PUBLIC KEY----- ...").
84+
*/
85+
private function buildXmlSecurityPublicKey(string $publicKeyPem): XMLSecurityKey
86+
{
87+
$key = new XMLSecurityKey(XMLSecurityKey::RSA_SHA256, ['type' => 'public']);
88+
$key->loadKey($publicKeyPem, false);
89+
90+
return $key;
91+
}
92+
93+
94+
/**
95+
* @return array<string, array{soapResponse: string, expectedMessage: string}>
96+
*/
97+
public static function provideBadSoapResponses(): array
98+
{
99+
return [
100+
// Use case: SOAP transport returns an empty response body (e.g., TLS/cert failure, transport error).
101+
'empty SOAP response' => [
102+
'soapResponse' => '',
103+
'expectedMessage' => 'Empty SOAP response, check peer certificate.',
104+
],
105+
// Use case: SOAP endpoint returns a SOAP Fault; send() must throw with extracted fields.
106+
'SOAP fault response' => [
107+
'soapResponse' =>
108+
'<env:Envelope xmlns:env="http://schemas.xmlsoap.org/soap/envelope/">' .
109+
'<env:Body>' .
110+
'<env:Fault>' .
111+
'<faultcode>env:Server</faultcode>' .
112+
'<faultstring>oops</faultstring>' .
113+
'<faultactor>actor-1</faultactor>' .
114+
'</env:Fault>' .
115+
'</env:Body>' .
116+
'</env:Envelope>',
117+
'expectedMessage' => "Actor: 'actor-1'; Message: 'oops'; Code: 'env:Server'",
118+
],
119+
];
120+
}
121+
122+
123+
/**
124+
* Use case: send() must throw if the outbound message has no Destination.
125+
public function testSendThrowsWhenDestinationIsMissing(): void
126+
{
127+
$client = new class extends SOAPClient {
128+
protected function createSoapClient(array $options): \SoapClient
129+
{
130+
throw new \LogicException('createSoapClient() should not be called when destination is missing.');
131+
}
132+
133+
134+
protected function doSoapRequest(
135+
\SoapClient $client,
136+
?string $request,
137+
string $destination,
138+
string $action,
139+
): string {
140+
throw new \LogicException('doSoapRequest() should not be called when destination is missing.');
141+
}
142+
};
143+
144+
$msg = $this->createStub(Message::class);
145+
$msg->method('getDestination')->willReturn(null);
146+
147+
$src = Configuration::loadFromArray([], '[src]');
148+
149+
$this->expectException(Exception::class);
150+
$this->expectExceptionMessage('Cannot send SOAP message, no destination set.');
151+
152+
$client->send($msg, $src, null);
153+
}
154+
*/
155+
156+
157+
/**
158+
* Use case: send() must throw on an empty SOAP response and on a SOAP Fault response.
159+
#[DataProvider('provideBadSoapResponses')]
160+
public function testSendThrowsOnEmptySoapResponseOrSoapFault(string $soapResponse, string $expectedMessage): void
161+
{
162+
$destination = 'https://example.org/soap-endpoint';
163+
164+
$client = new class ($soapResponse) extends SOAPClient {
165+
public function __construct(private readonly string $soapResponse)
166+
{
167+
}
168+
169+
170+
protected function createSoapClient(array $options): \SoapClient
171+
{
172+
// A real SoapClient instance, but it will never be used for I/O because doSoapRequest() is overridden.
173+
return new \SoapClient(null, [
174+
'location' => 'http://localhost/soap',
175+
'uri' => 'urn:test',
176+
'exceptions' => true,
177+
'trace' => false,
178+
]);
179+
}
180+
181+
182+
protected function doSoapRequest(
183+
\SoapClient $client,
184+
?string $request,
185+
string $destination,
186+
string $action,
187+
): string {
188+
return $this->soapResponse;
189+
}
190+
};
191+
192+
$msg = $this->createStub(Response::class);
193+
$msg->method('getIssuer')->willReturn(null);
194+
$msg->method('getDestination')->willReturn($destination);
195+
196+
$msg->method('toUnsignedXML')->willReturnCallback(static function () {
197+
$doc = new DOMDocument('1.0', 'UTF-8');
198+
return $doc->appendChild($doc->createElement('TestRequest'));
199+
});
200+
201+
$src = Configuration::loadFromArray([], '[src]');
202+
203+
$this->expectException(Exception::class);
204+
$this->expectExceptionMessage($expectedMessage);
205+
206+
$client->send($msg, $src, null);
207+
}
208+
*/
209+
}

0 commit comments

Comments
 (0)