saml2
saml2 copied to clipboard
simplesamlphp
Fix encrypted id serialization
This fixes the issue from #295, and adds a test to (hopefully) assure that this doesn't happen again.
I'm not 100% sure that this test tests everything, but re-adding the methods makes the test fail so it at least makes sure that it is working better then it currently is.
This does re-introduce some of the issues that were fixed in https://github.com/simplesamlphp/saml2/pull/292, but because it is only a deprecation and not a complete fail, I think it is preferred to release this even if the 'real fix' is not available yet.
Hi @bartlangelaan ! Reverting the serialization-patch is not a solution. Deprecation-warnings are treated as errors in PHP8+, so they will break hard..
Are you sure that this is a change with PHP8? Because that would, like, completely make deprecations unusable. Then every deprecation would be a breaking change.
Deprecations are meant to be warnings until the next major version (PHP9), where all warnings will become errors, right?
Ah, I didn't know that.
From what I found, it will now report the warnings by default. But that doesn't mean that it is an error that 'will break hard'. Also, 'display_errors' should only be on during development. So it shouldn't affect any end-user if a deprecation is hit. It should just report the warning to the error log, and not show it visually.
So, imho, introducing a deprecation is less worse then breaking all eToegang (eHerkenning / eIDAS) connections and other IDPs that use EncryptedIDs.
They are treated as errors and will simply break.. Go ahead and try if you don't believe me. Merging this might fix PHP 8.0 support, but will eventually break in 8.1 because we need the magic methods there. We just have to properly fix this before we can support PHP 8.x at all... No way we're merging this PR as is.
They are treated as errors and will simply break.. Go ahead and try if you don't believe me.
https://onlinephp.io/c/c403c
But yes, the magic methods are a requirement for PHP 9.
Also, I do understand that this is not the proper fix. I just don't know how to fix this properly. We are using this PR as a patch in our project right now, which works for us.
Also, I do understand that this is not the proper fix. I just don't know how to fix this properly. We are using this PR as a patch in our project right now, which works for us.
The proper fix is to make the magic methods to properly function, so we can seamlessly support any version of PHP without issues. I've already pinpointed the issue to the constructor of the AttributeValue-class, but I haven't been able to dig deeper. My best guess right now is that the copyElement-method there is doing something unexpected.
Any updates on this issue @tvdijen? Is there anything I can do to assist here?
No update.. This is nowhere on my radar right now.. I see a lot of issues with supporting PHP 7.4 and 8.1 (8.0 is okayish) simultaneously.. Not just here, unfortunately..
What you could do is dive deeper than I already did in my last comment and actually find the issue here so we can fix it. Alternatively you could consider donating to the project so we can hire a new maintainer.
Our focus right now is to work on a complete rewrite of this library and, most importantly, drop our dependency on xmlseclibs. We can however hotfix the 4.x branch if we get a reasonable PR.
@bartlangelaan & @tommy2d could you guys run a test with 582a1fdeca9a676dd58c8ea451cf9235d6be8393 ? I think I fixed it
When i cherrypick this sole commit, clean all sqlite tables and start a new tvs session in an incognito chrome tab I still get this error:
Uncaught TypeError: SAML2\XML\saml\AttributeValue::getElement(): Return value must be of type DOMElement, null returned in tvs/vendor/simplesamlphp/saml2/src/SAML2/XML/saml/AttributeValue.php:74
Is this to be expected while still being on v4.6.3 of SAML2 and 1.19.5 of simplesaml?
Same seems to happen when I revert to simplesaml 1.9.7 and the bundled version of saml2, so I'm afraid we're not quite there yet.
Same situation, same error on same line. Just curious: How come the commit date of this commit is in the past (yesterday)?
Because I squashed the commit with the one from yesterday. I can't figure out how the element-property can be null :(
I put a var_export() of $parameters['RawAttributes'] in session_unserialize() and it gives me this:
2022/11/08 10:29:26 [error] 8378#8378: *2707139 FastCGI sent in stderr: "PHP message: PHP Fatal error: Uncaught Exception: array ( 'urn:nl-eid-gdi:1.0:ActingSubjectID' => array ( 0 => SAML2\XML\saml\AttributeValue::__set_state(array( 'element' => NULL, )), ), )
This is so hard to debug, not being able to reproduce this myself.. I'm starting to think that we have two individual issues here. 1) the serialization of AttributeValue that I supposedly fixed in the referred commit and 2) serialization of a session holding a complex AttributeValue...
Would you know if TVS has some kind of test-environment that is generally available?
I'm quite certain that there is no general testing environment that you can just hook on to. It's all quite regulated. Even if such a thing were available, we still have a special kind of account ("Meervoudige aansluiting") that is technically different from the regular thing.
I can share my serialized session content with you over the mail. Furthermore, we can also meet here at the office in Hoofddorp/Amsterdam, or at your offices to debug the situation using our test-environment. Please send me a PM / e-mail if you're interested.
@tommy2d or @bartlangelaan Just to make 100% sure that this is still an issue; what happens if you update to saml2 v4.6.5 and clear all the existing sessions from the sessions table? I spent another hour together with a fellow dev and we can't comprehend this.
I removed the entire SQLite DB, replaced the composer saml2 folder with the downloaded zip from github releases and I get this error when trying to log on:
PHP message: PHP Fatal error: Uncaught TypeError: SAML2\XML\saml\AttributeValue::getElement(): Return value must be of type DOMElement, null returned in /tvs/vendor/simplesamlphp/saml2/src/SAML2/XML/saml/AttributeValue.php:73 Stack trace: #0 /tvs/vendor/simplesamlphp/simplesamlphp/lib/SimpleSAML/Session.php(242): SAML2\XML\saml\AttributeValue->getElement() #1 [internal function]: SimpleSAML\Session->unserialize() #2 /tvs/vendor/simplesamlphp/simplesamlphp/lib/SimpleSAML/Store/SQL.php(337): unserialize() #3 /tvs/vendor/simplesamlphp/simplesamlphp/lib/SimpleSAML/SessionHandlerStore.php(55): SimpleSAML\Store\SQL->get() #4 /tvs/vendor/simplesamlphp/simplesamlphp/lib/SimpleSAML/Session.php(355): SimpleSAML\SessionHandlerStore->loadSession() #5 /tvs/vendor/simplesamlphp/simplesamlphp/lib/SimpleSAML/Session.php(270): SimpleSAML\Session::getSession()
@tommy2d One more check.. You say Github releases, but the latest version is only a tag, not a release.. Which one did you use?
Also.. Those line-numbers start to confuse me.. There is nothing going on at Session.php line 242. I feel like I'm chasing a red herring here
Is this to be expected while still being on v4.6.3 of SAML2 and 1.19.5 of simplesaml?
Are you still running 1.19.5 ? Because relevant changes have been done in 1.19.6 ...
I have just released 1.19.7 that has all the fixes in one place. No messing around with vendor-directories and zip-files... It would be great of you could run a test with that one (don't forget to clear the sessions again)
@tvdijen I can confirm that 1.19.7 fixes the problem! I now however run into a new program with the recently fixed verify_peer for the SAML HTTP Artifact binding. SOAPClient.php seems to assume the peer should be verifiable through the certificate that is used fore signing. But in case of the pre-production tvs, the signing certificate that is included in the IDP metadata (CN: preprod-saml-tvs-rd.dictu.nl) does not equal the certificate offered during the backchannel call (CN: artifact-pp2.toegang.overheid.nl). Both do actually share the same issuer: QuoVadis PKIoverheid Private Services CA - G1.
I worked around this for now by commenting out the verify_peer part, shall I make a separate issue post for the above issue, or do I need to explicitly set a config flag for this to work?
Hallelujah, glad we got that sorted! If you wouldn't mind opening a new issue for the certificate verification, I'll dive into that first thing tomorrow.
Edit: Never mind, there is already an open issue for it in #309 I made the destination metadata available as a starter to fix the guy's issue, but I now understand that's half a fix. To properly fix this we need much more control over the TLS-context. I think the current implementation is really strange, because TLS-certificate and signing certificate, as a best practise, should never be the same cert.. It really makes no sense to use the metadata signing certs here. TVS using a private chain for it's TLS-endpoint is pretty unusual too, but a las. That all being said, I think it makes sense to rationalize the SOAP metadata configuration a bit, but we may not do this in 1.19 anymore.
Hallelujah for you in this case, for fixing the issue ;).
I do understand that this fine grained control is not going to land anymore in 1.19. But how to proceed now? I still need to fork saml2 for it to work with TVS. The previous situation (=no peer verification) worked better than what we have now. Also, I would not mind doing actual peer verification through locally installed CA certificates, even in this case in which a private CA is used for the certificate that is being verified. This however, would also imply changing the current setup in which the signing certificate is assumed to be also used for the https context of the artefact binding.
I'm going to discuss this internally and will hopefully have a plan for it by the end of the day!