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

3.0.0 Signature validation fails when using ADFS 3.0 as IdP with encryption #283

Closed
wants to merge 2 commits into from

Conversation

gmoniker
Copy link

@gmoniker gmoniker commented Jan 8, 2018

I am offering a new method of importing a decrypted assertion into the XML document to replace the EncryptedAssertion. This method should solve the problem of Assertions using default namespace declarations which get mangled by importNode with deep cloning and then do not pass signature verification.

This solves problems when the IdP signs its assertion and encrypts it, AND uses default namespaces in the Assertion elements. As for example with ADFS 3.0.

Don't use the replaceChild method of DomDocument.
Use a shallow clone with tree recurse to retain canonical form.
Signature in the decrypted Assertion should verify.
@gmoniker
Copy link
Author

gmoniker commented Jan 8, 2018

Travis testing is failing.

It seems that you have to use --root-dir in .travis.yml now in the call to vendor/bin/coveralls and remove src_dir in .coveralls.yml
See WordPoints/dev-lib#109

Also I saw a hint that it would be better to set coveralls task in after_succes in .travis.yml.

@pitbulk
Copy link
Contributor

pitbulk commented Jan 8, 2018

Thanks, I will review that tomorrow and let you know. Thanks for contributing!.

@pitbulk
Copy link
Contributor

pitbulk commented Jan 10, 2018

@gmoniker can you add a Test with an example of a SAMLResponse with encrypted assertion that failed previously and is working now? (if you can use the private key/public cert located here so we avoid to introduce in the future any change that break it.

btw Travis reported the following issues:

FILE: /home/travis/build/onelogin/php-saml/lib/Saml2/Utils.php
--------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------
 180 | ERROR | [x] Opening brace should be on a new line
 191 | ERROR | [ ] Expected "foreach (...) {\n"; found "foreach(...) {\n"

Once fixed, it should pass (check how past a recent commit).

@pitbulk pitbulk changed the title 3.0.0 3.0.0 Signature validation fails when using ADFS 3.0 as IdP with encryption Jan 15, 2018
@pitbulk pitbulk closed this in 428ffa8 Jun 7, 2018
@gmoniker
Copy link
Author

gmoniker commented Mar 4, 2020

@pitbulk

Just discovered that the treeCopyReplace routine I created became part of the onelogin codebase. Awesome. 😀

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.

2 participants