Skip to content

Commit 8b26c66

Browse files
ioigoumeioigoume
authored
SSP-Improved_Error_Messaging_for_Invalid_Signatures (#2506)
* Improve Error Messaging on checkSign failure when the certificate is invalid. * update error message check in testing * Use PEMCertificateMock instead of hardcoded certificates * Fix title_0 issue.Add NOTVALIDCERTIFICATESIGNATURE error code. * Fix tests
1 parent d3b1ea9 commit 8b26c66

6 files changed

Lines changed: 307 additions & 20 deletions

File tree

‎modules/saml/src/Message.php‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
use SimpleSAML\Assert\Assert;
2222
use SimpleSAML\Configuration;
2323
use SimpleSAML\Error as SSP_Error;
24+
use SimpleSAML\Error\ErrorCodes;
2425
use SimpleSAML\Logger;
2526
use SimpleSAML\Module\saml\Error as SAMLError;
2627
use SimpleSAML\SAML2\Constants;
@@ -175,17 +176,30 @@ public static function checkSign(Configuration $srcMetadata, SignedElement $elem
175176
}
176177
Logger::debug('Validation with key #' . $i . ' failed without exception.');
177178
} catch (\Exception $e) {
179+
Logger::debug('Check Signature for ' . get_class($element) . ' element');
180+
Logger::debug('EntityID: ' . $srcMetadata->getString('entityid'));
178181
Logger::debug('Validation with key #' . $i . ' failed with exception: ' . $e->getMessage());
179-
$lastException = $e;
182+
183+
// Clone the exception and improve the message
184+
$lastException = new SSP_Error\Error(
185+
[
186+
ErrorCodes::NOTVALIDCERTSIGNATURE,
187+
'message' => (new ErrorCodes())->getMessage(ErrorCodes::NOTVALIDCERTSIGNATURE),
188+
'element' => get_class($element),
189+
'issuer' => $element->getIssuer()->getValue(),
190+
'entityid' => $srcMetadata->getString('entityid'),
191+
],
192+
$e->getPrevious(),
193+
);
180194
}
181195
}
182196

183197
// we were unable to validate the signature with any of our keys
184198
if ($lastException !== null) {
185199
throw $lastException;
186-
} else {
187-
return false;
188200
}
201+
202+
return false;
189203
}
190204

191205

‎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
*

‎src/SimpleSAML/Error/ErrorCodes.php‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ public function __construct()
2424
}
2525

2626
final public const ACSPARAMS = 'ACSPARAMS';
27+
final public const ADMINNOTHASHED = 'ADMINNOTHASHED';
2728
final public const ARSPARAMS = 'ARSPARAMS';
2829
final public const AUTHSOURCEERROR = 'AUTHSOURCEERROR';
2930
final public const BADREQUEST = 'BADREQUEST';
@@ -47,8 +48,8 @@ public function __construct()
4748
final public const NOTFOUND = 'NOTFOUND';
4849
final public const NOTFOUNDREASON = 'NOTFOUNDREASON';
4950
final public const NOTSET = 'NOTSET';
50-
final public const ADMINNOTHASHED = 'ADMINNOTHASHED';
5151
final public const NOTVALIDCERT = 'NOTVALIDCERT';
52+
final public const NOTVALIDCERTSIGNATURE = 'NOTVALIDCERTSIGNATURE';
5253
final public const PROCESSASSERTION = 'PROCESSASSERTION';
5354
final public const PROCESSAUTHNREQUEST = 'PROCESSAUTHNREQUEST';
5455
final public const RESPONSESTATUSNOSUCCESS = 'RESPONSESTATUSNOSUCCESS';
@@ -76,6 +77,7 @@ final public static function defaultGetAllErrorCodeTitles(): array
7677
{
7778
return [
7879
self::ACSPARAMS => Translate::noop('No SAML response provided'),
80+
self::ADMINNOTHASHED => Translate::noop('Admin password not set to a hashed value'),
7981
self::ARSPARAMS => Translate::noop('No SAML message provided'),
8082
self::AUTHSOURCEERROR => Translate::noop('Authentication source error'),
8183
self::BADREQUEST => Translate::noop('Bad request received'),
@@ -99,8 +101,8 @@ final public static function defaultGetAllErrorCodeTitles(): array
99101
self::NOTFOUND => Translate::noop('Page not found'),
100102
self::NOTFOUNDREASON => Translate::noop('Page not found'),
101103
self::NOTSET => Translate::noop('Password not set'),
102-
self::ADMINNOTHASHED => Translate::noop('Admin password not set to a hashed value'),
103104
self::NOTVALIDCERT => Translate::noop('Invalid certificate'),
105+
self::NOTVALIDCERTSIGNATURE => Translate::noop('Invalid certificate signature'),
104106
self::PROCESSASSERTION => Translate::noop('Error processing response from Identity Provider'),
105107
self::PROCESSAUTHNREQUEST => Translate::noop('Error processing request from Service Provider'),
106108
self::RESPONSESTATUSNOSUCCESS => Translate::noop('Error received from Identity Provider'),
@@ -204,7 +206,8 @@ final public static function defaultGetAllErrorCodeDescriptions(): array
204206
" not intended to be accessed directly."),
205207
self::AUTHSOURCEERROR => Translate::noop("" .
206208
'Authentication error in source %AUTHSOURCE%. The reason was: %REASON%'),
207-
self::BADREQUEST => Translate::noop('There is an error in the request to this page. The reason was: %REASON%'),
209+
self::BADREQUEST =>
210+
Translate::noop('There is an error in the request to this page. The reason was: %REASON%'),
208211
self::CASERROR => Translate::noop('Error when communicating with the CAS server.'),
209212
self::CONFIG => Translate::noop('SimpleSAMLphp appears to be misconfigured.'),
210213
self::CREATEREQUEST => Translate::noop("An error occurred when trying to create the SAML request."),
@@ -261,7 +264,9 @@ final public static function defaultGetAllErrorCodeDescriptions(): array
261264
"admin-page-with-and-error-message-admin-password-" .
262265
"not-set-to-a-hashed-value"),
263266
self::NOTVALIDCERT => Translate::noop('You did not present a valid certificate.'),
264-
self::PROCESSASSERTION => Translate::noop('We did not accept the response sent from the Identity Provider.'),
267+
self::NOTVALIDCERTSIGNATURE => Translate::noop('Unable to validate certificate signature.'),
268+
self::PROCESSASSERTION =>
269+
Translate::noop('We did not accept the response sent from the Identity Provider.'),
265270
self::PROCESSAUTHNREQUEST => Translate::noop("" .
266271
"This Identity Provider received an Authentication Request from a Service " .
267272
"Provider, but an error occurred when trying to process the request."),

‎tests/modules/saml/MessageTest.php‎

Lines changed: 264 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,264 @@
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\Error\ErrorCodes;
12+
use SimpleSAML\Module\saml\Message;
13+
use SimpleSAML\XMLSecurity\TestUtils\PEMCertificatesMock;
14+
15+
/**
16+
* Test for SAML Message handling
17+
*
18+
* @package SimpleSAML\Test\Module\saml
19+
*/
20+
class MessageTest extends TestCase
21+
{
22+
/** @var string */
23+
protected string $acmeeEntityId;
24+
25+
/** @var string */
26+
protected string $acmeeCertificate;
27+
28+
/** @var array */
29+
protected array $acmeeMetadata;
30+
31+
/** @var string */
32+
protected string $spEntityId;
33+
34+
/** @var array */
35+
protected array $spMetadata;
36+
37+
/** @var string */
38+
protected string $acmeeCertificateWrong;
39+
40+
/** @var string */
41+
protected string $acmeeCertificateMismatch;
42+
43+
/**
44+
* Set up for each test.
45+
*/
46+
protected function setUp(): void
47+
{
48+
$this->acmeeEntityId = 'https://idp.acmee.com/example';
49+
50+
$this->acmeeCertificate = PEMCertificatesMock::getPlainCertificateContents();
51+
52+
$this->acmeeCertificateMismatch = PEMCertificatesMock::getPlainCertificateContents(
53+
PEMCertificatesMock::OTHER_CERTIFICATE,
54+
);
55+
56+
$this->acmeeCertificateWrong = PEMCertificatesMock::getPlainCertificateContents(
57+
PEMCertificatesMock::CORRUPTED_CERTIFICATE,
58+
);
59+
60+
// Metadata array, just like you'd see in saml20-idp-remote.php
61+
$this->acmeeMetadata = [
62+
'entityid' => $this->acmeeEntityId,
63+
'description' => [
64+
'en' => 'Acmee IdP for testing',
65+
],
66+
'OrganizationName' => [
67+
'en' => 'Acmee',
68+
],
69+
'name' => [
70+
'en' => 'Acmee Example Identity Provider',
71+
],
72+
'OrganizationDisplayName' => [
73+
'en' => 'Acmee',
74+
],
75+
'url' => [
76+
'en' => 'https://www.acmee.com/',
77+
],
78+
'OrganizationURL' => [
79+
'en' => 'https://www.acmee.com/',
80+
],
81+
'contacts' => [
82+
[
83+
'contactType' => 'technical',
84+
'givenName' => 'Test Admin',
85+
'emailAddress' => [
86+
'admin@acmee.com',
87+
],
88+
],
89+
],
90+
'metadata-set' => 'saml20-idp-remote',
91+
'SingleSignOnService' => [
92+
[
93+
'Binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect',
94+
'Location' => 'https://idp.acmee.com/example/sso',
95+
],
96+
[
97+
'Binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST',
98+
'Location' => 'https://idp.acmee.com/example/sso',
99+
],
100+
],
101+
'SingleLogoutService' => [
102+
[
103+
'Binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect',
104+
'Location' => 'https://idp.acmee.com/example/logout',
105+
],
106+
],
107+
'ArtifactResolutionService' => [],
108+
'NameIDFormats' => [
109+
'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent',
110+
'urn:oasis:names:tc:SAML:2.0:nameid-format:transient',
111+
],
112+
'keys' => [
113+
[
114+
'encryption' => false,
115+
'signing' => true,
116+
'type' => 'X509Certificate',
117+
'X509Certificate' => $this->acmeeCertificate,
118+
],
119+
],
120+
'scope' => [
121+
'acmee.com',
122+
],
123+
'UIInfo' => [
124+
'DisplayName' => [
125+
'en' => 'Acmee Identity Provider',
126+
],
127+
'Description' => [],
128+
'InformationURL' => [],
129+
'PrivacyStatementURL' => [],
130+
],
131+
];
132+
133+
// Build minimal SP metadata:
134+
$this->spEntityId = 'https://sp.acmee.com/demo';
135+
$this->spMetadata = [
136+
'entityID' => $this->spEntityId,
137+
'AssertionConsumerService' => [
138+
[
139+
'Binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST',
140+
'Location' => 'https://sp.acmee.com/demo/acs',
141+
],
142+
],
143+
];
144+
145+
parent::setUp();
146+
}
147+
148+
public function testCheckSignThrowsWhenBadCertificate(): void
149+
{
150+
$this->expectException(\Exception::class);
151+
152+
$localMetadata = $this->acmeeMetadata;
153+
foreach ($localMetadata['keys'] as $index => $cert) {
154+
$localMetadata['keys'][$index]['X509Certificate'] = $this->acmeeCertificateWrong;
155+
}
156+
$idpConfig = Configuration::loadFromArray(
157+
$localMetadata,
158+
$localMetadata['entityid'],
159+
);
160+
161+
$spConfig = Configuration::loadFromArray(
162+
$this->spMetadata,
163+
$this->spMetadata['entityID'],
164+
);
165+
166+
// Build the AuthnRequest using the Message helper:
167+
$authnRequest = Message::buildAuthnRequest($spConfig, $idpConfig);
168+
169+
// You may now use $authnRequest with checkSign:
170+
Message::checkSign($idpConfig, $authnRequest);
171+
}
172+
173+
public function testCheckSignThrowsWhenMissingCertificate(): void
174+
{
175+
$this->expectException(SSP_Error\Exception::class);
176+
$this->expectExceptionMessage('Missing certificate in metadata for \'https://idp.acmee.com/example\'');
177+
178+
$localMetadata = $this->acmeeMetadata;
179+
unset($localMetadata['keys']);
180+
$idpConfig = Configuration::loadFromArray(
181+
$localMetadata,
182+
$localMetadata['entityid'],
183+
);
184+
185+
$spConfig = Configuration::loadFromArray(
186+
$this->spMetadata,
187+
$this->spMetadata['entityID'],
188+
);
189+
190+
// Build the AuthnRequest using the Message helper:
191+
$authnRequest = Message::buildAuthnRequest($spConfig, $idpConfig);
192+
193+
// You may now use $authnRequest with checkSign:
194+
Message::checkSign($idpConfig, $authnRequest);
195+
}
196+
197+
public function testCheckSignThrowsWhenCertificateMismatch(): void
198+
{
199+
$this->expectException(SSP_Error\Error::class);
200+
$expectedMessage = [
201+
'errorCode' => ErrorCodes::NOTVALIDCERTSIGNATURE,
202+
'message' => (new ErrorCodes())->getMessage(ErrorCodes::NOTVALIDCERTSIGNATURE),
203+
'element' => 'SAML2\AuthnRequest',
204+
'issuer' => 'https://sp.acmee.com/demo',
205+
'entityid' => 'https://idp.acmee.com/example',
206+
];
207+
208+
$this->expectExceptionMessage(json_encode($expectedMessage));
209+
210+
$localMetadata = $this->acmeeMetadata;
211+
$localMetadata['signature.privatekey'] = PEMCertificatesMock::buildKeysPath(
212+
PEMCertificatesMock::PRIVATE_KEY,
213+
);
214+
$localMetadata['signature.privatekey_pass'] = PEMCertificatesMock::PASSPHRASE;
215+
$localMetadata['sign.authnrequest'] = true;
216+
foreach ($localMetadata['keys'] as $index => $cert) {
217+
$localMetadata['keys'][$index]['X509Certificate'] = $this->acmeeCertificateMismatch;
218+
}
219+
$idpConfig = Configuration::loadFromArray(
220+
$localMetadata,
221+
$localMetadata['entityid'],
222+
);
223+
224+
$spConfig = Configuration::loadFromArray(
225+
$this->spMetadata,
226+
$this->spMetadata['entityID'],
227+
);
228+
229+
// Build the AuthnRequest using the Message helper:
230+
$authnRequest = Message::buildAuthnRequest($spConfig, $idpConfig);
231+
// Extract to signed xml and then parse again to validate
232+
$signedDomElement = $authnRequest->toSignedXML();
233+
$parsed = new AuthnRequest($signedDomElement);
234+
Message::checkSign($idpConfig, $parsed);
235+
}
236+
237+
public function testCheckSignSucceeds(): void
238+
{
239+
$localMetadata = $this->acmeeMetadata;
240+
$localMetadata['signature.privatekey'] = PEMCertificatesMock::buildKeysPath(
241+
PEMCertificatesMock::PRIVATE_KEY,
242+
);
243+
$localMetadata['signature.privatekey_pass'] = PEMCertificatesMock::PASSPHRASE;
244+
$localMetadata['sign.authnrequest'] = true;
245+
246+
$idpConfig = Configuration::loadFromArray(
247+
$localMetadata,
248+
$localMetadata['entityid'],
249+
);
250+
251+
$spConfig = Configuration::loadFromArray(
252+
$this->spMetadata,
253+
$this->spMetadata['entityID'],
254+
);
255+
256+
// Build the AuthnRequest using the Message helper:
257+
$authnRequest = Message::buildAuthnRequest($spConfig, $idpConfig);
258+
// Extract to signed xml and then parse again to validate
259+
$signedDomElement = $authnRequest->toSignedXML();
260+
$parsed = new AuthnRequest($signedDomElement);
261+
$isValid = Message::checkSign($idpConfig, $parsed);
262+
$this->assertTrue($isValid);
263+
}
264+
}

0 commit comments

Comments
 (0)