From e6c01fa9b0e815682e24916f03a84d245480c4a0 Mon Sep 17 00:00:00 2001 From: Joris Steyn Date: Tue, 1 May 2018 16:45:19 +0200 Subject: [PATCH] Separate assertion validation from decryption In order to check if an assertion is signed without validating the contents, we need to separate the logic. Code using the response processor is not affected, the behaviour is identical. But if users of the SAML2 library are calling the attribute processor directly, they must not call decryptAssertions() before calling processAssertions(). There is no easy way to make this backwards compatible but it should not cause problems because you would usually not use the assertion processor stand-alone. --- src/SAML2/Assertion/Processor.php | 30 +++++++++++++++---- src/SAML2/Response/Processor.php | 9 ++++-- .../Response/SignatureValidationTest.php | 19 +++++++++--- 3 files changed, 46 insertions(+), 12 deletions(-) diff --git a/src/SAML2/Assertion/Processor.php b/src/SAML2/Assertion/Processor.php index 882d73c309..2dcf499de1 100644 --- a/src/SAML2/Assertion/Processor.php +++ b/src/SAML2/Assertion/Processor.php @@ -74,14 +74,36 @@ public function __construct( } /** - * @param \SAML2\Utilities\ArrayCollection $assertions + * Decrypt assertions, or do nothing if assertions are already decrypted. + * + * @param ArrayCollection $assertions + * + * @return \SAML2\Assertion[] Collection (\SAML2\Utilities\ArrayCollection) of decrypted assertions + */ + public function decryptAssertions(ArrayCollection $assertions) + { + $decrypted = new ArrayCollection(); + foreach ($assertions->getIterator() as $assertion) { + $decrypted->add($this->decryptAssertion($assertion)); + } + + return $decrypted; + } + + /** + * @param \SAML2\Utilities\ArrayCollection|array $assertions Collection of decrypted assertions * * @return \SAML2\Assertion[] Collection (\SAML2\Utilities\ArrayCollection) of processed assertions */ public function processAssertions($assertions) { + // BC compatibility, allow array argument. + if (is_array($assertions)) { + $assertions = new ArrayCollection($assertions); + } + $processed = new ArrayCollection(); - foreach ($assertions as $assertion) { + foreach ($assertions->getIterator() as $assertion) { $processed->add($this->process($assertion)); } @@ -89,14 +111,12 @@ public function processAssertions($assertions) } /** - * @param \SAML2\Assertion|\SAML2\EncryptedAssertion $assertion + * @param \SAML2\Assertion $assertion * * @return \SAML2\Assertion */ public function process($assertion) { - $assertion = $this->decryptAssertion($assertion); - if (!$assertion->getWasSignedAtConstruction()) { $this->logger->info(sprintf( 'Assertion with id "%s" was not signed at construction, not verifying the signature', diff --git a/src/SAML2/Response/Processor.php b/src/SAML2/Response/Processor.php index b0769e0ca4..197350b36e 100644 --- a/src/SAML2/Response/Processor.php +++ b/src/SAML2/Response/Processor.php @@ -14,6 +14,7 @@ use SAML2\Response\Exception\UnsignedResponseException; use SAML2\Response\Validation\PreconditionValidator; use SAML2\Signature\Validator; +use SAML2\Utilities\ArrayCollection; /** * @SuppressWarnings(PHPMD.CouplingBetweenObjects) - due to specific exceptions @@ -144,10 +145,12 @@ private function processAssertions(Response $response) throw new NoAssertionsFoundException('No assertions found in response from IdP.'); } - $assertions = $this->assertionProcessor->processAssertions($assertions); + $decryptedAssertions = $this->assertionProcessor->decryptAssertions( + new ArrayCollection($assertions) + ); if (!$this->responseIsSigned) { - foreach ($assertions->getIterator() as $assertion) { + foreach ($decryptedAssertions->getIterator() as $assertion) { if (!$assertion->getWasSignedAtConstruction()) { throw new UnsignedResponseException( 'Both the response and the assertion it contains are not signed.' @@ -156,6 +159,6 @@ private function processAssertions(Response $response) } } - return $assertions; + return $this->assertionProcessor->processAssertions($decryptedAssertions); } } diff --git a/tests/SAML2/Response/SignatureValidationTest.php b/tests/SAML2/Response/SignatureValidationTest.php index c6bf5f1f44..d009857a9a 100644 --- a/tests/SAML2/Response/SignatureValidationTest.php +++ b/tests/SAML2/Response/SignatureValidationTest.php @@ -77,10 +77,12 @@ public function tearDown() */ public function testThatAnUnsignedResponseWithASignedAssertionCanBeProcessed() { - $this->assertionProcessor->shouldReceive('processAssertions') + $this->assertionProcessor->shouldReceive('decryptAssertions') ->once() ->andReturn(new ArrayCollection()); + $this->assertionProcessor->shouldReceive('processAssertions')->once(); + $processor = new Processor(new \Psr\Log\NullLogger()); $processor->process( @@ -96,6 +98,10 @@ public function testThatAnUnsignedResponseWithASignedAssertionCanBeProcessed() */ public function testThatAnSignedResponseWithAnUnsignedAssertionCanBeProcessed() { + $this->assertionProcessor->shouldReceive('decryptAssertions') + ->once() + ->andReturn(new ArrayCollection()); + $this->assertionProcessor->shouldReceive('processAssertions')->once(); $processor = new Processor(new \Psr\Log\NullLogger()); @@ -113,6 +119,10 @@ public function testThatAnSignedResponseWithAnUnsignedAssertionCanBeProcessed() */ public function testThatASignedResponseWithASignedAssertionIsValid() { + $this->assertionProcessor->shouldReceive('decryptAssertions') + ->once() + ->andReturn(new ArrayCollection()); + $this->assertionProcessor->shouldReceive('processAssertions')->once(); $processor = new Processor(new \Psr\Log\NullLogger()); @@ -136,14 +146,15 @@ public function testThatAnUnsignedResponseWithNoSignedAssertionsThrowsAnExceptio ->once() ->andReturn(false); - // The processAssertions is called to decrypt possible encrypted assertions, - // after which it should fail with an exception due to having no signature - $this->assertionProcessor->shouldReceive('processAssertions') + $this->assertionProcessor->shouldReceive('decryptAssertions') ->once() ->andReturn(new ArrayCollection([ $assertion ])); + // here the processAssertions may not be called as it should fail with an exception due to having no signature + $this->assertionProcessor->shouldReceive('processAssertions')->never(); + $processor = new Processor(new \Psr\Log\NullLogger()); $processor->process(