saml2 icon indicating copy to clipboard operation
saml2 copied to clipboard

Cannot use the library with SimpleSAML

Open mglaman opened this issue 6 years ago • 11 comments

When trying to generate a SAMLRequest object from an incoming string, the library crashes.

The following is from the Message constructor: https://github.com/simplesamlphp/saml2/blob/master/src/SAML2/Message.php#L143

    protected function __construct(string $tagName, DOMElement $xml = null)
    {
        $this->tagName = $tagName;
        $this->id = Utils::getContainer()->generateId();

The container is part of the compat layer with SimpleSAML. The generateId method uses the Random library from SimpleSAML

use SimpleSAML\Utils\HTTP; use SimpleSAML\Utils\Random; use SimpleSAML\Utils\System; use SimpleSAML\Utils\XML;

    /**
     * {@inheritdoc}
     * @return string
     */
    public function generateId() : string
    {
        /** @psalm-suppress UndefinedClass */
        return Random::generateID();
    }

It's even marked as supressed in Psalm.

mglaman avatar Jul 25 '19 04:07 mglaman

Just found https://github.com/simplesamlphp/saml2/issues/125, which links to

Provide the required external dependencies by extending and implementing the SAML2\Compat\AbstractContainer then injecting it in the ContainerSingleton (see example below).

So we have to define a container ahead of time to replace the Ssp bridge?

mglaman avatar Jul 25 '19 04:07 mglaman

Yes, you can either use SSP directly or provide your own substitutions.

@tvdijen would it be an idea to move some of the basic things that the SAML2 library uses to another repo, shared between this library and SSP itself? I don't know how much is depending on SSP, or how much of that is not depending on configuration, though.

jaimeperez avatar Jul 25 '19 11:07 jaimeperez

Good suggestion, will investigate! I think it's mainly the Logger

tvdijen avatar Jul 25 '19 11:07 tvdijen

@tvdijen I can give a hand, too. We're building an idP for our Drupal instance.

mglaman avatar Jul 25 '19 15:07 mglaman

That would be great, cause I'm not able to work on this on the real short-term.. Let me know if you need anything

tvdijen avatar Jul 25 '19 15:07 tvdijen

It's actually a bit more than I anticipated on:

$ grep -R "use SimpleSAML" *
src/SAML2/Compat/Ssp/Container.php:use SimpleSAML\Utils\HTTP;
src/SAML2/Compat/Ssp/Container.php:use SimpleSAML\Utils\Random;
src/SAML2/Compat/Ssp/Container.php:use SimpleSAML\Utils\System;
src/SAML2/Compat/Ssp/Container.php:use SimpleSAML\Utils\XML;
src/SAML2/Compat/Ssp/Logger.php:use SimpleSAML\Logger as SspLogger;
src/SAML2/Configuration/SimpleSAMLConverter.php:use SimpleSAML\Configuration;
src/SAML2/HTTPArtifact.php:use SimpleSAML\Configuration;
src/SAML2/HTTPArtifact.php:use SimpleSAML\Metadata\MetaDataStorageHandler;
src/SAML2/HTTPArtifact.php:use SimpleSAML\Module\saml\Message as MSG;
src/SAML2/HTTPArtifact.php:use SimpleSAML\Store;
src/SAML2/HTTPArtifact.php:use SimpleSAML\Utils\HTTP;
src/SAML2/SOAPClient.php:use SimpleSAML\Configuration;
src/SAML2/SOAPClient.php:use SimpleSAML\Utils\Config;
src/SAML2/SOAPClient.php:use SimpleSAML\Utils\Crypto;
tests/SAML2/HTTPPostTest.php:use SimpleSAML\Utils\HTTP;

tvdijen avatar Jul 25 '19 15:07 tvdijen

Is this something feasible for the library? LightSAML is not feasible, either. SAML2 is too tightly coupled to SimpleSAML. We need to have a working idP and I would be willing to help on this effort.

mglaman avatar Jul 25 '19 19:07 mglaman

If you need an IdP you should just use SimpleSAMLphp.. The library is only useful if you want to develop your own IdP, and trust me, you don't want to reinvent the wheel when it comes to SAML.

tvdijen avatar Jul 25 '19 22:07 tvdijen

I'm not a fan of the library, it's hard to integrate with other systems and has its own response handling and assumptions. I was hoping for a more "lean" library to work with. I do not want to reinvent the wheel, I was hoping to contribute to the libraries.

mglaman avatar Jul 25 '19 22:07 mglaman

Contributions are always welcome! The library was split off from SimpleSAMLphp years ago and may have been a bit neglected ever since.. I've recently put a lot of effort in bringing it back up to today's standards for a future 4.0 release.. If you feel we need to change things, this would be the time!

tvdijen avatar Jul 25 '19 22:07 tvdijen

@mglaman SimpleSAMLphp makes assumptions either to make your life easier, or to follow common standards and best practices. Of course, there might be cases where something that should be possible to customise is not, and then contributions are always welcome to improve. But in general, you should use SimpleSAMLphp, not this library. You need to know SAML2 in deep to be on the safe side, and i'ts much more complicated than it might appear. Furthermore, there are lots of security-related issues that SimpleSAMLphp is tackling for you, while you would need to have all those in mind and approach them manually if you use this library on its own.

jaimeperez avatar Jul 30 '19 07:07 jaimeperez