Should be fixed in the feature/library branch now. If no keys are available, the code will continue without signing the assertion.
@monkeyiq Since you have some sort of test-setup, would you mind confirming that the signing issue was resolved?

My setup is probably a little strange.

I have in my config.php:

    'metadata.sign.enable' => false,

    'metadata.sign.privatekey' => null,
    'metadata.sign.privatekey_pass' => null,
    'metadata.sign.certificate' => null,

I have in my adfs-idp-hosted.php metadata file

    'privatekey' => 'host200404.pem',
    'certificate' => 'host200404.crt',

With that configuration I have to add a test like this in ADFS.php

        if(Configuration::getInstance()->getOptionalBoolean('metadata.sign.enable',true)) {
            if ($privateKeyFile !== null && $certificateFile !== null && $algo !== null) {
                $assertion = ADFS::signAssertion($assertion, $privateKeyFile, $certificateFile, $algo, $passphrase);
                $assertion = Assertion::fromXML($assertion->toXML());
            }
        }

After digging into this a bit more what I think we want is for me to set the privatekey/certificate to null in the idp file and the code to load keys changes to something like

        $assertion = ADFS::generateAssertion($idpEntityId, $spEntityId, $nameid, $attributes, $assertionLifetime);

        $configUtils = new Utils\Config();
        $privateKeyFile = null;
        $certificateFile = null;
        $path = $idpMetadata->getOptionalString('privatekey',null);
        if( $path ) {
            $privateKeyFile = $configUtils->getCertPath($path);
        }
        $path = $idpMetadata->getOptionalString('certificate',null);
        if( $path ) {
            $certificateFile = $configUtils->getCertPath($path);
        }
        $path = null;
        
        $passphrase = $idpMetadata->getOptionalString('privatekey_pass', null);

Otherwise the getString on 'privatekey' fails because it is set to null. getString calls getValue which calls hasValue which checks for the key and also !is_null($this->configuration[$name])

Oct 17 19:47:36 simplesamlphp ERROR [0bb4150be6] 12 /opt/sspdev/vendor/simplesamlphp/assert/src/Assert.php:389 (SimpleSAML\Assert\Assert::__callStatic)
Oct 17 19:47:36 simplesamlphp ERROR [0bb4150be6] 11 /opt/sspdev/src/SimpleSAML/Configuration.php:402 (SimpleSAML\Configuration::getValue)
Oct 17 19:47:36 simplesamlphp ERROR [0bb4150be6] 10 /opt/sspdev/src/SimpleSAML/Configuration.php:693 (SimpleSAML\Configuration::getString)
Oct 17 19:47:36 simplesamlphp ERROR [0bb4150be6] 9 /opt/sspdev/modules/adfs/src/IdP/ADFS.php:445 (SimpleSAML\Module\adfs\IdP\ADFS::sendResponse)

Thinking a bit more, it is a bit cleaner if the privatekey/certificate calls remain not optional but are moved into a block that is wrapped in a test if signing is desired (true by default).

if($idpMetadata->getOptionalBoolean('metadata.sign.enable', true)) {
   // sign it here
   $privateKeyFile = $configUtils->getCertPath($idpMetadata->getString('privatekey'));
   $certificateFile = $configUtils->getCertPath($idpMetadata->getString('certificate'));
   $passphrase = $idpMetadata->getOptionalString('privatekey_pass', null);
   ...

I'm not sure I follow what you're doing? What does metadata.sign.enable have to do with signing assertions? Metadata-signing uses a separate key-pair from config.php and doesn't affect the authentication process.

If you want your assertions signed, you have to set a key-pair in the adfs-idp-hosted.php file. This is not different from SAML IDPs by the way.

Please ignore my comment about 'metadata.sign.enable' I was thinking about how the code might be a little simpler if it was using a config key but it was the end of a long day so the key I mentioned was not valid.

The end of my slightly older comment is still ok though... the code does need to check for privatekey using getOptionalString or it will assert out if privatekey is set to null in the idp configuration.

Gotcha, changed two getString with getOptionalString

Hmm, getCertPath can not take a null value. Perhaps we can wrap the signing block with

if( $idpMetadata->getOptionalString('privatekey', null) !== null
  && $idpMetadata->getOptionalString('certificate', null) !== null ) {
...
}

I think I properly fixed it now

Cool. Login works without key/cert here.

Should we tag a release of the module before merging?

I've tagged v3.0.0-rc1 for the module and I will rebase this PR against the 2.4 release branch

I took a quick look at the PR after the rebase. It seems LanguageTest.php is being changed in the PR and src/SimpleSAML/Locale/Language.php. The former seems ok but the later seems to be removing the lower casing of the language from 2.4 so maybe something was a little funky in the rebase?

The strtolower only seems to exist in 2.4 ... Not in 2.3, not in master.. Something wasn't properly cherry-picked along the different branches here

Ah I see commit b9e38b9 above seems to be it.

Right, so it was intentionally removed in the 2.3 branch, but we failed to cherry-pick it to 2.4 ..

I am wondering about the many other commits at the top of the PR. For example the "Add link to 2.3 upgrade notes" doesn't seem to be in 2.4 either.

Ah I see, they were cherry picked into feature/adfs-upgrade and are now being merged into simplesamlphp-2.4.

I am thinking the "rebase and merge" option is probably the way to go? This will keep info about the cherry picked stuff separate in the history.

I will merge this tomorrow morning.

hmm, the rebase and merge is saying it cannot be done due to conflicts :(

I guess the normal merge is the second best option. The squash seems like the least desirable for this PR.