Skip to content

Commit eba0ddf

Browse files
ioigoumeioigoume
committed
Improve Error Messaging on checkSign failure when the certificate is invalid.
1 parent 9a516f9 commit eba0ddf

4 files changed

Lines changed: 341 additions & 13 deletions

File tree

‎modules/saml/src/Message.php‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -175,17 +175,30 @@ public static function checkSign(Configuration $srcMetadata, SignedElement $elem
175175
}
176176
Logger::debug('Validation with key #' . $i . ' failed without exception.');
177177
} catch (\Exception $e) {
178+
Logger::debug('Check Signature for ' . get_class($element) . ' element');
179+
Logger::debug('EntityID: ' . $srcMetadata->getString('entityid'));
178180
Logger::debug('Validation with key #' . $i . ' failed with exception: ' . $e->getMessage());
179-
$lastException = $e;
181+
182+
// Clone the exception and improve the message
183+
$lastException = new SSP_Error\Error(
184+
[
185+
(string)$e->getCode(),
186+
'element' => get_class($element),
187+
'message' => $e->getMessage(),
188+
'issuer' => $element->getIssuer()->getValue(),
189+
'entityid' => $srcMetadata->getString('entityid'),
190+
],
191+
$e->getPrevious(),
192+
);
180193
}
181194
}
182195

183196
// we were unable to validate the signature with any of our keys
184197
if ($lastException !== null) {
185198
throw $lastException;
186-
} else {
187-
return false;
188199
}
200+
201+
return false;
189202
}
190203

191204

‎src/SimpleSAML/Error/Error.php‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -101,22 +101,14 @@ public function __construct(
101101
$this->dictDescr = $errorCodes->getDescription($this->errorCode);
102102

103103
if (!empty($this->parameters)) {
104-
$msg = $this->errorCode . '(';
105-
foreach ($this->parameters as $k => $v) {
106-
if ($k === 0) {
107-
continue;
108-
}
109-
110-
$msg .= var_export($k, true) . ' => ' . var_export($v, true) . ', ';
111-
}
112-
$msg = substr($msg, 0, -2) . ')';
104+
$msgData = ['errorCode' => $this->errorCode] + $this->parameters;
105+
$msg = json_encode($msgData);
113106
} else {
114107
$msg = $this->errorCode;
115108
}
116109
parent::__construct($msg, -1, $cause);
117110
}
118111

119-
120112
/**
121113
* Retrieve the ErrorCodes instance to use for resolving dictionary title and description tags.
122114
*

‎tests/modules/saml/MessageTest.php‎

Lines changed: 307 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,307 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace SimpleSAML\Test\Module\saml;
6+
7+
use PHPUnit\Framework\TestCase;
8+
use SAML2\AuthnRequest;
9+
use SimpleSAML\Configuration;
10+
use SimpleSAML\Error as SSP_Error;
11+
use SimpleSAML\Module\saml\Message;
12+
13+
/**
14+
* Test for SAML Message handling
15+
*
16+
* @package SimpleSAML\Test\Module\saml
17+
*/
18+
class MessageTest extends TestCase
19+
{
20+
/** @var string */
21+
protected string $acmeeEntityId;
22+
23+
/** @var string */
24+
protected string $acmeeCertificate;
25+
26+
/** @var array */
27+
protected array $acmeeMetadata;
28+
29+
/** @var string */
30+
protected string $spEntityId;
31+
32+
/** @var array */
33+
protected array $spMetadata;
34+
35+
/** @var string */
36+
protected string $acmeeCertificateWrong;
37+
38+
/** @var string */
39+
protected string $acmeeCertificateMismatch;
40+
41+
/**
42+
* Set up for each test.
43+
*/
44+
protected function setUp(): void
45+
{
46+
$this->acmeeEntityId = 'https://idp.acmee.com/example';
47+
48+
$this->acmeeCertificate = <<<EOCERTVALID
49+
MIICVDCCAb2gAwIBAgIBADANBgkqhkiG9w0BAQ0FADBGMQswCQYDVQQGEwJ1czET
50+
MBEGA1UECAwKY2FsaWZvcm5pYTEOMAwGA1UECgwFYWNtZWUxEjAQBgNVBAMMCWFj
51+
bWVlLmNvbTAgFw0yNTA4MjgxNDE3MTZaGA8zMDI0MTIyOTE0MTcxNlowRjELMAkG
52+
A1UEBhMCdXMxEzARBgNVBAgMCmNhbGlmb3JuaWExDjAMBgNVBAoMBWFjbWVlMRIw
53+
EAYDVQQDDAlhY21lZS5jb20wgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBALfi
54+
YX68QeDEaOD1srtxTwtIMat1jtrqjdadI/Ksa/JPfQNv1Wjz1SvRgvAi4JXrl9KQ
55+
7iAv7wn0UNV7HKEAQws0iFheHUHiqk9VrwxLTrZTwFiBjwkzoDo4yrLCArurPaDu
56+
rqH03KD9dMu1EOgMHWvNYm3o+D2X60UelAVKP2DVAgMBAAGjUDBOMB0GA1UdDgQW
57+
BBS3dLfltNkyLvyuktzZRjReuR3ZUjAfBgNVHSMEGDAWgBS3dLfltNkyLvyuktzZ
58+
RjReuR3ZUjAMBgNVHRMEBTADAQH/MA0GCSqGSIb3DQEBDQUAA4GBAB4+5VK0QaEM
59+
EF8pvC+qKs4qsnoojMkOzsswHZYoSHHF2m+ZWLnTZd2o1DQ2ZCV+Y8G/xKAgREns
60+
3jvtWEBojSVJfwhkG/mwfxeGgTnVwOctmTJTRmRaT560L8BX0avx6tcexJal6G3y
61+
CEpB/IjANYp6rT2+iCdpgbPJBlWbJsJx
62+
EOCERTVALID;
63+
64+
$this->acmeeCertificateMismatch = <<<EOCERTINVALID
65+
MIIDnTCCAoWgAwIBAgIgLehYh1dpeK6jw2fEbJf2eapg3+74qpNqYXwj3pP9AxIw
66+
DQYJKoZIhvcNAQEFBQAwZTEJMAcGA1UEBhMAMRAwDgYDVQQKDAdleGFtcGxlMQkw
67+
BwYDVQQLDAAxFDASBgNVBAMMC2V4YW1wbGUuY29tMQ8wDQYJKoZIhvcNAQkBFgAx
68+
FDASBgNVBAMMC2V4YW1wbGUuY29tMB4XDTI1MDgyMzEzMDQ0MVoXDTM1MDgyNDEz
69+
MDQ0MVowTzEJMAcGA1UEBhMAMRAwDgYDVQQKDAdleGFtcGxlMQkwBwYDVQQLDAAx
70+
FDASBgNVBAMMC2V4YW1wbGUuY29tMQ8wDQYJKoZIhvcNAQkBFgAwggEiMA0GCSqG
71+
SIb3DQEBAQUAA4IBDwAwggEKAoIBAQDY7Y4ENmUFIjmiKaFK6HG/FvhuGG/yVAAp
72+
7v+smI33lKnvTdkTrH3MujYp+3R5GyivcP6M4iY6Wi5VJ9vzUV0W7tNjD2NngG/0
73+
0kM/6u41sMhHBZnSB97HcAwzOcPrPZjvjntG4UDemGDJ3clw6VL3/DMzJTsLGwx/
74+
050sIiHNXsL3WsuP/XKY2aEmg6S4PoiKYiNz9NabCV2RROKGGk9Ar8+c4HD43x/x
75+
VJdj36HizNaHB9tXQoJ00YVq8J3jYTwUCo9AnKDczRHZk2w0niCMolMi327hsx1K
76+
H3uuL741KIhquOEzPzb90NaNd2yjHAHl5gXitBs7A3qZhhQYIgULAgMBAAGjTzBN
77+
MB0GA1UdDgQWBBQf+e9Md02jLhFD6MXGOGsO/7YGrDAfBgNVHSMEGDAWgBQf+e9M
78+
d02jLhFD6MXGOGsO/7YGrDALBgNVHREEBDACggAwDQYJKoZIhvcNAQEFBQADggEB
79+
AD/wuKcY+NsctxeEO17Upd+7XSCau3GjAp+tT7Y7VZ7jDcW3x7zqWfQppmLThmIq
80+
iXAMgKz2WTjtJphj/AV/QFUHff7wadFm4WLAL5a66LbQZN6/r6olvBw2rJq4Fwyl
81+
QX4echQghnopDbdKNA516PCq9jxttQxdO+x1kSRqm05lkK51cV4aG1Nyt+kA+cKc
82+
mnxDWVg96iuAKtPV5WBaksb821/AgOFLbNEW37y037Gr8UaLEhvZWpMIx9r+XO2u
83+
bPkKAQmfc2FPQV9m4bEO0ihl8fKSljXB8WOefL/7jvHMUFTPOSzLZggHYXirAQ5r
84+
OKTV+3OdzdvDlTqKhof9qKs=
85+
EOCERTINVALID;
86+
87+
$this->acmeeCertificateWrong = <<<EOCERTWRONG
88+
MIIDiDCCAnACCQC5NZIb4AVJuDANBgkqhkiG9w0BAQsFADCBhzELMAkGA1UEBhMC
89+
VVMxCzAJBgNVBAgMAkNBMQswCQYDVQQHDAJMQTEQMA4GA1UECgwHQWNtZWUgSW5j
90+
MQ4wDAYDVQQLDAVJVCBEZXAxKjAoBgNVBAMMIWh0dHBzOi8vaWRwLmFjbWVlLmNv
91+
bS9leGFtcGxlMB4XDTI0MDgyODEzNDk1OFoXDTM0MDgyNTEzNDk1OFowgYcxCzAJ
92+
BgNVBAYTAlVTMQswCQYDVQQIDAJDQTELMAkGA1UEBwwCTEEhEDAOBgNVBAoMB0Fj
93+
bWVlIEluYzEOMAwGA1UECwwFSVQgRGVwMSowKAYDVQQDDCFTSFQ1czBJZHBcY21l
94+
ZS5jb20vZXhhbXBsZTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALXN
95+
dlFSXwL5MZqcmF7vPhBk8Nqpw1PQAS1/aCta/CcZtqcaOrJgEbg4qM5QgRAkmk5B
96+
YT/JQqwyhrIUF7fArw9dZyEvNk96zM1rNR9ez8rANMTb8P6+WM7CjQnUeHLJ2jno
97+
FEvKTPio4co3M6wrCkD7tDjposL7YTbD7cGylWHaIyzFzmDbkEi8UuPQq10uQxtz
98+
K3gvt08HcgccpLMhq1pyqStwoxjhNBU5B616uQuT6ayEPyAAriBo5gvUG3iSw5f7
99+
XapQidoxvN7MTEswtbyxxWhHT1Edk0QXOgZIa4vDCilAGe3A2If77mIhbIqSmqmP
100+
X3AiBOOnfR4r+3vK/cMCAwEAATANBgkqhkiG9w0BAQsFAAOCAQEAM7QjRrw8Da7A
101+
R39pFka3lrQOjYFo0U49TAhCE7SiHsVJjaJ7MTGvPA1uNBW5SmzxXKWC5NRuFkgU
102+
3OhXHn6cS3MEruHDYtT/jldwP6SAAV4AfQRd2rzGptO2au/cAnGCb5WQ6kV1Kv8Y
103+
IMNGvNnj3eLhtAf7EXQHvFb3n0MkzyJtvHNPy0lcmJF7daRtaDKmKo+ooKwCw8he
104+
8YAT4CtGJJf3hWwZUVugCi1Eu+h4lW3u+09GyjhNoC6GBrTEPUm5TfAAKQk2294G
105+
rmt3CCz2F8T6fboKo+LwoAAp/Y8PaCnebR81OHeuMY8LB1u5lIFASrufA+haOg7i
106+
50Qk4cxlzg==
107+
EOCERTWRONG;
108+
109+
// Metadata array, just like you'd see in saml20-idp-remote.php
110+
$this->acmeeMetadata = [
111+
'entityid' => $this->acmeeEntityId,
112+
'description' => [
113+
'en' => 'Acmee IdP for testing',
114+
],
115+
'OrganizationName' => [
116+
'en' => 'Acmee',
117+
],
118+
'name' => [
119+
'en' => 'Acmee Example Identity Provider',
120+
],
121+
'OrganizationDisplayName' => [
122+
'en' => 'Acmee',
123+
],
124+
'url' => [
125+
'en' => 'https://www.acmee.com/',
126+
],
127+
'OrganizationURL' => [
128+
'en' => 'https://www.acmee.com/',
129+
],
130+
'contacts' => [
131+
[
132+
'contactType' => 'technical',
133+
'givenName' => 'Test Admin',
134+
'emailAddress' => [
135+
'admin@acmee.com',
136+
],
137+
],
138+
],
139+
'metadata-set' => 'saml20-idp-remote',
140+
'SingleSignOnService' => [
141+
[
142+
'Binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect',
143+
'Location' => 'https://idp.acmee.com/example/sso',
144+
],
145+
[
146+
'Binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST',
147+
'Location' => 'https://idp.acmee.com/example/sso',
148+
],
149+
],
150+
'SingleLogoutService' => [
151+
[
152+
'Binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect',
153+
'Location' => 'https://idp.acmee.com/example/logout',
154+
],
155+
],
156+
'ArtifactResolutionService' => [],
157+
'NameIDFormats' => [
158+
'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent',
159+
'urn:oasis:names:tc:SAML:2.0:nameid-format:transient',
160+
],
161+
'keys' => [
162+
[
163+
'encryption' => false,
164+
'signing' => true,
165+
'type' => 'X509Certificate',
166+
'X509Certificate' => $this->acmeeCertificate,
167+
],
168+
],
169+
'scope' => [
170+
'acmee.com',
171+
],
172+
'UIInfo' => [
173+
'DisplayName' => [
174+
'en' => 'Acmee Identity Provider',
175+
],
176+
'Description' => [],
177+
'InformationURL' => [],
178+
'PrivacyStatementURL' => [],
179+
],
180+
];
181+
182+
// Build minimal SP metadata:
183+
$this->spEntityId = 'https://sp.acmee.com/demo';
184+
$this->spMetadata = [
185+
'entityID' => $this->spEntityId,
186+
'AssertionConsumerService' => [
187+
[
188+
'Binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST',
189+
'Location' => 'https://sp.acmee.com/demo/acs',
190+
],
191+
],
192+
];
193+
194+
parent::setUp();
195+
}
196+
197+
public function testCheckSignThrowsWhenBadCertificate(): void
198+
{
199+
$this->expectException(\Exception::class);
200+
201+
$localMetadata = $this->acmeeMetadata;
202+
foreach ($localMetadata['keys'] as $index => $cert) {
203+
$localMetadata['keys'][$index]['X509Certificate'] = $this->acmeeCertificateWrong;
204+
}
205+
$idpConfig = Configuration::loadFromArray(
206+
$localMetadata,
207+
$localMetadata['entityid'],
208+
);
209+
210+
$spConfig = Configuration::loadFromArray(
211+
$this->spMetadata,
212+
$this->spMetadata['entityID'],
213+
);
214+
215+
// Build the AuthnRequest using the Message helper:
216+
$authnRequest = Message::buildAuthnRequest($spConfig, $idpConfig);
217+
218+
// You may now use $authnRequest with checkSign:
219+
Message::checkSign($idpConfig, $authnRequest);
220+
}
221+
222+
public function testCheckSignThrowsWhenMissingCertificate(): void
223+
{
224+
$this->expectException(SSP_Error\Exception::class);
225+
$this->expectExceptionMessage('Missing certificate in metadata for \'https://idp.acmee.com/example\'');
226+
227+
$localMetadata = $this->acmeeMetadata;
228+
unset($localMetadata['keys']);
229+
$idpConfig = Configuration::loadFromArray(
230+
$localMetadata,
231+
$localMetadata['entityid'],
232+
);
233+
234+
$spConfig = Configuration::loadFromArray(
235+
$this->spMetadata,
236+
$this->spMetadata['entityID'],
237+
);
238+
239+
// Build the AuthnRequest using the Message helper:
240+
$authnRequest = Message::buildAuthnRequest($spConfig, $idpConfig);
241+
242+
// You may now use $authnRequest with checkSign:
243+
Message::checkSign($idpConfig, $authnRequest);
244+
}
245+
246+
public function testCheckSignThrowsWhenCertificateMismatch(): void
247+
{
248+
$this->expectException(SSP_Error\Error::class);
249+
$expectedMessage = [
250+
'errorCode' => '0',
251+
'element' => 'SAML2\AuthnRequest',
252+
'message' => 'Unable to validate Signature',
253+
'issuer' => 'https://sp.acmee.com/demo',
254+
'entityid' => 'https://idp.acmee.com/example',
255+
];
256+
257+
$this->expectExceptionMessage(json_encode($expectedMessage));
258+
259+
$localMetadata = $this->acmeeMetadata;
260+
$localMetadata['signature.privatekey'] = __DIR__ . '/certs/acmee.priv.key';
261+
$localMetadata['sign.authnrequest'] = true;
262+
foreach ($localMetadata['keys'] as $index => $cert) {
263+
$localMetadata['keys'][$index]['X509Certificate'] = $this->acmeeCertificateMismatch;
264+
}
265+
$idpConfig = Configuration::loadFromArray(
266+
$localMetadata,
267+
$localMetadata['entityid'],
268+
);
269+
270+
$spConfig = Configuration::loadFromArray(
271+
$this->spMetadata,
272+
$this->spMetadata['entityID'],
273+
);
274+
275+
// Build the AuthnRequest using the Message helper:
276+
$authnRequest = Message::buildAuthnRequest($spConfig, $idpConfig);
277+
// Extract to signed xml and then parse again to validate
278+
$signedDomElement = $authnRequest->toSignedXML();
279+
$parsed = new AuthnRequest($signedDomElement);
280+
Message::checkSign($idpConfig, $parsed);
281+
}
282+
283+
public function testCheckSignSucceeds(): void
284+
{
285+
$localMetadata = $this->acmeeMetadata;
286+
$localMetadata['signature.privatekey'] = __DIR__ . '/certs/acmee.priv.key';
287+
$localMetadata['sign.authnrequest'] = true;
288+
289+
$idpConfig = Configuration::loadFromArray(
290+
$localMetadata,
291+
$localMetadata['entityid'],
292+
);
293+
294+
$spConfig = Configuration::loadFromArray(
295+
$this->spMetadata,
296+
$this->spMetadata['entityID'],
297+
);
298+
299+
// Build the AuthnRequest using the Message helper:
300+
$authnRequest = Message::buildAuthnRequest($spConfig, $idpConfig);
301+
// Extract to signed xml and then parse again to validate
302+
$signedDomElement = $authnRequest->toSignedXML();
303+
$parsed = new AuthnRequest($signedDomElement);
304+
$isValid = Message::checkSign($idpConfig, $parsed);
305+
$this->assertTrue($isValid);
306+
}
307+
}

‎tests/modules/saml/certs/acmee.priv.key‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
-----BEGIN PRIVATE KEY-----
2+
MIICdQIBADANBgkqhkiG9w0BAQEFAASCAl8wggJbAgEAAoGBALfiYX68QeDEaOD1
3+
srtxTwtIMat1jtrqjdadI/Ksa/JPfQNv1Wjz1SvRgvAi4JXrl9KQ7iAv7wn0UNV7
4+
HKEAQws0iFheHUHiqk9VrwxLTrZTwFiBjwkzoDo4yrLCArurPaDurqH03KD9dMu1
5+
EOgMHWvNYm3o+D2X60UelAVKP2DVAgMBAAECgYAHG2vTPyl4q362OyjWT9HTSM4K
6+
p3eHBIvI4Lfz+DAP5HybdmYUMWBq2iUqbN6rTLjIfauGePPPOa8qISEBJAZzRqXn
7+
6GaNv2pzfKCkpA0nsdGvkmdPzAZYRxWaVmd8XfY8eVBRMA9jFGDdaLsyeb3RE2Rn
8+
ZPOUQlkqFyH1/agewQJBANnyaYSXbZQFs5VejMG1vKMREkbYOnWWst/rJxU2tOVm
9+
wrJ5uULcxTPSn+fopTsyiO3S7WqJZTVMgk93AE/NbHECQQDX/Xgq1Y4rp3K15Dq+
10+
Eke3NysieFQdh7q/EB2dDG22uDN6r8Ej8ZgY3/Gg1EEaqoaUN8xbxSJfcb4HZuLb
11+
TjylAkAGmRAYs3zdvk5xdytLsfTD+wBSpLkgVi+UF8pXGhDf4PyD6qtxGr3dk8LD
12+
god+A0mh6YDGeOJXerl3LmMUB2QBAkBQql1iwfci3pq8y8wUiIc4KeZ2LTJdBP/9
13+
s2sb6DRhdVHklBcx8Vy4jYqUYjEeYGl6mYw9CdbYhoZOBWLcPM/xAkB37ZfnM0ec
14+
NjELIlbZPFXj+D4MLEOpkQZLZI83u41HDq7hGrFfCn/aT7M4HMJlq/QDM9RYc+4h
15+
sANE05T5j/+j
16+
-----END PRIVATE KEY-----

0 commit comments

Comments
 (0)