Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security updates #113

Merged
merged 3 commits into from
Feb 21, 2019
Merged

Security updates #113

merged 3 commits into from
Feb 21, 2019

Conversation

MKodde
Copy link
Member

@MKodde MKodde commented Feb 20, 2019

Fix the security warnings that are reported by the security checker.

Note that we use the SimpleSAMLphp SAML2 library at version 3.2 until 3.3 is considered stable.

Version 3.3 saw the introduction of more strict parsing of saml
messages. This caused EB to break when invalid data is set in the saml
response according to the SAML2 lib.
@MKodde
Copy link
Member Author

MKodde commented Feb 20, 2019

Interesting, the failing test is broken because of a SAML2 3.2 feature which I am yet to find.

public function epti_attribute_cannot_be_set_if_its_value_has_multiple_name_ids_when_creating_an_authenticated_user()
{
$this->expectException(RuntimeException::class, 'must be a NameID');
$assertionWithEpti = $this->getAssertionWithEptiWithTooManyValues();
$attributeDictionary = $this->getAttributeDictionary();
$assertionAdapter = $this->mockAssertionAdapterWith(
AttributeSet::createFrom($assertionWithEpti, $attributeDictionary),
'abcd-some-value-xyz'
);
AuthenticatedUser::createFrom($assertionAdapter, []);
}

The test works with SAML2 @ 3.1.6 but breaks on 3.2.6.

@MKodde
Copy link
Member Author

MKodde commented Feb 20, 2019

Ive been researching the problem. And the SAML2 lib used to have a very explicit check on the correctness of the EPTI attribute. This was added in:

simplesamlphp/saml2@d3b8bb0 (simplesamlphp/saml2#60 and later simplesamlphp/saml2#76)

Then @thijskh made the validation less harsh, by logging the problem instead of throwing an Exception. in this commit: simplesamlphp/saml2@6db9836 which landed in v3.2.3.

In order to be compatible with versions >= v3.2.3 we need to update the test or make SAML2 more strict again.

I'm opting for the first option.

@thijskh can you remember the reason for such a specific test case in the AuthenticatedUserTest.php? Is it something that happens often, having an assertion with more than one EPTI values?

@thijskh
Copy link
Member

thijskh commented Feb 20, 2019

That should really never happen. Does the commit history not tell anything about it? Otherwise safe to drop it.

@MKodde
Copy link
Member Author

MKodde commented Feb 21, 2019

Nope no insights to be found in gitlog. The test is actually more a test of the SAML bundle. Removing the test.

First, these tests are acually testing the StepUp SAML bundle and not
really any features of Profile. Second, the SAML2 library no longer
throws exceptions when more than one attribute value is set on the epti
attribute. This is now logged.

In accordance with the PO, we decided to remove these tests and rely
on EB to never release more than one epti value.

So, in addition to removing the tests, I also dropped the version
restraint on SAML2 3.2

#113 (comment)
@pablothedude pablothedude merged commit 1f5467e into develop Feb 21, 2019
@pablothedude pablothedude deleted the bugfix/security-updates branch February 21, 2019 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants